On Fri Dec 06, 2024 at 09:08, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Junio C Hamano <gitster@xxxxxxxxx> 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@xxxxxxxxxx> > [jc: tests are also borrowed from Josh's patch] > Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> > --- > > * 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 &&
