On 25 Sep 2016, at 15:46, Jakub Narębski <jnareb@xxxxxxxxx> wrote:
> W dniu 20.09.2016 o 21:02, larsxschneider@xxxxxxxxx pisze:
>> From: Lars Schneider <larsxschneider@xxxxxxxxx>
>> +{
>> + static char buf[PKTLINE_DATA_MAXLEN];
>
> Sidenote: we have LARGE_PACKET_MAX (used in previous patch), but
> PKTLINE_DATA_MAXLEN not LARGE_PACKET_DATA_MAX.
Agreed, I will rename it.
>
>> + int err = 0;
>> + ssize_t bytes_to_write;
>> +
>> + while (!err) {
>> + bytes_to_write = xread(fd_in, buf, sizeof(buf));
>> + if (bytes_to_write < 0)
>> + return COPY_READ_ERROR;
>> + if (bytes_to_write == 0)
>> + break;
>> + err = packet_write_gently(fd_out, buf, bytes_to_write);
>> + }
>> + if (!err)
>> + err = packet_flush_gently(fd_out);
>> + return err;
>> +}
>
> Looks good: clean and readable.
>
> Sidenote (probably outside of scope of this patch): what are the
> errors that we can get from this function, beside COPY_READ_ERROR
> of course?
Everything that is returned by "read()"
>> +
>> static int get_packet_data(int fd, char **src_buf, size_t *src_size,
>> void *dst, unsigned size, int options)
>> {
>> @@ -305,3 +346,30 @@ char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)
>> {
>> return packet_read_line_generic(-1, src, src_len, dst_len);
>> }
>> +
>> +ssize_t read_packetized_to_buf(int fd_in, struct strbuf *sb_out)
>
> It's a bit strange that the signature of write_packetized_from_buf() is
> that different from read_packetized_to_buf(). This includes the return
> value: int vs ssize_t. As I have checked, write() and read() both
> use ssize_t, while fread() and fwrite() both use size_t.
read_packetized_to_buf() returns the number of bytes read or a negative
error code.
write_packetized_from_buf() returns 0 if the call was successful and an
error code if not.
That's the reason these two functions have a different signature
> Perhaps this function should be named read_packetized_to_strbuf()
> (err, I asked this already)?
I agree with the rename as makes it distinct from
write_packetized_from_buf().
>> +{
>> + int paket_len;
>
> Possible typo: shouldn't it be called packet_len?
True!
> Shouldn't it be initialized to 0?
Well, it is set for sure later. That's why I think it is not necessary.
Plus, Eric Wong thought me not to:
"Compilers complain about uninitialized variables."
http://public-inbox.org/git/20160725072745.GB11634@starla/
(Note: he was talking about pointers there :-)
>> + int options = PACKET_READ_GENTLE_ON_EOF;
>
> Why is this even a local variable? It is never changed, and it is
> used only in one place; we can inline it.
Removed.
>> +
>> + size_t oldlen = sb_out->len;
>> + size_t oldalloc = sb_out->alloc;
>
> Just a nitpick (feel free to ignore): doesn't this looks better:
>
> + size_t old_len = sb_out->len;
> + size_t old_alloc = sb_out->alloc;
>
> Also perhaps s/old_/orig_/g.
Agreed. That matches the other variables better.
>> + 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);
>
> A question (which perhaps was answered during the development of this
> patch series): why is this +1 in PKTLINE_DATA_MAXLEN+1 here?
Nice catch. I think this is wrong:
https://github.com/git/git/blob/6fe1b1407ed91823daa5d487abe457ff37463349/pkt-line.c#L196
It should be "if (len > size)" ... then we don't need the "+1" here.
(but I need to think a bit more about this)
>
>> + if (paket_len <= 0)
>> + break;
>> + sb_out->len += paket_len;
>> + }
>> +
>> + if (paket_len < 0) {
>> + if (oldalloc == 0)
>> + strbuf_release(sb_out);
>> + else
>> + strbuf_setlen(sb_out, oldlen);
>
> A question (maybe I don't understand strbufs): why there is a special
> case for oldalloc == 0?
I tried to mimic the behavior of strbuf_read() [1]. The error handling
was introduced in 2fc647 [2] to ease error handling:
"This allows for easier error handling, as callers only need to call
strbuf_release() if A) the command succeeded or B) if they would have had
to do so anyway because they added something to the strbuf themselves."
[1] https://github.com/git/git/blob/cda1bbd474805e653dda8a71d4ea3790e2a66cbb/strbuf.c#L377-L383
[2] https://github.com/git/git/commit/2fc647004ac7016128372a85db8245581e493812
Thanks,
Lars