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

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

[Jonathan Tan]

This is on ms/packet-err-check.

This is a follow-up of my earlier work to allow partial CDN offloading
of fetch v2 response packfiles, which had the issue of having to buffer
(or suspend) progress and keepalive messages because we didn't have
sideband until the packfile section started (as I wrote here [1]). These
patches expand the sideband to the whole response, eliminating that
problem.

There are 8 patches total. I'm most interested in review of the first 4
patches for inclusion into Git - I think it would be useful for servers
to be allowed to send progress messages and other notices whenever they
need to, especially if other sections are added to the response (like in
the case of CDN offloading).

The other 4 patches are from my CDN offloading work, included here to
show how this sideband-all feature could be used. But take note that
they are all WIP and that I still haven't incorporated some of the
design considerations from the earlier review [2].

[1] https://public-inbox.org/git/20181206232538.141378-1-jonathantanmy@google.com/
[2] https://public-inbox.org/git/cover.1543879256.git.jonathantanmy@google.com/

Jonathan Tan (8):
  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: order protocol v2 sections
  Documentation: add Packfile URIs design doc
  upload-pack: refactor reading of pack-objects out
  upload-pack: send part of packfile response as uri

 Documentation/technical/packfile-uri.txt |  83 ++++++++
 Documentation/technical/protocol-v2.txt  |  32 ++-
 builtin/pack-objects.c                   |  48 +++++
 fetch-pack.c                             |  22 +-
 pkt-line.c                               | 114 ++++++++--
 pkt-line.h                               |  34 +++
 sideband.c                               | 161 +++++++-------
 sideband.h                               |  15 +-
 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                   |  31 ++-
 upload-pack.c                            | 259 +++++++++++++++--------
 14 files changed, 607 insertions(+), 203 deletions(-)
 create mode 100644 Documentation/technical/packfile-uri.txt

-- 
2.19.0.271.gfe8321ec05.dirty

[PATCH 1/4] pkt-line: introduce struct packet_writer

[Jonathan Tan]

It will be convenient for a future patch if writing options
(specifically, whether the written data is to be multiplexed) could be
controlled from a single place, so create struct packet_writer to serve
as that place, and modify upload-pack to use it.

Currently, it only stores the output fd, but a subsequent patch will (as
described above) introduce an option to determine if the written data is
to be multiplexed.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 pkt-line.c    |  47 ++++++++++++++++++---
 pkt-line.h    |  14 +++++++
 upload-pack.c | 112 +++++++++++++++++++++++++++-----------------------
 3 files changed, 115 insertions(+), 58 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index e70ea6d88f..9d3e402d41 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -129,12 +129,14 @@ static void set_packet_header(char *buf, const int size)
 	#undef hex
 }
 
-static void format_packet(struct strbuf *out, const char *fmt, va_list args)
+static void format_packet(struct strbuf *out, const char *prefix,
+			  const char *fmt, va_list args)
 {
 	size_t orig_len, n;
 
 	orig_len = out->len;
 	strbuf_addstr(out, "0000");
+	strbuf_addstr(out, prefix);
 	strbuf_vaddf(out, fmt, args);
 	n = out->len - orig_len;
 
@@ -145,13 +147,13 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)
 	packet_trace(out->buf + orig_len + 4, n - 4, 1);
 }
 
-static int packet_write_fmt_1(int fd, int gently,
+static int packet_write_fmt_1(int fd, int gently, const char *prefix,
 			      const char *fmt, va_list args)
 {
 	static struct strbuf buf = STRBUF_INIT;
 
 	strbuf_reset(&buf);
-	format_packet(&buf, fmt, args);
+	format_packet(&buf, prefix, fmt, args);
 	if (write_in_full(fd, buf.buf, buf.len) < 0) {
 		if (!gently) {
 			check_pipe(errno);
@@ -168,7 +170,7 @@ void packet_write_fmt(int fd, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	packet_write_fmt_1(fd, 0, fmt, args);
+	packet_write_fmt_1(fd, 0, "", fmt, args);
 	va_end(args);
 }
 
@@ -178,7 +180,7 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	status = packet_write_fmt_1(fd, 1, fmt, args);
+	status = packet_write_fmt_1(fd, 1, "", fmt, args);
 	va_end(args);
 	return status;
 }
@@ -211,7 +213,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	format_packet(buf, fmt, args);
+	format_packet(buf, "", fmt, args);
 	va_end(args);
 }
 
@@ -486,3 +488,36 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)
 	reader->line_peeked = 1;
 	return reader->status;
 }
+
+void packet_writer_init(struct packet_writer *writer, int dest_fd)
+{
+	writer->dest_fd = dest_fd;
+}
+
+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
+{
+	va_list args;
+
+	va_start(args, fmt);
+	packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args);
+	va_end(args);
+}
+
+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
+{
+	va_list args;
+
+	va_start(args, fmt);
+	packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args);
+	va_end(args);
+}
+
+void packet_writer_delim(struct packet_writer *writer)
+{
+	packet_delim(writer->dest_fd);
+}
+
+void packet_writer_flush(struct packet_writer *writer)
+{
+	packet_flush(writer->dest_fd);
+}
diff --git a/pkt-line.h b/pkt-line.h
index d7e1dbc047..023ad2951d 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -183,4 +183,18 @@ extern enum packet_read_status packet_reader_peek(struct packet_reader *reader);
 #define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)
 extern char packet_buffer[LARGE_PACKET_MAX];
 
+struct packet_writer {
+	int dest_fd;
+};
+
+void packet_writer_init(struct packet_writer *writer, int dest_fd);
+
+/* These functions die upon failure. */
+__attribute__((format (printf, 2, 3)))
+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...);
+__attribute__((format (printf, 2, 3)))
+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...);
+void packet_writer_delim(struct packet_writer *writer);
+void packet_writer_flush(struct packet_writer *writer);
+
 #endif
diff --git a/upload-pack.c b/upload-pack.c
index 08b547cf01..60a26bbbfd 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -613,13 +613,14 @@ static void check_non_tip(struct object_array *want_obj)
 	}
 }
 
-static void send_shallow(struct commit_list *result)
+static void send_shallow(struct packet_writer *writer,
+			 struct commit_list *result)
 {
 	while (result) {
 		struct object *object = &result->item->object;
 		if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
-			packet_write_fmt(1, "shallow %s",
-					 oid_to_hex(&object->oid));
+			packet_writer_write(writer, "shallow %s",
+					    oid_to_hex(&object->oid));
 			register_shallow(the_repository, &object->oid);
 			shallow_nr++;
 		}
@@ -627,7 +628,8 @@ static void send_shallow(struct commit_list *result)
 	}
 }
 
