Re: [RFC PATCH] fetch-pack: try harder to read an ERR packet

[Jeff King <peff@peff.net>]

On Wed, Apr 22, 2020 at 05:27:44PM -0600, Taylor Blau wrote:

> > -		if (write_in_full(fd, buf->buf, buf->len) < 0)
> > +		if (write_in_full(fd, buf->buf, buf->len) < 0) {
> 
> This makes sense; if 'write_in_full' fails, we want to go into the
> error-handling case below.
> 
> But, I wonder if we could do better than re-using 'write_in_full' here.
> We definitely do want to write 'buf->len' bytes overall, but what
> happens when a 'write()' fails? I think it's at _that_ point we want to
> try and read a packet or two off from the remote.
> 
> What if instead this looked something like:
> 
>   const char *p = buf;
>   ssize_t total = 0;
> 
>   while (count > 0) {
>     ssize_t written = xwrite(fd, p, count);
>     if (written < 0)
>       return -1;
>     /* note the change on this line */
>     if (!written && packet_reader_read(reader) == PACKET_READ_EOF) {
>       errno = ENOSPC;
>       return -1;
>     }
>     count -= written;
>     p += written;
>     total += written;
>   }
> 
>   return total;
> 
> That is basically the definition of 'write_in_full', but when we didn't
> get a chance to write anything, then we try to read one packet.

I'm not sure I understand what this is trying to do. If write_in_full()
returns an error, then we know that write() just failed, and it would be
appropriate to check errno at that point and decide whether to read a
packet.

The code you've written above doesn't make sense to me. If we see an
error, we'd return _before_ doing any packet_reader_read() stuff. We'd
trigger it only when write() returns 0. But it should only do that if we
fed it 0 bytes, which we know we'd never pass (because we wouldn't run
the loop if count==0).

-Peff