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

[Johannes Sixt <j6t@kdbg.org>]

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