From d084c1cb410679ba2f80998e2263c60108aa0115 Mon Sep 17 00:00:00 2001
From: Eelco Dolstra <edolstra@gmail.com>
Date: Wed, 10 Apr 2024 12:46:21 +0200
Subject: [PATCH 1/4] Remove the "locked" flag from the fetcher cache

This also reworks the Mercurial fetcher (which was still using the
old cache interface) to have two distinct cache mappings:

* A ref-to-rev mapping, which is store-independent.
* A rev-to-store-path mapping.
---
 src/libfetchers/cache.cc           |  9 ++--
 src/libfetchers/cache.hh           |  3 +-
 src/libfetchers/fetch-to-store.cc  |  3 +-
 src/libfetchers/tarball.cc         |  3 +-
 src/libfetchers/unix/mercurial.cc  | 84 ++++++++++++++----------------
 tests/functional/fetchMercurial.sh |  1 +
 6 files changed, 45 insertions(+), 58 deletions(-)

diff --git a/src/libfetchers/cache.cc b/src/libfetchers/cache.cc
index e071b4717..83962856c 100644
--- a/src/libfetchers/cache.cc
+++ b/src/libfetchers/cache.cc
@@ -14,7 +14,7 @@ create table if not exists Cache (
     input     text not null,
     info      text not null,
     path      text not null,
-    immutable integer not null,
+    immutable integer not null, /* obsolete */
     timestamp integer not null,
     primary key (input)
 );
@@ -45,7 +45,7 @@ struct CacheImpl : Cache
         state->db.exec(schema);
 
         state->add.create(state->db,
-            "insert or replace into Cache(input, info, path, immutable, timestamp) values (?, ?, ?, ?, ?)");
+            "insert or replace into Cache(input, info, path, immutable, timestamp) values (?, ?, ?, false, ?)");
 
         state->lookup.create(state->db,
             "select info, path, immutable, timestamp from Cache where input = ?");
@@ -59,7 +59,6 @@ struct CacheImpl : Cache
             (attrsToJSON(inAttrs).dump())
             (attrsToJSON(infoAttrs).dump())
             ("") // no path
-            (false)
             (time(0)).exec();
     }
 
@@ -109,14 +108,12 @@ struct CacheImpl : Cache
         Store & store,
         const Attrs & inAttrs,
         const Attrs & infoAttrs,
-        const StorePath & storePath,
-        bool locked) override
+        const StorePath & storePath) override
     {
         _state.lock()->add.use()
             (attrsToJSON(inAttrs).dump())
             (attrsToJSON(infoAttrs).dump())
             (store.printStorePath(storePath))
-            (locked)
             (time(0)).exec();
     }
 
diff --git a/src/libfetchers/cache.hh b/src/libfetchers/cache.hh
index 791d77025..5e05d7af8 100644
--- a/src/libfetchers/cache.hh
+++ b/src/libfetchers/cache.hh
@@ -53,8 +53,7 @@ struct Cache
         Store & store,
         const Attrs & inAttrs,
         const Attrs & infoAttrs,
-        const StorePath & storePath,
-        bool locked) = 0;
+        const StorePath & storePath) = 0;
 
     virtual std::optional<std::pair<Attrs, StorePath>> lookup(
         Store & store,
diff --git a/src/libfetchers/fetch-to-store.cc b/src/libfetchers/fetch-to-store.cc
index 398286065..4156302c4 100644
--- a/src/libfetchers/fetch-to-store.cc
+++ b/src/libfetchers/fetch-to-store.cc
@@ -47,10 +47,9 @@ StorePath fetchToStore(
             name, *path.accessor, path.path, method, HashAlgorithm::SHA256, {}, filter2, repair);
 
     if (cacheKey && mode == FetchMode::Copy)
-        fetchers::getCache()->add(store, *cacheKey, {}, storePath, true);
+        fetchers::getCache()->add(store, *cacheKey, {}, storePath);
 
     return storePath;
 }
 
-
 }
diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc
index a1f934c35..fd59c1132 100644
--- a/src/libfetchers/tarball.cc
+++ b/src/libfetchers/tarball.cc
@@ -99,8 +99,7 @@ DownloadFileResult downloadFile(
             *store,
             inAttrs,
             infoAttrs,
-            *storePath,
-            false);
+            *storePath);
     }
 
     return {
diff --git a/src/libfetchers/unix/mercurial.cc b/src/libfetchers/unix/mercurial.cc
index a2702338f..783e338bf 100644
--- a/src/libfetchers/unix/mercurial.cc
+++ b/src/libfetchers/unix/mercurial.cc
@@ -224,22 +224,17 @@ struct MercurialInputScheme : InputScheme
 
         if (!input.getRef()) input.attrs.insert_or_assign("ref", "default");
 
-        auto checkHashAlgorithm = [&](const std::optional<Hash> & hash)
+        auto revInfoCacheKey = [&](const Hash & rev)
         {
-            if (hash.has_value() && hash->algo != HashAlgorithm::SHA1)
-                throw Error("Hash '%s' is not supported by Mercurial. Only sha1 is supported.", hash->to_string(HashFormat::Base16, true));
-        };
+            if (rev.algo != HashAlgorithm::SHA1)
+                throw Error("Hash '%s' is not supported by Mercurial. Only sha1 is supported.", rev.to_string(HashFormat::Base16, true));
 
-
-        auto getLockedAttrs = [&]()
-        {
-            checkHashAlgorithm(input.getRev());
-
-            return Attrs({
-                {"type", "hg"},
+            return Attrs{
+                {"_what", "hgRev"},
+                {"store", store->storeDir},
                 {"name", name},
-                {"rev", input.getRev()->gitRev()},
-            });
+                {"rev", input.getRev()->gitRev()}
+            };
         };
 
         auto makeResult = [&](const Attrs & infoAttrs, const StorePath & storePath) -> StorePath
@@ -250,26 +245,22 @@ struct MercurialInputScheme : InputScheme
             return storePath;
         };
 
-        if (input.getRev()) {
-            if (auto res = getCache()->lookup(*store, getLockedAttrs()))
-                return makeResult(res->first, std::move(res->second));
+        /* Check the cache for the most recent rev for this URL/ref. */
+        Attrs refToRevCacheKey{
+            {"_what", "hgRefToRev"},
+            {"url", actualUrl},
+            {"ref", *input.getRef()}
+        };
+
+        if (!input.getRev()) {
+            if (auto res = getCache()->lookupWithTTL(refToRevCacheKey))
+                input.attrs.insert_or_assign("rev", getRevAttr(*res, "rev").gitRev());
         }
 
-        auto revOrRef = input.getRev() ? input.getRev()->gitRev() : *input.getRef();
-
-        Attrs unlockedAttrs({
-            {"type", "hg"},
-            {"name", name},
-            {"url", actualUrl},
-            {"ref", *input.getRef()},
-        });
-
-        if (auto res = getCache()->lookup(*store, unlockedAttrs)) {
-            auto rev2 = Hash::parseAny(getStrAttr(res->first, "rev"), HashAlgorithm::SHA1);
-            if (!input.getRev() || input.getRev() == rev2) {
-                input.attrs.insert_or_assign("rev", rev2.gitRev());
-                return makeResult(res->first, std::move(res->second));
-            }
+        /* If we have a rev, check if we have a cached store path. */
+        if (auto rev = input.getRev()) {
+            if (auto res = getCache()->lookupExpired(*store, revInfoCacheKey(*rev)))
+                return makeResult(res->infoAttrs, res->storePath);
         }
 
         Path cacheDir = fmt("%s/nix/hg/%s", getCacheDir(), hashString(HashAlgorithm::SHA256, actualUrl).to_string(HashFormat::Nix32, false));
@@ -302,21 +293,29 @@ struct MercurialInputScheme : InputScheme
             }
         }
 
