Merge pull request #12667 from NixOS/in-dir-cleanup

Cleanup `isInDir` and  `isDirOrInDir`

src/libstore/gc.cc

@@ -245,7 +245,7 @@ void LocalStore::findRoots(const Path & path, std::filesystem::file_type type, R
             else {
                 target = absPath(target, dirOf(path));
                 if (!pathExists(target)) {
-                    if (isInDir(path, stateDir + "/" + gcRootsDir + "/auto")) {
+                    if (isInDir(path, std::filesystem::path{stateDir.get()} / gcRootsDir / "auto")) {
                         printInfo("removing stale link from '%1%' to '%2%'", path, target);
                         unlink(path.c_str());
                     }

src/libstore/unix/build/local-derivation-goal.cc

@@ -1046,7 +1046,7 @@ void LocalDerivationGoal::startBuilder()
             /* If only we had a trie to do this more efficiently :) luckily, these are generally going to be pretty small */
             for (auto & a : allowedPaths) {
                 Path canonA = canonPath(a);
-                if (canonI == canonA || isInDir(canonI, canonA)) {
+                if (isDirOrInDir(canonI, canonA)) {
                     found = true;
                     break;
                 }

src/libutil-tests/file-system.cc

@@ -203,12 +203,10 @@ TEST(isInDir, notInDir)
     ASSERT_EQ(p1, false);
 }
 
-// XXX: hm, bug or feature? :) Looking at the implementation
-// this might be problematic.
 TEST(isInDir, emptyDir)
 {
     auto p1 = isInDir("/zes/foo/bar", "");
-    ASSERT_EQ(p1, true);
+    ASSERT_EQ(p1, false);
 }
 
 /* ----------------------------------------------------------------------------
@@ -233,14 +231,12 @@ TEST(isDirOrInDir, falseForDisjunctPaths)
 
 TEST(isDirOrInDir, relativePaths)
 {
-    ASSERT_EQ(isDirOrInDir("/foo/..", "/foo"), true);
+    ASSERT_EQ(isDirOrInDir("/foo/..", "/foo"), false);
 }
 
-// XXX: while it is possible to use "." or ".." in the
+TEST(isDirOrInDir, relativePathsTwice)
-// first argument this doesn't seem to work in the second.
-TEST(isDirOrInDir, DISABLED_shouldWork)
 {
-    ASSERT_EQ(isDirOrInDir("/foo/..", "/foo/."), true);
+    ASSERT_EQ(isDirOrInDir("/foo/..", "/foo/."), false);
 }
 
 /* ----------------------------------------------------------------------------

src/libutil/file-system.cc

@@ -149,16 +149,18 @@ std::string_view baseNameOf(std::string_view path)
 }
 
 
-bool isInDir(std::string_view path, std::string_view dir)
+bool isInDir(const fs::path & path, const fs::path & dir)
 {
-    return path.substr(0, 1) == "/"
+    /* Note that while the standard doesn't guarantee this, the
-        && path.substr(0, dir.size()) == dir
+      `lexically_*` functions should do no IO and not throw. */
-        && path.size() >= dir.size() + 2
+    auto rel = path.lexically_relative(dir);
-        && path[dir.size()] == '/';
+    /* Method from
+       https://stackoverflow.com/questions/62503197/check-if-path-contains-another-in-c++ */
+    return !rel.empty() && rel.native()[0] != OS_STR('.');
 }
 
 
-bool isDirOrInDir(std::string_view path, std::string_view dir)
+bool isDirOrInDir(const fs::path & path, const fs::path & dir)
 {
     return path == dir || isInDir(path, dir);
 }

src/libutil/file-system.hh

@@ -105,13 +105,13 @@ std::string_view baseNameOf(std::string_view path);
  * Check whether 'path' is a descendant of 'dir'. Both paths must be
  * canonicalized.
  */
-bool isInDir(std::string_view path, std::string_view dir);
+bool isInDir(const std::filesystem::path & path, const std::filesystem::path & dir);
 
 /**
  * Check whether 'path' is equal to 'dir' or a descendant of
  * 'dir'. Both paths must be canonicalized.
  */
-bool isDirOrInDir(std::string_view path, std::string_view dir);
+bool isDirOrInDir(const std::filesystem::path & path, const std::filesystem::path & dir);
 
 /**
  * Get status of `path`.