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

[Tao Klerks <tao@klerks.biz>]

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?

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