[Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Randall S. Becker]

Hi all,

After months of arguing with some platform developers on this subject, the
perl spec was held over my head repeatedly about a few lines that are
causing issues. The basic problem is this line (test-lib-functions.sh, line
633, still in ffa952497)

>        elif test $exit_code -gt 129 && test $exit_code -le 192
>       then
>               echo >&2 "test_must_fail: died by signal $(($exit_code -
128)):

According to the perl spec http://perldoc.perl.org/functions/die.html, die
basically takes whatever errno is, mods it with 256 and there you go. EBADF
is what is used when perl reads from stdin and calls die - that's standard
perl. In most systems, you end up with something useful, when EBADF is 9.
But when it is 4009, you get a garbage answer (4009 mod 256 a.k.a. 169).
However, only 128-165 are technically reserved for signals, rather than all
the way up to 192, which may be true in some places but not everywhere.

The advice (I'm putting that nicely) I received was to use exit so that the
result is predictable - unlikely to be useful in the 15K test suites in git.
However, dropping this to 165 conditionally might help.

I'm looking for what approach to take here, because I don't think I'm going
to get perl fixed any time soon, or the error number range on the platform
fixed ... ever.

This is causing only two breaks that I have lived with and probably still
could. Consider me begging for a suggestion.

Sincerest,
Randall

-- Brief whoami:
 NonStop developer since approximately 211288444200000000
 UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Jonathan Nieder]

Hi,

Randall S. Becker wrote:

> After months of arguing with some platform developers on this subject, the
> perl spec was held over my head repeatedly about a few lines that are
> causing issues. The basic problem is this line (test-lib-functions.sh, line
> 633, still in ffa952497)
>
>>        elif test $exit_code -gt 129 && test $exit_code -le 192
>>       then
>>               echo >&2 "test_must_fail: died by signal $(($exit_code - 128)):
>
> According to the perl spec http://perldoc.perl.org/functions/die.html, die
> basically takes whatever errno is, mods it with 256 and there you go. EBADF
> is what is used when perl reads from stdin and calls die - that's standard
> perl. In most systems, you end up with something useful, when EBADF is 9.
> But when it is 4009, you get a garbage answer (4009 mod 256 a.k.a. 169).
> However, only 128-165 are technically reserved for signals, rather than all
> the way up to 192, which may be true in some places but not everywhere.
>
> The advice (I'm putting that nicely) I received was to use exit so that the
> result is predictable - unlikely to be useful in the 15K test suites in git.

The fundamental thing is the actual Git commands, not the tests in the
testsuite, no?

In the rest of git, die() makes a command exit with status 128.  The
trouble here is that our code in Perl is assuming the same meaning for
die() but using perl's die builtin instead.  That suggests a few
options:

 a) We could override the meaning of die() in Git.pm.  This feels
    ugly but if it works, it would be a very small patch.

 b) We could forbid use of die() and use some git_die() instead (but
    with a better name) for our own error handling.

 c) We could have a special different exit code convention for
    commands written in Perl.  And then change expectations whenever a
    command is rewritten in C.  As you might expect, I don't like this
    option.

 d) We could wrap each command in an eval {...} block to convert the
    result from die() to exit 128.

Option (b) feels simplest to me.

Thoughts?

Thanks,
Jonathan

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Eric Wong]

Jonathan Nieder <jrnieder@gmail.com> wrote:
> The fundamental thing is the actual Git commands, not the tests in the
> testsuite, no?

Right.  I've never been picky about exit codes; only that a
non-zero happens on errors.

> In the rest of git, die() makes a command exit with status 128.  The
> trouble here is that our code in Perl is assuming the same meaning for
> die() but using perl's die builtin instead.  That suggests a few
> options:
> 
>  a) We could override the meaning of die() in Git.pm.  This feels
>     ugly but if it works, it would be a very small patch.

Unlikely to work since I think we use eval {} to trap exceptions
from die.

>  b) We could forbid use of die() and use some git_die() instead (but
>     with a better name) for our own error handling.

Call sites may be dual-use: "die" can either be caught by an
eval or used to show an error message to the user.

>  c) We could have a special different exit code convention for
>     commands written in Perl.  And then change expectations whenever a
>     command is rewritten in C.  As you might expect, I don't like this
>     option.

I don't like it, either.

>  d) We could wrap each command in an eval {...} block to convert the
>     result from die() to exit 128.

I prefer option d)

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Jeff King]

On Wed, Feb 28, 2018 at 04:07:18AM +0000, Eric Wong wrote:

> > In the rest of git, die() makes a command exit with status 128.  The
> > trouble here is that our code in Perl is assuming the same meaning for
> > die() but using perl's die builtin instead.  That suggests a few
> > options:
> > 
> >  a) We could override the meaning of die() in Git.pm.  This feels
> >     ugly but if it works, it would be a very small patch.
> 
> Unlikely to work since I think we use eval {} to trap exceptions
> from die.
> 
> >  b) We could forbid use of die() and use some git_die() instead (but
> >     with a better name) for our own error handling.
> 
> Call sites may be dual-use: "die" can either be caught by an
> eval or used to show an error message to the user.
> 
> >  c) We could have a special different exit code convention for
> >     commands written in Perl.  And then change expectations whenever a
> >     command is rewritten in C.  As you might expect, I don't like this
> >     option.
> 
> I don't like it, either.
> 
> >  d) We could wrap each command in an eval {...} block to convert the
> >     result from die() to exit 128.
> 
> I prefer option d)

FWIW, I agree with all of that. You can do (d) without an enclosing eval
block by just hooking the __DIE__ handler, like:

$SIG{__DIE__} = sub {
  print STDERR "fatal: @_\n";
  exit 128;
};

-Peff

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Eric Wong]