-static void send_unshallow(const struct object_array *shallows,
+static void send_unshallow(struct packet_writer *writer,
+			   const struct object_array *shallows,
 			   struct object_array *want_obj)
 {
 	int i;
@@ -636,8 +638,8 @@ static void send_unshallow(const struct object_array *shallows,
 		struct object *object = shallows->objects[i].item;
 		if (object->flags & NOT_SHALLOW) {
 			struct commit_list *parents;
-			packet_write_fmt(1, "unshallow %s",
-					 oid_to_hex(&object->oid));
+			packet_writer_write(writer, "unshallow %s",
+					    oid_to_hex(&object->oid));
 			object->flags &= ~CLIENT_SHALLOW;
 			/*
 			 * We want to _register_ "object" as shallow, but we
@@ -662,7 +664,7 @@ static void send_unshallow(const struct object_array *shallows,
 	}
 }
 
-static void deepen(int depth, int deepen_relative,
+static void deepen(struct packet_writer *writer, int depth, int deepen_relative,
 		   struct object_array *shallows, struct object_array *want_obj)
 {
 	if (depth == INFINITE_DEPTH && !is_repository_shallow(the_repository)) {
@@ -680,7 +682,7 @@ static void deepen(int depth, int deepen_relative,
 		result = get_shallow_commits(&reachable_shallows,
 					     depth + 1,
 					     SHALLOW, NOT_SHALLOW);
-		send_shallow(result);
+		send_shallow(writer, result);
 		free_commit_list(result);
 		object_array_clear(&reachable_shallows);
 	} else {
@@ -688,14 +690,15 @@ static void deepen(int depth, int deepen_relative,
 
 		result = get_shallow_commits(want_obj, depth,
 					     SHALLOW, NOT_SHALLOW);
-		send_shallow(result);
+		send_shallow(writer, result);
 		free_commit_list(result);
 	}
 
-	send_unshallow(shallows, want_obj);
+	send_unshallow(writer, shallows, want_obj);
 }
 
-static void deepen_by_rev_list(int ac, const char **av,
+static void deepen_by_rev_list(struct packet_writer *writer, int ac,
+			       const char **av,
 			       struct object_array *shallows,
 			       struct object_array *want_obj)
 {
@@ -703,13 +706,14 @@ static void deepen_by_rev_list(int ac, const char **av,
 
 	close_commit_graph(the_repository);
 	result = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);
-	send_shallow(result);
+	send_shallow(writer, result);
 	free_commit_list(result);
-	send_unshallow(shallows, want_obj);
+	send_unshallow(writer, shallows, want_obj);
 }
 
 /* Returns 1 if a shallow list is sent or 0 otherwise */
-static int send_shallow_list(int depth, int deepen_rev_list,
+static int send_shallow_list(struct packet_writer *writer,
+			     int depth, int deepen_rev_list,
 			     timestamp_t deepen_since,
 			     struct string_list *deepen_not,
 			     struct object_array *shallows,
@@ -720,7 +724,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,
 	if (depth > 0 && deepen_rev_list)
 		die("git upload-pack: deepen and deepen-since (or deepen-not) cannot be used together");
 	if (depth > 0) {
-		deepen(depth, deepen_relative, shallows, want_obj);
+		deepen(writer, depth, deepen_relative, shallows, want_obj);
 		ret = 1;
 	} else if (deepen_rev_list) {
 		struct argv_array av = ARGV_ARRAY_INIT;
@@ -741,7 +745,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,
 			struct object *o = want_obj->objects[i].item;
 			argv_array_push(&av, oid_to_hex(&o->oid));
 		}
-		deepen_by_rev_list(av.argc, av.argv, shallows, want_obj);
+		deepen_by_rev_list(writer, av.argc, av.argv, shallows, want_obj);
 		argv_array_clear(&av);
 		ret = 1;
 	} else {
@@ -834,8 +838,10 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
 	int has_non_tip = 0;
 	timestamp_t deepen_since = 0;
 	int deepen_rev_list = 0;
+	struct packet_writer writer;
 
 	shallow_nr = 0;
+	packet_writer_init(&writer, 1);
 	for (;;) {
 		struct object *o;
 		const char *features;
@@ -892,9 +898,9 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
 
 		o = parse_object(the_repository, &oid_buf);
 		if (!o) {
-			packet_write_fmt(1,
-					 "ERR upload-pack: not our ref %s",
-					 oid_to_hex(&oid_buf));
+			packet_writer_error(&writer,
+					    "upload-pack: not our ref %s",
+					    oid_to_hex(&oid_buf));
 			die("git upload-pack: not our ref %s",
 			    oid_to_hex(&oid_buf));
 		}
@@ -923,7 +929,7 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
 	if (depth == 0 && !deepen_rev_list && shallows.nr == 0)
 		return;
 
-	if (send_shallow_list(depth, deepen_rev_list, deepen_since,
+	if (send_shallow_list(&writer, depth, deepen_rev_list, deepen_since,
 			      &deepen_not, &shallows, want_obj))
 		packet_flush(1);
 	object_array_clear(&shallows);
@@ -1102,6 +1108,8 @@ struct upload_pack_data {
 	int deepen_rev_list;
 	int deepen_relative;
 
+	struct packet_writer writer;
+
 	unsigned stateless_rpc : 1;
 
 	unsigned use_thin_pack : 1;
@@ -1125,6 +1133,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
 	data->haves = haves;
 	data->shallows = shallows;
 	data->deepen_not = deepen_not;
+	packet_writer_init(&data->writer, 1);
 }
 
 static void upload_pack_data_clear(struct upload_pack_data *data)
@@ -1136,7 +1145,8 @@ static void upload_pack_data_clear(struct upload_pack_data *data)
 	string_list_clear(&data->deepen_not, 0);
 }
 
-static int parse_want(const char *line, struct object_array *want_obj)
+static int parse_want(struct packet_writer *writer, const char *line,
+		      struct object_array *want_obj)
 {
 	const char *arg;
 	if (skip_prefix(line, "want ", &arg)) {
@@ -1149,9 +1159,9 @@ static int parse_want(const char *line, struct object_array *want_obj)
 
 		o = parse_object(the_repository, &oid);
 		if (!o) {
-			packet_write_fmt(1,
-					 "ERR upload-pack: not our ref %s",
-					 oid_to_hex(&oid));
+			packet_writer_error(writer,
+					    "upload-pack: not our ref %s",
+					    oid_to_hex(&oid));
 			die("git upload-pack: not our ref %s",
 			    oid_to_hex(&oid));
 		}
@@ -1167,7 +1177,8 @@ static int parse_want(const char *line, struct object_array *want_obj)
 	return 0;
 }
 
-static int parse_want_ref(const char *line, struct string_list *wanted_refs,
+static int parse_want_ref(struct packet_writer *writer, const char *line,
+			  struct string_list *wanted_refs,
 			  struct object_array *want_obj)
 {
 	const char *arg;
@@ -1177,7 +1188,7 @@ static int parse_want_ref(const char *line, struct string_list *wanted_refs,
 		struct object *o;
 
 		if (read_ref(arg, &oid)) {
-			packet_write_fmt(1, "ERR unknown ref %s", arg);
+			packet_writer_error(writer, "unknown ref %s", arg);
 			die("unknown ref %s", arg);
 		}
 
@@ -1220,10 +1231,11 @@ static void process_args(struct packet_reader *request,
 		const char *p;
 
 		/* process want */
-		if (parse_want(arg, want_obj))
+		if (parse_want(&data->writer, arg, want_obj))
 			continue;
 		if (allow_ref_in_want &&
-		    parse_want_ref(arg, &data->wanted_refs, want_obj))
+		    parse_want_ref(&data->writer, arg, &data->wanted_refs,
+				   want_obj))
 			continue;
 		/* process have line */
 		if (parse_have(arg, &data->haves))
@@ -1317,26 +1329,26 @@ static int process_haves(struct oid_array *haves, struct oid_array *common,
 	return 0;
 }
 
-static int send_acks(struct oid_array *acks, struct strbuf *response,
+static int send_acks(struct packet_writer *writer, struct oid_array *acks,
 		     const struct object_array *have_obj,
 		     struct object_array *want_obj)
 {
 	int i;
 
-	packet_buf_write(response, "acknowledgments\n");
+	packet_writer_write(writer, "acknowledgments\n");
 
 	/* Send Acks */
 	if (!acks->nr)
-		packet_buf_write(response, "NAK\n");
+		packet_writer_write(writer, "NAK\n");
 
 	for (i = 0; i < acks->nr; i++) {
-		packet_buf_write(response, "ACK %s\n",
-				 oid_to_hex(&acks->oid[i]));
+		packet_writer_write(writer, "ACK %s\n",
+				    oid_to_hex(&acks->oid[i]));
 	}
 
 	if (ok_to_give_up(have_obj, want_obj)) {
 		/* Send Ready */
-		packet_buf_write(response, "ready\n");
+		packet_writer_write(writer, "ready\n");
 		return 1;
 	}
 
@@ -1348,25 +1360,20 @@ static int process_haves_and_send_acks(struct upload_pack_data *data,
 				       struct object_array *want_obj)
 {
 	struct oid_array common = OID_ARRAY_INIT;
-	struct strbuf response = STRBUF_INIT;
 	int ret = 0;
 
 	process_haves(&data->haves, &common, have_obj);
 	if (data->done) {
 		ret = 1;
-	} else if (send_acks(&common, &response, have_obj, want_obj)) {
-		packet_buf_delim(&response);
+	} else if (send_acks(&data->writer, &common, have_obj, want_obj)) {
+		packet_writer_delim(&data->writer);
 		ret = 1;
 	} else {
 		/* Add Flush */
-		packet_buf_flush(&response);
+		packet_writer_flush(&data->writer);
 		ret = 0;
 	}
 
-	/* Send response */
-	write_or_die(1, response.buf, response.len);
-	strbuf_release(&response);
-
 	oid_array_clear(&data->haves);
 	oid_array_clear(&common);
 	return ret;
@@ -1379,15 +1386,15 @@ static void send_wanted_ref_info(struct upload_pack_data *data)
 	if (!data->wanted_refs.nr)
 		return;
 
-	packet_write_fmt(1, "wanted-refs\n");
+	packet_writer_write(&data->writer, "wanted-refs\n");
 
 	for_each_string_list_item(item, &data->wanted_refs) {
-		packet_write_fmt(1, "%s %s\n",
-				 oid_to_hex(item->util),
-				 item->string);
+		packet_writer_write(&data->writer, "%s %s\n",
+				    oid_to_hex(item->util),
+				    item->string);
 	}
 
-	packet_delim(1);
+	packet_writer_delim(&data->writer);
 }
 
 static void send_shallow_info(struct upload_pack_data *data,
@@ -1398,14 +1405,15 @@ static void send_shallow_info(struct upload_pack_data *data,
 	    !is_repository_shallow(the_repository))
 		return;
 
-	packet_write_fmt(1, "shallow-info\n");
+	packet_writer_write(&data->writer, "shallow-info\n");
 
-	if (!send_shallow_list(data->depth, data->deepen_rev_list,
+	if (!send_shallow_list(&data->writer, data->depth,
+			       data->deepen_rev_list,
 			       data->deepen_since, &data->deepen_not,
 			       &data->shallows, want_obj) &&
 	    is_repository_shallow(the_repository))
-		deepen(INFINITE_DEPTH, data->deepen_relative, &data->shallows,
-		       want_obj);
+		deepen(&data->writer, INFINITE_DEPTH, data->deepen_relative,
+		       &data->shallows, want_obj);
 
 	packet_delim(1);
 }
@@ -1467,7 +1475,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
 			send_wanted_ref_info(&data);
 			send_shallow_info(&data, &want_obj);
 
-			packet_write_fmt(1, "packfile\n");
+			packet_writer_write(&data.writer, "packfile\n");
 			create_pack_file(&have_obj, &want_obj);
 			state = FETCH_DONE;
 			break;
-- 
2.19.0.271.gfe8321ec05.dirty

Re: [PATCH 1/4] pkt-line: introduce struct packet_writer

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

The patch itself look quite noisy, but in esssense, at the lowest
level we used to have a single format_packet() that was used to
write out normal payload and an error message prefixed with "ERR ";
now the users of the function are updated to call one of the two
helper functions, packet_writer_write() or packet_writer_error().
Most of the patch noise comes from the helper functions at higher
levels getting updated to pass the packet_writer struct through the
callchain.

Which makes tons of sense.

> It will be convenient for a future patch if writing options
> (specifically, whether the written data is to be multiplexed) could be
> controlled from a single place, so create struct packet_writer to serve
> as that place, and modify upload-pack to use it.

I've singled out "ERR " in my comment above, but this only refers to
"multiplexed".  Are there reasons why we want multiplexing other
than the "are we sending payload, or an error message"?

> Currently, it only stores the output fd, but a subsequent patch will (as
> described above) introduce an option to determine if the written data is
> to be multiplexed.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---

Re: [PATCH 1/4] pkt-line: introduce struct packet_writer

[Jonathan Tan]

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> The patch itself look quite noisy, but in esssense, at the lowest
> level we used to have a single format_packet() that was used to
> write out normal payload and an error message prefixed with "ERR ";
> now the users of the function are updated to call one of the two
> helper functions, packet_writer_write() or packet_writer_error().
> Most of the patch noise comes from the helper functions at higher
> levels getting updated to pass the packet_writer struct through the
> callchain.
> 
> Which makes tons of sense.

Yes, that's right. Thanks.

> > It will be convenient for a future patch if writing options
> > (specifically, whether the written data is to be multiplexed) could be
> > controlled from a single place, so create struct packet_writer to serve
> > as that place, and modify upload-pack to use it.
> 
> I've singled out "ERR " in my comment above, but this only refers to
> "multiplexed".  Are there reasons why we want multiplexing other
> than the "are we sending payload, or an error message"?

The main reason is to allow progress and keepalive messages at any time,
actually. I'll add this paragraph at the start of the commit message:

    A future patch will allow the client to request multiplexing of the
    entire fetch response (and not only during packfile transmission), which
    in turn allows the server to send progress and keepalive messages at any
    time during the response.

[PATCH 2/4] sideband: reverse its dependency on pkt-line

[Jonathan Tan]

A subsequent patch will teach struct packet_reader a new field that, if
set, instructs it to interpret read data as multiplexed. This will
create a dependency from pkt-line to sideband.

To avoid a circular dependency, split recv_sideband() into 2 parts: the
reading loop (left in recv_sideband()) and the processing of the
contents (in diagnose_sideband()), and move the former into pkt-line.
This reverses the direction of dependency: sideband no longer depends on
pkt-line, and pkt-line now depends on sideband.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 pkt-line.c |  22 ++++++++
 pkt-line.h |  16 ++++++
 sideband.c | 156 +++++++++++++++++++++++++----------------------------
 sideband.h |  15 +++++-
 4 files changed, 125 insertions(+), 84 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index 9d3e402d41..ebdc6c2530 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -439,6 +439,28 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)
 	return sb_out->len - orig_len;
 }
 
+int recv_sideband(const char *me, int in_stream, int out)
+{
+	char buf[LARGE_PACKET_MAX + 1];
+	int retval = 0;
+	int len;
+
+	while (1) {
+		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
+		retval = diagnose_sideband(me, buf, len);
+		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;
+		}
+	}
+}
+
 /* Packet Reader Functions */
 void packet_reader_init(struct packet_reader *reader, int fd,
 			char *src_buffer, size_t src_len,
diff --git a/pkt-line.h b/pkt-line.h
index 023ad2951d..a8e92a4b63 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -3,6 +3,7 @@
 
 #include "git-compat-util.h"
 #include "strbuf.h"
+#include "sideband.h"
 
 /*
  * Write a packetized stream, where each line is preceded by
@@ -120,6 +121,21 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
  */
 ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
 
+/*
+ * Receive multiplexed output stream over git native protocol.
+ * in_stream is the input stream from the remote, which carries data
+ * in pkt_line format with band designator.  Demultiplex it into out
+ * and err and return error appropriately.  Band #1 carries the
+ * primary payload.  Things coming over band #2 is not necessarily
+ * error; they are usually informative message on the standard error
+ * stream, aka "verbose").  A message over band #3 is a signal that
+ * the remote died unexpectedly.  A flush() concludes the stream.
+ *
+ * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR
+ * or SIDEBAND_REMOTE_ERROR if an error occurred.
+ */
+int recv_sideband(const char *me, int in_stream, int out);
+
 struct packet_reader {
 	/* source file descriptor */
 	int fd;
diff --git a/sideband.c b/sideband.c
index 368647acf8..842a92e975 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,7 +1,6 @@
 #include "cache.h"
 #include "color.h"
 #include "config.h"
-#include "pkt-line.h"
 #include "sideband.h"
 #include "help.h"
 
@@ -109,103 +108,94 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 }
 
 
-/*
- * Receive multiplexed output stream over git native protocol.
- * in_stream is the input stream from the remote, which carries data
- * in pkt_line format with band designator.  Demultiplex it into out
- * and err and return error appropriately.  Band #1 carries the
- * primary payload.  Things coming over band #2 is not necessarily
- * error; they are usually informative message on the standard error
- * stream, aka "verbose").  A message over band #3 is a signal that
- * the remote died unexpectedly.  A flush() concludes the stream.
- */
-
 #define DISPLAY_PREFIX "remote: "
 
 #define ANSI_SUFFIX "\033[K"
 #define DUMB_SUFFIX "        "
 
-int recv_sideband(const char *me, int in_stream, int out)
+int diagnose_sideband(const char *me, char *buf, int len)
 {
-	const char *suffix;
-	char buf[LARGE_PACKET_MAX + 1];
+	static const char *suffix;
 	struct strbuf outbuf = STRBUF_INIT;
 	int retval = 0;
+	const char *b, *brk;
+	int band;
+
+	if (!suffix) {
+		if (isatty(2) && !is_terminal_dumb())
+			suffix = ANSI_SUFFIX;
+		else
+			suffix = DUMB_SUFFIX;
+	}
 
-	if (isatty(2) && !is_terminal_dumb())
-		suffix = ANSI_SUFFIX;
-	else
-		suffix = DUMB_SUFFIX;
+	if (len == 0) {
+		retval = SIDEBAND_FLUSH;
+		goto cleanup;
+	}
+	if (len < 1) {
+		strbuf_addf(&outbuf,
+			    "%s%s: protocol error: no band designator",
+			    outbuf.len ? "\n" : "", me);
+		retval = SIDEBAND_PROTOCOL_ERROR;
+		goto cleanup;
+	}
+	band = buf[0] & 0xff;
+	buf[len] = '\0';
+	len--;
+	switch (band) {
+	case 3:
+		strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
+			    DISPLAY_PREFIX);
+		maybe_colorize_sideband(&outbuf, buf + 1, len);
+
+		retval = SIDEBAND_REMOTE_ERROR;
+		break;
+	case 2:
+		b = buf + 1;
 
-	while (!retval) {
-		const char *b, *brk;
-		int band, len;
-		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
-		if (len == 0)
-			break;
-		if (len < 1) {
-			strbuf_addf(&outbuf,
-				    "%s%s: protocol error: no band designator",
-				    outbuf.len ? "\n" : "", me);
-			retval = SIDEBAND_PROTOCOL_ERROR;
-			break;
-		}
-		band = buf[0] & 0xff;
-		buf[len] = '\0';
-		len--;
-		switch (band) {
-		case 3:
-			strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
-				    DISPLAY_PREFIX);
-			maybe_colorize_sideband(&outbuf, buf + 1, len);
-
-			retval = SIDEBAND_REMOTE_ERROR;
-			break;
-		case 2:
-			b = buf + 1;
-
-			/*
-			 * Append a suffix to each nonempty line to clear the
-			 * end of the screen line.
-			 *
-			 * The output is accumulated in a buffer and
-			 * each line is printed to stderr using
-			 * write(2) to ensure inter-process atomicity.
-			 */
-			while ((brk = strpbrk(b, "\n\r"))) {
-				int linelen = brk - b;
-
-				if (!outbuf.len)
-					strbuf_addstr(&outbuf, DISPLAY_PREFIX);
-				if (linelen > 0) {
-					maybe_colorize_sideband(&outbuf, b, linelen);
-					strbuf_addstr(&outbuf, suffix);
-				}
-
-				strbuf_addch(&outbuf, *brk);
-				xwrite(2, outbuf.buf, outbuf.len);
-				strbuf_reset(&outbuf);
-
-				b = brk + 1;
+		/*
+		 * Append a suffix to each nonempty line to clear the
+		 * end of the screen line.
+		 *
+		 * The output is accumulated in a buffer and
+		 * each line is printed to stderr using
+		 * write(2) to ensure inter-process atomicity.
+		 */
+		while ((brk = strpbrk(b, "\n\r"))) {
+			int linelen = brk - b;
+
+			if (!outbuf.len)
+				strbuf_addstr(&outbuf, DISPLAY_PREFIX);
+			if (linelen > 0) {
+				maybe_colorize_sideband(&outbuf, b, linelen);
+				strbuf_addstr(&outbuf, suffix);
 			}
 
-			if (*b) {
-				strbuf_addstr(&outbuf, outbuf.len ?
-					    "" : DISPLAY_PREFIX);
-				maybe_colorize_sideband(&outbuf, b, strlen(b));
-			}
-			break;
-		case 1:
-			write_or_die(out, buf + 1, len);
-			break;
-		default:
-			strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d",
-				    outbuf.len ? "\n" : "", me, band);
-			retval = SIDEBAND_PROTOCOL_ERROR;
-			break;
+			strbuf_addch(&outbuf, *brk);
+			xwrite(2, outbuf.buf, outbuf.len);
+			strbuf_reset(&outbuf);
+
+			b = brk + 1;
+		}
+
+		if (*b) {
+			strbuf_addstr(&outbuf, outbuf.len ?
+				    "" : DISPLAY_PREFIX);
+			maybe_colorize_sideband(&outbuf, b, strlen(b));
 		}
+		retval = SIDEBAND_PROGRESS;
+		break;
+	case 1:
+		retval = SIDEBAND_PRIMARY;
+		break;
+	default:
+		strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d",
+			    outbuf.len ? "\n" : "", me, band);
+		retval = SIDEBAND_PROTOCOL_ERROR;
+		break;
 	}
 
+cleanup:
 	if (outbuf.len) {
 		strbuf_addch(&outbuf, '\n');
 		xwrite(2, outbuf.buf, outbuf.len);
diff --git a/sideband.h b/sideband.h
index 7a8146f161..a56cd86287 100644
--- a/sideband.h
+++ b/sideband.h
@@ -3,8 +3,21 @@
 
 #define SIDEBAND_PROTOCOL_ERROR -2
 #define SIDEBAND_REMOTE_ERROR -1
+#define SIDEBAND_FLUSH 0
+#define SIDEBAND_PRIMARY 1
+#define SIDEBAND_PROGRESS 2
+
+/*
+ * buf and len should be the result of reading a line from a remote sending
+ * multiplexed data.
+ *
+ * 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.
+ */
+int diagnose_sideband(const char *me, char *buf, int len);
 
-int recv_sideband(const char *me, int in_stream, int out);
 void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
 
 #endif
-- 
2.19.0.271.gfe8321ec05.dirty

Re: [PATCH 2/4] sideband: reverse its dependency on pkt-line

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> +int recv_sideband(const char *me, int in_stream, int out)
> +{
> +	char buf[LARGE_PACKET_MAX + 1];
> +	int retval = 0;
> +	int len;
> +
> +	while (1) {
> +		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
> +		retval = diagnose_sideband(me, buf, len);

This name of the helper does not convey much useful information.
Let's think about it more later.

> +		switch (retval) {
> +			case SIDEBAND_PRIMARY:

Dedent; case/default arms align with 's' in switch in our codebase.

> +				write_or_die(out, buf + 1, len - 1);
> +				break;

OK, so PRIMARY return (band#1) gives us the payload unwrapped back
in "out" and we are responsible for writing it out here.

> +			case SIDEBAND_PROGRESS:
> +				/* already written by diagnose_sideband() */

True, that happens in "switch(band) { case 2: }" in the other helper.

> +				break;
> +			default: /* flush or error */
> +				return retval;

Lack of corresponding comment bothers readers.  In all of
REMOTE_ERROR, PROGRESS and PROTOCOL_ERROR cases, the other helper
stuffs the message in outbuf in "switch (band) { ... }" and writes
it out with xwrite(2, outbuf.buf, outbuf.len) [*1*], so I can see
there is no need for us to write anything out here.  Perhaps

		case SIDEBAND_FLUSH:
		default: /* errors: message already written */
			return retval;

or something to clarify?


> +/*
> + * Receive multiplexed output stream over git native protocol.
> + * in_stream is the input stream from the remote, which carries data
> + * in pkt_line format with band designator.  Demultiplex it into out
> + * and err and return error appropriately.  Band #1 carries the
> + * primary payload.  Things coming over band #2 is not necessarily
> + * error; they are usually informative message on the standard error
> + * stream, aka "verbose").  A message over band #3 is a signal that
> + * the remote died unexpectedly.  A flush() concludes the stream.
> + *
> + * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR
> + * or SIDEBAND_REMOTE_ERROR if an error occurred.
> + */
> +int recv_sideband(const char *me, int in_stream, int out);

This is well described.

> diff --git a/sideband.c b/sideband.c
> index 368647acf8..842a92e975 100644
> --- a/sideband.c
> +++ b/sideband.c
> ...
> +int diagnose_sideband(const char *me, char *buf, int len)
>  {
> +	static const char *suffix;
>  	struct strbuf outbuf = STRBUF_INIT;
>  	int retval = 0;
> +	const char *b, *brk;
> +	int band;
> +
> +	if (!suffix) {
> +		if (isatty(2) && !is_terminal_dumb())
> +			suffix = ANSI_SUFFIX;
> +		else
> +			suffix = DUMB_SUFFIX;
> +	}

It may be worth considering a further clean-up for the progress
code, that consumes lines in the "switch(band)" below that are
disproportionate to what it does in this function by introducing
another helper function that is called from here.  When it happens,
the above "suffix" thing should move to the helper function, too.

> +	if (len == 0) {
> +		retval = SIDEBAND_FLUSH;
> +		goto cleanup;
> +	}
> +	if (len < 1) {
> +		strbuf_addf(&outbuf,
> +			    "%s%s: protocol error: no band designator",
> +			    outbuf.len ? "\n" : "", me);
> +		retval = SIDEBAND_PROTOCOL_ERROR;
> +		goto cleanup;
> +	}
> +	band = buf[0] & 0xff;
> +	buf[len] = '\0';
> +	len--;
> +	switch (band) {
> +	case 3:
> +...
> +	case 2:
> +...
> +	case 1:
> +		retval = SIDEBAND_PRIMARY;
> +		break;
> +	default:
> +		strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d",
> +			    outbuf.len ? "\n" : "", me, band);
> +		retval = SIDEBAND_PROTOCOL_ERROR;
> +		break;

So, the point of this helper is to inspect a single packet-line data
and dispatch the payload of the packet based on which band it is
sent to.  Perhaps call it with a name with demultiplex or dispatch
in it?  "diagnose" is a bit unclear in that what trait you are
diagnosing and for what purpose.

> diff --git a/sideband.h b/sideband.h
> index 7a8146f161..a56cd86287 100644
> --- a/sideband.h
> +++ b/sideband.h
> @@ -3,8 +3,21 @@
>  
>  #define SIDEBAND_PROTOCOL_ERROR -2
>  #define SIDEBAND_REMOTE_ERROR -1
> +#define SIDEBAND_FLUSH 0
> +#define SIDEBAND_PRIMARY 1
> +#define SIDEBAND_PROGRESS 2
> +
> +/*
> + * buf and len should be the result of reading a line from a remote sending
> + * multiplexed data.
> + *
> + * Determines the nature of the result and returns it. If

"the result" may be from the point of view of the implementor or the
"recv_sideband()" function who called packet_read(), but a casual
reader would perceive it more natural if you referred it as "a
packet read from a remote".  "Inspect a packet read from the remote
and returns which sideband it is for", perhaps?


> + * 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 recv_sideband(const char *me, int in_stream, int out);
>  void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
>  
>  #endif

Re: [PATCH 2/4] sideband: reverse its dependency on pkt-line

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Lack of corresponding comment bothers readers.  In all of
> REMOTE_ERROR, PROGRESS and PROTOCOL_ERROR cases, the other helper
> stuffs the message in outbuf in "switch (band) { ... }" and writes
> it out with xwrite(2, outbuf.buf, outbuf.len) [*1*], so I can see
> there is no need for us to write anything out here.  Perhaps
>
> 		case SIDEBAND_FLUSH:
> 		default: /* errors: message already written */
> 			return retval;
>
> or something to clarify?

Forgot a footnote for *1* above.  I was wondering if xwrite is what
we really want, rather than write_in_full().

Re: [PATCH 2/4] sideband: reverse its dependency on pkt-line

[Jonathan Tan]

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Lack of corresponding comment bothers readers.  In all of
> > REMOTE_ERROR, PROGRESS and PROTOCOL_ERROR cases, the other helper
> > stuffs the message in outbuf in "switch (band) { ... }" and writes
> > it out with xwrite(2, outbuf.buf, outbuf.len) [*1*], so I can see
> > there is no need for us to write anything out here.  Perhaps
> >
> > 		case SIDEBAND_FLUSH:
> > 		default: /* errors: message already written */
> > 			return retval;
> >
> > or something to clarify?
> 
> Forgot a footnote for *1* above.  I was wondering if xwrite is what
> we really want, rather than write_in_full().

There is an existing comment explaining this:

	 * The output is accumulated in a buffer and
	 * each line is printed to stderr using
	 * write(2) to ensure inter-process atomicity.

which seems reasonable to me (I know that write(2) does not guarantee to
write everything, but write_in_full() does not guarantee atomicity, as
far as I know).

Re: [PATCH 2/4] sideband: reverse its dependency on pkt-line

[Jonathan Tan]

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > +int recv_sideband(const char *me, int in_stream, int out)
> > +{
> > +	char buf[LARGE_PACKET_MAX + 1];
> > +	int retval = 0;
> > +	int len;
> > +
> > +	while (1) {
> > +		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
> > +		retval = diagnose_sideband(me, buf, len);
> 
> This name of the helper does not convey much useful information.
> Let's think about it more later.

OK - after reading the later comments, switched to demultiplex_sideband.

> > +		switch (retval) {
> > +			case SIDEBAND_PRIMARY:
> 
> Dedent; case/default arms align with 's' in switch in our codebase.

Done.

> > +				break;
> > +			default: /* flush or error */
> > +				return retval;
> 
> Lack of corresponding comment bothers readers.  In all of
> REMOTE_ERROR, PROGRESS and PROTOCOL_ERROR cases, the other helper
> stuffs the message in outbuf in "switch (band) { ... }" and writes
> it out with xwrite(2, outbuf.buf, outbuf.len) [*1*], so I can see
> there is no need for us to write anything out here.  Perhaps
> 
> 		case SIDEBAND_FLUSH:
> 		default: /* errors: message already written */
> 			return retval;
> 
> or something to clarify?

Done.

> > +/*
> > + * Receive multiplexed output stream over git native protocol.
> > + * in_stream is the input stream from the remote, which carries data
> > + * in pkt_line format with band designator.  Demultiplex it into out
> > + * and err and return error appropriately.  Band #1 carries the
> > + * primary payload.  Things coming over band #2 is not necessarily
> > + * error; they are usually informative message on the standard error
> > + * stream, aka "verbose").  A message over band #3 is a signal that
> > + * the remote died unexpectedly.  A flush() concludes the stream.
> > + *
> > + * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR
> > + * or SIDEBAND_REMOTE_ERROR if an error occurred.
> > + */
> > +int recv_sideband(const char *me, int in_stream, int out);
> 
> This is well described.

Thanks, although the credit should be given to the original author - most of
this was moved.

> > diff --git a/sideband.c b/sideband.c
> > index 368647acf8..842a92e975 100644
> > --- a/sideband.c
> > +++ b/sideband.c
> > ...
> > +int diagnose_sideband(const char *me, char *buf, int len)
> >  {
> > +	static const char *suffix;
> >  	struct strbuf outbuf = STRBUF_INIT;
> >  	int retval = 0;
> > +	const char *b, *brk;
> > +	int band;
> > +
> > +	if (!suffix) {
> > +		if (isatty(2) && !is_terminal_dumb())
> > +			suffix = ANSI_SUFFIX;
> > +		else
> > +			suffix = DUMB_SUFFIX;
> > +	}
> 
> It may be worth considering a further clean-up for the progress
> code, that consumes lines in the "switch(band)" below that are
> disproportionate to what it does in this function by introducing
> another helper function that is called from here.  When it happens,
> the above "suffix" thing should move to the helper function, too.

That's reasonable. If it's OK, I would like to limit the scope of this
patch to splitting the existing recv_sideband() into recv_sideband() and
demultiplex_sideband() (the latter is quite close to the old
recv_sideband(), and the diff looks larger only because most of it is a
dedent).

> So, the point of this helper is to inspect a single packet-line data
> and dispatch the payload of the packet based on which band it is
> sent to.  Perhaps call it with a name with demultiplex or dispatch
> in it?  "diagnose" is a bit unclear in that what trait you are
> diagnosing and for what purpose.

Changed to demultiplex_sideband.

> > +/*
> > + * buf and len should be the result of reading a line from a remote sending
> > + * multiplexed data.
> > + *
> > + * Determines the nature of the result and returns it. If
> 
> "the result" may be from the point of view of the implementor or the
> "recv_sideband()" function who called packet_read(), but a casual
> reader would perceive it more natural if you referred it as "a
> packet read from a remote".  "Inspect a packet read from the remote
> and returns which sideband it is for", perhaps?

Done.

[PATCH 3/4] {fetch,upload}-pack: sideband v2 fetch response

[Jonathan Tan]

Currently, a response to a fetch request has sideband support only while
the packfile is being sent, meaning that the server cannot send notices
until the start of the packfile.

Extend sideband support in protocol v2 fetch responses to the whole
response. upload-pack will advertise it if the
uploadpack.allowsidebandall configuration variable is set, and
fetch-pack will automatically request it if advertised.

If the sideband is to be used throughout the whole response, upload-pack
will use it to send errors instead of prefixing a PKT-LINE payload with
"ERR ".

This will be tested in a subsequent patch.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 Documentation/technical/protocol-v2.txt | 10 +++++
 fetch-pack.c                            | 12 +++++-
 pkt-line.c                              | 51 +++++++++++++++++++------
 pkt-line.h                              |  4 ++
 sideband.c                              |  7 +++-
 sideband.h                              |  2 +-
 upload-pack.c                           | 16 ++++++++
 7 files changed, 87 insertions(+), 15 deletions(-)

diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 09e4e0273f..1b0633f59f 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -307,6 +307,16 @@ the 'wanted-refs' section in the server's response as explained below.
 	particular ref, where <ref> is the full name of a ref on the
 	server.
 
+If the 'sideband-all' feature is advertised, the following argument can be
+included in the client's request:
+
+    sideband-all
+	Instruct the server to send the whole response multiplexed, not just
+	the packfile section. All non-flush and non-delim PKT-LINE in the
+	response (not only in the packfile section) will then start with a byte
+	indicating its sideband (1, 2, or 3), and the server may send "0005\1"
+	(a PKT-LINE of sideband 1 with no payload) as a keepalive packet.
+
 The response of `fetch` is broken into a number of sections separated by
 delimiter packets (0001), with each section beginning with its section
 header.
diff --git a/fetch-pack.c b/fetch-pack.c
index 3f9626dbf7..b89199891d 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1125,7 +1125,8 @@ static int add_haves(struct fetch_negotiator *negotiator,
 static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
 			      const struct fetch_pack_args *args,
 			      const struct ref *wants, struct oidset *common,
-			      int *haves_to_send, int *in_vain)
+			      int *haves_to_send, int *in_vain,
+			      int sideband_all)
 {
 	int ret = 0;
 	struct strbuf req_buf = STRBUF_INIT;
@@ -1151,6 +1152,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
 		packet_buf_write(&req_buf, "include-tag");
 	if (prefer_ofs_delta)
 		packet_buf_write(&req_buf, "ofs-delta");
+	if (sideband_all)
+		packet_buf_write(&req_buf, "sideband-all");
 
 	/* Add shallow-info and deepen request */
 	if (server_supports_feature("fetch", "shallow", 0))
@@ -1359,6 +1362,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	packet_reader_init(&reader, fd[0], NULL, 0,
 			   PACKET_READ_CHOMP_NEWLINE |
 			   PACKET_READ_DIE_ON_ERR_PACKET);
+	if (server_supports_feature("fetch", "sideband-all", 0)) {
+		reader.use_sideband = 1;
+		reader.me = "fetch-pack";
+	}
 
 	while (state != FETCH_DONE) {
 		switch (state) {
@@ -1392,7 +1399,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		case FETCH_SEND_REQUEST:
 			if (send_fetch_request(&negotiator, fd[1], args, ref,
 					       &common,
-					       &haves_to_send, &in_vain))
+					       &haves_to_send, &in_vain,
+					       reader.use_sideband))
 				state = FETCH_GET_PACK;
 			else
 				state = FETCH_PROCESS_ACKS;
diff --git a/pkt-line.c b/pkt-line.c
index ebdc6c2530..71b17e6b93 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -447,7 +447,7 @@ 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);
+		retval = diagnose_sideband(me, buf, len, 0);
 		switch (retval) {
 			case SIDEBAND_PRIMARY:
 				write_or_die(out, buf + 1, len - 1);
@@ -483,16 +483,42 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
 		return reader->status;
 	}
 
-	reader->status = packet_read_with_status(reader->fd,
-						 &reader->src_buffer,
-						 &reader->src_len,
-						 reader->buffer,
-						 reader->buffer_size,
-						 &reader->pktlen,
-						 reader->options);
+	while (1) {
+		int retval;
+		reader->status = packet_read_with_status(reader->fd,
+							 &reader->src_buffer,
+							 &reader->src_len,
+							 reader->buffer,
+							 reader->buffer_size,
+							 &reader->pktlen,
+							 reader->options);
+		if (!reader->use_sideband)
+			break;
+		retval = diagnose_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 */
+				;
+		}
+	}
 
+nonprogress:
 	if (reader->status == PACKET_READ_NORMAL)
-		reader->line = reader->buffer;
+		reader->line = reader->use_sideband ?
+			reader->buffer + 1 : reader->buffer;
 	else
 		reader->line = NULL;
 
@@ -514,6 +540,7 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)
 void packet_writer_init(struct packet_writer *writer, int dest_fd)
 {
 	writer->dest_fd = dest_fd;
+	writer->use_sideband = 0;
 }
 
 void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
@@ -521,7 +548,8 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args);
+	packet_write_fmt_1(writer->dest_fd, 0,
+			   writer->use_sideband ? "\1" : "", fmt, args);
 	va_end(args);
 }
 
@@ -530,7 +558,8 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args);
+	packet_write_fmt_1(writer->dest_fd, 0,
+			   writer->use_sideband ? "\3" : "ERR ", fmt, args);
 	va_end(args);
 }
 
diff --git a/pkt-line.h b/pkt-line.h
index a8e92a4b63..ad9a4a2cd7 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -162,6 +162,9 @@ struct packet_reader {
 
 	/* indicates if a line has been peeked */
 	int line_peeked;
+
+	unsigned use_sideband : 1;
+	const char *me;
 };
 
 /*
@@ -201,6 +204,7 @@ extern char packet_buffer[LARGE_PACKET_MAX];
 
 struct packet_writer {
 	int dest_fd;
+	unsigned use_sideband : 1;
 };
 
 void packet_writer_init(struct packet_writer *writer, int dest_fd);
diff --git a/sideband.c b/sideband.c
index 842a92e975..cbeab380ae 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 diagnose_sideband(const char *me, char *buf, int len, int die_on_error)
 {
 	static const char *suffix;
 	struct strbuf outbuf = STRBUF_INIT;
@@ -144,6 +144,9 @@ int diagnose_sideband(const char *me, char *buf, int len)
 	len--;
 	switch (band) {
 	case 3:
+		if (die_on_error)
+			die("remote error: %s", buf + 1);
+
 		strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
 			    DISPLAY_PREFIX);
 		maybe_colorize_sideband(&outbuf, buf + 1, len);
@@ -196,6 +199,8 @@ int diagnose_sideband(const char *me, char *buf, int len)
 	}
 
 cleanup:
+	if (die_on_error && retval == SIDEBAND_PROTOCOL_ERROR)
+		die("%s", outbuf.buf);
 	if (outbuf.len) {
 		strbuf_addch(&outbuf, '\n');
 		xwrite(2, outbuf.buf, outbuf.len);
diff --git a/sideband.h b/sideband.h
index a56cd86287..3786670694 100644
--- a/sideband.h
+++ b/sideband.h
@@ -16,7 +16,7 @@
  * 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 diagnose_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);
 
diff --git a/upload-pack.c b/upload-pack.c
index 60a26bbbfd..765b7695d2 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -71,6 +71,8 @@ static int allow_filter;
 static int allow_ref_in_want;
 static struct list_objects_filter_options filter_options;
 
+static int allow_sideband_all;
+
 static void reset_timeout(void)
 {
 	alarm(timeout);
@@ -1046,6 +1048,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
 		allow_filter = git_config_bool(var, value);
 	} else if (!strcmp("uploadpack.allowrefinwant", var)) {
 		allow_ref_in_want = git_config_bool(var, value);
+	} else if (!strcmp("uploadpack.allowsidebandall", var)) {
+		allow_sideband_all = git_config_bool(var, value);
 	}
 
 	if (current_config_scope() != CONFIG_SCOPE_REPO) {
@@ -1284,6 +1288,11 @@ static void process_args(struct packet_reader *request,
 			continue;
 		}
 
+		if (allow_sideband_all && !strcmp(arg, "sideband-all")) {
+			data->writer.use_sideband = 1;
+			continue;
+		}
+
 		/* ignore unknown lines maybe? */
 		die("unexpected line: '%s'", arg);
 	}
@@ -1496,6 +1505,7 @@ int upload_pack_advertise(struct repository *r,
 	if (value) {
 		int allow_filter_value;
 		int allow_ref_in_want;
+		int allow_sideband_all_value;
 
 		strbuf_addstr(value, "shallow");
 
@@ -1510,6 +1520,12 @@ int upload_pack_advertise(struct repository *r,
 					 &allow_ref_in_want) &&
 		    allow_ref_in_want)
 			strbuf_addstr(value, " ref-in-want");
+
+		if (!repo_config_get_bool(the_repository,
+					 "uploadpack.allowsidebandall",
+					 &allow_sideband_all_value) &&
+		    allow_sideband_all_value)
+			strbuf_addstr(value, " sideband-all");
 	}
 
 	return 1;
-- 
2.19.0.271.gfe8321ec05.dirty

Re: [PATCH 3/4] {fetch,upload}-pack: sideband v2 fetch response

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Currently, a response to a fetch request has sideband support only while
> the packfile is being sent, meaning that the server cannot send notices
> until the start of the packfile.
>
> Extend sideband support in protocol v2 fetch responses to the whole
> response. upload-pack will advertise it if the
> uploadpack.allowsidebandall configuration variable is set, and
> fetch-pack will automatically request it if advertised.
>
> If the sideband is to be used throughout the whole response, upload-pack
> will use it to send errors instead of prefixing a PKT-LINE payload with
> "ERR ".

Makes sense.

> diff --git a/fetch-pack.c b/fetch-pack.c
> index 3f9626dbf7..b89199891d 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1125,7 +1125,8 @@ static int add_haves(struct fetch_negotiator *negotiator,
>  static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
>  			      const struct fetch_pack_args *args,
>  			      const struct ref *wants, struct oidset *common,
> -			      int *haves_to_send, int *in_vain)
> +			      int *haves_to_send, int *in_vain,
> +			      int sideband_all)
>  {
>  	int ret = 0;
>  	struct strbuf req_buf = STRBUF_INIT;
> @@ -1151,6 +1152,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
>  		packet_buf_write(&req_buf, "include-tag");
>  	if (prefer_ofs_delta)
>  		packet_buf_write(&req_buf, "ofs-delta");
> +	if (sideband_all)
> +		packet_buf_write(&req_buf, "sideband-all");
>  
>  	/* Add shallow-info and deepen request */
>  	if (server_supports_feature("fetch", "shallow", 0))
> @@ -1359,6 +1362,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  	packet_reader_init(&reader, fd[0], NULL, 0,
>  			   PACKET_READ_CHOMP_NEWLINE |
>  			   PACKET_READ_DIE_ON_ERR_PACKET);
> +	if (server_supports_feature("fetch", "sideband-all", 0)) {
> +		reader.use_sideband = 1;
> +		reader.me = "fetch-pack";
> +	}
>  
>  	while (state != FETCH_DONE) {
>  		switch (state) {
> @@ -1392,7 +1399,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  		case FETCH_SEND_REQUEST:
>  			if (send_fetch_request(&negotiator, fd[1], args, ref,
>  					       &common,
> -					       &haves_to_send, &in_vain))
> +					       &haves_to_send, &in_vain,
> +					       reader.use_sideband))
>  				state = FETCH_GET_PACK;
>  			else
>  				state = FETCH_PROCESS_ACKS;

OK, so the "fetch" side enables sideband-all any time it knows that
the other side supports it.

It feels a bit strange at the logical level that reader.me is set
only when we are going to talk over "sideband-all".  I know that at
the implementation level, reader.me is only looked at when sideband
is use, but it still feels somewhat funny.  Future developers may
want to use reader.me to identify the entity that found some error
during a read, even when the sideband is not yet in use, and at that
point, uninitialized .me would be a source of an error.  IOW, that
assignment smells like a timebomb waiting to go off.

> @@ -483,16 +483,42 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
>  		return reader->status;
>  	}
>  
> -	reader->status = packet_read_with_status(reader->fd,
> -						 &reader->src_buffer,
> -						 &reader->src_len,
> -						 reader->buffer,
> -						 reader->buffer_size,
> -						 &reader->pktlen,
> -						 reader->options);
> +	while (1) {
> +		int retval;
> +		reader->status = packet_read_with_status(reader->fd,
> +							 &reader->src_buffer,
> +							 &reader->src_len,
> +							 reader->buffer,
> +							 reader->buffer_size,
> +							 &reader->pktlen,
> +							 reader->options);
> +		if (!reader->use_sideband)
> +			break;
> +		retval = diagnose_sideband(reader->me, reader->buffer,
> +					   reader->pktlen, 1);
> +		switch (retval) {
> +			case SIDEBAND_PROTOCOL_ERROR:

Dedent (see previous step).

> +			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.
> +				 */

What guarantees that a normal payload packet is never of length==1?

> +				break;
> +			default: /* SIDEBAND_PROGRESS */
> +				;
> +		}
> +	}
>  
> +nonprogress:

It is totally unclear why this label is called nonprogress.  Is it
that the primary purpose of the while loop above is to spin and eat
the keep-alive packets from the other side?  If so, then it sort-of
makes sense (i.e. "we are looking for progress-meter related
packets, but now we found something else, so let's leave the loop").

It would have greatly helped reviewers to have a comment in front of
that infinite loop, perhaps like

	/*
	 * Spin and consume the keep-alive packets
	 */
	while (1) {
		int keepalive = 0;

		get_packet();

		/* decide if we got a keepalive here */
		...
		keepalive = ...;

		if (keepalive) {
			/* do the keep-alive thing here */
			...
			continue;
		}

		/* do one-off thing for non-keepalive packet if any */
		...
		break;
	}

>  	if (reader->status == PACKET_READ_NORMAL)
> -		reader->line = reader->buffer;
> +		reader->line = reader->use_sideband ?
> +			reader->buffer + 1 : reader->buffer;

Is the one byte the sideband designator?

>  void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
> @@ -521,7 +548,8 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
>  	va_list args;
>  
>  	va_start(args, fmt);
> -	packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args);
> +	packet_write_fmt_1(writer->dest_fd, 0,
> +			   writer->use_sideband ? "\1" : "", fmt, args);
>  	va_end(args);
>  }

As I am superstitious, I'd prefer to see octal literals to be fully
spelled out as 3-digit sequence, i.e. "\001".  Likewise for "\003".

> @@ -530,7 +558,8 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
>  	va_list args;
>  
>  	va_start(args, fmt);
> -	packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args);
> +	packet_write_fmt_1(writer->dest_fd, 0,
> +			   writer->use_sideband ? "\3" : "ERR ", fmt, args);

OK, band#3 is an error message for emergency exit.  It is a bit
curious that this patch does not show any line from the existing
code in the context that reacts to the ERR string, but that is
because the errors are noticed at a lot lower level when the
sideband is in use than the code that currently checks for errors?

Re: [PATCH 3/4] {fetch,upload}-pack: sideband v2 fetch response

[Jonathan Tan]

> OK, so the "fetch" side enables sideband-all any time it knows that
> the other side supports it.
> 
> It feels a bit strange at the logical level that reader.me is set
> only when we are going to talk over "sideband-all".  I know that at
> the implementation level, reader.me is only looked at when sideband
> is use, but it still feels somewhat funny.  Future developers may
> want to use reader.me to identify the entity that found some error
> during a read, even when the sideband is not yet in use, and at that
> point, uninitialized .me would be a source of an error.  IOW, that
> assignment smells like a timebomb waiting to go off.

I think the best solution here is to initialize .me to "git", like how
we do it for trace. I've done that for now.

> > +		switch (retval) {
> > +			case SIDEBAND_PROTOCOL_ERROR:
> 
> Dedent (see previous step).

Done.

> > +			case SIDEBAND_PRIMARY:
> > +				if (reader->pktlen != 1)
> > +					goto nonprogress;
> > +				/*
> > +				 * Since pktlen is 1, this is a keepalive
> > +				 * packet. Wait for the next one.
> > +				 */
> 
> What guarantees that a normal payload packet is never of length==1?

This is indeed assuming that we currently don't send 0004 (the
equivalent of 0005\1 without the sideband). I chose this because it is
currently what we use in create_pack_file() in upload-pack.c, but I'm OK
with alternatives (e.g. if we use 0005\2 instead).

> > +				break;
> > +			default: /* SIDEBAND_PROGRESS */
> > +				;
> > +		}
> > +	}
> >  
> > +nonprogress:
> 
> It is totally unclear why this label is called nonprogress.  Is it
> that the primary purpose of the while loop above is to spin and eat
> the keep-alive packets from the other side?  If so, then it sort-of
> makes sense (i.e. "we are looking for progress-meter related
> packets, but now we found something else, so let's leave the loop").
> 
> It would have greatly helped reviewers to have a comment in front of
> that infinite loop, perhaps like
> 
> 	/*
> 	 * Spin and consume the keep-alive packets
> 	 */

[snip]

Yes, it's meant to spin and consume the progress and keepalive packets.
I've added a comment at the top of the loop and renamed the jump label
to "nonprogress_received".

> >  	if (reader->status == PACKET_READ_NORMAL)
> > -		reader->line = reader->buffer;
> > +		reader->line = reader->use_sideband ?
> > +			reader->buffer + 1 : reader->buffer;
> 
> Is the one byte the sideband designator?

Yes. Added comment to clarify that.

> >  void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
> > @@ -521,7 +548,8 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
> >  	va_list args;
> >  
> >  	va_start(args, fmt);
> > -	packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args);
> > +	packet_write_fmt_1(writer->dest_fd, 0,
> > +			   writer->use_sideband ? "\1" : "", fmt, args);
> >  	va_end(args);
> >  }
> 
> As I am superstitious, I'd prefer to see octal literals to be fully
> spelled out as 3-digit sequence, i.e. "\001".  Likewise for "\003".

Done.

> > @@ -530,7 +558,8 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
> >  	va_list args;
> >  
> >  	va_start(args, fmt);
> > -	packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args);
> > +	packet_write_fmt_1(writer->dest_fd, 0,
> > +			   writer->use_sideband ? "\3" : "ERR ", fmt, args);
> 
> OK, band#3 is an error message for emergency exit.  It is a bit
> curious that this patch does not show any line from the existing
> code in the context that reacts to the ERR string, but that is
> because the errors are noticed at a lot lower level when the
> sideband is in use than the code that currently checks for errors?

Yes - the branch that this patch set is on (ms/packet-err-check) handles
this. I have taken care in this patch to ensure that the error message
printed matches what Masaya used, so that in a future patch, when we
force sideband-all (using GIT_TEST_SIDEBAND_ALL), the same message is
printed so that the existing tests still pass.

[PATCH 4/4] tests: define GIT_TEST_SIDEBAND_ALL

[Jonathan Tan]

Define a GIT_TEST_SIDEBAND_ALL environment variable meant to be used
from tests. When set to true, this overrides uploadpack.allowsidebandall
to true, allowing the entire test suite to be run as if this
configuration is in place for all repositories.

As of this patch, all tests pass whether GIT_TEST_SIDEBAND_ALL is unset
or set to 1.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 fetch-pack.c             |  3 ++-
 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            | 13 ++++++++-----
 7 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index b89199891d..4618568fee 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1362,7 +1362,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	packet_reader_init(&reader, fd[0], NULL, 0,
 			   PACKET_READ_CHOMP_NEWLINE |
 			   PACKET_READ_DIE_ON_ERR_PACKET);
-	if (server_supports_feature("fetch", "sideband-all", 0)) {
+	if (git_env_bool("GIT_TEST_SIDEBAND_ALL", 1) &&
+	    server_supports_feature("fetch", "sideband-all", 0)) {
 		reader.use_sideband = 1;
 		reader.me = "fetch-pack";
 	}
diff --git a/t/README b/t/README
index 28711cc508..b275c883b8 100644
--- a/t/README
+++ b/t/README
@@ -358,6 +358,11 @@ GIT_TEST_MULTI_PACK_INDEX=<boolean>, when true, forces the multi-pack-
 index to be written after every 'git repack' command, and overrides the
 'core.multiPackIndex' setting to true.
 
+GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the
+'uploadpack.allowSidebandAll' setting to true, and when false, forces
+fetch-pack to not request sideband-all (even if the server advertises
+sideband-all).
+
 Naming Tests
 ------------
 
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8f..9a6d368247 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -78,6 +78,7 @@ PassEnv GNUPGHOME
 PassEnv ASAN_OPTIONS
 PassEnv GIT_TRACE
 PassEnv GIT_CONFIG_NOSYSTEM
+PassEnv GIT_TEST_SIDEBAND_ALL
 
 SetEnvIf Git-Protocol ".*" GIT_PROTOCOL=$0
 
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 6faf17e17a..6caf628efa 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -243,7 +243,8 @@ test_expect_success 'shallow fetches check connectivity before writing shallow f
 	       "$(git -C "$REPO" rev-parse HEAD)" \
 	       "$(git -C "$REPO" rev-parse HEAD^)" \
 	       >"$HTTPD_ROOT_PATH/one-time-sed" &&
-	test_must_fail git -C client fetch --depth=1 "$HTTPD_URL/one_time_sed/repo" \
+	test_must_fail env GIT_TEST_SIDEBAND_ALL=0 git -C client \
+		fetch --depth=1 "$HTTPD_URL/one_time_sed/repo" \
 		master:a_branch &&
 
 	# Ensure that the one-time-sed script was used.
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index ae79c6bbc0..fe45bf828d 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -14,7 +14,7 @@ test_expect_success 'test capability advertisement' '
 	0000
 	EOF
 
-	git serve --advertise-capabilities >out &&
+	GIT_TEST_SIDEBAND_ALL=0 git serve --advertise-capabilities >out &&
 	test-tool pkt-line unpack <out >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 0f2b09ebb8..b491c62e3e 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -583,8 +583,8 @@ test_expect_success 'when server does not send "ready", expect FLUSH' '
 	test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" git -C http_child \
 		-c protocol.version=2 \
 		fetch "$HTTPD_URL/one_time_sed/http_parent" 2> err &&
-	grep "fetch< acknowledgments" log &&
-	! grep "fetch< ready" log &&
+	grep "fetch< .*acknowledgments" log &&
+	! grep "fetch< .*ready" log &&
 	test_i18ngrep "expected no other sections to be sent after no .ready." err
 '
 
diff --git a/upload-pack.c b/upload-pack.c
index 765b7695d2..0c1feccaab 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1288,7 +1288,9 @@ static void process_args(struct packet_reader *request,
 			continue;
 		}
 
-		if (allow_sideband_all && !strcmp(arg, "sideband-all")) {
+		if ((git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) ||
+		     allow_sideband_all) &&
+		    !strcmp(arg, "sideband-all")) {
 			data->writer.use_sideband = 1;
 			continue;
 		}
@@ -1521,10 +1523,11 @@ int upload_pack_advertise(struct repository *r,
 		    allow_ref_in_want)
 			strbuf_addstr(value, " ref-in-want");
 
-		if (!repo_config_get_bool(the_repository,
-					 "uploadpack.allowsidebandall",
-					 &allow_sideband_all_value) &&
-		    allow_sideband_all_value)
+		if (git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) ||
+		    (!repo_config_get_bool(the_repository,
+					   "uploadpack.allowsidebandall",
+					   &allow_sideband_all_value) &&
+		     allow_sideband_all_value))
 			strbuf_addstr(value, " sideband-all");
 	}
 
-- 
2.19.0.271.gfe8321ec05.dirty

[WIP 5/4] Documentation: order protocol v2 sections

[Jonathan Tan]

The current C Git implementation expects Git servers to follow a
specific order of sections when transmitting protocol v2 responses, but
this is not explicit in the documentation. Make the order explicit.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 Documentation/technical/protocol-v2.txt | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 1b0633f59f..c76f2311c2 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -319,11 +319,11 @@ included in the client's request:
 
 The response of `fetch` is broken into a number of sections separated by
 delimiter packets (0001), with each section beginning with its section
-header.
+header. Most sections are sent only when the packfile is sent.
 
-    output = *section
-    section = (acknowledgments | shallow-info | wanted-refs | packfile)
-	      (flush-pkt | delim-pkt)
+    output = acknowledgements flush-pkt |
+	     [acknowledgments delim-pkt] [shallow-info delim-pkt]
+	     [wanted-refs delim-pkt] packfile flush-pkt
 
     acknowledgments = PKT-LINE("acknowledgments" LF)
 		      (nak | *ack)
@@ -345,9 +345,10 @@ header.
 	       *PKT-LINE(%x01-03 *%x00-ff)
 
     acknowledgments section
-	* If the client determines that it is finished with negotiations
-	  by sending a "done" line, the acknowledgments sections MUST be
-	  omitted from the server's response.
+	* If the client determines that it is finished with negotiations by
+	  sending a "done" line (thus requiring the server to send a packfile),
+	  the acknowledgments sections MUST be omitted from the server's
+	  response.
 
 	* Always begins with the section header "acknowledgments"
 
@@ -398,9 +399,6 @@ header.
 	  which the client has not indicated was shallow as a part of
 	  its request.
 
-	* This section is only included if a packfile section is also
-	  included in the response.
-
     wanted-refs section
 	* This section is only included if the client has requested a
 	  ref using a 'want-ref' line and if a packfile section is also
-- 
2.19.0.271.gfe8321ec05.dirty

[WIP 6/4] Documentation: add Packfile URIs design doc

[Jonathan Tan]

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 Documentation/technical/packfile-uri.txt | 83 ++++++++++++++++++++++++
 Documentation/technical/protocol-v2.txt  |  6 +-
 2 files changed, 88 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/technical/packfile-uri.txt

diff --git a/Documentation/technical/packfile-uri.txt b/Documentation/technical/packfile-uri.txt
new file mode 100644
index 0000000000..6535801486
--- /dev/null
+++ b/Documentation/technical/packfile-uri.txt
@@ -0,0 +1,83 @@
+Packfile URIs
+=============
+
+This feature allows servers to serve part of their packfile response as URIs.
+This allows server designs that improve scalability in bandwidth and CPU usage
+(for example, by serving some data through a CDN), and (in the future) provides
+some measure of resumability to clients.
+
+This feature is available only in protocol version 2.
+
+Protocol
+--------
+
+The server advertises `packfile-uris`.
+
+If the client replies with the following arguments:
+
+ * packfile-uris
+ * thin-pack
+ * ofs-delta
+
+when the server sends the packfile, it MAY send a `packfile-uris` section
+directly before the `packfile` section (right after `wanted-refs` if it is
+sent) containing HTTP(S) URIs. See protocol-v2.txt for the documentation of
+this section.
+
+Clients then should understand that the returned packfile could be incomplete,
+and that it needs to download all the given URIs before the fetch or clone is
+complete. Each URI should point to a Git packfile (which may be a thin pack and
+which may contain offset deltas).
+
+Server design
+-------------
+
+The server can be trivially made compatible with the proposed protocol by
+having it advertise `packfile-uris`, tolerating the client sending
+`packfile-uris`, and never sending any `packfile-uris` section. But we should
+include some sort of non-trivial implementation in the Minimum Viable Product,
+at least so that we can test the client.
+
+This is the implementation: a feature, marked experimental, that allows the
+server to be configured by one or more `uploadpack.blobPackfileUri=<sha1>
+<uri>` entries. Whenever the list of objects to be sent is assembled, a blob
+with the given sha1 can be replaced by the given URI. This allows, for example,
+servers to delegate serving of large blobs to CDNs.
+
+Client design
+-------------
+
+While fetching, the client needs to remember the list of URIs and cannot
+declare that the fetch is complete until all URIs have been downloaded as
+packfiles.
+
+The division of work (initial fetch + additional URIs) introduces convenient
+points for resumption of an interrupted clone - such resumption can be done
+after the Minimum Viable Product (see "Future work").
+
+The client can inhibit this feature (i.e. refrain from sending the
+`packfile-urls` parameter) by passing --no-packfile-urls to `git fetch`.
+
+Future work
+-----------
+
+The protocol design allows some evolution of the server and client without any
+need for protocol changes, so only a small-scoped design is included here to
+form the MVP. For example, the following can be done:
+
+ * On the server, a long-running process that takes in entire requests and
+   outputs a list of URIs and the corresponding inclusion and exclusion sets of
+   objects. This allows, e.g., signed URIs to be used and packfiles for common
+   requests to be cached.
+ * On the client, resumption of clone. If a clone is interrupted, information
+   could be recorded in the repository's config and a "clone-resume" command
+   can resume the clone in progress. (Resumption of subsequent fetches is more
+   difficult because that must deal with the user wanting to use the repository
+   even after the fetch was interrupted.)
+
+There are some possible features that will require a change in protocol:
+
+ * Additional HTTP headers (e.g. authentication)
+ * Byte range support
+ * Different file formats referenced by URIs (e.g. raw object)
+
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index c76f2311c2..de5c63bc9b 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -323,7 +323,8 @@ header. Most sections are sent only when the packfile is sent.
 
     output = acknowledgements flush-pkt |
 	     [acknowledgments delim-pkt] [shallow-info delim-pkt]
-	     [wanted-refs delim-pkt] packfile flush-pkt
+	     [wanted-refs delim-pkt] [packfile-uris delim-pkt]
+	     packfile flush-pkt
 
     acknowledgments = PKT-LINE("acknowledgments" LF)
 		      (nak | *ack)
@@ -341,6 +342,9 @@ header. Most sections are sent only when the packfile is sent.
 		  *PKT-LINE(wanted-ref LF)
     wanted-ref = obj-id SP refname
 
+    packfile-uris = PKT-LINE("packfile-uris" LF) *packfile-uri
+    packfile-uri = PKT-LINE("uri" SP *%x20-ff LF)
+
     packfile = PKT-LINE("packfile" LF)
 	       *PKT-LINE(%x01-03 *%x00-ff)
 
-- 
2.19.0.271.gfe8321ec05.dirty

[WIP 7/4] upload-pack: refactor reading of pack-objects out

[Jonathan Tan]

Subsequent patches will change how the output of pack-objects is
processed, so extract that processing into its own function.

Currently, at most 1 character can be buffered (in the "buffered" local
variable). One of those patches will require a larger buffer, so replace
that "buffered" local variable with a buffer array.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 upload-pack.c | 80 +++++++++++++++++++++++++++++----------------------
 1 file changed, 46 insertions(+), 34 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index 0c1feccaab..c87b752550 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -103,14 +103,51 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
 	return 0;
 }
 
+struct output_state {
+	char buffer[8193];
+	int used;
+};
+
+static int read_pack_objects_stdout(int outfd, struct output_state *os)
+{
+	/* Data ready; we keep the last byte to ourselves
+	 * in case we detect broken rev-list, so that we
+	 * can leave the stream corrupted.  This is
+	 * unfortunate -- unpack-objects would happily
+	 * accept a valid packdata with trailing garbage,
+	 * so appending garbage after we pass all the
+	 * pack data is not good enough to signal
+	 * breakage to downstream.
+	 */
+	ssize_t readsz;
+
+	readsz = xread(outfd, os->buffer + os->used,
+		       sizeof(os->buffer) - os->used);
+	if (readsz < 0) {
+		return readsz;
+	}
+	os->used += readsz;
+
+	if (os->used > 1) {
+		send_client_data(1, os->buffer, os->used - 1);
+		os->buffer[0] = os->buffer[os->used - 1];
+		os->used = 1;
+	} else {
+		send_client_data(1, os->buffer, os->used);
+		os->used = 0;
+	}
+
+	return readsz;
+}
+
 static void create_pack_file(const struct object_array *have_obj,
 			     const struct object_array *want_obj)
 {
 	struct child_process pack_objects = CHILD_PROCESS_INIT;
-	char data[8193], progress[128];
+	struct output_state output_state = {0};
+	char progress[128];
 	char abort_msg[] = "aborting due to possible repository "
 		"corruption on the remote side.";
-	int buffered = -1;
 	ssize_t sz;
 	int i;
 	FILE *pipe_fd;
@@ -237,39 +274,15 @@ static void create_pack_file(const struct object_array *have_obj,
 			continue;
 		}
 		if (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {
-			/* Data ready; we keep the last byte to ourselves
-			 * in case we detect broken rev-list, so that we
-			 * can leave the stream corrupted.  This is
-			 * unfortunate -- unpack-objects would happily
-			 * accept a valid packdata with trailing garbage,
-			 * so appending garbage after we pass all the
-			 * pack data is not good enough to signal
-			 * breakage to downstream.
-			 */
-			char *cp = data;
-			ssize_t outsz = 0;
-			if (0 <= buffered) {
-				*cp++ = buffered;
-				outsz++;
-			}
-			sz = xread(pack_objects.out, cp,
-				  sizeof(data) - outsz);
-			if (0 < sz)
-				;
-			else if (sz == 0) {
+			int result = read_pack_objects_stdout(pack_objects.out,
+							      &output_state);
+
+			if (result == 0) {
 				close(pack_objects.out);
 				pack_objects.out = -1;
-			}
-			else
+			} else if (result < 0) {
 				goto fail;
-			sz += outsz;
-			if (1 < sz) {
-				buffered = data[sz-1] & 0xFF;
-				sz--;
 			}
-			else
-				buffered = -1;
-			send_client_data(1, data, sz);
 		}
 
 		/*
@@ -294,9 +307,8 @@ static void create_pack_file(const struct object_array *have_obj,
 	}
 
 	/* flush the data */
-	if (0 <= buffered) {
-		data[0] = buffered;
-		send_client_data(1, data, 1);
+	if (output_state.used > 0) {
+		send_client_data(1, output_state.buffer, output_state.used);
 		fprintf(stderr, "flushed.\n");
 	}
 	if (use_sideband)
-- 
2.19.0.271.gfe8321ec05.dirty

[WIP 8/4] upload-pack: send part of packfile response as uri

[Jonathan Tan]

This is a partial implementation of upload-pack sending part of its
packfile response as URIs.

The client is not fully implemented - it knows to ignore the
"packfile-uris" section, but because it does not actually fetch those
URIs, the returned packfile is incomplete. A test is included to show
that the appropriate URI is indeed transmitted, and that the returned
packfile is lacking exactly the expected object.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/pack-objects.c | 48 ++++++++++++++++++++++++++++++++++++
 fetch-pack.c           |  9 +++++++
 t/t5702-protocol-v2.sh | 27 ++++++++++++++++++++
 upload-pack.c          | 56 ++++++++++++++++++++++++++++++++++++------
 4 files changed, 133 insertions(+), 7 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 411aefd687..448c42a666 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -117,6 +117,15 @@ enum missing_action {
 static enum missing_action arg_missing_action;
 static show_object_fn fn_show_object;
 
+struct configured_exclusion {
+	struct oidmap_entry e;
+	char *uri;
+};
+static struct oidmap configured_exclusions;
+
+static int exclude_configured_blobs;
+static struct oidset excluded_by_config;
+
 /*
  * stats
  */
@@ -831,6 +840,23 @@ static off_t write_reused_pack(struct hashfile *f)
 	return reuse_packfile_offset - sizeof(struct pack_header);
 }
 
+static void write_excluded_by_configs(void)
+{
+	struct oidset_iter iter;
+	const struct object_id *oid;
+
+	oidset_iter_init(&excluded_by_config, &iter);
+	while ((oid = oidset_iter_next(&iter))) {
+		struct configured_exclusion *ex =
+			oidmap_get(&configured_exclusions, oid);
+
+		if (!ex)
+			BUG("configured exclusion wasn't configured");
+		write_in_full(1, ex->uri, strlen(ex->uri));
+		write_in_full(1, "\n", 1);
+	}
+}
+
 static const char no_split_warning[] = N_(
 "disabling bitmap writing, packs are split due to pack.packSizeLimit"
 );
@@ -1124,6 +1150,12 @@ static int want_object_in_pack(const struct object_id *oid,
 		}
 	}
 
+	if (exclude_configured_blobs &&
+	    oidmap_get(&configured_exclusions, oid)) {
+		oidset_insert(&excluded_by_config, oid);
+		return 0;
+	}
+
 	return 1;
 }
 
@@ -2728,6 +2760,19 @@ static int git_pack_config(const char *k, const char *v, void *cb)
 			    pack_idx_opts.version);
 		return 0;
 	}
+	if (!strcmp(k, "uploadpack.blobpackfileuri")) {
+		struct configured_exclusion *ex = xmalloc(sizeof(*ex));
+		const char *end;
+
+		if (parse_oid_hex(v, &ex->e.oid, &end) || *end != ' ')
+			die(_("value of uploadpack.blobpackfileuri must be "
+			      "of the form '<sha-1> <uri>' (got '%s')"), v);
+		if (oidmap_get(&configured_exclusions, &ex->e.oid))
+			die(_("object already configured in another "
+			      "uploadpack.blobpackfileuri (got '%s')"), v);
+		ex->uri = xstrdup(end + 1);
+		oidmap_put(&configured_exclusions, ex);
+	}
 	return git_default_config(k, v, cb);
 }
 
@@ -3316,6 +3361,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			 N_("do not pack objects in promisor packfiles")),
 		OPT_BOOL(0, "delta-islands", &use_delta_islands,
 			 N_("respect islands during delta compression")),
+		OPT_BOOL(0, "exclude-configured-blobs", &exclude_configured_blobs,
+			 N_("respect uploadpack.blobpackfileuri")),
 		OPT_END(),
 	};
 
@@ -3489,6 +3536,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		return 0;
 	if (nr_result)
 		prepare_pack(window, depth);
+	write_excluded_by_configs();
 	write_pack_file();
 	if (progress)
 		fprintf_ln(stderr,
diff --git a/fetch-pack.c b/fetch-pack.c
index 4618568fee..79af87b2cf 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1429,6 +1429,15 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 				receive_wanted_refs(&reader, sought, nr_sought);
 
 			/* get the pack */
+			if (process_section_header(&reader, "packfile-uris", 1)) {
+				/* skip the whole section */
+				process_section_header(&reader, "packfile-uris", 0);
+				while (packet_reader_read(&reader) == PACKET_READ_NORMAL) {
+					/* do nothing */
+				}
+				if (reader.status != PACKET_READ_DELIM)
+					die("expected DELIM");
+			}
 			process_section_header(&reader, "packfile", 0);
 			if (get_pack(args, fd, pack_lockfile))
 				die(_("git fetch-pack: fetch failed."));
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index b491c62e3e..ba85ee4dd9 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -588,6 +588,33 @@ test_expect_success 'when server does not send "ready", expect FLUSH' '
 	test_i18ngrep "expected no other sections to be sent after no .ready." err
 '
 
+test_expect_success 'part of packfile response provided as URI' '
+	rm -rf "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" http_child log &&
+
+	git init "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
+	echo my-blob >"$HTTPD_DOCUMENT_ROOT_PATH/http_parent/my-blob" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent/" add my-blob &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent/" commit -m x &&
+
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent/" hash-object my-blob >h &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent/" config \
+		"uploadpack.blobpackfileuri" \
+		"$(cat h) https://example.com/a-uri" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent/" config \
+		"uploadpack.allowsidebandall" "true" &&
+
+	# NEEDSWORK: "git clone" fails here because it ignores the URI provided
+	# instead of fetching it.
+	test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" \
+		git -c protocol.version=2 clone \
+		"$HTTPD_URL/smart/http_parent" http_child 2>err &&
+	# Although "git clone" fails, we can still check that the server
+	# provided the URI we requested and that the error message pinpoints
+	# the object that is missing.
+	grep "clone< .*uri https://example.com/a-uri" log &&
+	test_i18ngrep "did not receive expected object $(cat h)" err
+'
+
 stop_httpd
 
 test_done
diff --git a/upload-pack.c b/upload-pack.c
index c87b752550..fe6e2300e3 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -106,9 +106,12 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
 struct output_state {
 	char buffer[8193];
 	int used;
+	unsigned packfile_uris_started : 1;
+	unsigned packfile_started : 1;
 };
 
-static int read_pack_objects_stdout(int outfd, struct output_state *os)
+static int read_pack_objects_stdout(int outfd, struct output_state *os,
+				    int write_packfile_line)
 {
 	/* Data ready; we keep the last byte to ourselves
 	 * in case we detect broken rev-list, so that we
@@ -128,6 +131,37 @@ static int read_pack_objects_stdout(int outfd, struct output_state *os)
 	}
 	os->used += readsz;
 
+	while (!os->packfile_started) {
+		char *p;
+		if (os->used >= 4 && !memcmp(os->buffer, "PACK", 4)) {
+			os->packfile_started = 1;
+			if (write_packfile_line) {
+				if (os->packfile_uris_started)
+					packet_delim(1);
+				packet_write_fmt(1, "\1packfile\n");
+			}
+			break;
+		}
+		if ((p = memchr(os->buffer, '\n', os->used))) {
+			if (!os->packfile_uris_started) {
+				os->packfile_uris_started = 1;
+				if (!write_packfile_line)
+					BUG("packfile_uris requires sideband-all");
+				packet_write_fmt(1, "\1packfile-uris\n");
+			}
+			*p = '\0';
+			packet_write_fmt(1, "\1uri %s\n", os->buffer);
+
+			os->used -= p - os->buffer + 1;
+			memmove(os->buffer, p, os->used);
+		} else {
+			/*
+			 * Incomplete line.
+			 */
+			return readsz;
+		}
+	}
+
 	if (os->used > 1) {
 		send_client_data(1, os->buffer, os->used - 1);
 		os->buffer[0] = os->buffer[os->used - 1];
@@ -141,7 +175,8 @@ static int read_pack_objects_stdout(int outfd, struct output_state *os)
 }
 
 static void create_pack_file(const struct object_array *have_obj,
-			     const struct object_array *want_obj)
+			     const struct object_array *want_obj,
+			     int write_packfile_line)
 {
 	struct child_process pack_objects = CHILD_PROCESS_INIT;
 	struct output_state output_state = {0};
@@ -189,6 +224,9 @@ static void create_pack_file(const struct object_array *have_obj,
 					 filter_options.filter_spec);
 		}
 	}
+	if (write_packfile_line)
+		argv_array_push(&pack_objects.args,
+				"--exclude-configured-blobs");
 
 	pack_objects.in = -1;
 	pack_objects.out = -1;
@@ -275,8 +313,8 @@ static void create_pack_file(const struct object_array *have_obj,
 		}
 		if (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {
 			int result = read_pack_objects_stdout(pack_objects.out,
-							      &output_state);
-
+							      &output_state,
+							      write_packfile_line);
 			if (result == 0) {
 				close(pack_objects.out);
 				pack_objects.out = -1;
@@ -1108,7 +1146,7 @@ void upload_pack(struct upload_pack_options *options)
 	if (want_obj.nr) {
 		struct object_array have_obj = OBJECT_ARRAY_INIT;
 		get_common_commits(&reader, &have_obj, &want_obj);
-		create_pack_file(&have_obj, &want_obj);
+		create_pack_file(&have_obj, &want_obj, 0);
 	}
 }
 
@@ -1498,8 +1536,12 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
 			send_wanted_ref_info(&data);
 			send_shallow_info(&data, &want_obj);
 
-			packet_writer_write(&data.writer, "packfile\n");
-			create_pack_file(&have_obj, &want_obj);
+			if (data.writer.use_sideband) {
+				create_pack_file(&have_obj, &want_obj, 1);
+			} else {
+				packet_write_fmt(1, "packfile\n");
+				create_pack_file(&have_obj, &want_obj, 0);
+			}
 			state = FETCH_DONE;
 			break;
 		case FETCH_DONE:
-- 
2.19.0.271.gfe8321ec05.dirty

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

[Jonathan Tan]

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

[PATCH v2 1/4] pkt-line: introduce struct packet_writer

[Jonathan Tan]

A future patch will allow the client to request multiplexing of the
entire fetch response (and not only during packfile transmission), which
in turn allows the server to send progress and keepalive messages at any
time during the response.

It will be convenient for a future patch if writing options
(specifically, whether the written data is to be multiplexed) could be
controlled from a single place, so create struct packet_writer to serve
as that place, and modify upload-pack to use it.

Currently, it only stores the output fd, but a subsequent patch will (as
described above) introduce an option to determine if the written data is
to be multiplexed.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 pkt-line.c    |  47 ++++++++++++++++++---
 pkt-line.h    |  14 +++++++
 upload-pack.c | 112 +++++++++++++++++++++++++++-----------------------
 3 files changed, 115 insertions(+), 58 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index e70ea6d88f..9d3e402d41 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -129,12 +129,14 @@ static void set_packet_header(char *buf, const int size)
 	#undef hex
 }
 
-static void format_packet(struct strbuf *out, const char *fmt, va_list args)
+static void format_packet(struct strbuf *out, const char *prefix,
+			  const char *fmt, va_list args)
 {
 	size_t orig_len, n;
 
 	orig_len = out->len;
 	strbuf_addstr(out, "0000");
+	strbuf_addstr(out, prefix);
 	strbuf_vaddf(out, fmt, args);
 	n = out->len - orig_len;
 
@@ -145,13 +147,13 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)
 	packet_trace(out->buf + orig_len + 4, n - 4, 1);
 }
 
-static int packet_write_fmt_1(int fd, int gently,
+static int packet_write_fmt_1(int fd, int gently, const char *prefix,
 			      const char *fmt, va_list args)
 {
 	static struct strbuf buf = STRBUF_INIT;
 
 	strbuf_reset(&buf);
-	format_packet(&buf, fmt, args);
+	format_packet(&buf, prefix, fmt, args);
 	if (write_in_full(fd, buf.buf, buf.len) < 0) {
 		if (!gently) {
 			check_pipe(errno);
@@ -168,7 +170,7 @@ void packet_write_fmt(int fd, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	packet_write_fmt_1(fd, 0, fmt, args);
+	packet_write_fmt_1(fd, 0, "", fmt, args);
 	va_end(args);
 }
 
@@ -178,7 +180,7 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	status = packet_write_fmt_1(fd, 1, fmt, args);
+	status = packet_write_fmt_1(fd, 1, "", fmt, args);
 	va_end(args);
 	return status;
 }
@@ -211,7 +213,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	format_packet(buf, fmt, args);
+	format_packet(buf, "", fmt, args);
 	va_end(args);
 }
 
@@ -486,3 +488,36 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)
 	reader->line_peeked = 1;
 	return reader->status;
 }
+
+void packet_writer_init(struct packet_writer *writer, int dest_fd)
+{
+	writer->dest_fd = dest_fd;
+}
+
+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
+{
+	va_list args;
+
+	va_start(args, fmt);
+	packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args);
+	va_end(args);
+}
+
+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
+{
+	va_list args;
+
+	va_start(args, fmt);
+	packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args);
+	va_end(args);
+}
+
+void packet_writer_delim(struct packet_writer *writer)
+{
+	packet_delim(writer->dest_fd);
+}
+
+void packet_writer_flush(struct packet_writer *writer)
+{
+	packet_flush(writer->dest_fd);
+}
diff --git a/pkt-line.h b/pkt-line.h
index d7e1dbc047..023ad2951d 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -183,4 +183,18 @@ extern enum packet_read_status packet_reader_peek(struct packet_reader *reader);
 #define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)
 extern char packet_buffer[LARGE_PACKET_MAX];
 
