From 90f968073338d6ba276994bc281fa5efa5306e17 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?=
 <theophane.hufschmitt@tweag.io>
Date: Wed, 13 Apr 2022 14:19:42 +0200
Subject: [PATCH] Only use `renameFile` where needed
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In most places the fallback to copying isn’t needed and can actually be
bad, so we’d rather not transparently fallback
---
 src/libstore/local-store.cc |  2 +-
 src/libutil/filesystem.cc   | 14 +++++++++-----
 src/libutil/util.hh         |  9 +++++++++
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index 2d076f404..a272e4301 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -1430,7 +1430,7 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name
                     writeFile(realPath, dumpSource);
             } else {
                 /* Move the temporary path we restored above. */
-                renameFile(tempPath, realPath);
+                moveFile(tempPath, realPath);
             }
 
             /* For computing the nar hash. In recursive SHA-256 mode, this
diff --git a/src/libutil/filesystem.cc b/src/libutil/filesystem.cc
index fee40d09e..9cc18507b 100644
--- a/src/libutil/filesystem.cc
+++ b/src/libutil/filesystem.cc
@@ -54,7 +54,6 @@ void setWriteTime(const fs::path & p, const struct stat & st)
         .tv_sec = st.st_mtime,
         .tv_usec = 0,
     };
-    warn("Setting the mtime of %s to %d", p.c_str(), st.st_mtim.tv_sec);
     if (lutimes(p.c_str(), times) != 0)
         throw SysError("changing modification time of '%s'", p);
 }
@@ -92,14 +91,19 @@ void copy(const fs::directory_entry & from, const fs::path & to, bool andDelete)
 
 void renameFile(const Path & oldName, const Path & newName)
 {
-    auto oldPath = fs::path(oldName);
-    auto newPath = fs::path(newName);
+    fs::rename(oldName, newName);
+}
+
+void moveFile(const Path & oldName, const Path & newName)
+{
     try {
-        fs::rename(oldPath, newPath);
+        renameFile(oldName, newName);
     } catch (fs::filesystem_error & e) {
+        auto oldPath = fs::path(oldName);
+        auto newPath = fs::path(newName);
         if (e.code().value() == EXDEV) {
             fs::remove(newPath);
-            warn("Copying %s to %s", oldName, newName);
+            warn("Can’t rename %s as %s, copying instead", oldName, newName);
             copy(fs::directory_entry(oldPath), newPath, true);
         }
     }
diff --git a/src/libutil/util.hh b/src/libutil/util.hh
index 186513cea..cd83f250f 100644
--- a/src/libutil/util.hh
+++ b/src/libutil/util.hh
@@ -170,6 +170,15 @@ void replaceSymlink(const Path & target, const Path & link,
 
 void renameFile(const Path & src, const Path & dst);
 
+/**
+ * Similar to 'renameFile', but fallback to a copy+remove if `src` and `dst`
+ * are on a different filesystem.
+ *
+ * Beware that this might not be atomic because of the copy that happens behind
+ * the scenes
+ */
+void moveFile(const Path & src, const Path & dst);
+
 
 /* Wrappers arount read()/write() that read/write exactly the
    requested number of bytes. */