{"thread":{"id":"4383","subject":"Re: [PATCH] Cleanup git-send-email.perl:extract_valid_email","startedAt":"2006-06-04T00:10:23Z","lastAt":"2006-06-06T06:59:00Z","messageCount":2,"participants":["Horst von Brand","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"21174","messageId":"200606040010.k540ANa4015204@laptop11.inf.utfsm.cl","threadId":"4383","inReplyTo":"normalperson@yhbt.net","subject":"Re: [PATCH] Cleanup git-send-email.perl:extract_valid_email","fromName":"Horst von Brand","fromEmail":"vonbrand@inf.utfsm.cl","sentAt":"2006-06-04T00:10:23Z","receivedAt":"2006-06-04T00:10:23Z","isPatch":true,"sender":{"key":"vonbrand@inf.utfsm.cl","avatar":"https://avatars.githubusercontent.com/u/211384?v=4"},"body":"Eric Wong wrote:\n> \"Horst H. von Brand\" wrote:\n> > - Fix the regular expressions for local addresses\n> > - Fix the fallback regexp for non-local addresses, simplify the logic\n> > \n> > Signed-off-by: Horst H. von Brand \n> > ---\n> > git-send-email.perl | 9 +++------\n> > 1 files changed, 3 insertions(+), 6 deletions(-)\n> > \n> > diff --git a/git-send-email.perl b/git-send-email.perl\n> > index ed1d89b..a7a7797 100755\n> > --- a/git-send-email.perl\n> > +++ b/git-send-email.perl\n> > @@ -314,18 +314,15 @@ sub extract_valid_address {\n> > \tmy $address = shift;\n> > \n> > \t# check for a local address:\n> > -\treturn $address if ($address =~ /^([\\w\\-]+)$/);\n> > +\treturn $address if ($address =~ /^([\\w\\-.]+)$/);\n> \n> I keep forgetting this, '+' is a valid (and useful) setup, too.\n\nOops...\n\n> > \tif ($have_email_valid) {\n> > \t\treturn Email::Valid->address($address);\n> > \t} else {\n> > \t\t# less robust/correct than the monster regexp in Email::Valid,\n> > \t\t# but still does a 99% job, and one less dependency\n> > -\t\tmy $cleaned_address;\n> > -\t\tif ($address =~ /([^\\\"<>\\s]+@[^<>\\s]+)/) {\n> > -\t\t\t$cleaned_address = $1;\n> > -\t\t}\n> > -\t\treturn $cleaned_address;\n> > +\t\t$address =~ /([\\w\\-.]+@[\\w\\-.]+)/;\n> > +\t\treturn $1;\n> \n> Actually, I'm retracting my earlier ack on this. This is way too\n> restrictive. I'd rather allow an occasional invalid email address than\n> to reject valid ones. I generally trust git users to know what they're\n> doing when entering email addresses[1].\n> \n> *, $, ^, +, = are all valid characters in the username portion (not sure\n> about local accounts, though), and I'm sure there are more that I don't\n> know about.\n\nAs a general principle, I prefer to check what is legal instead of trying\nto filter out what isn't.\n-- \nDr. Horst H. von Brand User #22616 counter.li.org\nDepartamento de Informatica Fono: +56 32 654431\nUniversidad Tecnica Federico Santa Maria +56 32 654239\nCasilla 110-V, Valparaiso, Chile Fax: +56 32 797513\n"},{"id":"21307","messageId":"7v1wu2zs5n.fsf@assigned-by-dhcp.cox.net","threadId":"4383","inReplyTo":"200606040010.k540ANa4015204@laptop11.inf.utfsm.cl","subject":"Re: [PATCH] Cleanup git-send-email.perl:extract_valid_email","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-06-06T06:59:00Z","receivedAt":"2006-06-06T06:59:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Horst von Brand writes:\n\n>> > \t# check for a local address:\n>> > -\treturn $address if ($address =~ /^([\\w\\-]+)$/);\n>> > +\treturn $address if ($address =~ /^([\\w\\-.]+)$/);\n>> \n>> I keep forgetting this, '+' is a valid (and useful) setup, too.\n>\n> Oops...\n>> \n>> Actually, I'm retracting my earlier ack on this. This is way too\n>> restrictive. I'd rather allow an occasional invalid email address than\n>> to reject valid ones. I generally trust git users to know what they're\n>> doing when entering email addresses[1].\n>> \n>> *, $, ^, +, = are all valid characters in the username portion (not sure\n>> about local accounts, though), and I'm sure there are more that I don't\n>> know about.\n>\n> As a general principle, I prefer to check what is legal instead of trying\n> to filter out what isn't.\n\nIf we start doing addr-spec in RFC2822 (page 17) ourselves, we\nshould rather be using Email::Valid. A permissive sanity check\nto catch obvious mistakes would be more appropriate here than\nbeing RFC police.\n\nI think something like the attached, on top of your patch, would\nbe appropriate for upcoming 1.4.0.\n\n-- >8 --\nsend-email: be more lenient and just catch obvious mistakes.\n\nThis cleans up the pattern matching subroutine by introducing\ntwo variables to hold regexp to approximately match local-part\nand domain in the e-mail address. It is meant to catch obvious\nmistakes with a cheap check.\n\nThe patch also moves \"scalar\" to force Email::Valid->address()\nto work in !wantarray environment to extract_valid_address;\nearlier it was in the caller of the subroutine, which was way\ntoo error prone.\n\n---\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex a7a7797..700d0c3 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -312,16 +312,18 @@ our ($message_id, $cc, %mail, $subject, \n \n sub extract_valid_address {\n \tmy $address = shift;\n+\tmy $local_part_regexp = '[^<>\"\\s@]+';\n+\tmy $domain_regexp = '[^.<>\"\\s@]+\\.[^<>\"\\s@]+';\n \n \t# check for a local address:\n-\treturn $address if ($address =~ /^([\\w\\-.]+)$/);\n+\treturn $address if ($address =~ /^($local_part_regexp)$/);\n \n \tif ($have_email_valid) {\n-\t\treturn Email::Valid->address($address);\n+\t\treturn scalar Email::Valid->address($address);\n \t} else {\n \t\t# less robust/correct than the monster regexp in Email::Valid,\n \t\t# but still does a 99% job, and one less dependency\n-\t\t$address =~ /([\\w\\-.]+@[\\w\\-.]+)/;\n+\t\t$address =~ /($local_part_regexp\\@$domain_regexp)/;\n \t\treturn $1;\n \t}\n }\n@@ -384,7 +386,7 @@ X-Mailer: git-send-email $gitversion\n \t\tdefined $pid or die $!;\n \t\tif (!$pid) {\n \t\t\texec($smtp_server,'-i',\n-\t\t\t map { scalar extract_valid_address($_) }\n+\t\t\t map { extract_valid_address($_) }\n \t\t\t @recipients) or die $!;\n \t\t}\n \t\tprint $sm \"$header\\n$message\";\n"}]}