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

[Jeff King <peff@peff.net>]

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

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