From: Vincent Lefevre <hidden> Date: 2021-01-15 16:19:47
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. And of course, I don't want to hide error messages
by default, because this would hide *real* errors.
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
like currently.
--
Vincent Lefèvre [off-list ref] - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
From: Denton Liu <hidden> Date: 2021-01-29 23:50:03
If the pager closes before the git command feeding the pager finishes,
git is killed by a SIGPIPE and the corresponding exit code is 141.
Since the pipe is just an implementation detail, it does not make sense
for this error code to be user-facing.
Handle SIGPIPEs by simply calling exit(0) in wait_for_pager_signal().
Introduce `test-tool pager` which infinitely prints `y` to the pager in
order to test the new behavior. This cannot be tested with any existing
git command because there are no other commands which produce infinite
output. Without the change to pager.c, the newly introduced test fails.
Reported-by: Vincent Lefevre <redacted>
Signed-off-by: Denton Liu <redacted>
---
Sorry for the resend, it seems like vger has dropped the first patch.
Makefile | 1 +
pager.c | 2 ++
t/helper/test-pager.c | 12 ++++++++++++
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/t7006-pager.sh | 4 ++++
6 files changed, 21 insertions(+)
create mode 100644 t/helper/test-pager.c
From: Johannes Sixt <hidden> Date: 2021-01-30 09:29:36
Am 30.01.21 um 00:48 schrieb Denton Liu:
If the pager closes before the git command feeding the pager finishes,
git is killed by a SIGPIPE and the corresponding exit code is 141.
Since the pipe is just an implementation detail, it does not make sense
for this error code to be user-facing.
Handle SIGPIPEs by simply calling exit(0) in wait_for_pager_signal().
Introduce `test-tool pager` which infinitely prints `y` to the pager in
order to test the new behavior. This cannot be tested with any existing
git command because there are no other commands which produce infinite
output. Without the change to pager.c, the newly introduced test fails.
Reported-by: Vincent Lefevre <redacted>
Signed-off-by: Denton Liu <redacted>
My gut feeling tells that this will end in an infinite loop on Windows.
There are no signals on Windows that would kill the upstream of a pipe.
This call site will only notice that the downstream of the pipe was
closed, when it checks for write errors.
Let me test it.
-- Hannes
My gut feeling tells that this will end in an infinite loop on Windows.
There are no signals on Windows that would kill the upstream of a pipe.
This call site will only notice that the downstream of the pipe was
closed, when it checks for write errors.
Let me test it.
The test case is protected by a TTY prerequisite; that is not satisfied
on Windows, and the test is skipped. No harm done so far.
But when I run `test-tool pager` manually and quit out of the pager, the
tool does spin in the endless loop. The following fixup helps.
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.
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?
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?
In both cases the exit code means the same thing, that something in a
pipe wasn't fully consumed being signalled to calling processes is the
point of SIGPIPE.
From: Vincent Lefevre <hidden> Date: 2021-01-31 03:38:39
On 2021-01-31 02:47:59 +0100, Ævar Arnfjörð Bjarmason wrote:
On Fri, Jan 15 2021, Vincent Lefevre wrote:
quoted
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
quoted
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.
quoted
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>
--
Vincent Lefèvre [off-list ref] - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
From: Vincent Lefevre <hidden> Date: 2021-01-31 03:49:12
On 2021-01-31 04:36:52 +0100, Vincent Lefevre wrote:
On 2021-01-31 02:47:59 +0100, Ævar Arnfjörð Bjarmason wrote:
quoted
On Fri, Jan 15 2021, Vincent Lefevre wrote:
quoted
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>
Well, more precisely, I mean that it is not reported. But
the SIGPIPE itself still occurs as expected, e.g. for scripts,
and one may choose to ignore it or not, as usual.
--
Vincent Lefèvre [off-list ref] - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
On 2021-01-31 02:47:59 +0100, Ævar Arnfjörð Bjarmason wrote:
quoted
On Fri, Jan 15 2021, Vincent Lefevre wrote:
quoted
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.
quoted
quoted
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.
quoted
quoted
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.
From: Vincent Lefevre <hidden> Date: 2021-02-01 10:35:14
On 2021-01-31 21:49:49 +0100, Ævar Arnfjörð Bjarmason wrote:
On Sun, Jan 31 2021, Vincent Lefevre wrote:
quoted
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
If the pager exists with a non-zero exit status, it is normal to
return a non-zero exit status. This was not the bug I reported.
quoted
No! I want to be warned about real SIGPIPEs.
Not being able to write "git log" output is a real SIGPIPE.
Which is not the case here, because the full output has never been
requested by the user.
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
See the difference (without the patch) between
$ git log && echo foo; echo $?
141
and
$ git log | head; echo $?
[...]
0
[...]
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?).
zsh is working as expected, and as I've already said, I ***WANT***
SIGPIPE to be reported by the shell, as it may indicate a real failure
in a script. BTW, I even have a script using git that relies on that:
{ git rev-list --author "$@[-1]" HEAD &&
git rev-list --grep "$@[-1]" HEAD } | \
git "${@[1,-2]:-lv}" --no-walk --stdin
return $((pipestatus[2] ? pipestatus[2] : pipestatus[1]))
Here it is important not to lose any information. No pager is
involved, the full output is needed. If for some reason, the
LHS of the pipe fails due to a SIGPIPE but the right hand side
succeeds, the error will be reported.
The fact is that with a pager, the SIGPIPE with a pager is normal.
Thus with a pager, git is reporting a spurious SIGPIPE, and this
is disturbing.
--
Vincent Lefèvre [off-list ref] - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
From: Chris Torek <hidden> Date: 2021-02-01 11:35:13
On 2021-01-31 21:49:49 +0100, Ævar Arnfjörð Bjarmason wrote:
quoted
... 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
This example has a minor flaw: it should use `git -c core.pager=/bin/true`,
probably.
On Mon, Feb 1, 2021 at 2:36 AM Vincent Lefevre [off-list ref] wrote:
If the pager exists with a non-zero exit status, it is normal to
return a non-zero exit status. This was not the bug I reported.
That's the flaw in the example. The key though is that the program
we ran as the pager—false, true, whatever—*did not read any of its input*.
quoted
Not being able to write "git log" output is a real SIGPIPE.
Worth noting: Linux has a pretty large pipe buffer. POSIX requires
at least 4k here, as I recall, but Linux will buffer 64k or more, so that
if `git log` is able to write the entire log text (will be the case for small
repositories) *before* the program on the right side of the pager pipe
exits (this depends on many things), the pager's exit *won't* cause
a SIGPIPE. You'll get the SIGPIPE if either the pager exits very
quickly, so that `git log` is unable to write much before the exit, or
if the repository is sufficiently large so that the pipe blocks first.
Which is not the case here, because the full output has never been
requested by the user.
The `git log` command *did* request the full output.
The problem that has come up is, if I understand correctly, that
some Linux distributions have come with misconfigured pagers
that don't bother reading their input, and silently exit zero. This
causes all kinds of Git commands to *seem* to fail. The Git commands
are just fine; the bug is that the pager doesn't read or write anything.
Unfortunately, the way that pipes work -- asynchronously -- means
that Git really *can't* catch all problems here. But catching a SIGPIPE,
whether Git itself spawned the pager or not, does indicate that
something has gone wrong ... *unless* Git was piping to, e.g., less,
and the user read enough, and the user typed `q` at less, and less
exited without bothering to read the rest of the input.
There's no good way for Git to be able to tell which of these was
the case.
I'm not sure what this actually argues for. ;-)
Chris
On 2021-01-31 21:49:49 +0100, �var Arnfj�r� Bjarmason wrote:
quoted
On Sun, Jan 31 2021, Vincent Lefevre wrote:
quoted
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
If the pager exists with a non-zero exit status, it is normal to
return a non-zero exit status. This was not the bug I reported.
Is it normal? Isn't this subject to the same race noted in
https://lore.kernel.org/git/20191115040909.GA21654@sigill.intra.peff.net/
I.e. we start the /bin/false process, then start spewing output to it,
so maybe we'll get a SIGPIPE first because it's not being consumed, or
maybe /bin/false (or whatever else exits with non-zero) will exit first.
Unrelated to that there's at least a bug in wait_for_pager_signal() in
how we log the pager's exit. We just rely on "ret" in
finish_command_in_signal() before calling trace2_child_exit(), but
should probably log -1 there or something and defer.
quoted
quoted
No! I want to be warned about real SIGPIPEs.
Not being able to write "git log" output is a real SIGPIPE.
Which is not the case here, because the full output has never been
requested by the user.
They requested it by running "git log", which e.g. for git.git is ~1
million lines. Then presumably paged down just a few pages and issued
"q" in their pager. At which point we'll fail on the write() in git-log.
The pager's exit status is usually/always 0 in those cases
(e.g. https://pubs.opengroup.org/onlinepubs/9699919799/utilities/more.html). So
we've got the SIGPIPE to indicate the output wasn't fully consumed.
quoted
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
See the difference (without the patch) between
$ git log && echo foo; echo $?
141
and
$ git log | head; echo $?
[...]
0
Presumably that first command is one where you exited your pager before
the output wasn't fully consumed, see above.
[...]
quoted
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?).
zsh is working as expected, and as I've already said, I ***WANT***
SIGPIPE to be reported by the shell, as it may indicate a real failure
in a script. BTW, I even have a script using git that relies on that:
{ git rev-list --author "$@[-1]" HEAD &&
git rev-list --grep "$@[-1]" HEAD } | \
git "${@[1,-2]:-lv}" --no-walk --stdin
return $((pipestatus[2] ? pipestatus[2] : pipestatus[1]))
Here it is important not to lose any information. No pager is
involved, the full output is needed. If for some reason, the
LHS of the pipe fails due to a SIGPIPE but the right hand side
succeeds, the error will be reported.
Sorry, I really don't see how this is different. I think this goes back
to my "'|' shell piping construct[...]" question in the E-Mail you're
replying to.
in both the "git log &&" case and potentially here you'll get a program
writing to a pipe getting a SIGPIPE, which is then reflected in the exit
code.
The fact is that with a pager, the SIGPIPE with a pager is normal.
Thus with a pager, git is reporting a spurious SIGPIPE, and this
is disturbing.
I don't get what you're trying to say here, sorry.
Maybe this helps. So first, I don't know if your report came out of
reading the recent "set -o pipefail" traffic on-list. As you can see in
[1] I'm not some zealot for PIPEFAIL always being returned no matter
what.
The difference between that though and what you're proposing is there
you have the shell getting an exit code and opting to ignore it, as
opposed to the program itself sweeping it under the rug.
I don't think either that just because you run a pager you're obligated
to ferry down a SIGPIPE if you get it. E.g. mysql and postgresql both
have interactive shells where you can open pagers. I didn't bother to
check, but you can imagine doing a "show tables" or whatever and only
viewing the first page, then quitting in the pager.
If that's part of a long interactive SQL session it would make no sense
for the eventual exit code of mysql(1) or psql(1) to reflect that.
But with git we're (mostly) executing one-shot commands, e.g. with "git
log" you give it some params, and it spews all the output at you, maybe
with the help of a pager.
So then if we fail on the write() I don't see how it doesn't make sense
to return the appropriate exit code for that failure downstream.
1. https://lore.kernel.org/git/20210116153554.12604-12-avarab@gmail.com/
From: Vincent Lefevre <hidden> Date: 2021-02-01 12:37:42
On 2021-02-01 03:33:54 -0800, Chris Torek wrote:
quoted
On 2021-01-31 21:49:49 +0100, Ævar Arnfjörð Bjarmason wrote:
quoted
... 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
This example has a minor flaw: it should use `git -c core.pager=/bin/true`,
probably.
In this case, since /bin/true doesn't read anything on purpose,
I would not expect any non-zero exit status.
And note that
git -c core.pager="sh -c 'cat; /bin/false'" log
exits with a zero exit status, which is unexpected since the
pager failed. For instance, in practice, the pager could be
killed by the system, but the user would not necessarily notice
this as the pager may be configured to quit automatically when
reaching the end of the output (there are some "less" options
to do that: -E, -F). So, the user would think that he got the
full output while he didn't.
[...]
quoted
quoted
Not being able to write "git log" output is a real SIGPIPE.
Worth noting: Linux has a pretty large pipe buffer. POSIX requires
at least 4k here, as I recall, but Linux will buffer 64k or more, so that
if `git log` is able to write the entire log text (will be the case for small
repositories) *before* the program on the right side of the pager pipe
exits (this depends on many things), the pager's exit *won't* cause
a SIGPIPE. You'll get the SIGPIPE if either the pager exits very
quickly, so that `git log` is unable to write much before the exit, or
if the repository is sufficiently large so that the pipe blocks first.
In general, repositories have more than 64k log.
quoted
Which is not the case here, because the full output has never been
requested by the user.
The `git log` command *did* request the full output.
No, because the output is sent to a pager. As long as the user
does not look at more than what he looks for, no more "git log"
output is requested (such output can happen internally, but it
is not requested by the user).
The problem that has come up is, if I understand correctly, that
some Linux distributions have come with misconfigured pagers
that don't bother reading their input, and silently exit zero.
They are not misconfigured. This is how they work. Actually I don't
see why they should read more than needed: this would be a useless
waste of memory.
This causes all kinds of Git commands to *seem* to fail. The Git
commands are just fine; the bug is that the pager doesn't read or
write anything.
Unfortunately, the way that pipes work -- asynchronously -- means
that Git really *can't* catch all problems here. But catching a SIGPIPE,
whether Git itself spawned the pager or not, does indicate that
something has gone wrong ... *unless* Git was piping to, e.g., less,
and the user read enough, and the user typed `q` at less, and less
exited without bothering to read the rest of the input.
There's no good way for Git to be able to tell which of these was
the case.
In the case git spawns a pager, it knows that this is a pager
(as per documentation).
--
Vincent Lefèvre [off-list ref] - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
From: Chris Torek <hidden> Date: 2021-02-01 12:54:24
On Mon, Feb 1, 2021 at 4:36 AM Vincent Lefevre [off-list ref] wrote:
In general, repositories have more than 64k log.
Please don't focus on the exact size. Some system might
have a multi-gigabyte pipe buffer, and some other system
might have a tiny one; we'd like consistent behavior no matter
what size the system uses. Can we *get* consistent behavior?
I don't know.
[me]
quoted
The problem that has come up is, if I understand correctly, that
some Linux distributions have come with misconfigured pagers
that don't bother reading their input, and silently exit zero.
They are not misconfigured. This is how they work.
A pager that reads nothing and writes nothing does not seem
very useful to me. (Perhaps we can disregard these cases
entirely. It's not like we should expect Git to handle things if
someone builds a version of `less` that doesn't work. The
fact is that on these Linux systems, running `$pager foo` on a
file `foo` does nothing at all, for some values of `$pager`. I
believe I ran into this on a Docker setup at least once. It's
not Git's fault and hence not something for it to correct.)
[on various exit cases]
quoted
There's no good way for Git to be able to tell which of these was
the case.
In the case git spawns a pager, it knows that this is a pager
(as per documentation).
Again, this seems irrelevant. If the pager exited correctly
while reading everything, or it exited correctly without reading
everything, or if it exited incorrectly with or without reading
everything, is not something *Git* can tell. I'm therefore not
sure that Git should *try* to tell -- which is the point I'm trying
to make here. The question is this: if we can only do a poor
job, should we try at all? What *should* we do, given what
we *can* do? All we get is SIGPIPE and an exit status, and
the SIGPIPE may or may not be meaningful.
That seems to be what you're arguing as well. So I'm not sure
why you're objecting to what I'm pointing out. :-)
Chris
When git invokes a pager that exits with non-zero the common case is
that we'll already return the correct SIGPIPE failure from git itself,
but the exit code logged in trace2 has always been incorrectly
reported[1]. Fix that and log the correct exit code in the logs.
Since this gives us something to test outside of our recently-added
tests needing a !MINGW prerequisite, let's refactor the test to run on
MINGW and actually check for SIGPIPE outside of MINGW.
The wait_or_whine() is only called with a true "in_signal" from from
finish_command_in_signal(), which in turn is only used in pager.c.
I'm not quite sure about that BUG() case. Can we have a true in_signal
and not have a true WIFEXITED(status)? I haven't been able to think of
a test case for it.
1. The incorrect logging of the exit code in was seemingly copy/pasted
into finish_command_in_signal() in ee4512ed481 (trace2: create new
combined trace facility, 2019-02-22)
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
run-command.c | 8 +++++--
t/t7006-pager.sh | 61 +++++++++++++++++++++++++++++++++++++++++-------
2 files changed, 58 insertions(+), 11 deletions(-)
@@ -551,8 +551,12 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)while((waiting=waitpid(pid,&status,0))<0&&errno==EINTR);/* nothing */-if(in_signal)-return0;+if(in_signal&&WIFEXITED(status))+returnWEXITSTATUS(status);+if(in_signal){+BUG("was not expecting waitpid() status %d",status);+return-1;+}if(waiting<0){failed_errno=errno;
Refactor the wait_for_pager() function. Since 507d7804c0b (pager:
don't use unsafe functions in signal handlers, 2015-09-04) the
wait_for_pager() and wait_for_pager_atexit() callers diverged on more
than they shared.
Let's extract the common code into a new close_pager_fds() helper, and
move the parts unique to the only to callers to those functions.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
pager.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
@@ -11,29 +11,25 @@staticstructchild_processpager_process=CHILD_PROCESS_INIT;staticconstchar*pager_program;-staticvoidwait_for_pager(intin_signal)+staticvoidclose_pager_fds(void){-if(!in_signal){-fflush(stdout);-fflush(stderr);-}/* signal EOF to pager */close(1);close(2);-if(in_signal)-finish_command_in_signal(&pager_process);-else-finish_command(&pager_process);}staticvoidwait_for_pager_atexit(void){-wait_for_pager(0);+fflush(stdout);+fflush(stderr);+close_pager_fds();+finish_command(&pager_process);}staticvoidwait_for_pager_signal(intsigno){-wait_for_pager(1);+close_pager_fds();+finish_command_in_signal(&pager_process);sigchain_pop(signo);raise(signo);}
Add tests for how git behaves when the pager itself exits with
non-zero, as well as for us exiting with 141 when we're killed with
SIGPIPE due to the pager not consuming its output.
There is some recent discussion[1] about these semantics, but aside
from what we want to do in the future, we should have a test for the
current behavior.
This test construct is stolen from 7559a1be8a0 (unblock and unignore
SIGPIPE, 2014-09-18).
1. https://lore.kernel.org/git/87o8h4omqa.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t7006-pager.sh | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
While reading the pager code I discovered[1] that we log the wrong
exit code when the pager itself exits with non-zero under trace2. This
fixes that bug.
I think whatever the consensus is on the SIGPIPE exit status
propagating it makes sense if we'd ignore it to rebase the patch to do
so[1] on this. I think the addition of a new "test-tool pager" there
is redundant to testing SIGHUP from git itself as 1/3 does here, but
maybe I'm missing something...
2/3 is not needed for the end-state here, but I figured it was a good
refactoring while I was at it.
1. https://lore.kernel.org/git/bc88492979fee215d5be06ccbc246ae0171a9ced.1611910122.git.liu.denton@gmail.com/
Ævar Arnfjörð Bjarmason (3):
pager: test for exit code
pager: refactor wait_for_pager() function
pager: properly log pager exit code when signalled
pager.c | 18 +++++--------
run-command.c | 8 ++++--
t/t7006-pager.sh | 70 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 83 insertions(+), 13 deletions(-)
--
2.30.0.284.gd98b1dd5eaa7
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".
On 2021-01-31 21:49:49 +0100, Ævar Arnfjörð Bjarmason wrote:
quoted
... 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
This example has a minor flaw: it should use `git -c core.pager=/bin/true`,
probably.
FWIW it doesn't have a flaw It should be /bin/false, not /bin/true. See
this reply in a side-thread:
https://lore.kernel.org/git/87im7cng42.fsf@evledraar.gmail.com/
I.e. part of the point here (which I realize I forgot to articulate...)
is that we have a hard reliance on SIGHUP to report *any* pager failures
as a matter of the current implementation.
Part of that has to do with internal git implementation details, i.e. we
get the exit code for the pager either in an atexit() handler (we've
already picked the exit code) or when handling a signal.
Perhaps we could do better there and e.g. exit with <num> if the pager
exits with <num>. I don't know what's the conventional behavior in that
case.
But in any case, we exit with SIGPIPE in those cases in any reasonable
failure mode. That is, unless the pager consumed all the output, and
*then* died that is.
I submitted
https://lore.kernel.org/git/20210201144921.8664-1-avarab@gmail.com/ to
try to address the lack of testing around this, which has tests for the
true/false case.
From: Vincent Lefevre <hidden> Date: 2021-02-01 15:18:32
On 2021-02-01 04:53:03 -0800, Chris Torek wrote:
On Mon, Feb 1, 2021 at 4:36 AM Vincent Lefevre [off-list ref] wrote:
quoted
In general, repositories have more than 64k log.
Please don't focus on the exact size. Some system might
have a multi-gigabyte pipe buffer, and some other system
might have a tiny one; we'd like consistent behavior no matter
what size the system uses. Can we *get* consistent behavior?
I don't know.
The consistent behavior can be obtained by ignoring the broken pipe
(in the case where git starts the pager).
[me]
quoted
quoted
The problem that has come up is, if I understand correctly, that
some Linux distributions have come with misconfigured pagers
that don't bother reading their input, and silently exit zero.
They are not misconfigured. This is how they work.
A pager that reads nothing and writes nothing does not seem
very useful to me. [...]
I agree.
[on various exit cases]
quoted
quoted
There's no good way for Git to be able to tell which of these was
the case.
In the case git spawns a pager, it knows that this is a pager
(as per documentation).
Again, this seems irrelevant. If the pager exited correctly
while reading everything, or it exited correctly without reading
everything, or if it exited incorrectly with or without reading
everything, is not something *Git* can tell.
No, Git can tell when the pager exited abnormally: it suffices to
check its exit status. Git currently doesn't do that, and this is
bad, because it can miss real issues, which cannot always be detected
by the user.
If the pager exits with exit code 0, this means normal termination,
whether the user has read the full output or not.
I'm therefore not sure that Git should *try* to tell -- which is the
point I'm trying to make here. The question is this: if we can only
do a poor job, should we try at all? What *should* we do, given what
we *can* do? All we get is SIGPIPE and an exit status, and the
SIGPIPE may or may not be meaningful.
That seems to be what you're arguing as well. So I'm not sure
why you're objecting to what I'm pointing out. :-)
Well, my objection is based on the fact that it is possible to get
the information from the exit status of the pager (I originally
thought that Git was taking it into account).
BTW, another related thing I dislike about Git, and I think that this
should also be regarded as a bug, is that when doing a commit, Git
doesn't check the exit status of the editor for the commit message.
Say, for instance, if something on the system kills the editor, Git
applies the commit with an incorrect or incomplete log message though
the commit wasn't validated yet by the user. Fortunately, the user
can amend the commit, but IMHO, that's an incorrect behavior.
--
Vincent Lefèvre [off-list ref] - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
From: Vincent Lefevre <hidden> Date: 2021-02-01 15:25:57
On 2021-02-01 13:10:21 +0100, Ævar Arnfjörð Bjarmason wrote:
On Mon, Feb 01 2021, Vincent Lefevre wrote:
quoted
On 2021-01-31 21:49:49 +0100, �var Arnfj�r� Bjarmason wrote:
quoted
On Sun, Jan 31 2021, Vincent Lefevre wrote:
quoted
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
If the pager exists with a non-zero exit status, it is normal to
return a non-zero exit status. This was not the bug I reported.
There's a race only because the command is buggy under bash's
pipefail.
Something like
git status -s -b | head -1
is fine by default, because the exist status of the LHS command is
ignored. With pipefail, you may start getting SIGPIPE exit codes,
which, depending on the context, you may want to ignore or not.
I suppose that the user who writes something like the above would
like to ignore SIGPIPE.
So, that should be:
{ git status -s -b; if [[ $? = 141 ]]; then return 0; fi } | head -1
(though that's 100% safe only if git catches/blocks/ignores SIGPIPE
and detect the broken pipe with EPIPE, so that an abnormal termination
due to a "kill -PIPE ..." from another process would not be ignored).
It appears that pipefail was designed mainly for scripts. So, having
to handle SIGPIPE like that is OK in scripts. For interactive use,
this would be bad, but that's not the purpose of pipefail (or bash
should have an option to regard 141 as 0 in any LHS command).
FYI, I have a zsh function to automatically pipe some commands to
"less" when connected to a terminal (a bit like what git does),
where I explicitly ignore SIGPIPE for the command:
pager-wrapper()
{
local -a opt
while [[ $1 == -* ]]
do
opt+=$1
shift
done
if [[ -t 1 ]] then
$@ $opt |& less -+c -FRX
return $(( $pipestatus[2] != 0 ? $pipestatus[2] :
$pipestatus[1] != 128 + $(kill -l PIPE) ? $pipestatus[1] : 0 ))
else
$@
fi
}
So no SIGPIPE is reported when I quit the pager. I can still get a
reported SIGPIPE, e.g. if "less" is killed by SIGPIPE (e.g., this
is possible with "kill -PIPE ..."), and this one is meaningful.
quoted
quoted
quoted
No! I want to be warned about real SIGPIPEs.
Not being able to write "git log" output is a real SIGPIPE.
Which is not the case here, because the full output has never been
requested by the user.
They requested it by running "git log", which e.g. for git.git is ~1
million lines. Then presumably paged down just a few pages and issued
"q" in their pager. At which point we'll fail on the write() in git-log.
But when outputting to a pager, this should not be regarded as an
error: the reason is either the user has quit the pager normally
(after having read what he wanted to read: the user did not need
more output) or the pager has terminated in an abnormal way, in
which case the exit status of the pager should be non-zero.
Yes, and there's no reason to return anything else, as quitting the
pager before reading the full output is not an error.
So we've got the SIGPIPE to indicate the output wasn't fully
consumed.
But the user doesn't care: he quit the pager because he didn't
need more output. So there is no need to signal that the output
wasn't fully consumed. The user already knew that before quitting
the pager!
quoted
[...]
quoted
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?).
zsh is working as expected, and as I've already said, I ***WANT***
SIGPIPE to be reported by the shell, as it may indicate a real failure
in a script. BTW, I even have a script using git that relies on that:
{ git rev-list --author "$@[-1]" HEAD &&
git rev-list --grep "$@[-1]" HEAD } | \
git "${@[1,-2]:-lv}" --no-walk --stdin
return $((pipestatus[2] ? pipestatus[2] : pipestatus[1]))
Here it is important not to lose any information. No pager is
involved, the full output is needed. If for some reason, the
LHS of the pipe fails due to a SIGPIPE but the right hand side
succeeds, the error will be reported.
Sorry, I really don't see how this is different. I think this goes back
to my "'|' shell piping construct[...]" question in the E-Mail you're
replying to.
in both the "git log &&" case and potentially here you'll get a program
writing to a pipe getting a SIGPIPE, which is then reflected in the exit
code.
I mean that there are SIGPIPEs that one does not want to ignore
(because they would indicate a problem -- in general in scripts),
and other ones that should be ignored because they don't indicate
an error.
quoted
The fact is that with a pager, the SIGPIPE with a pager is normal.
Thus with a pager, git is reporting a spurious SIGPIPE, and this
is disturbing.
I don't get what you're trying to say here, sorry.
I mean that when the user quits the pager, there is no reason to
report an error because the user explicitly wanted to quit now.
Similarly, if I run a text viewer on a file, I don't want a SIGPIPE
to be reported if I do not go to the end of the file (if a pipe was
used to read the file, e.g. to do some filtering, as "less" can do).
Maybe this helps. So first, I don't know if your report came out of
reading the recent "set -o pipefail" traffic on-list. As you can see in
[1] I'm not some zealot for PIPEFAIL always being returned no matter
what.
This is not related. And [1] is from 2021 (with a thread started
in 2019), while my report dates back to 2018:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914896
Moreover, [1] is only about the use of pipes in the shell command.
My bug report is about the internal use of a pager by git.
The difference between that though and what you're proposing is there
you have the shell getting an exit code and opting to ignore it, as
opposed to the program itself sweeping it under the rug.
I don't think either that just because you run a pager you're obligated
to ferry down a SIGPIPE if you get it. E.g. mysql and postgresql both
have interactive shells where you can open pagers. I didn't bother to
check, but you can imagine doing a "show tables" or whatever and only
viewing the first page, then quitting in the pager.
If that's part of a long interactive SQL session it would make no sense
for the eventual exit code of mysql(1) or psql(1) to reflect that.
But with git we're (mostly) executing one-shot commands, e.g. with "git
log" you give it some params, and it spews all the output at you, maybe
with the help of a pager.
So then if we fail on the write() I don't see how it doesn't make sense
to return the appropriate exit code for that failure downstream.
This depends on the kind of error. I agree for an unexpected error.
But for a broken pipe because git started a pager on its own and
the user chose to quit the pager, this should not be regarded as
an error.
On 2021-02-01 13:10:21 +0100, �var Arnfj�r� Bjarmason wrote:
quoted
n>> On Mon, Feb 01 2021, Vincent Lefevre wrote:
quoted
quoted
On 2021-01-31 21:49:49 +0100, �var Arnfj�r� Bjarmason wrote:
quoted
On Sun, Jan 31 2021, Vincent Lefevre wrote:
quoted
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
If the pager exists with a non-zero exit status, it is normal to
return a non-zero exit status. This was not the bug I reported.
There's a race only because the command is buggy under bash's
pipefail.
Something like
git status -s -b | head -1
is fine by default, because the exist status of the LHS command is
ignored. With pipefail, you may start getting SIGPIPE exit codes,
which, depending on the context, you may want to ignore or not.
I suppose that the user who writes something like the above would
like to ignore SIGPIPE.
So, that should be:
{ git status -s -b; if [[ $? = 141 ]]; then return 0; fi } | head -1
(though that's 100% safe only if git catches/blocks/ignores SIGPIPE
and detect the broken pipe with EPIPE, so that an abnormal termination
due to a "kill -PIPE ..." from another process would not be ignored).
It appears that pipefail was designed mainly for scripts. So, having
to handle SIGPIPE like that is OK in scripts. For interactive use,
this would be bad, but that's not the purpose of pipefail (or bash
should have an option to regard 141 as 0 in any LHS command).
FYI, I have a zsh function to automatically pipe some commands to
"less" when connected to a terminal (a bit like what git does),
where I explicitly ignore SIGPIPE for the command:
I think there's some confusion here. I'm not referring to how "set -o
pipefail" behaves in bash. But pointing to Jeff King's simple example[1]
of how a command like "git status -sb" might exit (note that it may
print more than 1 line) due to a race with how SIGPIPE interacts with
exit statuses. That's *nix/POSIX behavior, nothing to do with bash.
The same will apply to a pager we launch on a command like "git log".
As Chris Torek noted in a side-thread[2] the buffers involved here are
OS-defined. In the general case you may get a PIPEFAIL or not depending
on whether you e.g. cross a PIPE_BUF boundary to get from line 1 to 2 of
your output, while "head -n 1" is consuming it.
But then consider a pager like:
while (wantit())
consume_and_print_output();
sleep(10);
exit(1);
Now we can just exit early if it decides it doesn't want our output, as
we'll likely get a SIGPIPE, but if we're ignoring SIGPIPE and we want to
distinguish that from non-zero pager exit codes, we need to wait 10
seconds until waitpid() tells us what the exit status is.
That's obviously a contrived example, but demonstrates the race
condition involved.
1. https://lore.kernel.org/git/20191115040909.GA21654@sigill.intra.peff.net/
2. https://lore.kernel.org/git/CAPx1Gverh2E2h5JOSOfJ7JYvbhjv8hJNLE8y4VA2fNv0La8Rtw@mail.gmail.com/
quoted
quoted
quoted
Not being able to write "git log" output is a real SIGPIPE.
Which is not the case here, because the full output has never been
requested by the user.
They requested it by running "git log", which e.g. for git.git is ~1
million lines. Then presumably paged down just a few pages and issued
"q" in their pager. At which point we'll fail on the write() in git-log.
But when outputting to a pager, this should not be regarded as an
error: the reason is either the user has quit the pager normally
(after having read what he wanted to read: the user did not need
more output) or the pager has terminated in an abnormal way, in
which case the exit status of the pager should be non-zero.
In an ideal world, or something we can plausibly implement in a portable
manner on systems that exist in the wild?
Yes I agree that this sort of behavior would be stupid e.g. for an
integrated GUI application, but that's not what we've got. We're calling
an arbitrary user-supplied command and piping output to it, and are then
going to get SIGPIPE or an exit code back.
Yes, and there's no reason to return anything else, as quitting the
pager before reading the full output is not an error.
quoted
So we've got the SIGPIPE to indicate the output wasn't fully
consumed.
But the user doesn't care: he quit the pager because he didn't
need more output. So there is no need to signal that the output
wasn't fully consumed. The user already knew that before quitting
the pager!
As noted above, this is assuming way too much about the functionality of
the pager command. We can get a SIGPIPE without the user's intent in
this way. Consider e.g. piping to some remote system via netcat.
quoted
quoted
[...]
quoted
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?).
zsh is working as expected, and as I've already said, I ***WANT***
SIGPIPE to be reported by the shell, as it may indicate a real failure
in a script. BTW, I even have a script using git that relies on that:
{ git rev-list --author "$@[-1]" HEAD &&
git rev-list --grep "$@[-1]" HEAD } | \
git "${@[1,-2]:-lv}" --no-walk --stdin
return $((pipestatus[2] ? pipestatus[2] : pipestatus[1]))
Here it is important not to lose any information. No pager is
involved, the full output is needed. If for some reason, the
LHS of the pipe fails due to a SIGPIPE but the right hand side
succeeds, the error will be reported.
Sorry, I really don't see how this is different. I think this goes back
to my "'|' shell piping construct[...]" question in the E-Mail you're
replying to.
in both the "git log &&" case and potentially here you'll get a program
writing to a pipe getting a SIGPIPE, which is then reflected in the exit
code.
I mean that there are SIGPIPEs that one does not want to ignore
(because they would indicate a problem -- in general in scripts),
and other ones that should be ignored because they don't indicate
an error.
quoted
quoted
The fact is that with a pager, the SIGPIPE with a pager is normal.
Thus with a pager, git is reporting a spurious SIGPIPE, and this
is disturbing.
I don't get what you're trying to say here, sorry.
I mean that when the user quits the pager, there is no reason to
report an error because the user explicitly wanted to quit now.
Sure, in an ideal world. But we don't get a SIGUSERPRESSEDTHEQBUTTON, we
get a SIGPIPE.
Similarly, if I run a text viewer on a file, I don't want a SIGPIPE
to be reported if I do not go to the end of the file (if a pipe was
used to read the file, e.g. to do some filtering, as "less" can do).
Yes, that makes perfect sense. Neither would I, but that text viewer is
one process, so it doesn't have to deal with IPC and propagating exit
codes from failed IPC.
quoted
Maybe this helps. So first, I don't know if your report came out of
reading the recent "set -o pipefail" traffic on-list. As you can see in
[1] I'm not some zealot for PIPEFAIL always being returned no matter
what.
Indeed, I just misread (or didn't read in the first place) the times
involved. I started reading at Denton Liu's patch sent a couple of days
ago.
Moreover, [1] is only about the use of pipes in the shell command.
My bug report is about the internal use of a pager by git.
I probably shouldn't have linked to that thread, but as as noted at the
start of the E-Mail I was referring to it for the SIGPIPE behavior
discussed there, not bash/set -o pipefail etc.
quoted
The difference between that though and what you're proposing is there
you have the shell getting an exit code and opting to ignore it, as
opposed to the program itself sweeping it under the rug.
I don't think either that just because you run a pager you're obligated
to ferry down a SIGPIPE if you get it. E.g. mysql and postgresql both
have interactive shells where you can open pagers. I didn't bother to
check, but you can imagine doing a "show tables" or whatever and only
viewing the first page, then quitting in the pager.
If that's part of a long interactive SQL session it would make no sense
for the eventual exit code of mysql(1) or psql(1) to reflect that.
But with git we're (mostly) executing one-shot commands, e.g. with "git
log" you give it some params, and it spews all the output at you, maybe
with the help of a pager.
So then if we fail on the write() I don't see how it doesn't make sense
to return the appropriate exit code for that failure downstream.
This depends on the kind of error. I agree for an unexpected error.
But for a broken pipe because git started a pager on its own and
the user chose to quit the pager, this should not be regarded as
an error.
As noted above, we don't have a way of knowing that, we're not the
pager.
It also seems to me that whether git should report errors, and what 3rd
party tools that might invoke git are going to do with a SIGPIPE exit
code is being mixed up here.
And then whether it makes sense to ignore SIGPIPE for all users, or
e.g. if it's some opt-in setting in some situations that users might
want to turn on because they're aware of how their pager behaves and
want to work around some zsh mode.
From: Johannes Sixt <hidden> Date: 2021-02-01 22:05:48
Am 31.01.21 um 21:49 schrieb Ævar Arnfjörð Bjarmason:
On Sun, Jan 31 2021, Vincent Lefevre wrote:
quoted
On 2021-01-31 02:47:59 +0100, Ævar Arnfjörð Bjarmason wrote:
quoted
On Fri, Jan 15 2021, Vincent Lefevre wrote:
quoted
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.
When Git is talking to a pager *and* it knows about it because has
started it itself, SIGPIPE is just a nuisance, not a useful behavior.
Guess why `git log` works on Windows when the pager is quit early, where
we do not have SIGPIPE? Because write errors are checked in sufficiently
many places.
I propose to do just this:
@@ -138,6 +138,7 @@ void setup_pager(void)/* this makes sure that the parent terminates after the pager */sigchain_push_common(wait_for_pager_signal);+sigchain_push(SIGPIPE,SIG_IGN);atexit(wait_for_pager_atexit);}
@@ -1165,10 +1165,6 @@ void check_pipe(int err)if(err==EPIPE){if(in_async())async_exit(141);--signal(SIGPIPE,SIG_DFL);-raise(SIGPIPE);-/* Should never happen, but just in case... */exit(141);}}
From: Johannes Sixt <hidden> Date: 2021-02-01 22:17:13
Am 01.02.21 um 16:44 schrieb Ævar Arnfjörð Bjarmason:
On Mon, Feb 01 2021, Vincent Lefevre wrote:
quoted
On 2021-02-01 13:10:21 +0100, Ævar Arnfjörð Bjarmason:
quoted
So we've got the SIGPIPE to indicate the output wasn't fully
consumed.
But the user doesn't care: he quit the pager because he didn't
need more output. So there is no need to signal that the output
wasn't fully consumed. The user already knew that before quitting
the pager!
As noted above, this is assuming way too much about the functionality of
the pager command. We can get a SIGPIPE without the user's intent in
this way. Consider e.g. piping to some remote system via netcat.
That assumption is warranted, IMO. Aren't _you_ stretching the meaning
of "pager" too far here? A pager is intended for presentation to the
user. If someone plays games with it, they should know what they get.
-- Hannes
Refactor the wait_for_pager() function. Since 507d7804c0b (pager:
don't use unsafe functions in signal handlers, 2015-09-04) the
wait_for_pager() and wait_for_pager_atexit() callers diverged on more
than they shared.
Let's extract the common code into a new close_pager_fds() helper, and
move the parts unique to the only to callers to those functions.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
pager.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
@@ -11,29 +11,25 @@staticstructchild_processpager_process=CHILD_PROCESS_INIT;staticconstchar*pager_program;-staticvoidwait_for_pager(intin_signal)+staticvoidclose_pager_fds(void){-if(!in_signal){-fflush(stdout);-fflush(stderr);-}/* signal EOF to pager */close(1);close(2);-if(in_signal)-finish_command_in_signal(&pager_process);-else-finish_command(&pager_process);}staticvoidwait_for_pager_atexit(void){-wait_for_pager(0);+fflush(stdout);+fflush(stderr);+close_pager_fds();+finish_command(&pager_process);}staticvoidwait_for_pager_signal(intsigno){-wait_for_pager(1);+close_pager_fds();+finish_command_in_signal(&pager_process);sigchain_pop(signo);raise(signo);}
Add braces to an "if" block in the wait_or_whine() function. This
isn't needed now, but will make a subsequent commit easier to read.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
run-command.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Add tests for how git behaves when the pager itself exits with
non-zero, as well as for us exiting with 141 when we're killed with
SIGPIPE due to the pager not consuming its output.
There is some recent discussion[1] about these semantics, but aside
from what we want to do in the future, we should have a test for the
current behavior.
This test construct is stolen from 7559a1be8a0 (unblock and unignore
SIGPIPE, 2014-09-18). The reason not to make the test itself depend on
the MINGW prerequisite is to make a subsequent commit easier to read.
1. https://lore.kernel.org/git/87o8h4omqa.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t7006-pager.sh | 82 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 82 insertions(+)
When git invokes a pager that exits with non-zero the common case is
that we'll already return the correct SIGPIPE failure from git itself,
but the exit code logged in trace2 has always been incorrectly
reported[1]. Fix that and log the correct exit code in the logs.
Since this gives us something to test outside of our recently-added
tests needing a !MINGW prerequisite, let's refactor the test to run on
MINGW and actually check for SIGPIPE outside of MINGW.
The wait_or_whine() is only called with a true "in_signal" from from
finish_command_in_signal(), which in turn is only used in pager.c.
The "in_signal && !WIFEXITED(status)" case is not covered by
tests. Let's log the default -1 in that case for good measure.
1. The incorrect logging of the exit code in was seemingly copy/pasted
into finish_command_in_signal() in ee4512ed481 (trace2: create new
combined trace facility, 2019-02-22)
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
run-command.c | 4 +++-
t/t7006-pager.sh | 60 +++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 57 insertions(+), 7 deletions(-)
As discussed on-list starting with [1] I don't think this patch makes
sense, but this "passes tests", at least on Debian with glibc, and is
food for thought for those who like the approach of git not
propagating the pager-induced SIGPIPE in git's own exit code.
The exit() here in wait_for_pager_atexit() isn't portable though[2],
we could probably use _exit(1) instead, but then we're going to
abruptly put a stop to further atexit handler processing. We're far
from the only one, tempfile.c, run-command.c, gc.c etc. all rely on
it, and that's just the git.git code.
If we drop the "if (code)" condition we can see that our pager exit
code will override the exit code of other commands in t7006-pager.sh,
causing numerous tests to fail. Of course if we don't do that all
tests pass.
But that experiment suggests regressions introduced here that we just
don't have good test coverage for. I.e. we're running code before the
atexit() here which expects to exit() with a given status code, and
we're clobbering it with ours because the pager also happened to fail
as we were exiting.
So a real implementation of this would, I think, have to at least:
A. Refactor all use of atexit() to use some git-specific registry,
hard assert somehow that we're never going to have atexit() by
anything else (a library we use might call it).
B. Because we used some atexit() wrapper API we'd know if we were in
the last atexit() handler, which would need to re-evaluate the
decision about the "real" exit code.
C. We could not call exit() anywhere, but would have to make a
git_exit() wrapper. We'd then assign the desired exit code to a
global variable, and then only override our "real" non-zero exit
code with the pager's non-zero, in cases where the pager also
failed.
D. I haven't found whether calling _exit() in the atexit() handler
even has defined behavior, but in any case using it would
short-circuit the documented program exit behavior defined in the
C standard, of which calling atexit() handlers is just the first
step.
1. https://lore.kernel.org/git/8735yhq3lc.fsf@evledraar.gmail.com/
2. https://pubs.opengroup.org/onlinepubs/009695399/functions/exit.html
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
pager.c | 10 ++++++++--
t/t7006-pager.sh | 8 ++++----
2 files changed, 12 insertions(+), 6 deletions(-)
From: Denton Liu <hidden> Date: 2021-02-02 08:51:06
On Tue, Feb 02, 2021 at 02:59:58AM +0100, Ævar Arnfjörð Bjarmason wrote:
This test construct is stolen from 7559a1be8a0 (unblock and unignore
SIGPIPE, 2014-09-18). The reason not to make the test itself depend on
the MINGW prerequisite is to make a subsequent commit easier to read.
@@ -656,4 +656,86 @@ test_expect_success TTY 'git tag with auto-columns ' 'test_cmpexpectactual'+test_expect_successTTY'git returns SIGPIPE on early pager exit''+test_when_finished"rm pager-used"&&+test_configcore.pager">pager-used; head -n 1; exit 0"&&
I may be missing something but this code seems racy, especially since
the history is relatively short at this point. It seems like it's
plausible for git log to be able to dump its output entirely before the
pager part even runs. In that case, it'd fail due to success being its
exit code since it wouldn't be killed by SIGPIPE.
This is what my `test-tool pager` approach was hoping to prevent since
that would guarantee a SIGPIPE.
Sidenote, going back to 7559a1be8a0 (unblock and unignore SIGPIPE,
2014-09-18), it seems like those tests are also racy since it's
theoretically possible for all of the output to be produced before the
piped command gets to it. However, in that case, they're producing a
huge amount of output so this raciness seems mostly academic.
Thanks,
Denton
Am 01.02.21 um 16:44 schrieb Ævar Arnfjörð Bjarmason:
quoted
On Mon, Feb 01 2021, Vincent Lefevre wrote:
quoted
On 2021-02-01 13:10:21 +0100, Ævar Arnfjörð Bjarmason:
quoted
So we've got the SIGPIPE to indicate the output wasn't fully
consumed.
But the user doesn't care: he quit the pager because he didn't
need more output. So there is no need to signal that the output
wasn't fully consumed. The user already knew that before quitting
the pager!
As noted above, this is assuming way too much about the functionality of
the pager command. We can get a SIGPIPE without the user's intent in
this way. Consider e.g. piping to some remote system via netcat.
That assumption is warranted, IMO. Aren't _you_ stretching the meaning
of "pager" too far here? A pager is intended for presentation to the
user. If someone plays games with it, they should know what they get.
From: Vincent Lefevre <hidden> Date: 2021-02-03 15:27:33
On 2021-02-01 16:44:24 +0100, Ævar Arnfjörð Bjarmason wrote:
And then whether it makes sense to ignore SIGPIPE for all users, or
e.g. if it's some opt-in setting in some situations that users might
want to turn on because they're aware of how their pager behaves and
want to work around some zsh mode.
AFAIK, SIGPIPE exists for the following reason. Most programs that
generate output are not written to specifically handle pipes. So,
if SIGPIPE did not exist, there would be 2 kinds of behavior:
1. The program doesn't check for errors, and still outputs data,
wasting time and resources as output will be ignored.
2. The program sees that the write() failed and terminates with
an error message. However, in most cases, such a failure is
not an error: the consumer has terminated either because it
no longer needs any input (e.g. with the "head" utility or a
pager), or because it has terminated abnormally, in which case
the real error is on the side of the consumer. So, the error
message from the LHS of the pipe would be annoying.
SIGPIPE solves this issue: the program is simply killed with SIGPIPE.
In a shell, one gets a non-zero exit code (128 + 13) due to the
signal, but as being on the left-hand side of the pipe, such a
non-zero exit code is normally not reported, so that this will not
annoy the user.
Note 1: Non-zero exit codes from right-hand side are not reported
either by most shells, but zsh can report them, and this is very
useful for developers, as programs may fail with a non-zero exit
code but without an error message. (Reports may also be done by
looking at the standard $? in some hook.)
Note 2: Failures on the left-hand side are less interesting in practice
and generally ignored, at least for commands run in interactive shells.
For scripts, there are various (non-simple) ways to handle them.
Now, I think that in the case (like Git) a program creates a pipe,
it should use its knowledge to handle SIGPIPE / EPIPE. Either this
is regarded as an error because the full output is *always expected*
to be read, in which case there should be an error message in addition
to the usual non-zero exist status (not necessarily 141), or this is
regarded as OK (if there is a real failure, this is on the side of
the consumer). In the case of Git, the consumer is documented to be
a pager, which obviously may not read the full output (e.g. for the
GCC repository, "git log" returns more than 3 million lines, back to
the year 1988, while one is generally interested in the latest changes
only). If the user wants to pipe to something else, he can always use
an explicit pipe.
--
Vincent Lefèvre [off-list ref] - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
From: Johannes Sixt <hidden> Date: 2021-02-03 17:12:44
Am 03.02.21 um 03:48 schrieb Ævar Arnfjörð Bjarmason:
On Mon, Feb 01 2021, Johannes Sixt wrote:
quoted
Am 01.02.21 um 16:44 schrieb Ævar Arnfjörð Bjarmason:
quoted
On Mon, Feb 01 2021, Vincent Lefevre wrote:
quoted
On 2021-02-01 13:10:21 +0100, Ævar Arnfjörð Bjarmason:
quoted
So we've got the SIGPIPE to indicate the output wasn't fully
consumed.
But the user doesn't care: he quit the pager because he didn't
need more output. So there is no need to signal that the output
wasn't fully consumed. The user already knew that before quitting
the pager!
As noted above, this is assuming way too much about the functionality of
the pager command. We can get a SIGPIPE without the user's intent in
this way. Consider e.g. piping to some remote system via netcat.
That assumption is warranted, IMO. Aren't _you_ stretching the meaning
of "pager" too far here? A pager is intended for presentation to the
user. If someone plays games with it, they should know what they get.
A pager in any form is fair game. That point is that it is an
*interactive* form of presentation. But you should not use git's pager
as data post-processing facility; that would stretch the meaning of
"pager" too far, and we do not have cater for such abuse of the feature.
-- Hannes
On 2021-02-01 16:44:24 +0100, Ævar Arnfjörð Bjarmason wrote:
quoted
And then whether it makes sense to ignore SIGPIPE for all users, or
e.g. if it's some opt-in setting in some situations that users might
want to turn on because they're aware of how their pager behaves and
want to work around some zsh mode.
AFAIK, SIGPIPE exists for the following reason. Most programs that
generate output are not written to specifically handle pipes. So,
if SIGPIPE did not exist, there would be 2 kinds of behavior:
1. The program doesn't check for errors, and still outputs data,
wasting time and resources as output will be ignored.
2. The program sees that the write() failed and terminates with
an error message. However, in most cases, such a failure is
not an error: the consumer has terminated either because it
no longer needs any input (e.g. with the "head" utility or a
pager), or because it has terminated abnormally, in which case
the real error is on the side of the consumer. So, the error
message from the LHS of the pipe would be annoying.
SIGPIPE solves this issue: the program is simply killed with SIGPIPE.
In a shell, one gets a non-zero exit code (128 + 13) due to the
signal, but as being on the left-hand side of the pipe, such a
non-zero exit code is normally not reported, so that this will not
annoy the user.
Note 1: Non-zero exit codes from right-hand side are not reported
either by most shells, but zsh can report them, and this is very
useful for developers, as programs may fail with a non-zero exit
code but without an error message. (Reports may also be done by
looking at the standard $? in some hook.)
Note 2: Failures on the left-hand side are less interesting in practice
and generally ignored, at least for commands run in interactive shells.
For scripts, there are various (non-simple) ways to handle them.
Now, I think that in the case (like Git) a program creates a pipe,
it should use its knowledge to handle SIGPIPE / EPIPE. Either this
is regarded as an error because the full output is *always expected*
to be read, in which case there should be an error message in addition
to the usual non-zero exist status (not necessarily 141), or this is
regarded as OK (if there is a real failure, this is on the side of
the consumer). In the case of Git, the consumer is documented to be
a pager, which obviously may not read the full output (e.g. for the
GCC repository, "git log" returns more than 3 million lines, back to
the year 1988, while one is generally interested in the latest changes
only). If the user wants to pipe to something else, he can always use
an explicit pipe.
SIGPIPE exists because *nix systems are composable, so you can make
something useful by stringing together unrelated programs via files and
pipes, and with exit codes and signal mostly everyone's happy.
I follow what you're saying right until the point of arguing that
because either your shell or POSIX shells in general have decided to
either be sloppy or overzelous in how they show you some
information. That we should use their behavior as a guide in
pro-actively suppressing our own exit code.
And that's not because I think (to the tune of Monty Python...) that
every exit code is sacret. It's because when we invoke a pager handing
it data is *the* thing we're doing. If we can fully hand it over, great,
if not, let's tell the user with the appropriate exit code.
Yeah it's annoying with zsh's PRINT_EXIT_VALUE, but the same is true of
POSIX "set -e". Not every shell option is meant for general use. The
shell is very forgiving of things like pipe failures by default for a
reason.
But "connected to a terminal" (isatty(1)) and "invoked by Vincent's zsh
instance" aren't the same thing. And I think it makes sense to be
conservative in preserving exit codes.
In the early days of git complaining about "Broken pipe" in the exact
same scenario was the default behavior of bash's overzelous reporting,
as you can read about starting here:
https://lore.kernel.org/git/?q=%22Broken+pipe%22+bash&o=-1
AFAICT that changed by default in bash 3.1, released in 2005-12-08. It
was a FAQ in the early days, now nobody cares.
Have you reported this as a bug to zsh? I think it's likely that the
motivation for wanting this squashed in git is going to be as transitory
as bash's once-default verbosity was.
I also tested "hg log", it behaves the same way, although interestingly
they cast SIGPIPE to 255 in their exit code.
From: Vincent Lefevre <hidden> Date: 2021-02-04 15:49:52
On 2021-02-04 01:14:10 +0100, Ævar Arnfjörð Bjarmason wrote:
Have you reported this as a bug to zsh?
I repeat: there is no bug in zsh. It is my choice to output the
exit status when it is non-zero because I want to know when the
command I've typed fails. This is useful in practice. Ignoring the
specific value 141 (corresponding to SIGPIPE) is not a solution
because it can be a real failure with some utilities. BTW, the
association with a signal like SIGPIPE is just a convention; apart
from that, 141 is a non-zero status like others (in particular
with programs that have not been written for POSIX).
For instance, in any shell:
$ sh -c "echo foo; exit 141"
foo
$ echo $?
141
while no broken pipe is involved here. How would you differentiate
such a failure from a broken pipe?
I also tested "hg log", it behaves the same way, although interestingly
they cast SIGPIPE to 255 in their exit code.
I get 141, like with git:
$ hg log
$ echo $?
141
--
Vincent Lefèvre [off-list ref] - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
From: Johannes Sixt <hidden> Date: 2021-02-05 07:48:40
Am 02.02.21 um 02:59 schrieb Ævar Arnfjörð Bjarmason:
Add tests for how git behaves when the pager itself exits with
non-zero, as well as for us exiting with 141 when we're killed with
SIGPIPE due to the pager not consuming its output.
There is some recent discussion[1] about these semantics, but aside
from what we want to do in the future, we should have a test for the
current behavior.
This test construct is stolen from 7559a1be8a0 (unblock and unignore
SIGPIPE, 2014-09-18). The reason not to make the test itself depend on
the MINGW prerequisite is to make a subsequent commit easier to read.
At least for my Windows build, the MINGW games do not make a difference:
The test is skipped anyway due to the unsatisfied TTY prerequisite.
From: Johannes Sixt <hidden> Date: 2021-02-05 07:59:31
Am 02.02.21 um 03:00 schrieb Ævar Arnfjörð Bjarmason:
When git invokes a pager that exits with non-zero the common case is
that we'll already return the correct SIGPIPE failure from git itself,
but the exit code logged in trace2 has always been incorrectly
reported[1]. Fix that and log the correct exit code in the logs.
There's a more severe problem here, not with your patch, but with trace2
in general: it invokes async-signal-unsafe functions from a signal
handler, in particular, realloc, vsnprintf, gettimeofday, localtime_r
(and probably a lot more) via fn_child_exit_fl of trace2/tr2_tgt_normal.c
Is that something that we should care about?
-- Hannes