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

On August 16, 2021 6:23 PM, Jeff King wrote:

On Mon, Aug 16, 2021 at 05:54:44PM -0400, Randall S. Becker wrote:

quoted

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 😊

quoted

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.

quoted

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.