git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: "René Scharfe" <l.s.r@web.de>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Git List" <git@vger.kernel.org>,
	"Victoria Dye" <vdye@github.com>
Subject: Re: [PATCH] cache-tree: fix strbuf growth in prime_cache_tree_rec()
Date: Fri, 10 Feb 2023 18:46:06 -0800	[thread overview]
Message-ID: <xmqqa61kgbrl.fsf@gitster.g> (raw)
In-Reply-To: <Y+b6TAmxel48QHJa@coredump.intra.peff.net> (Jeff King's message of "Fri, 10 Feb 2023 21:15:40 -0500")

Jeff King <peff@peff.net> writes:

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

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

  reply	other threads:[~2023-02-11  2:46 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-04 19:10 [PATCH] cache-tree: fix strbuf growth in prime_cache_tree_rec() René Scharfe
2023-02-05 21:12 ` Ævar Arnfjörð Bjarmason
2023-02-06 15:27   ` Derrick Stolee
2023-02-06 16:18     ` Ævar Arnfjörð Bjarmason
2023-02-12 11:20       ` René Scharfe
2023-02-06 18:55     ` Junio C Hamano
2023-02-10 20:20       ` René Scharfe
2023-02-10 20:20   ` René Scharfe
2023-02-10 20:33     ` Junio C Hamano
2023-02-11  2:15       ` Jeff King
2023-02-11  2:46         ` Junio C Hamano [this message]
2023-02-10 20:20 ` [PATCH v2] " René Scharfe
2023-02-13 13:37   ` Derrick Stolee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqa61kgbrl.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=l.s.r@web.de \
    --cc=peff@peff.net \
    --cc=vdye@github.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).