+        /* Fetch the remote rev or ref. */
         auto tokens = tokenizeString<std::vector<std::string>>(
-            runHg({ "log", "-R", cacheDir, "-r", revOrRef, "--template", "{node} {rev} {branch}" }));
+            runHg({
+                "log", "-R", cacheDir,
+                "-r", input.getRev() ? input.getRev()->gitRev() : *input.getRef(),
+                "--template", "{node} {rev} {branch}"
+            }));
         assert(tokens.size() == 3);
 
-        input.attrs.insert_or_assign("rev", Hash::parseAny(tokens[0], HashAlgorithm::SHA1).gitRev());
+        auto rev = Hash::parseAny(tokens[0], HashAlgorithm::SHA1);
+        input.attrs.insert_or_assign("rev", rev.gitRev());
         auto revCount = std::stoull(tokens[1]);
         input.attrs.insert_or_assign("ref", tokens[2]);
 
-        if (auto res = getCache()->lookup(*store, getLockedAttrs()))
-            return makeResult(res->first, std::move(res->second));
+        /* Now that we have the rev, check the cache again for a
+           cached store path. */
+        if (auto res = getCache()->lookupExpired(*store, revInfoCacheKey(rev)))
+            return makeResult(res->infoAttrs, res->storePath);
 
         Path tmpDir = createTempDir();
         AutoDelete delTmpDir(tmpDir, true);
 
-        runHg({ "archive", "-R", cacheDir, "-r", input.getRev()->gitRev(), tmpDir });
+        runHg({ "archive", "-R", cacheDir, "-r", rev.gitRev(), tmpDir });
 
         deletePath(tmpDir + "/.hg_archival.txt");
 
@@ -324,24 +323,17 @@ struct MercurialInputScheme : InputScheme
         auto storePath = store->addToStore(name, accessor, CanonPath { tmpDir });
 
         Attrs infoAttrs({
-            {"rev", input.getRev()->gitRev()},
             {"revCount", (uint64_t) revCount},
         });
 
         if (!origRev)
-            getCache()->add(
-                *store,
-                unlockedAttrs,
-                infoAttrs,
-                storePath,
-                false);
+            getCache()->upsert(refToRevCacheKey, {{"rev", rev.gitRev()}});
 
         getCache()->add(
             *store,
-            getLockedAttrs(),
+            revInfoCacheKey(rev),
             infoAttrs,
-            storePath,
-            true);
+            storePath);
 
         return makeResult(infoAttrs, std::move(storePath));
     }
diff --git a/tests/functional/fetchMercurial.sh b/tests/functional/fetchMercurial.sh
index e133df1f8..9f7cef7b2 100644
--- a/tests/functional/fetchMercurial.sh
+++ b/tests/functional/fetchMercurial.sh
@@ -101,6 +101,7 @@ path4=$(nix eval --impure --refresh --raw --expr "(builtins.fetchMercurial file:
 [[ $path2 = $path4 ]]
 
 echo paris > $repo/hello
+
 # Passing a `name` argument should be reflected in the output path
 path5=$(nix eval -vvvvv --impure --refresh --raw --expr "(builtins.fetchMercurial { url = \"file://$repo\"; name = \"foo\"; } ).outPath")
 [[ $path5 =~ -foo$ ]]

From aad11f44962aa49a61cf73155d61204ad48e42e3 Mon Sep 17 00:00:00 2001
From: Eelco Dolstra <edolstra@gmail.com>
Date: Wed, 10 Apr 2024 20:59:18 +0200
Subject: [PATCH 2/4] Simplify the fetcher cache

---
 src/libfetchers/cache.cc          | 186 +++++++++++++++---------------
 src/libfetchers/cache.hh          |  61 ++++++----
 src/libfetchers/fetch-to-store.cc |   9 +-
 src/libfetchers/git-utils.cc      |   7 +-
 src/libfetchers/github.cc         |  14 ++-
 src/libfetchers/tarball.cc        |  43 ++++---
 src/libfetchers/unix/git.cc       |  14 ++-
 src/libfetchers/unix/mercurial.cc |  26 ++---
 8 files changed, 187 insertions(+), 173 deletions(-)

diff --git a/src/libfetchers/cache.cc b/src/libfetchers/cache.cc
index 83962856c..34eb6f32c 100644
--- a/src/libfetchers/cache.cc
+++ b/src/libfetchers/cache.cc
@@ -11,12 +11,11 @@ namespace nix::fetchers {
 static const char * schema = R"sql(
 
 create table if not exists Cache (
-    input     text not null,
-    info      text not null,
-    path      text not null,
-    immutable integer not null, /* obsolete */
+    domain    text not null,
+    key       text not null,
+    value     text not null,
     timestamp integer not null,
-    primary key (input)
+    primary key (domain, key)
 );
 )sql";
 
@@ -28,7 +27,7 @@ struct CacheImpl : Cache
     struct State
     {
         SQLite db;
-        SQLiteStmt add, lookup;
+        SQLiteStmt upsert, lookup;
     };
 
     Sync<State> _state;
@@ -37,133 +36,134 @@ struct CacheImpl : Cache
     {
         auto state(_state.lock());
 
-        auto dbPath = getCacheDir() + "/nix/fetcher-cache-v1.sqlite";
+        auto dbPath = getCacheDir() + "/nix/fetcher-cache-v2.sqlite";
         createDirs(dirOf(dbPath));
 
         state->db = SQLite(dbPath);
         state->db.isCache();
         state->db.exec(schema);
 
-        state->add.create(state->db,
-            "insert or replace into Cache(input, info, path, immutable, timestamp) values (?, ?, ?, false, ?)");
+        state->upsert.create(state->db,
+            "insert or replace into Cache(domain, key, value, timestamp) values (?, ?, ?, ?)");
 
         state->lookup.create(state->db,
-            "select info, path, immutable, timestamp from Cache where input = ?");
+            "select value, timestamp from Cache where domain = ? and key = ?");
     }
 
     void upsert(
-        const Attrs & inAttrs,
-        const Attrs & infoAttrs) override
+        std::string_view domain,
+        const Attrs & key,
+        const Attrs & value) override
     {
-        _state.lock()->add.use()
-            (attrsToJSON(inAttrs).dump())
-            (attrsToJSON(infoAttrs).dump())
-            ("") // no path
+        _state.lock()->upsert.use()
+            (domain)
+            (attrsToJSON(key).dump())
+            (attrsToJSON(value).dump())
             (time(0)).exec();
     }
 
-    std::optional<Attrs> lookup(const Attrs & inAttrs) override
+    std::optional<Attrs> lookup(
+        std::string_view domain,
+        const Attrs & key) override
     {
-        if (auto res = lookupExpired(inAttrs))
-            return std::move(res->infoAttrs);
+        if (auto res = lookupExpired(domain, key))
+            return std::move(res->value);
         return {};
     }
 
