Re: [PATCH v4 2/5] strbuf_attach: fix all call sites to pass correct alloc

[Junio C Hamano <gitster@pobox.com>]

"Vaidas Pilkauskas via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> - mailinfo, am, refs/files-backend, fast-import, trailer: pass len+1
>   when the buffer is a NUL-terminated string (or from strbuf_detach).

These are good.

> - rerere, apply: ll_merge returns a buffer with exactly result.size
>   bytes (no extra NUL). Use strbuf_add() to copy and NUL-terminate
>   into the strbuf, then free the merge result, so alloc is correct.

I am not sure about this, because this will result in unnecessary
reallocation.

For example

> -	strbuf_attach(&image->buf, result.ptr, result.size, result.size);

This would have resulted in realloc(result.ptr, result.size + X) to
preserve the strbuf invariants that len + 1 <= alloc inside the
strbuf_attach().

It depends on what the system allocator does, but when X is a small
number, often no new memory needs to be carved out when this
realloc() happens, and all that needs to happen is that the size of
the memory region recorded by the system allocator is adjusted, and
the program will keep using the same memory region plus X bytes out
of the slop that has already been there when result.ptr was
allocated.  We will call realloc(), and it may result in a true
allocation and copy when X is larger than the existing slop, but it
may end up to be a cheap operation.

But if we rewrite it to do this ...

> +	strbuf_add(&image->buf, result.ptr, result.size);
> +	free(result.ptr);

... we will allocate as much as result.size and copy the bytes.
Guaranteed, regardless of how much slop the system allocator left
after result.ptr+result.size when it allocated result.ptr.

Of course, we could rewrite the original to

	result.ptr = realloc(result.ptr, result.size + 1);
        strbuf_attach(&image->buf, result.ptr, result.size, result.size + 1);

which would avoid the extra allocation and copy when there is even a
single byte of slop after result.ptr+result.size, but at that point,
for the sake of simplicity, we may be better off with the original
implementation of strbuf_attach() that automatically does that for
us.

So, I dunno.

Thanks.