| Message ID | 61147be4b9a0ee76f1fe0f3376d7316205da350c.1741389941.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fetch: fix following tags when fetching specific OID | expand |
On Fri, Mar 07, 2025 at 06:27:03PM -0500, Taylor Blau wrote: > 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 && Oops. This line is stray from when I wrote the test with a more conservative approach that matches the above. It's harmless, but could equally be removed while queueing. Sorry for not catching earlier. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > 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). Good. "implicitly through a full advertisement" is a good thing to explicitly state here ;-) > 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. Thanks, both.
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!
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