Jeff King <peff@peff.net> wrote:
> On Wed, Feb 28, 2018 at 04:07:18AM +0000, Eric Wong wrote:
> 
> > > In the rest of git, die() makes a command exit with status 128.  The
> > > trouble here is that our code in Perl is assuming the same meaning for
> > > die() but using perl's die builtin instead.  That suggests a few
> > > options:
> > > 
> > >  a) We could override the meaning of die() in Git.pm.  This feels
> > >     ugly but if it works, it would be a very small patch.
> > 
> > Unlikely to work since I think we use eval {} to trap exceptions
> > from die.
> > 
> > >  b) We could forbid use of die() and use some git_die() instead (but
> > >     with a better name) for our own error handling.
> > 
> > Call sites may be dual-use: "die" can either be caught by an
> > eval or used to show an error message to the user.

<snip>

> > >  d) We could wrap each command in an eval {...} block to convert the
> > >     result from die() to exit 128.
> > 
> > I prefer option d)
> 
> FWIW, I agree with all of that. You can do (d) without an enclosing eval
> block by just hooking the __DIE__ handler, like:
> 
> $SIG{__DIE__} = sub {
>   print STDERR "fatal: @_\n";
>   exit 128;
> };

Looks like it has the same problems I pointed out with a) and b).

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Jeff King]

On Wed, Feb 28, 2018 at 07:42:51AM +0000, Eric Wong wrote:

> > > >  a) We could override the meaning of die() in Git.pm.  This feels
> > > >     ugly but if it works, it would be a very small patch.
> > > 
> > > Unlikely to work since I think we use eval {} to trap exceptions
> > > from die.
> > > 
> > > >  b) We could forbid use of die() and use some git_die() instead (but
> > > >     with a better name) for our own error handling.
> > > 
> > > Call sites may be dual-use: "die" can either be caught by an
> > > eval or used to show an error message to the user.
> 
> <snip>
> 
> > > >  d) We could wrap each command in an eval {...} block to convert the
> > > >     result from die() to exit 128.
> > > 
> > > I prefer option d)
> > 
> > FWIW, I agree with all of that. You can do (d) without an enclosing eval
> > block by just hooking the __DIE__ handler, like:
> > 
> > $SIG{__DIE__} = sub {
> >   print STDERR "fatal: @_\n";
> >   exit 128;
> > };
> 
> Looks like it has the same problems I pointed out with a) and b).

You're right. I cut down my example too much and dropped the necessary
eval magic. Try this:

-- >8 --
SIG{__DIE__} = sub {
  CORE::die @_ if $^S || !defined($^S);
  print STDERR "fatal: @_";
  exit 128;
};

eval {
  die "inside eval";
};
print "eval status: $@" if $@;

die "outside eval";
-- 8< --

Running that should produce:

$ perl foo.pl; echo $?
eval status: inside eval at foo.pl line 8.
fatal: outside eval at foo.pl line 12.
128

It may be getting a little too black-magic, though. Embedding in an eval
is at least straightforward, if a bit more invasive.

-Peff

RE: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Randall S. Becker]

On February 28, 2018 2:49 AM, Peff wrote:
> On Wed, Feb 28, 2018 at 07:42:51AM +0000, Eric Wong wrote:
> 
> > > > >  a) We could override the meaning of die() in Git.pm.  This feels
> > > > >     ugly but if it works, it would be a very small patch.
> > > >
> > > > Unlikely to work since I think we use eval {} to trap exceptions
> > > > from die.
> > > >
> > > > >  b) We could forbid use of die() and use some git_die() instead (but
> > > > >     with a better name) for our own error handling.
> > > >
> > > > Call sites may be dual-use: "die" can either be caught by an eval
> > > > or used to show an error message to the user.
> >
> > <snip>
> >
> > > > >  d) We could wrap each command in an eval {...} block to convert the
> > > > >     result from die() to exit 128.
> > > >
> > > > I prefer option d)
> > >
> > > FWIW, I agree with all of that. You can do (d) without an enclosing
> > > eval block by just hooking the __DIE__ handler, like:
> > >
> > > $SIG{__DIE__} = sub {
> > >   print STDERR "fatal: @_\n";
> > >   exit 128;
> > > };
> >
> > Looks like it has the same problems I pointed out with a) and b).
> 
> You're right. I cut down my example too much and dropped the necessary
> eval magic. Try this:
> 
> -- >8 --
> SIG{__DIE__} = sub {
>   CORE::die @_ if $^S || !defined($^S);
>   print STDERR "fatal: @_";
>   exit 128;
> };
> 
> eval {
>   die "inside eval";
> };
> print "eval status: $@" if $@;
> 
> die "outside eval";
> -- 8< --
> 
> Running that should produce:
> 
> $ perl foo.pl; echo $?
> eval status: inside eval at foo.pl line 8.
> fatal: outside eval at foo.pl line 12.
> 128
> 
> It may be getting a little too black-magic, though. Embedding in an eval is at
> least straightforward, if a bit more invasive.

I like this solution. The $64K question for me is how (a.k.a. where) to instrument this broadly instead of in each perl fragment in the test suite.  The code:

$SIG{__DIE__} = sub {
  CORE::die @_ if $^S || !defined($^S);
  print STDERR "fatal: @_";
  exit 128;
};

eval {
  die "inside eval";
};

print "eval status: $@" if $@;

die "outside eval";

as tested above, in NonStop results in an exit code of 128 whether run from a script or from stdin (a good thing). I'm happy to do the heavy lifting on this, but  a bit more direction as to the implementation would help.

Cheers,
Randall

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> You're right. I cut down my example too much and dropped the necessary
> eval magic. Try this:
>
> -- >8 --
> SIG{__DIE__} = sub {
>   CORE::die @_ if $^S || !defined($^S);
>   print STDERR "fatal: @_";
>   exit 128;
> };
>
> eval {
>   die "inside eval";
> };
> print "eval status: $@" if $@;
>
> die "outside eval";
> -- 8< --
>
> Running that should produce:
>
> $ perl foo.pl; echo $?
> eval status: inside eval at foo.pl line 8.
> fatal: outside eval at foo.pl line 12.
> 128
>
> It may be getting a little too black-magic, though. Embedding in an eval
> is at least straightforward, if a bit more invasive.