-    std::optional<Attrs> lookupWithTTL(const Attrs & inAttrs) override
+    std::optional<Attrs> lookupWithTTL(
+        std::string_view domain,
+        const Attrs & key) override
     {
-        if (auto res = lookupExpired(inAttrs)) {
+        if (auto res = lookupExpired(domain, key)) {
             if (!res->expired)
-                return std::move(res->infoAttrs);
-            debug("ignoring expired cache entry '%s'",
-                attrsToJSON(inAttrs).dump());
-        }
-        return {};
-    }
-
-    std::optional<Result2> lookupExpired(const Attrs & inAttrs) override
-    {
-        auto state(_state.lock());
-
-        auto inAttrsJSON = attrsToJSON(inAttrs).dump();
-
-        auto stmt(state->lookup.use()(inAttrsJSON));
-        if (!stmt.next()) {
-            debug("did not find cache entry for '%s'", inAttrsJSON);
-            return {};
-        }
-
-        auto infoJSON = stmt.getStr(0);
-        auto locked = stmt.getInt(2) != 0;
-        auto timestamp = stmt.getInt(3);
-
-        debug("using cache entry '%s' -> '%s'", inAttrsJSON, infoJSON);
-
-        return Result2 {
-            .expired = !locked && (settings.tarballTtl.get() == 0 || timestamp + settings.tarballTtl < time(0)),
-            .infoAttrs = jsonToAttrs(nlohmann::json::parse(infoJSON)),
-        };
-    }
-
-    void add(
-        Store & store,
-        const Attrs & inAttrs,
-        const Attrs & infoAttrs,
-        const StorePath & storePath) override
-    {
-        _state.lock()->add.use()
-            (attrsToJSON(inAttrs).dump())
-            (attrsToJSON(infoAttrs).dump())
-            (store.printStorePath(storePath))
-            (time(0)).exec();
-    }
-
-    std::optional<std::pair<Attrs, StorePath>> lookup(
-        Store & store,
-        const Attrs & inAttrs) override
-    {
-        if (auto res = lookupExpired(store, inAttrs)) {
-            if (!res->expired)
-                return std::make_pair(std::move(res->infoAttrs), std::move(res->storePath));
-            debug("ignoring expired cache entry '%s'",
-                attrsToJSON(inAttrs).dump());
+                return std::move(res->value);
+            debug("ignoring expired cache entry '%s:%s'",
+                domain, attrsToJSON(key).dump());
         }
         return {};
     }
 
     std::optional<Result> lookupExpired(
-        Store & store,
-        const Attrs & inAttrs) override
+        std::string_view domain,
+        const Attrs & key) override
     {
         auto state(_state.lock());
 
-        auto inAttrsJSON = attrsToJSON(inAttrs).dump();
+        auto keyJSON = attrsToJSON(key).dump();
 
-        auto stmt(state->lookup.use()(inAttrsJSON));
+        auto stmt(state->lookup.use()(domain)(keyJSON));
         if (!stmt.next()) {
-            debug("did not find cache entry for '%s'", inAttrsJSON);
+            debug("did not find cache entry for '%s:%s'", domain, keyJSON);
             return {};
         }
 
-        auto infoJSON = stmt.getStr(0);
-        auto storePath = store.parseStorePath(stmt.getStr(1));
-        auto locked = stmt.getInt(2) != 0;
-        auto timestamp = stmt.getInt(3);
+        auto valueJSON = stmt.getStr(0);
+        auto timestamp = stmt.getInt(1);
 
-        store.addTempRoot(storePath);
-        if (!store.isValidPath(storePath)) {
+        debug("using cache entry '%s:%s' -> '%s'", domain, keyJSON, valueJSON);
+
+        return Result {
+            .expired = settings.tarballTtl.get() == 0 || timestamp + settings.tarballTtl < time(0),
+            .value = jsonToAttrs(nlohmann::json::parse(valueJSON)),
+        };
+    }
+
+    void upsert(
+        std::string_view domain,
+        Attrs key,
+        Store & store,
+        Attrs value,
+        const StorePath & storePath)
+    {
+        /* Add the store prefix to the cache key to handle multiple
+           store prefixes. */
+        key.insert_or_assign("store", store.storeDir);
+
+        value.insert_or_assign("storePath", (std::string) storePath.to_string());
+
+        upsert(domain, key, value);
+    }
+
+    std::optional<ResultWithStorePath> lookupStorePath(
+        std::string_view domain,
+        Attrs key,
+        Store & store) override
+    {
+        key.insert_or_assign("store", store.storeDir);
+
+        auto res = lookupExpired(domain, key);
+        if (!res) return std::nullopt;
+
+        auto storePathS = getStrAttr(res->value, "storePath");
+        res->value.erase("storePath");
+
+        ResultWithStorePath res2(*res, StorePath(storePathS));
+
+        store.addTempRoot(res2.storePath);
+        if (!store.isValidPath(res2.storePath)) {
             // FIXME: we could try to substitute 'storePath'.
-            debug("ignoring disappeared cache entry '%s'", inAttrsJSON);
-            return {};
+            debug("ignoring disappeared cache entry '%s' -> '%s'",
+                attrsToJSON(key).dump(),
+                store.printStorePath(res2.storePath));
+            return std::nullopt;
         }
 
         debug("using cache entry '%s' -> '%s', '%s'",
-            inAttrsJSON, infoJSON, store.printStorePath(storePath));
+            attrsToJSON(key).dump(),
+            attrsToJSON(res2.value).dump(),
+            store.printStorePath(res2.storePath));
 
-        return Result {
-            .expired = !locked && (settings.tarballTtl.get() == 0 || timestamp + settings.tarballTtl < time(0)),
-            .infoAttrs = jsonToAttrs(nlohmann::json::parse(infoJSON)),
-            .storePath = std::move(storePath)
-        };
+        return res2;
+    }
+
+    std::optional<ResultWithStorePath> lookupStorePathWithTTL(
+        std::string_view domain,
+        Attrs key,
+        Store & store) override
+    {
+        auto res = lookupStorePath(domain, std::move(key), store);
+        return res && !res->expired ? res : std::nullopt;
     }
 };
 
diff --git a/src/libfetchers/cache.hh b/src/libfetchers/cache.hh
index 5e05d7af8..3295b56bc 100644
--- a/src/libfetchers/cache.hh
+++ b/src/libfetchers/cache.hh
@@ -19,56 +19,73 @@ struct Cache
      * Attrs to Attrs.
      */
     virtual void upsert(
-        const Attrs & inAttrs,
-        const Attrs & infoAttrs) = 0;
+        std::string_view domain,
+        const Attrs & key,
+        const Attrs & value) = 0;
 
     /**
      * Look up a key with infinite TTL.
      */
     virtual std::optional<Attrs> lookup(
-        const Attrs & inAttrs) = 0;
+        std::string_view domain,
+        const Attrs & key) = 0;
 
     /**
      * Look up a key. Return nothing if its TTL has exceeded
      * `settings.tarballTTL`.
      */
     virtual std::optional<Attrs> lookupWithTTL(
-        const Attrs & inAttrs) = 0;
+        std::string_view domain,
+        const Attrs & key) = 0;
 
-    struct Result2
+    struct Result
     {
         bool expired = false;
-        Attrs infoAttrs;
+        Attrs value;
     };
 
     /**
      * Look up a key. Return a bool denoting whether its TTL has
      * exceeded `settings.tarballTTL`.
      */
-    virtual std::optional<Result2> lookupExpired(
-        const Attrs & inAttrs) = 0;
+    virtual std::optional<Result> lookupExpired(
+        std::string_view domain,
+        const Attrs & key) = 0;
 
