On Tue, Sep 27, 2016 at 10:14:16AM +0200, Lars Schneider wrote:
> >>> + strbuf_grow(sb_out, PKTLINE_DATA_MAXLEN+1);
> >>> + paket_len = packet_read(fd_in, NULL, NULL,
> >>> + sb_out->buf + sb_out->len, PKTLINE_DATA_MAXLEN+1,
> >>> options);
> [...]
> After looking at it with fresh eyes I think the existing code is probably
> correct,
> but maybe a bit confusing.
>
> packet_read() adds a '\0' at the end of the destination buffer:
> https://github.com/git/git/blob/21f862b498925194f8f1ebe8203b7a7df756555b/pkt-line.c#L206
>
> That is why the destination buffer needs to be one byte larger than the
> expected content.
>
> However, in this particular case that wouldn't be necessary because the
> destination
> buffer is a 'strbuf' that allocates an extra byte for '\0' at the end. But we
> are not
> supposed to write to this extra byte:
> https://github.com/git/git/blob/21f862b498925194f8f1ebe8203b7a7df756555b/strbuf.h#L25-L31
Right. The allocation happens as part of strbuf_grow(), but whatever
fills the buffer is expected to write the actual NUL (either manually,
or by calling strbuf_setlen().
I see the bit you quoted warns not to touch the extra byte yourself,
though I wonder if that is a bit heavy-handed (I guess it would matter
if we moved the extra 1-byte growth into strbuf_setlen(), but I find
that a rather unlikely change).
That being said, why don't you just use LARGE_PACKET_MAX here? It is
already the accepted size for feeding to packet_read(), and we know it
has enough space to hold a NUL terminator. Yes, we may over-allocate by
4 bytes, but that isn't really relevant. Strbufs over-allocate anyway.
So just:
for (;;) {
strbuf_grow(sb_out, LARGE_PACKET_MAX);
packet_len = packet_read(fd_in, NULL, NULL,
sb_out->buf + sb_out->len, LARGE_PACKET_MAX,
options);
if (packet_len <= 0)
break;
/*
* no need for strbuf_setlen() here; packet_read always adds a
* NUL terminator.
*/
sb_out->len += packet_len;
}
You _could_ make the final line of the loop use strbuf_setlen(); it
would just overwrite something we already know is a NUL (and we know
that no extra allocation is necessary).
Also, using LARGE_PACKET_MAX fixes the fact that this patch is using
PKTLINE_DATA_MAXLEN before it is actually defined. :)
You might want to occasionally run:
git rebase -x make
to make sure all of your incremental steps are valid (or even "make
test" if you are extremely patient; I often do that once after a big
round of complex interactive-rebase reordering).
> I see two options:
>
>
> (1) I leave the +1 as is and add a comment why the extra byte is necessary.
>
> Pro: No change in existing code necessary
> Con: The destination buffer has two '\0' at the end.
>
>
> (2) I add an option PACKET_READ_DISABLE_NUL_TERMINATION. If the option is
> set then no '\0' byte is added to the end.
>
> Pro: Correct solution, no byte wasted.
> Con: Change in existing code required.
>
>
> Any preference?
Of the two, I prefer (1), though I like what I suggested above even more
(big surprise, I know).
-Peff