I briefly wondered if this affects die that are called by code that
we did not write (i.e. from core or cpan via "use/require"), but (1)
we do want this to affect them, so that we die with status code
known to us, and (2) the way this uses CORE::die or exit depending
on the surrounding 'eval' would "fool" their uses just like we want
it to fool our uses, which is exactly what we want.

So, it looks like a good "black magic" ;-).

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[demerphq]

On 28 February 2018 at 08:49, Jeff King <peff@peff.net> wrote:
> On Wed, Feb 28, 2018 at 07:42:51AM +0000, Eric Wong wrote:
>
>> > > >  a) We could override the meaning of die() in Git.pm.  This feels
>> > > >     ugly but if it works, it would be a very small patch.
>> > >
>> > > Unlikely to work since I think we use eval {} to trap exceptions
>> > > from die.
>> > >
>> > > >  b) We could forbid use of die() and use some git_die() instead (but
>> > > >     with a better name) for our own error handling.
>> > >
>> > > Call sites may be dual-use: "die" can either be caught by an
>> > > eval or used to show an error message to the user.
>>
>> <snip>
>>
>> > > >  d) We could wrap each command in an eval {...} block to convert the
>> > > >     result from die() to exit 128.
>> > >
>> > > I prefer option d)
>> >
>> > FWIW, I agree with all of that. You can do (d) without an enclosing eval
>> > block by just hooking the __DIE__ handler, like:
>> >
>> > $SIG{__DIE__} = sub {
>> >   print STDERR "fatal: @_\n";
>> >   exit 128;
>> > };
>>
>> Looks like it has the same problems I pointed out with a) and b).
>
> You're right. I cut down my example too much and dropped the necessary
> eval magic. Try this:
>
> -- >8 --
> SIG{__DIE__} = sub {
>   CORE::die @_ if $^S || !defined($^S);
>   print STDERR "fatal: @_";
>   exit 128;
> };

FWIW, this doesn't need to use CORE::die like that unless you have
code that overrides die() or CORE::GLOBAL::die, which would be pretty
unusual.

die() within $SIG{__DIE__} is special cased not to trigger $SIG{__DIE__} again.

Of course it doesn't hurt, but it might make a perl hacker do a double
take why you are doing it. Maybe add a comment like

# using CORE::die to armor against overridden die()

cheers,
Yves

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[demerphq]

On 28 February 2018 at 15:55, Randall S. Becker <rsbecker@nexbridge.com> wrote:
> On February 28, 2018 2:49 AM, Peff wrote:
>> On Wed, Feb 28, 2018 at 07:42:51AM +0000, Eric Wong wrote:
>>
>> > > > >  a) We could override the meaning of die() in Git.pm.  This feels
>> > > > >     ugly but if it works, it would be a very small patch.
>> > > >
>> > > > Unlikely to work since I think we use eval {} to trap exceptions
>> > > > from die.
>> > > >
>> > > > >  b) We could forbid use of die() and use some git_die() instead (but
>> > > > >     with a better name) for our own error handling.
>> > > >
>> > > > Call sites may be dual-use: "die" can either be caught by an eval
>> > > > or used to show an error message to the user.
>> >
>> > <snip>
>> >
>> > > > >  d) We could wrap each command in an eval {...} block to convert the
>> > > > >     result from die() to exit 128.
>> > > >
>> > > > I prefer option d)
>> > >
>> > > FWIW, I agree with all of that. You can do (d) without an enclosing
>> > > eval block by just hooking the __DIE__ handler, like:
>> > >
>> > > $SIG{__DIE__} = sub {
>> > >   print STDERR "fatal: @_\n";
>> > >   exit 128;
>> > > };
>> >
>> > Looks like it has the same problems I pointed out with a) and b).
>>
>> You're right. I cut down my example too much and dropped the necessary
>> eval magic. Try this:
>>
>> -- >8 --
>> SIG{__DIE__} = sub {
>>   CORE::die @_ if $^S || !defined($^S);
>>   print STDERR "fatal: @_";
>>   exit 128;
>> };
>>
>> eval {
>>   die "inside eval";
>> };
>> print "eval status: $@" if $@;
>>
>> die "outside eval";
>> -- 8< --
>>
>> Running that should produce:
>>
>> $ perl foo.pl; echo $?
>> eval status: inside eval at foo.pl line 8.
>> fatal: outside eval at foo.pl line 12.
>> 128
>>
>> It may be getting a little too black-magic, though. Embedding in an eval is at
>> least straightforward, if a bit more invasive.
>
> I like this solution. The $64K question for me is how (a.k.a. where) to instrument this broadly instead of in each perl fragment in the test suite.  The code:
>
> $SIG{__DIE__} = sub {
>   CORE::die @_ if $^S || !defined($^S);
>   print STDERR "fatal: @_";
>   exit 128;
> };
>
> eval {
>   die "inside eval";
> };
>
> print "eval status: $@" if $@;
>
> die "outside eval";
>
> as tested above, in NonStop results in an exit code of 128 whether run from a script or from stdin (a good thing). I'm happy to do the heavy lifting on this, but  a bit more direction as to the implementation would help.

I would look into putting it into a module and then using the PERL5OPT
environment var to have it loaded automagically in any of your perl
scripts.

For instance if you put that code into a module called Git/DieTrap.pm

then you could do:

PERL5OPT=-MGit::DieTrap

In your test setup code assuming you have some. Then you don't need to
change any of your scripts just the test runner framework.

Yves

-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

RE: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Randall S. Becker]

