From 6b297e5895e62d91963be49e5d301604d0b8fd09 Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Wed, 2 Aug 2023 14:38:22 -0400
Subject: [PATCH] Make `verifyAllValidPaths` more functional

return map rather than mutate one passed in by reference
---
 src/libstore/local-overlay-store.cc |  8 +++++---
 src/libstore/local-overlay-store.hh |  4 ++--
 src/libstore/local-store.cc         | 12 ++++++------
 src/libstore/local-store.hh         |  7 ++++++-
 4 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/src/libstore/local-overlay-store.cc b/src/libstore/local-overlay-store.cc
index 4bfad6d32..4ba98a02d 100644
--- a/src/libstore/local-overlay-store.cc
+++ b/src/libstore/local-overlay-store.cc
@@ -243,19 +243,21 @@ void LocalOverlayStore::optimiseStore()
 }
 
 
-bool LocalOverlayStore::verifyAllValidPaths(RepairFlag repair, StorePathSet & validPaths)
+std::pair<bool, StorePathSet> LocalOverlayStore::verifyAllValidPaths(RepairFlag repair)
 {
     StorePathSet done;
-    bool errors = false;
 
     auto existsInStoreDir = [&](const StorePath & storePath) {
         return pathExists(realStoreDir.get() + "/" + storePath.to_string());
     };
 
+    bool errors = false;
+    StorePathSet validPaths;
+
     for (auto & i : queryAllValidPaths())
         verifyPath(i, existsInStoreDir, done, validPaths, repair, errors);
 
-    return errors;
+    return { errors, validPaths };
 }
 
 
diff --git a/src/libstore/local-overlay-store.hh b/src/libstore/local-overlay-store.hh
index c0fa0ffa7..40ee75bca 100644
--- a/src/libstore/local-overlay-store.hh
+++ b/src/libstore/local-overlay-store.hh
@@ -135,11 +135,11 @@ private:
      *
      * Note that this includes store objects that reside in either overlayfs layer;
      * just enumerating the contents of the upper layer would skip them.
-     * 
+     *
      * We don't verify the contents of both layers on the assumption that the lower layer is far bigger,
      * and also the observation that anything not in the upper db the overlayfs doesn't yet care about.
      */
-    bool verifyAllValidPaths(RepairFlag repair, StorePathSet & validPaths) override;
+    std::pair<bool, StorePathSet> verifyAllValidPaths(RepairFlag repair) override;
 
     /**
      * For lower-store paths, we used the lower store location. This avoids the
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index 2e662ad66..42347d80a 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -1503,9 +1503,7 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair)
     auto fdGCLock = openGCLock();
     FdLock gcLock(fdGCLock.get(), ltRead, true, "waiting for the big garbage collector lock...");
 
-    StorePathSet validPaths;
-
-    bool errors = verifyAllValidPaths(repair, validPaths);
+    auto [errors, validPaths] = verifyAllValidPaths(repair);
 
     /* Optionally, check the content hashes (slow). */
     if (checkContents) {
@@ -1591,7 +1589,7 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair)
 }
 
 
-bool LocalStore::verifyAllValidPaths(RepairFlag repair, StorePathSet & validPaths)
+std::pair<bool, StorePathSet> LocalStore::verifyAllValidPaths(RepairFlag repair)
 {
     StorePathSet storePathsInStoreDir;
     /* Why aren't we using `queryAllValidPaths`? Because that would
@@ -1613,16 +1611,18 @@ bool LocalStore::verifyAllValidPaths(RepairFlag repair, StorePathSet & validPath
     printInfo("checking path existence...");
 
     StorePathSet done;
-    bool errors = false;
 
     auto existsInStoreDir = [&](const StorePath & storePath) {
         return storePathsInStoreDir.count(storePath);
     };
 
+    bool errors = false;
+    StorePathSet validPaths;
+
     for (auto & i : queryAllValidPaths())
         verifyPath(i, existsInStoreDir, done, validPaths, repair, errors);
 
-    return errors;
+    return { errors, validPaths };
 }
 
 
diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh
index 322d27932..88549eea5 100644
--- a/src/libstore/local-store.hh
+++ b/src/libstore/local-store.hh
@@ -265,7 +265,12 @@ public:
 
     bool verifyStore(bool checkContents, RepairFlag repair) override;
 
-    virtual bool verifyAllValidPaths(RepairFlag repair, StorePathSet & validPaths);
+    /**
+     * @return A pair of whether any errors were encountered, and a set of
+     * (so-far) valid paths. The store objects pointed to by those paths are
+     * suitable for further validation checking.
+     */
+    virtual std::pair<bool, StorePathSet> verifyAllValidPaths(RepairFlag repair);
 
     /**
      * Register the validity of a path, i.e., that `path` exists, that