{"thread":{"id":"6164","subject":"[PATCH 2/3] Redirect update hook stdout to stderr.","startedAt":"2006-12-31T02:55:19Z","lastAt":"2006-12-31T06:03:34Z","messageCount":3,"participants":["Shawn O. Pearce","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"30537","messageId":"20061231025519.GB5530@spearce.org","threadId":"6164","inReplyTo":"a0aecffe21074288c911c396f92901bfb558d591.1167533707.git.spearce@spearce.org","subject":"[PATCH 2/3] Redirect update hook stdout to stderr.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-31T02:55:19Z","receivedAt":"2006-12-31T02:55:19Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If an update hook outputs to stdout then that output will be sent\nback over the wire to the push client as though it were part of\nthe git protocol.  This tends to cause protocol errors on the\nclient end of the connection, as the hook output is not expected\nin that context.  Most hook developers work around this by making\nsure their hook outputs everything to stderr.\n\nBut hooks shouldn't need to perform such special behavior.  Instead\nwe can just dup stderr to stdout prior to invoking the update hook.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n receive-pack.c |    3 ++-\n run-command.c  |   32 ++++++++++++++++++++++++++------\n run-command.h  |    2 ++\n 3 files changed, 30 insertions(+), 7 deletions(-)\n\ndiff --git a/receive-pack.c b/receive-pack.c\nindex af05a61..64289e9 100644\n--- a/receive-pack.c\n+++ b/receive-pack.c\n@@ -73,7 +73,8 @@ static int run_update_hook(const char *refname,\n \n \tif (access(update_hook, X_OK) < 0)\n \t\treturn 0;\n-\tcode = run_command(update_hook, refname, old_hex, new_hex, NULL);\n+\tcode = run_command_opt(RUN_COMMAND_STDOUT_TO_STDERR,\n+\t\tupdate_hook, refname, old_hex, new_hex, NULL);\n \tswitch (code) {\n \tcase 0:\n \t\treturn 0;\ndiff --git a/run-command.c b/run-command.c\nindex d0330c3..7e4ca43 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -14,7 +14,8 @@ int run_command_v_opt(const char **argv, int flags)\n \t\t\tdup2(fd, 0);\n \t\t\tdup2(fd, 1);\n \t\t\tclose(fd);\n-\t\t}\n+\t\t} else if (flags & RUN_COMMAND_STDOUT_TO_STDERR)\n+\t\t\tdup2(2, 1);\n \t\tif (flags & RUN_GIT_CMD) {\n \t\t\texecv_git_cmd(argv);\n \t\t} else {\n@@ -51,14 +52,12 @@ int run_command_v(const char **argv)\n \treturn run_command_v_opt(argv, 0);\n }\n \n-int run_command(const char *cmd, ...)\n+static int run_command_va_opt(int opt, const char *cmd, va_list param)\n {\n \tint argc;\n \tconst char *argv[MAX_RUN_COMMAND_ARGS];\n \tconst char *arg;\n-\tva_list param;\n \n-\tva_start(param, cmd);\n \targv[0] = (char*) cmd;\n \targc = 1;\n \twhile (argc < MAX_RUN_COMMAND_ARGS) {\n@@ -66,8 +65,29 @@ int run_command(const char *cmd, ...)\n \t\tif (!arg)\n \t\t\tbreak;\n \t}\n-\tva_end(param);\n \tif (MAX_RUN_COMMAND_ARGS <= argc)\n \t\treturn error(\"too many args to run %s\", cmd);\n-\treturn run_command_v_opt(argv, 0);\n+\treturn run_command_v_opt(argv, opt);\n+}\n+\n+int run_command_opt(int opt, const char *cmd, ...)\n+{\n+\tva_list params;\n+\tint r;\n+\n+\tva_start(params, cmd);\n+\tr = run_command_va_opt(opt, cmd, params);\n+\tva_end(params);\n+\treturn r;\n+}\n+\n+int run_command(const char *cmd, ...)\n+{\n+\tva_list params;\n+\tint r;\n+\n+\tva_start(params, cmd);\n+\tr = run_command_va_opt(0, cmd, params);\n+\tva_end(params);\n+\treturn r;\n }\ndiff --git a/run-command.h b/run-command.h\nindex 82a0861..8156eac 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -13,8 +13,10 @@ enum {\n \n #define RUN_COMMAND_NO_STDIO 1\n #define RUN_GIT_CMD\t     2\t/*If this is to be git sub-command */\n+#define RUN_COMMAND_STDOUT_TO_STDERR 4\n int run_command_v_opt(const char **argv, int opt);\n int run_command_v(const char **argv);\n+int run_command_opt(int opt, const char *cmd, ...);\n int run_command(const char *cmd, ...);\n \n #endif\n-- \n1.5.0.rc0.g6bb1\n"},{"id":"30539","messageId":"20061231025522.GC5530@spearce.org","threadId":"6164","inReplyTo":"a0aecffe21074288c911c396f92901bfb558d591.1167533707.git.spearce@spearce.org","subject":"[PATCH 3/3] Use /dev/null for update hook stdin.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-31T02:55:22Z","receivedAt":"2006-12-31T02:55:22Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Currently the update hook invoked by receive-pack has its stdin\nconnected to the pushing client.  The hook shouldn't attempt to\nread from this stream, and doing so may consume data that was\nmeant for receive-pack.  Instead we should give the update hook\n/dev/null as its stdin, ensuring that it always receives EOF and\ndoesn't disrupt the protocol if it attempts to read any data.\n\nThe post-update hook is similar, as it gets invoked with /dev/null\non stdin to prevent the hook from reading data from the client.\nPreviously we had invoked it with stdout also connected to /dev/null,\nthrowing away anything on stdout, to prevent client protocol errors.\nInstead we should redirect stdout to stderr, like we do with the\nupdate hook.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n receive-pack.c |    6 ++++--\n run-command.c  |    6 +++---\n run-command.h  |    2 +-\n 3 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/receive-pack.c b/receive-pack.c\nindex 64289e9..cf83109 100644\n--- a/receive-pack.c\n+++ b/receive-pack.c\n@@ -73,7 +73,8 @@ static int run_update_hook(const char *refname,\n \n \tif (access(update_hook, X_OK) < 0)\n \t\treturn 0;\n-\tcode = run_command_opt(RUN_COMMAND_STDOUT_TO_STDERR,\n+\tcode = run_command_opt(RUN_COMMAND_NO_STDIN\n+\t\t| RUN_COMMAND_STDOUT_TO_STDERR,\n \t\tupdate_hook, refname, old_hex, new_hex, NULL);\n \tswitch (code) {\n \tcase 0:\n@@ -188,7 +189,8 @@ static void run_update_post_hook(struct command *cmd)\n \t\targc++;\n \t}\n \targv[argc] = NULL;\n-\trun_command_v_opt(argv, RUN_COMMAND_NO_STDIO);\n+\trun_command_v_opt(argv, RUN_COMMAND_NO_STDIN\n+\t\t| RUN_COMMAND_STDOUT_TO_STDERR);\n }\n \n /*\ndiff --git a/run-command.c b/run-command.c\nindex 7e4ca43..cfbad74 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -9,12 +9,12 @@ int run_command_v_opt(const char **argv, int flags)\n \tif (pid < 0)\n \t\treturn -ERR_RUN_COMMAND_FORK;\n \tif (!pid) {\n-\t\tif (flags & RUN_COMMAND_NO_STDIO) {\n+\t\tif (flags & RUN_COMMAND_NO_STDIN) {\n \t\t\tint fd = open(\"/dev/null\", O_RDWR);\n \t\t\tdup2(fd, 0);\n-\t\t\tdup2(fd, 1);\n \t\t\tclose(fd);\n-\t\t} else if (flags & RUN_COMMAND_STDOUT_TO_STDERR)\n+\t\t}\n+\t\tif (flags & RUN_COMMAND_STDOUT_TO_STDERR)\n \t\t\tdup2(2, 1);\n \t\tif (flags & RUN_GIT_CMD) {\n \t\t\texecv_git_cmd(argv);\ndiff --git a/run-command.h b/run-command.h\nindex 8156eac..59c4476 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -11,7 +11,7 @@ enum {\n \tERR_RUN_COMMAND_WAITPID_NOEXIT,\n };\n \n-#define RUN_COMMAND_NO_STDIO 1\n+#define RUN_COMMAND_NO_STDIN 1\n #define RUN_GIT_CMD\t     2\t/*If this is to be git sub-command */\n #define RUN_COMMAND_STDOUT_TO_STDERR 4\n int run_command_v_opt(const char **argv, int opt);\n-- \n1.5.0.rc0.g6bb1\n"},{"id":"30559","messageId":"7vtzzciouh.fsf@assigned-by-dhcp.cox.net","threadId":"6164","inReplyTo":"20061231025522.GC5530@spearce.org","subject":"Re: [PATCH 3/3] Use /dev/null for update hook stdin.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-31T06:03:34Z","receivedAt":"2006-12-31T06:03:34Z","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> Currently the update hook invoked by receive-pack has its stdin\n> connected to the pushing client.  The hook shouldn't attempt to\n> read from this stream, and doing so may consume data that was\n> meant for receive-pack.  Instead we should give the update hook\n> /dev/null as its stdin, ensuring that it always receives EOF and\n> doesn't disrupt the protocol if it attempts to read any data.\n\nBoth 2 and 3 look sane, but can we have a few new tests too?\n"}]}