Refactor and simplify get_path_in_cache and tests

This avoids depending on global state, making testing and reasoning
about code behavior easier.

diff --git a/src/Config.hpp b/src/Config.hpp
index ad3c35c931998c2e461e1d093ffce6c45adcfb30..bf8dbfaa48c86f8965c3b3db4356baa394326b27 100644 (file)
--- a/src/Config.hpp
+++ b/src/Config.hpp
@@ -151,8 +151,6 @@ private:
                 bool from_env_variable,
                 bool negate,
                 const std::string& origin);
-
-  friend struct ConfigTester;
 };
 
 inline const std::string&

diff --git a/src/Util.cpp b/src/Util.cpp
index b44c3d18974440aea7da82d36a4b9ed0b5f5077b..871fbeb9a4bfb6b28bea9ccbe0d4932afa05f7e2 100644 (file)
--- a/src/Util.cpp
+++ b/src/Util.cpp
@@ -216,16 +216,17 @@ get_level_1_files(const std::string& dir,
 }
 
 std::string
-get_path_in_cache(string_view name, string_view suffix)
+get_path_in_cache(string_view cache_dir,
+                  uint32_t levels,
+                  string_view name,
+                  string_view suffix)
 {
-  std::string path = g_config.cache_dir();
-
-  auto cache_dir_levels = g_config.cache_dir_levels();
-  path.reserve(path.size() + cache_dir_levels * 2 + 1 + name.length()
-               - cache_dir_levels + suffix.length());
+  std::string path(cache_dir);
+  path.reserve(path.size() + levels * 2 + 1 + name.length() - levels
+               + suffix.length());
 
   unsigned level = 0;
-  for (; level < cache_dir_levels; ++level) {
+  for (; level < levels; ++level) {
     path.push_back('/');
     path.push_back(name.at(level));
   }

diff --git a/src/Util.hpp b/src/Util.hpp
index 03bf9ac66f93505cd5007ed4cd11e29c6a4b2955..7b489b7ff623547a7d69216822395fe23b33df52 100644 (file)
--- a/src/Util.hpp
+++ b/src/Util.hpp
@@ -21,6 +21,7 @@
 #include "system.hpp"
 
 #include "CacheFile.hpp"
+#include "Config.hpp"
 
 #include "third_party/nonstd/string_view.hpp"
 
@@ -141,16 +142,17 @@ void get_level_1_files(const std::string& dir,
                        const ProgressReceiver& progress_receiver,
                        std::vector<std::shared_ptr<CacheFile>>& files);
 
-// Join the global cache directory, a '/', `name`, and `suffix` into a single
-// path and return it. Additionally N single-character, '/'-separated subpaths
-// are split from the beginning of `name` before joining them all, where N is
-// the number of globally configured cache dir levels.
+// Join `cache_dir`, a '/', `name`, and `suffix` into a single path and return
+// it. Additionally `levels` single-character, '/'-separated subpaths are split
+// from the beginning of `name` before joining them all.
 //
 // Throws if cache dir levels is greater than the length of `name`.
 //
 // E.g. "ABCDEF" and ".foo" will become "/ccache/A/B/CDEF.foo" when the cache
 // directory is "/ccache" and cache dir levels is 2.
-std::string get_path_in_cache(nonstd::string_view name,
+std::string get_path_in_cache(nonstd::string_view cache_dir,
+                              uint32_t levels,
+                              nonstd::string_view name,
                               nonstd::string_view suffix);
 
 // Write bytes in big endian order from an integer value.

diff --git a/src/ccache.cpp b/src/ccache.cpp
index 901a366eea701026fba8a3fa9eec1a6b2eaffd97..81f738113da5b5b9ee876d36dbd891b64d341f61 100644 (file)
--- a/src/ccache.cpp
+++ b/src/ccache.cpp
@@ -1238,7 +1238,11 @@ update_cached_result_globals(struct digest* result_name)
   digest_as_string(result_name, result_name_string);
   cached_result_name = result_name;
   cached_result_path =
-    x_strdup(Util::get_path_in_cache(result_name_string, ".result").c_str());
+    x_strdup(Util::get_path_in_cache(g_config.cache_dir(),
+                                     g_config.cache_dir_levels(),
+                                     result_name_string,
+                                     ".result")
+               .c_str());
   stats_file =
     format("%s/%c/stats", g_config.cache_dir().c_str(), result_name_string[0]);
 }
@@ -2132,8 +2136,12 @@ calculate_result_name(struct args* args, struct hash* hash, int direct_mode)
 
     char manifest_name_string[DIGEST_STRING_BUFFER_SIZE];
     hash_result_as_string(hash, manifest_name_string);
-    manifest_path = x_strdup(
-      Util::get_path_in_cache(manifest_name_string, ".manifest").c_str());
+    manifest_path =
+      x_strdup(Util::get_path_in_cache(g_config.cache_dir(),
+                                       g_config.cache_dir_levels(),
+                                       manifest_name_string,
+                                       ".manifest")
+                 .c_str());
     manifest_stats_file = format(
       "%s/%c/stats", g_config.cache_dir().c_str(), manifest_name_string[0]);
 

diff --git a/unittest/test_Config.cpp b/unittest/test_Config.cpp
index 0edb75075ae1741e7a5c0f2aac231d70497e4fbe..2521421bd266fc52fe8779b3057800d60142d8db 100644 (file)
--- a/unittest/test_Config.cpp
+++ b/unittest/test_Config.cpp
@@ -16,8 +16,6 @@
 // this program; if not, write to the Free Software Foundation, Inc., 51
 // Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
-#include "test_Config.hpp"
-
 #include "../src/Config.hpp"
 #include "../src/Error.hpp"
 #include "../src/Util.hpp"

diff --git a/unittest/test_Config.hpp b/unittest/test_Config.hpp
deleted file mode 100644 (file)
index f5e4fbf..0000000
--- a/unittest/test_Config.hpp
+++ /dev/null
@@ -1,11 +0,0 @@
-#pragma once
-
-#include "../src/Config.hpp"
-
-// Helper to modify private Config variables without adding setters that would
-// only be used for testing.
-// This struct MUST NOT have any data members, only the required private member
-// variables should be exposed.
-struct ConfigTester : Config {
-  using Config::m_cache_dir_levels;
-};

diff --git a/unittest/test_Util.cpp b/unittest/test_Util.cpp
index ce809aa5026a4a409942e4fe3bc6d21792a368c2..665d6650169a0e9627430efcf0cb2459f1a61564 100644 (file)
--- a/unittest/test_Util.cpp
+++ b/unittest/test_Util.cpp
@@ -18,7 +18,6 @@
 
 #include "../src/Config.hpp"
 #include "../src/Util.hpp"
-#include "test_Config.hpp"
 
 #include "third_party/catch.hpp"
 
@@ -251,54 +250,37 @@ TEST_CASE("Util::get_level_1_files")
 
 TEST_CASE("Util::get_path_in_cache")
 {
-  Config saved = g_config;
-
-  ConfigTester testconfig;
-  testconfig.set_cache_dir("/zz/ccache");
-
   {
-    testconfig.m_cache_dir_levels = 0;
-    g_config = testconfig;
-    std::string path = Util::get_path_in_cache("ABCDEF", ".suffix");
+    std::string path =
+      Util::get_path_in_cache("/zz/ccache", 0, "ABCDEF", ".suffix");
     CHECK(path == "/zz/ccache/ABCDEF.suffix");
   }
 
   {
-    testconfig.m_cache_dir_levels = 1;
-    g_config = testconfig;
-    std::string path = Util::get_path_in_cache("ABCDEF", ".suffix");
+    std::string path =
+      Util::get_path_in_cache("/zz/ccache", 1, "ABCDEF", ".suffix");
     CHECK(path == "/zz/ccache/A/BCDEF.suffix");
   }
 
   {
-    testconfig.m_cache_dir_levels = 4;
-    g_config = testconfig;
-    std::string path = Util::get_path_in_cache("ABCDEF", ".suffix");
+    std::string path =
+      Util::get_path_in_cache("/zz/ccache", 4, "ABCDEF", ".suffix");
     CHECK(path == "/zz/ccache/A/B/C/D/EF.suffix");
   }
 
   {
-    testconfig.m_cache_dir_levels = 0;
-    g_config = testconfig;
-    std::string path = Util::get_path_in_cache("", ".suffix");
+    std::string path = Util::get_path_in_cache("/zz/ccache", 0, "", ".suffix");
     CHECK(path == "/zz/ccache/.suffix");
   }
 
   {
-    testconfig.m_cache_dir_levels = 2;
-    g_config = testconfig;
-    std::string path = Util::get_path_in_cache("AB", ".suffix");
+    std::string path =
+      Util::get_path_in_cache("/zz/ccache", 2, "AB", ".suffix");
     CHECK(path == "/zz/ccache/A/B/.suffix");
   }
 
-  {
-    testconfig.m_cache_dir_levels = 3;
-    g_config = testconfig;
-    REQUIRE_THROWS_WITH(Util::get_path_in_cache("AB", ".suffix"),
-                        EndsWith("string_view::at()"));
-  }
-
-  g_config = saved;
+  REQUIRE_THROWS_WITH(Util::get_path_in_cache("/zz/ccache", 3, "AB", ".suffix"),
+                      EndsWith("string_view::at()"));
 }
 
 TEST_CASE("Util::int_to_big_endian")