From 4d0ecda33e29520756fdb7ccb7549205ed1afd52 Mon Sep 17 00:00:00 2001 From: DavHau Date: Sun, 19 Nov 2023 20:37:42 +0700 Subject: [PATCH 01/17] fetchTree/fetchGit: add test for .gitattributes ...with the intention to prevent future regressions in fetchGit --- tests/functional/fetchGit.sh | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/functional/fetchGit.sh b/tests/functional/fetchGit.sh index 4985c7764..f0438f548 100644 --- a/tests/functional/fetchGit.sh +++ b/tests/functional/fetchGit.sh @@ -229,6 +229,15 @@ rev_tag2=$(git -C $repo rev-parse refs/tags/tag2) [[ $rev_tag2_nix = $rev_tag2 ]] unset _NIX_FORCE_HTTP +# Ensure .gitattributes is respected +touch $repo/not-exported-file +echo "/not-exported-file export-ignore" >> $repo/.gitattributes +git -C $repo add not-exported-file .gitattributes +git -C $repo commit -m 'Bla6' +rev5=$(git -C $repo rev-parse HEAD) +path12=$(nix eval --impure --raw --expr "(builtins.fetchGit { url = file://$repo; rev = \"$rev5\"; }).outPath") +[[ ! -e $path12/not-exported-file ]] + # should fail if there is no repo rm -rf $repo/.git (! nix eval --impure --raw --expr "(builtins.fetchGit \"file://$repo\").outPath") From ce6d58a97cf6f975a0b930605605fab153445b22 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 27 Nov 2023 22:34:41 +0100 Subject: [PATCH 02/17] git fetcher: Add exportIgnore parameter Enabled for fetchGit, which historically had this behavior, among other behaviors we do not want in fetchGit. fetchTree disables this parameter by default. It can choose the simpler behavior, as it is still experimental. I am not confident that the filtering implementation is future proof. It should reuse a source filtering wrapper, which I believe Eelco has already written, but not merged yet. --- src/libexpr/primops/fetchTree.cc | 14 ++++++++ src/libfetchers/git-utils.cc | 57 +++++++++++++++++++++++++++----- src/libfetchers/git-utils.hh | 4 +-- src/libfetchers/git.cc | 15 +++++++-- tests/functional/fetchGit.sh | 5 ++- 5 files changed, 81 insertions(+), 14 deletions(-) diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc index eb2df8626..e00c4f190 100644 --- a/src/libexpr/primops/fetchTree.cc +++ b/src/libexpr/primops/fetchTree.cc @@ -39,6 +39,10 @@ void emitTreeAttrs( attrs.alloc("submodules").mkBool( fetchers::maybeGetBoolAttr(input.attrs, "submodules").value_or(false)); + if (input.getType() == "git") + attrs.alloc("exportIgnore").mkBool( + fetchers::maybeGetBoolAttr(input.attrs, "exportIgnore").value_or(false)); + if (!forceDirty) { if (auto rev = input.getRev()) { @@ -112,6 +116,11 @@ static void fetchTree( attrs.emplace("type", type.value()); + if (params.isFetchGit) { + // Default value; user attrs are assigned later. + attrs.emplace("exportIgnore", Explicit{true}); + } + for (auto & attr : *args[0]->attrs) { if (attr.name == state.sType) continue; state.forceValue(*attr.value, attr.pos); @@ -593,6 +602,11 @@ static RegisterPrimOp primop_fetchGit({ A Boolean parameter that specifies whether submodules should be checked out. + - `exportIgnore` (default: `true`) + + A Boolean parameter that specifies whether `export-ignore` from `.gitattributes` should be applied. + This approximates part of the `git archive` behavior. + - `shallow` (default: `false`) A Boolean parameter that specifies whether fetching from a shallow remote repository is allowed. diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index 65f7b45ef..4dc749504 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -7,6 +7,7 @@ #include +#include #include #include #include @@ -21,6 +22,7 @@ #include #include +#include #include #include #include @@ -307,7 +309,7 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this return std::nullopt; } - std::vector> getSubmodules(const Hash & rev) override; + std::vector> getSubmodules(const Hash & rev, bool exportIgnore) override; std::string resolveSubmoduleUrl( const std::string & url, @@ -340,7 +342,7 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this return true; } - ref getAccessor(const Hash & rev) override; + ref getAccessor(const Hash & rev, bool exportIgnore) override; static int sidebandProgressCallback(const char * str, int len, void * payload) { @@ -460,10 +462,12 @@ struct GitInputAccessor : InputAccessor { ref repo; Tree root; + bool exportIgnore; - GitInputAccessor(ref repo_, const Hash & rev) + GitInputAccessor(ref repo_, const Hash & rev, bool exportIgnore) : repo(repo_) , root(peelObject(*repo, lookupObject(*repo, hashToOID(rev)).get(), GIT_OBJECT_TREE)) + , exportIgnore(exportIgnore) { } @@ -492,7 +496,7 @@ struct GitInputAccessor : InputAccessor return Stat { .type = tDirectory }; auto entry = lookup(path); - if (!entry) + if (!entry || isExportIgnored(path)) return std::nullopt; auto mode = git_tree_entry_filemode(entry); @@ -527,6 +531,12 @@ struct GitInputAccessor : InputAccessor for (size_t n = 0; n < count; ++n) { auto entry = git_tree_entry_byindex(tree.get(), n); + if (exportIgnore) { + if (isExportIgnored(path + git_tree_entry_name(entry))) { + continue; + } + } + // FIXME: add to cache res.emplace(std::string(git_tree_entry_name(entry)), DirEntry{}); } @@ -556,6 +566,33 @@ struct GitInputAccessor : InputAccessor std::unordered_map lookupCache; + bool isExportIgnored(const CanonPath & path) { + if (!exportIgnore) + return false; + + const char *exportIgnoreEntry = nullptr; + + // GIT_ATTR_CHECK_INDEX_ONLY: + // > It will use index only for creating archives or for a bare repo + // > (if an index has been specified for the bare repo). + // -- https://github.com/libgit2/libgit2/blob/HEAD/include/git2/attr.h#L113C62-L115C48 + if (git_attr_get(&exportIgnoreEntry, + *repo, + GIT_ATTR_CHECK_INDEX_ONLY, + std::string(path.rel()).c_str(), + "export-ignore")) { + if (git_error_last()->klass == GIT_ENOTFOUND) + return false; + else + throw Error("looking up '%s': %s", showPath(path), git_error_last()->message); + } + else { + // Official git will silently reject export-ignore lines that have + // values. We do the same. + return GIT_ATTR_IS_TRUE(exportIgnoreEntry); + } + } + /* Recursively look up 'path' relative to the root. */ git_tree_entry * lookup(const CanonPath & path) { @@ -569,6 +606,10 @@ struct GitInputAccessor : InputAccessor throw Error("looking up '%s': %s", showPath(path), git_error_last()->message); } + if (entry && isExportIgnored(path)) { + entry.reset(); + } + i = lookupCache.emplace(path, std::move(entry)).first; } @@ -644,17 +685,17 @@ struct GitInputAccessor : InputAccessor } }; -ref GitRepoImpl::getAccessor(const Hash & rev) +ref GitRepoImpl::getAccessor(const Hash & rev, bool exportIgnore) { - return make_ref(ref(shared_from_this()), rev); + return make_ref(ref(shared_from_this()), rev, exportIgnore); } -std::vector> GitRepoImpl::getSubmodules(const Hash & rev) +std::vector> GitRepoImpl::getSubmodules(const Hash & rev, bool exportIgnore) { /* Read the .gitmodules files from this revision. */ CanonPath modulesFile(".gitmodules"); - auto accessor = getAccessor(rev); + auto accessor = getAccessor(rev, exportIgnore); if (!accessor->pathExists(modulesFile)) return {}; /* Parse it and get the revision of each submodule. */ diff --git a/src/libfetchers/git-utils.hh b/src/libfetchers/git-utils.hh index 1def82071..f1cb48065 100644 --- a/src/libfetchers/git-utils.hh +++ b/src/libfetchers/git-utils.hh @@ -57,7 +57,7 @@ struct GitRepo * Return the submodules of this repo at the indicated revision, * along with the revision of each submodule. */ - virtual std::vector> getSubmodules(const Hash & rev) = 0; + virtual std::vector> getSubmodules(const Hash & rev, bool exportIgnore) = 0; virtual std::string resolveSubmoduleUrl( const std::string & url, @@ -71,7 +71,7 @@ struct GitRepo virtual bool hasObject(const Hash & oid) = 0; - virtual ref getAccessor(const Hash & rev) = 0; + virtual ref getAccessor(const Hash & rev, bool exportIgnore) = 0; virtual void fetch( const std::string & url, diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 79270c317..fb8bf5bf4 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -174,7 +174,7 @@ struct GitInputScheme : InputScheme for (auto & [name, value] : url.query) { if (name == "rev" || name == "ref" || name == "keytype" || name == "publicKey" || name == "publicKeys") attrs.emplace(name, value); - else if (name == "shallow" || name == "submodules" || name == "allRefs" || name == "verifyCommit") + else if (name == "shallow" || name == "submodules" || name == "exportIgnore" || name == "allRefs" || name == "verifyCommit") attrs.emplace(name, Explicit { value == "1" }); else url2.query.emplace(name, value); @@ -199,6 +199,7 @@ struct GitInputScheme : InputScheme "rev", "shallow", "submodules", + "exportIgnore", "lastModified", "revCount", "narHash", @@ -250,6 +251,8 @@ struct GitInputScheme : InputScheme url.query.insert_or_assign("shallow", "1"); if (getSubmodulesAttr(input)) url.query.insert_or_assign("submodules", "1"); + if (maybeGetBoolAttr(input.attrs, "exportIgnore").value_or(false)) + url.query.insert_or_assign("exportIgnore", "1"); if (maybeGetBoolAttr(input.attrs, "verifyCommit").value_or(false)) url.query.insert_or_assign("verifyCommit", "1"); auto publicKeys = getPublicKeys(input.attrs); @@ -372,6 +375,11 @@ struct GitInputScheme : InputScheme return maybeGetBoolAttr(input.attrs, "submodules").value_or(false); } + bool getExportIgnoreAttr(const Input & input) const + { + return maybeGetBoolAttr(input.attrs, "exportIgnore").value_or(false); + } + bool getAllRefsAttr(const Input & input) const { return maybeGetBoolAttr(input.attrs, "allRefs").value_or(false); @@ -600,7 +608,8 @@ struct GitInputScheme : InputScheme verifyCommit(input, repo); - auto accessor = repo->getAccessor(rev); + bool exportIgnore = getExportIgnoreAttr(input); + auto accessor = repo->getAccessor(rev, exportIgnore); accessor->setPathDisplay("«" + input.to_string() + "»"); @@ -610,7 +619,7 @@ struct GitInputScheme : InputScheme if (getSubmodulesAttr(input)) { std::map> mounts; - for (auto & [submodule, submoduleRev] : repo->getSubmodules(rev)) { + for (auto & [submodule, submoduleRev] : repo->getSubmodules(rev, exportIgnore)) { auto resolved = repo->resolveSubmoduleUrl(submodule.url, repoInfo.url); debug("Git submodule %s: %s %s %s -> %s", submodule.path, submodule.url, submodule.branch, submoduleRev.gitRev(), resolved); diff --git a/tests/functional/fetchGit.sh b/tests/functional/fetchGit.sh index f0438f548..46532c025 100644 --- a/tests/functional/fetchGit.sh +++ b/tests/functional/fetchGit.sh @@ -231,12 +231,15 @@ unset _NIX_FORCE_HTTP # Ensure .gitattributes is respected touch $repo/not-exported-file +touch $repo/exported-wonky echo "/not-exported-file export-ignore" >> $repo/.gitattributes -git -C $repo add not-exported-file .gitattributes +echo "/exported-wonky export-ignore=wonk" >> $repo/.gitattributes +git -C $repo add not-exported-file exported-wonky .gitattributes git -C $repo commit -m 'Bla6' rev5=$(git -C $repo rev-parse HEAD) path12=$(nix eval --impure --raw --expr "(builtins.fetchGit { url = file://$repo; rev = \"$rev5\"; }).outPath") [[ ! -e $path12/not-exported-file ]] +[[ -e $path12/exported-wonky ]] # should fail if there is no repo rm -rf $repo/.git From 1c6bb609af3277ff3f747f49d04be80463d1f153 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 28 Nov 2023 00:41:01 +0100 Subject: [PATCH 03/17] fetchTree: allow larger output attrsets Intentionally dumb change ahead of architectural improvements. --- src/libexpr/primops/fetchTree.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc index e00c4f190..d04908b77 100644 --- a/src/libexpr/primops/fetchTree.cc +++ b/src/libexpr/primops/fetchTree.cc @@ -25,7 +25,7 @@ void emitTreeAttrs( { assert(input.isLocked()); - auto attrs = state.buildBindings(10); + auto attrs = state.buildBindings(100); state.mkStorePathString(storePath, attrs.alloc(state.sOutPath)); From f6b1d155804a946ff2965b5fd1a57159770e8b58 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 11 Dec 2023 19:27:13 +0100 Subject: [PATCH 04/17] MakeNotAllowedError: Touch up doc --- src/libfetchers/filtering-input-accessor.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libfetchers/filtering-input-accessor.hh b/src/libfetchers/filtering-input-accessor.hh index a352a33a6..2e2495c78 100644 --- a/src/libfetchers/filtering-input-accessor.hh +++ b/src/libfetchers/filtering-input-accessor.hh @@ -6,7 +6,7 @@ namespace nix { /** - * A function that should throw an exception of type + * A function that returns an exception of type * `RestrictedPathError` explaining that access to `path` is * forbidden. */ From cd5e752fa72bf15ba8fe6fcdae92c77ac6dc2375 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 11 Dec 2023 19:30:10 +0100 Subject: [PATCH 05/17] GitRepoImpl::getSubmodules: Access getSubmoduleRev without cast This will be needed because the accessor will be wrapped, and therefore not be an instance of GitInputAccessor anymore. --- src/libfetchers/git-utils.cc | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index 4dc749504..d8a4f1778 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -52,6 +52,8 @@ bool operator == (const git_oid & oid1, const git_oid & oid2) namespace nix { +struct GitInputAccessor; + // Some wrapper types that ensure that the git_*_free functions get called. template struct Deleter @@ -342,6 +344,11 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this return true; } + /** + * A 'GitInputAccessor' with no regard for export-ignore or any other transformations. + */ + ref getRawAccessor(const Hash & rev); + ref getAccessor(const Hash & rev, bool exportIgnore) override; static int sidebandProgressCallback(const char * str, int len, void * payload) @@ -685,6 +692,12 @@ struct GitInputAccessor : InputAccessor } }; +ref GitRepoImpl::getRawAccessor(const Hash & rev) +{ + auto self = ref(shared_from_this()); + return make_ref(self, rev); +} + ref GitRepoImpl::getAccessor(const Hash & rev, bool exportIgnore) { return make_ref(ref(shared_from_this()), rev, exportIgnore); @@ -706,8 +719,10 @@ std::vector> GitRepoImpl::getSubmodules std::vector> result; + auto rawAccessor = getRawAccessor(rev); + for (auto & submodule : parseSubmodules(CanonPath(pathTemp))) { - auto rev = accessor.dynamic_pointer_cast()->getSubmoduleRev(submodule.path); + auto rev = rawAccessor->getSubmoduleRev(submodule.path); result.push_back({std::move(submodule), rev}); } From 467c62a96eaabe2f71939a07d923a759f82a466f Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 11 Dec 2023 19:32:18 +0100 Subject: [PATCH 06/17] GitRepoImpl: Move exportIgnore into a filtering accessor --- src/libfetchers/git-utils.cc | 96 ++++++++++++++++++++---------------- 1 file changed, 54 insertions(+), 42 deletions(-) diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index d8a4f1778..f8b2afeef 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -1,5 +1,6 @@ #include "git-utils.hh" #include "input-accessor.hh" +#include "filtering-input-accessor.hh" #include "cache.hh" #include "finally.hh" #include "processes.hh" @@ -465,16 +466,17 @@ ref GitRepo::openRepo(const CanonPath & path, bool create, bool bare) return make_ref(path, create, bare); } +/** + * Raw git tree input accessor. + */ struct GitInputAccessor : InputAccessor { ref repo; Tree root; - bool exportIgnore; - GitInputAccessor(ref repo_, const Hash & rev, bool exportIgnore) + GitInputAccessor(ref repo_, const Hash & rev) : repo(repo_) , root(peelObject(*repo, lookupObject(*repo, hashToOID(rev)).get(), GIT_OBJECT_TREE)) - , exportIgnore(exportIgnore) { } @@ -503,7 +505,7 @@ struct GitInputAccessor : InputAccessor return Stat { .type = tDirectory }; auto entry = lookup(path); - if (!entry || isExportIgnored(path)) + if (!entry) return std::nullopt; auto mode = git_tree_entry_filemode(entry); @@ -538,12 +540,6 @@ struct GitInputAccessor : InputAccessor for (size_t n = 0; n < count; ++n) { auto entry = git_tree_entry_byindex(tree.get(), n); - if (exportIgnore) { - if (isExportIgnored(path + git_tree_entry_name(entry))) { - continue; - } - } - // FIXME: add to cache res.emplace(std::string(git_tree_entry_name(entry)), DirEntry{}); } @@ -573,33 +569,6 @@ struct GitInputAccessor : InputAccessor std::unordered_map lookupCache; - bool isExportIgnored(const CanonPath & path) { - if (!exportIgnore) - return false; - - const char *exportIgnoreEntry = nullptr; - - // GIT_ATTR_CHECK_INDEX_ONLY: - // > It will use index only for creating archives or for a bare repo - // > (if an index has been specified for the bare repo). - // -- https://github.com/libgit2/libgit2/blob/HEAD/include/git2/attr.h#L113C62-L115C48 - if (git_attr_get(&exportIgnoreEntry, - *repo, - GIT_ATTR_CHECK_INDEX_ONLY, - std::string(path.rel()).c_str(), - "export-ignore")) { - if (git_error_last()->klass == GIT_ENOTFOUND) - return false; - else - throw Error("looking up '%s': %s", showPath(path), git_error_last()->message); - } - else { - // Official git will silently reject export-ignore lines that have - // values. We do the same. - return GIT_ATTR_IS_TRUE(exportIgnoreEntry); - } - } - /* Recursively look up 'path' relative to the root. */ git_tree_entry * lookup(const CanonPath & path) { @@ -613,10 +582,6 @@ struct GitInputAccessor : InputAccessor throw Error("looking up '%s': %s", showPath(path), git_error_last()->message); } - if (entry && isExportIgnored(path)) { - entry.reset(); - } - i = lookupCache.emplace(path, std::move(entry)).first; } @@ -692,6 +657,46 @@ struct GitInputAccessor : InputAccessor } }; +struct GitExportIgnoreInputAccessor : FilteringInputAccessor { + ref repo; + + GitExportIgnoreInputAccessor(ref repo, ref next) + : FilteringInputAccessor(next, [&](const CanonPath & path) { + return RestrictedPathError(fmt("'%s' does not exist because it was fetched with exportIgnore enabled", path)); + }) + , repo(repo) + { } + + bool isExportIgnored(const CanonPath & path) { + const char *exportIgnoreEntry = nullptr; + + // GIT_ATTR_CHECK_INDEX_ONLY: + // > It will use index only for creating archives or for a bare repo + // > (if an index has been specified for the bare repo). + // -- https://github.com/libgit2/libgit2/blob/HEAD/include/git2/attr.h#L113C62-L115C48 + if (git_attr_get(&exportIgnoreEntry, + *repo, + GIT_ATTR_CHECK_INDEX_ONLY, + std::string(path.rel()).c_str(), + "export-ignore")) { + if (git_error_last()->klass == GIT_ENOTFOUND) + return false; + else + throw Error("looking up '%s': %s", showPath(path), git_error_last()->message); + } + else { + // Official git will silently reject export-ignore lines that have + // values. We do the same. + return GIT_ATTR_IS_TRUE(exportIgnoreEntry); + } + } + + bool isAllowed(const CanonPath & path) override { + return !isExportIgnored(path); + } + +}; + ref GitRepoImpl::getRawAccessor(const Hash & rev) { auto self = ref(shared_from_this()); @@ -700,7 +705,14 @@ ref GitRepoImpl::getRawAccessor(const Hash & rev) ref GitRepoImpl::getAccessor(const Hash & rev, bool exportIgnore) { - return make_ref(ref(shared_from_this()), rev, exportIgnore); + auto self = ref(shared_from_this()); + ref rawGitAccessor = getRawAccessor(rev); + if (exportIgnore) { + return make_ref(self, rawGitAccessor); + } + else { + return rawGitAccessor; + } } std::vector> GitRepoImpl::getSubmodules(const Hash & rev, bool exportIgnore) From 8024b954d702e0693b532650230037e398453693 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 11 Dec 2023 19:42:46 +0100 Subject: [PATCH 07/17] fetchTree: Recommend against exportIgnore --- src/libexpr/primops/fetchTree.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc index d04908b77..2e4b72c9f 100644 --- a/src/libexpr/primops/fetchTree.cc +++ b/src/libexpr/primops/fetchTree.cc @@ -607,6 +607,8 @@ static RegisterPrimOp primop_fetchGit({ A Boolean parameter that specifies whether `export-ignore` from `.gitattributes` should be applied. This approximates part of the `git archive` behavior. + Enabling this option is not recommended because it is unknown whether the Git developers commit to the reproducibility of `export-ignore` in newer Git versions. + - `shallow` (default: `false`) A Boolean parameter that specifies whether fetching from a shallow remote repository is allowed. From 7774eff10e0ec1f540a6dc22d8fd78de40714bdf Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 11 Dec 2023 22:28:53 +0100 Subject: [PATCH 08/17] libfetchers/git: Move workdir accessor into GitRepo::getAccessor --- src/libfetchers/git-utils.cc | 19 +++++++++++++++++++ src/libfetchers/git-utils.hh | 3 +++ src/libfetchers/git.cc | 8 ++++---- 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index f8b2afeef..d218276b4 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -1,4 +1,5 @@ #include "git-utils.hh" +#include "fs-input-accessor.hh" #include "input-accessor.hh" #include "filtering-input-accessor.hh" #include "cache.hh" @@ -352,6 +353,8 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this ref getAccessor(const Hash & rev, bool exportIgnore) override; + ref getAccessor(const WorkdirInfo & wd, bool exportIgnore, MakeNotAllowedError e) override; + static int sidebandProgressCallback(const char * str, int len, void * payload) { auto act = (Activity *) payload; @@ -715,6 +718,22 @@ ref GitRepoImpl::getAccessor(const Hash & rev, bool exportIgnore) } } +ref GitRepoImpl::getAccessor(const WorkdirInfo & wd, bool exportIgnore, MakeNotAllowedError makeNotAllowedError) +{ + auto self = ref(shared_from_this()); + ref fileAccessor = + AllowListInputAccessor::create( + makeFSInputAccessor(path), + std::set { wd.files }, + std::move(makeNotAllowedError)); + if (exportIgnore) { + return make_ref(self, fileAccessor); + } + else { + return fileAccessor; + } +} + std::vector> GitRepoImpl::getSubmodules(const Hash & rev, bool exportIgnore) { /* Read the .gitmodules files from this revision. */ diff --git a/src/libfetchers/git-utils.hh b/src/libfetchers/git-utils.hh index f1cb48065..768554780 100644 --- a/src/libfetchers/git-utils.hh +++ b/src/libfetchers/git-utils.hh @@ -1,5 +1,6 @@ #pragma once +#include "filtering-input-accessor.hh" #include "input-accessor.hh" namespace nix { @@ -73,6 +74,8 @@ struct GitRepo virtual ref getAccessor(const Hash & rev, bool exportIgnore) = 0; + virtual ref getAccessor(const WorkdirInfo & wd, bool exportIgnore, MakeNotAllowedError makeNotAllowedError) = 0; + virtual void fetch( const std::string & url, const std::string & refspec, diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index fb8bf5bf4..d7818988f 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -9,7 +9,6 @@ #include "processes.hh" #include "git.hh" #include "fs-input-accessor.hh" -#include "filtering-input-accessor.hh" #include "mounted-input-accessor.hh" #include "git-utils.hh" #include "logging.hh" @@ -659,10 +658,11 @@ struct GitInputScheme : InputScheme for (auto & submodule : repoInfo.workdirInfo.submodules) repoInfo.workdirInfo.files.insert(submodule.path); + auto repo = GitRepo::openRepo(CanonPath(repoInfo.url), false, false); + ref accessor = - AllowListInputAccessor::create( - makeFSInputAccessor(CanonPath(repoInfo.url)), - std::move(repoInfo.workdirInfo.files), + repo->getAccessor(repoInfo.workdirInfo, + getExportIgnoreAttr(input), makeNotAllowedError(repoInfo.url)); /* If the repo has submodules, return a mounted input accessor From 1bbe8371849f33da4edba23289de7b7e3c5d6c83 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 11 Dec 2023 22:35:11 +0100 Subject: [PATCH 09/17] fetchTree: Add isFetchGit exportIgnore --- src/libexpr/primops/fetchTree.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc index 2e4b72c9f..c167444b0 100644 --- a/src/libexpr/primops/fetchTree.cc +++ b/src/libexpr/primops/fetchTree.cc @@ -161,6 +161,7 @@ static void fetchTree( fetchers::Attrs attrs; attrs.emplace("type", "git"); attrs.emplace("url", fixGitURL(url)); + attrs.emplace("exportIgnore", Explicit{true}); input = fetchers::Input::fromAttrs(std::move(attrs)); } else { if (!experimentalFeatureSettings.isEnabled(Xp::Flakes)) From 99bd12f0b18b1a2a94639134c49c478c9ab56b3b Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 11 Dec 2023 22:36:08 +0100 Subject: [PATCH 10/17] fetchGit/fetchTree: Improve exportIgnore, submodule interaction Also fingerprint and some preparatory improvements. Testing is still not up to scratch because lots of logic is duplicated between the workdir and commit cases. --- src/libexpr/primops/fetchTree.cc | 16 ++++++---- src/libfetchers/fetchers.hh | 7 +++++ src/libfetchers/git-utils.cc | 43 +++++++++++++++++++++----- src/libfetchers/git.cc | 9 ++++-- tests/functional/fetchGitSubmodules.sh | 42 +++++++++++++++++++++++++ 5 files changed, 101 insertions(+), 16 deletions(-) diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc index c167444b0..7a4725334 100644 --- a/src/libexpr/primops/fetchTree.cc +++ b/src/libexpr/primops/fetchTree.cc @@ -116,11 +116,6 @@ static void fetchTree( attrs.emplace("type", type.value()); - if (params.isFetchGit) { - // Default value; user attrs are assigned later. - attrs.emplace("exportIgnore", Explicit{true}); - } - for (auto & attr : *args[0]->attrs) { if (attr.name == state.sType) continue; state.forceValue(*attr.value, attr.pos); @@ -144,6 +139,12 @@ static void fetchTree( state.symbols[attr.name], showType(*attr.value))); } + if (params.isFetchGit && !attrs.contains("exportIgnore")) { + // Default value; user attrs are assigned later. + // FIXME: exportIgnore := !submodules + attrs.emplace("exportIgnore", Explicit{true}); + } + if (!params.allowNameArgument) if (auto nameIter = attrs.find("name"); nameIter != attrs.end()) state.debugThrowLastTrace(EvalError({ @@ -161,7 +162,10 @@ static void fetchTree( fetchers::Attrs attrs; attrs.emplace("type", "git"); attrs.emplace("url", fixGitURL(url)); - attrs.emplace("exportIgnore", Explicit{true}); + if (!attrs.contains("exportIgnore")) { + // FIXME: exportIgnore := !submodules + attrs.emplace("exportIgnore", Explicit{true}); + } input = fetchers::Input::fromAttrs(std::move(attrs)); } else { if (!experimentalFeatureSettings.isEnabled(Xp::Flakes)) diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh index 5f3254b6d..036647830 100644 --- a/src/libfetchers/fetchers.hh +++ b/src/libfetchers/fetchers.hh @@ -187,6 +187,13 @@ struct InputScheme virtual bool isDirect(const Input & input) const { return true; } + /** + * A sufficiently unique string that can be used as a cache key to identify the `input`. + * + * Only known-equivalent inputs should return the same fingerprint. + * + * This is not a stable identifier between Nix versions, but not guaranteed to change either. + */ virtual std::optional getFingerprint(ref store, const Input & input) const { return std::nullopt; } }; diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index d218276b4..cd65e0fda 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -662,14 +662,45 @@ struct GitInputAccessor : InputAccessor struct GitExportIgnoreInputAccessor : FilteringInputAccessor { ref repo; + std::optional rev; - GitExportIgnoreInputAccessor(ref repo, ref next) + GitExportIgnoreInputAccessor(ref repo, ref next, std::optional rev) : FilteringInputAccessor(next, [&](const CanonPath & path) { return RestrictedPathError(fmt("'%s' does not exist because it was fetched with exportIgnore enabled", path)); }) , repo(repo) + , rev(rev) { } + bool gitAttrGet(const CanonPath & path, const char * attrName, const char * & valueOut) + { + std::string pathStr {path.rel()}; + const char * pathCStr = pathStr.c_str(); + + if (rev) { + git_attr_options opts = GIT_ATTR_OPTIONS_INIT; + opts.attr_commit_id = hashToOID(*rev); + // TODO: test that gitattributes from global and system are not used + // (ie more or less: home and etc - both of them!) + opts.flags = GIT_ATTR_CHECK_INCLUDE_COMMIT | GIT_ATTR_CHECK_NO_SYSTEM; + return git_attr_get_ext( + &valueOut, + *repo, + &opts, + pathCStr, + attrName + ); + } + else { + return git_attr_get( + &valueOut, + *repo, + GIT_ATTR_CHECK_INDEX_ONLY | GIT_ATTR_CHECK_NO_SYSTEM, + pathCStr, + attrName); + } + } + bool isExportIgnored(const CanonPath & path) { const char *exportIgnoreEntry = nullptr; @@ -677,11 +708,7 @@ struct GitExportIgnoreInputAccessor : FilteringInputAccessor { // > It will use index only for creating archives or for a bare repo // > (if an index has been specified for the bare repo). // -- https://github.com/libgit2/libgit2/blob/HEAD/include/git2/attr.h#L113C62-L115C48 - if (git_attr_get(&exportIgnoreEntry, - *repo, - GIT_ATTR_CHECK_INDEX_ONLY, - std::string(path.rel()).c_str(), - "export-ignore")) { + if (gitAttrGet(path, "export-ignore", exportIgnoreEntry)) { if (git_error_last()->klass == GIT_ENOTFOUND) return false; else @@ -711,7 +738,7 @@ ref GitRepoImpl::getAccessor(const Hash & rev, bool exportIgnore) auto self = ref(shared_from_this()); ref rawGitAccessor = getRawAccessor(rev); if (exportIgnore) { - return make_ref(self, rawGitAccessor); + return make_ref(self, rawGitAccessor, rev); } else { return rawGitAccessor; @@ -727,7 +754,7 @@ ref GitRepoImpl::getAccessor(const WorkdirInfo & wd, bool exportI std::set { wd.files }, std::move(makeNotAllowedError)); if (exportIgnore) { - return make_ref(self, fileAccessor); + return make_ref(self, fileAccessor, std::nullopt); } else { return fileAccessor; diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index d7818988f..10c0aef97 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -628,6 +628,7 @@ struct GitInputScheme : InputScheme if (submodule.branch != "") attrs.insert_or_assign("ref", submodule.branch); attrs.insert_or_assign("rev", submoduleRev.gitRev()); + attrs.insert_or_assign("exportIgnore", Explicit{ exportIgnore }); auto submoduleInput = fetchers::Input::fromAttrs(std::move(attrs)); auto [submoduleAccessor, submoduleInput2] = submoduleInput.getAccessor(store); @@ -660,9 +661,11 @@ struct GitInputScheme : InputScheme auto repo = GitRepo::openRepo(CanonPath(repoInfo.url), false, false); + auto exportIgnore = getExportIgnoreAttr(input); + ref accessor = repo->getAccessor(repoInfo.workdirInfo, - getExportIgnoreAttr(input), + exportIgnore, makeNotAllowedError(repoInfo.url)); /* If the repo has submodules, return a mounted input accessor @@ -676,6 +679,8 @@ struct GitInputScheme : InputScheme fetchers::Attrs attrs; attrs.insert_or_assign("type", "git"); attrs.insert_or_assign("url", submodulePath.abs()); + attrs.insert_or_assign("exportIgnore", Explicit{ exportIgnore }); + auto submoduleInput = fetchers::Input::fromAttrs(std::move(attrs)); auto [submoduleAccessor, submoduleInput2] = submoduleInput.getAccessor(store); @@ -747,7 +752,7 @@ struct GitInputScheme : InputScheme std::optional getFingerprint(ref store, const Input & input) const override { if (auto rev = input.getRev()) - return rev->gitRev() + (getSubmodulesAttr(input) ? ";s" : ""); + return rev->gitRev() + (getSubmodulesAttr(input) ? ";s" : "") + (getExportIgnoreAttr(input) ? ";e" : ""); else return std::nullopt; } diff --git a/tests/functional/fetchGitSubmodules.sh b/tests/functional/fetchGitSubmodules.sh index 369cdc5db..1b425820e 100644 --- a/tests/functional/fetchGitSubmodules.sh +++ b/tests/functional/fetchGitSubmodules.sh @@ -118,3 +118,45 @@ cloneRepo=$TEST_ROOT/a/b/gitSubmodulesClone # NB /a/b to make the relative path git clone $rootRepo $cloneRepo pathIndirect=$(nix eval --raw --expr "(builtins.fetchGit { url = file://$cloneRepo; rev = \"$rev2\"; submodules = true; }).outPath") [[ $pathIndirect = $pathWithRelative ]] + +# Test submodule export-ignore interaction +git -C $rootRepo/sub config user.email "foobar@example.com" +git -C $rootRepo/sub config user.name "Foobar" + +echo "/exclude-from-root export-ignore" >> $rootRepo/.gitattributes +echo nope > $rootRepo/exclude-from-root +git -C $rootRepo add .gitattributes exclude-from-root +git -C $rootRepo commit -m "Add export-ignore" + +echo "/exclude-from-sub export-ignore" >> $rootRepo/sub/.gitattributes +echo nope > $rootRepo/sub/exclude-from-sub +git -C $rootRepo/sub add .gitattributes exclude-from-sub +git -C $rootRepo/sub commit -m "Add export-ignore (sub)" + +git -C $rootRepo add sub +git -C $rootRepo commit -m "Update submodule" + +git -C $rootRepo status + +# exportIgnore can be used with submodules +pathWithExportIgnore=$(nix eval --impure --raw --expr "(builtins.fetchGit { url = file://$rootRepo; submodules = true; exportIgnore = true; }).outPath") +# find $pathWithExportIgnore +# git -C $rootRepo archive --format=tar HEAD | tar -t +# cp -a $rootRepo /tmp/rootRepo + +[[ -e $pathWithExportIgnore/sub/content ]] +[[ ! -e $pathWithExportIgnore/exclude-from-root ]] +[[ ! -e $pathWithExportIgnore/sub/exclude-from-sub ]] + +# exportIgnore can be explicitly disabled with submodules +pathWithoutExportIgnore=$(nix eval --impure --raw --expr "(builtins.fetchGit { url = file://$rootRepo; submodules = true; exportIgnore = false; }).outPath") +# find $pathWithoutExportIgnore + +[[ -e $pathWithoutExportIgnore/exclude-from-root ]] +[[ -e $pathWithoutExportIgnore/sub/exclude-from-sub ]] + +# exportIgnore defaults to false when submodules = true +pathWithSubmodules=$(nix eval --impure --raw --expr "(builtins.fetchGit { url = file://$rootRepo; submodules = true; }).outPath") + +[[ -e $pathWithoutExportIgnore/exclude-from-root ]] +[[ -e $pathWithoutExportIgnore/sub/exclude-from-sub ]] From 71d08af15bb2973dc2a1cb7fee18f94d779dfed7 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 5 Jan 2024 19:01:12 +0100 Subject: [PATCH 11/17] rl-next: Add *general* note about git fetcher reimpl --- doc/manual/rl-next/git-fetcher.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 doc/manual/rl-next/git-fetcher.md diff --git a/doc/manual/rl-next/git-fetcher.md b/doc/manual/rl-next/git-fetcher.md new file mode 100644 index 000000000..54c0d216d --- /dev/null +++ b/doc/manual/rl-next/git-fetcher.md @@ -0,0 +1,18 @@ +--- +synopsis: "Nix now uses `libgit2` for Git fetching" +prs: + - 9240 + - 9241 + - 9258 + - 9480 +issues: + - 5313 +--- + +Nix has built-in support for fetching sources from Git, during evaluation and locking; outside the sandbox. +The existing implementation based on the Git CLI had issues regarding reproducibility and performance. + +Most of the original `fetchGit` behavior has been implemented using the `libgit2` library, which gives the fetcher fine-grained control. + +Known issues: +- The `export-subst` behavior has not been reimplemented. [Partial](https://github.com/NixOS/nix/pull/9391#issuecomment-1872503447) support for this Git feature is feasible, but it did not make the release window. From 692e9197bc91f874ec30f839b1ae6d1beefa1eeb Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 5 Jan 2024 19:49:39 +0100 Subject: [PATCH 12/17] fetchTree: Disallow combination of submodules and exportIgnore for now --- src/libexpr/primops/fetchTree.cc | 8 +++----- src/libfetchers/git.cc | 11 +++++++++++ tests/functional/fetchGitSubmodules.sh | 26 ++++++++++++++++++-------- 3 files changed, 32 insertions(+), 13 deletions(-) diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc index 7a4725334..4d22ca01e 100644 --- a/src/libexpr/primops/fetchTree.cc +++ b/src/libexpr/primops/fetchTree.cc @@ -1,3 +1,4 @@ +#include "libfetchers/attrs.hh" #include "primops.hh" #include "eval-inline.hh" #include "eval-settings.hh" @@ -139,9 +140,7 @@ static void fetchTree( state.symbols[attr.name], showType(*attr.value))); } - if (params.isFetchGit && !attrs.contains("exportIgnore")) { - // Default value; user attrs are assigned later. - // FIXME: exportIgnore := !submodules + if (params.isFetchGit && !attrs.contains("exportIgnore") && (!attrs.contains("submodules") || !*fetchers::maybeGetBoolAttr(attrs, "submodules"))) { attrs.emplace("exportIgnore", Explicit{true}); } @@ -162,8 +161,7 @@ static void fetchTree( fetchers::Attrs attrs; attrs.emplace("type", "git"); attrs.emplace("url", fixGitURL(url)); - if (!attrs.contains("exportIgnore")) { - // FIXME: exportIgnore := !submodules + if (!attrs.contains("exportIgnore") && (!attrs.contains("submodules") || !*fetchers::maybeGetBoolAttr(attrs, "submodules"))) { attrs.emplace("exportIgnore", Explicit{true}); } input = fetchers::Input::fromAttrs(std::move(attrs)); diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 10c0aef97..6ecb7a4ea 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -1,3 +1,4 @@ +#include "error.hh" #include "fetchers.hh" #include "users.hh" #include "cache.hh" @@ -739,6 +740,16 @@ struct GitInputScheme : InputScheme auto repoInfo = getRepoInfo(input); + if (getExportIgnoreAttr(input) + && getSubmodulesAttr(input)) { + /* In this situation, we don't have a git CLI behavior that we can copy. + `git archive` does not support submodules, so it is unclear whether + rules from the parent should affect the submodule or not. + When git may eventually implement this, we need Nix to match its + behavior. */ + throw UnimplementedError("exportIgnore and submodules are not supported together yet"); + } + auto [accessor, final] = input.getRef() || input.getRev() || !repoInfo.isLocal ? getAccessorFromCommit(store, repoInfo, std::move(input)) diff --git a/tests/functional/fetchGitSubmodules.sh b/tests/functional/fetchGitSubmodules.sh index 1b425820e..cd180815d 100644 --- a/tests/functional/fetchGitSubmodules.sh +++ b/tests/functional/fetchGitSubmodules.sh @@ -124,12 +124,16 @@ git -C $rootRepo/sub config user.email "foobar@example.com" git -C $rootRepo/sub config user.name "Foobar" echo "/exclude-from-root export-ignore" >> $rootRepo/.gitattributes +# TBD possible semantics for submodules + exportIgnore +# echo "/sub/exclude-deep export-ignore" >> $rootRepo/.gitattributes echo nope > $rootRepo/exclude-from-root git -C $rootRepo add .gitattributes exclude-from-root git -C $rootRepo commit -m "Add export-ignore" echo "/exclude-from-sub export-ignore" >> $rootRepo/sub/.gitattributes echo nope > $rootRepo/sub/exclude-from-sub +# TBD possible semantics for submodules + exportIgnore +# echo aye > $rootRepo/sub/exclude-from-root git -C $rootRepo/sub add .gitattributes exclude-from-sub git -C $rootRepo/sub commit -m "Add export-ignore (sub)" @@ -138,15 +142,21 @@ git -C $rootRepo commit -m "Update submodule" git -C $rootRepo status -# exportIgnore can be used with submodules -pathWithExportIgnore=$(nix eval --impure --raw --expr "(builtins.fetchGit { url = file://$rootRepo; submodules = true; exportIgnore = true; }).outPath") -# find $pathWithExportIgnore -# git -C $rootRepo archive --format=tar HEAD | tar -t -# cp -a $rootRepo /tmp/rootRepo +# # TBD: not supported yet, because semantics are undecided and current implementation leaks rules from the root to submodules +# # exportIgnore can be used with submodules +# pathWithExportIgnore=$(nix eval --impure --raw --expr "(builtins.fetchGit { url = file://$rootRepo; submodules = true; exportIgnore = true; }).outPath") +# # find $pathWithExportIgnore +# # git -C $rootRepo archive --format=tar HEAD | tar -t +# # cp -a $rootRepo /tmp/rootRepo + +# [[ -e $pathWithExportIgnore/sub/content ]] +# [[ ! -e $pathWithExportIgnore/exclude-from-root ]] +# [[ ! -e $pathWithExportIgnore/sub/exclude-from-sub ]] +# TBD possible semantics for submodules + exportIgnore +# # root .gitattribute has no power across submodule boundary +# [[ -e $pathWithExportIgnore/sub/exclude-from-root ]] +# [[ -e $pathWithExportIgnore/sub/exclude-deep ]] -[[ -e $pathWithExportIgnore/sub/content ]] -[[ ! -e $pathWithExportIgnore/exclude-from-root ]] -[[ ! -e $pathWithExportIgnore/sub/exclude-from-sub ]] # exportIgnore can be explicitly disabled with submodules pathWithoutExportIgnore=$(nix eval --impure --raw --expr "(builtins.fetchGit { url = file://$rootRepo; submodules = true; exportIgnore = false; }).outPath") From 469cf263c7d1b7991a9122b76b827f3d65a02301 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 12 Jan 2024 14:02:58 +0100 Subject: [PATCH 13/17] Format --- src/libfetchers/git-utils.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index cd65e0fda..b416c3b52 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -701,7 +701,8 @@ struct GitExportIgnoreInputAccessor : FilteringInputAccessor { } } - bool isExportIgnored(const CanonPath & path) { + bool isExportIgnored(const CanonPath & path) + { const char *exportIgnoreEntry = nullptr; // GIT_ATTR_CHECK_INDEX_ONLY: @@ -721,7 +722,8 @@ struct GitExportIgnoreInputAccessor : FilteringInputAccessor { } } - bool isAllowed(const CanonPath & path) override { + bool isAllowed(const CanonPath & path) override + { return !isExportIgnored(path); } From f68ad5acbb74c32d7ae6019bc17931940456603a Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 12 Jan 2024 16:05:36 +0100 Subject: [PATCH 14/17] fetchTree/git: Don't expose exportIgnore attr --- src/libexpr/primops/fetchTree.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc index 4d22ca01e..7251cbbbe 100644 --- a/src/libexpr/primops/fetchTree.cc +++ b/src/libexpr/primops/fetchTree.cc @@ -40,10 +40,6 @@ void emitTreeAttrs( attrs.alloc("submodules").mkBool( fetchers::maybeGetBoolAttr(input.attrs, "submodules").value_or(false)); - if (input.getType() == "git") - attrs.alloc("exportIgnore").mkBool( - fetchers::maybeGetBoolAttr(input.attrs, "exportIgnore").value_or(false)); - if (!forceDirty) { if (auto rev = input.getRev()) { From d80c582b783e4c189432a2afd383be39cc09f17c Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 12 Jan 2024 17:16:59 +0100 Subject: [PATCH 15/17] libfetchers: Add CachingFilteringInputAccessor Co-authored-by: Eelco Dolstra --- src/libfetchers/filtering-input-accessor.cc | 9 +++++++++ src/libfetchers/filtering-input-accessor.hh | 14 ++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/src/libfetchers/filtering-input-accessor.cc b/src/libfetchers/filtering-input-accessor.cc index 5ae416fd3..581ce3c1d 100644 --- a/src/libfetchers/filtering-input-accessor.cc +++ b/src/libfetchers/filtering-input-accessor.cc @@ -80,4 +80,13 @@ ref AllowListInputAccessor::create( return make_ref(next, std::move(allowedPaths), std::move(makeNotAllowedError)); } +bool CachingFilteringInputAccessor::isAllowed(const CanonPath & path) +{ + auto i = cache.find(path); + if (i != cache.end()) return i->second; + auto res = isAllowedUncached(path); + cache.emplace(path, res); + return res; +} + } diff --git a/src/libfetchers/filtering-input-accessor.hh b/src/libfetchers/filtering-input-accessor.hh index 2e2495c78..8a9b206ee 100644 --- a/src/libfetchers/filtering-input-accessor.hh +++ b/src/libfetchers/filtering-input-accessor.hh @@ -71,4 +71,18 @@ struct AllowListInputAccessor : public FilteringInputAccessor using FilteringInputAccessor::FilteringInputAccessor; }; +/** + * A wrapping `InputAccessor` mix-in where `isAllowed()` caches the result of virtual `isAllowedUncached()`. + */ +struct CachingFilteringInputAccessor : FilteringInputAccessor +{ + std::map cache; + + using FilteringInputAccessor::FilteringInputAccessor; + + bool isAllowed(const CanonPath & path) override; + + virtual bool isAllowedUncached(const CanonPath & path) = 0; +}; + } From 274d887feee7e8bc3d4a7e6c5087fbe5aec4fd4e Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 12 Jan 2024 17:18:56 +0100 Subject: [PATCH 16/17] fetchTree/git: Cache export-ignore filter --- src/libfetchers/git-utils.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index b416c3b52..bfc7059fe 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -660,12 +660,12 @@ struct GitInputAccessor : InputAccessor } }; -struct GitExportIgnoreInputAccessor : FilteringInputAccessor { +struct GitExportIgnoreInputAccessor : CachingFilteringInputAccessor { ref repo; std::optional rev; GitExportIgnoreInputAccessor(ref repo, ref next, std::optional rev) - : FilteringInputAccessor(next, [&](const CanonPath & path) { + : CachingFilteringInputAccessor(next, [&](const CanonPath & path) { return RestrictedPathError(fmt("'%s' does not exist because it was fetched with exportIgnore enabled", path)); }) , repo(repo) @@ -722,7 +722,7 @@ struct GitExportIgnoreInputAccessor : FilteringInputAccessor { } } - bool isAllowed(const CanonPath & path) override + bool isAllowedUncached(const CanonPath & path) override { return !isExportIgnored(path); } From 15f7bdaf276252f7b536c189b9b3eef73ad0e6e7 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 12 Jan 2024 22:55:37 +0100 Subject: [PATCH 17/17] CanonPath: Add rel_c_str() Defensively because isRoot() is also defensive. --- src/libfetchers/git-utils.cc | 3 +-- src/libutil/canon-path.hh | 7 +++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index bfc7059fe..6726407b5 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -674,8 +674,7 @@ struct GitExportIgnoreInputAccessor : CachingFilteringInputAccessor { bool gitAttrGet(const CanonPath & path, const char * attrName, const char * & valueOut) { - std::string pathStr {path.rel()}; - const char * pathCStr = pathStr.c_str(); + const char * pathCStr = path.rel_c_str(); if (rev) { git_attr_options opts = GIT_ATTR_OPTIONS_INIT; diff --git a/src/libutil/canon-path.hh b/src/libutil/canon-path.hh index 6aff4ec0d..997c8c731 100644 --- a/src/libutil/canon-path.hh +++ b/src/libutil/canon-path.hh @@ -88,6 +88,13 @@ public: std::string_view rel() const { return ((std::string_view) path).substr(1); } + const char * rel_c_str() const + { + auto cs = path.c_str(); + assert(cs[0]); // for safety if invariant is broken + return &cs[1]; + } + struct Iterator { std::string_view remaining;