From d1165d8791f559352ff6aa7348e1293b2873db1c Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 17 Mar 2020 21:34:38 +0100 Subject: [PATCH] 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. --- src/libexpr/eval.cc | 2 +- src/libexpr/primops/fetchTree.cc | 4 +++- src/libstore/fetchers/attrs.cc | 25 +++++++++++++++++++++++-- src/libstore/fetchers/attrs.hh | 13 ++++++++++++- src/libstore/fetchers/git.cc | 16 +++++++++++++--- src/libstore/fetchers/mercurial.cc | 2 +- tests/fetchGit.sh | 10 +++++++--- 7 files changed, 60 insertions(+), 12 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 852e8aa11..6fcb2917c 100644 --- 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 index a9becddd9..2b3f1b76d 100644 --- 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 index 83b6d9164..40c02de42 100644 --- 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()); else if (i.value().is_string()) attrs.emplace(i.key(), i.value().get()); + else if (i.value().is_boolean()) + attrs.emplace(i.key(), i.value().get()); 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(&attr.second)) { json[attr.first] = *v; + } else if (auto v = std::get_if>(&attr.second)) { + json[attr.first] = v->t; } else abort(); } return json; @@ -40,7 +44,7 @@ std::optional maybeGetStrAttr(const Attrs & attrs, const std::strin if (i == attrs.end()) return {}; if (auto v = std::get_if(&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 maybeGetIntAttr(const Attrs & attrs, const std::string & if (i == attrs.end()) return {}; if (auto v = std::get_if(&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 maybeGetBoolAttr(const Attrs & attrs, const std::string & name) +{ + auto i = attrs.find(name); + if (i == attrs.end()) return {}; + if (auto v = std::get_if(&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 index b7f98d71b..2c9e772d2 100644 --- a/src/libstore/fetchers/attrs.hh +++ b/src/libstore/fetchers/attrs.hh @@ -8,7 +8,14 @@ namespace nix::fetchers { -typedef std::variant Attr; +/* Wrap bools to prevent string literals (i.e. 'char *') from being + cast to a bool in Attr. */ +template +struct Explicit { + T t; +}; + +typedef std::variant> Attr; typedef std::map Attrs; Attrs jsonToAttrs(const nlohmann::json & json); @@ -23,4 +30,8 @@ std::optional maybeGetIntAttr(const Attrs & attrs, const std::string & int64_t getIntAttr(const Attrs & attrs, const std::string & name); +std::optional 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 index 179a5fb83..c5d4f019c 100644 --- a/src/libstore/fetchers/git.cc +++ b/src/libstore/fetchers/git.cc @@ -83,6 +83,7 @@ struct GitInput : Input ParsedURL url; std::optional ref; std::optional 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(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 index a9c86f1b4..3a767ef17 100644 --- 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 index 6e5a4750e..bca581438 100644 --- 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 ]]