{"thread":{"id":"47537","subject":"[RFC PATCH 1/2] add a local copy of Mail::Address from CPAN","startedAt":"2018-01-04T19:15:41Z","lastAt":"2018-02-14T15:00:03Z","messageCount":22,"participants":["Matthieu Moy","Eric Sunshine","Alex Bennée","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"335840","messageId":"1515092151-14423-1-git-send-email-git@matthieu-moy.fr","threadId":"47537","inReplyTo":null,"subject":"[RFC PATCH 1/2] add a local copy of Mail::Address from CPAN","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2018-01-04T18:55:50Z","receivedAt":"2018-01-04T19:15:41Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"We used to have two versions of the email parsing code. Our\nparse_mailboxes (in Git.pm), and Mail::Address which we used if\ninstalled. Unfortunately, both versions have different sets of bugs, and\nchanging the behavior of git depending on whether Mail::Address is\ninstalled was a bad idea.\n\nA first attempt to solve this was cc90750 (send-email: don't use\nMail::Address, even if available, 2017-08-23), but it turns out our\nparse_mailboxes is too buggy for some uses. For example the lack of\nabout nested comments support breaks get_maintainer.pl in the Linux\nkernel tree:\n\n  https://public-inbox.org/git/20171116154814.23785-1-alex.bennee@linaro.org/\n\nThis patch goes the other way: use Mail::Address anyway, but have a\nlocal copy as a fallback, when the system one is not available.\n\nThe duplicated script is small (276 lines of code) and stable in time.\nMaintaining the local copy should not be an issue, and will certainly be\nless burden than maintaining our own parse_mailboxes.\n\nAnother option would be to consider Mail::Address as a hard dependency,\nbut it's easy enough to save the trouble of extra-dependency to the end\nuser or packager.\n\nSigned-off-by: Matthieu Moy <git@matthieu-moy.fr>\n---\nI looked at the perl/Git/Error.pm wrapper, and ended up writting a\ndifferent, much simpler version. I'm not sure the same approach would\napply to Error.pm, but my straightforward version does the job for\nMail/Address.pm.\n\nI would also be fine with using our local copy unconditionaly.\n\n git-send-email.perl               |   3 +-\n perl/Git/FromCPAN/Mail/Address.pm | 276 ++++++++++++++++++++++++++++++++++++++\n perl/Git/Mail/Address.pm          |  24 ++++\n 3 files changed, 302 insertions(+), 1 deletion(-)\n create mode 100644 perl/Git/FromCPAN/Mail/Address.pm\n create mode 100755 perl/Git/Mail/Address.pm\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 02747b6..d0dcc6d 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -30,6 +30,7 @@ use Git::Error qw(:try);\n use Cwd qw(abs_path cwd);\n use Git;\n use Git::I18N;\n+use Git::Mail::Address;\n \n Getopt::Long::Configure qw/ pass_through /;\n \n@@ -489,7 +490,7 @@ my ($repoauthor, $repocommitter);\n ($repocommitter) = Git::ident_person(@repo, 'committer');\n \n sub parse_address_line {\n-\treturn Git::parse_mailboxes($_[0]);\n+\treturn map { $_->format } Mail::Address->parse($_[0]);\n }\n \n sub split_addrs {\ndiff --git a/perl/Git/FromCPAN/Mail/Address.pm b/perl/Git/FromCPAN/Mail/Address.pm\nnew file mode 100644\nindex 0000000..13b2ff7\n--- /dev/null\n+++ b/perl/Git/FromCPAN/Mail/Address.pm\n@@ -0,0 +1,276 @@\n+# Copyrights 1995-2017 by [Mark Overmeer <perl@overmeer.net>].\n+#  For other contributors see ChangeLog.\n+# See the manual pages for details on the licensing terms.\n+# Pod stripped from pm file by OODoc 2.02.\n+package Mail::Address;\n+use vars '$VERSION';\n+$VERSION = '2.19';\n+\n+use strict;\n+\n+use Carp;\n+\n+# use locale;   removed in version 1.78, because it causes taint problems\n+\n+sub Version { our $VERSION }\n+\n+\n+\n+# given a comment, attempt to extract a person's name\n+sub _extract_name\n+{   # This function can be called as method as well\n+    my $self = @_ && ref $_[0] ? shift : undef;\n+\n+    local $_ = shift\n+        or return '';\n+\n+    # Using encodings, too hard. See Mail::Message::Field::Full.\n+    return '' if m/\\=\\?.*?\\?\\=/;\n+\n+    # trim whitespace\n+    s/^\\s+//;\n+    s/\\s+$//;\n+    s/\\s+/ /;\n+\n+    # Disregard numeric names (e.g. 123456.1234@compuserve.com)\n+    return \"\" if /^[\\d ]+$/;\n+\n+    s/^\\((.*)\\)$/$1/; # remove outermost parenthesis\n+    s/^\"(.*)\"$/$1/;   # remove outer quotation marks\n+    s/\\(.*?\\)//g;     # remove minimal embedded comments\n+    s/\\\\//g;          # remove all escapes\n+    s/^\"(.*)\"$/$1/;   # remove internal quotation marks\n+    s/^([^\\s]+) ?, ?(.*)$/$2 $1/; # reverse \"Last, First M.\" if applicable\n+    s/,.*//;\n+\n+    # Change casing only when the name contains only upper or only\n+    # lower cased characters.\n+    unless( m/[A-Z]/ && m/[a-z]/ )\n+    {   # Set the case of the name to first char upper rest lower\n+        s/\\b(\\w+)/\\L\\u$1/igo;  # Upcase first letter on name\n+        s/\\bMc(\\w)/Mc\\u$1/igo; # Scottish names such as 'McLeod'\n+        s/\\bo'(\\w)/O'\\u$1/igo; # Irish names such as 'O'Malley, O'Reilly'\n+        s/\\b(x*(ix)?v*(iv)?i*)\\b/\\U$1/igo; # Roman numerals, eg 'Level III Support'\n+    }\n+\n+    # some cleanup\n+    s/\\[[^\\]]*\\]//g;\n+    s/(^[\\s'\"]+|[\\s'\"]+$)//g;\n+    s/\\s{2,}/ /g;\n+\n+    $_;\n+}\n+\n+sub _tokenise\n+{   local $_ = join ',', @_;\n+    my (@words,$snippet,$field);\n+\n+    s/\\A\\s+//;\n+    s/[\\r\\n]+/ /g;\n+\n+    while ($_ ne '')\n+    {   $field = '';\n+        if(s/^\\s*\\(/(/ )    # (...)\n+        {   my $depth = 0;\n+\n+     PAREN: while(s/^(\\(([^\\(\\)\\\\]|\\\\.)*)//)\n+            {   $field .= $1;\n+                $depth++;\n+                while(s/^(([^\\(\\)\\\\]|\\\\.)*\\)\\s*)//)\n+                {   $field .= $1;\n+                    last PAREN unless --$depth;\n+\t            $field .= $1 if s/^(([^\\(\\)\\\\]|\\\\.)+)//;\n+                }\n+            }\n+\n+            carp \"Unmatched () '$field' '$_'\"\n+                if $depth;\n+\n+            $field =~ s/\\s+\\Z//;\n+            push @words, $field;\n+\n+            next;\n+        }\n+\n+        if( s/^(\"(?:[^\"\\\\]+|\\\\.)*\")\\s*//       # \"...\"\n+         || s/^(\\[(?:[^\\]\\\\]+|\\\\.)*\\])\\s*//    # [...]\n+         || s/^([^\\s()<>\\@,;:\\\\\".[\\]]+)\\s*//\n+         || s/^([()<>\\@,;:\\\\\".[\\]])\\s*//\n+          )\n+        {   push @words, $1;\n+            next;\n+        }\n+\n+        croak \"Unrecognised line: $_\";\n+    }\n+\n+    push @words, \",\";\n+    \\@words;\n+}\n+\n+sub _find_next\n+{   my ($idx, $tokens, $len) = @_;\n+\n+    while($idx < $len)\n+    {   my $c = $tokens->[$idx];\n+        return $c if $c eq ',' || $c eq ';' || $c eq '<';\n+        $idx++;\n+    }\n+\n+    \"\";\n+}\n+\n+sub _complete\n+{   my ($class, $phrase, $address, $comment) = @_;\n+\n+    @$phrase || @$comment || @$address\n+       or return undef;\n+\n+    my $o = $class->new(join(\" \",@$phrase), join(\"\",@$address), join(\" \",@$comment));\n+    @$phrase = @$address = @$comment = ();\n+    $o;\n+}\n+\n+#------------\n+\n+sub new(@)\n+{   my $class = shift;\n+    bless [@_], $class;\n+}\n+\n+\n+sub parse(@)\n+{   my $class = shift;\n+    my @line  = grep {defined} @_;\n+    my $line  = join '', @line;\n+\n+    my (@phrase, @comment, @address, @objs);\n+    my ($depth, $idx) = (0, 0);\n+\n+    my $tokens  = _tokenise @line;\n+    my $len     = @$tokens;\n+    my $next    = _find_next $idx, $tokens, $len;\n+\n+    local $_;\n+    for(my $idx = 0; $idx < $len; $idx++)\n+    {   $_ = $tokens->[$idx];\n+\n+        if(substr($_,0,1) eq '(') { push @comment, $_ }\n+        elsif($_ eq '<')    { $depth++ }\n+        elsif($_ eq '>')    { $depth-- if $depth }\n+        elsif($_ eq ',' || $_ eq ';')\n+        {   warn \"Unmatched '<>' in $line\" if $depth;\n+            my $o = $class->_complete(\\@phrase, \\@address, \\@comment);\n+            push @objs, $o if defined $o;\n+            $depth = 0;\n+            $next = _find_next $idx+1, $tokens, $len;\n+        }\n+        elsif($depth)       { push @address, $_ }\n+        elsif($next eq '<') { push @phrase,  $_ }\n+        elsif( /^[.\\@:;]$/ || !@address || $address[-1] =~ /^[.\\@:;]$/ )\n+        {   push @address, $_ }\n+        else\n+        {   warn \"Unmatched '<>' in $line\" if $depth;\n+            my $o = $class->_complete(\\@phrase, \\@address, \\@comment);\n+            push @objs, $o if defined $o;\n+            $depth = 0;\n+            push @address, $_;\n+        }\n+    }\n+    @objs;\n+}\n+\n+#------------\n+\n+sub phrase  { shift->set_or_get(0, @_) }\n+sub address { shift->set_or_get(1, @_) }\n+sub comment { shift->set_or_get(2, @_) }\n+\n+sub set_or_get($)\n+{   my ($self, $i) = (shift, shift);\n+    @_ or return $self->[$i];\n+\n+    my $val = $self->[$i];\n+    $self->[$i] = shift if @_;\n+    $val;\n+}\n+\n+\n+my $atext = '[\\-\\w !#$%&\\'*+/=?^`{|}~]';\n+sub format\n+{   my @addrs;\n+\n+    foreach (@_)\n+    {   my ($phrase, $email, $comment) = @$_;\n+        my @addr;\n+\n+        if(defined $phrase && length $phrase)\n+        {   push @addr\n+              , $phrase =~ /^(?:\\s*$atext\\s*)+$/o ? $phrase\n+              : $phrase =~ /(?<!\\\\)\"/             ? $phrase\n+              :                                    qq(\"$phrase\");\n+\n+            push @addr, \"<$email>\"\n+                if defined $email && length $email;\n+        }\n+        elsif(defined $email && length $email)\n+        {   push @addr, $email;\n+        }\n+\n+        if(defined $comment && $comment =~ /\\S/)\n+        {   $comment =~ s/^\\s*\\(?/(/;\n+            $comment =~ s/\\)?\\s*$/)/;\n+        }\n+\n+        push @addr, $comment\n+            if defined $comment && length $comment;\n+\n+        push @addrs, join(\" \", @addr)\n+            if @addr;\n+    }\n+\n+    join \", \", @addrs;\n+}\n+\n+#------------\n+\n+sub name\n+{   my $self   = shift;\n+    my $phrase = $self->phrase;\n+    my $addr   = $self->address;\n+\n+    $phrase    = $self->comment\n+        unless defined $phrase && length $phrase;\n+\n+    my $name   = $self->_extract_name($phrase);\n+\n+    # first.last@domain address\n+    if($name eq '' && $addr =~ /([^\\%\\.\\@_]+([\\._][^\\%\\.\\@_]+)+)[\\@\\%]/)\n+    {   ($name  = $1) =~ s/[\\._]+/ /g;\n+\t$name   = _extract_name $name;\n+    }\n+\n+    if($name eq '' && $addr =~ m#/g=#i)    # X400 style address\n+    {   my ($f) = $addr =~ m#g=([^/]*)#i;\n+\tmy ($l) = $addr =~ m#s=([^/]*)#i;\n+\t$name   = _extract_name \"$f $l\";\n+    }\n+\n+    length $name ? $name : undef;\n+}\n+\n+\n+sub host\n+{   my $addr = shift->address || '';\n+    my $i    = rindex $addr, '@';\n+    $i >= 0 ? substr($addr, $i+1) : undef;\n+}\n+\n+\n+sub user\n+{   my $addr = shift->address || '';\n+    my $i    = rindex $addr, '@';\n+    $i >= 0 ? substr($addr,0,$i) : $addr;\n+}\n+\n+1;\ndiff --git a/perl/Git/Mail/Address.pm b/perl/Git/Mail/Address.pm\nnew file mode 100755\nindex 0000000..2ce3e84\n--- /dev/null\n+++ b/perl/Git/Mail/Address.pm\n@@ -0,0 +1,24 @@\n+package Git::Mail::Address;\n+use 5.008;\n+use strict;\n+use warnings;\n+\n+=head1 NAME\n+\n+Git::Mail::Address - Wrapper for the L<Mail::Address> module, in case it's not installed\n+\n+=head1 DESCRIPTION\n+\n+This module is only intended to be used for code shipping in the\n+C<git.git> repository. Use it for anything else at your peril!\n+\n+=cut\n+\n+eval {\n+    require Mail::Address;\n+    1;\n+} or do {\n+    require Git::FromCPAN::Mail::Address;\n+};\n+\n+1;\n-- \n2.8.1.116.g7b0d47b\n\n"},{"id":"335852","messageId":"1515092151-14423-2-git-send-email-git@matthieu-moy.fr","threadId":"47537","inReplyTo":"1515092151-14423-1-git-send-email-git@matthieu-moy.fr","subject":"[RFC PATCH 2/2] Remove now useless email-address parsing code","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2018-01-04T18:55:51Z","receivedAt":"2018-01-04T19:29:40Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"We now use Mail::Address unconditionaly, hence parse_mailboxes is now\ndead code. Remove it and its tests.\n\nSigned-off-by: Matthieu Moy <git@matthieu-moy.fr>\n---\n perl/Git.pm          | 71 ----------------------------------------------------\n t/t9000-addresses.sh | 27 --------------------\n t/t9000/test.pl      | 67 -------------------------------------------------\n 3 files changed, 165 deletions(-)\n delete mode 100755 t/t9000-addresses.sh\n delete mode 100755 t/t9000/test.pl\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 02a3871..9d60d79 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -880,77 +880,6 @@ sub ident_person {\n \treturn \"$ident[0] <$ident[1]>\";\n }\n \n-=item parse_mailboxes\n-\n-Return an array of mailboxes extracted from a string.\n-\n-=cut\n-\n-# Very close to Mail::Address's parser, but we still have minor\n-# differences in some cases (see t9000 for examples).\n-sub parse_mailboxes {\n-\tmy $re_comment = qr/\\((?:[^)]*)\\)/;\n-\tmy $re_quote = qr/\"(?:[^\\\"\\\\]|\\\\.)*\"/;\n-\tmy $re_word = qr/(?:[^][\"\\s()<>:;@\\\\,.]|\\\\.)+/;\n-\n-\t# divide the string in tokens of the above form\n-\tmy $re_token = qr/(?:$re_quote|$re_word|$re_comment|\\S)/;\n-\tmy @tokens = map { $_ =~ /\\s*($re_token)\\s*/g } @_;\n-\tmy $end_of_addr_seen = 0;\n-\n-\t# add a delimiter to simplify treatment for the last mailbox\n-\tpush @tokens, \",\";\n-\n-\tmy (@addr_list, @phrase, @address, @comment, @buffer) = ();\n-\tforeach my $token (@tokens) {\n-\t\tif ($token =~ /^[,;]$/) {\n-\t\t\t# if buffer still contains undeterminated strings\n-\t\t\t# append it at the end of @address or @phrase\n-\t\t\tif ($end_of_addr_seen) {\n-\t\t\t\tpush @phrase, @buffer;\n-\t\t\t} else {\n-\t\t\t\tpush @address, @buffer;\n-\t\t\t}\n-\n-\t\t\tmy $str_phrase = join ' ', @phrase;\n-\t\t\tmy $str_address = join '', @address;\n-\t\t\tmy $str_comment = join ' ', @comment;\n-\n-\t\t\t# quote are necessary if phrase contains\n-\t\t\t# special characters\n-\t\t\tif ($str_phrase =~ /[][()<>:;@\\\\,.\\000-\\037\\177]/) {\n-\t\t\t\t$str_phrase =~ s/(^|[^\\\\])\"/$1/g;\n-\t\t\t\t$str_phrase = qq[\"$str_phrase\"];\n-\t\t\t}\n-\n-\t\t\t# add \"<>\" around the address if necessary\n-\t\t\tif ($str_address ne \"\" && $str_phrase ne \"\") {\n-\t\t\t\t$str_address = qq[<$str_address>];\n-\t\t\t}\n-\n-\t\t\tmy $str_mailbox = \"$str_phrase $str_address $str_comment\";\n-\t\t\t$str_mailbox =~ s/^\\s*|\\s*$//g;\n-\t\t\tpush @addr_list, $str_mailbox if ($str_mailbox);\n-\n-\t\t\t@phrase = @address = @comment = @buffer = ();\n-\t\t\t$end_of_addr_seen = 0;\n-\t\t} elsif ($token =~ /^\\(/) {\n-\t\t\tpush @comment, $token;\n-\t\t} elsif ($token eq \"<\") {\n-\t\t\tpush @phrase, (splice @address), (splice @buffer);\n-\t\t} elsif ($token eq \">\") {\n-\t\t\t$end_of_addr_seen = 1;\n-\t\t\tpush @address, (splice @buffer);\n-\t\t} elsif ($token eq \"@\" && !$end_of_addr_seen) {\n-\t\t\tpush @address, (splice @buffer), \"@\";\n-\t\t} else {\n-\t\t\tpush @buffer, $token;\n-\t\t}\n-\t}\n-\n-\treturn @addr_list;\n-}\n-\n =item hash_object ( TYPE, FILENAME )\n \n Compute the SHA1 object id of the given C<FILENAME> considering it is\ndiff --git a/t/t9000-addresses.sh b/t/t9000-addresses.sh\ndeleted file mode 100755\nindex a1ebef6..0000000\n--- a/t/t9000-addresses.sh\n+++ /dev/null\n@@ -1,27 +0,0 @@\n-#!/bin/sh\n-\n-test_description='compare address parsing with and without Mail::Address'\n-. ./test-lib.sh\n-\n-if ! test_have_prereq PERL; then\n-\tskip_all='skipping perl interface tests, perl not available'\n-\ttest_done\n-fi\n-\n-perl -MTest::More -e 0 2>/dev/null || {\n-\tskip_all=\"Perl Test::More unavailable, skipping test\"\n-\ttest_done\n-}\n-\n-perl -MMail::Address -e 0 2>/dev/null || {\n-\tskip_all=\"Perl Mail::Address unavailable, skipping test\"\n-\ttest_done\n-}\n-\n-test_external_has_tap=1\n-\n-test_external_without_stderr \\\n-\t'Perl address parsing function' \\\n-\tperl \"$TEST_DIRECTORY\"/t9000/test.pl\n-\n-test_done\ndiff --git a/t/t9000/test.pl b/t/t9000/test.pl\ndeleted file mode 100755\nindex dfeaa9c..0000000\n--- a/t/t9000/test.pl\n+++ /dev/null\n@@ -1,67 +0,0 @@\n-#!/usr/bin/perl\n-use lib (split(/:/, $ENV{GITPERLLIB}));\n-\n-use 5.008;\n-use warnings;\n-use strict;\n-\n-use Test::More qw(no_plan);\n-use Mail::Address;\n-\n-BEGIN { use_ok('Git') }\n-\n-my @success_list = (q[Jane],\n-\tq[jdoe@example.com],\n-\tq[<jdoe@example.com>],\n-\tq[Jane <jdoe@example.com>],\n-\tq[Jane Doe <jdoe@example.com>],\n-\tq[\"Jane\" <jdoe@example.com>],\n-\tq[\"Doe, Jane\" <jdoe@example.com>],\n-\tq[\"Jane@:;\\>.,()<Doe\" <jdoe@example.com>],\n-\tq[Jane!#$%&'*+-/=?^_{|}~Doe' <jdoe@example.com>],\n-\tq[\"<jdoe@example.com>\"],\n-\tq[\"Jane jdoe@example.com\"],\n-\tq[Jane Doe <jdoe    @   example.com  >],\n-\tq[Jane       Doe <  jdoe@example.com  >],\n-\tq[Jane @ Doe @ Jane @ Doe],\n-\tq[\"Jane, 'Doe'\" <jdoe@example.com>],\n-\tq['Doe, \"Jane' <jdoe@example.com>],\n-\tq[\"Jane\" \"Do\"e <jdoe@example.com>],\n-\tq[\"Jane' Doe\" <jdoe@example.com>],\n-\tq[\"Jane Doe <jdoe@example.com>\" <jdoe@example.com>],\n-\tq[\"Jane\\\" Doe\" <jdoe@example.com>],\n-\tq[Doe, jane <jdoe@example.com>],\n-\tq[\"Jane Doe <jdoe@example.com>],\n-\tq['Jane 'Doe' <jdoe@example.com>],\n-\tq[Jane@:;\\.,()<>Doe <jdoe@example.com>],\n-\tq[Jane <jdoe@example.com> Doe],\n-\tq[<jdoe@example.com> Jane Doe]);\n-\n-my @known_failure_list = (q[Jane\\ Doe <jdoe@example.com>],\n-\tq[\"Doe, Ja\"ne <jdoe@example.com>],\n-\tq[\"Doe, Katarina\" Jane <jdoe@example.com>],\n-\tq[Jane jdoe@example.com],\n-\tq[\"Jane \"Kat\"a\" ri\"na\" \",Doe\" <jdoe@example.com>],\n-\tq[Jane Doe],\n-\tq[Jane \"Doe <jdoe@example.com>\"],\n-\tq[\\\"Jane Doe <jdoe@example.com>],\n-\tq[Jane\\\"\\\" Doe <jdoe@example.com>],\n-\tq['Jane \"Katarina\\\" \\' Doe' <jdoe@example.com>]);\n-\n-foreach my $str (@success_list) {\n-\tmy @expected = map { $_->format } Mail::Address->parse(\"$str\");\n-\tmy @actual = Git::parse_mailboxes(\"$str\");\n-\tis_deeply(\\@expected, \\@actual, qq[same output : $str]);\n-}\n-\n-TODO: {\n-\tlocal $TODO = \"known breakage\";\n-\tforeach my $str (@known_failure_list) {\n-\t\tmy @expected = map { $_->format } Mail::Address->parse(\"$str\");\n-\t\tmy @actual = Git::parse_mailboxes(\"$str\");\n-\t\tis_deeply(\\@expected, \\@actual, qq[same output : $str]);\n-\t}\n-}\n-\n-my $is_passing = eval { Test::More->is_passing };\n-exit($is_passing ? 0 : 1) unless $@ =~ /Can't locate object method/;\n-- \n2.8.1.116.g7b0d47b\n\n"},{"id":"335865","messageId":"CAPig+cTrevRN64Mv2HS25_Z6ZJAf_wyHN8jqOSx5s3dfAf_0ow@mail.gmail.com","threadId":"47537","inReplyTo":"1515092151-14423-1-git-send-email-git@matthieu-moy.fr","subject":"Re: [RFC PATCH 1/2] add a local copy of Mail::Address from CPAN","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-01-04T21:02:35Z","receivedAt":"2018-01-04T21:02:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jan 4, 2018 at 1:55 PM, Matthieu Moy <git@matthieu-moy.fr> wrote:\n> We used to have two versions of the email parsing code. Our\n> parse_mailboxes (in Git.pm), and Mail::Address which we used if\n> installed. Unfortunately, both versions have different sets of bugs, and\n> changing the behavior of git depending on whether Mail::Address is\n> installed was a bad idea.\n>\n> A first attempt to solve this was cc90750 (send-email: don't use\n> Mail::Address, even if available, 2017-08-23), but it turns out our\n> parse_mailboxes is too buggy for some uses. For example the lack of\n> about nested comments support breaks get_maintainer.pl in the Linux\n\ns/about//\n\n> kernel tree:\n>\n>   https://public-inbox.org/git/20171116154814.23785-1-alex.bennee@linaro.org/\n>\n> This patch goes the other way: use Mail::Address anyway, but have a\n> local copy as a fallback, when the system one is not available.\n>\n> The duplicated script is small (276 lines of code) and stable in time.\n> Maintaining the local copy should not be an issue, and will certainly be\n> less burden than maintaining our own parse_mailboxes.\n>\n> Another option would be to consider Mail::Address as a hard dependency,\n> but it's easy enough to save the trouble of extra-dependency to the end\n> user or packager.\n>\n> Signed-off-by: Matthieu Moy <git@matthieu-moy.fr>\n"},{"id":"335874","messageId":"87po6pcm08.fsf@linaro.org","threadId":"47537","inReplyTo":"1515092151-14423-2-git-send-email-git@matthieu-moy.fr","subject":"Re: [RFC PATCH 2/2] Remove now useless email-address parsing code","fromName":"Alex Bennée","fromEmail":"alex.bennee@linaro.org","sentAt":"2018-01-04T22:11:35Z","receivedAt":"2018-01-04T22:11:46Z","isPatch":true,"sender":{"key":"alex.bennee@linaro.org","avatar":"https://avatars.githubusercontent.com/u/22458?v=4"},"body":"\nMatthieu Moy <git@matthieu-moy.fr> writes:\n\n> We now use Mail::Address unconditionaly, hence parse_mailboxes is now\n> dead code. Remove it and its tests.\n>\n> Signed-off-by: Matthieu Moy <git@matthieu-moy.fr>\n> ---\n>  perl/Git.pm          | 71 ----------------------------------------------------\n>  t/t9000-addresses.sh | 27 --------------------\n>  t/t9000/test.pl      | 67 -------------------------------------------------\n>  3 files changed, 165 deletions(-)\n>  delete mode 100755 t/t9000-addresses.sh\n>  delete mode 100755 t/t9000/test.pl\n\nShould we add the tests for t9001-send-email.sh to guard against regressions?\n\n>\n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index 02a3871..9d60d79 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -880,77 +880,6 @@ sub ident_person {\n>  \treturn \"$ident[0] <$ident[1]>\";\n>  }\n>\n> -=item parse_mailboxes\n> -\n> -Return an array of mailboxes extracted from a string.\n> -\n> -=cut\n> -\n> -# Very close to Mail::Address's parser, but we still have minor\n> -# differences in some cases (see t9000 for examples).\n> -sub parse_mailboxes {\n> -\tmy $re_comment = qr/\\((?:[^)]*)\\)/;\n> -\tmy $re_quote = qr/\"(?:[^\\\"\\\\]|\\\\.)*\"/;\n> -\tmy $re_word = qr/(?:[^][\"\\s()<>:;@\\\\,.]|\\\\.)+/;\n> -\n> -\t# divide the string in tokens of the above form\n> -\tmy $re_token = qr/(?:$re_quote|$re_word|$re_comment|\\S)/;\n> -\tmy @tokens = map { $_ =~ /\\s*($re_token)\\s*/g } @_;\n> -\tmy $end_of_addr_seen = 0;\n> -\n> -\t# add a delimiter to simplify treatment for the last mailbox\n> -\tpush @tokens, \",\";\n> -\n> -\tmy (@addr_list, @phrase, @address, @comment, @buffer) = ();\n> -\tforeach my $token (@tokens) {\n> -\t\tif ($token =~ /^[,;]$/) {\n> -\t\t\t# if buffer still contains undeterminated strings\n> -\t\t\t# append it at the end of @address or @phrase\n> -\t\t\tif ($end_of_addr_seen) {\n> -\t\t\t\tpush @phrase, @buffer;\n> -\t\t\t} else {\n> -\t\t\t\tpush @address, @buffer;\n> -\t\t\t}\n> -\n> -\t\t\tmy $str_phrase = join ' ', @phrase;\n> -\t\t\tmy $str_address = join '', @address;\n> -\t\t\tmy $str_comment = join ' ', @comment;\n> -\n> -\t\t\t# quote are necessary if phrase contains\n> -\t\t\t# special characters\n> -\t\t\tif ($str_phrase =~ /[][()<>:;@\\\\,.\\000-\\037\\177]/) {\n> -\t\t\t\t$str_phrase =~ s/(^|[^\\\\])\"/$1/g;\n> -\t\t\t\t$str_phrase = qq[\"$str_phrase\"];\n> -\t\t\t}\n> -\n> -\t\t\t# add \"<>\" around the address if necessary\n> -\t\t\tif ($str_address ne \"\" && $str_phrase ne \"\") {\n> -\t\t\t\t$str_address = qq[<$str_address>];\n> -\t\t\t}\n> -\n> -\t\t\tmy $str_mailbox = \"$str_phrase $str_address $str_comment\";\n> -\t\t\t$str_mailbox =~ s/^\\s*|\\s*$//g;\n> -\t\t\tpush @addr_list, $str_mailbox if ($str_mailbox);\n> -\n> -\t\t\t@phrase = @address = @comment = @buffer = ();\n> -\t\t\t$end_of_addr_seen = 0;\n> -\t\t} elsif ($token =~ /^\\(/) {\n> -\t\t\tpush @comment, $token;\n> -\t\t} elsif ($token eq \"<\") {\n> -\t\t\tpush @phrase, (splice @address), (splice @buffer);\n> -\t\t} elsif ($token eq \">\") {\n> -\t\t\t$end_of_addr_seen = 1;\n> -\t\t\tpush @address, (splice @buffer);\n> -\t\t} elsif ($token eq \"@\" && !$end_of_addr_seen) {\n> -\t\t\tpush @address, (splice @buffer), \"@\";\n> -\t\t} else {\n> -\t\t\tpush @buffer, $token;\n> -\t\t}\n> -\t}\n> -\n> -\treturn @addr_list;\n> -}\n> -\n>  =item hash_object ( TYPE, FILENAME )\n>\n>  Compute the SHA1 object id of the given C<FILENAME> considering it is\n> diff --git a/t/t9000-addresses.sh b/t/t9000-addresses.sh\n> deleted file mode 100755\n> index a1ebef6..0000000\n> --- a/t/t9000-addresses.sh\n> +++ /dev/null\n> @@ -1,27 +0,0 @@\n> -#!/bin/sh\n> -\n> -test_description='compare address parsing with and without Mail::Address'\n> -. ./test-lib.sh\n> -\n> -if ! test_have_prereq PERL; then\n> -\tskip_all='skipping perl interface tests, perl not available'\n> -\ttest_done\n> -fi\n> -\n> -perl -MTest::More -e 0 2>/dev/null || {\n> -\tskip_all=\"Perl Test::More unavailable, skipping test\"\n> -\ttest_done\n> -}\n> -\n> -perl -MMail::Address -e 0 2>/dev/null || {\n> -\tskip_all=\"Perl Mail::Address unavailable, skipping test\"\n> -\ttest_done\n> -}\n> -\n> -test_external_has_tap=1\n> -\n> -test_external_without_stderr \\\n> -\t'Perl address parsing function' \\\n> -\tperl \"$TEST_DIRECTORY\"/t9000/test.pl\n> -\n> -test_done\n> diff --git a/t/t9000/test.pl b/t/t9000/test.pl\n> deleted file mode 100755\n> index dfeaa9c..0000000\n> --- a/t/t9000/test.pl\n> +++ /dev/null\n> @@ -1,67 +0,0 @@\n> -#!/usr/bin/perl\n> -use lib (split(/:/, $ENV{GITPERLLIB}));\n> -\n> -use 5.008;\n> -use warnings;\n> -use strict;\n> -\n> -use Test::More qw(no_plan);\n> -use Mail::Address;\n> -\n> -BEGIN { use_ok('Git') }\n> -\n> -my @success_list = (q[Jane],\n> -\tq[jdoe@example.com],\n> -\tq[<jdoe@example.com>],\n> -\tq[Jane <jdoe@example.com>],\n> -\tq[Jane Doe <jdoe@example.com>],\n> -\tq[\"Jane\" <jdoe@example.com>],\n> -\tq[\"Doe, Jane\" <jdoe@example.com>],\n> -\tq[\"Jane@:;\\>.,()<Doe\" <jdoe@example.com>],\n> -\tq[Jane!#$%&'*+-/=?^_{|}~Doe' <jdoe@example.com>],\n> -\tq[\"<jdoe@example.com>\"],\n> -\tq[\"Jane jdoe@example.com\"],\n> -\tq[Jane Doe <jdoe    @   example.com  >],\n> -\tq[Jane       Doe <  jdoe@example.com  >],\n> -\tq[Jane @ Doe @ Jane @ Doe],\n> -\tq[\"Jane, 'Doe'\" <jdoe@example.com>],\n> -\tq['Doe, \"Jane' <jdoe@example.com>],\n> -\tq[\"Jane\" \"Do\"e <jdoe@example.com>],\n> -\tq[\"Jane' Doe\" <jdoe@example.com>],\n> -\tq[\"Jane Doe <jdoe@example.com>\" <jdoe@example.com>],\n> -\tq[\"Jane\\\" Doe\" <jdoe@example.com>],\n> -\tq[Doe, jane <jdoe@example.com>],\n> -\tq[\"Jane Doe <jdoe@example.com>],\n> -\tq['Jane 'Doe' <jdoe@example.com>],\n> -\tq[Jane@:;\\.,()<>Doe <jdoe@example.com>],\n> -\tq[Jane <jdoe@example.com> Doe],\n> -\tq[<jdoe@example.com> Jane Doe]);\n> -\n> -my @known_failure_list = (q[Jane\\ Doe <jdoe@example.com>],\n> -\tq[\"Doe, Ja\"ne <jdoe@example.com>],\n> -\tq[\"Doe, Katarina\" Jane <jdoe@example.com>],\n> -\tq[Jane jdoe@example.com],\n> -\tq[\"Jane \"Kat\"a\" ri\"na\" \",Doe\" <jdoe@example.com>],\n> -\tq[Jane Doe],\n> -\tq[Jane \"Doe <jdoe@example.com>\"],\n> -\tq[\\\"Jane Doe <jdoe@example.com>],\n> -\tq[Jane\\\"\\\" Doe <jdoe@example.com>],\n> -\tq['Jane \"Katarina\\\" \\' Doe' <jdoe@example.com>]);\n> -\n> -foreach my $str (@success_list) {\n> -\tmy @expected = map { $_->format } Mail::Address->parse(\"$str\");\n> -\tmy @actual = Git::parse_mailboxes(\"$str\");\n> -\tis_deeply(\\@expected, \\@actual, qq[same output : $str]);\n> -}\n> -\n> -TODO: {\n> -\tlocal $TODO = \"known breakage\";\n> -\tforeach my $str (@known_failure_list) {\n> -\t\tmy @expected = map { $_->format } Mail::Address->parse(\"$str\");\n> -\t\tmy @actual = Git::parse_mailboxes(\"$str\");\n> -\t\tis_deeply(\\@expected, \\@actual, qq[same output : $str]);\n> -\t}\n> -}\n> -\n> -my $is_passing = eval { Test::More->is_passing };\n> -exit($is_passing ? 0 : 1) unless $@ =~ /Can't locate object method/;\n\n\n--\nAlex Bennée\n"},{"id":"335920","messageId":"q7h9wp0wod8y.fsf@orange.lip.ens-lyon.fr","threadId":"47537","inReplyTo":"87po6pcm08.fsf@linaro.org","subject":"Re: [RFC PATCH 2/2] Remove now useless email-address parsing code","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2018-01-05T09:39:57Z","receivedAt":"2018-01-05T09:40:17Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Alex Bennée <alex.bennee@linaro.org> writes:\n\n> Matthieu Moy <git@matthieu-moy.fr> writes:\n>\n>> We now use Mail::Address unconditionaly, hence parse_mailboxes is now\n>> dead code. Remove it and its tests.\n>>\n>> Signed-off-by: Matthieu Moy <git@matthieu-moy.fr>\n>> ---\n>>  perl/Git.pm          | 71 ----------------------------------------------------\n>>  t/t9000-addresses.sh | 27 --------------------\n>>  t/t9000/test.pl      | 67 -------------------------------------------------\n>>  3 files changed, 165 deletions(-)\n>>  delete mode 100755 t/t9000-addresses.sh\n>>  delete mode 100755 t/t9000/test.pl\n>\n> Should we add the tests for t9001-send-email.sh to guard against regressions?\n\nTests in t9001 were only useful with our parse_mailboxes (they were just\ncomparing parse_mailboxes and Mail::Address), so there's no point\nkeeping them after we delete parse_mailboxes.\n\nYour added tests from\nhttps://public-inbox.org/git/20171116154814.23785-1-alex.bennee@linaro.org\nwould make sense OTOH. Not breaking Linux's flow is a nice thing to\ndo ... Patch doing this follows (I'll resend the whole series with\nEric's nit later).\n\n-- \nMatthieu Moy\nhttps://matthieu-moy.fr/\n"},{"id":"335921","messageId":"1515147109-8077-1-git-send-email-git@matthieu-moy.fr","threadId":"47537","inReplyTo":"q7h9wp0wod8y.fsf@orange.lip.ens-lyon.fr","subject":"[PATCH] send-email: add test for Linux's get_maintainer.pl","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2018-01-05T10:11:49Z","receivedAt":"2018-01-05T10:12:09Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"From: Alex Bennée <alex.bennee@linaro.org>\n\nWe had a regression that broke Linux's get_maintainer.pl. Using\nMail::Address to parse email addresses fixed it, but let's protect\nagainst future regressions.\n\nPatch-edited-by: Matthieu Moy <git@matthieu-moy.fr>\nSigned-off-by: Alex Bennée <alex.bennee@linaro.org>\nSigned-off-by: Matthieu Moy <git@matthieu-moy.fr>\n---\n t/t9001-send-email.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 4d261c2..f126177 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -172,6 +172,28 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '\n \ttest_cmp expected-cc commandline1\n '\n \n+test_expect_success $PREREQ 'setup fake get_maintainer.pl script for cc trailer' \"\n+\tcat >expected-cc-script.sh <<-EOF &&\n+\t#!/bin/sh\n+\techo 'One Person <one@example.com> (supporter:THIS (FOO/bar))'\n+\techo 'Two Person <two@example.com> (maintainer:THIS THING)'\n+\techo 'Third List <three@example.com> (moderated list:THIS THING (FOO/bar))'\n+\techo '<four@example.com> (moderated list:FOR THING)'\n+\techo 'five@example.com (open list:FOR THING (FOO/bar))'\n+\techo 'six@example.com (open list)'\n+\tEOF\n+\tchmod +x expected-cc-script.sh\n+\"\n+\n+test_expect_success $PREREQ 'cc trailer with get_maintainer.pl output' '\n+\ttest_commit cc-trailer-getmaint &&\n+\tclean_fake_sendmail &&\n+\tgit send-email -1 --to=recipient@example.com \\\n+\t\t--cc-cmd=\"./expected-cc-script.sh\" \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" &&\n+\ttest_cmp expected-cc commandline1\n+'\n+\n test_expect_success $PREREQ 'setup expect' \"\n cat >expected-show-all-headers <<\\EOF\n 0001-Second.patch\n-- \n2.7.4\n\n"},{"id":"335924","messageId":"87o9m8d09u.fsf@linaro.org","threadId":"47537","inReplyTo":"1515147109-8077-1-git-send-email-git@matthieu-moy.fr","subject":"Re: [PATCH] send-email: add test for Linux's get_maintainer.pl","fromName":"Alex Bennée","fromEmail":"alex.bennee@linaro.org","sentAt":"2018-01-05T11:15:41Z","receivedAt":"2018-01-05T11:15:48Z","isPatch":true,"sender":{"key":"alex.bennee@linaro.org","avatar":"https://avatars.githubusercontent.com/u/22458?v=4"},"body":"\nMatthieu Moy <git@matthieu-moy.fr> writes:\n\n> From: Alex Bennée <alex.bennee@linaro.org>\n>\n> We had a regression that broke Linux's get_maintainer.pl. Using\n> Mail::Address to parse email addresses fixed it, but let's protect\n> against future regressions.\n>\n> Patch-edited-by: Matthieu Moy <git@matthieu-moy.fr>\n> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>\n> Signed-off-by: Matthieu Moy <git@matthieu-moy.fr>\n> ---\n>  t/t9001-send-email.sh | 22 ++++++++++++++++++++++\n>  1 file changed, 22 insertions(+)\n>\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> index 4d261c2..f126177 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -172,6 +172,28 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '\n>  \ttest_cmp expected-cc commandline1\n>  '\n>\n> +test_expect_success $PREREQ 'setup fake get_maintainer.pl script for cc trailer' \"\n> +\tcat >expected-cc-script.sh <<-EOF &&\n> +\t#!/bin/sh\n> +\techo 'One Person <one@example.com> (supporter:THIS (FOO/bar))'\n> +\techo 'Two Person <two@example.com> (maintainer:THIS THING)'\n> +\techo 'Third List <three@example.com> (moderated list:THIS THING (FOO/bar))'\n> +\techo '<four@example.com> (moderated list:FOR THING)'\n> +\techo 'five@example.com (open list:FOR THING (FOO/bar))'\n> +\techo 'six@example.com (open list)'\n> +\tEOF\n> +\tchmod +x expected-cc-script.sh\n> +\"\n> +\n> +test_expect_success $PREREQ 'cc trailer with get_maintainer.pl output' '\n> +\ttest_commit cc-trailer-getmaint &&\n> +\tclean_fake_sendmail &&\n> +\tgit send-email -1 --to=recipient@example.com \\\n> +\t\t--cc-cmd=\"./expected-cc-script.sh\" \\\n> +\t\t--smtp-server=\"$(pwd)/fake.sendmail\" &&\n> +\ttest_cmp expected-cc commandline1\n> +'\n> +\n>  test_expect_success $PREREQ 'setup expect' \"\n>  cat >expected-show-all-headers <<\\EOF\n>  0001-Second.patch\n\nI think you need to apply Eric's suggestions from:\n\n  From: Eric Sunshine <sunshine@sunshineco.com>\n  Date: Sat, 18 Nov 2017 21:54:46 -0500\n  Message-ID: <CAPig+cSh0tVVkh0xF9FwCfM4gngAWMSN_FXd2zhzHcy2trYXfw@mail.gmail.com>\n\n--\nAlex Bennée\n"},{"id":"335926","messageId":"q7h9h8s0o7vj.fsf@orange.lip.ens-lyon.fr","threadId":"47537","inReplyTo":"87o9m8d09u.fsf@linaro.org","subject":"Re: [PATCH] send-email: add test for Linux's get_maintainer.pl","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@univ-lyon1.fr","sentAt":"2018-01-05T11:36:00Z","receivedAt":"2018-01-05T11:36:20Z","isPatch":true,"sender":{"key":"matthieu.moy@univ-lyon1.fr","avatar":"https://gravatar.com/avatar/8ab83b763226bd297b59ddd1463a8bfc852924577191233f73a953b86888fe0c?d=mp&s=160"},"body":"Alex Bennée <alex.bennee@linaro.org> writes:\n\n> I think you need to apply Eric's suggestions from:\n>\n>   From: Eric Sunshine <sunshine@sunshineco.com>\n>   Date: Sat, 18 Nov 2017 21:54:46 -0500\n>   Message-ID: <CAPig+cSh0tVVkh0xF9FwCfM4gngAWMSN_FXd2zhzHcy2trYXfw@mail.gmail.com>\n\nIndeed. I'm squashing this into the patch:\n\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex f126177..d13d8c3 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -173,8 +173,7 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '\n '\n \n test_expect_success $PREREQ 'setup fake get_maintainer.pl script for cc trailer' \"\n-\tcat >expected-cc-script.sh <<-EOF &&\n-\t#!/bin/sh\n+\twrite_script expected-cc-script.sh <<-EOF &&\n \techo 'One Person <one@example.com> (supporter:THIS (FOO/bar))'\n \techo 'Two Person <two@example.com> (maintainer:THIS THING)'\n \techo 'Third List <three@example.com> (moderated list:THIS THING (FOO/bar))'\n@@ -186,7 +185,6 @@ test_expect_success $PREREQ 'setup fake get_maintainer.pl script for cc trailer'\n \"\n \n test_expect_success $PREREQ 'cc trailer with get_maintainer.pl output' '\n-\ttest_commit cc-trailer-getmaint &&\n \tclean_fake_sendmail &&\n \tgit send-email -1 --to=recipient@example.com \\\n \t\t--cc-cmd=\"./expected-cc-script.sh\" \\\n\n\n-- \nMatthieu Moy\nhttps://matthieu-moy.fr/\n"},{"id":"335928","messageId":"87shbkbd6r.fsf@evledraar.gmail.com","threadId":"47537","inReplyTo":"1515092151-14423-1-git-send-email-git@matthieu-moy.fr","subject":"Re: [RFC PATCH 1/2] add a local copy of Mail::Address from CPAN","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-01-05T14:19:40Z","receivedAt":"2018-01-05T14:19:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 04 2018, Matthieu Moy jotted:\n\n> I looked at the perl/Git/Error.pm wrapper, and ended up writting a\n> different, much simpler version. I'm not sure the same approach would\n> apply to Error.pm, but my straightforward version does the job for\n> Mail/Address.pm.\n\nYeah, yours is much simpler because Mail::Address doesn't have an import\nmethod, which is the entire complexity in the Error.pm wrapper.\n\nI'll probably submit a wrapper-for-the-wrappers patch at some point\nafter this gets in, i.e. both of these would become:\n\n    package Git::Error;\n    use Git::WrapCPAN 'Error';\n    1;\n\n    package Git::Mail::Address;\n    use Git::WrapCPAN 'Mail::Address';\n    1;\n\nThen the Git::WrapCPAN package would do all the magic the Git::Error\nwrapper is doing now, but with a configurable package.\n\nBut this doesn't have to wait for that.\n\n> I would also be fine with using our local copy unconditionaly.\n\nSome notes on your patch:\n\n * If I comment out your whole eval/or-do I was puzzled because tests\n   still pass, turns out the eval { require Email::Valid } will bring in\n   Mail::Address from CPAN.\n\n   Not a bug, just something to note for others poking at this.\n\n * You didn't update t/t9000/test.pl to use the wrapper, which I thought\n   was a bug until I realized this is built on top of\n   gitster/mm/send-email-fallback-to-local-mail-address.\n"},{"id":"335934","messageId":"1515177413-12526-1-git-send-email-git@matthieu-moy.fr","threadId":"47537","inReplyTo":"1515092151-14423-1-git-send-email-git@matthieu-moy.fr","subject":"[PATCH v2 1/3] send-email: add and use a local copy of Mail::Address","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2018-01-05T18:36:51Z","receivedAt":"2018-01-05T18:37:27Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"We used to have two versions of the email parsing code. Our\nparse_mailboxes (in Git.pm), and Mail::Address which we used if\ninstalled. Unfortunately, both versions have different sets of bugs, and\nchanging the behavior of git depending on whether Mail::Address is\ninstalled was a bad idea.\n\nA first attempt to solve this was cc90750 (send-email: don't use\nMail::Address, even if available, 2017-08-23), but it turns out our\nparse_mailboxes is too buggy for some uses. For example the lack of\nnested comments support breaks get_maintainer.pl in the Linux kernel\ntree:\n\n  https://public-inbox.org/git/20171116154814.23785-1-alex.bennee@linaro.org/\n\nThis patch goes the other way: use Mail::Address anyway, but have a\nlocal copy from CPAN as a fallback, when the system one is not\navailable.\n\nThe duplicated script is small (276 lines of code) and stable in time.\nMaintaining the local copy should not be an issue, and will certainly be\nless burden than maintaining our own parse_mailboxes.\n\nAnother option would be to consider Mail::Address as a hard dependency,\nbut it's easy enough to save the trouble of extra-dependency to the end\nuser or packager.\n\nSigned-off-by: Matthieu Moy <git@matthieu-moy.fr>\n---\nChange since v1: just reworded the commit message.\n\n git-send-email.perl               |   3 +-\n perl/Git/FromCPAN/Mail/Address.pm | 276 ++++++++++++++++++++++++++++++++++++++\n perl/Git/Mail/Address.pm          |  24 ++++\n 3 files changed, 302 insertions(+), 1 deletion(-)\n create mode 100644 perl/Git/FromCPAN/Mail/Address.pm\n create mode 100755 perl/Git/Mail/Address.pm\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex edcc6d3..340b5c8 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -30,6 +30,7 @@ use Error qw(:try);\n use Cwd qw(abs_path cwd);\n use Git;\n use Git::I18N;\n+use Git::Mail::Address;\n \n Getopt::Long::Configure qw/ pass_through /;\n \n@@ -489,7 +490,7 @@ my ($repoauthor, $repocommitter);\n ($repocommitter) = Git::ident_person(@repo, 'committer');\n \n sub parse_address_line {\n-\treturn Git::parse_mailboxes($_[0]);\n+\treturn map { $_->format } Mail::Address->parse($_[0]);\n }\n \n sub split_addrs {\ndiff --git a/perl/Git/FromCPAN/Mail/Address.pm b/perl/Git/FromCPAN/Mail/Address.pm\nnew file mode 100644\nindex 0000000..13b2ff7\n--- /dev/null\n+++ b/perl/Git/FromCPAN/Mail/Address.pm\n@@ -0,0 +1,276 @@\n+# Copyrights 1995-2017 by [Mark Overmeer <perl@overmeer.net>].\n+#  For other contributors see ChangeLog.\n+# See the manual pages for details on the licensing terms.\n+# Pod stripped from pm file by OODoc 2.02.\n+package Mail::Address;\n+use vars '$VERSION';\n+$VERSION = '2.19';\n+\n+use strict;\n+\n+use Carp;\n+\n+# use locale;   removed in version 1.78, because it causes taint problems\n+\n+sub Version { our $VERSION }\n+\n+\n+\n+# given a comment, attempt to extract a person's name\n+sub _extract_name\n+{   # This function can be called as method as well\n+    my $self = @_ && ref $_[0] ? shift : undef;\n+\n+    local $_ = shift\n+        or return '';\n+\n+    # Using encodings, too hard. See Mail::Message::Field::Full.\n+    return '' if m/\\=\\?.*?\\?\\=/;\n+\n+    # trim whitespace\n+    s/^\\s+//;\n+    s/\\s+$//;\n+    s/\\s+/ /;\n+\n+    # Disregard numeric names (e.g. 123456.1234@compuserve.com)\n+    return \"\" if /^[\\d ]+$/;\n+\n+    s/^\\((.*)\\)$/$1/; # remove outermost parenthesis\n+    s/^\"(.*)\"$/$1/;   # remove outer quotation marks\n+    s/\\(.*?\\)//g;     # remove minimal embedded comments\n+    s/\\\\//g;          # remove all escapes\n+    s/^\"(.*)\"$/$1/;   # remove internal quotation marks\n+    s/^([^\\s]+) ?, ?(.*)$/$2 $1/; # reverse \"Last, First M.\" if applicable\n+    s/,.*//;\n+\n+    # Change casing only when the name contains only upper or only\n+    # lower cased characters.\n+    unless( m/[A-Z]/ && m/[a-z]/ )\n+    {   # Set the case of the name to first char upper rest lower\n+        s/\\b(\\w+)/\\L\\u$1/igo;  # Upcase first letter on name\n+        s/\\bMc(\\w)/Mc\\u$1/igo; # Scottish names such as 'McLeod'\n+        s/\\bo'(\\w)/O'\\u$1/igo; # Irish names such as 'O'Malley, O'Reilly'\n+        s/\\b(x*(ix)?v*(iv)?i*)\\b/\\U$1/igo; # Roman numerals, eg 'Level III Support'\n+    }\n+\n+    # some cleanup\n+    s/\\[[^\\]]*\\]//g;\n+    s/(^[\\s'\"]+|[\\s'\"]+$)//g;\n+    s/\\s{2,}/ /g;\n+\n+    $_;\n+}\n+\n+sub _tokenise\n+{   local $_ = join ',', @_;\n+    my (@words,$snippet,$field);\n+\n+    s/\\A\\s+//;\n+    s/[\\r\\n]+/ /g;\n+\n+    while ($_ ne '')\n+    {   $field = '';\n+        if(s/^\\s*\\(/(/ )    # (...)\n+        {   my $depth = 0;\n+\n+     PAREN: while(s/^(\\(([^\\(\\)\\\\]|\\\\.)*)//)\n+            {   $field .= $1;\n+                $depth++;\n+                while(s/^(([^\\(\\)\\\\]|\\\\.)*\\)\\s*)//)\n+                {   $field .= $1;\n+                    last PAREN unless --$depth;\n+\t            $field .= $1 if s/^(([^\\(\\)\\\\]|\\\\.)+)//;\n+                }\n+            }\n+\n+            carp \"Unmatched () '$field' '$_'\"\n+                if $depth;\n+\n+            $field =~ s/\\s+\\Z//;\n+            push @words, $field;\n+\n+            next;\n+        }\n+\n+        if( s/^(\"(?:[^\"\\\\]+|\\\\.)*\")\\s*//       # \"...\"\n+         || s/^(\\[(?:[^\\]\\\\]+|\\\\.)*\\])\\s*//    # [...]\n+         || s/^([^\\s()<>\\@,;:\\\\\".[\\]]+)\\s*//\n+         || s/^([()<>\\@,;:\\\\\".[\\]])\\s*//\n+          )\n+        {   push @words, $1;\n+            next;\n+        }\n+\n+        croak \"Unrecognised line: $_\";\n+    }\n+\n+    push @words, \",\";\n+    \\@words;\n+}\n+\n+sub _find_next\n+{   my ($idx, $tokens, $len) = @_;\n+\n+    while($idx < $len)\n+    {   my $c = $tokens->[$idx];\n+        return $c if $c eq ',' || $c eq ';' || $c eq '<';\n+        $idx++;\n+    }\n+\n+    \"\";\n+}\n+\n+sub _complete\n+{   my ($class, $phrase, $address, $comment) = @_;\n+\n+    @$phrase || @$comment || @$address\n+       or return undef;\n+\n+    my $o = $class->new(join(\" \",@$phrase), join(\"\",@$address), join(\" \",@$comment));\n+    @$phrase = @$address = @$comment = ();\n+    $o;\n+}\n+\n+#------------\n+\n+sub new(@)\n+{   my $class = shift;\n+    bless [@_], $class;\n+}\n+\n+\n+sub parse(@)\n+{   my $class = shift;\n+    my @line  = grep {defined} @_;\n+    my $line  = join '', @line;\n+\n+    my (@phrase, @comment, @address, @objs);\n+    my ($depth, $idx) = (0, 0);\n+\n+    my $tokens  = _tokenise @line;\n+    my $len     = @$tokens;\n+    my $next    = _find_next $idx, $tokens, $len;\n+\n+    local $_;\n+    for(my $idx = 0; $idx < $len; $idx++)\n+    {   $_ = $tokens->[$idx];\n+\n+        if(substr($_,0,1) eq '(') { push @comment, $_ }\n+        elsif($_ eq '<')    { $depth++ }\n+        elsif($_ eq '>')    { $depth-- if $depth }\n+        elsif($_ eq ',' || $_ eq ';')\n+        {   warn \"Unmatched '<>' in $line\" if $depth;\n+            my $o = $class->_complete(\\@phrase, \\@address, \\@comment);\n+            push @objs, $o if defined $o;\n+            $depth = 0;\n+            $next = _find_next $idx+1, $tokens, $len;\n+        }\n+        elsif($depth)       { push @address, $_ }\n+        elsif($next eq '<') { push @phrase,  $_ }\n+        elsif( /^[.\\@:;]$/ || !@address || $address[-1] =~ /^[.\\@:;]$/ )\n+        {   push @address, $_ }\n+        else\n+        {   warn \"Unmatched '<>' in $line\" if $depth;\n+            my $o = $class->_complete(\\@phrase, \\@address, \\@comment);\n+            push @objs, $o if defined $o;\n+            $depth = 0;\n+            push @address, $_;\n+        }\n+    }\n+    @objs;\n+}\n+\n+#------------\n+\n+sub phrase  { shift->set_or_get(0, @_) }\n+sub address { shift->set_or_get(1, @_) }\n+sub comment { shift->set_or_get(2, @_) }\n+\n+sub set_or_get($)\n+{   my ($self, $i) = (shift, shift);\n+    @_ or return $self->[$i];\n+\n+    my $val = $self->[$i];\n+    $self->[$i] = shift if @_;\n+    $val;\n+}\n+\n+\n+my $atext = '[\\-\\w !#$%&\\'*+/=?^`{|}~]';\n+sub format\n+{   my @addrs;\n+\n+    foreach (@_)\n+    {   my ($phrase, $email, $comment) = @$_;\n+        my @addr;\n+\n+        if(defined $phrase && length $phrase)\n+        {   push @addr\n+              , $phrase =~ /^(?:\\s*$atext\\s*)+$/o ? $phrase\n+              : $phrase =~ /(?<!\\\\)\"/             ? $phrase\n+              :                                    qq(\"$phrase\");\n+\n+            push @addr, \"<$email>\"\n+                if defined $email && length $email;\n+        }\n+        elsif(defined $email && length $email)\n+        {   push @addr, $email;\n+        }\n+\n+        if(defined $comment && $comment =~ /\\S/)\n+        {   $comment =~ s/^\\s*\\(?/(/;\n+            $comment =~ s/\\)?\\s*$/)/;\n+        }\n+\n+        push @addr, $comment\n+            if defined $comment && length $comment;\n+\n+        push @addrs, join(\" \", @addr)\n+            if @addr;\n+    }\n+\n+    join \", \", @addrs;\n+}\n+\n+#------------\n+\n+sub name\n+{   my $self   = shift;\n+    my $phrase = $self->phrase;\n+    my $addr   = $self->address;\n+\n+    $phrase    = $self->comment\n+        unless defined $phrase && length $phrase;\n+\n+    my $name   = $self->_extract_name($phrase);\n+\n+    # first.last@domain address\n+    if($name eq '' && $addr =~ /([^\\%\\.\\@_]+([\\._][^\\%\\.\\@_]+)+)[\\@\\%]/)\n+    {   ($name  = $1) =~ s/[\\._]+/ /g;\n+\t$name   = _extract_name $name;\n+    }\n+\n+    if($name eq '' && $addr =~ m#/g=#i)    # X400 style address\n+    {   my ($f) = $addr =~ m#g=([^/]*)#i;\n+\tmy ($l) = $addr =~ m#s=([^/]*)#i;\n+\t$name   = _extract_name \"$f $l\";\n+    }\n+\n+    length $name ? $name : undef;\n+}\n+\n+\n+sub host\n+{   my $addr = shift->address || '';\n+    my $i    = rindex $addr, '@';\n+    $i >= 0 ? substr($addr, $i+1) : undef;\n+}\n+\n+\n+sub user\n+{   my $addr = shift->address || '';\n+    my $i    = rindex $addr, '@';\n+    $i >= 0 ? substr($addr,0,$i) : $addr;\n+}\n+\n+1;\ndiff --git a/perl/Git/Mail/Address.pm b/perl/Git/Mail/Address.pm\nnew file mode 100755\nindex 0000000..2ce3e84\n--- /dev/null\n+++ b/perl/Git/Mail/Address.pm\n@@ -0,0 +1,24 @@\n+package Git::Mail::Address;\n+use 5.008;\n+use strict;\n+use warnings;\n+\n+=head1 NAME\n+\n+Git::Mail::Address - Wrapper for the L<Mail::Address> module, in case it's not installed\n+\n+=head1 DESCRIPTION\n+\n+This module is only intended to be used for code shipping in the\n+C<git.git> repository. Use it for anything else at your peril!\n+\n+=cut\n+\n+eval {\n+    require Mail::Address;\n+    1;\n+} or do {\n+    require Git::FromCPAN::Mail::Address;\n+};\n+\n+1;\n-- \n2.7.4\n\n"},{"id":"335935","messageId":"1515177413-12526-2-git-send-email-git@matthieu-moy.fr","threadId":"47537","inReplyTo":"1515177413-12526-1-git-send-email-git@matthieu-moy.fr","subject":"[PATCH v2 2/3] Remove now useless email-address parsing code","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2018-01-05T18:36:52Z","receivedAt":"2018-01-05T18:38:00Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"We now use Mail::Address unconditionaly, hence parse_mailboxes is now\ndead code. Remove it and its tests.\n\nSigned-off-by: Matthieu Moy <git@matthieu-moy.fr>\n---\n perl/Git.pm          | 71 ----------------------------------------------------\n t/t9000-addresses.sh | 27 --------------------\n t/t9000/test.pl      | 67 -------------------------------------------------\n 3 files changed, 165 deletions(-)\n delete mode 100755 t/t9000-addresses.sh\n delete mode 100755 t/t9000/test.pl\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex ffa09ac..65e6b32 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -880,77 +880,6 @@ sub ident_person {\n \treturn \"$ident[0] <$ident[1]>\";\n }\n \n-=item parse_mailboxes\n-\n-Return an array of mailboxes extracted from a string.\n-\n-=cut\n-\n-# Very close to Mail::Address's parser, but we still have minor\n-# differences in some cases (see t9000 for examples).\n-sub parse_mailboxes {\n-\tmy $re_comment = qr/\\((?:[^)]*)\\)/;\n-\tmy $re_quote = qr/\"(?:[^\\\"\\\\]|\\\\.)*\"/;\n-\tmy $re_word = qr/(?:[^][\"\\s()<>:;@\\\\,.]|\\\\.)+/;\n-\n-\t# divide the string in tokens of the above form\n-\tmy $re_token = qr/(?:$re_quote|$re_word|$re_comment|\\S)/;\n-\tmy @tokens = map { $_ =~ /\\s*($re_token)\\s*/g } @_;\n-\tmy $end_of_addr_seen = 0;\n-\n-\t# add a delimiter to simplify treatment for the last mailbox\n-\tpush @tokens, \",\";\n-\n-\tmy (@addr_list, @phrase, @address, @comment, @buffer) = ();\n-\tforeach my $token (@tokens) {\n-\t\tif ($token =~ /^[,;]$/) {\n-\t\t\t# if buffer still contains undeterminated strings\n-\t\t\t# append it at the end of @address or @phrase\n-\t\t\tif ($end_of_addr_seen) {\n-\t\t\t\tpush @phrase, @buffer;\n-\t\t\t} else {\n-\t\t\t\tpush @address, @buffer;\n-\t\t\t}\n-\n-\t\t\tmy $str_phrase = join ' ', @phrase;\n-\t\t\tmy $str_address = join '', @address;\n-\t\t\tmy $str_comment = join ' ', @comment;\n-\n-\t\t\t# quote are necessary if phrase contains\n-\t\t\t# special characters\n-\t\t\tif ($str_phrase =~ /[][()<>:;@\\\\,.\\000-\\037\\177]/) {\n-\t\t\t\t$str_phrase =~ s/(^|[^\\\\])\"/$1/g;\n-\t\t\t\t$str_phrase = qq[\"$str_phrase\"];\n-\t\t\t}\n-\n-\t\t\t# add \"<>\" around the address if necessary\n-\t\t\tif ($str_address ne \"\" && $str_phrase ne \"\") {\n-\t\t\t\t$str_address = qq[<$str_address>];\n-\t\t\t}\n-\n-\t\t\tmy $str_mailbox = \"$str_phrase $str_address $str_comment\";\n-\t\t\t$str_mailbox =~ s/^\\s*|\\s*$//g;\n-\t\t\tpush @addr_list, $str_mailbox if ($str_mailbox);\n-\n-\t\t\t@phrase = @address = @comment = @buffer = ();\n-\t\t\t$end_of_addr_seen = 0;\n-\t\t} elsif ($token =~ /^\\(/) {\n-\t\t\tpush @comment, $token;\n-\t\t} elsif ($token eq \"<\") {\n-\t\t\tpush @phrase, (splice @address), (splice @buffer);\n-\t\t} elsif ($token eq \">\") {\n-\t\t\t$end_of_addr_seen = 1;\n-\t\t\tpush @address, (splice @buffer);\n-\t\t} elsif ($token eq \"@\" && !$end_of_addr_seen) {\n-\t\t\tpush @address, (splice @buffer), \"@\";\n-\t\t} else {\n-\t\t\tpush @buffer, $token;\n-\t\t}\n-\t}\n-\n-\treturn @addr_list;\n-}\n-\n =item hash_object ( TYPE, FILENAME )\n \n Compute the SHA1 object id of the given C<FILENAME> considering it is\ndiff --git a/t/t9000-addresses.sh b/t/t9000-addresses.sh\ndeleted file mode 100755\nindex a1ebef6..0000000\n--- a/t/t9000-addresses.sh\n+++ /dev/null\n@@ -1,27 +0,0 @@\n-#!/bin/sh\n-\n-test_description='compare address parsing with and without Mail::Address'\n-. ./test-lib.sh\n-\n-if ! test_have_prereq PERL; then\n-\tskip_all='skipping perl interface tests, perl not available'\n-\ttest_done\n-fi\n-\n-perl -MTest::More -e 0 2>/dev/null || {\n-\tskip_all=\"Perl Test::More unavailable, skipping test\"\n-\ttest_done\n-}\n-\n-perl -MMail::Address -e 0 2>/dev/null || {\n-\tskip_all=\"Perl Mail::Address unavailable, skipping test\"\n-\ttest_done\n-}\n-\n-test_external_has_tap=1\n-\n-test_external_without_stderr \\\n-\t'Perl address parsing function' \\\n-\tperl \"$TEST_DIRECTORY\"/t9000/test.pl\n-\n-test_done\ndiff --git a/t/t9000/test.pl b/t/t9000/test.pl\ndeleted file mode 100755\nindex dfeaa9c..0000000\n--- a/t/t9000/test.pl\n+++ /dev/null\n@@ -1,67 +0,0 @@\n-#!/usr/bin/perl\n-use lib (split(/:/, $ENV{GITPERLLIB}));\n-\n-use 5.008;\n-use warnings;\n-use strict;\n-\n-use Test::More qw(no_plan);\n-use Mail::Address;\n-\n-BEGIN { use_ok('Git') }\n-\n-my @success_list = (q[Jane],\n-\tq[jdoe@example.com],\n-\tq[<jdoe@example.com>],\n-\tq[Jane <jdoe@example.com>],\n-\tq[Jane Doe <jdoe@example.com>],\n-\tq[\"Jane\" <jdoe@example.com>],\n-\tq[\"Doe, Jane\" <jdoe@example.com>],\n-\tq[\"Jane@:;\\>.,()<Doe\" <jdoe@example.com>],\n-\tq[Jane!#$%&'*+-/=?^_{|}~Doe' <jdoe@example.com>],\n-\tq[\"<jdoe@example.com>\"],\n-\tq[\"Jane jdoe@example.com\"],\n-\tq[Jane Doe <jdoe    @   example.com  >],\n-\tq[Jane       Doe <  jdoe@example.com  >],\n-\tq[Jane @ Doe @ Jane @ Doe],\n-\tq[\"Jane, 'Doe'\" <jdoe@example.com>],\n-\tq['Doe, \"Jane' <jdoe@example.com>],\n-\tq[\"Jane\" \"Do\"e <jdoe@example.com>],\n-\tq[\"Jane' Doe\" <jdoe@example.com>],\n-\tq[\"Jane Doe <jdoe@example.com>\" <jdoe@example.com>],\n-\tq[\"Jane\\\" Doe\" <jdoe@example.com>],\n-\tq[Doe, jane <jdoe@example.com>],\n-\tq[\"Jane Doe <jdoe@example.com>],\n-\tq['Jane 'Doe' <jdoe@example.com>],\n-\tq[Jane@:;\\.,()<>Doe <jdoe@example.com>],\n-\tq[Jane <jdoe@example.com> Doe],\n-\tq[<jdoe@example.com> Jane Doe]);\n-\n-my @known_failure_list = (q[Jane\\ Doe <jdoe@example.com>],\n-\tq[\"Doe, Ja\"ne <jdoe@example.com>],\n-\tq[\"Doe, Katarina\" Jane <jdoe@example.com>],\n-\tq[Jane jdoe@example.com],\n-\tq[\"Jane \"Kat\"a\" ri\"na\" \",Doe\" <jdoe@example.com>],\n-\tq[Jane Doe],\n-\tq[Jane \"Doe <jdoe@example.com>\"],\n-\tq[\\\"Jane Doe <jdoe@example.com>],\n-\tq[Jane\\\"\\\" Doe <jdoe@example.com>],\n-\tq['Jane \"Katarina\\\" \\' Doe' <jdoe@example.com>]);\n-\n-foreach my $str (@success_list) {\n-\tmy @expected = map { $_->format } Mail::Address->parse(\"$str\");\n-\tmy @actual = Git::parse_mailboxes(\"$str\");\n-\tis_deeply(\\@expected, \\@actual, qq[same output : $str]);\n-}\n-\n-TODO: {\n-\tlocal $TODO = \"known breakage\";\n-\tforeach my $str (@known_failure_list) {\n-\t\tmy @expected = map { $_->format } Mail::Address->parse(\"$str\");\n-\t\tmy @actual = Git::parse_mailboxes(\"$str\");\n-\t\tis_deeply(\\@expected, \\@actual, qq[same output : $str]);\n-\t}\n-}\n-\n-my $is_passing = eval { Test::More->is_passing };\n-exit($is_passing ? 0 : 1) unless $@ =~ /Can't locate object method/;\n-- \n2.7.4\n\n"},{"id":"335936","messageId":"1515177413-12526-3-git-send-email-git@matthieu-moy.fr","threadId":"47537","inReplyTo":"1515177413-12526-1-git-send-email-git@matthieu-moy.fr","subject":"[PATCH v2 3/3] send-email: add test for Linux's get_maintainer.pl","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2018-01-05T18:36:53Z","receivedAt":"2018-01-05T18:38:02Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"From: Alex Bennée <alex.bennee@linaro.org>\n\nWe had a regression that broke Linux's get_maintainer.pl. Using\nMail::Address to parse email addresses fixed it, but let's protect\nagainst future regressions.\n\nPatch-edited-by: Matthieu Moy <git@matthieu-moy.fr>\nSigned-off-by: Alex Bennée <alex.bennee@linaro.org>\nSigned-off-by: Matthieu Moy <git@matthieu-moy.fr>\n---\nChange since v1: fixed proposed by Eric Sunshine and pointed out by\nAlex Bennée.\n\nEric pointed out that using --cc-cmd=$(pwd)/expected-cc-script.sh did\nnot work because $(pwd) had spaces in it, but I already turned it into\n./expected-cc-script.sh.\n\n t/t9001-send-email.sh | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 4d261c2..d13d8c3 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -172,6 +172,26 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '\n \ttest_cmp expected-cc commandline1\n '\n \n+test_expect_success $PREREQ 'setup fake get_maintainer.pl script for cc trailer' \"\n+\twrite_script expected-cc-script.sh <<-EOF &&\n+\techo 'One Person <one@example.com> (supporter:THIS (FOO/bar))'\n+\techo 'Two Person <two@example.com> (maintainer:THIS THING)'\n+\techo 'Third List <three@example.com> (moderated list:THIS THING (FOO/bar))'\n+\techo '<four@example.com> (moderated list:FOR THING)'\n+\techo 'five@example.com (open list:FOR THING (FOO/bar))'\n+\techo 'six@example.com (open list)'\n+\tEOF\n+\tchmod +x expected-cc-script.sh\n+\"\n+\n+test_expect_success $PREREQ 'cc trailer with get_maintainer.pl output' '\n+\tclean_fake_sendmail &&\n+\tgit send-email -1 --to=recipient@example.com \\\n+\t\t--cc-cmd=\"./expected-cc-script.sh\" \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" &&\n+\ttest_cmp expected-cc commandline1\n+'\n+\n test_expect_success $PREREQ 'setup expect' \"\n cat >expected-show-all-headers <<\\EOF\n 0001-Second.patch\n-- \n2.7.4\n\n"},{"id":"335941","messageId":"CAPig+cQURBQxw69RFyOGKxqyQihTh1c7djsFx3H2MJtWNXKryg@mail.gmail.com","threadId":"47537","inReplyTo":"1515177413-12526-3-git-send-email-git@matthieu-moy.fr","subject":"Re: [PATCH v2 3/3] send-email: add test for Linux's get_maintainer.pl","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-01-05T18:59:27Z","receivedAt":"2018-01-05T18:59:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jan 5, 2018 at 1:36 PM, Matthieu Moy <git@matthieu-moy.fr> wrote:\n> From: Alex Bennée <alex.bennee@linaro.org>\n>\n> We had a regression that broke Linux's get_maintainer.pl. Using\n> Mail::Address to parse email addresses fixed it, but let's protect\n> against future regressions.\n>\n> Patch-edited-by: Matthieu Moy <git@matthieu-moy.fr>\n> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>\n> Signed-off-by: Matthieu Moy <git@matthieu-moy.fr>\n> ---\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> @@ -172,6 +172,26 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '\n> +test_expect_success $PREREQ 'setup fake get_maintainer.pl script for cc trailer' \"\n> +       write_script expected-cc-script.sh <<-EOF &&\n> +       echo 'One Person <one@example.com> (supporter:THIS (FOO/bar))'\n> +       echo 'Two Person <two@example.com> (maintainer:THIS THING)'\n> +       echo 'Third List <three@example.com> (moderated list:THIS THING (FOO/bar))'\n> +       echo '<four@example.com> (moderated list:FOR THING)'\n> +       echo 'five@example.com (open list:FOR THING (FOO/bar))'\n> +       echo 'six@example.com (open list)'\n> +       EOF\n> +       chmod +x expected-cc-script.sh\n> +\"\n> +\n> +test_expect_success $PREREQ 'cc trailer with get_maintainer.pl output' '\n> +       clean_fake_sendmail &&\n> +       git send-email -1 --to=recipient@example.com \\\n> +               --cc-cmd=\"./expected-cc-script.sh\" \\\n> +               --smtp-server=\"$(pwd)/fake.sendmail\" &&\n\nAside from the unnecessary (thus noisy) quotes around the --cc-cmd\nvalue, my one concern is that someone may come along and want to\n\"normalize\" it to --cc-cmd=\"$(pwd)/expected-cc-script.sh\" for\nconsistency with the following --smtp-server line. This worry is\ncompounded by the commit message not explaining why these two lines\ndiffer (one using \"./\" and one using \"$(pwd)/\"). So, at minimum, it\nmight be a good idea to explain why \"./\" is used for this one distinct\ncase, compared with all the others which use \"$(pwd)/\". An alternative\nwould be to insert a cleanup/modernization patch before this one which\nchanges all the \"$(pwd)/\" to \"./\", although you'd still want to\nexplain why that's being done (to wit: because --cc-cmd behavior with\nspaces is not well defined). Or, perhaps this isn't an issue and my\nworry is not justified (after all, the test will break if someone\nchanges the \"./\" to \"$(pwd)/\"). At any rate, such a concern probably\nshouldn't hold up this patch.\n\n> +       test_cmp expected-cc commandline1\n> +'\n> +\n>  test_expect_success $PREREQ 'setup expect' \"\n>  cat >expected-show-all-headers <<\\EOF\n>  0001-Second.patch\n"},{"id":"335960","messageId":"xmqqzi5sawna.fsf@gitster.mtv.corp.google.com","threadId":"47537","inReplyTo":"q7h9h8s0o7vj.fsf@orange.lip.ens-lyon.fr","subject":"Re: [PATCH] send-email: add test for Linux's get_maintainer.pl","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-05T20:16:57Z","receivedAt":"2018-01-05T20:17:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@univ-lyon1.fr> writes:\n\n> Alex Bennée <alex.bennee@linaro.org> writes:\n>\n>> I think you need to apply Eric's suggestions from:\n>>\n>>   From: Eric Sunshine <sunshine@sunshineco.com>\n>>   Date: Sat, 18 Nov 2017 21:54:46 -0500\n>>   Message-ID: <CAPig+cSh0tVVkh0xF9FwCfM4gngAWMSN_FXd2zhzHcy2trYXfw@mail.gmail.com>\n>\n> Indeed. I'm squashing this into the patch:\n>\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> index f126177..d13d8c3 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -173,8 +173,7 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '\n>  '\n>  \n>  test_expect_success $PREREQ 'setup fake get_maintainer.pl script for cc trailer' \"\n> -\tcat >expected-cc-script.sh <<-EOF &&\n> -\t#!/bin/sh\n> +\twrite_script expected-cc-script.sh <<-EOF &&\n>  \techo 'One Person <one@example.com> (supporter:THIS (FOO/bar))'\n>  \techo 'Two Person <two@example.com> (maintainer:THIS THING)'\n>  \techo 'Third List <three@example.com> (moderated list:THIS THING (FOO/bar))'\n\nPlease do not forget to lose \"chmod +x\", which becomes unneeded when\nyou use write_script.\n\n> @@ -186,7 +185,6 @@ test_expect_success $PREREQ 'setup fake get_maintainer.pl script for cc trailer'\n>  \"\n>  \n>  test_expect_success $PREREQ 'cc trailer with get_maintainer.pl output' '\n> -\ttest_commit cc-trailer-getmaint &&\n>  \tclean_fake_sendmail &&\n>  \tgit send-email -1 --to=recipient@example.com \\\n>  \t\t--cc-cmd=\"./expected-cc-script.sh\" \\\n"},{"id":"336127","messageId":"q7h91sj0od7a.fsf@orange.lip.ens-lyon.fr","threadId":"47537","inReplyTo":"CAPig+cQURBQxw69RFyOGKxqyQihTh1c7djsFx3H2MJtWNXKryg@mail.gmail.com","subject":"Re: [PATCH v2 3/3] send-email: add test for Linux's get_maintainer.pl","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2018-01-08T10:30:01Z","receivedAt":"2018-01-08T10:30:20Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Fri, Jan 5, 2018 at 1:36 PM, Matthieu Moy <git@matthieu-moy.fr> wrote:\n>> From: Alex Bennée <alex.bennee@linaro.org>\n>>\n>> We had a regression that broke Linux's get_maintainer.pl. Using\n>> Mail::Address to parse email addresses fixed it, but let's protect\n>> against future regressions.\n>>\n>> Patch-edited-by: Matthieu Moy <git@matthieu-moy.fr>\n>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>\n>> Signed-off-by: Matthieu Moy <git@matthieu-moy.fr>\n>> ---\n>> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n>> @@ -172,6 +172,26 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '\n>> +test_expect_success $PREREQ 'setup fake get_maintainer.pl script for cc trailer' \"\n>> +       write_script expected-cc-script.sh <<-EOF &&\n>> +       echo 'One Person <one@example.com> (supporter:THIS (FOO/bar))'\n>> +       echo 'Two Person <two@example.com> (maintainer:THIS THING)'\n>> +       echo 'Third List <three@example.com> (moderated list:THIS THING (FOO/bar))'\n>> +       echo '<four@example.com> (moderated list:FOR THING)'\n>> +       echo 'five@example.com (open list:FOR THING (FOO/bar))'\n>> +       echo 'six@example.com (open list)'\n>> +       EOF\n>> +       chmod +x expected-cc-script.sh\n>> +\"\n>> +\n>> +test_expect_success $PREREQ 'cc trailer with get_maintainer.pl output' '\n>> +       clean_fake_sendmail &&\n>> +       git send-email -1 --to=recipient@example.com \\\n>> +               --cc-cmd=\"./expected-cc-script.sh\" \\\n>> +               --smtp-server=\"$(pwd)/fake.sendmail\" &&\n>\n> Aside from the unnecessary (thus noisy) quotes around the --cc-cmd\n\nIndeed, removed.\n\n> value, my one concern is that someone may come along and want to\n> \"normalize\" it to --cc-cmd=\"$(pwd)/expected-cc-script.sh\" for\n> consistency with the following --smtp-server line. This worry is\n> compounded by the commit message not explaining why these two lines\n> differ (one using \"./\" and one using \"$(pwd)/\").\n\nAdded a note in the commit message.\n\n> An alternative would be to insert a cleanup/modernization\n> patch before this one which changes all the \"$(pwd)/\" to \"./\",\n\nFor --smtp-server, doing so results in a failing tests. I didn't\ninvestigate on why.\n\n> although you'd still want to explain why that's being done (to wit:\n> because --cc-cmd behavior with spaces is not well defined). Or,\n> perhaps this isn't an issue and my worry is not justified (after all,\n> the test will break if someone changes the \"./\" to \"$(pwd)/\").\n\nAlso, the existing code is written like this: --cc-cmd is always\nrelative, --stmp-server is always absolute, including when they're used\nin the same command:\n\ntest_suppress_self () {\n[...]\n\tgit send-email --from=\"$1 <$2>\" \\\n\t\t--to=nobody@example.com \\\n\t\t--cc-cmd=./cccmd-sed \\\n\t\t--suppress-cc=self \\\n\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n\nThanks for your careful review,\n\n-- \nMatthieu Moy\nhttps://matthieu-moy.fr/\n"},{"id":"336128","messageId":"1515407674-5233-1-git-send-email-git@matthieu-moy.fr","threadId":"47537","inReplyTo":"1515177413-12526-1-git-send-email-git@matthieu-moy.fr","subject":"[PATCH v3 1/3] send-email: add and use a local copy of Mail::Address","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2018-01-08T10:34:32Z","receivedAt":"2018-01-08T10:35:02Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"We used to have two versions of the email parsing code. Our\nparse_mailboxes (in Git.pm), and Mail::Address which we used if\ninstalled. Unfortunately, both versions have different sets of bugs, and\nchanging the behavior of git depending on whether Mail::Address is\ninstalled was a bad idea.\n\nA first attempt to solve this was cc90750 (send-email: don't use\nMail::Address, even if available, 2017-08-23), but it turns out our\nparse_mailboxes is too buggy for some uses. For example the lack of\nnested comments support breaks get_maintainer.pl in the Linux kernel\ntree:\n\n  https://public-inbox.org/git/20171116154814.23785-1-alex.bennee@linaro.org/\n\nThis patch goes the other way: use Mail::Address anyway, but have a\nlocal copy from CPAN as a fallback, when the system one is not\navailable.\n\nThe duplicated script is small (276 lines of code) and stable in time.\nMaintaining the local copy should not be an issue, and will certainly be\nless burden than maintaining our own parse_mailboxes.\n\nAnother option would be to consider Mail::Address as a hard dependency,\nbut it's easy enough to save the trouble of extra-dependency to the end\nuser or packager.\n\nSigned-off-by: Matthieu Moy <git@matthieu-moy.fr>\n---\nNo change since v2.\n\n git-send-email.perl               |   3 +-\n perl/Git/FromCPAN/Mail/Address.pm | 276 ++++++++++++++++++++++++++++++++++++++\n perl/Git/Mail/Address.pm          |  24 ++++\n 3 files changed, 302 insertions(+), 1 deletion(-)\n create mode 100644 perl/Git/FromCPAN/Mail/Address.pm\n create mode 100755 perl/Git/Mail/Address.pm\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex edcc6d3..340b5c8 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -30,6 +30,7 @@ use Error qw(:try);\n use Cwd qw(abs_path cwd);\n use Git;\n use Git::I18N;\n+use Git::Mail::Address;\n \n Getopt::Long::Configure qw/ pass_through /;\n \n@@ -489,7 +490,7 @@ my ($repoauthor, $repocommitter);\n ($repocommitter) = Git::ident_person(@repo, 'committer');\n \n sub parse_address_line {\n-\treturn Git::parse_mailboxes($_[0]);\n+\treturn map { $_->format } Mail::Address->parse($_[0]);\n }\n \n sub split_addrs {\ndiff --git a/perl/Git/FromCPAN/Mail/Address.pm b/perl/Git/FromCPAN/Mail/Address.pm\nnew file mode 100644\nindex 0000000..13b2ff7\n--- /dev/null\n+++ b/perl/Git/FromCPAN/Mail/Address.pm\n@@ -0,0 +1,276 @@\n+# Copyrights 1995-2017 by [Mark Overmeer <perl@overmeer.net>].\n+#  For other contributors see ChangeLog.\n+# See the manual pages for details on the licensing terms.\n+# Pod stripped from pm file by OODoc 2.02.\n+package Mail::Address;\n+use vars '$VERSION';\n+$VERSION = '2.19';\n+\n+use strict;\n+\n+use Carp;\n+\n+# use locale;   removed in version 1.78, because it causes taint problems\n+\n+sub Version { our $VERSION }\n+\n+\n+\n+# given a comment, attempt to extract a person's name\n+sub _extract_name\n+{   # This function can be called as method as well\n+    my $self = @_ && ref $_[0] ? shift : undef;\n+\n+    local $_ = shift\n+        or return '';\n+\n+    # Using encodings, too hard. See Mail::Message::Field::Full.\n+    return '' if m/\\=\\?.*?\\?\\=/;\n+\n+    # trim whitespace\n+    s/^\\s+//;\n+    s/\\s+$//;\n+    s/\\s+/ /;\n+\n+    # Disregard numeric names (e.g. 123456.1234@compuserve.com)\n+    return \"\" if /^[\\d ]+$/;\n+\n+    s/^\\((.*)\\)$/$1/; # remove outermost parenthesis\n+    s/^\"(.*)\"$/$1/;   # remove outer quotation marks\n+    s/\\(.*?\\)//g;     # remove minimal embedded comments\n+    s/\\\\//g;          # remove all escapes\n+    s/^\"(.*)\"$/$1/;   # remove internal quotation marks\n+    s/^([^\\s]+) ?, ?(.*)$/$2 $1/; # reverse \"Last, First M.\" if applicable\n+    s/,.*//;\n+\n+    # Change casing only when the name contains only upper or only\n+    # lower cased characters.\n+    unless( m/[A-Z]/ && m/[a-z]/ )\n+    {   # Set the case of the name to first char upper rest lower\n+        s/\\b(\\w+)/\\L\\u$1/igo;  # Upcase first letter on name\n+        s/\\bMc(\\w)/Mc\\u$1/igo; # Scottish names such as 'McLeod'\n+        s/\\bo'(\\w)/O'\\u$1/igo; # Irish names such as 'O'Malley, O'Reilly'\n+        s/\\b(x*(ix)?v*(iv)?i*)\\b/\\U$1/igo; # Roman numerals, eg 'Level III Support'\n+    }\n+\n+    # some cleanup\n+    s/\\[[^\\]]*\\]//g;\n+    s/(^[\\s'\"]+|[\\s'\"]+$)//g;\n+    s/\\s{2,}/ /g;\n+\n+    $_;\n+}\n+\n+sub _tokenise\n+{   local $_ = join ',', @_;\n+    my (@words,$snippet,$field);\n+\n+    s/\\A\\s+//;\n+    s/[\\r\\n]+/ /g;\n+\n+    while ($_ ne '')\n+    {   $field = '';\n+        if(s/^\\s*\\(/(/ )    # (...)\n+        {   my $depth = 0;\n+\n+     PAREN: while(s/^(\\(([^\\(\\)\\\\]|\\\\.)*)//)\n+            {   $field .= $1;\n+                $depth++;\n+                while(s/^(([^\\(\\)\\\\]|\\\\.)*\\)\\s*)//)\n+                {   $field .= $1;\n+                    last PAREN unless --$depth;\n+\t            $field .= $1 if s/^(([^\\(\\)\\\\]|\\\\.)+)//;\n+                }\n+            }\n+\n+            carp \"Unmatched () '$field' '$_'\"\n+                if $depth;\n+\n+            $field =~ s/\\s+\\Z//;\n+            push @words, $field;\n+\n+            next;\n+        }\n+\n+        if( s/^(\"(?:[^\"\\\\]+|\\\\.)*\")\\s*//       # \"...\"\n+         || s/^(\\[(?:[^\\]\\\\]+|\\\\.)*\\])\\s*//    # [...]\n+         || s/^([^\\s()<>\\@,;:\\\\\".[\\]]+)\\s*//\n+         || s/^([()<>\\@,;:\\\\\".[\\]])\\s*//\n+          )\n+        {   push @words, $1;\n+            next;\n+        }\n+\n+        croak \"Unrecognised line: $_\";\n+    }\n+\n+    push @words, \",\";\n+    \\@words;\n+}\n+\n+sub _find_next\n+{   my ($idx, $tokens, $len) = @_;\n+\n+    while($idx < $len)\n+    {   my $c = $tokens->[$idx];\n+        return $c if $c eq ',' || $c eq ';' || $c eq '<';\n+        $idx++;\n+    }\n+\n+    \"\";\n+}\n+\n+sub _complete\n+{   my ($class, $phrase, $address, $comment) = @_;\n+\n+    @$phrase || @$comment || @$address\n+       or return undef;\n+\n+    my $o = $class->new(join(\" \",@$phrase), join(\"\",@$address), join(\" \",@$comment));\n+    @$phrase = @$address = @$comment = ();\n+    $o;\n+}\n+\n+#------------\n+\n+sub new(@)\n+{   my $class = shift;\n+    bless [@_], $class;\n+}\n+\n+\n+sub parse(@)\n+{   my $class = shift;\n+    my @line  = grep {defined} @_;\n+    my $line  = join '', @line;\n+\n+    my (@phrase, @comment, @address, @objs);\n+    my ($depth, $idx) = (0, 0);\n+\n+    my $tokens  = _tokenise @line;\n+    my $len     = @$tokens;\n+    my $next    = _find_next $idx, $tokens, $len;\n+\n+    local $_;\n+    for(my $idx = 0; $idx < $len; $idx++)\n+    {   $_ = $tokens->[$idx];\n+\n+        if(substr($_,0,1) eq '(') { push @comment, $_ }\n+        elsif($_ eq '<')    { $depth++ }\n+        elsif($_ eq '>')    { $depth-- if $depth }\n+        elsif($_ eq ',' || $_ eq ';')\n+        {   warn \"Unmatched '<>' in $line\" if $depth;\n+            my $o = $class->_complete(\\@phrase, \\@address, \\@comment);\n+            push @objs, $o if defined $o;\n+            $depth = 0;\n+            $next = _find_next $idx+1, $tokens, $len;\n+        }\n+        elsif($depth)       { push @address, $_ }\n+        elsif($next eq '<') { push @phrase,  $_ }\n+        elsif( /^[.\\@:;]$/ || !@address || $address[-1] =~ /^[.\\@:;]$/ )\n+        {   push @address, $_ }\n+        else\n+        {   warn \"Unmatched '<>' in $line\" if $depth;\n+            my $o = $class->_complete(\\@phrase, \\@address, \\@comment);\n+            push @objs, $o if defined $o;\n+            $depth = 0;\n+            push @address, $_;\n+        }\n+    }\n+    @objs;\n+}\n+\n+#------------\n+\n+sub phrase  { shift->set_or_get(0, @_) }\n+sub address { shift->set_or_get(1, @_) }\n+sub comment { shift->set_or_get(2, @_) }\n+\n+sub set_or_get($)\n+{   my ($self, $i) = (shift, shift);\n+    @_ or return $self->[$i];\n+\n+    my $val = $self->[$i];\n+    $self->[$i] = shift if @_;\n+    $val;\n+}\n+\n+\n+my $atext = '[\\-\\w !#$%&\\'*+/=?^`{|}~]';\n+sub format\n+{   my @addrs;\n+\n+    foreach (@_)\n+    {   my ($phrase, $email, $comment) = @$_;\n+        my @addr;\n+\n+        if(defined $phrase && length $phrase)\n+        {   push @addr\n+              , $phrase =~ /^(?:\\s*$atext\\s*)+$/o ? $phrase\n+              : $phrase =~ /(?<!\\\\)\"/             ? $phrase\n+              :                                    qq(\"$phrase\");\n+\n+            push @addr, \"<$email>\"\n+                if defined $email && length $email;\n+        }\n+        elsif(defined $email && length $email)\n+        {   push @addr, $email;\n+        }\n+\n+        if(defined $comment && $comment =~ /\\S/)\n+        {   $comment =~ s/^\\s*\\(?/(/;\n+            $comment =~ s/\\)?\\s*$/)/;\n+        }\n+\n+        push @addr, $comment\n+            if defined $comment && length $comment;\n+\n+        push @addrs, join(\" \", @addr)\n+            if @addr;\n+    }\n+\n+    join \", \", @addrs;\n+}\n+\n+#------------\n+\n+sub name\n+{   my $self   = shift;\n+    my $phrase = $self->phrase;\n+    my $addr   = $self->address;\n+\n+    $phrase    = $self->comment\n+        unless defined $phrase && length $phrase;\n+\n+    my $name   = $self->_extract_name($phrase);\n+\n+    # first.last@domain address\n+    if($name eq '' && $addr =~ /([^\\%\\.\\@_]+([\\._][^\\%\\.\\@_]+)+)[\\@\\%]/)\n+    {   ($name  = $1) =~ s/[\\._]+/ /g;\n+\t$name   = _extract_name $name;\n+    }\n+\n+    if($name eq '' && $addr =~ m#/g=#i)    # X400 style address\n+    {   my ($f) = $addr =~ m#g=([^/]*)#i;\n+\tmy ($l) = $addr =~ m#s=([^/]*)#i;\n+\t$name   = _extract_name \"$f $l\";\n+    }\n+\n+    length $name ? $name : undef;\n+}\n+\n+\n+sub host\n+{   my $addr = shift->address || '';\n+    my $i    = rindex $addr, '@';\n+    $i >= 0 ? substr($addr, $i+1) : undef;\n+}\n+\n+\n+sub user\n+{   my $addr = shift->address || '';\n+    my $i    = rindex $addr, '@';\n+    $i >= 0 ? substr($addr,0,$i) : $addr;\n+}\n+\n+1;\ndiff --git a/perl/Git/Mail/Address.pm b/perl/Git/Mail/Address.pm\nnew file mode 100755\nindex 0000000..2ce3e84\n--- /dev/null\n+++ b/perl/Git/Mail/Address.pm\n@@ -0,0 +1,24 @@\n+package Git::Mail::Address;\n+use 5.008;\n+use strict;\n+use warnings;\n+\n+=head1 NAME\n+\n+Git::Mail::Address - Wrapper for the L<Mail::Address> module, in case it's not installed\n+\n+=head1 DESCRIPTION\n+\n+This module is only intended to be used for code shipping in the\n+C<git.git> repository. Use it for anything else at your peril!\n+\n+=cut\n+\n+eval {\n+    require Mail::Address;\n+    1;\n+} or do {\n+    require Git::FromCPAN::Mail::Address;\n+};\n+\n+1;\n-- \n2.7.4\n\n"},{"id":"336129","messageId":"1515407674-5233-3-git-send-email-git@matthieu-moy.fr","threadId":"47537","inReplyTo":"1515407674-5233-1-git-send-email-git@matthieu-moy.fr","subject":"[PATCH v3 3/3] send-email: add test for Linux's get_maintainer.pl","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2018-01-08T10:34:34Z","receivedAt":"2018-01-08T10:35:05Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"From: Alex Bennée <alex.bennee@linaro.org>\n\nWe had a regression that broke Linux's get_maintainer.pl. Using\nMail::Address to parse email addresses fixed it, but let's protect\nagainst future regressions.\n\nNote that we need --cc-cmd to be relative because this option doesn't\naccept spaces in script names (probably to allow --cc-cmd=\"executable\n--option\"), while --smtp-server needs to be absolute.\n\nPatch-edited-by: Matthieu Moy <git@matthieu-moy.fr>\nSigned-off-by: Alex Bennée <alex.bennee@linaro.org>\nSigned-off-by: Matthieu Moy <git@matthieu-moy.fr>\n---\nChange since v2:\n\n* Mention relative Vs absolute path in commit message.\n\n* Remove useless \"chmod +x\"\n\n* Remove useless double quotes\n\n t/t9001-send-email.sh | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 4d261c2..a06e5d7 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -172,6 +172,25 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '\n \ttest_cmp expected-cc commandline1\n '\n \n+test_expect_success $PREREQ 'setup fake get_maintainer.pl script for cc trailer' \"\n+\twrite_script expected-cc-script.sh <<-EOF\n+\techo 'One Person <one@example.com> (supporter:THIS (FOO/bar))'\n+\techo 'Two Person <two@example.com> (maintainer:THIS THING)'\n+\techo 'Third List <three@example.com> (moderated list:THIS THING (FOO/bar))'\n+\techo '<four@example.com> (moderated list:FOR THING)'\n+\techo 'five@example.com (open list:FOR THING (FOO/bar))'\n+\techo 'six@example.com (open list)'\n+\tEOF\n+\"\n+\n+test_expect_success $PREREQ 'cc trailer with get_maintainer.pl output' '\n+\tclean_fake_sendmail &&\n+\tgit send-email -1 --to=recipient@example.com \\\n+\t\t--cc-cmd=./expected-cc-script.sh \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" &&\n+\ttest_cmp expected-cc commandline1\n+'\n+\n test_expect_success $PREREQ 'setup expect' \"\n cat >expected-show-all-headers <<\\EOF\n 0001-Second.patch\n-- \n2.7.4\n\n"},{"id":"336130","messageId":"1515407674-5233-2-git-send-email-git@matthieu-moy.fr","threadId":"47537","inReplyTo":"1515407674-5233-1-git-send-email-git@matthieu-moy.fr","subject":"[PATCH v3 2/3] Remove now useless email-address parsing code","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2018-01-08T10:34:33Z","receivedAt":"2018-01-08T10:35:09Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"We now use Mail::Address unconditionaly, hence parse_mailboxes is now\ndead code. Remove it and its tests.\n\nSigned-off-by: Matthieu Moy <git@matthieu-moy.fr>\n---\nNo change since v2.\n\n perl/Git.pm          | 71 ----------------------------------------------------\n t/t9000-addresses.sh | 27 --------------------\n t/t9000/test.pl      | 67 -------------------------------------------------\n 3 files changed, 165 deletions(-)\n delete mode 100755 t/t9000-addresses.sh\n delete mode 100755 t/t9000/test.pl\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex ffa09ac..65e6b32 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -880,77 +880,6 @@ sub ident_person {\n \treturn \"$ident[0] <$ident[1]>\";\n }\n \n-=item parse_mailboxes\n-\n-Return an array of mailboxes extracted from a string.\n-\n-=cut\n-\n-# Very close to Mail::Address's parser, but we still have minor\n-# differences in some cases (see t9000 for examples).\n-sub parse_mailboxes {\n-\tmy $re_comment = qr/\\((?:[^)]*)\\)/;\n-\tmy $re_quote = qr/\"(?:[^\\\"\\\\]|\\\\.)*\"/;\n-\tmy $re_word = qr/(?:[^][\"\\s()<>:;@\\\\,.]|\\\\.)+/;\n-\n-\t# divide the string in tokens of the above form\n-\tmy $re_token = qr/(?:$re_quote|$re_word|$re_comment|\\S)/;\n-\tmy @tokens = map { $_ =~ /\\s*($re_token)\\s*/g } @_;\n-\tmy $end_of_addr_seen = 0;\n-\n-\t# add a delimiter to simplify treatment for the last mailbox\n-\tpush @tokens, \",\";\n-\n-\tmy (@addr_list, @phrase, @address, @comment, @buffer) = ();\n-\tforeach my $token (@tokens) {\n-\t\tif ($token =~ /^[,;]$/) {\n-\t\t\t# if buffer still contains undeterminated strings\n-\t\t\t# append it at the end of @address or @phrase\n-\t\t\tif ($end_of_addr_seen) {\n-\t\t\t\tpush @phrase, @buffer;\n-\t\t\t} else {\n-\t\t\t\tpush @address, @buffer;\n-\t\t\t}\n-\n-\t\t\tmy $str_phrase = join ' ', @phrase;\n-\t\t\tmy $str_address = join '', @address;\n-\t\t\tmy $str_comment = join ' ', @comment;\n-\n-\t\t\t# quote are necessary if phrase contains\n-\t\t\t# special characters\n-\t\t\tif ($str_phrase =~ /[][()<>:;@\\\\,.\\000-\\037\\177]/) {\n-\t\t\t\t$str_phrase =~ s/(^|[^\\\\])\"/$1/g;\n-\t\t\t\t$str_phrase = qq[\"$str_phrase\"];\n-\t\t\t}\n-\n-\t\t\t# add \"<>\" around the address if necessary\n-\t\t\tif ($str_address ne \"\" && $str_phrase ne \"\") {\n-\t\t\t\t$str_address = qq[<$str_address>];\n-\t\t\t}\n-\n-\t\t\tmy $str_mailbox = \"$str_phrase $str_address $str_comment\";\n-\t\t\t$str_mailbox =~ s/^\\s*|\\s*$//g;\n-\t\t\tpush @addr_list, $str_mailbox if ($str_mailbox);\n-\n-\t\t\t@phrase = @address = @comment = @buffer = ();\n-\t\t\t$end_of_addr_seen = 0;\n-\t\t} elsif ($token =~ /^\\(/) {\n-\t\t\tpush @comment, $token;\n-\t\t} elsif ($token eq \"<\") {\n-\t\t\tpush @phrase, (splice @address), (splice @buffer);\n-\t\t} elsif ($token eq \">\") {\n-\t\t\t$end_of_addr_seen = 1;\n-\t\t\tpush @address, (splice @buffer);\n-\t\t} elsif ($token eq \"@\" && !$end_of_addr_seen) {\n-\t\t\tpush @address, (splice @buffer), \"@\";\n-\t\t} else {\n-\t\t\tpush @buffer, $token;\n-\t\t}\n-\t}\n-\n-\treturn @addr_list;\n-}\n-\n =item hash_object ( TYPE, FILENAME )\n \n Compute the SHA1 object id of the given C<FILENAME> considering it is\ndiff --git a/t/t9000-addresses.sh b/t/t9000-addresses.sh\ndeleted file mode 100755\nindex a1ebef6..0000000\n--- a/t/t9000-addresses.sh\n+++ /dev/null\n@@ -1,27 +0,0 @@\n-#!/bin/sh\n-\n-test_description='compare address parsing with and without Mail::Address'\n-. ./test-lib.sh\n-\n-if ! test_have_prereq PERL; then\n-\tskip_all='skipping perl interface tests, perl not available'\n-\ttest_done\n-fi\n-\n-perl -MTest::More -e 0 2>/dev/null || {\n-\tskip_all=\"Perl Test::More unavailable, skipping test\"\n-\ttest_done\n-}\n-\n-perl -MMail::Address -e 0 2>/dev/null || {\n-\tskip_all=\"Perl Mail::Address unavailable, skipping test\"\n-\ttest_done\n-}\n-\n-test_external_has_tap=1\n-\n-test_external_without_stderr \\\n-\t'Perl address parsing function' \\\n-\tperl \"$TEST_DIRECTORY\"/t9000/test.pl\n-\n-test_done\ndiff --git a/t/t9000/test.pl b/t/t9000/test.pl\ndeleted file mode 100755\nindex dfeaa9c..0000000\n--- a/t/t9000/test.pl\n+++ /dev/null\n@@ -1,67 +0,0 @@\n-#!/usr/bin/perl\n-use lib (split(/:/, $ENV{GITPERLLIB}));\n-\n-use 5.008;\n-use warnings;\n-use strict;\n-\n-use Test::More qw(no_plan);\n-use Mail::Address;\n-\n-BEGIN { use_ok('Git') }\n-\n-my @success_list = (q[Jane],\n-\tq[jdoe@example.com],\n-\tq[<jdoe@example.com>],\n-\tq[Jane <jdoe@example.com>],\n-\tq[Jane Doe <jdoe@example.com>],\n-\tq[\"Jane\" <jdoe@example.com>],\n-\tq[\"Doe, Jane\" <jdoe@example.com>],\n-\tq[\"Jane@:;\\>.,()<Doe\" <jdoe@example.com>],\n-\tq[Jane!#$%&'*+-/=?^_{|}~Doe' <jdoe@example.com>],\n-\tq[\"<jdoe@example.com>\"],\n-\tq[\"Jane jdoe@example.com\"],\n-\tq[Jane Doe <jdoe    @   example.com  >],\n-\tq[Jane       Doe <  jdoe@example.com  >],\n-\tq[Jane @ Doe @ Jane @ Doe],\n-\tq[\"Jane, 'Doe'\" <jdoe@example.com>],\n-\tq['Doe, \"Jane' <jdoe@example.com>],\n-\tq[\"Jane\" \"Do\"e <jdoe@example.com>],\n-\tq[\"Jane' Doe\" <jdoe@example.com>],\n-\tq[\"Jane Doe <jdoe@example.com>\" <jdoe@example.com>],\n-\tq[\"Jane\\\" Doe\" <jdoe@example.com>],\n-\tq[Doe, jane <jdoe@example.com>],\n-\tq[\"Jane Doe <jdoe@example.com>],\n-\tq['Jane 'Doe' <jdoe@example.com>],\n-\tq[Jane@:;\\.,()<>Doe <jdoe@example.com>],\n-\tq[Jane <jdoe@example.com> Doe],\n-\tq[<jdoe@example.com> Jane Doe]);\n-\n-my @known_failure_list = (q[Jane\\ Doe <jdoe@example.com>],\n-\tq[\"Doe, Ja\"ne <jdoe@example.com>],\n-\tq[\"Doe, Katarina\" Jane <jdoe@example.com>],\n-\tq[Jane jdoe@example.com],\n-\tq[\"Jane \"Kat\"a\" ri\"na\" \",Doe\" <jdoe@example.com>],\n-\tq[Jane Doe],\n-\tq[Jane \"Doe <jdoe@example.com>\"],\n-\tq[\\\"Jane Doe <jdoe@example.com>],\n-\tq[Jane\\\"\\\" Doe <jdoe@example.com>],\n-\tq['Jane \"Katarina\\\" \\' Doe' <jdoe@example.com>]);\n-\n-foreach my $str (@success_list) {\n-\tmy @expected = map { $_->format } Mail::Address->parse(\"$str\");\n-\tmy @actual = Git::parse_mailboxes(\"$str\");\n-\tis_deeply(\\@expected, \\@actual, qq[same output : $str]);\n-}\n-\n-TODO: {\n-\tlocal $TODO = \"known breakage\";\n-\tforeach my $str (@known_failure_list) {\n-\t\tmy @expected = map { $_->format } Mail::Address->parse(\"$str\");\n-\t\tmy @actual = Git::parse_mailboxes(\"$str\");\n-\t\tis_deeply(\\@expected, \\@actual, qq[same output : $str]);\n-\t}\n-}\n-\n-my $is_passing = eval { Test::More->is_passing };\n-exit($is_passing ? 0 : 1) unless $@ =~ /Can't locate object method/;\n-- \n2.7.4\n\n"},{"id":"336134","messageId":"87incco97k.fsf@linaro.org","threadId":"47537","inReplyTo":"1515407674-5233-1-git-send-email-git@matthieu-moy.fr","subject":"Re: [PATCH v3 1/3] send-email: add and use a local copy of Mail::Address","fromName":"Alex Bennée","fromEmail":"alex.bennee@linaro.org","sentAt":"2018-01-08T11:56:15Z","receivedAt":"2018-01-08T11:56:25Z","isPatch":true,"sender":{"key":"alex.bennee@linaro.org","avatar":"https://avatars.githubusercontent.com/u/22458?v=4"},"body":"\nMatthieu Moy <git@matthieu-moy.fr> writes:\n\n> We used to have two versions of the email parsing code. Our\n> parse_mailboxes (in Git.pm), and Mail::Address which we used if\n> installed. Unfortunately, both versions have different sets of bugs, and\n> changing the behavior of git depending on whether Mail::Address is\n> installed was a bad idea.\n>\n> A first attempt to solve this was cc90750 (send-email: don't use\n> Mail::Address, even if available, 2017-08-23), but it turns out our\n> parse_mailboxes is too buggy for some uses. For example the lack of\n> nested comments support breaks get_maintainer.pl in the Linux kernel\n> tree:\n>\n>   https://public-inbox.org/git/20171116154814.23785-1-alex.bennee@linaro.org/\n>\n> This patch goes the other way: use Mail::Address anyway, but have a\n> local copy from CPAN as a fallback, when the system one is not\n> available.\n>\n> The duplicated script is small (276 lines of code) and stable in time.\n> Maintaining the local copy should not be an issue, and will certainly be\n> less burden than maintaining our own parse_mailboxes.\n>\n> Another option would be to consider Mail::Address as a hard dependency,\n> but it's easy enough to save the trouble of extra-dependency to the end\n> user or packager.\n>\n> Signed-off-by: Matthieu Moy <git@matthieu-moy.fr>\n\nReviewed-by: Alex Bennée <alex.bennee@linaro.org>\n\n\n> ---\n> No change since v2.\n>\n>  git-send-email.perl               |   3 +-\n>  perl/Git/FromCPAN/Mail/Address.pm | 276 ++++++++++++++++++++++++++++++++++++++\n>  perl/Git/Mail/Address.pm          |  24 ++++\n>  3 files changed, 302 insertions(+), 1 deletion(-)\n>  create mode 100644 perl/Git/FromCPAN/Mail/Address.pm\n>  create mode 100755 perl/Git/Mail/Address.pm\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index edcc6d3..340b5c8 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -30,6 +30,7 @@ use Error qw(:try);\n>  use Cwd qw(abs_path cwd);\n>  use Git;\n>  use Git::I18N;\n> +use Git::Mail::Address;\n>\n>  Getopt::Long::Configure qw/ pass_through /;\n>\n> @@ -489,7 +490,7 @@ my ($repoauthor, $repocommitter);\n>  ($repocommitter) = Git::ident_person(@repo, 'committer');\n>\n>  sub parse_address_line {\n> -\treturn Git::parse_mailboxes($_[0]);\n> +\treturn map { $_->format } Mail::Address->parse($_[0]);\n>  }\n>\n>  sub split_addrs {\n> diff --git a/perl/Git/FromCPAN/Mail/Address.pm b/perl/Git/FromCPAN/Mail/Address.pm\n> new file mode 100644\n> index 0000000..13b2ff7\n> --- /dev/null\n> +++ b/perl/Git/FromCPAN/Mail/Address.pm\n> @@ -0,0 +1,276 @@\n> +# Copyrights 1995-2017 by [Mark Overmeer <perl@overmeer.net>].\n> +#  For other contributors see ChangeLog.\n> +# See the manual pages for details on the licensing terms.\n> +# Pod stripped from pm file by OODoc 2.02.\n> +package Mail::Address;\n> +use vars '$VERSION';\n> +$VERSION = '2.19';\n> +\n> +use strict;\n> +\n> +use Carp;\n> +\n> +# use locale;   removed in version 1.78, because it causes taint problems\n> +\n> +sub Version { our $VERSION }\n> +\n> +\n> +\n> +# given a comment, attempt to extract a person's name\n> +sub _extract_name\n> +{   # This function can be called as method as well\n> +    my $self = @_ && ref $_[0] ? shift : undef;\n> +\n> +    local $_ = shift\n> +        or return '';\n> +\n> +    # Using encodings, too hard. See Mail::Message::Field::Full.\n> +    return '' if m/\\=\\?.*?\\?\\=/;\n> +\n> +    # trim whitespace\n> +    s/^\\s+//;\n> +    s/\\s+$//;\n> +    s/\\s+/ /;\n> +\n> +    # Disregard numeric names (e.g. 123456.1234@compuserve.com)\n> +    return \"\" if /^[\\d ]+$/;\n> +\n> +    s/^\\((.*)\\)$/$1/; # remove outermost parenthesis\n> +    s/^\"(.*)\"$/$1/;   # remove outer quotation marks\n> +    s/\\(.*?\\)//g;     # remove minimal embedded comments\n> +    s/\\\\//g;          # remove all escapes\n> +    s/^\"(.*)\"$/$1/;   # remove internal quotation marks\n> +    s/^([^\\s]+) ?, ?(.*)$/$2 $1/; # reverse \"Last, First M.\" if applicable\n> +    s/,.*//;\n> +\n> +    # Change casing only when the name contains only upper or only\n> +    # lower cased characters.\n> +    unless( m/[A-Z]/ && m/[a-z]/ )\n> +    {   # Set the case of the name to first char upper rest lower\n> +        s/\\b(\\w+)/\\L\\u$1/igo;  # Upcase first letter on name\n> +        s/\\bMc(\\w)/Mc\\u$1/igo; # Scottish names such as 'McLeod'\n> +        s/\\bo'(\\w)/O'\\u$1/igo; # Irish names such as 'O'Malley, O'Reilly'\n> +        s/\\b(x*(ix)?v*(iv)?i*)\\b/\\U$1/igo; # Roman numerals, eg 'Level III Support'\n> +    }\n> +\n> +    # some cleanup\n> +    s/\\[[^\\]]*\\]//g;\n> +    s/(^[\\s'\"]+|[\\s'\"]+$)//g;\n> +    s/\\s{2,}/ /g;\n> +\n> +    $_;\n> +}\n> +\n> +sub _tokenise\n> +{   local $_ = join ',', @_;\n> +    my (@words,$snippet,$field);\n> +\n> +    s/\\A\\s+//;\n> +    s/[\\r\\n]+/ /g;\n> +\n> +    while ($_ ne '')\n> +    {   $field = '';\n> +        if(s/^\\s*\\(/(/ )    # (...)\n> +        {   my $depth = 0;\n> +\n> +     PAREN: while(s/^(\\(([^\\(\\)\\\\]|\\\\.)*)//)\n> +            {   $field .= $1;\n> +                $depth++;\n> +                while(s/^(([^\\(\\)\\\\]|\\\\.)*\\)\\s*)//)\n> +                {   $field .= $1;\n> +                    last PAREN unless --$depth;\n> +\t            $field .= $1 if s/^(([^\\(\\)\\\\]|\\\\.)+)//;\n> +                }\n> +            }\n> +\n> +            carp \"Unmatched () '$field' '$_'\"\n> +                if $depth;\n> +\n> +            $field =~ s/\\s+\\Z//;\n> +            push @words, $field;\n> +\n> +            next;\n> +        }\n> +\n> +        if( s/^(\"(?:[^\"\\\\]+|\\\\.)*\")\\s*//       # \"...\"\n> +         || s/^(\\[(?:[^\\]\\\\]+|\\\\.)*\\])\\s*//    # [...]\n> +         || s/^([^\\s()<>\\@,;:\\\\\".[\\]]+)\\s*//\n> +         || s/^([()<>\\@,;:\\\\\".[\\]])\\s*//\n> +          )\n> +        {   push @words, $1;\n> +            next;\n> +        }\n> +\n> +        croak \"Unrecognised line: $_\";\n> +    }\n> +\n> +    push @words, \",\";\n> +    \\@words;\n> +}\n> +\n> +sub _find_next\n> +{   my ($idx, $tokens, $len) = @_;\n> +\n> +    while($idx < $len)\n> +    {   my $c = $tokens->[$idx];\n> +        return $c if $c eq ',' || $c eq ';' || $c eq '<';\n> +        $idx++;\n> +    }\n> +\n> +    \"\";\n> +}\n> +\n> +sub _complete\n> +{   my ($class, $phrase, $address, $comment) = @_;\n> +\n> +    @$phrase || @$comment || @$address\n> +       or return undef;\n> +\n> +    my $o = $class->new(join(\" \",@$phrase), join(\"\",@$address), join(\" \",@$comment));\n> +    @$phrase = @$address = @$comment = ();\n> +    $o;\n> +}\n> +\n> +#------------\n> +\n> +sub new(@)\n> +{   my $class = shift;\n> +    bless [@_], $class;\n> +}\n> +\n> +\n> +sub parse(@)\n> +{   my $class = shift;\n> +    my @line  = grep {defined} @_;\n> +    my $line  = join '', @line;\n> +\n> +    my (@phrase, @comment, @address, @objs);\n> +    my ($depth, $idx) = (0, 0);\n> +\n> +    my $tokens  = _tokenise @line;\n> +    my $len     = @$tokens;\n> +    my $next    = _find_next $idx, $tokens, $len;\n> +\n> +    local $_;\n> +    for(my $idx = 0; $idx < $len; $idx++)\n> +    {   $_ = $tokens->[$idx];\n> +\n> +        if(substr($_,0,1) eq '(') { push @comment, $_ }\n> +        elsif($_ eq '<')    { $depth++ }\n> +        elsif($_ eq '>')    { $depth-- if $depth }\n> +        elsif($_ eq ',' || $_ eq ';')\n> +        {   warn \"Unmatched '<>' in $line\" if $depth;\n> +            my $o = $class->_complete(\\@phrase, \\@address, \\@comment);\n> +            push @objs, $o if defined $o;\n> +            $depth = 0;\n> +            $next = _find_next $idx+1, $tokens, $len;\n> +        }\n> +        elsif($depth)       { push @address, $_ }\n> +        elsif($next eq '<') { push @phrase,  $_ }\n> +        elsif( /^[.\\@:;]$/ || !@address || $address[-1] =~ /^[.\\@:;]$/ )\n> +        {   push @address, $_ }\n> +        else\n> +        {   warn \"Unmatched '<>' in $line\" if $depth;\n> +            my $o = $class->_complete(\\@phrase, \\@address, \\@comment);\n> +            push @objs, $o if defined $o;\n> +            $depth = 0;\n> +            push @address, $_;\n> +        }\n> +    }\n> +    @objs;\n> +}\n> +\n> +#------------\n> +\n> +sub phrase  { shift->set_or_get(0, @_) }\n> +sub address { shift->set_or_get(1, @_) }\n> +sub comment { shift->set_or_get(2, @_) }\n> +\n> +sub set_or_get($)\n> +{   my ($self, $i) = (shift, shift);\n> +    @_ or return $self->[$i];\n> +\n> +    my $val = $self->[$i];\n> +    $self->[$i] = shift if @_;\n> +    $val;\n> +}\n> +\n> +\n> +my $atext = '[\\-\\w !#$%&\\'*+/=?^`{|}~]';\n> +sub format\n> +{   my @addrs;\n> +\n> +    foreach (@_)\n> +    {   my ($phrase, $email, $comment) = @$_;\n> +        my @addr;\n> +\n> +        if(defined $phrase && length $phrase)\n> +        {   push @addr\n> +              , $phrase =~ /^(?:\\s*$atext\\s*)+$/o ? $phrase\n> +              : $phrase =~ /(?<!\\\\)\"/             ? $phrase\n> +              :                                    qq(\"$phrase\");\n> +\n> +            push @addr, \"<$email>\"\n> +                if defined $email && length $email;\n> +        }\n> +        elsif(defined $email && length $email)\n> +        {   push @addr, $email;\n> +        }\n> +\n> +        if(defined $comment && $comment =~ /\\S/)\n> +        {   $comment =~ s/^\\s*\\(?/(/;\n> +            $comment =~ s/\\)?\\s*$/)/;\n> +        }\n> +\n> +        push @addr, $comment\n> +            if defined $comment && length $comment;\n> +\n> +        push @addrs, join(\" \", @addr)\n> +            if @addr;\n> +    }\n> +\n> +    join \", \", @addrs;\n> +}\n> +\n> +#------------\n> +\n> +sub name\n> +{   my $self   = shift;\n> +    my $phrase = $self->phrase;\n> +    my $addr   = $self->address;\n> +\n> +    $phrase    = $self->comment\n> +        unless defined $phrase && length $phrase;\n> +\n> +    my $name   = $self->_extract_name($phrase);\n> +\n> +    # first.last@domain address\n> +    if($name eq '' && $addr =~ /([^\\%\\.\\@_]+([\\._][^\\%\\.\\@_]+)+)[\\@\\%]/)\n> +    {   ($name  = $1) =~ s/[\\._]+/ /g;\n> +\t$name   = _extract_name $name;\n> +    }\n> +\n> +    if($name eq '' && $addr =~ m#/g=#i)    # X400 style address\n> +    {   my ($f) = $addr =~ m#g=([^/]*)#i;\n> +\tmy ($l) = $addr =~ m#s=([^/]*)#i;\n> +\t$name   = _extract_name \"$f $l\";\n> +    }\n> +\n> +    length $name ? $name : undef;\n> +}\n> +\n> +\n> +sub host\n> +{   my $addr = shift->address || '';\n> +    my $i    = rindex $addr, '@';\n> +    $i >= 0 ? substr($addr, $i+1) : undef;\n> +}\n> +\n> +\n> +sub user\n> +{   my $addr = shift->address || '';\n> +    my $i    = rindex $addr, '@';\n> +    $i >= 0 ? substr($addr,0,$i) : $addr;\n> +}\n> +\n> +1;\n> diff --git a/perl/Git/Mail/Address.pm b/perl/Git/Mail/Address.pm\n> new file mode 100755\n> index 0000000..2ce3e84\n> --- /dev/null\n> +++ b/perl/Git/Mail/Address.pm\n> @@ -0,0 +1,24 @@\n> +package Git::Mail::Address;\n> +use 5.008;\n> +use strict;\n> +use warnings;\n> +\n> +=head1 NAME\n> +\n> +Git::Mail::Address - Wrapper for the L<Mail::Address> module, in case it's not installed\n> +\n> +=head1 DESCRIPTION\n> +\n> +This module is only intended to be used for code shipping in the\n> +C<git.git> repository. Use it for anything else at your peril!\n> +\n> +=cut\n> +\n> +eval {\n> +    require Mail::Address;\n> +    1;\n> +} or do {\n> +    require Git::FromCPAN::Mail::Address;\n> +};\n> +\n> +1;\n\n\n--\nAlex Bennée\n"},{"id":"336135","messageId":"87h8rwo966.fsf@linaro.org","threadId":"47537","inReplyTo":"1515407674-5233-2-git-send-email-git@matthieu-moy.fr","subject":"Re: [PATCH v3 2/3] Remove now useless email-address parsing code","fromName":"Alex Bennée","fromEmail":"alex.bennee@linaro.org","sentAt":"2018-01-08T11:57:05Z","receivedAt":"2018-01-08T11:57:17Z","isPatch":true,"sender":{"key":"alex.bennee@linaro.org","avatar":"https://avatars.githubusercontent.com/u/22458?v=4"},"body":"\nMatthieu Moy <git@matthieu-moy.fr> writes:\n\n> We now use Mail::Address unconditionaly, hence parse_mailboxes is now\n> dead code. Remove it and its tests.\n>\n> Signed-off-by: Matthieu Moy <git@matthieu-moy.fr>\n\nReviewed-by: Alex Bennée <alex.bennee@linaro.org>\n\n> ---\n> No change since v2.\n>\n>  perl/Git.pm          | 71 ----------------------------------------------------\n>  t/t9000-addresses.sh | 27 --------------------\n>  t/t9000/test.pl      | 67 -------------------------------------------------\n>  3 files changed, 165 deletions(-)\n>  delete mode 100755 t/t9000-addresses.sh\n>  delete mode 100755 t/t9000/test.pl\n>\n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index ffa09ac..65e6b32 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -880,77 +880,6 @@ sub ident_person {\n>  \treturn \"$ident[0] <$ident[1]>\";\n>  }\n>\n> -=item parse_mailboxes\n> -\n> -Return an array of mailboxes extracted from a string.\n> -\n> -=cut\n> -\n> -# Very close to Mail::Address's parser, but we still have minor\n> -# differences in some cases (see t9000 for examples).\n> -sub parse_mailboxes {\n> -\tmy $re_comment = qr/\\((?:[^)]*)\\)/;\n> -\tmy $re_quote = qr/\"(?:[^\\\"\\\\]|\\\\.)*\"/;\n> -\tmy $re_word = qr/(?:[^][\"\\s()<>:;@\\\\,.]|\\\\.)+/;\n> -\n> -\t# divide the string in tokens of the above form\n> -\tmy $re_token = qr/(?:$re_quote|$re_word|$re_comment|\\S)/;\n> -\tmy @tokens = map { $_ =~ /\\s*($re_token)\\s*/g } @_;\n> -\tmy $end_of_addr_seen = 0;\n> -\n> -\t# add a delimiter to simplify treatment for the last mailbox\n> -\tpush @tokens, \",\";\n> -\n> -\tmy (@addr_list, @phrase, @address, @comment, @buffer) = ();\n> -\tforeach my $token (@tokens) {\n> -\t\tif ($token =~ /^[,;]$/) {\n> -\t\t\t# if buffer still contains undeterminated strings\n> -\t\t\t# append it at the end of @address or @phrase\n> -\t\t\tif ($end_of_addr_seen) {\n> -\t\t\t\tpush @phrase, @buffer;\n> -\t\t\t} else {\n> -\t\t\t\tpush @address, @buffer;\n> -\t\t\t}\n> -\n> -\t\t\tmy $str_phrase = join ' ', @phrase;\n> -\t\t\tmy $str_address = join '', @address;\n> -\t\t\tmy $str_comment = join ' ', @comment;\n> -\n> -\t\t\t# quote are necessary if phrase contains\n> -\t\t\t# special characters\n> -\t\t\tif ($str_phrase =~ /[][()<>:;@\\\\,.\\000-\\037\\177]/) {\n> -\t\t\t\t$str_phrase =~ s/(^|[^\\\\])\"/$1/g;\n> -\t\t\t\t$str_phrase = qq[\"$str_phrase\"];\n> -\t\t\t}\n> -\n> -\t\t\t# add \"<>\" around the address if necessary\n> -\t\t\tif ($str_address ne \"\" && $str_phrase ne \"\") {\n> -\t\t\t\t$str_address = qq[<$str_address>];\n> -\t\t\t}\n> -\n> -\t\t\tmy $str_mailbox = \"$str_phrase $str_address $str_comment\";\n> -\t\t\t$str_mailbox =~ s/^\\s*|\\s*$//g;\n> -\t\t\tpush @addr_list, $str_mailbox if ($str_mailbox);\n> -\n> -\t\t\t@phrase = @address = @comment = @buffer = ();\n> -\t\t\t$end_of_addr_seen = 0;\n> -\t\t} elsif ($token =~ /^\\(/) {\n> -\t\t\tpush @comment, $token;\n> -\t\t} elsif ($token eq \"<\") {\n> -\t\t\tpush @phrase, (splice @address), (splice @buffer);\n> -\t\t} elsif ($token eq \">\") {\n> -\t\t\t$end_of_addr_seen = 1;\n> -\t\t\tpush @address, (splice @buffer);\n> -\t\t} elsif ($token eq \"@\" && !$end_of_addr_seen) {\n> -\t\t\tpush @address, (splice @buffer), \"@\";\n> -\t\t} else {\n> -\t\t\tpush @buffer, $token;\n> -\t\t}\n> -\t}\n> -\n> -\treturn @addr_list;\n> -}\n> -\n>  =item hash_object ( TYPE, FILENAME )\n>\n>  Compute the SHA1 object id of the given C<FILENAME> considering it is\n> diff --git a/t/t9000-addresses.sh b/t/t9000-addresses.sh\n> deleted file mode 100755\n> index a1ebef6..0000000\n> --- a/t/t9000-addresses.sh\n> +++ /dev/null\n> @@ -1,27 +0,0 @@\n> -#!/bin/sh\n> -\n> -test_description='compare address parsing with and without Mail::Address'\n> -. ./test-lib.sh\n> -\n> -if ! test_have_prereq PERL; then\n> -\tskip_all='skipping perl interface tests, perl not available'\n> -\ttest_done\n> -fi\n> -\n> -perl -MTest::More -e 0 2>/dev/null || {\n> -\tskip_all=\"Perl Test::More unavailable, skipping test\"\n> -\ttest_done\n> -}\n> -\n> -perl -MMail::Address -e 0 2>/dev/null || {\n> -\tskip_all=\"Perl Mail::Address unavailable, skipping test\"\n> -\ttest_done\n> -}\n> -\n> -test_external_has_tap=1\n> -\n> -test_external_without_stderr \\\n> -\t'Perl address parsing function' \\\n> -\tperl \"$TEST_DIRECTORY\"/t9000/test.pl\n> -\n> -test_done\n> diff --git a/t/t9000/test.pl b/t/t9000/test.pl\n> deleted file mode 100755\n> index dfeaa9c..0000000\n> --- a/t/t9000/test.pl\n> +++ /dev/null\n> @@ -1,67 +0,0 @@\n> -#!/usr/bin/perl\n> -use lib (split(/:/, $ENV{GITPERLLIB}));\n> -\n> -use 5.008;\n> -use warnings;\n> -use strict;\n> -\n> -use Test::More qw(no_plan);\n> -use Mail::Address;\n> -\n> -BEGIN { use_ok('Git') }\n> -\n> -my @success_list = (q[Jane],\n> -\tq[jdoe@example.com],\n> -\tq[<jdoe@example.com>],\n> -\tq[Jane <jdoe@example.com>],\n> -\tq[Jane Doe <jdoe@example.com>],\n> -\tq[\"Jane\" <jdoe@example.com>],\n> -\tq[\"Doe, Jane\" <jdoe@example.com>],\n> -\tq[\"Jane@:;\\>.,()<Doe\" <jdoe@example.com>],\n> -\tq[Jane!#$%&'*+-/=?^_{|}~Doe' <jdoe@example.com>],\n> -\tq[\"<jdoe@example.com>\"],\n> -\tq[\"Jane jdoe@example.com\"],\n> -\tq[Jane Doe <jdoe    @   example.com  >],\n> -\tq[Jane       Doe <  jdoe@example.com  >],\n> -\tq[Jane @ Doe @ Jane @ Doe],\n> -\tq[\"Jane, 'Doe'\" <jdoe@example.com>],\n> -\tq['Doe, \"Jane' <jdoe@example.com>],\n> -\tq[\"Jane\" \"Do\"e <jdoe@example.com>],\n> -\tq[\"Jane' Doe\" <jdoe@example.com>],\n> -\tq[\"Jane Doe <jdoe@example.com>\" <jdoe@example.com>],\n> -\tq[\"Jane\\\" Doe\" <jdoe@example.com>],\n> -\tq[Doe, jane <jdoe@example.com>],\n> -\tq[\"Jane Doe <jdoe@example.com>],\n> -\tq['Jane 'Doe' <jdoe@example.com>],\n> -\tq[Jane@:;\\.,()<>Doe <jdoe@example.com>],\n> -\tq[Jane <jdoe@example.com> Doe],\n> -\tq[<jdoe@example.com> Jane Doe]);\n> -\n> -my @known_failure_list = (q[Jane\\ Doe <jdoe@example.com>],\n> -\tq[\"Doe, Ja\"ne <jdoe@example.com>],\n> -\tq[\"Doe, Katarina\" Jane <jdoe@example.com>],\n> -\tq[Jane jdoe@example.com],\n> -\tq[\"Jane \"Kat\"a\" ri\"na\" \",Doe\" <jdoe@example.com>],\n> -\tq[Jane Doe],\n> -\tq[Jane \"Doe <jdoe@example.com>\"],\n> -\tq[\\\"Jane Doe <jdoe@example.com>],\n> -\tq[Jane\\\"\\\" Doe <jdoe@example.com>],\n> -\tq['Jane \"Katarina\\\" \\' Doe' <jdoe@example.com>]);\n> -\n> -foreach my $str (@success_list) {\n> -\tmy @expected = map { $_->format } Mail::Address->parse(\"$str\");\n> -\tmy @actual = Git::parse_mailboxes(\"$str\");\n> -\tis_deeply(\\@expected, \\@actual, qq[same output : $str]);\n> -}\n> -\n> -TODO: {\n> -\tlocal $TODO = \"known breakage\";\n> -\tforeach my $str (@known_failure_list) {\n> -\t\tmy @expected = map { $_->format } Mail::Address->parse(\"$str\");\n> -\t\tmy @actual = Git::parse_mailboxes(\"$str\");\n> -\t\tis_deeply(\\@expected, \\@actual, qq[same output : $str]);\n> -\t}\n> -}\n> -\n> -my $is_passing = eval { Test::More->is_passing };\n> -exit($is_passing ? 0 : 1) unless $@ =~ /Can't locate object method/;\n\n\n--\nAlex Bennée\n"},{"id":"336165","messageId":"xmqqzi5o8a12.fsf@gitster.mtv.corp.google.com","threadId":"47537","inReplyTo":"1515407674-5233-3-git-send-email-git@matthieu-moy.fr","subject":"Re: [PATCH v3 3/3] send-email: add test for Linux's get_maintainer.pl","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-08T18:45:13Z","receivedAt":"2018-01-08T18:45:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <git@matthieu-moy.fr> writes:\n\n> From: Alex Bennée <alex.bennee@linaro.org>\n>\n> We had a regression that broke Linux's get_maintainer.pl. Using\n> Mail::Address to parse email addresses fixed it, but let's protect\n> against future regressions.\n>\n> Note that we need --cc-cmd to be relative because this option doesn't\n> accept spaces in script names (probably to allow --cc-cmd=\"executable\n> --option\"), while --smtp-server needs to be absolute.\n>\n> Patch-edited-by: Matthieu Moy <git@matthieu-moy.fr>\n> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>\n> Signed-off-by: Matthieu Moy <git@matthieu-moy.fr>\n> ---\n> Change since v2:\n>\n> * Mention relative Vs absolute path in commit message.\n>\n> * Remove useless \"chmod +x\"\n>\n> * Remove useless double quotes\n>\n>  t/t9001-send-email.sh | 19 +++++++++++++++++++\n>  1 file changed, 19 insertions(+)\n\nThanks, both.  \n\n\"while --smtp-server needs to be ...\" gave a \"Huh?\" for somebody who\nweren't familiar with the discussion that led to the addition of\nthat note (i.e. \"unlike a near-by test that passes a full-path\n$(pwd)/fake.endmail to --smtp-server\" would have helped); it is not\na big deal, though.\n\nLet's merge this to 'next' and try to merge in the first batch post\nthe release.\n\nThanks.\n\n\n\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> index 4d261c2..a06e5d7 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -172,6 +172,25 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '\n>  \ttest_cmp expected-cc commandline1\n>  '\n>  \n> +test_expect_success $PREREQ 'setup fake get_maintainer.pl script for cc trailer' \"\n> +\twrite_script expected-cc-script.sh <<-EOF\n> +\techo 'One Person <one@example.com> (supporter:THIS (FOO/bar))'\n> +\techo 'Two Person <two@example.com> (maintainer:THIS THING)'\n> +\techo 'Third List <three@example.com> (moderated list:THIS THING (FOO/bar))'\n> +\techo '<four@example.com> (moderated list:FOR THING)'\n> +\techo 'five@example.com (open list:FOR THING (FOO/bar))'\n> +\techo 'six@example.com (open list)'\n> +\tEOF\n> +\"\n> +\n> +test_expect_success $PREREQ 'cc trailer with get_maintainer.pl output' '\n> +\tclean_fake_sendmail &&\n> +\tgit send-email -1 --to=recipient@example.com \\\n> +\t\t--cc-cmd=./expected-cc-script.sh \\\n> +\t\t--smtp-server=\"$(pwd)/fake.sendmail\" &&\n> +\ttest_cmp expected-cc commandline1\n> +'\n> +\n>  test_expect_success $PREREQ 'setup expect' \"\n>  cat >expected-show-all-headers <<\\EOF\n>  0001-Second.patch\n"},{"id":"339283","messageId":"CACBZZX7xC37W5+MLtYSrBaPawh+QfOSqci_rFOp_ukVi4fp6Gg@mail.gmail.com","threadId":"47537","inReplyTo":"1515177413-12526-1-git-send-email-git@matthieu-moy.fr","subject":"Re: [PATCH v2 1/3] send-email: add and use a local copy of Mail::Address","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-02-14T14:59:28Z","receivedAt":"2018-02-14T15:00:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Jan 5, 2018 at 7:36 PM, Matthieu Moy <git@matthieu-moy.fr> wrote:\n\n>  create mode 100644 perl/Git/FromCPAN/Mail/Address.pm\n>  create mode 100755 perl/Git/Mail/Address.pm\n\nI didn't notice this in my initial review, but just now when it's\nlanded in master and it's shiny-green in my terminal, this file should\nbe 644, not 755.\n"}]}