+struct packet_writer {
+	int dest_fd;
+};
+
+void packet_writer_init(struct packet_writer *writer, int dest_fd);
+
+/* These functions die upon failure. */
+__attribute__((format (printf, 2, 3)))
+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...);
+__attribute__((format (printf, 2, 3)))
+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...);
+void packet_writer_delim(struct packet_writer *writer);
+void packet_writer_flush(struct packet_writer *writer);
+
 #endif
diff --git a/upload-pack.c b/upload-pack.c
index 08b547cf01..60a26bbbfd 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -613,13 +613,14 @@ static void check_non_tip(struct object_array *want_obj)
 	}
 }
 
-static void send_shallow(struct commit_list *result)
+static void send_shallow(struct packet_writer *writer,
+			 struct commit_list *result)
 {
 	while (result) {
 		struct object *object = &result->item->object;
 		if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
-			packet_write_fmt(1, "shallow %s",
-					 oid_to_hex(&object->oid));
+			packet_writer_write(writer, "shallow %s",
+					    oid_to_hex(&object->oid));
 			register_shallow(the_repository, &object->oid);
 			shallow_nr++;
 		}
@@ -627,7 +628,8 @@ static void send_shallow(struct commit_list *result)
 	}
 }
 
-static void send_unshallow(const struct object_array *shallows,
+static void send_unshallow(struct packet_writer *writer,
+			   const struct object_array *shallows,
 			   struct object_array *want_obj)
 {
 	int i;
@@ -636,8 +638,8 @@ static void send_unshallow(const struct object_array *shallows,
 		struct object *object = shallows->objects[i].item;
 		if (object->flags & NOT_SHALLOW) {
 			struct commit_list *parents;
-			packet_write_fmt(1, "unshallow %s",
-					 oid_to_hex(&object->oid));
+			packet_writer_write(writer, "unshallow %s",
+					    oid_to_hex(&object->oid));
 			object->flags &= ~CLIENT_SHALLOW;
 			/*
 			 * We want to _register_ "object" as shallow, but we
@@ -662,7 +664,7 @@ static void send_unshallow(const struct object_array *shallows,
 	}
 }
 
-static void deepen(int depth, int deepen_relative,
+static void deepen(struct packet_writer *writer, int depth, int deepen_relative,
 		   struct object_array *shallows, struct object_array *want_obj)
 {
 	if (depth == INFINITE_DEPTH && !is_repository_shallow(the_repository)) {
@@ -680,7 +682,7 @@ static void deepen(int depth, int deepen_relative,
 		result = get_shallow_commits(&reachable_shallows,
 					     depth + 1,
 					     SHALLOW, NOT_SHALLOW);
-		send_shallow(result);
+		send_shallow(writer, result);
 		free_commit_list(result);
 		object_array_clear(&reachable_shallows);
 	} else {
@@ -688,14 +690,15 @@ static void deepen(int depth, int deepen_relative,
 
 		result = get_shallow_commits(want_obj, depth,
 					     SHALLOW, NOT_SHALLOW);
-		send_shallow(result);
+		send_shallow(writer, result);
 		free_commit_list(result);
 	}
 
-	send_unshallow(shallows, want_obj);
+	send_unshallow(writer, shallows, want_obj);
 }
 
-static void deepen_by_rev_list(int ac, const char **av,
+static void deepen_by_rev_list(struct packet_writer *writer, int ac,
+			       const char **av,
 			       struct object_array *shallows,
 			       struct object_array *want_obj)
 {
@@ -703,13 +706,14 @@ static void deepen_by_rev_list(int ac, const char **av,
 
 	close_commit_graph(the_repository);
 	result = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);
-	send_shallow(result);
+	send_shallow(writer, result);
 	free_commit_list(result);
-	send_unshallow(shallows, want_obj);
+	send_unshallow(writer, shallows, want_obj);
 }
 
 /* Returns 1 if a shallow list is sent or 0 otherwise */
-static int send_shallow_list(int depth, int deepen_rev_list,
+static int send_shallow_list(struct packet_writer *writer,
+			     int depth, int deepen_rev_list,
 			     timestamp_t deepen_since,
 			     struct string_list *deepen_not,
 			     struct object_array *shallows,
@@ -720,7 +724,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,
 	if (depth > 0 && deepen_rev_list)
 		die("git upload-pack: deepen and deepen-since (or deepen-not) cannot be used together");
 	if (depth > 0) {
-		deepen(depth, deepen_relative, shallows, want_obj);
+		deepen(writer, depth, deepen_relative, shallows, want_obj);
 		ret = 1;
 	} else if (deepen_rev_list) {
 		struct argv_array av = ARGV_ARRAY_INIT;
@@ -741,7 +745,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,
 			struct object *o = want_obj->objects[i].item;
 			argv_array_push(&av, oid_to_hex(&o->oid));
 		}
-		deepen_by_rev_list(av.argc, av.argv, shallows, want_obj);
+		deepen_by_rev_list(writer, av.argc, av.argv, shallows, want_obj);
 		argv_array_clear(&av);
 		ret = 1;
 	} else {
@@ -834,8 +838,10 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
 	int has_non_tip = 0;
 	timestamp_t deepen_since = 0;
 	int deepen_rev_list = 0;
+	struct packet_writer writer;
 
 	shallow_nr = 0;
+	packet_writer_init(&writer, 1);
 	for (;;) {
 		struct object *o;
 		const char *features;
@@ -892,9 +898,9 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
 
 		o = parse_object(the_repository, &oid_buf);
 		if (!o) {
-			packet_write_fmt(1,
-					 "ERR upload-pack: not our ref %s",
-					 oid_to_hex(&oid_buf));
+			packet_writer_error(&writer,
+					    "upload-pack: not our ref %s",
+					    oid_to_hex(&oid_buf));
 			die("git upload-pack: not our ref %s",
 			    oid_to_hex(&oid_buf));
 		}
@@ -923,7 +929,7 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
 	if (depth == 0 && !deepen_rev_list && shallows.nr == 0)
 		return;
 
-	if (send_shallow_list(depth, deepen_rev_list, deepen_since,
+	if (send_shallow_list(&writer, depth, deepen_rev_list, deepen_since,
 			      &deepen_not, &shallows, want_obj))
 		packet_flush(1);
 	object_array_clear(&shallows);
@@ -1102,6 +1108,8 @@ struct upload_pack_data {
 	int deepen_rev_list;
 	int deepen_relative;
 
+	struct packet_writer writer;
+
 	unsigned stateless_rpc : 1;
 
 	unsigned use_thin_pack : 1;
@@ -1125,6 +1133,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
 	data->haves = haves;
 	data->shallows = shallows;
 	data->deepen_not = deepen_not;
+	packet_writer_init(&data->writer, 1);
 }
 
 static void upload_pack_data_clear(struct upload_pack_data *data)
@@ -1136,7 +1145,8 @@ static void upload_pack_data_clear(struct upload_pack_data *data)
 	string_list_clear(&data->deepen_not, 0);
 }
 
-static int parse_want(const char *line, struct object_array *want_obj)
+static int parse_want(struct packet_writer *writer, const char *line,
+		      struct object_array *want_obj)
 {
 	const char *arg;
 	if (skip_prefix(line, "want ", &arg)) {
@@ -1149,9 +1159,9 @@ static int parse_want(const char *line, struct object_array *want_obj)
 
 		o = parse_object(the_repository, &oid);
 		if (!o) {
-			packet_write_fmt(1,
-					 "ERR upload-pack: not our ref %s",
-					 oid_to_hex(&oid));
+			packet_writer_error(writer,
+					    "upload-pack: not our ref %s",
+					    oid_to_hex(&oid));
 			die("git upload-pack: not our ref %s",
 			    oid_to_hex(&oid));
 		}
@@ -1167,7 +1177,8 @@ static int parse_want(const char *line, struct object_array *want_obj)
 	return 0;
 }
 
-static int parse_want_ref(const char *line, struct string_list *wanted_refs,
+static int parse_want_ref(struct packet_writer *writer, const char *line,
+			  struct string_list *wanted_refs,
 			  struct object_array *want_obj)
 {
 	const char *arg;
@@ -1177,7 +1188,7 @@ static int parse_want_ref(const char *line, struct string_list *wanted_refs,
 		struct object *o;
 
 		if (read_ref(arg, &oid)) {
-			packet_write_fmt(1, "ERR unknown ref %s", arg);
+			packet_writer_error(writer, "unknown ref %s", arg);
 			die("unknown ref %s", arg);
 		}
 
@@ -1220,10 +1231,11 @@ static void process_args(struct packet_reader *request,
 		const char *p;
 
 		/* process want */
-		if (parse_want(arg, want_obj))
+		if (parse_want(&data->writer, arg, want_obj))
 			continue;
 		if (allow_ref_in_want &&
-		    parse_want_ref(arg, &data->wanted_refs, want_obj))
+		    parse_want_ref(&data->writer, arg, &data->wanted_refs,
+				   want_obj))
 			continue;
 		/* process have line */
 		if (parse_have(arg, &data->haves))
@@ -1317,26 +1329,26 @@ static int process_haves(struct oid_array *haves, struct oid_array *common,
 	return 0;
 }
 
-static int send_acks(struct oid_array *acks, struct strbuf *response,
+static int send_acks(struct packet_writer *writer, struct oid_array *acks,
 		     const struct object_array *have_obj,
 		     struct object_array *want_obj)
 {
 	int i;
 
-	packet_buf_write(response, "acknowledgments\n");
+	packet_writer_write(writer, "acknowledgments\n");
 
 	/* Send Acks */
 	if (!acks->nr)
-		packet_buf_write(response, "NAK\n");
+		packet_writer_write(writer, "NAK\n");
 
 	for (i = 0; i < acks->nr; i++) {
-		packet_buf_write(response, "ACK %s\n",
-				 oid_to_hex(&acks->oid[i]));
+		packet_writer_write(writer, "ACK %s\n",
+				    oid_to_hex(&acks->oid[i]));
 	}
 
 	if (ok_to_give_up(have_obj, want_obj)) {
 		/* Send Ready */
-		packet_buf_write(response, "ready\n");
+		packet_writer_write(writer, "ready\n");
 		return 1;
 	}
 
@@ -1348,25 +1360,20 @@ static int process_haves_and_send_acks(struct upload_pack_data *data,
 				       struct object_array *want_obj)
 {
 	struct oid_array common = OID_ARRAY_INIT;
-	struct strbuf response = STRBUF_INIT;
 	int ret = 0;
 
 	process_haves(&data->haves, &common, have_obj);
 	if (data->done) {
 		ret = 1;
-	} else if (send_acks(&common, &response, have_obj, want_obj)) {
-		packet_buf_delim(&response);
+	} else if (send_acks(&data->writer, &common, have_obj, want_obj)) {
+		packet_writer_delim(&data->writer);
 		ret = 1;
 	} else {
 		/* Add Flush */
-		packet_buf_flush(&response);
+		packet_writer_flush(&data->writer);
 		ret = 0;
 	}
 
-	/* Send response */
-	write_or_die(1, response.buf, response.len);
-	strbuf_release(&response);
-
 	oid_array_clear(&data->haves);
 	oid_array_clear(&common);
 	return ret;
@@ -1379,15 +1386,15 @@ static void send_wanted_ref_info(struct upload_pack_data *data)
 	if (!data->wanted_refs.nr)
 		return;
 
-	packet_write_fmt(1, "wanted-refs\n");
+	packet_writer_write(&data->writer, "wanted-refs\n");
 
 	for_each_string_list_item(item, &data->wanted_refs) {
-		packet_write_fmt(1, "%s %s\n",
-				 oid_to_hex(item->util),
-				 item->string);
+		packet_writer_write(&data->writer, "%s %s\n",
+				    oid_to_hex(item->util),
+				    item->string);
 	}
 
-	packet_delim(1);
+	packet_writer_delim(&data->writer);
 }
 
 static void send_shallow_info(struct upload_pack_data *data,
@@ -1398,14 +1405,15 @@ static void send_shallow_info(struct upload_pack_data *data,
 	    !is_repository_shallow(the_repository))
 		return;
 
-	packet_write_fmt(1, "shallow-info\n");
+	packet_writer_write(&data->writer, "shallow-info\n");
 
-	if (!send_shallow_list(data->depth, data->deepen_rev_list,
+	if (!send_shallow_list(&data->writer, data->depth,
+			       data->deepen_rev_list,
 			       data->deepen_since, &data->deepen_not,
 			       &data->shallows, want_obj) &&
 	    is_repository_shallow(the_repository))
-		deepen(INFINITE_DEPTH, data->deepen_relative, &data->shallows,
-		       want_obj);
+		deepen(&data->writer, INFINITE_DEPTH, data->deepen_relative,
+		       &data->shallows, want_obj);
 
 	packet_delim(1);
 }
@@ -1467,7 +1475,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
 			send_wanted_ref_info(&data);
 			send_shallow_info(&data, &want_obj);
 
-			packet_write_fmt(1, "packfile\n");
+			packet_writer_write(&data.writer, "packfile\n");
 			create_pack_file(&have_obj, &want_obj);
 			state = FETCH_DONE;
 			break;
-- 
2.19.0.271.gfe8321ec05.dirty

[PATCH v2 2/4] sideband: reverse its dependency on pkt-line

[Jonathan Tan]

A subsequent patch will teach struct packet_reader a new field that, if
set, instructs it to interpret read data as multiplexed. This will
create a dependency from pkt-line to sideband.

To avoid a circular dependency, split recv_sideband() into 2 parts: the
reading loop (left in recv_sideband()) and the processing of the
contents (in demultiplex_sideband()), and move the former into pkt-line.
This reverses the direction of dependency: sideband no longer depends on
pkt-line, and pkt-line now depends on sideband.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 pkt-line.c |  22 ++++++++
 pkt-line.h |  16 ++++++
 sideband.c | 156 +++++++++++++++++++++++++----------------------------
 sideband.h |  14 ++++-
 4 files changed, 124 insertions(+), 84 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index 9d3e402d41..5feb73ebec 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -439,6 +439,28 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)
 	return sb_out->len - orig_len;
 }
 
+int recv_sideband(const char *me, int in_stream, int out)
+{
+	char buf[LARGE_PACKET_MAX + 1];
+	int retval = 0;
+	int len;
+
+	while (1) {
+		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
+		retval = demultiplex_sideband(me, buf, len);
+		switch (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;
+		}
+	}
+}
+
 /* Packet Reader Functions */
 void packet_reader_init(struct packet_reader *reader, int fd,
 			char *src_buffer, size_t src_len,
diff --git a/pkt-line.h b/pkt-line.h
index 023ad2951d..a8e92a4b63 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -3,6 +3,7 @@
 
 #include "git-compat-util.h"
 #include "strbuf.h"
+#include "sideband.h"
 
 /*
  * Write a packetized stream, where each line is preceded by
@@ -120,6 +121,21 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
  */
 ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
 
+/*
+ * Receive multiplexed output stream over git native protocol.
+ * in_stream is the input stream from the remote, which carries data
+ * in pkt_line format with band designator.  Demultiplex it into out
+ * and err and return error appropriately.  Band #1 carries the
+ * primary payload.  Things coming over band #2 is not necessarily
+ * error; they are usually informative message on the standard error
+ * stream, aka "verbose").  A message over band #3 is a signal that
+ * the remote died unexpectedly.  A flush() concludes the stream.
+ *
+ * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR
+ * or SIDEBAND_REMOTE_ERROR if an error occurred.
+ */
+int recv_sideband(const char *me, int in_stream, int out);
+
 struct packet_reader {
 	/* source file descriptor */
 	int fd;
diff --git a/sideband.c b/sideband.c
index 368647acf8..dce22d20b1 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,7 +1,6 @@
 #include "cache.h"
 #include "color.h"
 #include "config.h"
-#include "pkt-line.h"
 #include "sideband.h"
 #include "help.h"
 
@@ -109,103 +108,94 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 }
 
 
-/*
- * Receive multiplexed output stream over git native protocol.
- * in_stream is the input stream from the remote, which carries data
- * in pkt_line format with band designator.  Demultiplex it into out
- * and err and return error appropriately.  Band #1 carries the
- * primary payload.  Things coming over band #2 is not necessarily
- * error; they are usually informative message on the standard error
- * stream, aka "verbose").  A message over band #3 is a signal that
- * the remote died unexpectedly.  A flush() concludes the stream.
- */
-
 #define DISPLAY_PREFIX "remote: "
 
 #define ANSI_SUFFIX "\033[K"
 #define DUMB_SUFFIX "        "
 
-int recv_sideband(const char *me, int in_stream, int out)
+int demultiplex_sideband(const char *me, char *buf, int len)
 {
-	const char *suffix;
-	char buf[LARGE_PACKET_MAX + 1];
+	static const char *suffix;
 	struct strbuf outbuf = STRBUF_INIT;
 	int retval = 0;
+	const char *b, *brk;
+	int band;
+
+	if (!suffix) {
+		if (isatty(2) && !is_terminal_dumb())
+			suffix = ANSI_SUFFIX;
+		else
+			suffix = DUMB_SUFFIX;
+	}
 
-	if (isatty(2) && !is_terminal_dumb())
-		suffix = ANSI_SUFFIX;
-	else
-		suffix = DUMB_SUFFIX;
+	if (len == 0) {
+		retval = SIDEBAND_FLUSH;
+		goto cleanup;
+	}
+	if (len < 1) {
+		strbuf_addf(&outbuf,
+			    "%s%s: protocol error: no band designator",
+			    outbuf.len ? "\n" : "", me);
+		retval = SIDEBAND_PROTOCOL_ERROR;
+		goto cleanup;
+	}
+	band = buf[0] & 0xff;
+	buf[len] = '\0';
+	len--;
+	switch (band) {
+	case 3:
+		strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
+			    DISPLAY_PREFIX);
+		maybe_colorize_sideband(&outbuf, buf + 1, len);
+
+		retval = SIDEBAND_REMOTE_ERROR;
+		break;
+	case 2:
+		b = buf + 1;
 
-	while (!retval) {
-		const char *b, *brk;
-		int band, len;
-		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
-		if (len == 0)
-			break;
-		if (len < 1) {
-			strbuf_addf(&outbuf,
-				    "%s%s: protocol error: no band designator",
-				    outbuf.len ? "\n" : "", me);
-			retval = SIDEBAND_PROTOCOL_ERROR;
-			break;
-		}
-		band = buf[0] & 0xff;
-		buf[len] = '\0';
-		len--;
-		switch (band) {
-		case 3:
-			strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
-				    DISPLAY_PREFIX);
-			maybe_colorize_sideband(&outbuf, buf + 1, len);
-
-			retval = SIDEBAND_REMOTE_ERROR;
-			break;
-		case 2:
-			b = buf + 1;
-
-			/*
-			 * Append a suffix to each nonempty line to clear the
-			 * end of the screen line.
-			 *
-			 * The output is accumulated in a buffer and
-			 * each line is printed to stderr using
-			 * write(2) to ensure inter-process atomicity.
-			 */
-			while ((brk = strpbrk(b, "\n\r"))) {
-				int linelen = brk - b;
-
-				if (!outbuf.len)
-					strbuf_addstr(&outbuf, DISPLAY_PREFIX);
-				if (linelen > 0) {
-					maybe_colorize_sideband(&outbuf, b, linelen);
-					strbuf_addstr(&outbuf, suffix);
-				}
-
-				strbuf_addch(&outbuf, *brk);
-				xwrite(2, outbuf.buf, outbuf.len);
-				strbuf_reset(&outbuf);
-
-				b = brk + 1;
+		/*
+		 * Append a suffix to each nonempty line to clear the
+		 * end of the screen line.
+		 *
+		 * The output is accumulated in a buffer and
+		 * each line is printed to stderr using
+		 * write(2) to ensure inter-process atomicity.
+		 */
+		while ((brk = strpbrk(b, "\n\r"))) {
+			int linelen = brk - b;
+
+			if (!outbuf.len)
+				strbuf_addstr(&outbuf, DISPLAY_PREFIX);
+			if (linelen > 0) {
+				maybe_colorize_sideband(&outbuf, b, linelen);
+				strbuf_addstr(&outbuf, suffix);
 			}
 
-			if (*b) {
-				strbuf_addstr(&outbuf, outbuf.len ?
-					    "" : DISPLAY_PREFIX);
-				maybe_colorize_sideband(&outbuf, b, strlen(b));
-			}
-			break;
-		case 1:
-			write_or_die(out, buf + 1, len);
-			break;
-		default:
-			strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d",
-				    outbuf.len ? "\n" : "", me, band);
-			retval = SIDEBAND_PROTOCOL_ERROR;
-			break;
+			strbuf_addch(&outbuf, *brk);
+			xwrite(2, outbuf.buf, outbuf.len);
+			strbuf_reset(&outbuf);
+
+			b = brk + 1;
+		}
+
+		if (*b) {
+			strbuf_addstr(&outbuf, outbuf.len ?
+				    "" : DISPLAY_PREFIX);
+			maybe_colorize_sideband(&outbuf, b, strlen(b));
 		}
+		retval = SIDEBAND_PROGRESS;
+		break;
+	case 1:
+		retval = SIDEBAND_PRIMARY;
+		break;
+	default:
+		strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d",
+			    outbuf.len ? "\n" : "", me, band);
+		retval = SIDEBAND_PROTOCOL_ERROR;
+		break;
 	}
 
+cleanup:
 	if (outbuf.len) {
 		strbuf_addch(&outbuf, '\n');
 		xwrite(2, outbuf.buf, outbuf.len);
diff --git a/sideband.h b/sideband.h
index 7a8146f161..7244971231 100644
--- a/sideband.h
+++ b/sideband.h
@@ -3,8 +3,20 @@
 
 #define SIDEBAND_PROTOCOL_ERROR -2
 #define SIDEBAND_REMOTE_ERROR -1
+#define SIDEBAND_FLUSH 0
+#define SIDEBAND_PRIMARY 1
+#define SIDEBAND_PROGRESS 2
+
+/*
+ * Inspects a multiplexed packet read from the remote and returns which
+ * sideband it is for.
+ *
+ * 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 demultiplex_sideband(const char *me, char *buf, int len);
 
-int recv_sideband(const char *me, int in_stream, int out);
 void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
 
 #endif
-- 
2.19.0.271.gfe8321ec05.dirty

Re: [PATCH v2 2/4] sideband: reverse its dependency on pkt-line

[SZEDER Gábor]

On Tue, Jan 15, 2019 at 11:40:28AM -0800, Jonathan Tan wrote:
> A subsequent patch will teach struct packet_reader a new field that, if
> set, instructs it to interpret read data as multiplexed. This will
> create a dependency from pkt-line to sideband.
> 
> To avoid a circular dependency, split recv_sideband() into 2 parts: the
> reading loop (left in recv_sideband()) and the processing of the
> contents (in demultiplex_sideband()), and move the former into pkt-line.
> This reverses the direction of dependency: sideband no longer depends on
> pkt-line, and pkt-line now depends on sideband.

In the last couple of days I noticed occasional but frequent failures
in the test 'leading space' in 't5409-colorize-remote-messas.sh' on
'pu' and on this topic.  Bisect suggests that this patch is the
culprit, but of course bisecting an occasional test failure can't be
completely trusted.

The trace output of the failing test looks like this:

  + git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/leading-space
  + cat output
  remote: error: error        
  remote: ERROR: also highlighted        
  remote: hint: hint        
  remote: hinting: not highlighted        
  remote: success: success        
  remote: warning: warning        
  remote: prefixerror: error        
  remote:   
  remote: error: leading space        
  remote:             
  remote: Err        
  To /<...>/trash directory.t5409-colorize-remote-messages.stress-4/.
   * [new branch]      HEAD -> leading-space
  + test_decode_color
  + awk 
  < ... snip enormous awk script ... >
  + grep   <BOLD;RED>error<RESET>: leading space decoded
  error: last command exited with $?=1
  not ok 6 - leading space

Notice how that "error: leading space" lines up with the other
messages.  On 'master' that line looks like this:

  remote:   error: leading space        

Re: [PATCH v2 2/4] sideband: reverse its dependency on pkt-line

[Jonathan Tan]

> On Tue, Jan 15, 2019 at 11:40:28AM -0800, Jonathan Tan wrote:
> > A subsequent patch will teach struct packet_reader a new field that, if
> > set, instructs it to interpret read data as multiplexed. This will
> > create a dependency from pkt-line to sideband.
> > 
> > To avoid a circular dependency, split recv_sideband() into 2 parts: the
> > reading loop (left in recv_sideband()) and the processing of the
> > contents (in demultiplex_sideband()), and move the former into pkt-line.
> > This reverses the direction of dependency: sideband no longer depends on
> > pkt-line, and pkt-line now depends on sideband.
> 
> In the last couple of days I noticed occasional but frequent failures
> in the test 'leading space' in 't5409-colorize-remote-messas.sh' on
> 'pu' and on this topic.  Bisect suggests that this patch is the
> culprit, but of course bisecting an occasional test failure can't be
> completely trusted.
> 
> The trace output of the failing test looks like this:

Thanks for checking. Junio noticed a similar thing and I have replied to
it here:

https://public-inbox.org/git/20190116003828.64889-1-jonathantanmy@google.com/

I'll try to ensure that this issue is fixed in the subsequent version.

Re: [PATCH v2 2/4] sideband: reverse its dependency on pkt-line

[SZEDER Gábor]

On Wed, Jan 16, 2019 at 09:43:19AM -0800, Jonathan Tan wrote:
> > On Tue, Jan 15, 2019 at 11:40:28AM -0800, Jonathan Tan wrote:
> > In the last couple of days I noticed occasional but frequent failures
> > in the test 'leading space' in 't5409-colorize-remote-messas.sh' on
 
> Thanks for checking. Junio noticed a similar thing and I have replied to
> it here:
> 
> https://public-inbox.org/git/20190116003828.64889-1-jonathantanmy@google.com/
> 
> I'll try to ensure that this issue is fixed in the subsequent version.

Oh, indeed, that must be the same issue.  After I bisected the issue I
only paid attention to the discussion of patch 2/4 in v1, and
completely ignored the replies to the cover letter.

But I'm glad that --stress makes itself useful :)

[PATCH v2 3/4] {fetch,upload}-pack: sideband v2 fetch response

[Jonathan Tan]

Currently, a response to a fetch request has sideband support only while
the packfile is being sent, meaning that the server cannot send notices
until the start of the packfile.

Extend sideband support in protocol v2 fetch responses to the whole
response. upload-pack will advertise it if the
uploadpack.allowsidebandall configuration variable is set, and
fetch-pack will automatically request it if advertised.

If the sideband is to be used throughout the whole response, upload-pack
will use it to send errors instead of prefixing a PKT-LINE payload with
"ERR ".

This will be tested in a subsequent patch.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 Documentation/technical/protocol-v2.txt | 10 +++++
 fetch-pack.c                            | 12 ++++-
 pkt-line.c                              | 58 ++++++++++++++++++++-----
 pkt-line.h                              |  4 ++
 sideband.c                              |  7 ++-
 sideband.h                              |  2 +-
 upload-pack.c                           | 16 +++++++
 7 files changed, 94 insertions(+), 15 deletions(-)

diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 09e4e0273f..1b0633f59f 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -307,6 +307,16 @@ the 'wanted-refs' section in the server's response as explained below.
 	particular ref, where <ref> is the full name of a ref on the
 	server.
 
+If the 'sideband-all' feature is advertised, the following argument can be
+included in the client's request:
+
+    sideband-all
+	Instruct the server to send the whole response multiplexed, not just
+	the packfile section. All non-flush and non-delim PKT-LINE in the
+	response (not only in the packfile section) will then start with a byte
+	indicating its sideband (1, 2, or 3), and the server may send "0005\1"
+	(a PKT-LINE of sideband 1 with no payload) as a keepalive packet.
+
 The response of `fetch` is broken into a number of sections separated by
 delimiter packets (0001), with each section beginning with its section
 header.
diff --git a/fetch-pack.c b/fetch-pack.c
index 3f9626dbf7..b89199891d 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1125,7 +1125,8 @@ static int add_haves(struct fetch_negotiator *negotiator,
 static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
 			      const struct fetch_pack_args *args,
 			      const struct ref *wants, struct oidset *common,
-			      int *haves_to_send, int *in_vain)
+			      int *haves_to_send, int *in_vain,
+			      int sideband_all)
 {
 	int ret = 0;
 	struct strbuf req_buf = STRBUF_INIT;
@@ -1151,6 +1152,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
 		packet_buf_write(&req_buf, "include-tag");
 	if (prefer_ofs_delta)
 		packet_buf_write(&req_buf, "ofs-delta");
+	if (sideband_all)
+		packet_buf_write(&req_buf, "sideband-all");
 
 	/* Add shallow-info and deepen request */
 	if (server_supports_feature("fetch", "shallow", 0))
@@ -1359,6 +1362,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	packet_reader_init(&reader, fd[0], NULL, 0,
 			   PACKET_READ_CHOMP_NEWLINE |
 			   PACKET_READ_DIE_ON_ERR_PACKET);
+	if (server_supports_feature("fetch", "sideband-all", 0)) {
+		reader.use_sideband = 1;
+		reader.me = "fetch-pack";
+	}
 
 	while (state != FETCH_DONE) {
 		switch (state) {
@@ -1392,7 +1399,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		case FETCH_SEND_REQUEST:
 			if (send_fetch_request(&negotiator, fd[1], args, ref,
 					       &common,
-					       &haves_to_send, &in_vain))
+					       &haves_to_send, &in_vain,
+					       reader.use_sideband))
 				state = FETCH_GET_PACK;
 			else
 				state = FETCH_PROCESS_ACKS;
diff --git a/pkt-line.c b/pkt-line.c
index 5feb73ebec..69162f3990 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -447,7 +447,7 @@ 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 = demultiplex_sideband(me, buf, len);
+		retval = demultiplex_sideband(me, buf, len, 0);
 		switch (retval) {
 		case SIDEBAND_PRIMARY:
 			write_or_die(out, buf + 1, len - 1);
@@ -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,16 +484,48 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
 		return reader->status;
 	}
 
-	reader->status = packet_read_with_status(reader->fd,
-						 &reader->src_buffer,
-						 &reader->src_len,
-						 reader->buffer,
-						 reader->buffer_size,
-						 &reader->pktlen,
-						 reader->options);
+	/*
+	 * 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,
+							 &reader->src_buffer,
+							 &reader->src_len,
+							 reader->buffer,
+							 reader->buffer_size,
+							 &reader->pktlen,
+							 reader->options);
+		if (!reader->use_sideband)
+			break;
+		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_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_received:
 	if (reader->status == PACKET_READ_NORMAL)
-		reader->line = reader->buffer;
+		/* Skip the sideband designator if sideband is used */
+		reader->line = reader->use_sideband ?
+			reader->buffer + 1 : reader->buffer;
 	else
 		reader->line = NULL;
 
@@ -514,6 +547,7 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)
 void packet_writer_init(struct packet_writer *writer, int dest_fd)
 {
 	writer->dest_fd = dest_fd;
+	writer->use_sideband = 0;
 }
 
 void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
@@ -521,7 +555,8 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args);
+	packet_write_fmt_1(writer->dest_fd, 0,
+			   writer->use_sideband ? "\001" : "", fmt, args);
 	va_end(args);
 }
 
@@ -530,7 +565,8 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args);
+	packet_write_fmt_1(writer->dest_fd, 0,
+			   writer->use_sideband ? "\003" : "ERR ", fmt, args);
 	va_end(args);
 }
 
diff --git a/pkt-line.h b/pkt-line.h
index a8e92a4b63..ad9a4a2cd7 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -162,6 +162,9 @@ struct packet_reader {
 
 	/* indicates if a line has been peeked */
 	int line_peeked;
+
+	unsigned use_sideband : 1;
+	const char *me;
 };
 
 /*
@@ -201,6 +204,7 @@ extern char packet_buffer[LARGE_PACKET_MAX];
 
 struct packet_writer {
 	int dest_fd;
+	unsigned use_sideband : 1;
 };
 
 void packet_writer_init(struct packet_writer *writer, int dest_fd);
diff --git a/sideband.c b/sideband.c
index dce22d20b1..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 demultiplex_sideband(const char *me, char *buf, int len)
+int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)
 {
 	static const char *suffix;
 	struct strbuf outbuf = STRBUF_INIT;
@@ -144,6 +144,9 @@ int demultiplex_sideband(const char *me, char *buf, int len)
 	len--;
 	switch (band) {
 	case 3:
+		if (die_on_error)
+			die("remote error: %s", buf + 1);
+
 		strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
 			    DISPLAY_PREFIX);
 		maybe_colorize_sideband(&outbuf, buf + 1, len);
@@ -196,6 +199,8 @@ int demultiplex_sideband(const char *me, char *buf, int len)
 	}
 
 cleanup:
+	if (die_on_error && retval == SIDEBAND_PROTOCOL_ERROR)
+		die("%s", outbuf.buf);
 	if (outbuf.len) {
 		strbuf_addch(&outbuf, '\n');
 		xwrite(2, outbuf.buf, outbuf.len);
diff --git a/sideband.h b/sideband.h
index 7244971231..f75c4fde2a 100644
--- a/sideband.h
+++ b/sideband.h
@@ -15,7 +15,7 @@
  * also prints a message (or the formatted contents of the notice in the case
  * of SIDEBAND_PROGRESS) to stderr.
  */
-int demultiplex_sideband(const char *me, char *buf, int len);
+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);
 
diff --git a/upload-pack.c b/upload-pack.c
index 60a26bbbfd..765b7695d2 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -71,6 +71,8 @@ static int allow_filter;
 static int allow_ref_in_want;
 static struct list_objects_filter_options filter_options;
 
+static int allow_sideband_all;
+
 static void reset_timeout(void)
 {
 	alarm(timeout);
@@ -1046,6 +1048,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
 		allow_filter = git_config_bool(var, value);
 	} else if (!strcmp("uploadpack.allowrefinwant", var)) {
 		allow_ref_in_want = git_config_bool(var, value);
+	} else if (!strcmp("uploadpack.allowsidebandall", var)) {
+		allow_sideband_all = git_config_bool(var, value);
 	}
 
 	if (current_config_scope() != CONFIG_SCOPE_REPO) {
@@ -1284,6 +1288,11 @@ static void process_args(struct packet_reader *request,
 			continue;
 		}
 
+		if (allow_sideband_all && !strcmp(arg, "sideband-all")) {
+			data->writer.use_sideband = 1;
+			continue;
+		}
+
 		/* ignore unknown lines maybe? */
 		die("unexpected line: '%s'", arg);
 	}
@@ -1496,6 +1505,7 @@ int upload_pack_advertise(struct repository *r,
 	if (value) {
 		int allow_filter_value;
 		int allow_ref_in_want;
+		int allow_sideband_all_value;
 
 		strbuf_addstr(value, "shallow");
 
@@ -1510,6 +1520,12 @@ int upload_pack_advertise(struct repository *r,
 					 &allow_ref_in_want) &&
 		    allow_ref_in_want)
 			strbuf_addstr(value, " ref-in-want");
+
+		if (!repo_config_get_bool(the_repository,
+					 "uploadpack.allowsidebandall",
+					 &allow_sideband_all_value) &&
+		    allow_sideband_all_value)
+			strbuf_addstr(value, " sideband-all");
 	}
 
 	return 1;
-- 
2.19.0.271.gfe8321ec05.dirty

[PATCH v2 4/4] tests: define GIT_TEST_SIDEBAND_ALL

[Jonathan Tan]

Define a GIT_TEST_SIDEBAND_ALL environment variable meant to be used
from tests. When set to true, this overrides uploadpack.allowsidebandall
to true, allowing the entire test suite to be run as if this
configuration is in place for all repositories.

As of this patch, all tests pass whether GIT_TEST_SIDEBAND_ALL is unset
or set to 1.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 fetch-pack.c             |  3 ++-
 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            | 13 ++++++++-----
 7 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index b89199891d..4618568fee 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1362,7 +1362,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	packet_reader_init(&reader, fd[0], NULL, 0,
 			   PACKET_READ_CHOMP_NEWLINE |
 			   PACKET_READ_DIE_ON_ERR_PACKET);
-	if (server_supports_feature("fetch", "sideband-all", 0)) {
+	if (git_env_bool("GIT_TEST_SIDEBAND_ALL", 1) &&
+	    server_supports_feature("fetch", "sideband-all", 0)) {
 		reader.use_sideband = 1;
 		reader.me = "fetch-pack";
 	}
diff --git a/t/README b/t/README
index 28711cc508..b275c883b8 100644
--- a/t/README
+++ b/t/README
@@ -358,6 +358,11 @@ GIT_TEST_MULTI_PACK_INDEX=<boolean>, when true, forces the multi-pack-
 index to be written after every 'git repack' command, and overrides the
 'core.multiPackIndex' setting to true.
 
+GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the
+'uploadpack.allowSidebandAll' setting to true, and when false, forces
+fetch-pack to not request sideband-all (even if the server advertises
+sideband-all).
+
 Naming Tests
 ------------
 
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8f..9a6d368247 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -78,6 +78,7 @@ PassEnv GNUPGHOME
 PassEnv ASAN_OPTIONS
 PassEnv GIT_TRACE
 PassEnv GIT_CONFIG_NOSYSTEM
+PassEnv GIT_TEST_SIDEBAND_ALL
 
 SetEnvIf Git-Protocol ".*" GIT_PROTOCOL=$0
 
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 6faf17e17a..6caf628efa 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -243,7 +243,8 @@ test_expect_success 'shallow fetches check connectivity before writing shallow f
 	       "$(git -C "$REPO" rev-parse HEAD)" \
 	       "$(git -C "$REPO" rev-parse HEAD^)" \
 	       >"$HTTPD_ROOT_PATH/one-time-sed" &&
-	test_must_fail git -C client fetch --depth=1 "$HTTPD_URL/one_time_sed/repo" \
+	test_must_fail env GIT_TEST_SIDEBAND_ALL=0 git -C client \
+		fetch --depth=1 "$HTTPD_URL/one_time_sed/repo" \
 		master:a_branch &&
 
 	# Ensure that the one-time-sed script was used.
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index ae79c6bbc0..fe45bf828d 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -14,7 +14,7 @@ test_expect_success 'test capability advertisement' '
 	0000
 	EOF
 
-	git serve --advertise-capabilities >out &&
+	GIT_TEST_SIDEBAND_ALL=0 git serve --advertise-capabilities >out &&
 	test-tool pkt-line unpack <out >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 0f2b09ebb8..b491c62e3e 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -583,8 +583,8 @@ test_expect_success 'when server does not send "ready", expect FLUSH' '
 	test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" git -C http_child \
 		-c protocol.version=2 \
 		fetch "$HTTPD_URL/one_time_sed/http_parent" 2> err &&
-	grep "fetch< acknowledgments" log &&
-	! grep "fetch< ready" log &&
+	grep "fetch< .*acknowledgments" log &&
+	! grep "fetch< .*ready" log &&
 	test_i18ngrep "expected no other sections to be sent after no .ready." err
 '
 
diff --git a/upload-pack.c b/upload-pack.c
index 765b7695d2..0c1feccaab 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1288,7 +1288,9 @@ static void process_args(struct packet_reader *request,
 			continue;
 		}
 
-		if (allow_sideband_all && !strcmp(arg, "sideband-all")) {
+		if ((git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) ||
+		     allow_sideband_all) &&
+		    !strcmp(arg, "sideband-all")) {
 			data->writer.use_sideband = 1;
 			continue;
 		}
@@ -1521,10 +1523,11 @@ int upload_pack_advertise(struct repository *r,
 		    allow_ref_in_want)
 			strbuf_addstr(value, " ref-in-want");
 
-		if (!repo_config_get_bool(the_repository,
-					 "uploadpack.allowsidebandall",
-					 &allow_sideband_all_value) &&
-		    allow_sideband_all_value)
+		if (git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) ||
+		    (!repo_config_get_bool(the_repository,
+					   "uploadpack.allowsidebandall",
+					   &allow_sideband_all_value) &&
+		     allow_sideband_all_value))
 			strbuf_addstr(value, " sideband-all");
 	}
 
-- 
2.19.0.271.gfe8321ec05.dirty

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

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> @@ -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";
>  }

This was somewhat unexpected.  I would have thought that an
interdiff would be more like

        +	reader.me = "fetch-pack";
                if (using sideband-all) {
                        reader.use_sideband = 1;
        -		reader.me = "fetch-pack";
                }

> +		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 */
> +			;

OK.

Will replace.  Thanks.

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

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

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

By the way, when merged to 'pu' as one of the earlier topic, t5409
starts to fail under --stress.

	$ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}'
	$ make
	$ cd t && sh ./t5409-col*.sh --stress

This is not new to this round; v1 exhibited the same symptom.

Thanks.

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

[Jonathan Tan]

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > Like v1, this is on origin/ms/packet-err-check.
> 
> By the way, when merged to 'pu' as one of the earlier topic, t5409
> starts to fail under --stress.
> 
> 	$ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}'
> 	$ make
> 	$ cd t && sh ./t5409-col*.sh --stress
> 
> This is not new to this round; v1 exhibited the same symptom.
> 
> Thanks.

Thanks for checking. I don't think this branch is the cause of this
issue, though. I ran the same stress test on both:

 - f3035d003e ("Merge branch 'sg/stress-test' into jch", 2019-01-14) and
 - the result of merging sg/stress-test into master,

and the test fails with the same result.

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

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

>> Jonathan Tan <jonathantanmy@google.com> writes:
>> 
>> > Like v1, this is on origin/ms/packet-err-check.
>> 
>> By the way, when merged to 'pu' as one of the earlier topic, t5409
>> starts to fail under --stress.
>> 
>> 	$ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}'
>> 	$ make
>> 	$ cd t && sh ./t5409-col*.sh --stress
>> 
>> This is not new to this round; v1 exhibited the same symptom.
>> 
>> Thanks.
>
> Thanks for checking. I don't think this branch is the cause of this
> issue, though. I ran the same stress test on both:
>
>  - f3035d003e ("Merge branch 'sg/stress-test' into jch", 2019-01-14) and
>  - the result of merging sg/stress-test into master,
>
> and the test fails with the same result.

Interesting.  That is not what I am seeing (as I manually bisected
the first-parent chain between f3035d003e and the tip of pu).

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

[Jonathan Tan]

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> >> Jonathan Tan <jonathantanmy@google.com> writes:
> >> 
> >> > Like v1, this is on origin/ms/packet-err-check.
> >> 
> >> By the way, when merged to 'pu' as one of the earlier topic, t5409
> >> starts to fail under --stress.
> >> 
> >> 	$ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}'
> >> 	$ make
> >> 	$ cd t && sh ./t5409-col*.sh --stress
> >> 
> >> This is not new to this round; v1 exhibited the same symptom.
> >> 
> >> Thanks.
> >
> > Thanks for checking. I don't think this branch is the cause of this
> > issue, though. I ran the same stress test on both:
> >
> >  - f3035d003e ("Merge branch 'sg/stress-test' into jch", 2019-01-14) and
> >  - the result of merging sg/stress-test into master,
> >
> > and the test fails with the same result.
> 
> Interesting.  That is not what I am seeing (as I manually bisected
> the first-parent chain between f3035d003e and the tip of pu).

Ah...yes, you're right. I forgot to build before running the tests. I'll
take a look.

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

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

>> Jonathan Tan <jonathantanmy@google.com> writes:
>> 
>> >> Jonathan Tan <jonathantanmy@google.com> writes:
>> >> 
>> >> > Like v1, this is on origin/ms/packet-err-check.
>> >> 
>> >> By the way, when merged to 'pu' as one of the earlier topic, t5409
>> >> starts to fail under --stress.
>> >> 
>> >> 	$ git checkout 'origin/pu^{/^Merge branch .jt/fetch-v2-sideband}'
>> >> 	$ make
>> >> 	$ cd t && sh ./t5409-col*.sh --stress
>> >> 
>> >> This is not new to this round; v1 exhibited the same symptom.
>> >> 
>> >> Thanks.
>> >
>> > Thanks for checking. I don't think this branch is the cause of this
>> > issue, though. I ran the same stress test on both:
>> >
>> >  - f3035d003e ("Merge branch 'sg/stress-test' into jch", 2019-01-14) and
>> >  - the result of merging sg/stress-test into master,
>> >
>> > and the test fails with the same result.
>> 
>> Interesting.  That is not what I am seeing (as I manually bisected
>> the first-parent chain between f3035d003e and the tip of pu).
>
> Ah...yes, you're right. I forgot to build before running the tests. I'll
> take a look.

Thanks.

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

[Jonathan Tan]

> > Ah...yes, you're right. I forgot to build before running the tests. I'll
> > take a look.
> 
> Thanks.

Thanks once again for taking a look. It turns out that it's because
progress messages are sometimes split across PKT-LINEs depending on your
luck, and we need to retain the "leftover" on a \2 sideband in order to
combine it with the next one if necessary. So, for example, the
following fixup works:

	diff --git a/sideband.c b/sideband.c
	index c185c38637..d5da587d68 100644
	--- a/sideband.c
	+++ b/sideband.c
	@@ -117,7 +117,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
	 int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)
	 {
	        static const char *suffix;
	-       struct strbuf outbuf = STRBUF_INIT;
	+       static struct strbuf outbuf = STRBUF_INIT;
	        int retval = 0;
	        const char *b, *brk;
	        int band;
	@@ -187,8 +187,7 @@ int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)
	                                    "" : DISPLAY_PREFIX);
	                        maybe_colorize_sideband(&outbuf, b, strlen(b));
	                }
	-               retval = SIDEBAND_PROGRESS;
	-               break;
	+               return SIDEBAND_PROGRESS; /* skip cleanup */
	        case 1:
	                retval = SIDEBAND_PRIMARY;
	                break;

We could make the caller of demultiplex_sideband() store the outbuf, but
at this point, it might be better to refactor packet_reader into its own
file and have it depend on both pkt-line.h and sideband.h. If you (or
anyone else) have any ideas, let me know what you think. I'll think
further about this too.

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

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> We could make the caller of demultiplex_sideband() store the outbuf, but
> at this point, it might be better to refactor packet_reader into its own
> file and have it depend on both pkt-line.h and sideband.h.

I do not know or too deeply care about the pkt-line / sideband
distinction, as they are logically one thing.  With a packet-reader
abstraction in place, doesn't this "here is the remainder of the
packet from the  previous round" belong to each reader instance?

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

[Jonathan Tan]

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

First of all, thanks to those who noticed the issue with t5409. I have
merged sg/stress-test and ran:

    make DEVELOPER=1 && (cd t && sh ./t5409-col*.sh --stress)

with no issues.

demultiplex_sideband() now requires its caller to maintain a strbuf,
since progress messages can be split across packets. I have also changed
the API of demultiplex_sideband() to hopefully make things clearer.

As for keepalive packets, I think it's best if we state that the remote
should use 0005\2 instead of 0005\1, and have made the change
accordingly. This does not need to be treated specially by the client.
(I shouldn't have a problem updating my CDN offloading patches [1] to
use 0005\2 before the packfile starts and 0005\1 after the packfile
starts.)

[1] Patches 5-8 of https://public-inbox.org/git/cover.1547244620.git.jonathantanmy@google.com/

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                              | 107 ++++++++++++--
 pkt-line.h                              |  34 +++++
 sideband.c                              | 178 ++++++++++++------------
 sideband.h                              |  25 +++-
 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, 346 insertions(+), 167 deletions(-)

Interdiff against v2:
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 1b0633f59f..39b40c0dc1 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -314,8 +314,8 @@ included in the client's request:
 	Instruct the server to send the whole response multiplexed, not just
 	the packfile section. All non-flush and non-delim PKT-LINE in the
 	response (not only in the packfile section) will then start with a byte
-	indicating its sideband (1, 2, or 3), and the server may send "0005\1"
-	(a PKT-LINE of sideband 1 with no payload) as a keepalive packet.
+	indicating its sideband (1, 2, or 3), and the server may send "0005\2"
+	(a PKT-LINE of sideband 2 with no payload) as a keepalive packet.
 
 The response of `fetch` is broken into a number of sections separated by
 delimiter packets (0001), with each section beginning with its section
diff --git a/pkt-line.c b/pkt-line.c
index 69162f3990..d4b71d3e82 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -442,21 +442,22 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)
 int recv_sideband(const char *me, int in_stream, int out)
 {
 	char buf[LARGE_PACKET_MAX + 1];
-	int retval = 0;
 	int len;
+	struct strbuf scratch = STRBUF_INIT;
+	enum sideband_type sideband_type;
 
 	while (1) {
-		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
-		retval = demultiplex_sideband(me, buf, len, 0);
-		switch (retval) {
+		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX,
+				  0);
+		if (!demultiplex_sideband(me, buf, len, 0, &scratch,
+					  &sideband_type))
+			continue;
+		switch (sideband_type) {
 		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;
+			return sideband_type;
 		}
 	}
 }
