git-cvsserver test failures (still)

git-cvsserver test failures (still)

[Brian Gernhardt]

Periodically I am reminded that the git-cvsserver does not pass it's  
tests (t9400-git-cvsserver.sh) on my machine, so I once again ask for  
help.  The failing tests are: (they say skip because this is from the  
last "find other errors" run)

* skip 9: req_Root failure (strict-paths)
* skip 11: req_Root failure (w/o strict-paths)
* skip 13: req_Root failure (base-path)

No other tests fail, including other req_Root failure tests :

*   ok 6: req_Root failure (relative pathname)
*   ok 7: req_Root failure (conflicting roots)
*   ok 15: req_Root failure (export-all w/o whitelist)

All three tests fail with the exact same error log:

E /Users/brian/dev/git/t/trash/gitcvs.git/ does not seem to be a  
valid GIT repository
E
error 1 /Users/brian/dev/git/t/trash/gitcvs.git/ is not a valid  
repository
E Invalid root /Users/brian/dev/git/t/trash/gitcvs.git

This appears to be from git-cvsserver.perl:148-9:

     req_Root('root', $line) # reuse Root
        or die "E Invalid root $line \n";

This fails the test suite because die() exits with code 255 (checked  
with "perl -e 'die'; echo $?"), which is outside what  
test_expect_failure accepts (see t/test-lib.sh:179).

My questions become:
1) Why hasn't this hit anyone else?
2) Is this where these tests are supposed to fail?
3) If it is, should the code be using print and exit 1 instead of die?
4) If not, should the test be altered to end with "|| false" or  
similar so the test passes?

I'd happily submit a patch to fix this, but don't know what the  
correct fix is.

~~ Brian Gernhardt

Re: git-cvsserver test failures (still)

[Frank Lichtenheld]

Hi.

Some investigations on this:

On Wed, Oct 03, 2007 at 03:50:18PM -0400, Brian Gernhardt wrote:
> This appears to be from git-cvsserver.perl:148-9:
> 
>     req_Root('root', $line) # reuse Root
>        or die "E Invalid root $line \n";
> 
> This fails the test suite because die() exits with code 255 (checked  
> with "perl -e 'die'; echo $?"), which is outside what  
> test_expect_failure accepts (see t/test-lib.sh:179).
> 
> My questions become:
> 1) Why hasn't this hit anyone else?

die doesn't always quit with 255:

"exits with the current value of $! (errno).  If $! is 0, exits with the
value of "($? >> 8)" (backtick ‘command‘ status).  If "($? >> 8)" is 0,
exits with 255"

On my system $! is 9 for these cases and so it exits with exit code 9.

> 2) Is this where these tests are supposed to fail?

yes, IIRC

> 3) If it is, should the code be using print and exit 1 instead of die?

I think that would be the best solution, yes.

> 4) If not, should the test be altered to end with "|| false" or  
> similar so the test passes?

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

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

[Brian Gernhardt]

This was causing test failures because die was exiting 255.
---

 This finally takes care of my test failures.
 
 git-cvsserver.perl |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

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.203.gcc61a

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

[Brian Gernhardt]

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(-)

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

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

[Morten Welinder]

>  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

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

[Brian Gernhardt]

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.

~~ Brian Gernhardt

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

[Lars Hjemli]

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

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

[Brian Gernhardt]

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

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

[Lars Hjemli]

On 10/17/07, Brian Gernhardt <benji@silverinsanity.com> wrote:
> 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

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

[Jan Hudec]

[-- Attachment #1: Type: text/plain, Size: 2093 bytes --]

On Wed, Oct 17, 2007 at 11:16:34 -0400, Brian Gernhardt wrote:
>
> 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>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Frank Lichtenheld]

On Wed, Oct 17, 2007 at 10:39:52AM -0400, 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.

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/

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

[Frank Lichtenheld]

On Wed, Oct 17, 2007 at 10:05:47AM -0400, Brian Gernhardt wrote:
> 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/

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

[Brian Gernhardt]

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

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

[Shawn O. Pearce]

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.

-- 
Shawn.

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

[Frank Lichtenheld]

On Thu, Oct 18, 2007 at 01:04:56AM -0400, Shawn O. Pearce wrote:
> 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/