Re: Tags are no longer fetched when fetching specific commit

[Taylor Blau <me@ttaylorr.com>]

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@ferdinandy.com
>     Signed-off-by: Junio C Hamano gitster@pobox.com
>

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?

Thanks,
Taylor