[PATCH] run-command: simplify wait_or_whine

[PATCH] run-command: simplify wait_or_whine

[Felipe Contreras]

Nobody is checking for specific error codes; it's the errno that's
important.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 run-command.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/run-command.c b/run-command.c
index 1b32a12..e54e943 100644
--- a/run-command.c
+++ b/run-command.c
@@ -244,21 +244,11 @@ static int wait_or_whine(pid_t pid, const char *argv0)
 		code = WTERMSIG(status);
 		if (code != SIGINT && code != SIGQUIT)
 			error("%s died of signal %d", argv0, code);
-		/*
-		 * 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;
 	} else if (WIFEXITED(status)) {
 		code = WEXITSTATUS(status);
-		/*
-		 * Convert special exit code when execvp failed.
-		 */
-		if (code == 127) {
-			code = -1;
+		/* convert special exit code when execvp failed. */
+		if (code == 127)
 			failed_errno = ENOENT;
-		}
 	} else {
 		error("waitpid is confused (%s)", argv0);
 	}
-- 
1.8.3.358.g5a91d05

Re: [PATCH] run-command: simplify wait_or_whine

[Duy Nguyen]

On Sat, Jun 1, 2013 at 8:51 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Nobody is checking for specific error codes; it's the errno that's
> important.

Have you just disregarded the in-code comment you just removed with
one statement? Did you check all its callers?

>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  run-command.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 1b32a12..e54e943 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -244,21 +244,11 @@ static int wait_or_whine(pid_t pid, const char *argv0)
>                 code = WTERMSIG(status);
>                 if (code != SIGINT && code != SIGQUIT)
>                         error("%s died of signal %d", argv0, code);
> -               /*
> -                * 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;
>         } else if (WIFEXITED(status)) {
>                 code = WEXITSTATUS(status);
> -               /*
> -                * Convert special exit code when execvp failed.
> -                */
> -               if (code == 127) {
> -                       code = -1;
> +               /* convert special exit code when execvp failed. */
> +               if (code == 127)
>                         failed_errno = ENOENT;
> -               }
>         } else {
>                 error("waitpid is confused (%s)", argv0);
>         }
> --
> 1.8.3.358.g5a91d05
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
Duy

Re: [PATCH] run-command: simplify wait_or_whine

[Felipe Contreras]

On Sat, Jun 1, 2013 at 9:03 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sat, Jun 1, 2013 at 8:51 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> Nobody is checking for specific error codes; it's the errno that's
>> important.
>
> Have you just disregarded the in-code comment you just removed with
> one statement?

Who cares about the comment? As I said, nobody is checking for those codes.

> Did you check all its callers?

Yes, that's why I said nobody is checking for those codes.

-- 
Felipe Contreras

Re: [PATCH] run-command: simplify wait_or_whine

[Duy Nguyen]

On Sat, Jun 1, 2013 at 9:06 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sat, Jun 1, 2013 at 9:03 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Sat, Jun 1, 2013 at 8:51 PM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>>> Nobody is checking for specific error codes; it's the errno that's
>>> important.
>>
>> Have you just disregarded the in-code comment you just removed with
>> one statement?
>
> Who cares about the comment? As I said, nobody is checking for those codes.

Apparently I do.

>> Did you check all its callers?
>
> Yes, that's why I said nobody is checking for those codes.

Thanks. If would be a few mails less if you stated so in the original message.
--
Duy

Re: [PATCH] run-command: simplify wait_or_whine

