Re: [PATCH] Cleanup git-send-email.perl:extract_valid_email
From: Horst von Brand <hidden>
Date: 2016-06-15 22:42:28
Junio C Hamano [off-list ref] wrote:
Horst von Brand [off-list ref] writes:quoted
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.
OK.
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.
Right.
quoted hunk
---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@]+';
This forces a '.' in the domain, while vonbrand@localhost is perfectly
reasonable. Plus it doesn't disallow adyacent '.'s. What about:
my $domain_regexp = '[^.<>"\s@]+(\.[^<>"\s@]+)*';
(but this is probably nitpicking...)
--
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