git fails with a broken pipe when one quits the pager

git fails with a broken pipe when one quits the pager

[Vincent Lefevre]

I had reported the following bug at
  https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914896

It still occurs with Git 2.30.0.

Some git commands with a lot of output fail with a broken pipe when
one quits the pager (without going to the end of the output).

For instance, in zsh:

cventin% setopt PRINT_EXIT_VALUE
cventin% git log
zsh: broken pipe  git log
cventin% echo $?
141
cventin% 

This is annoying. And of course, I don't want to hide error messages
by default, because this would hide *real* errors.

The broken pipe is internally expected, thus should not be reported
by git.

Just to be clear: this broken pipe should be discarded only when git
uses its builtin pager feature, not with a general pipe, where the
error may be important.

For instance,

$ { git log ; echo "Exit status: $?" >&2 ; } | true

should still output

Exit status: 141

like currently.

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

[PATCH] pager: exit without error on SIGPIPE

[Denton Liu]

If the pager closes before the git command feeding the pager finishes,
git is killed by a SIGPIPE and the corresponding exit code is 141.
Since the pipe is just an implementation detail, it does not make sense
for this error code to be user-facing.

Handle SIGPIPEs by simply calling exit(0) in wait_for_pager_signal().

Introduce `test-tool pager` which infinitely prints `y` to the pager in
order to test the new behavior. This cannot be tested with any existing
git command because there are no other commands which produce infinite
output. Without the change to pager.c, the newly introduced test fails.

Reported-by: Vincent Lefevre <vincent@vinc17.net>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Sorry for the resend, it seems like vger has dropped the first patch.

 Makefile              |  1 +
 pager.c               |  2 ++
 t/helper/test-pager.c | 12 ++++++++++++
 t/helper/test-tool.c  |  1 +
 t/helper/test-tool.h  |  1 +
 t/t7006-pager.sh      |  4 ++++
 6 files changed, 21 insertions(+)
 create mode 100644 t/helper/test-pager.c

diff --git a/Makefile b/Makefile
index 4edfda3e00..38a1a20f31 100644
--- a/Makefile
+++ b/Makefile
@@ -719,6 +719,7 @@ TEST_BUILTINS_OBJS += test-mktemp.o
 TEST_BUILTINS_OBJS += test-oid-array.o
 TEST_BUILTINS_OBJS += test-oidmap.o
 TEST_BUILTINS_OBJS += test-online-cpus.o
+TEST_BUILTINS_OBJS += test-pager.o
 TEST_BUILTINS_OBJS += test-parse-options.o
 TEST_BUILTINS_OBJS += test-parse-pathspec-file.o
 TEST_BUILTINS_OBJS += test-path-utils.o
diff --git a/pager.c b/pager.c
index ee435de675..5922d99dc8 100644
--- a/pager.c
+++ b/pager.c
@@ -34,6 +34,8 @@ static void wait_for_pager_atexit(void)
 static void wait_for_pager_signal(int signo)
 {
 	wait_for_pager(1);
+	if (signo == SIGPIPE)
+		exit(0);
 	sigchain_pop(signo);
 	raise(signo);
 }
