fetchTree/fetchGit: fix ref not respected when rev is set

[DavHau]

reverts #9410 (except using depth=1 when shallow=1 as that won't make sense anymore)
fixes #9667

[edolstra]

Isn't disabling shallow fetching a pretty massive performance regression?

[DavHau]

It performs badly, but not worse compared to the latest stable version. Stable nix never did shallow fetching in any scenario, even when shallow was set 'true'.

Setting shallow=true simply allowed the remote to be shallow, but cloned it as is. If you look at the code prior to libgit2, you will find that --depth was never set.
Possibly it wasn't intended like that originally, but fixing the behavior now would break compatibility.

If we care strongly about backward compatibility, the most plausible way forward seems to be introducing a new code path for shallow cloning, that's activated by a new flag, as discussed in #9402.

I already started with an implementation of this which I will try to finish by next week.

[DavHau]

The reason why we simply cannot do a depth=1 right now is:

1. We need to fetch by ref in order to verify that the revision is in fact in ref (ignoring that would break compat)
2. But if we fetch by ref then depth=1 will only ever result in the latest revision, which is not necessarily the revision we are looking for.

-> As long as we have to respect ref we can never fetch shallowly.

[edolstra]

Nix team discussion notes:

• Backwards compatibility doesn't extend to allowing things that were previously disallowed.
• If checking ref requires a non-shallow fetch, then setting shallow = true doesn't make sense and should probably be an error.

[roberth]

It seems that this affects the (internal) fetching of submodules as well.

nix/src/libfetchers/git.cc

Lines 620 to 622 in 9651034

	if (submodule.branch != "")
	attrs.insert_or_assign("ref", submodule.branch);
	attrs.insert_or_assign("rev", submoduleRev.gitRev());

[roberth]

• Backwards compatibility doesn't extend to allowing things that were previously disallowed.

That's unless the disallowing guarantees an important property, and in this case, I don't think it's desirable.
Suppose that ref was intended to be an extra check, then that check is not fixed by fetching less, because the local cache may already contain the desired rev. In other words, it was already broken.
Then there's an argument against checking ref for performance, and also one of practicality. If a ref is deleted (e.g. after merge), should that break expressions because of the ref check? That seems too much of a hindrance.

[x10an14]

I'm not sure if this is considered (the meeting summary doesn't mention it, and @roberth's #9668 (comment) only tangentially mentions it), but I feel like there's some lack of nuance regarding the utility of ref?

* If checking `ref` requires a non-shallow fetch, then setting `shallow = true` doesn't make sense and should probably be an error.

1. If the ref points to a git sha (e.g. commit/tag); shallow makes no sense. The git history/list of commits/list of tags has to be fetched to find/confirm the expected sha.
2. But if the ref points to a refname (examples of both cases can be found on Github), then shallow can make sense when pointing to a branch (like latest stable/main branch).

Examples: https://github.com/search?q=language%3ANix+shallow+%3D+path%3Aflake.nix&type=code&query=language%3ANix+shallow+%3D+path%3Aflake.nix

This is quickly typed out on handheld smartphone device. Will come w/a do-over edit later for any corrections/clarifications.

[DavHau]

Backwards compatibility doesn't extend to allowing things that were previously disallowed.

OK, I wasn't quite aware of that, but it seems to make sense

I agree with @roberth, checking for ref is broken by design as ref can change at any time on the remote and is not reproducible. So let's better ignore it.
Since we can fetch directly by rev there is really no good reason to consider ref anymore.

Closing this and the corresponding issue.