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

From: Tao Klerks <tao@klerks.biz>
Date: Mar 28, 2022, 18:02 UTC
Message-ID: <CAPMMpojYJ9sB7nbAAt1b_yH0Um1O-+TpSRYXTkZ6aDHobhS59A@mail.gmail.com>
In-Reply-To: <xmqqilry2dq7.fsf@gitster.g>

On Mon, Mar 28, 2022 at 7:23 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 5 quoted lines

>
> 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?

Show 6 quoted lines

> Having said that, as long as you do that lazily not to penalize
> those who have sane setting without the need for advice/error to
> trigger, I do not particularly care how the list of matching remote
> names are kept.  Having string_list_append() unconditionally like
> the above patch has, even for folks with just a single match without
> need for the advice/error message is suboptimal, I would think.

Again, I'm new here, and not a great coder to start with, but I'm having a hard time understanding why the single extra/gratuitous strbuf_addf() or string_list_append() call that we stand to optimize (I haven't understood whether there is a significant difference between them) would ever be noticeable in the context of creating a branch.

I of course completely understand optimizing anything that will end up looping, but this is a max of 1 iteration's savings; I would have thought that at these levels, readability/maintainability (and succinctness) of the code would trump any marginal performance savings.

To that end, I'd understand going back to string_list_append() as Glen proposes, and building a formatted string in a single place (setup_tracking()) only when required - both for readability, and in case some aspect of strbuf_addf() is non-trivially expensive - but is the "only append to the string_list() on the rare second pass" optimization really worth the increase in amount of code?

Is "performance over succinctness" a general principle that could or should be noted somewhere?