Johannes Sixt [off-list ref] writes:
quoted
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.
I guess we are then in agreement---I do think it makes sense to send
the true exit code from the pager as the exit code from the pager to
the trace output, which is what the early part of Ævar's patch does,
but I do not think the exit code of the pager should affect the exit
code from "git log" as a whole.
Am 02.02.21 um 23:21 schrieb Junio C Hamano:
Johannes Sixt [off-list ref] writes:
quoted
quoted
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.
I guess we are then in agreement---I do think it makes sense to send
the true exit code from the pager as the exit code from the pager to
the trace output, which is what the early part of Ævar's patch does,
but I do not think the exit code of the pager should affect the exit
code from "git log" as a whole.
Then we do not agree. The exit code of `git log | less` is ignored, and
I regard `git -p log` just as a short-hand for that.
That said, I don't care a lot about the exit code. When a pager is in
the game, we are talking about an interactive command invocation, and
what the exit code of that is, is irrelevant in practice.
The only thing I care is that git does not die due to a SIGPIPE when the
pager is closed early.
-- Hannes
On 2021-02-03 18:07:02 +0100, Johannes Sixt wrote:
Then we do not agree. The exit code of `git log | less` is ignored,
and I regard `git -p log` just as a short-hand for that.
If git didn't have the -p feature, "git log | less" would just be
one way to get a pager. For instance, I use an alias that does
pager-wrapper grep --color=always --line-buffered -E
which sends the grep output to a pager, and returns the exit status
of the pager if non-zero, otherwise the exit status of grep, except
when this is SIGPIPE. An unavoidable issue is that if there has been
an error in grep, I could still get the exit status 0. But as a user,
this is a choice I have done by quitting the pager before letting
grep terminate in the normal way (which could have been costly) so
that it could report the error, say, about unreadable files with a
recursive grep (grep -r).
So Git could do the same thing, and even better, because it controls
its own exit status: if there has been an error in the generation of
the Git output (for instance, I can see that there is a --check option
of "git log" that can trigger an error), then this error should be
reported (with a non-zero exit status) after the pager is quit, as if
a pager were not used. Otherwise, terminate with the exit status of
the pager.
--
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)