diff --git a/t/helper/test-pager.c b/t/helper/test-pager.c
new file mode 100644
index 0000000000..feb68b8643
--- /dev/null
+++ b/t/helper/test-pager.c
@@ -0,0 +1,12 @@
+#include "test-tool.h"
+#include "cache.h"
+
+int cmd__pager(int argc, const char **argv)
+{
+	if (argc > 1)
+		usage("\ttest-tool pager");
+
+	setup_pager();
+	for (;;)
+		puts("y");
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 9d6d14d929..88269a7156 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -43,6 +43,7 @@ static struct test_cmd cmds[] = {
 	{ "oid-array", cmd__oid_array },
 	{ "oidmap", cmd__oidmap },
 	{ "online-cpus", cmd__online_cpus },
+	{ "pager", cmd__pager },
 	{ "parse-options", cmd__parse_options },
 	{ "parse-pathspec-file", cmd__parse_pathspec_file },
 	{ "path-utils", cmd__path_utils },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index a6470ff62c..78900f7938 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -32,6 +32,7 @@ int cmd__mergesort(int argc, const char **argv);
 int cmd__mktemp(int argc, const char **argv);
 int cmd__oidmap(int argc, const char **argv);
 int cmd__online_cpus(int argc, const char **argv);
+int cmd__pager(int argc, const char **argv);
 int cmd__parse_options(int argc, const char **argv);
 int cmd__parse_pathspec_file(int argc, const char** argv);
 int cmd__path_utils(int argc, const char **argv);
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index fdb450e446..2eb89e8f75 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -656,4 +656,8 @@ test_expect_success TTY 'git tag with auto-columns ' '
 	test_cmp expect actual
 '
 
+test_expect_success TTY 'SIGPIPE from pager returns success' '
+	test_terminal env PAGER=true test-tool pager
+'
+
 test_done
-- 
2.30.0.478.g8a0d178c01

Re: [PATCH] pager: exit without error on SIGPIPE

[Johannes Sixt]

Am 30.01.21 um 00:48 schrieb Denton Liu:
> If the pager closes before the git command feeding the pager finishes,
> git is killed by a SIGPIPE and the corresponding exit code is 141.
> Since the pipe is just an implementation detail, it does not make sense
> for this error code to be user-facing.
> 
> Handle SIGPIPEs by simply calling exit(0) in wait_for_pager_signal().
> 
> Introduce `test-tool pager` which infinitely prints `y` to the pager in
> order to test the new behavior. This cannot be tested with any existing
> git command because there are no other commands which produce infinite
> output. Without the change to pager.c, the newly introduced test fails.
> 
> Reported-by: Vincent Lefevre <vincent@vinc17.net>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>

...

> diff --git a/pager.c b/pager.c
> index ee435de675..5922d99dc8 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -34,6 +34,8 @@ static void wait_for_pager_atexit(void)
>  static void wait_for_pager_signal(int signo)
>  {
>  	wait_for_pager(1);
> +	if (signo == SIGPIPE)
> +		exit(0);
>  	sigchain_pop(signo);
>  	raise(signo);
>  }
> diff --git a/t/helper/test-pager.c b/t/helper/test-pager.c
> new file mode 100644
> index 0000000000..feb68b8643
> --- /dev/null
> +++ b/t/helper/test-pager.c
> @@ -0,0 +1,12 @@
> +#include "test-tool.h"
> +#include "cache.h"
> +
> +int cmd__pager(int argc, const char **argv)
> +{
> +	if (argc > 1)
> +		usage("\ttest-tool pager");
> +
> +	setup_pager();
> +	for (;;)
> +		puts("y");
> +}

My gut feeling tells that this will end in an infinite loop on Windows.
There are no signals on Windows that would kill the upstream of a pipe.
This call site will only notice that the downstream of the pipe was
closed, when it checks for write errors.

Let me test it.

-- Hannes

Re: [PATCH] pager: exit without error on SIGPIPE

[Johannes Sixt]

Am 30.01.21 um 09:29 schrieb Johannes Sixt:
> Am 30.01.21 um 00:48 schrieb Denton Liu:
>> +++ b/t/helper/test-pager.c
>> @@ -0,0 +1,12 @@
>> +#include "test-tool.h"
>> +#include "cache.h"
>> +
>> +int cmd__pager(int argc, const char **argv)
>> +{
>> +	if (argc > 1)
>> +		usage("\ttest-tool pager");
>> +
>> +	setup_pager();
>> +	for (;;)
>> +		puts("y");
>> +}
> 
> My gut feeling tells that this will end in an infinite loop on Windows.
> There are no signals on Windows that would kill the upstream of a pipe.
> This call site will only notice that the downstream of the pipe was
> closed, when it checks for write errors.
> 
> Let me test it.

The test case is protected by a TTY prerequisite; that is not satisfied
on Windows, and the test is skipped. No harm done so far.

But when I run `test-tool pager` manually and quit out of the pager, the
tool does spin in the endless loop. The following fixup helps.


diff --git a/t/helper/test-pager.c b/t/helper/test-pager.c
index feb68b8643..5f1982411f 100644
--- a/t/helper/test-pager.c
+++ b/t/helper/test-pager.c
@@ -7,6 +7,8 @@ int cmd__pager(int argc, const char **argv)
 		usage("\ttest-tool pager");
 
 	setup_pager();
-	for (;;)
-		puts("y");
+	while (write_in_full(1, "y\n", 2) > 0)
+		;
+
+	return 0;
 }
-- 
2.30.0.119.g680bcb97f5

Re: [PATCH] pager: exit without error on SIGPIPE

[Ævar Arnfjörð Bjarmason]

On Sat, Jan 30 2021, Denton Liu wrote:

> [...]

The thread at large has enough about whether this approach even makes
sense. I won't repeat that here. Just small notes on the patch itself:

> diff --git a/Makefile b/Makefile
> index 4edfda3e00..38a1a20f31 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -719,6 +719,7 @@ TEST_BUILTINS_OBJS += test-mktemp.o
>  TEST_BUILTINS_OBJS += test-oid-array.o
>  TEST_BUILTINS_OBJS += test-oidmap.o
>  TEST_BUILTINS_OBJS += test-online-cpus.o
> +TEST_BUILTINS_OBJS += test-pager.o
>  TEST_BUILTINS_OBJS += test-parse-options.o
>  TEST_BUILTINS_OBJS += test-parse-pathspec-file.o
>  TEST_BUILTINS_OBJS += test-path-utils.o
> diff --git a/pager.c b/pager.c
> index ee435de675..5922d99dc8 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -34,6 +34,8 @@ static void wait_for_pager_atexit(void)
>  static void wait_for_pager_signal(int signo)
>  {
>  	wait_for_pager(1);
> +	if (signo == SIGPIPE)
> +		exit(0);

As shown in
https://lore.kernel.org/git/20210201144921.8664-1-avarab@gmail.com/ this
leaves us without guard rails where the pager dies/segfaults or
whatever.

That's an existing bug, but by not carrying the SIGPIPE forward it
changes from "most of the time we'd exit with SIGPIPE anyway" to "we'll
never notice".

> [...]
> +test_expect_success TTY 'SIGPIPE from pager returns success' '
> +	test_terminal env PAGER=true test-tool pager
> +'
> +
>  test_done

As noted in
https://lore.kernel.org/git/20210201144921.8664-1-avarab@gmail.com/ I
think this whole "test-tool pager" isn't needed. We can just use git
itself with some trickery.

Re: [PATCH] pager: exit without error on SIGPIPE

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> diff --git a/pager.c b/pager.c
>> index ee435de675..5922d99dc8 100644
>> --- a/pager.c
>> +++ b/pager.c
>> @@ -34,6 +34,8 @@ static void wait_for_pager_atexit(void)
>>  static void wait_for_pager_signal(int signo)
>>  {
>>  	wait_for_pager(1);
>> +	if (signo == SIGPIPE)
>> +		exit(0);
>
> As shown in
> https://lore.kernel.org/git/20210201144921.8664-1-avarab@gmail.com/ this
> leaves us without guard rails where the pager dies/segfaults or
> whatever.
>
> That's an existing bug, but by not carrying the SIGPIPE forward it
> changes from "most of the time we'd exit with SIGPIPE anyway" to "we'll
> never notice".

Would it be the matter of propagating the exit status of the pager
noticed by wait_or_white() down thru finish_command_in_signal() and
wait_for_pager(1) to here, so

 - If we know pager exited with non-zero status, we would report,
   perhaps with warning(_("..."));

 - If we notice we got a SIGPIPE, we ignore it---it is nothing of
   interest to the end-user;

 - Otherwise we do not do anything differently.

would be sufficient?  Implementors of "git -p" may know that "git"
happens to implement its paging by piping its output to an external
pager, but the end-users do not care.  Implementors may say they are
giving 'q' to their pager "less", but to the end-users, who report
"I ran 'git log' and after reading a pageful, I told it to 'q'uit",
the distinction does not have any importance.

Or are there more to it, in that the exit status we get from the
pager, combined with the kind of signal we are getting, is not
sufficient for us to tell what is going on?

Re: [PATCH] pager: exit without error on SIGPIPE

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 01 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> diff --git a/pager.c b/pager.c
>>> index ee435de675..5922d99dc8 100644
>>> --- a/pager.c
>>> +++ b/pager.c
>>> @@ -34,6 +34,8 @@ static void wait_for_pager_atexit(void)
>>>  static void wait_for_pager_signal(int signo)
>>>  {
>>>  	wait_for_pager(1);
>>> +	if (signo == SIGPIPE)
>>> +		exit(0);
>>
>> As shown in
>> https://lore.kernel.org/git/20210201144921.8664-1-avarab@gmail.com/ this
>> leaves us without guard rails where the pager dies/segfaults or
>> whatever.
>>
>> That's an existing bug, but by not carrying the SIGPIPE forward it
>> changes from "most of the time we'd exit with SIGPIPE anyway" to "we'll
>> never notice".
>
> Would it be the matter of propagating the exit status of the pager
> noticed by wait_or_white() down thru finish_command_in_signal() and
> wait_for_pager(1) to here, so
>
>  - If we know pager exited with non-zero status, we would report,
>    perhaps with warning(_("..."));
>
>  - If we notice we got a SIGPIPE, we ignore it---it is nothing of
>    interest to the end-user;
>
>  - Otherwise we do not do anything differently.
>
> would be sufficient?  Implementors of "git -p" may know that "git"
> happens to implement its paging by piping its output to an external
> pager, but the end-users do not care.  Implementors may say they are
> giving 'q' to their pager "less", but to the end-users, who report
> "I ran 'git log' and after reading a pageful, I told it to 'q'uit",
> the distinction does not have any importance.
>
> Or are there more to it, in that the exit status we get from the
> pager, combined with the kind of signal we are getting, is not
> sufficient for us to tell what is going on?

It is, I just wonder if ignoring the exit code is a practical issue as
long as we're not clobbering SIGPIPE, particularly with my trace2
logging patch in this thread.

But yeah, we could patch git to handle this in the general case. I think
it's probably a bit of a PITA to do, since for the general case we need
to munge the exit code in an atexit() handler.

Which means calling _exit() (if that's even portable), and presumably
changing from the atexit() API to our own registry of how many times we
called atexit(), which would introduce logic bugs if we ever use a
library that wants to have atexit(). I.e. if we _exit() before its
atexit() handler runs because we wanted to munge the exit code.

Re: [PATCH] pager: exit without error on SIGPIPE

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> Would it be the matter of propagating the exit status of the pager
>> noticed by wait_or_white() down thru finish_command_in_signal() and
>> wait_for_pager(1) to here, so
>>
>>  - If we know pager exited with non-zero status, we would report,
>>    perhaps with warning(_("..."));
>>
>>  - If we notice we got a SIGPIPE, we ignore it---it is nothing of
>>    interest to the end-user;
>>
>>  - Otherwise we do not do anything differently.
>>
>> would be sufficient?  Implementors of "git -p" may know that "git"
>> happens to implement its paging by piping its output to an external
>> pager, but the end-users do not care.  Implementors may say they are
>> giving 'q' to their pager "less", but to the end-users, who report
>> "I ran 'git log' and after reading a pageful, I told it to 'q'uit",
>> the distinction does not have any importance.
>>
>> Or are there more to it, in that the exit status we get from the
>> pager, combined with the kind of signal we are getting, is not
>> sufficient for us to tell what is going on?
>
> It is, I just wonder if ignoring the exit code is a practical issue as
> long as we're not clobbering SIGPIPE, particularly with my trace2
> logging patch in this thread.
>
> But yeah, we could patch git to handle this in the general case....

Sorry, but now you lost me.

I was merely wondering if Denton's patch can become a small update
on top of these, if we just made sure that the exit code of the
pager noticed by wait_or_whine() is reported to the code where
Denton makes the decision to say "let's not re-raise but simply exit
with 0 return as what we got is SIGPIPE".  I guess we could even
make git exit with the pager's return code in that case, as the
end-user observable result would be similar to "git log | less"
where 'less' may be segfaulting or exiting cleanly.

IOW, something like this on top of your three-patch series?

 pager.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git c/pager.c w/pager.c
index 3d37dd7ada..73bc5fc0e4 100644
--- c/pager.c
+++ w/pager.c
@@ -28,8 +28,14 @@ static void wait_for_pager_atexit(void)
 
 static void wait_for_pager_signal(int signo)
 {
+	int status;
+
 	close_pager_fds();
-	finish_command_in_signal(&pager_process);
+	status = finish_command_in_signal(&pager_process);
+
+	if (signo == SIGPIPE)
+		exit(status);
+
 	sigchain_pop(signo);
 	raise(signo);
 }

Re: [PATCH] pager: exit without error on SIGPIPE

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 01 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> Would it be the matter of propagating the exit status of the pager
>>> noticed by wait_or_white() down thru finish_command_in_signal() and
>>> wait_for_pager(1) to here, so
>>>
>>>  - If we know pager exited with non-zero status, we would report,
>>>    perhaps with warning(_("..."));
>>>
>>>  - If we notice we got a SIGPIPE, we ignore it---it is nothing of
>>>    interest to the end-user;
>>>
>>>  - Otherwise we do not do anything differently.
>>>
>>> would be sufficient?  Implementors of "git -p" may know that "git"
>>> happens to implement its paging by piping its output to an external
>>> pager, but the end-users do not care.  Implementors may say they are
>>> giving 'q' to their pager "less", but to the end-users, who report
>>> "I ran 'git log' and after reading a pageful, I told it to 'q'uit",
>>> the distinction does not have any importance.
>>>
>>> Or are there more to it, in that the exit status we get from the
>>> pager, combined with the kind of signal we are getting, is not
>>> sufficient for us to tell what is going on?
>>
>> It is, I just wonder if ignoring the exit code is a practical issue as
>> long as we're not clobbering SIGPIPE, particularly with my trace2
>> logging patch in this thread.
>>
>> But yeah, we could patch git to handle this in the general case....
>
> Sorry, but now you lost me.
>
> I was merely wondering if Denton's patch can become a small update
> on top of these, if we just made sure that the exit code of the
> pager noticed by wait_or_whine() is reported to the code where
> Denton makes the decision to say "let's not re-raise but simply exit
> with 0 return as what we got is SIGPIPE".  I guess we could even
> make git exit with the pager's return code in that case, as the
> end-user observable result would be similar to "git log | less"
> where 'less' may be segfaulting or exiting cleanly.
>
> IOW, something like this on top of your three-patch series?
>
>  pager.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git c/pager.c w/pager.c
> index 3d37dd7ada..73bc5fc0e4 100644
> --- c/pager.c
> +++ w/pager.c
> @@ -28,8 +28,14 @@ static void wait_for_pager_atexit(void)
>  
>  static void wait_for_pager_signal(int signo)
>  {
> +	int status;
> +
>  	close_pager_fds();
> -	finish_command_in_signal(&pager_process);
> +	status = finish_command_in_signal(&pager_process);
> +
> +	if (signo == SIGPIPE)
> +		exit(status);
> +
>  	sigchain_pop(signo);
>  	raise(signo);
>  }

I sent a WIP start at something like this at the end of my v2, please
discard it when picking up the rest:
https://lore.kernel.org/git/20210202020001.31601-6-avarab@gmail.com/

As noted there it's going to be a lot more complex than this.

Re: [PATCH] pager: exit without error on SIGPIPE

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>> But yeah, we could patch git to handle this in the general case....
>>
>> Sorry, but now you lost me.
>>
>> I was merely wondering if Denton's patch can become a small update
>> on top of these, if we just made sure that the exit code of the
>> pager noticed by wait_or_whine() is reported to the code where
>> Denton makes the decision to say "let's not re-raise but simply exit
>> with 0 return as what we got is SIGPIPE".  I guess we could even
>> make git exit with the pager's return code in that case, as the
>> end-user observable result would be similar to "git log | less"
>> where 'less' may be segfaulting or exiting cleanly.
>>
>> IOW, something like this on top of your three-patch series?
>>
>>  pager.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>> ...
> I sent a WIP start at something like this at the end of my v2, please
> discard it when picking up the rest:
> https://lore.kernel.org/git/20210202020001.31601-6-avarab@gmail.com/
>
> As noted there it's going to be a lot more complex than this.

Sorry, but you still have lost me---I do not see if/why we even care
about atexit codepath.  As far as the end users are concered, they
are running "git" and observing the exit code from "git".  There,
reporting that "git" was killed by SIGPIPE, instead of exiting
normally, is not something they want to hear about after quitting
their pager, and that is why the signal reception codepath matters.

Yes, I can see you are making it "a lot more complex" in your patch,
but what I do not see is why we even need to.

Thanks.

Re: [PATCH] pager: exit without error on SIGPIPE

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Sorry, but you still have lost me---I do not see if/why we even care
> about atexit codepath.  As far as the end users are concered, they
> are running "git" and observing the exit code from "git".  There,
> reporting that "git" was killed by SIGPIPE, instead of exiting
> normally, is not something they want to hear about after quitting
> their pager, and that is why the signal reception codepath matters.

(something I noticed that I left unsaid...)

On the other hand, "git" spawns not just pager but other
subprocesses (e.g. "hooks"), and it is entirely up to us what to do
with the exit code from them.  When we care about making an external
effect (e.g. post-$action hooks that are run for their side effects),
we can ignore their exit status just fine.

And I do not see why the "we waited before leaving, and noticed the
pager exited with non-zero status" that we could notice in the
atexit codepath has to be so special.  We _could_ (modulo the "exit
there is not portable" you noted) make our exit status reflect that,
but I do not think it is all that important a "failure" (as opposed
to, say, we tried to show a commit message but failed to recode it
into utf-8, or we tried to spawn the pager but failed to start a
process) to clobber _our_ exit status with pager's exit status.

So...

Re: [PATCH] pager: exit without error on SIGPIPE

[Johannes Sixt]

Am 02.02.21 um 06:25 schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Sorry, but you still have lost me---I do not see if/why we even care
>> about atexit codepath.  As far as the end users are concered, they
>> are running "git" and observing the exit code from "git".  There,
>> reporting that "git" was killed by SIGPIPE, instead of exiting
>> normally, is not something they want to hear about after quitting
>> their pager, and that is why the signal reception codepath matters.
> 
> (something I noticed that I left unsaid...)
> 
> On the other hand, "git" spawns not just pager but other
> subprocesses (e.g. "hooks"), and it is entirely up to us what to do
> with the exit code from them.  When we care about making an external
> effect (e.g. post-$action hooks that are run for their side effects),
> we can ignore their exit status just fine.
> 
> And I do not see why the "we waited before leaving, and noticed the
> pager exited with non-zero status" that we could notice in the
> atexit codepath has to be so special.  We _could_ (modulo the "exit
> there is not portable" you noted) make our exit status reflect that,
> but I do not think it is all that important a "failure" (as opposed
> to, say, we tried to show a commit message but failed to recode it
> into utf-8, or we tried to spawn the pager but failed to start a
> process) to clobber _our_ exit status with pager's exit status.
> 
> So...

The pager is a special case of a sub-process spawned, as it really only
a courtesy for the user. Without the pager facility, the user would have
to use

    git log | less

In that situation, the exit code of the pager *does* override git's, and
it is also irrelevant for the user that git was killed by SIGPIPE and is
not worth a visible notice.

-- Hannes

Re: [PATCH] pager: exit without error on SIGPIPE

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 02.02.21 um 06:25 schrieb Junio C Hamano:
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>>> Sorry, but you still have lost me---I do not see if/why we even care
>>> about atexit codepath.  As far as the end users are concered, they
>>> are running "git" and observing the exit code from "git".  There,
>>> reporting that "git" was killed by SIGPIPE, instead of exiting
>>> normally, is not something they want to hear about after quitting
>>> their pager, and that is why the signal reception codepath matters.
>> 
>> (something I noticed that I left unsaid...)
>> 
>> On the other hand, "git" spawns not just pager but other
>> subprocesses (e.g. "hooks"), and it is entirely up to us what to do
>> with the exit code from them.  When we care about making an external
>> effect (e.g. post-$action hooks that are run for their side effects),
>> we can ignore their exit status just fine.
>> 
>> And I do not see why the "we waited before leaving, and noticed the
>> pager exited with non-zero status" that we could notice in the
>> atexit codepath has to be so special.  We _could_ (modulo the "exit
>> there is not portable" you noted) make our exit status reflect that,
>> but I do not think it is all that important a "failure" (as opposed
>> to, say, we tried to show a commit message but failed to recode it
>> into utf-8, or we tried to spawn the pager but failed to start a
>> process) to clobber _our_ exit status with pager's exit status.
>> 
>> So...
>
> The pager is a special case of a sub-process spawned, as it really only
> a courtesy for the user. Without the pager facility, the user would have
> to use
>
>     git log | less
>
> In that situation, the exit code of the pager *does* override git's, and
> it is also irrelevant for the user that git was killed by SIGPIPE and is
> not worth a visible notice.

All true, except that "GIT_PAGER=less git -p log" reports the exit
status of "git" and not "less" when the entire command finishes
(regardless of how it happens, like user typing 'q', output of log
is shorter than one page and "less" automatically exiting at the
end, etc.), unlike "git log | less", where the exit status of "git"
is hidden.  But from the end-user's point of view, I do think it
is not a good idea to report an abnormal exit of "git" with SIGPIPE;
it is an irrelevant implementation detail.

Anyway, my opinion in the message you are responding to was that the
exit status of the pager subprocess wait_for_pager_atexit() finds
does not matter, and there is no reason to overly complicate the
implementation like the comments in Ævar's [v2 5/5] implies, and it
is sufficient to just hide the fact in wait_for_pager_signal() that
we got SIGPIPE.  I am not sure if you are agreeing with me, or are
showing me where/why I was wrong.

Thanks.

Re: [PATCH] pager: exit without error on SIGPIPE

[Johannes Sixt]

Am 02.02.21 um 21:13 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> Am 02.02.21 um 06:25 schrieb Junio C Hamano:
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>> Sorry, but you still have lost me---I do not see if/why we even care
>>>> about atexit codepath.  As far as the end users are concered, they
>>>> are running "git" and observing the exit code from "git".  There,
>>>> reporting that "git" was killed by SIGPIPE, instead of exiting
>>>> normally, is not something they want to hear about after quitting
>>>> their pager, and that is why the signal reception codepath matters.
>>>
>>> (something I noticed that I left unsaid...)
>>>
>>> On the other hand, "git" spawns not just pager but other
>>> subprocesses (e.g. "hooks"), and it is entirely up to us what to do
>>> with the exit code from them.  When we care about making an external
>>> effect (e.g. post-$action hooks that are run for their side effects),
>>> we can ignore their exit status just fine.
>>>
>>> And I do not see why the "we waited before leaving, and noticed the
>>> pager exited with non-zero status" that we could notice in the
>>> atexit codepath has to be so special.  We _could_ (modulo the "exit
>>> there is not portable" you noted) make our exit status reflect that,
>>> but I do not think it is all that important a "failure" (as opposed
>>> to, say, we tried to show a commit message but failed to recode it
>>> into utf-8, or we tried to spawn the pager but failed to start a
>>> process) to clobber _our_ exit status with pager's exit status.
>>>
>>> So...
>>
>> The pager is a special case of a sub-process spawned, as it really only
>> a courtesy for the user. Without the pager facility, the user would have
>> to use
>>
>>     git log | less
>>
>> In that situation, the exit code of the pager *does* override git's, and
>> it is also irrelevant for the user that git was killed by SIGPIPE and is
>> not worth a visible notice.
> 
> All true, except that "GIT_PAGER=less git -p log" reports the exit
> status of "git" and not "less" when the entire command finishes
> (regardless of how it happens, like user typing 'q', output of log
> is shorter than one page and "less" automatically exiting at the
> end, etc.), unlike "git log | less", where the exit status of "git"
> is hidden.  But from the end-user's point of view, I do think it
> is not a good idea to report an abnormal exit of "git" with SIGPIPE;
> it is an irrelevant implementation detail.
> 
> Anyway, my opinion in the message you are responding to was that the
> exit status of the pager subprocess wait_for_pager_atexit() finds
> does not matter, and there is no reason to overly complicate the
> implementation like the comments in Ævar's [v2 5/5] implies, and it
> is sufficient to just hide the fact in wait_for_pager_signal() that
> we got SIGPIPE.  I am not sure if you are agreeing with me, or are
> showing me where/why I was wrong.

We are agreeing that the SIGPIPE should not be reported.

We may be disagreeing whether it is good or bad that git's exit code is
overridden by the pager's exit code, which Ævar wanted to implement,
IIUC. I think that is reasonable and I base my opinion on the comparison
with the pipeline `git log | less`, where git's exit code is ignored.

-- Hannes

Re: [PATCH] pager: exit without error on SIGPIPE

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> Anyway, my opinion in the message you are responding to was that the
>> exit status of the pager subprocess wait_for_pager_atexit() finds
>> does not matter, and there is no reason to overly complicate the
>> implementation like the comments in Ævar's [v2 5/5] implies, and it
>> is sufficient to just hide the fact in wait_for_pager_signal() that
>> we got SIGPIPE.  I am not sure if you are agreeing with me, or are
>> showing me where/why I was wrong.
>
> We are agreeing that the SIGPIPE should not be reported.
>
> We may be disagreeing whether it is good or bad that git's exit code is
> overridden by the pager's exit code, which Ævar wanted to implement,
> IIUC. I think that is reasonable and I base my opinion on the comparison
> with the pipeline `git log | less`, where git's exit code is ignored.

I guess we are then in agreement---I do think it makes sense to send
the true exit code from the pager as the exit code from the pager to
the trace output, which is what the early part of Ævar's patch does,
but I do not think the exit code of the pager should affect the exit
code from "git log" as a whole.

Re: [PATCH] pager: exit without error on SIGPIPE

[Johannes Sixt]

Am 02.02.21 um 23:21 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>>> Anyway, my opinion in the message you are responding to was that the
>>> exit status of the pager subprocess wait_for_pager_atexit() finds
>>> does not matter, and there is no reason to overly complicate the
>>> implementation like the comments in Ævar's [v2 5/5] implies, and it
>>> is sufficient to just hide the fact in wait_for_pager_signal() that
>>> we got SIGPIPE.  I am not sure if you are agreeing with me, or are
>>> showing me where/why I was wrong.
>>
>> We are agreeing that the SIGPIPE should not be reported.
>>
>> We may be disagreeing whether it is good or bad that git's exit code is
>> overridden by the pager's exit code, which Ævar wanted to implement,
>> IIUC. I think that is reasonable and I base my opinion on the comparison
>> with the pipeline `git log | less`, where git's exit code is ignored.
> 
> I guess we are then in agreement---I do think it makes sense to send
> the true exit code from the pager as the exit code from the pager to
> the trace output, which is what the early part of Ævar's patch does,
> but I do not think the exit code of the pager should affect the exit
> code from "git log" as a whole.

Then we do not agree. The exit code of `git log | less` is ignored, and
I regard `git -p log` just as a short-hand for that.

That said, I don't care a lot about the exit code. When a pager is in
the game, we are talking about an interactive command invocation, and
what the exit code of that is, is irrelevant in practice.

The only thing I care is that git does not die due to a SIGPIPE when the
pager is closed early.

-- Hannes

Re: [PATCH] pager: exit without error on SIGPIPE

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> I guess we are then in agreement---I do think it makes sense to send
>> the true exit code from the pager as the exit code from the pager to
>> the trace output, which is what the early part of Ævar's patch does,
>> but I do not think the exit code of the pager should affect the exit
>> code from "git log" as a whole.
>
> Then we do not agree. The exit code of `git log | less` is ignored, and
> I regard `git -p log` just as a short-hand for that.

I think you skipped "not" while reading the "but I do not think"
part of the last sentence.

> The only thing I care is that git does not die due to a SIGPIPE when the
> pager is closed early.

Makes two of us ;-)

Re: [PATCH] pager: exit without error on SIGPIPE

[Vincent Lefevre]

On 2021-02-03 18:07:02 +0100, Johannes Sixt wrote:
> Then we do not agree. The exit code of `git log | less` is ignored,
> and I regard `git -p log` just as a short-hand for that.

If git didn't have the -p feature, "git log | less" would just be
one way to get a pager. For instance, I use an alias that does

  pager-wrapper grep --color=always --line-buffered -E

which sends the grep output to a pager, and returns the exit status
of the pager if non-zero, otherwise the exit status of grep, except
when this is SIGPIPE. An unavoidable issue is that if there has been
an error in grep, I could still get the exit status 0. But as a user,
this is a choice I have done by quitting the pager before letting
grep terminate in the normal way (which could have been costly) so
that it could report the error, say, about unreadable files with a
recursive grep (grep -r).

So Git could do the same thing, and even better, because it controls
its own exit status: if there has been an error in the generation of
the Git output (for instance, I can see that there is a --check option
of "git log" that can trigger an error), then this error should be
reported (with a non-zero exit status) after the pager is quit, as if
a pager were not used. Otherwise, terminate with the exit status of
the pager.

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: [PATCH] pager: exit without error on SIGPIPE

[Ævar Arnfjörð Bjarmason]

On Tue, Feb 02 2021, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sorry, but you still have lost me---I do not see if/why we even care
>> about atexit codepath.  As far as the end users are concered, they
>> are running "git" and observing the exit code from "git".  There,
>> reporting that "git" was killed by SIGPIPE, instead of exiting
>> normally, is not something they want to hear about after quitting
>> their pager, and that is why the signal reception codepath matters.
>
> (something I noticed that I left unsaid...)
>
> On the other hand, "git" spawns not just pager but other
> subprocesses (e.g. "hooks"), and it is entirely up to us what to do
> with the exit code from them.  When we care about making an external
> effect (e.g. post-$action hooks that are run for their side effects),
> we can ignore their exit status just fine.
>
> And I do not see why the "we waited before leaving, and noticed the
> pager exited with non-zero status" that we could notice in the
> atexit codepath has to be so special.  We _could_ (modulo the "exit
> there is not portable" you noted) make our exit status reflect that,
> but I do not think it is all that important a "failure" (as opposed
> to, say, we tried to show a commit message but failed to recode it
> into utf-8, or we tried to spawn the pager but failed to start a
> process) to clobber _our_ exit status with pager's exit status.

Because your patch upthread makes git's exit code on pager failure a
function of how your PIPE_BUF happens to be consumed on your OS.

You can see this if you do this:

    diff --git a/pager.c b/pager.c
    index ee435de6756..5124124ac36 100644
    --- a/pager.c
    +++ b/pager.c
    @@ -30,0 +31 @@ static void wait_for_pager_atexit(void)
    +       trace2_region_enter_printf("pager", "wait_for_pager_atexit", the_repository, "%d", 0);
    @@ -35,0 +37 @@ static void wait_for_pager_signal(int signo)
    +       trace2_region_enter_printf("pager", "wait_for_pager_signal", the_repository, "%d", 1);

And then e.g.:

    GIT_TRACE2_EVENT=/tmp/trace2.json ~/g/git/git -c core.pager="less; exit 1" log -100; echo $?

On my laptop & screen size I get around ~20 page lenghts with less
before I get to the end.

If I quit on the first page I get an exit code of 141, ditto the second,
on everything from the 3rd forward I get an exit code of 0. Because at
that point git's/the OS/pipe buffers etc. have flushed the rest to the
pager.

So:

 1. With something like your patch we'd get an exit code of !0 on pager
    failure only if the user won the PIPE_BUF roulette.

 2. With finishing up my "WIP/PATCH v2 5/5" we'd get consistent exit
    codes carried down, but that patch is insanity already, and
    finishing it would be even crazier.

So I haven't been advocating for #2, just the #0 of "I don't really see
the problem with the current behavior of SIGHUP, maybe configure your
shell?".

B.t.w. to <5772995f-c887-7f13-6b5f-dc44f4477dcb@kdbg.org> in the
side-thread: Having a smarter pager than just less isn't really all that
unusual, e.g. it's very handy on a remote system to type commands
interactively but sloooowly, but then configure a pager with some
combination of an ssh tunnel + nc + remote system's screen so you can
browse around without every search/page up/down taking 1-2 seconds.

It's also nice when a thing like that can quit as fast as possible when
it gets SIGHUP, not wait on the exit code of the spawned program, which
may involve tearing down an ssh session or whatever.

But I digress.

Getting back to the point, whatever anyone thinks of returning SIGHUP as
we do now or not, I think it's bonkers to ignore SIGHUP and *then*
return a non-zero *only in the non-atexit case*.

That just means that if you do have a broken pager you're going to get
flaky exits depending on the state of our flushed buffers, who's going
to be helped by such a fickle exit code?

So if we're going to change the behavior to not return SIGHUP, and don't
want to refactor our entire atexit() handling in #2 to be guaranteed to
pass down the pager's exit code, I don't see how anything except the
approach of just exit(0) in that case makes sense, which is what Denton
Liu's patch initially suggested doing.

Re: [PATCH] pager: exit without error on SIGPIPE

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Getting back to the point, whatever anyone thinks of returning SIGHUP as
> we do now or not, I think it's bonkers to ignore SIGHUP and *then*
> return a non-zero *only in the non-atexit case*.
>
> That just means that if you do have a broken pager you're going to get
> flaky exits depending on the state of our flushed buffers, who's going
> to be helped by such a fickle exit code?
>
> So if we're going to change the behavior to not return SIGHUP, and don't
> want to refactor our entire atexit() handling in #2 to be guaranteed to
> pass down the pager's exit code, I don't see how anything except the
> approach of just exit(0) in that case makes sense, which is what Denton
> Liu's patch initially suggested doing.

Then we are on the same page (assuming that all your HUPs are
PIPEs), as the "perhaps we could even exit with pager's error" was
my mistaken reaction to your "we have been losing pager's exit
status" message.

I do want to ignore, as I said in the message you are responding to,
the exit status of the pager, just like we ignore exit status of
some of the hooks that we spawn primarily for their side effects (as
opposed to the pre-* hooks whose status we do use to base our
decision on).

I guess we just want to take just a half of your [WIP/PATCH v2 5/5],
ignoring the return values from finish_command*() and exiting with 0
when we got SIGPIPE (that would mean that there will be no change on
the atexit codepath).  Unlike Denton's change directly on the current
codebase, the resulting code would clearly show that we only care about
the signal codepath, thanks to the refactoring [PATCH v2 1/5] has
done.

Re: [PATCH] pager: exit without error on SIGPIPE

[Ævar Arnfjörð Bjarmason]

On Wed, Feb 03 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Getting back to the point, whatever anyone thinks of returning SIGHUP as
>> we do now or not, I think it's bonkers to ignore SIGHUP and *then*
>> return a non-zero *only in the non-atexit case*.
>>
>> That just means that if you do have a broken pager you're going to get
>> flaky exits depending on the state of our flushed buffers, who's going
>> to be helped by such a fickle exit code?
>>
>> So if we're going to change the behavior to not return SIGHUP, and don't
>> want to refactor our entire atexit() handling in #2 to be guaranteed to
>> pass down the pager's exit code, I don't see how anything except the
>> approach of just exit(0) in that case makes sense, which is what Denton
>> Liu's patch initially suggested doing.
>
> Then we are on the same page (assuming that all your HUPs are
> PIPEs)

Yes, sorry, PBCAK :)

Re: [PATCH] pager: exit without error on SIGPIPE

[Johannes Sixt]

Am 03.02.21 um 03:54 schrieb Junio C Hamano:
> I guess we just want to take just a half of your [WIP/PATCH v2 5/5],
> ignoring the return values from finish_command*() and exiting with 0
> when we got SIGPIPE (that would mean that there will be no change on
> the atexit codepath).  Unlike Denton's change directly on the current
> codebase, the resulting code would clearly show that we only care about
> the signal codepath, thanks to the refactoring [PATCH v2 1/5] has
> done.

Note though, that we cannot call exit() from a signal handler: it is not
async-signal safe.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03

-- Hannes

Re: git fails with a broken pipe when one quits the pager

[Ævar Arnfjörð Bjarmason]

On Fri, Jan 15 2021, Vincent Lefevre wrote:

> I had reported the following bug at
>   https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914896
>
> It still occurs with Git 2.30.0.
>
> Some git commands with a lot of output fail with a broken pipe when
> one quits the pager (without going to the end of the output).
>
> For instance, in zsh:
>
> cventin% setopt PRINT_EXIT_VALUE
> cventin% git log
> zsh: broken pipe  git log
> cventin% echo $?
> 141
> cventin% 
>
> This is annoying[...]

Yes it's annoying, but the annoying output is from zsh, not
git. Consider a smarter implementation like:

    case $__exit_status in
        0) __exit_emoji=😀;;
        1) __exit_emoji=☹️ ;;
        141) __exit_emoji=🤕 ;;
        [...]

Then put the $__exit_emoji in your $PS1 prompt, now when you 'q' in a
pager you know the difference between having quit at the full output
being emitted or not.

> And of course, I don't want to hide error messages by default, because
> this would hide *real* errors.

Isn't the solution to this that your shell stops reporting failures due
to SIGPIPE in such a prominent way then?

> The broken pipe is internally expected, thus should not be reported
> by git.
>
> Just to be clear: this broken pipe should be discarded only when git
> uses its builtin pager feature, not with a general pipe, where the
> error may be important.
>
> For instance,
>
> $ { git log ; echo "Exit status: $?" >&2 ; } | true
>
> should still output
>
> Exit status: 141

I don't get it, how is it less meaningful when git itself invokes the
pager?

In both cases the exit code means the same thing, that something in a
pipe wasn't fully consumed being signalled to calling processes is the
point of SIGPIPE.

Re: git fails with a broken pipe when one quits the pager

[Vincent Lefevre]

On 2021-01-31 02:47:59 +0100, Ævar Arnfjörð Bjarmason wrote:
> On Fri, Jan 15 2021, Vincent Lefevre wrote:
> > I had reported the following bug at
> >   https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914896
> >
> > It still occurs with Git 2.30.0.
> >
> > Some git commands with a lot of output fail with a broken pipe when
> > one quits the pager (without going to the end of the output).
> >
> > For instance, in zsh:
> >
> > cventin% setopt PRINT_EXIT_VALUE
> > cventin% git log
> > zsh: broken pipe  git log
> > cventin% echo $?
> > 141
> > cventin% 
> >
> > This is annoying[...]
> 
> Yes it's annoying, but the annoying output is from zsh, not
> git. Consider a smarter implementation like:
> 
>     case $__exit_status in
>         0) __exit_emoji=😀;;
>         1) __exit_emoji=☹️ ;;
>         141) __exit_emoji=🤕 ;;
>         [...]
> 
> Then put the $__exit_emoji in your $PS1 prompt, now when you 'q' in a
> pager you know the difference between having quit at the full output
> being emitted or not.

FYI, I already have the exit status already in my prompt (the above
commands were just for the example). Still, the git behavior is
disturbing.

Moreover, this doesn't solve the issue when doing something like

  git log && some_other_command

> > And of course, I don't want to hide error messages by default, because
> > this would hide *real* errors.
> 
> Isn't the solution to this that your shell stops reporting failures due
> to SIGPIPE in such a prominent way then?

No! I want to be warned about real SIGPIPEs.

> > The broken pipe is internally expected, thus should not be reported
> > by git.
> >
> > Just to be clear: this broken pipe should be discarded only when git
> > uses its builtin pager feature, not with a general pipe, where the
> > error may be important.
> >
> > For instance,
> >
> > $ { git log ; echo "Exit status: $?" >&2 ; } | true
> >
> > should still output
> >
> > Exit status: 141
> 
> I don't get it, how is it less meaningful when git itself invokes the
> pager?

I don't understand your question. If I invoke the pager myself,
I don't get a SIGPIPE:

cventin:~/software/gcc-trunk> git log
cventin:~/software/gcc-trunk[PIPE]> git log|m
cventin:~/software/gcc-trunk>

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: git fails with a broken pipe when one quits the pager

[Vincent Lefevre]

On 2021-01-31 04:36:52 +0100, Vincent Lefevre wrote:
> On 2021-01-31 02:47:59 +0100, Ævar Arnfjörð Bjarmason wrote:
> > On Fri, Jan 15 2021, Vincent Lefevre wrote:
> > > The broken pipe is internally expected, thus should not be reported
> > > by git.
> > >
> > > Just to be clear: this broken pipe should be discarded only when git
> > > uses its builtin pager feature, not with a general pipe, where the
> > > error may be important.
> > >
> > > For instance,
> > >
> > > $ { git log ; echo "Exit status: $?" >&2 ; } | true
> > >
> > > should still output
> > >
> > > Exit status: 141
> > 
> > I don't get it, how is it less meaningful when git itself invokes the
> > pager?
> 
> I don't understand your question. If I invoke the pager myself,
> I don't get a SIGPIPE:
> 
> cventin:~/software/gcc-trunk> git log
> cventin:~/software/gcc-trunk[PIPE]> git log|m
> cventin:~/software/gcc-trunk>

Well, more precisely, I mean that it is not reported. But
the SIGPIPE itself still occurs as expected, e.g. for scripts,
and one may choose to ignore it or not, as usual.

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: git fails with a broken pipe when one quits the pager

[Ævar Arnfjörð Bjarmason]

On Sun, Jan 31 2021, Vincent Lefevre wrote:

> On 2021-01-31 02:47:59 +0100, Ævar Arnfjörð Bjarmason wrote:
>> On Fri, Jan 15 2021, Vincent Lefevre wrote:
>> > I had reported the following bug at
>> >   https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914896
>> >
>> > It still occurs with Git 2.30.0.
>> >
>> > Some git commands with a lot of output fail with a broken pipe when
>> > one quits the pager (without going to the end of the output).
>> >
>> > For instance, in zsh:
>> >
>> > cventin% setopt PRINT_EXIT_VALUE
>> > cventin% git log
>> > zsh: broken pipe  git log
>> > cventin% echo $?
>> > 141
>> > cventin% 
>> >
>> > This is annoying[...]
>> 
>> Yes it's annoying, but the annoying output is from zsh, not
>> git. Consider a smarter implementation like:
>> 
>>     case $__exit_status in
>>         0) __exit_emoji=😀;;
>>         1) __exit_emoji=☹️ ;;
>>         141) __exit_emoji=🤕 ;;
>>         [...]
>> 
>> Then put the $__exit_emoji in your $PS1 prompt, now when you 'q' in a
>> pager you know the difference between having quit at the full output
>> being emitted or not.
>
> FYI, I already have the exit status already in my prompt (the above
> commands were just for the example). Still, the git behavior is
> disturbing.
>
> Moreover, this doesn't solve the issue when doing something like
>
>   git log && some_other_command

