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

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

Glen Choo <chooglen@google.com> writes:

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