Require shallow clones to be requested explicitly

If you do a fetchTree on a Git repository, whether the result contains a revCount attribute should not depend on whether that repository happens to be a shallow clone or not. That would complicate caching a lot and would be semantically messy. So applying fetchTree/fetchGit to a shallow repository is now an error unless you pass the attribute 'shallow = true'. If 'shallow = true', we don't return revCount, even if the repository is not actually shallow. Note that Nix itself is not doing shallow clones at the moment. But it could do so as an optimisation if the user specifies 'shallow = true'. Issue #2988.
NixOS · Mar 17, 2020 · d1165d8 · d1165d8
1 parent 2a4e4f6
commit d1165d8
Show file tree

Hide file tree

Showing 7 changed files with 60 additions and 12 deletions.
diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
@@ -158,7 +158,7 @@ string showType(ValueType type)
 {
     switch (type) {
         case tInt: return "an integer";
-        case tBool: return "a boolean";
+        case tBool: return "a Boolean";
         case tString: return "a string";
         case tPath: return "a path";
         case tNull: return "null";

diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc
@@ -58,8 +58,10 @@ static void prim_fetchTree(EvalState & state, const Pos & pos, Value * * args, V
             state.forceValue(*attr.value);
             if (attr.value->type == tString)
                 attrs.emplace(attr.name, attr.value->string.s);
+            else if (attr.value->type == tBool)
+                attrs.emplace(attr.name, attr.value->boolean);
             else
-                throw TypeError("fetchTree argument '%s' is %s while a string is expected",
+                throw TypeError("fetchTree argument '%s' is %s while a string or Boolean is expected",
                     attr.name, showType(*attr.value));
         }
 

diff --git a/src/libstore/fetchers/attrs.cc b/src/libstore/fetchers/attrs.cc
@@ -14,6 +14,8 @@ Attrs jsonToAttrs(const nlohmann::json & json)
             attrs.emplace(i.key(), i.value().get<int64_t>());
         else if (i.value().is_string())
             attrs.emplace(i.key(), i.value().get<std::string>());
+        else if (i.value().is_boolean())
+            attrs.emplace(i.key(), i.value().get<bool>());
         else
             throw Error("unsupported input attribute type in lock file");
     }
@@ -29,6 +31,8 @@ nlohmann::json attrsToJson(const Attrs & attrs)
             json[attr.first] = *v;
         } else if (auto v = std::get_if<std::string>(&attr.second)) {
             json[attr.first] = *v;
+        } else if (auto v = std::get_if<Explicit<bool>>(&attr.second)) {
+            json[attr.first] = v->t;
         } else abort();
     }
     return json;
@@ -40,7 +44,7 @@ std::optional<std::string> maybeGetStrAttr(const Attrs & attrs, const std::strin
     if (i == attrs.end()) return {};
     if (auto v = std::get_if<std::string>(&i->second))
         return *v;
-    throw Error("input attribute '%s' is not a string", name);
+    throw Error("input attribute '%s' is not a string %s", name, attrsToJson(attrs).dump());
 }
 
 std::string getStrAttr(const Attrs & attrs, const std::string & name)
@@ -57,7 +61,7 @@ std::optional<int64_t> maybeGetIntAttr(const Attrs & attrs, const std::string &
     if (i == attrs.end()) return {};
     if (auto v = std::get_if<int64_t>(&i->second))
         return *v;
-    throw Error("input attribute '%s' is not a string", name);
+    throw Error("input attribute '%s' is not an integer", name);
 }
 
 int64_t getIntAttr(const Attrs & attrs, const std::string & name)
@@ -68,4 +72,21 @@ int64_t getIntAttr(const Attrs & attrs, const std::string & name)
     return *s;
 }
 
+std::optional<bool> maybeGetBoolAttr(const Attrs & attrs, const std::string & name)
+{
+    auto i = attrs.find(name);
+    if (i == attrs.end()) return {};
+    if (auto v = std::get_if<int64_t>(&i->second))
+        return *v;
+    throw Error("input attribute '%s' is not a Boolean", name);
+}
+
+bool getBoolAttr(const Attrs & attrs, const std::string & name)
+{
+    auto s = maybeGetBoolAttr(attrs, name);
+    if (!s)
+        throw Error("input attribute '%s' is missing", name);
+    return *s;
+}
+
 }
