From: Bence Ferdinandy <bence@ferdinandy.com>
Date: Sat, 08 Mar 2025 03:23:00 GMT
Subject: Re: [PATCH] fetch: fix following tags when fetching specific OID
Message-ID: <6d77cffc-94a3-42ca-938d-791256b508fd@ferdinandy.com>
In-Reply-To: <61147be4b9a0ee76f1fe0f3376d7316205da350c.1741389941.git.me@ttaylorr.com>

2025. márc. 8. 0:27:43 Taylor Blau <me@ttaylorr.com>:

> In 3f763ddf28 (fetch: set remote/HEAD if it does not exist, 
> 2024-11-22),
> unconditionally adds "HEAD" to the list of ref prefixes we send to the
> server.
>
> This breaks a core assumption that the list of prefixes we send to the
> server is complete. We must either send all prefixes we care about, or
> none at all (in the latter case the server then advertises everything).
>
> The tag following code is careful to only add "refs/tags/" to the list
> of prefixes if there are already entries in the prefix list. But 
> because
> the new code from 3f763ddf28 runs after the tag code, and because it
> unconditionally adds to the prefix list, we may end up with a prefix
> list that _should_ have "refs/tags/" in it, but doesn't.
>
> When that is the case, the server does not advertise any tags, and our
> auto-following breaks because we never learned about any tags in the
> first place.
>
> Fix this by only adding "HEAD" to the ref prefixes when we know that we
> are already limiting the advertisement. In either case we'll learn 
> about
> HEAD (either through the limited advertisement, or implicitly through a
> full advertisement).
>
> Reported-by: Igor Todorovski <itodorov@ca.ibm.com>
> Co-authored-by: Jeff King <peff@peff.net>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> Peff and I talked about this today, and neither of us could find any
> reasons not to pursue the approach listed in the footnote of
>
>   <20250221072558.GA572877@coredump.intra.peff.net>
>
> , but this is a more conservative approach that should fix the issue 
> and
> apply cleanly on top of 'maint'. It may be worth picking this into 
> 2.49,
> even though we are already quite late into the -rc cycle, this is a
> fairly nasty bug.
>
> builtin/fetch.c      |  4 +++-
> t/t5503-tagfollow.sh | 15 +++++++++++++++
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index fe2b26c74ae..a06d1305016 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1777,7 +1777,9 @@ static int do_fetch(struct transport *transport,
>
>     if (uses_remote_tracking(transport, rs)) {
>         must_list_refs = 1;
> -       strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD");
> +       if (transport_ls_refs_options.ref_prefixes.nr)
> +           strvec_push(&transport_ls_refs_options.ref_prefixes,
> +                   "HEAD");
>     }
>
>     if (must_list_refs) {
> diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
> index 195fc64dd44..8495730e264 100755
> --- a/t/t5503-tagfollow.sh
> +++ b/t/t5503-tagfollow.sh
> @@ -160,4 +160,19 @@ test_expect_success 'new clone fetch main and 
> tags' '
>     test_cmp expect actual
> '
>
> +test_expect_success 'fetch specific OID with tag following' '
> +   rm -f $U &&
> +   git init --bare clone3.git &&
> +   (
> +       cd clone3.git &&
> +       git remote add origin .. &&
> +       git fetch origin $B:refs/heads/main &&
> +
> +       git -C .. for-each-ref >expect &&
> +       git for-each-ref >actual &&
> +
> +       test_cmp expect actual
> +   )
> +'
> +
> test_done
>
> base-commit: f93ff170b93a1782659637824b25923245ac9dd1
> --
> 2.48.1.1.g965d2fe18fa

Thanks for cleaning up my mess!