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

[Junio C Hamano <gitster@pobox.com>]

Tao Klerks <tao@klerks.biz> writes:

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

Small things accumulate.

Unless you have to bend over backwards to do so, avoid computing
unconditionally what you only need in an error case.  People do not
make mistake all the time, and they shouldn't be forced to pay for
the possibility that other people may make mistakes and may want to
get help.

When you see that you are wasting cycles "just in case I may see an
error", you may stop, take a deep breath, and think if you can push
the extra work to error code path with equally simple and easy code.
And in this case, I think it is just as equally easy and simple as
the unconditional code in your patch to optimize for the case where
there is *no* duplicate.

It is a good discipline to learn, especially if you are new, as it
is something that stays with you even after you move on to different
projects.

> I of course completely understand optimizing anything that will
> end up looping, but this is a max of 1 iteration's savings; I would

When there is no error, you do not even need to keep the "names that
will become necessary for advice" at all, so you are comparing 0
with 1.  And if you read the no-error case of the suggested rewrite,
you'd realize that it is much easier to reason about.  You do not
have to wonder what the remotes_advice strbuf (or string_list) is
doing in the no-error code path at all.  This is not about
performance, it is about clarity of the code (not doing unnecessary
thing means doing only essential thing, after all).

Between strbuf appending and string_list appending, I do not
personally care.  As long as the code can produce the same output,
it is OK.  Using string_list _may_ have an advantage that string
formatting logic will be concentrated in a single place, as opposed
to strbuf appending, where part of formatting decisions need to be
made in the callback and other part is left in the advise-string.
And because this should all happen only in error code path, the
performance between two APIs would not matter at all (and for that
to truly hold, you need to stick to "do not unconditionally compute 
what you need only in an error case" discipline).