{"thread":{"id":"2875","subject":"[PATCH] Fix race and deadlock when sending pack","startedAt":"2005-12-19T03:28:54Z","lastAt":"2005-12-19T22:44:56Z","messageCount":10,"participants":["Paul Serice","Junio C Hamano","Daniel Barkalow"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"13798","messageId":"43A628F6.1060807@serice.net","threadId":"2875","inReplyTo":null,"subject":"[PATCH] Fix race and deadlock when sending pack","fromName":"Paul Serice","fromEmail":"paul@serice.net","sentAt":"2005-12-19T03:28:54Z","receivedAt":"2005-12-19T03:28:54Z","isPatch":true,"sender":{"key":"paul@serice.net","avatar":null},"body":"Fix race and deadlock when sending pack.\n\nThe best way to reproduce the problem is to locally clone your\nrepository. When you perform a push, git-send-pack will directly set\nup pipes connected to stdin and stdout of git-receive-pack. You\nshould then set up hook/post-update or hook/update to try to write\nlots of text to stdout. (You want to use the local protocol because\nssh is robust enough to mask the worst behavior.)\n\nThe first problem is that git-send-pack closes git-receive-pack's\nstdout (which is inherited by the hooks) immediately after sending the\npack. This almost always causes the hooks to receive SIGPIPE when\nthey try to write to stdout.\n\nAfter fixing the SIGPIPE problem, you then run into a deadlock because\ngit-send-pack is blocked trying to reap git-receive-pack and\ngit-receive-pack (or one of its hooks) is blocked waiting for\ngit-send-pack to read its output.\n\nI've also added an example a one-liner to both hooks demonstrating how\nto redirect all subsequent output to stderr. Because\ngit-receive-pack's stderr is not redirected, it has always been safe\nto write to stderr. Thus, all current status related output appears\non stderr. This can lead to confusing ordering of messages if only\nthe hooks are using stdout. The patch has the one-liner commented\nout, but perhaps it should be enabled by default.\n\nIn addition, this commit reverts the work-around provided by\n128aed684d0b3099092b7597c8644599b45b7503 which redirected both stdout\nand stderr for the hooks to /dev/null.\n\nSigned-off-by: Paul Serice \n\n\n---\n\n receive-pack.c | 2 +\n run-command.c | 27 +++++++++---------\n run-command.h | 3 --\n send-pack.c | 64 +++++++++++++++++++++++++++++++++++++++++-\n templates/hooks--post-update | 4 +++\n templates/hooks--update | 4 +++\n 6 files changed, 85 insertions(+), 19 deletions(-)\n\n36800ae8c6aa1427608a2d131b24986edba91bc9\ndiff --git a/receive-pack.c b/receive-pack.c\nindex cbe37e7..1873506 100644\n--- a/receive-pack.c\n+++ b/receive-pack.c\n@@ -173,7 +173,7 @@ static void run_update_post_hook(struct \n \t\targc++;\n \t}\n \targv[argc] = NULL;\n-\trun_command_v_opt(argc, argv, RUN_COMMAND_NO_STDIO);\n+\trun_command_v(argc, argv);\n }\n \n /*\ndiff --git a/run-command.c b/run-command.c\nindex 8bf5922..38cd6cb 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -2,19 +2,23 @@\n #include \"run-command.h\"\n #include \n \n-int run_command_v_opt(int argc, char **argv, int flags)\n+int run_command_v(int argc, char **argv)\n {\n-\tpid_t pid = fork();\n+ \n+\tpid_t pid = (pid_t)-1;\n+\n+\t/* Because each process has independent buffering, if you\n+\t * don't flush before the fork, it can seem like the new\n+\t * output for the child occurs before the old output of the\n+\t * parent which can be confusing at times. */\n+\tfflush(stdout);\n+\tfflush(stderr);\n+\n+\tpid = fork();\n \n \tif (pid < 0)\n \t\treturn -ERR_RUN_COMMAND_FORK;\n \tif (!pid) {\n-\t\tif (flags & RUN_COMMAND_NO_STDIO) {\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);\t\t\t\n-\t\t}\n \t\texecvp(argv[0], (char *const*) argv);\n \t\tdie(\"exec %s failed.\", argv[0]);\n \t}\n@@ -42,11 +46,6 @@ int run_command_v_opt(int argc, char **a\n \t}\n }\n \n-int run_command_v(int argc, char **argv)\n-{\n-\treturn run_command_v_opt(argc, argv, 0);\n-}\n-\n int run_command(const char *cmd, ...)\n {\n \tint argc;\n@@ -65,5 +64,5 @@ int run_command(const char *cmd, ...)\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(argc, argv, 0);\n+\treturn run_command_v(argc, argv);\n }\ndiff --git a/run-command.h b/run-command.h\nindex 2469eea..5ee0972 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -11,9 +11,6 @@ enum {\n \tERR_RUN_COMMAND_WAITPID_NOEXIT,\n };\n \n-#define RUN_COMMAND_NO_STDIO 1\n-\n-int run_command_v_opt(int argc, char **argv, int opt);\n int run_command_v(int argc, char **argv);\n int run_command(const char *cmd, ...);\n \ndiff --git a/send-pack.c b/send-pack.c\nindex 6ce0d9f..efc66ca 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -274,6 +274,55 @@ static int send_pack(int in, int out, in\n \treturn ret;\n }\n \n+/* This function copies the data from in_fd to out_fd. It returns 1 if\n+ * all data was successfully copied; otherwise, it returns 0, and\n+ * errno will be set depending on how read() or write() failed. */\n+static int cpfd(int in_fd, int out_fd)\n+{\n+\tint rv = 0;\n+\tssize_t rcount = -1;\n+\tssize_t wcount = 0;\n+\tssize_t wtmp = -1;\n+\tstatic char cpfd_buf[4096];\n+\n+\tfor (;;) {\n+\n+\t\t/* Read buffer. */\n+\t\trcount = read(in_fd, cpfd_buf, sizeof(cpfd_buf));\n+\n+\t\t/* Done. */\n+\t\tif (rcount == 0) {\n+\t\t\trv = 1;\n+\t\t\tgoto out;\n+\t\t}\n+\n+\t\t/* Error. */\n+\t\tif (rcount < 0) {\n+\t\t\tif (errno == EINTR) {\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tgoto out;\n+\t\t}\n+\n+\t\t/* Write buffer. */\n+\t\twcount = 0;\n+\t\twhile (wcount < rcount) {\n+\t\t\twtmp = write(out_fd,\n+\t\t\t cpfd_buf + wcount,\n+\t\t\t rcount - wcount);\n+\t\t\tif (wtmp < 0) {\n+\t\t\t\tif (errno == EINTR) {\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\t\t\t\tgoto out;\n+\t\t\t}\n+\t\t\twcount += wtmp;\n+\t\t}\n+\t}\n+\n+ out:\n+\treturn rv;\n+}\n \n int main(int argc, char **argv)\n {\n@@ -319,8 +368,21 @@ int main(int argc, char **argv)\n \tif (pid < 0)\n \t\treturn 1;\n \tret = send_pack(fd[0], fd[1], nr_heads, heads);\n-\tclose(fd[0]);\n+\n+\t/* git_connect() sets up a pipe between this program and\n+\t * \"exec\" (typically git-receive-pack).\t The git-receive-pack\n+\t * program in turn executes the \"update\" and \"post-update\"\n+\t * hooks which might write to this program's fd[0]. To avoid\n+\t * deadlock, this program must consume all of the data on\n+\t * fd[0]. (The hooks are called after the pack has been\n+\t * transfered so it should it should be safe to allow them to\n+\t * write to stdout because it should not interfere with the\n+\t * transfer protocol which also occurs on stdout). */\n+\tcpfd(fd[0], fileno(stdout));\n+\n \tclose(fd[1]);\n \tfinish_connect(pid);\n+\tclose(fd[0]);\n+\n \treturn ret;\n }\ndiff --git a/templates/hooks--post-update b/templates/hooks--post-update\nindex bcba893..d470dcc 100644\n--- a/templates/hooks--post-update\n+++ b/templates/hooks--post-update\n@@ -5,4 +5,8 @@\n #\n # To enable this hook, make this file executable by \"chmod +x post-update\".\n \n+# If your stdout and stderr messages are interleaved, uncomment the\n+# following line.\n+#exec 1>&2\n+\n exec git-update-server-info\ndiff --git a/templates/hooks--update b/templates/hooks--update\nindex 6db555f..6199deb 100644\n--- a/templates/hooks--update\n+++ b/templates/hooks--update\n@@ -8,6 +8,10 @@\n # (2) make this file executable by \"chmod +x update\".\n #\n \n+# If your stdout and stderr messages are interleaved, uncomment the\n+# following line.\n+#exec 1>&2\n+\n recipient=\"commit-list@example.com\"\n \n if expr \"$2\" : '0*$' >/dev/null\n-- \n0.99.9.GIT\n"},{"id":"13803","messageId":"7vzmmxlkbq.fsf@assigned-by-dhcp.cox.net","threadId":"2875","inReplyTo":"43A628F6.1060807@serice.net","subject":"Re: [PATCH] Fix race and deadlock when sending pack","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-12-19T05:36:25Z","receivedAt":"2005-12-19T05:36:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Serice writes:\n\n> The best way to reproduce the problem is to locally clone your\n> repository. When you perform a push, git-send-pack will directly set\n> up pipes connected to stdin and stdout of git-receive-pack. You\n> should then set up hook/post-update or hook/update to try to write\n> lots of text to stdout. (You want to use the local protocol because\n> ssh is robust enough to mask the worst behavior.)\n\nMy immediate reaction was \"do not do it then\", but you are\nright. Hooks are run after all the protocol exchanges are done,\nso they should be free to throw any garbage at the other end.\n\nIt appears cpfd() seems to mostly duplicate what is in copy.c;\nis there any particular reason?\n"},{"id":"13806","messageId":"Pine.LNX.4.64.0512190130450.25300@iabervon.org","threadId":"2875","inReplyTo":"43A628F6.1060807@serice.net","subject":"Re: [PATCH] Fix race and deadlock when sending pack","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2005-12-19T06:49:44Z","receivedAt":"2005-12-19T06:49:44Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 18 Dec 2005, Paul Serice wrote:\n\n> Fix race and deadlock when sending pack.\n> \n> The best way to reproduce the problem is to locally clone your\n> repository. When you perform a push, git-send-pack will directly set\n> up pipes connected to stdin and stdout of git-receive-pack. You\n> should then set up hook/post-update or hook/update to try to write\n> lots of text to stdout. (You want to use the local protocol because\n> ssh is robust enough to mask the worst behavior.)\n> \n> The first problem is that git-send-pack closes git-receive-pack's\n> stdout (which is inherited by the hooks) immediately after sending the\n> pack. This almost always causes the hooks to receive SIGPIPE when\n> they try to write to stdout.\n> \n> After fixing the SIGPIPE problem, you then run into a deadlock because\n> git-send-pack is blocked trying to reap git-receive-pack and\n> git-receive-pack (or one of its hooks) is blocked waiting for\n> git-send-pack to read its output.\n> \n> I've also added an example a one-liner to both hooks demonstrating how\n> to redirect all subsequent output to stderr. Because\n> git-receive-pack's stderr is not redirected, it has always been safe\n> to write to stderr. Thus, all current status related output appears\n> on stderr. This can lead to confusing ordering of messages if only\n> the hooks are using stdout. The patch has the one-liner commented\n> out, but perhaps it should be enabled by default.\n> \n> In addition, this commit reverts the work-around provided by\n> 128aed684d0b3099092b7597c8644599b45b7503 which redirected both stdout\n> and stderr for the hooks to /dev/null.\n\nActually, that was stdin and stdout. If, for some reason, a hook looked at \nstdin, it could get surprising results. I don't think that it's actually a \ngood idea to have output to stdout from hooks go to git-send-pack's \nstdout, since we may want to have git-send-pack report some sort of \ninformation of its own to stdout, which would then get confused with \noutput from hooks. I think /dev/null, a log file, and stderr are the \nreasonable choices for what happens to output (and input pretty much has \nto be /dev/null).\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"13807","messageId":"7vvexlihmq.fsf@assigned-by-dhcp.cox.net","threadId":"2875","inReplyTo":"Pine.LNX.4.64.0512190130450.25300@iabervon.org","subject":"Re: [PATCH] Fix race and deadlock when sending pack","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-12-19T09:02:53Z","receivedAt":"2005-12-19T09:02:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow writes:\n\n> ... I don't think that it's actually a \n> good idea to have output to stdout from hooks go to git-send-pack's \n> stdout, since we may want to have git-send-pack report some sort of \n> information of its own to stdout,...\n\nI admit that I haven't thought things through yet, but I do not\noffhand think of an argument against Paul's patch (a scenario\nthat may be broken by the patch, that is), so I am inclined to\ntake it, perhaps after hearing about the cpfd() thing I\nmentioned in the previous response to Paul.\n\nIt is conceivable that we may want to later extend the protocol\nso that the receiver can tell the sender the result of what\nhappened to each of the ref-update request. Right now, the\nsender refuses to listen to what receiver says after it learns\nthe current object names, but after pack transfer finishes and\nreceiver decides what to do with each ref update request, we\nmight want to add status, like this:\n\n\t# Tell the pusher what commits we have and what their names are\n\tR: SHA1 name\n\tR: ...\n\tR: SHA1 name\n\tR: # flush -- it's your turn\n\t# Tell the puller what the pusher wants to happen\n\tS: old-SHA1 new-SHA1 name\n\tS: old-SHA1 new-SHA1 name\n\tS: ...\n\tS: # flush -- done with the list\n\tS: XXXXXXX --- packfile contents.\n\t# current protocol exchange ends here, but we could add...\n\n # ... what happened to each ref-update request.\n\tR: name OK\n R: name FAIL\n R: ...\n\nIf we do something like this, we might want to say why things\nfailed on \"FAIL\" line, and the output from hooks/update that\nprevented the ref-update would probably belong there.\n"},{"id":"13816","messageId":"43A6E54B.3080708@serice.net","threadId":"2875","inReplyTo":"7vzmmxlkbq.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix race and deadlock when sending pack","fromName":"Paul Serice","fromEmail":"paul@serice.net","sentAt":"2005-12-19T16:52:27Z","receivedAt":"2005-12-19T16:52:27Z","isPatch":true,"sender":{"key":"paul@serice.net","avatar":null},"body":"> It appears cpfd() seems to mostly duplicate what is in copy.c;\n> is there any particular reason?\n\nNo, I just wasn't aware of it. I've made another patch to account for\nit. I wonder why copy_fd() (usually) closes ifd though. I also\nwonder why, if it is going to close ifd, doesn't it do so when there\nan error is detected after the write() call.\n\nI would also like to mention that I'm on the fence about all of this.\nI think a lot of it is a matter of policy, and I'm just not sure what\nthe policy is regarding the hooks. I wanted to submit the patch in it\nfull form to show that it is possible to redirect stdout of the hooks\nto the terminal. I also feel that, even if the patch is ultimately\nrejected (no hard feelings :-), it would be interesting to know what's\nthe underlying problem.\n\nPaul Serice\n\n========================================================================\n\nSubject: [PATCH] Fix race and deadlock when sending pack.\n\nThe best way to reproduce the problem is to locally clone your\nrepository. When you perform a push, git-send-pack will directly set\nup pipes connected to stdin and stdout of git-receive-pack. You\nshould then set up hook/post-update or hook/update to try to write\nlots of text to stdout. (You want to use the local protocol because\nssh is robust enough to mask the worst behavior.)\n\nThe first problem is that git-send-pack closes git-receive-pack's\nstdout (which is inherited by the hooks) immediately after sending the\npack. This almost always causes the hooks to receive SIGPIPE when\nthey try to write to stdout.\n\nAfter fixing the SIGPIPE problem, you then run into a deadlock because\ngit-send-pack is blocked trying to reap git-receive-pack and\ngit-receive-pack (or one of its hooks) is blocked waiting for\ngit-send-pack to read its output.\n\nI've also added an example a one-liner to both hooks demonstrating how\nto redirect all subsequent output to stderr. Because\ngit-receive-pack's stderr is not redirected, it has always been safe\nto write to stderr. Thus, all current status related output appears\non stderr. This can lead to confusing ordering of messages if only\nthe hooks are using stdout. The patch has the one-liner commented\nout, but perhaps it should be enabled by default.\n\nIn addition, this commit reverts the work-around provided by\n128aed684d0b3099092b7597c8644599b45b7503 which redirected stdin and\nstdout to /dev/null.\n\nSigned-off-by: Paul Serice \n\n\n---\n\n receive-pack.c | 2 +-\n run-command.c | 27 +++++++++++++--------------\n run-command.h | 3 ---\n send-pack.c | 9 ++++++++-\n templates/hooks--post-update | 4 ++++\n templates/hooks--update | 4 ++++\n 6 files changed, 30 insertions(+), 19 deletions(-)\n\n87b7ee91cbe1b90bbd0937a85595de3933fc9459\ndiff --git a/receive-pack.c b/receive-pack.c\nindex cbe37e7..1873506 100644\n--- a/receive-pack.c\n+++ b/receive-pack.c\n@@ -173,7 +173,7 @@ static void run_update_post_hook(struct \n \t\targc++;\n \t}\n \targv[argc] = NULL;\n-\trun_command_v_opt(argc, argv, RUN_COMMAND_NO_STDIO);\n+\trun_command_v(argc, argv);\n }\n \n /*\ndiff --git a/run-command.c b/run-command.c\nindex 8bf5922..38cd6cb 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -2,19 +2,23 @@\n #include \"run-command.h\"\n #include \n \n-int run_command_v_opt(int argc, char **argv, int flags)\n+int run_command_v(int argc, char **argv)\n {\n-\tpid_t pid = fork();\n+ \n+\tpid_t pid = (pid_t)-1;\n+\n+\t/* Because each process has independent buffering, if you\n+\t * don't flush before the fork, it can seem like the new\n+\t * output for the child occurs before the old output of the\n+\t * parent which can be confusing at times. */\n+\tfflush(stdout);\n+\tfflush(stderr);\n+\n+\tpid = fork();\n \n \tif (pid < 0)\n \t\treturn -ERR_RUN_COMMAND_FORK;\n \tif (!pid) {\n-\t\tif (flags & RUN_COMMAND_NO_STDIO) {\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);\t\t\t\n-\t\t}\n \t\texecvp(argv[0], (char *const*) argv);\n \t\tdie(\"exec %s failed.\", argv[0]);\n \t}\n@@ -42,11 +46,6 @@ int run_command_v_opt(int argc, char **a\n \t}\n }\n \n-int run_command_v(int argc, char **argv)\n-{\n-\treturn run_command_v_opt(argc, argv, 0);\n-}\n-\n int run_command(const char *cmd, ...)\n {\n \tint argc;\n@@ -65,5 +64,5 @@ int run_command(const char *cmd, ...)\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(argc, argv, 0);\n+\treturn run_command_v(argc, argv);\n }\ndiff --git a/run-command.h b/run-command.h\nindex 2469eea..5ee0972 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -11,9 +11,6 @@ enum {\n \tERR_RUN_COMMAND_WAITPID_NOEXIT,\n };\n \n-#define RUN_COMMAND_NO_STDIO 1\n-\n-int run_command_v_opt(int argc, char **argv, int opt);\n int run_command_v(int argc, char **argv);\n int run_command(const char *cmd, ...);\n \ndiff --git a/send-pack.c b/send-pack.c\nindex 6ce0d9f..5a99ba9 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -319,8 +319,15 @@ int main(int argc, char **argv)\n \tif (pid < 0)\n \t\treturn 1;\n \tret = send_pack(fd[0], fd[1], nr_heads, heads);\n-\tclose(fd[0]);\n+\n+\t/* Close our side of the conversation.\tWait for the child to\n+\t * close its side of the conversation (copying the remainder\n+\t * to our stdout). Note that copy_fd() has the side effect of\n+\t * closing fd[0]. */\n \tclose(fd[1]);\n+\tcopy_fd(fd[0], fileno(stdout));\n+\n \tfinish_connect(pid);\n+\n \treturn ret;\n }\ndiff --git a/templates/hooks--post-update b/templates/hooks--post-update\nindex bcba893..d470dcc 100644\n--- a/templates/hooks--post-update\n+++ b/templates/hooks--post-update\n@@ -5,4 +5,8 @@\n #\n # To enable this hook, make this file executable by \"chmod +x post-update\".\n \n+# If your stdout and stderr messages are interleaved, uncomment the\n+# following line.\n+#exec 1>&2\n+\n exec git-update-server-info\ndiff --git a/templates/hooks--update b/templates/hooks--update\nindex 6db555f..6199deb 100644\n--- a/templates/hooks--update\n+++ b/templates/hooks--update\n@@ -8,6 +8,10 @@\n # (2) make this file executable by \"chmod +x update\".\n #\n \n+# If your stdout and stderr messages are interleaved, uncomment the\n+# following line.\n+#exec 1>&2\n+\n recipient=\"commit-list@example.com\"\n \n if expr \"$2\" : '0*$' >/dev/null\n-- \n0.99.9.GIT\n"},{"id":"13817","messageId":"43A6EE06.8040108@serice.net","threadId":"2875","inReplyTo":"Pine.LNX.4.64.0512190130450.25300@iabervon.org","subject":"Re: [PATCH] Fix race and deadlock when sending pack","fromName":"Paul Serice","fromEmail":"paul@serice.net","sentAt":"2005-12-19T17:29:42Z","receivedAt":"2005-12-19T17:29:42Z","isPatch":true,"sender":{"key":"paul@serice.net","avatar":null},"body":"> If, for some reason, a hook looked at stdin, it could get surprising\n> results.\n\nIn the first patch, the hook would hang waiting for input. That was a\nbug. In the second patch, the hook's standard input has been closed\nby git-send-pack (which is the standard thing for the parent to do in\na pipeline), thus if a hook looks at stdin it will get EOF (the same\nas if connected to /dev/null).\n\n\nPaul\n"},{"id":"13820","messageId":"Pine.LNX.4.64.0512191236290.25300@iabervon.org","threadId":"2875","inReplyTo":"7vzmmxlkbq.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix race and deadlock when sending pack","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2005-12-19T18:40:37Z","receivedAt":"2005-12-19T18:40:37Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 18 Dec 2005, Junio C Hamano wrote:\n\n> Paul Serice writes:\n> \n> > The best way to reproduce the problem is to locally clone your\n> > repository. When you perform a push, git-send-pack will directly set\n> > up pipes connected to stdin and stdout of git-receive-pack. You\n> > should then set up hook/post-update or hook/update to try to write\n> > lots of text to stdout. (You want to use the local protocol because\n> > ssh is robust enough to mask the worst behavior.)\n> \n> My immediate reaction was \"do not do it then\", but you are\n> right. Hooks are run after all the protocol exchanges are done,\n> so they should be free to throw any garbage at the other end.\n\nIf we extend it to transfer multiple things, wouldn't we want to run hooks \nafter each of them, rather than all at the end?\n\nAs for the policy:\n\nWe definitely want to let hooks write to stdout, because git programs that \nyou might want to run in hooks write to stdout. I can't figure out what \n\"cvs\" does with trigger script output and \"at\" and \"cron\" email the output \nto the owners. I'd sort of like to avoid making people expect that there \nis necessarily a path for text going back to the user directly. We may, \nfor example, want to support these hooks with pushes over HTTP(/WebDAV). I \nalso think that messages are likely to be at least as useful to the owner \nof the target repository as the person pushing, which is why I'd prefer a \nlog file. E.g., if you've got a group central repository that different \npeople push to, it may be other group members who want to know what \nhappened with the output from a post-update hook, not the group member who \npushed.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"13824","messageId":"7vy82gg5t7.fsf@assigned-by-dhcp.cox.net","threadId":"2875","inReplyTo":"Pine.LNX.4.64.0512191236290.25300@iabervon.org","subject":"Re: [PATCH] Fix race and deadlock when sending pack","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-12-19T21:01:08Z","receivedAt":"2005-12-19T21:01:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow writes:\n\n>> My immediate reaction was \"do not do it then\", but you are\n>> right. Hooks are run after all the protocol exchanges are done,\n>> so they should be free to throw any garbage at the other end.\n>\n> If we extend it to transfer multiple things, wouldn't we want to run hooks \n> after each of them, rather than all at the end?\n\nWe do transfer multiple things already, and all protocol\nexchange happens before everything is transferred. And hooks\nare run for each refs being updated, one by one. What we do not\nhave is a reporting mechanism that says \"we refused to update\nthis ref because of the hooks/update policy return value for\nit\". Even if we later add that reporting mechanism, as I\noutlined in a separate message earlier, I think it is OK to keep\nrunning the update hooks after the pack transfer part.\n\n> As for the policy:\n>\n> We definitely want to let hooks write to stdout, because git programs that \n> you might want to run in hooks write to stdout.\n> ... I'd sort of like to avoid making people expect that there \n> is necessarily a path for text going back to the user directly.\n> ... I \n> also think that messages are likely to be at least as useful to the owner \n> of the target repository as the person pushing, which is why I'd prefer a \n> log file.\n\nThis part I mostly agree with. Will have to think about the\ndetails but probably I'd punt this for now and declare it post\n1.0 ;-).\n"},{"id":"13825","messageId":"Pine.LNX.4.64.0512191645230.25300@iabervon.org","threadId":"2875","inReplyTo":"7vy82gg5t7.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix race and deadlock when sending pack","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2005-12-19T22:00:45Z","receivedAt":"2005-12-19T22:00:45Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 19 Dec 2005, Junio C Hamano wrote:\n\n> Daniel Barkalow writes:\n> \n> >> My immediate reaction was \"do not do it then\", but you are\n> >> right. Hooks are run after all the protocol exchanges are done,\n> >> so they should be free to throw any garbage at the other end.\n> >\n> > If we extend it to transfer multiple things, wouldn't we want to run hooks \n> > after each of them, rather than all at the end?\n> \n> We do transfer multiple things already, and all protocol\n> exchange happens before everything is transferred. And hooks\n> are run for each refs being updated, one by one. What we do not\n> have is a reporting mechanism that says \"we refused to update\n> this ref because of the hooks/update policy return value for\n> it\". Even if we later add that reporting mechanism, as I\n> outlined in a separate message earlier, I think it is OK to keep\n> running the update hooks after the pack transfer part.\n\nIf we have the reporting mechanism, that will effectively be part of the \nprotocol. It's obviously done transferring the pack at that point, but it \nstill wants fixed-format communication, so switching over to being the \nstardard output of the hooks would cause problems with this.\n\n> > As for the policy:\n> >\n> > We definitely want to let hooks write to stdout, because git programs that \n> > you might want to run in hooks write to stdout.\n> > ... I'd sort of like to avoid making people expect that there \n> > is necessarily a path for text going back to the user directly.\n> > ... I \n> > also think that messages are likely to be at least as useful to the owner \n> > of the target repository as the person pushing, which is why I'd prefer a \n> > log file.\n> \n> This part I mostly agree with. Will have to think about the\n> details but probably I'd punt this for now and declare it post\n> 1.0 ;-).\n\nIt's probably worth making sure that all the hooks run with something \nsane, and punt making it configurable andnice until post-1.0. I was only \nreally looking at post-update, so I don't know how the others run.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"13829","messageId":"7vr788emfr.fsf@assigned-by-dhcp.cox.net","threadId":"2875","inReplyTo":"Pine.LNX.4.64.0512191645230.25300@iabervon.org","subject":"Re: [PATCH] Fix race and deadlock when sending pack","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-12-19T22:44:56Z","receivedAt":"2005-12-19T22:44:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow writes:\n\n> If we have the reporting mechanism, that will effectively be part of the \n> protocol. It's obviously done transferring the pack at that point, but it \n> still wants fixed-format communication, so switching over to being the \n> stardard output of the hooks would cause problems with this.\n\nIn order to add reporting mechanism later, I think we need to be\nable to identify the protocol version in a backward compatible\nway, something like the \"server capabilities hidden behind the\nNUL\" trick we did for fetch-pack/upload-pack protocol. Once\nthat is in place, it does not cause harm even if the current\nprotocol program connects hooks' stdout to send-pack, at least\nin theory. If we take Paul's patch now, however, it would add\nmore work for us later when we do that protocol change, because\nwe will need to wrap the output from the hook in the pkt-line\ninterface in the new protocol, in order to give that back to the\nstdout of send-pack. Considering that, I think we may want to\ndrop Paul's patch and declare that hooks stdout does not come\nback to send-pack.\n\nHonestly speaking, I do not really care where stdout of hooks go\nas long as that does not cause breakage/deadlocks, and I think\nyour earlier patch on December 7th is serving us well enough; we\nneeded to have told users to do an \"exec 1>somewhere\" in their\nhooks before that fix, which was not nice at all (and we even\nforgot to tell them that). If people want to send the output to\na log file, they can do so; if they want e-mails, they can do\nso; if they want to show the output to the pusher, they can do\n1>&2; all inside their hooks. I do \"echo nitfol | at now\" and\nlove the way that I do not have to worry about how \"at\" command\ngives me back execution report via e-mail at all ;-).\n\n> It's probably worth making sure that all the hooks run with something \n> sane, and punt making it configurable andnice until post-1.0.\n\nI think we agree that /dev/null is one of the sane choices as\nyou did in your earlier fix. Duping stderr would have been\nanother sane choice, but I honestly do not think we care much\neither way.\n"}]}