Re: [PATCH v2 04/11] reftable/stack: verify that `reftable_stack_add()` uses auto-compaction

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1910 bytes --]

On Fri, Dec 08, 2023 at 06:46:33PM -0500, Eric Sunshine wrote:
> On Fri, Dec 8, 2023 at 4:35 PM Taylor Blau <me@ttaylorr.com> wrote:
> > On Fri, Dec 08, 2023 at 03:53:10PM +0100, Patrick Steinhardt wrote:
> > > +static void test_reftable_stack_add_performs_auto_compaction(void)
> > > +{
> > > +             char name[100];
> > > +             snprintf(name, sizeof(name), "branch%04d", i);
> > > +             ref.refname = name;
> >
> > Is there a reason that we have to use snprintf() here and not a strbuf?
> >
> > I would have expected to see something like:
> >
> >     struct strbuf buf = STRBUF_INIT;
> >     /* ... */
> >     strbuf_addf(&buf, "branch%04d", i);
> >     ref.refname = strbuf_detach(&buf, NULL);
> 
> If I'm reading the code correctly, this use of strbuf would leak each
> time through the loop.
> 
> > I guess it doesn't matter too much, but I think if we can avoid using
> > snprintf(), it's worth doing. If we must use snprintf() here, we should
> > probably use Git's xsnprintf() instead.
> 
> xstrfmt() from strbuf.h would be even simpler if the intention is to
> allocate a new string which will be freed later.
> 
> In this case, though, assuming I understand the intent, I think the
> more common and safe idiom in this codebase is something like this:
> 
>     struct strbuf name = STRBUF_INIT;
>     strbuf_addstr(&name, "branch");
>     size_t len = name.len;
>     for (...) {
>         strbuf_setlen(&name, len);
>         strbuf_addf(&name, "%04d", i);
>         ref.refname = name.buf;
>         ...
>     }
>     strbuf_release(&name);

Yeah, I'll convert this to use a `struct strbuf` instead. But instead of
tracking the length I'll just use a `strbuf_reset()` followed by
`strbuf_addf("branch-%04d")`. It's simpler to read and we don't need to
squeeze every last drop of performance out of this loop anyway.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]