Make Util::make_relative_path able to find matches for canonical path (#760)

Scenario:

- /tmp is a symlink to /private/tmp.
- Both apparent and actual CWD is /private/tmp/dir.
- A path (e.g. the object file) on the command line is /tmp/dir/file.o.
- Base directory is set up to match /tmp/dir/file.o.

Ccache then rewrites /tmp/dir/file.o into ../../private/tmp/dir/file.o,
which is correct but not optimal since ./file.o would be a better
relative path. Especially on macOS where, for unknown reasons, the
kernel sometimes disallows opening a file like
../../private/tmp/dir/file.o for writing.

Improve this by letting Util::make_relative_path try to match
real_path(path) against the CWDs and choose the best match.

Closes #724.

diff --git a/src/Util.cpp b/src/Util.cpp
index edeba42325df1b4bd1d3084a3766ddcf8cd08af9..4be232edcba84713e99c6de143fed8367ed374df 100644 (file)
--- a/src/Util.cpp
+++ b/src/Util.cpp
@@ -1,4 +1,4 @@
-// Copyright (C) 2019-2020 Joel Rosdahl and other contributors
+// Copyright (C) 2019-2021 Joel Rosdahl and other contributors
 //
 // See doc/AUTHORS.adoc for a complete list of contributors.
 //
@@ -895,28 +895,36 @@ make_relative_path(const std::string& base_dir,
   // The algorithm for computing relative paths below only works for existing
   // paths. If the path doesn't exist, find the first ancestor directory that
   // does exist and assemble the path again afterwards.
-  string_view original_path = path;
-  std::string path_suffix;
+
+  std::vector<std::string> relpath_candidates;
+  const auto original_path = path;
   Stat path_stat;
   while (!(path_stat = Stat::stat(std::string(path)))) {
     path = Util::dir_name(path);
   }
-  path_suffix = std::string(original_path.substr(path.length()));
+  const auto path_suffix = std::string(original_path.substr(path.length()));
+  const auto real_path = Util::real_path(std::string(path));
 
-  std::string path_str(path);
-  std::string normalized_path = Util::normalize_absolute_path(path_str);
-  std::vector<std::string> relpath_candidates = {
-    Util::get_relative_path(actual_cwd, normalized_path),
-  };
-  if (apparent_cwd != actual_cwd) {
-    relpath_candidates.emplace_back(
-      Util::get_relative_path(apparent_cwd, normalized_path));
-    // Move best (= shortest) match first:
-    if (relpath_candidates[0].length() > relpath_candidates[1].length()) {
-      std::swap(relpath_candidates[0], relpath_candidates[1]);
+  const auto add_relpath_candidates = [&](nonstd::string_view path) {
+    const std::string normalized_path = Util::normalize_absolute_path(path);
+    relpath_candidates.push_back(
+      Util::get_relative_path(actual_cwd, normalized_path));
+    if (apparent_cwd != actual_cwd) {
+      relpath_candidates.emplace_back(
+        Util::get_relative_path(apparent_cwd, normalized_path));
     }
+  };
+  add_relpath_candidates(path);
+  if (real_path != path) {
+    add_relpath_candidates(real_path);
   }
 
+  // Find best (i.e. shortest existing) match:
+  std::sort(relpath_candidates.begin(),
+            relpath_candidates.end(),
+            [](const std::string& path1, const std::string& path2) {
+              return path1.length() < path2.length();
+            });
   for (const auto& relpath : relpath_candidates) {
     if (Stat::stat(relpath).same_inode_as(path_stat)) {
       return relpath + path_suffix;

diff --git a/unittest/test_Util.cpp b/unittest/test_Util.cpp
index 0d3cae367590f9ccc89bac6d3925f43d82427b27..ce3c1115deb57ae85000f8adf9ad9d6fbc2b92a1 100644 (file)
--- a/unittest/test_Util.cpp
+++ b/unittest/test_Util.cpp
@@ -564,7 +564,6 @@ TEST_CASE("Util::is_dir_separator")
 #endif
 }
 
-#ifndef _WIN32
 TEST_CASE("Util::make_relative_path")
 {
   using Util::make_relative_path;
@@ -573,11 +572,17 @@ TEST_CASE("Util::make_relative_path")
 
   const std::string cwd = Util::get_actual_cwd();
   const std::string actual_cwd = FMT("{}/d", cwd);
+#ifdef _WIN32
+  const std::string apparent_cwd = actual_cwd;
+#else
   const std::string apparent_cwd = FMT("{}/s", cwd);
+#endif
 
   REQUIRE(Util::create_dir("d"));
+#ifndef _WIN32
   REQUIRE(symlink("d", "s") == 0);
-  REQUIRE(chdir("s") == 0);
+#endif
+  REQUIRE(chdir("d") == 0);
   Util::setenv("PWD", apparent_cwd);
 
   SUBCASE("No base directory")
@@ -587,22 +592,40 @@ TEST_CASE("Util::make_relative_path")
 
   SUBCASE("Path matches neither actual nor apparent CWD")
   {
+#ifdef _WIN32
+    CHECK(make_relative_path("C:/", "C:/a", "C:/b", "C:/x") == "C:/x");
+#else
     CHECK(make_relative_path("/", "/a", "/b", "/x") == "/x");
+#endif
   }
 
   SUBCASE("Match of actual CWD")
   {
+#ifdef _WIN32
+    CHECK(
+      make_relative_path(
+        actual_cwd.substr(0, 3), actual_cwd, apparent_cwd, actual_cwd + "/x")
+      == "./x");
+#else
     CHECK(make_relative_path("/", actual_cwd, apparent_cwd, actual_cwd + "/x")
           == "./x");
+#endif
   }
 
+#ifndef _WIN32
   SUBCASE("Match of apparent CWD")
   {
     CHECK(make_relative_path("/", actual_cwd, apparent_cwd, apparent_cwd + "/x")
           == "./x");
   }
-}
+
+  SUBCASE("Match if using resolved (using realpath(3)) path")
+  {
+    CHECK(make_relative_path("/", actual_cwd, actual_cwd, apparent_cwd + "/x")
+          == "./x");
+  }
 #endif
+}
 
 TEST_CASE("Util::matches_dir_prefix_or_file")
 {