libfetchers/git: Support export-ignore by roberth · Pull Request #9480 · NixOS/nix

roberth · 2023-11-27T21:45:01Z

Motivation

Reintroduce export-ignore processing for fetchGit, which historically had this behavior, among other behaviors that I believe we do not want in fetchGit.

Resolves this issue raised in PR fetchTree/fetchGit: add test for .gitattributes #9391

Change

Add exportIgnore parameter to the git fetcher
fetchTree disables this parameter by default. It can choose the
simpler behavior, as it is still experimental.
- (relevant to fetchTree too!) Nix.fetchGit surprisingly uses export-ignore git attribute #7195
fetchGit enables this parameter by default, to approximate legacy behavior.

TODO

I am was not confident that the filtering implementation is future proof. It should reuse a source filtering wrapper, such as FilteringInputAccessor from Lazy trees #6530. Not a straightforward backport because we don't have virtual bool isAllowed(const CanonPath & path) = 0; (yet?)
POSTPONED Test export-ignore in submodule (exportIgnore (Nix) parameter should be inherited)
- basic tests
- somewhat decent coverage (perhaps via DRY item below?)
Release note
Use _ext git attributes functions, ~~or reimplement~~ (no, half of semantics already hardcoded; not worth it). Need to pass rev
DRY workdir vs rev more
Discourage use, because this assumes that the feature is as stable as the tree data model and interpretation. I don't expect the git developer to apply the same level of rigor to it, although it's probably still pretty good.
POSTPONED Decide how the interaction works between submodules and export-ignore. Currently root ignores affect the submodule, and I'm not happy about that. Maybe just forbid it for now. fetchGit does not export-ignore when it submodules.

Context

Uses reproducer from fetchTree/fetchGit: add test for .gitattributes #9391
Either
- depends on Move restricted/pure-eval access control out of the evaluator and into the accessor #9497
- or factor out the filtering class ahead of time

Priorities

Add 👍 to pull requests you find important.

roberth · 2023-11-29T20:38:22Z

@edolstra How do you want source filtering input accessors to work?

edolstra · 2023-11-30T21:04:51Z

@roberth #9497 adds FilteringInputAccessor and AllowListInputAccessor. The latter is used by the Git input type for workdirs to allow access to the tracked files in the workdir only, like this:

        ref<InputAccessor> accessor =
            AllowListInputAccessor::create(
                makeFSInputAccessor(CanonPath(repoInfo.url)),
                std::move(repoInfo.workdirInfo.files),
                makeNotAllowedError(repoInfo.url));

Maybe something similar can be done for export-ignore, i.e. wrap the GitAccessor into an AllowListInputAccessor that contains the unfiltered paths. Or it can subclass FilteringInputAccessor (where isExportIgnored() would become the mandatory isAllowed() method).

dpulls · 2023-12-08T21:21:53Z

🎉 All dependencies have been resolved !

DavHau · 2023-12-30T10:11:41Z

There is also export-subst which we would need to re-implemented for compatibility.
I added a test for it at: #9391

src/libexpr/primops/fetchTree.cc

roberth · 2024-01-05T18:55:45Z

src/libfetchers/git.cc