On February 28, 2018 11:46 AM, demerphq wrote:
> On 28 February 2018 at 08:49, Jeff King <peff@peff.net> wrote:
> > On Wed, Feb 28, 2018 at 07:42:51AM +0000, Eric Wong wrote:
> >
> >> > > >  a) We could override the meaning of die() in Git.pm.  This feels
> >> > > >     ugly but if it works, it would be a very small patch.
> >> > >
> >> > > Unlikely to work since I think we use eval {} to trap exceptions
> >> > > from die.
> >> > >
> >> > > >  b) We could forbid use of die() and use some git_die() instead (but
> >> > > >     with a better name) for our own error handling.
> >> > >
> >> > > Call sites may be dual-use: "die" can either be caught by an eval
> >> > > or used to show an error message to the user.
> >>
> >> <snip>
> >>
> >> > > >  d) We could wrap each command in an eval {...} block to convert the
> >> > > >     result from die() to exit 128.
> >> > >
> >> > > I prefer option d)
> >> >
> >> > FWIW, I agree with all of that. You can do (d) without an enclosing
> >> > eval block by just hooking the __DIE__ handler, like:
> >> >
> >> > $SIG{__DIE__} = sub {
> >> >   print STDERR "fatal: @_\n";
> >> >   exit 128;
> >> > };
> >>
> >> Looks like it has the same problems I pointed out with a) and b).
> >
> > You're right. I cut down my example too much and dropped the necessary
> > eval magic. Try this:
> >
> > -- >8 --
> > SIG{__DIE__} = sub {
> >   CORE::die @_ if $^S || !defined($^S);
> >   print STDERR "fatal: @_";
> >   exit 128;
> > };
> 
> FWIW, this doesn't need to use CORE::die like that unless you have code that
> overrides die() or CORE::GLOBAL::die, which would be pretty unusual.
> 
> die() within $SIG{__DIE__} is special cased not to trigger $SIG{__DIE__}
> again.
> 
> Of course it doesn't hurt, but it might make a perl hacker do a double take
> why you are doing it. Maybe add a comment like
> 
> # using CORE::die to armor against overridden die()

The problem is actually in git code in its test suite that uses perl inline, not in my test code itself. The difficulty I'm having is placing this appropriate so that the signal handler gets used throughout the test suite including in the perl -e invocations. This is more a lack of my own understanding of plumbing of git test framework rather than of using or coding perl.

Cheers,
Randall

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[demerphq]

On 28 February 2018 at 18:10, Randall S. Becker <rsbecker@nexbridge.com> wrote:
> On February 28, 2018 11:46 AM, demerphq wrote:
>> On 28 February 2018 at 08:49, Jeff King <peff@peff.net> wrote:
>> > On Wed, Feb 28, 2018 at 07:42:51AM +0000, Eric Wong wrote:
>> >
>> >> > > >  a) We could override the meaning of die() in Git.pm.  This feels
>> >> > > >     ugly but if it works, it would be a very small patch.
>> >> > >
>> >> > > Unlikely to work since I think we use eval {} to trap exceptions
>> >> > > from die.
>> >> > >
>> >> > > >  b) We could forbid use of die() and use some git_die() instead (but
>> >> > > >     with a better name) for our own error handling.
>> >> > >
>> >> > > Call sites may be dual-use: "die" can either be caught by an eval
>> >> > > or used to show an error message to the user.
>> >>
>> >> <snip>
>> >>
>> >> > > >  d) We could wrap each command in an eval {...} block to convert the
>> >> > > >     result from die() to exit 128.
>> >> > >
>> >> > > I prefer option d)
>> >> >
>> >> > FWIW, I agree with all of that. You can do (d) without an enclosing
>> >> > eval block by just hooking the __DIE__ handler, like:
>> >> >
>> >> > $SIG{__DIE__} = sub {
>> >> >   print STDERR "fatal: @_\n";
>> >> >   exit 128;
>> >> > };
>> >>
>> >> Looks like it has the same problems I pointed out with a) and b).
>> >
>> > You're right. I cut down my example too much and dropped the necessary
>> > eval magic. Try this:
>> >
>> > -- >8 --
>> > SIG{__DIE__} = sub {
>> >   CORE::die @_ if $^S || !defined($^S);
>> >   print STDERR "fatal: @_";
>> >   exit 128;
>> > };
>>
>> FWIW, this doesn't need to use CORE::die like that unless you have code that
>> overrides die() or CORE::GLOBAL::die, which would be pretty unusual.
>>
>> die() within $SIG{__DIE__} is special cased not to trigger $SIG{__DIE__}
>> again.
>>
>> Of course it doesn't hurt, but it might make a perl hacker do a double take
>> why you are doing it. Maybe add a comment like
>>
>> # using CORE::die to armor against overridden die()
>
> The problem is actually in git code in its test suite that uses perl inline, not in my test code itself. The difficulty I'm having is placing this appropriate so that the signal handler gets used throughout the test suite including in the perl -e invocations. This is more a lack of my own understanding of plumbing of git test framework rather than of using or coding perl.

Did you reply to the wrong mail?

Create a file like:

.../Git/DieTrap.pm

which would look like  this:

package Git::DieTrap;
use strict;
use warnings;

SIG{__DIE__} = sub {
   CORE::die @_ if $^S || !defined($^S);
   print STDERR "fatal: @_";
   exit 128;
};

1;
__END__

and then you would do:

export PERL5OPT=-MGit::DieTrap

before executing any tests. ANY use of perl from that point on will
behave as though it has:

use Git::DieTrap;

at the top of the script, be it a -e, or any other way that Perl code
is executed.

cheers,
Yves

-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[demerphq]

