Fix exceptions thrown from directory::copy

.clang-tidy

@@ -27,5 +27,5 @@ Checks: >
   -readability-named-parameter
 
 CheckOptions:
-  readability-identifier-length.IgnoredParameterNames: 'ec|to'
-  readability-identifier-length.IgnoredVariableNames: 'ec|to'
+  readability-identifier-length.IgnoredParameterNames: 'ec|to|it'
+  readability-identifier-length.IgnoredVariableNames: 'ec|to|it'

include/tmp/directory

@@ -70,7 +70,7 @@ public:
   ~directory() noexcept;
 
   directory(directory&&) noexcept;                    ///< MoveConstructible
-  directory& operator=(directory&&);                  ///< MoveAssignable
+  directory& operator=(directory&&) noexcept;         ///< MoveAssignable
   directory(const directory&) = delete;               ///< not CopyConstructible
   directory& operator=(const directory&) = delete;    ///< not CopyAssignable
 

src/directory.cpp

@@ -9,6 +9,25 @@
 #include <utility>
 
 namespace tmp {
+namespace {
+
+/// Deletes a directory recursively, ignoring any errors
+/// @param[in] directory The directory to delete
+void remove_directory(const directory& directory) noexcept {
+  try {
+    if (!directory.path().empty()) {
+      // Calling the `std::error_code` overload of `fs::remove_all` should be
+      // more optimal here since it would not require creating
+      // a `fs::filesystem_error` message before we suppress the exception
+      std::error_code ec;
+      fs::remove_all(directory, ec);
+    }
+  } catch (...) {
+    // Do nothing: if we failed to delete the temporary directory,
+    // the system should do it later
+  }
+}
+}    // namespace
 
 directory::directory(std::string_view prefix)
     : pathobject(create_directory(prefix)) {}
@@ -20,8 +39,18 @@ directory directory::copy(const fs::path& path, std::string_view prefix) {
   // since there is no way to tell it to fail if `from` is not a directory;
   // a properly implemented `fs::directory_iterator` opens a path and checks
   // whether it is a directory atomically
-  for (const fs::directory_entry& entry : fs::directory_iterator(path)) {
-    fs::copy(entry, dir / entry.path().filename(), fs::copy_options::recursive);
+
+  std::error_code ec;
+  for (fs::directory_iterator it = fs::directory_iterator(path, ec);
+       !ec && it != fs::directory_iterator(); it.increment(ec)) {
+    fs::copy(*it, dir / it->path().filename(), fs::copy_options::recursive, ec);
+    if (ec) {
+      break;
+    }
+  }
+
+  if (ec) {
+    throw fs::filesystem_error("Cannot create a temporary copy", path, ec);
   }
 
   return dir;
@@ -41,28 +70,14 @@ fs::path directory::operator/(const fs::path& source) const {
 
 directory::~directory() noexcept {
   (void)reserved;    // Old compilers do not want to accept `[[maybe_unused]]`
-
-  try {
-    if (!path().empty()) {
-      // Calling the `std::error_code` overload of `fs::remove_all` should be
-      // more optimal here since it would not require creating
-      // a `fs::filesystem_error` message before we suppress the exception
-      std::error_code ec;
-      fs::remove_all(path(), ec);
-    }
-  } catch (...) {
-    // Do nothing: if we failed to delete the temporary directory,
-    // the system should do it later
-  }
+  remove_directory(*this);
 }
 
 directory::directory(directory&& other) noexcept
     : pathobject(std::exchange(other.pathobject, fs::path())) {}
 
-directory& directory::operator=(directory&& other) {
-  // Here we intentionally call the throwing overload of `fs::remove_all`
-  // to report errors with exceptions when deleting the old directory
-  fs::remove_all(path());
+directory& directory::operator=(directory&& other) noexcept {
+  remove_directory(*this);
 
   pathobject = std::exchange(other.pathobject, fs::path());
   return *this;

tests/directory.cpp

@@ -11,6 +11,12 @@
 #include <string_view>
 #include <utility>
 
+#ifdef _WIN32
+#define NOMINMAX
+#define UNICODE
+#include <Windows.h>
+#endif
+
 namespace tmp {
 namespace {
 
@@ -85,6 +91,8 @@ TEST(directory, create_invalid_prefix) {
 TEST(directory, copy_directory) {
   directory tmpdir = directory();
   std::ofstream(tmpdir / "file") << "Hello, world!";
+  fs::create_directory(tmpdir / "dir");
+  std::ofstream(tmpdir / "dir" / "file2") << "Goodbye, world!";
 
   directory copy = directory::copy(tmpdir);
   EXPECT_TRUE(fs::exists(tmpdir));
@@ -93,15 +101,57 @@ TEST(directory, copy_directory) {
 
   EXPECT_TRUE(fs::is_directory(copy));
 
-  std::ifstream stream = std::ifstream(copy / "file");
-  std::string content = std::string(std::istreambuf_iterator(stream), {});
-  EXPECT_EQ(content, "Hello, world!");
+  {
+    std::ifstream stream = std::ifstream(copy / "file");
+    std::string content = std::string(std::istreambuf_iterator(stream), {});
+    EXPECT_EQ(content, "Hello, world!");
+  }
+
+  {
+    std::ifstream stream = std::ifstream(copy / "dir" / "file2");
+    std::string content = std::string(std::istreambuf_iterator(stream), {});
+    EXPECT_EQ(content, "Goodbye, world!");
+  }
 }
 
 /// Tests creation of a temporary copy of a file
 TEST(directory, copy_file) {
   std::ofstream("existing.txt", std::ios::binary) << "Hello, world!";
-  EXPECT_THROW(directory::copy("existing.txt"), fs::filesystem_error);
+
+  try {
+    directory::copy("existing.txt");
+    FAIL();
+  } catch (const fs::filesystem_error& ex) {
+    EXPECT_EQ(ex.path1(), "existing.txt");
+    EXPECT_EQ(ex.path2(), fs::path());
+  }
+}
+
+/// Tests creation of a temporary copy without permissions
+TEST(directory, copy_directory_without_permissions) {
+  directory tmpdir = directory();
+  std::ofstream(tmpdir / "file") << "Hello, world!";
+
+#ifdef _WIN32
+  HANDLE handle = CreateFile((tmpdir / "file").c_str(), GENERIC_READ, 0, NULL,
+                             OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+#else
+  fs::permissions(tmpdir / "file", fs::perms::none);
+#endif
+
+  try {
+    directory::copy(tmpdir);
+    FAIL();
+  } catch (const fs::filesystem_error& ex) {
+    EXPECT_EQ(ex.path1(), tmpdir);
+    EXPECT_EQ(ex.path2(), fs::path());
+  }
+
+#ifdef _WIN32
+  CloseHandle(handle);
+#else
+  fs::permissions(tmpdir / "file", fs::perms::all);
+#endif
 }
 
 /// Tests `operator/` of directory