From: Joel Rosdahl <joel@rosdahl.net>
Date: Tue, 11 Jul 2023 13:08:29 +0000 (+0200)
Subject: refactor: Move Util::rename to util
X-Git-Tag: v4.9~136
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=876b4688a134e966632e8bb67986d89021f1c88c;p=thirdparty%2Fccache.git

refactor: Move Util::rename to util
---

diff --git a/src/AtomicFile.cpp b/src/AtomicFile.cpp
index 9c0f0cb3c..ef6fe8e12 100644
--- a/src/AtomicFile.cpp
+++ b/src/AtomicFile.cpp
@@ -1,4 +1,4 @@
-// Copyright (C) 2019-2022 Joel Rosdahl and other contributors
+// Copyright (C) 2019-2023 Joel Rosdahl and other contributors
 //
 // See doc/AUTHORS.adoc for a complete list of contributors.
 //
@@ -24,6 +24,7 @@
 
 #include <core/exceptions.hpp>
 #include <fmtmacros.hpp>
+#include <util/file.hpp>
 
 AtomicFile::AtomicFile(const std::string& path, Mode mode) : m_path(path)
 {
@@ -72,12 +73,16 @@ void
 AtomicFile::commit()
 {
   ASSERT(m_stream);
-  int result = fclose(m_stream);
+  int retcode = fclose(m_stream);
   m_stream = nullptr;
-  if (result == EOF) {
+  if (retcode == EOF) {
     Util::unlink_tmp(m_tmp_path);
     throw core::Error(
       FMT("failed to write data to {}: {}", m_path, strerror(errno)));
   }
-  Util::rename(m_tmp_path, m_path);
+  const auto result = util::rename(m_tmp_path, m_path);
+  if (!result) {
+    throw core::Error(
+      FMT("failed to rename {} to {}: {}", m_tmp_path, m_path, result.error()));
+  }
 }
diff --git a/src/Util.cpp b/src/Util.cpp
index 6e3083c46..2efbe2e99 100644
--- a/src/Util.cpp
+++ b/src/Util.cpp
@@ -784,28 +784,6 @@ remove_extension(std::string_view path)
   return path.substr(0, path.length() - get_extension(path).length());
 }
 
-void
-rename(const std::string& oldpath, const std::string& newpath)
-{
-#ifndef _WIN32
-  if (::rename(oldpath.c_str(), newpath.c_str()) != 0) {
-    throw core::Error(
-      FMT("failed to rename {} to {}: {}", oldpath, newpath, strerror(errno)));
-  }
-#else
-  // Windows' rename() won't overwrite an existing file, so need to use
-  // MoveFileEx instead.
-  if (!MoveFileExA(
-        oldpath.c_str(), newpath.c_str(), MOVEFILE_REPLACE_EXISTING)) {
-    DWORD error = GetLastError();
-    throw core::Error(FMT("failed to rename {} to {}: {}",
-                          oldpath,
-                          newpath,
-                          Win32Util::error_message(error)));
-  }
-#endif
-}
-
 void
 send_to_fd(const Context& ctx, std::string_view text, int fd)
 {
@@ -998,9 +976,8 @@ unlink_safe(const std::string& path, UnlinkLog unlink_log)
     FMT("{}.ccache{}unlink", path, TemporaryFile::tmp_file_infix);
 
   bool success = true;
-  try {
-    Util::rename(path, tmp_name);
-  } catch (core::Error&) {
+  const auto result = util::rename(path, tmp_name);
+  if (!result) {
     success = false;
     saved_errno = errno;
   }
diff --git a/src/Util.hpp b/src/Util.hpp
index 42cea7a13..9ff59de73 100644
--- a/src/Util.hpp
+++ b/src/Util.hpp
@@ -189,10 +189,6 @@ std::string real_path(const std::string& path,
 // extension as determined by `get_extension()`.
 std::string_view remove_extension(std::string_view path);
 
-// Rename `oldpath` to `newpath` (deleting `newpath`). Throws `core::Error` on
-// error.
-void rename(const std::string& oldpath, const std::string& newpath);
-
 // Send `text` to file descriptor `fd`, optionally stripping ANSI color
 // sequences if `ctx.args_info.strip_diagnostics_colors` is true and rewriting
 // paths to absolute if `ctx.config.absolute_paths_in_stderr` is true. Throws
diff --git a/src/storage/local/LocalStorage.cpp b/src/storage/local/LocalStorage.cpp
index 1d073f7fb..e46abcaa3 100644
--- a/src/storage/local/LocalStorage.cpp
+++ b/src/storage/local/LocalStorage.cpp
@@ -258,7 +258,11 @@ clone_file(const std::string& src, const std::string& dest, bool via_tmp_file)
   src_fd.close();
 
   if (via_tmp_file) {
-    Util::rename(tmp_file, dest);
+    const auto result = util::rename(tmp_file, dest);
+    if (!result) {
+      throw core::Error(
+        FMT("failed to rename {} to {}: {}", tmp_file, dest, result.error()));
+    }
   }
 #  elif defined(__APPLE__)
   (void)via_tmp_file;
@@ -1024,22 +1028,15 @@ LocalStorage::move_to_wanted_cache_level(const StatisticsCounters& counters,
     wanted_level, util::format_digest(key) + suffix_from_type(type));
   if (cache_file_path != wanted_path) {
     Util::ensure_dir_exists(Util::dir_name(wanted_path));
+
+    // Note: Two ccache processes may move the file at the same time, so failure
+    // to rename is OK.
     LOG("Moving {} to {}", cache_file_path, wanted_path);
-    try {
-      Util::rename(cache_file_path, wanted_path);
-    } catch (const core::Error&) {
-      // Two ccache processes may move the file at the same time, so failure
-      // to rename is OK.
-    }
+    util::rename(cache_file_path, wanted_path);
     for (const auto& raw_file : m_added_raw_files) {
-      try {
-        Util::rename(
-          raw_file,
-          FMT("{}/{}", Util::dir_name(wanted_path), Util::base_name(raw_file)));
-      } catch (const core::Error&) {
-        // Two ccache processes may move the file at the same time, so failure
-        // to rename is OK.
-      }
+      util::rename(
+        raw_file,
+        FMT("{}/{}", Util::dir_name(wanted_path), Util::base_name(raw_file)));
     }
   }
 }
diff --git a/src/util/file.cpp b/src/util/file.cpp
index 48d987af6..ed6d8f932 100644
--- a/src/util/file.cpp
+++ b/src/util/file.cpp
@@ -22,11 +22,11 @@
 #include <Logging.hpp>
 #include <Stat.hpp>
 #include <TemporaryFile.hpp>
-#include <Util.hpp>
 #include <Win32Util.hpp>
 #include <fmtmacros.hpp>
 #include <util/Bytes.hpp>
 #include <util/expected.hpp>
+#include <util/file.hpp>
 
 #ifdef HAVE_UNISTD_H
 #  include <unistd.h>
@@ -94,7 +94,11 @@ copy_file(const std::string& src,
   src_fd.close();
 
   if (via_tmp_file == ViaTmpFile::yes) {
-    Util::rename(tmp_file, dest);
+    const auto result = util::rename(tmp_file, dest);
+    if (!result) {
+      return nonstd::make_unexpected(
+        FMT("Failed to rename {} to {}: {}", tmp_file, dest, result.error()));
+    }
   }
 
   return {};
@@ -310,6 +314,25 @@ read_file_part(const std::string& path, size_t pos, size_t count);
 template nonstd::expected<std::vector<uint8_t>, std::string>
 read_file_part(const std::string& path, size_t pos, size_t count);
 
+nonstd::expected<void, std::string>
+rename(const std::string& oldpath, const std::string& newpath)
+{
+#ifndef _WIN32
+  if (::rename(oldpath.c_str(), newpath.c_str()) != 0) {
+    return nonstd::make_unexpected(strerror(errno));
+  }
+#else
+  // Windows' rename() won't overwrite an existing file, so need to use
+  // MoveFileEx instead.
+  if (!MoveFileExA(
+        oldpath.c_str(), newpath.c_str(), MOVEFILE_REPLACE_EXISTING)) {
+    DWORD error = GetLastError();
+    return nonstd::make_unexpected(Win32Util::error_message(error));
+  }
+#endif
+  return {};
+}
+
 void
 set_timestamps(const std::string& path,
                std::optional<util::TimePoint> mtime,
diff --git a/src/util/file.hpp b/src/util/file.hpp
index 286974e00..fd81cfaa1 100644
--- a/src/util/file.hpp
+++ b/src/util/file.hpp
@@ -77,6 +77,14 @@ template<typename T>
 nonstd::expected<T, std::string>
 read_file_part(const std::string& path, size_t pos, size_t count);
 
+// Rename `oldpath` to `newpath` (deleting `newpath`).
+//
+// Note: Mingw-w64's std::filesystem::rename is buggy and doesn't properly
+// overwrite an existing file, at least in version 9.1.0, hence this utility
+// function.
+nonstd::expected<void, std::string> rename(const std::string& oldpath,
+                                           const std::string& newpath);
+
 // Set atime/mtime of `path`. If `mtime` is std::nullopt, set to the current
 // time. If `atime` is std::nullopt, set to what `mtime` specifies.
 void set_timestamps(const std::string& path,