On 28 February 2018 at 18:19, demerphq <demerphq@gmail.com> wrote:
> On 28 February 2018 at 18:10, Randall S. Becker <rsbecker@nexbridge.com> wrote:
>> On February 28, 2018 11:46 AM, demerphq wrote:
>>> On 28 February 2018 at 08:49, Jeff King <peff@peff.net> wrote:
>>> > On Wed, Feb 28, 2018 at 07:42:51AM +0000, Eric Wong wrote:
>>> >
>>> >> > > >  a) We could override the meaning of die() in Git.pm.  This feels
>>> >> > > >     ugly but if it works, it would be a very small patch.
>>> >> > >
>>> >> > > Unlikely to work since I think we use eval {} to trap exceptions
>>> >> > > from die.
>>> >> > >
>>> >> > > >  b) We could forbid use of die() and use some git_die() instead (but
>>> >> > > >     with a better name) for our own error handling.
>>> >> > >
>>> >> > > Call sites may be dual-use: "die" can either be caught by an eval
>>> >> > > or used to show an error message to the user.
>>> >>
>>> >> <snip>
>>> >>
>>> >> > > >  d) We could wrap each command in an eval {...} block to convert the
>>> >> > > >     result from die() to exit 128.
>>> >> > >
>>> >> > > I prefer option d)
>>> >> >
>>> >> > FWIW, I agree with all of that. You can do (d) without an enclosing
>>> >> > eval block by just hooking the __DIE__ handler, like:
>>> >> >
>>> >> > $SIG{__DIE__} = sub {
>>> >> >   print STDERR "fatal: @_\n";
>>> >> >   exit 128;
>>> >> > };
>>> >>
>>> >> Looks like it has the same problems I pointed out with a) and b).
>>> >
>>> > You're right. I cut down my example too much and dropped the necessary
>>> > eval magic. Try this:
>>> >
>>> > -- >8 --
>>> > SIG{__DIE__} = sub {
>>> >   CORE::die @_ if $^S || !defined($^S);
>>> >   print STDERR "fatal: @_";
>>> >   exit 128;
>>> > };
>>>
>>> FWIW, this doesn't need to use CORE::die like that unless you have code that
>>> overrides die() or CORE::GLOBAL::die, which would be pretty unusual.
>>>
>>> die() within $SIG{__DIE__} is special cased not to trigger $SIG{__DIE__}
>>> again.
>>>
>>> Of course it doesn't hurt, but it might make a perl hacker do a double take
>>> why you are doing it. Maybe add a comment like
>>>
>>> # using CORE::die to armor against overridden die()
>>
>> The problem is actually in git code in its test suite that uses perl inline, not in my test code itself. The difficulty I'm having is placing this appropriate so that the signal handler gets used throughout the test suite including in the perl -e invocations. This is more a lack of my own understanding of plumbing of git test framework rather than of using or coding perl.
>
> Did you reply to the wrong mail?
>
> Create a file like:
>
> .../Git/DieTrap.pm
>
> which would look like  this:
>
> package Git::DieTrap;
> use strict;
> use warnings;
>
> SIG{__DIE__} = sub {

OOPs, that should read

$SIG{__DIE__} = sub {

sorry about that.

>    CORE::die @_ if $^S || !defined($^S);
>    print STDERR "fatal: @_";
>    exit 128;
> };
>
> 1;
> __END__
>
> and then you would do:
>
> export PERL5OPT=-MGit::DieTrap
>
> before executing any tests. ANY use of perl from that point on will
> behave as though it has:
>
> use Git::DieTrap;
>
> at the top of the script, be it a -e, or any other way that Perl code
> is executed.
>
> cheers,
> Yves
>
> --
> perl -Mre=debug -e "/just|another|perl|hacker/"



-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

RE: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Randall S. Becker]

On February 28, 2018 12:19 PM, demerphq wrote:
> On 28 February 2018 at 18:10, Randall S. Becker <rsbecker@nexbridge.com>
> wrote:
> > On February 28, 2018 11:46 AM, demerphq wrote:
> >> On 28 February 2018 at 08:49, Jeff King <peff@peff.net> wrote:
> >> > On Wed, Feb 28, 2018 at 07:42:51AM +0000, Eric Wong wrote:
> >> >
> >> >> > > >  a) We could override the meaning of die() in Git.pm.  This feels
> >> >> > > >     ugly but if it works, it would be a very small patch.
> >> >> > >
> >> >> > > Unlikely to work since I think we use eval {} to trap
> >> >> > > exceptions from die.
> >> >> > >
> >> >> > > >  b) We could forbid use of die() and use some git_die() instead
> (but
> >> >> > > >     with a better name) for our own error handling.
> >> >> > >
> >> >> > > Call sites may be dual-use: "die" can either be caught by an
> >> >> > > eval or used to show an error message to the user.
> >> >>
> >> >> <snip>
> >> >>
> >> >> > > >  d) We could wrap each command in an eval {...} block to convert
> the
> >> >> > > >     result from die() to exit 128.
> >> >> > >
> >> >> > > I prefer option d)
> >> >> >
> >> >> > FWIW, I agree with all of that. You can do (d) without an
> >> >> > enclosing eval block by just hooking the __DIE__ handler, like:
> >> >> >
> >> >> > $SIG{__DIE__} = sub {
> >> >> >   print STDERR "fatal: @_\n";
> >> >> >   exit 128;
> >> >> > };
> >> >>
> >> >> Looks like it has the same problems I pointed out with a) and b).
> >> >
> >> > You're right. I cut down my example too much and dropped the
> >> > necessary eval magic. Try this:
> >> >
> >> > -- >8 --
> >> > SIG{__DIE__} = sub {
> >> >   CORE::die @_ if $^S || !defined($^S);
> >> >   print STDERR "fatal: @_";
> >> >   exit 128;
> >> > };
> >>
> >> FWIW, this doesn't need to use CORE::die like that unless you have
> >> code that overrides die() or CORE::GLOBAL::die, which would be pretty
> unusual.
> >>
> >> die() within $SIG{__DIE__} is special cased not to trigger
> >> $SIG{__DIE__} again.
> >>
> >> Of course it doesn't hurt, but it might make a perl hacker do a
> >> double take why you are doing it. Maybe add a comment like
> >>
> >> # using CORE::die to armor against overridden die()
> >
> > The problem is actually in git code in its test suite that uses perl inline, not in
> my test code itself. The difficulty I'm having is placing this appropriate so that
> the signal handler gets used throughout the test suite including in the perl -e
> invocations. This is more a lack of my own understanding of plumbing of git
> test framework rather than of using or coding perl.
> 
> Did you reply to the wrong mail?
> 
> Create a file like:
> 
> .../Git/DieTrap.pm
> 
> which would look like  this:
> 
> package Git::DieTrap;
> use strict;
> use warnings;
> 
> SIG{__DIE__} = sub {
>    CORE::die @_ if $^S || !defined($^S);
>    print STDERR "fatal: @_";
>    exit 128;
> };
> 
> 1;
> __END__
> 
> and then you would do:
> 
> export PERL5OPT=-MGit::DieTrap
> 
> before executing any tests. ANY use of perl from that point on will behave as
> though it has:
> 
> use Git::DieTrap;
> 
> at the top of the script, be it a -e, or any other way that Perl code is
> executed.

