libutil: handle all rename errors in moveFile

what the actual fuck. swallowing all errors *except* EXDEV is not okay.
renames do not do this, so moves should not do it either. luckily we do
not use moveFile anywhere except the store path registration code. this
may or may not have caused problems in the past. probably nobody knows.

Change-Id: I2b0255a5703983cbd129abc3219c11ac7171fd12

lix/libutil/file-system.cc

@@ -824,22 +824,23 @@ void moveFile(const Path & oldName, const Path & newName)
     try {
         fs::rename(oldName, newName);
     } catch (fs::filesystem_error & e) { // NOLINT(lix-foreign-exceptions)
-        auto oldPath = fs::path(oldName);
-        auto newPath = fs::path(newName);
-        // For the move to be as atomic as possible, copy to a temporary
-        // directory
         try {
+            if (e.code().value() != EXDEV) {
+                throw SysError(e.code().value(), "failed to move %s to %s", oldName, newName);
+            }
+            auto oldPath = fs::path(oldName);
+            auto newPath = fs::path(newName);
+            // For the move to be as atomic as possible, copy to a temporary
+            // directory
             fs::path temp = createTempSubdir(newPath.parent_path(), "rename-tmp");
             Finally removeTemp = [&]() { fs::remove(temp); };
             auto tempCopyTarget = temp / "copy-target";
-            if (e.code().value() == EXDEV) {
+            fs::remove(newPath);
-                fs::remove(newPath);
+            printTaggedWarning("Can’t rename %s as %s, copying instead", oldName, newName);
-                printTaggedWarning("Can’t rename %s as %s, copying instead", oldName, newName);
+            copy(fs::directory_entry(oldPath), tempCopyTarget, {.deleteAfter = true});
-                copy(fs::directory_entry(oldPath), tempCopyTarget, {.deleteAfter = true});
+            fs::rename(tempCopyTarget, newPath);
-                fs::rename(tempCopyTarget, newPath);
-            }
         } catch (fs::filesystem_error & e) { // NOLINT(lix-foreign-exceptions)
-            throw Error("failed to move %s to %s: %s", oldName, newName, e.what());
+            throw SysError(e.code().value(), "failed to move %s to %s", oldName, newName);
         }
     }
 }

tests/unit/libutil/tests.cc

@@ -1,3 +1,4 @@
+#include "lix/libstore/temporary-dir.hh"
 #include "lix/libutil/c-calls.hh"
 #include "lix/libutil/file-system.hh"
 #include "lix/libutil/processes.hh"
@@ -210,6 +211,31 @@ namespace nix {
         ASSERT_FALSE(pathExists("/schnitzel/darmstadt/pommes"));
     }
 
+    /* ----------------------------------------------------------------------------
+     * moveFile
+     * --------------------------------------------------------------------------*/
+
+    TEST(moveFile, handlesErrors)
+    {
+        auto tmpDir = createTempDir();
+        AutoDelete _delete(tmpDir);
+
+        auto source = tmpDir + "/source";
+        auto target = tmpDir + "/target";
+
+        // xdev should cause copies, everything else must throw. this is a small selection.
+        ASSERT_THROW(moveFile(source, target), Error); // ENOENT
+        createDirs(source);
+        createSymlink(target, target);
+        ASSERT_THROW(moveFile(source, target), Error); // ENOTDIR
+        ASSERT_THROW(moveFile(target, source), Error); // EISDIR
+        ASSERT_THROW(moveFile(target + "/foo", source), Error); // ELOOP
+
+        deletePath(target);
+        createDirs(target + "/snafu");
+        ASSERT_THROW(moveFile(source, target), Error); // ENOTEMPTY or EEXIST
+    }
+
     /* ----------------------------------------------------------------------------
      * AutoCloseFD::guessOrInventPath
      * --------------------------------------------------------------------------*/