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,
        if (packet_len <= 0)
         * 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).


Reply via email to