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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 31, 2021, 20:49 UTC
Message-ID: <87o8h4omqa.fsf@evledraar.gmail.com>
In-Reply-To: <20210131033652.GK623063@zira.vinc17.org>

On Sun, Jan 31 2021, Vincent Lefevre wrote:

Show 41 quoted lines

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

Show 7 quoted lines

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

Show 24 quoted lines

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