FileSystemManager: Always remove directories recursively

9 files changed, 23 insertions, 33 deletions

diff --git a/src/buildtool/execution_api/local/local_action.cpp b/src/buildtool/execution_api/local/local_action.cpp
index 9884a5ec..7ea0ce34 100644
--- a/src/buildtool/execution_api/local/local_action.cpp
+++ b/src/buildtool/execution_api/local/local_action.cpp
@@ -55,7 +55,7 @@ class BuildCleanupAnchor {
     auto operator=(BuildCleanupAnchor const&) -> BuildCleanupAnchor& = delete;
     auto operator=(BuildCleanupAnchor&&) -> BuildCleanupAnchor& = delete;
     ~BuildCleanupAnchor() {
-        if (not FileSystemManager::RemoveDirectory(build_path_, true)) {
+        if (not FileSystemManager::RemoveDirectory(build_path_)) {
             Logger::Log(LogLevel::Error,
                         "Could not cleanup build directory {}",
                         build_path_.string());
@@ -347,7 +347,7 @@ auto LocalAction::StageInputs(
 auto LocalAction::CreateDirectoryStructure(
     std::filesystem::path const& exec_path) const noexcept -> bool {
     // clean execution directory
-    if (not FileSystemManager::RemoveDirectory(exec_path, true)) {
+    if (not FileSystemManager::RemoveDirectory(exec_path)) {
         logger_.Emit(LogLevel::Error, "failed to clean exec_path");
         return false;
     }
diff --git a/src/buildtool/file_system/file_system_manager.hpp b/src/buildtool/file_system/file_system_manager.hpp
index fe316451..a6d15c84 100644
--- a/src/buildtool/file_system/file_system_manager.hpp
+++ b/src/buildtool/file_system/file_system_manager.hpp
@@ -44,6 +44,7 @@
 #include <string>
 #include <string_view>
 #include <system_error>
+#include <tuple>
 #include <unordered_set>
 #include <utility>
 #include <variant>
@@ -510,9 +511,9 @@ class FileSystemManager {
         }
     }
 
-    [[nodiscard]] static auto RemoveDirectory(std::filesystem::path const& dir,
-                                              bool recursively = false) noexcept
-        -> bool {
+    /// \brief Remove directory recursively.
+    [[nodiscard]] static auto RemoveDirectory(
+        std::filesystem::path const& dir) noexcept -> bool {
         try {
             auto status = std::filesystem::symlink_status(dir);
             if (not std::filesystem::exists(status)) {
@@ -521,11 +522,9 @@ class FileSystemManager {
             if (not std::filesystem::is_directory(status)) {
                 return false;
             }
-            if (recursively) {
-                return (std::filesystem::remove_all(dir) !=
-                        static_cast<uintmax_t>(-1));
-            }
-            return std::filesystem::remove(dir);
+            // If it doesn't throw, it succeeds:
+            std::ignore = std::filesystem::remove_all(dir);
+            return true;
         } catch (std::exception const& e) {
             Logger::Log(LogLevel::Error,
                         "removing directory {}:\n{}",
diff --git a/src/buildtool/storage/compactifier.cpp b/src/buildtool/storage/compactifier.cpp
index 66fa6035..ab1f557e 100644
--- a/src/buildtool/storage/compactifier.cpp
+++ b/src/buildtool/storage/compactifier.cpp
@@ -163,8 +163,7 @@ template <ObjectType kType>
     std::string const d_name = directory.filename();
     if (d_name.size() != FileStorageData::kDirectoryNameLength or
         not FromHexString(d_name)) {
-        static constexpr bool kRecursively = true;
-        if (FileSystemManager::RemoveDirectory(directory, kRecursively)) {
+        if (FileSystemManager::RemoveDirectory(directory)) {
             return true;
         }
 
diff --git a/src/buildtool/storage/garbage_collector.cpp b/src/buildtool/storage/garbage_collector.cpp
index f1254eed..1ac1bf93 100644
--- a/src/buildtool/storage/garbage_collector.cpp
+++ b/src/buildtool/storage/garbage_collector.cpp
@@ -39,8 +39,7 @@ auto RemoveDirs(const std::vector<std::filesystem::path>& directories) -> bool {
     bool success = true;
     for (auto const& d : directories) {
         if (FileSystemManager::IsDirectory(d)) {
-            if (not FileSystemManager::RemoveDirectory(d,
-                                                       /*recursively=*/true)) {
+            if (not FileSystemManager::RemoveDirectory(d)) {
                 Logger::Log(LogLevel::Warning,
                             "Failed to remove directory {}",
                             d.string());
diff --git a/src/buildtool/storage/repository_garbage_collector.cpp b/src/buildtool/storage/repository_garbage_collector.cpp
index be3d7857..95b90f9d 100644
--- a/src/buildtool/storage/repository_garbage_collector.cpp
+++ b/src/buildtool/storage/repository_garbage_collector.cpp
@@ -60,8 +60,7 @@ auto RepositoryGarbageCollector::TriggerGarbageCollection(
             return false;
         }
         if (FileSystemManager::IsDirectory(remove_me)) {
-            if (not FileSystemManager::RemoveDirectory(remove_me,
-                                                       /*recursively=*/true)) {
+            if (not FileSystemManager::RemoveDirectory(remove_me)) {
                 Logger::Log(LogLevel::Error,
                             "Failed to remove directory {}",
                             remove_me.string());
@@ -136,8 +135,7 @@ auto RepositoryGarbageCollector::TriggerGarbageCollection(
             return false;
         }
         if (FileSystemManager::IsDirectory(remove_me)) {
-            if (not FileSystemManager::RemoveDirectory(remove_me,
-                                                       /*recursively=*/true)) {
+            if (not FileSystemManager::RemoveDirectory(remove_me)) {
                 Logger::Log(LogLevel::Error,
                             "Failed to remove directory {}",
                             remove_me.string());
diff --git a/src/utils/cpp/tmp_dir.cpp b/src/utils/cpp/tmp_dir.cpp
index 8d6e0724..9a38aca2 100644
--- a/src/utils/cpp/tmp_dir.cpp
+++ b/src/utils/cpp/tmp_dir.cpp
@@ -93,5 +93,5 @@ auto TmpDir::CreateImpl(TmpDir::Ptr parent,
 
 TmpDir::~TmpDir() noexcept {
     // try to remove the tmp dir and all its content
-    std::ignore = FileSystemManager::RemoveDirectory(tmp_dir_, true);
+    std::ignore = FileSystemManager::RemoveDirectory(tmp_dir_);
 }
diff --git a/test/buildtool/execution_engine/executor/executor_api.test.hpp b/test/buildtool/execution_engine/executor/executor_api.test.hpp
index 2043da88..ff4c4999 100644
--- a/test/buildtool/execution_engine/executor/executor_api.test.hpp
+++ b/test/buildtool/execution_engine/executor/executor_api.test.hpp
@@ -479,7 +479,7 @@ static inline void TestUploadAndDownloadTrees(
         CHECK(FileSystemManager::IsNonUpwardsSymlink(tmpdir / "b"));
         CHECK(*FileSystemManager::ReadFile(tmpdir / "a") == "foo");
         CHECK(*FileSystemManager::ReadSymlink(tmpdir / "b") == "bar");
-        REQUIRE(FileSystemManager::RemoveDirectory(tmpdir, true));
+        REQUIRE(FileSystemManager::RemoveDirectory(tmpdir));
     }
 
     SECTION("Subdir in tree path") {
@@ -496,7 +496,7 @@ static inline void TestUploadAndDownloadTrees(
         CHECK(FileSystemManager::IsNonUpwardsSymlink(tmpdir / "b" / "a"));
         CHECK(*FileSystemManager::ReadFile(tmpdir / "a") == "foo");
         CHECK(*FileSystemManager::ReadSymlink(tmpdir / "b" / "a") == "bar");
-        REQUIRE(FileSystemManager::RemoveDirectory(tmpdir, true));
+        REQUIRE(FileSystemManager::RemoveDirectory(tmpdir));
     }
 
     SECTION("Nested trees") {
@@ -517,7 +517,7 @@ static inline void TestUploadAndDownloadTrees(
         CHECK(FileSystemManager::IsNonUpwardsSymlink(tmpdir / "b" / "a"));
         CHECK(*FileSystemManager::ReadFile(tmpdir / "a") == "foo");
         CHECK(*FileSystemManager::ReadSymlink(tmpdir / "b" / "a") == "bar");
-        REQUIRE(FileSystemManager::RemoveDirectory(tmpdir, true));
+        REQUIRE(FileSystemManager::RemoveDirectory(tmpdir));
     }
 
     SECTION("Dot-path tree as action input") {
@@ -550,7 +550,7 @@ static inline void TestUploadAndDownloadTrees(
         CHECK(FileSystemManager::IsNonUpwardsSymlink(tmpdir / "b" / "a"));
         CHECK(*FileSystemManager::ReadFile(tmpdir / "a") == "foo");
         CHECK(*FileSystemManager::ReadSymlink(tmpdir / "b" / "a") == "bar");
-        REQUIRE(FileSystemManager::RemoveDirectory(tmpdir, true));
+        REQUIRE(FileSystemManager::RemoveDirectory(tmpdir));
     }
 
     SECTION("Dot-path non-tree as action input") {
diff --git a/test/buildtool/graph_traverser/graph_traverser_remote.test.cpp b/test/buildtool/graph_traverser/graph_traverser_remote.test.cpp
index f2b2d852..a611caab 100644
--- a/test/buildtool/graph_traverser/graph_traverser_remote.test.cpp
+++ b/test/buildtool/graph_traverser/graph_traverser_remote.test.cpp
@@ -35,7 +35,7 @@
 [[nodiscard]] static auto CreateStorageConfig(
     RemoteExecutionConfig const& remote_config) -> StorageConfig {
     auto cache_dir = FileSystemManager::GetCurrentDirectory() / "cache";
-    if (not FileSystemManager::RemoveDirectory(cache_dir, true) or
+    if (not FileSystemManager::RemoveDirectory(cache_dir) or
         not FileSystemManager::CreateDirectoryExclusive(cache_dir)) {
         Logger::Log(LogLevel::Error,
                     "failed to create a test-local cache dir {}",
diff --git a/test/utils/archive/archive_usage.test.cpp b/test/utils/archive/archive_usage.test.cpp
index 030cd223..ddf71353 100644
--- a/test/utils/archive/archive_usage.test.cpp
+++ b/test/utils/archive/archive_usage.test.cpp
@@ -339,8 +339,7 @@ TEST_CASE("Read-write archives", "[archive_read_write]") {
     auto const& scenario = kTestScenarios[test_index];
 
     // perform the test
-    REQUIRE(FileSystemManager::RemoveDirectory(scenario.test_dir,
-                                               /*recursively=*/true));
+    REQUIRE(FileSystemManager::RemoveDirectory(scenario.test_dir));
     REQUIRE(FileSystemManager::CreateDirectory(scenario.test_dir));
     auto anchor = FileSystemManager::ChangeDirectory(scenario.test_dir);
 
@@ -395,8 +394,7 @@ TEST_CASE("ArchiveOps", "[archive_ops]") {
     std::optional<std::string> res{std::nullopt};
 
     SECTION(std::string("Write ") + scenario.test_name) {
-        REQUIRE(FileSystemManager::RemoveDirectory(scenario.test_dir,
-                                                   /*recursively=*/true));
+        REQUIRE(FileSystemManager::RemoveDirectory(scenario.test_dir));
         REQUIRE(FileSystemManager::CreateDirectory(scenario.test_dir));
 
         create_files(scenario.test_dir);
@@ -408,8 +406,7 @@ TEST_CASE("ArchiveOps", "[archive_ops]") {
         }
 
         SECTION(std::string("Extract ") + scenario.test_name + " to disk") {
-            REQUIRE(FileSystemManager::RemoveDirectory(scenario.test_dir,
-                                                       /*recursively=*/true));
+            REQUIRE(FileSystemManager::RemoveDirectory(scenario.test_dir));
             REQUIRE(FileSystemManager::CreateDirectory(scenario.test_dir));
             res = ArchiveOps::ExtractArchive(
                 scenario.type, scenario.filename, ".");
@@ -430,9 +427,7 @@ TEST_CASE("ArchiveOps", "[archive_ops]") {
                 path = std::string{env_path} + ":" + path;
             }
             SECTION("Extract via system tools") {
-                REQUIRE(
-                    FileSystemManager::RemoveDirectory(scenario.test_dir,
-                                                       /*recursively=*/true));
+                REQUIRE(FileSystemManager::RemoveDirectory(scenario.test_dir));
                 REQUIRE(FileSystemManager::CreateDirectory(scenario.test_dir));
                 REQUIRE(system(("export PATH=" + path + " && " + scenario.cmd +
                                 " " + scenario.filename)