What issue? That we're returning an exit code per getting a SIGHUP here
is a feature. Consider:

    git -c core.pager=/bin/false log && echo showed you the output

Before the patch from Denton Liu we'd correctly not say that worked, now
we'll just ignore that we couldn't give the output to the pager.

>> > And of course, I don't want to hide error messages by default, because
>> > this would hide *real* errors.
>> 
>> Isn't the solution to this that your shell stops reporting failures due
>> to SIGPIPE in such a prominent way then?
>
> No! I want to be warned about real SIGPIPEs.

Not being able to write "git log" output is a real SIGPIPE.

I'm genuinely not trying to be difficult here, I just really don't see
what the conceptual difference is that would cause you to say that's not
a "real" SIGPIPE.

Is it because in your mind it's got something to do with the "|" shell
piping construct? The SIGPIPE is sent by the kernel, so it's no less
expected in cases like:

    git log && echo foo

Than:

    git log | cat

If something were to fail or the write() to the pager/pipe.

>> > The broken pipe is internally expected, thus should not be reported
>> > by git.
>> >
>> > Just to be clear: this broken pipe should be discarded only when git
>> > uses its builtin pager feature, not with a general pipe, where the
>> > error may be important.
>> >
>> > For instance,
>> >
>> > $ { git log ; echo "Exit status: $?" >&2 ; } | true
>> >
>> > should still output
>> >
>> > Exit status: 141
>> 
>> I don't get it, how is it less meaningful when git itself invokes the
>> pager?
>
> I don't understand your question. If I invoke the pager myself,
> I don't get a SIGPIPE:
>
> cventin:~/software/gcc-trunk> git log
> cventin:~/software/gcc-trunk[PIPE]> git log|m
> cventin:~/software/gcc-trunk>

Do you mean if you invoke "less <file>" yourself, as opposed to "git
log" doing it for you? I.e.:

    git log >log.txt
    less log.txt
    <type 'q' to early exit>
   # returns 0

v.s.:

    git log # using less
    <type 'q' to early exit>
    # returns 141

Yes, because e.g. under less aborting before you view the whole output
isn't an error, the SIGPIPE is sent to the writer trying to
unsuccessfully spew output to the pager.

To git the pager should be a black box. We don't know if the reason we
couldn't write output to it is because it's what the user wanted, or the
pager died on our input or whatever (as shown by setting it to
/bin/false above).

Anyway, I'm not saying that there's no place for this as an optional
feature or whatever.

Maybe we have users who'd like to work around zsh's "setopt
PRINT_EXIT_VALUE" mode (would you want this patch if you could make zsh
ignore 141?). But I think it should at least be hidden behind some
core.pagerErrorIgnore=141 or something. Some of us like standard *nix
semantics.

Re: git fails with a broken pipe when one quits the pager

[Vincent Lefevre]

On 2021-01-31 21:49:49 +0100, Ævar Arnfjörð Bjarmason wrote:
> On Sun, Jan 31 2021, Vincent Lefevre wrote:
> > FYI, I already have the exit status already in my prompt (the above
> > commands were just for the example). Still, the git behavior is
> > disturbing.
> >
> > Moreover, this doesn't solve the issue when doing something like
> >
> >   git log && some_other_command
> 
> What issue? That we're returning an exit code per getting a SIGHUP here
> is a feature. Consider:
> 
>     git -c core.pager=/bin/false log && echo showed you the output

If the pager exists with a non-zero exit status, it is normal to
return a non-zero exit status. This was not the bug I reported.

> > No! I want to be warned about real SIGPIPEs.
> 
> Not being able to write "git log" output is a real SIGPIPE.

Which is not the case here, because the full output has never been
requested by the user.

> Is it because in your mind it's got something to do with the "|" shell
> piping construct? The SIGPIPE is sent by the kernel, so it's no less
> expected in cases like:
> 
>     git log && echo foo
> 
> Than:
> 
>     git log | cat

See the difference (without the patch) between

$ git log && echo foo; echo $?
141

and

$ git log | head; echo $?
[...]
0

[...]
> Maybe we have users who'd like to work around zsh's "setopt
> PRINT_EXIT_VALUE" mode (would you want this patch if you could make zsh
> ignore 141?).

zsh is working as expected, and as I've already said, I ***WANT***
SIGPIPE to be reported by the shell, as it may indicate a real failure
in a script. BTW, I even have a script using git that relies on that:

{ git rev-list --author "$@[-1]" HEAD &&
  git rev-list --grep   "$@[-1]" HEAD } | \
  git "${@[1,-2]:-lv}" --no-walk --stdin

return $((pipestatus[2] ? pipestatus[2] : pipestatus[1]))

Here it is important not to lose any information. No pager is
involved, the full output is needed. If for some reason, the
LHS of the pipe fails due to a SIGPIPE but the right hand side
succeeds, the error will be reported.

The fact is that with a pager, the SIGPIPE with a pager is normal.
Thus with a pager, git is reporting a spurious SIGPIPE, and this
is disturbing.

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: git fails with a broken pipe when one quits the pager

[Chris Torek]

> On 2021-01-31 21:49:49 +0100, Ævar Arnfjörð Bjarmason wrote:
> > ... That we're returning an exit code per getting a SIGHUP here
> > is a feature. Consider:
> >
> >     git -c core.pager=/bin/false log && echo showed you the output

This example has a minor flaw: it should use `git -c core.pager=/bin/true`,
probably.

On Mon, Feb 1, 2021 at 2:36 AM Vincent Lefevre <vincent@vinc17.net> wrote:
> If the pager exists with a non-zero exit status, it is normal to
> return a non-zero exit status. This was not the bug I reported.

That's the flaw in the example.  The key though is that the program
we ran as the pager—false, true, whatever—*did not read any of its input*.

> > Not being able to write "git log" output is a real SIGPIPE.

Worth noting: Linux has a pretty large pipe buffer.  POSIX requires
at least 4k here, as I recall, but Linux will buffer 64k or more, so that
if `git log` is able to write the entire log text (will be the case for small
repositories) *before* the program on the right side of the pager pipe
exits (this depends on many things), the pager's exit *won't* cause
a SIGPIPE.  You'll get the SIGPIPE if either the pager exits very
quickly, so that `git log` is unable to write much before the exit, or
if the repository is sufficiently large so that the pipe blocks first.

> Which is not the case here, because the full output has never been
> requested by the user.

The `git log` command *did* request the full output.

The problem that has come up is, if I understand correctly, that
some Linux distributions have come with misconfigured pagers
that don't bother reading their input, and silently exit zero.  This
causes all kinds of Git commands to *seem* to fail.  The Git commands
are just fine; the bug is that the pager doesn't read or write anything.

Unfortunately, the way that pipes work -- asynchronously -- means
that Git really *can't* catch all problems here.  But catching a SIGPIPE,
whether Git itself spawned the pager or not, does indicate that
something has gone wrong ... *unless* Git was piping to, e.g., less,
and the user read enough, and the user typed `q` at less, and less
exited without bothering to read the rest of the input.

There's no good way for Git to be able to tell which of these was
the case.

I'm not sure what this actually argues for. ;-)

Chris

Re: git fails with a broken pipe when one quits the pager

[Vincent Lefevre]

On 2021-02-01 03:33:54 -0800, Chris Torek wrote:
> > On 2021-01-31 21:49:49 +0100, Ævar Arnfjörð Bjarmason wrote:
> > > ... That we're returning an exit code per getting a SIGHUP here
> > > is a feature. Consider:
> > >
> > >     git -c core.pager=/bin/false log && echo showed you the output
> 
> This example has a minor flaw: it should use `git -c core.pager=/bin/true`,
> probably.

In this case, since /bin/true doesn't read anything on purpose,
I would not expect any non-zero exit status.

And note that

  git -c core.pager="sh -c 'cat; /bin/false'" log

exits with a zero exit status, which is unexpected since the
pager failed. For instance, in practice, the pager could be
killed by the system, but the user would not necessarily notice
this as the pager may be configured to quit automatically when
reaching the end of the output (there are some "less" options
to do that: -E, -F). So, the user would think that he got the
full output while he didn't.

[...]
> > > Not being able to write "git log" output is a real SIGPIPE.
> 
> Worth noting: Linux has a pretty large pipe buffer.  POSIX requires
> at least 4k here, as I recall, but Linux will buffer 64k or more, so that
> if `git log` is able to write the entire log text (will be the case for small
> repositories) *before* the program on the right side of the pager pipe
> exits (this depends on many things), the pager's exit *won't* cause
> a SIGPIPE.  You'll get the SIGPIPE if either the pager exits very
> quickly, so that `git log` is unable to write much before the exit, or
> if the repository is sufficiently large so that the pipe blocks first.

In general, repositories have more than 64k log.

> > Which is not the case here, because the full output has never been
> > requested by the user.
> 
> The `git log` command *did* request the full output.

No, because the output is sent to a pager. As long as the user
does not look at more than what he looks for, no more "git log"
output is requested (such output can happen internally, but it
is not requested by the user).

> The problem that has come up is, if I understand correctly, that
> some Linux distributions have come with misconfigured pagers
> that don't bother reading their input, and silently exit zero.

They are not misconfigured. This is how they work. Actually I don't
see why they should read more than needed: this would be a useless
waste of memory.

> This causes all kinds of Git commands to *seem* to fail. The Git
> commands are just fine; the bug is that the pager doesn't read or
> write anything.
> 
> Unfortunately, the way that pipes work -- asynchronously -- means
> that Git really *can't* catch all problems here.  But catching a SIGPIPE,
> whether Git itself spawned the pager or not, does indicate that
> something has gone wrong ... *unless* Git was piping to, e.g., less,
> and the user read enough, and the user typed `q` at less, and less
> exited without bothering to read the rest of the input.
> 
> There's no good way for Git to be able to tell which of these was
> the case.

In the case git spawns a pager, it knows that this is a pager
(as per documentation).

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: git fails with a broken pipe when one quits the pager

[Chris Torek]

