From 0f6d596df5866d3f08e743ac0f1a282daf6b2155 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Fri, 19 May 2023 09:49:18 +0200
Subject: [PATCH 01/16] fetchClosure: Factor out attribute hint

---
 src/libexpr/primops/fetchClosure.cc | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc
index bae849f61..26c2b90bf 100644
--- a/src/libexpr/primops/fetchClosure.cc
+++ b/src/libexpr/primops/fetchClosure.cc
@@ -16,11 +16,13 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
 
     for (auto & attr : *args[0]->attrs) {
         const auto & attrName = state.symbols[attr.name];
+        auto attrHint = [&]() -> std::string {
+            return "while evaluating the '" + attrName + "' attribute passed to builtins.fetchClosure";
+        };
 
         if (attrName == "fromPath") {
             NixStringContext context;
-            fromPath = state.coerceToStorePath(attr.pos, *attr.value, context,
-                    "while evaluating the 'fromPath' attribute passed to builtins.fetchClosure");
+            fromPath = state.coerceToStorePath(attr.pos, *attr.value, context, attrHint());
         }
 
         else if (attrName == "toPath") {
@@ -28,14 +30,13 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
             toCA = true;
             if (attr.value->type() != nString || attr.value->string.s != std::string("")) {
                 NixStringContext context;
-                toPath = state.coerceToStorePath(attr.pos, *attr.value, context,
-                        "while evaluating the 'toPath' attribute passed to builtins.fetchClosure");
+                toPath = state.coerceToStorePath(attr.pos, *attr.value, context, attrHint());
             }
         }
 
         else if (attrName == "fromStore")
             fromStoreUrl = state.forceStringNoCtx(*attr.value, attr.pos,
-                    "while evaluating the 'fromStore' attribute passed to builtins.fetchClosure");
+                    attrHint());
 
         else
             throw Error({

From 7e5b6d2c4550a3007946271768e114531d7b89f0 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Fri, 19 May 2023 10:03:25 +0200
Subject: [PATCH 02/16] fetchClosure: Refactor: rename toCA -> enableRewriting

---
 src/libexpr/primops/fetchClosure.cc | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc
index 26c2b90bf..8f8e033a5 100644
--- a/src/libexpr/primops/fetchClosure.cc
+++ b/src/libexpr/primops/fetchClosure.cc
@@ -11,7 +11,7 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
 
     std::optional<std::string> fromStoreUrl;
     std::optional<StorePath> fromPath;
-    bool toCA = false;
+    bool enableRewriting = false;
     std::optional<StorePath> toPath;
 
     for (auto & attr : *args[0]->attrs) {
@@ -27,7 +27,7 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
 
         else if (attrName == "toPath") {
             state.forceValue(*attr.value, attr.pos);
-            toCA = true;
+            enableRewriting = true;
             if (attr.value->type() != nString || attr.value->string.s != std::string("")) {
                 NixStringContext context;
                 toPath = state.coerceToStorePath(attr.pos, *attr.value, context, attrHint());
@@ -75,7 +75,7 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
 
     auto fromStore = openStore(parsedURL.to_string());
 
-    if (toCA) {
+    if (enableRewriting) {
         if (!toPath || !state.store->isValidPath(*toPath)) {
             auto remappings = makeContentAddressed(*fromStore, *state.store, { *fromPath });
             auto i = remappings.find(*fromPath);

From ea30f152b738fe65f216f3173d3a19adaaef5323 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Fri, 19 May 2023 10:48:53 +0200
Subject: [PATCH 03/16] fetchClosure: Allow input addressed paths in pure mode

When explicitly requested by the caller, as suggested in the meeting
(https://github.com/NixOS/nix/pull/8090#issuecomment-1531139324)

> @edolstra: { toPath } vs { fromPath } is too implicit

I've opted for the `inputAddressed = true` requirement, because it
we did not agree on renaming the path attributes.

> @roberth: more explicit
> @edolstra: except for the direction; not immediately clear in which direction the rewriting happens

This is in fact the most explicit syntax and a bit redundant, which is
good, because that redundancy lets us deliver an error message that
reminds expression authors that CA provides a better experience to
their users.
---
 src/libexpr/primops/fetchClosure.cc | 54 +++++++++++++++++++++++------
 tests/fetchClosure.sh               | 16 +++++++--
 2 files changed, 57 insertions(+), 13 deletions(-)

diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc
index 8f8e033a5..96c1df544 100644
--- a/src/libexpr/primops/fetchClosure.cc
+++ b/src/libexpr/primops/fetchClosure.cc
@@ -13,6 +13,7 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
     std::optional<StorePath> fromPath;
     bool enableRewriting = false;
     std::optional<StorePath> toPath;
+    bool inputAddressed = false;
 
     for (auto & attr : *args[0]->attrs) {
         const auto & attrName = state.symbols[attr.name];
@@ -38,6 +39,9 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
             fromStoreUrl = state.forceStringNoCtx(*attr.value, attr.pos,
                     attrHint());
 
+        else if (attrName == "inputAddressed")
+            inputAddressed = state.forceBool(*attr.value, attr.pos, attrHint());
+
         else
             throw Error({
                 .msg = hintfmt("attribute '%s' isn't supported in call to 'fetchClosure'", attrName),
@@ -51,6 +55,18 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
             .errPos = state.positions[pos]
         });
 
+    if (inputAddressed) {
+        if (toPath && toPath != fromPath)
+            throw Error({
+                .msg = hintfmt("attribute '%s' is set to true, but 'toPath' does not match 'fromPath'. 'toPath' should be equal, or should be omitted. Instead 'toPath' was '%s' and 'fromPath' was '%s'",
+                    "inputAddressed",
+                    state.store->printStorePath(*toPath),
+                    state.store->printStorePath(*fromPath)),
+                .errPos = state.positions[pos]
+            });
+        assert(!enableRewriting);
+    }
+
     if (!fromStoreUrl)
         throw Error({
             .msg = hintfmt("attribute '%s' is missing in call to 'fetchClosure'", "fromStore"),
@@ -104,15 +120,28 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
         toPath = fromPath;
     }
 
-    /* In pure mode, require a CA path. */
-    if (evalSettings.pureEval) {
+    /* We want input addressing to be explicit, to inform readers and to give
+       expression authors an opportunity to improve their user experience. */
+    if (!inputAddressed) {
         auto info = state.store->queryPathInfo(*toPath);
-        if (!info->isContentAddressed(*state.store))
-            throw Error({
-                .msg = hintfmt("in pure mode, 'fetchClosure' requires a content-addressed path, which '%s' isn't",
-                    state.store->printStorePath(*toPath)),
-                .errPos = state.positions[pos]
-            });
+        if (!info->isContentAddressed(*state.store)) {
+            if (enableRewriting) {
+                throw Error({
+                    // Ideally we'd compute the path for them, but this error message is unlikely to occur in practice, so we keep it simple.
+                    .msg = hintfmt("Rewriting was requested, but 'toPath' is not content addressed. This is impossible. Please change 'toPath' to the correct path, or to a non-existing path, and try again",
+                        state.store->printStorePath(*toPath)),
+                    .errPos = state.positions[pos]
+                });
+            } else {
+                // We just checked toPath, but we report fromPath, because that's what the user certainly passed.
+                assert (toPath == fromPath);
+                throw Error({
+                    .msg = hintfmt("The 'fromPath' value '%s' is input addressed, but input addressing was not requested. If you do intend to return an input addressed store path, add 'inputAddressed = true;' to the 'fetchClosure' arguments. Note that content addressing does not require users to configure a trusted binary cache public key on their systems, and is therefore preferred.",
+                        state.store->printStorePath(*fromPath)),
+                    .errPos = state.positions[pos]
+                });
+            }
+        }
     }
 
     state.mkStorePathString(*toPath, v);
@@ -138,7 +167,7 @@ static RegisterPrimOp primop_fetchClosure({
       `/nix/store/ldbh...`.
 
       If `fromPath` is already content-addressed, or if you are
-      allowing impure evaluation (`--impure`), then `toPath` may be
+      allowing input addressing (`inputAddressed = true;`), then `toPath` may be
       omitted.
 
       To find out the correct value for `toPath` given a `fromPath`,
@@ -153,8 +182,11 @@ static RegisterPrimOp primop_fetchClosure({
       allows you to use a previously built store path in a Nix
       expression. However, it is more reproducible because it requires
       specifying a binary cache from which the path can be fetched.
-      Also, requiring a content-addressed final store path avoids the
-      need for users to configure binary cache public keys.
+      Also, the default requirement of a content-addressed final store path
+      avoids the need for users to configure [`trusted-public-keys`](@docroot@/command-ref/conf-file.md#conf-trusted-public-keys).
+
+      This function is only available if you enable the experimental
+      feature `fetch-closure`.
     )",
     .fun = prim_fetchClosure,
     .experimentalFeature = Xp::FetchClosure,
diff --git a/tests/fetchClosure.sh b/tests/fetchClosure.sh
index 21650eb06..4b8198e94 100644
--- a/tests/fetchClosure.sh
+++ b/tests/fetchClosure.sh
@@ -35,16 +35,28 @@ clearStore
 
 if [[ "$NIX_REMOTE" != "daemon" ]]; then
 
-    # In impure mode, we can use non-CA paths.
-    [[ $(nix eval --raw --no-require-sigs --impure --expr "
+    # We can use non-CA paths when we ask explicitly.
+    [[ $(nix eval --raw --no-require-sigs --expr "
       builtins.fetchClosure {
         fromStore = \"file://$cacheDir\";
         fromPath = $nonCaPath;
+        inputAddressed = true;
       }
     ") = $nonCaPath ]]
 
     [ -e $nonCaPath ]
 
+    # .. but only if we ask explicitly.
+    expectStderr 1 nix eval --raw --no-require-sigs --expr "
+      builtins.fetchClosure {
+        fromStore = \"file://$cacheDir\";
+        fromPath = $nonCaPath;
+      }
+    " | grepQuiet -E "The .fromPath. value .* is input addressed, but input addressing was not requested. If you do intend to return an input addressed store path, add .inputAddressed = true;. to the .fetchClosure. arguments."
+
+    [ -e $nonCaPath ]
+
+
 fi
 
 # 'toPath' set to empty string should fail but print the expected path.

From 508aa58e671583bf06fa3597629839827c8d1bd7 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Fri, 19 May 2023 16:25:23 +0200
Subject: [PATCH 04/16] fetchClosure: Always check that inputAddressed matches
 the result

---
 src/libexpr/primops/fetchClosure.cc | 42 +++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc
index 96c1df544..c66fed633 100644
--- a/src/libexpr/primops/fetchClosure.cc
+++ b/src/libexpr/primops/fetchClosure.cc
@@ -13,7 +13,7 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
     std::optional<StorePath> fromPath;
     bool enableRewriting = false;
     std::optional<StorePath> toPath;
-    bool inputAddressed = false;
+    std::optional<bool> inputAddressedMaybe;
 
     for (auto & attr : *args[0]->attrs) {
         const auto & attrName = state.symbols[attr.name];
@@ -40,7 +40,7 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
                     attrHint());
 
         else if (attrName == "inputAddressed")
-            inputAddressed = state.forceBool(*attr.value, attr.pos, attrHint());
+            inputAddressedMaybe = state.forceBool(*attr.value, attr.pos, attrHint());
 
         else
             throw Error({
@@ -55,6 +55,8 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
             .errPos = state.positions[pos]
         });
 
+    bool inputAddressed = inputAddressedMaybe.value_or(false);
+
     if (inputAddressed) {
         if (toPath && toPath != fromPath)
             throw Error({
@@ -120,15 +122,39 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
         toPath = fromPath;
     }
 
-    /* We want input addressing to be explicit, to inform readers and to give
-       expression authors an opportunity to improve their user experience. */
-    if (!inputAddressed) {
-        auto info = state.store->queryPathInfo(*toPath);
+    auto info = state.store->queryPathInfo(*toPath);
+
+    /* If inputAddressed is set, make sure the result is as expected. */
+    if (inputAddressedMaybe) {
+        bool expectCA = !inputAddressed;
+        if (info->isContentAddressed(*state.store) != expectCA) {
+            if (inputAddressed)
+                throw Error({
+                    .msg = hintfmt("The 'fetchClosure' result, '%s' is not input addressed, despite 'inputAddressed' being set to true. It is ok or even preferable to return a content addressed path, so you probably want to remove the 'inputAddressed' attribute",
+                        state.store->printStorePath(*toPath)),
+                    .errPos = state.positions[pos]
+                });
+            else
+                throw Error({
+                    .msg = hintfmt("The 'fetchClosure' result, '%s' is input addressed, despite 'inputAddressed' being set to false. Please make sure you passed the correct path(s) or set 'inputAddressed' to true if you intended to return an input addressed path",
+                        state.store->printStorePath(*toPath)),
+                    .errPos = state.positions[pos]
+                });
+        }
+    } else {
+        /*
+          While it's fine to omit the inputAddressed attribute for CA paths,
+          we want input addressing to be explicit, to inform readers and to give
+          expression authors an opportunity to improve their user experience;
+          see message below.
+        */
         if (!info->isContentAddressed(*state.store)) {
             if (enableRewriting) {
+                // We don't perform the rewriting when outPath already exists, as an optimisation.
+                // However, we can quickly detect a mistake if the toPath is input addressed.
+                // Ideally we'd compute the path for them here, but this error message is unlikely to occur in practice, so we keep it simple.
                 throw Error({
-                    // Ideally we'd compute the path for them, but this error message is unlikely to occur in practice, so we keep it simple.
-                    .msg = hintfmt("Rewriting was requested, but 'toPath' is not content addressed. This is impossible. Please change 'toPath' to the correct path, or to a non-existing path, and try again",
+                    .msg = hintfmt("The 'toPath' value '%s' is input addressed, so it can't possibly be the result of rewriting. You may set 'toPath' to an empty string to figure out the correct path.",
                         state.store->printStorePath(*toPath)),
                     .errPos = state.positions[pos]
                 });

From 8dca95386c9ab8b13886716ec31e1b2936a3ef10 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Mon, 5 Jun 2023 11:11:53 +0200
Subject: [PATCH 05/16] fetchClosure: Disallow toPath for inputAddressed = true

---
 src/libexpr/primops/fetchClosure.cc | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc
index c66fed633..6d2271853 100644
--- a/src/libexpr/primops/fetchClosure.cc
+++ b/src/libexpr/primops/fetchClosure.cc
@@ -58,12 +58,11 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
     bool inputAddressed = inputAddressedMaybe.value_or(false);
 
     if (inputAddressed) {
-        if (toPath && toPath != fromPath)
+        if (toPath)
             throw Error({
-                .msg = hintfmt("attribute '%s' is set to true, but 'toPath' does not match 'fromPath'. 'toPath' should be equal, or should be omitted. Instead 'toPath' was '%s' and 'fromPath' was '%s'",
+                .msg = hintfmt("attribute '%s' is set to true, but '%s' is also set. Please remove one of them",
                     "inputAddressed",
-                    state.store->printStorePath(*toPath),
-                    state.store->printStorePath(*fromPath)),
+                    "toPath"),
                 .errPos = state.positions[pos]
             });
         assert(!enableRewriting);

From 55888633dd323c83cd1db3773d82dc954da34888 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Mon, 5 Jun 2023 11:37:41 +0200
Subject: [PATCH 06/16] makeContentAddressed: Add single path helper

---
 src/libexpr/primops/fetchClosure.cc    | 10 ++++------
 src/libstore/make-content-addressed.cc | 11 +++++++++++
 src/libstore/make-content-addressed.hh | 13 ++++++++++++-
 3 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc
index 6d2271853..6a15c826f 100644
--- a/src/libexpr/primops/fetchClosure.cc
+++ b/src/libexpr/primops/fetchClosure.cc
@@ -94,14 +94,12 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
 
     if (enableRewriting) {
         if (!toPath || !state.store->isValidPath(*toPath)) {
-            auto remappings = makeContentAddressed(*fromStore, *state.store, { *fromPath });
-            auto i = remappings.find(*fromPath);
-            assert(i != remappings.end());
-            if (toPath && *toPath != i->second)
+            auto rewrittenPath = makeContentAddressed(*fromStore, *state.store, *fromPath);
+            if (toPath && *toPath != rewrittenPath)
                 throw Error({
                     .msg = hintfmt("rewriting '%s' to content-addressed form yielded '%s', while '%s' was expected",
                         state.store->printStorePath(*fromPath),
-                        state.store->printStorePath(i->second),
+                        state.store->printStorePath(rewrittenPath),
                         state.store->printStorePath(*toPath)),
                     .errPos = state.positions[pos]
                 });
@@ -111,7 +109,7 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
                         "rewriting '%s' to content-addressed form yielded '%s'; "
                         "please set this in the 'toPath' attribute passed to 'fetchClosure'",
                         state.store->printStorePath(*fromPath),
-                        state.store->printStorePath(i->second)),
+                        state.store->printStorePath(rewrittenPath)),
                     .errPos = state.positions[pos]
                 });
         }
diff --git a/src/libstore/make-content-addressed.cc b/src/libstore/make-content-addressed.cc
index 53fe04704..626a22480 100644
--- a/src/libstore/make-content-addressed.cc
+++ b/src/libstore/make-content-addressed.cc
@@ -80,4 +80,15 @@ std::map<StorePath, StorePath> makeContentAddressed(
     return remappings;
 }
 
+StorePath makeContentAddressed(
+    Store & srcStore,
+    Store & dstStore,
+    const StorePath & fromPath)
+{
+    auto remappings = makeContentAddressed(srcStore, dstStore, StorePathSet { fromPath });
+    auto i = remappings.find(fromPath);
+    assert(i != remappings.end());
+    return i->second;
+}
+
 }
diff --git a/src/libstore/make-content-addressed.hh b/src/libstore/make-content-addressed.hh
index 2ce6ec7bc..60bb2b477 100644
--- a/src/libstore/make-content-addressed.hh
+++ b/src/libstore/make-content-addressed.hh
@@ -5,9 +5,20 @@
 
 namespace nix {
 
+/** Rewrite a closure of store paths to be completely content addressed.
+ */
 std::map<StorePath, StorePath> makeContentAddressed(
     Store & srcStore,
     Store & dstStore,
-    const StorePathSet & storePaths);
+    const StorePathSet & rootPaths);
+
+/** Rewrite a closure of a store path to be completely content addressed.
+ *
+ * This is a convenience function for the case where you only have one root path.
+ */
+StorePath makeContentAddressed(
+    Store & srcStore,
+    Store & dstStore,
+    const StorePath & rootPath);
 
 }

From 5bdca46117c701cd1cd47700c755367442537157 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Mon, 5 Jun 2023 12:35:03 +0200
Subject: [PATCH 07/16] fetchClosure: Split into three cases

---
 src/libexpr/primops/fetchClosure.cc | 177 ++++++++++++++++------------
 1 file changed, 101 insertions(+), 76 deletions(-)

diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc
index 6a15c826f..e8c32de96 100644
--- a/src/libexpr/primops/fetchClosure.cc
+++ b/src/libexpr/primops/fetchClosure.cc
@@ -5,6 +5,101 @@
 
 namespace nix {
 
+/**
+ * Handler for the content addressed case.
+ *
+ * @param state The evaluator state and store to write to.
+ * @param fromStore The store containing the path to rewrite.
+ * @param fromPath The source path to be rewritten.
+ * @param toPathMaybe The path to write the rewritten path to. If empty, the error shows the actual path.
+ * @param v The return `Value`
+ */
+static void runFetchClosureWithRewrite(EvalState & state, const PosIdx pos, Store & fromStore, const StorePath & fromPath, const std::optional<StorePath> & toPathMaybe, Value &v) {
+
+    // establish toPath or throw
+
+    if (!toPathMaybe || !state.store->isValidPath(*toPathMaybe)) {
+        auto rewrittenPath = makeContentAddressed(fromStore, *state.store, fromPath);
+        if (toPathMaybe && *toPathMaybe != rewrittenPath)
+            throw Error({
+                .msg = hintfmt("rewriting '%s' to content-addressed form yielded '%s', while '%s' was expected",
+                    state.store->printStorePath(fromPath),
+                    state.store->printStorePath(rewrittenPath),
+                    state.store->printStorePath(*toPathMaybe)),
+                .errPos = state.positions[pos]
+            });
+        if (!toPathMaybe)
+            throw Error({
+                .msg = hintfmt(
+                    "rewriting '%s' to content-addressed form yielded '%s'; "
+                    "please set this in the 'toPath' attribute passed to 'fetchClosure'",
+                    state.store->printStorePath(fromPath),
+                    state.store->printStorePath(rewrittenPath)),
+                .errPos = state.positions[pos]
+            });
+    }
+
+    auto toPath = *toPathMaybe;
+
+    // check and return
+
+    auto resultInfo = state.store->queryPathInfo(toPath);
+
+    if (!resultInfo->isContentAddressed(*state.store)) {
+        // We don't perform the rewriting when outPath already exists, as an optimisation.
+        // However, we can quickly detect a mistake if the toPath is input addressed.
+        throw Error({
+            .msg = hintfmt("The 'toPath' value '%s' is input addressed, so it can't possibly be the result of rewriting. You may set 'toPath' to an empty string to figure out the correct path.",
+                state.store->printStorePath(toPath)),
+            .errPos = state.positions[pos]
+        });
+    }
+
+    state.mkStorePathString(toPath, v);
+}
+
+/**
+ * Fetch the closure and make sure it's content addressed.
+ */
+static void runFetchClosureWithContentAddressedPath(EvalState & state, const PosIdx pos, Store & fromStore, const StorePath & fromPath, Value & v) {
+
+    if (!state.store->isValidPath(fromPath))
+        copyClosure(fromStore, *state.store, RealisedPath::Set { fromPath });
+
+    auto info = state.store->queryPathInfo(fromPath);
+
+    if (!info->isContentAddressed(*state.store)) {
+        throw Error({
+            .msg = hintfmt("The 'fromPath' value '%s' is input addressed, but input addressing was not requested. If you do intend to return an input addressed store path, add 'inputAddressed = true;' to the 'fetchClosure' arguments. Note that content addressing does not require users to configure a trusted binary cache public key on their systems, and is therefore preferred.",
+                state.store->printStorePath(fromPath)),
+            .errPos = state.positions[pos]
+        });
+    }
+
+    state.mkStorePathString(fromPath, v);
+}
+
+/**
+ * Fetch the closure and make sure it's input addressed.
+ */
+static void runFetchClosureWithInputAddressedPath(EvalState & state, const PosIdx pos, Store & fromStore, const StorePath & fromPath, Value & v) {
+
+    if (!state.store->isValidPath(fromPath))
+        copyClosure(fromStore, *state.store, RealisedPath::Set { fromPath });
+
+    auto info = state.store->queryPathInfo(fromPath);
+
+    if (info->isContentAddressed(*state.store)) {
+        throw Error({
+            .msg = hintfmt("The 'fetchClosure' result, '%s' is not input addressed, despite 'inputAddressed' being set to true. It is preferable to return a content addressed path, so remove the 'inputAddressed' attribute to ensure content addressing is used in the future",
+                state.store->printStorePath(fromPath)),
+            .errPos = state.positions[pos]
+        });
+    }
+
+    state.mkStorePathString(fromPath, v);
+}
+
 static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
     state.forceAttrs(*args[0], pos, "while evaluating the argument passed to builtins.fetchClosure");
@@ -92,82 +187,12 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
 
     auto fromStore = openStore(parsedURL.to_string());
 
-    if (enableRewriting) {
-        if (!toPath || !state.store->isValidPath(*toPath)) {
-            auto rewrittenPath = makeContentAddressed(*fromStore, *state.store, *fromPath);
-            if (toPath && *toPath != rewrittenPath)
-                throw Error({
-                    .msg = hintfmt("rewriting '%s' to content-addressed form yielded '%s', while '%s' was expected",
-                        state.store->printStorePath(*fromPath),
-                        state.store->printStorePath(rewrittenPath),
-                        state.store->printStorePath(*toPath)),
-                    .errPos = state.positions[pos]
-                });
-            if (!toPath)
-                throw Error({
-                    .msg = hintfmt(
-                        "rewriting '%s' to content-addressed form yielded '%s'; "
-                        "please set this in the 'toPath' attribute passed to 'fetchClosure'",
-                        state.store->printStorePath(*fromPath),
-                        state.store->printStorePath(rewrittenPath)),
-                    .errPos = state.positions[pos]
-                });
-        }
-    } else {
-        if (!state.store->isValidPath(*fromPath))
-            copyClosure(*fromStore, *state.store, RealisedPath::Set { *fromPath });
-        toPath = fromPath;
-    }
-
-    auto info = state.store->queryPathInfo(*toPath);
-
-    /* If inputAddressed is set, make sure the result is as expected. */
-    if (inputAddressedMaybe) {
-        bool expectCA = !inputAddressed;
-        if (info->isContentAddressed(*state.store) != expectCA) {
-            if (inputAddressed)
-                throw Error({
-                    .msg = hintfmt("The 'fetchClosure' result, '%s' is not input addressed, despite 'inputAddressed' being set to true. It is ok or even preferable to return a content addressed path, so you probably want to remove the 'inputAddressed' attribute",
-                        state.store->printStorePath(*toPath)),
-                    .errPos = state.positions[pos]
-                });
-            else
-                throw Error({
-                    .msg = hintfmt("The 'fetchClosure' result, '%s' is input addressed, despite 'inputAddressed' being set to false. Please make sure you passed the correct path(s) or set 'inputAddressed' to true if you intended to return an input addressed path",
-                        state.store->printStorePath(*toPath)),
-                    .errPos = state.positions[pos]
-                });
-        }
-    } else {
-        /*
-          While it's fine to omit the inputAddressed attribute for CA paths,
-          we want input addressing to be explicit, to inform readers and to give
-          expression authors an opportunity to improve their user experience;
-          see message below.
-        */
-        if (!info->isContentAddressed(*state.store)) {
-            if (enableRewriting) {
-                // We don't perform the rewriting when outPath already exists, as an optimisation.
-                // However, we can quickly detect a mistake if the toPath is input addressed.
-                // Ideally we'd compute the path for them here, but this error message is unlikely to occur in practice, so we keep it simple.
-                throw Error({
-                    .msg = hintfmt("The 'toPath' value '%s' is input addressed, so it can't possibly be the result of rewriting. You may set 'toPath' to an empty string to figure out the correct path.",
-                        state.store->printStorePath(*toPath)),
-                    .errPos = state.positions[pos]
-                });
-            } else {
-                // We just checked toPath, but we report fromPath, because that's what the user certainly passed.
-                assert (toPath == fromPath);
-                throw Error({
-                    .msg = hintfmt("The 'fromPath' value '%s' is input addressed, but input addressing was not requested. If you do intend to return an input addressed store path, add 'inputAddressed = true;' to the 'fetchClosure' arguments. Note that content addressing does not require users to configure a trusted binary cache public key on their systems, and is therefore preferred.",
-                        state.store->printStorePath(*fromPath)),
-                    .errPos = state.positions[pos]
-                });
-            }
-        }
-    }
-
-    state.mkStorePathString(*toPath, v);
+    if (enableRewriting)
+        runFetchClosureWithRewrite(state, pos, *fromStore, *fromPath, toPath, v);
+    else if (inputAddressed)
+        runFetchClosureWithInputAddressedPath(state, pos, *fromStore, *fromPath, v);
+    else
+        runFetchClosureWithContentAddressedPath(state, pos, *fromStore, *fromPath, v);
 }
 
 static RegisterPrimOp primop_fetchClosure({

From dc79636007ff6982300980ba7eb1c2be3e45bece Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Mon, 5 Jun 2023 12:55:58 +0200
Subject: [PATCH 08/16] fetchClosure: Refactor: replace enableRewriting

A single variable is nice and self-contained.
---
 src/libexpr/primops/fetchClosure.cc | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc
index e8c32de96..e048a2629 100644
--- a/src/libexpr/primops/fetchClosure.cc
+++ b/src/libexpr/primops/fetchClosure.cc
@@ -100,14 +100,15 @@ static void runFetchClosureWithInputAddressedPath(EvalState & state, const PosId
     state.mkStorePathString(fromPath, v);
 }
 
+typedef std::optional<StorePath> StorePathOrGap;
+
 static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
     state.forceAttrs(*args[0], pos, "while evaluating the argument passed to builtins.fetchClosure");
 
     std::optional<std::string> fromStoreUrl;
     std::optional<StorePath> fromPath;
-    bool enableRewriting = false;
-    std::optional<StorePath> toPath;
+    std::optional<StorePathOrGap> toPath;
     std::optional<bool> inputAddressedMaybe;
 
     for (auto & attr : *args[0]->attrs) {
@@ -123,8 +124,11 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
 
         else if (attrName == "toPath") {
             state.forceValue(*attr.value, attr.pos);
-            enableRewriting = true;
-            if (attr.value->type() != nString || attr.value->string.s != std::string("")) {
+            bool isEmptyString = attr.value->type() == nString && attr.value->string.s == std::string("");
+            if (isEmptyString) {
+                toPath = StorePathOrGap {};
+            }
+            else {
                 NixStringContext context;
                 toPath = state.coerceToStorePath(attr.pos, *attr.value, context, attrHint());
             }
@@ -160,7 +164,6 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
                     "toPath"),
                 .errPos = state.positions[pos]
             });
-        assert(!enableRewriting);
     }
 
     if (!fromStoreUrl)
@@ -187,8 +190,8 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
 
     auto fromStore = openStore(parsedURL.to_string());
 
-    if (enableRewriting)
-        runFetchClosureWithRewrite(state, pos, *fromStore, *fromPath, toPath, v);
+    if (toPath)
+        runFetchClosureWithRewrite(state, pos, *fromStore, *fromPath, *toPath, v);
     else if (inputAddressed)
         runFetchClosureWithInputAddressedPath(state, pos, *fromStore, *fromPath, v);
     else

From 32c69e2b17ad7bd9721b584c3733629fe6d8c7f7 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Mon, 5 Jun 2023 16:25:02 +0200
Subject: [PATCH 09/16] doc: Typo

---
 doc/manual/src/language/builtins-prefix.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/manual/src/language/builtins-prefix.md b/doc/manual/src/language/builtins-prefix.md
index 35e3dccc3..7b2321466 100644
--- a/doc/manual/src/language/builtins-prefix.md
+++ b/doc/manual/src/language/builtins-prefix.md
@@ -3,7 +3,7 @@
 This section lists the functions built into the Nix language evaluator.
 All built-in functions are available through the global [`builtins`](./builtin-constants.md#builtins-builtins) constant.
 
-For convenience, some built-ins are can be accessed directly:
+For convenience, some built-ins can be accessed directly:
 
 - [`derivation`](#builtins-derivation)
 - [`import`](#builtins-import)

From 50de11d662ced6bfef792af54ad57553f1c3208a Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Mon, 5 Jun 2023 16:25:22 +0200
Subject: [PATCH 10/16] doc: Improve `fetchClosure` documentation

---
 doc/manual/src/release-notes/rl-next.md |  2 +
 src/libexpr/primops.cc                  |  2 +
 src/libexpr/primops/fetchClosure.cc     | 66 +++++++++++++++++++------
 3 files changed, 55 insertions(+), 15 deletions(-)

diff --git a/doc/manual/src/release-notes/rl-next.md b/doc/manual/src/release-notes/rl-next.md
index 8479b166a..e64839fa2 100644
--- a/doc/manual/src/release-notes/rl-next.md
+++ b/doc/manual/src/release-notes/rl-next.md
@@ -2,5 +2,7 @@
 
 - [`nix-channel`](../command-ref/nix-channel.md) now supports a `--list-generations` subcommand
 
+- The function [`builtins.fetchClosure`](../language/builtins.md#builtins-fetchClosure) can now fetch input-addressed paths in [pure mode](../command-ref/conf-file.md#conf-pure-eval).
+
 - Nix now allows unprivileged/[`allowed-users`](../command-ref/conf-file.md#conf-allowed-users) to sign paths.
   Previously, only [`trusted-users`](../command-ref/conf-file.md#conf-trusted-users) users could sign paths.
diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc
index 5dfad470a..f4bdf8fc6 100644
--- a/src/libexpr/primops.cc
+++ b/src/libexpr/primops.cc
@@ -1502,6 +1502,8 @@ static RegisterPrimOp primop_storePath({
       in a new path (e.g. `/nix/store/ld01dnzc…-source-source`).
 
       Not available in [pure evaluation mode](@docroot@/command-ref/conf-file.md#conf-pure-eval).
+
+      See also [`builtins.fetchClosure`](#builtins-fetchClosure).
     )",
     .fun = prim_storePath,
 });
diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc
index e048a2629..e344c9513 100644
--- a/src/libexpr/primops/fetchClosure.cc
+++ b/src/libexpr/primops/fetchClosure.cc
@@ -202,40 +202,76 @@ static RegisterPrimOp primop_fetchClosure({
     .name = "__fetchClosure",
     .args = {"args"},
     .doc = R"(
-      Fetch a Nix store closure from a binary cache, rewriting it into
-      content-addressed form. For example,
+      Fetch a Nix store closure from a binary cache.
 
-      ```nix
-      builtins.fetchClosure {
-        fromStore = "https://cache.nixos.org";
-        fromPath = /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1;
-        toPath = /nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1;
-      }
-      ```
+      This function can be used in three ways:
 
-      fetches `/nix/store/r2jd...` from the specified binary cache,
+      - Fetch any store path and rewrite it to a fully content-addressed store path. Example:
+
+        ```nix
+        builtins.fetchClosure {
+            fromStore = "https://cache.nixos.org";
+            fromPath = /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1;
+            toPath = /nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1;
+        }
+        ```
+
+      - Fetch a content-addressed store path.
+
+        ```nix
+        builtins.fetchClosure {
+            fromStore = "https://cache.nixos.org";
+            fromPath = /nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1;
+        }
+        ```
+
+      - Fetch an [input-addressed store path](@docroot@/glossary.md#gloss-input-addressed-store-object) as is. This depends on user configuration, which is less preferable.
+
+        ```nix
+        builtins.fetchClosure {
+            fromStore = "https://cache.nixos.org";
+            fromPath = /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1;
+            inputAddressed = true;
+        }
+        ```
+
+      **Rewriting a store path**
+
+      This first example fetches `/nix/store/r2jd...` from the specified binary cache,
       and rewrites it into the content-addressed store path
       `/nix/store/ldbh...`.
 
-      If `fromPath` is already content-addressed, or if you are
-      allowing input addressing (`inputAddressed = true;`), then `toPath` may be
-      omitted.
+      By rewriting the store paths, you can add the contents to any store without requiring that you or perhaps your users configure any extra trust.
 
       To find out the correct value for `toPath` given a `fromPath`,
-      you can use `nix store make-content-addressed`:
+      you can use [`nix store make-content-addressed`](@docroot@/command-ref/new-cli/nix3-store-make-content-addressed.md):
 
       ```console
       # nix store make-content-addressed --from https://cache.nixos.org /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1
       rewrote '/nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1' to '/nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1'
       ```
 
-      This function is similar to `builtins.storePath` in that it
+      Alternatively, you may set `toPath = ""` and find the correct `toPath` in the error message.
+
+      **Not rewriting**
+
+      `toPath` may be omitted when either
+       - `fromPath` is already content-addressed, or
+       - `inputAddressed = true;` is set.
+
+      Fetching an [input addressed path](@docroot@/glossary.md#gloss-input-addressed-store-object) is not recommended because it requires that the source be trusted by the Nix configuration.
+
+      **`builtins.storePath`**
+
+      This function is similar to [`builtins.storePath`](#builtins-storePath) in that it
       allows you to use a previously built store path in a Nix
       expression. However, it is more reproducible because it requires
       specifying a binary cache from which the path can be fetched.
       Also, the default requirement of a content-addressed final store path
       avoids the need for users to configure [`trusted-public-keys`](@docroot@/command-ref/conf-file.md#conf-trusted-public-keys).
 
+      **Experimental feature**
+
       This function is only available if you enable the experimental
       feature `fetch-closure`.
     )",

From 40052c76132d79561aa50ec9349ad22b51c64505 Mon Sep 17 00:00:00 2001
From: Robert Hensing <roberth@users.noreply.github.com>
Date: Wed, 7 Jun 2023 12:47:18 +0200
Subject: [PATCH 11/16] fetchClosure: Docs and error message improvements

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
---
 doc/manual/src/release-notes/rl-next.md |  2 +-
 src/libexpr/primops/fetchClosure.cc     | 38 ++++++++++++++++---------
 tests/fetchClosure.sh                   |  2 +-
 3 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/doc/manual/src/release-notes/rl-next.md b/doc/manual/src/release-notes/rl-next.md
index e64839fa2..139d07188 100644
--- a/doc/manual/src/release-notes/rl-next.md
+++ b/doc/manual/src/release-notes/rl-next.md
@@ -2,7 +2,7 @@
 
 - [`nix-channel`](../command-ref/nix-channel.md) now supports a `--list-generations` subcommand
 
-- The function [`builtins.fetchClosure`](../language/builtins.md#builtins-fetchClosure) can now fetch input-addressed paths in [pure mode](../command-ref/conf-file.md#conf-pure-eval).
+* The function [`builtins.fetchClosure`](../language/builtins.md#builtins-fetchClosure) can now fetch input-addressed paths in [pure evaluation mode](../command-ref/conf-file.md#conf-pure-eval), as those are not impure.
 
 - Nix now allows unprivileged/[`allowed-users`](../command-ref/conf-file.md#conf-allowed-users) to sign paths.
   Previously, only [`trusted-users`](../command-ref/conf-file.md#conf-trusted-users) users could sign paths.
diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc
index e344c9513..5114df553 100644
--- a/src/libexpr/primops/fetchClosure.cc
+++ b/src/libexpr/primops/fetchClosure.cc
@@ -8,11 +8,11 @@ namespace nix {
 /**
  * Handler for the content addressed case.
  *
- * @param state The evaluator state and store to write to.
- * @param fromStore The store containing the path to rewrite.
- * @param fromPath The source path to be rewritten.
- * @param toPathMaybe The path to write the rewritten path to. If empty, the error shows the actual path.
- * @param v The return `Value`
+ * @param state Evaluator state and store to write to.
+ * @param fromStore Store containing the path to rewrite.
+ * @param fromPath Source path to be rewritten.
+ * @param toPathMaybe Path to write the rewritten path to. If empty, the error shows the actual path.
+ * @param v Return `Value`
  */
 static void runFetchClosureWithRewrite(EvalState & state, const PosIdx pos, Store & fromStore, const StorePath & fromPath, const std::optional<StorePath> & toPathMaybe, Value &v) {
 
@@ -31,8 +31,8 @@ static void runFetchClosureWithRewrite(EvalState & state, const PosIdx pos, Stor
         if (!toPathMaybe)
             throw Error({
                 .msg = hintfmt(
-                    "rewriting '%s' to content-addressed form yielded '%s'; "
-                    "please set this in the 'toPath' attribute passed to 'fetchClosure'",
+                    "rewriting '%s' to content-addressed form yielded '%s'\n"
+                    "Use this value for the 'toPath' attribute passed to 'fetchClosure'",
                     state.store->printStorePath(fromPath),
                     state.store->printStorePath(rewrittenPath)),
                 .errPos = state.positions[pos]
@@ -49,7 +49,9 @@ static void runFetchClosureWithRewrite(EvalState & state, const PosIdx pos, Stor
         // We don't perform the rewriting when outPath already exists, as an optimisation.
         // However, we can quickly detect a mistake if the toPath is input addressed.
         throw Error({
-            .msg = hintfmt("The 'toPath' value '%s' is input addressed, so it can't possibly be the result of rewriting. You may set 'toPath' to an empty string to figure out the correct path.",
+            .msg = hintfmt(
+                "The 'toPath' value '%s' is input-addressed, so it can't possibly be the result of rewriting to a content-addressed path.\n\n"
+                "Set 'toPath' to an empty string to make Nix report the correct content-addressed path.",
                 state.store->printStorePath(toPath)),
             .errPos = state.positions[pos]
         });
@@ -70,7 +72,9 @@ static void runFetchClosureWithContentAddressedPath(EvalState & state, const Pos
 
     if (!info->isContentAddressed(*state.store)) {
         throw Error({
-            .msg = hintfmt("The 'fromPath' value '%s' is input addressed, but input addressing was not requested. If you do intend to return an input addressed store path, add 'inputAddressed = true;' to the 'fetchClosure' arguments. Note that content addressing does not require users to configure a trusted binary cache public key on their systems, and is therefore preferred.",
+            .msg = hintfmt(
+                "The 'fromPath' value '%s' is input-addressed, but 'inputAddressed' is set to 'false' (default).\n\n"
+                "If you do intend to fetch an input-addressed store path, add 'inputAddressed = true;' to the 'fetchClosure' arguments. Note that content addressing does not require users to configure a trusted binary cache public key on their systems, and is therefore preferred.",
                 state.store->printStorePath(fromPath)),
             .errPos = state.positions[pos]
         });
@@ -91,7 +95,9 @@ static void runFetchClosureWithInputAddressedPath(EvalState & state, const PosId
 
     if (info->isContentAddressed(*state.store)) {
         throw Error({
-            .msg = hintfmt("The 'fetchClosure' result, '%s' is not input addressed, despite 'inputAddressed' being set to true. It is preferable to return a content addressed path, so remove the 'inputAddressed' attribute to ensure content addressing is used in the future",
+            .msg = hintfmt(
+                "The store object referred to by 'fromPath' at '%s' is not input-addressed, but 'inputAddressed' is set to 'true'.\n\n"
+                "Remove the 'inputAddressed' attribute (it defaults to 'false') to expect 'fromPath' to be content-addressed",
                 state.store->printStorePath(fromPath)),
             .errPos = state.positions[pos]
         });
@@ -202,11 +208,13 @@ static RegisterPrimOp primop_fetchClosure({
     .name = "__fetchClosure",
     .args = {"args"},
     .doc = R"(
-      Fetch a Nix store closure from a binary cache.
+      Fetch a store path [closure](@docroot@/glossary.md#gloss-closure) from a binary cache.
 
       This function can be used in three ways:
 
-      - Fetch any store path and rewrite it to a fully content-addressed store path. Example:
+      - Fetch any store path and rewrite it to a fully content-addressed store path.
+
+        Example:
 
         ```nix
         builtins.fetchClosure {
@@ -218,6 +226,8 @@ static RegisterPrimOp primop_fetchClosure({
 
       - Fetch a content-addressed store path.
 
+Example:
+
         ```nix
         builtins.fetchClosure {
             fromStore = "https://cache.nixos.org";
@@ -227,6 +237,8 @@ static RegisterPrimOp primop_fetchClosure({
 
       - Fetch an [input-addressed store path](@docroot@/glossary.md#gloss-input-addressed-store-object) as is. This depends on user configuration, which is less preferable.
 
+        Example:
+
         ```nix
         builtins.fetchClosure {
             fromStore = "https://cache.nixos.org";
@@ -244,7 +256,7 @@ static RegisterPrimOp primop_fetchClosure({
       By rewriting the store paths, you can add the contents to any store without requiring that you or perhaps your users configure any extra trust.
 
       To find out the correct value for `toPath` given a `fromPath`,
-      you can use [`nix store make-content-addressed`](@docroot@/command-ref/new-cli/nix3-store-make-content-addressed.md):
+      use [`nix store make-content-addressed`](@docroot@/command-ref/new-cli/nix3-store-make-content-addressed.md):
 
       ```console
       # nix store make-content-addressed --from https://cache.nixos.org /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1
diff --git a/tests/fetchClosure.sh b/tests/fetchClosure.sh
index 4b8198e94..0f1c64372 100644
--- a/tests/fetchClosure.sh
+++ b/tests/fetchClosure.sh
@@ -52,7 +52,7 @@ if [[ "$NIX_REMOTE" != "daemon" ]]; then
         fromStore = \"file://$cacheDir\";
         fromPath = $nonCaPath;
       }
-    " | grepQuiet -E "The .fromPath. value .* is input addressed, but input addressing was not requested. If you do intend to return an input addressed store path, add .inputAddressed = true;. to the .fetchClosure. arguments."
+    " | grepQuiet -E "The .fromPath. value .* is input-addressed, but .inputAddressed. is set to .false."
 
     [ -e $nonCaPath ]
 

From 1db81f710725eb83597b1761076e9ca28317a52e Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Mon, 19 Jun 2023 23:22:37 +0200
Subject: [PATCH 12/16] tests/fetchClosure: Improve coverage of new and some
 existing flows

---
 tests/fetchClosure.sh | 79 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 69 insertions(+), 10 deletions(-)

diff --git a/tests/fetchClosure.sh b/tests/fetchClosure.sh
index 0f1c64372..a02d1ce7a 100644
--- a/tests/fetchClosure.sh
+++ b/tests/fetchClosure.sh
@@ -33,8 +33,26 @@ clearStore
 [ ! -e $nonCaPath ]
 [ -e $caPath ]
 
+clearStore
+
+# The daemon will reject input addressed paths unless configured to trust the
+# cache key or the user. This behavior should be covered by another test, so we
+# skip this part when using the daemon.
 if [[ "$NIX_REMOTE" != "daemon" ]]; then
 
+    # If we want to return a non-CA path, we have to be explicit about it.
+    expectStderr 1 nix eval --raw --no-require-sigs --expr "
+      builtins.fetchClosure {
+        fromStore = \"file://$cacheDir\";
+        fromPath = $nonCaPath;
+      }
+    " | grepQuiet -E "The .fromPath. value .* is input-addressed, but .inputAddressed. is set to .false."
+
+    # TODO: Should the closure be rejected, despite single user mode?
+    # [ ! -e $nonCaPath ]
+
+    [ ! -e $caPath ]
+
     # We can use non-CA paths when we ask explicitly.
     [[ $(nix eval --raw --no-require-sigs --expr "
       builtins.fetchClosure {
@@ -45,20 +63,13 @@ if [[ "$NIX_REMOTE" != "daemon" ]]; then
     ") = $nonCaPath ]]
 
     [ -e $nonCaPath ]
-
-    # .. but only if we ask explicitly.
-    expectStderr 1 nix eval --raw --no-require-sigs --expr "
-      builtins.fetchClosure {
-        fromStore = \"file://$cacheDir\";
-        fromPath = $nonCaPath;
-      }
-    " | grepQuiet -E "The .fromPath. value .* is input-addressed, but .inputAddressed. is set to .false."
-
-    [ -e $nonCaPath ]
+    [ ! -e $caPath ]
 
 
 fi
 
+[ ! -e $caPath ]
+
 # 'toPath' set to empty string should fail but print the expected path.
 expectStderr 1 nix eval -v --json --expr "
   builtins.fetchClosure {
@@ -71,6 +82,10 @@ expectStderr 1 nix eval -v --json --expr "
 # If fromPath is CA, then toPath isn't needed.
 nix copy --to file://$cacheDir $caPath
 
+clearStore
+
+[ ! -e $caPath ]
+
 [[ $(nix eval -v --raw --expr "
   builtins.fetchClosure {
     fromStore = \"file://$cacheDir\";
@@ -78,6 +93,8 @@ nix copy --to file://$cacheDir $caPath
   }
 ") = $caPath ]]
 
+[ -e $caPath ]
+
 # Check that URL query parameters aren't allowed.
 clearStore
 narCache=$TEST_ROOT/nar-cache
@@ -89,3 +106,45 @@ rm -rf $narCache
   }
 ")
 (! [ -e $narCache ])
+
+# If toPath is specified but wrong, we check it (only) when the path is missing.
+clearStore
+
+badPath=$(echo $caPath | sed -e 's!/store/................................-!/store/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx-!')
+
+[ ! -e $badPath ]
+
+expectStderr 1 nix eval -v --raw --expr "
+  builtins.fetchClosure {
+    fromStore = \"file://$cacheDir\";
+    fromPath = $nonCaPath;
+    toPath = $badPath;
+  }
+" | grep "error: rewriting.*$nonCaPath.*yielded.*$caPath.*while.*$badPath.*was expected"
+
+[ ! -e $badPath ]
+
+# We only check it when missing, as a performance optimization similar to what we do for fixed output derivations. So if it's already there, we don't check it.
+# It would be nice for this to fail, but checking it would be too(?) slow.
+[ -e $caPath ]
+
+[[ $(nix eval -v --raw --expr "
+  builtins.fetchClosure {
+    fromStore = \"file://$cacheDir\";
+    fromPath = $badPath;
+    toPath = $caPath;
+  }
+") = $caPath ]]
+
+
+# However, if the output address is unexpected, we can report it
+
+
+expectStderr 1 nix eval -v --raw --expr "
+  builtins.fetchClosure {
+    fromStore = \"file://$cacheDir\";
+    fromPath = $caPath;
+    inputAddressed = true;
+  }
+" | grepQuiet 'error.*The store object referred to by.*fromPath.* at .* is not input-addressed, but .*inputAddressed.* is set to .*true.*'
+

From fefb94713295d63d30a8bba257c41c3eb54e0998 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Mon, 19 Jun 2023 23:22:56 +0200
Subject: [PATCH 13/16] tests/signing.sh: Check signature checking error
 message

We should check error messages, so that we know the command fails for
the right reason.
Alternatively, a mere typo can run the test undetected.
---
 tests/signing.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/signing.sh b/tests/signing.sh
index 9b673c609..f14b53e7f 100644
--- a/tests/signing.sh
+++ b/tests/signing.sh
@@ -84,7 +84,7 @@ info=$(nix path-info --store file://$cacheDir --json $outPath2)
 # Copying to a diverted store should fail due to a lack of signatures by trusted keys.
 chmod -R u+w $TEST_ROOT/store0 || true
 rm -rf $TEST_ROOT/store0
-(! nix copy --to $TEST_ROOT/store0 $outPath)
+expectStderr 1 nix copy --to $TEST_ROOT/store0 $outPath | grepQuiet -E 'cannot add path .* because it lacks a signature by a trusted key'
 
 # But succeed if we supply the public keys.
 nix copy --to $TEST_ROOT/store0 $outPath --trusted-public-keys $pk1

From 537e8beb770ed92dd1d5a20796a86620c4c61389 Mon Sep 17 00:00:00 2001
From: Robert Hensing <roberth@users.noreply.github.com>
Date: Fri, 7 Jul 2023 11:00:40 +0200
Subject: [PATCH 14/16] fetchClosure: Apply suggestions from code review

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
---
 src/libexpr/primops/fetchClosure.cc | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc
index 5114df553..22bae19b7 100644
--- a/src/libexpr/primops/fetchClosure.cc
+++ b/src/libexpr/primops/fetchClosure.cc
@@ -74,7 +74,10 @@ static void runFetchClosureWithContentAddressedPath(EvalState & state, const Pos
         throw Error({
             .msg = hintfmt(
                 "The 'fromPath' value '%s' is input-addressed, but 'inputAddressed' is set to 'false' (default).\n\n"
-                "If you do intend to fetch an input-addressed store path, add 'inputAddressed = true;' to the 'fetchClosure' arguments. Note that content addressing does not require users to configure a trusted binary cache public key on their systems, and is therefore preferred.",
+                "If you do intend to fetch an input-addressed store path, add\n\n"
+                "    inputAddressed = true;\n\n"
+                "to the 'fetchClosure' arguments.\n\n"
+                "Note that to ensure authenticity input-addressed store paths, users must configure a trusted binary cache public key on their systems. This is not needed for content-addressed paths.",
                 state.store->printStorePath(fromPath)),
             .errPos = state.positions[pos]
         });
@@ -226,7 +229,7 @@ static RegisterPrimOp primop_fetchClosure({
 
       - Fetch a content-addressed store path.
 
-Example:
+      Example:
 
         ```nix
         builtins.fetchClosure {
@@ -263,7 +266,7 @@ Example:
       rewrote '/nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1' to '/nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1'
       ```
 
-      Alternatively, you may set `toPath = ""` and find the correct `toPath` in the error message.
+      Alternatively, set `toPath = ""` and find the correct `toPath` in the error message.
 
       **Not rewriting**
 
@@ -279,13 +282,7 @@ Example:
       allows you to use a previously built store path in a Nix
       expression. However, it is more reproducible because it requires
       specifying a binary cache from which the path can be fetched.
-      Also, the default requirement of a content-addressed final store path
-      avoids the need for users to configure [`trusted-public-keys`](@docroot@/command-ref/conf-file.md#conf-trusted-public-keys).
-
-      **Experimental feature**
-
-      This function is only available if you enable the experimental
-      feature `fetch-closure`.
+      Also, using content-addressed store paths does not require users to configure [`trusted-public-keys`](@docroot@/command-ref/conf-file.md#conf-trusted-public-keys) to ensure their authenticity.
     )",
     .fun = prim_fetchClosure,
     .experimentalFeature = Xp::FetchClosure,

From b4b02d084f066d6391074ac807e532e36e4eccd9 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Fri, 7 Jul 2023 11:40:40 +0200
Subject: [PATCH 15/16] fetchClosure: Interleave the examples in the docs

---
 src/libexpr/primops/fetchClosure.cc | 79 ++++++++++++++---------------
 1 file changed, 37 insertions(+), 42 deletions(-)

diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc
index 22bae19b7..7fe8203f4 100644
--- a/src/libexpr/primops/fetchClosure.cc
+++ b/src/libexpr/primops/fetchClosure.cc
@@ -211,52 +211,42 @@ static RegisterPrimOp primop_fetchClosure({
     .name = "__fetchClosure",
     .args = {"args"},
     .doc = R"(
-      Fetch a store path [closure](@docroot@/glossary.md#gloss-closure) from a binary cache.
+      Fetch a store path [closure](@docroot@/glossary.md#gloss-closure) from a binary cache, and return the store path as a string with context.
 
-      This function can be used in three ways:
+      This function can be invoked in three ways, that we will discuss in order of preference.
 
-      - Fetch any store path and rewrite it to a fully content-addressed store path.
-
-        Example:
-
-        ```nix
-        builtins.fetchClosure {
-            fromStore = "https://cache.nixos.org";
-            fromPath = /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1;
-            toPath = /nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1;
-        }
-        ```
-
-      - Fetch a content-addressed store path.
+      **Fetch a content-addressed store path**
 
       Example:
 
-        ```nix
-        builtins.fetchClosure {
-            fromStore = "https://cache.nixos.org";
-            fromPath = /nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1;
-        }
-        ```
+      ```nix
+      builtins.fetchClosure {
+        fromStore = "https://cache.nixos.org";
+        fromPath = /nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1;
+      }
+      ```
 
-      - Fetch an [input-addressed store path](@docroot@/glossary.md#gloss-input-addressed-store-object) as is. This depends on user configuration, which is less preferable.
+      This is the simplest invocation, and it does not require the user of the expression to configure [`trusted-public-keys`](@docroot@/command-ref/conf-file.md#conf-trusted-public-keys) to ensure their authenticity.
 
-        Example:
+      If your store path is [input addressed](@docroot@/glossary.md#gloss-input-addressed-store-object) instead of content addressed, consider the other two invocations.
 
-        ```nix
-        builtins.fetchClosure {
-            fromStore = "https://cache.nixos.org";
-            fromPath = /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1;
-            inputAddressed = true;
-        }
-        ```
+      **Fetch any store path and rewrite it to a fully content-addressed store path**
 
-      **Rewriting a store path**
+      Example:
 
-      This first example fetches `/nix/store/r2jd...` from the specified binary cache,
+      ```nix
+      builtins.fetchClosure {
+        fromStore = "https://cache.nixos.org";
+        fromPath = /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1;
+        toPath = /nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1;
+      }
+      ```
+
+      This example fetches `/nix/store/r2jd...` from the specified binary cache,
       and rewrites it into the content-addressed store path
       `/nix/store/ldbh...`.
 
-      By rewriting the store paths, you can add the contents to any store without requiring that you or perhaps your users configure any extra trust.
+      Like the previous example, no extra configuration or privileges are required.
 
       To find out the correct value for `toPath` given a `fromPath`,
       use [`nix store make-content-addressed`](@docroot@/command-ref/new-cli/nix3-store-make-content-addressed.md):
@@ -268,20 +258,25 @@ static RegisterPrimOp primop_fetchClosure({
 
       Alternatively, set `toPath = ""` and find the correct `toPath` in the error message.
 
-      **Not rewriting**
+      **Fetch an input-addressed store path as is**
 
-      `toPath` may be omitted when either
-       - `fromPath` is already content-addressed, or
-       - `inputAddressed = true;` is set.
+      Example:
 
-      Fetching an [input addressed path](@docroot@/glossary.md#gloss-input-addressed-store-object) is not recommended because it requires that the source be trusted by the Nix configuration.
+      ```nix
+      builtins.fetchClosure {
+        fromStore = "https://cache.nixos.org";
+        fromPath = /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1;
+        inputAddressed = true;
+      }
+      ```
+
+      It is possible to fetch an [input-addressed store path](@docroot@/glossary.md#gloss-input-addressed-store-object) and return it as is.
+      However, this is the least preferred way of invoking `fetchClosure`, because it requires that the input-addressed paths are trusted by the Nix configuration.
 
       **`builtins.storePath`**
 
-      This function is similar to [`builtins.storePath`](#builtins-storePath) in that it
-      allows you to use a previously built store path in a Nix
-      expression. However, it is more reproducible because it requires
-      specifying a binary cache from which the path can be fetched.
+      `fetchClosure` is similar to [`builtins.storePath`](#builtins-storePath) in that it allows you to use a previously built store path in a Nix expression.
+      However, `fetchClosure` is more reproducible because it specifies a binary cache from which the path can be fetched.
       Also, using content-addressed store paths does not require users to configure [`trusted-public-keys`](@docroot@/command-ref/conf-file.md#conf-trusted-public-keys) to ensure their authenticity.
     )",
     .fun = prim_fetchClosure,

From 9fc82de49388a58240234d10893673566432f7ab Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Fri, 7 Jul 2023 15:07:30 +0200
Subject: [PATCH 16/16] signing.sh: Revert test improvement because it fails on
 GHA + macOS

---
 tests/signing.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/signing.sh b/tests/signing.sh
index f14b53e7f..942b51630 100644
--- a/tests/signing.sh
+++ b/tests/signing.sh
@@ -84,7 +84,11 @@ info=$(nix path-info --store file://$cacheDir --json $outPath2)
 # Copying to a diverted store should fail due to a lack of signatures by trusted keys.
 chmod -R u+w $TEST_ROOT/store0 || true
 rm -rf $TEST_ROOT/store0
-expectStderr 1 nix copy --to $TEST_ROOT/store0 $outPath | grepQuiet -E 'cannot add path .* because it lacks a signature by a trusted key'
+
+# Fails or very flaky only on GHA + macOS:
+#     expectStderr 1 nix copy --to $TEST_ROOT/store0 $outPath | grepQuiet -E 'cannot add path .* because it lacks a signature by a trusted key'
+# but this works:
+(! nix copy --to $TEST_ROOT/store0 $outPath)
 
 # But succeed if we supply the public keys.
 nix copy --to $TEST_ROOT/store0 $outPath --trusted-public-keys $pk1