threads / patch / 10343

patchUse exit 1 instead of die when req_Root fails.

Subject: [PATCH] Use exit 1 instead of die when req_Root fails.

## tl;dr

12 messages between Oct 17, 2007 and Oct 18, 2007. Diffs are folded; open one to read it.

replies: 11people: 6as markdown or json

Brian Gernhardt· Oct 17, 2007, 14:05 UTC · lore
This was causing test failures because die was exiting 255.
Signed-off-by: Brian Gernhardt <benji@silverinsanity.com>
---
 Shawn, I sent this in a couple weeks ago but it looks like it never
 made it into your repo.  It fixes test failures on my machine that have
 been plauging me for months.
 git-cvsserver.perl |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
Show changes to git-cvsserver.perl +4 −2
diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 13dbd27..0d55fec 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -145,8 +145,10 @@ if ($state->{method} eq 'pserver') {
     }
     my $request = $1;
     $line = <STDIN>; chomp $line;
-    req_Root('root', $line) # reuse Root
-       or die "E Invalid root $line \n";
+    unless (req_Root('root', $line)) { # reuse Root
+       print "E Invalid root $line \n";
+       exit 1;
+    }
     $line = <STDIN>; chomp $line;
     unless ($line eq 'anonymous') {
        print "E Only anonymous user allowed via pserver\n";

-- 
1.5.3.4.206.g0cef9
Morten Welinder· Oct 17, 2007, 14:39 UTC · re: Brian Gernhardt · lore

Re: [PATCH] Use exit 1 instead of die when req_Root fails.

>  made it into your repo.  It fixes test failures on my machine that have
>  been plauging me for months.

That sounds more like a reason to fix the test. "die" is the perl standard way of reporting an error. It will print the error message on stderr, not on stdout like your version does.

IMHO, of course.
Morten
Brian Gernhardt· Oct 17, 2007, 15:16 UTC · re: Morten Welinder · lore

Re: [PATCH] Use exit 1 instead of die when req_Root fails.

On Oct 17, 2007, at 10:39 AM, Morten Welinder wrote:
Show 9 quoted lines
>>  made it into your repo.  It fixes test failures on my machine  
>> that have
>>  been plauging me for months.
>
> That sounds more like a reason to fix the test.  "die" is the perl
> standard way of reporting an error.  It will print the error message
> on stderr, not on stdout like your version does.
>
> IMHO, of course.

The problem is that die can exit with varying exit codes, and exit codes >= 128 make the test suite assume something has gone wrong with the test. In particular, because $! (errcode) and $? (previous shell command return) are both 0, it returns 255. Or at least that's how it works out on my system. I'm not sure why it doesn't do that on others.

But the test is expecting a failure here and it appears to be failing in the correct way, just with an error code test-lib.sh doesn't like. I asked on list the best way to fix it and Frank Lichtenheld said (and nobody objected until now) that this was the best way to fix it.

Also, the not printing on STRERR is identical to another section of code just below mine:

Show 5 quoted lines
>     unless ($line eq 'anonymous') {
>        print "E Only anonymous user allowed via pserver\n";
>        print "I HATE YOU\n";
>        exit 1;
>     }
However, amending my patch to print to STDERR is not difficult.
~~ Brian Gernhardt
Lars Hjemli· Oct 17, 2007, 15:39 UTC · re: Brian Gernhardt · lore

Re: [PATCH] Use exit 1 instead of die when req_Root fails.

On 10/17/07, Brian Gernhardt <benji@silverinsanity.com> wrote:
> The problem is that die can exit with varying exit codes, and exit
> codes >= 128 make the test suite assume something has gone wrong with
> the test.

This makes me wonder: what about all the other instances of die() in git-cvsserver? Or in any of the other perl scripts, for that matter? Should they all be fixed, or is it this particular test that is wrong?

-- 
larsh
Brian Gernhardt· Oct 17, 2007, 18:40 UTC · re: Lars Hjemli · lore

Re: [PATCH] Use exit 1 instead of die when req_Root fails.

I wish I got this much attention the first time I tried to get this problem fixed. ;-)

On Oct 17, 2007, at 11:39 AM, Lars Hjemli wrote:
> This makes me wonder: what about all the other instances of die() in
> git-cvsserver? Or in any of the other perl scripts, for that matter?
> Should they all be fixed, or is it this particular test that is wrong?

