[v4,4/6] clone: add tags refspec earlier to fetch refspec

Commit Message

Toon Claes Jan. 31, 2025, 3:30 p.m. UTC

In clone.c we call refspec_ref_prefixes() to copy the fetch refspecs
from the `remote->fetch` refspec into `ref_prefixes` of
`transport_ls_refs_options`. Afterward we add the tags prefix
`refs/tags/` prefix as well. At a later point, in wanted_peer_refs() we
process refs using both `remote->fetch` and `TAG_REFSPEC`.

Simplify the code by appending `TAG_REFSPEC` to `remote->fetch` before
calling refspec_ref_prefixes().

To be able to do this, we set `option_tags` to 0 when --mirror is given.
This is because --mirror mirrors (hence the name) all the refs,
including tags and they do not need to be treated separately.

Signed-off-by: Toon Claes <[e-mail]>
---
 builtin/clone.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

Comments

Patrick Steinhardt Feb. 3, 2025, 7:51 a.m. UTC | #1

On Fri, Jan 31, 2025 at 04:30:32PM +0100, Toon Claes wrote:
> In clone.c we call refspec_ref_prefixes() to copy the fetch refspecs
> from the `remote->fetch` refspec into `ref_prefixes` of
> `transport_ls_refs_options`. Afterward we add the tags prefix

s/Afterward/&s/

> diff --git a/builtin/clone.c b/builtin/clone.c
> index d652682494d0d27dd73cd0585e28b23f2883786d..7ab156ac00240de89baca6533ed2541839286fc4 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1375,14 +1365,15 @@ int cmd_clone(int argc,
>  		transport->smart_options->check_self_contained_and_connected = 1;
>  
>  	strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD");
> +
> +	if (option_tags || option_branch)
> +		refspec_append(&remote->fetch, TAG_REFSPEC);

It's a bit surprising that we also do this with `option_branch`, which
only seems to indicate which branch git-clone(1) is supposed to check
out. But in fact, the documentation mentions that it may also be used to
check out a tag. Principle of least surprise at its best.

In any case, I think it would be nice to have a comment here explaining
why this is the correct thing to do.

Patrick

Patch

diff --git a/builtin/clone.c b/builtin/clone.c
index d652682494d0d27dd73cd0585e28b23f2883786d..7ab156ac00240de89baca6533ed2541839286fc4 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -435,11 +435,8 @@  static struct ref *wanted_peer_refs(const struct ref *refs,
 	struct ref *head = copy_ref(find_ref_by_name(refs, "HEAD"));
 	struct ref *local_refs = head;
 	struct ref **tail = local_refs ? &local_refs->next : &local_refs;
-	struct refspec_item tag_refspec;
 	struct ref *to_free = NULL;
 
-	refspec_item_init(&tag_refspec, TAG_REFSPEC, 0);
-
 	if (option_single_branch) {
 		if (!option_branch)
 			refs = to_free = guess_remote_head(head, refs, 0);
@@ -454,16 +451,7 @@  static struct ref *wanted_peer_refs(const struct ref *refs,
 	for (int i = 0; i < refspec->nr; i++)
 		get_fetch_map(refs, &refspec->items[i], &tail, 0);
 
-	/*
-	 * Grab all refs that match the TAG_REFSPEC. Any tags we don't care
-	 * about won't be present in `refs` anyway.
-	 * Except with option --mirror, where we grab all refs already.
-	 */
-	if (!option_mirror)
-		get_fetch_map(refs, &tag_refspec, &tail, 0);
-
 	free_one_ref(to_free);
-	refspec_item_clear(&tag_refspec);
 
 	return local_refs;
 }
@@ -1011,8 +999,10 @@  int cmd_clone(int argc,
 			die(_("unknown ref storage format '%s'"), ref_format);
 	}
 
-	if (option_mirror)
+	if (option_mirror) {
 		option_bare = 1;
+		option_tags = 0;
+	}
 
 	if (option_bare) {
 		if (real_git_dir)
@@ -1375,14 +1365,15 @@  int cmd_clone(int argc,
 		transport->smart_options->check_self_contained_and_connected = 1;
 
 	strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD");
+
+	if (option_tags || option_branch)
+		refspec_append(&remote->fetch, TAG_REFSPEC);
+
 	refspec_ref_prefixes(&remote->fetch,
 			     &transport_ls_refs_options.ref_prefixes);
 	if (option_branch)
 		expand_ref_prefix(&transport_ls_refs_options.ref_prefixes,
 				  option_branch);
-	if (option_tags)
-		strvec_push(&transport_ls_refs_options.ref_prefixes,
-			    "refs/tags/");
 
 	refs = transport_get_remote_refs(transport, &transport_ls_refs_options);