From: Jeff King <peff@peff.net>
Date: Sun, 03 May 2009 19:46:00 GMT
Subject: Re: [PATCH 1/6] Remove return undef from validate_patch
Message-ID: <20090503194600.GB20468@coredump.intra.peff.net>
In-Reply-To: <1241010743-7020-2-git-send-email-wfp5p@virginia.edu>

On Wed, Apr 29, 2009 at 09:12:18AM -0400, Bill Pemberton wrote:

> Returning undef is rarely the correct way to return a failure.
> Replace it with return 0

No, it's the right way to return failure here. The function returns
either an error string, prefixed with the problematic line number, or
undef. So 'undef' is working as a sentinel value here, not as part of a
boolean.

That being said, the _calling_ code is a bit sloppy in checking "$error"
instead of "defined($error)". It is not an actual bug because the
beginning of the string is always a line number >= 1, so it always
triggers as desired. However, it would probably be more clear to write
it like this:

---
diff --git a/git-send-email.perl b/git-send-email.perl
index cccbf45..168b2c2 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -495,7 +495,7 @@ if ($validate) {
 	foreach my $f (@files) {
 		unless (-p $f) {
 			my $error = validate_patch($f);
-			$error and die "fatal: $f: $error\nwarning: no patches were sent\n";
+			defined($error) and die "fatal: $f: $error\nwarning: no patches were sent\n";
 		}
 	}
 }