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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 1, 2021, 17:47 UTC
Message-ID: <xmqqtuqvn0i7.fsf@gitster.c.googlers.com>
In-Reply-To: <87czxjomn8.fsf@evledraar.gmail.com>

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

Show 19 quoted lines

>> 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?