Re: [PATCH 06/10] builtin/gc.c: use "unsorted_string_list_has_string()" where appropriate

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Oct 27 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Refactor a "do I have an element like this?" pattern added in [1] and
>> [2] to use unsorted_string_list_has_string() instead of a
>> for_each_string_list_item() loop.
>
> In the longer term, I am not sure if we want to keep such code that
> uses string-list as a "database to be looked up with the string as
> the key".  I am not sure it is worth our review bandwidth to change
> a for-each-string-list that terminates early to its shorthand
> unsorted_string_list_has_string().  Surely each such conversation
> would allow us to lose 4 to 5 lines, but longer term we should be
> discuraging the use of unsorted_string_list_has_string() in the
> first place.

I haven't benchmarked, but I'd think on modern computers O(n) for such
short lists would be more performance due to cache locality, i.e. not
worth pre-sorting it, or making it a hash table.

But for such small amounts of data I'd think it would be fine either
way.

As to the change, I'm fine with leaving this out.

The reason it's in here is because this series came out as a reply to
Stolee's earlier RFC.

I think it's a fair summary to say that the reason we started talking
about this at all is because the topic at hand was how to make this
exact code in builtin/gc.c safer and more idiomatic. I.e. see:

	https://lore.kernel.org/git/e06cb4df081bc2222731f9185a22ed7ad67e3814.1664287711.git.gitgitgadget@gmail.com/

And my earlier summary of that, the very beginning showing the API forms
under discussion:

	https://lore.kernel.org/git/220928.868rm3w9d4.gmgdl@evledraar.gmail.com/

So Re this & your "it might be good, but why does it have to be done
here now?" reply to the CL: Yeah I can eject some of this, but having a
series (and a predecessor RFC) whose main reason for existing is making
this API safer & nicer seems incomplete unless we're also converting
callers to use those supposedly nicer patterns.

In general I think this sort of change is exactly the sort of thing
you'd want in such a series. A test of a good API isn't just that it
looks or acts nicely in isolation, but that it's easily combined with
other things you might expect to use with it.

Hence this 04-06/10. I.e. the original contention in the RFC was that we
had to return a dummy string list to make these sort of patterns
safer/nicer/idiomatic. I think these patches serve as a convincing
counter-argument to that.