Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.
On 1 March 2018 at 16:08, Jeff King 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/"
Re: [Problem] test_must_fail makes possibly questionable assumptions about exit_code.
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.
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.
On 1 March 2018 at 08:36, 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. 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.
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.
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.
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-896107c29...@kdbg.org/T > /#m0b30f0857feae2044f38e04dc6b8565b68b7d52b That is: test_must_fail git send-email --dump-aliases --to=jan...@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.
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-896107c29...@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 2112884442 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.
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 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.
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.
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.
On February 28, 2018 12:19 PM, demerphq wrote: > On 28 February 2018 at 18:10, Randall S. Becker > wrote: > > On February 28, 2018 11:46 AM, demerphq wrote: > >> On 28 February 2018 at 08:49, Jeff King wrote: > >> > On Wed, Feb 28, 2018 at 07:42:51AM +, 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. > >> >> > >> >> > >> >> > >> >> > > > 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.
On 28 February 2018 at 18:19, demerphq wrote: > On 28 February 2018 at 18:10, Randall S. Becker > wrote: >> On February 28, 2018 11:46 AM, demerphq wrote: >>> On 28 February 2018 at 08:49, Jeff King wrote: >>> > On Wed, Feb 28, 2018 at 07:42:51AM +, 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. >>> >> >>> >> >>> >> >>> >> > > > 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.
On 28 February 2018 at 18:10, Randall S. Becker wrote: > On February 28, 2018 11:46 AM, demerphq wrote: >> On 28 February 2018 at 08:49, Jeff King wrote: >> > On Wed, Feb 28, 2018 at 07:42:51AM +, 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. >> >> >> >> >> >> >> >> > > > 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.
On February 28, 2018 11:46 AM, demerphq wrote: > On 28 February 2018 at 08:49, Jeff King wrote: > > On Wed, Feb 28, 2018 at 07:42:51AM +, 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. > >> > >> > >> > >> > > > 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.
On 28 February 2018 at 15:55, Randall S. Becker wrote: > On February 28, 2018 2:49 AM, Peff wrote: >> On Wed, Feb 28, 2018 at 07:42:51AM +, 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. >> > >> > >> > >> > > > > 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.
On 28 February 2018 at 08:49, Jeff King wrote: > On Wed, Feb 28, 2018 at 07:42:51AM +, 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. >> >> >> >> > > > 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.
Jeff King 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.
On February 28, 2018 2:49 AM, Peff wrote: > On Wed, Feb 28, 2018 at 07:42:51AM +, 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. > > > > > > > > > > > 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.
On Wed, Feb 28, 2018 at 07:42:51AM +, 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. > > > > > > > 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.
Jeff King wrote: > On Wed, Feb 28, 2018 at 04:07:18AM +, 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. > > > 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.
On Wed, Feb 28, 2018 at 04:07:18AM +, 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.
Jonathan Nieder 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.
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