The context of this request, perhaps missing, what that I have been trying to move the platform to the standard git code base. It is not my issue specifically. Rather, if someone else wants to build and test git on the platform, they should not have to have any knowledge of putting in hacks to make it work. I can personally make this work. That's not the point. It is to allow others on platform to make it work without deep knowledge. Otherwise, I am not being productive with my efforts.

Cheers,
Randall

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Jonathan Nieder]

Randall S. Becker wrote:

> The problem is actually in git code in its test suite that uses perl
> inline, not in my test code itself. The difficulty I'm having is
> placing this appropriate so that the signal handler gets used
> throughout the test suite including in the perl -e invocations. This
> is more a lack of my own understanding of plumbing of git test
> framework rather than of using or coding perl.

Can you elaborate with an example?  My understanding was that
test_must_fail is only for running git.  If a test is running perl and
wants to check its exit code, the test is supposed to use !, not
test_must_fail.

t/README backs me up:

 - use '! git cmd' when you want to make sure the git command exits
   with failure in a controlled way by calling "die()".  Instead,
   use 'test_must_fail git cmd'.  This will signal a failure if git
   dies in an unexpected way (e.g. segfault).

   On the other hand, don't use test_must_fail for running regular
   platform commands; just use '! cmd'.  We are not in the business
   of verifying that the world given to us sanely works.

So I don't consider the initial issue you raised a test issue at all!
It's a bug in the git commands, and a fix for it should not be
specific to the test suite.

And now it sounds like there is a second issue: the test suite is
overusing test_must_fail in some context and that needs to be fixed as
well.

Thanks,
Jonathan

RE: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Randall S. Becker]

On February 28, 2018 12:44 PM, Jonathan Nieder wrote:
> Randall S. Becker wrote:
> 
> > The problem is actually in git code in its test suite that uses perl
> > inline, not in my test code itself. The difficulty I'm having is
> > placing this appropriate so that the signal handler gets used
> > throughout the test suite including in the perl -e invocations. This
> > is more a lack of my own understanding of plumbing of git test
> > framework rather than of using or coding perl.
> 
> Can you elaborate with an example?  My understanding was that
> test_must_fail is only for running git.  If a test is running perl and
wants to
> check its exit code, the test is supposed to use !, not test_must_fail.
> 
> t/README backs me up:
> 
>  - use '! git cmd' when you want to make sure the git command exits
>    with failure in a controlled way by calling "die()".  Instead,
>    use 'test_must_fail git cmd'.  This will signal a failure if git
>    dies in an unexpected way (e.g. segfault).
> 
>    On the other hand, don't use test_must_fail for running regular
>    platform commands; just use '! cmd'.  We are not in the business
>    of verifying that the world given to us sanely works.
> 
> So I don't consider the initial issue you raised a test issue at all!
> It's a bug in the git commands, and a fix for it should not be specific to
the
> test suite.
> 
> And now it sounds like there is a second issue: the test suite is
overusing
> test_must_fail in some context and that needs to be fixed as well.

Have a look at a recent t1404 as a sample. Line 615 is the one causing the
platform grief, because it triggers a 'die'. However, the particular test
case #54, had no difference on platform with test_must_fail or !, which has
the same underlying EBADF completion after digging and digging.

not ok 52 - delete fails cleanly if packed-refs file is locked
#
#               prefix=refs/locked-packed-refs &&
#               # Set up a reference with differing loose and packed
versions:
#               git update-ref $prefix/foo $C &&
#               git pack-refs --all &&
#               git update-ref $prefix/foo $D &&
#               git for-each-ref $prefix >unchanged &&
#               # Now try to delete it while the `packed-refs` lock is held:
#               : >.git/packed-refs.lock &&
#               test_when_finished "rm -f .git/packed-refs.lock" &&
#               ! git update-ref -d $prefix/foo >out 2>err &&
#               git for-each-ref $prefix >actual &&
#               test_i18ngrep "Unable to create $Q.*packed-refs.lock$Q: File
exists" err &&
#               test_cmp unchanged actual
#

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Jonathan Nieder]

Randall S. Becker wrote:
> On February 28, 2018 12:44 PM, Jonathan Nieder wrote:
>> Randall S. Becker wrote:

>>> The problem is actually in git code in its test suite that uses perl
>>> inline, not in my test code itself.
[...]
>> Can you elaborate with an example?  My understanding was that
>> test_must_fail is only for running git.
[...]
> Have a look at a recent t1404 as a sample. Line 615 is the one causing the
> platform grief, because it triggers a 'die'. However, the particular test
> case #54, had no difference on platform with test_must_fail or !, which has
> the same underlying EBADF completion after digging and digging.

