On Thu, Apr 23, 2020 at 1:27 AM Taylor Blau <me@xxxxxxxxxxxx> wrote: > > On Wed, Apr 22, 2020 at 06:33:57PM +0200, Christian Couder wrote: > > It looks like this may be missing a: > > From: SZEDER Gábor <szeder.dev@xxxxxxxxx> > > header. Yeah, I wanted to have him as the author but forgot. The next version will have it. [...] > > --- > > This just formats the following patch from SZEDER Gábor: > > > > https://lore.kernel.org/git/20190830121005.GI8571@xxxxxxxxxx/ > > > > I haven't tried to implement some suggestions discussed later > > in the above thread like: > > > > - renaming send_request() > > Agreed that this is probably something we should do. Maybe > 'send_request_retry' or something? That name is kind of terrible. Not sure 'send_request_retry' is better as we are not really retrying to send the request. My take would be something like 'send_request_read_err'. For now I have left it as is though. > > - covering more code pathes > > I see where Peff raised this point originally, but I think that this is > a good step forward as-is. No need to hold this up looking for complete > coverage, since this is obviously improving the situation. Ok. > > - avoid blocking indefinitely by looking for an ERR packet > > only if the write() resulted in ECONNRESET or EPIPE > > I think that I may have addressed this point below. > > > - first printing an error message with error_errno() before > > going on to look for an ERR packet > > I'm not sure what I think about this one. I'd certainly welcome all > opinions on this matter. > > > - implementing a timeout > > A timeout may be a good thing to do. See what you think about my > suggestion below, first, though. Ok, thanks for your suggestion. > > diff --git a/fetch-pack.c b/fetch-pack.c > > index 1734a573b0..63e8925e2b 100644 > > --- a/fetch-pack.c > > +++ b/fetch-pack.c > > @@ -185,14 +185,27 @@ static enum ack_type get_ack(struct packet_reader *reader, > > } > > > > static void send_request(struct fetch_pack_args *args, > > - int fd, struct strbuf *buf) > > + int fd, struct strbuf *buf, > > + struct packet_reader *reader) > > { > > if (args->stateless_rpc) { > > send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX); > > packet_flush(fd); > > } else { > > - 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. Yeah, good idea. > 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. Yeah, your code above looks correct. I have added a new function doing the above in the new version I will send soon. > This way, we only read exactly as many packets as we need to when we hit > this case. I'm not sure that it matters in practice, though. I am not sure I understand what you think doesn't matter in practice. Thanks, Christian.
