Op di 20 feb 2024 om 03:12 schreef Jeff King <peff@xxxxxxxx>:
>
> 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
