{"thread":{"id":"58109","subject":"bug report: pre-commit & pre-push hook output is redirected differently","startedAt":"2022-07-06T20:41:06Z","lastAt":"2022-07-07T20:55:59Z","messageCount":5,"participants":["Adam Zethraeus","Junio C Hamano","Ævar Arnfjörð Bjarmason","Emily Shaffer"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"458541","messageId":"CAP4e5TNdvE1gmt9bERT5sL+D3r4Fbypk57KJJ+wYHD=M=T+uWA@mail.gmail.com","threadId":"58109","inReplyTo":null,"subject":"bug report: pre-commit & pre-push hook output is redirected differently","fromName":"Adam Zethraeus","fromEmail":"adam.zethraeus@includedhealth.com","sentAt":"2022-07-06T20:40:45Z","receivedAt":"2022-07-06T20:41:06Z","isPatch":false,"sender":{"key":"adam.zethraeus@includedhealth.com","avatar":null},"body":"What did you do before the bug happened? (Steps to reproduce your issue)\n\nInstalled identical pre-commit and pre-push hooks:\n\n```\n#!/bin/bash\n\n>&1 echo \"stdout\"\n>&2 echo \"stderr\"\nexit 1\n```\n\nWhat did you expect to happen? (Expected behavior)\n\n`git push` and `git commit` should have the same hook behavior.\n\nWhat happened instead? (Actual behavior)\n\nThe pre-commit hook was run with stdout redirected to stderr but the\npre-push hook's output was unaltered.\n\n```\n> git commit -m \"-\" 1>/dev/null\nstdout\nstderr\n> git commit -m \"-\" 2>/dev/null\n> git commit -m \"-\" --no-verify\n> git push 1>/dev/null\nstderr\nerror: failed to push some refs to 'github.com:org/repo.git'\n> git push 2>/dev/null\nstdout\n```\n\nWhat's different between what you expected and what actually happened?\n\nThere was inconsistency in the behavior when I expected consistency.\n\nAnything else you want to add:\n\n```\n> /bin/bash --version\nGNU bash, version 3.2.57(1)-release (x86_64-apple-darwin21)\n```\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.37.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nfeature: fsmonitor--daemon\nuname: Darwin 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:22\nPDT 2022; root:xnu-8020.121.3~4/RELEASE_X86_64 x86_64\ncompiler info: clang: 13.1.6 (clang-1316.0.21.2.5)\nlibc info: no libc information available\n$SHELL (typically, interactive shell): /bin/zsh\n\n\n[Enabled Hooks]\npre-commit\npre-push\n"},{"id":"458542","messageId":"xmqqa69mgdde.fsf@gitster.g","threadId":"58109","inReplyTo":"CAP4e5TNdvE1gmt9bERT5sL+D3r4Fbypk57KJJ+wYHD=M=T+uWA@mail.gmail.com","subject":"Re: bug report: pre-commit & pre-push hook output is redirected differently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-06T21:06:21Z","receivedAt":"2022-07-06T21:06:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Zethraeus <adam.zethraeus@includedhealth.com> writes:\n\n> What did you do before the bug happened? (Steps to reproduce your issue)\n>\n> Installed identical pre-commit and pre-push hooks:\n>\n> ```\n> #!/bin/bash\n>\n>>&1 echo \"stdout\"\n>>&2 echo \"stderr\"\n> exit 1\n> ```\n>\n> What did you expect to happen? (Expected behavior)\n>\n> `git push` and `git commit` should have the same hook behavior.\n>\n> What happened instead? (Actual behavior)\n>\n> The pre-commit hook was run with stdout redirected to stderr but the\n> pre-push hook's output was unaltered.\n\nWithout looking into it very much, the output of hooks is an area\nwith known regression at 2.36, so let me redirect it to those who\nare likely to know it ;-)\n\nThanks for a report.\n\n>\n> ```\n>> git commit -m \"-\" 1>/dev/null\n> stdout\n> stderr\n>> git commit -m \"-\" 2>/dev/null\n>> git commit -m \"-\" --no-verify\n>> git push 1>/dev/null\n> stderr\n> error: failed to push some refs to 'github.com:org/repo.git'\n>> git push 2>/dev/null\n> stdout\n> ```\n>\n> What's different between what you expected and what actually happened?\n>\n> There was inconsistency in the behavior when I expected consistency.\n>\n> Anything else you want to add:\n>\n> ```\n>> /bin/bash --version\n> GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin21)\n> ```\n>\n> Please review the rest of the bug report below.\n> You can delete any lines you don't wish to share.\n>\n>\n> [System Info]\n> git version:\n> git version 2.37.0\n> cpu: x86_64\n> no commit associated with this build\n> sizeof-long: 8\n> sizeof-size_t: 8\n> shell-path: /bin/sh\n> feature: fsmonitor--daemon\n> uname: Darwin 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:22\n> PDT 2022; root:xnu-8020.121.3~4/RELEASE_X86_64 x86_64\n> compiler info: clang: 13.1.6 (clang-1316.0.21.2.5)\n> libc info: no libc information available\n> $SHELL (typically, interactive shell): /bin/zsh\n>\n>\n> [Enabled Hooks]\n> pre-commit\n> pre-push\n"},{"id":"458563","messageId":"220707.86tu7t84zh.gmgdl@evledraar.gmail.com","threadId":"58109","inReplyTo":"xmqqa69mgdde.fsf@gitster.g","subject":"Re: bug report: pre-commit & pre-push hook output is redirected differently","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-07T12:40:49Z","receivedAt":"2022-07-07T12:47:08Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jul 06 2022, Junio C Hamano wrote:\n\n> Adam Zethraeus <adam.zethraeus@includedhealth.com> writes:\n>\n>> What did you do before the bug happened? (Steps to reproduce your issue)\n>>\n>> Installed identical pre-commit and pre-push hooks:\n>>\n>> ```\n>> #!/bin/bash\n>>\n>>>&1 echo \"stdout\"\n>>>&2 echo \"stderr\"\n>> exit 1\n>> ```\n>>\n>> What did you expect to happen? (Expected behavior)\n>>\n>> `git push` and `git commit` should have the same hook behavior.\n>>\n>> What happened instead? (Actual behavior)\n>>\n>> The pre-commit hook was run with stdout redirected to stderr but the\n>> pre-push hook's output was unaltered.\n>\n> Without looking into it very much, the output of hooks is an area\n> with known regression at 2.36, so let me redirect it to those who\n> are likely to know it ;-)\n>\n> Thanks for a report.\n\nI may be missing something, but I think this report has nothing to do\nwith any recent changes or regressions, but is merely noting a behavior\nchange between pre-push and some other hooks that we've had since 1.8.2,\nor since the \"pre-push\" hook was added in ec55559f937 (push: Add support\nfor pre-push hooks, 2013-01-13).\n\nI tested this with a local v2.30.0, and the behavior was the same.\n\nThis will \"fix\" it:\n\t\n\tdiff --git a/transport.c b/transport.c\n\tindex 52db7a3cb09..0cc7d05e0da 100644\n\t--- a/transport.c\n\t+++ b/transport.c\n\t@@ -1225,6 +1225,7 @@ static int run_pre_push_hook(struct transport *transport,\n\t \tstrvec_push(&proc.args, transport->url);\n\t \n\t \tproc.in = -1;\n\t+\tproc.stdout_to_stderr = 1;\n\t \tproc.trace2_hook_name = \"pre-push\";\n\t \n\t \tif (start_command(&proc)) {\n\nBut whether that's a fix or not depends on whether we think we should\nmake this behavior consistent. I tend to think so, but it would be a\nbehavior change to long-established behavior in pre-push.\n\nIt *is* something we need to be careful of when converting the rest of\nthe hooks to the hooks API, i.e. we need tests for how stderr/stdout is\nhandled for each one.\n\nBut this being different is just because some hook use the hook.c API\n(and before that the helper in run-command.c), and others use \"struct\nchild_process\" or whatever explicitly (such as \"pre-push\").\n\nSince it's up to each callsite to set up the \"proc\" (or equivalent) some\nsupply \"stdout_to_stderr\", some don't.\n\nFrom some quick grepping it seems the odd ones out are pre-push and\nproc-receive, but I only skimmed a \"git grep\" to find the second one,\nand may have missed others.\n"},{"id":"458577","messageId":"xmqq4jzs3lp1.fsf@gitster.g","threadId":"58109","inReplyTo":"220707.86tu7t84zh.gmgdl@evledraar.gmail.com","subject":"Re: bug report: pre-commit & pre-push hook output is redirected differently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-07T16:57:14Z","receivedAt":"2022-07-07T16:57:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> I may be missing something, but I think this report has nothing to do\n> with any recent changes or regressions, but is merely noting a behavior\n> change between pre-push and some other hooks that we've had since 1.8.2,\n> or since the \"pre-push\" hook was added in ec55559f937 (push: Add support\n> for pre-push hooks, 2013-01-13).\n\n\"behavior change\" meaning a regression of a particular hook, or\n\"behavior difference\" between hooks, each of which never changed the\nbehavior?\n\n> I tested this with a local v2.30.0, and the behavior was the same.\n\nI guess you meant the latter.  If so, the inconsistency may be\nunfortunate, but I agree that it is not cut-and-dried that it is a\ngood idea to change pre-push to spew its output to standard error\nstream.\n\n> It *is* something we need to be careful of when converting the rest of\n> the hooks to the hooks API, i.e. we need tests for how stderr/stdout is\n> handled for each one.\n\nAbsolutely.\n\n> But this being different is just because some hook use the hook.c API\n> (and before that the helper in run-command.c), and others use \"struct\n> child_process\" or whatever explicitly (such as \"pre-push\").\n>\n> Since it's up to each callsite to set up the \"proc\" (or equivalent) some\n> supply \"stdout_to_stderr\", some don't.\n>\n> From some quick grepping it seems the odd ones out are pre-push and\n> proc-receive, but I only skimmed a \"git grep\" to find the second one,\n> and may have missed others.\n\nWe'd probably need an inventory of them all anyway before we can\npush the hook.c API forward.  If the oddball ones are very small\nminority, it may be worth having a transition period and make\nbackward incompatible change to unify where the output goes.  If\nthey are random mixture, on the other hand, the hook.c API may have\nto gain a bit for the caller to tell where the output of the hook\nshould go.\n\nThanks.\n\n"},{"id":"458602","messageId":"CAJoAoZnpryUpOVHQ8sCkkF19so=OjS+B46UB1MwN=wmLqqn=Pw@mail.gmail.com","threadId":"58109","inReplyTo":"xmqq4jzs3lp1.fsf@gitster.g","subject":"Re: bug report: pre-commit & pre-push hook output is redirected differently","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2022-07-07T20:55:43Z","receivedAt":"2022-07-07T20:55:59Z","isPatch":false,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Jul 7, 2022 at 9:57 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n> > I may be missing something, but I think this report has nothing to do\n> > with any recent changes or regressions, but is merely noting a behavior\n> > change between pre-push and some other hooks that we've had since 1.8.2,\n> > or since the \"pre-push\" hook was added in ec55559f937 (push: Add support\n> > for pre-push hooks, 2013-01-13).\n>\n> \"behavior change\" meaning a regression of a particular hook, or\n> \"behavior difference\" between hooks, each of which never changed the\n> behavior?\n>\n> > I tested this with a local v2.30.0, and the behavior was the same.\n>\n> I guess you meant the latter.  If so, the inconsistency may be\n> unfortunate, but I agree that it is not cut-and-dried that it is a\n> good idea to change pre-push to spew its output to standard error\n> stream.\n>\n> > It *is* something we need to be careful of when converting the rest of\n> > the hooks to the hooks API, i.e. we need tests for how stderr/stdout is\n> > handled for each one.\n>\n> Absolutely.\n\nOn the one hand, we just had a particularly frustrating regression\naround hook output direction, and it's likely none of us are\ninterested in repeating such pain anytime soon.\n\nOn the other hand, I wonder how worried we actually should be about\nchanging the behavior to normalize pre-push and other \"oddball\" hooks.\nIf the pipe used changes, what changes for the hook author? What about\nfor the person running the hook on their repository?\n\nFor the hook author, there is no change, in that the hook author\ncannot control which pipe we decide to send their output to, and\nshould have written their hook with that in mind in the first place. I\nfound one example[1] of a hook which manually sends its only output to\nstderr, everything else I saw just 'echo'es with abandon. I did notice\nmany hooks relying on colored output, but we've fixed that bug ;)\n\nFor the hook runner, the main difference has to do with parsing output\nfor scripts. That is, if I'm just looking with my human eyes, I don't\ncare which pipe is used, as long as it comes to the terminal. Of\ncourse for scripting that is a different story, and if output I was\nexpecting on a certain pipe suddenly disappears (or vice versa) that\nis a pain. However, I don't see much evidence that hook output can be\nused in that way. In fact, I think that is the reason we shunt most\nhook output to stderr; Git typically says \"stdout is what you should\nparse with your script, and stderr is what humans should look at with\ntheir eyeballs.\" Through the very scientific method of looking at the\nfirst 10 pages of GitHub search results[2], I didn't find any evidence\nthat pre-push hooks are being written with the intent for their output\nto be parsed. That makes sense to me - I'd need to carefully\nsynchronize the output from my pre-push hook *and* the parser in my\nscript, and at that point it makes more sense to just teach the\npre-push hook to take whatever action it's trying to communicate to\nthe parser and cut out the interprocess communication, right?\n\nAnyway, tl;dr - I think it would actually be a good and reasonable\nthing to normalize the pre-push hook, either now or at the point where\nit's converted to hook.c.\n\nAs for proc-receive, I think that one does something weird - the\nhook's output is intended to be parsed by Git itself, and isn't shown\nto the user. proc-receive is in fact such a weird and different hook\nthat it is completely exempt from the migration to hook.c. It does\nbidirectional communication with the Git parent process and the parent\nprocess decides how to act based on that communication, which means\nthat even if run_processes_parallel could support that (it doesn't) it\nwouldn't make sense to configure more than one proc-receive hook\nanyways. So proc-receive is intended to continue being an oddball and\nnot use the hook.c library (other than to check for the existence of 0\nor 1 proc-receive hooks).\n\n - Emily\n\n1: https://github.com/Kaliraj-hesabe/hesabe-reactnative-app/blob/3526fbe3fed8f08dadf10483c1292b8cd8f7d4ed/node_modules/realm/vendor/realm-core/tools/pre-push\n2: https://github.com/search?l=&p=9&q=filename%3Apre-push+-filename%3A%2A.sample&type=Code\n"}]}