[v4,3/6] clone: refactor wanted_peer_refs()

Commit Message

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

The function wanted_peer_refs() is used to map the refs returned by the
server to refs we will save in our clone.

Over time this function grown to be very complex. Refactor it.

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

Comments

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

On Fri, Jan 31, 2025 at 04:30:31PM +0100, Toon Claes wrote:
> The function wanted_peer_refs() is used to map the refs returned by the
> server to refs we will save in our clone.
> 
> Over time this function grown to be very complex. Refactor it.

The diff of this commit is a bit on the harder side to read, so it would
be nice if the message guided the reader a bit.

> Signed-off-by: Toon Claes <[e-mail]>
> ---
>  builtin/clone.c | 39 +++++++++++++++------------------------
>  1 file changed, 15 insertions(+), 24 deletions(-)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 69d1ad029dfa84a2f7136fa4a3c4c8a594b179c4..d652682494d0d27dd73cd0585e28b23f2883786d 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -434,46 +434,37 @@ static struct ref *wanted_peer_refs(const struct ref *refs,
[snip]
> -	if (!option_mirror && !option_single_branch && option_tags)
> +	for (int i = 0; i < refspec->nr; i++)

While at it: this should be `size_t i` to match the type of
`refspec->nr`.

Patrick

Patch

diff --git a/builtin/clone.c b/builtin/clone.c
index 69d1ad029dfa84a2f7136fa4a3c4c8a594b179c4..d652682494d0d27dd73cd0585e28b23f2883786d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -434,46 +434,37 @@  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 = head ? &head->next : &local_refs;
+	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) {
-		struct ref *remote_head = NULL;
-
 		if (!option_branch)
-			remote_head = guess_remote_head(head, refs, 0);
+			refs = to_free = guess_remote_head(head, refs, 0);
 		else {
 			free_one_ref(head);
 			local_refs = head = NULL;
 			tail = &local_refs;
-			remote_head = copy_ref(find_remote_branch(refs, option_branch));
-		}
-
-		if (!remote_head && option_branch)
-			warning(_("Could not find remote branch %s to clone."),
-				option_branch);
-		else {
-			int i;
-			for (i = 0; i < refspec->nr; i++)
-				get_fetch_map(remote_head, &refspec->items[i],
-					      &tail, 0);
-
-			/* if --branch=tag, pull the requested tag explicitly */
-			get_fetch_map(remote_head, &tag_refspec, &tail, 0);
+			refs = to_free = copy_ref(find_remote_branch(refs, option_branch));
 		}
-		free_refs(remote_head);
-	} else {
-		int i;
-		for (i = 0; i < refspec->nr; i++)
-			get_fetch_map(refs, &refspec->items[i], &tail, 0);
 	}
 
-	if (!option_mirror && !option_single_branch && option_tags)
+	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;
 }