On Mon, Feb 1, 2021 at 4:36 AM Vincent Lefevre <vincent@vinc17.net> wrote:
> In general, repositories have more than 64k log.

Please don't focus on the exact size.  Some system might
have a multi-gigabyte pipe buffer, and some other system
might have a tiny one; we'd like consistent behavior no matter
what size the system uses.  Can we *get* consistent behavior?
I don't know.

[me]
> > The problem that has come up is, if I understand correctly, that
> > some Linux distributions have come with misconfigured pagers
> > that don't bother reading their input, and silently exit zero.
>
> They are not misconfigured. This is how they work.

A pager that reads nothing and writes nothing does not seem
very useful to me.  (Perhaps we can disregard these cases
entirely.  It's not like we should expect Git to handle things if
someone builds a version of `less` that doesn't work.  The
fact is that on these Linux systems, running `$pager foo` on a
file `foo` does nothing at all, for some values of `$pager`.  I
believe I ran into this on a Docker setup at least once.  It's
not Git's fault and hence not something for it to correct.)

[on various exit cases]
> > There's no good way for Git to be able to tell which of these was
> > the case.
>
> In the case git spawns a pager, it knows that this is a pager
> (as per documentation).

Again, this seems irrelevant.  If the pager exited correctly
while reading everything, or it exited correctly without reading
everything, or if it exited incorrectly with or without reading
everything, is not something *Git* can tell.  I'm therefore not
sure that Git should *try* to tell -- which is the point I'm trying
to make here.  The question is this: if we can only do a poor
job, should we try at all?  What *should* we do, given what
we *can* do?  All we get is SIGPIPE and an exit status, and
the SIGPIPE may or may not be meaningful.

That seems to be what you're arguing as well.  So I'm not sure
why you're objecting to what I'm pointing out. :-)

Chris

Re: git fails with a broken pipe when one quits the pager

[Vincent Lefevre]

On 2021-02-01 04:53:03 -0800, Chris Torek wrote:
> On Mon, Feb 1, 2021 at 4:36 AM Vincent Lefevre <vincent@vinc17.net> wrote:
> > In general, repositories have more than 64k log.
> 
> Please don't focus on the exact size.  Some system might
> have a multi-gigabyte pipe buffer, and some other system
> might have a tiny one; we'd like consistent behavior no matter
> what size the system uses.  Can we *get* consistent behavior?
> I don't know.

The consistent behavior can be obtained by ignoring the broken pipe
(in the case where git starts the pager).

> [me]
> > > The problem that has come up is, if I understand correctly, that
> > > some Linux distributions have come with misconfigured pagers
> > > that don't bother reading their input, and silently exit zero.
> >
> > They are not misconfigured. This is how they work.
> 
> A pager that reads nothing and writes nothing does not seem
> very useful to me. [...]

I agree.

> [on various exit cases]
> > > There's no good way for Git to be able to tell which of these was
> > > the case.
> >
> > In the case git spawns a pager, it knows that this is a pager
> > (as per documentation).
> 
> Again, this seems irrelevant.  If the pager exited correctly
> while reading everything, or it exited correctly without reading
> everything, or if it exited incorrectly with or without reading
> everything, is not something *Git* can tell.

No, Git can tell when the pager exited abnormally: it suffices to
check its exit status. Git currently doesn't do that, and this is
bad, because it can miss real issues, which cannot always be detected
by the user.

If the pager exits with exit code 0, this means normal termination,
whether the user has read the full output or not.

> I'm therefore not sure that Git should *try* to tell -- which is the
> point I'm trying to make here. The question is this: if we can only
> do a poor job, should we try at all? What *should* we do, given what
> we *can* do? All we get is SIGPIPE and an exit status, and the
> SIGPIPE may or may not be meaningful.
> 
> That seems to be what you're arguing as well.  So I'm not sure
> why you're objecting to what I'm pointing out. :-)

Well, my objection is based on the fact that it is possible to get
the information from the exit status of the pager (I originally
thought that Git was taking it into account).

BTW, another related thing I dislike about Git, and I think that this
should also be regarded as a bug, is that when doing a commit, Git
doesn't check the exit status of the editor for the commit message.
Say, for instance, if something on the system kills the editor, Git
applies the commit with an incorrect or incomplete log message though
the commit wasn't validated yet by the user. Fortunately, the user
can amend the commit, but IMHO, that's an incorrect behavior.

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: git fails with a broken pipe when one quits the pager

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 01 2021, Chris Torek wrote:

>> On 2021-01-31 21:49:49 +0100, Ævar Arnfjörð Bjarmason wrote:
>> > ... That we're returning an exit code per getting a SIGHUP here
>> > is a feature. Consider:
>> >
>> >     git -c core.pager=/bin/false log && echo showed you the output
>
> This example has a minor flaw: it should use `git -c core.pager=/bin/true`,
> probably.

FWIW it doesn't have a flaw It should be /bin/false, not /bin/true. See
this reply in a side-thread:
https://lore.kernel.org/git/87im7cng42.fsf@evledraar.gmail.com/

I.e. part of the point here (which I realize I forgot to articulate...)
is that we have a hard reliance on SIGHUP to report *any* pager failures
as a matter of the current implementation.

Part of that has to do with internal git implementation details, i.e. we
get the exit code for the pager either in an atexit() handler (we've
already picked the exit code) or when handling a signal.

Perhaps we could do better there and e.g. exit with <num> if the pager
exits with <num>. I don't know what's the conventional behavior in that
case.

But in any case, we exit with SIGPIPE in those cases in any reasonable
failure mode. That is, unless the pager consumed all the output, and
*then* died that is.

I submitted
https://lore.kernel.org/git/20210201144921.8664-1-avarab@gmail.com/ to
try to address the lack of testing around this, which has tests for the
true/false case.

Re: git fails with a broken pipe when one quits the pager

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 01 2021, Vincent Lefevre wrote:

> On 2021-01-31 21:49:49 +0100, �var Arnfj�r� Bjarmason wrote:
>> On Sun, Jan 31 2021, Vincent Lefevre wrote:
>> > FYI, I already have the exit status already in my prompt (the above
>> > commands were just for the example). Still, the git behavior is
>> > disturbing.
>> >
>> > Moreover, this doesn't solve the issue when doing something like
>> >
>> >   git log && some_other_command
>> 
>> What issue? That we're returning an exit code per getting a SIGHUP here
>> is a feature. Consider:
>> 
>>     git -c core.pager=/bin/false log && echo showed you the output
>
> If the pager exists with a non-zero exit status, it is normal to
> return a non-zero exit status. This was not the bug I reported.

Is it normal? Isn't this subject to the same race noted in
https://lore.kernel.org/git/20191115040909.GA21654@sigill.intra.peff.net/

I.e. we start the /bin/false process, then start spewing output to it,
so maybe we'll get a SIGPIPE first because it's not being consumed, or
maybe /bin/false (or whatever else exits with non-zero) will exit first.

Unrelated to that there's at least a bug in wait_for_pager_signal() in
how we log the pager's exit. We just rely on "ret" in
finish_command_in_signal() before calling trace2_child_exit(), but
should probably log -1 there or something and defer.

>> > No! I want to be warned about real SIGPIPEs.
>> 
>> Not being able to write "git log" output is a real SIGPIPE.
>
> Which is not the case here, because the full output has never been
> requested by the user.

They requested it by running "git log", which e.g. for git.git is ~1
million lines. Then presumably paged down just a few pages and issued
"q" in their pager. At which point we'll fail on the write() in git-log.

The pager's exit status is usually/always 0 in those cases
(e.g. https://pubs.opengroup.org/onlinepubs/9699919799/utilities/more.html). So
we've got the SIGPIPE to indicate the output wasn't fully consumed.

>> Is it because in your mind it's got something to do with the "|" shell
>> piping construct? The SIGPIPE is sent by the kernel, so it's no less
>> expected in cases like:
>> 
>>     git log && echo foo
>> 
>> Than:
>> 
>>     git log | cat
>
> See the difference (without the patch) between
>
> $ git log && echo foo; echo $?
> 141
>
> and
>
> $ git log | head; echo $?
> [...]
> 0

Presumably that first command is one where you exited your pager before
the output wasn't fully consumed, see above.

> [...]
>> Maybe we have users who'd like to work around zsh's "setopt
>> PRINT_EXIT_VALUE" mode (would you want this patch if you could make zsh
>> ignore 141?).
>
> zsh is working as expected, and as I've already said, I ***WANT***
> SIGPIPE to be reported by the shell, as it may indicate a real failure
> in a script. BTW, I even have a script using git that relies on that:
>
> { git rev-list --author "$@[-1]" HEAD &&
>   git rev-list --grep   "$@[-1]" HEAD } | \
>   git "${@[1,-2]:-lv}" --no-walk --stdin
>
> return $((pipestatus[2] ? pipestatus[2] : pipestatus[1]))
>
> Here it is important not to lose any information. No pager is
> involved, the full output is needed. If for some reason, the
> LHS of the pipe fails due to a SIGPIPE but the right hand side
> succeeds, the error will be reported.

Sorry, I really don't see how this is different. I think this goes back
to my "'|' shell piping construct[...]" question in the E-Mail you're
replying to.

in both the "git log &&" case and potentially here you'll get a program
writing to a pipe getting a SIGPIPE, which is then reflected in the exit
code.

> The fact is that with a pager, the SIGPIPE with a pager is normal.
> Thus with a pager, git is reporting a spurious SIGPIPE, and this
> is disturbing.

I don't get what you're trying to say here, sorry.

Maybe this helps. So first, I don't know if your report came out of
reading the recent "set -o pipefail" traffic on-list. As you can see in
[1] I'm not some zealot for PIPEFAIL always being returned no matter
what.

The difference between that though and what you're proposing is there
you have the shell getting an exit code and opting to ignore it, as
opposed to the program itself sweeping it under the rug.

I don't think either that just because you run a pager you're obligated
to ferry down a SIGPIPE if you get it. E.g. mysql and postgresql both
have interactive shells where you can open pagers. I didn't bother to
check, but you can imagine doing a "show tables" or whatever and only
viewing the first page, then quitting in the pager.

If that's part of a long interactive SQL session it would make no sense
for the eventual exit code of mysql(1) or psql(1) to reflect that.

But with git we're (mostly) executing one-shot commands, e.g. with "git
log" you give it some params, and it spews all the output at you, maybe
with the help of a pager.

So then if we fail on the write() I don't see how it doesn't make sense
to return the appropriate exit code for that failure downstream.

1. https://lore.kernel.org/git/20210116153554.12604-12-avarab@gmail.com/

Re: git fails with a broken pipe when one quits the pager

[Vincent Lefevre]

On 2021-02-01 13:10:21 +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Feb 01 2021, Vincent Lefevre wrote:
> 
> > On 2021-01-31 21:49:49 +0100, �var Arnfj�r� Bjarmason wrote:
> >> On Sun, Jan 31 2021, Vincent Lefevre wrote:
> >> > FYI, I already have the exit status already in my prompt (the above
> >> > commands were just for the example). Still, the git behavior is
> >> > disturbing.
> >> >
> >> > Moreover, this doesn't solve the issue when doing something like
> >> >
> >> >   git log && some_other_command
> >> 
> >> What issue? That we're returning an exit code per getting a SIGHUP here
> >> is a feature. Consider:
> >> 
> >>     git -c core.pager=/bin/false log && echo showed you the output
> >
> > If the pager exists with a non-zero exit status, it is normal to
> > return a non-zero exit status. This was not the bug I reported.
> 
> Is it normal? Isn't this subject to the same race noted in
> https://lore.kernel.org/git/20191115040909.GA21654@sigill.intra.peff.net/

There's a race only because the command is buggy under bash's
pipefail.

Something like

  git status -s -b | head -1

is fine by default, because the exist status of the LHS command is
ignored. With pipefail, you may start getting SIGPIPE exit codes,
which, depending on the context, you may want to ignore or not.
I suppose that the user who writes something like the above would
like to ignore SIGPIPE.

So, that should be:

  { git status -s -b; if [[ $? = 141 ]]; then return 0; fi } | head -1

(though that's 100% safe only if git catches/blocks/ignores SIGPIPE
and detect the broken pipe with EPIPE, so that an abnormal termination
due to a "kill -PIPE ..." from another process would not be ignored).

It appears that pipefail was designed mainly for scripts. So, having
to handle SIGPIPE like that is OK in scripts. For interactive use,
this would be bad, but that's not the purpose of pipefail (or bash
should have an option to regard 141 as 0 in any LHS command).

FYI, I have a zsh function to automatically pipe some commands to
"less" when connected to a terminal (a bit like what git does),
where I explicitly ignore SIGPIPE for the command:

pager-wrapper()
{
  local -a opt
  while [[ $1 == -* ]]
  do
    opt+=$1
    shift
  done
  if [[ -t 1 ]] then
    $@ $opt |& less -+c -FRX
    return $(( $pipestatus[2] != 0 ? $pipestatus[2] :
               $pipestatus[1] != 128 + $(kill -l PIPE) ? $pipestatus[1] : 0 ))
  else
    $@
  fi
}

So no SIGPIPE is reported when I quit the pager. I can still get a
reported SIGPIPE, e.g. if "less" is killed by SIGPIPE (e.g., this
is possible with "kill -PIPE ..."), and this one is meaningful.

> >> > No! I want to be warned about real SIGPIPEs.
> >> 
> >> Not being able to write "git log" output is a real SIGPIPE.
> >
> > Which is not the case here, because the full output has never been
> > requested by the user.
> 
> They requested it by running "git log", which e.g. for git.git is ~1
> million lines. Then presumably paged down just a few pages and issued
> "q" in their pager. At which point we'll fail on the write() in git-log.

But when outputting to a pager, this should not be regarded as an
error: the reason is either the user has quit the pager normally
(after having read what he wanted to read: the user did not need
more output) or the pager has terminated in an abnormal way, in
which case the exit status of the pager should be non-zero.

> The pager's exit status is usually/always 0 in those cases
> (e.g. https://pubs.opengroup.org/onlinepubs/9699919799/utilities/more.html).

Yes, and there's no reason to return anything else, as quitting the
pager before reading the full output is not an error.

> So we've got the SIGPIPE to indicate the output wasn't fully
> consumed.

But the user doesn't care: he quit the pager because he didn't
need more output. So there is no need to signal that the output
wasn't fully consumed. The user already knew that before quitting
the pager!

> > [...]
> >> Maybe we have users who'd like to work around zsh's "setopt
> >> PRINT_EXIT_VALUE" mode (would you want this patch if you could make zsh
> >> ignore 141?).
> >
> > zsh is working as expected, and as I've already said, I ***WANT***
> > SIGPIPE to be reported by the shell, as it may indicate a real failure
> > in a script. BTW, I even have a script using git that relies on that:
> >
> > { git rev-list --author "$@[-1]" HEAD &&
> >   git rev-list --grep   "$@[-1]" HEAD } | \
> >   git "${@[1,-2]:-lv}" --no-walk --stdin
> >
> > return $((pipestatus[2] ? pipestatus[2] : pipestatus[1]))
> >
> > Here it is important not to lose any information. No pager is
> > involved, the full output is needed. If for some reason, the
> > LHS of the pipe fails due to a SIGPIPE but the right hand side
> > succeeds, the error will be reported.
> 
> Sorry, I really don't see how this is different. I think this goes back
> to my "'|' shell piping construct[...]" question in the E-Mail you're
> replying to.
> 
> in both the "git log &&" case and potentially here you'll get a program
> writing to a pipe getting a SIGPIPE, which is then reflected in the exit
> code.

I mean that there are SIGPIPEs that one does not want to ignore
(because they would indicate a problem -- in general in scripts),
and other ones that should be ignored because they don't indicate
an error.

> > The fact is that with a pager, the SIGPIPE with a pager is normal.
> > Thus with a pager, git is reporting a spurious SIGPIPE, and this
> > is disturbing.
> 
> I don't get what you're trying to say here, sorry.

I mean that when the user quits the pager, there is no reason to
report an error because the user explicitly wanted to quit now.

Similarly, if I run a text viewer on a file, I don't want a SIGPIPE
to be reported if I do not go to the end of the file (if a pipe was
used to read the file, e.g. to do some filtering, as "less" can do).

> Maybe this helps. So first, I don't know if your report came out of
> reading the recent "set -o pipefail" traffic on-list. As you can see in
> [1] I'm not some zealot for PIPEFAIL always being returned no matter
> what.

This is not related. And [1] is from 2021 (with a thread started
in 2019), while my report dates back to 2018:

  https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914896

Moreover, [1] is only about the use of pipes in the shell command.
My bug report is about the internal use of a pager by git.

> The difference between that though and what you're proposing is there
> you have the shell getting an exit code and opting to ignore it, as
> opposed to the program itself sweeping it under the rug.
> 
> I don't think either that just because you run a pager you're obligated
> to ferry down a SIGPIPE if you get it. E.g. mysql and postgresql both
> have interactive shells where you can open pagers. I didn't bother to
> check, but you can imagine doing a "show tables" or whatever and only
> viewing the first page, then quitting in the pager.
> 
> If that's part of a long interactive SQL session it would make no sense
> for the eventual exit code of mysql(1) or psql(1) to reflect that.
> 
> But with git we're (mostly) executing one-shot commands, e.g. with "git
> log" you give it some params, and it spews all the output at you, maybe
> with the help of a pager.
> 
> So then if we fail on the write() I don't see how it doesn't make sense
> to return the appropriate exit code for that failure downstream.

This depends on the kind of error. I agree for an unexpected error.
But for a broken pipe because git started a pager on its own and
the user chose to quit the pager, this should not be regarded as
an error.

> 1. https://lore.kernel.org/git/20210116153554.12604-12-avarab@gmail.com/

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: git fails with a broken pipe when one quits the pager

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 01 2021, Vincent Lefevre wrote:

