builtins.fetchGit regression in 2.4 when coming from 2.3

[andir]

Describe the bug

When using builtins.fetchGit like below, Nix used to warn that the refname might be ambiguous but still did the right thing. With Nix 2.4 the warning is gone and Git fails to fetch the path leading to expressions that worked on 2.3 failing to build.

Steps To Reproduce

1. Create a file reproducer.nix:

# reproducer.nix
builtins.fetchGit {
  ref = "db1442a0556c2b133627ffebf455a78a1ced64b9";
  rev = "db1442a0556c2b133627ffebf455a78a1ced64b9";
  url = "https://github.com/tmcw/leftpad";
}

2. run nix-build reproducer.nix on both Nix 2.3 and 2.4
3. On 2.3 you should see

warning: refname 'db1442a0556c2b133627ffebf455a78a1ced64b9' is ambiguous.
Git normally never creates a ref that ends with 40 hex characters
because it will be ignored when you just specify 40-hex. These refs
may be created by mistake. For example,

  git switch -c $br $(git rev-parse ...)

where "$br" is somehow empty and a 40-hex ref is created. Please
examine these refs and maybe delete them. Turn this message off by
running "git config advice.objectNameWarning false"

Regardless of the above warning fetching works. It doesn't produce a result symlink as builtin fetchers tend to not do that when invoked outside of a derivation.

4. On 2.4 you will see

fatal: couldn't find remote ref refs/heads/db1442a0556c2b133627ffebf455a78a1ced64b9
error: program 'git' failed with exit code 128
(use '--show-trace' to show detailed location information)

Expected behavior

Nix should allow using the same fetchers as in previous versions even after upgrading even in the situation where the inputs to the fetchers were previously warned about.

Additional context

The problem was initially reported over here: nix-community/npmlock2nix#91

The way we are using builtins.fetchGit over there is super simple and pragmatic as NPM just throws some string at you. Sometimes it is a ref and in other scenarios a revision. I'll try to find a way to distinguish between them better and improve our code but IMO this bug is still valid as it breaks old expressions.

[yu-re-ka]

Passing allRefs = true; 'fixes' this on Nix unstable/master, but breaks stable nix.
However, this workaround should work on both:

# reproducer.nix
let
  ref = "db1442a0556c2b133627ffebf455a78a1ced64b9";
  rev = "db1442a0556c2b133627ffebf455a78a1ced64b9";
  url = "https://github.com/tmcw/leftpad";
in
if builtins.substring 0 3 builtins.nixVersion == "2.4" then
  builtins.fetchGit {
    inherit url rev;
    allRefs = true;
  }
else
  builtins.fetchGit {
    inherit url rev ref;
  }

[samueldr]

@edolstra can we either have a post-release errata about the compatibility issue with existing expressions, or is this something that should and can be fixed in 2.4.1?

[nixos-discourse]

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

https://discourse.nixos.org/t/nix-2-4-and-what-s-next/16257/1

[goertzenator]

Could allRefs default be changed to true? Or, could we have a switch for nix-build to trigger pre-2.4 fetchGit behavior?

Now that I've installed 22.05 and upgraded away from nix-2.3, this issue is causing me grief. Let me explain my unusual use-case:

We build several embedded products, and they are all put together with nix. The top level build products are "install" scripts that will partition, format, and install to a given disk device. All these scripts are added to a customized NixOS ISO image that we boot on the target hardware, and then run the "install" script for the specific product we are making that day. This works great... I can drop our code repo on a sparkling new NixOS machine and hit nix-build to repeatably and reliably build all our products. Not only that, I can build multiple versions of the same product in one go.

The embedded products are mostly nix-2.3 era but newer ones in development will use current nix and nixpkgs. I cannot build those older 2.3 era products with current nix-2.8 because of changes to fetchGit. Specifically, all those older products also have older nixpkgs that predate the allRefs parameter for fetchGit; I get git failures because the implicit 2.3 allRefs is now absent.

In the short term I will investigate using a nix with hacked fetchGit, but it would be good to not have to resort to hacked tools to do what I want.

EDIT: My hacked nix for those that are interested:

• shell.nix
• nix_fetchGit.patch

[edolstra]

The issue here is that db1442a0556c2b133627ffebf455a78a1ced64b9 is not a valid ref. So it was a bug in Nix 2.3 that it accepted that. Maybe for compatibility we could set allRefs = true if ref looks like a commit hash...

