On Thu Feb 13, 2025 at 23:38, Taylor Blau <me@xxxxxxxxxxxx> wrote: > On Thu, Jan 30, 2025 at 03:49:20AM +0000, Igor Todorovski wrote: >> Hi, we have noticed a change in behaviour with commit 3f763ddf28d28fe63963991513c8db4045eabadc. >> >> Here’s the steps to reproduce: >> >> mkdir git-test-dir >> cd git-test-dir >> git init --bare >> git remote add origin -- https://github.com/golang/go >> git -c protocol.version=2 fetch -f --depth=1 origin 16afa6a740fac7442e94dcd2ec5ea4a4853e45dc:refs/dummy >> git -c log.showsignature=false log --no-decorate -n1 --format="format:%H %ct %D" 16afa6a740fac7442e94dcd2ec5ea4a4853e45dc -- >> >> # Expected: >> # 16afa6a740fac7442e94dcd2ec5ea4a4853e45dc 1734108730 grafted, tag: go1.24rc1, refs/dummy >> >> # Tags are not fetch when using 2.48.1: >> # 16afa6a740fac7442e94dcd2ec5ea4a4853e45dc 1734108730 grafted >> >> --- >> >> git bisect revealed 3f763ddf28d28fe63963991513c8db4045eabadc as the culprit: >> >> commit 3f763ddf28d28fe63963991513c8db4045eabadc >> Author: Bence Ferdinandy >> Date: Fri Nov 22 13:28:50 2024 +0100 >> >> fetch: set remote/HEAD if it does not exist >> >> When cloning a repository remote/HEAD is created, but when the user >> creates a repository with git init, and later adds a remote, remote/HEAD >> is only created if the user explicitly runs a variant of "remote >> set-head". Attempt to set remote/HEAD during fetch, if the user does not >> have it already set. Silently ignore any errors. >> >> Signed-off-by: Bence Ferdinandy bence@xxxxxxxxxxxxxx >> Signed-off-by: Junio C Hamano gitster@xxxxxxxxx >> > > Thanks for the report and bisection recipe. I was able to bisect the > same issue myself, and also found myself at 3f763ddf28 (fetch: set > remote/HEAD if it does not exist, 2024-11-22). > >> Is this intended? > > I don't think this was intentional, though the commit's author Bence > (CC'd) can confirm. > > I suspect what's going on here is that in 3f763ddf28 and onwards we are > explicitly adding "HEAD" to the list of ref_prefixes, which causes the > server to respond only to the prefixes being asked for. In a > pre-3f763ddf28 world, the ref_prefixes list would be empty (if invoked > according to your script above), which allowed us to learn about any > tags pointing at that commit. > > One way to fix it is to move adding the "HEAD" prefix to above where we > check > > if (tags == TAGS_SET || tags == TAGS_DEFAULT) > > , which would allow us to enter the inner-most conditional which guards > us actually adding the refs/tags prefix to our list. > > But I don't love that solution, and I think even that is incomplete > since as of 6c915c3f85 (fetch: do not ask for HEAD unnecessarily, > 2024-12-06) we only ask for "HEAD" if we have a remote in the first > place. > > I think the real culprit is that we can no longer hold the same > assumption from e70a3030e7 (fetch: do not list refs if fetching only > hashes, 2018-09-27), which is that we can avoid asking for refs/tags as > an explicit prefix if we're (a) fetching literal hashes, (b) tag > following wasn't requested, and (c) the fetch is done with protocol v2. > > So I think the right fix would really be something like: > > --- 8< --- > diff --git a/builtin/fetch.c b/builtin/fetch.c > index fe2b26c74a..0e63621e6c 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -1770,9 +1770,8 @@ static int do_fetch(struct transport *transport, > > if (tags == TAGS_SET || tags == TAGS_DEFAULT) { > must_list_refs = 1; > - if (transport_ls_refs_options.ref_prefixes.nr) > - strvec_push(&transport_ls_refs_options.ref_prefixes, > - "refs/tags/"); > + strvec_push(&transport_ls_refs_options.ref_prefixes, > + "refs/tags/"); > } > > if (uses_remote_tracking(transport, rs)) { > --- >8 --- > > But I'm unfamiliar enough with this area that I'd appreciate comments > from the authors of these various commits, all of whom have been CC'd. > Does this seem right to you, or am I totally down the wrong path? This is the same error that came up already in another thread, and I came to a similar conclusion as you did (also about asking previous authors, which is why it is a bit stuck I guess): https://lore.kernel.org/git/D7D031QT4HEX.14TRNKRC6FC7S@xxxxxxxxxxxxxx/ I'll copy the relevant part of my previous message: What is not quite clear to me, is that it looks like that the original intention was to pretty much always fetch tags, yet it was not achieved by always pushing `refs/tags` into ref_prefixes. Deleting the check for `ref_prefixes` being empty [1] breaks quite a lot of things, but reversing the order [2] does not. That feels a bit strange tbh since it feels like the two should bring about the same state ... Hopefully someone more knowledgeable knows why things are as they are, but it seems that reversing the order really is a band-aid here. 1: https://github.com/ferdinandyb/git/commit/6074a9b8c88451e589eade4034282dd9b6c86345 2: https://github.com/ferdinandyb/git/commit/31e3f0a6b829d6c7953bf89d015b98e7edabe6b5 So there is some magic going on later on which I don't know enough about. Best, Bence
