[libcxx] Fix create_directories returning false but actually creating the directories, if the path name is with trailing seperators

[caojingfan]

Summary

if the path name is with trailing seperators like 'dir1/dir2/dir3/',
create_directories should process it just like 'dir1/dir2/dir3',
but according to https://godbolt.org/z/Wh3axvYM5, the trailing seperators
case was handled mistakenly, this patch fix it.

[ldionne]

It looks like CI is failing, so that should be addressed. Also, can you provide a link to the place where it says how create_directories should behave the way you say (I believe you, I just don't know the filesystem spec by heart and I want to check). Thanks for fixing this!

[caojingfan]

In D106217#2891410, @ldionne wrote:

It looks like CI is failing, so that should be addressed. Also, can you provide a link to the place where it says how create_directories should behave the way you say (I believe you, I just don't know the filesystem spec by heart and I want to check). Thanks for fixing this!

Thanks for reviewing.

The CI failing actually is involving a more complicated (possibly)misbehavior about path::parent_path() function which I am currently investigating.
As for why create_directories should behave this way, it is obvious, according to the definition of the return value of create_directories https://en.cppreference.com/w/cpp/filesystem/create_directory, that create_directories can not create the directories and return false indicating that it didn't create the directories at the same time, also I can list some supplementaries:

1. GCC 8.3 has fixed this, see https://godbolt.org/z/qe8YGj9zr
2. A stackoverflow discussion regarding to this, https://stackoverflow.com/questions/60130796/return-value-of-stdfilesystemcreate-directories-on-paths-with-trailing-sla

[caojingfan]

rewrite the fix in a more elegant way, also add more test cases

[Mordante]

We're moving to GitHub PRs it would be great when this patch can be finished before moving. Are you interested in finishing it? If not I'll take over. Please start by rebasing the patch to see whether it passes the CI.

libcxx/src/filesystem/operations.cpp

bool __create_directories(const path& p, error_code* ec) {
   auto const st = detail::posix_stat(p, &m_ec);
   if (!status_known(st))
     return err.report(m_ec);
   else if (is_directory(st))
     return false;
   else if (exists(st))
     return err.report(errc::file_exists);
 
-  const path parent = p.parent_path();
+  path m_p = p;
+  // removing trailing slashes
+  if (p.has_relative_path() && !p.has_filename())
+    m_p = p.parent_path();
+
+  const path parent = m_p.parent_path();
   if (!parent.empty()) {
     const file_status parent_st = status(parent, m_ec);
     if (not status_known(parent_st))
       return err.report(m_ec);
     if (not exists(parent_st)) {
-      if (parent == p)
+      if (parent == m_p)
         return err.report(errc::invalid_argument);
       __create_directories(parent, ec);
       if (ec && *ec) {
         return false;
       }
     } else if (not is_directory(parent_st))
       return err.report(errc::not_a_directory);
   }
-  bool ret = __create_directory(p, &m_ec);
+  bool ret = __create_directory(m_p, &m_ec);
   if (m_ec)
     return err.report(m_ec);
   return ret;
 }
 
 bool __create_directory(const path& p, error_code* ec) {
   ErrorHandler<bool> err("create_directory", ec, &p);
 

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.create_directories/create_directories.pass.cpp

TEST_CASE(dest_is_empty_path)
     TEST_CHECK(ec);
     TEST_CHECK(ErrorIs(ec, std::errc::no_such_file_or_directory));
     ExceptionChecker Checker(path{}, std::errc::no_such_file_or_directory,
                              "create_directories");
     TEST_CHECK_THROW_RESULT(filesystem_error, Checker,
                             fs::create_directories(path{}));
 }
 
+TEST_CASE(dest_is_with_trailing_sep) {
+  scoped_test_env env;
+
+  const struct {
+    const std::string dir_name;
+    const bool exists_before;
+    const bool create;
+    const bool exists_after;
+  } TEST_CASES[] = {{"dir1/", false, true, true},
+                    {"dir3//", false, true, true},
+                    {"dir1/dir2/dir3/", false, true, true},
+                    {"dir2//dir3///dir4////", false, true, true},
+                    {"/", true, false, true},
+                    {"./", true, false, true}};
+
+  for (const auto& TC : TEST_CASES) {
+    const path dir = env.make_env_path(TC.dir_name);
+    TEST_CHECK(!dir.has_filename());
+    TEST_CHECK(exists(dir) == TC.exists_before);
+    std::error_code ec = GetTestEC();
+    TEST_CHECK(fs::create_directories(dir, ec) == TC.create);
+    TEST_CHECK(!ec);
+    TEST_CHECK(exists(dir) == TC.exists_after);
+  }
+}
+
 #ifdef _WIN32
 TEST_CASE(nonexistent_root)
 {
     std::error_code ec = GetTestEC();
     // If Q:\ doesn't exist, create_directories would try to recurse upwards
     // to parent_path() until it finds a directory that does exist. As the
     // whole path is the root name, parent_path() returns itself, and it
     // would recurse indefinitely, unless the recursion is broken.
     if (!exists("Q:\\"))
        TEST_CHECK(fs::create_directories("Q:\\", ec) == false);
     TEST_CHECK(fs::create_directories("\\\\nonexistentserver", ec) == false);
 }
 #endif
 
 TEST_SUITE_END()