Re: [PATCH] Cleanup git-send-email.perl:extract_valid_email

Horst von Brand [off-list ref] writes:

quoted

quoted

 	# check for a local address:
-	return $address if ($address =~ /^([\w\-]+)$/);
+	return $address if ($address =~ /^([\w\-.]+)$/);

I keep forgetting this, '+' is a valid (and useful) setup, too.

Oops...

quoted

Actually, I'm retracting my earlier ack on this.  This is way too
restrictive.  I'd rather allow an occasional invalid email address than
to reject valid ones.  I generally trust git users to know what they're
doing when entering email addresses[1].

*, $, ^, +, = are all valid characters in the username portion (not sure
about local accounts, though), and I'm sure there are more that I don't
know about.

As a general principle, I prefer to check what is legal instead of trying
to filter out what isn't.

If we start doing addr-spec in RFC2822 (page 17) ourselves, we
should rather be using Email::Valid.  A permissive sanity check
to catch obvious mistakes would be more appropriate here than
being RFC police.

I think something like the attached, on top of your patch, would
be appropriate for upcoming 1.4.0.

-- >8 --
send-email: be more lenient and just catch obvious mistakes.

This cleans up the pattern matching subroutine by introducing
two variables to hold regexp to approximately match local-part
and domain in the e-mail address.  It is meant to catch obvious
mistakes with a cheap check.

The patch also moves "scalar" to force Email::Valid->address()
to work in !wantarray environment to extract_valid_address;
earlier it was in the caller of the subroutine, which was way
too error prone.

---

diff --git a/git-send-email.perl b/git-send-email.perl
index a7a7797..700d0c3 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl

@@ -312,16 +312,18 @@ our ($message_id, $cc, %mail, $subject, 
 
 sub extract_valid_address {
 	my $address = shift;
+	my $local_part_regexp = '[^<>"\s@]+';
+	my $domain_regexp = '[^.<>"\s@]+\.[^<>"\s@]+';
 
 	# check for a local address:
-	return $address if ($address =~ /^([\w\-.]+)$/);
+	return $address if ($address =~ /^($local_part_regexp)$/);
 
 	if ($have_email_valid) {
-		return Email::Valid->address($address);
+		return scalar Email::Valid->address($address);
 	} else {
 		# less robust/correct than the monster regexp in Email::Valid,
 		# but still does a 99% job, and one less dependency
-		$address =~ /([\w\-.]+@[\w\-.]+)/;
+		$address =~ /($local_part_regexp\@$domain_regexp)/;
 		return $1;
 	}
 }

@@ -384,7 +386,7 @@ X-Mailer: git-send-email $gitversion
 		defined $pid or die $!;
 		if (!$pid) {
 			exec($smtp_server,'-i',
-			     map { scalar extract_valid_address($_) }
+			     map { extract_valid_address($_) }
 			     @recipients) or die $!;
 		}
 		print $sm "$header\n$message";