The reason this comes up is because t/test-lib.sh:test_expect_failure () thinks codes > 128 (or negative values if you want to look at it that way) are bad tests. I believe this is because many shells use these codes to indicate things like "command not found" or other probably unexpected failures.

Other than that, does it matter what die() returns, as long as it's non-zero?

~~Brian
Lars Hjemli· Oct 17, 2007, 19:27 UTC · re: Brian Gernhardt · lore

Re: [PATCH] Use exit 1 instead of die when req_Root fails.

On 10/17/07, Brian Gernhardt <benji@silverinsanity.com> wrote:
Show 17 quoted lines
> I wish I got this much attention the first time I tried to get this
> problem fixed.  ;-)
>
> On Oct 17, 2007, at 11:39 AM, Lars Hjemli wrote:
>
> > This makes me wonder: what about all the other instances of die() in
> > git-cvsserver? Or in any of the other perl scripts, for that matter?
> > Should they all be fixed, or is it this particular test that is wrong?
>
> The reason this comes up is because t/test-lib.sh:test_expect_failure
> () thinks codes > 128 (or negative values if you want to look at it
> that way) are bad tests.  I believe this is because many shells use
> these codes to indicate things like "command not found" or other
> probably unexpected failures.
>
> Other than that, does it matter what die() returns, as long as it's
> non-zero?
My point exactly ;-)

If the test is changed to use 'test_expect_success' in the same way as 'req_Root failure (relative pathname)' does it, the test no longer depends on the exact exit-code returned by die().

IMHO this is much better future-proofing than replacing die() with print()/exit 1 whenever one of these tests fails.

-- larsh

Jan Hudec· Oct 18, 2007, 18:54 UTC · re: Brian Gernhardt · lore

Re: [PATCH] Use exit 1 instead of die when req_Root fails.

On Wed, Oct 17, 2007 at 11:16:34 -0400, Brian Gernhardt wrote:
Show 33 quoted lines
>
> On Oct 17, 2007, at 10:39 AM, Morten Welinder wrote:
>
>>>  made it into your repo.  It fixes test failures on my machine that have
>>>  been plauging me for months.
>>
>> That sounds more like a reason to fix the test.  "die" is the perl
>> standard way of reporting an error.  It will print the error message
>> on stderr, not on stdout like your version does.
>>
>> IMHO, of course.
>
> The problem is that die can exit with varying exit codes, and exit codes >= 
> 128 make the test suite assume something has gone wrong with the test.  In 
> particular, because $! (errcode) and $? (previous shell command return) are 
> both 0, it returns 255.  Or at least that's how it works out on my system.  
> I'm not sure why it doesn't do that on others.
>
> But the test is expecting a failure here and it appears to be failing in 
> the correct way, just with an error code test-lib.sh doesn't like.  I asked 
> on list the best way to fix it and Frank Lichtenheld said (and nobody 
> objected until now) that this was the best way to fix it.
>
> Also, the not printing on STRERR is identical to another section of code 
> just below mine:
>
>>     unless ($line eq 'anonymous') {
>>        print "E Only anonymous user allowed via pserver\n";
>>        print "I HATE YOU\n";
>>        exit 1;
>>     }
>
> However, amending my patch to print to STDERR is not difficult.

Hm. There are two kinds of errors in git-cvsserver and they should be handled differently.

If the error is an invalid request (like the quoted one), there should be normal print, to STDOUT, starting with "E", because it's an error message that should be sent to the client.

On the other hand if the error is internal error in the script or it's configuration, than the error should probably be reported via die, *without* E at the begining (it will be prefixed with location anyway). Such message will probably end up in the log rather than sent to the client.

-- 
						 Jan 'Bulb' Hudec <bulb@ucw.cz>
Frank Lichtenheld· Oct 17, 2007, 19:08 UTC · re: Morten Welinder · lore

Re: [PATCH] Use exit 1 instead of die when req_Root fails.

On Wed, Oct 17, 2007 at 10:39:52AM -0400, Morten Welinder wrote:
Show 8 quoted lines
> >  made it into your repo.  It fixes test failures on my machine that have
> >  been plauging me for months.
> 
> That sounds more like a reason to fix the test.  "die" is the perl
> standard way of
> reporting an error.  It will print the error message on stderr, not on
> stdout like
> your version does.