+        if (getExportIgnoreAttr(input)
+            && getSubmodulesAttr(input)) {
+            throw UnimplementedError("exportIgnore and submodules are not supported together yet");


Note that this combination isn't used in any existing expressions, because the previous implementation did not apply export-ignore when submodules were enabled.

Apart from this check, why don't they work together yet? Given that submodules are also implemented using Git accessors, I would expect the filtering to just work for submodules.

I've added the comment:

/* 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. */

nixos-discourse · 2024-01-12T08:34:25Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-01-08-nix-team-meeting-minutes-114/38156/1

edolstra

Some comment, but LGTM overall.

doc/manual/rl-next/git-fetcher.md

src/libexpr/primops/fetchTree.cc

edolstra · 2024-01-12T12:20:05Z

src/libexpr/primops/fetchTree.cc

+        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.


It's a bit odd to say that enabling this option is not recommended, and then having the default as "enabled". Probably better to say "We recommend disabling this option because bla bla".

It's not enabled for fetchTree though; just for fetchGit.

(And we can't unrecommend fetchGit until this is stable.)

edolstra · 2024-01-12T12:25:29Z

src/libfetchers/git-utils.cc

+        std::string pathStr {path.rel()};
+        const char * pathCStr = pathStr.c_str();


Better to add a rel_c_str() to CanonPath. I.e.

const char * raw_c_str() const { return path.c_str() + 1; }

src/libfetchers/git-utils.cc

edolstra · 2024-01-12T12:29:23Z

src/libfetchers/git.cc


+        if (getExportIgnoreAttr(input)
+            && getSubmodulesAttr(input)) {
+            throw UnimplementedError("exportIgnore and submodules are not supported together yet");


Apart from this check, why don't they work together yet? Given that submodules are also implemented using Git accessors, I would expect the filtering to just work for submodules.

src/libfetchers/git-utils.cc

edolstra · 2024-01-12T12:32:44Z

src/libfetchers/git-utils.cc

+    }
+
+    bool isAllowed(const CanonPath & path) override {
+        return !isExportIgnored(path);


What's the performance penalty for export-ignore lookups? Should this be cached? The lazy-trees branch has a CachingFilteringInputAccessor that caches isAllowed(), which might be useful here.

It takes about 2× as long on my local nixpkgs clone. Not great.
Caching could reduce the overhead by a factor three, so we may expect no better than 1.3× from that.
Using the batch variation of the libgit2 call could bring down the overhead some more though. That makes the lookups more eager, which in turns means that it's not a great match with the CachingFilteringInputAccessor interface ("protected" interface part as it were).

I've picked CachingFilteringInputAccessor from the diff, and that brought it down to a 15% penalty.

batch variation

I misremembered while libgit2.org was down. It retrieves multiple attrs, not multiple files, so this is not a solution.

New idea: turn off filter when export-ignore is not present at all

I've tried this, but it seems to make fetching Nixpkgs with fetchGit slower (local repo, no fetcher cache, probably low single digit %) That's probably because I did an extra traversal of the whole repo before returning the accessor.

It's possible that a per-directory, on-the-fly approach does go below that 15%, but I don't have a lot of confidence right now.

Another possible strategy:
Cache it in a table like (parent_dir, filename, allow_bool), which could be queried quite efficiently for either the whole repo or particular directories when lazy. Nonetheless, there's a risk that it's not much better, and this is a lot more work to pull of, so again I'd say not for this release.

Conclusion: will stop optimizing now.

...with the intention to prevent future regressions in fetchGit

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.

Intentionally dumb change ahead of architectural improvements.

This will be needed because the accessor will be wrapped, and therefore not be an instance of GitInputAccessor anymore.

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.

Co-authored-by: Eelco Dolstra <edolstra@gmail.com>

Defensively because isRoot() is also defensive.

github-actions bot added with-tests Issues related to testing. PRs with tests have some priority fetching Networking with the outside (non-Nix) world, input locking labels Nov 27, 2023

roberth added the bug label Nov 27, 2023

roberth marked this pull request as ready for review November 29, 2023 20:37

roberth requested a review from edolstra as a code owner November 29, 2023 20:37

roberth assigned edolstra Nov 29, 2023

roberth added the regression Something doesn't work anymore label Dec 1, 2023

roberth self-assigned this Dec 8, 2023

roberth force-pushed the libfetchers-git-exportIgnore branch from 5d8c99c to a5f5744 Compare December 11, 2023 18:35

roberth marked this pull request as draft December 11, 2023 21:40

fricklerhandwerk added this to the fetchTree stabilisation milestone Dec 18, 2023

roberth mentioned this pull request Dec 29, 2023

builtins.gitLsFiles #2944

Open

roberth force-pushed the libfetchers-git-exportIgnore branch 3 times, most recently from 2f41fa4 to 1ea9930 Compare January 5, 2024 18:04

roberth marked this pull request as ready for review January 5, 2024 18:07

roberth commented Jan 5, 2024

View reviewed changes

src/libexpr/primops/fetchTree.cc Outdated Show resolved Hide resolved

roberth commented Jan 5, 2024

View reviewed changes

edolstra approved these changes Jan 12, 2024

View reviewed changes

DavHau and others added 6 commits January 12, 2024 15:31

fetchTree/fetchGit: add test for .gitattributes

4d0ecda

...with the intention to prevent future regressions in fetchGit

fetchTree: allow larger output attrsets

1c6bb60

Intentionally dumb change ahead of architectural improvements.

MakeNotAllowedError: Touch up doc

f6b1d15

GitRepoImpl::getSubmodules: Access getSubmoduleRev without cast

cd5e752

This will be needed because the accessor will be wrapped, and therefore not be an instance of GitInputAccessor anymore.

GitRepoImpl: Move exportIgnore into a filtering accessor

467c62a

roberth added 7 commits January 12, 2024 15:31

fetchTree: Recommend against exportIgnore

8024b95

libfetchers/git: Move workdir accessor into GitRepo::getAccessor

7774eff

fetchTree: Add isFetchGit exportIgnore

1bbe837

fetchGit/fetchTree: Improve exportIgnore, submodule interaction

99bd12f

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.

rl-next: Add *general* note about git fetcher reimpl

71d08af

fetchTree: Disallow combination of submodules and exportIgnore for now

692e919

Format

469cf26

roberth force-pushed the libfetchers-git-exportIgnore branch from efa08de to 469cf26 Compare January 12, 2024 15:03

roberth and others added 4 commits January 12, 2024 16:05

fetchTree/git: Don't expose exportIgnore attr

f68ad5a

libfetchers: Add CachingFilteringInputAccessor

d80c582

Co-authored-by: Eelco Dolstra <edolstra@gmail.com>

fetchTree/git: Cache export-ignore filter

274d887

CanonPath: Add rel_c_str()

15f7bda

Defensively because isRoot() is also defensive.

roberth requested a review from edolstra January 12, 2024 21:57

roberth merged commit 2a3c5e6 into master Jan 16, 2024

roberth deleted the libfetchers-git-exportIgnore branch January 16, 2024 22:03

		std::string pathStr {path.rel()};
		const char * pathCStr = pathStr.c_str();

Uh oh!

Conversation

roberth commented Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Change

TODO

Context

Priorities

Uh oh!

roberth commented Nov 29, 2023

Uh oh!

edolstra commented Nov 30, 2023

Uh oh!

dpulls bot commented Dec 8, 2023

Uh oh!

DavHau commented Dec 30, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nixos-discourse commented Jan 12, 2024

Uh oh!

edolstra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

roberth commented Nov 27, 2023 •

edited

Loading