threads / patch / 4383

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

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

## tl;dr

2 messages between Jun 4, 2006 and Jun 6, 2006. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

Horst von Brand· Jun 4, 2006, 00:10 UTC · lore
Eric Wong <normalperson@yhbt.net> wrote:
Show 21 quoted lines
> "Horst H. von Brand" <vonbrand@inf.utfsm.cl> wrote:
> > - Fix the regular expressions for local addresses
> > - Fix the fallback regexp for non-local addresses, simplify the logic
> > 
> > Signed-off-by: Horst H. von Brand <vonbrand@inf.utfsm.cl>
> > ---
> >  git-send-email.perl |    9 +++------
> >  1 files changed, 3 insertions(+), 6 deletions(-)
> > 
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > index ed1d89b..a7a7797 100755
> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -314,18 +314,15 @@ sub extract_valid_address {
> >  	my $address = shift;
> >  
> >  	# 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...
Show 21 quoted lines
> >  	if ($have_email_valid) {
> >  		return 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
> > -		my $cleaned_address;
> > -		if ($address =~ /([^\"<>\s]+@[^<>\s]+)/) {
> > -			$cleaned_address = $1;
> > -		}
> > -		return $cleaned_address;
> > +		$address =~ /([\w\-.]+@[\w\-.]+)/;
> > +		return $1;
> 
> 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.

-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513
Junio C Hamano· Jun 6, 2006, 06:59 UTC · re: Horst von Brand · lore
Horst von Brand <vonbrand@inf.utfsm.cl> writes:
Show 19 quoted lines
>> >  	# 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...
>> 
>> 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.

---
Show changes to git-send-email.perl +6 −4
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";

← back to recent threads