-    /* Old cache for things that have a store path. */
-    virtual void add(
+    /**
+     * Insert a cache entry that has a store path associated with
+     * it. Such cache entries are always considered stale if the
+     * associated store path is invalid.
+     */
+    virtual void upsert(
+        std::string_view domain,
+        Attrs key,
         Store & store,
-        const Attrs & inAttrs,
-        const Attrs & infoAttrs,
+        Attrs value,
         const StorePath & storePath) = 0;
 
-    virtual std::optional<std::pair<Attrs, StorePath>> lookup(
-        Store & store,
-        const Attrs & inAttrs) = 0;
-
-    struct Result
+    struct ResultWithStorePath : Result
     {
-        bool expired = false;
-        Attrs infoAttrs;
         StorePath storePath;
     };
 
-    virtual std::optional<Result> lookupExpired(
-        Store & store,
-        const Attrs & inAttrs) = 0;
+    /**
+     * Look up a store path in the cache. The returned store path will
+     * be valid, but it may be expired.
+     */
+    virtual std::optional<ResultWithStorePath> lookupStorePath(
+        std::string_view domain,
+        Attrs key,
+        Store & store) = 0;
+
+    /**
+     * Look up a store path in the cache. Return nothing if its TTL
+     * has exceeded `settings.tarballTTL`.
+     */
+    virtual std::optional<ResultWithStorePath> lookupStorePathWithTTL(
+        std::string_view domain,
+        Attrs key,
+        Store & store) = 0;
 };
 
 ref<Cache> getCache();
