Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 28, 2022, 17:23 UTC
Message-ID: <xmqqilry2dq7.fsf@gitster.g>
In-Reply-To: <kl6l7d8et314.fsf@chooglen-macbookpro.roam.corp.google.com>

Glen Choo <chooglen@google.com> writes:

Show 28 quoted lines

> Hm, what do you think of an alternate approach of storing of the
> matching remotes in a string_list, something like:
>
>   struct find_tracked_branch_cb {
>   	struct tracking *tracking;
>   	struct string_list matching_remotes;
>   };
>   
>   static int find_tracked_branch(struct remote *remote, void *priv)
>   {
>   	struct tracking *tracking = priv;
>   	struct find_tracked_branch_cb *ftb = priv;
>   	struct tracking *tracking = ftb->tracking;
>   
>   	if (!remote_find_tracking(remote, &tracking->spec)) {
>   		if (++tracking->matches == 1) {
>   			string_list_append(tracking->srcs, tracking->spec.src);
>   			tracking->remote = remote->name;
>   		} else {
>   			free(tracking->spec.src);
>   			string_list_clear(tracking->srcs, 0);
>   		}
>   		string_list_append(&ftb->matching_remotes, remote->name);
>   		tracking->spec.src = NULL;
>   	}
>
> then construct the advice message in setup_tracking()? To my untrained
> eye, "case 2" requires a bit of extra work to understand.

If I were writing this code from scratch, I would have probably collected the names first in this callback, and assembled them at the end into a single string when we need a single string to show.

Having said that, as long as you do that lazily not to penalize those who have sane setting without the need for advice/error to trigger, I do not particularly care how the list of matching remote names are kept. Having string_list_append() unconditionally like the above patch has, even for folks with just a single match without need for the advice/error message is suboptimal, I would think.