{"thread":{"id":"10343","subject":"[PATCH] Use exit 1 instead of die when req_Root fails.","startedAt":"2007-10-17T14:05:47Z","lastAt":"2007-10-18T18:54:14Z","messageCount":12,"participants":["Brian Gernhardt","Morten Welinder","Lars Hjemli","Frank Lichtenheld","Shawn O. Pearce","Jan Hudec"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"56247","messageId":"20071017140547.GA21691@Hermes.cust.hotspot.t-mobile.com","threadId":"10343","inReplyTo":null,"subject":"[PATCH] Use exit 1 instead of die when req_Root fails.","fromName":"Brian Gernhardt","fromEmail":"benji@silverinsanity.com","sentAt":"2007-10-17T14:05:47Z","receivedAt":"2007-10-17T14:05:47Z","isPatch":true,"sender":{"key":"benji@silverinsanity.com","avatar":"https://gravatar.com/avatar/e06c101dbc25c68114d859b4a9ec7cf8a2c52fd2b0270ef0eac0e2e63ff22311?d=mp&s=160"},"body":"This was causing test failures because die was exiting 255.\n\nSigned-off-by: Brian Gernhardt <benji@silverinsanity.com>\n---\n\n Shawn, I sent this in a couple weeks ago but it looks like it never\n made it into your repo.  It fixes test failures on my machine that have\n been plauging me for months.\n\n git-cvsserver.perl |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/git-cvsserver.perl b/git-cvsserver.perl\nindex 13dbd27..0d55fec 100755\n--- a/git-cvsserver.perl\n+++ b/git-cvsserver.perl\n@@ -145,8 +145,10 @@ if ($state->{method} eq 'pserver') {\n     }\n     my $request = $1;\n     $line = <STDIN>; chomp $line;\n-    req_Root('root', $line) # reuse Root\n-       or die \"E Invalid root $line \\n\";\n+    unless (req_Root('root', $line)) { # reuse Root\n+       print \"E Invalid root $line \\n\";\n+       exit 1;\n+    }\n     $line = <STDIN>; chomp $line;\n     unless ($line eq 'anonymous') {\n        print \"E Only anonymous user allowed via pserver\\n\";\n-- \n1.5.3.4.206.g0cef9\n"},{"id":"56248","messageId":"118833cc0710170739i179e7389k1e44f70086ca88be@mail.gmail.com","threadId":"10343","inReplyTo":"20071017140547.GA21691@Hermes.cust.hotspot.t-mobile.com","subject":"Re: [PATCH] Use exit 1 instead of die when req_Root fails.","fromName":"Morten Welinder","fromEmail":"mwelinder@gmail.com","sentAt":"2007-10-17T14:39:52Z","receivedAt":"2007-10-17T14:39:52Z","isPatch":true,"sender":{"key":"mwelinder@gmail.com","avatar":null},"body":">  made it into your repo.  It fixes test failures on my machine that have\n>  been plauging me for months.\n\nThat sounds more like a reason to fix the test.  \"die\" is the perl\nstandard way of\nreporting an error.  It will print the error message on stderr, not on\nstdout like\nyour version does.\n\nIMHO, of course.\n\nMorten\n"},{"id":"56251","messageId":"DC3D43FD-F03D-4E0A-9C2B-DE56F9C16D60@silverinsanity.com","threadId":"10343","inReplyTo":"118833cc0710170739i179e7389k1e44f70086ca88be@mail.gmail.com","subject":"Re: [PATCH] Use exit 1 instead of die when req_Root fails.","fromName":"Brian Gernhardt","fromEmail":"benji@silverinsanity.com","sentAt":"2007-10-17T15:16:34Z","receivedAt":"2007-10-17T15:16:34Z","isPatch":true,"sender":{"key":"benji@silverinsanity.com","avatar":"https://gravatar.com/avatar/e06c101dbc25c68114d859b4a9ec7cf8a2c52fd2b0270ef0eac0e2e63ff22311?d=mp&s=160"},"body":"\nOn Oct 17, 2007, at 10:39 AM, Morten Welinder wrote:\n\n>>  made it into your repo.  It fixes test failures on my machine  \n>> that have\n>>  been plauging me for months.\n>\n> That sounds more like a reason to fix the test.  \"die\" is the perl\n> standard way of reporting an error.  It will print the error message\n> on stderr, not on stdout like your version does.\n>\n> IMHO, of course.\n\nThe problem is that die can exit with varying exit codes, and exit  \ncodes >= 128 make the test suite assume something has gone wrong with  \nthe test.  In particular, because $! (errcode) and $? (previous shell  \ncommand return) are both 0, it returns 255.  Or at least that's how  \nit works out on my system.  I'm not sure why it doesn't do that on  \nothers.\n\nBut the test is expecting a failure here and it appears to be failing  \nin the correct way, just with an error code test-lib.sh doesn't  \nlike.  I asked on list the best way to fix it and Frank Lichtenheld  \nsaid (and nobody objected until now) that this was the best way to  \nfix it.\n\nAlso, the not printing on STRERR is identical to another section of  \ncode just below mine:\n\n>     unless ($line eq 'anonymous') {\n>        print \"E Only anonymous user allowed via pserver\\n\";\n>        print \"I HATE YOU\\n\";\n>        exit 1;\n>     }\n\nHowever, amending my patch to print to STDERR is not difficult.\n\n~~ Brian Gernhardt\n"},{"id":"56253","messageId":"8c5c35580710170839l4b31a4fao5b41efafc5a83883@mail.gmail.com","threadId":"10343","inReplyTo":"DC3D43FD-F03D-4E0A-9C2B-DE56F9C16D60@silverinsanity.com","subject":"Re: [PATCH] Use exit 1 instead of die when req_Root fails.","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2007-10-17T15:39:47Z","receivedAt":"2007-10-17T15:39:47Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On 10/17/07, Brian Gernhardt <benji@silverinsanity.com> wrote:\n> The problem is that die can exit with varying exit codes, and exit\n> codes >= 128 make the test suite assume something has gone wrong with\n> the test.\n\nThis makes me wonder: what about all the other instances of die() in\ngit-cvsserver? Or in any of the other perl scripts, for that matter?\nShould they all be fixed, or is it this particular test that is wrong?\n\n-- \nlarsh\n"},{"id":"56275","messageId":"8B2F7666-DCEB-4D58-ACFE-F40587CD415D@silverinsanity.com","threadId":"10343","inReplyTo":"8c5c35580710170839l4b31a4fao5b41efafc5a83883@mail.gmail.com","subject":"Re: [PATCH] Use exit 1 instead of die when req_Root fails.","fromName":"Brian Gernhardt","fromEmail":"benji@silverinsanity.com","sentAt":"2007-10-17T18:40:41Z","receivedAt":"2007-10-17T18:40:41Z","isPatch":true,"sender":{"key":"benji@silverinsanity.com","avatar":"https://gravatar.com/avatar/e06c101dbc25c68114d859b4a9ec7cf8a2c52fd2b0270ef0eac0e2e63ff22311?d=mp&s=160"},"body":"I wish I got this much attention the first time I tried to get this  \nproblem fixed.  ;-)\n\nOn Oct 17, 2007, at 11:39 AM, Lars Hjemli wrote:\n\n> This makes me wonder: what about all the other instances of die() in\n> git-cvsserver? Or in any of the other perl scripts, for that matter?\n> Should they all be fixed, or is it this particular test that is wrong?\n\nThe reason this comes up is because t/test-lib.sh:test_expect_failure \n() thinks codes > 128 (or negative values if you want to look at it  \nthat way) are bad tests.  I believe this is because many shells use  \nthese codes to indicate things like \"command not found\" or other  \nprobably unexpected failures.\n\nOther than that, does it matter what die() returns, as long as it's  \nnon-zero?\n\n~~Brian\n"},{"id":"56279","messageId":"20071017190646.GC9041@planck.djpig.de","threadId":"10343","inReplyTo":"20071017140547.GA21691@Hermes.cust.hotspot.t-mobile.com","subject":"Re: [PATCH] Use exit 1 instead of die when req_Root fails.","fromName":"Frank Lichtenheld","fromEmail":"frank@lichtenheld.de","sentAt":"2007-10-17T19:06:46Z","receivedAt":"2007-10-17T19:06:46Z","isPatch":true,"sender":{"key":"frank@lichtenheld.de","avatar":"https://gravatar.com/avatar/b9f1d4b120e138f157c9e480d0818197c474628923786adb98f30017cdb99c3c?d=mp&s=160"},"body":"On Wed, Oct 17, 2007 at 10:05:47AM -0400, Brian Gernhardt wrote:\n> This was causing test failures because die was exiting 255.\n> \n> Signed-off-by: Brian Gernhardt <benji@silverinsanity.com>\n> ---\n> \n>  Shawn, I sent this in a couple weeks ago but it looks like it never\n>  made it into your repo.  It fixes test failures on my machine that have\n>  been plauging me for months.\n\nI have this in my repo and will submit this with the other git-cvsserver\nchanges. I was just waiting for either Junio to return or someone else\nstepping up.\n\nGruesse,\n-- \nFrank Lichtenheld <frank@lichtenheld.de>\nwww: http://www.djpig.de/\n"},{"id":"56280","messageId":"20071017190822.GD9041@planck.djpig.de","threadId":"10343","inReplyTo":"118833cc0710170739i179e7389k1e44f70086ca88be@mail.gmail.com","subject":"Re: [PATCH] Use exit 1 instead of die when req_Root fails.","fromName":"Frank Lichtenheld","fromEmail":"frank@lichtenheld.de","sentAt":"2007-10-17T19:08:23Z","receivedAt":"2007-10-17T19:08:23Z","isPatch":true,"sender":{"key":"frank@lichtenheld.de","avatar":"https://gravatar.com/avatar/b9f1d4b120e138f157c9e480d0818197c474628923786adb98f30017cdb99c3c?d=mp&s=160"},"body":"On Wed, Oct 17, 2007 at 10:39:52AM -0400, Morten Welinder wrote:\n> >  made it into your repo.  It fixes test failures on my machine that have\n> >  been plauging me for months.\n> \n> That sounds more like a reason to fix the test.  \"die\" is the perl\n> standard way of\n> reporting an error.  It will print the error message on stderr, not on\n> stdout like\n> your version does.\n\nPlease note that git-cvsserver is special in that its output will never\nbe displayed directly to the user but is always interpreted first by\nthe client. So print \"E something\" is acutally in some cases more\ncorrect than print STDERR something.\n\nGruesse,\n-- \nFrank Lichtenheld <frank@lichtenheld.de>\nwww: http://www.djpig.de/\n"},{"id":"56281","messageId":"98FA3712-B4E5-499A-B3E5-818FC33833DA@silverinsanity.com","threadId":"10343","inReplyTo":"20071017190646.GC9041@planck.djpig.de","subject":"Re: [PATCH] Use exit 1 instead of die when req_Root fails.","fromName":"Brian Gernhardt","fromEmail":"benji@silverinsanity.com","sentAt":"2007-10-17T19:15:26Z","receivedAt":"2007-10-17T19:15:26Z","isPatch":true,"sender":{"key":"benji@silverinsanity.com","avatar":"https://gravatar.com/avatar/e06c101dbc25c68114d859b4a9ec7cf8a2c52fd2b0270ef0eac0e2e63ff22311?d=mp&s=160"},"body":"\nOn Oct 17, 2007, at 3:06 PM, Frank Lichtenheld wrote:\n\n> I have this in my repo and will submit this with the other git- \n> cvsserver\n> changes. I was just waiting for either Junio to return or someone else\n> stepping up.\n\nAh.  I had missed that.  I just dug up the patch when switching to  \nShawn's repo gave me those old testing errors.  Had thought it had  \ngotten lost in the shuffle.\n\nNever mind me then,\n~~ Brian\n"},{"id":"56283","messageId":"8c5c35580710171227o45c6b113ve10f293a4505bab5@mail.gmail.com","threadId":"10343","inReplyTo":"8B2F7666-DCEB-4D58-ACFE-F40587CD415D@silverinsanity.com","subject":"Re: [PATCH] Use exit 1 instead of die when req_Root fails.","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2007-10-17T19:27:26Z","receivedAt":"2007-10-17T19:27:26Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On 10/17/07, Brian Gernhardt <benji@silverinsanity.com> wrote:\n> I wish I got this much attention the first time I tried to get this\n> problem fixed.  ;-)\n>\n> On Oct 17, 2007, at 11:39 AM, Lars Hjemli wrote:\n>\n> > This makes me wonder: what about all the other instances of die() in\n> > git-cvsserver? Or in any of the other perl scripts, for that matter?\n> > Should they all be fixed, or is it this particular test that is wrong?\n>\n> The reason this comes up is because t/test-lib.sh:test_expect_failure\n> () thinks codes > 128 (or negative values if you want to look at it\n> that way) are bad tests.  I believe this is because many shells use\n> these codes to indicate things like \"command not found\" or other\n> probably unexpected failures.\n>\n> Other than that, does it matter what die() returns, as long as it's\n> non-zero?\n\nMy point exactly ;-)\n\nIf the test is changed to use 'test_expect_success' in the same way as\n'req_Root failure (relative pathname)' does it, the test no longer\ndepends on the exact exit-code returned by die().\n\nIMHO this is much better future-proofing than replacing die() with\nprint()/exit 1 whenever one of these tests fails.\n\n--\nlarsh\n"},{"id":"56343","messageId":"20071018050456.GD14735@spearce.org","threadId":"10343","inReplyTo":"98FA3712-B4E5-499A-B3E5-818FC33833DA@silverinsanity.com","subject":"Re: [PATCH] Use exit 1 instead of die when req_Root fails.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-10-18T05:04:56Z","receivedAt":"2007-10-18T05:04:56Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Brian Gernhardt <benji@silverinsanity.com> wrote:\n> On Oct 17, 2007, at 3:06 PM, Frank Lichtenheld wrote:\n> \n> >I have this in my repo and will submit this with the other git- \n> >cvsserver\n> >changes. I was just waiting for either Junio to return or someone else\n> >stepping up.\n> \n> Ah.  I had missed that.  I just dug up the patch when switching to  \n> Shawn's repo gave me those old testing errors.  Had thought it had  \n> gotten lost in the shuffle.\n\nIt did.  I have your resubmit that started this thread in my INQ and\nwill try to get to it today.  But if Frank has a queue of stuff I've\nmissed I'd like a pointer to it so I can also try to start working\nthrough it.  I know I also have some other topics that Lars Hjemli\naccumlated in his tree that I still need to cherry-pick over into\nmine, but I don't see the one we are talking about in there.\n\n-- \nShawn.\n"},{"id":"56397","messageId":"20071018133322.GC3917@planck.djpig.de","threadId":"10343","inReplyTo":"20071018050456.GD14735@spearce.org","subject":"Re: [PATCH] Use exit 1 instead of die when req_Root fails.","fromName":"Frank Lichtenheld","fromEmail":"frank@lichtenheld.de","sentAt":"2007-10-18T13:33:22Z","receivedAt":"2007-10-18T13:33:22Z","isPatch":true,"sender":{"key":"frank@lichtenheld.de","avatar":"https://gravatar.com/avatar/b9f1d4b120e138f157c9e480d0818197c474628923786adb98f30017cdb99c3c?d=mp&s=160"},"body":"On Thu, Oct 18, 2007 at 01:04:56AM -0400, Shawn O. Pearce wrote:\n> Brian Gernhardt <benji@silverinsanity.com> wrote:\n> > On Oct 17, 2007, at 3:06 PM, Frank Lichtenheld wrote:\n> > \n> > >I have this in my repo and will submit this with the other git- \n> > >cvsserver\n> > >changes. I was just waiting for either Junio to return or someone else\n> > >stepping up.\n> > \n> > Ah.  I had missed that.  I just dug up the patch when switching to  \n> > Shawn's repo gave me those old testing errors.  Had thought it had  \n> > gotten lost in the shuffle.\n> \n> It did.  I have your resubmit that started this thread in my INQ and\n> will try to get to it today.  But if Frank has a queue of stuff I've\n> missed I'd like a pointer to it so I can also try to start working\n> through it.  I know I also have some other topics that Lars Hjemli\n> accumlated in his tree that I still need to cherry-pick over into\n> mine, but I don't see the one we are talking about in there.\n\nYeah, I postponed sending it to tomorrow for some days now :/\n\nAnyway, my current queue is visible at\ngit://source.djpig.de/git/git-cvsserver.git cvsserver\nSo you can fetch and merge/cherry-pick it yourself if you prefer.\n\nGruesse,\n-- \nFrank Lichtenheld <frank@lichtenheld.de>\nwww: http://www.djpig.de/\n"},{"id":"56424","messageId":"20071018185414.GL26127@efreet.light.src","threadId":"10343","inReplyTo":"DC3D43FD-F03D-4E0A-9C2B-DE56F9C16D60@silverinsanity.com","subject":"Re: [PATCH] Use exit 1 instead of die when req_Root fails.","fromName":"Jan Hudec","fromEmail":"bulb@ucw.cz","sentAt":"2007-10-18T18:54:14Z","receivedAt":"2007-10-18T18:54:14Z","isPatch":true,"sender":{"key":"bulb@ucw.cz","avatar":null},"body":"On Wed, Oct 17, 2007 at 11:16:34 -0400, Brian Gernhardt wrote:\n>\n> On Oct 17, 2007, at 10:39 AM, Morten Welinder wrote:\n>\n>>>  made it into your repo.  It fixes test failures on my machine that have\n>>>  been plauging me for months.\n>>\n>> That sounds more like a reason to fix the test.  \"die\" is the perl\n>> standard way of reporting an error.  It will print the error message\n>> on stderr, not on stdout like your version does.\n>>\n>> IMHO, of course.\n>\n> The problem is that die can exit with varying exit codes, and exit codes >= \n> 128 make the test suite assume something has gone wrong with the test.  In \n> particular, because $! (errcode) and $? (previous shell command return) are \n> both 0, it returns 255.  Or at least that's how it works out on my system.  \n> I'm not sure why it doesn't do that on others.\n>\n> But the test is expecting a failure here and it appears to be failing in \n> the correct way, just with an error code test-lib.sh doesn't like.  I asked \n> on list the best way to fix it and Frank Lichtenheld said (and nobody \n> objected until now) that this was the best way to fix it.\n>\n> Also, the not printing on STRERR is identical to another section of code \n> just below mine:\n>\n>>     unless ($line eq 'anonymous') {\n>>        print \"E Only anonymous user allowed via pserver\\n\";\n>>        print \"I HATE YOU\\n\";\n>>        exit 1;\n>>     }\n>\n> However, amending my patch to print to STDERR is not difficult.\n\nHm. There are two kinds of errors in git-cvsserver and they should be handled\ndifferently.\n\nIf the error is an invalid request (like the quoted one), there should be\nnormal print, to STDOUT, starting with \"E\", because it's an error message\nthat should be sent to the client.\n\nOn the other hand if the error is internal error in the script or it's\nconfiguration, than the error should probably be reported via die, *without*\nE at the begining (it will be prefixed with location anyway). Such message\nwill probably end up in the log rather than sent to the client.\n\n-- \n\t\t\t\t\t\t Jan 'Bulb' Hudec <bulb@ucw.cz>\n"}]}