RE: [ANNOUNCE] Git v2.33.0-rc2 (Build/Test Report)

["Randall S. Becker" <rsbecker@nexbridge.com>]

On August 16, 2021 6:23 PM, Jeff King wrote:
>On Mon, Aug 16, 2021 at 05:54:44PM -0400, Randall S. Becker wrote:
>
>> Running git-send-email reports completion 162. The code variable is
>> optimized out but looks like it also is 162 when returning. The
>> WIFEXITED(status) code did not appear to execute, although I think
>> that also was optimized out. finish_command ret is 162. So perl looks
>> like it is completing with a bad completion code. This percolates up
>> to git, which also reports the same value.
>
>OK, at least that absolves git.c. :)

Too right 😊

>> I went to the perl maintainer on this subject. What I got back was
>> that die is not guaranteed to return a specific value other than 0 for
>> success and non-zero for failure. There are platforms where the return
>> might known and has meaning but that is not portable. According to the
>> current official perl documentation:
>>
>> "die raises an exception. Inside an eval the exception is stuffed into
>> $@ and the eval is terminated with the undefined value. If the
>> exception is outside of all enclosing evals, then the uncaught
>> exception is printed to STDERR and perl exits with an exit code
>> indicating failure. If you need to exit the process with a specific
>> exit code, see exit."
>
>Ouch. I mean, sure, if you need a specific code, I get that die is not a good tool. But getting arbitrary values seems kind of weird and
>unfriendly. The perldoc for die does say it gives you $! (errno), or $?
>(the last child exit value) if appropriate. So it's not completely arbitrary, but I think your errno value may just be unlucky.

Very unlucky.
>
>> So assuming that a signal occurred because the value is between 129
>> and 192 is not correct in the case of perl. Could we do something like
>> test_expect_perl_die that does not perform the signal check at line
>> 980 in test-lib-functions.sh so just checks 0 vs. non-zero, which
>> would be semantically correct no matter what the platform?
>> Alternatively, and possibly better, the die could be caught and then
>> exit() called in git-send-email, as in:
>>
>> eval { die "Something bad happened" };
>> exit(255) if $@;
>
>Yeah, I think we are better to get a consistent exit code from perl.
>There are a few options here:
>
> - wrapping in an eval works, as you showed above. It's a little awkward
>   to wrap the whole script, though.
>
> - there's $SIG{__DIE__}, but the manpage warns against using it. You
>   can use it something like this:
>
>     sub catch_top {
>       CORE::die @_ if $^S; # in an eval; use regular die
>       CORE::die @_ if !defined $^S; in perl's parser
>       print STDERR "@_\n";
>       exit 255; # or whatever we want
>     }
>     $SIG{__DIE__} = \&catch_top;
>
> - you can hook die() like this:
>
>     BEGIN { *CORE::GLOBAL::die = \&my_die; }
>
>   but I expect would still need to check that you're not in an eval, as
>   above.
>
>  - The SIG{__DIE__} docs mention using an END{} block, but I'm not sure
>    how you determine if we hit a die or not (at that point, $@ won't
>    actually be set).
>
>I've used the catch_top() thing before and it does work (it's just ugly that you have to deal with $^S).
>
>I guess yet another alternative is that we could avoid using perl's
>die() in favor of our own custom-named function. It seems like that may confuse folks who come later, though.

I have tried to arrange a more consistent error code from the perl team. There are too many platform mods, and getting this particular change done won't happen - I have already been told something like "Sure, but in an RFE. We'll evaluate it.", which will mean 18 months minimum on x86 if approved (unlikely) and won't happen on the ia64 platform - ever. I think this is ultimately resulting from what is on the stack when the exception is thrown inside perl that is different on the platform. It did change - it was actually 169 two years ago with the same effect. Changing it will mean a custom mod to base perl - not likely going to happen.

What it we left the perl code as is and went with the following, since this seems to happen only above 128:

# Similar to test_must_fail, but just considers pass/fail on completion codes
test_perl_must_die () {
        case "$1" in
        ok=*)
                _test_ok=${1#ok=}
                shift
                ;;
        *)
                _test_ok=
                ;;
        esac
        if ! test_must_fail_acceptable "$@"
        then
                echo >&7 " test_perl_must_die: only 'git' is allowed: $*"
                return 1
        fi
        "$@" 2>&7
        exit_code=$?
        if test $exit_code -eq 0 && ! list_contains "$_test_ok" success
        then
                echo >&4 " test_perl_must_die: command succeeded: $*"
                return 1
        elif test_match_signal 13 $exit_code && list_contains "$_test_ok" sigpipe
        then
                return 0
        elif test $exit_code -eq 127
        then
                echo >&4 " test_perl_must_die: command not found: $*"
                return 1
        elif test $exit_code -eq 126
        then
                echo >&4 " test_perl_must_die: valgrind error: $*"
                return 1
        fi
        return 0
} 7>&2 2>&4

Then change t9001 to refer to this function instead of test_must_fail.