@@ -479,17 +480,19 @@ void packet_reader_init(struct packet_reader *reader, int fd,
 
 enum packet_read_status packet_reader_read(struct packet_reader *reader)
 {
+	struct strbuf scratch = STRBUF_INIT;
+
 	if (reader->line_peeked) {
 		reader->line_peeked = 0;
 		return reader->status;
 	}
 
 	/*
-	 * Consume all progress and keepalive packets until a primary payload
-	 * packet is received
+	 * Consume all progress packets until a primary payload packet is
+	 * received
 	 */
 	while (1) {
-		int retval;
+		enum sideband_type sideband_type;
 		reader->status = packet_read_with_status(reader->fd,
 							 &reader->src_buffer,
 							 &reader->src_len,
@@ -499,29 +502,12 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
 							 reader->options);
 		if (!reader->use_sideband)
 			break;
-		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_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.
-			 */
+		if (demultiplex_sideband(reader->me, reader->buffer,
+					 reader->pktlen, 1, &scratch,
+					 &sideband_type))
 			break;
-		default: /* SIDEBAND_PROGRESS */
-			;
-		}
 	}
 
-nonprogress_received:
 	if (reader->status == PACKET_READ_NORMAL)
 		/* Skip the sideband designator if sideband is used */
 		reader->line = reader->use_sideband ?
diff --git a/sideband.c b/sideband.c
index 9d3051e3fe..6a16feb262 100644
--- a/sideband.c
+++ b/sideband.c
@@ -113,11 +113,12 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 #define ANSI_SUFFIX "\033[K"
 #define DUMB_SUFFIX "        "
 
-int demultiplex_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,
+			 struct strbuf *scratch,
+			 enum sideband_type *sideband_type)
 {
 	static const char *suffix;
-	struct strbuf outbuf = STRBUF_INIT;
-	int retval = 0;
 	const char *b, *brk;
 	int band;
 
@@ -129,14 +130,14 @@ int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)
 	}
 
 	if (len == 0) {
-		retval = SIDEBAND_FLUSH;
+		*sideband_type = SIDEBAND_FLUSH;
 		goto cleanup;
 	}
 	if (len < 1) {
-		strbuf_addf(&outbuf,
+		strbuf_addf(scratch,
 			    "%s%s: protocol error: no band designator",
-			    outbuf.len ? "\n" : "", me);
-		retval = SIDEBAND_PROTOCOL_ERROR;
+			    scratch->len ? "\n" : "", me);
+		*sideband_type = SIDEBAND_PROTOCOL_ERROR;
 		goto cleanup;
 	}
 	band = buf[0] & 0xff;