[toraritte]

In addition, allRefs only masked the incorrect ref so that only rev (a.k.a. commit hash) has been used to fetch the repo.

---

With that said, allRefss only seems to resolve ambiguities when there shouldn't be any to begin with because there is no real point using rev and rev together (at least, not in Nix 2.10.3). Came across some inconsistencies of builtins.fetchGit's behaviour while I was trying to understand and update the docs; some of these are documented on Discourse, but found some more since so starting with a recap:

rev

is simply an alias to "commit hash" and not a Git revision

ref

is ignored when rev is provided (sometimes altogether; see gist below)

allRefs

Calls with and without allRefs have the same output

allRefs is only used twice in Nixpkgs:

• once is in generateNix.js where a comment points to this issue

• the other is mk-poetry-dep.nix)

Using allRefs also seems to introduce idempotency issues when builtins.fetchGit is used repeatedly. Documented the calls in this gist, but was able to strictly reproduce only some of the issues.. It seems that fetchGit sometimes depends on an external state (maybe in ~/.cache?) that wreaks havoc at times.

edit: mv ~/.cache/nix{,_old} helped in a way that the first fetch with a branchname got resolved, but any subsequent calls just fetched the exact same output, so deleting/moving ~/.cache/nix is not required each time.

shallow

shallow defaults to false but then .git is always removed from the store so isn't it in effect true by default? Calls with shallow = true; differ only in revCount (0 when true, commit count otherwise).

edit-3: Just saw in git.cc#506 (some related commentary) that fetch() calls git init --bare at one point. This would explain the missing .git directory, but the store location is still pretty empty:

# State of ~/shed/my-project repo
* 5f45e9c (topic)
* 6692c9c 
* c277976 (main)
* abb0819 (HEAD)
* e67cb07 init

nix-repl> builtins.fetchGit { url = ~/shed/my-project; }
{ lastModified = 1658846012;
  lastModifiedDate = "20220726143332"; 
  narHash = "sha256-04L6PpdxbfBTc1EFlf2FKmtQuG7trnlzLU4XkY+DkSs=";
  outPath = "/nix/store/byn7arpnwbxmxnccvssvxw9c9wq7g6sd-source";
  rev = "abb08192ed875ef73fa66029994aa2f6700befd0";
  revCount = 2;
  shortRev = "abb0819"; 
  submodules = false; 
}

(abb0819...) [~/shed/my-project]$ ll /nix/store/byn7arpnwbxmxnccvssvxw9c9wq7g6sd-source/
total 11896
dr-xr-xr-x     2 root root       4096 Dec 31  1969 ./
drwxrwxr-t 14260 root nixbld 12169216 Jul 28 21:41 ../
-r--r--r--     1 root root         25 Dec 31  1969 a_file

Couldn't find a single use of shallow being used explicitly in Nixpkgs, but then there may be instances that are set programmatically and my regexes didn't catch those.

edit-2: Just saw that theshallow attribute has only recently been added to the docs (see commit).

[nixos-discourse]

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

https://discourse.nixos.org/t/rev-and-ref-attributes-in-builtins-fetchgit-and-maybe-flakes-too/20588/1

[nixos-discourse]

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

https://discourse.nixos.org/t/summer-of-nix-documentation-stream/20351/3

[nixos-discourse]

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

https://discourse.nixos.org/t/summer-of-nix-documentation-stream/20351/2

[tomberek]

This was called out as a bug in an earlier comment. There is currently work-in-progress for fetchGit,fetchTree and associated behavior.

needs a decision:

• BUG - auto detect if ref looks like a rev and set allRefs to true, document said behavior
  • edit: idea approved, team will review a proposed PR when available. Note the libgit2 rewrite and potential conflicts with (Refactor git fetcher #9031)
• WONT FIX - supporting rev in ref is a misfeature leading to longer-term confusion

[RaitoBezarius]

I guess this is related #5313.

[fricklerhandwerk]

Triaged in Nix team meeting:

If someone wants to make a PR that fixes the problem and issues a warning, we'll put it through the pipeline.

[nixos-discourse]

This issue 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

[tomberek]

Seems to be fixed between 2.19.2 and master.

[nixos-discourse]

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

https://discourse.nixos.org/t/2024-02-16-nix-team-meeting-minutes-124/39870/1

[tomberek]

Re-open if not fixed.