Sorry to be dense: what I see on that line is

	test_must_fail git update-ref -d $prefix/foo >out 2>err &&

How is perl involved?

Puzzled,
Jonathan

RE: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Randall S. Becker]

On February 28, 2018 1:52 PM, Jonathan Nieder wrote:
> Randall S. Becker wrote:
> > On February 28, 2018 12:44 PM, Jonathan Nieder wrote:
> >> Randall S. Becker wrote:
> 
> >>> The problem is actually in git code in its test suite that uses perl
> >>> inline, not in my test code itself.
> [...]
> >> Can you elaborate with an example?  My understanding was that
> >> test_must_fail is only for running git.
> [...]
> > Have a look at a recent t1404 as a sample. Line 615 is the one causing
> > the platform grief, because it triggers a 'die'. However, the
> > particular test case #54, had no difference on platform with
> > test_must_fail or !, which has the same underlying EBADF completion
after
> digging and digging.
> 
> Sorry to be dense: what I see on that line is
> 
> 	test_must_fail git update-ref -d $prefix/foo >out 2>err &&

My bad, I think. I'm going to go looking through my notes and get back on
which line in the test was the issue. I assumed from your response that it
might have been the test_must_fail, which is throughout the git test suite.
Obviously it isn't the line failing in this case. Stay tuned.

RE: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Randall S. Becker]

On February 28, 2018 3:04 PM, Jonathan Nieder wrote:
> On February 28, 2018 1:52 PM, Jonathan Nieder wrote:
> > Randall S. Becker wrote:
> > > On February 28, 2018 12:44 PM, Jonathan Nieder wrote:
> > >> Randall S. Becker wrote:
> >
> > >>> The problem is actually in git code in its test suite that uses
> > >>> perl inline, not in my test code itself.
> > [...]
> > >> Can you elaborate with an example?  My understanding was that
> > >> test_must_fail is only for running git.
> > [...]
> > > Have a look at a recent t1404 as a sample. Line 615 is the one
> > > causing the platform grief, because it triggers a 'die'. However,
> > > the particular test case #54, had no difference on platform with
> > > test_must_fail or !, which has the same underlying EBADF completion
> after
> > digging and digging.
> >
> > Sorry to be dense: what I see on that line is
> >
> > 	test_must_fail git update-ref -d $prefix/foo >out 2>err &&
> 
> My bad, I think. I'm going to go looking through my notes and get back on
> which line in the test was the issue. I assumed from your response that it
> might have been the test_must_fail, which is throughout the git test
suite.
> Obviously it isn't the line failing in this case. Stay tuned.

The original thread below has details of what the original issue was and
why. It hit three tests specifically on this platform where die was invoked
(at least on one of them). Perl was invoked under the covers and the
completion code of 169 propagated back through git to the test framework.
https://public-inbox.org/git/499fb29f-ca34-8d28-256d-896107c29a3e@kdbg.org/T
/#m0b30f0857feae2044f38e04dc6b8565b68b7d52b

While removing test_must_fail is laudable, it won't seemingly solve the
underlying cause that I am trying to work through, which is inserting the
$SIGdie reporting 169 on platform and inserting:

 SIG{__DIE__} = sub {
   CORE::die @_ if $^S || !defined($^S);
   print STDERR "fatal: @_";
   exit 128;
 };

I just don't know the framework well enough (or perl for that matter) to
know the exact spot to place this so that it would work properly and be
acceptable to the committers (you know who you are :-) ). 

I hope that provides info on what is going on and why I am motivated to fix
this by (nearly) whatever means necessary.

Cheers,
Randall

-- Brief whoami:
 NonStop developer since approximately 211288444200000000
 UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Jonathan Nieder]

Randall S. Becker wrote:

> The original thread below has details of what the original issue was and
> why. It hit three tests specifically on this platform where die was invoked
> (at least on one of them). Perl was invoked under the covers and the
> completion code of 169 propagated back through git to the test framework.
> https://public-inbox.org/git/499fb29f-ca34-8d28-256d-896107c29a3e@kdbg.org/T
> /#m0b30f0857feae2044f38e04dc6b8565b68b7d52b

That is:

	test_must_fail git send-email --dump-aliases --to=janice@example.com -1 refs/heads/accounting

Which matches the case described elsewhere in this thread.  It's
git-send-email.perl.  test_must_fail is doing its job correctly by
diagnosing a real bug in git-send-email.perl, which we're grateful for
you having discovered. :)

Thanks,
Jonathan

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Jeff King]

On Wed, Feb 28, 2018 at 05:46:22PM +0100, demerphq wrote:

> > You're right. I cut down my example too much and dropped the necessary
> > eval magic. Try this:
> >
> > -- >8 --
> > SIG{__DIE__} = sub {
> >   CORE::die @_ if $^S || !defined($^S);
> >   print STDERR "fatal: @_";
> >   exit 128;
> > };
> 
> FWIW, this doesn't need to use CORE::die like that unless you have
> code that overrides die() or CORE::GLOBAL::die, which would be pretty
> unusual.
> 
> die() within $SIG{__DIE__} is special cased not to trigger $SIG{__DIE__} again.
> 
> Of course it doesn't hurt, but it might make a perl hacker do a double
> take why you are doing it. Maybe add a comment like
> 
> # using CORE::die to armor against overridden die()

Thanks, I agree it should just be "die". I pulled this from an old
error-handling pattern I used in some of my scripts (which _does_
override die). I screwed it up when cutting it down the first time, but
then I didn't cut enough the second. :)

-Peff

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Jeff King]

On Wed, Feb 28, 2018 at 05:51:14PM +0100, demerphq wrote:

