Junio C Hamano <gitster@xxxxxxxxx> 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.
