* bug report: pre-commit & pre-push hook output is redirected differently @ 2022-07-06 20:40 Adam Zethraeus 2022-07-06 21:06 ` Junio C Hamano 0 siblings, 1 reply; 5+ messages in thread From: Adam Zethraeus @ 2022-07-06 20:40 UTC (permalink / raw) To: git What did you do before the bug happened? (Steps to reproduce your issue) Installed identical pre-commit and pre-push hooks: ``` #!/bin/bash >&1 echo "stdout" >&2 echo "stderr" exit 1 ``` What did you expect to happen? (Expected behavior) `git push` and `git commit` should have the same hook behavior. What happened instead? (Actual behavior) The pre-commit hook was run with stdout redirected to stderr but the pre-push hook's output was unaltered. ``` > git commit -m "-" 1>/dev/null stdout stderr > git commit -m "-" 2>/dev/null > git commit -m "-" --no-verify > git push 1>/dev/null stderr error: failed to push some refs to 'github.com:org/repo.git' > git push 2>/dev/null stdout ``` What's different between what you expected and what actually happened? There was inconsistency in the behavior when I expected consistency. Anything else you want to add: ``` > /bin/bash --version GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin21) ``` Please review the rest of the bug report below. You can delete any lines you don't wish to share. [System Info] git version: git version 2.37.0 cpu: x86_64 no commit associated with this build sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh feature: fsmonitor--daemon uname: Darwin 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:22 PDT 2022; root:xnu-8020.121.3~4/RELEASE_X86_64 x86_64 compiler info: clang: 13.1.6 (clang-1316.0.21.2.5) libc info: no libc information available $SHELL (typically, interactive shell): /bin/zsh [Enabled Hooks] pre-commit pre-push ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: bug report: pre-commit & pre-push hook output is redirected differently 2022-07-06 20:40 bug report: pre-commit & pre-push hook output is redirected differently Adam Zethraeus @ 2022-07-06 21:06 ` Junio C Hamano 2022-07-07 12:40 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 5+ messages in thread From: Junio C Hamano @ 2022-07-06 21:06 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, Glen Choo; +Cc: git, Adam Zethraeus Adam Zethraeus <adam.zethraeus@includedhealth.com> writes: > What did you do before the bug happened? (Steps to reproduce your issue) > > Installed identical pre-commit and pre-push hooks: > > ``` > #!/bin/bash > >>&1 echo "stdout" >>&2 echo "stderr" > exit 1 > ``` > > What did you expect to happen? (Expected behavior) > > `git push` and `git commit` should have the same hook behavior. > > What happened instead? (Actual behavior) > > The pre-commit hook was run with stdout redirected to stderr but the > pre-push hook's output was unaltered. Without looking into it very much, the output of hooks is an area with known regression at 2.36, so let me redirect it to those who are likely to know it ;-) Thanks for a report. > > ``` >> git commit -m "-" 1>/dev/null > stdout > stderr >> git commit -m "-" 2>/dev/null >> git commit -m "-" --no-verify >> git push 1>/dev/null > stderr > error: failed to push some refs to 'github.com:org/repo.git' >> git push 2>/dev/null > stdout > ``` > > What's different between what you expected and what actually happened? > > There was inconsistency in the behavior when I expected consistency. > > Anything else you want to add: > > ``` >> /bin/bash --version > GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin21) > ``` > > Please review the rest of the bug report below. > You can delete any lines you don't wish to share. > > > [System Info] > git version: > git version 2.37.0 > cpu: x86_64 > no commit associated with this build > sizeof-long: 8 > sizeof-size_t: 8 > shell-path: /bin/sh > feature: fsmonitor--daemon > uname: Darwin 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:22 > PDT 2022; root:xnu-8020.121.3~4/RELEASE_X86_64 x86_64 > compiler info: clang: 13.1.6 (clang-1316.0.21.2.5) > libc info: no libc information available > $SHELL (typically, interactive shell): /bin/zsh > > > [Enabled Hooks] > pre-commit > pre-push ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: bug report: pre-commit & pre-push hook output is redirected differently 2022-07-06 21:06 ` Junio C Hamano @ 2022-07-07 12:40 ` Ævar Arnfjörð Bjarmason 2022-07-07 16:57 ` Junio C Hamano 0 siblings, 1 reply; 5+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-07-07 12:40 UTC (permalink / raw) To: Junio C Hamano; +Cc: Glen Choo, git, Adam Zethraeus On Wed, Jul 06 2022, Junio C Hamano wrote: > Adam Zethraeus <adam.zethraeus@includedhealth.com> writes: > >> What did you do before the bug happened? (Steps to reproduce your issue) >> >> Installed identical pre-commit and pre-push hooks: >> >> ``` >> #!/bin/bash >> >>>&1 echo "stdout" >>>&2 echo "stderr" >> exit 1 >> ``` >> >> What did you expect to happen? (Expected behavior) >> >> `git push` and `git commit` should have the same hook behavior. >> >> What happened instead? (Actual behavior) >> >> The pre-commit hook was run with stdout redirected to stderr but the >> pre-push hook's output was unaltered. > > Without looking into it very much, the output of hooks is an area > with known regression at 2.36, so let me redirect it to those who > are likely to know it ;-) > > Thanks for a report. I may be missing something, but I think this report has nothing to do with any recent changes or regressions, but is merely noting a behavior change between pre-push and some other hooks that we've had since 1.8.2, or since the "pre-push" hook was added in ec55559f937 (push: Add support for pre-push hooks, 2013-01-13). I tested this with a local v2.30.0, and the behavior was the same. This will "fix" it: diff --git a/transport.c b/transport.c index 52db7a3cb09..0cc7d05e0da 100644 --- a/transport.c +++ b/transport.c @@ -1225,6 +1225,7 @@ static int run_pre_push_hook(struct transport *transport, strvec_push(&proc.args, transport->url); proc.in = -1; + proc.stdout_to_stderr = 1; proc.trace2_hook_name = "pre-push"; if (start_command(&proc)) { But whether that's a fix or not depends on whether we think we should make this behavior consistent. I tend to think so, but it would be a behavior change to long-established behavior in pre-push. It *is* something we need to be careful of when converting the rest of the hooks to the hooks API, i.e. we need tests for how stderr/stdout is handled for each one. But this being different is just because some hook use the hook.c API (and before that the helper in run-command.c), and others use "struct child_process" or whatever explicitly (such as "pre-push"). Since it's up to each callsite to set up the "proc" (or equivalent) some supply "stdout_to_stderr", some don't. From some quick grepping it seems the odd ones out are pre-push and proc-receive, but I only skimmed a "git grep" to find the second one, and may have missed others. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: bug report: pre-commit & pre-push hook output is redirected differently 2022-07-07 12:40 ` Ævar Arnfjörð Bjarmason @ 2022-07-07 16:57 ` Junio C Hamano 2022-07-07 20:55 ` Emily Shaffer 0 siblings, 1 reply; 5+ messages in thread From: Junio C Hamano @ 2022-07-07 16:57 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Glen Choo, git, Adam Zethraeus Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I may be missing something, but I think this report has nothing to do > with any recent changes or regressions, but is merely noting a behavior > change between pre-push and some other hooks that we've had since 1.8.2, > or since the "pre-push" hook was added in ec55559f937 (push: Add support > for pre-push hooks, 2013-01-13). "behavior change" meaning a regression of a particular hook, or "behavior difference" between hooks, each of which never changed the behavior? > I tested this with a local v2.30.0, and the behavior was the same. I guess you meant the latter. If so, the inconsistency may be unfortunate, but I agree that it is not cut-and-dried that it is a good idea to change pre-push to spew its output to standard error stream. > It *is* something we need to be careful of when converting the rest of > the hooks to the hooks API, i.e. we need tests for how stderr/stdout is > handled for each one. Absolutely. > But this being different is just because some hook use the hook.c API > (and before that the helper in run-command.c), and others use "struct > child_process" or whatever explicitly (such as "pre-push"). > > Since it's up to each callsite to set up the "proc" (or equivalent) some > supply "stdout_to_stderr", some don't. > > From some quick grepping it seems the odd ones out are pre-push and > proc-receive, but I only skimmed a "git grep" to find the second one, > and may have missed others. We'd probably need an inventory of them all anyway before we can push the hook.c API forward. If the oddball ones are very small minority, it may be worth having a transition period and make backward incompatible change to unify where the output goes. If they are random mixture, on the other hand, the hook.c API may have to gain a bit for the caller to tell where the output of the hook should go. Thanks. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: bug report: pre-commit & pre-push hook output is redirected differently 2022-07-07 16:57 ` Junio C Hamano @ 2022-07-07 20:55 ` Emily Shaffer 0 siblings, 0 replies; 5+ messages in thread From: Emily Shaffer @ 2022-07-07 20:55 UTC (permalink / raw) To: Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, Glen Choo, Git List, Adam Zethraeus On Thu, Jul 7, 2022 at 9:57 AM Junio C Hamano <gitster@pobox.com> wrote: > > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > > I may be missing something, but I think this report has nothing to do > > with any recent changes or regressions, but is merely noting a behavior > > change between pre-push and some other hooks that we've had since 1.8.2, > > or since the "pre-push" hook was added in ec55559f937 (push: Add support > > for pre-push hooks, 2013-01-13). > > "behavior change" meaning a regression of a particular hook, or > "behavior difference" between hooks, each of which never changed the > behavior? > > > I tested this with a local v2.30.0, and the behavior was the same. > > I guess you meant the latter. If so, the inconsistency may be > unfortunate, but I agree that it is not cut-and-dried that it is a > good idea to change pre-push to spew its output to standard error > stream. > > > It *is* something we need to be careful of when converting the rest of > > the hooks to the hooks API, i.e. we need tests for how stderr/stdout is > > handled for each one. > > Absolutely. On the one hand, we just had a particularly frustrating regression around hook output direction, and it's likely none of us are interested in repeating such pain anytime soon. On the other hand, I wonder how worried we actually should be about changing the behavior to normalize pre-push and other "oddball" hooks. If the pipe used changes, what changes for the hook author? What about for the person running the hook on their repository? For the hook author, there is no change, in that the hook author cannot control which pipe we decide to send their output to, and should have written their hook with that in mind in the first place. I found one example[1] of a hook which manually sends its only output to stderr, everything else I saw just 'echo'es with abandon. I did notice many hooks relying on colored output, but we've fixed that bug ;) For the hook runner, the main difference has to do with parsing output for scripts. That is, if I'm just looking with my human eyes, I don't care which pipe is used, as long as it comes to the terminal. Of course for scripting that is a different story, and if output I was expecting on a certain pipe suddenly disappears (or vice versa) that is a pain. However, I don't see much evidence that hook output can be used in that way. In fact, I think that is the reason we shunt most hook output to stderr; Git typically says "stdout is what you should parse with your script, and stderr is what humans should look at with their eyeballs." Through the very scientific method of looking at the first 10 pages of GitHub search results[2], I didn't find any evidence that pre-push hooks are being written with the intent for their output to be parsed. That makes sense to me - I'd need to carefully synchronize the output from my pre-push hook *and* the parser in my script, and at that point it makes more sense to just teach the pre-push hook to take whatever action it's trying to communicate to the parser and cut out the interprocess communication, right? Anyway, tl;dr - I think it would actually be a good and reasonable thing to normalize the pre-push hook, either now or at the point where it's converted to hook.c. As for proc-receive, I think that one does something weird - the hook's output is intended to be parsed by Git itself, and isn't shown to the user. proc-receive is in fact such a weird and different hook that it is completely exempt from the migration to hook.c. It does bidirectional communication with the Git parent process and the parent process decides how to act based on that communication, which means that even if run_processes_parallel could support that (it doesn't) it wouldn't make sense to configure more than one proc-receive hook anyways. So proc-receive is intended to continue being an oddball and not use the hook.c library (other than to check for the existence of 0 or 1 proc-receive hooks). - Emily 1: https://github.com/Kaliraj-hesabe/hesabe-reactnative-app/blob/3526fbe3fed8f08dadf10483c1292b8cd8f7d4ed/node_modules/realm/vendor/realm-core/tools/pre-push 2: https://github.com/search?l=&p=9&q=filename%3Apre-push+-filename%3A%2A.sample&type=Code ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-07 20:55 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-06 20:40 bug report: pre-commit & pre-push hook output is redirected differently Adam Zethraeus 2022-07-06 21:06 ` Junio C Hamano 2022-07-07 12:40 ` Ævar Arnfjörð Bjarmason 2022-07-07 16:57 ` Junio C Hamano 2022-07-07 20:55 ` Emily Shaffer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).