@@ -146,12 +147,11 @@ int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)
 	case 3:
 		if (die_on_error)
 			die("remote error: %s", buf + 1);
-
-		strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
+		strbuf_addf(scratch, "%s%s", scratch->len ? "\n" : "",
 			    DISPLAY_PREFIX);
-		maybe_colorize_sideband(&outbuf, buf + 1, len);
+		maybe_colorize_sideband(scratch, buf + 1, len);
 
-		retval = SIDEBAND_REMOTE_ERROR;
+		*sideband_type = SIDEBAND_REMOTE_ERROR;
 		break;
 	case 2:
 		b = buf + 1;
@@ -167,46 +167,45 @@ int demultiplex_sideband(const char *me, char *buf, int len, int die_on_error)
 		while ((brk = strpbrk(b, "\n\r"))) {
 			int linelen = brk - b;
 
-			if (!outbuf.len)
-				strbuf_addstr(&outbuf, DISPLAY_PREFIX);
+			if (!scratch->len)
+				strbuf_addstr(scratch, DISPLAY_PREFIX);
 			if (linelen > 0) {
-				maybe_colorize_sideband(&outbuf, b, linelen);
-				strbuf_addstr(&outbuf, suffix);
+				maybe_colorize_sideband(scratch, b, linelen);
+				strbuf_addstr(scratch, suffix);
 			}
 
-			strbuf_addch(&outbuf, *brk);
-			xwrite(2, outbuf.buf, outbuf.len);
-			strbuf_reset(&outbuf);
+			strbuf_addch(scratch, *brk);
+			xwrite(2, scratch->buf, scratch->len);
+			strbuf_reset(scratch);
 
 			b = brk + 1;
 		}
 
 		if (*b) {
-			strbuf_addstr(&outbuf, outbuf.len ?
+			strbuf_addstr(scratch, scratch->len ?
 				    "" : DISPLAY_PREFIX);
-			maybe_colorize_sideband(&outbuf, b, strlen(b));
+			maybe_colorize_sideband(scratch, b, strlen(b));
 		}
-		retval = SIDEBAND_PROGRESS;
-		break;
+		return 0;
 	case 1:
-		retval = SIDEBAND_PRIMARY;
+		*sideband_type = SIDEBAND_PRIMARY;
 		break;
 	default:
-		strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d",
-			    outbuf.len ? "\n" : "", me, band);
-		retval = SIDEBAND_PROTOCOL_ERROR;
+		strbuf_addf(scratch, "%s%s: protocol error: bad band #%d",
+			    scratch->len ? "\n" : "", me, band);
+		*sideband_type = SIDEBAND_PROTOCOL_ERROR;
 		break;
 	}
 
 cleanup:
-	if (die_on_error && retval == SIDEBAND_PROTOCOL_ERROR)
-		die("%s", outbuf.buf);
-	if (outbuf.len) {
-		strbuf_addch(&outbuf, '\n');
-		xwrite(2, outbuf.buf, outbuf.len);
+	if (die_on_error && *sideband_type == SIDEBAND_PROTOCOL_ERROR)
+		die("%s", scratch->buf);
+	if (scratch->len) {
+		strbuf_addch(scratch, '\n');
+		xwrite(2, scratch->buf, scratch->len);
 	}
-	strbuf_release(&outbuf);
-	return retval;
+	strbuf_release(scratch);
+	return 1;
 }
 
 /*
diff --git a/sideband.h b/sideband.h
index f75c4fde2a..227740a58e 100644
--- a/sideband.h
+++ b/sideband.h
@@ -1,21 +1,28 @@
 #ifndef SIDEBAND_H
 #define SIDEBAND_H
 
-#define SIDEBAND_PROTOCOL_ERROR -2
-#define SIDEBAND_REMOTE_ERROR -1
-#define SIDEBAND_FLUSH 0
-#define SIDEBAND_PRIMARY 1
-#define SIDEBAND_PROGRESS 2
+enum sideband_type {
+	SIDEBAND_PROTOCOL_ERROR = -2,
+	SIDEBAND_REMOTE_ERROR = -1,
+	SIDEBAND_FLUSH = 0,
+	SIDEBAND_PRIMARY = 1
+};
 
 /*
- * Inspects a multiplexed packet read from the remote and returns which
- * sideband it is for.
+ * Inspects a multiplexed packet read from the remote. If this packet is a
+ * progress packet and thus should not be processed by the caller, returns 0.
+ * Otherwise, returns 1, releases scratch, and sets sideband_type.
  *
- * 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 this packet is SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or a
+ * progress packet, also prints a message to stderr.
+ *
+ * scratch must be a struct strbuf allocated by the caller. It is used to store
+ * progress messages split across multiple packets.
  */
