[PATCH v2 0/6] receive-pack hooks over sideband

[PATCH v2 0/6] receive-pack hooks over sideband

[Shawn O. Pearce]

This is a respin of the patch I posted yesterday.  Same idea,
we want to make the messages printed by hooks available over the
sideband protocol so git:// and http:// smart clients are able to
present them, similar to how ssh:// works.

No real major changes from v1, the series is just broken apart to
make it easier to review, and Erik Faye-Lund's patch was integrated
to get a bidirectional async interface, rather than my hacked
.is_reader attempt in v1.

Erik Faye-Lund (1):
  run-command: support custom fd-set in async

Shawn O. Pearce (5):
  run-command: Allow stderr to be a caller supplied pipe
  send-pack: demultiplex a sideband stream with status data
  receive-pack: Refactor how capabilities are shown to the client
  receive-pack: Wrap status reports inside side-band-64k
  receive-pack: Send hook output over side band #2

 Documentation/technical/api-run-command.txt |   52 ++++++++++---
 builtin-fetch-pack.c                        |    7 +-
 builtin-receive-pack.c                      |  111 +++++++++++++++++++++------
 builtin-send-pack.c                         |   66 ++++++++++++----
 convert.c                                   |    5 +-
 remote-curl.c                               |    7 +-
 run-command.c                               |   91 +++++++++++++++++++---
 run-command.h                               |   11 ++-
 t/t5401-update-hooks.sh                     |   22 +++---
 upload-pack.c                               |    7 +-
 10 files changed, 290 insertions(+), 89 deletions(-)

[PATCH v2 1/6] run-command: Allow stderr to be a caller supplied pipe

[Shawn O. Pearce]

Like .out, .err may now be set to a file descriptor > 0, which
is a writable pipe/socket/file that the child's stderr will be
redirected into.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 Documentation/technical/api-run-command.txt |    2 +-
 run-command.c                               |    8 ++++++++
 run-command.h                               |    2 +-
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt
index b26c281..a1280dd 100644
--- a/Documentation/technical/api-run-command.txt
+++ b/Documentation/technical/api-run-command.txt
@@ -135,7 +135,7 @@ stderr as follows:
 
 	.in: The FD must be readable; it becomes child's stdin.
 	.out: The FD must be writable; it becomes child's stdout.
-	.err > 0 is not supported.
+	.err: The FD must be writable; it becomes child's stderr.
 
   The specified FD is closed by start_command(), even if it fails to
   run the sub-process!
diff --git a/run-command.c b/run-command.c
index cf2d8f7..bfd2312 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)
@@ -156,6 +159,9 @@ fail_pipe:
 	} else if (need_err) {
 		s2 = dup(2);
 		dup2(fderr[1], 2);
+	} else if (cmd->err > 2) {
+		s2 = dup(2);
+		dup2(cmd->err, 2);
 	}
 
 	if (cmd->no_stdout) {
@@ -228,6 +234,8 @@ fail_pipe:
 
 	if (need_err)
 		close(fderr[1]);
+	else if (cmd->err)
+		close(cmd->err);
 
 	return 0;
 }
