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

fetchGit: don't prefix "refs/heads/" on ref = "HEAD" #4676

Merged
merged 2 commits into from
Mar 30, 2021

Conversation

bjornfor
Copy link
Contributor

@bjornfor bjornfor commented Mar 27, 2021

This fixes builtins.fetchGit { url = ...; ref = "HEAD"; }, that works in
stable nix (v2.3.10), but is broken in nix master:

  $ ./result/bin/nix repl
  Welcome to Nix version 2.4pre19700101_dd77f71. Type :? for help.

  nix-repl> builtins.fetchGit { url = "https://github.com/NixOS/nix"; ref = "HEAD"; }
  fetching Git repository 'https://github.com/NixOS/nix'fatal: couldn't find remote ref refs/heads/HEAD
  error: program 'git' failed with exit code 128

The documentation for builtins.fetchGit says ref = "HEAD" is the
default, so it should also be supported to explicitly pass it.

I came across this issue because poetry2nix can use ref = "HEAD" in some
situations.

Fixes #4674.

I'm not 100 % sure how this change interacts with this nix git cache code (and the code that follows), I'd appreciate some review from somewone who knows the Nix code base :-)

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall.

I think it's worth a test though (tests/fetchGit.sh seems like the right place for that)

@@ -365,7 +365,9 @@ struct GitInputScheme : InputScheme
? "refs/*"
: ref->compare(0, 5, "refs/") == 0
? *ref
: "refs/heads/" + *ref;
: ref->compare("HEAD") == 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
: ref->compare("HEAD") == 0
: ref == "HEAD"

This fixes builtins.fetchGit { url = ...; ref = "HEAD"; }, that works in
stable nix (v2.3.10), but is broken in nix master:

  $ ./result/bin/nix repl
  Welcome to Nix version 2.4pre19700101_dd77f71. Type :? for help.

  nix-repl> builtins.fetchGit { url = "https://github.com/NixOS/nix"; ref = "HEAD"; }
  fetching Git repository 'https://github.com/NixOS/nix'fatal: couldn't find remote ref refs/heads/HEAD
  error: program 'git' failed with exit code 128

The documentation for builtins.fetchGit says ref = "HEAD" is the
default, so it should also be supported to explicitly pass it.

I came across this issue because poetry2nix can use ref = "HEAD" in some
situations.

Fixes NixOS#4674.
@bjornfor bjornfor force-pushed the fix-fetchgit-ref-head branch from 2464a92 to edd606a Compare March 30, 2021 09:21
@bjornfor
Copy link
Contributor Author

@regnat: Thanks for the review! I added your code suggestion and 2 tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

builtins.fetchGit { ..., ref = "HEAD"; } wrongly tries to fetch "refs/heads/HEAD"
3 participants