filesystem: Avoid unwanted indirections...

...that std::filesystem::* calls produce. This is because existence
and type checks use almost exclusively std::filesystem::status,
which follows symbolic links, when being called with path arguments.
Instead, one should instead use these methods with the value
returned by a call of std::filesystem::symlink_status.

This commit also streamlines the FileSystemManager tests, as well
as replace bare calls to std::filesystem with their FileSystemManager
counterparts (where suitable).

5 files changed, 85 insertions, 69 deletions

diff --git a/src/buildtool/file_system/file_system_manager.hpp b/src/buildtool/file_system/file_system_manager.hpp
index 868acbcc..04f84cf4 100644
--- a/src/buildtool/file_system/file_system_manager.hpp
+++ b/src/buildtool/file_system/file_system_manager.hpp
@@ -437,10 +437,11 @@ class FileSystemManager {
     [[nodiscard]] static auto RemoveFile(
         std::filesystem::path const& file) noexcept -> bool {
         try {
-            if (!std::filesystem::exists(file)) {
+            auto status = std::filesystem::symlink_status(file);
+            if (!std::filesystem::exists(status)) {
                 return true;
             }
-            if (!IsFile(file)) {
+            if (!std::filesystem::is_regular_file(status)) {
                 return false;
             }
             return std::filesystem::remove(file);
@@ -457,9 +458,13 @@ class FileSystemManager {
                                               bool recursively = false) noexcept
         -> bool {
         try {
-            if (!std::filesystem::exists(dir)) {
+            auto status = std::filesystem::symlink_status(dir);
+            if (!std::filesystem::exists(status)) {
                 return true;
             }
+            if (!std::filesystem::is_directory(status)) {
+                return false;
+            }
             if (recursively) {
                 return (std::filesystem::remove_all(dir) !=
                         static_cast<uintmax_t>(-1));
@@ -476,14 +481,16 @@ class FileSystemManager {
 
     /// \brief Returns if symlink is non-upwards, i.e., its string content path
     /// never passes itself in the directory tree.
+    /// \param non_strict if set, do not check non-upwardness. Use with care!
     [[nodiscard]] static auto IsNonUpwardsSymlink(
-        std::filesystem::path const& link) noexcept -> bool {
+        std::filesystem::path const& link,
+        bool non_strict = false) noexcept -> bool {
         try {
             if (not std::filesystem::is_symlink(link)) {
                 return false;
             }
-            auto dest = std::filesystem::read_symlink(link);
-            return PathIsNonUpwards(dest);
+            return non_strict or
+                   PathIsNonUpwards(std::filesystem::read_symlink(link));
         } catch (std::exception const& e) {
             Logger::Log(LogLevel::Error, e.what());
             return false;
@@ -929,7 +936,8 @@ class FileSystemManager {
     [[nodiscard]] static auto CreateDirectoryImpl(
         std::filesystem::path const& dir) noexcept -> CreationStatus {
         try {
-            if (std::filesystem::is_directory(dir)) {
+            if (std::filesystem::is_directory(
+                    std::filesystem::symlink_status(dir))) {
                 return CreationStatus::Exists;
             }
             if (std::filesystem::create_directories(dir)) {
@@ -939,7 +947,8 @@ class FileSystemManager {
             // after the current thread checked if it existed. For that reason,
             // we try to create it and check if it exists if create_directories
             // was not successful.
-            if (std::filesystem::is_directory(dir)) {
+            if (std::filesystem::is_directory(
+                    std::filesystem::symlink_status(dir))) {
                 return CreationStatus::Exists;
             }
 
@@ -955,7 +964,8 @@ class FileSystemManager {
     [[nodiscard]] static auto CreateFileImpl(
         std::filesystem::path const& file) noexcept -> CreationStatus {
         try {
-            if (std::filesystem::is_regular_file(file)) {
+            if (std::filesystem::is_regular_file(
+                    std::filesystem::symlink_status(file))) {
                 return CreationStatus::Exists;
             }
             if (gsl::owner<FILE*> fp = std::fopen(file.c_str(), "wx")) {
@@ -966,7 +976,8 @@ class FileSystemManager {
             // the current thread checked if it existed. For that reason, we try
             // to create it and check if it exists if fopen() with exclusive bit
             // was not successful.
-            if (std::filesystem::is_regular_file(file)) {
+            if (std::filesystem::is_regular_file(
+                    std::filesystem::symlink_status(file))) {
                 return CreationStatus::Exists;
             }
             return CreationStatus::Failed;
@@ -983,6 +994,11 @@ class FileSystemManager {
             std::filesystem::copy_options::overwrite_existing) noexcept
         -> bool {
         try {
+            // src and dst should be actual files
+            if (std::filesystem::is_symlink(src) or
+                std::filesystem::is_symlink(dst)) {
+                return false;
+            }
             return std::filesystem::copy_file(src, dst, opt);
         } catch (std::exception const& e) {
             Logger::Log(LogLevel::Error,
diff --git a/src/buildtool/main/main.cpp b/src/buildtool/main/main.cpp
index 846fe257..8d2d4d2e 100644
--- a/src/buildtool/main/main.cpp
+++ b/src/buildtool/main/main.cpp
@@ -479,7 +479,7 @@ void SetupHashFunction() {
     -> Configuration {
     Configuration config{};
     if (not clargs.config_file.empty()) {
-        if (not std::filesystem::exists(clargs.config_file)) {
+        if (not FileSystemManager::Exists(clargs.config_file)) {
             Logger::Log(LogLevel::Error,
                         "Config file {} does not exist.",
                         clargs.config_file.string());
diff --git a/test/buildtool/execution_api/bazel/bazel_msg_factory.test.cpp b/test/buildtool/execution_api/bazel/bazel_msg_factory.test.cpp
index 2d7aa602..fde64e10 100644
--- a/test/buildtool/execution_api/bazel/bazel_msg_factory.test.cpp
+++ b/test/buildtool/execution_api/bazel/bazel_msg_factory.test.cpp
@@ -97,7 +97,8 @@ TEST_CASE("Bazel internals: MessageFactory", "[execution_api]") {
             for (auto const& digest : digests) {
                 REQUIRE(fake_cas.contains(digest));
                 auto fpath = fake_cas[digest];
-                if (std::filesystem::is_symlink(fpath)) {
+                if (FileSystemManager::IsNonUpwardsSymlink(
+                        fpath, /*non_strict=*/true)) {
                     auto content = FileSystemManager::ReadSymlink(fpath);
                     REQUIRE(content);
                     targets->emplace_back(*content);
diff --git a/test/buildtool/file_system/file_system_manager.test.cpp b/test/buildtool/file_system/file_system_manager.test.cpp
index 0c2ad292..8b1ac5c8 100644
--- a/test/buildtool/file_system/file_system_manager.test.cpp
+++ b/test/buildtool/file_system/file_system_manager.test.cpp
@@ -225,21 +225,12 @@ auto HasEpochTime(fs::path const& path) noexcept -> bool {
 
 }  // namespace
 
-TEST_CASE("CreateDirectory", "[file_system]") {
-    auto const dir = GENERATE(as<std::filesystem::path>{},
-                              "level0",
-                              "level0/level1",
-                              "a/b/c/d",
-                              "./a/../e");
-    CHECK(FileSystemManager::CreateDirectory(dir));
-    CHECK(std::filesystem::exists(dir));
-    CHECK(std::filesystem::is_directory(dir));
-
-    // If we have created the directory already, CreateDirectory() returns true
-    // and the state of things doesn't change
-    CHECK(FileSystemManager::CreateDirectory(dir));
-    CHECK(std::filesystem::exists(dir));
-    CHECK(std::filesystem::is_directory(dir));
+TEST_CASE("Exists", "[file_system]") {
+    CHECK(FileSystemManager::Exists("test/buildtool/file_system/data/"));
+    CHECK(FileSystemManager::Exists(
+        "test/buildtool/file_system/data/example_file"));
+    CHECK(FileSystemManager::Exists(
+        "test/buildtool/file_system/data/empty_executable"));
 }
 
 TEST_CASE("IsFile", "[file_system]") {
@@ -259,6 +250,14 @@ TEST_CASE("IsExecutable", "[file_system]") {
         FileSystemManager::IsExecutable("test/buildtool/file_system/data/"));
 }
 
+TEST_CASE("IsDirectory", "[file_system]") {
+    CHECK(FileSystemManager::IsDirectory("test/buildtool/file_system/data"));
+    CHECK_FALSE(FileSystemManager::IsDirectory(
+        "test/buildtool/file_system/data/example_file"));
+    CHECK_FALSE(FileSystemManager::IsDirectory(
+        "test/buildtool/file_system/data/empty_executable"));
+}
+
 TEST_CASE("Type", "[file_system]") {
     auto const type_file =
         FileSystemManager::Type("test/buildtool/file_system/data/example_file");
@@ -276,6 +275,21 @@ TEST_CASE("Type", "[file_system]") {
     CHECK(*type_dir == ObjectType::Tree);
 }
 
+TEST_CASE("CreateDirectory", "[file_system]") {
+    auto const dir = GENERATE(as<std::filesystem::path>{},
+                              "level0",
+                              "level0/level1",
+                              "a/b/c/d",
+                              "./a/../e");
+    CHECK(FileSystemManager::CreateDirectory(dir));
+    CHECK(FileSystemManager::IsDirectory(dir));
+
+    // If we have created the directory already, CreateDirectory() returns true
+    // and the state of things doesn't change
+    CHECK(FileSystemManager::CreateDirectory(dir));
+    CHECK(FileSystemManager::IsDirectory(dir));
+}
+
 TEST_CASE("ChangeDirectory", "[file_system]") {
     auto const starting_dir = FileSystemManager::GetCurrentDirectory();
 
@@ -311,7 +325,7 @@ TEST_CASE("ReadFile", "[file_system]") {
     SECTION("Non-existing file") {
         std::filesystem::path file{
             "test/buildtool/file_system/data/this_file_does_not_exist"};
-        REQUIRE(not std::filesystem::exists(file));
+        REQUIRE(not FileSystemManager::Exists(file));
 
         auto const content = FileSystemManager::ReadFile(file);
         CHECK_FALSE(content.has_value());
@@ -324,8 +338,7 @@ TEST_CASE_METHOD(CopyFileFixture, "CopyFile", "[file_system]") {
         CHECK(FileSystemManager::CopyFile(from_, to_, fd_less));
 
         // file exists
-        CHECK(std::filesystem::exists(to_));
-        CHECK(std::filesystem::is_regular_file(to_));
+        CHECK(FileSystemManager::IsFile(to_));
 
         // Contents are equal
         auto const content_from = FileSystemManager::ReadFile(from_);
@@ -347,8 +360,7 @@ TEST_CASE_METHOD(CopyFileFixture, "CopyFileAs", "[file_system]") {
                 from_, to_, ObjectType::File, fd_less));
 
             // file exists
-            CHECK(std::filesystem::exists(to_));
-            CHECK(std::filesystem::is_regular_file(to_));
+            CHECK(FileSystemManager::IsFile(to_));
             CHECK(not FileSystemManager::IsExecutable(to_));
 
             // Contents are equal
@@ -382,8 +394,7 @@ TEST_CASE_METHOD(CopyFileFixture, "CopyFileAs", "[file_system]") {
                 from_, to_, ObjectType::File, fd_less));
 
             // file exists
-            CHECK(std::filesystem::exists(to_));
-            CHECK(std::filesystem::is_regular_file(to_));
+            CHECK(FileSystemManager::IsFile(to_));
             CHECK(not FileSystemManager::IsExecutable(to_));
 
             // Contents are equal
@@ -416,8 +427,6 @@ TEST_CASE_METHOD(CopyFileFixture, "CopyFileAs", "[file_system]") {
                 from_, to_, ObjectType::Executable, fd_less));
 
             // file exists
-            CHECK(std::filesystem::exists(to_));
-            CHECK(std::filesystem::is_regular_file(to_));
             CHECK(FileSystemManager::IsExecutable(to_));
 
             // Contents are equal
@@ -451,8 +460,6 @@ TEST_CASE_METHOD(CopyFileFixture, "CopyFileAs", "[file_system]") {
                 from_, to_, ObjectType::Executable, fd_less));
 
             // file exists
-            CHECK(std::filesystem::exists(to_));
-            CHECK(std::filesystem::is_regular_file(to_));
             CHECK(FileSystemManager::IsExecutable(to_));
 
             // Contents are equal
@@ -490,24 +497,24 @@ TEST_CASE("RemoveFile", "[file_system]") {
 
         CHECK(FileSystemManager::CopyFile(from, to));
 
-        CHECK(std::filesystem::exists(to));
+        CHECK(FileSystemManager::Exists(to));
 
         CHECK(FileSystemManager::RemoveFile(to));
 
-        CHECK(not std::filesystem::exists(to));
+        CHECK(not FileSystemManager::Exists(to));
     }
     SECTION("Non-existing file") {
         std::filesystem::path file{
             "test/buildtool/file_system/data/"
             "this_file_does_not_exist_neither"};
-        CHECK(not std::filesystem::exists(file));
+        CHECK(not FileSystemManager::Exists(file));
         CHECK(FileSystemManager::RemoveFile(file));  // nothing to delete
     }
     SECTION("Existing but not file") {
         std::filesystem::path dir{"./tmp-RemoveFile/dir"};
         CHECK(FileSystemManager::CreateDirectory(dir));
         CHECK(not FileSystemManager::RemoveFile(dir));
-        CHECK(std::filesystem::exists(dir));
+        CHECK(FileSystemManager::Exists(dir));
     }
 }
 
@@ -516,9 +523,8 @@ TEST_CASE_METHOD(WriteFileFixture, "WriteFile", "[file_system]") {
 
     auto run_test = [&](bool fd_less) {
         CHECK(FileSystemManager::WriteFile(content, file_path_, fd_less));
-        CHECK(std::filesystem::exists(file_path_));
-        CHECK(std::filesystem::is_directory(file_path_.parent_path()));
-        CHECK(std::filesystem::is_regular_file(file_path_));
+        CHECK(FileSystemManager::IsDirectory(file_path_.parent_path()));
+        CHECK(FileSystemManager::IsFile(file_path_));
 
         auto const written_content = FileSystemManager::ReadFile(file_path_);
         CHECK(written_content.has_value());
@@ -536,9 +542,8 @@ TEST_CASE_METHOD(WriteFileFixture, "WriteFileAs", "[file_system]") {
         auto run_test = [&]<bool kSetEpochTime = false>(bool fd_less) {
             CHECK(FileSystemManager::WriteFileAs<kSetEpochTime>(
                 content, file_path_, ObjectType::File, fd_less));
-            CHECK(std::filesystem::exists(file_path_));
-            CHECK(std::filesystem::is_directory(file_path_.parent_path()));
-            CHECK(std::filesystem::is_regular_file(file_path_));
+            CHECK(FileSystemManager::IsDirectory(file_path_.parent_path()));
+            CHECK(FileSystemManager::IsFile(file_path_));
             CHECK(not FileSystemManager::IsExecutable(file_path_));
 
             auto const written_content =
@@ -568,9 +573,7 @@ TEST_CASE_METHOD(WriteFileFixture, "WriteFileAs", "[file_system]") {
         auto run_test = [&]<bool kSetEpochTime = false>(bool fd_less) {
             CHECK(FileSystemManager::WriteFileAs<kSetEpochTime>(
                 content, file_path_, ObjectType::Executable, fd_less));
-            CHECK(std::filesystem::exists(file_path_));
-            CHECK(std::filesystem::is_directory(file_path_.parent_path()));
-            CHECK(std::filesystem::is_regular_file(file_path_));
+            CHECK(FileSystemManager::IsDirectory(file_path_.parent_path()));
             CHECK(FileSystemManager::IsExecutable(file_path_));
 
             auto const written_content =
@@ -663,27 +666,27 @@ TEST_CASE("CreateFileHardlink", "[file_system]") {
         }
 
         CHECK(FileSystemManager::CreateFileHardlink(from, to));
-        CHECK(std::filesystem::exists(to));
+        CHECK(FileSystemManager::Exists(to));
 
         CHECK_FALSE(FileSystemManager::CreateFileHardlink(from, to));
-        CHECK(std::filesystem::exists(to));
+        CHECK(FileSystemManager::Exists(to));
 
         CHECK(FileSystemManager::RemoveFile(to));
-        CHECK(not std::filesystem::exists(to));
+        CHECK(not FileSystemManager::Exists(to));
     }
     SECTION("Non-existing file") {
         std::filesystem::path from{
             "test/buildtool/file_system/data/this_file_does_not_exist"};
 
         CHECK_FALSE(FileSystemManager::CreateFileHardlink(from, to));
-        CHECK_FALSE(std::filesystem::exists(to));
+        CHECK_FALSE(FileSystemManager::Exists(to));
     }
     SECTION("Existing but not file") {
         std::filesystem::path from{"./tmp-CreateFileHardlink/dir"};
         CHECK(FileSystemManager::CreateDirectory(from));
 
         CHECK_FALSE(FileSystemManager::CreateFileHardlink(from, to));
-        CHECK_FALSE(std::filesystem::exists(to));
+        CHECK_FALSE(FileSystemManager::Exists(to));
     }
 }
 
@@ -692,8 +695,7 @@ TEST_CASE("CopyDirectoryImpl", "[file_system]") {
     REQUIRE(FileSystemManager::CreateDirectory(to.parent_path()));
 
     CHECK(FileSystemManager::CreateDirectory("a/b/c/d"));
-    CHECK(std::filesystem::exists("a/b/c/d"));
-    CHECK(std::filesystem::is_directory("a/b/c/d"));
+    CHECK(FileSystemManager::IsDirectory("a/b/c/d"));
 
     CHECK(FileSystemManager::WriteFile("boo", "a/bb.txt"));
 
@@ -701,17 +703,13 @@ TEST_CASE("CopyDirectoryImpl", "[file_system]") {
     CHECK(FileSystemManager::CopyDirectoryImpl("a", to, true));
 
     // Result should be in tmp-dir now
-    CHECK(std::filesystem::exists(to));
-    CHECK(std::filesystem::is_directory(to));
+    CHECK(FileSystemManager::IsDirectory(to));
 
-    CHECK(std::filesystem::exists(to / "b"));
-    CHECK(std::filesystem::is_directory(to / "b"));
+    CHECK(FileSystemManager::IsDirectory(to / "b"));
 
-    CHECK(std::filesystem::exists(to / "b/c"));
-    CHECK(std::filesystem::is_directory(to / "b/c"));
+    CHECK(FileSystemManager::IsDirectory(to / "b/c"));
 
-    CHECK(std::filesystem::exists(to / "bb.txt"));
-    CHECK(std::filesystem::is_regular_file(to / "bb.txt"));
+    CHECK(FileSystemManager::IsFile(to / "bb.txt"));
 }
 
 TEST_CASE_METHOD(CopyFileFixture, "CreateFileHardlinkAs", "[file_system]") {
@@ -732,10 +730,11 @@ TEST_CASE_METHOD(CopyFileFixture, "CreateFileHardlinkAs", "[file_system]") {
             is_executable ? ObjectType::Executable : ObjectType::File));
 
         // file exists
-        CHECK(std::filesystem::exists(to_));
-        CHECK(std::filesystem::is_regular_file(to_));
         CHECK(is_executable == FileSystemManager::IsExecutable(to_));
 
+        // executables are special permission files
+        CHECK(FileSystemManager::IsFile(to_));
+
         // permissions should be 0555 or 0444
         CHECK((is_executable ? HasExecutablePermissions(to_)
                              : HasFilePermissions(to_)));
diff --git a/test/buildtool/graph_traverser/graph_traverser.test.hpp b/test/buildtool/graph_traverser/graph_traverser.test.hpp
index c5ffb840..84b348e4 100644
--- a/test/buildtool/graph_traverser/graph_traverser.test.hpp
+++ b/test/buildtool/graph_traverser/graph_traverser.test.hpp
@@ -107,7 +107,7 @@ class TestProject {
         clargs.artifacts = entry_points;
         auto const comp_graph = root_dir_ / "graph_description_compatible";
         if (Compatibility::IsCompatible() and
-            std::filesystem::exists(comp_graph)) {
+            FileSystemManager::Exists(comp_graph)) {
             clargs.graph_description = comp_graph;
         }
         else {