diff --git a/run-command.h b/run-command.h
index fb34209..a29171a 100644
--- a/run-command.h
+++ b/run-command.h
@@ -18,7 +18,7 @@ struct child_process {
 	 * - Specify > 0 to set a channel to a particular FD as follows:
 	 *     .in: a readable FD, becomes child's stdin
 	 *     .out: a writable FD, becomes child's stdout/stderr
-	 *     .err > 0 not supported
+	 *     .err: a writable FD, becomes child's stderr
 	 *   The specified FD is closed by start_command(), even in case
 	 *   of errors!
 	 */
-- 
1.7.0.rc1.199.g9253ab

[PATCH v2 2/6] run-command: support custom fd-set in async

[Shawn O. Pearce]

From: Erik Faye-Lund <kusmabite@gmail.com>

This patch adds the possibility to supply a set of non-0 file
descriptors for async process communication instead of the
default-created pipe.

Additionally, we now support bi-directional communiction with the
async procedure, by giving the async function both read and write
file descriptors.

To retain compatiblity and similar "API feel" with start_command,
we require start_async callers to set .out = -1 to get a readable
file descriptor.  If either of .in or .out is 0, we supply no file
descriptor to the async process.

[sp: Note: Erik started this patch, and a huge bulk of it is
     his work.  All bugs were introduced later by Shawn.]

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 Documentation/technical/api-run-command.txt |   50 +++++++++++++---
 builtin-fetch-pack.c                        |    7 +-
 convert.c                                   |    5 +-
 remote-curl.c                               |    7 +-
 run-command.c                               |   83 ++++++++++++++++++++++----
 run-command.h                               |    9 ++-
 upload-pack.c                               |    7 +-
 7 files changed, 131 insertions(+), 37 deletions(-)

diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt
index a1280dd..8994859 100644
--- a/Documentation/technical/api-run-command.txt
+++ b/Documentation/technical/api-run-command.txt
@@ -64,8 +64,8 @@ The functions above do the following:
 `start_async`::
 
 	Run a function asynchronously. Takes a pointer to a `struct
-	async` that specifies the details and returns a pipe FD
-	from which the caller reads. See below for details.
+	async` that specifies the details and returns a set of pipe FDs
+	for communication with the function. See below for details.
 
 `finish_async`::
 
@@ -180,17 +180,47 @@ The caller:
    struct async variable;
 2. initializes .proc and .data;
 3. calls start_async();
-4. processes the data by reading from the fd in .out;
-5. closes .out;
+4. processes communicates with proc through .in and .out;
+5. closes .in and .out;
 6. calls finish_async().
 
+The members .in, .out are used to provide a set of fd's for
+communication between the caller and the callee as follows:
+
+. Specify 0 to have no file descriptor passed.  The callee will
+  receive -1 in the corresponding argument.
+
+. Specify < 0 to have a pipe allocated; start_async() replaces
+  with the pipe FD in the following way:
+
+	.in: Returns the writable pipe end into which the caller
+	writes; the readable end of the pipe becomes the function's
+	in argument.
+
+	.out: Returns the readable pipe end from which the caller
+	reads; the writable end of the pipe becomes the function's
+	out argument.
+
+  The caller of start_async() must close the returned FDs after it
+  has completed reading from/writing from them.
+
+. Specify a file descriptor > 0 to be used by the function:
+
+	.in: The FD must be readable; it becomes the function's in.
+	.out: The FD must be writable; it becomes the function's out.
+
+  The specified FD is closed by start_async(), even if it fails to
+  run the function.
+
 The function pointer in .proc has the following signature:
 
-	int proc(int fd, void *data);
+	int proc(int in, int out, void *data);
 
-. fd specifies a writable file descriptor to which the function must
-  write the data that it produces. The function *must* close this
-  descriptor before it returns.
+. in, out specifies a set of file descriptors to which the function
+  must read/write the data that it needs/produces.  The function
+  *must* close these descriptors before it returns.  A descriptor
+  may be -1 if the caller did not configure a descriptor for that
+  direction.
 
 . data is the value that the caller has specified in the .data member
   of struct async.
@@ -205,8 +235,8 @@ because this facility is implemented by a pipe to a forked process on
 UNIX, but by a thread in the same address space on Windows:
 
 . It cannot change the program's state (global variables, environment,
-  etc.) in a way that the caller notices; in other words, .out is the
-  only communication channel to the caller.
+  etc.) in a way that the caller notices; in other words, .in and .out
+  are the only communication channels to the caller.
 
 . It must not change the program's state that the caller of the
   facility also uses.
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 8ed4a6f..dbd8b7b 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -586,12 +586,12 @@ static int everything_local(struct ref **refs, int nr_match, char **match)
 	return retval;
 }
 
-static int sideband_demux(int fd, void *data)
+static int sideband_demux(int in, int out, void *data)
 {
 	int *xd = data;
 
-	int ret = recv_sideband("fetch-pack", xd[0], fd);
-	close(fd);
+	int ret = recv_sideband("fetch-pack", xd[0], out);
+	close(out);
 	return ret;
 }
 
@@ -613,6 +613,7 @@ static int get_pack(int xd[2], char **pack_lockfile)
 		 */
 		demux.proc = sideband_demux;
 		demux.data = xd;
+		demux.out = -1;
 		if (start_async(&demux))
 			die("fetch-pack: unable to fork off sideband"
 			    " demultiplexer");
diff --git a/convert.c b/convert.c
index 491e714..e70ee09 100644
--- a/convert.c
+++ b/convert.c
@@ -241,7 +241,7 @@ struct filter_params {
 	const char *cmd;
 };
 
-static int filter_buffer(int fd, void *data)
+static int filter_buffer(int in, int out, void *data)
 {
 	/*
 	 * Spawn cmd and feed the buffer contents through its stdin.
@@ -254,7 +254,7 @@ static int filter_buffer(int fd, void *data)
 	memset(&child_process, 0, sizeof(child_process));
 	child_process.argv = argv;
 	child_process.in = -1;
-	child_process.out = fd;
+	child_process.out = out;
 
 	if (start_command(&child_process))
 		return error("cannot fork to run external filter %s", params->cmd);
@@ -291,6 +291,7 @@ static int apply_filter(const char *path, const char *src, size_t len,
 	memset(&async, 0, sizeof(async));
 	async.proc = filter_buffer;
 	async.data = &params;
+	async.out = -1;
 	params.src = src;
 	params.size = len;
 	params.cmd = cmd;
diff --git a/remote-curl.c b/remote-curl.c
index 3edbf57..6bb3366 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -184,13 +184,13 @@ static struct discovery* discover_refs(const char *service)
 	return last;
 }
 
-static int write_discovery(int fd, void *data)
+static int write_discovery(int in, int out, void *data)
 {
 	struct discovery *heads = data;
 	int err = 0;
-	if (write_in_full(fd, heads->buf, heads->len) != heads->len)
+	if (write_in_full(out, heads->buf, heads->len) != heads->len)
 		err = 1;
-	close(fd);
+	close(out);
 	return err;
 }
 
@@ -202,6 +202,7 @@ static struct ref *parse_git_refs(struct discovery *heads)
 	memset(&async, 0, sizeof(async));
 	async.proc = write_discovery;
 	async.data = heads;
+	async.out = -1;
 
 	if (start_async(&async))
 		die("cannot start thread to parse advertised refs");
diff --git a/run-command.c b/run-command.c
index bfd2312..0d95340 100644
--- a/run-command.c
+++ b/run-command.c
@@ -327,17 +327,51 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const
 static unsigned __stdcall run_thread(void *data)
 {
 	struct async *async = data;
-	return async->proc(async->fd_for_proc, async->data);
+	return async->proc(async->proc_in, async->proc_out, async->data);
 }
 #endif
 
 int start_async(struct async *async)
 {
-	int pipe_out[2];
+	int need_in, need_out;
+	int fdin[2], fdout[2];
+	int proc_in, proc_out;
 
-	if (pipe(pipe_out) < 0)
-		return error("cannot create pipe: %s", strerror(errno));
-	async->out = pipe_out[0];
+	need_in = async->in < 0;
+	if (need_in) {
+		if (pipe(fdin) < 0) {
+			if (async->out > 0)
+				close(async->out);
+			return error("cannot create pipe: %s", strerror(errno));
+		}
+		async->in = fdin[1];
+	}
+
+	need_out = async->out < 0;
+	if (need_out) {
+		if (pipe(fdout) < 0) {
+			if (need_in)
+				close_pair(fdin);
+			else if (async->in)
+				close(async->in);
+			return error("cannot create pipe: %s", strerror(errno));
+		}
+		async->out = fdout[0];
+	}
+
+	if (need_in)
+		proc_in = fdin[0];
+	else if (async->in)
+		proc_in = async->in;
+	else
+		proc_in = -1;
+
+	if (need_out)
+		proc_out = fdout[1];
+	else if (async->out)
+		proc_out = async->out;
+	else
+		proc_out = -1;
 
 #ifndef WIN32
 	/* Flush stdio before fork() to avoid cloning buffers */
@@ -346,24 +380,47 @@ int start_async(struct async *async)
 	async->pid = fork();
 	if (async->pid < 0) {
 		error("fork (async) failed: %s", strerror(errno));
-		close_pair(pipe_out);
-		return -1;
+		goto error;
 	}
 	if (!async->pid) {
-		close(pipe_out[0]);
-		exit(!!async->proc(pipe_out[1], async->data));
+		if (need_in)
+			close(fdin[1]);
+		if (need_out)
+			close(fdout[0]);
+		exit(!!async->proc(proc_in, proc_out, async->data));
 	}
-	close(pipe_out[1]);
+
+	if (need_in)
+		close(fdin[0]);
+	else if (async->in)
+		close(async->in);
+
+	if (need_out)
+		close(fdout[1]);
+	else if (async->out)
+		close(async->out);
 #else
-	async->fd_for_proc = pipe_out[1];
+	async->proc_in = proc_in;
+	async->proc_out = proc_out;
 	async->tid = (HANDLE) _beginthreadex(NULL, 0, run_thread, async, 0, NULL);
 	if (!async->tid) {
 		error("cannot create thread: %s", strerror(errno));
-		close_pair(pipe_out);
-		return -1;
+		goto error;
 	}
 #endif
 	return 0;
+
+error:
+	if (need_in)
+		close_pair(fdin);
+	else if (async->in)
+		close(async->in);
+
+	if (need_out)
+		close_pair(fdout);
+	else if (async->out)
+		close(async->out);
+	return -1;
 }
 
 int finish_async(struct async *async)
diff --git a/run-command.h b/run-command.h
index a29171a..65ccb1c 100644
--- a/run-command.h
+++ b/run-command.h
@@ -64,17 +64,20 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const
  */
 struct async {
 	/*
-	 * proc writes to fd and closes it;
+	 * proc reads from in; closes it before return
+	 * proc writes to out; closes it before return
 	 * returns 0 on success, non-zero on failure
 	 */
-	int (*proc)(int fd, void *data);
+	int (*proc)(int in, int out, void *data);
 	void *data;
+	int in;		/* caller writes here and closes it */
 	int out;	/* caller reads from here and closes it */
 #ifndef WIN32
 	pid_t pid;
 #else
 	HANDLE tid;
-	int fd_for_proc;
+	int proc_in;
+	int proc_out;
 #endif
 };
 
diff --git a/upload-pack.c b/upload-pack.c
index df15181..dc464d7 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -105,12 +105,12 @@ static void show_edge(struct commit *commit)
 	fprintf(pack_pipe, "-%s\n", sha1_to_hex(commit->object.sha1));
 }
 
-static int do_rev_list(int fd, void *create_full_pack)
+static int do_rev_list(int in, int out, void *create_full_pack)
 {
 	int i;
 	struct rev_info revs;
 
-	pack_pipe = xfdopen(fd, "w");
+	pack_pipe = xfdopen(out, "w");
 	init_revisions(&revs, NULL);
 	revs.tag_objects = 1;
 	revs.tree_objects = 1;
@@ -162,8 +162,9 @@ static void create_pack_file(void)
 	int arg = 0;
 
 	if (shallow_nr) {
+		memset(&rev_list, 0, sizeof(rev_list));
 		rev_list.proc = do_rev_list;
-		rev_list.data = 0;
+		rev_list.out = -1;
 		if (start_async(&rev_list))
 			die("git upload-pack: unable to fork git-rev-list");
 		argv[arg++] = "pack-objects";
-- 
1.7.0.rc1.199.g9253ab

[PATCH v2 3/6] send-pack: demultiplex a sideband stream with status data

[Shawn O. Pearce]

If the server advertises side-band-64k capability, we request
it and pull the status report data out of side band #1, and let
side band #2 go to our stderr.  The latter channel be used by the
remote side to send our user messages.  This basically mirrors the
side-band-64k capability in upload-pack.

Servers may choose to use side band #2 to send error messages from
hook scripts that are meant for the push end user.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 builtin-send-pack.c |   66 ++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 50 insertions(+), 16 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 8fffdbf..2478e18 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -372,6 +372,14 @@ static void print_helper_status(struct ref *ref)
 	strbuf_release(&buf);
 }
 
+static int sideband_demux(int in, int out, void *data)
+{
+	int *fd = data;
+	int ret = recv_sideband("send-pack", fd[0], 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 +390,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 demux;
 
 	/* 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 +468,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 +501,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(&demux, 0, sizeof(demux));
+		demux.proc = sideband_demux;
+		demux.data = fd;
+		demux.out = -1;
+		if (start_async(&demux))
+			die("receive-pack: unable to fork off sideband demultiplexer");
+		in = demux.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(&demux);
 			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) {
+		if (finish_async(&demux)) {
+			error("error in sideband demultiplexer");
+			ret = -1;
+		}
+		close(demux.out);
+	}
+
 	if (ret < 0)
 		return ret;
 	for (ref = remote_refs; ref; ref = ref->next) {
-- 
1.7.0.rc1.199.g9253ab

[PATCH v2 4/6] receive-pack: Refactor how capabilities are shown to the client

[Shawn O. Pearce]

Moving capability advertisement into the packet_write call itself
makes it easier to add additional capabilities to the list, be
it optional by configuration, or always present in the protocol.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 builtin-receive-pack.c |   18 ++++++++----------
 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 78c0e69..325ec6e 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -31,7 +31,7 @@ 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 sent_capabilities;
 
 static enum deny_action parse_deny_action(const char *var, const char *value)
 {
@@ -105,19 +105,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 (sent_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",
+			     prefer_ofs_delta ? " ofs-delta" : "");
+	sent_capabilities = 1;
 	return 0;
 }
 
 static void write_head_info(void)
 {
 	for_each_ref(show_ref, NULL);
-	if (capabilities_to_send)
+	if (!sent_capabilities)
 		show_ref("capabilities^{}", null_sha1, 0, NULL);
 
 }
@@ -670,10 +672,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();
-- 
1.7.0.rc1.199.g9253ab

[PATCH v2 5/6] receive-pack: Wrap status reports inside side-band-64k

[Shawn O. Pearce]

If the client requests the side-band-64k protocol capability we
now wrap the status report data inside of packets sent to band #1.
This permits us to later send additional progress or informational
messages down band #2.

If side-band-64k was enabled, we always send a final flush packet
to let the client know we are done transmitting.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 builtin-receive-pack.c |   30 ++++++++++++++++++++++--------
 1 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 325ec6e..ff3f117 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,6 +28,7 @@ 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;
@@ -110,7 +112,7 @@ static int show_ref(const char *path, const unsigned char *sha1, int flag, void
 	else
 		packet_write(1, "%s %s%c%s%s\n",
 			     sha1_to_hex(sha1), path, 0,
-			     " report-status delete-refs",
+			     " report-status delete-refs side-band-64k",
 			     prefer_ofs_delta ? " ofs-delta" : "");
 	sent_capabilities = 1;
 	return 0;
@@ -466,6 +468,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);
@@ -565,17 +569,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)
@@ -705,5 +717,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;
 }
-- 
1.7.0.rc1.199.g9253ab

Re: [PATCH v2 5/6] receive-pack: Wrap status reports inside side-band-64k

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> If the client requests the side-band-64k protocol capability we
> now wrap the status report data inside of packets sent to band #1.
> This permits us to later send additional progress or informational
> messages down band #2.
>
> If side-band-64k was enabled, we always send a final flush packet
> to let the client know we are done transmitting.

Two questions.

 - Why does use_sideband, the variable with the same name as a boolean
   variable used by other parts of the system to decide whether we should
   or should not use the sideband communiocation, get a value other than 0
   or 1?  What is the benefit of using it to keep an actual value?  Does
   the benefit outweigh the confusion factor?

 - What happens if client wants only side-band, not 64k?  This is just
   theoretical and "we don't bother" is a perfectly acceptable answer.  I
   am just curious ;-).

Re: [PATCH v2 5/6] receive-pack: Wrap status reports inside side-band-64k

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > If the client requests the side-band-64k protocol capability we
> > now wrap the status report data inside of packets sent to band #1.
> > This permits us to later send additional progress or informational
> > messages down band #2.
> >
> > If side-band-64k was enabled, we always send a final flush packet
> > to let the client know we are done transmitting.
> 
> Two questions.
> 
>  - Why does use_sideband, the variable with the same name as a boolean
>    variable used by other parts of the system to decide whether we should
>    or should not use the sideband communiocation, get a value other than 0
>    or 1?  What is the benefit of using it to keep an actual value?  Does
>    the benefit outweigh the confusion factor?

I was following the existing convention of use_sideband is
maximum-packet-size in server code, and boolean in client code.

Well, I do diverage a bit in the client, in the client side
use_sideband = 2 in builtin-fetch-pack.c for side-band-64k and
use_sideband = 1 in builtin-send-pack.c for the same thing.
 
>  - What happens if client wants only side-band, not 64k?  This is just
>    theoretical and "we don't bother" is a perfectly acceptable answer.  I
>    am just curious ;-).

Why bother?  What client doesn't understand side-band-64k but would
understand this?

-- 
Shawn.

Re: [PATCH v2 5/6] receive-pack: Wrap status reports inside side-band-64k

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> I was following the existing convention of use_sideband is
> maximum-packet-size in server code, and boolean in client code.

Ahh, I forgot about that -- we did it when we introduced different packet
size limit.

Now it all makes sense.  Thanks.

[PATCH v2 6/6] receive-pack: Send hook output over side band #2

[Shawn O. Pearce]

If the client requests to enable side-band-64k capability we can
safely send any hook stdout or stderr data down side band #2,
so the client can present it to the user.

If side-band-64k isn't enabled, hooks continue to inherit stderr
from the parent receive-pack process.

When the side band channel is being used the push client will wind up
prefixing all server messages with "remote: ", just like fetch does,
so our test vector has to be updated with the new expected output.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 builtin-receive-pack.c  |   65 ++++++++++++++++++++++++++++++++++++++++++----
 t/t5401-update-hooks.sh |   22 ++++++++--------
 2 files changed, 70 insertions(+), 17 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index ff3f117..da1c26b 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -139,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, int out, 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 muxer;
 	const char *argv[2];
 	int have_input = 0, code;
 
@@ -163,9 +177,23 @@ static int run_receive_hook(const char *hook_name)
 	proc.in = -1;
 	proc.stdout_to_stderr = 1;
 
+	if (use_sideband) {
+		memset(&muxer, 0, sizeof(muxer));
+		muxer.proc = copy_to_sideband;
+		muxer.in = -1;
+		code = start_async(&muxer);
+		if (code)
+			return code;
+		proc.err = muxer.in;
+	}
+
 	code = start_command(&proc);
-	if (code)
+	if (code) {
+		if (use_sideband)
+			finish_async(&muxer);
 		return code;
+	}
+
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (!cmd->error_string) {
 			size_t n = snprintf(buf, sizeof(buf), "%s %s %s\n",
@@ -177,6 +205,8 @@ static int run_receive_hook(const char *hook_name)
 		}
 	}
 	close(proc.in);
+	if (use_sideband)
+		finish_async(&muxer);
 	return finish_command(&proc);
 }
 
@@ -184,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;
@@ -194,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, -1, NULL);
+	return finish_command(&proc);
 }
 
 static int is_ref_checked_out(const char *ref)
@@ -384,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)
@@ -407,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, -1, NULL);
+		finish_command(&proc);
+	}
 }
 
 static void execute_commands(const char *unpacker_error)
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

Re: [PATCH v2 6/6] receive-pack: Send hook output over side band #2

[Larry D'Anna]

This breaks t5401.  See <7v4olqlva7.fsf@alter.siamese.dyndns.org> in another thread.

     --larry

Re: [PATCH v2 6/6] receive-pack: Send hook output over side band #2

[Shawn O. Pearce]

Larry D'Anna <larry@elder-gods.org> wrote:
> 
> This breaks t5401.  See <7v4olqlva7.fsf@alter.siamese.dyndns.org> in another thread.

No.  Your patch causes t5401 to break.  If you apply this series on
top of maint, its fine.  If you merge this series into master, and
correctly fix the Win32 merge conflict in run-command.c, its fine.

/me goes to look at your series, to see if I can figure out what
broke along the way.

-- 
Shawn.

Re: [PATCH v2 6/6] receive-pack: Send hook output over side band #2

[Larry D'Anna]

* Shawn O. Pearce (spearce@spearce.org) [100209 12:20]:
> Larry D'Anna <larry@elder-gods.org> wrote:
> > 
> > This breaks t5401.  See <7v4olqlva7.fsf@alter.siamese.dyndns.org> in another thread.
> 
> No.  Your patch causes t5401 to break.  If you apply this series on
> top of maint, its fine.  If you merge this series into master, and
> correctly fix the Win32 merge conflict in run-command.c, its fine.

Well, the version of this commit that's in pu is defiantly failing t5401 on my
machine.  Just tested it again with a clean build.  My series isn't an ancestor
of it either.  Did you notice the failure is intermittent?  You have to run it a
few times before it fails.

       --larry

Re: [PATCH v2 6/6] receive-pack: Send hook output over side band #2

[Larry D'Anna]

* Larry D'Anna (larry@elder-gods.org) [100209 12:33]:
> * Shawn O. Pearce (spearce@spearce.org) [100209 12:20]:
> > Larry D'Anna <larry@elder-gods.org> wrote:
> > > 
> > > This breaks t5401.  See <7v4olqlva7.fsf@alter.siamese.dyndns.org> in another thread.
> > 
> > No.  Your patch causes t5401 to break.  If you apply this series on
> > top of maint, its fine.  If you merge this series into master, and
> > correctly fix the Win32 merge conflict in run-command.c, its fine.
> 
> Well, the version of this commit that's in pu is defiantly failing t5401 on my
> machine.  Just tested it again with a clean build.  My series isn't an ancestor
> of it either.  Did you notice the failure is intermittent?  You have to run it a
> few times before it fails.

I also just tried applying the series on top of maint myself, same result.


  --larry