{"thread":{"id":"22529","subject":"[PATCH] push: Use sideband channel for hook messages","startedAt":"2010-02-05T03:37:48Z","lastAt":"2010-02-05T17:29:16Z","messageCount":8,"participants":["Shawn O. Pearce","Junio C Hamano","Johannes Sixt","Ilari Liusvaara","Erik Faye-Lund"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"133664","messageId":"20100205033748.GA19255@spearce.org","threadId":"22529","inReplyTo":null,"subject":"[PATCH] push: Use sideband channel for hook messages","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-05T03:37:48Z","receivedAt":"2010-02-05T03:37:48Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Rather than sending hook messages over stderr, and losing them\nentirely on git:// and smart HTTP transports, receive-pack now\nputs them onto a multiplexed sideband channel if the send-pack\nclient asks for the side-band-64k capablity.  This ensures that\nhooks from the server can report their detailed error messages,\nif any, no matter what git-aware transport is being used.\n\nWhen the side band channel is being used the push client will wind up\nprefixing all server messages with \"remote: \", just like fetch does.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n\n We should have done this back when we added smart HTTP, because\n otherwise the server can't send its hook messages back to the\n client.  I'd argue that is a bug, and so this should go in maint,\n but I can also see how some might consider this a new feature... so\n whatever.  :-)\n\n builtin-receive-pack.c  |  111 +++++++++++++++++++++++++++++++++++++----------\n builtin-send-pack.c     |   67 +++++++++++++++++++++-------\n run-command.c           |   24 ++++++++--\n run-command.h           |    1 +\n t/t5401-update-hooks.sh |   22 +++++-----\n 5 files changed, 170 insertions(+), 55 deletions(-)\n\ndiff --git a/builtin-receive-pack.c b/builtin-receive-pack.c\nindex 78c0e69..e7bdd71 100644\n--- a/builtin-receive-pack.c\n+++ b/builtin-receive-pack.c\n@@ -2,6 +2,7 @@\n #include \"pack.h\"\n #include \"refs.h\"\n #include \"pkt-line.h\"\n+#include \"sideband.h\"\n #include \"run-command.h\"\n #include \"exec_cmd.h\"\n #include \"commit.h\"\n@@ -27,11 +28,12 @@ static int receive_unpack_limit = -1;\n static int transfer_unpack_limit = -1;\n static int unpack_limit = 100;\n static int report_status;\n+static int use_sideband;\n static int prefer_ofs_delta = 1;\n static int auto_update_server_info;\n static int auto_gc = 1;\n static const char *head_name;\n-static char *capabilities_to_send;\n+static int send_capabilities = 1;\n \n static enum deny_action parse_deny_action(const char *var, const char *value)\n {\n@@ -105,19 +107,21 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \n static int show_ref(const char *path, const unsigned char *sha1, int flag, void *cb_data)\n {\n-\tif (!capabilities_to_send)\n+\tif (!send_capabilities)\n \t\tpacket_write(1, \"%s %s\\n\", sha1_to_hex(sha1), path);\n \telse\n-\t\tpacket_write(1, \"%s %s%c%s\\n\",\n-\t\t\t     sha1_to_hex(sha1), path, 0, capabilities_to_send);\n-\tcapabilities_to_send = NULL;\n+\t\tpacket_write(1, \"%s %s%c%s%s\\n\",\n+\t\t\t     sha1_to_hex(sha1), path, 0,\n+\t\t\t     \" report-status delete-refs side-band-64k\",\n+\t\t\t     prefer_ofs_delta ? \" ofs-delta\" : \"\");\n+\tsend_capabilities = 0;\n \treturn 0;\n }\n \n static void write_head_info(void)\n {\n \tfor_each_ref(show_ref, NULL);\n-\tif (capabilities_to_send)\n+\tif (send_capabilities)\n \t\tshow_ref(\"capabilities^{}\", null_sha1, 0, NULL);\n \n }\n@@ -135,11 +139,25 @@ static struct command *commands;\n static const char pre_receive_hook[] = \"hooks/pre-receive\";\n static const char post_receive_hook[] = \"hooks/post-receive\";\n \n+static int copy_to_sideband(int in, void *arg)\n+{\n+\tchar data[128];\n+\twhile (1) {\n+\t\tssize_t sz = xread(in, data, sizeof(data));\n+\t\tif (sz <= 0)\n+\t\t\tbreak;\n+\t\tsend_sideband(1, 2, data, sz, use_sideband);\n+\t}\n+\tclose(in);\n+\treturn 0;\n+}\n+\n static int run_receive_hook(const char *hook_name)\n {\n \tstatic char buf[sizeof(commands->old_sha1) * 2 + PATH_MAX + 4];\n \tstruct command *cmd;\n \tstruct child_process proc;\n+\tstruct async sideband_mux;\n \tconst char *argv[2];\n \tint have_input = 0, code;\n \n@@ -159,9 +177,23 @@ static int run_receive_hook(const char *hook_name)\n \tproc.in = -1;\n \tproc.stdout_to_stderr = 1;\n \n+\tif (use_sideband) {\n+\t\tmemset(&sideband_mux, 0, sizeof(sideband_mux));\n+\t\tsideband_mux.proc = copy_to_sideband;\n+\t\tsideband_mux.is_reader = 1;\n+\t\tcode = start_async(&sideband_mux);\n+\t\tif (code)\n+\t\t\treturn code;\n+\t\tproc.err = sideband_mux.out;\n+\t}\n+\n \tcode = start_command(&proc);\n-\tif (code)\n+\tif (code) {\n+\t\tif (use_sideband)\n+\t\t\tfinish_async(&sideband_mux);\n \t\treturn code;\n+\t}\n+\n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\tif (!cmd->error_string) {\n \t\t\tsize_t n = snprintf(buf, sizeof(buf), \"%s %s %s\\n\",\n@@ -173,6 +205,8 @@ static int run_receive_hook(const char *hook_name)\n \t\t}\n \t}\n \tclose(proc.in);\n+\tif (use_sideband)\n+\t\tfinish_async(&sideband_mux);\n \treturn finish_command(&proc);\n }\n \n@@ -180,6 +214,8 @@ static int run_update_hook(struct command *cmd)\n {\n \tstatic const char update_hook[] = \"hooks/update\";\n \tconst char *argv[5];\n+\tstruct child_process proc;\n+\tint code;\n \n \tif (access(update_hook, X_OK) < 0)\n \t\treturn 0;\n@@ -190,8 +226,18 @@ static int run_update_hook(struct command *cmd)\n \targv[3] = sha1_to_hex(cmd->new_sha1);\n \targv[4] = NULL;\n \n-\treturn run_command_v_opt(argv, RUN_COMMAND_NO_STDIN |\n-\t\t\t\t\tRUN_COMMAND_STDOUT_TO_STDERR);\n+\tmemset(&proc, 0, sizeof(proc));\n+\tproc.no_stdin = 1;\n+\tproc.stdout_to_stderr = 1;\n+\tproc.err = use_sideband ? -1 : 0;\n+\tproc.argv = argv;\n+\n+\tcode = start_command(&proc);\n+\tif (code)\n+\t\treturn code;\n+\tif (use_sideband)\n+\t\tcopy_to_sideband(proc.err, NULL);\n+\treturn finish_command(&proc);\n }\n \n static int is_ref_checked_out(const char *ref)\n@@ -380,8 +426,9 @@ static char update_post_hook[] = \"hooks/post-update\";\n static void run_update_post_hook(struct command *cmd)\n {\n \tstruct command *cmd_p;\n-\tint argc, status;\n+\tint argc;\n \tconst char **argv;\n+\tstruct child_process proc;\n \n \tfor (argc = 0, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {\n \t\tif (cmd_p->error_string)\n@@ -403,8 +450,18 @@ static void run_update_post_hook(struct command *cmd)\n \t\targc++;\n \t}\n \targv[argc] = NULL;\n-\tstatus = run_command_v_opt(argv, RUN_COMMAND_NO_STDIN\n-\t\t\t| RUN_COMMAND_STDOUT_TO_STDERR);\n+\n+\tmemset(&proc, 0, sizeof(proc));\n+\tproc.no_stdin = 1;\n+\tproc.stdout_to_stderr = 1;\n+\tproc.err = use_sideband ? -1 : 0;\n+\tproc.argv = argv;\n+\n+\tif (!start_command(&proc)) {\n+\t\tif (use_sideband)\n+\t\t\tcopy_to_sideband(proc.err, NULL);\n+\t\tfinish_command(&proc);\n+\t}\n }\n \n static void execute_commands(const char *unpacker_error)\n@@ -464,6 +521,8 @@ static void read_head_info(void)\n \t\tif (reflen + 82 < len) {\n \t\t\tif (strstr(refname + reflen + 1, \"report-status\"))\n \t\t\t\treport_status = 1;\n+\t\t\tif (strstr(refname + reflen + 1, \"side-band-64k\"))\n+\t\t\t\tuse_sideband = LARGE_PACKET_MAX;\n \t\t}\n \t\tcmd = xmalloc(sizeof(struct command) + len - 80);\n \t\thashcpy(cmd->old_sha1, old_sha1);\n@@ -563,17 +622,25 @@ static const char *unpack(void)\n static void report(const char *unpack_status)\n {\n \tstruct command *cmd;\n-\tpacket_write(1, \"unpack %s\\n\",\n-\t\t     unpack_status ? unpack_status : \"ok\");\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tpacket_buf_write(&buf, \"unpack %s\\n\",\n+\t\t\t unpack_status ? unpack_status : \"ok\");\n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\tif (!cmd->error_string)\n-\t\t\tpacket_write(1, \"ok %s\\n\",\n-\t\t\t\t     cmd->ref_name);\n+\t\t\tpacket_buf_write(&buf, \"ok %s\\n\",\n+\t\t\t\t\t cmd->ref_name);\n \t\telse\n-\t\t\tpacket_write(1, \"ng %s %s\\n\",\n-\t\t\t\t     cmd->ref_name, cmd->error_string);\n+\t\t\tpacket_buf_write(&buf, \"ng %s %s\\n\",\n+\t\t\t\t\t cmd->ref_name, cmd->error_string);\n \t}\n-\tpacket_flush(1);\n+\tpacket_buf_flush(&buf);\n+\n+\tif (use_sideband)\n+\t\tsend_sideband(1, 1, buf.buf, buf.len, use_sideband);\n+\telse\n+\t\tsafe_write(1, buf.buf, buf.len);\n+\tstrbuf_release(&buf);\n }\n \n static int delete_only(struct command *cmd)\n@@ -670,10 +737,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \telse if (0 <= receive_unpack_limit)\n \t\tunpack_limit = receive_unpack_limit;\n \n-\tcapabilities_to_send = (prefer_ofs_delta) ?\n-\t\t\" report-status delete-refs ofs-delta \" :\n-\t\t\" report-status delete-refs \";\n-\n \tif (advertise_refs || !stateless_rpc) {\n \t\tadd_alternate_refs();\n \t\twrite_head_info();\n@@ -707,5 +770,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\tif (auto_update_server_info)\n \t\t\tupdate_server_info(0);\n \t}\n+\tif (use_sideband)\n+\t\tpacket_flush(1);\n \treturn 0;\n }\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 8fffdbf..4bbc39f 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -372,6 +372,15 @@ static void print_helper_status(struct ref *ref)\n \tstrbuf_release(&buf);\n }\n \n+static int sideband_decoder_callback(int out, void *data)\n+{\n+\tint *fd = data;\n+\tint in = fd[0];\n+\tint ret = recv_sideband(\"send-pack\", in, out);\n+\tclose(out);\n+\treturn ret;\n+}\n+\n int send_pack(struct send_pack_args *args,\n \t      int fd[], struct child_process *conn,\n \t      struct ref *remote_refs,\n@@ -382,18 +391,22 @@ int send_pack(struct send_pack_args *args,\n \tstruct strbuf req_buf = STRBUF_INIT;\n \tstruct ref *ref;\n \tint new_refs;\n-\tint ask_for_status_report = 0;\n \tint allow_deleting_refs = 0;\n-\tint expect_status_report = 0;\n+\tint status_report = 0;\n+\tint use_sideband = 0;\n+\tunsigned cmds_sent = 0;\n \tint ret;\n+\tstruct async sideband_decoder;\n \n \t/* Does the other end support the reporting? */\n \tif (server_supports(\"report-status\"))\n-\t\task_for_status_report = 1;\n+\t\tstatus_report = 1;\n \tif (server_supports(\"delete-refs\"))\n \t\tallow_deleting_refs = 1;\n \tif (server_supports(\"ofs-delta\"))\n \t\targs->use_ofs_delta = 1;\n+\tif (server_supports(\"side-band-64k\"))\n+\t\tuse_sideband = 1;\n \n \tif (!remote_refs) {\n \t\tfprintf(stderr, \"No refs in common and none specified; doing nothing.\\n\"\n@@ -456,28 +469,30 @@ int send_pack(struct send_pack_args *args,\n \t\tif (!ref->deletion)\n \t\t\tnew_refs++;\n \n-\t\tif (!args->dry_run) {\n+\t\tif (args->dry_run) {\n+\t\t\tref->status = REF_STATUS_OK;\n+\t\t} else {\n \t\t\tchar *old_hex = sha1_to_hex(ref->old_sha1);\n \t\t\tchar *new_hex = sha1_to_hex(ref->new_sha1);\n \n-\t\t\tif (ask_for_status_report) {\n-\t\t\t\tpacket_buf_write(&req_buf, \"%s %s %s%c%s\",\n+\t\t\tif (!cmds_sent && (status_report || use_sideband)) {\n+\t\t\t\tpacket_buf_write(&req_buf, \"%s %s %s%c%s%s\",\n \t\t\t\t\told_hex, new_hex, ref->name, 0,\n-\t\t\t\t\t\"report-status\");\n-\t\t\t\task_for_status_report = 0;\n-\t\t\t\texpect_status_report = 1;\n+\t\t\t\t\tstatus_report ? \" report-status\" : \"\",\n+\t\t\t\t\tuse_sideband ? \" side-band-64k\" : \"\");\n \t\t\t}\n \t\t\telse\n \t\t\t\tpacket_buf_write(&req_buf, \"%s %s %s\",\n \t\t\t\t\told_hex, new_hex, ref->name);\n+\t\t\tref->status = status_report ?\n+\t\t\t\tREF_STATUS_EXPECTING_REPORT :\n+\t\t\t\tREF_STATUS_OK;\n+\t\t\tcmds_sent++;\n \t\t}\n-\t\tref->status = expect_status_report ?\n-\t\t\tREF_STATUS_EXPECTING_REPORT :\n-\t\t\tREF_STATUS_OK;\n \t}\n \n \tif (args->stateless_rpc) {\n-\t\tif (!args->dry_run) {\n+\t\tif (!args->dry_run && cmds_sent) {\n \t\t\tpacket_buf_flush(&req_buf);\n \t\t\tsend_sideband(out, -1, req_buf.buf, req_buf.len, LARGE_PACKET_MAX);\n \t\t}\n@@ -487,23 +502,43 @@ int send_pack(struct send_pack_args *args,\n \t}\n \tstrbuf_release(&req_buf);\n \n-\tif (new_refs && !args->dry_run) {\n+\tif (use_sideband && cmds_sent) {\n+\t\tmemset(&sideband_decoder, 0, sizeof(sideband_decoder));\n+\t\tsideband_decoder.proc = sideband_decoder_callback;\n+\t\tsideband_decoder.data = fd;\n+\t\tif (start_async(&sideband_decoder))\n+\t\t\tdie(\"cannot start sideband decoder\");\n+\t\tin = sideband_decoder.out;\n+\t}\n+\n+\tif (new_refs && cmds_sent) {\n \t\tif (pack_objects(out, remote_refs, extra_have, args) < 0) {\n \t\t\tfor (ref = remote_refs; ref; ref = ref->next)\n \t\t\t\tref->status = REF_STATUS_NONE;\n+\t\t\tif (use_sideband)\n+\t\t\t\tfinish_async(&sideband_decoder);\n \t\t\treturn -1;\n \t\t}\n \t}\n-\tif (args->stateless_rpc && !args->dry_run)\n+\tif (args->stateless_rpc && cmds_sent)\n \t\tpacket_flush(out);\n \n-\tif (expect_status_report)\n+\tif (status_report && cmds_sent)\n \t\tret = receive_status(in, remote_refs);\n \telse\n \t\tret = 0;\n \tif (args->stateless_rpc)\n \t\tpacket_flush(out);\n \n+\tif (use_sideband && cmds_sent) {\n+\t\tint err = finish_async(&sideband_decoder);\n+\t\tif (err) {\n+\t\t\terror(\"sideband decoder failed %d\", err);\n+\t\t\tret = -1;\n+\t\t}\n+\t\tclose(sideband_decoder.out);\n+\t}\n+\n \tif (ret < 0)\n \t\treturn ret;\n \tfor (ref = remote_refs; ref; ref = ref->next) {\ndiff --git a/run-command.c b/run-command.c\nindex cf2d8f7..7d1fd88 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -94,6 +94,9 @@ fail_pipe:\n \t\telse if (need_err) {\n \t\t\tdup2(fderr[1], 2);\n \t\t\tclose_pair(fderr);\n+\t\t} else if (cmd->err > 1) {\n+\t\t\tdup2(cmd->err, 2);\n+\t\t\tclose(cmd->err);\n \t\t}\n \n \t\tif (cmd->no_stdout)\n@@ -228,6 +231,8 @@ fail_pipe:\n \n \tif (need_err)\n \t\tclose(fderr[1]);\n+\telse if (cmd->err)\n+\t\tclose(cmd->err);\n \n \treturn 0;\n }\n@@ -326,10 +331,19 @@ static unsigned __stdcall run_thread(void *data)\n int start_async(struct async *async)\n {\n \tint pipe_out[2];\n+\tint proc_fd, call_fd;\n \n \tif (pipe(pipe_out) < 0)\n \t\treturn error(\"cannot create pipe: %s\", strerror(errno));\n-\tasync->out = pipe_out[0];\n+\n+\tif (async->is_reader) {\n+\t\tproc_fd = pipe_out[0];\n+\t\tcall_fd = pipe_out[1];\n+\t} else {\n+\t\tcall_fd = pipe_out[0];\n+\t\tproc_fd = pipe_out[1];\n+\t}\n+\tasync->out = call_fd;\n \n #ifndef WIN32\n \t/* Flush stdio before fork() to avoid cloning buffers */\n@@ -342,12 +356,12 @@ int start_async(struct async *async)\n \t\treturn -1;\n \t}\n \tif (!async->pid) {\n-\t\tclose(pipe_out[0]);\n-\t\texit(!!async->proc(pipe_out[1], async->data));\n+\t\tclose(call_fd);\n+\t\texit(!!async->proc(proc_fd, async->data));\n \t}\n-\tclose(pipe_out[1]);\n+\tclose(proc_fd);\n #else\n-\tasync->fd_for_proc = pipe_out[1];\n+\tasync->fd_for_proc = proc_fd;\n \tasync->tid = (HANDLE) _beginthreadex(NULL, 0, run_thread, async, 0, NULL);\n \tif (!async->tid) {\n \t\terror(\"cannot create thread: %s\", strerror(errno));\ndiff --git a/run-command.h b/run-command.h\nindex fb34209..8aed83f 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -70,6 +70,7 @@ struct async {\n \tint (*proc)(int fd, void *data);\n \tvoid *data;\n \tint out;\t/* caller reads from here and closes it */\n+\tunsigned is_reader:1;\n #ifndef WIN32\n \tpid_t pid;\n #else\ndiff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh\nindex 64f66c9..c3cf397 100755\n--- a/t/t5401-update-hooks.sh\n+++ b/t/t5401-update-hooks.sh\n@@ -118,19 +118,19 @@ test_expect_success 'send-pack produced no output' '\n '\n \n cat <<EOF >expect\n-STDOUT pre-receive\n-STDERR pre-receive\n-STDOUT update refs/heads/master\n-STDERR update refs/heads/master\n-STDOUT update refs/heads/tofail\n-STDERR update refs/heads/tofail\n-STDOUT post-receive\n-STDERR post-receive\n-STDOUT post-update\n-STDERR post-update\n+remote: STDOUT pre-receive\n+remote: STDERR pre-receive\n+remote: STDOUT update refs/heads/master\n+remote: STDERR update refs/heads/master\n+remote: STDOUT update refs/heads/tofail\n+remote: STDERR update refs/heads/tofail\n+remote: STDOUT post-receive\n+remote: STDERR post-receive\n+remote: STDOUT post-update\n+remote: STDERR post-update\n EOF\n test_expect_success 'send-pack stderr contains hook messages' '\n-\tgrep ^STD send.err >actual &&\n+\tgrep ^remote: send.err | sed \"s/ *\\$//\" >actual &&\n \ttest_cmp - actual <expect\n '\n \n-- \n1.7.0.rc1.199.g9253ab\n\n-- \nShawn.\n"},{"id":"133669","messageId":"7v3a1gmdo3.fsf@alter.siamese.dyndns.org","threadId":"22529","inReplyTo":"20100205033748.GA19255@spearce.org","subject":"Re: [PATCH] push: Use sideband channel for hook messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-05T05:53:16Z","receivedAt":"2010-02-05T05:53:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> Rather than sending hook messages over stderr, and losing them\n> entirely on git:// and smart HTTP transports, receive-pack now\n> puts them onto a multiplexed sideband channel if the send-pack\n> client asks for the side-band-64k capablity.  This ensures that\n> hooks from the server can report their detailed error messages,\n> if any, no matter what git-aware transport is being used.\n>\n> When the side band channel is being used the push client will wind up\n> prefixing all server messages with \"remote: \", just like fetch does.\n>\n> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>\n\nThis feels to me a topic that has multiple unrelated changes mixed\ntogether, making it unnecessarily confusing to review.\n\n - capabilities_to_send -> send_capabilities;\n\n - use of sideband in communication from send-pack to receive-pack;\n\n - adding communication in the opposite direction to async infrastructure;\n\n - use_sideband that is a boolean but is also used to hold\n   LARGE_PACKET_MAX (on the receiving end).\n"},{"id":"133679","messageId":"4B6C07E3.5030705@viscovery.net","threadId":"22529","inReplyTo":"20100205033748.GA19255@spearce.org","subject":"Re: [PATCH] push: Use sideband channel for hook messages","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-02-05T11:58:27Z","receivedAt":"2010-02-05T11:58:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Shawn O. Pearce schrieb:\n> Rather than sending hook messages over stderr, and losing them\n> entirely on git:// and smart HTTP transports,\n\nI don't think \"losing them entirely\" is true for the git:// protocol:\ngit-daemon writes receive-pack's stderr to the syslog.\n\nThe question is whether hook errors are intended for the remote side or\nfor the repository owner. Generally, I'd say for the latter. But since\nyour patch is about pushing, a repository owner must already trust the\nremote side, and then it can be argued that in this case errors can be\nsent to the remote.\n\n> diff --git a/run-command.c b/run-command.c\n> index cf2d8f7..7d1fd88 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -94,6 +94,9 @@ fail_pipe:\n>  \t\telse if (need_err) {\n>  \t\t\tdup2(fderr[1], 2);\n>  \t\t\tclose_pair(fderr);\n> +\t\t} else if (cmd->err > 1) {\n> +\t\t\tdup2(cmd->err, 2);\n> +\t\t\tclose(cmd->err);\n>  \t\t}\n>  \n>  \t\tif (cmd->no_stdout)\n> @@ -228,6 +231,8 @@ fail_pipe:\n>  \n>  \tif (need_err)\n>  \t\tclose(fderr[1]);\n> +\telse if (cmd->err)\n> +\t\tclose(cmd->err);\n>  \n>  \treturn 0;\n>  }\n\nThis requires similar adjustments in the Windows part.\n\nDocumentation/technical/api-runcommand.txt should be an update, too.\n\n> @@ -326,10 +331,19 @@ static unsigned __stdcall run_thread(void *data)\n>  int start_async(struct async *async)\n>  {\n>  \tint pipe_out[2];\n> +\tint proc_fd, call_fd;\n>  \n>  \tif (pipe(pipe_out) < 0)\n>  \t\treturn error(\"cannot create pipe: %s\", strerror(errno));\n> -\tasync->out = pipe_out[0];\n> +\n> +\tif (async->is_reader) {\n> +\t\tproc_fd = pipe_out[0];\n> +\t\tcall_fd = pipe_out[1];\n> +\t} else {\n> +\t\tcall_fd = pipe_out[0];\n> +\t\tproc_fd = pipe_out[1];\n> +\t}\n> +\tasync->out = call_fd;\n\nI don't particularly like this approach because it restricts the async\nprocedures to a one-way communication.\n\nWhat would you think about passing both channels to the async callback,\nand the communicating parties must agree on which channel they communicate\nby closing the unused one? It would require slight changes to all current\nasync users, though. (It also requires in the threaded case that we pass\ndup()s of the pipe channels.)\n\n-- Hannes\n"},{"id":"133680","messageId":"20100205120758.GA22943@Knoppix","threadId":"22529","inReplyTo":"20100205033748.GA19255@spearce.org","subject":"Re: [PATCH] push: Use sideband channel for hook messages","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-02-05T12:07:58Z","receivedAt":"2010-02-05T12:07:58Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Thu, Feb 04, 2010 at 07:37:48PM -0800, Shawn O. Pearce wrote:\n> Rather than sending hook messages over stderr, and losing them\n> entirely on git:// and smart HTTP transports, receive-pack now\n> puts them onto a multiplexed sideband channel if the send-pack\n> client asks for the side-band-64k capablity.  This ensures that\n> hooks from the server can report their detailed error messages,\n> if any, no matter what git-aware transport is being used.\n> \n> When the side band channel is being used the push client will wind up\n> prefixing all server messages with \"remote: \", just like fetch does.\n\nI tested this a bit with git-remote-gits / git-daemon2. Seems to\nwork.\n\n-Ilari\n"},{"id":"133685","messageId":"20100205153252.GC19255@spearce.org","threadId":"22529","inReplyTo":"4B6C07E3.5030705@viscovery.net","subject":"Re: [PATCH] push: Use sideband channel for hook messages","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-05T15:32:52Z","receivedAt":"2010-02-05T15:32:52Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Shawn O. Pearce schrieb:\n> > Rather than sending hook messages over stderr, and losing them\n> > entirely on git:// and smart HTTP transports,\n> \n> I don't think \"losing them entirely\" is true for the git:// protocol:\n> git-daemon writes receive-pack's stderr to the syslog.\n> \n> The question is whether hook errors are intended for the remote side or\n> for the repository owner. Generally, I'd say for the latter. But since\n> your patch is about pushing, a repository owner must already trust the\n> remote side, and then it can be argued that in this case errors can be\n> sent to the remote.\n\nI think everyone expects hook errors on the client.\n\nMost people push over authenticated SSH, where hook messages come\nback on stderr.  For these folks, if they wanted something in syslog,\nthey'd send it there directly from the hook.\n\nI doubt there are many anonymous pushes allowed over git://.  But,\nyea, sure, I could see someone setting up their server with an\nupdate hook to only permit pushes to the refs/heads/mob branch and\nlogging access failures to syslog by echoing to stderr.  I think\nthese people are in the vast minority, and might not even want this\nbehavior by default.  Maybe I'm just a jerk, but if they want their\nhooks to continue to echo to syslog rather than to the client,\nthey can build a patch on top of this to git daemon which passes\na flag down into receive-pack to disable the side band channel.\n\n \n> > diff --git a/run-command.c b/run-command.c\n> > index cf2d8f7..7d1fd88 100644\n> > @@ -228,6 +231,8 @@ fail_pipe:\n> >  \n> >  \tif (need_err)\n> >  \t\tclose(fderr[1]);\n> > +\telse if (cmd->err)\n> > +\t\tclose(cmd->err);\n> \n> This requires similar adjustments in the Windows part.\n> \n> Documentation/technical/api-runcommand.txt should be an update, too.\n\nOuch, good catch.  Will fix.\n\n \n> > @@ -326,10 +331,19 @@ static unsigned __stdcall run_thread(void *data)\n> >  int start_async(struct async *async)\n> >  {\n> >  \tint pipe_out[2];\n> > +\tint proc_fd, call_fd;\n> >  \n> >  \tif (pipe(pipe_out) < 0)\n> >  \t\treturn error(\"cannot create pipe: %s\", strerror(errno));\n> > -\tasync->out = pipe_out[0];\n> > +\n> > +\tif (async->is_reader) {\n> > +\t\tproc_fd = pipe_out[0];\n> > +\t\tcall_fd = pipe_out[1];\n> > +\t} else {\n> > +\t\tcall_fd = pipe_out[0];\n> > +\t\tproc_fd = pipe_out[1];\n> > +\t}\n> > +\tasync->out = call_fd;\n> \n> I don't particularly like this approach because it restricts the async\n> procedures to a one-way communication.\n\nWell, its always been one way.  With the async function writing to the\nmain application consumer.\n \n> What would you think about passing both channels to the async callback,\n> and the communicating parties must agree on which channel they communicate\n> by closing the unused one? It would require slight changes to all current\n> async users, though. (It also requires in the threaded case that we pass\n> dup()s of the pipe channels.)\n\nYup, I could do that.  I feel like it might be over-engineering the\nsolution a bit.  But I'll respin the patch by splitting it apart,\nand doing a bidirectional async here, since you asked nicely.\n\n-- \nShawn.\n"},{"id":"133687","messageId":"4B6C3E9C.7040009@viscovery.net","threadId":"22529","inReplyTo":"20100205153252.GC19255@spearce.org","subject":"Re: [PATCH] push: Use sideband channel for hook messages","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-02-05T15:51:56Z","receivedAt":"2010-02-05T15:51:56Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Shawn O. Pearce schrieb:\n> Johannes Sixt <j.sixt@viscovery.net> wrote:\n>> What would you think about passing both channels to the async callback,\n>> and the communicating parties must agree on which channel they communicate\n>> by closing the unused one? It would require slight changes to all current\n>> async users, though. (It also requires in the threaded case that we pass\n>> dup()s of the pipe channels.)\n> \n> Yup, I could do that.  I feel like it might be over-engineering the\n> solution a bit.  But I'll respin the patch by splitting it apart,\n> and doing a bidirectional async here, since you asked nicely.\n\nI do agree about the over-engineering aspect. I mentioned it because in\none patch in the past Erik Faye-Lund also extended the async\ninfrastructure for bidirectional communication to use it in git-daemon\n(Windows port). Meanwhile, he's abandoned this approach because there were\nunsurmountable obstacles elsewhere; so if you introduce bidi now, it would\nnot immediately buy us anything.\n\nIt's your draw.\n\n-- Hannes\n"},{"id":"133689","messageId":"40aa078e1002050814m1edb90f8lf3d8d138fa65c332@mail.gmail.com","threadId":"22529","inReplyTo":"4B6C3E9C.7040009@viscovery.net","subject":"Re: [PATCH] push: Use sideband channel for hook messages","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-02-05T16:14:12Z","receivedAt":"2010-02-05T16:14:12Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Feb 5, 2010 at 4:51 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Shawn O. Pearce schrieb:\n>> Johannes Sixt <j.sixt@viscovery.net> wrote:\n>>> What would you think about passing both channels to the async callback,\n>>> and the communicating parties must agree on which channel they communicate\n>>> by closing the unused one? It would require slight changes to all current\n>>> async users, though. (It also requires in the threaded case that we pass\n>>> dup()s of the pipe channels.)\n>>\n>> Yup, I could do that.  I feel like it might be over-engineering the\n>> solution a bit.  But I'll respin the patch by splitting it apart,\n>> and doing a bidirectional async here, since you asked nicely.\n>\n> I do agree about the over-engineering aspect. I mentioned it because in\n> one patch in the past Erik Faye-Lund also extended the async\n> infrastructure for bidirectional communication to use it in git-daemon\n> (Windows port).\n\nJust for reference, here's the latest version I wrote of that patch,\nin case it's useful to have a peak at or something:\n\nhttp://repo.or.cz/w/git/kusma.git/commit/682d90a174fc128910c1c8a4f81edb3cf9f0d9e2\n\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"133698","messageId":"20100205172916.GB27498@spearce.org","threadId":"22529","inReplyTo":"40aa078e1002050814m1edb90f8lf3d8d138fa65c332@mail.gmail.com","subject":"Re: [PATCH] push: Use sideband channel for hook messages","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-05T17:29:16Z","receivedAt":"2010-02-05T17:29:16Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Erik Faye-Lund <kusmabite@googlemail.com> wrote:\n> On Fri, Feb 5, 2010 at 4:51 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> > I do agree about the over-engineering aspect. I mentioned it because in\n> > one patch in the past Erik Faye-Lund also extended the async\n> > infrastructure for bidirectional communication to use it in git-daemon\n> > (Windows port).\n> \n> Just for reference, here's the latest version I wrote of that patch,\n> in case it's useful to have a peak at or something:\n> \n> http://repo.or.cz/w/git/kusma.git/commit/682d90a174fc128910c1c8a4f81edb3cf9f0d9e2\n\nThanks Erik.  This is actually somewhat close to what I need.\nI'm going to take your patch and include it in my series, with\nsome fixups.  I'll CC you on it when I send it out later.\n\n-- \nShawn.\n"}]}