On Mon, Mar 28 2022, Tao Klerks wrote:
> On Mon, Mar 28, 2022 at 7:23 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>
>> Glen Choo <chooglen@xxxxxxxxxx> writes:
>>
>> > Hm, what do you think of an alternate approach of storing of the
>> > matching remotes in a string_list, something like:
> [...]
>> > then construct the advice message in setup_tracking()? To my untrained
>> > eye, "case 2" requires a bit of extra work to understand.
>
> Interestingly, that was what I had in the original RFC. I started using
> the strbuf later, after Ævar confirmed that a single "advise()" call is
> the way to go. I understood building the string as we go to lead to
> simpler code, as it meant one less loop. On the other hand I
> understand Junio is more concerned about performance than the
> existence of a second loop that we should almost never hit.
>
> I'm very happy to switch from strbuf-building to string_list-appending,
> but I'm curious to understand how/why the performance of
> strbuf_addf() would be notably worse than that of
> string_list_append().
>
> Is there public doc about this somewhere?
We could do a string_list as in your v1 and concat it as we're
formatting it, but pushing new strings to a string_list v.s. appending
to a strbuf is actually more efficient in favor of the strbuf.
But as to not penalizing those who don't have the advice enabled,
something like this (untested)?:
diff --git a/branch.c b/branch.c
index 5c28d432103..83545456c36 100644
--- a/branch.c
+++ b/branch.c
@@ -20,6 +20,7 @@ struct tracking {
struct find_tracked_branch_cb {
struct tracking *tracking;
+ unsigned int do_advice:1;
struct strbuf remotes_advice;
};
@@ -36,6 +37,9 @@ static int find_tracked_branch(struct remote *remote, void *priv)
free(tracking->spec.src);
string_list_clear(tracking->srcs, 0);
}
+ tracking->spec.src = NULL;
+ if (!ftb->do_advice)
+ return 0;
/*
* TRANSLATORS: This is a line listing a remote with duplicate
* refspecs, to be later included in advice message
@@ -43,7 +47,6 @@ static int find_tracked_branch(struct remote *remote, void *priv)
* to swap the "%s" and leading " " space around.
*/
strbuf_addf(&ftb->remotes_advice, _(" %s\n"), remote->name);
- tracking->spec.src = NULL;
}
return 0;
@@ -249,6 +252,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
struct find_tracked_branch_cb ftb_cb = {
.tracking = &tracking,
.remotes_advice = STRBUF_INIT,
+ .do_advice = advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC),
};
memset(&tracking, 0, sizeof(tracking));
@@ -273,7 +277,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
if (tracking.matches > 1) {
int status = die_message(_("not tracking: ambiguous information for ref %s"),
orig_ref);
- if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))
+ if (ftb_cb.do_advice)
advise(_("There are multiple remotes whose fetch refspecs map to the remote\n"
"tracking ref %s:\n"
"%s"
