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

From: Christian Couder <christian.couder@gmail.com>
Date: Apr 22, 2020, 16:33 UTC
Message-ID: <20200422163357.27056-1-chriscool@tuxfamily.org>

When the server has hung up after sending an ERR packet to the client, the client might still be writing, for example a "done" line. Therefore the client might get a write error before reading the ERR packet.

When fetching this could result in the client displaying a "Broken pipe" error, instead of the more useful error sent by the server in the ERR packet.

Instead of immediately die()ing after write_in_full() returned an error, fetch should first try to read all incoming packets in the hope that the remote did send an ERR packet before it died, and then die with the error in that packet, or fall back to the current generic error message if there is no ERR packet (e.g. remote segfaulted or something similarly horrible).

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
This just formats the following patch from SZEDER Gábor:

https://lore.kernel.org/git/20190830121005.GI8571@szeder.dev/

I haven't tried to implement some suggestions discussed later in the above thread like:

  - renaming send_request()
  - covering more code pathes
  - avoid blocking indefinitely by looking for an ERR packet
    only if the write() resulted in ECONNRESET or EPIPE
  - first printing an error message with error_errno() before
    going on to look for an ERR packet
  - implementing a timeout

as it seems to me that there was no consensus about those.

It follows up from discussions in this thread:

https://lore.kernel.org/git/cover.1584477196.git.me@ttaylorr.com/

 fetch-pack.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

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) {
+			int save_errno = errno;
+			/*
+			 * Read everything the remote has sent to us.
+			 * If there is an ERR packet, then the loop die()s
+			 * with the received error message.
+			 * If we reach EOF without seeing an ERR, then die()
+			 * with a generic error message, most likely "Broken
+			 * pipe".
+			 */
+			while (packet_reader_read(reader) != PACKET_READ_EOF);
+			errno = save_errno;
 			die_errno(_("unable to write to remote"));
+		}
 	}
 }
 
@@ -349,7 +362,7 @@ static int find_common(struct fetch_negotiator *negotiator,
 		const char *arg;
 		struct object_id oid;
 
-		send_request(args, fd[1], &req_buf);
+		send_request(args, fd[1], &req_buf, &reader);
 		while (packet_reader_read(&reader) == PACKET_READ_NORMAL) {
 			if (skip_prefix(reader.line, "shallow ", &arg)) {
 				if (get_oid_hex(arg, &oid))
@@ -372,7 +385,7 @@ static int find_common(struct fetch_negotiator *negotiator,
 			die(_("expected shallow/unshallow, got %s"), reader.line);
 		}
 	} else if (!args->stateless_rpc)
-		send_request(args, fd[1], &req_buf);
+		send_request(args, fd[1], &req_buf, &reader);
 
 	if (!args->stateless_rpc) {
 		/* If we aren't using the stateless-rpc interface
@@ -395,7 +408,7 @@ static int find_common(struct fetch_negotiator *negotiator,
 			int ack;
 
 			packet_buf_flush(&req_buf);
-			send_request(args, fd[1], &req_buf);
+			send_request(args, fd[1], &req_buf, &reader);
 			strbuf_setlen(&req_buf, state_len);
 			flushes++;
 			flush_at = next_flush(args->stateless_rpc, count);
@@ -470,7 +483,7 @@ static int find_common(struct fetch_negotiator *negotiator,
 	trace2_region_leave("fetch-pack", "negotiation_v0_v1", the_repository);
 	if (!got_ready || !no_done) {
 		packet_buf_write(&req_buf, "done\n");
-		send_request(args, fd[1], &req_buf);
+		send_request(args, fd[1], &req_buf, &reader);
 	}
 	print_verbose(args, _("done"));
 	if (retval != 0) {

-- 
2.17.1