| Message ID | xmqqcyi5xmhr.fsf@gitster.g (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Re* [PATCH] Fix `git fetch --tags` in repo with no configured remote | expand |
On Fri Dec 06, 2024 at 09:08, Junio C Hamano <gitster@pobox.com> wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> Stepping back a bit, do we even need to learn where HEAD points at >> in the remote, when we are not doing the "opportunistically set >> $REMOTE/HEAD"? Your example is "in repo with no configured remote", >> which by definition means that we do not use any refs/remotes/*/ ref >> hierarchy to keep track of the remote-tracking branches for the >> remote we are fetching from. There is no place we record what we >> learn by running ls-remote HEAD against them, so should we even push >> "HEAD" to the ls-remote prefixes in such a case? Yes, it probably doesn't make any sense to do that. >> >> While this change may hide the breakage you saw in your set-up, we >> may be now asking to ls-remote HEAD even in cases we do not need to. >> >>> Fix this by adding HEAD to the ref_prefixes list prior to handling the >>> `--tags` flag, and removing the now obsolete check whether ref_prefixes >>> is empty or not. >> >> And if we unconditionally add HEAD even when we do not need to, >> especially with the loss of the ref-prefixes condition that was >> there in order to implement "learn refs/tags/* hierarchy only when >> we are doing the default fetch", wouldn't it mean we may learn >> refs/tags/* even when we do not have to? > > In other words, what I think the "fix" should look like is more like > the attached. It seems to pass your test, as well as existing tests > Bence added and other tests about "git fetch". > > One thing I am not happy about is the abstraction violation that is > needed to make the uses_remote_tracking() helper aware of the "use > the rs, the refspec given from the command line, or if it is empty, > use the configured 'fetch' refspec from the remote" rule, which is > primarily used by get_ref_map() that is much later called, but the > layering violation started when we started limiting the ls-remote > request with narrowing common prefixes, and it would take a larger > surgery to fix, I would think. > > ---- >8 ---- > Subject: [PATCH] fetch: do not ask for HEAD unnecessarily > > In 3f763ddf28 (fetch: set remote/HEAD if it does not exist, > 2024-11-22), git-fetch learned to opportunistically set $REMOTE/HEAD > when fetching by always asking for remote HEAD, in the hope that it > will help setting refs/remotes/<name>/HEAD if missing. > > But it is not needed to always ask for remote HEAD. When we are > fetching from a remote, for which we have remote-tracking branches, > we do need to know about HEAD. But if we are doing one-shot fetch, > e.g., > > $ git fetch --tags https://github.com/git/git > > we do not even know what sub-hierarchy of refs/remotes/<remote>/ > we need to adjust the remote HEAD for. There is no need to ask for > HEAD in such a case. > > Incidentally, because the unconditional request to list "HEAD" > affected the number of ref-prefixes requested in the ls-remote > request, this affected how the requests for tags are added to the > same ls-remote request, breaking "git fetch --tags $URL" performed > against a URL that is not configured as a remote. > > Reported-by: Josh Steadmon <steadmon@google.com> > [jc: tests are also borrowed from Josh's patch] > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > > * Even though I borrowed some part of the commit log message from > Josh's version, it not clear to me how "*after* deciding" led to > whatever the observed breakage (which was not described in the > log message), in the following part. > > Specifically, we now unconditionally add HEAD to the > ref_prefixes list, but we did this *after* deciding whether we > also need to explicitly request tags. > > Bence's change asks "HEAD" after "if we are fetching something, > then also ask about refs/tags/" logic thought we are not fetching > anything (i.e. ref_prefixes.nr == 0 at that point). But before > Bence's series, the same refs/tags/ logic saw that (ref_prefix.nr > == 0), didn't it? So that does not sound like a sufficient > explanation on how the series regressed. I did a bit of poking around on what is happening. For one I can confirm, that both before and after the set_head series `transport_ls_refs_options.ref_prefixes.nr` is 0. So the difference must be happening somewhere after that, and is not a side effect of calling set_head either, but I didn't manage to pin it down further. I also checked what happens in set_head, just to be on the safe side: `heads` is empty so we reach the if where we check `heads.nr` which is 0. So at least no strange refs are created :) > --- > builtin/fetch.c | 20 +++++++++++++++++++- > t/t5510-fetch.sh | 17 +++++++++++++++++ > 2 files changed, 36 insertions(+), 1 deletion(-) > > diff --git a/builtin/fetch.c b/builtin/fetch.c > index a64de4485f..3eb6f3acc9 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -1643,6 +1643,21 @@ static int set_head(const struct ref *remote_refs) > return result; > } > > +static int uses_remote_tracking(struct transport *transport, struct refspec *rs) > +{ > + if (!remote_is_configured(transport->remote, 0)) > + return 0; > + > + if (!rs->nr) > + rs = &transport->remote->fetch; > + > + for (int i = 0; i < rs->nr; i++) > + if (rs->items[i].dst) > + return 1; > + > + return 0; > +} > + > static int do_fetch(struct transport *transport, > struct refspec *rs, > const struct fetch_config *config) > @@ -1712,7 +1727,10 @@ static int do_fetch(struct transport *transport, > "refs/tags/"); > } > > - strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD"); > + if (uses_remote_tracking(transport, rs)) { > + must_list_refs = 1; > + strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD"); > + } > > if (must_list_refs) { > trace2_region_enter("fetch", "remote_refs", the_repository); > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh > index 87698341f5..d7602333ff 100755 > --- a/t/t5510-fetch.sh > +++ b/t/t5510-fetch.sh > @@ -189,6 +189,23 @@ test_expect_success 'fetch --prune --tags with refspec prunes based on refspec' > git rev-parse sometag > ' > > +test_expect_success 'fetch --tags gets tags even without a configured remote' ' > + REMOTE="$(pwd)/test_tag_1" && > + git init test_tag_1 && > + ( > + cd test_tag_1 && > + test_commit foo > + ) && > + git init test_tag_2 && > + ( > + cd test_tag_2 && > + git fetch --tags "file://$REMOTE" && > + echo "foo" >expect && > + git tag >actual && > + test_cmp expect actual > + ) > +' > + > test_expect_success REFFILES 'fetch --prune fails to delete branches' ' > cd "$D" && > git clone . prune-fail &&
Junio C Hamano <gitster@pobox.com> writes: > +static int uses_remote_tracking(struct transport *transport, struct refspec *rs) > +{ > + if (!remote_is_configured(transport->remote, 0)) > + return 0; > + > + if (!rs->nr) > + rs = &transport->remote->fetch; > + > + for (int i = 0; i < rs->nr; i++) > + if (rs->items[i].dst) > + return 1; > + > + return 0; > +} For the purpose of adjusting refs/remotes/<name>/HEAD, which is expected to point at something that we copy from their refs/heads/, it may be tempting to tighten the above logic to say, instead of "we have a refspec item that stores to somewhere", do something like for (int i = 0; i < rs->nr; i++) { struct refspec_item *item = &rs->items[i]; if (item->dst && item->src && starts_with(item->src, "refs/heads/")) return 1; } to make sure that we are tracking their branch refs, not some random hierarchy for which refs/remotes/<name>/HEAD does not matter. I will leave the simplest "just check we have .dst" version (in other words, without any additional check on the .src side) in my tree, but if anybody wants to pursue "let's make sure that .src copies from somewhere in refs/heads/", please make sure that you consider "--mirror", where an equivalent of "refs/*:refs/*" is used as the refspec, i.e. there is a case where .src does not begin with "refs/heads/" but does cover the hierarchy and wants to learn about "HEAD". Thanks.
diff --git a/builtin/fetch.c b/builtin/fetch.c index a64de4485f..3eb6f3acc9 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1643,6 +1643,21 @@ static int set_head(const struct ref *remote_refs) return result; } +static int uses_remote_tracking(struct transport *transport, struct refspec *rs) +{ + if (!remote_is_configured(transport->remote, 0)) + return 0; + + if (!rs->nr) + rs = &transport->remote->fetch; + + for (int i = 0; i < rs->nr; i++) + if (rs->items[i].dst) + return 1; + + return 0; +} + static int do_fetch(struct transport *transport, struct refspec *rs, const struct fetch_config *config) @@ -1712,7 +1727,10 @@ static int do_fetch(struct transport *transport, "refs/tags/"); } - strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD"); + if (uses_remote_tracking(transport, rs)) { + must_list_refs = 1; + strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD"); + } if (must_list_refs) { trace2_region_enter("fetch", "remote_refs", the_repository); diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 87698341f5..d7602333ff 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -189,6 +189,23 @@ test_expect_success 'fetch --prune --tags with refspec prunes based on refspec' git rev-parse sometag ' +test_expect_success 'fetch --tags gets tags even without a configured remote' ' + REMOTE="$(pwd)/test_tag_1" && + git init test_tag_1 && + ( + cd test_tag_1 && + test_commit foo + ) && + git init test_tag_2 && + ( + cd test_tag_2 && + git fetch --tags "file://$REMOTE" && + echo "foo" >expect && + git tag >actual && + test_cmp expect actual + ) +' + test_expect_success REFFILES 'fetch --prune fails to delete branches' ' cd "$D" && git clone . prune-fail &&