Please note that git-cvsserver is special in that its output will never be displayed directly to the user but is always interpreted first by the client. So print "E something" is acutally in some cases more correct than print STDERR something.

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/
Frank Lichtenheld· Oct 17, 2007, 19:06 UTC · re: Brian Gernhardt · lore

Re: [PATCH] Use exit 1 instead of die when req_Root fails.

On Wed, Oct 17, 2007 at 10:05:47AM -0400, Brian Gernhardt wrote:
Show 8 quoted lines
> This was causing test failures because die was exiting 255.
> 
> Signed-off-by: Brian Gernhardt <benji@silverinsanity.com>
> ---
> 
>  Shawn, I sent this in a couple weeks ago but it looks like it never
>  made it into your repo.  It fixes test failures on my machine that have
>  been plauging me for months.

I have this in my repo and will submit this with the other git-cvsserver changes. I was just waiting for either Junio to return or someone else stepping up.

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/
Brian Gernhardt· Oct 17, 2007, 19:15 UTC · re: Frank Lichtenheld · lore

Re: [PATCH] Use exit 1 instead of die when req_Root fails.

On Oct 17, 2007, at 3:06 PM, Frank Lichtenheld wrote:
> I have this in my repo and will submit this with the other git- 
> cvsserver
> changes. I was just waiting for either Junio to return or someone else
> stepping up.

Ah. I had missed that. I just dug up the patch when switching to Shawn's repo gave me those old testing errors. Had thought it had gotten lost in the shuffle.

Never mind me then, ~~ Brian

Shawn O. Pearce· Oct 18, 2007, 05:04 UTC · re: Brian Gernhardt · lore

Re: [PATCH] Use exit 1 instead of die when req_Root fails.

Brian Gernhardt <benji@silverinsanity.com> wrote:
Show 10 quoted lines
> On Oct 17, 2007, at 3:06 PM, Frank Lichtenheld wrote:
> 
> >I have this in my repo and will submit this with the other git- 
> >cvsserver
> >changes. I was just waiting for either Junio to return or someone else
> >stepping up.
> 
> Ah.  I had missed that.  I just dug up the patch when switching to  
> Shawn's repo gave me those old testing errors.  Had thought it had  
> gotten lost in the shuffle.

It did. I have your resubmit that started this thread in my INQ and will try to get to it today. But if Frank has a queue of stuff I've missed I'd like a pointer to it so I can also try to start working through it. I know I also have some other topics that Lars Hjemli accumlated in his tree that I still need to cherry-pick over into mine, but I don't see the one we are talking about in there.

-- 
Shawn.
Frank Lichtenheld· Oct 18, 2007, 13:33 UTC · re: Shawn O. Pearce · lore

Re: [PATCH] Use exit 1 instead of die when req_Root fails.

On Thu, Oct 18, 2007 at 01:04:56AM -0400, Shawn O. Pearce wrote:
Show 18 quoted lines
> Brian Gernhardt <benji@silverinsanity.com> wrote:
> > On Oct 17, 2007, at 3:06 PM, Frank Lichtenheld wrote:
> > 
> > >I have this in my repo and will submit this with the other git- 
> > >cvsserver
> > >changes. I was just waiting for either Junio to return or someone else
> > >stepping up.
> > 
> > Ah.  I had missed that.  I just dug up the patch when switching to  
> > Shawn's repo gave me those old testing errors.  Had thought it had  
> > gotten lost in the shuffle.
> 
> It did.  I have your resubmit that started this thread in my INQ and
> will try to get to it today.  But if Frank has a queue of stuff I've
> missed I'd like a pointer to it so I can also try to start working
> through it.  I know I also have some other topics that Lars Hjemli
> accumlated in his tree that I still need to cherry-pick over into
> mine, but I don't see the one we are talking about in there.
Yeah, I postponed sending it to tomorrow for some days now :/

Anyway, my current queue is visible at git://source.djpig.de/git/git-cvsserver.git cvsserver So you can fetch and merge/cherry-pick it yourself if you prefer.

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

← back to recent threads