diff --git a/src/libstore/fetchers/attrs.hh b/src/libstore/fetchers/attrs.hh
@@ -8,7 +8,14 @@
 
 namespace nix::fetchers {
 
-typedef std::variant<std::string, int64_t> Attr;
+/* Wrap bools to prevent string literals (i.e. 'char *') from being
+   cast to a bool in Attr. */
+template<typename T>
+struct Explicit {
+    T t;
+};
+
+typedef std::variant<std::string, int64_t, Explicit<bool>> Attr;
 typedef std::map<std::string, Attr> Attrs;
 
 Attrs jsonToAttrs(const nlohmann::json & json);
@@ -23,4 +30,8 @@ std::optional<int64_t> maybeGetIntAttr(const Attrs & attrs, const std::string &
 
 int64_t getIntAttr(const Attrs & attrs, const std::string & name);
 
+std::optional<bool> maybeGetBoolAttr(const Attrs & attrs, const std::string & name);
+
+bool getBoolAttr(const Attrs & attrs, const std::string & name);
+
 }
diff --git a/src/libstore/fetchers/git.cc b/src/libstore/fetchers/git.cc
@@ -83,6 +83,7 @@ struct GitInput : Input
     ParsedURL url;
     std::optional<std::string> ref;
     std::optional<Hash> rev;
+    bool shallow = false;
 
     GitInput(const ParsedURL & url) : url(url)
     { }
@@ -114,6 +115,7 @@ struct GitInput : Input
         if (url2.scheme != "git") url2.scheme = "git+" + url2.scheme;
         if (rev) url2.query.insert_or_assign("rev", rev->gitRev());
         if (ref) url2.query.insert_or_assign("ref", *ref);
+        if (shallow) url2.query.insert_or_assign("shallow", "1");
         return url2.to_string();
     }
 
@@ -125,6 +127,8 @@ struct GitInput : Input
             attrs.emplace("ref", *ref);
         if (rev)
             attrs.emplace("rev", rev->gitRev());
+        if (shallow)
+            attrs.emplace("shallow", true);
         return attrs;
     }
 
@@ -361,9 +365,12 @@ struct GitInput : Input
 
         bool isShallow = chomp(runProgram("git", true, { "-C", repoDir, "rev-parse", "--is-shallow-repository" })) == "true";
 
+        if (isShallow && !shallow)
+            throw Error("'%s' is a shallow Git repository, but a non-shallow repository is needed", actualUrl);
+
         if (auto tree = lookupGitInfo(store, name, *input->rev)) {
             assert(*input->rev == tree->first);
-            if (isShallow) tree->second.info.revCount.reset();
+            if (shallow) tree->second.info.revCount.reset();
             return {std::move(tree->second), input};
         }
 
@@ -393,7 +400,7 @@ struct GitInput : Input
             .storePath = std::move(storePath),
             .info = TreeInfo {
                 .revCount =
-                    !isShallow
+                    !shallow
                     ? std::optional(std::stoull(runProgram("git", true, { "-C", repoDir, "rev-list", "--count", input->rev->gitRev() })))
                     : std::nullopt,
                 .lastModified = lastModified
@@ -440,7 +447,7 @@ struct GitInputScheme : InputScheme
         if (maybeGetStrAttr(attrs, "type") != "git") return {};
 
         for (auto & [name, value] : attrs)
-            if (name != "type" && name != "url" && name != "ref" && name != "rev")
+            if (name != "type" && name != "url" && name != "ref" && name != "rev" && name != "shallow")
                 throw Error("unsupported Git input attribute '%s'", name);
 
         auto input = std::make_unique<GitInput>(parseURL(getStrAttr(attrs, "url")));
@@ -451,6 +458,9 @@ struct GitInputScheme : InputScheme
         }
         if (auto rev = maybeGetStrAttr(attrs, "rev"))
             input->rev = Hash(*rev, htSHA1);
+
+        input->shallow = maybeGetBoolAttr(attrs, "shallow").value_or(false);
+
         return input;
     }
 };

diff --git a/src/libstore/fetchers/mercurial.cc b/src/libstore/fetchers/mercurial.cc
@@ -260,7 +260,7 @@ struct MercurialInput : Input
 
         Attrs infoAttrs({
             {"rev", input->rev->gitRev()},
-            {"revCount", revCount},
+            {"revCount", (int64_t) revCount},
         });
 
         if (!this->rev)

diff --git a/tests/fetchGit.sh b/tests/fetchGit.sh
@@ -148,8 +148,12 @@ NIX=$(command -v nix)
 path5=$(nix eval --impure --raw --expr "(builtins.fetchGit { url = $repo; ref = \"dev\"; }).outPath")
 [[ $path3 = $path5 ]]
 
-# Check that shallow clones work and don't return a revcount.
+# Fetching a shallow repo shouldn't work by default, because we can't
+# return a revCount.
 git clone --depth 1 file://$repo $TEST_ROOT/shallow
-path6=$(nix eval --impure --raw --expr "(builtins.fetchGit { url = $TEST_ROOT/shallow; ref = \"dev\"; }).outPath")
+(! nix eval --impure --raw --expr "(builtins.fetchGit { url = $TEST_ROOT/shallow; ref = \"dev\"; }).outPath")
+
+# But you can request a shallow clone, which won't return a revCount.
+path6=$(nix eval --impure --raw --expr "(builtins.fetchTree { type = \"git\"; url = \"file://$TEST_ROOT/shallow\"; ref = \"dev\"; shallow = true; }).outPath")
 [[ $path3 = $path6 ]]
-[[ $(nix eval --impure --expr "(builtins.fetchTree { type = \"git\"; url = \"file://$TEST_ROOT/shallow\"; ref = \"dev\"; }).revCount or 123") == 123 ]]
+[[ $(nix eval --impure --expr "(builtins.fetchTree { type = \"git\"; url = \"file://$TEST_ROOT/shallow\"; ref = \"dev\"; shallow = true; }).revCount or 123") == 123 ]]