All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "René Scharfe" <l.s.r@web.de>
Cc: "Æ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 12:33:00 -0800	[thread overview]
Message-ID: <xmqqwn4pgt1f.fsf@gitster.g> (raw)
In-Reply-To: <850c94d5-973a-335e-903f-e0ab7e52f37e@web.de> ("René Scharfe"'s message of "Fri, 10 Feb 2023 21:20:19 +0100")

René Scharfe <l.s.r@web.de> writes:

>>> -				strbuf_grow(tree_path, base_path_len + entry.pathlen + 1);
>>> +				strbuf_grow(tree_path, entry.pathlen + 1);
>>>  				strbuf_add(tree_path, entry.path, entry.pathlen);
>>>  				strbuf_addch(tree_path, '/');
>>>  			}
>>
>> The size_t conversion is trivially correct.
>>
>> But what do you mean with "don't double the[...]"? Do you mean that this
>> manages to evade growing these to 24 etc?
>
> strbuf_setlen() truncates the string to the directory name.  strbuf_grow() then
> makes enough room to add that directory name again (that's what I mean with
> "double") plus the entry path.  We don't add the directory name a second time,
> so we don't need to make room for it.

Yeah, I think I made the same mistake number of years ago, thinking
that strbuf_grow() was to grow the buffer to the given size, but in
reality it is to grow the buffer by the given size, which felt a bit
unnatural, at least to me.  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-<.

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.


  reply	other threads:[~2023-02-10 20:33 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 [this message]
2023-02-11  2:15       ` Jeff King
2023-02-11  2:46         ` Junio C Hamano
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=xmqqwn4pgt1f.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=l.s.r@web.de \
    --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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.