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

[Junio C Hamano <gitster@pobox.com>]

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

> @@ -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".

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

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