From: Junio C Hamano <hidden> Date: 2016-06-15 22:55:39
Jeff King [off-list ref] writes:
I have two reservations with this patch:
1. We are ignoring SIGPIPE all the time. For an alias that is calling
"log", that is fine. But if pack-objects dies on the server side,
seeing that it died from SIGPIPE is useful data, and we are
squelching that. Maybe callers of run-command should have to pass
an "ignore SIGPIPE" flag?
What should this do:
GIT_PAGER='head -n 1' git -p -c alias.o='!cat longfile' o
Should it behave just like
cat longfile | head -n 1
or should it behave differently?
I am having a feeling that whatever external command given as the
value of alias.$cmd should choose what error status it wants to be
reported.
2. The die_errno in handle_alias is definitely wrong. Even if we want
to print a message for signal death, showing errno is bogus unless
the return value was -1. But is it the right thing to just pass the
negative value straight to exit()? It works, but it is depending on
the fact that (unsigned char)(ret & 0xff) behaves in a certain way
(i.e., that we are on a twos-complement platform, and -13 becomes
141).
Isn't that what POSIX.1 guarantees us, though?
The value of status may be 0, EXIT_SUCCESS, EXIT_FAILURE, or any
other value, though only the least significant 8 bits (that is,
status & 0377) shall be available to a waiting parent process.
From: Jeff King <hidden> Date: 2016-06-15 22:55:39
On Fri, Jan 04, 2013 at 02:20:52PM -0800, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
I have two reservations with this patch:
1. We are ignoring SIGPIPE all the time. For an alias that is calling
"log", that is fine. But if pack-objects dies on the server side,
seeing that it died from SIGPIPE is useful data, and we are
squelching that. Maybe callers of run-command should have to pass
an "ignore SIGPIPE" flag?
What should this do:
GIT_PAGER='head -n 1' git -p -c alias.o='!cat longfile' o
Should it behave just like
cat longfile | head -n 1
or should it behave differently?
With respect to error messages, I'd think they should behave the same.
But they don't necessarily. The latter does not print any message at
all. But consider this version of the former:
$ cat foo
#!/bin/sh
exec cat longfile
$ git -c alias.o='!./foo' o | head -n 1
error: ./foo died of signal 13
fatal: While expanding alias 'o': './foo': Success
So why don't we see that message more often? There are two reasons.
One reason is that we piped it ourselves here. When git pipes to the
pager, it sends stderr along the same channel. So even if you did:
GIT_PAGER='head -n 1' git -p -c alias.o='!./foo' o
git writes the error, but it goes to the pager, which has already ended
(since that is what caused the SIGPIPE in the first place). But imagine
that your sub-command is actually invoking git itself, and it is the
sub-git which starts the pager. Meaning the outer git wrapper's stderr
is _not_ redirected. Like this:
$ cat foo
#!/bin/sh
exec git log -p
$ GIT_PAGER='head -n 1' git -c alias.o='!./foo' o
error: ./foo died of signal 13
fatal: While expanding alias 'o': './foo': Success
The second reason is that most shells will "eat" the SIGPIPE exit
status, and convert it into a high, positive error code. You can see
that effect here:
$ GIT_PAGER='head -n 1' git log -p
$ echo $?
141
And since we execute aliases via the shell, we end up seeing the
converted exit code (141) instead of the signal death. _Unless_ we
optimize out the shell call (which is why we see it by putting the
command inside "./foo", which git executes directly, but not when we
give the literal "cat longfile", which git will feed to the shell).
Or at least that's _one_ way to see it. Another way is to use a shell
that does not do such conversion. Setting SHELL_PATH to zsh seems to do
so, and I think that is how Bart ran into it (my patch is a followup to
a Google+ posting he made).
I am having a feeling that whatever external command given as the
value of alias.$cmd should choose what error status it wants to be
reported.
I suppose. It means that our "do not run the shell if there are no
meta-characters" optimization is leaky, since the exit code behaves
differently depending on whether we run the shell (and depending on your
exact shell). One solution would be to fix that leakiness, and if
use_shell is in effect for run-command, to convert a signal death into
the value that the shell would otherwise give us.
In fact, I really wonder if this code from wait_or_whine is actually
correct:
code = WTERMSIG(status);
/*
* This return value is chosen so that code & 0xff
* mimics the exit code that a POSIX shell would report for
* a program that died from this signal.
*/
code -= 128;
If we get signal 13, we end up with -115, because "code" is signed. When
the lower 8 bits are taken, and then converted into an unsigned value,
we get 141: the shell value.
But do we really want to return a negative value here? Should this
instead be:
code += 128
which yields the same code when fed to exit, but internally looks like
the shell version to us? So we get a consistent result whether the shell
was actually used or not.
That makes more sense to me, and would mean that whether we converted
the signal number or whether it was done by a subshell, it looks the
same to us. Callers which care about signals (e.g., the recent changes
to launch_editor to detect SIGINT) would have to be adjusted. But I
think it fixes an obscure bug there. Right now launch_editor is actually
checking the whether the _shell_ died from a signal, and will fail to
notice when an editor invoked by the shell is killed by those signals
(this would be pretty rare, though, because typically SIGINT is
delivered to the shell as well as the editor).
This would also fix the code in handle_alias. It looks for a negative
return code from run_command as the sign that there was an internal
error running the command, and that errno would be valid. But right now
a negative return can also come from signal death.
quoted
2. The die_errno in handle_alias is definitely wrong. Even if we want
to print a message for signal death, showing errno is bogus unless
the return value was -1. But is it the right thing to just pass the
negative value straight to exit()? It works, but it is depending on
the fact that (unsigned char)(ret & 0xff) behaves in a certain way
(i.e., that we are on a twos-complement platform, and -13 becomes
141).
Isn't that what POSIX.1 guarantees us, though?
The value of status may be 0, EXIT_SUCCESS, EXIT_FAILURE, or any
other value, though only the least significant 8 bits (that is,
status & 0377) shall be available to a waiting parent process.
Sort of. I was worried about:
1. Not-quite-POSIX platforms (i.e., Windows). But JSixt has said that
is fine, because we already have a compatibility wrapper which
masks off only the low byte.
2. We are relying on the specifics of how a negative value is treated
by exit(). The cast I gave above is guaranteed to work in standard
C, but we do not know the implementation details of exit(). Still,
I think that is being overly paranoid. Any sane implementation will
do what we expect.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:55:39
On Sat, Jan 05, 2013 at 09:03:16AM -0500, Jeff King wrote:
In fact, I really wonder if this code from wait_or_whine is actually
correct:
code = WTERMSIG(status);
/*
* This return value is chosen so that code & 0xff
* mimics the exit code that a POSIX shell would report for
* a program that died from this signal.
*/
code -= 128;
After looking at it some more, it is correct, but I think we could make
life slightly easier for callers. See the patch below. I've tried to
re-state the somewhat rambling argument from my previous email;
hopefully it makes sense.
-- >8 --
Subject: [PATCH] run-command: encode signal death as a positive integer
When a sub-command dies due to a signal, we encode the
signal number into the numeric exit status as "signal -
128". This is easy to identify (versus a regular positive
error code), and when cast to an unsigned integer (e.g., by
feeding it to exit), matches what a POSIX shell would return
when reporting a signal death in $? or through its own exit
code.
So we have a negative value inside the code, but once it
passes across an exit() barrier, it looks positive (and any
code we receive from a sub-shell will have the positive
form). E.g., death by SIGPIPE (signal 13) will look like
-115 to us in inside git, but will end up as 141 when we
call exit() with it. And a program killed by SIGPIPE but run
via the shell will come to us with an exit code of 141.
Unfortunately, this means that when the "use_shell" option
is set, we need to be on the lookout for _both_ forms. We
might or might not have actually invoked the shell (because
we optimize out some useless shell calls). If we didn't invoke
the shell, we will will see the sub-process's signal death
directly, and run-command converts it into a negative value.
But if we did invoke the shell, we will see the shell's
128+signal exit status. To be thorough, we would need to
check both, or cast the value to an unsigned char (after
checking that it is not -1, which is a magic error value).
Fortunately, most callsites do not care at all whether the
exit was from a code or from a signal; they merely check for
a non-zero status, and sometimes propagate the error via
exit(). But for the callers that do care, we can make life
slightly easier by just using the consistent positive form.
This actually fixes two minor bugs:
1. In launch_editor, we check whether the editor died from
SIGINT or SIGQUIT. But we checked only the negative
form, meaning that we would fail to notice a signal
death exit code which was propagated through the shell.
2. In handle_alias, we assume that a negative return value
from run_command means that errno tells us something
interesting (like a fork failure, or ENOENT).
Otherwise, we simply propagate the exit code. Negative
signal death codes confuse us, and we print a useless
"unable to run alias 'foo': Success" message. By
encoding signal deaths using the positive form, the
existing code just propagates it as it would a normal
non-zero exit code.
The downside is that callers of run_command can no longer
differentiate between a signal received directly by the
sub-process, and one propagated. However, no caller
currently cares, and since we already optimize out some
calls to the shell under the hood, that distinction is not
something that should be relied upon by callers.
Signed-off-by: Jeff King <redacted>
---
Documentation/technical/api-run-command.txt | 6 ++----
editor.c | 2 +-
run-command.c | 2 +-
3 files changed, 4 insertions(+), 6 deletions(-)
@@ -55,10 +55,8 @@ The functions above do the following: non-zero. . If the program terminated due to a signal, then the return value is the- signal number - 128, ie. it is negative and so indicates an unusual- condition; a diagnostic is printed. This return value can be passed to- exit(2), which will report the same code to the parent process that a- POSIX shell's $? would report for a program that died from the signal.+ signal number + 128, ie. the same value that a POSIX shell's $? would+ report. A diagnostic is printed. `start_async`::
From: Johannes Sixt <hidden> Date: 2016-06-15 22:55:39
Am 05.01.2013 15:49, schrieb Jeff King:
On Sat, Jan 05, 2013 at 09:03:16AM -0500, Jeff King wrote:
quoted
In fact, I really wonder if this code from wait_or_whine is actually
correct:
code = WTERMSIG(status);
/*
* This return value is chosen so that code & 0xff
* mimics the exit code that a POSIX shell would report for
* a program that died from this signal.
*/
code -= 128;
After looking at it some more, it is correct, but I think we could make
life slightly easier for callers. See the patch below. I've tried to
re-state the somewhat rambling argument from my previous email;
hopefully it makes sense.
-- >8 --
Subject: [PATCH] run-command: encode signal death as a positive integer
When a sub-command dies due to a signal, we encode the
signal number into the numeric exit status as "signal -
128". This is easy to identify (versus a regular positive
error code), and when cast to an unsigned integer (e.g., by
feeding it to exit), matches what a POSIX shell would return
when reporting a signal death in $? or through its own exit
code.
So we have a negative value inside the code, but once it
passes across an exit() barrier, it looks positive (and any
code we receive from a sub-shell will have the positive
form). E.g., death by SIGPIPE (signal 13) will look like
-115 to us in inside git, but will end up as 141 when we
call exit() with it. And a program killed by SIGPIPE but run
via the shell will come to us with an exit code of 141.
Unfortunately, this means that when the "use_shell" option
is set, we need to be on the lookout for _both_ forms. We
might or might not have actually invoked the shell (because
we optimize out some useless shell calls). If we didn't invoke
the shell, we will will see the sub-process's signal death
directly, and run-command converts it into a negative value.
But if we did invoke the shell, we will see the shell's
128+signal exit status. To be thorough, we would need to
check both, or cast the value to an unsigned char (after
checking that it is not -1, which is a magic error value).
Fortunately, most callsites do not care at all whether the
exit was from a code or from a signal; they merely check for
a non-zero status, and sometimes propagate the error via
exit(). But for the callers that do care, we can make life
slightly easier by just using the consistent positive form.
This actually fixes two minor bugs:
1. In launch_editor, we check whether the editor died from
SIGINT or SIGQUIT. But we checked only the negative
form, meaning that we would fail to notice a signal
death exit code which was propagated through the shell.
2. In handle_alias, we assume that a negative return value
from run_command means that errno tells us something
interesting (like a fork failure, or ENOENT).
Otherwise, we simply propagate the exit code. Negative
signal death codes confuse us, and we print a useless
"unable to run alias 'foo': Success" message. By
encoding signal deaths using the positive form, the
existing code just propagates it as it would a normal
non-zero exit code.
The downside is that callers of run_command can no longer
differentiate between a signal received directly by the
sub-process, and one propagated. However, no caller
currently cares, and since we already optimize out some
calls to the shell under the hood, that distinction is not
something that should be relied upon by callers.
The idea was initially to regard death by signal as an internal error.
But since (1) there are no callers that are really interested in the
difference and (2) we get it wrong in the shell case anyway, this change
makes total sense.
Acked-by: Johannes Sixt <redacted>
@@ -55,10 +55,8 @@ The functions above do the following: non-zero. . If the program terminated due to a signal, then the return value is the- signal number - 128, ie. it is negative and so indicates an unusual- condition; a diagnostic is printed. This return value can be passed to- exit(2), which will report the same code to the parent process that a- POSIX shell's $? would report for a program that died from the signal.+ signal number + 128, ie. the same value that a POSIX shell's $? would+ report. A diagnostic is printed. `start_async`::
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:55:39
Hi,
Jeff King wrote:
When a sub-command dies due to a signal, we encode the
signal number into the numeric exit status as "signal -
128".
[...]
So we have a negative value inside the code, but once it
passes across an exit() barrier, it looks positive (and any
code we receive from a sub-shell will have the positive
form).
[...]
Unfortunately, this means that when the "use_shell" option
is set, we need to be on the lookout for _both_ forms.
[...]
for the callers that do care, we can make life
slightly easier by just using the consistent positive form.
t/test-terminal.perl imitates the same logic. It doesn't check for
anything other than whether the exit status is 0, but maybe it would
be worth squashing in the below as a futureproofing measure
nonetheless.
Aside from the launch_editor bugfix, the only observable effects of
the above patch I can find are some changed error messages:
error: external filter cat failed -126
-> error: external filter cat failed 130
warning: svnrdump, returned -126
-> warning: svnrdump, returned 130
Those messages are equally senseless before and after the patch, so
for what it's worth,
Reviewed-by: Jonathan Nieder <redacted>
Thanks.
t/test-terminal.perl imitates the same logic. It doesn't check for
anything other than whether the exit status is 0, but maybe it would
be worth squashing in the below as a futureproofing measure
nonetheless.
Yeah, I think so. As you say, it does not matter, but it makes sense to
keep our conventions consistent.
Aside from the launch_editor bugfix, the only observable effects of
the above patch I can find are some changed error messages:
error: external filter cat failed -126
-> error: external filter cat failed 130
warning: svnrdump, returned -126
-> warning: svnrdump, returned 130
Those messages are equally senseless before and after the patch, so
for what it's worth,
Thanks. I agree that change isn't a big deal (I would argue the positive
return is slightly more coherent as it matches what the shell would
report, but I really think user-facing errors should probably not even
mention the exact exit code, as it is just noise most of the time, and
we already complain about signals in wait_or_whine).
I did try auditing the callers of finish_command (and run_command) to
make sure I wasn't regressing anybody, but there are a lot of call
sites. In some cases we immediately say:
if (finish_command(&child))
die("failed...");
which is obviously unaffected. But in many cases we pass the exit code
up through several levels. It usually just ends up in exit() or being
collapsed to an error boolean, which is fine, but I may have missed a
spot where it matters. I'd expecting cooking this patch for a while
would flush out any I missed.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:55:39
Jeff King wrote:
I'd expecting cooking this patch for a while
would flush out any I missed.
Heh, probably not. ;-) But I tried to examine all the callsites (and
only found the two messages I mentioned), and among the reviewers, I'm
guessing we hit them all.
Ciao,
Jonathan