From: Joel Rosdahl <joel@rosdahl.net>
Date: Sun, 9 Jul 2023 19:53:54 +0000 (+0200)
Subject: refactor: Use fs::create_hard_link instead of custom implementation
X-Git-Tag: v4.9~139
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=45dc2b3d59c0acd48a2d1d3832a80871c25351e1;p=thirdparty%2Fccache.git

refactor: Use fs::create_hard_link instead of custom implementation
---

diff --git a/src/Util.cpp b/src/Util.cpp
index ee0890fdc..61992478f 100644
--- a/src/Util.cpp
+++ b/src/Util.cpp
@@ -491,25 +491,6 @@ get_umask()
   return g_umask;
 }
 
-void
-hard_link(const std::string& oldpath, const std::string& newpath)
-{
-  // Assumption: newpath may already exist as a left-over file from a previous
-  // run, but it's only we who can create the file entry now so we don't try to
-  // handle a race between unlink() and link() below.
-  unlink(newpath.c_str());
-
-#ifndef _WIN32
-  if (link(oldpath.c_str(), newpath.c_str()) != 0) {
-    throw core::Error(strerror(errno));
-  }
-#else
-  if (!CreateHardLink(newpath.c_str(), oldpath.c_str(), nullptr)) {
-    throw core::Error(Win32Util::error_message(GetLastError()));
-  }
-#endif
-}
-
 std::optional<size_t>
 is_absolute_path_with_prefix(std::string_view path)
 {
diff --git a/src/Util.hpp b/src/Util.hpp
index bafd185a8..7026cfccb 100644
--- a/src/Util.hpp
+++ b/src/Util.hpp
@@ -107,9 +107,6 @@ std::string get_relative_path(std::string_view dir, std::string_view path);
 // Get process umask.
 mode_t get_umask();
 
-// Hard-link `oldpath` to `newpath`. Throws `core::Error` on error.
-void hard_link(const std::string& oldpath, const std::string& newpath);
-
 // Determine if `path` is an absolute path with prefix, returning the split
 // point.
 std::optional<size_t> is_absolute_path_with_prefix(std::string_view path);
diff --git a/src/storage/local/LocalStorage.cpp b/src/storage/local/LocalStorage.cpp
index bfb20ae3a..1d073f7fb 100644
--- a/src/storage/local/LocalStorage.cpp
+++ b/src/storage/local/LocalStorage.cpp
@@ -77,11 +77,14 @@
 #include <algorithm>
 #include <atomic>
 #include <cstdlib>
+#include <filesystem>
 #include <memory>
 #include <numeric>
 #include <string>
 #include <utility>
 
+namespace fs = std::filesystem;
+
 using core::Statistic;
 using core::StatisticsCounters;
 
@@ -638,19 +641,24 @@ LocalStorage::clone_hard_link_or_copy_file(const std::string& source,
 #endif
   }
   if (m_config.hard_link()) {
+    // Assumption: dest may already exist as a left-over file from a previous
+    // run, but it's only we who can create the file entry now so we don't try
+    // to handle a race between remove() and create_hard_link() below.
+
+    std::error_code ec;
+    fs::remove(dest, ec); // Ignore any error.
     LOG("Hard linking {} to {}", source, dest);
-    try {
-      Util::hard_link(source, dest);
+    fs::create_hard_link(source, dest, ec);
+    if (!ec) {
 #ifndef _WIN32
       if (chmod(dest.c_str(), 0444 & ~Util::get_umask()) != 0) {
         LOG("Failed to chmod {}: {}", dest.c_str(), strerror(errno));
       }
 #endif
       return;
-    } catch (const core::Error& e) {
-      LOG("Failed to hard link {} to {}: {}", source, dest, e.what());
-      // Fall back to copying.
     }
+    LOG("Failed to hard link {} to {}: {}", source, dest, ec.message());
+    // Fall back to copying.
   }
 
   LOG("Copying {} to {}", source, dest);
diff --git a/unittest/test_Util.cpp b/unittest/test_Util.cpp
index 5b5832541..321b69ed3 100644
--- a/unittest/test_Util.cpp
+++ b/unittest/test_Util.cpp
@@ -231,31 +231,6 @@ TEST_CASE("Util::get_relative_path")
 #endif
 }
 
-TEST_CASE("Util::hard_link")
-{
-  TestContext test_context;
-
-  SUBCASE("Link file to nonexistent destination")
-  {
-    util::write_file("old", "content");
-    CHECK_NOTHROW(Util::hard_link("old", "new"));
-    CHECK(*util::read_file<std::string>("new") == "content");
-  }
-
-  SUBCASE("Link file to existing destination")
-  {
-    util::write_file("old", "content");
-    util::write_file("new", "other content");
-    CHECK_NOTHROW(Util::hard_link("old", "new"));
-    CHECK(*util::read_file<std::string>("new") == "content");
-  }
-
-  SUBCASE("Link nonexistent file")
-  {
-    CHECK_THROWS_AS(Util::hard_link("old", "new"), core::Error);
-  }
-}
-
 TEST_CASE("Util::is_absolute_path_with_prefix")
 {
   CHECK(*Util::is_absolute_path_with_prefix("-I/c/foo") == 2);