> I would look into putting it into a module and then using the PERL5OPT
> environment var to have it loaded automagically in any of your perl
> scripts.
> 
> For instance if you put that code into a module called Git/DieTrap.pm
> 
> then you could do:
> 
> PERL5OPT=-MGit::DieTrap
> 
> In your test setup code assuming you have some. Then you don't need to
> change any of your scripts just the test runner framework.

That's a clever trick.

It's not clear to me though if we just want to tweak the programs run in
the test scripts in order to get test_must_fail to stop complaining, or
if we consider the unusual exit codes from our perl-based Git programs
to be an error that should be fixed for real use, too.

-Peff

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[demerphq]

On 1 March 2018 at 08:36, Jeff King <peff@peff.net> wrote:
> On Wed, Feb 28, 2018 at 05:51:14PM +0100, demerphq wrote:
>
>> I would look into putting it into a module and then using the PERL5OPT
>> environment var to have it loaded automagically in any of your perl
>> scripts.
>>
>> For instance if you put that code into a module called Git/DieTrap.pm
>>
>> then you could do:
>>
>> PERL5OPT=-MGit::DieTrap
>>
>> In your test setup code assuming you have some. Then you don't need to
>> change any of your scripts just the test runner framework.
>
> That's a clever trick.
>
> It's not clear to me though if we just want to tweak the programs run in
> the test scripts in order to get test_must_fail to stop complaining, or
> if we consider the unusual exit codes from our perl-based Git programs
> to be an error that should be fixed for real use, too.

Yeah, that is a decision you guys need to make, I am not familiar
enough with the issues to make any useful comment.

But I wanted to say that I will bring this subject up on perl5porters,
the exit code triggered by a die is a regular cause of trouble for
more than just you guys, and maybe we can get it changed for the
future. Nevertheless even if there was consensus it can be changed it
will take years before it is widely distributed enough to be useful to
you. :-(

Ill be bold and say sorry on the behalf of the perl committers for
this. Perl is so old sometimes things that used to make sense don't
make sense anymore.

cheers,
Yves

-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

RE: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Randall S. Becker]

On March 1, 2018 2:36 AM, Jeff King wrote:
> On Wed, Feb 28, 2018 at 05:51:14PM +0100, demerphq wrote:
> 
> > I would look into putting it into a module and then using the PERL5OPT
> > environment var to have it loaded automagically in any of your perl
> > scripts.
> >
> > For instance if you put that code into a module called Git/DieTrap.pm
> >
> > then you could do:
> >
> > PERL5OPT=-MGit::DieTrap
> >
> > In your test setup code assuming you have some. Then you don't need to
> > change any of your scripts just the test runner framework.
> 
> That's a clever trick.
> 
> It's not clear to me though if we just want to tweak the programs run in the
> test scripts in order to get test_must_fail to stop complaining, or if we
> consider the unusual exit codes from our perl-based Git programs to be an
> error that should be fixed for real use, too.

I'm living unusual exit code IRL all the time. So "fixed for real", is what I'm looking for. So if we were to do that, where is the best place to insert a fix - my original question - that would be permanent in the main git test code. Or perhaps this needs to be in the main code itself.

Cheers,
Randall

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[Jeff King]

On Thu, Mar 01, 2018 at 09:28:31AM -0500, Randall S. Becker wrote:

> > It's not clear to me though if we just want to tweak the programs run in the
> > test scripts in order to get test_must_fail to stop complaining, or if we
> > consider the unusual exit codes from our perl-based Git programs to be an
> > error that should be fixed for real use, too.
> 
> I'm living unusual exit code IRL all the time. So "fixed for real", is
> what I'm looking for. So if we were to do that, where is the best
> place to insert a fix - my original question - that would be permanent
> in the main git test code. Or perhaps this needs to be in the main
> code itself.

If it's fixed in the real world, then it needs to be in the main code
itself. It looks like git-svn already does this to some degree itself
(most of the work happens in an eval, and it calls the "fatal" function
if that throws an exception via 'die').

So I think git-send-email.perl (and maybe others) needs to learn the
same trick (by pushing the main bits of the script into an eval). Or it
needs to include the SIG{__DIE__} trickery at the beginning of the
script.

I think the SIG{__DIE__} stuff could go into Git/PredictableDie.pm or
something, and then any scripts that need it could just "use
Git::PredictableDie".

Does that make sense?

-Peff

Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.

[demerphq]

On 1 March 2018 at 16:08, Jeff King <peff@peff.net> wrote:
> On Thu, Mar 01, 2018 at 09:28:31AM -0500, Randall S. Becker wrote:
>
>> > It's not clear to me though if we just want to tweak the programs run in the
>> > test scripts in order to get test_must_fail to stop complaining, or if we
>> > consider the unusual exit codes from our perl-based Git programs to be an
>> > error that should be fixed for real use, too.
>>
>> I'm living unusual exit code IRL all the time. So "fixed for real", is
>> what I'm looking for. So if we were to do that, where is the best
>> place to insert a fix - my original question - that would be permanent
>> in the main git test code. Or perhaps this needs to be in the main
>> code itself.
>
> If it's fixed in the real world, then it needs to be in the main code
> itself. It looks like git-svn already does this to some degree itself
> (most of the work happens in an eval, and it calls the "fatal" function
> if that throws an exception via 'die').
>
> So I think git-send-email.perl (and maybe others) needs to learn the
> same trick (by pushing the main bits of the script into an eval). Or it
> needs to include the SIG{__DIE__} trickery at the beginning of the
> script.
>
> I think the SIG{__DIE__} stuff could go into Git/PredictableDie.pm or
> something, and then any scripts that need it could just "use
> Git::PredictableDie".
>
> Does that make sense?

To me yes. By putting it in a module and 'use'ing it early you
guarantee it will be set up before any code following it is even
compiled.

If there is an existing module that the git perl code always uses then
it could go in there in a BEGIN{} block instead of adding the new
module.

Yves


-- 
perl -Mre=debug -e "/just|another|perl|hacker/"