Fix tmp::entry::move method (#129)

- Fix a bug when moving an entry to itself caused a deletion of said
entry
- Fix a bug when moving a directory to an existing file between
filesystems would not throw an exception
- Fix a bug when move was constrained to regular files and directories
only
- Fix a bug when an entry could be deleted even if it was not moved due
to an error
- Add a lot of tests
- Explain the `entry::move` behaviour somewhat better

include/tmp/entry

@@ -42,7 +42,8 @@ public:
   native_handle_type native_handle() const noexcept;
 
   /// Moves the managed path recursively to a given target, releasing
-  /// ownership of the managed path
+  /// ownership of the managed path; behaves like `std::filesystem::rename`
+  /// even when moving between filesystems
   /// @note The target path parent is created when this function is called
   /// @param to        A path to the target file or directory
   /// @throws std::filesystem::filesystem_error if cannot move the owned path

src/entry.cpp

@@ -117,27 +117,54 @@ entry::native_handle_type entry::native_handle() const noexcept {
 
 void entry::move(const fs::path& to) {
   std::error_code ec;
+  if (fs::exists(to)) {
+    if (!fs::is_directory(*this) && fs::is_directory(to)) {
+      ec = std::make_error_code(std::errc::is_a_directory);
+      throw_move_error(to, ec);
+    }
+
+    if (fs::is_directory(*this) && !fs::is_directory(to)) {
+      ec = std::make_error_code(std::errc::not_a_directory);
+      throw_move_error(to, ec);
+    }
+  }
+
   create_parent(to, ec);
   if (ec) {
     throw_move_error(to, ec);
   }
 
-  fs::rename(*this, to, ec);
-  if (ec == std::errc::cross_device_link) {
-    if (fs::is_regular_file(*this) && fs::is_directory(to)) {
-      ec = std::make_error_code(std::errc::is_a_directory);
-      throw_move_error(to, ec);
-    }
+  bool copying = false;
 
+#ifdef _WIN32
+  // On Windows, the underlying `MoveFileExW` fails when moving a directory
+  // between drives; in that case we copy the directory manually
+  copying = fs::is_directory(*this) && path().root_name() != to.root_name();
+  if (copying) {
+    fs::copy(*this, to, copy_options, ec);
+  } else {
+    fs::rename(*this, to, ec);
+  }
+#else
+  // On POSIX-compliant systems, the underlying `rename` function may return
+  // `EXDEV` if the implementation does not support links between file systems;
+  // so we try to rename the file, and if we fail with `EXDEV`, move it manually
+  fs::rename(*this, to, ec);
+  copying = ec == std::errc::cross_device_link;
+  if (copying) {
     fs::remove_all(to);
     fs::copy(*this, to, copy_options, ec);
   }
+#endif
 
   if (ec) {
     throw_move_error(to, ec);
   }
 
-  remove(*this);
+  if (copying) {
+    remove(*this);
+  }
+
   pathobject.clear();
 }
 

tests/CMakeLists.txt

@@ -2,10 +2,11 @@ project(${PROJECT_NAME}.test)
 
 find_package(GTest)
 
-add_executable(${PROJECT_NAME} directory.cpp file.cpp)
+add_executable(${PROJECT_NAME} entry.cpp directory.cpp file.cpp)
 target_link_libraries(${PROJECT_NAME} tmp::tmp GTest::gtest_main)
 target_compile_definitions(${PROJECT_NAME}
-  PRIVATE LABEL="com.github.bugdea1er.tmp")
+  PRIVATE LABEL="com.github.bugdea1er.tmp"
+  BUILD_DIR="${CMAKE_CURRENT_BINARY_DIR}")
 
 # On some platforms (e.g. Windows) CMake doesn't write load paths properly
 # This solution to put outputs in the same directory is good enough

tests/directory.cpp

@@ -120,7 +120,7 @@ TEST(directory, list) {
 
 /// Tests that destructor removes a directory
 TEST(directory, destructor) {
-  fs::path path = fs::path();
+  fs::path path;
   entry::native_handle_type handle;
   {
     directory tmpdir = directory();
@@ -172,27 +172,6 @@ TEST(directory, move_assignment) {
   EXPECT_TRUE(native_handle_is_valid(fst.native_handle()));
 }
 
-/// Tests directory moving
-TEST(directory, move) {
-  fs::path path = fs::path();
-  entry::native_handle_type handle;
-
-  fs::path to = fs::temp_directory_path() / "non-existing" / "parent";
-  {
-    directory tmpdir = directory();
-    path = tmpdir;
-    handle = tmpdir.native_handle();
-
-    tmpdir.move(to);
-  }
-
-  EXPECT_FALSE(fs::exists(path));
-  EXPECT_TRUE(fs::exists(to));
-  EXPECT_FALSE(native_handle_is_valid(handle));
-
-  fs::remove_all(fs::temp_directory_path() / "non-existing");
-}
-
 /// Tests directory swapping
 TEST(directory, swap) {
   directory fst = directory();

tests/entry.cpp

@@ -0,0 +1,233 @@
+#include <tmp/directory>
+#include <tmp/entry>
+#include <tmp/file>
+
+#include "utils.hpp"
+
+#include <gtest/gtest.h>
+
+#include <filesystem>
+#include <fstream>
+#include <iterator>
+#include <string>
+
+namespace tmp {
+namespace {
+
+namespace fs = std::filesystem;
+
+/// Returns a temporary file containing `Hello world!`
+file test_file() {
+  file tmpfile = file();
+  tmpfile.write("Hello, world!");
+
+  return tmpfile;
+}
+
+/// Returns a temporary directory with a file containing `Hello world!`
+directory test_directory() {
+  directory tmpdir = directory();
+  std::ofstream(tmpdir / "file") << "Hello, world!";
+
+  return tmpdir;
+}
+}    // namespace
+
+/// Tests that moving a temporary file to itself does nothing
+TEST(entry, move_file_to_self) {
+  fs::path path;
+  entry::native_handle_type handle;
+
+  {
+    file tmpfile = test_file();
+    path = tmpfile;
+    handle = tmpfile.native_handle();
+
+    tmpfile.move(tmpfile);
+  }
+
+  EXPECT_TRUE(fs::exists(path));
+  EXPECT_FALSE(native_handle_is_valid(handle));
+
+  {
+    auto stream = std::ifstream(path);
+    auto content = std::string(std::istreambuf_iterator<char>(stream), {});
+    EXPECT_EQ(content, "Hello, world!");
+  }
+
+  fs::remove_all(path);
+}
+
+/// Tests moving a temporary file to existing non-directory file
+TEST(entry, move_file_to_existing_file) {
+  fs::path path;
+  entry::native_handle_type handle;
+
+  fs::path to = fs::path(BUILD_DIR) / "move_file_to_existing_test";
+  std::ofstream(to / "file") << "Goodbye, world!";
+
+  {
+    file tmpfile = test_file();
+    path = tmpfile;
+    handle = tmpfile.native_handle();
+
+    tmpfile.move(to);
+  }
+
+  EXPECT_TRUE(fs::exists(to));
+  EXPECT_FALSE(fs::exists(path));
+  EXPECT_FALSE(native_handle_is_valid(handle));
+
+  {
+    auto stream = std::ifstream(to);
+    auto content = std::string(std::istreambuf_iterator<char>(stream), {});
+    EXPECT_EQ(content, "Hello, world!");
+  }
+
+  fs::remove_all(to);
+}
+
+/// Tests moving a temporary file to an existing directory
+TEST(entry, move_file_to_existing_directory) {
+  fs::path directory = fs::path(BUILD_DIR) / "existing_directory";
+  fs::create_directories(directory);
+
+  EXPECT_THROW(test_file().move(directory), fs::filesystem_error);
+
+  fs::remove_all(directory);
+}
+
+/// Tests moving a temporary file to a non-existing file
+TEST(entry, move_file_to_non_existing_file) {
+  fs::path path;
+  entry::native_handle_type handle;
+
+  fs::path parent = fs::path(BUILD_DIR) / "non-existing1";
+  fs::path to = parent / "path";
+
+  {
+    file tmpfile = test_file();
+    path = tmpfile;
+    handle = tmpfile.native_handle();
+
+    tmpfile.move(to);
+  }
+
+  EXPECT_TRUE(fs::exists(to));
+  EXPECT_FALSE(fs::exists(path));
+  EXPECT_FALSE(native_handle_is_valid(handle));
+
+  {
+    auto stream = std::ifstream(to);
+    auto content = std::string(std::istreambuf_iterator<char>(stream), {});
+    EXPECT_EQ(content, "Hello, world!");
+  }
+
+  fs::remove_all(parent);
+}
+
+/// Tests moving a temporary file to a non-existing directory
+TEST(entry, move_file_to_non_existing_directory) {
+  fs::path parent = fs::path(BUILD_DIR) / "non-existing2";
+  fs::path to = parent / "path/";
+
+  EXPECT_THROW(test_file().move(to), fs::filesystem_error);
+
+  fs::remove_all(parent);
+}
+
+/// Tests that moving a temporary directory to itself does nothing
+TEST(entry, move_directory_to_self) {
+  fs::path path;
+  entry::native_handle_type handle;
+
+  {
+    directory tmpdir = test_directory();
+    path = tmpdir;
+    handle = tmpdir.native_handle();
+
+    tmpdir.move(tmpdir);
+  }
+
+  EXPECT_TRUE(fs::exists(path));
+  EXPECT_FALSE(native_handle_is_valid(handle));
+
+  {
+    auto stream = std::ifstream(path / "file");
+    auto content = std::string(std::istreambuf_iterator<char>(stream), {});
+    EXPECT_EQ(content, "Hello, world!");
+  }
+
+  fs::remove_all(path);
+}
+
+/// Tests moving a temporary directory to existing directory
+TEST(entry, move_directory_to_existing_directory) {
+  fs::path path;
+  entry::native_handle_type handle;
+
+  fs::path to = fs::path(BUILD_DIR) / "move_directory_to_existing_test";
+  std::ofstream(to / "file2") << "Goodbye, world!";
+
+  {
+    directory tmpdir = test_directory();
+    path = tmpdir;
+    handle = tmpdir.native_handle();
+
+    tmpdir.move(to);
+  }
+
+  EXPECT_TRUE(fs::exists(to));
+  EXPECT_FALSE(fs::exists(path));
+  EXPECT_FALSE(native_handle_is_valid(handle));
+
+  {
+    auto stream = std::ifstream(to / "file");
+    auto content = std::string(std::istreambuf_iterator<char>(stream), {});
+    EXPECT_EQ(content, "Hello, world!");
+  }
+
+  EXPECT_FALSE(fs::exists(to / "file2"));
+
+  fs::remove_all(to);
+}
+
+/// Tests moving a temporary directory to an existing file
+TEST(entry, move_directory_to_existing_file) {
+  fs::path to = fs::path(BUILD_DIR) / "existing_file";
+  std::ofstream(to) << "Goodbye, world!";
+
+  EXPECT_THROW(test_directory().move(to), fs::filesystem_error);
+
+  fs::remove_all(to);
+}
+
+/// Tests moving a temporary directory to a non-existing path
+TEST(entry, move_directory_to_non_existing_path) {
+  fs::path path;
+  entry::native_handle_type handle;
+
+  fs::path parent = fs::path(BUILD_DIR) / "non-existing3";
+  fs::path to = parent / "path";
+
+  {
+    directory tmpdir = test_directory();
+    path = tmpdir;
+    handle = tmpdir.native_handle();
+
+    tmpdir.move(to);
+  }
+
+  EXPECT_TRUE(fs::exists(to));
+  EXPECT_FALSE(fs::exists(path));
+  EXPECT_FALSE(native_handle_is_valid(handle));
+
+  {
+    auto stream = std::ifstream(to / "file");
+    auto content = std::string(std::istreambuf_iterator<char>(stream), {});
+    EXPECT_EQ(content, "Hello, world!");
+  }
+
+  fs::remove_all(parent);
+}
+}    // namespace tmp

tests/file.cpp

@@ -340,7 +340,7 @@ TEST(file, output_stream_append_text) {
 
 /// Tests that destructor removes a file
 TEST(file, destructor) {
-  fs::path path = fs::path();
+  fs::path path;
   entry::native_handle_type handle;
   {
     file tmpfile = file();
@@ -392,27 +392,6 @@ TEST(file, move_assignment) {
   EXPECT_TRUE(native_handle_is_valid(fst.native_handle()));
 }
 
-/// Tests file moving
-TEST(file, move) {
-  fs::path path = fs::path();
-  entry::native_handle_type handle;
-
-  fs::path to = fs::temp_directory_path() / "non-existing" / "parent";
-  {
-    file tmpfile = file();
-    path = tmpfile;
-    handle = tmpfile.native_handle();
-
-    tmpfile.move(to);
-  }
-
-  EXPECT_FALSE(fs::exists(path));
-  EXPECT_TRUE(fs::exists(to));
-  EXPECT_FALSE(native_handle_is_valid(handle));
-
-  fs::remove_all(fs::temp_directory_path() / "non-existing");
-}
-
 /// Tests file swapping
 TEST(file, swap) {
   file fst = file();