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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 28, 2022, 18:50 UTC
Message-ID: <220328.86y20t3o5s.gmgdl@evledraar.gmail.com>
In-Reply-To: <CAPMMpojYJ9sB7nbAAt1b_yH0Um1O-+TpSRYXTkZ6aDHobhS59A@mail.gmail.com>

On Mon, Mar 28 2022, Tao Klerks wrote:

Show 23 quoted lines

> On Mon, Mar 28, 2022 at 7:23 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> 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:
> [...]
>> > 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"