[PATCH] push: Use sideband channel for hook messages

["Shawn O. Pearce" <spearce@spearce.org>]

Rather than sending hook messages over stderr, and losing them
entirely on git:// and smart HTTP transports, receive-pack now
puts them onto a multiplexed sideband channel if the send-pack
client asks for the side-band-64k capablity.  This ensures that
hooks from the server can report their detailed error messages,
if any, no matter what git-aware transport is being used.

When the side band channel is being used the push client will wind up
prefixing all server messages with "remote: ", just like fetch does.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 We should have done this back when we added smart HTTP, because
 otherwise the server can't send its hook messages back to the
 client.  I'd argue that is a bug, and so this should go in maint,
 but I can also see how some might consider this a new feature... so
 whatever.  :-)

 builtin-receive-pack.c  |  111 +++++++++++++++++++++++++++++++++++++----------
 builtin-send-pack.c     |   67 +++++++++++++++++++++-------
 run-command.c           |   24 ++++++++--
 run-command.h           |    1 +
 t/t5401-update-hooks.sh |   22 +++++-----
 5 files changed, 170 insertions(+), 55 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 78c0e69..e7bdd71 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -2,6 +2,7 @@
 #include "pack.h"
 #include "refs.h"
 #include "pkt-line.h"
+#include "sideband.h"
 #include "run-command.h"
 #include "exec_cmd.h"
 #include "commit.h"
@@ -27,11 +28,12 @@ static int receive_unpack_limit = -1;
 static int transfer_unpack_limit = -1;
 static int unpack_limit = 100;
 static int report_status;
+static int use_sideband;
 static int prefer_ofs_delta = 1;
 static int auto_update_server_info;
 static int auto_gc = 1;
 static const char *head_name;
