Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fetchTree/fetchGit: add test for .gitattributes #9391

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

DavHau
Copy link
Member

@DavHau DavHau commented Nov 19, 2023

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

@DavHau DavHau requested a review from edolstra as a code owner November 19, 2023 13:38
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Nov 19, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-11-20-nix-team-meeting-minutes-105/35902/1

@thufschmitt
Copy link
Member

Discussed in the Nix maintenance meeting.

Not sure how to work forward with that (and #9240 probably just broke it), but we should at least either

  • Reimplement that on top of libgit2, or
  • Warn of the behaviour change

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-11-24-nix-team-meeting-minutes-106/35955/1

@roberth roberth force-pushed the fetchGit-test-gitattributes branch from e31e320 to e5c6a9e Compare November 27, 2023 14:26
@roberth
Copy link
Member

roberth commented Nov 27, 2023

Rebased

@roberth roberth added the fetching Networking with the outside (non-Nix) world, input locking label Nov 27, 2023
...with the intention to prevent future regressions in fetchGit
@DavHau DavHau force-pushed the fetchGit-test-gitattributes branch from e5c6a9e to eec2312 Compare December 30, 2023 10:04
@DavHau
Copy link
Member Author

DavHau commented Dec 30, 2023

I noticed there is actually another thing that gets processed by git-archive, which is export-subst.

I added a commit to the current PR adding a test which passes on the current stable nix but fails on master.

The bad news is that export-subst seems to be more complex to implement than export-ignore.

I posted some more info on a related issue on libgit2: libgit2/libgit2#918 (comment)

@roberth
Copy link
Member

roberth commented Dec 30, 2023

more complex

I wouldn't expect the log format to be particularly stable in future versions, so this could be painful to support. We'll probably want to support a subset, and error out if there's anything non-reproducible in there.
Even something seemingly innocuous like an abbreviated git hash might be impure by depending on the number of commits or objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fetching Networking with the outside (non-Nix) world, input locking with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: 🏁 Review
Development

Successfully merging this pull request may close these issues.

5 participants