Re* [PATCH] Fix `git fetch --tags` in repo with no configured remote

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 6, 2024, 08:08 UTC
Message-ID: <xmqqcyi5xmhr.fsf@gitster.g>
In-Reply-To: <xmqqcyi5zdzb.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 21 quoted lines

> 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?

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.
---
 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 &&

-- 
2.47.1-576-g9f8c8eb655