{"thread":{"id":"54992","subject":"git fails with a broken pipe when one quits the pager","startedAt":"2021-01-15T16:19:47Z","lastAt":"2021-02-05T11:40:42Z","messageCount":60,"participants":["Vincent Lefevre","Denton Liu","Johannes Sixt","Ævar Arnfjörð Bjarmason","Chris Torek","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"414456","messageId":"YAG/vzctP4JwSp5x@zira.vinc17.org","threadId":"54992","inReplyTo":null,"subject":"git fails with a broken pipe when one quits the pager","fromName":"Vincent Lefevre","fromEmail":"vincent@vinc17.net","sentAt":"2021-01-15T16:15:59Z","receivedAt":"2021-01-15T16:19:47Z","isPatch":false,"sender":{"key":"vincent@vinc17.net","avatar":null},"body":"I had reported the following bug at\n https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914896\n\nIt still occurs with Git 2.30.0.\n\nSome git commands with a lot of output fail with a broken pipe when\none quits the pager (without going to the end of the output).\n\nFor instance, in zsh:\n\ncventin% setopt PRINT_EXIT_VALUE\ncventin% git log\nzsh: broken pipe git log\ncventin% echo $?\n141\ncventin% \n\nThis is annoying. And of course, I don't want to hide error messages\nby default, because this would hide *real* errors.\n\nThe broken pipe is internally expected, thus should not be reported\nby git.\n\nJust to be clear: this broken pipe should be discarded only when git\nuses its builtin pager feature, not with a general pipe, where the\nerror may be important.\n\nFor instance,\n\n$ { git log ; echo \"Exit status: $?\" >&2 ; } | true\n\nshould still output\n\nExit status: 141\n\nlike currently.\n\n-- \nVincent Lefèvre - Web: \n100% accessible validated (X)HTML - Blog: \nWork: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)\n"},{"id":"415618","messageId":"bc88492979fee215d5be06ccbc246ae0171a9ced.1611910122.git.liu.denton@gmail.com","threadId":"54992","inReplyTo":"YAG/vzctP4JwSp5x@zira.vinc17.org","subject":"[PATCH] pager: exit without error on SIGPIPE","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2021-01-29T23:48:54Z","receivedAt":"2021-01-29T23:50:03Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"If the pager closes before the git command feeding the pager finishes,\ngit is killed by a SIGPIPE and the corresponding exit code is 141.\nSince the pipe is just an implementation detail, it does not make sense\nfor this error code to be user-facing.\n\nHandle SIGPIPEs by simply calling exit(0) in wait_for_pager_signal().\n\nIntroduce `test-tool pager` which infinitely prints `y` to the pager in\norder to test the new behavior. This cannot be tested with any existing\ngit command because there are no other commands which produce infinite\noutput. Without the change to pager.c, the newly introduced test fails.\n\nReported-by: Vincent Lefevre \nSigned-off-by: Denton Liu \n---\nSorry for the resend, it seems like vger has dropped the first patch.\n\n Makefile | 1 +\n pager.c | 2 ++\n t/helper/test-pager.c | 12 ++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t7006-pager.sh | 4 ++++\n 6 files changed, 21 insertions(+)\n create mode 100644 t/helper/test-pager.c\n\ndiff --git a/Makefile b/Makefile\nindex 4edfda3e00..38a1a20f31 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -719,6 +719,7 @@ TEST_BUILTINS_OBJS += test-mktemp.o\n TEST_BUILTINS_OBJS += test-oid-array.o\n TEST_BUILTINS_OBJS += test-oidmap.o\n TEST_BUILTINS_OBJS += test-online-cpus.o\n+TEST_BUILTINS_OBJS += test-pager.o\n TEST_BUILTINS_OBJS += test-parse-options.o\n TEST_BUILTINS_OBJS += test-parse-pathspec-file.o\n TEST_BUILTINS_OBJS += test-path-utils.o\ndiff --git a/pager.c b/pager.c\nindex ee435de675..5922d99dc8 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -34,6 +34,8 @@ static void wait_for_pager_atexit(void)\n static void wait_for_pager_signal(int signo)\n {\n \twait_for_pager(1);\n+\tif (signo == SIGPIPE)\n+\t\texit(0);\n \tsigchain_pop(signo);\n \traise(signo);\n }\ndiff --git a/t/helper/test-pager.c b/t/helper/test-pager.c\nnew file mode 100644\nindex 0000000000..feb68b8643\n--- /dev/null\n+++ b/t/helper/test-pager.c\n@@ -0,0 +1,12 @@\n+#include \"test-tool.h\"\n+#include \"cache.h\"\n+\n+int cmd__pager(int argc, const char **argv)\n+{\n+\tif (argc > 1)\n+\t\tusage(\"\\ttest-tool pager\");\n+\n+\tsetup_pager();\n+\tfor (;;)\n+\t\tputs(\"y\");\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 9d6d14d929..88269a7156 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -43,6 +43,7 @@ static struct test_cmd cmds[] = {\n \t{ \"oid-array\", cmd__oid_array },\n \t{ \"oidmap\", cmd__oidmap },\n \t{ \"online-cpus\", cmd__online_cpus },\n+\t{ \"pager\", cmd__pager },\n \t{ \"parse-options\", cmd__parse_options },\n \t{ \"parse-pathspec-file\", cmd__parse_pathspec_file },\n \t{ \"path-utils\", cmd__path_utils },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex a6470ff62c..78900f7938 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -32,6 +32,7 @@ int cmd__mergesort(int argc, const char **argv);\n int cmd__mktemp(int argc, const char **argv);\n int cmd__oidmap(int argc, const char **argv);\n int cmd__online_cpus(int argc, const char **argv);\n+int cmd__pager(int argc, const char **argv);\n int cmd__parse_options(int argc, const char **argv);\n int cmd__parse_pathspec_file(int argc, const char** argv);\n int cmd__path_utils(int argc, const char **argv);\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex fdb450e446..2eb89e8f75 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -656,4 +656,8 @@ test_expect_success TTY 'git tag with auto-columns ' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success TTY 'SIGPIPE from pager returns success' '\n+\ttest_terminal env PAGER=true test-tool pager\n+'\n+\n test_done\n-- \n2.30.0.478.g8a0d178c01\n\n"},{"id":"415646","messageId":"a3e738e2-695e-cf01-5d01-50b6fea272ec@kdbg.org","threadId":"54992","inReplyTo":"bc88492979fee215d5be06ccbc246ae0171a9ced.1611910122.git.liu.denton@gmail.com","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-01-30T08:29:18Z","receivedAt":"2021-01-30T09:29:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 30.01.21 um 00:48 schrieb Denton Liu:\n> If the pager closes before the git command feeding the pager finishes,\n> git is killed by a SIGPIPE and the corresponding exit code is 141.\n> Since the pipe is just an implementation detail, it does not make sense\n> for this error code to be user-facing.\n> \n> Handle SIGPIPEs by simply calling exit(0) in wait_for_pager_signal().\n> \n> Introduce `test-tool pager` which infinitely prints `y` to the pager in\n> order to test the new behavior. This cannot be tested with any existing\n> git command because there are no other commands which produce infinite\n> output. Without the change to pager.c, the newly introduced test fails.\n> \n> Reported-by: Vincent Lefevre \n> Signed-off-by: Denton Liu \n\n...\n\n> diff --git a/pager.c b/pager.c\n> index ee435de675..5922d99dc8 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -34,6 +34,8 @@ static void wait_for_pager_atexit(void)\n> static void wait_for_pager_signal(int signo)\n> {\n> \twait_for_pager(1);\n> +\tif (signo == SIGPIPE)\n> +\t\texit(0);\n> \tsigchain_pop(signo);\n> \traise(signo);\n> }\n> diff --git a/t/helper/test-pager.c b/t/helper/test-pager.c\n> new file mode 100644\n> index 0000000000..feb68b8643\n> --- /dev/null\n> +++ b/t/helper/test-pager.c\n> @@ -0,0 +1,12 @@\n> +#include \"test-tool.h\"\n> +#include \"cache.h\"\n> +\n> +int cmd__pager(int argc, const char **argv)\n> +{\n> +\tif (argc > 1)\n> +\t\tusage(\"\\ttest-tool pager\");\n> +\n> +\tsetup_pager();\n> +\tfor (;;)\n> +\t\tputs(\"y\");\n> +}\n\nMy gut feeling tells that this will end in an infinite loop on Windows.\nThere are no signals on Windows that would kill the upstream of a pipe.\nThis call site will only notice that the downstream of the pipe was\nclosed, when it checks for write errors.\n\nLet me test it.\n\n-- Hannes\n"},{"id":"415653","messageId":"869ba84b-a008-6061-be57-50ab678a154e@kdbg.org","threadId":"54992","inReplyTo":"a3e738e2-695e-cf01-5d01-50b6fea272ec@kdbg.org","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-01-30T12:52:40Z","receivedAt":"2021-01-30T12:53:41Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 30.01.21 um 09:29 schrieb Johannes Sixt:\n> Am 30.01.21 um 00:48 schrieb Denton Liu:\n>> +++ b/t/helper/test-pager.c\n>> @@ -0,0 +1,12 @@\n>> +#include \"test-tool.h\"\n>> +#include \"cache.h\"\n>> +\n>> +int cmd__pager(int argc, const char **argv)\n>> +{\n>> +\tif (argc > 1)\n>> +\t\tusage(\"\\ttest-tool pager\");\n>> +\n>> +\tsetup_pager();\n>> +\tfor (;;)\n>> +\t\tputs(\"y\");\n>> +}\n> \n> My gut feeling tells that this will end in an infinite loop on Windows.\n> There are no signals on Windows that would kill the upstream of a pipe.\n> This call site will only notice that the downstream of the pipe was\n> closed, when it checks for write errors.\n> \n> Let me test it.\n\nThe test case is protected by a TTY prerequisite; that is not satisfied\non Windows, and the test is skipped. No harm done so far.\n\nBut when I run `test-tool pager` manually and quit out of the pager, the\ntool does spin in the endless loop. The following fixup helps.\n\n\ndiff --git a/t/helper/test-pager.c b/t/helper/test-pager.c\nindex feb68b8643..5f1982411f 100644\n--- a/t/helper/test-pager.c\n+++ b/t/helper/test-pager.c\n@@ -7,6 +7,8 @@ int cmd__pager(int argc, const char **argv)\n \t\tusage(\"\\ttest-tool pager\");\n \n \tsetup_pager();\n-\tfor (;;)\n-\t\tputs(\"y\");\n+\twhile (write_in_full(1, \"y\\n\", 2) > 0)\n+\t\t;\n+\n+\treturn 0;\n }\n-- \n2.30.0.119.g680bcb97f5\n"},{"id":"415675","messageId":"8735yhq3lc.fsf@evledraar.gmail.com","threadId":"54992","inReplyTo":"YAG/vzctP4JwSp5x@zira.vinc17.org","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-31T01:47:59Z","receivedAt":"2021-01-31T01:48:45Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jan 15 2021, Vincent Lefevre wrote:\n\n> I had reported the following bug at\n> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914896\n>\n> It still occurs with Git 2.30.0.\n>\n> Some git commands with a lot of output fail with a broken pipe when\n> one quits the pager (without going to the end of the output).\n>\n> For instance, in zsh:\n>\n> cventin% setopt PRINT_EXIT_VALUE\n> cventin% git log\n> zsh: broken pipe git log\n> cventin% echo $?\n> 141\n> cventin% \n>\n> This is annoying[...]\n\nYes it's annoying, but the annoying output is from zsh, not\ngit. Consider a smarter implementation like:\n\n case $__exit_status in\n 0) __exit_emoji=😀;;\n 1) __exit_emoji=☹️ ;;\n 141) __exit_emoji=🤕 ;;\n [...]\n\nThen put the $__exit_emoji in your $PS1 prompt, now when you 'q' in a\npager you know the difference between having quit at the full output\nbeing emitted or not.\n\n> And of course, I don't want to hide error messages by default, because\n> this would hide *real* errors.\n\nIsn't the solution to this that your shell stops reporting failures due\nto SIGPIPE in such a prominent way then?\n\n> The broken pipe is internally expected, thus should not be reported\n> by git.\n>\n> Just to be clear: this broken pipe should be discarded only when git\n> uses its builtin pager feature, not with a general pipe, where the\n> error may be important.\n>\n> For instance,\n>\n> $ { git log ; echo \"Exit status: $?\" >&2 ; } | true\n>\n> should still output\n>\n> Exit status: 141\n\nI don't get it, how is it less meaningful when git itself invokes the\npager?\n\nIn both cases the exit code means the same thing, that something in a\npipe wasn't fully consumed being signalled to calling processes is the\npoint of SIGPIPE.\n"},{"id":"415679","messageId":"20210131033652.GK623063@zira.vinc17.org","threadId":"54992","inReplyTo":"8735yhq3lc.fsf@evledraar.gmail.com","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Vincent Lefevre","fromEmail":"vincent@vinc17.net","sentAt":"2021-01-31T03:36:52Z","receivedAt":"2021-01-31T03:38:39Z","isPatch":false,"sender":{"key":"vincent@vinc17.net","avatar":null},"body":"On 2021-01-31 02:47:59 +0100, Ævar Arnfjörð Bjarmason wrote:\n> On Fri, Jan 15 2021, Vincent Lefevre wrote:\n> > I had reported the following bug at\n> > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914896\n> >\n> > It still occurs with Git 2.30.0.\n> >\n> > Some git commands with a lot of output fail with a broken pipe when\n> > one quits the pager (without going to the end of the output).\n> >\n> > For instance, in zsh:\n> >\n> > cventin% setopt PRINT_EXIT_VALUE\n> > cventin% git log\n> > zsh: broken pipe git log\n> > cventin% echo $?\n> > 141\n> > cventin% \n> >\n> > This is annoying[...]\n> \n> Yes it's annoying, but the annoying output is from zsh, not\n> git. Consider a smarter implementation like:\n> \n> case $__exit_status in\n> 0) __exit_emoji=😀;;\n> 1) __exit_emoji=☹️ ;;\n> 141) __exit_emoji=🤕 ;;\n> [...]\n> \n> Then put the $__exit_emoji in your $PS1 prompt, now when you 'q' in a\n> pager you know the difference between having quit at the full output\n> being emitted or not.\n\nFYI, I already have the exit status already in my prompt (the above\ncommands were just for the example). Still, the git behavior is\ndisturbing.\n\nMoreover, this doesn't solve the issue when doing something like\n\n git log && some_other_command\n\n> > And of course, I don't want to hide error messages by default, because\n> > this would hide *real* errors.\n> \n> Isn't the solution to this that your shell stops reporting failures due\n> to SIGPIPE in such a prominent way then?\n\nNo! I want to be warned about real SIGPIPEs.\n\n> > The broken pipe is internally expected, thus should not be reported\n> > by git.\n> >\n> > Just to be clear: this broken pipe should be discarded only when git\n> > uses its builtin pager feature, not with a general pipe, where the\n> > error may be important.\n> >\n> > For instance,\n> >\n> > $ { git log ; echo \"Exit status: $?\" >&2 ; } | true\n> >\n> > should still output\n> >\n> > Exit status: 141\n> \n> I don't get it, how is it less meaningful when git itself invokes the\n> pager?\n\nI don't understand your question. If I invoke the pager myself,\nI don't get a SIGPIPE:\n\ncventin:~/software/gcc-trunk> git log\ncventin:~/software/gcc-trunk[PIPE]> git log|m\ncventin:~/software/gcc-trunk>\n\n-- \nVincent Lefèvre - Web: \n100% accessible validated (X)HTML - Blog: \nWork: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)\n"},{"id":"415681","messageId":"20210131034754.GL623063@zira.vinc17.org","threadId":"54992","inReplyTo":"20210131033652.GK623063@zira.vinc17.org","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Vincent Lefevre","fromEmail":"vincent@vinc17.net","sentAt":"2021-01-31T03:47:54Z","receivedAt":"2021-01-31T03:49:12Z","isPatch":false,"sender":{"key":"vincent@vinc17.net","avatar":null},"body":"On 2021-01-31 04:36:52 +0100, Vincent Lefevre wrote:\n> On 2021-01-31 02:47:59 +0100, Ævar Arnfjörð Bjarmason wrote:\n> > On Fri, Jan 15 2021, Vincent Lefevre wrote:\n> > > The broken pipe is internally expected, thus should not be reported\n> > > by git.\n> > >\n> > > Just to be clear: this broken pipe should be discarded only when git\n> > > uses its builtin pager feature, not with a general pipe, where the\n> > > error may be important.\n> > >\n> > > For instance,\n> > >\n> > > $ { git log ; echo \"Exit status: $?\" >&2 ; } | true\n> > >\n> > > should still output\n> > >\n> > > Exit status: 141\n> > \n> > I don't get it, how is it less meaningful when git itself invokes the\n> > pager?\n> \n> I don't understand your question. If I invoke the pager myself,\n> I don't get a SIGPIPE:\n> \n> cventin:~/software/gcc-trunk> git log\n> cventin:~/software/gcc-trunk[PIPE]> git log|m\n> cventin:~/software/gcc-trunk>\n\nWell, more precisely, I mean that it is not reported. But\nthe SIGPIPE itself still occurs as expected, e.g. for scripts,\nand one may choose to ignore it or not, as usual.\n\n-- \nVincent Lefèvre - Web: \n100% accessible validated (X)HTML - Blog: \nWork: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)\n"},{"id":"415697","messageId":"87o8h4omqa.fsf@evledraar.gmail.com","threadId":"54992","inReplyTo":"20210131033652.GK623063@zira.vinc17.org","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-31T20:49:49Z","receivedAt":"2021-01-31T20:50:50Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Jan 31 2021, Vincent Lefevre wrote:\n\n> On 2021-01-31 02:47:59 +0100, Ævar Arnfjörð Bjarmason wrote:\n>> On Fri, Jan 15 2021, Vincent Lefevre wrote:\n>> > I had reported the following bug at\n>> > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914896\n>> >\n>> > It still occurs with Git 2.30.0.\n>> >\n>> > Some git commands with a lot of output fail with a broken pipe when\n>> > one quits the pager (without going to the end of the output).\n>> >\n>> > For instance, in zsh:\n>> >\n>> > cventin% setopt PRINT_EXIT_VALUE\n>> > cventin% git log\n>> > zsh: broken pipe git log\n>> > cventin% echo $?\n>> > 141\n>> > cventin% \n>> >\n>> > This is annoying[...]\n>> \n>> Yes it's annoying, but the annoying output is from zsh, not\n>> git. Consider a smarter implementation like:\n>> \n>> case $__exit_status in\n>> 0) __exit_emoji=😀;;\n>> 1) __exit_emoji=☹️ ;;\n>> 141) __exit_emoji=🤕 ;;\n>> [...]\n>> \n>> Then put the $__exit_emoji in your $PS1 prompt, now when you 'q' in a\n>> pager you know the difference between having quit at the full output\n>> being emitted or not.\n>\n> FYI, I already have the exit status already in my prompt (the above\n> commands were just for the example). Still, the git behavior is\n> disturbing.\n>\n> Moreover, this doesn't solve the issue when doing something like\n>\n> git log && some_other_command\n\nWhat issue? That we're returning an exit code per getting a SIGHUP here\nis a feature. Consider:\n\n git -c core.pager=/bin/false log && echo showed you the output\n\nBefore the patch from Denton Liu we'd correctly not say that worked, now\nwe'll just ignore that we couldn't give the output to the pager.\n\n>> > And of course, I don't want to hide error messages by default, because\n>> > this would hide *real* errors.\n>> \n>> Isn't the solution to this that your shell stops reporting failures due\n>> to SIGPIPE in such a prominent way then?\n>\n> No! I want to be warned about real SIGPIPEs.\n\nNot being able to write \"git log\" output is a real SIGPIPE.\n\nI'm genuinely not trying to be difficult here, I just really don't see\nwhat the conceptual difference is that would cause you to say that's not\na \"real\" SIGPIPE.\n\nIs it because in your mind it's got something to do with the \"|\" shell\npiping construct? The SIGPIPE is sent by the kernel, so it's no less\nexpected in cases like:\n\n git log && echo foo\n\nThan:\n\n git log | cat\n\nIf something were to fail or the write() to the pager/pipe.\n\n>> > The broken pipe is internally expected, thus should not be reported\n>> > by git.\n>> >\n>> > Just to be clear: this broken pipe should be discarded only when git\n>> > uses its builtin pager feature, not with a general pipe, where the\n>> > error may be important.\n>> >\n>> > For instance,\n>> >\n>> > $ { git log ; echo \"Exit status: $?\" >&2 ; } | true\n>> >\n>> > should still output\n>> >\n>> > Exit status: 141\n>> \n>> I don't get it, how is it less meaningful when git itself invokes the\n>> pager?\n>\n> I don't understand your question. If I invoke the pager myself,\n> I don't get a SIGPIPE:\n>\n> cventin:~/software/gcc-trunk> git log\n> cventin:~/software/gcc-trunk[PIPE]> git log|m\n> cventin:~/software/gcc-trunk>\n\nDo you mean if you invoke \"less \" yourself, as opposed to \"git\nlog\" doing it for you? I.e.:\n\n git log >log.txt\n less log.txt\n \n # returns 0\n\nv.s.:\n\n git log # using less\n \n # returns 141\n\nYes, because e.g. under less aborting before you view the whole output\nisn't an error, the SIGPIPE is sent to the writer trying to\nunsuccessfully spew output to the pager.\n\nTo git the pager should be a black box. We don't know if the reason we\ncouldn't write output to it is because it's what the user wanted, or the\npager died on our input or whatever (as shown by setting it to\n/bin/false above).\n\nAnyway, I'm not saying that there's no place for this as an optional\nfeature or whatever.\n\nMaybe we have users who'd like to work around zsh's \"setopt\nPRINT_EXIT_VALUE\" mode (would you want this patch if you could make zsh\nignore 141?). But I think it should at least be hidden behind some\ncore.pagerErrorIgnore=141 or something. Some of us like standard *nix\nsemantics.\n"},{"id":"415729","messageId":"20210201103429.GT623063@zira.vinc17.org","threadId":"54992","inReplyTo":"87o8h4omqa.fsf@evledraar.gmail.com","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Vincent Lefevre","fromEmail":"vincent@vinc17.net","sentAt":"2021-02-01T10:34:29Z","receivedAt":"2021-02-01T10:35:14Z","isPatch":false,"sender":{"key":"vincent@vinc17.net","avatar":null},"body":"On 2021-01-31 21:49:49 +0100, Ævar Arnfjörð Bjarmason wrote:\n> On Sun, Jan 31 2021, Vincent Lefevre wrote:\n> > FYI, I already have the exit status already in my prompt (the above\n> > commands were just for the example). Still, the git behavior is\n> > disturbing.\n> >\n> > Moreover, this doesn't solve the issue when doing something like\n> >\n> > git log && some_other_command\n> \n> What issue? That we're returning an exit code per getting a SIGHUP here\n> is a feature. Consider:\n> \n> git -c core.pager=/bin/false log && echo showed you the output\n\nIf the pager exists with a non-zero exit status, it is normal to\nreturn a non-zero exit status. This was not the bug I reported.\n\n> > No! I want to be warned about real SIGPIPEs.\n> \n> Not being able to write \"git log\" output is a real SIGPIPE.\n\nWhich is not the case here, because the full output has never been\nrequested by the user.\n\n> Is it because in your mind it's got something to do with the \"|\" shell\n> piping construct? The SIGPIPE is sent by the kernel, so it's no less\n> expected in cases like:\n> \n> git log && echo foo\n> \n> Than:\n> \n> git log | cat\n\nSee the difference (without the patch) between\n\n$ git log && echo foo; echo $?\n141\n\nand\n\n$ git log | head; echo $?\n[...]\n0\n\n[...]\n> Maybe we have users who'd like to work around zsh's \"setopt\n> PRINT_EXIT_VALUE\" mode (would you want this patch if you could make zsh\n> ignore 141?).\n\nzsh is working as expected, and as I've already said, I ***WANT***\nSIGPIPE to be reported by the shell, as it may indicate a real failure\nin a script. BTW, I even have a script using git that relies on that:\n\n{ git rev-list --author \"$@[-1]\" HEAD &&\n git rev-list --grep \"$@[-1]\" HEAD } | \\\n git \"${@[1,-2]:-lv}\" --no-walk --stdin\n\nreturn $((pipestatus[2] ? pipestatus[2] : pipestatus[1]))\n\nHere it is important not to lose any information. No pager is\ninvolved, the full output is needed. If for some reason, the\nLHS of the pipe fails due to a SIGPIPE but the right hand side\nsucceeds, the error will be reported.\n\nThe fact is that with a pager, the SIGPIPE with a pager is normal.\nThus with a pager, git is reporting a spurious SIGPIPE, and this\nis disturbing.\n\n-- \nVincent Lefèvre - Web: \n100% accessible validated (X)HTML - Blog: \nWork: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)\n"},{"id":"415739","messageId":"CAPx1Gvf92eCnSCZJLeqwyL-SprCxmnfi4w=d0-MHddY38DzADg@mail.gmail.com","threadId":"54992","inReplyTo":"20210201103429.GT623063@zira.vinc17.org","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2021-02-01T11:33:54Z","receivedAt":"2021-02-01T11:35:13Z","isPatch":false,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"> On 2021-01-31 21:49:49 +0100, Ævar Arnfjörð Bjarmason wrote:\n> > ... That we're returning an exit code per getting a SIGHUP here\n> > is a feature. Consider:\n> >\n> > git -c core.pager=/bin/false log && echo showed you the output\n\nThis example has a minor flaw: it should use `git -c core.pager=/bin/true`,\nprobably.\n\nOn Mon, Feb 1, 2021 at 2:36 AM Vincent Lefevre wrote:\n> If the pager exists with a non-zero exit status, it is normal to\n> return a non-zero exit status. This was not the bug I reported.\n\nThat's the flaw in the example. The key though is that the program\nwe ran as the pager—false, true, whatever—*did not read any of its input*.\n\n> > Not being able to write \"git log\" output is a real SIGPIPE.\n\nWorth noting: Linux has a pretty large pipe buffer. POSIX requires\nat least 4k here, as I recall, but Linux will buffer 64k or more, so that\nif `git log` is able to write the entire log text (will be the case for small\nrepositories) *before* the program on the right side of the pager pipe\nexits (this depends on many things), the pager's exit *won't* cause\na SIGPIPE. You'll get the SIGPIPE if either the pager exits very\nquickly, so that `git log` is unable to write much before the exit, or\nif the repository is sufficiently large so that the pipe blocks first.\n\n> Which is not the case here, because the full output has never been\n> requested by the user.\n\nThe `git log` command *did* request the full output.\n\nThe problem that has come up is, if I understand correctly, that\nsome Linux distributions have come with misconfigured pagers\nthat don't bother reading their input, and silently exit zero. This\ncauses all kinds of Git commands to *seem* to fail. The Git commands\nare just fine; the bug is that the pager doesn't read or write anything.\n\nUnfortunately, the way that pipes work -- asynchronously -- means\nthat Git really *can't* catch all problems here. But catching a SIGPIPE,\nwhether Git itself spawned the pager or not, does indicate that\nsomething has gone wrong ... *unless* Git was piping to, e.g., less,\nand the user read enough, and the user typed `q` at less, and less\nexited without bothering to read the rest of the input.\n\nThere's no good way for Git to be able to tell which of these was\nthe case.\n\nI'm not sure what this actually argues for. ;-)\n\nChris\n"},{"id":"415740","messageId":"87im7cng42.fsf@evledraar.gmail.com","threadId":"54992","inReplyTo":"20210201103429.GT623063@zira.vinc17.org","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-01T12:10:21Z","receivedAt":"2021-02-01T12:11:20Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 01 2021, Vincent Lefevre wrote:\n\n> On 2021-01-31 21:49:49 +0100, �var Arnfj�r� Bjarmason wrote:\n>> On Sun, Jan 31 2021, Vincent Lefevre wrote:\n>> > FYI, I already have the exit status already in my prompt (the above\n>> > commands were just for the example). Still, the git behavior is\n>> > disturbing.\n>> >\n>> > Moreover, this doesn't solve the issue when doing something like\n>> >\n>> > git log && some_other_command\n>> \n>> What issue? That we're returning an exit code per getting a SIGHUP here\n>> is a feature. Consider:\n>> \n>> git -c core.pager=/bin/false log && echo showed you the output\n>\n> If the pager exists with a non-zero exit status, it is normal to\n> return a non-zero exit status. This was not the bug I reported.\n\nIs it normal? Isn't this subject to the same race noted in\nhttps://lore.kernel.org/git/20191115040909.GA21654@sigill.intra.peff.net/\n\nI.e. we start the /bin/false process, then start spewing output to it,\nso maybe we'll get a SIGPIPE first because it's not being consumed, or\nmaybe /bin/false (or whatever else exits with non-zero) will exit first.\n\nUnrelated to that there's at least a bug in wait_for_pager_signal() in\nhow we log the pager's exit. We just rely on \"ret\" in\nfinish_command_in_signal() before calling trace2_child_exit(), but\nshould probably log -1 there or something and defer.\n\n>> > No! I want to be warned about real SIGPIPEs.\n>> \n>> Not being able to write \"git log\" output is a real SIGPIPE.\n>\n> Which is not the case here, because the full output has never been\n> requested by the user.\n\nThey requested it by running \"git log\", which e.g. for git.git is ~1\nmillion lines. Then presumably paged down just a few pages and issued\n\"q\" in their pager. At which point we'll fail on the write() in git-log.\n\nThe pager's exit status is usually/always 0 in those cases\n(e.g. https://pubs.opengroup.org/onlinepubs/9699919799/utilities/more.html). So\nwe've got the SIGPIPE to indicate the output wasn't fully consumed.\n\n>> Is it because in your mind it's got something to do with the \"|\" shell\n>> piping construct? The SIGPIPE is sent by the kernel, so it's no less\n>> expected in cases like:\n>> \n>> git log && echo foo\n>> \n>> Than:\n>> \n>> git log | cat\n>\n> See the difference (without the patch) between\n>\n> $ git log && echo foo; echo $?\n> 141\n>\n> and\n>\n> $ git log | head; echo $?\n> [...]\n> 0\n\nPresumably that first command is one where you exited your pager before\nthe output wasn't fully consumed, see above.\n\n> [...]\n>> Maybe we have users who'd like to work around zsh's \"setopt\n>> PRINT_EXIT_VALUE\" mode (would you want this patch if you could make zsh\n>> ignore 141?).\n>\n> zsh is working as expected, and as I've already said, I ***WANT***\n> SIGPIPE to be reported by the shell, as it may indicate a real failure\n> in a script. BTW, I even have a script using git that relies on that:\n>\n> { git rev-list --author \"$@[-1]\" HEAD &&\n> git rev-list --grep \"$@[-1]\" HEAD } | \\\n> git \"${@[1,-2]:-lv}\" --no-walk --stdin\n>\n> return $((pipestatus[2] ? pipestatus[2] : pipestatus[1]))\n>\n> Here it is important not to lose any information. No pager is\n> involved, the full output is needed. If for some reason, the\n> LHS of the pipe fails due to a SIGPIPE but the right hand side\n> succeeds, the error will be reported.\n\nSorry, I really don't see how this is different. I think this goes back\nto my \"'|' shell piping construct[...]\" question in the E-Mail you're\nreplying to.\n\nin both the \"git log &&\" case and potentially here you'll get a program\nwriting to a pipe getting a SIGPIPE, which is then reflected in the exit\ncode.\n\n> The fact is that with a pager, the SIGPIPE with a pager is normal.\n> Thus with a pager, git is reporting a spurious SIGPIPE, and this\n> is disturbing.\n\nI don't get what you're trying to say here, sorry.\n\nMaybe this helps. So first, I don't know if your report came out of\nreading the recent \"set -o pipefail\" traffic on-list. As you can see in\n[1] I'm not some zealot for PIPEFAIL always being returned no matter\nwhat.\n\nThe difference between that though and what you're proposing is there\nyou have the shell getting an exit code and opting to ignore it, as\nopposed to the program itself sweeping it under the rug.\n\nI don't think either that just because you run a pager you're obligated\nto ferry down a SIGPIPE if you get it. E.g. mysql and postgresql both\nhave interactive shells where you can open pagers. I didn't bother to\ncheck, but you can imagine doing a \"show tables\" or whatever and only\nviewing the first page, then quitting in the pager.\n\nIf that's part of a long interactive SQL session it would make no sense\nfor the eventual exit code of mysql(1) or psql(1) to reflect that.\n\nBut with git we're (mostly) executing one-shot commands, e.g. with \"git\nlog\" you give it some params, and it spews all the output at you, maybe\nwith the help of a pager.\n\nSo then if we fail on the write() I don't see how it doesn't make sense\nto return the appropriate exit code for that failure downstream.\n\n1. https://lore.kernel.org/git/20210116153554.12604-12-avarab@gmail.com/\n\n"},{"id":"415741","messageId":"20210201123635.GA24560@zira.vinc17.org","threadId":"54992","inReplyTo":"CAPx1Gvf92eCnSCZJLeqwyL-SprCxmnfi4w=d0-MHddY38DzADg@mail.gmail.com","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Vincent Lefevre","fromEmail":"vincent@vinc17.net","sentAt":"2021-02-01T12:36:35Z","receivedAt":"2021-02-01T12:37:42Z","isPatch":false,"sender":{"key":"vincent@vinc17.net","avatar":null},"body":"On 2021-02-01 03:33:54 -0800, Chris Torek wrote:\n> > On 2021-01-31 21:49:49 +0100, Ævar Arnfjörð Bjarmason wrote:\n> > > ... That we're returning an exit code per getting a SIGHUP here\n> > > is a feature. Consider:\n> > >\n> > > git -c core.pager=/bin/false log && echo showed you the output\n> \n> This example has a minor flaw: it should use `git -c core.pager=/bin/true`,\n> probably.\n\nIn this case, since /bin/true doesn't read anything on purpose,\nI would not expect any non-zero exit status.\n\nAnd note that\n\n git -c core.pager=\"sh -c 'cat; /bin/false'\" log\n\nexits with a zero exit status, which is unexpected since the\npager failed. For instance, in practice, the pager could be\nkilled by the system, but the user would not necessarily notice\nthis as the pager may be configured to quit automatically when\nreaching the end of the output (there are some \"less\" options\nto do that: -E, -F). So, the user would think that he got the\nfull output while he didn't.\n\n[...]\n> > > Not being able to write \"git log\" output is a real SIGPIPE.\n> \n> Worth noting: Linux has a pretty large pipe buffer. POSIX requires\n> at least 4k here, as I recall, but Linux will buffer 64k or more, so that\n> if `git log` is able to write the entire log text (will be the case for small\n> repositories) *before* the program on the right side of the pager pipe\n> exits (this depends on many things), the pager's exit *won't* cause\n> a SIGPIPE. You'll get the SIGPIPE if either the pager exits very\n> quickly, so that `git log` is unable to write much before the exit, or\n> if the repository is sufficiently large so that the pipe blocks first.\n\nIn general, repositories have more than 64k log.\n\n> > Which is not the case here, because the full output has never been\n> > requested by the user.\n> \n> The `git log` command *did* request the full output.\n\nNo, because the output is sent to a pager. As long as the user\ndoes not look at more than what he looks for, no more \"git log\"\noutput is requested (such output can happen internally, but it\nis not requested by the user).\n\n> The problem that has come up is, if I understand correctly, that\n> some Linux distributions have come with misconfigured pagers\n> that don't bother reading their input, and silently exit zero.\n\nThey are not misconfigured. This is how they work. Actually I don't\nsee why they should read more than needed: this would be a useless\nwaste of memory.\n\n> This causes all kinds of Git commands to *seem* to fail. The Git\n> commands are just fine; the bug is that the pager doesn't read or\n> write anything.\n> \n> Unfortunately, the way that pipes work -- asynchronously -- means\n> that Git really *can't* catch all problems here. But catching a SIGPIPE,\n> whether Git itself spawned the pager or not, does indicate that\n> something has gone wrong ... *unless* Git was piping to, e.g., less,\n> and the user read enough, and the user typed `q` at less, and less\n> exited without bothering to read the rest of the input.\n> \n> There's no good way for Git to be able to tell which of these was\n> the case.\n\nIn the case git spawns a pager, it knows that this is a pager\n(as per documentation).\n\n-- \nVincent Lefèvre - Web: \n100% accessible validated (X)HTML - Blog: \nWork: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)\n"},{"id":"415748","messageId":"CAPx1Gverh2E2h5JOSOfJ7JYvbhjv8hJNLE8y4VA2fNv0La8Rtw@mail.gmail.com","threadId":"54992","inReplyTo":"20210201123635.GA24560@zira.vinc17.org","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2021-02-01T12:53:03Z","receivedAt":"2021-02-01T12:54:24Z","isPatch":false,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Mon, Feb 1, 2021 at 4:36 AM Vincent Lefevre wrote:\n> In general, repositories have more than 64k log.\n\nPlease don't focus on the exact size. Some system might\nhave a multi-gigabyte pipe buffer, and some other system\nmight have a tiny one; we'd like consistent behavior no matter\nwhat size the system uses. Can we *get* consistent behavior?\nI don't know.\n\n[me]\n> > The problem that has come up is, if I understand correctly, that\n> > some Linux distributions have come with misconfigured pagers\n> > that don't bother reading their input, and silently exit zero.\n>\n> They are not misconfigured. This is how they work.\n\nA pager that reads nothing and writes nothing does not seem\nvery useful to me. (Perhaps we can disregard these cases\nentirely. It's not like we should expect Git to handle things if\nsomeone builds a version of `less` that doesn't work. The\nfact is that on these Linux systems, running `$pager foo` on a\nfile `foo` does nothing at all, for some values of `$pager`. I\nbelieve I ran into this on a Docker setup at least once. It's\nnot Git's fault and hence not something for it to correct.)\n\n[on various exit cases]\n> > There's no good way for Git to be able to tell which of these was\n> > the case.\n>\n> In the case git spawns a pager, it knows that this is a pager\n> (as per documentation).\n\nAgain, this seems irrelevant. If the pager exited correctly\nwhile reading everything, or it exited correctly without reading\neverything, or if it exited incorrectly with or without reading\neverything, is not something *Git* can tell. I'm therefore not\nsure that Git should *try* to tell -- which is the point I'm trying\nto make here. The question is this: if we can only do a poor\njob, should we try at all? What *should* we do, given what\nwe *can* do? All we get is SIGPIPE and an exit status, and\nthe SIGPIPE may or may not be meaningful.\n\nThat seems to be what you're arguing as well. So I'm not sure\nwhy you're objecting to what I'm pointing out. :-)\n\nChris\n"},{"id":"415751","messageId":"20210201144921.8664-4-avarab@gmail.com","threadId":"54992","inReplyTo":"87im7cng42.fsf@evledraar.gmail.com","subject":"[PATCH 3/3] pager: properly log pager exit code when signalled","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-01T14:49:21Z","receivedAt":"2021-02-01T14:53:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When git invokes a pager that exits with non-zero the common case is\nthat we'll already return the correct SIGPIPE failure from git itself,\nbut the exit code logged in trace2 has always been incorrectly\nreported[1]. Fix that and log the correct exit code in the logs.\n\nSince this gives us something to test outside of our recently-added\ntests needing a !MINGW prerequisite, let's refactor the test to run on\nMINGW and actually check for SIGPIPE outside of MINGW.\n\nThe wait_or_whine() is only called with a true \"in_signal\" from from\nfinish_command_in_signal(), which in turn is only used in pager.c.\n\nI'm not quite sure about that BUG() case. Can we have a true in_signal\nand not have a true WIFEXITED(status)? I haven't been able to think of\na test case for it.\n\n1. The incorrect logging of the exit code in was seemingly copy/pasted\n into finish_command_in_signal() in ee4512ed481 (trace2: create new\n combined trace facility, 2019-02-22)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n run-command.c | 8 +++++--\n t/t7006-pager.sh | 61 +++++++++++++++++++++++++++++++++++++++++-------\n 2 files changed, 58 insertions(+), 11 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex ea4d0fb4b15..10e1c96c2bd 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -551,8 +551,12 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n \n \twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n \t\t;\t/* nothing */\n-\tif (in_signal)\n-\t\treturn 0;\n+\tif (in_signal && WIFEXITED(status))\n+\t\treturn WEXITSTATUS(status);\n+\tif (in_signal) {\n+\t\tBUG(\"was not expecting waitpid() status %d\", status);\n+\t\treturn -1;\n+\t}\n \n \tif (waiting < 0) {\n \t\tfailed_errno = errno;\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex c60886f43e6..1424466caf5 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -656,31 +656,74 @@ test_expect_success TTY 'git tag with auto-columns ' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager exit' '\n+test_expect_success TTY 'git returns SIGPIPE on early pager exit' '\n \ttest_when_finished \"rm pager-used\" &&\n \ttest_config core.pager \">pager-used; head -n 1; exit 0\" &&\n+\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n+\texport GIT_TRACE2 &&\n+\ttest_when_finished \"unset GIT_TRACE2\" &&\n \n-\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n-\ttest_match_signal 13 \"$OUT\" &&\n+\tif test_have_prereq !MINGW\n+\tthen\n+\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n+\t\ttest_match_signal 13 \"$OUT\"\n+\telse\n+\t\ttest_terminal git log\n+\tfi &&\n+\tgrep \"child_exit.* code:0 \" trace.normal &&\n \ttest_path_is_file pager-used\n '\n \n-test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager non-zero exit' '\n+test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '\n \ttest_when_finished \"rm pager-used\" &&\n \ttest_config core.pager \">pager-used; head -n 1; exit 1\" &&\n+\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n+\texport GIT_TRACE2 &&\n+\ttest_when_finished \"unset GIT_TRACE2\" &&\n \n-\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n-\ttest_match_signal 13 \"$OUT\" &&\n+\tif test_have_prereq !MINGW\n+\tthen\n+\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n+\t\ttest_match_signal 13 \"$OUT\"\n+\telse\n+\t\ttest_terminal git log\n+\tfi &&\n+\tgrep \"child_exit.* code:1 \" trace.normal &&\n \ttest_path_is_file pager-used\n '\n \n-test_expect_success TTY,!MINGW 'git discards pager non-zero exit' '\n+test_expect_success TTY 'git discards pager non-zero exit' '\n \ttest_when_finished \"rm pager-used\" &&\n \ttest_config core.pager \"wc >pager-used; exit 1\" &&\n+\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n+\texport GIT_TRACE2 &&\n+\ttest_when_finished \"unset GIT_TRACE2\" &&\n \n-\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n-\ttest \"$OUT\" -eq 0 &&\n+\tif test_have_prereq !MINGW\n+\tthen\n+\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n+\t\ttest \"$OUT\" -eq 0\n+\telse\n+\t\ttest_terminal git log\n+\tfi &&\n+\tgrep \"child_exit.* code:1 \" trace.normal &&\n \ttest_path_is_file pager-used\n '\n \n+test_expect_success TTY 'git logs nonexisting pager invocation' '\n+\ttest_config core.pager \"does-not-exist\" &&\n+\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n+\texport GIT_TRACE2 &&\n+\ttest_when_finished \"unset GIT_TRACE2\" &&\n+\n+\tif test_have_prereq !MINGW\n+\tthen\n+\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n+\t\ttest_match_signal 13 \"$OUT\"\n+\telse\n+\t\ttest_terminal git log\n+\tfi &&\n+\tgrep \"child_exit.* code:-1 \" trace.normal\n+'\n+\n test_done\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"415752","messageId":"20210201144921.8664-3-avarab@gmail.com","threadId":"54992","inReplyTo":"87im7cng42.fsf@evledraar.gmail.com","subject":"[PATCH 2/3] pager: refactor wait_for_pager() function","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-01T14:49:20Z","receivedAt":"2021-02-01T14:55:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor the wait_for_pager() function. Since 507d7804c0b (pager:\ndon't use unsafe functions in signal handlers, 2015-09-04) the\nwait_for_pager() and wait_for_pager_atexit() callers diverged on more\nthan they shared.\n\nLet's extract the common code into a new close_pager_fds() helper, and\nmove the parts unique to the only to callers to those functions.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n pager.c | 18 +++++++-----------\n 1 file changed, 7 insertions(+), 11 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex ee435de6756..3d37dd7adaa 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -11,29 +11,25 @@\n static struct child_process pager_process = CHILD_PROCESS_INIT;\n static const char *pager_program;\n \n-static void wait_for_pager(int in_signal)\n+static void close_pager_fds(void)\n {\n-\tif (!in_signal) {\n-\t\tfflush(stdout);\n-\t\tfflush(stderr);\n-\t}\n \t/* signal EOF to pager */\n \tclose(1);\n \tclose(2);\n-\tif (in_signal)\n-\t\tfinish_command_in_signal(&pager_process);\n-\telse\n-\t\tfinish_command(&pager_process);\n }\n \n static void wait_for_pager_atexit(void)\n {\n-\twait_for_pager(0);\n+\tfflush(stdout);\n+\tfflush(stderr);\n+\tclose_pager_fds();\n+\tfinish_command(&pager_process);\n }\n \n static void wait_for_pager_signal(int signo)\n {\n-\twait_for_pager(1);\n+\tclose_pager_fds();\n+\tfinish_command_in_signal(&pager_process);\n \tsigchain_pop(signo);\n \traise(signo);\n }\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"415753","messageId":"20210201144921.8664-2-avarab@gmail.com","threadId":"54992","inReplyTo":"87im7cng42.fsf@evledraar.gmail.com","subject":"[PATCH 1/3] pager: test for exit code","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-01T14:49:19Z","receivedAt":"2021-02-01T14:55:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add tests for how git behaves when the pager itself exits with\nnon-zero, as well as for us exiting with 141 when we're killed with\nSIGPIPE due to the pager not consuming its output.\n\nThere is some recent discussion[1] about these semantics, but aside\nfrom what we want to do in the future, we should have a test for the\ncurrent behavior.\n\nThis test construct is stolen from 7559a1be8a0 (unblock and unignore\nSIGPIPE, 2014-09-18).\n\n1. https://lore.kernel.org/git/87o8h4omqa.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7006-pager.sh | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex fdb450e446a..c60886f43e6 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -656,4 +656,31 @@ test_expect_success TTY 'git tag with auto-columns ' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager exit' '\n+\ttest_when_finished \"rm pager-used\" &&\n+\ttest_config core.pager \">pager-used; head -n 1; exit 0\" &&\n+\n+\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n+\ttest_match_signal 13 \"$OUT\" &&\n+\ttest_path_is_file pager-used\n+'\n+\n+test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager non-zero exit' '\n+\ttest_when_finished \"rm pager-used\" &&\n+\ttest_config core.pager \">pager-used; head -n 1; exit 1\" &&\n+\n+\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n+\ttest_match_signal 13 \"$OUT\" &&\n+\ttest_path_is_file pager-used\n+'\n+\n+test_expect_success TTY,!MINGW 'git discards pager non-zero exit' '\n+\ttest_when_finished \"rm pager-used\" &&\n+\ttest_config core.pager \"wc >pager-used; exit 1\" &&\n+\n+\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n+\ttest \"$OUT\" -eq 0 &&\n+\ttest_path_is_file pager-used\n+'\n+\n test_done\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"415754","messageId":"20210201144921.8664-1-avarab@gmail.com","threadId":"54992","inReplyTo":"87im7cng42.fsf@evledraar.gmail.com","subject":"[PATCH 0/3] pager: test for exit behavior & trace2 bug fix","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-01T14:49:18Z","receivedAt":"2021-02-01T14:57:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"While reading the pager code I discovered[1] that we log the wrong\nexit code when the pager itself exits with non-zero under trace2. This\nfixes that bug.\n\nI think whatever the consensus is on the SIGPIPE exit status\npropagating it makes sense if we'd ignore it to rebase the patch to do\nso[1] on this. I think the addition of a new \"test-tool pager\" there\nis redundant to testing SIGHUP from git itself as 1/3 does here, but\nmaybe I'm missing something...\n\n2/3 is not needed for the end-state here, but I figured it was a good\nrefactoring while I was at it.\n\n1. https://lore.kernel.org/git/bc88492979fee215d5be06ccbc246ae0171a9ced.1611910122.git.liu.denton@gmail.com/\n\nÆvar Arnfjörð Bjarmason (3):\n pager: test for exit code\n pager: refactor wait_for_pager() function\n pager: properly log pager exit code when signalled\n\n pager.c | 18 +++++--------\n run-command.c | 8 ++++--\n t/t7006-pager.sh | 70 ++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 83 insertions(+), 13 deletions(-)\n\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"415755","messageId":"87czxjomn8.fsf@evledraar.gmail.com","threadId":"54992","inReplyTo":"bc88492979fee215d5be06ccbc246ae0171a9ced.1611910122.git.liu.denton@gmail.com","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-01T15:03:55Z","receivedAt":"2021-02-01T15:06:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jan 30 2021, Denton Liu wrote:\n\n> [...]\n\nThe thread at large has enough about whether this approach even makes\nsense. I won't repeat that here. Just small notes on the patch itself:\n\n> diff --git a/Makefile b/Makefile\n> index 4edfda3e00..38a1a20f31 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -719,6 +719,7 @@ TEST_BUILTINS_OBJS += test-mktemp.o\n> TEST_BUILTINS_OBJS += test-oid-array.o\n> TEST_BUILTINS_OBJS += test-oidmap.o\n> TEST_BUILTINS_OBJS += test-online-cpus.o\n> +TEST_BUILTINS_OBJS += test-pager.o\n> TEST_BUILTINS_OBJS += test-parse-options.o\n> TEST_BUILTINS_OBJS += test-parse-pathspec-file.o\n> TEST_BUILTINS_OBJS += test-path-utils.o\n> diff --git a/pager.c b/pager.c\n> index ee435de675..5922d99dc8 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -34,6 +34,8 @@ static void wait_for_pager_atexit(void)\n> static void wait_for_pager_signal(int signo)\n> {\n> \twait_for_pager(1);\n> +\tif (signo == SIGPIPE)\n> +\t\texit(0);\n\nAs shown in\nhttps://lore.kernel.org/git/20210201144921.8664-1-avarab@gmail.com/ this\nleaves us without guard rails where the pager dies/segfaults or\nwhatever.\n\nThat's an existing bug, but by not carrying the SIGPIPE forward it\nchanges from \"most of the time we'd exit with SIGPIPE anyway\" to \"we'll\nnever notice\".\n\n> [...]\n> +test_expect_success TTY 'SIGPIPE from pager returns success' '\n> +\ttest_terminal env PAGER=true test-tool pager\n> +'\n> +\n> test_done\n\nAs noted in\nhttps://lore.kernel.org/git/20210201144921.8664-1-avarab@gmail.com/ I\nthink this whole \"test-tool pager\" isn't needed. We can just use git\nitself with some trickery.\n"},{"id":"415756","messageId":"87ft2fomsa.fsf@evledraar.gmail.com","threadId":"54992","inReplyTo":"CAPx1Gvf92eCnSCZJLeqwyL-SprCxmnfi4w=d0-MHddY38DzADg@mail.gmail.com","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-01T15:00:53Z","receivedAt":"2021-02-01T15:15:31Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 01 2021, Chris Torek wrote:\n\n>> On 2021-01-31 21:49:49 +0100, Ævar Arnfjörð Bjarmason wrote:\n>> > ... That we're returning an exit code per getting a SIGHUP here\n>> > is a feature. Consider:\n>> >\n>> > git -c core.pager=/bin/false log && echo showed you the output\n>\n> This example has a minor flaw: it should use `git -c core.pager=/bin/true`,\n> probably.\n\nFWIW it doesn't have a flaw It should be /bin/false, not /bin/true. See\nthis reply in a side-thread:\nhttps://lore.kernel.org/git/87im7cng42.fsf@evledraar.gmail.com/\n\nI.e. part of the point here (which I realize I forgot to articulate...)\nis that we have a hard reliance on SIGHUP to report *any* pager failures\nas a matter of the current implementation.\n\nPart of that has to do with internal git implementation details, i.e. we\nget the exit code for the pager either in an atexit() handler (we've\nalready picked the exit code) or when handling a signal.\n\nPerhaps we could do better there and e.g. exit with if the pager\nexits with . I don't know what's the conventional behavior in that\ncase.\n\nBut in any case, we exit with SIGPIPE in those cases in any reasonable\nfailure mode. That is, unless the pager consumed all the output, and\n*then* died that is.\n\nI submitted\nhttps://lore.kernel.org/git/20210201144921.8664-1-avarab@gmail.com/ to\ntry to address the lack of testing around this, which has tests for the\ntrue/false case.\n"},{"id":"415758","messageId":"20210201151703.GC24560@zira.vinc17.org","threadId":"54992","inReplyTo":"CAPx1Gverh2E2h5JOSOfJ7JYvbhjv8hJNLE8y4VA2fNv0La8Rtw@mail.gmail.com","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Vincent Lefevre","fromEmail":"vincent@vinc17.net","sentAt":"2021-02-01T15:17:03Z","receivedAt":"2021-02-01T15:18:32Z","isPatch":false,"sender":{"key":"vincent@vinc17.net","avatar":null},"body":"On 2021-02-01 04:53:03 -0800, Chris Torek wrote:\n> On Mon, Feb 1, 2021 at 4:36 AM Vincent Lefevre wrote:\n> > In general, repositories have more than 64k log.\n> \n> Please don't focus on the exact size. Some system might\n> have a multi-gigabyte pipe buffer, and some other system\n> might have a tiny one; we'd like consistent behavior no matter\n> what size the system uses. Can we *get* consistent behavior?\n> I don't know.\n\nThe consistent behavior can be obtained by ignoring the broken pipe\n(in the case where git starts the pager).\n\n> [me]\n> > > The problem that has come up is, if I understand correctly, that\n> > > some Linux distributions have come with misconfigured pagers\n> > > that don't bother reading their input, and silently exit zero.\n> >\n> > They are not misconfigured. This is how they work.\n> \n> A pager that reads nothing and writes nothing does not seem\n> very useful to me. [...]\n\nI agree.\n\n> [on various exit cases]\n> > > There's no good way for Git to be able to tell which of these was\n> > > the case.\n> >\n> > In the case git spawns a pager, it knows that this is a pager\n> > (as per documentation).\n> \n> Again, this seems irrelevant. If the pager exited correctly\n> while reading everything, or it exited correctly without reading\n> everything, or if it exited incorrectly with or without reading\n> everything, is not something *Git* can tell.\n\nNo, Git can tell when the pager exited abnormally: it suffices to\ncheck its exit status. Git currently doesn't do that, and this is\nbad, because it can miss real issues, which cannot always be detected\nby the user.\n\nIf the pager exits with exit code 0, this means normal termination,\nwhether the user has read the full output or not.\n\n> I'm therefore not sure that Git should *try* to tell -- which is the\n> point I'm trying to make here. The question is this: if we can only\n> do a poor job, should we try at all? What *should* we do, given what\n> we *can* do? All we get is SIGPIPE and an exit status, and the\n> SIGPIPE may or may not be meaningful.\n> \n> That seems to be what you're arguing as well. So I'm not sure\n> why you're objecting to what I'm pointing out. :-)\n\nWell, my objection is based on the fact that it is possible to get\nthe information from the exit status of the pager (I originally\nthought that Git was taking it into account).\n\nBTW, another related thing I dislike about Git, and I think that this\nshould also be regarded as a bug, is that when doing a commit, Git\ndoesn't check the exit status of the editor for the commit message.\nSay, for instance, if something on the system kills the editor, Git\napplies the commit with an incorrect or incomplete log message though\nthe commit wasn't validated yet by the user. Fortunately, the user\ncan amend the commit, but IMHO, that's an incorrect behavior.\n\n-- \nVincent Lefèvre - Web: \n100% accessible validated (X)HTML - Blog: \nWork: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)\n"},{"id":"415759","messageId":"20210201144857.GB24560@zira.vinc17.org","threadId":"54992","inReplyTo":"87im7cng42.fsf@evledraar.gmail.com","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Vincent Lefevre","fromEmail":"vincent@vinc17.net","sentAt":"2021-02-01T14:48:57Z","receivedAt":"2021-02-01T15:25:57Z","isPatch":false,"sender":{"key":"vincent@vinc17.net","avatar":null},"body":"On 2021-02-01 13:10:21 +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Feb 01 2021, Vincent Lefevre wrote:\n> \n> > On 2021-01-31 21:49:49 +0100, �var Arnfj�r� Bjarmason wrote:\n> >> On Sun, Jan 31 2021, Vincent Lefevre wrote:\n> >> > FYI, I already have the exit status already in my prompt (the above\n> >> > commands were just for the example). Still, the git behavior is\n> >> > disturbing.\n> >> >\n> >> > Moreover, this doesn't solve the issue when doing something like\n> >> >\n> >> > git log && some_other_command\n> >> \n> >> What issue? That we're returning an exit code per getting a SIGHUP here\n> >> is a feature. Consider:\n> >> \n> >> git -c core.pager=/bin/false log && echo showed you the output\n> >\n> > If the pager exists with a non-zero exit status, it is normal to\n> > return a non-zero exit status. This was not the bug I reported.\n> \n> Is it normal? Isn't this subject to the same race noted in\n> https://lore.kernel.org/git/20191115040909.GA21654@sigill.intra.peff.net/\n\nThere's a race only because the command is buggy under bash's\npipefail.\n\nSomething like\n\n git status -s -b | head -1\n\nis fine by default, because the exist status of the LHS command is\nignored. With pipefail, you may start getting SIGPIPE exit codes,\nwhich, depending on the context, you may want to ignore or not.\nI suppose that the user who writes something like the above would\nlike to ignore SIGPIPE.\n\nSo, that should be:\n\n { git status -s -b; if [[ $? = 141 ]]; then return 0; fi } | head -1\n\n(though that's 100% safe only if git catches/blocks/ignores SIGPIPE\nand detect the broken pipe with EPIPE, so that an abnormal termination\ndue to a \"kill -PIPE ...\" from another process would not be ignored).\n\nIt appears that pipefail was designed mainly for scripts. So, having\nto handle SIGPIPE like that is OK in scripts. For interactive use,\nthis would be bad, but that's not the purpose of pipefail (or bash\nshould have an option to regard 141 as 0 in any LHS command).\n\nFYI, I have a zsh function to automatically pipe some commands to\n\"less\" when connected to a terminal (a bit like what git does),\nwhere I explicitly ignore SIGPIPE for the command:\n\npager-wrapper()\n{\n local -a opt\n while [[ $1 == -* ]]\n do\n opt+=$1\n shift\n done\n if [[ -t 1 ]] then\n $@ $opt |& less -+c -FRX\n return $(( $pipestatus[2] != 0 ? $pipestatus[2] :\n $pipestatus[1] != 128 + $(kill -l PIPE) ? $pipestatus[1] : 0 ))\n else\n $@\n fi\n}\n\nSo no SIGPIPE is reported when I quit the pager. I can still get a\nreported SIGPIPE, e.g. if \"less\" is killed by SIGPIPE (e.g., this\nis possible with \"kill -PIPE ...\"), and this one is meaningful.\n\n> >> > No! I want to be warned about real SIGPIPEs.\n> >> \n> >> Not being able to write \"git log\" output is a real SIGPIPE.\n> >\n> > Which is not the case here, because the full output has never been\n> > requested by the user.\n> \n> They requested it by running \"git log\", which e.g. for git.git is ~1\n> million lines. Then presumably paged down just a few pages and issued\n> \"q\" in their pager. At which point we'll fail on the write() in git-log.\n\nBut when outputting to a pager, this should not be regarded as an\nerror: the reason is either the user has quit the pager normally\n(after having read what he wanted to read: the user did not need\nmore output) or the pager has terminated in an abnormal way, in\nwhich case the exit status of the pager should be non-zero.\n\n> The pager's exit status is usually/always 0 in those cases\n> (e.g. https://pubs.opengroup.org/onlinepubs/9699919799/utilities/more.html).\n\nYes, and there's no reason to return anything else, as quitting the\npager before reading the full output is not an error.\n\n> So we've got the SIGPIPE to indicate the output wasn't fully\n> consumed.\n\nBut the user doesn't care: he quit the pager because he didn't\nneed more output. So there is no need to signal that the output\nwasn't fully consumed. The user already knew that before quitting\nthe pager!\n\n> > [...]\n> >> Maybe we have users who'd like to work around zsh's \"setopt\n> >> PRINT_EXIT_VALUE\" mode (would you want this patch if you could make zsh\n> >> ignore 141?).\n> >\n> > zsh is working as expected, and as I've already said, I ***WANT***\n> > SIGPIPE to be reported by the shell, as it may indicate a real failure\n> > in a script. BTW, I even have a script using git that relies on that:\n> >\n> > { git rev-list --author \"$@[-1]\" HEAD &&\n> > git rev-list --grep \"$@[-1]\" HEAD } | \\\n> > git \"${@[1,-2]:-lv}\" --no-walk --stdin\n> >\n> > return $((pipestatus[2] ? pipestatus[2] : pipestatus[1]))\n> >\n> > Here it is important not to lose any information. No pager is\n> > involved, the full output is needed. If for some reason, the\n> > LHS of the pipe fails due to a SIGPIPE but the right hand side\n> > succeeds, the error will be reported.\n> \n> Sorry, I really don't see how this is different. I think this goes back\n> to my \"'|' shell piping construct[...]\" question in the E-Mail you're\n> replying to.\n> \n> in both the \"git log &&\" case and potentially here you'll get a program\n> writing to a pipe getting a SIGPIPE, which is then reflected in the exit\n> code.\n\nI mean that there are SIGPIPEs that one does not want to ignore\n(because they would indicate a problem -- in general in scripts),\nand other ones that should be ignored because they don't indicate\nan error.\n\n> > The fact is that with a pager, the SIGPIPE with a pager is normal.\n> > Thus with a pager, git is reporting a spurious SIGPIPE, and this\n> > is disturbing.\n> \n> I don't get what you're trying to say here, sorry.\n\nI mean that when the user quits the pager, there is no reason to\nreport an error because the user explicitly wanted to quit now.\n\nSimilarly, if I run a text viewer on a file, I don't want a SIGPIPE\nto be reported if I do not go to the end of the file (if a pipe was\nused to read the file, e.g. to do some filtering, as \"less\" can do).\n\n> Maybe this helps. So first, I don't know if your report came out of\n> reading the recent \"set -o pipefail\" traffic on-list. As you can see in\n> [1] I'm not some zealot for PIPEFAIL always being returned no matter\n> what.\n\nThis is not related. And [1] is from 2021 (with a thread started\nin 2019), while my report dates back to 2018:\n\n https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914896\n\nMoreover, [1] is only about the use of pipes in the shell command.\nMy bug report is about the internal use of a pager by git.\n\n> The difference between that though and what you're proposing is there\n> you have the shell getting an exit code and opting to ignore it, as\n> opposed to the program itself sweeping it under the rug.\n> \n> I don't think either that just because you run a pager you're obligated\n> to ferry down a SIGPIPE if you get it. E.g. mysql and postgresql both\n> have interactive shells where you can open pagers. I didn't bother to\n> check, but you can imagine doing a \"show tables\" or whatever and only\n> viewing the first page, then quitting in the pager.\n> \n> If that's part of a long interactive SQL session it would make no sense\n> for the eventual exit code of mysql(1) or psql(1) to reflect that.\n> \n> But with git we're (mostly) executing one-shot commands, e.g. with \"git\n> log\" you give it some params, and it spews all the output at you, maybe\n> with the help of a pager.\n> \n> So then if we fail on the write() I don't see how it doesn't make sense\n> to return the appropriate exit code for that failure downstream.\n\nThis depends on the kind of error. I agree for an unexpected error.\nBut for a broken pipe because git started a pager on its own and\nthe user chose to quit the pager, this should not be regarded as\nan error.\n\n> 1. https://lore.kernel.org/git/20210116153554.12604-12-avarab@gmail.com/\n\n-- \nVincent Lefèvre - Web: \n100% accessible validated (X)HTML - Blog: \nWork: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)\n"},{"id":"415760","messageId":"87a6snokrr.fsf@evledraar.gmail.com","threadId":"54992","inReplyTo":"20210201144857.GB24560@zira.vinc17.org","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-01T15:44:24Z","receivedAt":"2021-02-01T15:45:24Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 01 2021, Vincent Lefevre wrote:\n\n> On 2021-02-01 13:10:21 +0100, �var Arnfj�r� Bjarmason wrote:\n>> \nn>> On Mon, Feb 01 2021, Vincent Lefevre wrote:\n>> \n>> > On 2021-01-31 21:49:49 +0100, �var Arnfj�r� Bjarmason wrote:\n>> >> On Sun, Jan 31 2021, Vincent Lefevre wrote:\n>> >> > FYI, I already have the exit status already in my prompt (the above\n>> >> > commands were just for the example). Still, the git behavior is\n>> >> > disturbing.\n>> >> >\n>> >> > Moreover, this doesn't solve the issue when doing something like\n>> >> >\n>> >> > git log && some_other_command\n>> >> \n>> >> What issue? That we're returning an exit code per getting a SIGHUP here\n>> >> is a feature. Consider:\n>> >> \n>> >> git -c core.pager=/bin/false log && echo showed you the output\n>> >\n>> > If the pager exists with a non-zero exit status, it is normal to\n>> > return a non-zero exit status. This was not the bug I reported.\n>> \n>> Is it normal? Isn't this subject to the same race noted in\n>> https://lore.kernel.org/git/20191115040909.GA21654@sigill.intra.peff.net/\n>\n> There's a race only because the command is buggy under bash's\n> pipefail.\n>\n> Something like\n>\n> git status -s -b | head -1\n>\n> is fine by default, because the exist status of the LHS command is\n> ignored. With pipefail, you may start getting SIGPIPE exit codes,\n> which, depending on the context, you may want to ignore or not.\n> I suppose that the user who writes something like the above would\n> like to ignore SIGPIPE.\n>\n> So, that should be:\n>\n> { git status -s -b; if [[ $? = 141 ]]; then return 0; fi } | head -1\n>\n> (though that's 100% safe only if git catches/blocks/ignores SIGPIPE\n> and detect the broken pipe with EPIPE, so that an abnormal termination\n> due to a \"kill -PIPE ...\" from another process would not be ignored).\n>\n> It appears that pipefail was designed mainly for scripts. So, having\n> to handle SIGPIPE like that is OK in scripts. For interactive use,\n> this would be bad, but that's not the purpose of pipefail (or bash\n> should have an option to regard 141 as 0 in any LHS command).\n>\n> FYI, I have a zsh function to automatically pipe some commands to\n> \"less\" when connected to a terminal (a bit like what git does),\n> where I explicitly ignore SIGPIPE for the command:\n\nI think there's some confusion here. I'm not referring to how \"set -o\npipefail\" behaves in bash. But pointing to Jeff King's simple example[1]\nof how a command like \"git status -sb\" might exit (note that it may\nprint more than 1 line) due to a race with how SIGPIPE interacts with\nexit statuses. That's *nix/POSIX behavior, nothing to do with bash.\n\nThe same will apply to a pager we launch on a command like \"git log\".\n\nAs Chris Torek noted in a side-thread[2] the buffers involved here are\nOS-defined. In the general case you may get a PIPEFAIL or not depending\non whether you e.g. cross a PIPE_BUF boundary to get from line 1 to 2 of\nyour output, while \"head -n 1\" is consuming it.\n\nBut then consider a pager like:\n\n while (wantit())\n\t consume_and_print_output();\n sleep(10);\n exit(1);\n\nNow we can just exit early if it decides it doesn't want our output, as\nwe'll likely get a SIGPIPE, but if we're ignoring SIGPIPE and we want to\ndistinguish that from non-zero pager exit codes, we need to wait 10\nseconds until waitpid() tells us what the exit status is.\n\nThat's obviously a contrived example, but demonstrates the race\ncondition involved.\n\n1. https://lore.kernel.org/git/20191115040909.GA21654@sigill.intra.peff.net/\n2. https://lore.kernel.org/git/CAPx1Gverh2E2h5JOSOfJ7JYvbhjv8hJNLE8y4VA2fNv0La8Rtw@mail.gmail.com/\n\n>> >> \n>> >> Not being able to write \"git log\" output is a real SIGPIPE.\n>> >\n>> > Which is not the case here, because the full output has never been\n>> > requested by the user.\n>> \n>> They requested it by running \"git log\", which e.g. for git.git is ~1\n>> million lines. Then presumably paged down just a few pages and issued\n>> \"q\" in their pager. At which point we'll fail on the write() in git-log.\n>\n> But when outputting to a pager, this should not be regarded as an\n> error: the reason is either the user has quit the pager normally\n> (after having read what he wanted to read: the user did not need\n> more output) or the pager has terminated in an abnormal way, in\n> which case the exit status of the pager should be non-zero.\n\nIn an ideal world, or something we can plausibly implement in a portable\nmanner on systems that exist in the wild?\n\nYes I agree that this sort of behavior would be stupid e.g. for an\nintegrated GUI application, but that's not what we've got. We're calling\nan arbitrary user-supplied command and piping output to it, and are then\ngoing to get SIGPIPE or an exit code back.\n\n>> The pager's exit status is usually/always 0 in those cases\n>> (e.g. https://pubs.opengroup.org/onlinepubs/9699919799/utilities/more.html).\n>\n> Yes, and there's no reason to return anything else, as quitting the\n> pager before reading the full output is not an error.\n>\n>> So we've got the SIGPIPE to indicate the output wasn't fully\n>> consumed.\n>\n> But the user doesn't care: he quit the pager because he didn't\n> need more output. So there is no need to signal that the output\n> wasn't fully consumed. The user already knew that before quitting\n> the pager!\n\nAs noted above, this is assuming way too much about the functionality of\nthe pager command. We can get a SIGPIPE without the user's intent in\nthis way. Consider e.g. piping to some remote system via netcat.\n\n>> > [...]\n>> >> Maybe we have users who'd like to work around zsh's \"setopt\n>> >> PRINT_EXIT_VALUE\" mode (would you want this patch if you could make zsh\n>> >> ignore 141?).\n>> >\n>> > zsh is working as expected, and as I've already said, I ***WANT***\n>> > SIGPIPE to be reported by the shell, as it may indicate a real failure\n>> > in a script. BTW, I even have a script using git that relies on that:\n>> >\n>> > { git rev-list --author \"$@[-1]\" HEAD &&\n>> > git rev-list --grep \"$@[-1]\" HEAD } | \\\n>> > git \"${@[1,-2]:-lv}\" --no-walk --stdin\n>> >\n>> > return $((pipestatus[2] ? pipestatus[2] : pipestatus[1]))\n>> >\n>> > Here it is important not to lose any information. No pager is\n>> > involved, the full output is needed. If for some reason, the\n>> > LHS of the pipe fails due to a SIGPIPE but the right hand side\n>> > succeeds, the error will be reported.\n>> \n>> Sorry, I really don't see how this is different. I think this goes back\n>> to my \"'|' shell piping construct[...]\" question in the E-Mail you're\n>> replying to.\n>> \n>> in both the \"git log &&\" case and potentially here you'll get a program\n>> writing to a pipe getting a SIGPIPE, which is then reflected in the exit\n>> code.\n>\n> I mean that there are SIGPIPEs that one does not want to ignore\n> (because they would indicate a problem -- in general in scripts),\n> and other ones that should be ignored because they don't indicate\n> an error.\n>\n>> > The fact is that with a pager, the SIGPIPE with a pager is normal.\n>> > Thus with a pager, git is reporting a spurious SIGPIPE, and this\n>> > is disturbing.\n>> \n>> I don't get what you're trying to say here, sorry.\n>\n> I mean that when the user quits the pager, there is no reason to\n> report an error because the user explicitly wanted to quit now.\n\nSure, in an ideal world. But we don't get a SIGUSERPRESSEDTHEQBUTTON, we\nget a SIGPIPE.\n\n> Similarly, if I run a text viewer on a file, I don't want a SIGPIPE\n> to be reported if I do not go to the end of the file (if a pipe was\n> used to read the file, e.g. to do some filtering, as \"less\" can do).\n\nYes, that makes perfect sense. Neither would I, but that text viewer is\none process, so it doesn't have to deal with IPC and propagating exit\ncodes from failed IPC.\n\n>> Maybe this helps. So first, I don't know if your report came out of\n>> reading the recent \"set -o pipefail\" traffic on-list. As you can see in\n>> [1] I'm not some zealot for PIPEFAIL always being returned no matter\n>> what.\n>\n> This is not related. And [1] is from 2021 (with a thread started\n> in 2019), while my report dates back to 2018:\n>\n> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914896\n\nIndeed, I just misread (or didn't read in the first place) the times\ninvolved. I started reading at Denton Liu's patch sent a couple of days\nago.\n\n> Moreover, [1] is only about the use of pipes in the shell command.\n> My bug report is about the internal use of a pager by git.\n\nI probably shouldn't have linked to that thread, but as as noted at the\nstart of the E-Mail I was referring to it for the SIGPIPE behavior\ndiscussed there, not bash/set -o pipefail etc.\n\n>> The difference between that though and what you're proposing is there\n>> you have the shell getting an exit code and opting to ignore it, as\n>> opposed to the program itself sweeping it under the rug.\n>> \n>> I don't think either that just because you run a pager you're obligated\n>> to ferry down a SIGPIPE if you get it. E.g. mysql and postgresql both\n>> have interactive shells where you can open pagers. I didn't bother to\n>> check, but you can imagine doing a \"show tables\" or whatever and only\n>> viewing the first page, then quitting in the pager.\n>> \n>> If that's part of a long interactive SQL session it would make no sense\n>> for the eventual exit code of mysql(1) or psql(1) to reflect that.\n>> \n>> But with git we're (mostly) executing one-shot commands, e.g. with \"git\n>> log\" you give it some params, and it spews all the output at you, maybe\n>> with the help of a pager.\n>> \n>> So then if we fail on the write() I don't see how it doesn't make sense\n>> to return the appropriate exit code for that failure downstream.\n>\n> This depends on the kind of error. I agree for an unexpected error.\n> But for a broken pipe because git started a pager on its own and\n> the user chose to quit the pager, this should not be regarded as\n> an error.\n\nAs noted above, we don't have a way of knowing that, we're not the\npager.\n\nIt also seems to me that whether git should report errors, and what 3rd\nparty tools that might invoke git are going to do with a SIGPIPE exit\ncode is being mixed up here.\n\nAnd then whether it makes sense to ignore SIGPIPE for all users, or\ne.g. if it's some opt-in setting in some situations that users might\nwant to turn on because they're aware of how their pager behaves and\nwant to work around some zsh mode.\n\n>> 1. https://lore.kernel.org/git/20210116153554.12604-12-avarab@gmail.com/\n\n"},{"id":"415776","messageId":"xmqqtuqvn0i7.fsf@gitster.c.googlers.com","threadId":"54992","inReplyTo":"87czxjomn8.fsf@evledraar.gmail.com","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-01T17:47:28Z","receivedAt":"2021-02-01T17:48:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> diff --git a/pager.c b/pager.c\n>> index ee435de675..5922d99dc8 100644\n>> --- a/pager.c\n>> +++ b/pager.c\n>> @@ -34,6 +34,8 @@ static void wait_for_pager_atexit(void)\n>> static void wait_for_pager_signal(int signo)\n>> {\n>> \twait_for_pager(1);\n>> +\tif (signo == SIGPIPE)\n>> +\t\texit(0);\n>\n> As shown in\n> https://lore.kernel.org/git/20210201144921.8664-1-avarab@gmail.com/ this\n> leaves us without guard rails where the pager dies/segfaults or\n> whatever.\n>\n> That's an existing bug, but by not carrying the SIGPIPE forward it\n> changes from \"most of the time we'd exit with SIGPIPE anyway\" to \"we'll\n> never notice\".\n\nWould it be the matter of propagating the exit status of the pager\nnoticed by wait_or_white() down thru finish_command_in_signal() and\nwait_for_pager(1) to here, so\n\n - If we know pager exited with non-zero status, we would report,\n perhaps with warning(_(\"...\"));\n\n - If we notice we got a SIGPIPE, we ignore it---it is nothing of\n interest to the end-user;\n\n - Otherwise we do not do anything differently.\n\nwould be sufficient? Implementors of \"git -p\" may know that \"git\"\nhappens to implement its paging by piping its output to an external\npager, but the end-users do not care. Implementors may say they are\ngiving 'q' to their pager \"less\", but to the end-users, who report\n\"I ran 'git log' and after reading a pageful, I told it to 'q'uit\",\nthe distinction does not have any importance.\n\nOr are there more to it, in that the exit status we get from the\npager, combined with the kind of signal we are getting, is not\nsufficient for us to tell what is going on?\n"},{"id":"415780","messageId":"xmqqft2fmzk6.fsf@gitster.c.googlers.com","threadId":"54992","inReplyTo":"20210201144921.8664-4-avarab@gmail.com","subject":"Re: [PATCH 3/3] pager: properly log pager exit code when signalled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-01T18:07:53Z","receivedAt":"2021-02-01T18:08:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> When git invokes a pager that exits with non-zero the common case is\n> that we'll already return the correct SIGPIPE failure from git itself,\n> but the exit code logged in trace2 has always been incorrectly\n> reported[1]. Fix that and log the correct exit code in the logs.\n>\n> Since this gives us something to test outside of our recently-added\n> tests needing a !MINGW prerequisite, let's refactor the test to run on\n> MINGW and actually check for SIGPIPE outside of MINGW.\n>\n> The wait_or_whine() is only called with a true \"in_signal\" from from\n> finish_command_in_signal(), which in turn is only used in pager.c.\n>\n> I'm not quite sure about that BUG() case. Can we have a true in_signal\n> and not have a true WIFEXITED(status)? I haven't been able to think of\n> a test case for it.\n>\n> 1. The incorrect logging of the exit code in was seemingly copy/pasted\n> into finish_command_in_signal() in ee4512ed481 (trace2: create new\n> combined trace facility, 2019-02-22)\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> run-command.c | 8 +++++--\n> t/t7006-pager.sh | 61 +++++++++++++++++++++++++++++++++++++++++-------\n> 2 files changed, 58 insertions(+), 11 deletions(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index ea4d0fb4b15..10e1c96c2bd 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -551,8 +551,12 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n> \n> \twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n> \t\t;\t/* nothing */\n> -\tif (in_signal)\n> -\t\treturn 0;\n> +\tif (in_signal && WIFEXITED(status))\n> +\t\treturn WEXITSTATUS(status);\n> +\tif (in_signal) {\n> +\t\tBUG(\"was not expecting waitpid() status %d\", status);\n> +\t\treturn -1;\n> +\t}\n\nDoesn't BUG die, never to return control back to us? How about\n\"warning()\" or \"error()\"?\n"},{"id":"415785","messageId":"xmqqbld3mz85.fsf@gitster.c.googlers.com","threadId":"54992","inReplyTo":"20210201144921.8664-4-avarab@gmail.com","subject":"Re: [PATCH 3/3] pager: properly log pager exit code when signalled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-01T18:15:06Z","receivedAt":"2021-02-01T18:30:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> diff --git a/run-command.c b/run-command.c\n> index ea4d0fb4b15..10e1c96c2bd 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -551,8 +551,12 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n> \n> \twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n> \t\t;\t/* nothing */\n> -\tif (in_signal)\n> -\t\treturn 0;\n> +\tif (in_signal && WIFEXITED(status))\n> +\t\treturn WEXITSTATUS(status);\n> +\tif (in_signal) {\n> +\t\tBUG(\"was not expecting waitpid() status %d\", status);\n> +\t\treturn -1;\n> +\t}\n\nThis starts reporting exit status of the pager back to\nfinish_command() and finish_command_in_signal(). But the code in\npager.c that call the finish_command*() ignore the returned value.\n\nSo, is the net result of these three patches just that the trace2\noutput gives the exit status of the pager, but \"git\" itself is not\naffected otherwise (not a complaint; trying to understand the\nintention) ?\n\n> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\n> index c60886f43e6..1424466caf5 100755\n> --- a/t/t7006-pager.sh\n> +++ b/t/t7006-pager.sh\n> @@ -656,31 +656,74 @@ test_expect_success TTY 'git tag with auto-columns ' '\n> \ttest_cmp expect actual\n> '\n> \n> -test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager exit' '\n> +test_expect_success TTY 'git returns SIGPIPE on early pager exit' '\n\nI somehow find 2/3 of this change belongs to the previous step.\nThat is, shouldn't this new test added in the previous step without\nthe !MINGW prerequisite, but without the trace2 bits (i.e. the first\nthree added lines and the last \"grep\" of trace.normal), and the change\nmade in this step limited only to those trace2 bits?\n\n> \ttest_when_finished \"rm pager-used\" &&\n> \ttest_config core.pager \">pager-used; head -n 1; exit 0\" &&\n> +\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n> +\texport GIT_TRACE2 &&\n> +\ttest_when_finished \"unset GIT_TRACE2\" &&\n> \n> -\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n> -\ttest_match_signal 13 \"$OUT\" &&\n> +\tif test_have_prereq !MINGW\n> +\tthen\n> +\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n> +\t\ttest_match_signal 13 \"$OUT\"\n> +\telse\n> +\t\ttest_terminal git log\n> +\tfi &&\n> +\tgrep \"child_exit.* code:0 \" trace.normal &&\n> \ttest_path_is_file pager-used\n> '\n\nThe same comment applies to this new one added in this step.\nShouldn't the bulk of the test (i.e. under !MINGW we get killed with\nsignal 13) be introduced in the previous step?\n\n\n> +test_expect_success TTY 'git logs nonexisting pager invocation' '\n> +\ttest_config core.pager \"does-not-exist\" &&\n> +\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n> +\texport GIT_TRACE2 &&\n> +\ttest_when_finished \"unset GIT_TRACE2\" &&\n> +\n> +\tif test_have_prereq !MINGW\n> +\tthen\n> +\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n> +\t\ttest_match_signal 13 \"$OUT\"\n> +\telse\n> +\t\ttest_terminal git log\n> +\tfi &&\n> +\tgrep \"child_exit.* code:-1 \" trace.normal\n> +'\n> +\n> test_done\n"},{"id":"415788","messageId":"8735yffvbj.fsf@evledraar.gmail.com","threadId":"54992","inReplyTo":"xmqqft2fmzk6.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/3] pager: properly log pager exit code when signalled","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-01T19:21:20Z","receivedAt":"2021-02-01T19:22:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 01 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> When git invokes a pager that exits with non-zero the common case is\n>> that we'll already return the correct SIGPIPE failure from git itself,\n>> but the exit code logged in trace2 has always been incorrectly\n>> reported[1]. Fix that and log the correct exit code in the logs.\n>>\n>> Since this gives us something to test outside of our recently-added\n>> tests needing a !MINGW prerequisite, let's refactor the test to run on\n>> MINGW and actually check for SIGPIPE outside of MINGW.\n>>\n>> The wait_or_whine() is only called with a true \"in_signal\" from from\n>> finish_command_in_signal(), which in turn is only used in pager.c.\n>>\n>> I'm not quite sure about that BUG() case. Can we have a true in_signal\n>> and not have a true WIFEXITED(status)? I haven't been able to think of\n>> a test case for it.\n>>\n>> 1. The incorrect logging of the exit code in was seemingly copy/pasted\n>> into finish_command_in_signal() in ee4512ed481 (trace2: create new\n>> combined trace facility, 2019-02-22)\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> run-command.c | 8 +++++--\n>> t/t7006-pager.sh | 61 +++++++++++++++++++++++++++++++++++++++++-------\n>> 2 files changed, 58 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/run-command.c b/run-command.c\n>> index ea4d0fb4b15..10e1c96c2bd 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -551,8 +551,12 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n>> \n>> \twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n>> \t\t;\t/* nothing */\n>> -\tif (in_signal)\n>> -\t\treturn 0;\n>> +\tif (in_signal && WIFEXITED(status))\n>> +\t\treturn WEXITSTATUS(status);\n>> +\tif (in_signal) {\n>> +\t\tBUG(\"was not expecting waitpid() status %d\", status);\n>> +\t\treturn -1;\n>> +\t}\n>\n> Doesn't BUG die, never to return control back to us? How about\n> \"warning()\" or \"error()\"?\n\nMaybe I shouldn't do that, but I'm doing it reflexively because SunCC\nwill yell at me otherwise. See 56f56ac50b9 (style: do not \"break\" in\nswitch() after \"return\", 2020-12-16).\n\nMaybe I should just deal with its complaints, or add an \"/* unreachable\n*/\" comment there...\n"},{"id":"415789","messageId":"87zh0negnx.fsf@evledraar.gmail.com","threadId":"54992","inReplyTo":"xmqqbld3mz85.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/3] pager: properly log pager exit code when signalled","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-01T19:23:14Z","receivedAt":"2021-02-01T19:24:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 01 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> diff --git a/run-command.c b/run-command.c\n>> index ea4d0fb4b15..10e1c96c2bd 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -551,8 +551,12 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n>> \n>> \twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n>> \t\t;\t/* nothing */\n>> -\tif (in_signal)\n>> -\t\treturn 0;\n>> +\tif (in_signal && WIFEXITED(status))\n>> +\t\treturn WEXITSTATUS(status);\n>> +\tif (in_signal) {\n>> +\t\tBUG(\"was not expecting waitpid() status %d\", status);\n>> +\t\treturn -1;\n>> +\t}\n>\n> This starts reporting exit status of the pager back to\n> finish_command() and finish_command_in_signal(). But the code in\n> pager.c that call the finish_command*() ignore the returned value.\n>\n> So, is the net result of these three patches just that the trace2\n> output gives the exit status of the pager, but \"git\" itself is not\n> affected otherwise (not a complaint; trying to understand the\n> intention) ?\n\nYes, it's just a bug fix for the trace2 output.\n\n>> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\n>> index c60886f43e6..1424466caf5 100755\n>> --- a/t/t7006-pager.sh\n>> +++ b/t/t7006-pager.sh\n>> @@ -656,31 +656,74 @@ test_expect_success TTY 'git tag with auto-columns ' '\n>> \ttest_cmp expect actual\n>> '\n>> \n>> -test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager exit' '\n>> +test_expect_success TTY 'git returns SIGPIPE on early pager exit' '\n>\n> I somehow find 2/3 of this change belongs to the previous step.\n> That is, shouldn't this new test added in the previous step without\n> the !MINGW prerequisite, but without the trace2 bits (i.e. the first\n> three added lines and the last \"grep\" of trace.normal), and the change\n> made in this step limited only to those trace2 bits?\n\nSure, I can re-arrange it like that. I figured 1/3 shouldn't assume 3/3,\nthere wouldn't be a reason not to use !MINGW except because 3/3 is going\nto remove it later, but I guess it makes the overall history easier to\nread...\n\n>> \ttest_when_finished \"rm pager-used\" &&\n>> \ttest_config core.pager \">pager-used; head -n 1; exit 0\" &&\n>> +\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n>> +\texport GIT_TRACE2 &&\n>> +\ttest_when_finished \"unset GIT_TRACE2\" &&\n>> \n>> -\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n>> -\ttest_match_signal 13 \"$OUT\" &&\n>> +\tif test_have_prereq !MINGW\n>> +\tthen\n>> +\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n>> +\t\ttest_match_signal 13 \"$OUT\"\n>> +\telse\n>> +\t\ttest_terminal git log\n>> +\tfi &&\n>> +\tgrep \"child_exit.* code:0 \" trace.normal &&\n>> \ttest_path_is_file pager-used\n>> '\n>\n> The same comment applies to this new one added in this step.\n> Shouldn't the bulk of the test (i.e. under !MINGW we get killed with\n> signal 13) be introduced in the previous step?\n\n*nod*\n\n>> +test_expect_success TTY 'git logs nonexisting pager invocation' '\n>> +\ttest_config core.pager \"does-not-exist\" &&\n>> +\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n>> +\texport GIT_TRACE2 &&\n>> +\ttest_when_finished \"unset GIT_TRACE2\" &&\n>> +\n>> +\tif test_have_prereq !MINGW\n>> +\tthen\n>> +\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n>> +\t\ttest_match_signal 13 \"$OUT\"\n>> +\telse\n>> +\t\ttest_terminal git log\n>> +\tfi &&\n>> +\tgrep \"child_exit.* code:-1 \" trace.normal\n>> +'\n>> +\n>> test_done\n\n"},{"id":"415807","messageId":"87wnvrefbv.fsf@evledraar.gmail.com","threadId":"54992","inReplyTo":"xmqqtuqvn0i7.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-01T19:52:04Z","receivedAt":"2021-02-01T19:54:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 01 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>>> diff --git a/pager.c b/pager.c\n>>> index ee435de675..5922d99dc8 100644\n>>> --- a/pager.c\n>>> +++ b/pager.c\n>>> @@ -34,6 +34,8 @@ static void wait_for_pager_atexit(void)\n>>> static void wait_for_pager_signal(int signo)\n>>> {\n>>> \twait_for_pager(1);\n>>> +\tif (signo == SIGPIPE)\n>>> +\t\texit(0);\n>>\n>> As shown in\n>> https://lore.kernel.org/git/20210201144921.8664-1-avarab@gmail.com/ this\n>> leaves us without guard rails where the pager dies/segfaults or\n>> whatever.\n>>\n>> That's an existing bug, but by not carrying the SIGPIPE forward it\n>> changes from \"most of the time we'd exit with SIGPIPE anyway\" to \"we'll\n>> never notice\".\n>\n> Would it be the matter of propagating the exit status of the pager\n> noticed by wait_or_white() down thru finish_command_in_signal() and\n> wait_for_pager(1) to here, so\n>\n> - If we know pager exited with non-zero status, we would report,\n> perhaps with warning(_(\"...\"));\n>\n> - If we notice we got a SIGPIPE, we ignore it---it is nothing of\n> interest to the end-user;\n>\n> - Otherwise we do not do anything differently.\n>\n> would be sufficient? Implementors of \"git -p\" may know that \"git\"\n> happens to implement its paging by piping its output to an external\n> pager, but the end-users do not care. Implementors may say they are\n> giving 'q' to their pager \"less\", but to the end-users, who report\n> \"I ran 'git log' and after reading a pageful, I told it to 'q'uit\",\n> the distinction does not have any importance.\n>\n> Or are there more to it, in that the exit status we get from the\n> pager, combined with the kind of signal we are getting, is not\n> sufficient for us to tell what is going on?\n\nIt is, I just wonder if ignoring the exit code is a practical issue as\nlong as we're not clobbering SIGPIPE, particularly with my trace2\nlogging patch in this thread.\n\nBut yeah, we could patch git to handle this in the general case. I think\nit's probably a bit of a PITA to do, since for the general case we need\nto munge the exit code in an atexit() handler.\n\nWhich means calling _exit() (if that's even portable), and presumably\nchanging from the atexit() API to our own registry of how many times we\ncalled atexit(), which would introduce logic bugs if we ever use a\nlibrary that wants to have atexit(). I.e. if we _exit() before its\natexit() handler runs because we wanted to munge the exit code.\n\n"},{"id":"415814","messageId":"xmqq8s87ld8y.fsf@gitster.c.googlers.com","threadId":"54992","inReplyTo":"87wnvrefbv.fsf@evledraar.gmail.com","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-01T20:55:09Z","receivedAt":"2021-02-01T20:55:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> Would it be the matter of propagating the exit status of the pager\n>> noticed by wait_or_white() down thru finish_command_in_signal() and\n>> wait_for_pager(1) to here, so\n>>\n>> - If we know pager exited with non-zero status, we would report,\n>> perhaps with warning(_(\"...\"));\n>>\n>> - If we notice we got a SIGPIPE, we ignore it---it is nothing of\n>> interest to the end-user;\n>>\n>> - Otherwise we do not do anything differently.\n>>\n>> would be sufficient? Implementors of \"git -p\" may know that \"git\"\n>> happens to implement its paging by piping its output to an external\n>> pager, but the end-users do not care. Implementors may say they are\n>> giving 'q' to their pager \"less\", but to the end-users, who report\n>> \"I ran 'git log' and after reading a pageful, I told it to 'q'uit\",\n>> the distinction does not have any importance.\n>>\n>> Or are there more to it, in that the exit status we get from the\n>> pager, combined with the kind of signal we are getting, is not\n>> sufficient for us to tell what is going on?\n>\n> It is, I just wonder if ignoring the exit code is a practical issue as\n> long as we're not clobbering SIGPIPE, particularly with my trace2\n> logging patch in this thread.\n>\n> But yeah, we could patch git to handle this in the general case....\n\nSorry, but now you lost me.\n\nI was merely wondering if Denton's patch can become a small update\non top of these, if we just made sure that the exit code of the\npager noticed by wait_or_whine() is reported to the code where\nDenton makes the decision to say \"let's not re-raise but simply exit\nwith 0 return as what we got is SIGPIPE\". I guess we could even\nmake git exit with the pager's return code in that case, as the\nend-user observable result would be similar to \"git log | less\"\nwhere 'less' may be segfaulting or exiting cleanly.\n\nIOW, something like this on top of your three-patch series?\n\n pager.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git c/pager.c w/pager.c\nindex 3d37dd7ada..73bc5fc0e4 100644\n--- c/pager.c\n+++ w/pager.c\n@@ -28,8 +28,14 @@ static void wait_for_pager_atexit(void)\n \n static void wait_for_pager_signal(int signo)\n {\n+\tint status;\n+\n \tclose_pager_fds();\n-\tfinish_command_in_signal(&pager_process);\n+\tstatus = finish_command_in_signal(&pager_process);\n+\n+\tif (signo == SIGPIPE)\n+\t\texit(status);\n+\n \tsigchain_pop(signo);\n \traise(signo);\n }\n"},{"id":"415829","messageId":"2f750bc9-e739-6b98-25a1-6f035123e0e0@kdbg.org","threadId":"54992","inReplyTo":"87o8h4omqa.fsf@evledraar.gmail.com","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-02-01T22:04:12Z","receivedAt":"2021-02-01T22:05:48Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 31.01.21 um 21:49 schrieb Ævar Arnfjörð Bjarmason:\n> On Sun, Jan 31 2021, Vincent Lefevre wrote:\n>> On 2021-01-31 02:47:59 +0100, Ævar Arnfjörð Bjarmason wrote:\n>>> On Fri, Jan 15 2021, Vincent Lefevre wrote:\n>>>> And of course, I don't want to hide error messages by default, because\n>>>> this would hide *real* errors.\n>>>\n>>> Isn't the solution to this that your shell stops reporting failures due\n>>> to SIGPIPE in such a prominent way then?\n>>\n>> No! I want to be warned about real SIGPIPEs.\n> \n> Not being able to write \"git log\" output is a real SIGPIPE.\n\nWhen Git is talking to a pager *and* it knows about it because has\nstarted it itself, SIGPIPE is just a nuisance, not a useful behavior.\n\nGuess why `git log` works on Windows when the pager is quit early, where\nwe do not have SIGPIPE? Because write errors are checked in sufficiently\nmany places.\n\nI propose to do just this:\n\ndiff --git a/pager.c b/pager.c\nindex ee435de675..9fcc36425f 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -138,6 +138,7 @@ void setup_pager(void)\n \n \t/* this makes sure that the parent terminates after the pager */\n \tsigchain_push_common(wait_for_pager_signal);\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n \tatexit(wait_for_pager_atexit);\n }\n \ndiff --git a/run-command.c b/run-command.c\nindex ea4d0fb4b1..c0041413b5 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1165,10 +1165,6 @@ void check_pipe(int err)\n \tif (err == EPIPE) {\n \t\tif (in_async())\n \t\t\tasync_exit(141);\n-\n-\t\tsignal(SIGPIPE, SIG_DFL);\n-\t\traise(SIGPIPE);\n-\t\t/* Should never happen, but just in case... */\n \t\texit(141);\n \t}\n }\n"},{"id":"415836","messageId":"5772995f-c887-7f13-6b5f-dc44f4477dcb@kdbg.org","threadId":"54992","inReplyTo":"87a6snokrr.fsf@evledraar.gmail.com","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-02-01T22:16:27Z","receivedAt":"2021-02-01T22:17:13Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 01.02.21 um 16:44 schrieb Ævar Arnfjörð Bjarmason:\n> On Mon, Feb 01 2021, Vincent Lefevre wrote:\n>> On 2021-02-01 13:10:21 +0100, Ævar Arnfjörð Bjarmason:\n>>> So we've got the SIGPIPE to indicate the output wasn't fully\n>>> consumed.\n>>\n>> But the user doesn't care: he quit the pager because he didn't\n>> need more output. So there is no need to signal that the output\n>> wasn't fully consumed. The user already knew that before quitting\n>> the pager!\n> \n> As noted above, this is assuming way too much about the functionality of\n> the pager command. We can get a SIGPIPE without the user's intent in\n> this way. Consider e.g. piping to some remote system via netcat.\n\nThat assumption is warranted, IMO. Aren't _you_ stretching the meaning\nof \"pager\" too far here? A pager is intended for presentation to the\nuser. If someone plays games with it, they should know what they get.\n\n-- Hannes\n"},{"id":"415848","messageId":"20210202020001.31601-2-avarab@gmail.com","threadId":"54992","inReplyTo":"20210201144921.8664-1-avarab@gmail.com","subject":"[PATCH v2 1/5] pager: refactor wait_for_pager() function","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-02T01:59:57Z","receivedAt":"2021-02-02T02:01:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor the wait_for_pager() function. Since 507d7804c0b (pager:\ndon't use unsafe functions in signal handlers, 2015-09-04) the\nwait_for_pager() and wait_for_pager_atexit() callers diverged on more\nthan they shared.\n\nLet's extract the common code into a new close_pager_fds() helper, and\nmove the parts unique to the only to callers to those functions.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n pager.c | 18 +++++++-----------\n 1 file changed, 7 insertions(+), 11 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex ee435de6756..3d37dd7adaa 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -11,29 +11,25 @@\n static struct child_process pager_process = CHILD_PROCESS_INIT;\n static const char *pager_program;\n \n-static void wait_for_pager(int in_signal)\n+static void close_pager_fds(void)\n {\n-\tif (!in_signal) {\n-\t\tfflush(stdout);\n-\t\tfflush(stderr);\n-\t}\n \t/* signal EOF to pager */\n \tclose(1);\n \tclose(2);\n-\tif (in_signal)\n-\t\tfinish_command_in_signal(&pager_process);\n-\telse\n-\t\tfinish_command(&pager_process);\n }\n \n static void wait_for_pager_atexit(void)\n {\n-\twait_for_pager(0);\n+\tfflush(stdout);\n+\tfflush(stderr);\n+\tclose_pager_fds();\n+\tfinish_command(&pager_process);\n }\n \n static void wait_for_pager_signal(int signo)\n {\n-\twait_for_pager(1);\n+\tclose_pager_fds();\n+\tfinish_command_in_signal(&pager_process);\n \tsigchain_pop(signo);\n \traise(signo);\n }\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"415849","messageId":"20210202020001.31601-1-avarab@gmail.com","threadId":"54992","inReplyTo":"20210201144921.8664-1-avarab@gmail.com","subject":"[PATCH v2 0/5] pager: test for exit behavior & trace2 bug fix","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-02T01:59:56Z","receivedAt":"2021-02-02T02:01:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A v2 with better tests and some misc adjustments. As noted in v1 1-4\nare just adding better test coverage for behavior we already have, and\nfixing a small bug in trace2 output.\n\nThe 5/5 is a WIP start at respecting the pager's exit code and\nignoring the SIGPIPE. Junio had a suggestion to do that in\n, but as seen & noted there\nit's quite a bit more complex when we have to deal with the atexit\nsibling function.\n\nÆvar Arnfjörð Bjarmason (5):\n pager: refactor wait_for_pager() function\n pager: test for exit code with and without SIGPIPE\n run-command: add braces for \"if\" block in wait_or_whine()\n pager: properly log pager exit code when signalled\n WIP pager: respect exit code of pager over SIGPIPE\n\n pager.c | 24 +++++----\n run-command.c | 7 ++-\n t/t7006-pager.sh | 130 +++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 148 insertions(+), 13 deletions(-)\n\nRange-diff:\n2: 6509ae44751 = 1: aab89cc8619 pager: refactor wait_for_pager() function\n1: cba284dcf55 ! 2: edf513bb174 pager: test for exit code\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - pager: test for exit code\n + pager: test for exit code with and without SIGPIPE\n \n Add tests for how git behaves when the pager itself exits with\n non-zero, as well as for us exiting with 141 when we're killed with\n @@ Commit message\n current behavior.\n \n This test construct is stolen from 7559a1be8a0 (unblock and unignore\n - SIGPIPE, 2014-09-18).\n + SIGPIPE, 2014-09-18). The reason not to make the test itself depend on\n + the MINGW prerequisite is to make a subsequent commit easier to read.\n \n 1. https://lore.kernel.org/git/87o8h4omqa.fsf@evledraar.gmail.com/\n \n @@ t/t7006-pager.sh: test_expect_success TTY 'git tag with auto-columns ' '\n \ttest_cmp expect actual\n '\n \n -+test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager exit' '\n ++test_expect_success TTY 'git returns SIGPIPE on early pager exit' '\n +\ttest_when_finished \"rm pager-used\" &&\n +\ttest_config core.pager \">pager-used; head -n 1; exit 0\" &&\n +\n -+\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n -+\ttest_match_signal 13 \"$OUT\" &&\n ++\tif test_have_prereq !MINGW\n ++\tthen\n ++\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n ++\t\ttest_match_signal 13 \"$OUT\"\n ++\telse\n ++\t\ttest_terminal git log\n ++\tfi &&\n +\ttest_path_is_file pager-used\n +'\n +\n -+test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager non-zero exit' '\n ++test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '\n +\ttest_when_finished \"rm pager-used\" &&\n +\ttest_config core.pager \">pager-used; head -n 1; exit 1\" &&\n +\n -+\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n -+\ttest_match_signal 13 \"$OUT\" &&\n ++\tif test_have_prereq !MINGW\n ++\tthen\n ++\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n ++\t\ttest_match_signal 13 \"$OUT\"\n ++\telse\n ++\t\ttest_terminal git log\n ++\tfi &&\n +\ttest_path_is_file pager-used\n +'\n +\n -+test_expect_success TTY,!MINGW 'git discards pager non-zero exit' '\n ++test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '\n +\ttest_when_finished \"rm pager-used\" &&\n +\ttest_config core.pager \"wc >pager-used; exit 1\" &&\n +\n -+\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n -+\ttest \"$OUT\" -eq 0 &&\n ++\tif test_have_prereq !MINGW\n ++\tthen\n ++\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n ++\t\ttest \"$OUT\" -eq 0\n ++\telse\n ++\t\ttest_terminal git log\n ++\tfi &&\n ++\ttest_path_is_file pager-used\n ++'\n ++\n ++test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '\n ++\ttest_when_finished \"rm pager-used\" &&\n ++\ttest_config core.pager \"wc >pager-used; does-not-exist\" &&\n ++\n ++\tif test_have_prereq !MINGW\n ++\tthen\n ++\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n ++\t\ttest \"$OUT\" -eq 0\n ++\telse\n ++\t\ttest_terminal git log\n ++\tfi &&\n ++\ttest_path_is_file pager-used\n ++'\n ++\n ++test_expect_success TTY 'git attempts to page to nonexisting pager command, gets SIGPIPE' '\n ++\ttest_config core.pager \"does-not-exist\" &&\n ++\n ++\tif test_have_prereq !MINGW\n ++\tthen\n ++\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n ++\t\ttest_match_signal 13 \"$OUT\"\n ++\telse\n ++\t\ttest_terminal git log\n ++\tfi\n ++'\n ++\n ++test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n ++\ttest_when_finished \"rm pager-used\" &&\n ++\ttest_config core.pager \">pager-used; test-tool sigchain\" &&\n ++\n ++\tif test_have_prereq !MINGW\n ++\tthen\n ++\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n ++\t\ttest_match_signal 13 \"$OUT\"\n ++\telse\n ++\t\ttest_terminal git log\n ++\tfi &&\n +\ttest_path_is_file pager-used\n +'\n +\n-: ----------- > 3: 0e4cbf80fe1 run-command: add braces for \"if\" block in wait_or_whine()\n3: d5db936bd11 ! 4: 527f69cf581 pager: properly log pager exit code when signalled\n @@ Commit message\n The wait_or_whine() is only called with a true \"in_signal\" from from\n finish_command_in_signal(), which in turn is only used in pager.c.\n \n - I'm not quite sure about that BUG() case. Can we have a true in_signal\n - and not have a true WIFEXITED(status)? I haven't been able to think of\n - a test case for it.\n + The \"in_signal && !WIFEXITED(status)\" case is not covered by\n + tests. Let's log the default -1 in that case for good measure.\n \n 1. The incorrect logging of the exit code in was seemingly copy/pasted\n into finish_command_in_signal() in ee4512ed481 (trace2: create new\n @@ Commit message\n \n ## run-command.c ##\n @@ run-command.c: static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n - \n \twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n \t\t;\t/* nothing */\n --\tif (in_signal)\n + \tif (in_signal) {\n -\t\treturn 0;\n -+\tif (in_signal && WIFEXITED(status))\n -+\t\treturn WEXITSTATUS(status);\n -+\tif (in_signal) {\n -+\t\tBUG(\"was not expecting waitpid() status %d\", status);\n -+\t\treturn -1;\n -+\t}\n ++\t\tif (WIFEXITED(status))\n ++\t\t\tcode = WEXITSTATUS(status);\n ++\t\treturn code;\n + \t}\n \n \tif (waiting < 0) {\n - \t\tfailed_errno = errno;\n \n ## t/t7006-pager.sh ##\n @@ t/t7006-pager.sh: test_expect_success TTY 'git tag with auto-columns ' '\n \ttest_cmp expect actual\n '\n \n --test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager exit' '\n -+test_expect_success TTY 'git returns SIGPIPE on early pager exit' '\n - \ttest_when_finished \"rm pager-used\" &&\n ++test_expect_success 'setup trace2' '\n ++\tGIT_TRACE2_BRIEF=1 &&\n ++\texport GIT_TRACE2_BRIEF\n ++'\n ++\n + test_expect_success TTY 'git returns SIGPIPE on early pager exit' '\n +-\ttest_when_finished \"rm pager-used\" &&\n ++\ttest_when_finished \"rm pager-used trace.normal\" &&\n \ttest_config core.pager \">pager-used; head -n 1; exit 0\" &&\n +\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n +\texport GIT_TRACE2 &&\n +\ttest_when_finished \"unset GIT_TRACE2\" &&\n \n --\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n --\ttest_match_signal 13 \"$OUT\" &&\n -+\tif test_have_prereq !MINGW\n -+\tthen\n -+\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n -+\t\ttest_match_signal 13 \"$OUT\"\n -+\telse\n -+\t\ttest_terminal git log\n -+\tfi &&\n -+\tgrep \"child_exit.* code:0 \" trace.normal &&\n + \tif test_have_prereq !MINGW\n + \tthen\n +@@ t/t7006-pager.sh: test_expect_success TTY 'git returns SIGPIPE on early pager exit' '\n + \telse\n + \t\ttest_terminal git log\n + \tfi &&\n ++\n ++\tgrep child_exit trace.normal >child-exits &&\n ++\ttest_line_count = 1 child-exits &&\n ++\tgrep \" code:0 \" child-exits &&\n \ttest_path_is_file pager-used\n '\n \n --test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager non-zero exit' '\n -+test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '\n - \ttest_when_finished \"rm pager-used\" &&\n + test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '\n +-\ttest_when_finished \"rm pager-used\" &&\n ++\ttest_when_finished \"rm pager-used trace.normal\" &&\n \ttest_config core.pager \">pager-used; head -n 1; exit 1\" &&\n +\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n +\texport GIT_TRACE2 &&\n +\ttest_when_finished \"unset GIT_TRACE2\" &&\n \n --\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n --\ttest_match_signal 13 \"$OUT\" &&\n -+\tif test_have_prereq !MINGW\n -+\tthen\n -+\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n -+\t\ttest_match_signal 13 \"$OUT\"\n -+\telse\n -+\t\ttest_terminal git log\n -+\tfi &&\n -+\tgrep \"child_exit.* code:1 \" trace.normal &&\n + \tif test_have_prereq !MINGW\n + \tthen\n +@@ t/t7006-pager.sh: test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '\n + \telse\n + \t\ttest_terminal git log\n + \tfi &&\n ++\n ++\tgrep child_exit trace.normal >child-exits &&\n ++\ttest_line_count = 1 child-exits &&\n ++\tgrep \" code:1 \" child-exits &&\n \ttest_path_is_file pager-used\n '\n \n --test_expect_success TTY,!MINGW 'git discards pager non-zero exit' '\n -+test_expect_success TTY 'git discards pager non-zero exit' '\n - \ttest_when_finished \"rm pager-used\" &&\n + test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '\n +-\ttest_when_finished \"rm pager-used\" &&\n ++\ttest_when_finished \"rm pager-used trace.normal\" &&\n \ttest_config core.pager \"wc >pager-used; exit 1\" &&\n +\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n +\texport GIT_TRACE2 &&\n +\ttest_when_finished \"unset GIT_TRACE2\" &&\n \n --\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n --\ttest \"$OUT\" -eq 0 &&\n -+\tif test_have_prereq !MINGW\n -+\tthen\n -+\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n -+\t\ttest \"$OUT\" -eq 0\n -+\telse\n -+\t\ttest_terminal git log\n -+\tfi &&\n -+\tgrep \"child_exit.* code:1 \" trace.normal &&\n + \tif test_have_prereq !MINGW\n + \tthen\n +@@ t/t7006-pager.sh: test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '\n + \telse\n + \t\ttest_terminal git log\n + \tfi &&\n ++\n ++\tgrep child_exit trace.normal >child-exits &&\n ++\ttest_line_count = 1 child-exits &&\n ++\tgrep \" code:1 \" child-exits &&\n \ttest_path_is_file pager-used\n '\n \n -+test_expect_success TTY 'git logs nonexisting pager invocation' '\n -+\ttest_config core.pager \"does-not-exist\" &&\n + test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '\n +-\ttest_when_finished \"rm pager-used\" &&\n ++\ttest_when_finished \"rm pager-used trace.normal\" &&\n + \ttest_config core.pager \"wc >pager-used; does-not-exist\" &&\n +\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n +\texport GIT_TRACE2 &&\n +\ttest_when_finished \"unset GIT_TRACE2\" &&\n + \n + \tif test_have_prereq !MINGW\n + \tthen\n +@@ t/t7006-pager.sh: test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '\n + \telse\n + \t\ttest_terminal git log\n + \tfi &&\n +\n -+\tif test_have_prereq !MINGW\n -+\tthen\n -+\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n -+\t\ttest_match_signal 13 \"$OUT\"\n -+\telse\n -+\t\ttest_terminal git log\n ++\tgrep child_exit trace.normal >child-exits &&\n ++\ttest_line_count = 1 child-exits &&\n ++\tgrep \" code:127 \" child-exits &&\n + \ttest_path_is_file pager-used\n + '\n + \n + test_expect_success TTY 'git attempts to page to nonexisting pager command, gets SIGPIPE' '\n ++\ttest_when_finished \"rm trace.normal\" &&\n + \ttest_config core.pager \"does-not-exist\" &&\n ++\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n ++\texport GIT_TRACE2 &&\n ++\ttest_when_finished \"unset GIT_TRACE2\" &&\n + \n + \tif test_have_prereq !MINGW\n + \tthen\n +@@ t/t7006-pager.sh: test_expect_success TTY 'git attempts to page to nonexisting pager command, gets\n + \t\ttest_match_signal 13 \"$OUT\"\n + \telse\n + \t\ttest_terminal git log\n +-\tfi\n +\tfi &&\n -+\tgrep \"child_exit.* code:-1 \" trace.normal\n -+'\n +\n - test_done\n ++\tgrep child_exit trace.normal >child-exits &&\n ++\ttest_line_count = 1 child-exits &&\n ++\tgrep \" code:-1 \" child-exits\n + '\n + \n + test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n +-\ttest_when_finished \"rm pager-used\" &&\n ++\ttest_when_finished \"rm pager-used trace.normal\" &&\n + \ttest_config core.pager \">pager-used; test-tool sigchain\" &&\n ++\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n ++\texport GIT_TRACE2 &&\n ++\ttest_when_finished \"unset GIT_TRACE2\" &&\n + \n + \tif test_have_prereq !MINGW\n + \tthen\n +@@ t/t7006-pager.sh: test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n + \telse\n + \t\ttest_terminal git log\n + \tfi &&\n ++\n ++\tgrep child_exit trace.normal >child-exits &&\n ++\ttest_line_count = 1 child-exits &&\n ++\tgrep \" code:143 \" child-exits &&\n + \ttest_path_is_file pager-used\n + '\n + \n-: ----------- > 5: 842f42340d0 WIP pager: respect exit code of pager over SIGPIPE\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"415850","messageId":"20210202020001.31601-4-avarab@gmail.com","threadId":"54992","inReplyTo":"20210201144921.8664-1-avarab@gmail.com","subject":"[PATCH v2 3/5] run-command: add braces for \"if\" block in wait_or_whine()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-02T01:59:59Z","receivedAt":"2021-02-02T02:01:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add braces to an \"if\" block in the wait_or_whine() function. This\nisn't needed now, but will make a subsequent commit easier to read.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n run-command.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex ea4d0fb4b15..00e68f37aba 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -551,8 +551,9 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n \n \twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n \t\t;\t/* nothing */\n-\tif (in_signal)\n+\tif (in_signal) {\n \t\treturn 0;\n+\t}\n \n \tif (waiting < 0) {\n \t\tfailed_errno = errno;\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"415851","messageId":"20210202020001.31601-3-avarab@gmail.com","threadId":"54992","inReplyTo":"20210201144921.8664-1-avarab@gmail.com","subject":"[PATCH v2 2/5] pager: test for exit code with and without SIGPIPE","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-02T01:59:58Z","receivedAt":"2021-02-02T02:01:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add tests for how git behaves when the pager itself exits with\nnon-zero, as well as for us exiting with 141 when we're killed with\nSIGPIPE due to the pager not consuming its output.\n\nThere is some recent discussion[1] about these semantics, but aside\nfrom what we want to do in the future, we should have a test for the\ncurrent behavior.\n\nThis test construct is stolen from 7559a1be8a0 (unblock and unignore\nSIGPIPE, 2014-09-18). The reason not to make the test itself depend on\nthe MINGW prerequisite is to make a subsequent commit easier to read.\n\n1. https://lore.kernel.org/git/87o8h4omqa.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7006-pager.sh | 82 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 82 insertions(+)\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex fdb450e446a..0aa030962b1 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -656,4 +656,86 @@ test_expect_success TTY 'git tag with auto-columns ' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success TTY 'git returns SIGPIPE on early pager exit' '\n+\ttest_when_finished \"rm pager-used\" &&\n+\ttest_config core.pager \">pager-used; head -n 1; exit 0\" &&\n+\n+\tif test_have_prereq !MINGW\n+\tthen\n+\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n+\t\ttest_match_signal 13 \"$OUT\"\n+\telse\n+\t\ttest_terminal git log\n+\tfi &&\n+\ttest_path_is_file pager-used\n+'\n+\n+test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '\n+\ttest_when_finished \"rm pager-used\" &&\n+\ttest_config core.pager \">pager-used; head -n 1; exit 1\" &&\n+\n+\tif test_have_prereq !MINGW\n+\tthen\n+\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n+\t\ttest_match_signal 13 \"$OUT\"\n+\telse\n+\t\ttest_terminal git log\n+\tfi &&\n+\ttest_path_is_file pager-used\n+'\n+\n+test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '\n+\ttest_when_finished \"rm pager-used\" &&\n+\ttest_config core.pager \"wc >pager-used; exit 1\" &&\n+\n+\tif test_have_prereq !MINGW\n+\tthen\n+\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n+\t\ttest \"$OUT\" -eq 0\n+\telse\n+\t\ttest_terminal git log\n+\tfi &&\n+\ttest_path_is_file pager-used\n+'\n+\n+test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '\n+\ttest_when_finished \"rm pager-used\" &&\n+\ttest_config core.pager \"wc >pager-used; does-not-exist\" &&\n+\n+\tif test_have_prereq !MINGW\n+\tthen\n+\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n+\t\ttest \"$OUT\" -eq 0\n+\telse\n+\t\ttest_terminal git log\n+\tfi &&\n+\ttest_path_is_file pager-used\n+'\n+\n+test_expect_success TTY 'git attempts to page to nonexisting pager command, gets SIGPIPE' '\n+\ttest_config core.pager \"does-not-exist\" &&\n+\n+\tif test_have_prereq !MINGW\n+\tthen\n+\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n+\t\ttest_match_signal 13 \"$OUT\"\n+\telse\n+\t\ttest_terminal git log\n+\tfi\n+'\n+\n+test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n+\ttest_when_finished \"rm pager-used\" &&\n+\ttest_config core.pager \">pager-used; test-tool sigchain\" &&\n+\n+\tif test_have_prereq !MINGW\n+\tthen\n+\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n+\t\ttest_match_signal 13 \"$OUT\"\n+\telse\n+\t\ttest_terminal git log\n+\tfi &&\n+\ttest_path_is_file pager-used\n+'\n+\n test_done\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"415852","messageId":"20210202020001.31601-5-avarab@gmail.com","threadId":"54992","inReplyTo":"20210201144921.8664-1-avarab@gmail.com","subject":"[PATCH v2 4/5] pager: properly log pager exit code when signalled","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-02T02:00:00Z","receivedAt":"2021-02-02T02:01:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When git invokes a pager that exits with non-zero the common case is\nthat we'll already return the correct SIGPIPE failure from git itself,\nbut the exit code logged in trace2 has always been incorrectly\nreported[1]. Fix that and log the correct exit code in the logs.\n\nSince this gives us something to test outside of our recently-added\ntests needing a !MINGW prerequisite, let's refactor the test to run on\nMINGW and actually check for SIGPIPE outside of MINGW.\n\nThe wait_or_whine() is only called with a true \"in_signal\" from from\nfinish_command_in_signal(), which in turn is only used in pager.c.\n\nThe \"in_signal && !WIFEXITED(status)\" case is not covered by\ntests. Let's log the default -1 in that case for good measure.\n\n1. The incorrect logging of the exit code in was seemingly copy/pasted\n into finish_command_in_signal() in ee4512ed481 (trace2: create new\n combined trace facility, 2019-02-22)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n run-command.c | 4 +++-\n t/t7006-pager.sh | 60 +++++++++++++++++++++++++++++++++++++++++++-----\n 2 files changed, 57 insertions(+), 7 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 00e68f37aba..509841bf273 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -552,7 +552,9 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n \twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n \t\t;\t/* nothing */\n \tif (in_signal) {\n-\t\treturn 0;\n+\t\tif (WIFEXITED(status))\n+\t\t\tcode = WEXITSTATUS(status);\n+\t\treturn code;\n \t}\n \n \tif (waiting < 0) {\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 0aa030962b1..0e7cf75435e 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -656,9 +656,17 @@ test_expect_success TTY 'git tag with auto-columns ' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setup trace2' '\n+\tGIT_TRACE2_BRIEF=1 &&\n+\texport GIT_TRACE2_BRIEF\n+'\n+\n test_expect_success TTY 'git returns SIGPIPE on early pager exit' '\n-\ttest_when_finished \"rm pager-used\" &&\n+\ttest_when_finished \"rm pager-used trace.normal\" &&\n \ttest_config core.pager \">pager-used; head -n 1; exit 0\" &&\n+\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n+\texport GIT_TRACE2 &&\n+\ttest_when_finished \"unset GIT_TRACE2\" &&\n \n \tif test_have_prereq !MINGW\n \tthen\n@@ -667,12 +675,19 @@ test_expect_success TTY 'git returns SIGPIPE on early pager exit' '\n \telse\n \t\ttest_terminal git log\n \tfi &&\n+\n+\tgrep child_exit trace.normal >child-exits &&\n+\ttest_line_count = 1 child-exits &&\n+\tgrep \" code:0 \" child-exits &&\n \ttest_path_is_file pager-used\n '\n \n test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '\n-\ttest_when_finished \"rm pager-used\" &&\n+\ttest_when_finished \"rm pager-used trace.normal\" &&\n \ttest_config core.pager \">pager-used; head -n 1; exit 1\" &&\n+\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n+\texport GIT_TRACE2 &&\n+\ttest_when_finished \"unset GIT_TRACE2\" &&\n \n \tif test_have_prereq !MINGW\n \tthen\n@@ -681,12 +696,19 @@ test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '\n \telse\n \t\ttest_terminal git log\n \tfi &&\n+\n+\tgrep child_exit trace.normal >child-exits &&\n+\ttest_line_count = 1 child-exits &&\n+\tgrep \" code:1 \" child-exits &&\n \ttest_path_is_file pager-used\n '\n \n test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '\n-\ttest_when_finished \"rm pager-used\" &&\n+\ttest_when_finished \"rm pager-used trace.normal\" &&\n \ttest_config core.pager \"wc >pager-used; exit 1\" &&\n+\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n+\texport GIT_TRACE2 &&\n+\ttest_when_finished \"unset GIT_TRACE2\" &&\n \n \tif test_have_prereq !MINGW\n \tthen\n@@ -695,12 +717,19 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '\n \telse\n \t\ttest_terminal git log\n \tfi &&\n+\n+\tgrep child_exit trace.normal >child-exits &&\n+\ttest_line_count = 1 child-exits &&\n+\tgrep \" code:1 \" child-exits &&\n \ttest_path_is_file pager-used\n '\n \n test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '\n-\ttest_when_finished \"rm pager-used\" &&\n+\ttest_when_finished \"rm pager-used trace.normal\" &&\n \ttest_config core.pager \"wc >pager-used; does-not-exist\" &&\n+\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n+\texport GIT_TRACE2 &&\n+\ttest_when_finished \"unset GIT_TRACE2\" &&\n \n \tif test_have_prereq !MINGW\n \tthen\n@@ -709,11 +738,19 @@ test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '\n \telse\n \t\ttest_terminal git log\n \tfi &&\n+\n+\tgrep child_exit trace.normal >child-exits &&\n+\ttest_line_count = 1 child-exits &&\n+\tgrep \" code:127 \" child-exits &&\n \ttest_path_is_file pager-used\n '\n \n test_expect_success TTY 'git attempts to page to nonexisting pager command, gets SIGPIPE' '\n+\ttest_when_finished \"rm trace.normal\" &&\n \ttest_config core.pager \"does-not-exist\" &&\n+\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n+\texport GIT_TRACE2 &&\n+\ttest_when_finished \"unset GIT_TRACE2\" &&\n \n \tif test_have_prereq !MINGW\n \tthen\n@@ -721,12 +758,19 @@ test_expect_success TTY 'git attempts to page to nonexisting pager command, gets\n \t\ttest_match_signal 13 \"$OUT\"\n \telse\n \t\ttest_terminal git log\n-\tfi\n+\tfi &&\n+\n+\tgrep child_exit trace.normal >child-exits &&\n+\ttest_line_count = 1 child-exits &&\n+\tgrep \" code:-1 \" child-exits\n '\n \n test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n-\ttest_when_finished \"rm pager-used\" &&\n+\ttest_when_finished \"rm pager-used trace.normal\" &&\n \ttest_config core.pager \">pager-used; test-tool sigchain\" &&\n+\tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n+\texport GIT_TRACE2 &&\n+\ttest_when_finished \"unset GIT_TRACE2\" &&\n \n \tif test_have_prereq !MINGW\n \tthen\n@@ -735,6 +779,10 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n \telse\n \t\ttest_terminal git log\n \tfi &&\n+\n+\tgrep child_exit trace.normal >child-exits &&\n+\ttest_line_count = 1 child-exits &&\n+\tgrep \" code:143 \" child-exits &&\n \ttest_path_is_file pager-used\n '\n \n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"415853","messageId":"20210202020001.31601-6-avarab@gmail.com","threadId":"54992","inReplyTo":"20210201144921.8664-1-avarab@gmail.com","subject":"[WIP/PATCH v2 5/5] WIP pager: respect exit code of pager over SIGPIPE","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-02T02:00:01Z","receivedAt":"2021-02-02T02:01:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As discussed on-list starting with [1] I don't think this patch makes\nsense, but this \"passes tests\", at least on Debian with glibc, and is\nfood for thought for those who like the approach of git not\npropagating the pager-induced SIGPIPE in git's own exit code.\n\nThe exit() here in wait_for_pager_atexit() isn't portable though[2],\nwe could probably use _exit(1) instead, but then we're going to\nabruptly put a stop to further atexit handler processing. We're far\nfrom the only one, tempfile.c, run-command.c, gc.c etc. all rely on\nit, and that's just the git.git code.\n\nIf we drop the \"if (code)\" condition we can see that our pager exit\ncode will override the exit code of other commands in t7006-pager.sh,\ncausing numerous tests to fail. Of course if we don't do that all\ntests pass.\n\nBut that experiment suggests regressions introduced here that we just\ndon't have good test coverage for. I.e. we're running code before the\natexit() here which expects to exit() with a given status code, and\nwe're clobbering it with ours because the pager also happened to fail\nas we were exiting.\n\nSo a real implementation of this would, I think, have to at least:\n\n A. Refactor all use of atexit() to use some git-specific registry,\n hard assert somehow that we're never going to have atexit() by\n anything else (a library we use might call it).\n\n B. Because we used some atexit() wrapper API we'd know if we were in\n the last atexit() handler, which would need to re-evaluate the\n decision about the \"real\" exit code.\n\n C. We could not call exit() anywhere, but would have to make a\n git_exit() wrapper. We'd then assign the desired exit code to a\n global variable, and then only override our \"real\" non-zero exit\n code with the pager's non-zero, in cases where the pager also\n failed.\n\n D. I haven't found whether calling _exit() in the atexit() handler\n even has defined behavior, but in any case using it would\n short-circuit the documented program exit behavior defined in the\n C standard, of which calling atexit() handlers is just the first\n step.\n\n1. https://lore.kernel.org/git/8735yhq3lc.fsf@evledraar.gmail.com/\n2. https://pubs.opengroup.org/onlinepubs/009695399/functions/exit.html\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n pager.c | 10 ++++++++--\n t/t7006-pager.sh | 8 ++++----\n 2 files changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 3d37dd7adaa..2e743bc0b1e 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -20,18 +20,24 @@ static void close_pager_fds(void)\n \n static void wait_for_pager_atexit(void)\n {\n+\tint code;\n \tfflush(stdout);\n \tfflush(stderr);\n \tclose_pager_fds();\n-\tfinish_command(&pager_process);\n+\tcode = finish_command(&pager_process);\n+\tif (code)\n+\t\texit(code);\n }\n \n static void wait_for_pager_signal(int signo)\n {\n+\tint code;\n \tclose_pager_fds();\n-\tfinish_command_in_signal(&pager_process);\n+\tcode = finish_command_in_signal(&pager_process);\n \tsigchain_pop(signo);\n \traise(signo);\n+\tif (signo == SIGPIPE)\n+\t\texit(code);\n }\n \n static int core_pager_config(const char *var, const char *value, void *data)\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 0e7cf75435e..69997fa48f2 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -703,7 +703,7 @@ test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '\n \ttest_path_is_file pager-used\n '\n \n-test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '\n+test_expect_success TTY 'git respects pager non-zero exit without SIGPIPE' '\n \ttest_when_finished \"rm pager-used trace.normal\" &&\n \ttest_config core.pager \"wc >pager-used; exit 1\" &&\n \tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n@@ -713,7 +713,7 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '\n \tif test_have_prereq !MINGW\n \tthen\n \t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n-\t\ttest \"$OUT\" -eq 0\n+\t\ttest \"$OUT\" -eq 1\n \telse\n \t\ttest_terminal git log\n \tfi &&\n@@ -724,7 +724,7 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '\n \ttest_path_is_file pager-used\n '\n \n-test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '\n+test_expect_success TTY 'git respects nonexisting pager without SIGPIPE' '\n \ttest_when_finished \"rm pager-used trace.normal\" &&\n \ttest_config core.pager \"wc >pager-used; does-not-exist\" &&\n \tGIT_TRACE2=\"$(pwd)/trace.normal\" &&\n@@ -734,7 +734,7 @@ test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '\n \tif test_have_prereq !MINGW\n \tthen\n \t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n-\t\ttest \"$OUT\" -eq 0\n+\t\ttest \"$OUT\" -eq 127\n \telse\n \t\ttest_terminal git log\n \tfi &&\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"415855","messageId":"87tuqvdy1b.fsf@evledraar.gmail.com","threadId":"54992","inReplyTo":"xmqq8s87ld8y.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-02T02:05:36Z","receivedAt":"2021-02-02T02:06:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 01 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>>> Would it be the matter of propagating the exit status of the pager\n>>> noticed by wait_or_white() down thru finish_command_in_signal() and\n>>> wait_for_pager(1) to here, so\n>>>\n>>> - If we know pager exited with non-zero status, we would report,\n>>> perhaps with warning(_(\"...\"));\n>>>\n>>> - If we notice we got a SIGPIPE, we ignore it---it is nothing of\n>>> interest to the end-user;\n>>>\n>>> - Otherwise we do not do anything differently.\n>>>\n>>> would be sufficient? Implementors of \"git -p\" may know that \"git\"\n>>> happens to implement its paging by piping its output to an external\n>>> pager, but the end-users do not care. Implementors may say they are\n>>> giving 'q' to their pager \"less\", but to the end-users, who report\n>>> \"I ran 'git log' and after reading a pageful, I told it to 'q'uit\",\n>>> the distinction does not have any importance.\n>>>\n>>> Or are there more to it, in that the exit status we get from the\n>>> pager, combined with the kind of signal we are getting, is not\n>>> sufficient for us to tell what is going on?\n>>\n>> It is, I just wonder if ignoring the exit code is a practical issue as\n>> long as we're not clobbering SIGPIPE, particularly with my trace2\n>> logging patch in this thread.\n>>\n>> But yeah, we could patch git to handle this in the general case....\n>\n> Sorry, but now you lost me.\n>\n> I was merely wondering if Denton's patch can become a small update\n> on top of these, if we just made sure that the exit code of the\n> pager noticed by wait_or_whine() is reported to the code where\n> Denton makes the decision to say \"let's not re-raise but simply exit\n> with 0 return as what we got is SIGPIPE\". I guess we could even\n> make git exit with the pager's return code in that case, as the\n> end-user observable result would be similar to \"git log | less\"\n> where 'less' may be segfaulting or exiting cleanly.\n>\n> IOW, something like this on top of your three-patch series?\n>\n> pager.c | 8 +++++++-\n> 1 file changed, 7 insertions(+), 1 deletion(-)\n>\n> diff --git c/pager.c w/pager.c\n> index 3d37dd7ada..73bc5fc0e4 100644\n> --- c/pager.c\n> +++ w/pager.c\n> @@ -28,8 +28,14 @@ static void wait_for_pager_atexit(void)\n> \n> static void wait_for_pager_signal(int signo)\n> {\n> +\tint status;\n> +\n> \tclose_pager_fds();\n> -\tfinish_command_in_signal(&pager_process);\n> +\tstatus = finish_command_in_signal(&pager_process);\n> +\n> +\tif (signo == SIGPIPE)\n> +\t\texit(status);\n> +\n> \tsigchain_pop(signo);\n> \traise(signo);\n> }\n\nI sent a WIP start at something like this at the end of my v2, please\ndiscard it when picking up the rest:\nhttps://lore.kernel.org/git/20210202020001.31601-6-avarab@gmail.com/\n\nAs noted there it's going to be a lot more complex than this.\n"},{"id":"415880","messageId":"xmqqo8h3hybf.fsf@gitster.c.googlers.com","threadId":"54992","inReplyTo":"87tuqvdy1b.fsf@evledraar.gmail.com","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-02T04:45:56Z","receivedAt":"2021-02-02T04:46:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>>> But yeah, we could patch git to handle this in the general case....\n>>\n>> Sorry, but now you lost me.\n>>\n>> I was merely wondering if Denton's patch can become a small update\n>> on top of these, if we just made sure that the exit code of the\n>> pager noticed by wait_or_whine() is reported to the code where\n>> Denton makes the decision to say \"let's not re-raise but simply exit\n>> with 0 return as what we got is SIGPIPE\". I guess we could even\n>> make git exit with the pager's return code in that case, as the\n>> end-user observable result would be similar to \"git log | less\"\n>> where 'less' may be segfaulting or exiting cleanly.\n>>\n>> IOW, something like this on top of your three-patch series?\n>>\n>> pager.c | 8 +++++++-\n>> 1 file changed, 7 insertions(+), 1 deletion(-)\n>> ...\n> I sent a WIP start at something like this at the end of my v2, please\n> discard it when picking up the rest:\n> https://lore.kernel.org/git/20210202020001.31601-6-avarab@gmail.com/\n>\n> As noted there it's going to be a lot more complex than this.\n\nSorry, but you still have lost me---I do not see if/why we even care\nabout atexit codepath. As far as the end users are concered, they\nare running \"git\" and observing the exit code from \"git\". There,\nreporting that \"git\" was killed by SIGPIPE, instead of exiting\nnormally, is not something they want to hear about after quitting\ntheir pager, and that is why the signal reception codepath matters.\n\nYes, I can see you are making it \"a lot more complex\" in your patch,\nbut what I do not see is why we even need to.\n\nThanks.\n"},{"id":"415882","messageId":"xmqqczxjhwgv.fsf@gitster.c.googlers.com","threadId":"54992","inReplyTo":"xmqqo8h3hybf.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-02T05:25:52Z","receivedAt":"2021-02-02T05:26:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Sorry, but you still have lost me---I do not see if/why we even care\n> about atexit codepath. As far as the end users are concered, they\n> are running \"git\" and observing the exit code from \"git\". There,\n> reporting that \"git\" was killed by SIGPIPE, instead of exiting\n> normally, is not something they want to hear about after quitting\n> their pager, and that is why the signal reception codepath matters.\n\n(something I noticed that I left unsaid...)\n\nOn the other hand, \"git\" spawns not just pager but other\nsubprocesses (e.g. \"hooks\"), and it is entirely up to us what to do\nwith the exit code from them. When we care about making an external\neffect (e.g. post-$action hooks that are run for their side effects),\nwe can ignore their exit status just fine.\n\nAnd I do not see why the \"we waited before leaving, and noticed the\npager exited with non-zero status\" that we could notice in the\natexit codepath has to be so special. We _could_ (modulo the \"exit\nthere is not portable\" you noted) make our exit status reflect that,\nbut I do not think it is all that important a \"failure\" (as opposed\nto, say, we tried to show a commit message but failed to recode it\ninto utf-8, or we tried to spawn the pager but failed to start a\nprocess) to clobber _our_ exit status with pager's exit status.\n\nSo...\n\n"},{"id":"415887","messageId":"1dfb079e-a472-0259-2a00-100eb7a06297@kdbg.org","threadId":"54992","inReplyTo":"xmqqczxjhwgv.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-02-02T07:45:51Z","receivedAt":"2021-02-02T07:46:54Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 02.02.21 um 06:25 schrieb Junio C Hamano:\n> Junio C Hamano writes:\n> \n>> Sorry, but you still have lost me---I do not see if/why we even care\n>> about atexit codepath. As far as the end users are concered, they\n>> are running \"git\" and observing the exit code from \"git\". There,\n>> reporting that \"git\" was killed by SIGPIPE, instead of exiting\n>> normally, is not something they want to hear about after quitting\n>> their pager, and that is why the signal reception codepath matters.\n> \n> (something I noticed that I left unsaid...)\n> \n> On the other hand, \"git\" spawns not just pager but other\n> subprocesses (e.g. \"hooks\"), and it is entirely up to us what to do\n> with the exit code from them. When we care about making an external\n> effect (e.g. post-$action hooks that are run for their side effects),\n> we can ignore their exit status just fine.\n> \n> And I do not see why the \"we waited before leaving, and noticed the\n> pager exited with non-zero status\" that we could notice in the\n> atexit codepath has to be so special. We _could_ (modulo the \"exit\n> there is not portable\" you noted) make our exit status reflect that,\n> but I do not think it is all that important a \"failure\" (as opposed\n> to, say, we tried to show a commit message but failed to recode it\n> into utf-8, or we tried to spawn the pager but failed to start a\n> process) to clobber _our_ exit status with pager's exit status.\n> \n> So...\n\nThe pager is a special case of a sub-process spawned, as it really only\na courtesy for the user. Without the pager facility, the user would have\nto use\n\n git log | less\n\nIn that situation, the exit code of the pager *does* override git's, and\nit is also irrelevant for the user that git was killed by SIGPIPE and is\nnot worth a visible notice.\n\n-- Hannes\n"},{"id":"415889","messageId":"YBkSPO9tFb3JXmql@generichostname","threadId":"54992","inReplyTo":"20210202020001.31601-3-avarab@gmail.com","subject":"Re: [PATCH v2 2/5] pager: test for exit code with and without SIGPIPE","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2021-02-02T08:50:04Z","receivedAt":"2021-02-02T08:51:06Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Tue, Feb 02, 2021 at 02:59:58AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> This test construct is stolen from 7559a1be8a0 (unblock and unignore\n> SIGPIPE, 2014-09-18). The reason not to make the test itself depend on\n> the MINGW prerequisite is to make a subsequent commit easier to read.\n\n[...]\n\n> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\n> index fdb450e446a..0aa030962b1 100755\n> --- a/t/t7006-pager.sh\n> +++ b/t/t7006-pager.sh\n> @@ -656,4 +656,86 @@ test_expect_success TTY 'git tag with auto-columns ' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success TTY 'git returns SIGPIPE on early pager exit' '\n> +\ttest_when_finished \"rm pager-used\" &&\n> +\ttest_config core.pager \">pager-used; head -n 1; exit 0\" &&\n\nI may be missing something but this code seems racy, especially since\nthe history is relatively short at this point. It seems like it's\nplausible for git log to be able to dump its output entirely before the\npager part even runs. In that case, it'd fail due to success being its\nexit code since it wouldn't be killed by SIGPIPE. \n\nThis is what my `test-tool pager` approach was hoping to prevent since\nthat would guarantee a SIGPIPE.\n\nSidenote, going back to 7559a1be8a0 (unblock and unignore SIGPIPE,\n2014-09-18), it seems like those tests are also racy since it's\ntheoretically possible for all of the output to be produced before the\npiped command gets to it. However, in that case, they're producing a\nhuge amount of output so this raciness seems mostly academic.\n\nThanks,\nDenton\n\n> +\n> +\tif test_have_prereq !MINGW\n> +\tthen\n> +\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n> +\t\ttest_match_signal 13 \"$OUT\"\n> +\telse\n> +\t\ttest_terminal git log\n> +\tfi &&\n> +\ttest_path_is_file pager-used\n> +'\n"},{"id":"415954","messageId":"xmqq35yegrcv.fsf@gitster.c.googlers.com","threadId":"54992","inReplyTo":"1dfb079e-a472-0259-2a00-100eb7a06297@kdbg.org","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-02T20:13:52Z","receivedAt":"2021-02-02T20:15:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 02.02.21 um 06:25 schrieb Junio C Hamano:\n>> Junio C Hamano writes:\n>> \n>>> Sorry, but you still have lost me---I do not see if/why we even care\n>>> about atexit codepath. As far as the end users are concered, they\n>>> are running \"git\" and observing the exit code from \"git\". There,\n>>> reporting that \"git\" was killed by SIGPIPE, instead of exiting\n>>> normally, is not something they want to hear about after quitting\n>>> their pager, and that is why the signal reception codepath matters.\n>> \n>> (something I noticed that I left unsaid...)\n>> \n>> On the other hand, \"git\" spawns not just pager but other\n>> subprocesses (e.g. \"hooks\"), and it is entirely up to us what to do\n>> with the exit code from them. When we care about making an external\n>> effect (e.g. post-$action hooks that are run for their side effects),\n>> we can ignore their exit status just fine.\n>> \n>> And I do not see why the \"we waited before leaving, and noticed the\n>> pager exited with non-zero status\" that we could notice in the\n>> atexit codepath has to be so special. We _could_ (modulo the \"exit\n>> there is not portable\" you noted) make our exit status reflect that,\n>> but I do not think it is all that important a \"failure\" (as opposed\n>> to, say, we tried to show a commit message but failed to recode it\n>> into utf-8, or we tried to spawn the pager but failed to start a\n>> process) to clobber _our_ exit status with pager's exit status.\n>> \n>> So...\n>\n> The pager is a special case of a sub-process spawned, as it really only\n> a courtesy for the user. Without the pager facility, the user would have\n> to use\n>\n> git log | less\n>\n> In that situation, the exit code of the pager *does* override git's, and\n> it is also irrelevant for the user that git was killed by SIGPIPE and is\n> not worth a visible notice.\n\nAll true, except that \"GIT_PAGER=less git -p log\" reports the exit\nstatus of \"git\" and not \"less\" when the entire command finishes\n(regardless of how it happens, like user typing 'q', output of log\nis shorter than one page and \"less\" automatically exiting at the\nend, etc.), unlike \"git log | less\", where the exit status of \"git\"\nis hidden. But from the end-user's point of view, I do think it\nis not a good idea to report an abnormal exit of \"git\" with SIGPIPE;\nit is an irrelevant implementation detail.\n\nAnyway, my opinion in the message you are responding to was that the\nexit status of the pager subprocess wait_for_pager_atexit() finds\ndoes not matter, and there is no reason to overly complicate the\nimplementation like the comments in Ævar's [v2 5/5] implies, and it\nis sufficient to just hide the fact in wait_for_pager_signal() that\nwe got SIGPIPE. I am not sure if you are agreeing with me, or are\nshowing me where/why I was wrong.\n\nThanks.\n\n"},{"id":"415967","messageId":"12a440af-c080-089d-bf60-76262d5aec7a@kdbg.org","threadId":"54992","inReplyTo":"xmqq35yegrcv.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-02-02T22:15:24Z","receivedAt":"2021-02-02T22:16:32Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 02.02.21 um 21:13 schrieb Junio C Hamano:\n> Johannes Sixt writes:\n> \n>> Am 02.02.21 um 06:25 schrieb Junio C Hamano:\n>>> Junio C Hamano writes:\n>>>\n>>>> Sorry, but you still have lost me---I do not see if/why we even care\n>>>> about atexit codepath. As far as the end users are concered, they\n>>>> are running \"git\" and observing the exit code from \"git\". There,\n>>>> reporting that \"git\" was killed by SIGPIPE, instead of exiting\n>>>> normally, is not something they want to hear about after quitting\n>>>> their pager, and that is why the signal reception codepath matters.\n>>>\n>>> (something I noticed that I left unsaid...)\n>>>\n>>> On the other hand, \"git\" spawns not just pager but other\n>>> subprocesses (e.g. \"hooks\"), and it is entirely up to us what to do\n>>> with the exit code from them. When we care about making an external\n>>> effect (e.g. post-$action hooks that are run for their side effects),\n>>> we can ignore their exit status just fine.\n>>>\n>>> And I do not see why the \"we waited before leaving, and noticed the\n>>> pager exited with non-zero status\" that we could notice in the\n>>> atexit codepath has to be so special. We _could_ (modulo the \"exit\n>>> there is not portable\" you noted) make our exit status reflect that,\n>>> but I do not think it is all that important a \"failure\" (as opposed\n>>> to, say, we tried to show a commit message but failed to recode it\n>>> into utf-8, or we tried to spawn the pager but failed to start a\n>>> process) to clobber _our_ exit status with pager's exit status.\n>>>\n>>> So...\n>>\n>> The pager is a special case of a sub-process spawned, as it really only\n>> a courtesy for the user. Without the pager facility, the user would have\n>> to use\n>>\n>> git log | less\n>>\n>> In that situation, the exit code of the pager *does* override git's, and\n>> it is also irrelevant for the user that git was killed by SIGPIPE and is\n>> not worth a visible notice.\n> \n> All true, except that \"GIT_PAGER=less git -p log\" reports the exit\n> status of \"git\" and not \"less\" when the entire command finishes\n> (regardless of how it happens, like user typing 'q', output of log\n> is shorter than one page and \"less\" automatically exiting at the\n> end, etc.), unlike \"git log | less\", where the exit status of \"git\"\n> is hidden. But from the end-user's point of view, I do think it\n> is not a good idea to report an abnormal exit of \"git\" with SIGPIPE;\n> it is an irrelevant implementation detail.\n> \n> Anyway, my opinion in the message you are responding to was that the\n> exit status of the pager subprocess wait_for_pager_atexit() finds\n> does not matter, and there is no reason to overly complicate the\n> implementation like the comments in Ævar's [v2 5/5] implies, and it\n> is sufficient to just hide the fact in wait_for_pager_signal() that\n> we got SIGPIPE. I am not sure if you are agreeing with me, or are\n> showing me where/why I was wrong.\n\nWe are agreeing that the SIGPIPE should not be reported.\n\nWe may be disagreeing whether it is good or bad that git's exit code is\noverridden by the pager's exit code, which Ævar wanted to implement,\nIIUC. I think that is reasonable and I base my opinion on the comparison\nwith the pipeline `git log | less`, where git's exit code is ignored.\n\n-- Hannes\n"},{"id":"415969","messageId":"xmqqwnvqdsax.fsf@gitster.c.googlers.com","threadId":"54992","inReplyTo":"12a440af-c080-089d-bf60-76262d5aec7a@kdbg.org","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-02T22:21:42Z","receivedAt":"2021-02-02T22:23:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n>> Anyway, my opinion in the message you are responding to was that the\n>> exit status of the pager subprocess wait_for_pager_atexit() finds\n>> does not matter, and there is no reason to overly complicate the\n>> implementation like the comments in Ævar's [v2 5/5] implies, and it\n>> is sufficient to just hide the fact in wait_for_pager_signal() that\n>> we got SIGPIPE. I am not sure if you are agreeing with me, or are\n>> showing me where/why I was wrong.\n>\n> We are agreeing that the SIGPIPE should not be reported.\n>\n> We may be disagreeing whether it is good or bad that git's exit code is\n> overridden by the pager's exit code, which Ævar wanted to implement,\n> IIUC. I think that is reasonable and I base my opinion on the comparison\n> with the pipeline `git log | less`, where git's exit code is ignored.\n\nI guess we are then in agreement---I do think it makes sense to send\nthe true exit code from the pager as the exit code from the pager to\nthe trace output, which is what the early part of Ævar's patch does,\nbut I do not think the exit code of the pager should affect the exit\ncode from \"git log\" as a whole.\n\n"},{"id":"415991","messageId":"87r1lxeuoj.fsf@evledraar.gmail.com","threadId":"54992","inReplyTo":"xmqqczxjhwgv.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-03T02:45:00Z","receivedAt":"2021-02-03T02:46:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Feb 02 2021, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n>> Sorry, but you still have lost me---I do not see if/why we even care\n>> about atexit codepath. As far as the end users are concered, they\n>> are running \"git\" and observing the exit code from \"git\". There,\n>> reporting that \"git\" was killed by SIGPIPE, instead of exiting\n>> normally, is not something they want to hear about after quitting\n>> their pager, and that is why the signal reception codepath matters.\n>\n> (something I noticed that I left unsaid...)\n>\n> On the other hand, \"git\" spawns not just pager but other\n> subprocesses (e.g. \"hooks\"), and it is entirely up to us what to do\n> with the exit code from them. When we care about making an external\n> effect (e.g. post-$action hooks that are run for their side effects),\n> we can ignore their exit status just fine.\n>\n> And I do not see why the \"we waited before leaving, and noticed the\n> pager exited with non-zero status\" that we could notice in the\n> atexit codepath has to be so special. We _could_ (modulo the \"exit\n> there is not portable\" you noted) make our exit status reflect that,\n> but I do not think it is all that important a \"failure\" (as opposed\n> to, say, we tried to show a commit message but failed to recode it\n> into utf-8, or we tried to spawn the pager but failed to start a\n> process) to clobber _our_ exit status with pager's exit status.\n\nBecause your patch upthread makes git's exit code on pager failure a\nfunction of how your PIPE_BUF happens to be consumed on your OS.\n\nYou can see this if you do this:\n\n diff --git a/pager.c b/pager.c\n index ee435de6756..5124124ac36 100644\n --- a/pager.c\n +++ b/pager.c\n @@ -30,0 +31 @@ static void wait_for_pager_atexit(void)\n + trace2_region_enter_printf(\"pager\", \"wait_for_pager_atexit\", the_repository, \"%d\", 0);\n @@ -35,0 +37 @@ static void wait_for_pager_signal(int signo)\n + trace2_region_enter_printf(\"pager\", \"wait_for_pager_signal\", the_repository, \"%d\", 1);\n\nAnd then e.g.:\n\n GIT_TRACE2_EVENT=/tmp/trace2.json ~/g/git/git -c core.pager=\"less; exit 1\" log -100; echo $?\n\nOn my laptop & screen size I get around ~20 page lenghts with less\nbefore I get to the end.\n\nIf I quit on the first page I get an exit code of 141, ditto the second,\non everything from the 3rd forward I get an exit code of 0. Because at\nthat point git's/the OS/pipe buffers etc. have flushed the rest to the\npager.\n\nSo:\n\n 1. With something like your patch we'd get an exit code of !0 on pager\n failure only if the user won the PIPE_BUF roulette.\n\n 2. With finishing up my \"WIP/PATCH v2 5/5\" we'd get consistent exit\n codes carried down, but that patch is insanity already, and\n finishing it would be even crazier.\n\nSo I haven't been advocating for #2, just the #0 of \"I don't really see\nthe problem with the current behavior of SIGHUP, maybe configure your\nshell?\".\n\nB.t.w. to <5772995f-c887-7f13-6b5f-dc44f4477dcb@kdbg.org> in the\nside-thread: Having a smarter pager than just less isn't really all that\nunusual, e.g. it's very handy on a remote system to type commands\ninteractively but sloooowly, but then configure a pager with some\ncombination of an ssh tunnel + nc + remote system's screen so you can\nbrowse around without every search/page up/down taking 1-2 seconds.\n\nIt's also nice when a thing like that can quit as fast as possible when\nit gets SIGHUP, not wait on the exit code of the spawned program, which\nmay involve tearing down an ssh session or whatever.\n\nBut I digress.\n\nGetting back to the point, whatever anyone thinks of returning SIGHUP as\nwe do now or not, I think it's bonkers to ignore SIGHUP and *then*\nreturn a non-zero *only in the non-atexit case*.\n\nThat just means that if you do have a broken pager you're going to get\nflaky exits depending on the state of our flushed buffers, who's going\nto be helped by such a fickle exit code?\n\nSo if we're going to change the behavior to not return SIGHUP, and don't\nwant to refactor our entire atexit() handling in #2 to be guaranteed to\npass down the pager's exit code, I don't see how anything except the\napproach of just exit(0) in that case makes sense, which is what Denton\nLiu's patch initially suggested doing.\n"},{"id":"415992","messageId":"87o8h1euix.fsf@evledraar.gmail.com","threadId":"54992","inReplyTo":"5772995f-c887-7f13-6b5f-dc44f4477dcb@kdbg.org","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-03T02:48:22Z","receivedAt":"2021-02-03T02:49:09Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 01 2021, Johannes Sixt wrote:\n\n> Am 01.02.21 um 16:44 schrieb Ævar Arnfjörð Bjarmason:\n>> On Mon, Feb 01 2021, Vincent Lefevre wrote:\n>>> On 2021-02-01 13:10:21 +0100, Ævar Arnfjörð Bjarmason:\n>>>> So we've got the SIGPIPE to indicate the output wasn't fully\n>>>> consumed.\n>>>\n>>> But the user doesn't care: he quit the pager because he didn't\n>>> need more output. So there is no need to signal that the output\n>>> wasn't fully consumed. The user already knew that before quitting\n>>> the pager!\n>> \n>> As noted above, this is assuming way too much about the functionality of\n>> the pager command. We can get a SIGPIPE without the user's intent in\n>> this way. Consider e.g. piping to some remote system via netcat.\n>\n> That assumption is warranted, IMO. Aren't _you_ stretching the meaning\n> of \"pager\" too far here? A pager is intended for presentation to the\n> user. If someone plays games with it, they should know what they get.\n\nFWIW I replied to this in\nhttps://lore.kernel.org/git/87r1lxeuoj.fsf@evledraar.gmail.com/\n\nWhatever anyone thinks of the virtues of passing down SIGHUP having e.g\na nc to a remote box be your pager isn't all that unusual.\n"},{"id":"415993","messageId":"xmqq35ydeu94.fsf@gitster.c.googlers.com","threadId":"54992","inReplyTo":"87r1lxeuoj.fsf@evledraar.gmail.com","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-03T02:54:15Z","receivedAt":"2021-02-03T02:55:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Getting back to the point, whatever anyone thinks of returning SIGHUP as\n> we do now or not, I think it's bonkers to ignore SIGHUP and *then*\n> return a non-zero *only in the non-atexit case*.\n>\n> That just means that if you do have a broken pager you're going to get\n> flaky exits depending on the state of our flushed buffers, who's going\n> to be helped by such a fickle exit code?\n>\n> So if we're going to change the behavior to not return SIGHUP, and don't\n> want to refactor our entire atexit() handling in #2 to be guaranteed to\n> pass down the pager's exit code, I don't see how anything except the\n> approach of just exit(0) in that case makes sense, which is what Denton\n> Liu's patch initially suggested doing.\n\nThen we are on the same page (assuming that all your HUPs are\nPIPEs), as the \"perhaps we could even exit with pager's error\" was\nmy mistaken reaction to your \"we have been losing pager's exit\nstatus\" message.\n\nI do want to ignore, as I said in the message you are responding to,\nthe exit status of the pager, just like we ignore exit status of\nsome of the hooks that we spawn primarily for their side effects (as\nopposed to the pre-* hooks whose status we do use to base our\ndecision on).\n\nI guess we just want to take just a half of your [WIP/PATCH v2 5/5],\nignoring the return values from finish_command*() and exiting with 0\nwhen we got SIGPIPE (that would mean that there will be no change on\nthe atexit codepath). Unlike Denton's change directly on the current\ncodebase, the resulting code would clearly show that we only care about\nthe signal codepath, thanks to the refactoring [PATCH v2 1/5] has\ndone.\n\n\n"},{"id":"416022","messageId":"87lfc5esao.fsf@evledraar.gmail.com","threadId":"54992","inReplyTo":"xmqq35ydeu94.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-03T03:36:31Z","receivedAt":"2021-02-03T03:37:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 03 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Getting back to the point, whatever anyone thinks of returning SIGHUP as\n>> we do now or not, I think it's bonkers to ignore SIGHUP and *then*\n>> return a non-zero *only in the non-atexit case*.\n>>\n>> That just means that if you do have a broken pager you're going to get\n>> flaky exits depending on the state of our flushed buffers, who's going\n>> to be helped by such a fickle exit code?\n>>\n>> So if we're going to change the behavior to not return SIGHUP, and don't\n>> want to refactor our entire atexit() handling in #2 to be guaranteed to\n>> pass down the pager's exit code, I don't see how anything except the\n>> approach of just exit(0) in that case makes sense, which is what Denton\n>> Liu's patch initially suggested doing.\n>\n> Then we are on the same page (assuming that all your HUPs are\n> PIPEs)\n\nYes, sorry, PBCAK :)\n"},{"id":"416044","messageId":"20210203152634.GA22673@joooj.vinc17.net","threadId":"54992","inReplyTo":"87a6snokrr.fsf@evledraar.gmail.com","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Vincent Lefevre","fromEmail":"vincent@vinc17.net","sentAt":"2021-02-03T15:26:34Z","receivedAt":"2021-02-03T15:27:33Z","isPatch":false,"sender":{"key":"vincent@vinc17.net","avatar":null},"body":"On 2021-02-01 16:44:24 +0100, Ævar Arnfjörð Bjarmason wrote:\n> And then whether it makes sense to ignore SIGPIPE for all users, or\n> e.g. if it's some opt-in setting in some situations that users might\n> want to turn on because they're aware of how their pager behaves and\n> want to work around some zsh mode.\n\nAFAIK, SIGPIPE exists for the following reason. Most programs that\ngenerate output are not written to specifically handle pipes. So,\nif SIGPIPE did not exist, there would be 2 kinds of behavior:\n 1. The program doesn't check for errors, and still outputs data,\n wasting time and resources as output will be ignored.\n 2. The program sees that the write() failed and terminates with\n an error message. However, in most cases, such a failure is\n not an error: the consumer has terminated either because it\n no longer needs any input (e.g. with the \"head\" utility or a\n pager), or because it has terminated abnormally, in which case\n the real error is on the side of the consumer. So, the error\n message from the LHS of the pipe would be annoying.\n\nSIGPIPE solves this issue: the program is simply killed with SIGPIPE.\nIn a shell, one gets a non-zero exit code (128 + 13) due to the\nsignal, but as being on the left-hand side of the pipe, such a\nnon-zero exit code is normally not reported, so that this will not\nannoy the user.\n\nNote 1: Non-zero exit codes from right-hand side are not reported\neither by most shells, but zsh can report them, and this is very\nuseful for developers, as programs may fail with a non-zero exit\ncode but without an error message. (Reports may also be done by\nlooking at the standard $? in some hook.)\n\nNote 2: Failures on the left-hand side are less interesting in practice\nand generally ignored, at least for commands run in interactive shells.\nFor scripts, there are various (non-simple) ways to handle them.\n\nNow, I think that in the case (like Git) a program creates a pipe,\nit should use its knowledge to handle SIGPIPE / EPIPE. Either this\nis regarded as an error because the full output is *always expected*\nto be read, in which case there should be an error message in addition\nto the usual non-zero exist status (not necessarily 141), or this is\nregarded as OK (if there is a real failure, this is on the side of\nthe consumer). In the case of Git, the consumer is documented to be\na pager, which obviously may not read the full output (e.g. for the\nGCC repository, \"git log\" returns more than 3 million lines, back to\nthe year 1988, while one is generally interested in the latest changes\nonly). If the user wants to pipe to something else, he can always use\nan explicit pipe.\n\n-- \nVincent Lefèvre - Web: \n100% accessible validated (X)HTML - Blog: \nWork: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)\n"},{"id":"416064","messageId":"aa672f2b-6886-a2bf-5129-f10f4e488961@kdbg.org","threadId":"54992","inReplyTo":"xmqqwnvqdsax.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-02-03T17:07:02Z","receivedAt":"2021-02-03T17:08:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 02.02.21 um 23:21 schrieb Junio C Hamano:\n> Johannes Sixt writes:\n> \n>>> Anyway, my opinion in the message you are responding to was that the\n>>> exit status of the pager subprocess wait_for_pager_atexit() finds\n>>> does not matter, and there is no reason to overly complicate the\n>>> implementation like the comments in Ævar's [v2 5/5] implies, and it\n>>> is sufficient to just hide the fact in wait_for_pager_signal() that\n>>> we got SIGPIPE. I am not sure if you are agreeing with me, or are\n>>> showing me where/why I was wrong.\n>>\n>> We are agreeing that the SIGPIPE should not be reported.\n>>\n>> We may be disagreeing whether it is good or bad that git's exit code is\n>> overridden by the pager's exit code, which Ævar wanted to implement,\n>> IIUC. I think that is reasonable and I base my opinion on the comparison\n>> with the pipeline `git log | less`, where git's exit code is ignored.\n> \n> I guess we are then in agreement---I do think it makes sense to send\n> the true exit code from the pager as the exit code from the pager to\n> the trace output, which is what the early part of Ævar's patch does,\n> but I do not think the exit code of the pager should affect the exit\n> code from \"git log\" as a whole.\n\nThen we do not agree. The exit code of `git log | less` is ignored, and\nI regard `git -p log` just as a short-hand for that.\n\nThat said, I don't care a lot about the exit code. When a pager is in\nthe game, we are talking about an interactive command invocation, and\nwhat the exit code of that is, is irrelevant in practice.\n\nThe only thing I care is that git does not die due to a SIGPIPE when the\npager is closed early.\n\n-- Hannes\n"},{"id":"416065","messageId":"1222a249-818a-5ca7-2187-9a3b9ab5eb5b@kdbg.org","threadId":"54992","inReplyTo":"87o8h1euix.fsf@evledraar.gmail.com","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-02-03T17:11:40Z","receivedAt":"2021-02-03T17:12:44Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 03.02.21 um 03:48 schrieb Ævar Arnfjörð Bjarmason:\n> \n> On Mon, Feb 01 2021, Johannes Sixt wrote:\n> \n>> Am 01.02.21 um 16:44 schrieb Ævar Arnfjörð Bjarmason:\n>>> On Mon, Feb 01 2021, Vincent Lefevre wrote:\n>>>> On 2021-02-01 13:10:21 +0100, Ævar Arnfjörð Bjarmason:\n>>>>> So we've got the SIGPIPE to indicate the output wasn't fully\n>>>>> consumed.\n>>>>\n>>>> But the user doesn't care: he quit the pager because he didn't\n>>>> need more output. So there is no need to signal that the output\n>>>> wasn't fully consumed. The user already knew that before quitting\n>>>> the pager!\n>>>\n>>> As noted above, this is assuming way too much about the functionality of\n>>> the pager command. We can get a SIGPIPE without the user's intent in\n>>> this way. Consider e.g. piping to some remote system via netcat.\n>>\n>> That assumption is warranted, IMO. Aren't _you_ stretching the meaning\n>> of \"pager\" too far here? A pager is intended for presentation to the\n>> user. If someone plays games with it, they should know what they get.\n> \n> FWIW I replied to this in\n> https://lore.kernel.org/git/87r1lxeuoj.fsf@evledraar.gmail.com/\n> \n> Whatever anyone thinks of the virtues of passing down SIGHUP having e.g\n> a nc to a remote box be your pager isn't all that unusual.\n\nA pager in any form is fair game. That point is that it is an\n*interactive* form of presentation. But you should not use git's pager\nas data post-processing facility; that would stretch the meaning of\n\"pager\" too far, and we do not have cater for such abuse of the feature.\n\n-- Hannes\n"},{"id":"416067","messageId":"160415b7-3196-7653-7417-4d5af97a2567@kdbg.org","threadId":"54992","inReplyTo":"xmqq35ydeu94.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-02-03T17:19:12Z","receivedAt":"2021-02-03T17:19:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 03.02.21 um 03:54 schrieb Junio C Hamano:\n> I guess we just want to take just a half of your [WIP/PATCH v2 5/5],\n> ignoring the return values from finish_command*() and exiting with 0\n> when we got SIGPIPE (that would mean that there will be no change on\n> the atexit codepath). Unlike Denton's change directly on the current\n> codebase, the resulting code would clearly show that we only care about\n> the signal codepath, thanks to the refactoring [PATCH v2 1/5] has\n> done.\n\nNote though, that we cannot call exit() from a signal handler: it is not\nasync-signal safe.\n\nhttps://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03\n\n-- Hannes\n"},{"id":"416073","messageId":"xmqqr1lxc96e.fsf@gitster.c.googlers.com","threadId":"54992","inReplyTo":"aa672f2b-6886-a2bf-5129-f10f4e488961@kdbg.org","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-03T18:12:25Z","receivedAt":"2021-02-03T18:14:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n>> I guess we are then in agreement---I do think it makes sense to send\n>> the true exit code from the pager as the exit code from the pager to\n>> the trace output, which is what the early part of Ævar's patch does,\n>> but I do not think the exit code of the pager should affect the exit\n>> code from \"git log\" as a whole.\n>\n> Then we do not agree. The exit code of `git log | less` is ignored, and\n> I regard `git -p log` just as a short-hand for that.\n\nI think you skipped \"not\" while reading the \"but I do not think\"\npart of the last sentence.\n\n> The only thing I care is that git does not die due to a SIGPIPE when the\n> pager is closed early.\n\nMakes two of us ;-)\n"},{"id":"416119","messageId":"87v9b8d6zx.fsf@evledraar.gmail.com","threadId":"54992","inReplyTo":"20210203152634.GA22673@joooj.vinc17.net","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-04T00:14:10Z","receivedAt":"2021-02-04T00:15:11Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 03 2021, Vincent Lefevre wrote:\n\n> On 2021-02-01 16:44:24 +0100, Ævar Arnfjörð Bjarmason wrote:\n>> And then whether it makes sense to ignore SIGPIPE for all users, or\n>> e.g. if it's some opt-in setting in some situations that users might\n>> want to turn on because they're aware of how their pager behaves and\n>> want to work around some zsh mode.\n>\n> AFAIK, SIGPIPE exists for the following reason. Most programs that\n> generate output are not written to specifically handle pipes. So,\n> if SIGPIPE did not exist, there would be 2 kinds of behavior:\n> 1. The program doesn't check for errors, and still outputs data,\n> wasting time and resources as output will be ignored.\n> 2. The program sees that the write() failed and terminates with\n> an error message. However, in most cases, such a failure is\n> not an error: the consumer has terminated either because it\n> no longer needs any input (e.g. with the \"head\" utility or a\n> pager), or because it has terminated abnormally, in which case\n> the real error is on the side of the consumer. So, the error\n> message from the LHS of the pipe would be annoying.\n>\n> SIGPIPE solves this issue: the program is simply killed with SIGPIPE.\n> In a shell, one gets a non-zero exit code (128 + 13) due to the\n> signal, but as being on the left-hand side of the pipe, such a\n> non-zero exit code is normally not reported, so that this will not\n> annoy the user.\n>\n> Note 1: Non-zero exit codes from right-hand side are not reported\n> either by most shells, but zsh can report them, and this is very\n> useful for developers, as programs may fail with a non-zero exit\n> code but without an error message. (Reports may also be done by\n> looking at the standard $? in some hook.)\n>\n> Note 2: Failures on the left-hand side are less interesting in practice\n> and generally ignored, at least for commands run in interactive shells.\n> For scripts, there are various (non-simple) ways to handle them.\n>\n> Now, I think that in the case (like Git) a program creates a pipe,\n> it should use its knowledge to handle SIGPIPE / EPIPE. Either this\n> is regarded as an error because the full output is *always expected*\n> to be read, in which case there should be an error message in addition\n> to the usual non-zero exist status (not necessarily 141), or this is\n> regarded as OK (if there is a real failure, this is on the side of\n> the consumer). In the case of Git, the consumer is documented to be\n> a pager, which obviously may not read the full output (e.g. for the\n> GCC repository, \"git log\" returns more than 3 million lines, back to\n> the year 1988, while one is generally interested in the latest changes\n> only). If the user wants to pipe to something else, he can always use\n> an explicit pipe.\n\nSIGPIPE exists because *nix systems are composable, so you can make\nsomething useful by stringing together unrelated programs via files and\npipes, and with exit codes and signal mostly everyone's happy.\n\nI follow what you're saying right until the point of arguing that\nbecause either your shell or POSIX shells in general have decided to\neither be sloppy or overzelous in how they show you some\ninformation. That we should use their behavior as a guide in\npro-actively suppressing our own exit code.\n\nAnd that's not because I think (to the tune of Monty Python...) that\nevery exit code is sacret. It's because when we invoke a pager handing\nit data is *the* thing we're doing. If we can fully hand it over, great,\nif not, let's tell the user with the appropriate exit code.\n\nYeah it's annoying with zsh's PRINT_EXIT_VALUE, but the same is true of\nPOSIX \"set -e\". Not every shell option is meant for general use. The\nshell is very forgiving of things like pipe failures by default for a\nreason.\n\nBut \"connected to a terminal\" (isatty(1)) and \"invoked by Vincent's zsh\ninstance\" aren't the same thing. And I think it makes sense to be\nconservative in preserving exit codes.\n\nIn the early days of git complaining about \"Broken pipe\" in the exact\nsame scenario was the default behavior of bash's overzelous reporting,\nas you can read about starting here:\nhttps://lore.kernel.org/git/?q=%22Broken+pipe%22+bash&o=-1\n\nAFAICT that changed by default in bash 3.1, released in 2005-12-08. It\nwas a FAQ in the early days, now nobody cares.\n\nHave you reported this as a bug to zsh? I think it's likely that the\nmotivation for wanting this squashed in git is going to be as transitory\nas bash's once-default verbosity was.\n\nI also tested \"hg log\", it behaves the same way, although interestingly\nthey cast SIGPIPE to 255 in their exit code.\n\n"},{"id":"416150","messageId":"20210204153812.GI148009@zira.vinc17.org","threadId":"54992","inReplyTo":"87v9b8d6zx.fsf@evledraar.gmail.com","subject":"Re: git fails with a broken pipe when one quits the pager","fromName":"Vincent Lefevre","fromEmail":"vincent@vinc17.net","sentAt":"2021-02-04T15:38:12Z","receivedAt":"2021-02-04T15:49:52Z","isPatch":false,"sender":{"key":"vincent@vinc17.net","avatar":null},"body":"On 2021-02-04 01:14:10 +0100, Ævar Arnfjörð Bjarmason wrote:\n> Have you reported this as a bug to zsh?\n\nI repeat: there is no bug in zsh. It is my choice to output the\nexit status when it is non-zero because I want to know when the\ncommand I've typed fails. This is useful in practice. Ignoring the\nspecific value 141 (corresponding to SIGPIPE) is not a solution\nbecause it can be a real failure with some utilities. BTW, the\nassociation with a signal like SIGPIPE is just a convention; apart\nfrom that, 141 is a non-zero status like others (in particular\nwith programs that have not been written for POSIX).\n\nFor instance, in any shell:\n\n$ sh -c \"echo foo; exit 141\"\nfoo\n$ echo $?\n141\n\nwhile no broken pipe is involved here. How would you differentiate\nsuch a failure from a broken pipe?\n\n> I also tested \"hg log\", it behaves the same way, although interestingly\n> they cast SIGPIPE to 255 in their exit code.\n\nI get 141, like with git:\n\n$ hg log\n$ echo $?\n141\n\n-- \nVincent Lefèvre - Web: \n100% accessible validated (X)HTML - Blog: \nWork: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)\n"},{"id":"416153","messageId":"20210204151056.GH148009@zira.vinc17.org","threadId":"54992","inReplyTo":"aa672f2b-6886-a2bf-5129-f10f4e488961@kdbg.org","subject":"Re: [PATCH] pager: exit without error on SIGPIPE","fromName":"Vincent Lefevre","fromEmail":"vincent@vinc17.net","sentAt":"2021-02-04T15:10:56Z","receivedAt":"2021-02-04T16:21:53Z","isPatch":true,"sender":{"key":"vincent@vinc17.net","avatar":null},"body":"On 2021-02-03 18:07:02 +0100, Johannes Sixt wrote:\n> Then we do not agree. The exit code of `git log | less` is ignored,\n> and I regard `git -p log` just as a short-hand for that.\n\nIf git didn't have the -p feature, \"git log | less\" would just be\none way to get a pager. For instance, I use an alias that does\n\n pager-wrapper grep --color=always --line-buffered -E\n\nwhich sends the grep output to a pager, and returns the exit status\nof the pager if non-zero, otherwise the exit status of grep, except\nwhen this is SIGPIPE. An unavoidable issue is that if there has been\nan error in grep, I could still get the exit status 0. But as a user,\nthis is a choice I have done by quitting the pager before letting\ngrep terminate in the normal way (which could have been costly) so\nthat it could report the error, say, about unreadable files with a\nrecursive grep (grep -r).\n\nSo Git could do the same thing, and even better, because it controls\nits own exit status: if there has been an error in the generation of\nthe Git output (for instance, I can see that there is a --check option\nof \"git log\" that can trigger an error), then this error should be\nreported (with a non-zero exit status) after the pager is quit, as if\na pager were not used. Otherwise, terminate with the exit status of\nthe pager.\n\n-- \nVincent Lefèvre - Web: \n100% accessible validated (X)HTML - Blog: \nWork: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)\n"},{"id":"416247","messageId":"9c686629-82d9-f441-a255-a288fe322881@kdbg.org","threadId":"54992","inReplyTo":"20210202020001.31601-3-avarab@gmail.com","subject":"Re: [PATCH v2 2/5] pager: test for exit code with and without SIGPIPE","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-02-05T07:47:52Z","receivedAt":"2021-02-05T07:48:40Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 02.02.21 um 02:59 schrieb Ævar Arnfjörð Bjarmason:\n> Add tests for how git behaves when the pager itself exits with\n> non-zero, as well as for us exiting with 141 when we're killed with\n> SIGPIPE due to the pager not consuming its output.\n> \n> There is some recent discussion[1] about these semantics, but aside\n> from what we want to do in the future, we should have a test for the\n> current behavior.\n> \n> This test construct is stolen from 7559a1be8a0 (unblock and unignore\n> SIGPIPE, 2014-09-18). The reason not to make the test itself depend on\n> the MINGW prerequisite is to make a subsequent commit easier to read.\n\nAt least for my Windows build, the MINGW games do not make a difference:\nThe test is skipped anyway due to the unsatisfied TTY prerequisite.\n\n> \n> 1. https://lore.kernel.org/git/87o8h4omqa.fsf@evledraar.gmail.com/\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> t/t7006-pager.sh | 82 ++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 82 insertions(+)\n> \n> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\n> index fdb450e446a..0aa030962b1 100755\n> --- a/t/t7006-pager.sh\n> +++ b/t/t7006-pager.sh\n> @@ -656,4 +656,86 @@ test_expect_success TTY 'git tag with auto-columns ' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success TTY 'git returns SIGPIPE on early pager exit' '\n> +\ttest_when_finished \"rm pager-used\" &&\n> +\ttest_config core.pager \">pager-used; head -n 1; exit 0\" &&\n> +\n> +\tif test_have_prereq !MINGW\n> +\tthen\n> +\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n> +\t\ttest_match_signal 13 \"$OUT\"\n> +\telse\n> +\t\ttest_terminal git log\n> +\tfi &&\n> +\ttest_path_is_file pager-used\n> +'\n> +\n> +test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '\n> +\ttest_when_finished \"rm pager-used\" &&\n> +\ttest_config core.pager \">pager-used; head -n 1; exit 1\" &&\n> +\n> +\tif test_have_prereq !MINGW\n> +\tthen\n> +\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n> +\t\ttest_match_signal 13 \"$OUT\"\n> +\telse\n> +\t\ttest_terminal git log\n> +\tfi &&\n> +\ttest_path_is_file pager-used\n> +'\n> +\n> +test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '\n> +\ttest_when_finished \"rm pager-used\" &&\n> +\ttest_config core.pager \"wc >pager-used; exit 1\" &&\n> +\n> +\tif test_have_prereq !MINGW\n> +\tthen\n> +\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n> +\t\ttest \"$OUT\" -eq 0\n> +\telse\n> +\t\ttest_terminal git log\n> +\tfi &&\n> +\ttest_path_is_file pager-used\n> +'\n> +\n> +test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '\n> +\ttest_when_finished \"rm pager-used\" &&\n> +\ttest_config core.pager \"wc >pager-used; does-not-exist\" &&\n> +\n> +\tif test_have_prereq !MINGW\n> +\tthen\n> +\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n> +\t\ttest \"$OUT\" -eq 0\n> +\telse\n> +\t\ttest_terminal git log\n> +\tfi &&\n> +\ttest_path_is_file pager-used\n> +'\n> +\n> +test_expect_success TTY 'git attempts to page to nonexisting pager command, gets SIGPIPE' '\n> +\ttest_config core.pager \"does-not-exist\" &&\n> +\n> +\tif test_have_prereq !MINGW\n> +\tthen\n> +\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n> +\t\ttest_match_signal 13 \"$OUT\"\n> +\telse\n> +\t\ttest_terminal git log\n> +\tfi\n> +'\n> +\n> +test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n> +\ttest_when_finished \"rm pager-used\" &&\n> +\ttest_config core.pager \">pager-used; test-tool sigchain\" &&\n> +\n> +\tif test_have_prereq !MINGW\n> +\tthen\n> +\t\tOUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&\n> +\t\ttest_match_signal 13 \"$OUT\"\n> +\telse\n> +\t\ttest_terminal git log\n> +\tfi &&\n> +\ttest_path_is_file pager-used\n> +'\n> +\n> test_done\n> \n\n"},{"id":"416248","messageId":"5f5c5018-9fcc-6a9f-66fc-81d1c09946c3@kdbg.org","threadId":"54992","inReplyTo":"20210202020001.31601-5-avarab@gmail.com","subject":"Re: [PATCH v2 4/5] pager: properly log pager exit code when signalled","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-02-05T07:58:42Z","receivedAt":"2021-02-05T07:59:31Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 02.02.21 um 03:00 schrieb Ævar Arnfjörð Bjarmason:\n> When git invokes a pager that exits with non-zero the common case is\n> that we'll already return the correct SIGPIPE failure from git itself,\n> but the exit code logged in trace2 has always been incorrectly\n> reported[1]. Fix that and log the correct exit code in the logs.\n\nThere's a more severe problem here, not with your patch, but with trace2\nin general: it invokes async-signal-unsafe functions from a signal\nhandler, in particular, realloc, vsnprintf, gettimeofday, localtime_r\n(and probably a lot more) via fn_child_exit_fl of trace2/tr2_tgt_normal.c\n\nIs that something that we should care about?\n\n-- Hannes\n"},{"id":"416253","messageId":"xmqqr1lu21ab.fsf@gitster.c.googlers.com","threadId":"54992","inReplyTo":"5f5c5018-9fcc-6a9f-66fc-81d1c09946c3@kdbg.org","subject":"Re: [PATCH v2 4/5] pager: properly log pager exit code when signalled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-05T11:37:32Z","receivedAt":"2021-02-05T11:40:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 02.02.21 um 03:00 schrieb Ævar Arnfjörð Bjarmason:\n>> When git invokes a pager that exits with non-zero the common case is\n>> that we'll already return the correct SIGPIPE failure from git itself,\n>> but the exit code logged in trace2 has always been incorrectly\n>> reported[1]. Fix that and log the correct exit code in the logs.\n>\n> There's a more severe problem here, not with your patch, but with trace2\n> in general: it invokes async-signal-unsafe functions from a signal\n> handler, in particular, realloc, vsnprintf, gettimeofday, localtime_r\n> (and probably a lot more) via fn_child_exit_fl of trace2/tr2_tgt_normal.c\n>\n> Is that something that we should care about?\n\nYes, indeed X-<.\n"}]}