-int demultiplex_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,
+			 struct strbuf *scratch,
+			 enum sideband_type *sideband_type);
 
 void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
 
-- 
2.19.0.271.gfe8321ec05.dirty

[PATCH v3 1/4] pkt-line: introduce struct packet_writer

[Jonathan Tan]

A future patch will allow the client to request multiplexing of the
entire fetch response (and not only during packfile transmission), which
in turn allows the server to send progress and keepalive messages at any
time during the response.

It will be convenient for a future patch if writing options
(specifically, whether the written data is to be multiplexed) could be
controlled from a single place, so create struct packet_writer to serve
as that place, and modify upload-pack to use it.

Currently, it only stores the output fd, but a subsequent patch will (as
described above) introduce an option to determine if the written data is
to be multiplexed.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 pkt-line.c    |  47 ++++++++++++++++++---
 pkt-line.h    |  14 +++++++
 upload-pack.c | 112 +++++++++++++++++++++++++++-----------------------
 3 files changed, 115 insertions(+), 58 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index e70ea6d88f..9d3e402d41 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -129,12 +129,14 @@ static void set_packet_header(char *buf, const int size)
 	#undef hex
 }
 
-static void format_packet(struct strbuf *out, const char *fmt, va_list args)
+static void format_packet(struct strbuf *out, const char *prefix,
+			  const char *fmt, va_list args)
 {
 	size_t orig_len, n;
 
 	orig_len = out->len;
 	strbuf_addstr(out, "0000");
+	strbuf_addstr(out, prefix);
 	strbuf_vaddf(out, fmt, args);
 	n = out->len - orig_len;
 
@@ -145,13 +147,13 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)
 	packet_trace(out->buf + orig_len + 4, n - 4, 1);
 }
 
-static int packet_write_fmt_1(int fd, int gently,
+static int packet_write_fmt_1(int fd, int gently, const char *prefix,
 			      const char *fmt, va_list args)
 {
 	static struct strbuf buf = STRBUF_INIT;
 
 	strbuf_reset(&buf);
-	format_packet(&buf, fmt, args);
+	format_packet(&buf, prefix, fmt, args);
 	if (write_in_full(fd, buf.buf, buf.len) < 0) {
 		if (!gently) {
 			check_pipe(errno);
@@ -168,7 +170,7 @@ void packet_write_fmt(int fd, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	packet_write_fmt_1(fd, 0, fmt, args);
+	packet_write_fmt_1(fd, 0, "", fmt, args);
 	va_end(args);
 }
 
@@ -178,7 +180,7 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	status = packet_write_fmt_1(fd, 1, fmt, args);
+	status = packet_write_fmt_1(fd, 1, "", fmt, args);
 	va_end(args);
 	return status;
 }
@@ -211,7 +213,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	format_packet(buf, fmt, args);
+	format_packet(buf, "", fmt, args);
 	va_end(args);
 }
 
@@ -486,3 +488,36 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)
 	reader->line_peeked = 1;
 	return reader->status;
 }
+
+void packet_writer_init(struct packet_writer *writer, int dest_fd)
+{
+	writer->dest_fd = dest_fd;
+}
+
+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
+{
+	va_list args;
+
+	va_start(args, fmt);
+	packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args);
+	va_end(args);
+}
+
+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
+{
+	va_list args;
+
+	va_start(args, fmt);
+	packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args);
+	va_end(args);
+}
+
+void packet_writer_delim(struct packet_writer *writer)
+{
+	packet_delim(writer->dest_fd);
+}
+
+void packet_writer_flush(struct packet_writer *writer)
+{
+	packet_flush(writer->dest_fd);
+}
diff --git a/pkt-line.h b/pkt-line.h
index d7e1dbc047..023ad2951d 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -183,4 +183,18 @@ extern enum packet_read_status packet_reader_peek(struct packet_reader *reader);
 #define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)
 extern char packet_buffer[LARGE_PACKET_MAX];
 
+struct packet_writer {
+	int dest_fd;
+};
+
+void packet_writer_init(struct packet_writer *writer, int dest_fd);
+
+/* These functions die upon failure. */
+__attribute__((format (printf, 2, 3)))
+void packet_writer_write(struct packet_writer *writer, const char *fmt, ...);
+__attribute__((format (printf, 2, 3)))
+void packet_writer_error(struct packet_writer *writer, const char *fmt, ...);
+void packet_writer_delim(struct packet_writer *writer);
+void packet_writer_flush(struct packet_writer *writer);
+
 #endif
diff --git a/upload-pack.c b/upload-pack.c
index 08b547cf01..60a26bbbfd 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -613,13 +613,14 @@ static void check_non_tip(struct object_array *want_obj)
 	}
 }
 
-static void send_shallow(struct commit_list *result)
+static void send_shallow(struct packet_writer *writer,
+			 struct commit_list *result)
 {
 	while (result) {
 		struct object *object = &result->item->object;
 		if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
-			packet_write_fmt(1, "shallow %s",
-					 oid_to_hex(&object->oid));
+			packet_writer_write(writer, "shallow %s",
+					    oid_to_hex(&object->oid));
 			register_shallow(the_repository, &object->oid);
 			shallow_nr++;
 		}
@@ -627,7 +628,8 @@ static void send_shallow(struct commit_list *result)
 	}
 }
 