diff --git a/src/libfetchers/fetch-to-store.cc b/src/libfetchers/fetch-to-store.cc
index 4156302c4..2116906ad 100644
--- a/src/libfetchers/fetch-to-store.cc
+++ b/src/libfetchers/fetch-to-store.cc
@@ -16,20 +16,19 @@ StorePath fetchToStore(
     // FIXME: add an optimisation for the case where the accessor is
     // an FSInputAccessor pointing to a store path.
 
+    auto domain = "fetchToStore";
     std::optional<fetchers::Attrs> cacheKey;
 
     if (!filter && path.accessor->fingerprint) {
         cacheKey = fetchers::Attrs{
-            {"_what", "fetchToStore"},
-            {"store", store.storeDir},
             {"name", std::string{name}},
             {"fingerprint", *path.accessor->fingerprint},
             {"method", std::string{method.render()}},
             {"path", path.path.abs()}
         };
-        if (auto res = fetchers::getCache()->lookup(store, *cacheKey)) {
+        if (auto res = fetchers::getCache()->lookupStorePath(domain, *cacheKey, store)) {
             debug("store path cache hit for '%s'", path);
-            return res->second;
+            return res->storePath;
         }
     } else
         debug("source path '%s' is uncacheable", path);
@@ -47,7 +46,7 @@ StorePath fetchToStore(
             name, *path.accessor, path.path, method, HashAlgorithm::SHA256, {}, filter2, repair);
 
     if (cacheKey && mode == FetchMode::Copy)
-        fetchers::getCache()->add(store, *cacheKey, {}, storePath);
+        fetchers::getCache()->upsert(domain, *cacheKey, store, {}, storePath);
 
     return storePath;
 }
diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc
index 5ecd825b7..ae4facc67 100644
--- a/src/libfetchers/git-utils.cc
+++ b/src/libfetchers/git-utils.cc
@@ -456,14 +456,15 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this<GitRepoImpl>
     {
         auto accessor = getAccessor(treeHash, false);
 
-        fetchers::Attrs cacheKey({{"_what", "treeHashToNarHash"}, {"treeHash", treeHash.gitRev()}});
+        auto domain = "treeHashToNarHash";
+        fetchers::Attrs cacheKey({{"treeHash", treeHash.gitRev()}});
 
-        if (auto res = fetchers::getCache()->lookup(cacheKey))
+        if (auto res = fetchers::getCache()->lookup(domain, cacheKey))
             return Hash::parseAny(fetchers::getStrAttr(*res, "narHash"), HashAlgorithm::SHA256);
 
         auto narHash = accessor->hashPath(CanonPath::root);
 
-        fetchers::getCache()->upsert(cacheKey, fetchers::Attrs({{"narHash", narHash.to_string(HashFormat::SRI, true)}}));
+        fetchers::getCache()->upsert(domain, cacheKey, fetchers::Attrs({{"narHash", narHash.to_string(HashFormat::SRI, true)}}));
 
         return narHash;
     }
diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc
index 985f2e479..487144925 100644
--- a/src/libfetchers/github.cc
+++ b/src/libfetchers/github.cc
@@ -225,11 +225,13 @@ struct GitArchiveInputScheme : InputScheme
 
         auto cache = getCache();
 
-        Attrs treeHashKey{{"_what", "gitRevToTreeHash"}, {"rev", rev->gitRev()}};
-        Attrs lastModifiedKey{{"_what", "gitRevToLastModified"}, {"rev", rev->gitRev()}};
+        auto treeHashDomain = "gitRevToTreeHash";
+        Attrs treeHashKey{{"rev", rev->gitRev()}};
+        auto lastModifiedDomain = "gitRevToLastModified";
+        Attrs lastModifiedKey{{"rev", rev->gitRev()}};
 
-        if (auto treeHashAttrs = cache->lookup(treeHashKey)) {
-            if (auto lastModifiedAttrs = cache->lookup(lastModifiedKey)) {
+        if (auto treeHashAttrs = cache->lookup(treeHashDomain, treeHashKey)) {
+            if (auto lastModifiedAttrs = cache->lookup(lastModifiedDomain, lastModifiedKey)) {
                 auto treeHash = getRevAttr(*treeHashAttrs, "treeHash");
                 auto lastModified = getIntAttr(*lastModifiedAttrs, "lastModified");
                 if (getTarballCache()->hasObject(treeHash))
@@ -257,8 +259,8 @@ struct GitArchiveInputScheme : InputScheme
             .lastModified = lastModified
         };
 
-        cache->upsert(treeHashKey, Attrs{{"treeHash", tarballInfo.treeHash.gitRev()}});
-        cache->upsert(lastModifiedKey, Attrs{{"lastModified", (uint64_t) tarballInfo.lastModified}});
+        cache->upsert(treeHashDomain, treeHashKey, Attrs{{"treeHash", tarballInfo.treeHash.gitRev()}});
+        cache->upsert(lastModifiedDomain, lastModifiedKey, Attrs{{"lastModified", (uint64_t) tarballInfo.lastModified}});
 
         #if 0
         if (upstreamTreeHash != tarballInfo.treeHash)
diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc
index fd59c1132..285e2803c 100644
--- a/src/libfetchers/tarball.cc
+++ b/src/libfetchers/tarball.cc
@@ -23,21 +23,22 @@ DownloadFileResult downloadFile(
 {
     // FIXME: check store
 
-    Attrs inAttrs({
-        {"type", "file"},
+    auto domain = "file";
+
+    Attrs key({
         {"url", url},
         {"name", name},
     });
 
-    auto cached = getCache()->lookupExpired(*store, inAttrs);
+    auto cached = getCache()->lookupStorePath(domain, key, *store);
 
     auto useCached = [&]() -> DownloadFileResult
     {
         return {
             .storePath = std::move(cached->storePath),
-            .etag = getStrAttr(cached->infoAttrs, "etag"),
-            .effectiveUrl = getStrAttr(cached->infoAttrs, "url"),
-            .immutableUrl = maybeGetStrAttr(cached->infoAttrs, "immutableUrl"),
+            .etag = getStrAttr(cached->value, "etag"),
+            .effectiveUrl = getStrAttr(cached->value, "url"),
+            .immutableUrl = maybeGetStrAttr(cached->value, "immutableUrl"),
         };
     };
 
@@ -47,7 +48,7 @@ DownloadFileResult downloadFile(
     FileTransferRequest request(url);
     request.headers = headers;
     if (cached)
-        request.expectedETag = getStrAttr(cached->infoAttrs, "etag");
+        request.expectedETag = getStrAttr(cached->value, "etag");
     FileTransferResult res;
     try {
         res = getFileTransfer()->download(request);
@@ -93,13 +94,9 @@ DownloadFileResult downloadFile(
 
     /* Cache metadata for all URLs in the redirect chain. */
     for (auto & url : res.urls) {
-        inAttrs.insert_or_assign("url", url);
+        key.insert_or_assign("url", url);
         infoAttrs.insert_or_assign("url", *res.urls.rbegin());
-        getCache()->add(
-            *store,
-            inAttrs,
-            infoAttrs,
-            *storePath);
+        getCache()->upsert(domain, key, *store, infoAttrs, *storePath);
     }
 
     return {
@@ -114,12 +111,12 @@ DownloadTarballResult downloadTarball(
     const std::string & url,
     const Headers & headers)
 {
-    Attrs inAttrs({
-        {"_what", "tarballCache"},
+    auto domain = "tarball";
+    Attrs cacheKey{
         {"url", url},
-    });
+    };
 
-    auto cached = getCache()->lookupExpired(inAttrs);
+    auto cached = getCache()->lookupExpired(domain, cacheKey);
 
     auto attrsToResult = [&](const Attrs & infoAttrs)
     {
@@ -132,19 +129,19 @@ DownloadTarballResult downloadTarball(
         };
     };
 
-    if (cached && !getTarballCache()->hasObject(getRevAttr(cached->infoAttrs, "treeHash")))
+    if (cached && !getTarballCache()->hasObject(getRevAttr(cached->value, "treeHash")))
         cached.reset();
 
     if (cached && !cached->expired)
         /* We previously downloaded this tarball and it's younger than
            `tarballTtl`, so no need to check the server. */
-        return attrsToResult(cached->infoAttrs);
+        return attrsToResult(cached->value);
 
     auto _res = std::make_shared<Sync<FileTransferResult>>();
 
     auto source = sinkToSource([&](Sink & sink) {
         FileTransferRequest req(url);
-        req.expectedETag = cached ? getStrAttr(cached->infoAttrs, "etag") : "";
+        req.expectedETag = cached ? getStrAttr(cached->value, "etag") : "";
         getFileTransfer()->download(std::move(req), sink,
             [_res](FileTransferResult r)
             {
@@ -167,7 +164,7 @@ DownloadTarballResult downloadTarball(
     if (res->cached) {
         /* The server says that the previously downloaded version is
            still current. */
-        infoAttrs = cached->infoAttrs;
+        infoAttrs = cached->value;
     } else {
         infoAttrs.insert_or_assign("etag", res->etag);
         infoAttrs.insert_or_assign("treeHash", parseSink->sync().gitRev());
@@ -178,8 +175,8 @@ DownloadTarballResult downloadTarball(
 
     /* Insert a cache entry for every URL in the redirect chain. */
     for (auto & url : res->urls) {
-        inAttrs.insert_or_assign("url", url);
-        getCache()->upsert(inAttrs, infoAttrs);
+        cacheKey.insert_or_assign("url", url);
+        getCache()->upsert(domain, cacheKey, infoAttrs);
     }
 
     // FIXME: add a cache entry for immutableUrl? That could allow
diff --git a/src/libfetchers/unix/git.cc b/src/libfetchers/unix/git.cc
index 45e62ebe1..bb1bff7ab 100644
--- a/src/libfetchers/unix/git.cc
+++ b/src/libfetchers/unix/git.cc
@@ -427,34 +427,36 @@ struct GitInputScheme : InputScheme
 
     uint64_t getLastModified(const RepoInfo & repoInfo, const std::string & repoDir, const Hash & rev) const
     {
-        Attrs key{{"_what", "gitLastModified"}, {"rev", rev.gitRev()}};
+        auto domain = "gitLastModified";
+        Attrs key{{"rev", rev.gitRev()}};
 
         auto cache = getCache();
 
-        if (auto res = cache->lookup(key))
+        if (auto res = cache->lookup(domain, key))
             return getIntAttr(*res, "lastModified");
 
         auto lastModified = GitRepo::openRepo(repoDir)->getLastModified(rev);
 
-        cache->upsert(key, Attrs{{"lastModified", lastModified}});
+        cache->upsert(domain, key, {{"lastModified", lastModified}});
 
         return lastModified;
     }
 
     uint64_t getRevCount(const RepoInfo & repoInfo, const std::string & repoDir, const Hash & rev) const
     {
-        Attrs key{{"_what", "gitRevCount"}, {"rev", rev.gitRev()}};
+        auto domain = "gitRevCount";
+        Attrs key{{"rev", rev.gitRev()}};
 
         auto cache = getCache();
 
-        if (auto revCountAttrs = cache->lookup(key))
+        if (auto revCountAttrs = cache->lookup(domain, key))
             return getIntAttr(*revCountAttrs, "revCount");
 
         Activity act(*logger, lvlChatty, actUnknown, fmt("getting Git revision count of '%s'", repoInfo.url));
 
         auto revCount = GitRepo::openRepo(repoDir)->getRevCount(rev);
 
-        cache->upsert(key, Attrs{{"revCount", revCount}});
+        cache->upsert(domain, key, Attrs{{"revCount", revCount}});
 
         return revCount;
     }
diff --git a/src/libfetchers/unix/mercurial.cc b/src/libfetchers/unix/mercurial.cc
index 783e338bf..42757f2db 100644
--- a/src/libfetchers/unix/mercurial.cc
+++ b/src/libfetchers/unix/mercurial.cc
@@ -224,13 +224,13 @@ struct MercurialInputScheme : InputScheme
 
         if (!input.getRef()) input.attrs.insert_or_assign("ref", "default");
 
-        auto revInfoCacheKey = [&](const Hash & rev)
+        auto revInfoDomain = "hgRev";
+        auto revInfoKey = [&](const Hash & rev)
         {
             if (rev.algo != HashAlgorithm::SHA1)
                 throw Error("Hash '%s' is not supported by Mercurial. Only sha1 is supported.", rev.to_string(HashFormat::Base16, true));
 
             return Attrs{
-                {"_what", "hgRev"},
                 {"store", store->storeDir},
                 {"name", name},
                 {"rev", input.getRev()->gitRev()}
@@ -246,21 +246,21 @@ struct MercurialInputScheme : InputScheme
         };
 
         /* Check the cache for the most recent rev for this URL/ref. */
-        Attrs refToRevCacheKey{
-            {"_what", "hgRefToRev"},
+        auto refToRevDomain = "hgRefToRev";
+        Attrs refToRevKey{
             {"url", actualUrl},
             {"ref", *input.getRef()}
         };
 
         if (!input.getRev()) {
-            if (auto res = getCache()->lookupWithTTL(refToRevCacheKey))
+            if (auto res = getCache()->lookupWithTTL(refToRevDomain, refToRevKey))
                 input.attrs.insert_or_assign("rev", getRevAttr(*res, "rev").gitRev());
         }
 
         /* If we have a rev, check if we have a cached store path. */
         if (auto rev = input.getRev()) {
-            if (auto res = getCache()->lookupExpired(*store, revInfoCacheKey(*rev)))
-                return makeResult(res->infoAttrs, res->storePath);
+            if (auto res = getCache()->lookupStorePath(revInfoDomain, revInfoKey(*rev), *store))
+                return makeResult(res->value, res->storePath);
         }
 
         Path cacheDir = fmt("%s/nix/hg/%s", getCacheDir(), hashString(HashAlgorithm::SHA256, actualUrl).to_string(HashFormat::Nix32, false));
@@ -309,8 +309,8 @@ struct MercurialInputScheme : InputScheme
 
         /* Now that we have the rev, check the cache again for a
            cached store path. */
-        if (auto res = getCache()->lookupExpired(*store, revInfoCacheKey(rev)))
-            return makeResult(res->infoAttrs, res->storePath);
+        if (auto res = getCache()->lookupStorePath(revInfoDomain, revInfoKey(rev), *store))
+            return makeResult(res->value, res->storePath);
 
         Path tmpDir = createTempDir();
         AutoDelete delTmpDir(tmpDir, true);
@@ -327,13 +327,9 @@ struct MercurialInputScheme : InputScheme
         });
 
         if (!origRev)
-            getCache()->upsert(refToRevCacheKey, {{"rev", rev.gitRev()}});
+            getCache()->upsert(refToRevDomain, refToRevKey, {{"rev", rev.gitRev()}});
 
-        getCache()->add(
-            *store,
-            revInfoCacheKey(rev),
-            infoAttrs,
-            storePath);
+        getCache()->upsert(revInfoDomain, revInfoKey(rev), *store, infoAttrs, storePath);
 
         return makeResult(infoAttrs, std::move(storePath));
     }

From cceae30aafad32e7ba8301980aabee511e5c05de Mon Sep 17 00:00:00 2001
From: Eelco Dolstra <edolstra@gmail.com>
Date: Wed, 10 Apr 2024 21:39:40 +0200
Subject: [PATCH 3/4] Combine the domain and key arguments into a single value
 for convenience

---
 src/libfetchers/cache.cc          | 59 ++++++++++++++-----------------
 src/libfetchers/cache.hh          | 35 +++++++++---------
 src/libfetchers/fetch-to-store.cc | 11 +++---
 src/libfetchers/git-utils.cc      |  7 ++--
 src/libfetchers/github.cc         | 14 ++++----
 src/libfetchers/tarball.cc        | 23 +++++-------
 src/libfetchers/unix/git.cc       | 14 ++++----
 src/libfetchers/unix/mercurial.cc | 20 +++++------
 8 files changed, 84 insertions(+), 99 deletions(-)

diff --git a/src/libfetchers/cache.cc b/src/libfetchers/cache.cc
index 34eb6f32c..87a8e4702 100644
--- a/src/libfetchers/cache.cc
+++ b/src/libfetchers/cache.cc
@@ -51,57 +51,53 @@ struct CacheImpl : Cache
     }
 
     void upsert(
-        std::string_view domain,
-        const Attrs & key,
+        const Key & key,
         const Attrs & value) override
     {
         _state.lock()->upsert.use()
-            (domain)
-            (attrsToJSON(key).dump())
+            (key.first)
+            (attrsToJSON(key.second).dump())
             (attrsToJSON(value).dump())
             (time(0)).exec();
     }
 
     std::optional<Attrs> lookup(
-        std::string_view domain,
-        const Attrs & key) override
+        const Key & key) override
     {
-        if (auto res = lookupExpired(domain, key))
+        if (auto res = lookupExpired(key))
             return std::move(res->value);
         return {};
     }
 
     std::optional<Attrs> lookupWithTTL(
-        std::string_view domain,
-        const Attrs & key) override
+        const Key & key) override
     {
-        if (auto res = lookupExpired(domain, key)) {
+        if (auto res = lookupExpired(key)) {
             if (!res->expired)
                 return std::move(res->value);
             debug("ignoring expired cache entry '%s:%s'",
-                domain, attrsToJSON(key).dump());
+                key.first, attrsToJSON(key.second).dump());
         }
         return {};
     }
 
     std::optional<Result> lookupExpired(
-        std::string_view domain,
-        const Attrs & key) override
+        const Key & key) override
     {
         auto state(_state.lock());
 
-        auto keyJSON = attrsToJSON(key).dump();
+        auto keyJSON = attrsToJSON(key.second).dump();
 
-        auto stmt(state->lookup.use()(domain)(keyJSON));
+        auto stmt(state->lookup.use()(key.first)(keyJSON));
         if (!stmt.next()) {
-            debug("did not find cache entry for '%s:%s'", domain, keyJSON);
+            debug("did not find cache entry for '%s:%s'", key.first, keyJSON);
             return {};
         }
 
         auto valueJSON = stmt.getStr(0);
         auto timestamp = stmt.getInt(1);
 
-        debug("using cache entry '%s:%s' -> '%s'", domain, keyJSON, valueJSON);
+        debug("using cache entry '%s:%s' -> '%s'", key.first, keyJSON, valueJSON);
 
         return Result {
             .expired = settings.tarballTtl.get() == 0 || timestamp + settings.tarballTtl < time(0),
@@ -110,29 +106,27 @@ struct CacheImpl : Cache
     }
 
     void upsert(
-        std::string_view domain,
-        Attrs key,
+        Key key,
         Store & store,
         Attrs value,
         const StorePath & storePath)
     {
         /* Add the store prefix to the cache key to handle multiple
            store prefixes. */
-        key.insert_or_assign("store", store.storeDir);
+        key.second.insert_or_assign("store", store.storeDir);
 
         value.insert_or_assign("storePath", (std::string) storePath.to_string());
 
-        upsert(domain, key, value);
+        upsert(key, value);
     }
 
     std::optional<ResultWithStorePath> lookupStorePath(
-        std::string_view domain,
-        Attrs key,
+        Key key,
         Store & store) override
     {
-        key.insert_or_assign("store", store.storeDir);
+        key.second.insert_or_assign("store", store.storeDir);
 
-        auto res = lookupExpired(domain, key);
+        auto res = lookupExpired(key);
         if (!res) return std::nullopt;
 
         auto storePathS = getStrAttr(res->value, "storePath");
@@ -143,14 +137,16 @@ struct CacheImpl : Cache
         store.addTempRoot(res2.storePath);
         if (!store.isValidPath(res2.storePath)) {
             // FIXME: we could try to substitute 'storePath'.
-            debug("ignoring disappeared cache entry '%s' -> '%s'",
-                attrsToJSON(key).dump(),
+            debug("ignoring disappeared cache entry '%s:%s' -> '%s'",
+                key.first,
+                attrsToJSON(key.second).dump(),
                 store.printStorePath(res2.storePath));
             return std::nullopt;
         }
 
-        debug("using cache entry '%s' -> '%s', '%s'",
-            attrsToJSON(key).dump(),
+        debug("using cache entry '%s:%s' -> '%s', '%s'",
+            key.first,
+            attrsToJSON(key.second).dump(),
             attrsToJSON(res2.value).dump(),
             store.printStorePath(res2.storePath));
 
@@ -158,11 +154,10 @@ struct CacheImpl : Cache
     }
 
     std::optional<ResultWithStorePath> lookupStorePathWithTTL(
-        std::string_view domain,
-        Attrs key,
+        Key key,
         Store & store) override
     {
-        auto res = lookupStorePath(domain, std::move(key), store);
+        auto res = lookupStorePath(std::move(key), store);
         return res && !res->expired ? res : std::nullopt;
     }
 };
diff --git a/src/libfetchers/cache.hh b/src/libfetchers/cache.hh
index 3295b56bc..1a72162d7 100644
--- a/src/libfetchers/cache.hh
+++ b/src/libfetchers/cache.hh
@@ -15,28 +15,35 @@ struct Cache
     virtual ~Cache() { }
 
     /**
-     * Add a value to the cache. The cache is an arbitrary mapping of
-     * Attrs to Attrs.
+     * A domain is a partition of the key/value cache for a particular
+     * purpose, e.g. "Git revision to revcount".
+     */
+    using Domain = std::string_view;
+
+    /**
+     * A cache key is a domain and an arbitrary set of attributes.
+     */
+    using Key = std::pair<Domain, Attrs>;
+
+    /**
+     * Add a key/value pair to the cache.
      */
     virtual void upsert(
-        std::string_view domain,
-        const Attrs & key,
+        const Key & key,
         const Attrs & value) = 0;
 
     /**
      * Look up a key with infinite TTL.
      */
     virtual std::optional<Attrs> lookup(
-        std::string_view domain,
-        const Attrs & key) = 0;
+        const Key & key) = 0;
 
     /**
      * Look up a key. Return nothing if its TTL has exceeded
      * `settings.tarballTTL`.
      */
     virtual std::optional<Attrs> lookupWithTTL(
-        std::string_view domain,
-        const Attrs & key) = 0;
+        const Key & key) = 0;
 
     struct Result
     {
@@ -49,8 +56,7 @@ struct Cache
      * exceeded `settings.tarballTTL`.
      */
     virtual std::optional<Result> lookupExpired(
-        std::string_view domain,
-        const Attrs & key) = 0;
+        const Key & key) = 0;
 
     /**
      * Insert a cache entry that has a store path associated with
@@ -58,8 +64,7 @@ struct Cache
      * associated store path is invalid.
      */
     virtual void upsert(
-        std::string_view domain,
-        Attrs key,
+        Key key,
         Store & store,
         Attrs value,
         const StorePath & storePath) = 0;
@@ -74,8 +79,7 @@ struct Cache
      * be valid, but it may be expired.
      */
     virtual std::optional<ResultWithStorePath> lookupStorePath(
-        std::string_view domain,
-        Attrs key,
+        Key key,
         Store & store) = 0;
 
     /**
@@ -83,8 +87,7 @@ struct Cache
      * has exceeded `settings.tarballTTL`.
      */
     virtual std::optional<ResultWithStorePath> lookupStorePathWithTTL(
-        std::string_view domain,
-        Attrs key,
+        Key key,
         Store & store) = 0;
 };
 
diff --git a/src/libfetchers/fetch-to-store.cc b/src/libfetchers/fetch-to-store.cc
index 2116906ad..96743cb52 100644
--- a/src/libfetchers/fetch-to-store.cc
+++ b/src/libfetchers/fetch-to-store.cc
@@ -16,17 +16,16 @@ StorePath fetchToStore(
     // FIXME: add an optimisation for the case where the accessor is
     // an FSInputAccessor pointing to a store path.
 
-    auto domain = "fetchToStore";
-    std::optional<fetchers::Attrs> cacheKey;
+    std::optional<fetchers::Cache::Key> cacheKey;
 
     if (!filter && path.accessor->fingerprint) {
-        cacheKey = fetchers::Attrs{
+        cacheKey = fetchers::Cache::Key{"fetchToStore", {
             {"name", std::string{name}},
             {"fingerprint", *path.accessor->fingerprint},
             {"method", std::string{method.render()}},
             {"path", path.path.abs()}
-        };
-        if (auto res = fetchers::getCache()->lookupStorePath(domain, *cacheKey, store)) {
+        }};
+        if (auto res = fetchers::getCache()->lookupStorePath(*cacheKey, store)) {
             debug("store path cache hit for '%s'", path);
             return res->storePath;
         }
@@ -46,7 +45,7 @@ StorePath fetchToStore(
             name, *path.accessor, path.path, method, HashAlgorithm::SHA256, {}, filter2, repair);
 
     if (cacheKey && mode == FetchMode::Copy)
-        fetchers::getCache()->upsert(domain, *cacheKey, store, {}, storePath);
+        fetchers::getCache()->upsert(*cacheKey, store, {}, storePath);
 
     return storePath;
 }
diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc
index ae4facc67..d4ba1a91d 100644
--- a/src/libfetchers/git-utils.cc
+++ b/src/libfetchers/git-utils.cc
@@ -456,15 +456,14 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this<GitRepoImpl>
     {
         auto accessor = getAccessor(treeHash, false);
 
-        auto domain = "treeHashToNarHash";
-        fetchers::Attrs cacheKey({{"treeHash", treeHash.gitRev()}});
+        fetchers::Cache::Key cacheKey{"treeHashToNarHash", {{"treeHash", treeHash.gitRev()}}};
 
-        if (auto res = fetchers::getCache()->lookup(domain, cacheKey))
+        if (auto res = fetchers::getCache()->lookup(cacheKey))
             return Hash::parseAny(fetchers::getStrAttr(*res, "narHash"), HashAlgorithm::SHA256);
 
         auto narHash = accessor->hashPath(CanonPath::root);
 
-        fetchers::getCache()->upsert(domain, cacheKey, fetchers::Attrs({{"narHash", narHash.to_string(HashFormat::SRI, true)}}));
+        fetchers::getCache()->upsert(cacheKey, fetchers::Attrs({{"narHash", narHash.to_string(HashFormat::SRI, true)}}));
 
         return narHash;
     }
diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc
index 487144925..7aa857dfe 100644
--- a/src/libfetchers/github.cc
+++ b/src/libfetchers/github.cc
@@ -225,13 +225,11 @@ struct GitArchiveInputScheme : InputScheme
 
         auto cache = getCache();
 
-        auto treeHashDomain = "gitRevToTreeHash";
-        Attrs treeHashKey{{"rev", rev->gitRev()}};
-        auto lastModifiedDomain = "gitRevToLastModified";
-        Attrs lastModifiedKey{{"rev", rev->gitRev()}};
+        Cache::Key treeHashKey{"gitRevToTreeHash", {{"rev", rev->gitRev()}}};
+        Cache::Key lastModifiedKey{"gitRevToLastModified", {{"rev", rev->gitRev()}}};
 
-        if (auto treeHashAttrs = cache->lookup(treeHashDomain, treeHashKey)) {
-            if (auto lastModifiedAttrs = cache->lookup(lastModifiedDomain, lastModifiedKey)) {
+        if (auto treeHashAttrs = cache->lookup(treeHashKey)) {
+            if (auto lastModifiedAttrs = cache->lookup(lastModifiedKey)) {
                 auto treeHash = getRevAttr(*treeHashAttrs, "treeHash");
                 auto lastModified = getIntAttr(*lastModifiedAttrs, "lastModified");
                 if (getTarballCache()->hasObject(treeHash))
@@ -259,8 +257,8 @@ struct GitArchiveInputScheme : InputScheme
             .lastModified = lastModified
         };
 
-        cache->upsert(treeHashDomain, treeHashKey, Attrs{{"treeHash", tarballInfo.treeHash.gitRev()}});
-        cache->upsert(lastModifiedDomain, lastModifiedKey, Attrs{{"lastModified", (uint64_t) tarballInfo.lastModified}});
+        cache->upsert(treeHashKey, Attrs{{"treeHash", tarballInfo.treeHash.gitRev()}});
+        cache->upsert(lastModifiedKey, Attrs{{"lastModified", (uint64_t) tarballInfo.lastModified}});
 
         #if 0
         if (upstreamTreeHash != tarballInfo.treeHash)
diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc
index 285e2803c..89ef31c7e 100644
--- a/src/libfetchers/tarball.cc
+++ b/src/libfetchers/tarball.cc
@@ -23,14 +23,12 @@ DownloadFileResult downloadFile(
 {
     // FIXME: check store
 
-    auto domain = "file";
-
-    Attrs key({
+    Cache::Key key{"file", {{
         {"url", url},
         {"name", name},
-    });
+    }}};
 
-    auto cached = getCache()->lookupStorePath(domain, key, *store);
+    auto cached = getCache()->lookupStorePath(key, *store);
 
     auto useCached = [&]() -> DownloadFileResult
     {
@@ -94,9 +92,9 @@ DownloadFileResult downloadFile(
 
     /* Cache metadata for all URLs in the redirect chain. */
     for (auto & url : res.urls) {
-        key.insert_or_assign("url", url);
+        key.second.insert_or_assign("url", url);
         infoAttrs.insert_or_assign("url", *res.urls.rbegin());
-        getCache()->upsert(domain, key, *store, infoAttrs, *storePath);
+        getCache()->upsert(key, *store, infoAttrs, *storePath);
     }
 
     return {
@@ -111,12 +109,9 @@ DownloadTarballResult downloadTarball(
     const std::string & url,
     const Headers & headers)
 {
-    auto domain = "tarball";
-    Attrs cacheKey{
-        {"url", url},
-    };
+    Cache::Key cacheKey{"tarball", {{"url", url}}};
 
-    auto cached = getCache()->lookupExpired(domain, cacheKey);
+    auto cached = getCache()->lookupExpired(cacheKey);
 
     auto attrsToResult = [&](const Attrs & infoAttrs)
     {
@@ -175,8 +170,8 @@ DownloadTarballResult downloadTarball(
 
     /* Insert a cache entry for every URL in the redirect chain. */
     for (auto & url : res->urls) {
-        cacheKey.insert_or_assign("url", url);
-        getCache()->upsert(domain, cacheKey, infoAttrs);
+        cacheKey.second.insert_or_assign("url", url);
+        getCache()->upsert(cacheKey, infoAttrs);
     }
 
     // FIXME: add a cache entry for immutableUrl? That could allow
diff --git a/src/libfetchers/unix/git.cc b/src/libfetchers/unix/git.cc
index bb1bff7ab..1d7c719a6 100644
--- a/src/libfetchers/unix/git.cc
+++ b/src/libfetchers/unix/git.cc
@@ -427,36 +427,34 @@ struct GitInputScheme : InputScheme
 
     uint64_t getLastModified(const RepoInfo & repoInfo, const std::string & repoDir, const Hash & rev) const
     {
-        auto domain = "gitLastModified";
-        Attrs key{{"rev", rev.gitRev()}};
+        Cache::Key key{"gitLastModified", {{"rev", rev.gitRev()}}};
 
         auto cache = getCache();
 
-        if (auto res = cache->lookup(domain, key))
+        if (auto res = cache->lookup(key))
             return getIntAttr(*res, "lastModified");
 
         auto lastModified = GitRepo::openRepo(repoDir)->getLastModified(rev);
 
-        cache->upsert(domain, key, {{"lastModified", lastModified}});
+        cache->upsert(key, {{"lastModified", lastModified}});
 
         return lastModified;
     }
 
     uint64_t getRevCount(const RepoInfo & repoInfo, const std::string & repoDir, const Hash & rev) const
     {
-        auto domain = "gitRevCount";
-        Attrs key{{"rev", rev.gitRev()}};
+        Cache::Key key{"gitRevCount", {{"rev", rev.gitRev()}}};
 
         auto cache = getCache();
 
-        if (auto revCountAttrs = cache->lookup(domain, key))
+        if (auto revCountAttrs = cache->lookup(key))
             return getIntAttr(*revCountAttrs, "revCount");
 
         Activity act(*logger, lvlChatty, actUnknown, fmt("getting Git revision count of '%s'", repoInfo.url));
 
         auto revCount = GitRepo::openRepo(repoDir)->getRevCount(rev);
 
-        cache->upsert(domain, key, Attrs{{"revCount", revCount}});
+        cache->upsert(key, Attrs{{"revCount", revCount}});
 
         return revCount;
     }
diff --git a/src/libfetchers/unix/mercurial.cc b/src/libfetchers/unix/mercurial.cc
index 42757f2db..0dd1acf45 100644
--- a/src/libfetchers/unix/mercurial.cc
+++ b/src/libfetchers/unix/mercurial.cc
@@ -224,17 +224,16 @@ struct MercurialInputScheme : InputScheme
 
         if (!input.getRef()) input.attrs.insert_or_assign("ref", "default");
 
-        auto revInfoDomain = "hgRev";
         auto revInfoKey = [&](const Hash & rev)
         {
             if (rev.algo != HashAlgorithm::SHA1)
                 throw Error("Hash '%s' is not supported by Mercurial. Only sha1 is supported.", rev.to_string(HashFormat::Base16, true));
 
-            return Attrs{
+            return Cache::Key{"hgRev", {
                 {"store", store->storeDir},
                 {"name", name},
                 {"rev", input.getRev()->gitRev()}
-            };
+            }};
         };
 
         auto makeResult = [&](const Attrs & infoAttrs, const StorePath & storePath) -> StorePath
@@ -246,20 +245,19 @@ struct MercurialInputScheme : InputScheme
         };
 
         /* Check the cache for the most recent rev for this URL/ref. */
-        auto refToRevDomain = "hgRefToRev";
-        Attrs refToRevKey{
+        Cache::Key refToRevKey{"hgRefToRev", {
             {"url", actualUrl},
             {"ref", *input.getRef()}
-        };
+        }};
 
         if (!input.getRev()) {
-            if (auto res = getCache()->lookupWithTTL(refToRevDomain, refToRevKey))
+            if (auto res = getCache()->lookupWithTTL(refToRevKey))
                 input.attrs.insert_or_assign("rev", getRevAttr(*res, "rev").gitRev());
         }
 
         /* If we have a rev, check if we have a cached store path. */
         if (auto rev = input.getRev()) {
-            if (auto res = getCache()->lookupStorePath(revInfoDomain, revInfoKey(*rev), *store))
+            if (auto res = getCache()->lookupStorePath(revInfoKey(*rev), *store))
                 return makeResult(res->value, res->storePath);
         }
 
@@ -309,7 +307,7 @@ struct MercurialInputScheme : InputScheme
 
         /* Now that we have the rev, check the cache again for a
            cached store path. */
-        if (auto res = getCache()->lookupStorePath(revInfoDomain, revInfoKey(rev), *store))
+        if (auto res = getCache()->lookupStorePath(revInfoKey(rev), *store))
             return makeResult(res->value, res->storePath);
 
         Path tmpDir = createTempDir();
@@ -327,9 +325,9 @@ struct MercurialInputScheme : InputScheme
         });
 
         if (!origRev)
-            getCache()->upsert(refToRevDomain, refToRevKey, {{"rev", rev.gitRev()}});
+            getCache()->upsert(refToRevKey, {{"rev", rev.gitRev()}});
 
-        getCache()->upsert(revInfoDomain, revInfoKey(rev), *store, infoAttrs, storePath);
+        getCache()->upsert(revInfoKey(rev), *store, infoAttrs, storePath);
 
         return makeResult(infoAttrs, std::move(storePath));
     }

From c7216a416fb5ad902775520c18b3b5e3cf49fbe0 Mon Sep 17 00:00:00 2001
From: Eelco Dolstra <edolstra@gmail.com>
Date: Mon, 6 May 2024 21:11:41 +0200
Subject: [PATCH 4/4] Update src/libfetchers/cache.hh

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
---
 src/libfetchers/cache.hh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/libfetchers/cache.hh b/src/libfetchers/cache.hh
index 1a72162d7..4d834fe0c 100644
--- a/src/libfetchers/cache.hh
+++ b/src/libfetchers/cache.hh
@@ -16,7 +16,7 @@ struct Cache
 
     /**
      * A domain is a partition of the key/value cache for a particular
-     * purpose, e.g. "Git revision to revcount".
+     * purpose, e.g. git revision to revcount.
      */
     using Domain = std::string_view;