Re: [PATCH v2 5/5] notes: use strbuf_attach to take ownership of the object contents
- From
Jeff King <peff@peff.net>
- Date
- Feb 20, 2024, 02:12 UTC
- Message-ID
- <20240220021220.GC2713741@coredump.intra.peff.net>
- In-Reply-To
- <20240218195938.6253-6-maarten.bosmans@vortech.nl>
On Sun, Feb 18, 2024 at 08:59:38PM +0100, Maarten Bosmans wrote:
Show 13 quoted lines
> @@ -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).
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.
-Peff