[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: git fails with a broken pipe when one quits the pager
From:       Ævar Arnfjörð Bjarmason <avarab () gmail ! com>
Date:       2021-01-31 20:49:49
Message-ID: 87o8h4omqa.fsf () evledraar ! gmail ! com
[Download RAW message or body]


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.
[prev in list] [next in list] [prev in thread] [next in thread]