-static char *capabilities_to_send;
+static int send_capabilities = 1;
 
 static enum deny_action parse_deny_action(const char *var, const char *value)
 {
@@ -105,19 +107,21 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 
 static int show_ref(const char *path, const unsigned char *sha1, int flag, void *cb_data)
 {
-	if (!capabilities_to_send)
+	if (!send_capabilities)
 		packet_write(1, "%s %s\n", sha1_to_hex(sha1), path);
 	else
-		packet_write(1, "%s %s%c%s\n",
-			     sha1_to_hex(sha1), path, 0, capabilities_to_send);
-	capabilities_to_send = NULL;
+		packet_write(1, "%s %s%c%s%s\n",
+			     sha1_to_hex(sha1), path, 0,
+			     " report-status delete-refs side-band-64k",
+			     prefer_ofs_delta ? " ofs-delta" : "");
+	send_capabilities = 0;
 	return 0;
 }
 
 static void write_head_info(void)
 {
 	for_each_ref(show_ref, NULL);
-	if (capabilities_to_send)
+	if (send_capabilities)
 		show_ref("capabilities^{}", null_sha1, 0, NULL);
 
 }
@@ -135,11 +139,25 @@ static struct command *commands;
 static const char pre_receive_hook[] = "hooks/pre-receive";
 static const char post_receive_hook[] = "hooks/post-receive";
 
+static int copy_to_sideband(int in, void *arg)
+{
+	char data[128];
+	while (1) {
+		ssize_t sz = xread(in, data, sizeof(data));
+		if (sz <= 0)
+			break;
+		send_sideband(1, 2, data, sz, use_sideband);
+	}
+	close(in);
+	return 0;
+}
+
 static int run_receive_hook(const char *hook_name)
 {
 	static char buf[sizeof(commands->old_sha1) * 2 + PATH_MAX + 4];
 	struct command *cmd;
 	struct child_process proc;
+	struct async sideband_mux;
 	const char *argv[2];
 	int have_input = 0, code;
 
@@ -159,9 +177,23 @@ static int run_receive_hook(const char *hook_name)
 	proc.in = -1;
 	proc.stdout_to_stderr = 1;
 
+	if (use_sideband) {
+		memset(&sideband_mux, 0, sizeof(sideband_mux));
+		sideband_mux.proc = copy_to_sideband;
+		sideband_mux.is_reader = 1;
+		code = start_async(&sideband_mux);
+		if (code)
+			return code;
+		proc.err = sideband_mux.out;
+	}
+
 	code = start_command(&proc);
-	if (code)
+	if (code) {
+		if (use_sideband)
+			finish_async(&sideband_mux);
 		return code;
+	}
+
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (!cmd->error_string) {
 			size_t n = snprintf(buf, sizeof(buf), "%s %s %s\n",
@@ -173,6 +205,8 @@ static int run_receive_hook(const char *hook_name)
 		}
 	}
 	close(proc.in);
+	if (use_sideband)
+		finish_async(&sideband_mux);
 	return finish_command(&proc);
 }
 
@@ -180,6 +214,8 @@ static int run_update_hook(struct command *cmd)
 {
 	static const char update_hook[] = "hooks/update";
 	const char *argv[5];
+	struct child_process proc;
+	int code;
 
 	if (access(update_hook, X_OK) < 0)
 		return 0;
@@ -190,8 +226,18 @@ static int run_update_hook(struct command *cmd)
 	argv[3] = sha1_to_hex(cmd->new_sha1);
 	argv[4] = NULL;
 
-	return run_command_v_opt(argv, RUN_COMMAND_NO_STDIN |
-					RUN_COMMAND_STDOUT_TO_STDERR);
+	memset(&proc, 0, sizeof(proc));
+	proc.no_stdin = 1;
+	proc.stdout_to_stderr = 1;
+	proc.err = use_sideband ? -1 : 0;
+	proc.argv = argv;
+
+	code = start_command(&proc);
+	if (code)
+		return code;
+	if (use_sideband)
+		copy_to_sideband(proc.err, NULL);
+	return finish_command(&proc);
 }
 
 static int is_ref_checked_out(const char *ref)
@@ -380,8 +426,9 @@ static char update_post_hook[] = "hooks/post-update";
 static void run_update_post_hook(struct command *cmd)
 {
 	struct command *cmd_p;
-	int argc, status;
+	int argc;
 	const char **argv;
+	struct child_process proc;
 
 	for (argc = 0, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {
 		if (cmd_p->error_string)
@@ -403,8 +450,18 @@ static void run_update_post_hook(struct command *cmd)
 		argc++;
 	}
 	argv[argc] = NULL;
-	status = run_command_v_opt(argv, RUN_COMMAND_NO_STDIN
-			| RUN_COMMAND_STDOUT_TO_STDERR);
+
+	memset(&proc, 0, sizeof(proc));
+	proc.no_stdin = 1;
+	proc.stdout_to_stderr = 1;
+	proc.err = use_sideband ? -1 : 0;
+	proc.argv = argv;
+
+	if (!start_command(&proc)) {
+		if (use_sideband)
+			copy_to_sideband(proc.err, NULL);
+		finish_command(&proc);
+	}
 }
 
 static void execute_commands(const char *unpacker_error)
@@ -464,6 +521,8 @@ static void read_head_info(void)
 		if (reflen + 82 < len) {
 			if (strstr(refname + reflen + 1, "report-status"))
 				report_status = 1;
+			if (strstr(refname + reflen + 1, "side-band-64k"))
+				use_sideband = LARGE_PACKET_MAX;
 		}
 		cmd = xmalloc(sizeof(struct command) + len - 80);
 		hashcpy(cmd->old_sha1, old_sha1);
@@ -563,17 +622,25 @@ static const char *unpack(void)
 static void report(const char *unpack_status)
 {
 	struct command *cmd;
-	packet_write(1, "unpack %s\n",
-		     unpack_status ? unpack_status : "ok");
+	struct strbuf buf = STRBUF_INIT;
+
+	packet_buf_write(&buf, "unpack %s\n",
+			 unpack_status ? unpack_status : "ok");
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (!cmd->error_string)
-			packet_write(1, "ok %s\n",
-				     cmd->ref_name);
+			packet_buf_write(&buf, "ok %s\n",
+					 cmd->ref_name);
 		else
-			packet_write(1, "ng %s %s\n",
-				     cmd->ref_name, cmd->error_string);
+			packet_buf_write(&buf, "ng %s %s\n",
+					 cmd->ref_name, cmd->error_string);
 	}
-	packet_flush(1);
+	packet_buf_flush(&buf);
+
+	if (use_sideband)
+		send_sideband(1, 1, buf.buf, buf.len, use_sideband);
+	else
+		safe_write(1, buf.buf, buf.len);
+	strbuf_release(&buf);
 }
 
 static int delete_only(struct command *cmd)
@@ -670,10 +737,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 	else if (0 <= receive_unpack_limit)
 		unpack_limit = receive_unpack_limit;
 
-	capabilities_to_send = (prefer_ofs_delta) ?
-		" report-status delete-refs ofs-delta " :
-		" report-status delete-refs ";
-
 	if (advertise_refs || !stateless_rpc) {
 		add_alternate_refs();
 		write_head_info();
@@ -707,5 +770,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		if (auto_update_server_info)
 			update_server_info(0);
 	}
+	if (use_sideband)
+		packet_flush(1);
 	return 0;
 }
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 8fffdbf..4bbc39f 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -372,6 +372,15 @@ static void print_helper_status(struct ref *ref)
 	strbuf_release(&buf);
 }
 