> On 2021-02-01 13:10:21 +0100, �var Arnfj�r� Bjarmason wrote:
>> 
n>> On Mon, Feb 01 2021, Vincent Lefevre wrote:
>> 
>> > On 2021-01-31 21:49:49 +0100, �var Arnfj�r� Bjarmason wrote:
>> >> On Sun, Jan 31 2021, Vincent Lefevre wrote:
>> >> > FYI, I already have the exit status already in my prompt (the above
>> >> > commands were just for the example). Still, the git behavior is
>> >> > disturbing.
>> >> >
>> >> > Moreover, this doesn't solve the issue when doing something like
>> >> >
>> >> >   git log && some_other_command
>> >> 
>> >> What issue? That we're returning an exit code per getting a SIGHUP here
>> >> is a feature. Consider:
>> >> 
>> >>     git -c core.pager=/bin/false log && echo showed you the output
>> >
>> > If the pager exists with a non-zero exit status, it is normal to
>> > return a non-zero exit status. This was not the bug I reported.
>> 
>> Is it normal? Isn't this subject to the same race noted in
>> https://lore.kernel.org/git/20191115040909.GA21654@sigill.intra.peff.net/
>
> There's a race only because the command is buggy under bash's
> pipefail.
>
> Something like
>
>   git status -s -b | head -1
>
> is fine by default, because the exist status of the LHS command is
> ignored. With pipefail, you may start getting SIGPIPE exit codes,
> which, depending on the context, you may want to ignore or not.
> I suppose that the user who writes something like the above would
> like to ignore SIGPIPE.
>
> So, that should be:
>
>   { git status -s -b; if [[ $? = 141 ]]; then return 0; fi } | head -1
>
> (though that's 100% safe only if git catches/blocks/ignores SIGPIPE
> and detect the broken pipe with EPIPE, so that an abnormal termination
> due to a "kill -PIPE ..." from another process would not be ignored).
>
> It appears that pipefail was designed mainly for scripts. So, having
> to handle SIGPIPE like that is OK in scripts. For interactive use,
> this would be bad, but that's not the purpose of pipefail (or bash
> should have an option to regard 141 as 0 in any LHS command).
>
> FYI, I have a zsh function to automatically pipe some commands to
> "less" when connected to a terminal (a bit like what git does),
> where I explicitly ignore SIGPIPE for the command:

I think there's some confusion here. I'm not referring to how "set -o
pipefail" behaves in bash. But pointing to Jeff King's simple example[1]
of how a command like "git status -sb" might exit (note that it may
print more than 1 line) due to a race with how SIGPIPE interacts with
exit statuses. That's *nix/POSIX behavior, nothing to do with bash.

The same will apply to a pager we launch on a command like "git log".

As Chris Torek noted in a side-thread[2] the buffers involved here are
OS-defined. In the general case you may get a PIPEFAIL or not depending
on whether you e.g. cross a PIPE_BUF boundary to get from line 1 to 2 of
your output, while "head -n 1" is consuming it.

But then consider a pager like:

    while (wantit())
	    consume_and_print_output();
    sleep(10);
    exit(1);

Now we can just exit early if it decides it doesn't want our output, as
we'll likely get a SIGPIPE, but if we're ignoring SIGPIPE and we want to
distinguish that from non-zero pager exit codes, we need to wait 10
seconds until waitpid() tells us what the exit status is.

That's obviously a contrived example, but demonstrates the race
condition involved.

1. https://lore.kernel.org/git/20191115040909.GA21654@sigill.intra.peff.net/
2. https://lore.kernel.org/git/CAPx1Gverh2E2h5JOSOfJ7JYvbhjv8hJNLE8y4VA2fNv0La8Rtw@mail.gmail.com/

>> >> 
>> >> Not being able to write "git log" output is a real SIGPIPE.
>> >
>> > Which is not the case here, because the full output has never been
>> > requested by the user.
>> 
>> They requested it by running "git log", which e.g. for git.git is ~1
>> million lines. Then presumably paged down just a few pages and issued
>> "q" in their pager. At which point we'll fail on the write() in git-log.
>
> But when outputting to a pager, this should not be regarded as an
> error: the reason is either the user has quit the pager normally
> (after having read what he wanted to read: the user did not need
> more output) or the pager has terminated in an abnormal way, in
> which case the exit status of the pager should be non-zero.

In an ideal world, or something we can plausibly implement in a portable
manner on systems that exist in the wild?

Yes I agree that this sort of behavior would be stupid e.g. for an
integrated GUI application, but that's not what we've got. We're calling
an arbitrary user-supplied command and piping output to it, and are then
going to get SIGPIPE or an exit code back.

>> The pager's exit status is usually/always 0 in those cases
>> (e.g. https://pubs.opengroup.org/onlinepubs/9699919799/utilities/more.html).
>
> Yes, and there's no reason to return anything else, as quitting the
> pager before reading the full output is not an error.
>
>> So we've got the SIGPIPE to indicate the output wasn't fully
>> consumed.
>
> But the user doesn't care: he quit the pager because he didn't
> need more output. So there is no need to signal that the output
> wasn't fully consumed. The user already knew that before quitting
> the pager!

As noted above, this is assuming way too much about the functionality of
the pager command. We can get a SIGPIPE without the user's intent in
this way. Consider e.g. piping to some remote system via netcat.

>> > [...]
>> >> Maybe we have users who'd like to work around zsh's "setopt
>> >> PRINT_EXIT_VALUE" mode (would you want this patch if you could make zsh
>> >> ignore 141?).
>> >
>> > zsh is working as expected, and as I've already said, I ***WANT***
>> > SIGPIPE to be reported by the shell, as it may indicate a real failure
>> > in a script. BTW, I even have a script using git that relies on that:
>> >
>> > { git rev-list --author "$@[-1]" HEAD &&
>> >   git rev-list --grep   "$@[-1]" HEAD } | \
>> >   git "${@[1,-2]:-lv}" --no-walk --stdin
>> >
>> > return $((pipestatus[2] ? pipestatus[2] : pipestatus[1]))
>> >
>> > Here it is important not to lose any information. No pager is
>> > involved, the full output is needed. If for some reason, the
>> > LHS of the pipe fails due to a SIGPIPE but the right hand side
>> > succeeds, the error will be reported.
>> 
>> Sorry, I really don't see how this is different. I think this goes back
>> to my "'|' shell piping construct[...]" question in the E-Mail you're
>> replying to.
>> 
>> in both the "git log &&" case and potentially here you'll get a program
>> writing to a pipe getting a SIGPIPE, which is then reflected in the exit
>> code.
>
> I mean that there are SIGPIPEs that one does not want to ignore
> (because they would indicate a problem -- in general in scripts),
> and other ones that should be ignored because they don't indicate
> an error.
>
>> > The fact is that with a pager, the SIGPIPE with a pager is normal.
>> > Thus with a pager, git is reporting a spurious SIGPIPE, and this
>> > is disturbing.
>> 
>> I don't get what you're trying to say here, sorry.
>
> I mean that when the user quits the pager, there is no reason to
> report an error because the user explicitly wanted to quit now.

Sure, in an ideal world. But we don't get a SIGUSERPRESSEDTHEQBUTTON, we
get a SIGPIPE.

> Similarly, if I run a text viewer on a file, I don't want a SIGPIPE
> to be reported if I do not go to the end of the file (if a pipe was
> used to read the file, e.g. to do some filtering, as "less" can do).

Yes, that makes perfect sense. Neither would I, but that text viewer is
one process, so it doesn't have to deal with IPC and propagating exit
codes from failed IPC.

>> Maybe this helps. So first, I don't know if your report came out of
>> reading the recent "set -o pipefail" traffic on-list. As you can see in
>> [1] I'm not some zealot for PIPEFAIL always being returned no matter
>> what.
>
> This is not related. And [1] is from 2021 (with a thread started
> in 2019), while my report dates back to 2018:
>
>   https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914896

Indeed, I just misread (or didn't read in the first place) the times
involved. I started reading at Denton Liu's patch sent a couple of days
ago.

> Moreover, [1] is only about the use of pipes in the shell command.
> My bug report is about the internal use of a pager by git.

I probably shouldn't have linked to that thread, but as as noted at the
start of the E-Mail I was referring to it for the SIGPIPE behavior
discussed there, not bash/set -o pipefail etc.

>> The difference between that though and what you're proposing is there
>> you have the shell getting an exit code and opting to ignore it, as
>> opposed to the program itself sweeping it under the rug.
>> 
>> I don't think either that just because you run a pager you're obligated
>> to ferry down a SIGPIPE if you get it. E.g. mysql and postgresql both
>> have interactive shells where you can open pagers. I didn't bother to
>> check, but you can imagine doing a "show tables" or whatever and only
>> viewing the first page, then quitting in the pager.
>> 
>> If that's part of a long interactive SQL session it would make no sense
>> for the eventual exit code of mysql(1) or psql(1) to reflect that.
>> 
>> But with git we're (mostly) executing one-shot commands, e.g. with "git
>> log" you give it some params, and it spews all the output at you, maybe
>> with the help of a pager.
>> 
>> So then if we fail on the write() I don't see how it doesn't make sense
>> to return the appropriate exit code for that failure downstream.
>
> This depends on the kind of error. I agree for an unexpected error.
> But for a broken pipe because git started a pager on its own and
> the user chose to quit the pager, this should not be regarded as
> an error.

As noted above, we don't have a way of knowing that, we're not the
pager.

It also seems to me that whether git should report errors, and what 3rd
party tools that might invoke git are going to do with a SIGPIPE exit
code is being mixed up here.

And then whether it makes sense to ignore SIGPIPE for all users, or
e.g. if it's some opt-in setting in some situations that users might
want to turn on because they're aware of how their pager behaves and
want to work around some zsh mode.

>> 1. https://lore.kernel.org/git/20210116153554.12604-12-avarab@gmail.com/

Re: git fails with a broken pipe when one quits the pager

[Johannes Sixt]

Am 01.02.21 um 16:44 schrieb Ævar Arnfjörð Bjarmason:
> On Mon, Feb 01 2021, Vincent Lefevre wrote:
>> On 2021-02-01 13:10:21 +0100, Ævar Arnfjörð Bjarmason:
>>> So we've got the SIGPIPE to indicate the output wasn't fully
>>> consumed.
>>
>> But the user doesn't care: he quit the pager because he didn't
>> need more output. So there is no need to signal that the output
>> wasn't fully consumed. The user already knew that before quitting
>> the pager!
> 
> As noted above, this is assuming way too much about the functionality of
> the pager command. We can get a SIGPIPE without the user's intent in
> this way. Consider e.g. piping to some remote system via netcat.

That assumption is warranted, IMO. Aren't _you_ stretching the meaning
of "pager" too far here? A pager is intended for presentation to the
user. If someone plays games with it, they should know what they get.

-- Hannes

Re: git fails with a broken pipe when one quits the pager

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 01 2021, Johannes Sixt wrote:

> Am 01.02.21 um 16:44 schrieb Ævar Arnfjörð Bjarmason:
>> On Mon, Feb 01 2021, Vincent Lefevre wrote:
>>> On 2021-02-01 13:10:21 +0100, Ævar Arnfjörð Bjarmason:
>>>> So we've got the SIGPIPE to indicate the output wasn't fully
>>>> consumed.
>>>
>>> But the user doesn't care: he quit the pager because he didn't
>>> need more output. So there is no need to signal that the output
>>> wasn't fully consumed. The user already knew that before quitting
>>> the pager!
>> 
>> As noted above, this is assuming way too much about the functionality of
>> the pager command. We can get a SIGPIPE without the user's intent in
>> this way. Consider e.g. piping to some remote system via netcat.
>
> That assumption is warranted, IMO. Aren't _you_ stretching the meaning
> of "pager" too far here? A pager is intended for presentation to the
> user. If someone plays games with it, they should know what they get.

FWIW I replied to this in
https://lore.kernel.org/git/87r1lxeuoj.fsf@evledraar.gmail.com/

Whatever anyone thinks of the virtues of passing down SIGHUP having e.g
a nc to a remote box be your pager isn't all that unusual.

Re: git fails with a broken pipe when one quits the pager

[Johannes Sixt]

Am 03.02.21 um 03:48 schrieb Ævar Arnfjörð Bjarmason:
> 
> On Mon, Feb 01 2021, Johannes Sixt wrote:
> 
>> Am 01.02.21 um 16:44 schrieb Ævar Arnfjörð Bjarmason:
>>> On Mon, Feb 01 2021, Vincent Lefevre wrote:
>>>> On 2021-02-01 13:10:21 +0100, Ævar Arnfjörð Bjarmason:
>>>>> So we've got the SIGPIPE to indicate the output wasn't fully
>>>>> consumed.
>>>>
>>>> But the user doesn't care: he quit the pager because he didn't
>>>> need more output. So there is no need to signal that the output
>>>> wasn't fully consumed. The user already knew that before quitting
>>>> the pager!
>>>
>>> As noted above, this is assuming way too much about the functionality of
>>> the pager command. We can get a SIGPIPE without the user's intent in
>>> this way. Consider e.g. piping to some remote system via netcat.
>>
>> That assumption is warranted, IMO. Aren't _you_ stretching the meaning
>> of "pager" too far here? A pager is intended for presentation to the
>> user. If someone plays games with it, they should know what they get.
> 
> FWIW I replied to this in
> https://lore.kernel.org/git/87r1lxeuoj.fsf@evledraar.gmail.com/
> 
> Whatever anyone thinks of the virtues of passing down SIGHUP having e.g
> a nc to a remote box be your pager isn't all that unusual.

A pager in any form is fair game. That point is that it is an
*interactive* form of presentation. But you should not use git's pager
as data post-processing facility; that would stretch the meaning of
"pager" too far, and we do not have cater for such abuse of the feature.

-- Hannes

Re: git fails with a broken pipe when one quits the pager

[Vincent Lefevre]

On 2021-02-01 16:44:24 +0100, Ævar Arnfjörð Bjarmason wrote:
> And then whether it makes sense to ignore SIGPIPE for all users, or
> e.g. if it's some opt-in setting in some situations that users might
> want to turn on because they're aware of how their pager behaves and
> want to work around some zsh mode.

AFAIK, SIGPIPE exists for the following reason. Most programs that
generate output are not written to specifically handle pipes. So,
if SIGPIPE did not exist, there would be 2 kinds of behavior:
  1. The program doesn't check for errors, and still outputs data,
     wasting time and resources as output will be ignored.
  2. The program sees that the write() failed and terminates with
     an error message. However, in most cases, such a failure is
     not an error: the consumer has terminated either because it
     no longer needs any input (e.g. with the "head" utility or a
     pager), or because it has terminated abnormally, in which case
     the real error is on the side of the consumer. So, the error
     message from the LHS of the pipe would be annoying.

SIGPIPE solves this issue: the program is simply killed with SIGPIPE.
In a shell, one gets a non-zero exit code (128 + 13) due to the
signal, but as being on the left-hand side of the pipe, such a
non-zero exit code is normally not reported, so that this will not
annoy the user.

Note 1: Non-zero exit codes from right-hand side are not reported
either by most shells, but zsh can report them, and this is very
useful for developers, as programs may fail with a non-zero exit
code but without an error message. (Reports may also be done by
looking at the standard $? in some hook.)

Note 2: Failures on the left-hand side are less interesting in practice
and generally ignored, at least for commands run in interactive shells.
For scripts, there are various (non-simple) ways to handle them.

Now, I think that in the case (like Git) a program creates a pipe,
it should use its knowledge to handle SIGPIPE / EPIPE. Either this
is regarded as an error because the full output is *always expected*
to be read, in which case there should be an error message in addition
to the usual non-zero exist status (not necessarily 141), or this is
regarded as OK (if there is a real failure, this is on the side of
the consumer). In the case of Git, the consumer is documented to be
a pager, which obviously may not read the full output (e.g. for the
GCC repository, "git log" returns more than 3 million lines, back to
the year 1988, while one is generally interested in the latest changes
only). If the user wants to pipe to something else, he can always use
an explicit pipe.

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: git fails with a broken pipe when one quits the pager

[Ævar Arnfjörð Bjarmason]

On Wed, Feb 03 2021, Vincent Lefevre wrote:

> On 2021-02-01 16:44:24 +0100, Ævar Arnfjörð Bjarmason wrote:
>> And then whether it makes sense to ignore SIGPIPE for all users, or
>> e.g. if it's some opt-in setting in some situations that users might
>> want to turn on because they're aware of how their pager behaves and
>> want to work around some zsh mode.
>
> AFAIK, SIGPIPE exists for the following reason. Most programs that
> generate output are not written to specifically handle pipes. So,
> if SIGPIPE did not exist, there would be 2 kinds of behavior:
>   1. The program doesn't check for errors, and still outputs data,
>      wasting time and resources as output will be ignored.
>   2. The program sees that the write() failed and terminates with
>      an error message. However, in most cases, such a failure is
>      not an error: the consumer has terminated either because it
>      no longer needs any input (e.g. with the "head" utility or a
>      pager), or because it has terminated abnormally, in which case
>      the real error is on the side of the consumer. So, the error
>      message from the LHS of the pipe would be annoying.
>
> SIGPIPE solves this issue: the program is simply killed with SIGPIPE.
> In a shell, one gets a non-zero exit code (128 + 13) due to the
> signal, but as being on the left-hand side of the pipe, such a
> non-zero exit code is normally not reported, so that this will not
> annoy the user.
>
> Note 1: Non-zero exit codes from right-hand side are not reported
> either by most shells, but zsh can report them, and this is very
> useful for developers, as programs may fail with a non-zero exit
> code but without an error message. (Reports may also be done by
> looking at the standard $? in some hook.)
>
> Note 2: Failures on the left-hand side are less interesting in practice
> and generally ignored, at least for commands run in interactive shells.
> For scripts, there are various (non-simple) ways to handle them.
>
> Now, I think that in the case (like Git) a program creates a pipe,
> it should use its knowledge to handle SIGPIPE / EPIPE. Either this
> is regarded as an error because the full output is *always expected*
> to be read, in which case there should be an error message in addition
> to the usual non-zero exist status (not necessarily 141), or this is
> regarded as OK (if there is a real failure, this is on the side of
> the consumer). In the case of Git, the consumer is documented to be
> a pager, which obviously may not read the full output (e.g. for the
> GCC repository, "git log" returns more than 3 million lines, back to
> the year 1988, while one is generally interested in the latest changes
> only). If the user wants to pipe to something else, he can always use
> an explicit pipe.

SIGPIPE exists because *nix systems are composable, so you can make
something useful by stringing together unrelated programs via files and
pipes, and with exit codes and signal mostly everyone's happy.

I follow what you're saying right until the point of arguing that
because either your shell or POSIX shells in general have decided to
either be sloppy or overzelous in how they show you some
information. That we should use their behavior as a guide in
pro-actively suppressing our own exit code.

And that's not because I think (to the tune of Monty Python...) that
every exit code is sacret. It's because when we invoke a pager handing
it data is *the* thing we're doing. If we can fully hand it over, great,
if not, let's tell the user with the appropriate exit code.

Yeah it's annoying with zsh's PRINT_EXIT_VALUE, but the same is true of
POSIX "set -e". Not every shell option is meant for general use. The
shell is very forgiving of things like pipe failures by default for a
reason.

But "connected to a terminal" (isatty(1)) and "invoked by Vincent's zsh
instance" aren't the same thing. And I think it makes sense to be
conservative in preserving exit codes.

In the early days of git complaining about "Broken pipe" in the exact
same scenario was the default behavior of bash's overzelous reporting,
as you can read about starting here:
https://lore.kernel.org/git/?q=%22Broken+pipe%22+bash&o=-1

AFAICT that changed by default in bash 3.1, released in 2005-12-08. It
was a FAQ in the early days, now nobody cares.

Have you reported this as a bug to zsh? I think it's likely that the
motivation for wanting this squashed in git is going to be as transitory
as bash's once-default verbosity was.

I also tested "hg log", it behaves the same way, although interestingly
they cast SIGPIPE to 255 in their exit code.

Re: git fails with a broken pipe when one quits the pager

[Vincent Lefevre]

On 2021-02-04 01:14:10 +0100, Ævar Arnfjörð Bjarmason wrote:
> Have you reported this as a bug to zsh?

I repeat: there is no bug in zsh. It is my choice to output the
exit status when it is non-zero because I want to know when the
command I've typed fails. This is useful in practice. Ignoring the
specific value 141 (corresponding to SIGPIPE) is not a solution
because it can be a real failure with some utilities. BTW, the
association with a signal like SIGPIPE is just a convention; apart
from that, 141 is a non-zero status like others (in particular
with programs that have not been written for POSIX).

For instance, in any shell:

$ sh -c "echo foo; exit 141"
foo
$ echo $?
141

while no broken pipe is involved here. How would you differentiate
such a failure from a broken pipe?

> I also tested "hg log", it behaves the same way, although interestingly
> they cast SIGPIPE to 255 in their exit code.

I get 141, like with git:

$ hg log
$ echo $?
141

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

[PATCH 0/3] pager: test for exit behavior & trace2 bug fix

[Ævar Arnfjörð Bjarmason]

While reading the pager code I discovered[1] that we log the wrong
exit code when the pager itself exits with non-zero under trace2. This
fixes that bug.

I think whatever the consensus is on the SIGPIPE exit status
propagating it makes sense if we'd ignore it to rebase the patch to do
so[1] on this. I think the addition of a new "test-tool pager" there
is redundant to testing SIGHUP from git itself as 1/3 does here, but
maybe I'm missing something...

2/3 is not needed for the end-state here, but I figured it was a good
refactoring while I was at it.

1. https://lore.kernel.org/git/bc88492979fee215d5be06ccbc246ae0171a9ced.1611910122.git.liu.denton@gmail.com/

Ævar Arnfjörð Bjarmason (3):
  pager: test for exit code
  pager: refactor wait_for_pager() function
  pager: properly log pager exit code when signalled

 pager.c          | 18 +++++--------
 run-command.c    |  8 ++++--
 t/t7006-pager.sh | 70 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 13 deletions(-)

-- 
2.30.0.284.gd98b1dd5eaa7

[PATCH v2 0/5] pager: test for exit behavior & trace2 bug fix

[Ævar Arnfjörð Bjarmason]

A v2 with better tests and some misc adjustments. As noted in v1 1-4
are just adding better test coverage for behavior we already have, and
fixing a small bug in trace2 output.

The 5/5 is a WIP start at respecting the pager's exit code and
ignoring the SIGPIPE. Junio had a suggestion to do that in
<xmqq8s87ld8y.fsf@gitster.c.googlers.com>, but as seen & noted there
it's quite a bit more complex when we have to deal with the atexit
sibling function.

Ævar Arnfjörð Bjarmason (5):
  pager: refactor wait_for_pager() function
  pager: test for exit code with and without SIGPIPE
  run-command: add braces for "if" block in wait_or_whine()
  pager: properly log pager exit code when signalled
  WIP pager: respect exit code of pager over SIGPIPE

 pager.c          |  24 +++++----
 run-command.c    |   7 ++-
 t/t7006-pager.sh | 130 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 148 insertions(+), 13 deletions(-)

Range-diff:
2:  6509ae44751 = 1:  aab89cc8619 pager: refactor wait_for_pager() function
1:  cba284dcf55 ! 2:  edf513bb174 pager: test for exit code
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    pager: test for exit code
    +    pager: test for exit code with and without SIGPIPE
     
         Add tests for how git behaves when the pager itself exits with
         non-zero, as well as for us exiting with 141 when we're killed with
    @@ Commit message
         current behavior.
     
         This test construct is stolen from 7559a1be8a0 (unblock and unignore
    -    SIGPIPE, 2014-09-18).
    +    SIGPIPE, 2014-09-18). The reason not to make the test itself depend on
    +    the MINGW prerequisite is to make a subsequent commit easier to read.
     
         1. https://lore.kernel.org/git/87o8h4omqa.fsf@evledraar.gmail.com/
     
    @@ t/t7006-pager.sh: test_expect_success TTY 'git tag with auto-columns ' '
      	test_cmp expect actual
      '
      
    -+test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager exit' '
    ++test_expect_success TTY 'git returns SIGPIPE on early pager exit' '
     +	test_when_finished "rm pager-used" &&
     +	test_config core.pager ">pager-used; head -n 1; exit 0" &&
     +
    -+	OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
    -+	test_match_signal 13 "$OUT" &&
    ++	if test_have_prereq !MINGW
    ++	then
    ++		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
    ++		test_match_signal 13 "$OUT"
    ++	else
    ++		test_terminal git log
    ++	fi &&
     +	test_path_is_file pager-used
     +'
     +
    -+test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager non-zero exit' '
    ++test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '
     +	test_when_finished "rm pager-used" &&
     +	test_config core.pager ">pager-used; head -n 1; exit 1" &&
     +
    -+	OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
    -+	test_match_signal 13 "$OUT" &&
    ++	if test_have_prereq !MINGW
    ++	then
    ++		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
    ++		test_match_signal 13 "$OUT"
    ++	else
    ++		test_terminal git log
    ++	fi &&
     +	test_path_is_file pager-used
     +'
     +
    -+test_expect_success TTY,!MINGW 'git discards pager non-zero exit' '
    ++test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '
     +	test_when_finished "rm pager-used" &&
     +	test_config core.pager "wc >pager-used; exit 1" &&
     +
    -+	OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
    -+	test "$OUT" -eq 0 &&
    ++	if test_have_prereq !MINGW
    ++	then
    ++		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
    ++		test "$OUT" -eq 0
    ++	else
    ++		test_terminal git log
    ++	fi &&
    ++	test_path_is_file pager-used
    ++'
    ++
    ++test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '
    ++	test_when_finished "rm pager-used" &&
    ++	test_config core.pager "wc >pager-used; does-not-exist" &&
    ++
    ++	if test_have_prereq !MINGW
    ++	then
    ++		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
    ++		test "$OUT" -eq 0
    ++	else
    ++		test_terminal git log
    ++	fi &&
    ++	test_path_is_file pager-used
    ++'
    ++
    ++test_expect_success TTY 'git attempts to page to nonexisting pager command, gets SIGPIPE' '
    ++	test_config core.pager "does-not-exist" &&
    ++
    ++	if test_have_prereq !MINGW
    ++	then
    ++		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
    ++		test_match_signal 13 "$OUT"
    ++	else
    ++		test_terminal git log
    ++	fi
    ++'
    ++
    ++test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '
    ++	test_when_finished "rm pager-used" &&
    ++	test_config core.pager ">pager-used; test-tool sigchain" &&
    ++
    ++	if test_have_prereq !MINGW
    ++	then
    ++		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
    ++		test_match_signal 13 "$OUT"
    ++	else
    ++		test_terminal git log
    ++	fi &&
     +	test_path_is_file pager-used
     +'
     +
