Re: [PATCH v8 2/5] fetch: extract collect_upstream_from_remote() helper

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 10, 2026, 17:11 UTC
Message-ID: <xmqqik39iipg.fsf@gitster.g>
In-Reply-To: <08fea07a8be8a91ac54db44f9c035ecb49c86c9f.1791619334.git.gitgitgadget@gmail.com>

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 24 quoted lines

> @@ -1962,23 +1962,36 @@ static int do_fetch(struct transport *transport,
>  
>  	if (rs->nr) {
>  		refspec_ref_prefixes(rs, &transport_ls_refs_options.ref_prefixes);
>  	} else {
> -		struct branch *branch = branch_get(NULL);
>  
> -		if (transport->remote->fetch.nr) {
> -			refspec_ref_prefixes(&transport->remote->fetch,
> -					     &transport_ls_refs_options.ref_prefixes);
> -			if (follow_remote_head != FOLLOW_REMOTE_NEVER)
> -				do_set_head = 1;
> -		}
> -		if (branch && branch_has_merge_config(branch) &&
> -		    !strcmp(branch->remote_name, transport->remote->name)) {
> -			int i;
> -			for (i = 0; i < branch->merge_nr; i++) {
> -				strvec_push(&transport_ls_refs_options.ref_prefixes,
> -					    branch->merge[i]->src);
> -			}
> -		}
>  
>  		/*
>  		 * If there are no refs specified to fetch, then we just

We used to say "if there is remote.*.fetch, add them. whether there is remote.*.fetch or not, if the current branch builds on their branch(es), add them too".

Show 34 quoted lines

> @@ -1962,23 +1962,36 @@ static int do_fetch(struct transport *transport,
>  
>  	if (rs->nr) {
>  		refspec_ref_prefixes(rs, &transport_ls_refs_options.ref_prefixes);
> +	} else if (transport->remote->fetch.nr) {
> +		struct string_list tracked = STRING_LIST_INIT_DUP;
> +		struct string_list_item *item;
> +
> +		refspec_ref_prefixes(&transport->remote->fetch,
> +				     &transport_ls_refs_options.ref_prefixes);
> +		if (follow_remote_head != FOLLOW_REMOTE_NEVER)
> +			do_set_head = 1;
> +
> +		/*
> +		 * The configured refspec may not cover the current
> +		 * branch's upstream (e.g. a narrowed -t refspec), so
> +		 * make sure we can still fetch it regardless.
> +		 */
> +		collect_upstream_from_remote(the_repository, &tracked,
> +					      transport->remote, NULL);
> +		for_each_string_list_item(item, &tracked)
> +			strvec_push(&transport_ls_refs_options.ref_prefixes,
> +				    item->string);
> +		string_list_clear(&tracked, 0);
>  	} else {
> +		struct string_list tracked = STRING_LIST_INIT_DUP;
> +		struct string_list_item *item;
>  
> +		collect_upstream_from_remote(the_repository, &tracked,
> +					      transport->remote, NULL);
> +		for_each_string_list_item(item, &tracked)
> +			strvec_push(&transport_ls_refs_options.ref_prefixes,
> +				    item->string);
> +		string_list_clear(&tracked, 0);

This reorganizes it to say "if there is remote.*.fetch, do the same as before. If there is not remote.*.fetch, add their branch(es) the current branch wants".

This "duplicate what happens to the current branch" belongs to the next step where you want to add more logic to the latter part (i.e., "no remote.*.fetch" case), and not to this step.

IOW I would have expected to see

	} else {
		if (transport->remote->fetch.nr) {
			struct string_list tracked = STRING_LIST_INIT_DUP;
			struct string_list_item *item;

			refspec_ref_prefixes(&transport->remote->fetch,
					     &transport_ls_refs_options.ref_prefixes);
			if (follow_remote_head != FOLLOW_REMOTE_NEVER)
				do_set_head = 1;
		}

		/*
		 * The configured refspec may not cover the current
		 * branch's upstream (e.g. a narrowed -t refspec), so
		 * make sure we can still fetch it regardless.
		 */
		collect_upstream_from_remote(the_repository, &tracked,
					      transport->remote, NULL);
		for_each_string_list_item(item, &tracked)
			strvec_push(&transport_ls_refs_options.ref_prefixes,
				    item->string);
		string_list_clear(&tracked, 0);
	}

that touches "help the current branch when rs->nr == 0" part and nothing else.

Of course, because the next step [3/5] wants to split the above else{} block to do different things between the case where remote.*.fetch is and is not empty, at that point the extra code may be introduced there, ending up with the shape of if/else if cascade as we see in your patch.

Show 21 quoted lines

> diff --git a/remote.c b/remote.c
> index 99a086ea5a..5e980625b8 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1884,6 +1884,21 @@ int branch_merge_matches(struct branch *branch,
>  	return refname_match(branch->merge[i]->src, refname);
>  }
>  
> +void collect_upstream_from_remote(struct repository *repo,
> +				   struct string_list *tracked,
> +				   struct remote *remote,
> +				   const char *refname)
> +{
> +	struct branch *branch = repo_branch_get(repo, refname);
> +
> +	if (!branch_has_merge_config(branch) ||
> +	    strcmp(branch->remote_name, remote->name))
> +		return;
> +	for (int i = 0; i < branch->merge_nr; i++)
> +		string_list_insert(tracked, branch->merge[i]->src);
> +}

The original tries to avoid branch == NULL causing a segfault, but the above does not. Intended or overlooked?

Other than that, this looks like a straight-forward refactoring.