Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error
From: Glen Choo <hidden>
Date: 2022-03-28 17:12:14
Junio C Hamano [off-list ref] writes:
quoted
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.