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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 8, 2023, 23:46 UTC
Message-ID: <CAPig+cRGZvyhSs9=3-tkBKRZDjDUsb-VDs+dzOaZof__qyBjbA@mail.gmail.com>
In-Reply-To: <ZXOML2pcqVnVo0oX@nand.local>

On Fri, Dec 8, 2023 at 4:35 PM Taylor Blau <me@ttaylorr.com> wrote:

Show 15 quoted lines

> 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);