Re: [PATCH 1/6] Remove return undef from validate_patch

From: Jeff King <peff@peff.net>
Date: May 3, 2009, 19:46 UTC
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";
 		}
 	}
 }