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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.