[PATCH v2 0/4] Sideband the whole fetch v2 response

[Jonathan Tan <jonathantanmy@google.com>]

Like v1, this is on origin/ms/packet-err-check.

Thanks, Junio, for your reviews.

Jonathan Tan (4):
  pkt-line: introduce struct packet_writer
  sideband: reverse its dependency on pkt-line
  {fetch,upload}-pack: sideband v2 fetch response
  tests: define GIT_TEST_SIDEBAND_ALL

 Documentation/technical/protocol-v2.txt |  10 ++
 fetch-pack.c                            |  13 +-
 pkt-line.c                              | 121 +++++++++++++++---
 pkt-line.h                              |  34 +++++
 sideband.c                              | 161 ++++++++++++------------
 sideband.h                              |  14 ++-
 t/README                                |   5 +
 t/lib-httpd/apache.conf                 |   1 +
 t/t5537-fetch-shallow.sh                |   3 +-
 t/t5701-git-serve.sh                    |   2 +-
 t/t5702-protocol-v2.sh                  |   4 +-
 upload-pack.c                           | 131 +++++++++++--------
 12 files changed, 343 insertions(+), 156 deletions(-)

Interdiff against v1:
diff --git a/pkt-line.c b/pkt-line.c
index 71b17e6b93..69162f3990 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -447,16 +447,16 @@ int recv_sideband(const char *me, int in_stream, int out)
 
 	while (1) {
 		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
-		retval = diagnose_sideband(me, buf, len, 0);
+		retval = demultiplex_sideband(me, buf, len, 0);
 		switch (retval) {
-			case SIDEBAND_PRIMARY:
-				write_or_die(out, buf + 1, len - 1);
-				break;
-			case SIDEBAND_PROGRESS:
-				/* already written by diagnose_sideband() */
-				break;
-			default: /* flush or error */
-				return retval;
+		case SIDEBAND_PRIMARY:
+			write_or_die(out, buf + 1, len - 1);
+			break;
+		case SIDEBAND_PROGRESS:
+			/* already written by demultiplex_sideband() */
+			break;
+		default: /* errors: message already written */
+			return retval;
 		}
 	}
 }
@@ -474,6 +474,7 @@ void packet_reader_init(struct packet_reader *reader, int fd,
 	reader->buffer = packet_buffer;
 	reader->buffer_size = sizeof(packet_buffer);
 	reader->options = options;
+	reader->me = "git";
 }
 
 enum packet_read_status packet_reader_read(struct packet_reader *reader)
@@ -483,6 +484,10 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
 		return reader->status;
 	}
 
+	/*
+	 * Consume all progress and keepalive packets until a primary payload
+	 * packet is received
+	 */
 	while (1) {
 		int retval;
 		reader->status = packet_read_with_status(reader->fd,
@@ -494,29 +499,31 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
 							 reader->options);
 		if (!reader->use_sideband)
 			break;
-		retval = diagnose_sideband(reader->me, reader->buffer,
-					   reader->pktlen, 1);
+		retval = demultiplex_sideband(reader->me, reader->buffer,
+					      reader->pktlen, 1);
 		switch (retval) {
-			case SIDEBAND_PROTOCOL_ERROR:
-			case SIDEBAND_REMOTE_ERROR:
-				BUG("should have died in diagnose_sideband");
-			case SIDEBAND_FLUSH:
-				goto nonprogress;
-			case SIDEBAND_PRIMARY:
-				if (reader->pktlen != 1)
-					goto nonprogress;
-				/*
-				 * Since pktlen is 1, this is a keepalive
-				 * packet. Wait for the next one.
-				 */
-				break;
-			default: /* SIDEBAND_PROGRESS */
-				;
+		case SIDEBAND_PROTOCOL_ERROR:
+		case SIDEBAND_REMOTE_ERROR:
+			BUG("should have died in diagnose_sideband");
+		case SIDEBAND_FLUSH:
+			goto nonprogress_received;
+		case SIDEBAND_PRIMARY:
+			if (reader->pktlen != 1)
+				goto nonprogress_received;
+			/*
+			 * Since the packet contains nothing but the sideband
+			 * designator, this is a keepalive packet. Wait for the
+			 * next one.
+			 */
+			break;
+		default: /* SIDEBAND_PROGRESS */
+			;
 		}
 	}
 
-nonprogress:
+nonprogress_received:
 	if (reader->status == PACKET_READ_NORMAL)
+		/* Skip the sideband designator if sideband is used */
 		reader->line = reader->use_sideband ?
 			reader->buffer + 1 : reader->buffer;
 	else
@@ -549,7 +556,7 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
 
 	va_start(args, fmt);
 	packet_write_fmt_1(writer->dest_fd, 0,
-			   writer->use_sideband ? "\1" : "", fmt, args);
+			   writer->use_sideband ? "\001" : "", fmt, args);
 	va_end(args);
 }
 
@@ -559,7 +566,7 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
 
 	va_start(args, fmt);
 	packet_write_fmt_1(writer->dest_fd, 0,
-			   writer->use_sideband ? "\3" : "ERR ", fmt, args);
+			   writer->use_sideband ? "\003" : "ERR ", fmt, args);
 	va_end(args);
 }
 
diff --git a/sideband.c b/sideband.c
index cbeab380ae..9d3051e3fe 100644
--- a/sideband.c
+++ b/sideband.c
@@ -113,7 +113,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 #define ANSI_SUFFIX "\033[K"
 #define DUMB_SUFFIX "        "
 
-int diagnose_sideband(const char *me, char *buf, int len, int die_on_error)
+int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)
 {
 	static const char *suffix;
 	struct strbuf outbuf = STRBUF_INIT;
diff --git a/sideband.h b/sideband.h
index 3786670694..f75c4fde2a 100644
--- a/sideband.h
+++ b/sideband.h
@@ -8,15 +8,14 @@
 #define SIDEBAND_PROGRESS 2
 
 /*
- * buf and len should be the result of reading a line from a remote sending
- * multiplexed data.
+ * Inspects a multiplexed packet read from the remote and returns which
+ * sideband it is for.
  *
- * Determines the nature of the result and returns it. If
- * SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or SIDEBAND_PROGRESS, also
- * prints a message (or the formatted contents of the notice in the case of
- * SIDEBAND_PROGRESS) to stderr.
+ * If SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or SIDEBAND_PROGRESS,
+ * also prints a message (or the formatted contents of the notice in the case
+ * of SIDEBAND_PROGRESS) to stderr.
  */
-int diagnose_sideband(const char *me, char *buf, int len, int die_on_error);
+int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error);
 
 void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
 
-- 
2.19.0.271.gfe8321ec05.dirty