| Message ID | b41ae080654a3603af09801018df539f656cf9d8.1733430345.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix `git fetch --tags` in repo with no configured remote | expand |
Josh Steadmon <steadmon@google.com> writes: > In 3f763ddf28 (fetch: set remote/HEAD if it does not exist, 2024-11-22), > git-fetch learned to opportunistically set $REMOTE/HEAD when fetching. > However, this broke the logic for the `--tags` flag. 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. > > 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. > > Signed-off-by: Josh Steadmon <steadmon@google.com> > --- > builtin/fetch.c | 9 ++++----- > t/t5510-fetch.sh | 17 +++++++++++++++++ > 2 files changed, 21 insertions(+), 5 deletions(-) I see Bence is happy with the fix in a nearby message, so let me queue (and perhaps later amend it with Acked-by from Bence if we see one) the fix. Thanks.
Josh Steadmon <steadmon@google.com> writes: > In 3f763ddf28 (fetch: set remote/HEAD if it does not exist, 2024-11-22), > git-fetch learned to opportunistically set $REMOTE/HEAD when fetching. > However, this broke the logic for the `--tags` flag. 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. > > 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. > > Signed-off-by: Josh Steadmon <steadmon@google.com> > --- > builtin/fetch.c | 9 ++++----- > t/t5510-fetch.sh | 17 +++++++++++++++++ > 2 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/builtin/fetch.c b/builtin/fetch.c > index b2a36a5d95..e7b0c79678 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -1699,15 +1699,14 @@ static int do_fetch(struct transport *transport, > } > } > > + strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD"); > + > 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/"); > + strvec_push(&transport_ls_refs_options.ref_prefixes, > + "refs/tags/"); > } 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? 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? > Signed-off-by: Josh Steadmon <steadmon@google.com> > --- > builtin/fetch.c | 9 ++++----- > t/t5510-fetch.sh | 17 +++++++++++++++++ > 2 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/builtin/fetch.c b/builtin/fetch.c > index b2a36a5d95..e7b0c79678 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -1699,15 +1699,14 @@ static int do_fetch(struct transport *transport, > } > } > > + strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD"); > + > 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/"); > + strvec_push(&transport_ls_refs_options.ref_prefixes, > + "refs/tags/"); > } > > - strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD"); > - > if (must_list_refs) { > trace2_region_enter("fetch", "remote_refs", the_repository); > remote_refs = transport_get_remote_refs(transport, > 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 && > > base-commit: 3f763ddf28d28fe63963991513c8db4045eabadc
Junio C Hamano <gitster@pobox.com> writes: > 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? Micro-correction. "We grab tags only when we are fetching something else as well" is what I should have said. The general direction my comment leads to does not change, though. Because we ask for "HEAD" even when we do not need to, the additional change this patch names to unconditionally add "refs/tags/" would make us ask to list all the tags even when we do not need to see them. Thanks.
diff --git a/builtin/fetch.c b/builtin/fetch.c index b2a36a5d95..e7b0c79678 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1699,15 +1699,14 @@ static int do_fetch(struct transport *transport, } } + strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD"); + 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/"); + strvec_push(&transport_ls_refs_options.ref_prefixes, + "refs/tags/"); } - strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD"); - if (must_list_refs) { trace2_region_enter("fetch", "remote_refs", the_repository); remote_refs = transport_get_remote_refs(transport, 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 &&
In 3f763ddf28 (fetch: set remote/HEAD if it does not exist, 2024-11-22), git-fetch learned to opportunistically set $REMOTE/HEAD when fetching. However, this broke the logic for the `--tags` flag. 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. 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. Signed-off-by: Josh Steadmon <steadmon@google.com> --- builtin/fetch.c | 9 ++++----- t/t5510-fetch.sh | 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 5 deletions(-) base-commit: 3f763ddf28d28fe63963991513c8db4045eabadc