Re: Tags are no longer fetched when fetching specific commit

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 14, 2025, 18:35 UTC
Message-ID: <xmqqv7tcl6hp.fsf@gitster.g>
In-Reply-To: <Z650WoqFwCSo6svH@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 14 quoted lines

> 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/");

I guess the original "if we are giving prefixes already, then make sure we learn about tags" is because the semantics of the ref-prefix is peculiar, in that without _any_, there is no limitation and the remote end would advertise all its refs, but if there is even one, the remote end would advertise only the ones that match any prefix?

> +		strvec_push(&transport_ls_refs_options.ref_prefixes,
> +			    "refs/tags/");

So this change would make sure that, even if somebody else already added a prefix earlier, we would always ask about tags if we come into this block. It also means when nobody has added a prefix before we reach here, we still add prefix to ask about tags. The implication of which may be rather grave. If a later code relied on us _not_ asking for tags here (because nobody is limiting the ls-refs request with prefix), after this change, that code will now see that there is some prefix requested already, and if we wanted to retain what that code does, we would need to adjust that code (and then the need for similar modification cascades X-<).

Show 8 quoted lines

>  	}
>
>  	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?

I have no idea without doing a deep dive myself, for which I have failed to find time for so far.

I have to wonder if the first thing we should do is to update the logic to decide what ref prefixes to use, so that it does not do such an incremental strvec_push(). There are multiple questions in the form "Do we need to know about X?" the code path need to ask before deciding what prefixes to use, and before Bence's "We need to know about HEAD, because we want to know about its symref value", refs/tags hierarchy was one (and only?) thing we needed to ask about. The bug under discussion smells like the result of how brittle the code's assumption about what prefixes other parts would want to ask. This code did not assume there is anybody else, and that is why it broke when we started asking about HEAD. Now we have two possible X's in that question (refs/tags and HEAD). We may want to make sure the code is robust enough when we add the third one.

Thanks.