-:  ----------- > 3:  0e4cbf80fe1 run-command: add braces for "if" block in wait_or_whine()
3:  d5db936bd11 ! 4:  527f69cf581 pager: properly log pager exit code when signalled
    @@ Commit message
         The wait_or_whine() is only called with a true "in_signal" from from
         finish_command_in_signal(), which in turn is only used in pager.c.
     
    -    I'm not quite sure about that BUG() case. Can we have a true in_signal
    -    and not have a true WIFEXITED(status)? I haven't been able to think of
    -    a test case for it.
    +    The "in_signal && !WIFEXITED(status)" case is not covered by
    +    tests. Let's log the default -1 in that case for good measure.
     
         1. The incorrect logging of the exit code in was seemingly copy/pasted
            into finish_command_in_signal() in ee4512ed481 (trace2: create new
    @@ Commit message
     
      ## run-command.c ##
     @@ run-command.c: static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
    - 
      	while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
      		;	/* nothing */
    --	if (in_signal)
    + 	if (in_signal) {
     -		return 0;
    -+	if (in_signal && WIFEXITED(status))
    -+		return WEXITSTATUS(status);
    -+	if (in_signal) {
    -+		BUG("was not expecting waitpid() status %d", status);
    -+		return -1;
    -+	}
    ++		if (WIFEXITED(status))
    ++			code = WEXITSTATUS(status);
    ++		return code;
    + 	}
      
      	if (waiting < 0) {
    - 		failed_errno = errno;
     
      ## t/t7006-pager.sh ##
     @@ t/t7006-pager.sh: test_expect_success TTY 'git tag with auto-columns ' '
      	test_cmp expect actual
      '
      
    --test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager exit' '
    -+test_expect_success TTY 'git returns SIGPIPE on early pager exit' '
    - 	test_when_finished "rm pager-used" &&
    ++test_expect_success 'setup trace2' '
    ++	GIT_TRACE2_BRIEF=1 &&
    ++	export GIT_TRACE2_BRIEF
    ++'
    ++
    + test_expect_success TTY 'git returns SIGPIPE on early pager exit' '
    +-	test_when_finished "rm pager-used" &&
    ++	test_when_finished "rm pager-used trace.normal" &&
      	test_config core.pager ">pager-used; head -n 1; exit 0" &&
     +	GIT_TRACE2="$(pwd)/trace.normal" &&
     +	export GIT_TRACE2 &&
     +	test_when_finished "unset GIT_TRACE2" &&
      
    --	OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
    --	test_match_signal 13 "$OUT" &&
    -+	if test_have_prereq !MINGW
    -+	then
    -+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
    -+		test_match_signal 13 "$OUT"
    -+	else
    -+		test_terminal git log
    -+	fi &&
    -+	grep "child_exit.* code:0 " trace.normal &&
    + 	if test_have_prereq !MINGW
    + 	then
    +@@ t/t7006-pager.sh: test_expect_success TTY 'git returns SIGPIPE on early pager exit' '
    + 	else
    + 		test_terminal git log
    + 	fi &&
    ++
    ++	grep child_exit trace.normal >child-exits &&
    ++	test_line_count = 1 child-exits &&
    ++	grep " code:0 " child-exits &&
      	test_path_is_file pager-used
      '
      
    --test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager non-zero exit' '
    -+test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '
    - 	test_when_finished "rm pager-used" &&
    + test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '
    +-	test_when_finished "rm pager-used" &&
    ++	test_when_finished "rm pager-used trace.normal" &&
      	test_config core.pager ">pager-used; head -n 1; exit 1" &&
     +	GIT_TRACE2="$(pwd)/trace.normal" &&
     +	export GIT_TRACE2 &&
     +	test_when_finished "unset GIT_TRACE2" &&
      
    --	OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
    --	test_match_signal 13 "$OUT" &&
    -+	if test_have_prereq !MINGW
    -+	then
    -+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
    -+		test_match_signal 13 "$OUT"
    -+	else
    -+		test_terminal git log
    -+	fi &&
    -+	grep "child_exit.* code:1 " trace.normal &&
    + 	if test_have_prereq !MINGW
    + 	then
    +@@ t/t7006-pager.sh: test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '
    + 	else
    + 		test_terminal git log
    + 	fi &&
    ++
    ++	grep child_exit trace.normal >child-exits &&
    ++	test_line_count = 1 child-exits &&
    ++	grep " code:1 " child-exits &&
      	test_path_is_file pager-used
      '
      
    --test_expect_success TTY,!MINGW 'git discards pager non-zero exit' '
    -+test_expect_success TTY 'git discards pager non-zero exit' '
    - 	test_when_finished "rm pager-used" &&
    + test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '
    +-	test_when_finished "rm pager-used" &&
    ++	test_when_finished "rm pager-used trace.normal" &&
      	test_config core.pager "wc >pager-used; exit 1" &&
     +	GIT_TRACE2="$(pwd)/trace.normal" &&
     +	export GIT_TRACE2 &&
     +	test_when_finished "unset GIT_TRACE2" &&
      
    --	OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
    --	test "$OUT" -eq 0 &&
    -+	if test_have_prereq !MINGW
    -+	then
    -+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
    -+		test "$OUT" -eq 0
    -+	else
    -+		test_terminal git log
    -+	fi &&
    -+	grep "child_exit.* code:1 " trace.normal &&
    + 	if test_have_prereq !MINGW
    + 	then
    +@@ t/t7006-pager.sh: test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '
    + 	else
    + 		test_terminal git log
    + 	fi &&
    ++
    ++	grep child_exit trace.normal >child-exits &&
    ++	test_line_count = 1 child-exits &&
    ++	grep " code:1 " child-exits &&
      	test_path_is_file pager-used
      '
      
    -+test_expect_success TTY 'git logs nonexisting pager invocation' '
    -+	test_config core.pager "does-not-exist" &&
    + test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '
    +-	test_when_finished "rm pager-used" &&
    ++	test_when_finished "rm pager-used trace.normal" &&
    + 	test_config core.pager "wc >pager-used; does-not-exist" &&
     +	GIT_TRACE2="$(pwd)/trace.normal" &&
     +	export GIT_TRACE2 &&
     +	test_when_finished "unset GIT_TRACE2" &&
    + 
    + 	if test_have_prereq !MINGW
    + 	then
    +@@ t/t7006-pager.sh: test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '
    + 	else
    + 		test_terminal git log
    + 	fi &&
     +
    -+	if test_have_prereq !MINGW
    -+	then
    -+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
    -+		test_match_signal 13 "$OUT"
    -+	else
    -+		test_terminal git log
    ++	grep child_exit trace.normal >child-exits &&
    ++	test_line_count = 1 child-exits &&
    ++	grep " code:127 " child-exits &&
    + 	test_path_is_file pager-used
    + '
    + 
    + test_expect_success TTY 'git attempts to page to nonexisting pager command, gets SIGPIPE' '
    ++	test_when_finished "rm trace.normal" &&
    + 	test_config core.pager "does-not-exist" &&
    ++	GIT_TRACE2="$(pwd)/trace.normal" &&
    ++	export GIT_TRACE2 &&
    ++	test_when_finished "unset GIT_TRACE2" &&
    + 
    + 	if test_have_prereq !MINGW
    + 	then
    +@@ t/t7006-pager.sh: test_expect_success TTY 'git attempts to page to nonexisting pager command, gets
    + 		test_match_signal 13 "$OUT"
    + 	else
    + 		test_terminal git log
    +-	fi
     +	fi &&
    -+	grep "child_exit.* code:-1 " trace.normal
    -+'
     +
    - test_done
    ++	grep child_exit trace.normal >child-exits &&
    ++	test_line_count = 1 child-exits &&
    ++	grep " code:-1 " child-exits
    + '
    + 
    + test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '
    +-	test_when_finished "rm pager-used" &&
    ++	test_when_finished "rm pager-used trace.normal" &&
    + 	test_config core.pager ">pager-used; test-tool sigchain" &&
    ++	GIT_TRACE2="$(pwd)/trace.normal" &&
    ++	export GIT_TRACE2 &&
    ++	test_when_finished "unset GIT_TRACE2" &&
    + 
    + 	if test_have_prereq !MINGW
    + 	then
    +@@ t/t7006-pager.sh: test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '
    + 	else
    + 		test_terminal git log
    + 	fi &&
    ++
    ++	grep child_exit trace.normal >child-exits &&
    ++	test_line_count = 1 child-exits &&
    ++	grep " code:143 " child-exits &&
    + 	test_path_is_file pager-used
    + '
    + 
-:  ----------- > 5:  842f42340d0 WIP pager: respect exit code of pager over SIGPIPE
-- 
2.30.0.284.gd98b1dd5eaa7

[PATCH v2 1/5] pager: refactor wait_for_pager() function

[Ævar Arnfjörð Bjarmason]

Refactor the wait_for_pager() function. Since 507d7804c0b (pager:
don't use unsafe functions in signal handlers, 2015-09-04) the
wait_for_pager() and wait_for_pager_atexit() callers diverged on more
than they shared.

Let's extract the common code into a new close_pager_fds() helper, and
move the parts unique to the only to callers to those functions.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 pager.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/pager.c b/pager.c
index ee435de6756..3d37dd7adaa 100644
--- a/pager.c
+++ b/pager.c
@@ -11,29 +11,25 @@
 static struct child_process pager_process = CHILD_PROCESS_INIT;
 static const char *pager_program;
 
-static void wait_for_pager(int in_signal)
+static void close_pager_fds(void)
 {
-	if (!in_signal) {
-		fflush(stdout);
-		fflush(stderr);
-	}
 	/* signal EOF to pager */
 	close(1);
 	close(2);
-	if (in_signal)
-		finish_command_in_signal(&pager_process);
-	else
-		finish_command(&pager_process);
 }
 
 static void wait_for_pager_atexit(void)
 {
-	wait_for_pager(0);
+	fflush(stdout);
+	fflush(stderr);
+	close_pager_fds();
+	finish_command(&pager_process);
 }
 
 static void wait_for_pager_signal(int signo)
 {
-	wait_for_pager(1);
+	close_pager_fds();
+	finish_command_in_signal(&pager_process);
 	sigchain_pop(signo);
 	raise(signo);
 }
-- 
2.30.0.284.gd98b1dd5eaa7

[PATCH v2 2/5] pager: test for exit code with and without SIGPIPE

[Ævar Arnfjörð Bjarmason]

Add tests for how git behaves when the pager itself exits with
non-zero, as well as for us exiting with 141 when we're killed with
SIGPIPE due to the pager not consuming its output.

There is some recent discussion[1] about these semantics, but aside
from what we want to do in the future, we should have a test for the
current behavior.

This test construct is stolen from 7559a1be8a0 (unblock and unignore
SIGPIPE, 2014-09-18). The reason not to make the test itself depend on
the MINGW prerequisite is to make a subsequent commit easier to read.

1. https://lore.kernel.org/git/87o8h4omqa.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7006-pager.sh | 82 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index fdb450e446a..0aa030962b1 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -656,4 +656,86 @@ test_expect_success TTY 'git tag with auto-columns ' '
 	test_cmp expect actual
 '
 
+test_expect_success TTY 'git returns SIGPIPE on early pager exit' '
+	test_when_finished "rm pager-used" &&
+	test_config core.pager ">pager-used; head -n 1; exit 0" &&
+
+	if test_have_prereq !MINGW
+	then
+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+		test_match_signal 13 "$OUT"
+	else
+		test_terminal git log
+	fi &&
+	test_path_is_file pager-used
+'
+
+test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '
+	test_when_finished "rm pager-used" &&
+	test_config core.pager ">pager-used; head -n 1; exit 1" &&
+
+	if test_have_prereq !MINGW
+	then
+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+		test_match_signal 13 "$OUT"
+	else
+		test_terminal git log
+	fi &&
+	test_path_is_file pager-used
+'
+
+test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '
+	test_when_finished "rm pager-used" &&
+	test_config core.pager "wc >pager-used; exit 1" &&
+
+	if test_have_prereq !MINGW
+	then
+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+		test "$OUT" -eq 0
+	else
+		test_terminal git log
+	fi &&
+	test_path_is_file pager-used
+'
+
+test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '
+	test_when_finished "rm pager-used" &&
+	test_config core.pager "wc >pager-used; does-not-exist" &&
+
+	if test_have_prereq !MINGW
+	then
+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+		test "$OUT" -eq 0
+	else
+		test_terminal git log
+	fi &&
+	test_path_is_file pager-used
+'
+
+test_expect_success TTY 'git attempts to page to nonexisting pager command, gets SIGPIPE' '
+	test_config core.pager "does-not-exist" &&
+
+	if test_have_prereq !MINGW
+	then
+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+		test_match_signal 13 "$OUT"
+	else
+		test_terminal git log
+	fi
+'
+
+test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '
+	test_when_finished "rm pager-used" &&
+	test_config core.pager ">pager-used; test-tool sigchain" &&
+
+	if test_have_prereq !MINGW
+	then
+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+		test_match_signal 13 "$OUT"
+	else
+		test_terminal git log
+	fi &&
+	test_path_is_file pager-used
+'
+
 test_done
-- 
2.30.0.284.gd98b1dd5eaa7

Re: [PATCH v2 2/5] pager: test for exit code with and without SIGPIPE

[Denton Liu]

On Tue, Feb 02, 2021 at 02:59:58AM +0100, Ævar Arnfjörð Bjarmason wrote:
> This test construct is stolen from 7559a1be8a0 (unblock and unignore
> SIGPIPE, 2014-09-18). The reason not to make the test itself depend on
> the MINGW prerequisite is to make a subsequent commit easier to read.

[...]

> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index fdb450e446a..0aa030962b1 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -656,4 +656,86 @@ test_expect_success TTY 'git tag with auto-columns ' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success TTY 'git returns SIGPIPE on early pager exit' '
> +	test_when_finished "rm pager-used" &&
> +	test_config core.pager ">pager-used; head -n 1; exit 0" &&

I may be missing something but this code seems racy, especially since
the history is relatively short at this point. It seems like it's
plausible for git log to be able to dump its output entirely before the
pager part even runs. In that case, it'd fail due to success being its
exit code since it wouldn't be killed by SIGPIPE. 

This is what my `test-tool pager` approach was hoping to prevent since
that would guarantee a SIGPIPE.

Sidenote, going back to 7559a1be8a0 (unblock and unignore SIGPIPE,
2014-09-18), it seems like those tests are also racy since it's
theoretically possible for all of the output to be produced before the
piped command gets to it. However, in that case, they're producing a
huge amount of output so this raciness seems mostly academic.

Thanks,
Denton

> +
> +	if test_have_prereq !MINGW
> +	then
> +		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
> +		test_match_signal 13 "$OUT"
> +	else
> +		test_terminal git log
> +	fi &&
> +	test_path_is_file pager-used
> +'

Re: [PATCH v2 2/5] pager: test for exit code with and without SIGPIPE

[Johannes Sixt]

Am 02.02.21 um 02:59 schrieb Ævar Arnfjörð Bjarmason:
> Add tests for how git behaves when the pager itself exits with
> non-zero, as well as for us exiting with 141 when we're killed with
> SIGPIPE due to the pager not consuming its output.
> 
> There is some recent discussion[1] about these semantics, but aside
> from what we want to do in the future, we should have a test for the
> current behavior.
> 
> This test construct is stolen from 7559a1be8a0 (unblock and unignore
> SIGPIPE, 2014-09-18). The reason not to make the test itself depend on
> the MINGW prerequisite is to make a subsequent commit easier to read.

At least for my Windows build, the MINGW games do not make a difference:
The test is skipped anyway due to the unsatisfied TTY prerequisite.

> 
> 1. https://lore.kernel.org/git/87o8h4omqa.fsf@evledraar.gmail.com/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t7006-pager.sh | 82 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index fdb450e446a..0aa030962b1 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -656,4 +656,86 @@ test_expect_success TTY 'git tag with auto-columns ' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success TTY 'git returns SIGPIPE on early pager exit' '
> +	test_when_finished "rm pager-used" &&
> +	test_config core.pager ">pager-used; head -n 1; exit 0" &&
> +
> +	if test_have_prereq !MINGW
> +	then
> +		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
> +		test_match_signal 13 "$OUT"
> +	else
> +		test_terminal git log
> +	fi &&
> +	test_path_is_file pager-used
> +'
> +
> +test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '
> +	test_when_finished "rm pager-used" &&
> +	test_config core.pager ">pager-used; head -n 1; exit 1" &&
> +
> +	if test_have_prereq !MINGW
> +	then
> +		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
> +		test_match_signal 13 "$OUT"
> +	else
> +		test_terminal git log
> +	fi &&
> +	test_path_is_file pager-used
> +'
> +
> +test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '
> +	test_when_finished "rm pager-used" &&
> +	test_config core.pager "wc >pager-used; exit 1" &&
> +
> +	if test_have_prereq !MINGW
> +	then
> +		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
> +		test "$OUT" -eq 0
> +	else
> +		test_terminal git log
> +	fi &&
> +	test_path_is_file pager-used
> +'
> +
> +test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '
> +	test_when_finished "rm pager-used" &&
> +	test_config core.pager "wc >pager-used; does-not-exist" &&
> +
> +	if test_have_prereq !MINGW
> +	then
> +		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
> +		test "$OUT" -eq 0
> +	else
> +		test_terminal git log
> +	fi &&
> +	test_path_is_file pager-used
> +'
> +
> +test_expect_success TTY 'git attempts to page to nonexisting pager command, gets SIGPIPE' '
> +	test_config core.pager "does-not-exist" &&
> +
> +	if test_have_prereq !MINGW
> +	then
> +		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
> +		test_match_signal 13 "$OUT"
> +	else
> +		test_terminal git log
> +	fi
> +'
> +
> +test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '
> +	test_when_finished "rm pager-used" &&
> +	test_config core.pager ">pager-used; test-tool sigchain" &&
> +
> +	if test_have_prereq !MINGW
> +	then
> +		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
> +		test_match_signal 13 "$OUT"
> +	else
> +		test_terminal git log
> +	fi &&
> +	test_path_is_file pager-used
> +'
> +
>  test_done
> 

[PATCH v2 3/5] run-command: add braces for "if" block in wait_or_whine()

[Ævar Arnfjörð Bjarmason]

Add braces to an "if" block in the wait_or_whine() function. This
isn't needed now, but will make a subsequent commit easier to read.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index ea4d0fb4b15..00e68f37aba 100644
--- a/run-command.c
+++ b/run-command.c
@@ -551,8 +551,9 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
 
 	while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
 		;	/* nothing */
-	if (in_signal)
+	if (in_signal) {
 		return 0;
+	}
 
 	if (waiting < 0) {
 		failed_errno = errno;
-- 
2.30.0.284.gd98b1dd5eaa7

[PATCH v2 4/5] pager: properly log pager exit code when signalled

[Ævar Arnfjörð Bjarmason]

When git invokes a pager that exits with non-zero the common case is
that we'll already return the correct SIGPIPE failure from git itself,
but the exit code logged in trace2 has always been incorrectly
reported[1]. Fix that and log the correct exit code in the logs.

Since this gives us something to test outside of our recently-added
tests needing a !MINGW prerequisite, let's refactor the test to run on
MINGW and actually check for SIGPIPE outside of MINGW.

The wait_or_whine() is only called with a true "in_signal" from from
finish_command_in_signal(), which in turn is only used in pager.c.

The "in_signal && !WIFEXITED(status)" case is not covered by
tests. Let's log the default -1 in that case for good measure.

1. The incorrect logging of the exit code in was seemingly copy/pasted
   into finish_command_in_signal() in ee4512ed481 (trace2: create new
   combined trace facility, 2019-02-22)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c    |  4 +++-
 t/t7006-pager.sh | 60 +++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/run-command.c b/run-command.c
index 00e68f37aba..509841bf273 100644
--- a/run-command.c
+++ b/run-command.c
@@ -552,7 +552,9 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
 	while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
 		;	/* nothing */
 	if (in_signal) {
-		return 0;
+		if (WIFEXITED(status))
+			code = WEXITSTATUS(status);
+		return code;
 	}
 
 	if (waiting < 0) {
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 0aa030962b1..0e7cf75435e 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -656,9 +656,17 @@ test_expect_success TTY 'git tag with auto-columns ' '
 	test_cmp expect actual
 '
 
+test_expect_success 'setup trace2' '
+	GIT_TRACE2_BRIEF=1 &&
+	export GIT_TRACE2_BRIEF
+'
+
 test_expect_success TTY 'git returns SIGPIPE on early pager exit' '
-	test_when_finished "rm pager-used" &&
+	test_when_finished "rm pager-used trace.normal" &&
 	test_config core.pager ">pager-used; head -n 1; exit 0" &&
+	GIT_TRACE2="$(pwd)/trace.normal" &&
+	export GIT_TRACE2 &&
+	test_when_finished "unset GIT_TRACE2" &&
 
 	if test_have_prereq !MINGW
 	then
@@ -667,12 +675,19 @@ test_expect_success TTY 'git returns SIGPIPE on early pager exit' '
 	else
 		test_terminal git log
 	fi &&
+
+	grep child_exit trace.normal >child-exits &&
+	test_line_count = 1 child-exits &&
+	grep " code:0 " child-exits &&
 	test_path_is_file pager-used
 '
 
 test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '
-	test_when_finished "rm pager-used" &&
+	test_when_finished "rm pager-used trace.normal" &&
 	test_config core.pager ">pager-used; head -n 1; exit 1" &&
+	GIT_TRACE2="$(pwd)/trace.normal" &&
+	export GIT_TRACE2 &&
+	test_when_finished "unset GIT_TRACE2" &&
 
 	if test_have_prereq !MINGW
 	then
@@ -681,12 +696,19 @@ test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '
 	else
 		test_terminal git log
 	fi &&
+
+	grep child_exit trace.normal >child-exits &&
+	test_line_count = 1 child-exits &&
+	grep " code:1 " child-exits &&
 	test_path_is_file pager-used
 '
 
 test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '
-	test_when_finished "rm pager-used" &&
+	test_when_finished "rm pager-used trace.normal" &&
 	test_config core.pager "wc >pager-used; exit 1" &&
+	GIT_TRACE2="$(pwd)/trace.normal" &&
+	export GIT_TRACE2 &&
+	test_when_finished "unset GIT_TRACE2" &&
 
 	if test_have_prereq !MINGW
 	then
@@ -695,12 +717,19 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '
 	else
 		test_terminal git log
 	fi &&
+
+	grep child_exit trace.normal >child-exits &&
+	test_line_count = 1 child-exits &&
+	grep " code:1 " child-exits &&
 	test_path_is_file pager-used
 '
 
 test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '
-	test_when_finished "rm pager-used" &&
+	test_when_finished "rm pager-used trace.normal" &&
 	test_config core.pager "wc >pager-used; does-not-exist" &&
+	GIT_TRACE2="$(pwd)/trace.normal" &&
+	export GIT_TRACE2 &&
+	test_when_finished "unset GIT_TRACE2" &&
 
 	if test_have_prereq !MINGW
 	then
@@ -709,11 +738,19 @@ test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '
 	else
 		test_terminal git log
 	fi &&
+
+	grep child_exit trace.normal >child-exits &&
+	test_line_count = 1 child-exits &&
+	grep " code:127 " child-exits &&
 	test_path_is_file pager-used
 '
 
 test_expect_success TTY 'git attempts to page to nonexisting pager command, gets SIGPIPE' '
+	test_when_finished "rm trace.normal" &&
 	test_config core.pager "does-not-exist" &&
+	GIT_TRACE2="$(pwd)/trace.normal" &&
+	export GIT_TRACE2 &&
+	test_when_finished "unset GIT_TRACE2" &&
 
 	if test_have_prereq !MINGW
 	then
@@ -721,12 +758,19 @@ test_expect_success TTY 'git attempts to page to nonexisting pager command, gets
 		test_match_signal 13 "$OUT"
 	else
 		test_terminal git log
-	fi
+	fi &&
+
+	grep child_exit trace.normal >child-exits &&
+	test_line_count = 1 child-exits &&
+	grep " code:-1 " child-exits
 '
 
 test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '
-	test_when_finished "rm pager-used" &&
+	test_when_finished "rm pager-used trace.normal" &&
 	test_config core.pager ">pager-used; test-tool sigchain" &&
+	GIT_TRACE2="$(pwd)/trace.normal" &&
+	export GIT_TRACE2 &&
+	test_when_finished "unset GIT_TRACE2" &&
 
 	if test_have_prereq !MINGW
 	then
@@ -735,6 +779,10 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '
 	else
 		test_terminal git log
 	fi &&
+
+	grep child_exit trace.normal >child-exits &&
+	test_line_count = 1 child-exits &&
+	grep " code:143 " child-exits &&
 	test_path_is_file pager-used
 '
 
-- 
2.30.0.284.gd98b1dd5eaa7

Re: [PATCH v2 4/5] pager: properly log pager exit code when signalled

[Johannes Sixt]

Am 02.02.21 um 03:00 schrieb Ævar Arnfjörð Bjarmason:
> When git invokes a pager that exits with non-zero the common case is
> that we'll already return the correct SIGPIPE failure from git itself,
> but the exit code logged in trace2 has always been incorrectly
> reported[1]. Fix that and log the correct exit code in the logs.

There's a more severe problem here, not with your patch, but with trace2
in general: it invokes async-signal-unsafe functions from a signal
handler, in particular, realloc, vsnprintf, gettimeofday, localtime_r
(and probably a lot more) via fn_child_exit_fl of trace2/tr2_tgt_normal.c

Is that something that we should care about?

-- Hannes

Re: [PATCH v2 4/5] pager: properly log pager exit code when signalled

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 02.02.21 um 03:00 schrieb Ævar Arnfjörð Bjarmason:
>> When git invokes a pager that exits with non-zero the common case is
>> that we'll already return the correct SIGPIPE failure from git itself,
>> but the exit code logged in trace2 has always been incorrectly
>> reported[1]. Fix that and log the correct exit code in the logs.
>
> There's a more severe problem here, not with your patch, but with trace2
> in general: it invokes async-signal-unsafe functions from a signal
> handler, in particular, realloc, vsnprintf, gettimeofday, localtime_r
> (and probably a lot more) via fn_child_exit_fl of trace2/tr2_tgt_normal.c
>
> Is that something that we should care about?

Yes, indeed X-<.

[WIP/PATCH v2 5/5] WIP pager: respect exit code of pager over SIGPIPE

[Ævar Arnfjörð Bjarmason]

As discussed on-list starting with [1] I don't think this patch makes
sense, but this "passes tests", at least on Debian with glibc, and is
food for thought for those who like the approach of git not
propagating the pager-induced SIGPIPE in git's own exit code.

The exit() here in wait_for_pager_atexit() isn't portable though[2],
we could probably use _exit(1) instead, but then we're going to
abruptly put a stop to further atexit handler processing. We're far
from the only one, tempfile.c, run-command.c, gc.c etc. all rely on
it, and that's just the git.git code.

If we drop the "if (code)" condition we can see that our pager exit
code will override the exit code of other commands in t7006-pager.sh,
causing numerous tests to fail. Of course if we don't do that all
tests pass.

But that experiment suggests regressions introduced here that we just
don't have good test coverage for. I.e. we're running code before the
atexit() here which expects to exit() with a given status code, and
we're clobbering it with ours because the pager also happened to fail
as we were exiting.

So a real implementation of this would, I think, have to at least:

 A. Refactor all use of atexit() to use some git-specific registry,
    hard assert somehow that we're never going to have atexit() by
    anything else (a library we use might call it).

 B. Because we used some atexit() wrapper API we'd know if we were in
    the last atexit() handler, which would need to re-evaluate the
    decision about the "real" exit code.

 C. We could not call exit() anywhere, but would have to make a
    git_exit() wrapper. We'd then assign the desired exit code to a
    global variable, and then only override our "real" non-zero exit
    code with the pager's non-zero, in cases where the pager also
    failed.

 D. I haven't found whether calling _exit() in the atexit() handler
    even has defined behavior, but in any case using it would
    short-circuit the documented program exit behavior defined in the
    C standard, of which calling atexit() handlers is just the first
    step.

1. https://lore.kernel.org/git/8735yhq3lc.fsf@evledraar.gmail.com/
2. https://pubs.opengroup.org/onlinepubs/009695399/functions/exit.html

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 pager.c          | 10 ++++++++--
 t/t7006-pager.sh |  8 ++++----
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/pager.c b/pager.c
index 3d37dd7adaa..2e743bc0b1e 100644
--- a/pager.c
+++ b/pager.c
@@ -20,18 +20,24 @@ static void close_pager_fds(void)
 
 static void wait_for_pager_atexit(void)
 {
+	int code;
 	fflush(stdout);
 	fflush(stderr);
 	close_pager_fds();
-	finish_command(&pager_process);
+	code = finish_command(&pager_process);
+	if (code)
+		exit(code);
 }
 
 static void wait_for_pager_signal(int signo)
 {
+	int code;
 	close_pager_fds();
-	finish_command_in_signal(&pager_process);
+	code = finish_command_in_signal(&pager_process);
 	sigchain_pop(signo);
 	raise(signo);
+	if (signo == SIGPIPE)
+		exit(code);
 }
 
 static int core_pager_config(const char *var, const char *value, void *data)
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 0e7cf75435e..69997fa48f2 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -703,7 +703,7 @@ test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '
 	test_path_is_file pager-used
 '
 
-test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '
+test_expect_success TTY 'git respects pager non-zero exit without SIGPIPE' '
 	test_when_finished "rm pager-used trace.normal" &&
 	test_config core.pager "wc >pager-used; exit 1" &&
 	GIT_TRACE2="$(pwd)/trace.normal" &&
@@ -713,7 +713,7 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '
 	if test_have_prereq !MINGW
 	then
 		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
-		test "$OUT" -eq 0
+		test "$OUT" -eq 1
 	else
 		test_terminal git log
 	fi &&
@@ -724,7 +724,7 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '
 	test_path_is_file pager-used
 '
 
-test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '
+test_expect_success TTY 'git respects nonexisting pager without SIGPIPE' '
 	test_when_finished "rm pager-used trace.normal" &&
 	test_config core.pager "wc >pager-used; does-not-exist" &&
 	GIT_TRACE2="$(pwd)/trace.normal" &&
@@ -734,7 +734,7 @@ test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '
 	if test_have_prereq !MINGW
 	then
 		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
-		test "$OUT" -eq 0
+		test "$OUT" -eq 127
 	else
 		test_terminal git log
 	fi &&
-- 
2.30.0.284.gd98b1dd5eaa7

[PATCH 1/3] pager: test for exit code

[Ævar Arnfjörð Bjarmason]

Add tests for how git behaves when the pager itself exits with
non-zero, as well as for us exiting with 141 when we're killed with
SIGPIPE due to the pager not consuming its output.

There is some recent discussion[1] about these semantics, but aside
from what we want to do in the future, we should have a test for the
current behavior.

This test construct is stolen from 7559a1be8a0 (unblock and unignore
SIGPIPE, 2014-09-18).

1. https://lore.kernel.org/git/87o8h4omqa.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7006-pager.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index fdb450e446a..c60886f43e6 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -656,4 +656,31 @@ test_expect_success TTY 'git tag with auto-columns ' '
 	test_cmp expect actual
 '
 
+test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager exit' '
+	test_when_finished "rm pager-used" &&
+	test_config core.pager ">pager-used; head -n 1; exit 0" &&
+
+	OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+	test_match_signal 13 "$OUT" &&
+	test_path_is_file pager-used
+'
+
+test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager non-zero exit' '
+	test_when_finished "rm pager-used" &&
+	test_config core.pager ">pager-used; head -n 1; exit 1" &&
+
+	OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+	test_match_signal 13 "$OUT" &&
+	test_path_is_file pager-used
+'
+
+test_expect_success TTY,!MINGW 'git discards pager non-zero exit' '
+	test_when_finished "rm pager-used" &&
+	test_config core.pager "wc >pager-used; exit 1" &&
+
+	OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+	test "$OUT" -eq 0 &&
+	test_path_is_file pager-used
+'
+
 test_done
-- 
2.30.0.284.gd98b1dd5eaa7

[PATCH 2/3] pager: refactor wait_for_pager() function

[Ævar Arnfjörð Bjarmason]

Refactor the wait_for_pager() function. Since 507d7804c0b (pager:
don't use unsafe functions in signal handlers, 2015-09-04) the
wait_for_pager() and wait_for_pager_atexit() callers diverged on more
than they shared.

Let's extract the common code into a new close_pager_fds() helper, and
move the parts unique to the only to callers to those functions.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 pager.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/pager.c b/pager.c
index ee435de6756..3d37dd7adaa 100644
--- a/pager.c
+++ b/pager.c
@@ -11,29 +11,25 @@
 static struct child_process pager_process = CHILD_PROCESS_INIT;
 static const char *pager_program;
 
-static void wait_for_pager(int in_signal)
+static void close_pager_fds(void)
 {
-	if (!in_signal) {
-		fflush(stdout);
-		fflush(stderr);
-	}
 	/* signal EOF to pager */
 	close(1);
 	close(2);
-	if (in_signal)
-		finish_command_in_signal(&pager_process);
-	else
-		finish_command(&pager_process);
 }
 
 static void wait_for_pager_atexit(void)
 {
-	wait_for_pager(0);
+	fflush(stdout);
+	fflush(stderr);
+	close_pager_fds();
+	finish_command(&pager_process);
 }
 
 static void wait_for_pager_signal(int signo)
 {
-	wait_for_pager(1);
+	close_pager_fds();
+	finish_command_in_signal(&pager_process);
 	sigchain_pop(signo);
 	raise(signo);
 }
-- 
2.30.0.284.gd98b1dd5eaa7

[PATCH 3/3] pager: properly log pager exit code when signalled

[Ævar Arnfjörð Bjarmason]

When git invokes a pager that exits with non-zero the common case is
that we'll already return the correct SIGPIPE failure from git itself,
but the exit code logged in trace2 has always been incorrectly
reported[1]. Fix that and log the correct exit code in the logs.

Since this gives us something to test outside of our recently-added
tests needing a !MINGW prerequisite, let's refactor the test to run on
MINGW and actually check for SIGPIPE outside of MINGW.

The wait_or_whine() is only called with a true "in_signal" from from
finish_command_in_signal(), which in turn is only used in pager.c.

I'm not quite sure about that BUG() case. Can we have a true in_signal
and not have a true WIFEXITED(status)? I haven't been able to think of
a test case for it.

1. The incorrect logging of the exit code in was seemingly copy/pasted
   into finish_command_in_signal() in ee4512ed481 (trace2: create new
   combined trace facility, 2019-02-22)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c    |  8 +++++--
 t/t7006-pager.sh | 61 +++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 58 insertions(+), 11 deletions(-)

diff --git a/run-command.c b/run-command.c
index ea4d0fb4b15..10e1c96c2bd 100644
--- a/run-command.c
+++ b/run-command.c
@@ -551,8 +551,12 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
 
 	while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
 		;	/* nothing */
-	if (in_signal)
-		return 0;
+	if (in_signal && WIFEXITED(status))
+		return WEXITSTATUS(status);
+	if (in_signal) {
+		BUG("was not expecting waitpid() status %d", status);
+		return -1;
+	}
 
 	if (waiting < 0) {
 		failed_errno = errno;
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index c60886f43e6..1424466caf5 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -656,31 +656,74 @@ test_expect_success TTY 'git tag with auto-columns ' '
 	test_cmp expect actual
 '
 
-test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager exit' '
+test_expect_success TTY 'git returns SIGPIPE on early pager exit' '
 	test_when_finished "rm pager-used" &&
 	test_config core.pager ">pager-used; head -n 1; exit 0" &&
+	GIT_TRACE2="$(pwd)/trace.normal" &&
+	export GIT_TRACE2 &&
+	test_when_finished "unset GIT_TRACE2" &&
 
-	OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
-	test_match_signal 13 "$OUT" &&
+	if test_have_prereq !MINGW
+	then
+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+		test_match_signal 13 "$OUT"
+	else
+		test_terminal git log
+	fi &&
+	grep "child_exit.* code:0 " trace.normal &&
 	test_path_is_file pager-used
 '
 
-test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager non-zero exit' '
+test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '
 	test_when_finished "rm pager-used" &&
 	test_config core.pager ">pager-used; head -n 1; exit 1" &&
+	GIT_TRACE2="$(pwd)/trace.normal" &&
+	export GIT_TRACE2 &&
+	test_when_finished "unset GIT_TRACE2" &&
 
-	OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
-	test_match_signal 13 "$OUT" &&
+	if test_have_prereq !MINGW
+	then
+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+		test_match_signal 13 "$OUT"
+	else
+		test_terminal git log
+	fi &&
+	grep "child_exit.* code:1 " trace.normal &&
 	test_path_is_file pager-used
 '
 
-test_expect_success TTY,!MINGW 'git discards pager non-zero exit' '
+test_expect_success TTY 'git discards pager non-zero exit' '
 	test_when_finished "rm pager-used" &&
 	test_config core.pager "wc >pager-used; exit 1" &&
+	GIT_TRACE2="$(pwd)/trace.normal" &&
+	export GIT_TRACE2 &&
+	test_when_finished "unset GIT_TRACE2" &&
 
-	OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
-	test "$OUT" -eq 0 &&
+	if test_have_prereq !MINGW
+	then
+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+		test "$OUT" -eq 0
+	else
+		test_terminal git log
+	fi &&
+	grep "child_exit.* code:1 " trace.normal &&
 	test_path_is_file pager-used
 '
 
+test_expect_success TTY 'git logs nonexisting pager invocation' '
+	test_config core.pager "does-not-exist" &&
+	GIT_TRACE2="$(pwd)/trace.normal" &&
+	export GIT_TRACE2 &&
+	test_when_finished "unset GIT_TRACE2" &&
+
+	if test_have_prereq !MINGW
+	then
+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+		test_match_signal 13 "$OUT"
+	else
+		test_terminal git log
+	fi &&
+	grep "child_exit.* code:-1 " trace.normal
+'
+
 test_done
-- 
2.30.0.284.gd98b1dd5eaa7

Re: [PATCH 3/3] pager: properly log pager exit code when signalled

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> When git invokes a pager that exits with non-zero the common case is
> that we'll already return the correct SIGPIPE failure from git itself,
> but the exit code logged in trace2 has always been incorrectly
> reported[1]. Fix that and log the correct exit code in the logs.
>
> Since this gives us something to test outside of our recently-added
> tests needing a !MINGW prerequisite, let's refactor the test to run on
> MINGW and actually check for SIGPIPE outside of MINGW.
>
> The wait_or_whine() is only called with a true "in_signal" from from
> finish_command_in_signal(), which in turn is only used in pager.c.
>
> I'm not quite sure about that BUG() case. Can we have a true in_signal
> and not have a true WIFEXITED(status)? I haven't been able to think of
> a test case for it.
>
> 1. The incorrect logging of the exit code in was seemingly copy/pasted
>    into finish_command_in_signal() in ee4512ed481 (trace2: create new
>    combined trace facility, 2019-02-22)
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  run-command.c    |  8 +++++--
>  t/t7006-pager.sh | 61 +++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 58 insertions(+), 11 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index ea4d0fb4b15..10e1c96c2bd 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -551,8 +551,12 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
>  
>  	while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
>  		;	/* nothing */
> -	if (in_signal)
> -		return 0;
> +	if (in_signal && WIFEXITED(status))
> +		return WEXITSTATUS(status);
> +	if (in_signal) {
> +		BUG("was not expecting waitpid() status %d", status);
> +		return -1;
> +	}

Doesn't BUG die, never to return control back to us?  How about
"warning()" or "error()"?

Re: [PATCH 3/3] pager: properly log pager exit code when signalled

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 01 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> When git invokes a pager that exits with non-zero the common case is
>> that we'll already return the correct SIGPIPE failure from git itself,
>> but the exit code logged in trace2 has always been incorrectly
>> reported[1]. Fix that and log the correct exit code in the logs.
>>
>> Since this gives us something to test outside of our recently-added
>> tests needing a !MINGW prerequisite, let's refactor the test to run on
>> MINGW and actually check for SIGPIPE outside of MINGW.
>>
>> The wait_or_whine() is only called with a true "in_signal" from from
>> finish_command_in_signal(), which in turn is only used in pager.c.
>>
>> I'm not quite sure about that BUG() case. Can we have a true in_signal
>> and not have a true WIFEXITED(status)? I haven't been able to think of
>> a test case for it.
>>
>> 1. The incorrect logging of the exit code in was seemingly copy/pasted
>>    into finish_command_in_signal() in ee4512ed481 (trace2: create new
>>    combined trace facility, 2019-02-22)
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  run-command.c    |  8 +++++--
>>  t/t7006-pager.sh | 61 +++++++++++++++++++++++++++++++++++++++++-------
>>  2 files changed, 58 insertions(+), 11 deletions(-)
>>
>> diff --git a/run-command.c b/run-command.c
>> index ea4d0fb4b15..10e1c96c2bd 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -551,8 +551,12 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
>>  
>>  	while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
>>  		;	/* nothing */
>> -	if (in_signal)
>> -		return 0;
>> +	if (in_signal && WIFEXITED(status))
>> +		return WEXITSTATUS(status);
>> +	if (in_signal) {
>> +		BUG("was not expecting waitpid() status %d", status);
>> +		return -1;
>> +	}
>
> Doesn't BUG die, never to return control back to us?  How about
> "warning()" or "error()"?

Maybe I shouldn't do that, but I'm doing it reflexively because SunCC
will yell at me otherwise. See 56f56ac50b9 (style: do not "break" in
switch() after "return", 2020-12-16).

Maybe I should just deal with its complaints, or add an "/* unreachable
*/" comment there...

Re: [PATCH 3/3] pager: properly log pager exit code when signalled

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/run-command.c b/run-command.c
> index ea4d0fb4b15..10e1c96c2bd 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -551,8 +551,12 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
>  
>  	while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
>  		;	/* nothing */
> -	if (in_signal)
> -		return 0;
> +	if (in_signal && WIFEXITED(status))
> +		return WEXITSTATUS(status);
> +	if (in_signal) {
> +		BUG("was not expecting waitpid() status %d", status);
> +		return -1;
> +	}

This starts reporting exit status of the pager back to
finish_command() and finish_command_in_signal().  But the code in
pager.c that call the finish_command*() ignore the returned value.

So, is the net result of these three patches just that the trace2
output gives the exit status of the pager, but "git" itself is not
affected otherwise (not a complaint; trying to understand the
intention) ?

> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index c60886f43e6..1424466caf5 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -656,31 +656,74 @@ test_expect_success TTY 'git tag with auto-columns ' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager exit' '
> +test_expect_success TTY 'git returns SIGPIPE on early pager exit' '

I somehow find 2/3 of this change belongs to the previous step.
That is, shouldn't this new test added in the previous step without
the !MINGW prerequisite, but without the trace2 bits (i.e. the first
three added lines and the last "grep" of trace.normal), and the change
made in this step limited only to those trace2 bits?

>  	test_when_finished "rm pager-used" &&
>  	test_config core.pager ">pager-used; head -n 1; exit 0" &&
> +	GIT_TRACE2="$(pwd)/trace.normal" &&
> +	export GIT_TRACE2 &&
> +	test_when_finished "unset GIT_TRACE2" &&
>  
> -	OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
> -	test_match_signal 13 "$OUT" &&
> +	if test_have_prereq !MINGW
> +	then
> +		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
> +		test_match_signal 13 "$OUT"
> +	else
> +		test_terminal git log
> +	fi &&
> +	grep "child_exit.* code:0 " trace.normal &&
>  	test_path_is_file pager-used
>  '

The same comment applies to this new one added in this step.
Shouldn't the bulk of the test (i.e. under !MINGW we get killed with
signal 13) be introduced in the previous step?


> +test_expect_success TTY 'git logs nonexisting pager invocation' '
> +	test_config core.pager "does-not-exist" &&
> +	GIT_TRACE2="$(pwd)/trace.normal" &&
> +	export GIT_TRACE2 &&
> +	test_when_finished "unset GIT_TRACE2" &&
> +
> +	if test_have_prereq !MINGW
> +	then
> +		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
> +		test_match_signal 13 "$OUT"
> +	else
> +		test_terminal git log
> +	fi &&
> +	grep "child_exit.* code:-1 " trace.normal
> +'
> +
>  test_done

Re: [PATCH 3/3] pager: properly log pager exit code when signalled

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 01 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> diff --git a/run-command.c b/run-command.c
>> index ea4d0fb4b15..10e1c96c2bd 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -551,8 +551,12 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
>>  
>>  	while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
>>  		;	/* nothing */
>> -	if (in_signal)
>> -		return 0;
>> +	if (in_signal && WIFEXITED(status))
>> +		return WEXITSTATUS(status);
>> +	if (in_signal) {
>> +		BUG("was not expecting waitpid() status %d", status);
>> +		return -1;
>> +	}
>
> This starts reporting exit status of the pager back to
> finish_command() and finish_command_in_signal().  But the code in
> pager.c that call the finish_command*() ignore the returned value.
>
> So, is the net result of these three patches just that the trace2
> output gives the exit status of the pager, but "git" itself is not
> affected otherwise (not a complaint; trying to understand the
> intention) ?

Yes, it's just a bug fix for the trace2 output.

>> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
>> index c60886f43e6..1424466caf5 100755
>> --- a/t/t7006-pager.sh
>> +++ b/t/t7006-pager.sh
>> @@ -656,31 +656,74 @@ test_expect_success TTY 'git tag with auto-columns ' '
>>  	test_cmp expect actual
>>  '
>>  
>> -test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager exit' '
>> +test_expect_success TTY 'git returns SIGPIPE on early pager exit' '
>
> I somehow find 2/3 of this change belongs to the previous step.
> That is, shouldn't this new test added in the previous step without
> the !MINGW prerequisite, but without the trace2 bits (i.e. the first
> three added lines and the last "grep" of trace.normal), and the change
> made in this step limited only to those trace2 bits?

Sure, I can re-arrange it like that. I figured 1/3 shouldn't assume 3/3,
there wouldn't be a reason not to use !MINGW except because 3/3 is going
to remove it later, but I guess it makes the overall history easier to
read...

>>  	test_when_finished "rm pager-used" &&
>>  	test_config core.pager ">pager-used; head -n 1; exit 0" &&
>> +	GIT_TRACE2="$(pwd)/trace.normal" &&
>> +	export GIT_TRACE2 &&
>> +	test_when_finished "unset GIT_TRACE2" &&
>>  
>> -	OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
>> -	test_match_signal 13 "$OUT" &&
>> +	if test_have_prereq !MINGW
>> +	then
>> +		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
>> +		test_match_signal 13 "$OUT"
>> +	else
>> +		test_terminal git log
>> +	fi &&
>> +	grep "child_exit.* code:0 " trace.normal &&
>>  	test_path_is_file pager-used
>>  '
>
> The same comment applies to this new one added in this step.
> Shouldn't the bulk of the test (i.e. under !MINGW we get killed with
> signal 13) be introduced in the previous step?

*nod*

>> +test_expect_success TTY 'git logs nonexisting pager invocation' '
>> +	test_config core.pager "does-not-exist" &&
>> +	GIT_TRACE2="$(pwd)/trace.normal" &&
>> +	export GIT_TRACE2 &&
>> +	test_when_finished "unset GIT_TRACE2" &&
>> +
>> +	if test_have_prereq !MINGW
>> +	then
>> +		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
>> +		test_match_signal 13 "$OUT"
>> +	else
>> +		test_terminal git log
>> +	fi &&
>> +	grep "child_exit.* code:-1 " trace.normal
>> +'
>> +
>>  test_done

Re: git fails with a broken pipe when one quits the pager

[Johannes Sixt]

Am 31.01.21 um 21:49 schrieb Ævar Arnfjörð Bjarmason:
> On Sun, Jan 31 2021, Vincent Lefevre wrote:
>> On 2021-01-31 02:47:59 +0100, Ævar Arnfjörð Bjarmason wrote:
>>> On Fri, Jan 15 2021, Vincent Lefevre wrote:
>>>> And of course, I don't want to hide error messages by default, because
>>>> this would hide *real* errors.
>>>
>>> Isn't the solution to this that your shell stops reporting failures due
>>> to SIGPIPE in such a prominent way then?
>>
>> No! I want to be warned about real SIGPIPEs.
> 
> Not being able to write "git log" output is a real SIGPIPE.

When Git is talking to a pager *and* it knows about it because has
started it itself, SIGPIPE is just a nuisance, not a useful behavior.

Guess why `git log` works on Windows when the pager is quit early, where
we do not have SIGPIPE? Because write errors are checked in sufficiently
many places.

I propose to do just this:

diff --git a/pager.c b/pager.c
index ee435de675..9fcc36425f 100644
--- a/pager.c
+++ b/pager.c
@@ -138,6 +138,7 @@ void setup_pager(void)
 
 	/* this makes sure that the parent terminates after the pager */
 	sigchain_push_common(wait_for_pager_signal);
+	sigchain_push(SIGPIPE, SIG_IGN);
 	atexit(wait_for_pager_atexit);
 }
 
diff --git a/run-command.c b/run-command.c
index ea4d0fb4b1..c0041413b5 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1165,10 +1165,6 @@ void check_pipe(int err)
 	if (err == EPIPE) {
 		if (in_async())
 			async_exit(141);
-
-		signal(SIGPIPE, SIG_DFL);
-		raise(SIGPIPE);
-		/* Should never happen, but just in case... */
 		exit(141);
 	}
 }