Re: [PATCH] cache-tree: fix strbuf growth in prime_cache_tree_rec()

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 11, 2023, 02:46 UTC
Message-ID: <xmqqa61kgbrl.fsf@gitster.g>
In-Reply-To: <Y+b6TAmxel48QHJa@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 6 quoted lines

>> ...  I do not feel it too strongly but we
>> might want to rename _grow() to _grow_by() and make _grow() call it
>> while giving deprecation warning X-<.
>
> Having been confused by that myself, I would be happy to see such a
> name change.

If we did not know how useless explicit growth control is, we would likely have a pair of helpers, _grow_by() and _grow_to(), but given that ...

Show 7 quoted lines

>> There are ~45 calls to strbuf_grow() in C files other than strbuf.c;
>> I suspect probably a half or more of them can and should be removed
>> to reduce the resulting code size without hurting anything.
>
> My gut feeling is that your suspicion is giving strbuf_grow() users too
> much credit. ;) And having looked at the first 7 grep hits, every single
> one of them seemed pointless to me.

... we'd only have a very limited number of callers for which the helper makes sense, I am not sure if it is even worth the renaming.

Or just rename it to _grow_to() while fixing what it does, as grow_to() is what programmers would expect naturally?

> I wonder if these would make a good #leftoverbits / micro-project
> candidate.

The task is to pick one or two from these 45 hits, analyze what would happen if we remove the _grow() calls. For many of them, the result of such analysis would say the calls are pointless, but for some, there hopefully are solid reasons why explicit sizing makes sense. The former will be just removed, while the latter will be kept with a new in-code comment to record why it is worth having the call. The parameter may need to be updated for them at the same time.

It can be done extremely poorly without breaking anything in the test suite, or it can be done expertly. Unless the result are reviewed competently, it may make a rather poor micro-project experience.

So, I dunno.