[Thomas Rast]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Nobody is checking for specific error codes; it's the errno that's
> important.
[...]
> -		/*
> -		 * 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;

Have you checked the callers?  There are lots of callers of
finish_command(), which returns the value from wait_or_whine()
unmodified.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] run-command: simplify wait_or_whine

[Felipe Contreras]

On Sat, Jun 1, 2013 at 9:08 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sat, Jun 1, 2013 at 9:06 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Sat, Jun 1, 2013 at 9:03 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> On Sat, Jun 1, 2013 at 8:51 PM, Felipe Contreras
>>> <felipe.contreras@gmail.com> wrote:
>>>> Nobody is checking for specific error codes; it's the errno that's
>>>> important.
>>>
>>> Have you just disregarded the in-code comment you just removed with
>>> one statement?
>>
>> Who cares about the comment? As I said, nobody is checking for those codes.
>
> Apparently I do.

Why do you care about code comments that have no relation to reality?

>>> Did you check all its callers?
>>
>> Yes, that's why I said nobody is checking for those codes.
>
> Thanks. If would be a few mails less if you stated so in the original message.

So why did you think I said that that nobody is checking for those codes?

Anyway, apparently somebody added code that checks for the specific
code since I wrote this patch:

1250857 launch_editor: propagate signals from editor to git

To my knowledge this is the only place where the specific number us
actually checked.

-- 
Felipe Contreras

Re: [PATCH] run-command: simplify wait_or_whine

[Duy Nguyen]

On Sat, Jun 1, 2013 at 8:51 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Nobody is checking for specific error codes; it's the errno that's
> important.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  run-command.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 1b32a12..e54e943 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -244,21 +244,11 @@ static int wait_or_whine(pid_t pid, const char *argv0)
>                 code = WTERMSIG(status);
>                 if (code != SIGINT && code != SIGQUIT)
>                         error("%s died of signal %d", argv0, code);
> -               /*
> -                * 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;
>         } else if (WIFEXITED(status)) {

The original commit that introduces this says

    run_command: encode deadly signal number in the return value

    We now write the signal number in the error message if the program
    terminated by a signal. The negative return value is constructed such that
    after truncation to 8 bits it looks like a POSIX shell's $?:

       $ echo 0000 | { git upload-pack .; echo $? >&2; } | :
       error: git-upload-pack died of signal 13
       141

    Previously, the exit code was 255 instead of 141.

So this is part of the interface to the user. With your changes, the
exit code is now different. I tested by force segfaulting upload-pack.
$? returned 11. So NAK.
--
Duy

Re: [PATCH] run-command: simplify wait_or_whine

[Felipe Contreras]

On Sat, Jun 1, 2013 at 9:19 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Nobody is checking for specific error codes; it's the errno that's
>> important.
> [...]
>> -             /*
>> -              * 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;
>
> Have you checked the callers?  There are lots of callers of
> finish_command(), which returns the value from wait_or_whine()
> unmodified.

Yes I did. Most of them simply check that the number is not zero.

However, that was at the time I wrote the patch, and it seems there's
now one instance where the code is checked.

-- 
Felipe Contreras

Re: [PATCH] run-command: simplify wait_or_whine

[Felipe Contreras]

On Sat, Jun 1, 2013 at 9:21 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sat, Jun 1, 2013 at 8:51 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> Nobody is checking for specific error codes; it's the errno that's
>> important.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  run-command.c | 14 ++------------
>>  1 file changed, 2 insertions(+), 12 deletions(-)
>>
>> diff --git a/run-command.c b/run-command.c
>> index 1b32a12..e54e943 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -244,21 +244,11 @@ static int wait_or_whine(pid_t pid, const char *argv0)
>>                 code = WTERMSIG(status);
>>                 if (code != SIGINT && code != SIGQUIT)
>>                         error("%s died of signal %d", argv0, code);
>> -               /*
>> -                * 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;
>>         } else if (WIFEXITED(status)) {
>
> The original commit that introduces this says
>
>     run_command: encode deadly signal number in the return value
>
>     We now write the signal number in the error message if the program
>     terminated by a signal. The negative return value is constructed such that
>     after truncation to 8 bits it looks like a POSIX shell's $?:
>
>        $ echo 0000 | { git upload-pack .; echo $? >&2; } | :
>        error: git-upload-pack died of signal 13
>        141
>
>     Previously, the exit code was 255 instead of 141.
>
> So this is part of the interface to the user. With your changes, the
> exit code is now different. I tested by force segfaulting upload-pack.
> $? returned 11. So NAK.

Yeah, and last year we returned a different code. The world didn't
end, because nobody is checking for the specific code. But if you want
to retain complexity forever, suit yourselves.

commit 709ca730f8e093005cc882bfb86c0ca9c83d345b
Author: Jeff King <peff@peff.net>
Date:   Sat Jan 5 09:49:49 2013 -0500

    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.

    Fix the same logic in t/test-terminal.perl for consistency [jc:
    raised by Jonathan in the discussion].

    Signed-off-by: Jeff King <peff@peff.net>
    Acked-by: Johannes Sixt <j6t@kdbg.org>
    Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

-- 
Felipe Contreras

Re: [PATCH] run-command: simplify wait_or_whine

[Duy Nguyen]

On Sat, Jun 1, 2013 at 9:30 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sat, Jun 1, 2013 at 9:21 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Sat, Jun 1, 2013 at 8:51 PM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
> Yeah, and last year we returned a different code. The world didn't
> end, because nobody is checking for the specific code. But if you want
> to retain complexity forever, suit yourselves.

And that was changed for a reason, compared to this change "because I
like it". I maintain my NAK (not that it matters) until you justify
your change better than "nobody is using it".

> commit 709ca730f8e093005cc882bfb86c0ca9c83d345b
> Author: Jeff King <peff@peff.net>
> Date:   Sat Jan 5 09:49:49 2013 -0500
>
>     run-command: encode signal death as a positive integer
-- 
Duy

Re: [PATCH] run-command: simplify wait_or_whine

[Felipe Contreras]

On Sat, Jun 1, 2013 at 9:36 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sat, Jun 1, 2013 at 9:30 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Sat, Jun 1, 2013 at 9:21 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> On Sat, Jun 1, 2013 at 8:51 PM, Felipe Contreras
>>> <felipe.contreras@gmail.com> wrote:
>> Yeah, and last year we returned a different code. The world didn't
>> end, because nobody is checking for the specific code. But if you want
>> to retain complexity forever, suit yourselves.
>
> And that was changed for a reason, compared to this change "because I
> like it". I maintain my NAK (not that it matters) until you justify
> your change better than "nobody is using it".

Who said the reason was "because I like it"? You don't agree that
making the code simpler and more maintainable is a good reason for any
change?

Anyway, if you care so much about the current behavior, why isn't
there any tests that check for this?

My patch passes *all* the tests.

-- 
Felipe Contreras

Re: [PATCH] run-command: simplify wait_or_whine

[Jeff King]

On Sat, Jun 01, 2013 at 09:30:50AM -0500, Felipe Contreras wrote:

> > The original commit that introduces this says
> >
> >     run_command: encode deadly signal number in the return value
> >
> >     We now write the signal number in the error message if the program
> >     terminated by a signal. The negative return value is constructed such that
> >     after truncation to 8 bits it looks like a POSIX shell's $?:
> >
> >        $ echo 0000 | { git upload-pack .; echo $? >&2; } | :
> >        error: git-upload-pack died of signal 13
> >        141
> >
> >     Previously, the exit code was 255 instead of 141.
> >
> > So this is part of the interface to the user. With your changes, the
> > exit code is now different. I tested by force segfaulting upload-pack.
> > $? returned 11. So NAK.
> 
> Yeah, and last year we returned a different code. The world didn't
> end, because nobody is checking for the specific code. But if you want
> to retain complexity forever, suit yourselves.

Last year we returned a different code from the function that other C
code saw. But what got returned via exit() to exterior programs was
always 141 in the SIGPIPE case, both before and after my 709ca730. That
is explained in the first two paragraphs here:

> commit 709ca730f8e093005cc882bfb86c0ca9c83d345b
> Author: Jeff King <peff@peff.net>
> Date:   Sat Jan 5 09:49:49 2013 -0500
> 
>     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.

Your patch changes the error code that is propagated via exit() in this
case. We cannot know "nobody is checking for the specific code", because
the list of callers is every shell script or program which execs git.
Some of them do care about the exit code. I can give an example of a
case I have that cares, but I do not think it is even important. The
point is that we would be regressing an existing interface, and cannot
know who is broken by it.

-Peff

[PATCH] t0005: test git exit code from signal death

[Jeff King]

On Sat, Jun 01, 2013 at 10:01:49AM -0500, Felipe Contreras wrote:

> Anyway, if you care so much about the current behavior, why isn't
> there any tests that check for this?
> 
> My patch passes *all* the tests.

The test suite has never been (and probably never will be) a complete
specification of git's behavior. Noticing that a desired behavior is not
in the test suite is an opportunity to improve its coverage, not argue
that a change which breaks the desired behavior must therefore be
acceptable.

We could do something like the patch below, with two caveats:

  1. The test immediately before it checks for exit codes other than
     "143" on various platforms. I do not know to what degree we need to
     deal with that here. Git is doing the interpretation here rather
     than the shell, so the ksh case should not matter. But I do not
     know what part of Windows converts the exit code to 3. Do we need
     to be looking for 3? Or 131 (128+3)?

  2. The test detects a total breakage of the exit code, like the one
     caused by your patch. But I do not know if it would reliably detect
     us failing to convert the code at all, as the shell running the
     test harness would presumably convert it to shell-convention
     itself. Getting around that would require a custom C program
     checking the status returned by waitpid().

     On the other hand, perhaps it is not the conversion we care about;
     as long as we end up with 143, we are fine. We just want to make
     sure that signal death is recorded in _one_ of the potential signal
     formats. So we do not care if git or the shell did the conversion,
     as long as we end up with 143. The real breakage is exiting with
     code 15, which is losing information.

-- >8 --
Subject: [PATCH] t0005: test git exit code from signal death

When a sub-process dies with a signal, we convert the exit
code to the shell convention of 128+sig. Callers of git may
be relying on this behavior, so let's make sure it does not
break.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0005-signals.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh
index 93e58c0..63f9764 100755
--- a/t/t0005-signals.sh
+++ b/t/t0005-signals.sh
@@ -20,4 +20,11 @@ test_expect_success 'sigchain works' '
 	test_cmp expect actual
 '
 
+test_expect_success 'signals are propagated using shell convention' '
+	# we use exec here to avoid any sub-shell interpretation
+	# of the exit code
+	git config alias.sigterm "!exec test-sigchain" &&
+	test_expect_code 143 git sigterm
+'
+
 test_done
-- 
1.8.3.rc1.2.g12db477

Re: [PATCH] run-command: simplify wait_or_whine

[Felipe Contreras]

On Sat, Jun 1, 2013 at 12:01 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Jun 01, 2013 at 09:30:50AM -0500, Felipe Contreras wrote:

>> commit 709ca730f8e093005cc882bfb86c0ca9c83d345b
>> Author: Jeff King <peff@peff.net>
>> Date:   Sat Jan 5 09:49:49 2013 -0500
>>
>>     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.
>
> Your patch changes the error code that is propagated via exit() in this
> case. We cannot know "nobody is checking for the specific code", because
> the list of callers is every shell script or program which execs git.
> Some of them do care about the exit code. I can give an example of a
> case I have that cares, but I do not think it is even important. The
> point is that we would be regressing an existing interface, and cannot
> know who is broken by it.

Of course we can know, by going forward with the change, and quite
often that's the only way to know for sure.

But if it's true what you said that we haven't changed what the
process returns, then it doesn't make sense to attempt going forward,
because we have no reference point about the likelihood of scripts
relying on specific exit codes.

-- 
Felipe Contreras

Re: [PATCH] t0005: test git exit code from signal death

[Felipe Contreras]

On Sat, Jun 1, 2013 at 12:24 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Jun 01, 2013 at 10:01:49AM -0500, Felipe Contreras wrote:
>
>> Anyway, if you care so much about the current behavior, why isn't
>> there any tests that check for this?
>>
>> My patch passes *all* the tests.
>
> The test suite has never been (and probably never will be) a complete
> specification of git's behavior. Noticing that a desired behavior is not
> in the test suite is an opportunity to improve its coverage, not argue
> that a change which breaks the desired behavior must therefore be
> acceptable.

Nobody did such a thing. I asked a question, and it's good that you
are answering by filling the missing test-case.

-- 
Felipe Contreras