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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 28, 2022, 20:27 UTC
Message-ID: <xmqqo81p256z.fsf@gitster.g>
In-Reply-To: <CAPMMpojYJ9sB7nbAAt1b_yH0Um1O-+TpSRYXTkZ6aDHobhS59A@mail.gmail.com>

Tao Klerks <tao@klerks.biz> writes:

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

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