-static void send_unshallow(const struct object_array *shallows,
+static void send_unshallow(struct packet_writer *writer,
+			   const struct object_array *shallows,
 			   struct object_array *want_obj)
 {
 	int i;
@@ -636,8 +638,8 @@ static void send_unshallow(const struct object_array *shallows,
 		struct object *object = shallows->objects[i].item;
 		if (object->flags & NOT_SHALLOW) {
 			struct commit_list *parents;
-			packet_write_fmt(1, "unshallow %s",
-					 oid_to_hex(&object->oid));
+			packet_writer_write(writer, "unshallow %s",
+					    oid_to_hex(&object->oid));
 			object->flags &= ~CLIENT_SHALLOW;
 			/*
 			 * We want to _register_ "object" as shallow, but we
@@ -662,7 +664,7 @@ static void send_unshallow(const struct object_array *shallows,
 	}
 }
 
-static void deepen(int depth, int deepen_relative,
+static void deepen(struct packet_writer *writer, int depth, int deepen_relative,
 		   struct object_array *shallows, struct object_array *want_obj)
 {
 	if (depth == INFINITE_DEPTH && !is_repository_shallow(the_repository)) {
@@ -680,7 +682,7 @@ static void deepen(int depth, int deepen_relative,
 		result = get_shallow_commits(&reachable_shallows,
 					     depth + 1,
 					     SHALLOW, NOT_SHALLOW);
-		send_shallow(result);
+		send_shallow(writer, result);
 		free_commit_list(result);
 		object_array_clear(&reachable_shallows);
 	} else {
@@ -688,14 +690,15 @@ static void deepen(int depth, int deepen_relative,
 
 		result = get_shallow_commits(want_obj, depth,
 					     SHALLOW, NOT_SHALLOW);
-		send_shallow(result);
+		send_shallow(writer, result);
 		free_commit_list(result);
 	}
 
-	send_unshallow(shallows, want_obj);
+	send_unshallow(writer, shallows, want_obj);
 }
 
-static void deepen_by_rev_list(int ac, const char **av,
+static void deepen_by_rev_list(struct packet_writer *writer, int ac,
+			       const char **av,
 			       struct object_array *shallows,
 			       struct object_array *want_obj)
 {
@@ -703,13 +706,14 @@ static void deepen_by_rev_list(int ac, const char **av,
 
 	close_commit_graph(the_repository);
 	result = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);
-	send_shallow(result);
+	send_shallow(writer, result);
 	free_commit_list(result);
-	send_unshallow(shallows, want_obj);
+	send_unshallow(writer, shallows, want_obj);
 }
 
 /* Returns 1 if a shallow list is sent or 0 otherwise */
-static int send_shallow_list(int depth, int deepen_rev_list,
+static int send_shallow_list(struct packet_writer *writer,
+			     int depth, int deepen_rev_list,
 			     timestamp_t deepen_since,
 			     struct string_list *deepen_not,
 			     struct object_array *shallows,
@@ -720,7 +724,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,
 	if (depth > 0 && deepen_rev_list)
 		die("git upload-pack: deepen and deepen-since (or deepen-not) cannot be used together");
 	if (depth > 0) {
-		deepen(depth, deepen_relative, shallows, want_obj);
+		deepen(writer, depth, deepen_relative, shallows, want_obj);
 		ret = 1;
 	} else if (deepen_rev_list) {
 		struct argv_array av = ARGV_ARRAY_INIT;
@@ -741,7 +745,7 @@ static int send_shallow_list(int depth, int deepen_rev_list,
 			struct object *o = want_obj->objects[i].item;
 			argv_array_push(&av, oid_to_hex(&o->oid));
 		}
-		deepen_by_rev_list(av.argc, av.argv, shallows, want_obj);
+		deepen_by_rev_list(writer, av.argc, av.argv, shallows, want_obj);
 		argv_array_clear(&av);
 		ret = 1;
 	} else {
@@ -834,8 +838,10 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
 	int has_non_tip = 0;
 	timestamp_t deepen_since = 0;
 	int deepen_rev_list = 0;
+	struct packet_writer writer;
 
 	shallow_nr = 0;
+	packet_writer_init(&writer, 1);
 	for (;;) {
 		struct object *o;
 		const char *features;
@@ -892,9 +898,9 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
 
 		o = parse_object(the_repository, &oid_buf);
 		if (!o) {
-			packet_write_fmt(1,
-					 "ERR upload-pack: not our ref %s",
-					 oid_to_hex(&oid_buf));
+			packet_writer_error(&writer,
+					    "upload-pack: not our ref %s",
+					    oid_to_hex(&oid_buf));
 			die("git upload-pack: not our ref %s",
 			    oid_to_hex(&oid_buf));
 		}
@@ -923,7 +929,7 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
 	if (depth == 0 && !deepen_rev_list && shallows.nr == 0)
 		return;
 
-	if (send_shallow_list(depth, deepen_rev_list, deepen_since,
+	if (send_shallow_list(&writer, depth, deepen_rev_list, deepen_since,
 			      &deepen_not, &shallows, want_obj))
 		packet_flush(1);
 	object_array_clear(&shallows);
@@ -1102,6 +1108,8 @@ struct upload_pack_data {
 	int deepen_rev_list;
 	int deepen_relative;
 
+	struct packet_writer writer;
+
 	unsigned stateless_rpc : 1;
 
 	unsigned use_thin_pack : 1;
@@ -1125,6 +1133,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
 	data->haves = haves;
 	data->shallows = shallows;
 	data->deepen_not = deepen_not;
+	packet_writer_init(&data->writer, 1);
 }
 
 static void upload_pack_data_clear(struct upload_pack_data *data)
@@ -1136,7 +1145,8 @@ static void upload_pack_data_clear(struct upload_pack_data *data)
 	string_list_clear(&data->deepen_not, 0);
 }
 
-static int parse_want(const char *line, struct object_array *want_obj)
+static int parse_want(struct packet_writer *writer, const char *line,
+		      struct object_array *want_obj)
 {
 	const char *arg;
 	if (skip_prefix(line, "want ", &arg)) {
@@ -1149,9 +1159,9 @@ static int parse_want(const char *line, struct object_array *want_obj)
 
 		o = parse_object(the_repository, &oid);
 		if (!o) {
-			packet_write_fmt(1,
-					 "ERR upload-pack: not our ref %s",
-					 oid_to_hex(&oid));
+			packet_writer_error(writer,
+					    "upload-pack: not our ref %s",
+					    oid_to_hex(&oid));
 			die("git upload-pack: not our ref %s",
 			    oid_to_hex(&oid));
 		}
@@ -1167,7 +1177,8 @@ static int parse_want(const char *line, struct object_array *want_obj)
 	return 0;
 }
 
-static int parse_want_ref(const char *line, struct string_list *wanted_refs,
+static int parse_want_ref(struct packet_writer *writer, const char *line,
+			  struct string_list *wanted_refs,
 			  struct object_array *want_obj)
 {
 	const char *arg;
@@ -1177,7 +1188,7 @@ static int parse_want_ref(const char *line, struct string_list *wanted_refs,
 		struct object *o;
 
 		if (read_ref(arg, &oid)) {
-			packet_write_fmt(1, "ERR unknown ref %s", arg);
+			packet_writer_error(writer, "unknown ref %s", arg);
 			die("unknown ref %s", arg);
 		}
 
@@ -1220,10 +1231,11 @@ static void process_args(struct packet_reader *request,
 		const char *p;
 
 		/* process want */
-		if (parse_want(arg, want_obj))
+		if (parse_want(&data->writer, arg, want_obj))
 			continue;
 		if (allow_ref_in_want &&
-		    parse_want_ref(arg, &data->wanted_refs, want_obj))
+		    parse_want_ref(&data->writer, arg, &data->wanted_refs,
+				   want_obj))
 			continue;
 		/* process have line */
 		if (parse_have(arg, &data->haves))
@@ -1317,26 +1329,26 @@ static int process_haves(struct oid_array *haves, struct oid_array *common,
 	return 0;
 }
 
-static int send_acks(struct oid_array *acks, struct strbuf *response,
+static int send_acks(struct packet_writer *writer, struct oid_array *acks,
 		     const struct object_array *have_obj,
 		     struct object_array *want_obj)
 {
 	int i;
 
-	packet_buf_write(response, "acknowledgments\n");
+	packet_writer_write(writer, "acknowledgments\n");
 
 	/* Send Acks */
 	if (!acks->nr)
-		packet_buf_write(response, "NAK\n");
+		packet_writer_write(writer, "NAK\n");
 
 	for (i = 0; i < acks->nr; i++) {
-		packet_buf_write(response, "ACK %s\n",
-				 oid_to_hex(&acks->oid[i]));
+		packet_writer_write(writer, "ACK %s\n",
+				    oid_to_hex(&acks->oid[i]));
 	}
 
 	if (ok_to_give_up(have_obj, want_obj)) {
 		/* Send Ready */
-		packet_buf_write(response, "ready\n");
+		packet_writer_write(writer, "ready\n");
 		return 1;
 	}
 
@@ -1348,25 +1360,20 @@ static int process_haves_and_send_acks(struct upload_pack_data *data,
 				       struct object_array *want_obj)
 {
 	struct oid_array common = OID_ARRAY_INIT;
-	struct strbuf response = STRBUF_INIT;
 	int ret = 0;
 
 	process_haves(&data->haves, &common, have_obj);
 	if (data->done) {
 		ret = 1;
-	} else if (send_acks(&common, &response, have_obj, want_obj)) {
-		packet_buf_delim(&response);
+	} else if (send_acks(&data->writer, &common, have_obj, want_obj)) {
+		packet_writer_delim(&data->writer);
 		ret = 1;
 	} else {
 		/* Add Flush */
-		packet_buf_flush(&response);
+		packet_writer_flush(&data->writer);
 		ret = 0;
 	}
 
-	/* Send response */
-	write_or_die(1, response.buf, response.len);
-	strbuf_release(&response);
-
 	oid_array_clear(&data->haves);
 	oid_array_clear(&common);
 	return ret;
@@ -1379,15 +1386,15 @@ static void send_wanted_ref_info(struct upload_pack_data *data)
 	if (!data->wanted_refs.nr)
 		return;
 
-	packet_write_fmt(1, "wanted-refs\n");
+	packet_writer_write(&data->writer, "wanted-refs\n");
 
 	for_each_string_list_item(item, &data->wanted_refs) {
-		packet_write_fmt(1, "%s %s\n",
-				 oid_to_hex(item->util),
-				 item->string);
+		packet_writer_write(&data->writer, "%s %s\n",
+				    oid_to_hex(item->util),
+				    item->string);
 	}
 
-	packet_delim(1);
+	packet_writer_delim(&data->writer);
 }
 
 static void send_shallow_info(struct upload_pack_data *data,
@@ -1398,14 +1405,15 @@ static void send_shallow_info(struct upload_pack_data *data,
 	    !is_repository_shallow(the_repository))
 		return;
 
-	packet_write_fmt(1, "shallow-info\n");
+	packet_writer_write(&data->writer, "shallow-info\n");
 
-	if (!send_shallow_list(data->depth, data->deepen_rev_list,
+	if (!send_shallow_list(&data->writer, data->depth,
+			       data->deepen_rev_list,
 			       data->deepen_since, &data->deepen_not,
 			       &data->shallows, want_obj) &&
 	    is_repository_shallow(the_repository))
-		deepen(INFINITE_DEPTH, data->deepen_relative, &data->shallows,
-		       want_obj);
+		deepen(&data->writer, INFINITE_DEPTH, data->deepen_relative,
+		       &data->shallows, want_obj);
 
 	packet_delim(1);
 }
@@ -1467,7 +1475,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
 			send_wanted_ref_info(&data);
 			send_shallow_info(&data, &want_obj);
 
-			packet_write_fmt(1, "packfile\n");
+			packet_writer_write(&data.writer, "packfile\n");
 			create_pack_file(&have_obj, &want_obj);
 			state = FETCH_DONE;
 			break;
-- 
2.19.0.271.gfe8321ec05.dirty

[PATCH v3 2/4] sideband: reverse its dependency on pkt-line

[Jonathan Tan]

A subsequent patch will teach struct packet_reader a new field that, if
set, instructs it to interpret read data as multiplexed. This will
create a dependency from pkt-line to sideband.

To avoid a circular dependency, split recv_sideband() into 2 parts: the
reading loop (left in recv_sideband()) and the processing of the
contents (in demultiplex_sideband()), and move the former into pkt-line.
This reverses the direction of dependency: sideband no longer depends on
pkt-line, and pkt-line now depends on sideband.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 pkt-line.c |  23 +++++++
 pkt-line.h |  16 +++++
 sideband.c | 173 +++++++++++++++++++++++++----------------------------
 sideband.h |  24 +++++++-
 4 files changed, 141 insertions(+), 95 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index 9d3e402d41..321ff632a5 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -439,6 +439,29 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)
 	return sb_out->len - orig_len;
 }
 
+int recv_sideband(const char *me, int in_stream, int out)
+{
+	char buf[LARGE_PACKET_MAX + 1];
+	int len;
+	struct strbuf scratch = STRBUF_INIT;
+	enum sideband_type sideband_type;
+
+	while (1) {
+		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX,
+				  0);
+		if (!demultiplex_sideband(me, buf, len, &scratch,
+					  &sideband_type))
+			continue;
+		switch (sideband_type) {
+		case SIDEBAND_PRIMARY:
+			write_or_die(out, buf + 1, len - 1);
+			break;
+		default: /* errors: message already written */
+			return sideband_type;
+		}
+	}
+}
+
 /* Packet Reader Functions */
 void packet_reader_init(struct packet_reader *reader, int fd,
 			char *src_buffer, size_t src_len,
diff --git a/pkt-line.h b/pkt-line.h
index 023ad2951d..a8e92a4b63 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -3,6 +3,7 @@
 
 #include "git-compat-util.h"
 #include "strbuf.h"
+#include "sideband.h"
 
 /*
  * Write a packetized stream, where each line is preceded by
@@ -120,6 +121,21 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
  */
 ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
 
+/*
+ * Receive multiplexed output stream over git native protocol.
+ * in_stream is the input stream from the remote, which carries data
+ * in pkt_line format with band designator.  Demultiplex it into out
+ * and err and return error appropriately.  Band #1 carries the
+ * primary payload.  Things coming over band #2 is not necessarily
+ * error; they are usually informative message on the standard error
+ * stream, aka "verbose").  A message over band #3 is a signal that
+ * the remote died unexpectedly.  A flush() concludes the stream.
+ *
+ * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR
+ * or SIDEBAND_REMOTE_ERROR if an error occurred.
+ */
+int recv_sideband(const char *me, int in_stream, int out);
+
 struct packet_reader {
 	/* source file descriptor */
 	int fd;
diff --git a/sideband.c b/sideband.c
index 368647acf8..e89d677626 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,7 +1,6 @@
 #include "cache.h"
 #include "color.h"
 #include "config.h"
-#include "pkt-line.h"
 #include "sideband.h"
 #include "help.h"
 
@@ -109,109 +108,99 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 }
 
 
-/*
- * Receive multiplexed output stream over git native protocol.
- * in_stream is the input stream from the remote, which carries data
- * in pkt_line format with band designator.  Demultiplex it into out
- * and err and return error appropriately.  Band #1 carries the
- * primary payload.  Things coming over band #2 is not necessarily
- * error; they are usually informative message on the standard error
- * stream, aka "verbose").  A message over band #3 is a signal that
- * the remote died unexpectedly.  A flush() concludes the stream.
- */
-
 #define DISPLAY_PREFIX "remote: "
 
 #define ANSI_SUFFIX "\033[K"
 #define DUMB_SUFFIX "        "
 
-int recv_sideband(const char *me, int in_stream, int out)
+int demultiplex_sideband(const char *me, char *buf, int len,
+			 struct strbuf *scratch,
+			 enum sideband_type *sideband_type)
 {
-	const char *suffix;
-	char buf[LARGE_PACKET_MAX + 1];
-	struct strbuf outbuf = STRBUF_INIT;
-	int retval = 0;
-
-	if (isatty(2) && !is_terminal_dumb())
-		suffix = ANSI_SUFFIX;
-	else
-		suffix = DUMB_SUFFIX;
-
-	while (!retval) {
-		const char *b, *brk;
-		int band, len;
-		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
-		if (len == 0)
-			break;
-		if (len < 1) {
-			strbuf_addf(&outbuf,
-				    "%s%s: protocol error: no band designator",
-				    outbuf.len ? "\n" : "", me);
-			retval = SIDEBAND_PROTOCOL_ERROR;
-			break;
-		}
-		band = buf[0] & 0xff;
-		buf[len] = '\0';
-		len--;
-		switch (band) {
-		case 3:
-			strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
-				    DISPLAY_PREFIX);
-			maybe_colorize_sideband(&outbuf, buf + 1, len);
-
-			retval = SIDEBAND_REMOTE_ERROR;
-			break;
-		case 2:
-			b = buf + 1;
-
-			/*
-			 * Append a suffix to each nonempty line to clear the
-			 * end of the screen line.
-			 *
-			 * The output is accumulated in a buffer and
-			 * each line is printed to stderr using
-			 * write(2) to ensure inter-process atomicity.
-			 */
-			while ((brk = strpbrk(b, "\n\r"))) {
-				int linelen = brk - b;
-
-				if (!outbuf.len)
-					strbuf_addstr(&outbuf, DISPLAY_PREFIX);
-				if (linelen > 0) {
-					maybe_colorize_sideband(&outbuf, b, linelen);
-					strbuf_addstr(&outbuf, suffix);
-				}
-
-				strbuf_addch(&outbuf, *brk);
-				xwrite(2, outbuf.buf, outbuf.len);
-				strbuf_reset(&outbuf);
-
-				b = brk + 1;
-			}
+	static const char *suffix;
+	const char *b, *brk;
+	int band;
+
+	if (!suffix) {
+		if (isatty(2) && !is_terminal_dumb())
+			suffix = ANSI_SUFFIX;
+		else
+			suffix = DUMB_SUFFIX;
+	}
 
-			if (*b) {
-				strbuf_addstr(&outbuf, outbuf.len ?
-					    "" : DISPLAY_PREFIX);
-				maybe_colorize_sideband(&outbuf, b, strlen(b));
+	if (len == 0) {
+		*sideband_type = SIDEBAND_FLUSH;
+		goto cleanup;
+	}
+	if (len < 1) {
+		strbuf_addf(scratch,
+			    "%s%s: protocol error: no band designator",
+			    scratch->len ? "\n" : "", me);
+		*sideband_type = SIDEBAND_PROTOCOL_ERROR;
+		goto cleanup;
+	}
+	band = buf[0] & 0xff;
+	buf[len] = '\0';
+	len--;
+	switch (band) {
+	case 3:
+		strbuf_addf(scratch, "%s%s", scratch->len ? "\n" : "",
+			    DISPLAY_PREFIX);
+		maybe_colorize_sideband(scratch, buf + 1, len);
+
+		*sideband_type = SIDEBAND_REMOTE_ERROR;
+		break;
+	case 2:
+		b = buf + 1;
+
+		/*
+		 * Append a suffix to each nonempty line to clear the
+		 * end of the screen line.
+		 *
+		 * The output is accumulated in a buffer and
+		 * each line is printed to stderr using
+		 * write(2) to ensure inter-process atomicity.
+		 */
+		while ((brk = strpbrk(b, "\n\r"))) {
+			int linelen = brk - b;
+
+			if (!scratch->len)
+				strbuf_addstr(scratch, DISPLAY_PREFIX);
+			if (linelen > 0) {
+				maybe_colorize_sideband(scratch, b, linelen);
+				strbuf_addstr(scratch, suffix);
 			}
-			break;
-		case 1:
-			write_or_die(out, buf + 1, len);
-			break;
-		default:
-			strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d",
-				    outbuf.len ? "\n" : "", me, band);
-			retval = SIDEBAND_PROTOCOL_ERROR;
-			break;
+
+			strbuf_addch(scratch, *brk);
+			xwrite(2, scratch->buf, scratch->len);
+			strbuf_reset(scratch);
+
+			b = brk + 1;
+		}
+
+		if (*b) {
+			strbuf_addstr(scratch, scratch->len ?
+				    "" : DISPLAY_PREFIX);
+			maybe_colorize_sideband(scratch, b, strlen(b));
 		}
+		return 0;
+	case 1:
+		*sideband_type = SIDEBAND_PRIMARY;
+		break;
+	default:
+		strbuf_addf(scratch, "%s%s: protocol error: bad band #%d",
+			    scratch->len ? "\n" : "", me, band);
+		*sideband_type = SIDEBAND_PROTOCOL_ERROR;
+		break;
 	}
 
-	if (outbuf.len) {
-		strbuf_addch(&outbuf, '\n');
-		xwrite(2, outbuf.buf, outbuf.len);
+cleanup:
+	if (scratch->len) {
+		strbuf_addch(scratch, '\n');
+		xwrite(2, scratch->buf, scratch->len);
 	}
-	strbuf_release(&outbuf);
-	return retval;
+	strbuf_release(scratch);
+	return 1;
 }
 
 /*
diff --git a/sideband.h b/sideband.h
index 7a8146f161..2c4f021645 100644
--- a/sideband.h
+++ b/sideband.h
@@ -1,10 +1,28 @@
 #ifndef SIDEBAND_H
 #define SIDEBAND_H
 
-#define SIDEBAND_PROTOCOL_ERROR -2
-#define SIDEBAND_REMOTE_ERROR -1
+enum sideband_type {
+	SIDEBAND_PROTOCOL_ERROR = -2,
+	SIDEBAND_REMOTE_ERROR = -1,
+	SIDEBAND_FLUSH = 0,
+	SIDEBAND_PRIMARY = 1
+};
+
+/*
+ * Inspects a multiplexed packet read from the remote. If this packet is a
+ * progress packet and thus should not be processed by the caller, returns 0.
+ * Otherwise, returns 1, releases scratch, and sets sideband_type.
+ *
+ * If this packet is SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or a
+ * progress packet, also prints a message to stderr.
+ *
+ * scratch must be a struct strbuf allocated by the caller. It is used to store
+ * progress messages split across multiple packets.
+ */
+int demultiplex_sideband(const char *me, char *buf, int len,
+			 struct strbuf *scratch,
+			 enum sideband_type *sideband_type);
 
-int recv_sideband(const char *me, int in_stream, int out);
 void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
 
 #endif
-- 
2.19.0.271.gfe8321ec05.dirty

[PATCH v3 3/4] {fetch,upload}-pack: sideband v2 fetch response

[Jonathan Tan]

Currently, a response to a fetch request has sideband support only while
the packfile is being sent, meaning that the server cannot send notices
until the start of the packfile.

Extend sideband support in protocol v2 fetch responses to the whole
response. upload-pack will advertise it if the
uploadpack.allowsidebandall configuration variable is set, and
fetch-pack will automatically request it if advertised.

If the sideband is to be used throughout the whole response, upload-pack
will use it to send errors instead of prefixing a PKT-LINE payload with
"ERR ".

This will be tested in a subsequent patch.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/technical/protocol-v2.txt | 10 ++++++
 fetch-pack.c                            | 12 +++++--
 pkt-line.c                              | 43 ++++++++++++++++++-------
 pkt-line.h                              |  4 +++
 sideband.c                              |  5 +++
 sideband.h                              |  1 +
 upload-pack.c                           | 16 +++++++++
 7 files changed, 78 insertions(+), 13 deletions(-)

diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 09e4e0273f..39b40c0dc1 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -307,6 +307,16 @@ the 'wanted-refs' section in the server's response as explained below.
 	particular ref, where <ref> is the full name of a ref on the
 	server.
 
+If the 'sideband-all' feature is advertised, the following argument can be
+included in the client's request:
+
+    sideband-all
+	Instruct the server to send the whole response multiplexed, not just
+	the packfile section. All non-flush and non-delim PKT-LINE in the
+	response (not only in the packfile section) will then start with a byte
+	indicating its sideband (1, 2, or 3), and the server may send "0005\2"
+	(a PKT-LINE of sideband 2 with no payload) as a keepalive packet.
+
 The response of `fetch` is broken into a number of sections separated by
 delimiter packets (0001), with each section beginning with its section
 header.
diff --git a/fetch-pack.c b/fetch-pack.c
index ee0847e6ae..86e9e18901 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1090,7 +1090,8 @@ static int add_haves(struct fetch_negotiator *negotiator,
 static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
 			      const struct fetch_pack_args *args,
 			      const struct ref *wants, struct oidset *common,
-			      int *haves_to_send, int *in_vain)
+			      int *haves_to_send, int *in_vain,
+			      int sideband_all)
 {
 	int ret = 0;
 	struct strbuf req_buf = STRBUF_INIT;
@@ -1116,6 +1117,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
 		packet_buf_write(&req_buf, "include-tag");
 	if (prefer_ofs_delta)
 		packet_buf_write(&req_buf, "ofs-delta");
+	if (sideband_all)
+		packet_buf_write(&req_buf, "sideband-all");
 
 	/* Add shallow-info and deepen request */
 	if (server_supports_feature("fetch", "shallow", 0))
@@ -1324,6 +1327,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	packet_reader_init(&reader, fd[0], NULL, 0,
 			   PACKET_READ_CHOMP_NEWLINE |
 			   PACKET_READ_DIE_ON_ERR_PACKET);
+	if (server_supports_feature("fetch", "sideband-all", 0)) {
+		reader.use_sideband = 1;
+		reader.me = "fetch-pack";
+	}
 
 	while (state != FETCH_DONE) {
 		switch (state) {
@@ -1357,7 +1364,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		case FETCH_SEND_REQUEST:
 			if (send_fetch_request(&negotiator, fd[1], args, ref,
 					       &common,
-					       &haves_to_send, &in_vain))
+					       &haves_to_send, &in_vain,
+					       reader.use_sideband))
 				state = FETCH_GET_PACK;
 			else
 				state = FETCH_PROCESS_ACKS;
diff --git a/pkt-line.c b/pkt-line.c
index 321ff632a5..d4b71d3e82 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -449,7 +449,7 @@ 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);
-		if (!demultiplex_sideband(me, buf, len, &scratch,
+		if (!demultiplex_sideband(me, buf, len, 0, &scratch,
 					  &sideband_type))
 			continue;
 		switch (sideband_type) {
@@ -475,25 +475,43 @@ 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)
 {
+	struct strbuf scratch = STRBUF_INIT;
+
 	if (reader->line_peeked) {
 		reader->line_peeked = 0;
 		return reader->status;
 	}
 
-	reader->status = packet_read_with_status(reader->fd,
-						 &reader->src_buffer,
-						 &reader->src_len,
-						 reader->buffer,
-						 reader->buffer_size,
-						 &reader->pktlen,
-						 reader->options);
+	/*
+	 * Consume all progress packets until a primary payload packet is
+	 * received
+	 */
+	while (1) {
+		enum sideband_type sideband_type;
+		reader->status = packet_read_with_status(reader->fd,
+							 &reader->src_buffer,
+							 &reader->src_len,
+							 reader->buffer,
+							 reader->buffer_size,
+							 &reader->pktlen,
+							 reader->options);
+		if (!reader->use_sideband)
+			break;
+		if (demultiplex_sideband(reader->me, reader->buffer,
+					 reader->pktlen, 1, &scratch,
+					 &sideband_type))
+			break;
+	}
 
 	if (reader->status == PACKET_READ_NORMAL)
-		reader->line = reader->buffer;
+		/* Skip the sideband designator if sideband is used */
+		reader->line = reader->use_sideband ?
+			reader->buffer + 1 : reader->buffer;
 	else
 		reader->line = NULL;
 
@@ -515,6 +533,7 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader)
 void packet_writer_init(struct packet_writer *writer, int dest_fd)
 {
 	writer->dest_fd = dest_fd;
+	writer->use_sideband = 0;
 }
 
 void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
@@ -522,7 +541,8 @@ void packet_writer_write(struct packet_writer *writer, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	packet_write_fmt_1(writer->dest_fd, 0, "", fmt, args);
+	packet_write_fmt_1(writer->dest_fd, 0,
+			   writer->use_sideband ? "\001" : "", fmt, args);
 	va_end(args);
 }
 
@@ -531,7 +551,8 @@ void packet_writer_error(struct packet_writer *writer, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	packet_write_fmt_1(writer->dest_fd, 0, "ERR ", fmt, args);
+	packet_write_fmt_1(writer->dest_fd, 0,
+			   writer->use_sideband ? "\003" : "ERR ", fmt, args);
 	va_end(args);
 }
 
diff --git a/pkt-line.h b/pkt-line.h
index a8e92a4b63..ad9a4a2cd7 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -162,6 +162,9 @@ struct packet_reader {
 
 	/* indicates if a line has been peeked */
 	int line_peeked;
+
+	unsigned use_sideband : 1;
+	const char *me;
 };
 
 /*
@@ -201,6 +204,7 @@ extern char packet_buffer[LARGE_PACKET_MAX];
 
 struct packet_writer {
 	int dest_fd;
+	unsigned use_sideband : 1;
 };
 
 void packet_writer_init(struct packet_writer *writer, int dest_fd);
diff --git a/sideband.c b/sideband.c
index e89d677626..6a16feb262 100644
--- a/sideband.c
+++ b/sideband.c
@@ -114,6 +114,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 #define DUMB_SUFFIX "        "
 
 int demultiplex_sideband(const char *me, char *buf, int len,
+			 int die_on_error,
 			 struct strbuf *scratch,
 			 enum sideband_type *sideband_type)
 {
@@ -144,6 +145,8 @@ int demultiplex_sideband(const char *me, char *buf, int len,
 	len--;
 	switch (band) {
 	case 3:
+		if (die_on_error)
+			die("remote error: %s", buf + 1);
 		strbuf_addf(scratch, "%s%s", scratch->len ? "\n" : "",
 			    DISPLAY_PREFIX);
 		maybe_colorize_sideband(scratch, buf + 1, len);
@@ -195,6 +198,8 @@ int demultiplex_sideband(const char *me, char *buf, int len,
 	}
 
 cleanup:
+	if (die_on_error && *sideband_type == SIDEBAND_PROTOCOL_ERROR)
+		die("%s", scratch->buf);
 	if (scratch->len) {
 		strbuf_addch(scratch, '\n');
 		xwrite(2, scratch->buf, scratch->len);
diff --git a/sideband.h b/sideband.h
index 2c4f021645..227740a58e 100644
--- a/sideband.h
+++ b/sideband.h
@@ -20,6 +20,7 @@ enum sideband_type {
  * progress messages split across multiple packets.
  */
 int demultiplex_sideband(const char *me, char *buf, int len,
+			 int die_on_error,
 			 struct strbuf *scratch,
 			 enum sideband_type *sideband_type);
 
diff --git a/upload-pack.c b/upload-pack.c
index 60a26bbbfd..765b7695d2 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -71,6 +71,8 @@ static int allow_filter;
 static int allow_ref_in_want;
 static struct list_objects_filter_options filter_options;
 
+static int allow_sideband_all;
+
 static void reset_timeout(void)
 {
 	alarm(timeout);
@@ -1046,6 +1048,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
 		allow_filter = git_config_bool(var, value);
 	} else if (!strcmp("uploadpack.allowrefinwant", var)) {
 		allow_ref_in_want = git_config_bool(var, value);
+	} else if (!strcmp("uploadpack.allowsidebandall", var)) {
+		allow_sideband_all = git_config_bool(var, value);
 	}
 
 	if (current_config_scope() != CONFIG_SCOPE_REPO) {
@@ -1284,6 +1288,11 @@ static void process_args(struct packet_reader *request,
 			continue;
 		}
 
+		if (allow_sideband_all && !strcmp(arg, "sideband-all")) {
+			data->writer.use_sideband = 1;
+			continue;
+		}
+
 		/* ignore unknown lines maybe? */
 		die("unexpected line: '%s'", arg);
 	}
@@ -1496,6 +1505,7 @@ int upload_pack_advertise(struct repository *r,
 	if (value) {
 		int allow_filter_value;
 		int allow_ref_in_want;
+		int allow_sideband_all_value;
 
 		strbuf_addstr(value, "shallow");
 
@@ -1510,6 +1520,12 @@ int upload_pack_advertise(struct repository *r,
 					 &allow_ref_in_want) &&
 		    allow_ref_in_want)
 			strbuf_addstr(value, " ref-in-want");
+
+		if (!repo_config_get_bool(the_repository,
+					 "uploadpack.allowsidebandall",
+					 &allow_sideband_all_value) &&
+		    allow_sideband_all_value)
+			strbuf_addstr(value, " sideband-all");
 	}
 
 	return 1;
-- 
2.19.0.271.gfe8321ec05.dirty

[PATCH v3 4/4] tests: define GIT_TEST_SIDEBAND_ALL

[Jonathan Tan]

Define a GIT_TEST_SIDEBAND_ALL environment variable meant to be used
from tests. When set to true, this overrides uploadpack.allowsidebandall
to true, allowing the entire test suite to be run as if this
configuration is in place for all repositories.

As of this patch, all tests pass whether GIT_TEST_SIDEBAND_ALL is unset
or set to 1.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 fetch-pack.c             |  3 ++-
 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            | 13 ++++++++-----
 7 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 86e9e18901..e98294d918 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1327,7 +1327,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	packet_reader_init(&reader, fd[0], NULL, 0,
 			   PACKET_READ_CHOMP_NEWLINE |
 			   PACKET_READ_DIE_ON_ERR_PACKET);
-	if (server_supports_feature("fetch", "sideband-all", 0)) {
+	if (git_env_bool("GIT_TEST_SIDEBAND_ALL", 1) &&
+	    server_supports_feature("fetch", "sideband-all", 0)) {
 		reader.use_sideband = 1;
 		reader.me = "fetch-pack";
 	}
diff --git a/t/README b/t/README
index 28711cc508..b275c883b8 100644
--- a/t/README
+++ b/t/README
@@ -358,6 +358,11 @@ GIT_TEST_MULTI_PACK_INDEX=<boolean>, when true, forces the multi-pack-
 index to be written after every 'git repack' command, and overrides the
 'core.multiPackIndex' setting to true.
 
+GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the
+'uploadpack.allowSidebandAll' setting to true, and when false, forces
+fetch-pack to not request sideband-all (even if the server advertises
+sideband-all).
+
 Naming Tests
 ------------
 
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8f..9a6d368247 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -78,6 +78,7 @@ PassEnv GNUPGHOME
 PassEnv ASAN_OPTIONS
 PassEnv GIT_TRACE
 PassEnv GIT_CONFIG_NOSYSTEM
+PassEnv GIT_TEST_SIDEBAND_ALL
 
 SetEnvIf Git-Protocol ".*" GIT_PROTOCOL=$0
 
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 6faf17e17a..6caf628efa 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -243,7 +243,8 @@ test_expect_success 'shallow fetches check connectivity before writing shallow f
 	       "$(git -C "$REPO" rev-parse HEAD)" \
 	       "$(git -C "$REPO" rev-parse HEAD^)" \
 	       >"$HTTPD_ROOT_PATH/one-time-sed" &&
-	test_must_fail git -C client fetch --depth=1 "$HTTPD_URL/one_time_sed/repo" \
+	test_must_fail env GIT_TEST_SIDEBAND_ALL=0 git -C client \
+		fetch --depth=1 "$HTTPD_URL/one_time_sed/repo" \
 		master:a_branch &&
 
 	# Ensure that the one-time-sed script was used.
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index ae79c6bbc0..fe45bf828d 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -14,7 +14,7 @@ test_expect_success 'test capability advertisement' '
 	0000
 	EOF
 
-	git serve --advertise-capabilities >out &&
+	GIT_TEST_SIDEBAND_ALL=0 git serve --advertise-capabilities >out &&
 	test-tool pkt-line unpack <out >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 0f2b09ebb8..b491c62e3e 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -583,8 +583,8 @@ test_expect_success 'when server does not send "ready", expect FLUSH' '
 	test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" git -C http_child \
 		-c protocol.version=2 \
 		fetch "$HTTPD_URL/one_time_sed/http_parent" 2> err &&
-	grep "fetch< acknowledgments" log &&
-	! grep "fetch< ready" log &&
+	grep "fetch< .*acknowledgments" log &&
+	! grep "fetch< .*ready" log &&
 	test_i18ngrep "expected no other sections to be sent after no .ready." err
 '
 
diff --git a/upload-pack.c b/upload-pack.c
index 765b7695d2..0c1feccaab 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1288,7 +1288,9 @@ static void process_args(struct packet_reader *request,
 			continue;
 		}
 
-		if (allow_sideband_all && !strcmp(arg, "sideband-all")) {
+		if ((git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) ||
+		     allow_sideband_all) &&
+		    !strcmp(arg, "sideband-all")) {
 			data->writer.use_sideband = 1;
 			continue;
 		}
@@ -1521,10 +1523,11 @@ int upload_pack_advertise(struct repository *r,
 		    allow_ref_in_want)
 			strbuf_addstr(value, " ref-in-want");
 
-		if (!repo_config_get_bool(the_repository,
-					 "uploadpack.allowsidebandall",
-					 &allow_sideband_all_value) &&
-		    allow_sideband_all_value)
+		if (git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) ||
+		    (!repo_config_get_bool(the_repository,
+					   "uploadpack.allowsidebandall",
+					   &allow_sideband_all_value) &&
+		     allow_sideband_all_value))
 			strbuf_addstr(value, " sideband-all");
 	}
 
-- 
2.19.0.271.gfe8321ec05.dirty

Re: [PATCH v3 4/4] tests: define GIT_TEST_SIDEBAND_ALL

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -78,6 +78,7 @@ PassEnv GNUPGHOME
>  PassEnv ASAN_OPTIONS
>  PassEnv GIT_TRACE
>  PassEnv GIT_CONFIG_NOSYSTEM
> +PassEnv GIT_TEST_SIDEBAND_ALL

This is producing

 [env:warn] [pid 248632] AH01506: PassEnv variable GIT_TEST_SIDEBAND_ALL was undefined

when tests are run with "prove".

Should we set the envvar unconditionally in lib-httpd.sh?

Thanks,
Jonathan

Re: [PATCH v3 4/4] tests: define GIT_TEST_SIDEBAND_ALL

[Jeff King]

On Tue, Jan 29, 2019 at 03:27:32PM -0800, Jonathan Nieder wrote:

> Jonathan Tan wrote:
> 
> > --- a/t/lib-httpd/apache.conf
> > +++ b/t/lib-httpd/apache.conf
> > @@ -78,6 +78,7 @@ PassEnv GNUPGHOME
> >  PassEnv ASAN_OPTIONS
> >  PassEnv GIT_TRACE
> >  PassEnv GIT_CONFIG_NOSYSTEM
> > +PassEnv GIT_TEST_SIDEBAND_ALL
> 
> This is producing
> 
>  [env:warn] [pid 248632] AH01506: PassEnv variable GIT_TEST_SIDEBAND_ALL was undefined
> 
> when tests are run with "prove".
> 
> Should we set the envvar unconditionally in lib-httpd.sh?

Yes, that's the same solution we've had to use for GIT_VALGRIND, etc. I
don't think there's an easier way.

-Peff

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

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

>  /*
> + * Inspects a multiplexed packet read from the remote. If this packet is a
> + * progress packet and thus should not be processed by the caller, returns 0.
> + * Otherwise, returns 1, releases scratch, and sets sideband_type.
>   *
> + * If this packet is SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or a
> + * progress packet, also prints a message to stderr.
> + *
> + * scratch must be a struct strbuf allocated by the caller. It is used to store
> + * progress messages split across multiple packets.
>   */
> +int demultiplex_sideband(const char *me, char *buf, int len,
> +			 int die_on_error,
> +			 struct strbuf *scratch,
> +			 enum sideband_type *sideband_type);

OK.  That sounds like a sensible way to keep the leftover across
calls.

Will queue.