Merge pull request #13122 from Mic92/directory-iterator

Mic92 · web-flow · commit 143fb88ceba0 · 2025-05-01T14:22:21.000+02:00
Replace all instances of std::filesystem::directory_iterator with DirectoryIterator
diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc
@@ -244,14 +244,13 @@ StringSet NixRepl::completePrefix(const std::string & prefix)
         try {
             auto dir = std::string(cur, 0, slash);
             auto prefix2 = std::string(cur, slash + 1);
-            for (auto & entry : std::filesystem::directory_iterator{dir == "" ? "/" : dir}) {
+            for (auto & entry : DirectoryIterator{dir == "" ? "/" : dir}) {
                 checkInterrupt();
                 auto name = entry.path().filename().string();
                 if (name[0] != '.' && hasPrefix(name, prefix2))
                     completions.insert(prev + entry.path().string());
             }
         } catch (Error &) {
-        } catch (std::filesystem::filesystem_error &) {
         }
     } else if ((dot = cur.rfind('.')) == std::string::npos) {
         /* This is a variable name; look it up in the current scope. */
diff --git a/src/libmain/plugin.cc b/src/libmain/plugin.cc
@@ -6,6 +6,7 @@
 
 #include "nix/util/config-global.hh"
 #include "nix/util/signals.hh"
+#include "nix/util/file-system.hh"
 
 namespace nix {
 
@@ -77,13 +78,13 @@ void initPlugins()
     for (const auto & pluginFile : pluginSettings.pluginFiles.get()) {
         std::vector<std::filesystem::path> pluginFiles;
         try {
-            auto ents = std::filesystem::directory_iterator{pluginFile};
+            auto ents = DirectoryIterator{pluginFile};
             for (const auto & ent : ents) {
                 checkInterrupt();
                 pluginFiles.emplace_back(ent.path());
             }
-        } catch (std::filesystem::filesystem_error & e) {
-            if (e.code() != std::errc::not_a_directory)
+        } catch (SysError & e) {
+            if (e.errNo != ENOTDIR)
                 throw;
             pluginFiles.emplace_back(pluginFile);
         }
diff --git a/src/libstore/builtins/buildenv.cc b/src/libstore/builtins/buildenv.cc
@@ -18,12 +18,12 @@ struct State
 /* For each activated package, create symlinks */
 static void createLinks(State & state, const Path & srcDir, const Path & dstDir, int priority)
 {
-    std::filesystem::directory_iterator srcFiles;
+    DirectoryIterator srcFiles;
 
     try {
-        srcFiles = std::filesystem::directory_iterator{srcDir};
-    } catch (std::filesystem::filesystem_error & e) {
-        if (e.code() == std::errc::not_a_directory) {
+        srcFiles = DirectoryIterator{srcDir};
+    } catch (SysError & e) {
+        if (e.errNo == ENOTDIR) {
             warn("not including '%s' in the user environment because it's not a directory", srcDir);
             return;
         }
diff --git a/src/libstore/builtins/unpack-channel.cc b/src/libstore/builtins/unpack-channel.cc
@@ -29,13 +29,9 @@ void builtinUnpackChannel(
 
     size_t fileCount;
     std::string fileName;
-    try {
-        auto entries = fs::directory_iterator{out};
-        fileName = entries->path().string();
-        fileCount = std::distance(fs::begin(entries), fs::end(entries));
-    } catch (fs::filesystem_error &) {
-        throw SysError("failed to read directory %1%", out.string());
-    }
+    auto entries = DirectoryIterator{out};
+    fileName = entries->path().string();
+    fileCount = std::distance(entries.begin(), entries.end());
 
     if (fileCount != 1)
         throw Error("channel tarball '%s' contains more than one file", src);
diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc
@@ -164,7 +164,7 @@ void LocalStore::findTempRoots(Roots & tempRoots, bool censor)
 {
     /* Read the `temproots' directory for per-process temporary root
        files. */
-    for (auto & i : std::filesystem::directory_iterator{tempRootsDir}) {
+    for (auto & i : DirectoryIterator{tempRootsDir}) {
         checkInterrupt();
         auto name = i.path().filename().string();
         if (name[0] == '.') {
@@ -232,7 +232,7 @@ void LocalStore::findRoots(const Path & path, std::filesystem::file_type type, R
             type = std::filesystem::symlink_status(path).type();
 
         if (type == std::filesystem::file_type::directory) {
-            for (auto & i : std::filesystem::directory_iterator{path}) {
+            for (auto & i : DirectoryIterator{path}) {
                 checkInterrupt();
                 findRoots(i.path().string(), i.symlink_status().type(), roots);
             }
diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc
@@ -84,7 +84,7 @@ struct LocalBinaryCacheStore : virtual LocalBinaryCacheStoreConfig, virtual Bina
     {
         StorePathSet paths;
 
-        for (auto & entry : std::filesystem::directory_iterator{binaryCacheDir}) {
+        for (auto & entry : DirectoryIterator{binaryCacheDir}) {
             checkInterrupt();
             auto name = entry.path().filename().string();
             if (name.size() != 40 ||
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
@@ -1382,7 +1382,7 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair)
 
         printInfo("checking link hashes...");
 
-        for (auto & link : std::filesystem::directory_iterator{linksDir}) {
+        for (auto & link : DirectoryIterator{linksDir}) {
             checkInterrupt();
             auto name = link.path().filename();
             printMsg(lvlTalkative, "checking contents of '%s'", name);
@@ -1475,7 +1475,7 @@ LocalStore::VerificationResult LocalStore::verifyAllValidPaths(RepairFlag repair
        database and the filesystem) in the loop below, in order to catch
        invalid states.
      */
-    for (auto & i : std::filesystem::directory_iterator{realStoreDir.to_string()}) {
+    for (auto & i : DirectoryIterator{realStoreDir.to_string()}) {
         checkInterrupt();
         try {
             storePathsInStoreDir.insert({i.path().filename().string()});
diff --git a/src/libstore/posix-fs-canonicalise.cc b/src/libstore/posix-fs-canonicalise.cc
@@ -136,7 +136,7 @@ static void canonicalisePathMetaData_(
 #endif
 
     if (S_ISDIR(st.st_mode)) {
-        for (auto & i : std::filesystem::directory_iterator{path}) {
+        for (auto & i : DirectoryIterator{path}) {
             checkInterrupt();
             canonicalisePathMetaData_(
                 i.path().string(),
diff --git a/src/libstore/profiles.cc b/src/libstore/profiles.cc
@@ -38,7 +38,7 @@ std::pair<Generations, std::optional<GenerationNumber>> findGenerations(Path pro
     std::filesystem::path profileDir = dirOf(profile);
     auto profileName = std::string(baseNameOf(profile));
 
-    for (auto & i : std::filesystem::directory_iterator{profileDir}) {
+    for (auto & i : DirectoryIterator{profileDir}) {
         checkInterrupt();
         if (auto n = parseName(profileName, i.path().filename().string())) {
             auto path = i.path().string();
diff --git a/src/libutil-tests/file-system.cc b/src/libutil-tests/file-system.cc
@@ -297,4 +297,25 @@ TEST(chmodIfNeeded, nonexistent)
     ASSERT_THROW(chmodIfNeeded("/schnitzel/darmstadt/pommes", 0755), SysError);
 }
 
+/* ----------------------------------------------------------------------------
+ * DirectoryIterator
+ * --------------------------------------------------------------------------*/
+
+TEST(DirectoryIterator, works)
+{
+    auto tmpDir = nix::createTempDir();
+    nix::AutoDelete delTmpDir(tmpDir, true);
+
+    nix::writeFile(tmpDir + "/somefile", "");
+
+    for (auto path : DirectoryIterator(tmpDir)) {
+        ASSERT_EQ(path.path().string(), tmpDir + "/somefile");
+    }
+}
+
+TEST(DirectoryIterator, nonexistent)
+{
+    ASSERT_THROW(DirectoryIterator("/schnitzel/darmstadt/pommes"), SysError);
+}
+
 }
diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc
@@ -43,6 +43,34 @@ namespace fs {
   }
 }
 
+DirectoryIterator::DirectoryIterator(const std::filesystem::path& p) {
+    try {
+        // **Attempt to create the underlying directory_iterator**
+        it_ = std::filesystem::directory_iterator(p);
+    } catch (const std::filesystem::filesystem_error& e) {
+        // **Catch filesystem_error and throw SysError**
+        // Adapt the error message as needed for SysError
+        throw SysError("cannot read directory %s", p);
+    }
+}
+
+DirectoryIterator& DirectoryIterator::operator++() {
+    // **Attempt to increment the underlying iterator**
+    std::error_code ec;
+    it_.increment(ec);
+    if (ec) {
+        // Try to get path info if possible, might fail if iterator is bad
+        try {
+            if (it_ != std::filesystem::directory_iterator{}) {
+                throw SysError("cannot read directory past %s: %s", it_->path(), ec.message());
+            }
+        } catch (...) {
+            throw SysError("cannot read directory");
+        }
+    }
+    return *this;
+}
+
 bool isAbsolute(PathView path)
 {
     return fs::path { path }.is_absolute();
@@ -359,7 +387,7 @@ void recursiveSync(const Path & path)
     while (!dirsToEnumerate.empty()) {
         auto currentDir = dirsToEnumerate.back();
         dirsToEnumerate.pop_back();
-        for (auto & entry : std::filesystem::directory_iterator(currentDir)) {
+        for (auto & entry : DirectoryIterator(currentDir)) {
             auto st = entry.symlink_status();
             if (fs::is_directory(st)) {
                 dirsToEnumerate.emplace_back(entry.path());
@@ -663,7 +691,7 @@ void copyFile(const fs::path & from, const fs::path & to, bool andDelete)
         fs::copy(from, to, fs::copy_options::copy_symlinks | fs::copy_options::overwrite_existing);
     } else if (fs::is_directory(fromStatus)) {
         fs::create_directory(to);
-        for (auto & entry : fs::directory_iterator(from)) {
+        for (auto & entry : DirectoryIterator(from)) {
             copyFile(entry, to / entry.path().filename(), andDelete);
         }
     } else {
diff --git a/src/libutil/include/nix/util/file-system.hh b/src/libutil/include/nix/util/file-system.hh
@@ -376,4 +376,64 @@ extern PathFilter defaultPathFilter;
  */
 bool chmodIfNeeded(const std::filesystem::path & path, mode_t mode, mode_t mask = S_IRWXU | S_IRWXG | S_IRWXO);
 
+/**
+  * @brief A directory iterator that can be used to iterate over the
+  * contents of a directory. It is similar to std::filesystem::directory_iterator
+  * but throws NixError on failure instead of std::filesystem::filesystem_error.
+  */
+class DirectoryIterator {
+public:
+    // --- Iterator Traits ---
+    using iterator_category = std::input_iterator_tag;
+    using value_type        = std::filesystem::directory_entry;
+    using difference_type   = std::ptrdiff_t;
+    using pointer           = const std::filesystem::directory_entry*;
+    using reference         = const std::filesystem::directory_entry&;
+
+    // Default constructor (represents end iterator)
+    DirectoryIterator() noexcept = default;
+
+    // Constructor taking a path
+    explicit DirectoryIterator(const std::filesystem::path& p);
+
+    reference operator*() const {
+        // Accessing the value itself doesn't typically throw filesystem_error
+        // after successful construction/increment, but underlying operations might.
+        // If directory_entry methods called via -> could throw, add try-catch there.
+        return *it_;
+    }
+
+    pointer operator->() const {
+        return &(*it_);
+    }
+
+
+    DirectoryIterator& operator++();
+
+    // Postfix increment operator
+    DirectoryIterator operator++(int) {
+        DirectoryIterator temp = *this;
+        ++(*this); // Uses the prefix increment's try-catch logic
+        return temp;
+    }
+
+    // Equality comparison
+    friend bool operator==(const DirectoryIterator& a, const DirectoryIterator& b) noexcept {
+        return a.it_ == b.it_;
+    }
+
+    // Inequality comparison
+    friend bool operator!=(const DirectoryIterator& a, const DirectoryIterator& b) noexcept {
+        return !(a == b);
+    }
+
+    // Allow direct use in range-based for loops if iterating over an instance
+    DirectoryIterator begin() const { return *this; }
+    DirectoryIterator end() const { return DirectoryIterator{}; }
+
+
+private:
+    std::filesystem::directory_iterator it_;
+};
+
 }
diff --git a/src/libutil/linux/cgroup.cc b/src/libutil/linux/cgroup.cc
@@ -65,7 +65,7 @@ static CgroupStats destroyCgroup(const std::filesystem::path & cgroup, bool retu
 
     /* Otherwise, manually kill every process in the subcgroups and
        this cgroup. */
-    for (auto & entry : std::filesystem::directory_iterator{cgroup}) {
+    for (auto & entry : DirectoryIterator{cgroup}) {
         checkInterrupt();
         if (entry.symlink_status().type() != std::filesystem::file_type::directory) continue;
         destroyCgroup(cgroup / entry.path().filename(), false);
diff --git a/src/libutil/posix-source-accessor.cc b/src/libutil/posix-source-accessor.cc
@@ -138,42 +138,38 @@ SourceAccessor::DirEntries PosixSourceAccessor::readDirectory(const CanonPath &
 {
     assertNoSymlinks(path);
     DirEntries res;
-    try {
-        for (auto & entry : std::filesystem::directory_iterator{makeAbsPath(path)}) {
-            checkInterrupt();
-            auto type = [&]() -> std::optional<Type> {
-                std::filesystem::file_type nativeType;
-                try {
-                    nativeType = entry.symlink_status().type();
-                } catch (std::filesystem::filesystem_error & e) {
-                    // We cannot always stat the child. (Ideally there is no
-                    // stat because the native directory entry has the type
-                    // already, but this isn't always the case.)
-                    if (e.code() == std::errc::permission_denied || e.code() == std::errc::operation_not_permitted)
-                        return std::nullopt;
-                    else throw;
-                }
-
-                // cannot exhaustively enumerate because implementation-specific
-                // additional file types are allowed.
+    for (auto & entry : DirectoryIterator{makeAbsPath(path)}) {
+        checkInterrupt();
+        auto type = [&]() -> std::optional<Type> {
+            std::filesystem::file_type nativeType;
+            try {
+                nativeType = entry.symlink_status().type();
+            } catch (std::filesystem::filesystem_error & e) {
+                // We cannot always stat the child. (Ideally there is no
+                // stat because the native directory entry has the type
+                // already, but this isn't always the case.)
+                if (e.code() == std::errc::permission_denied || e.code() == std::errc::operation_not_permitted)
+                    return std::nullopt;
+                else throw;
+            }
+
+            // cannot exhaustively enumerate because implementation-specific
+            // additional file types are allowed.
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wswitch-enum"
-            switch (nativeType) {
-            case std::filesystem::file_type::regular: return Type::tRegular; break;
-            case std::filesystem::file_type::symlink: return Type::tSymlink; break;
-            case std::filesystem::file_type::directory: return Type::tDirectory; break;
-            case std::filesystem::file_type::character: return Type::tChar; break;
-            case std::filesystem::file_type::block: return Type::tBlock; break;
-            case std::filesystem::file_type::fifo: return Type::tFifo; break;
-            case std::filesystem::file_type::socket: return Type::tSocket; break;
-            default: return tUnknown;
-            }
-#pragma GCC diagnostic pop
-            }();
-            res.emplace(entry.path().filename().string(), type);
+        switch (nativeType) {
+        case std::filesystem::file_type::regular: return Type::tRegular; break;
+        case std::filesystem::file_type::symlink: return Type::tSymlink; break;
+        case std::filesystem::file_type::directory: return Type::tDirectory; break;
+        case std::filesystem::file_type::character: return Type::tChar; break;
+        case std::filesystem::file_type::block: return Type::tBlock; break;
+        case std::filesystem::file_type::fifo: return Type::tFifo; break;
+        case std::filesystem::file_type::socket: return Type::tSocket; break;
+        default: return tUnknown;
         }
-    } catch (std::filesystem::filesystem_error & e) {
-        throw SysError("reading directory %1%", showPath(path));
+#pragma GCC diagnostic pop
+        }();
+        res.emplace(entry.path().filename().string(), type);
     }
     return res;
 }
diff --git a/src/libutil/unix/file-descriptor.cc b/src/libutil/unix/file-descriptor.cc
@@ -191,7 +191,7 @@ void unix::closeExtraFDs()
 
 #ifdef __linux__
     try {
-        for (auto & s : std::filesystem::directory_iterator{"/proc/self/fd"}) {
+        for (auto & s : DirectoryIterator{"/proc/self/fd"}) {
             checkInterrupt();
             auto fd = std::stoi(s.path().filename());
             if (fd > MAX_KEPT_FD) {
@@ -201,7 +201,6 @@ void unix::closeExtraFDs()
         }
         return;
     } catch (SysError &) {
-    } catch (std::filesystem::filesystem_error &) {
     }
 #endif
 
diff --git a/src/nix-collect-garbage/nix-collect-garbage.cc b/src/nix-collect-garbage/nix-collect-garbage.cc
@@ -30,7 +30,7 @@ void removeOldGenerations(fs::path dir)
 
     bool canWrite = access(dir.string().c_str(), W_OK) == 0;
 
-    for (auto & i : fs::directory_iterator{dir}) {
+    for (auto & i : DirectoryIterator{dir}) {
         checkInterrupt();
 
         auto path = i.path().string();
diff --git a/src/nix/prefetch.cc b/src/nix/prefetch.cc
diff --git a/src/nix/run.cc b/src/nix/run.cc

Original file line number	Diff line number	Diff line change
`@@ -84,7 +84,7 @@ struct LocalBinaryCacheStore : virtual LocalBinaryCacheStoreConfig, virtual Bina`
`84`	`84`	`{`
`85`	`85`	`StorePathSet paths;`
`86`	`86`
`87`		`- for (auto & entry : std::filesystem::directory_iterator{binaryCacheDir}) {`
	`87`	`+ for (auto & entry : DirectoryIterator{binaryCacheDir}) {`
`88`	`88`	`checkInterrupt();`
`89`	`89`	`auto name = entry.path().filename().string();`
`90`	`90`	`if (name.size() != 40 \|\|`