+static int sideband_decoder_callback(int out, void *data)
+{
+	int *fd = data;
+	int in = fd[0];
+	int ret = recv_sideband("send-pack", in, out);
+	close(out);
+	return ret;
+}
+
 int send_pack(struct send_pack_args *args,
 	      int fd[], struct child_process *conn,
 	      struct ref *remote_refs,
@@ -382,18 +391,22 @@ int send_pack(struct send_pack_args *args,
 	struct strbuf req_buf = STRBUF_INIT;
 	struct ref *ref;
 	int new_refs;
-	int ask_for_status_report = 0;
 	int allow_deleting_refs = 0;
-	int expect_status_report = 0;
+	int status_report = 0;
+	int use_sideband = 0;
+	unsigned cmds_sent = 0;
 	int ret;
+	struct async sideband_decoder;
 
 	/* Does the other end support the reporting? */
 	if (server_supports("report-status"))
-		ask_for_status_report = 1;
+		status_report = 1;
 	if (server_supports("delete-refs"))
 		allow_deleting_refs = 1;
 	if (server_supports("ofs-delta"))
 		args->use_ofs_delta = 1;
+	if (server_supports("side-band-64k"))
+		use_sideband = 1;
 
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
@@ -456,28 +469,30 @@ int send_pack(struct send_pack_args *args,
 		if (!ref->deletion)
 			new_refs++;
 
-		if (!args->dry_run) {
+		if (args->dry_run) {
+			ref->status = REF_STATUS_OK;
+		} else {
 			char *old_hex = sha1_to_hex(ref->old_sha1);
 			char *new_hex = sha1_to_hex(ref->new_sha1);
 
-			if (ask_for_status_report) {
-				packet_buf_write(&req_buf, "%s %s %s%c%s",
+			if (!cmds_sent && (status_report || use_sideband)) {
+				packet_buf_write(&req_buf, "%s %s %s%c%s%s",
 					old_hex, new_hex, ref->name, 0,
-					"report-status");
-				ask_for_status_report = 0;
-				expect_status_report = 1;
+					status_report ? " report-status" : "",
+					use_sideband ? " side-band-64k" : "");
 			}
 			else
 				packet_buf_write(&req_buf, "%s %s %s",
 					old_hex, new_hex, ref->name);
+			ref->status = status_report ?
+				REF_STATUS_EXPECTING_REPORT :
+				REF_STATUS_OK;
+			cmds_sent++;
 		}
-		ref->status = expect_status_report ?
-			REF_STATUS_EXPECTING_REPORT :
-			REF_STATUS_OK;
 	}
 
 	if (args->stateless_rpc) {
-		if (!args->dry_run) {
+		if (!args->dry_run && cmds_sent) {
 			packet_buf_flush(&req_buf);
 			send_sideband(out, -1, req_buf.buf, req_buf.len, LARGE_PACKET_MAX);
 		}
@@ -487,23 +502,43 @@ int send_pack(struct send_pack_args *args,
 	}
 	strbuf_release(&req_buf);
 
-	if (new_refs && !args->dry_run) {
+	if (use_sideband && cmds_sent) {
+		memset(&sideband_decoder, 0, sizeof(sideband_decoder));
+		sideband_decoder.proc = sideband_decoder_callback;
+		sideband_decoder.data = fd;
+		if (start_async(&sideband_decoder))
+			die("cannot start sideband decoder");
+		in = sideband_decoder.out;
+	}
+
+	if (new_refs && cmds_sent) {
 		if (pack_objects(out, remote_refs, extra_have, args) < 0) {
 			for (ref = remote_refs; ref; ref = ref->next)
 				ref->status = REF_STATUS_NONE;
+			if (use_sideband)
+				finish_async(&sideband_decoder);
 			return -1;
 		}
 	}
-	if (args->stateless_rpc && !args->dry_run)
+	if (args->stateless_rpc && cmds_sent)
 		packet_flush(out);
 
-	if (expect_status_report)
+	if (status_report && cmds_sent)
 		ret = receive_status(in, remote_refs);
 	else
 		ret = 0;
 	if (args->stateless_rpc)
 		packet_flush(out);
 
+	if (use_sideband && cmds_sent) {
+		int err = finish_async(&sideband_decoder);
+		if (err) {
+			error("sideband decoder failed %d", err);
+			ret = -1;
+		}
+		close(sideband_decoder.out);
+	}
+
 	if (ret < 0)
 		return ret;
 	for (ref = remote_refs; ref; ref = ref->next) {
diff --git a/run-command.c b/run-command.c
index cf2d8f7..7d1fd88 100644
--- a/run-command.c
+++ b/run-command.c
@@ -94,6 +94,9 @@ fail_pipe:
 		else if (need_err) {
 			dup2(fderr[1], 2);
 			close_pair(fderr);
+		} else if (cmd->err > 1) {
+			dup2(cmd->err, 2);
+			close(cmd->err);
 		}
 
 		if (cmd->no_stdout)
@@ -228,6 +231,8 @@ fail_pipe:
 
 	if (need_err)
 		close(fderr[1]);
+	else if (cmd->err)
+		close(cmd->err);
 
 	return 0;
 }
@@ -326,10 +331,19 @@ static unsigned __stdcall run_thread(void *data)
 int start_async(struct async *async)
 {
 	int pipe_out[2];
+	int proc_fd, call_fd;
 
 	if (pipe(pipe_out) < 0)
 		return error("cannot create pipe: %s", strerror(errno));
-	async->out = pipe_out[0];
+
+	if (async->is_reader) {
+		proc_fd = pipe_out[0];
+		call_fd = pipe_out[1];
+	} else {
+		call_fd = pipe_out[0];
+		proc_fd = pipe_out[1];
+	}
+	async->out = call_fd;
 
 #ifndef WIN32
 	/* Flush stdio before fork() to avoid cloning buffers */
@@ -342,12 +356,12 @@ int start_async(struct async *async)
 		return -1;
 	}
 	if (!async->pid) {
-		close(pipe_out[0]);
-		exit(!!async->proc(pipe_out[1], async->data));
+		close(call_fd);
+		exit(!!async->proc(proc_fd, async->data));
 	}
-	close(pipe_out[1]);
+	close(proc_fd);
 #else
-	async->fd_for_proc = pipe_out[1];
+	async->fd_for_proc = proc_fd;
 	async->tid = (HANDLE) _beginthreadex(NULL, 0, run_thread, async, 0, NULL);
 	if (!async->tid) {
 		error("cannot create thread: %s", strerror(errno));
diff --git a/run-command.h b/run-command.h
index fb34209..8aed83f 100644
--- a/run-command.h
+++ b/run-command.h
@@ -70,6 +70,7 @@ struct async {
 	int (*proc)(int fd, void *data);
 	void *data;
 	int out;	/* caller reads from here and closes it */
+	unsigned is_reader:1;
 #ifndef WIN32
 	pid_t pid;
 #else
diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh
index 64f66c9..c3cf397 100755
--- a/t/t5401-update-hooks.sh
+++ b/t/t5401-update-hooks.sh
@@ -118,19 +118,19 @@ test_expect_success 'send-pack produced no output' '
 '
 
 cat <<EOF >expect
-STDOUT pre-receive
-STDERR pre-receive
-STDOUT update refs/heads/master
-STDERR update refs/heads/master
-STDOUT update refs/heads/tofail
-STDERR update refs/heads/tofail
-STDOUT post-receive
-STDERR post-receive
-STDOUT post-update
-STDERR post-update
+remote: STDOUT pre-receive
+remote: STDERR pre-receive
+remote: STDOUT update refs/heads/master
+remote: STDERR update refs/heads/master
+remote: STDOUT update refs/heads/tofail
+remote: STDERR update refs/heads/tofail
+remote: STDOUT post-receive
+remote: STDERR post-receive
+remote: STDOUT post-update
+remote: STDERR post-update
 EOF
 test_expect_success 'send-pack stderr contains hook messages' '
-	grep ^STD send.err >actual &&
+	grep ^remote: send.err | sed "s/ *\$//" >actual &&
 	test_cmp - actual <expect
 '
 
-- 
1.7.0.rc1.199.g9253ab

-- 
Shawn.