{"thread":{"id":"37548","subject":"[RFC] allowing hooks to ignore input?","startedAt":"2014-09-12T22:48:07Z","lastAt":"2014-09-18T11:14:12Z","messageCount":5,"participants":["Junio C Hamano","Johannes Sixt","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"249333","messageId":"xmqqzje48nh4.fsf@gitster.dls.corp.google.com","threadId":"37548","inReplyTo":null,"subject":"[RFC] allowing hooks to ignore input?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-12T22:48:07Z","receivedAt":"2014-09-12T22:48:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The pre-receive and post-receive hooks were designed to be an\nimprovement over old style update and post-update hooks that used to\ntake the update information on the command line and were limited by\nthe command line length limit.  They take the same information from\ntheir standard input.  It has been mandatory for these new style\nhooks to consume the update information fully from the standard\ninput stream.  Otherwise, they would risk killing the receive-pack\nprocess via SIGPIPE.\n\nThis is easy, and it has already been done by existing hooks that\nare written correctly, to work around, if a hook does not want to\nlook at all the information, by sending its standard input to\n/dev/null (perhaps a niche use of hook might need to know only the\nfact that a push was made, without having to know what objects have\nbeen pushed to update which refs).\n\nHowever, because there is no good way to consistently fail hooks\nthat do not consume the input fully, it can lead to a hard to\ndiagnose \"once in a blue moon\" phantom failure.\n\nI wonder if this \"you must consume the input fully\", which is a\nmandate that is not enforced strictly, is not helping us to catch\nmistakes in hooks more than it is hurting us.  Perhaps we can do\nsomething like the attached patch to make it optional for them to\nread the input we feed?\n\nI dunno.\n\n builtin/receive-pack.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 30560a7..9d9d16d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -17,6 +17,7 @@\n #include \"version.h\"\n #include \"tag.h\"\n #include \"gpg-interface.h\"\n+#include \"sigchain.h\"\n \n static const char receive_pack_usage[] = \"git receive-pack <git-dir>\";\n \n@@ -500,6 +501,8 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed, void *feed_sta\n \t\treturn code;\n \t}\n \n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n \twhile (1) {\n \t\tconst char *buf;\n \t\tsize_t n;\n@@ -511,6 +514,9 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed, void *feed_sta\n \tclose(proc.in);\n \tif (use_sideband)\n \t\tfinish_async(&muxer);\n+\n+\tsigchain_pop(SIGPIPE);\n+\n \treturn finish_command(&proc);\n }\n \n"},{"id":"249337","messageId":"5413FE06.1020901@kdbg.org","threadId":"37548","inReplyTo":"xmqqzje48nh4.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC] allowing hooks to ignore input?","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-09-13T08:19:18Z","receivedAt":"2014-09-13T08:19:18Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 13.09.2014 um 00:48 schrieb Junio C Hamano:\n> The pre-receive and post-receive hooks were designed to be an\n> improvement over old style update and post-update hooks that used to\n> take the update information on the command line and were limited by\n> the command line length limit.  They take the same information from\n> their standard input.  It has been mandatory for these new style\n> hooks to consume the update information fully from the standard\n> input stream.  Otherwise, they would risk killing the receive-pack\n> process via SIGPIPE.\n> \n> This is easy, and it has already been done by existing hooks that\n> are written correctly, to work around, if a hook does not want to\n> look at all the information, by sending its standard input to\n> /dev/null (perhaps a niche use of hook might need to know only the\n> fact that a push was made, without having to know what objects have\n> been pushed to update which refs).\n> \n> However, because there is no good way to consistently fail hooks\n> that do not consume the input fully, it can lead to a hard to\n> diagnose \"once in a blue moon\" phantom failure.\n> \n> I wonder if this \"you must consume the input fully\", which is a\n> mandate that is not enforced strictly, is not helping us to catch\n> mistakes in hooks more than it is hurting us.  Perhaps we can do\n> something like the attached patch to make it optional for them to\n> read the input we feed?\n> \n> I dunno.\n> \n>  builtin/receive-pack.c | 6 ++++++\n>  1 file changed, 6 insertions(+)\n> \n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 30560a7..9d9d16d 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -17,6 +17,7 @@\n>  #include \"version.h\"\n>  #include \"tag.h\"\n>  #include \"gpg-interface.h\"\n> +#include \"sigchain.h\"\n>  \n>  static const char receive_pack_usage[] = \"git receive-pack <git-dir>\";\n>  \n> @@ -500,6 +501,8 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed, void *feed_sta\n>  \t\treturn code;\n>  \t}\n>  \n> +\tsigchain_push(SIGPIPE, SIG_IGN);\n> +\n>  \twhile (1) {\n>  \t\tconst char *buf;\n>  \t\tsize_t n;\n> @@ -511,6 +514,9 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed, void *feed_sta\n>  \tclose(proc.in);\n>  \tif (use_sideband)\n>  \t\tfinish_async(&muxer);\n> +\n> +\tsigchain_pop(SIGPIPE);\n> +\n>  \treturn finish_command(&proc);\n>  }\n\nI think this is a good move. Hooks are written by users, who sometimes\nare not clueful enough.\n\nBut what do our writers do when they fail with EPIPE? Die? If so, this\npatch alone is not sufficient.\n\n-- Hannes\n"},{"id":"249436","messageId":"xmqqmwa093h1.fsf@gitster.dls.corp.google.com","threadId":"37548","inReplyTo":"5413FE06.1020901@kdbg.org","subject":"Re: [RFC] allowing hooks to ignore input?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-15T17:51:38Z","receivedAt":"2014-09-15T17:51:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> I think this is a good move. Hooks are written by users, who sometimes\n> are not clueful enough.\n>\n> But what do our writers do when they fail with EPIPE? Die? If so, this\n> patch alone is not sufficient.\n\nI think it is in a loop\n\n\twhile (...) {\n        \tif (write_in_full(...))\n                \tbreak;\n\t}\n\t...\n        return finish_command(&proc);\n\nand the caller only cares about the tatus of finish_command(),\ni.e. the exit status of the downstream of the pipe.  In an EPIPE\nsituation, they have already exited with the status they want to\ngive to us without listening to what we wanted to tell them, so I\nthink there is no issue.\n\nWe _might_ want to check, at that point of \"break\", if we got a\nwrite error other than EPIPE and act somewhat differently, but that\nwould be a different issue the current code already has.  E.g. if we\nbroke out of the loop due to an EIO or EPERM, it will not be affect\nwhat the later code would do in the current code.\n"},{"id":"249581","messageId":"xmqq7g1319rz.fsf@gitster.dls.corp.google.com","threadId":"37548","inReplyTo":"5413FE06.1020901@kdbg.org","subject":"Re: [RFC] allowing hooks to ignore input?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-16T22:27:12Z","receivedAt":"2014-09-16T22:27:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> I think this is a good move. Hooks are written by users, who sometimes\n> are not clueful enough.\n\nThanks for a sanity check.  I do not think it is about cluefulness\nin this particular case.  A rule that is not meaningfully enforced\nby reliably failing offenders is a rule that is hard to follow if a\nclueful user wanted to.\n\nThis round comes with a test, but depending on the size of your pipe\nbuffer and context switching race, an unpatched Git may pass it\n(reducing the test_seq number down to say 199 would certainly make\nit pass without the fix most of the time).\n\n-- >8 --\nSubject: [PATCH] receive-pack: allow hooks to ignore its standard input stream\n\nThe pre-receive and post-receive hooks were designed to be an\nimprovement over old style update and post-update hooks, which take\nthe update information on their command line and are limited by the\ncommand line length limit.  The same information is fed from the\nstandard input to pre/post-receive hooks instead to lift this\nlimitation.  It has been mandatory for these new style hooks to\nconsume the update information fully from the standard input stream.\nOtherwise, they would risk killing the receive-pack process via\nSIGPIPE.\n\nIf a hook does not want to look at all the information, it is easy\nto send its standard input to /dev/null (perhaps a niche use of hook\nmight need to know only the fact that a push was made, without\nhaving to know what objects have been pushed to update which refs),\nand this has already been done by existing hooks that are written\ncarefully.\n\nHowever, because there is no good way to consistently fail hooks\nthat do not consume the input fully (a small push may result in a\nshort update record that may fit within the pipe buffer, to which\nthe receive-pack process may manage to write before the hook has a\nchance to exit without reading anything, which will not result in a\ndeath-by-SIGPIPE of receive-pack), it can lead to a hard to diagnose\n\"once in a blue moon\" phantom failure.\n\nLift this \"hooks must consume their input fully\" mandate.  A mandate\nthat is not enforced strictly is not helping us to catch mistakes in\nhooks.  If a hook has a good reason to decide the outcome of its\noperation without reading the information we feed it, let it do so\nas it pleases.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/receive-pack.c  |  6 ++++++\n t/t5401-update-hooks.sh | 13 +++++++++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex f93ac45..516386f 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -15,6 +15,7 @@\n #include \"connected.h\"\n #include \"argv-array.h\"\n #include \"version.h\"\n+#include \"sigchain.h\"\n \n static const char receive_pack_usage[] = \"git receive-pack <git-dir>\";\n \n@@ -288,6 +289,8 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed, void *feed_sta\n \t\treturn code;\n \t}\n \n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n \twhile (1) {\n \t\tconst char *buf;\n \t\tsize_t n;\n@@ -299,6 +302,9 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed, void *feed_sta\n \tclose(proc.in);\n \tif (use_sideband)\n \t\tfinish_async(&muxer);\n+\n+\tsigchain_pop(SIGPIPE);\n+\n \treturn finish_command(&proc);\n }\n \ndiff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh\nindex 17bcb0b..7f278d8 100755\n--- a/t/t5401-update-hooks.sh\n+++ b/t/t5401-update-hooks.sh\n@@ -135,4 +135,17 @@ test_expect_success 'send-pack stderr contains hook messages' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pre-receive hook that forgets to read its input' '\n+\twrite_script victim.git/hooks/pre-receive <<-\\EOF &&\n+\texit 0\n+\tEOF\n+\trm -f victim.git/hooks/update victim.git/hooks/post-update &&\n+\n+\tfor v in $(test_seq 100 999)\n+\tdo\n+\t\tgit branch branch_$v master || return\n+\tdone &&\n+\tgit push ./victim.git \"+refs/heads/*:refs/heads/*\"\n+'\n+\n test_done\n-- \n2.1.0-403-g099cf47\n"},{"id":"249631","messageId":"20140918111412.GB13481@peff.net","threadId":"37548","inReplyTo":"xmqq7g1319rz.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC] allowing hooks to ignore input?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-18T11:14:12Z","receivedAt":"2014-09-18T11:14:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 16, 2014 at 03:27:12PM -0700, Junio C Hamano wrote:\n\n> Johannes Sixt <j6t@kdbg.org> writes:\n> \n> > I think this is a good move. Hooks are written by users, who sometimes\n> > are not clueful enough.\n> \n> Thanks for a sanity check.  I do not think it is about cluefulness\n> in this particular case.  A rule that is not meaningfully enforced\n> by reliably failing offenders is a rule that is hard to follow if a\n> clueful user wanted to.\n\nThis looks good to me. Here's a real-world example of somebody getting\nbitten by this (I _thought_ we had dealt with it then, but apparently\nnot):\n\n  http://article.gmane.org/gmane.comp.version-control.git/186287\n\n> This round comes with a test, but depending on the size of your pipe\n> buffer and context switching race, an unpatched Git may pass it\n> (reducing the test_seq number down to say 199 would certainly make\n> it pass without the fix most of the time).\n\nI took a look at your test. Having worked on racy sigpipe tests\nrecently, I think the only way to guarantee failure is to make sure you\nfill up the pipe buffer. My experiments showed this was typically 64K on\na variety of Linux systems, but I think can be bumped up at runtime.\n\nWhen working on the sanitized_signals() test, we decided to just pick an\narbitrarily high number like 1MB and use that (ideally you would send\ninfinite data until you get SIGPIPE, but the failure mode for your test\nis then that it doesn't finish :-/).\n\nYou have things much harder and much easier here. Harder, in that you\ncan only convince git to send ~100 bytes per ref, so you would need a\nlot of refs (and thus it's expensive to over-compensate). But easier, in\nthat you do not need to _reliably_ catch SIGPIPE. You only need to catch\nit often enough that somebody notices if the rest is broken, not catch\nit every time to ensure success.\n\nSo I think it is OK as-is. You should be generating ~91K of ref data\n(refname + two sha1s + spaces and newline), and I can't imagine many\nsystems have a pipe buffer bigger than 64K. If they do, the only\ndownside is that those systems might only intermittently catch a\nregression.\n\n-Peff\n"}]}