Re: [PATCH v2 5/5] notes: use strbuf_attach to take ownership of the object contents

From: Maarten Bosmans <mkbosmans@gmail.com>
Date: Feb 20, 2024, 07:42 UTC
Message-ID: <CA+CvcKQyXu5JzhTnfQ7W2uaJNk_aP-2bh0HqP3npsfwUJn0Cew@mail.gmail.com>
In-Reply-To: <20240220021220.GC2713741@coredump.intra.peff.net>

Op di 20 feb 2024 om 03:12 schreef Jeff King <peff@peff.net>:

Show 21 quoted lines

>
> On Sun, Feb 18, 2024 at 08:59:38PM +0100, Maarten Bosmans wrote:
>
> > @@ -705,12 +703,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> >               if (!prev_buf)
> >                       die(_("unable to read %s"), oid_to_hex(note));
> >               if (size)
> > -                     strbuf_add(&buf, prev_buf, size);
> > +                     strbuf_attach(&buf, prev_buf, size, size + 1);
> >               if (d.buf.len && size)
> >                       append_separator(&buf);
> >               strbuf_insert(&d.buf, 0, buf.buf, buf.len);
> >
> > -             free(prev_buf);
> >               strbuf_release(&buf);
> >       }
>
> Is it possible for "size" to be 0, but prev_buf to be non-NULL? I assume
> it is so if the previous note is the empty object (and anyway, we'd have
> died earlier if prev_buf was NULL). In that case your patch introduces a
> leak (we do not attach prev_buf to buf, but we no longer free prev_buf).

You are right. I think the `if (size)` is not needed and removing it would remove the potential for a leak.

> I'm a little skeptical that this is actually increasing the speed of the
> command in a measurable way, though. It's one allocation/copy, right
> next to a big old strbuf_insert() that is going to splice into an
> existing array.

Yeah, I was doubting this patch a bit too. The simple idiom of starting with an empty strbuf and appending strings to it seems pretty nice and clear, so may be there's value in leaving it at that. The speed increase is not measurable of course. I was simply operating in full on lets-eliminate-all-sources-of-overhead mode while profiling the notes show code.

I'll drop the patch, in order to keep focus in this series.

Maarten