Re: [PATCH] Fix `git fetch --tags` in repo with no configured remote
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 6, 2024, 03:28 UTC
- Message-ID
- <xmqqcyi5zdzb.fsf@gitster.g>
- In-Reply-To
- <b41ae080654a3603af09801018df539f656cf9d8.1733430345.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
Show 34 quoted lines
> 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?
Show 60 quoted lines
> 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