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

[Glen Choo <chooglen@google.com>]

Junio C Hamano <gitster@pobox.com> writes:

>> diff --git a/branch.c b/branch.c
>> index 6b31df539a5..5c28d432103 100644
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -18,9 +18,15 @@ struct tracking {
>>  	int matches;
>>  };
>>  
>> +struct find_tracked_branch_cb {
>> +	struct tracking *tracking;
>> +	struct strbuf remotes_advice;
>> +};
>> +
>>  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);
>>  		}
>> +		/*
>> +		 * TRANSLATORS: This is a line listing a remote with duplicate
>> +		 * refspecs, to be later included in advice message
>> +		 * ambiguousFetchRefspec. For RTL languages you'll probably want
>> +		 * to swap the "%s" and leading "  " space around.
>> +		 */
>> +		strbuf_addf(&ftb->remotes_advice, _("  %s\n"), remote->name);
>>  		tracking->spec.src = NULL;
>>  	}
>
> This is wasteful.  remotes_advice does not have to be filled when we
> are not going to give advice, i.e. there is only one matching remote
> and no second or subsequent ones, which should be the majority case.
> And we should not make majority of users who do not make a mistake
> that needs the advice message pay the cost of giving advice to those
> who screw up, if we can avoid it.
>
> Instead, only when we are looking at the second one, we can stuff
> tracking->remote (i.e. the first one) to remotes_advice, and then
> append remote->name there.  Perhaps we can do it like so?
>
> 	struct strbuf *names = &ftb->remotes_advice;
> 	const char *namefmt = N_("  %s\n");
>
> 	switch (++tracking->matches) {
> 	case 1:
> 		string_list_append(tracking->srcs, tracking->spec.src);
> 		tracking->remote = remote->name;
> 		break;
> 	case 2:
> 		strbuf_addf(names, _(namefmt), tracking->remote);
> 		/* fall through */
> 	default:
> 		strbuf_addf(names, _(namefmt), remote->name);
>                 free(tracking->spec.src);
>                 string_list_clear(tracking->srcs, 0);
>                 break;
> 	}
> 	tracking->spec.src = NULL;
>
> For a bonus point, remotes_advice member can be left empty without
> paying the cost to strbuf_addf() when the advice configuration says
> we are not going to show the message.
>
> Thanks.

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.