Re: [RFC PATCH 2/2] Remove now useless email-address parsing code

[Matthieu Moy]

Alex Bennée  writes:

> Matthieu Moy  writes:
>
>> We now use Mail::Address unconditionaly, hence parse_mailboxes is now
>> dead code. Remove it and its tests.
>>
>> Signed-off-by: Matthieu Moy 
>> ---
>>  perl/Git.pm  | 71 
>> 
>>  t/t9000-addresses.sh | 27 
>>  t/t9000/test.pl  | 67 -
>>  3 files changed, 165 deletions(-)
>>  delete mode 100755 t/t9000-addresses.sh
>>  delete mode 100755 t/t9000/test.pl
>
> Should we add the tests for t9001-send-email.sh to guard against regressions?

Tests in t9001 were only useful with our parse_mailboxes (they were just
comparing parse_mailboxes and Mail::Address), so there's no point
keeping them after we delete parse_mailboxes.

Your added tests from
https://public-inbox.org/git/20171116154814.23785-1-alex.ben...@linaro.org
would make sense OTOH. Not breaking Linux's flow is a nice thing to
do ... Patch doing this follows (I'll resend the whole series with
Eric's nit later).

-- 
Matthieu Moy
https://matthieu-moy.fr/

Re: [RFC PATCH 2/2] Remove now useless email-address parsing code

[Alex Bennée]

Matthieu Moy  writes:

> We now use Mail::Address unconditionaly, hence parse_mailboxes is now
> dead code. Remove it and its tests.
>
> Signed-off-by: Matthieu Moy 
> ---
>  perl/Git.pm  | 71 
> 
>  t/t9000-addresses.sh | 27 
>  t/t9000/test.pl  | 67 -
>  3 files changed, 165 deletions(-)
>  delete mode 100755 t/t9000-addresses.sh
>  delete mode 100755 t/t9000/test.pl

Should we add the tests for t9001-send-email.sh to guard against regressions?

>
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 02a3871..9d60d79 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -880,77 +880,6 @@ sub ident_person {
>   return "$ident[0] <$ident[1]>";
>  }
>
> -=item parse_mailboxes
> -
> -Return an array of mailboxes extracted from a string.
> -
> -=cut
> -
> -# Very close to Mail::Address's parser, but we still have minor
> -# differences in some cases (see t9000 for examples).
> -sub parse_mailboxes {
> - my $re_comment = qr/\((?:[^)]*)\)/;
> - my $re_quote = qr/"(?:[^\"\\]|\\.)*"/;
> - my $re_word = qr/(?:[^]["\s()<>:;@\\,.]|\\.)+/;
> -
> - # divide the string in tokens of the above form
> - my $re_token = qr/(?:$re_quote|$re_word|$re_comment|\S)/;
> - my @tokens = map { $_ =~ /\s*($re_token)\s*/g } @_;
> - my $end_of_addr_seen = 0;
> -
> - # add a delimiter to simplify treatment for the last mailbox
> - push @tokens, ",";
> -
> - my (@addr_list, @phrase, @address, @comment, @buffer) = ();
> - foreach my $token (@tokens) {
> - if ($token =~ /^[,;]$/) {
> - # if buffer still contains undeterminated strings
> - # append it at the end of @address or @phrase
> - if ($end_of_addr_seen) {
> - push @phrase, @buffer;
> - } else {
> - push @address, @buffer;
> - }
> -
> - my $str_phrase = join ' ', @phrase;
> - my $str_address = join '', @address;
> - my $str_comment = join ' ', @comment;
> -
> - # quote are necessary if phrase contains
> - # special characters
> - if ($str_phrase =~ /[][()<>:;@\\,.\000-\037\177]/) {
> - $str_phrase =~ s/(^|[^\\])"/$1/g;
> - $str_phrase = qq["$str_phrase"];
> - }
> -
> - # add "<>" around the address if necessary
> - if ($str_address ne "" && $str_phrase ne "") {
> - $str_address = qq[<$str_address>];
> - }
> -
> - my $str_mailbox = "$str_phrase $str_address 
> $str_comment";
> - $str_mailbox =~ s/^\s*|\s*$//g;
> - push @addr_list, $str_mailbox if ($str_mailbox);
> -
> - @phrase = @address = @comment = @buffer = ();
> - $end_of_addr_seen = 0;
> - } elsif ($token =~ /^\(/) {
> - push @comment, $token;
> - } elsif ($token eq "<") {
> - push @phrase, (splice @address), (splice @buffer);
> - } elsif ($token eq ">") {
> - $end_of_addr_seen = 1;
> - push @address, (splice @buffer);
> - } elsif ($token eq "@" && !$end_of_addr_seen) {
> - push @address, (splice @buffer), "@";
> - } else {
> - push @buffer, $token;
> - }
> - }
> -
> - return @addr_list;
> -}
> -
>  =item hash_object ( TYPE, FILENAME )
>
>  Compute the SHA1 object id of the given C considering it is
> diff --git a/t/t9000-addresses.sh b/t/t9000-addresses.sh
> deleted file mode 100755
> index a1ebef6..000
> --- a/t/t9000-addresses.sh
> +++ /dev/null
> @@ -1,27 +0,0 @@
> -#!/bin/sh
> -
> -test_description='compare address parsing with and without Mail::Address'
> -. ./test-lib.sh
> -
> -if ! test_have_prereq PERL; then
> - skip_all='skipping perl interface tests, perl not available'
> - test_done
> -fi
> -
> -perl -MTest::More -e 0 2>/dev/null || {
> - skip_all="Perl Test::More unavailable, skipping test"
> - test_done
> -}
> -
> -perl -MMail::Address -e 0 2>/dev/null || {
> - skip_all="Perl Mail::Address unavailable, skipping test"
> - test_done
> -}
> -
> -test_external_has_tap=1
> -
> -test_external_without_stderr \
> - 'Perl address parsing function' \
> - perl "$TEST_DIRECTORY"/t9000/test.pl
> -
> -test_done
> diff --git a/t/t9000/test.pl b/t/t9000/test.pl
> deleted file mode 100755
> index dfeaa9c..000
> --- a/t/t9000/test.pl
> +++ /dev/null
> @@ -1,67 +0,0 @@
> -#!/usr/bin/perl
> -use lib (split(/:/, $ENV{GITPERLLIB}));
> -
> -use 

[RFC PATCH 2/2] Remove now useless email-address parsing code

[Matthieu Moy]

We now use Mail::Address unconditionaly, hence parse_mailboxes is now
dead code. Remove it and its tests.

Signed-off-by: Matthieu Moy 
---
 perl/Git.pm  | 71 
 t/t9000-addresses.sh | 27 
 t/t9000/test.pl  | 67 -
 3 files changed, 165 deletions(-)
 delete mode 100755 t/t9000-addresses.sh
 delete mode 100755 t/t9000/test.pl

diff --git a/perl/Git.pm b/perl/Git.pm
index 02a3871..9d60d79 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -880,77 +880,6 @@ sub ident_person {
return "$ident[0] <$ident[1]>";
 }
 
-=item parse_mailboxes
-
-Return an array of mailboxes extracted from a string.
-
-=cut
-
-# Very close to Mail::Address's parser, but we still have minor
-# differences in some cases (see t9000 for examples).
-sub parse_mailboxes {
-   my $re_comment = qr/\((?:[^)]*)\)/;
-   my $re_quote = qr/"(?:[^\"\\]|\\.)*"/;
-   my $re_word = qr/(?:[^]["\s()<>:;@\\,.]|\\.)+/;
-
-   # divide the string in tokens of the above form
-   my $re_token = qr/(?:$re_quote|$re_word|$re_comment|\S)/;
-   my @tokens = map { $_ =~ /\s*($re_token)\s*/g } @_;
-   my $end_of_addr_seen = 0;
-
-   # add a delimiter to simplify treatment for the last mailbox
-   push @tokens, ",";
-
-   my (@addr_list, @phrase, @address, @comment, @buffer) = ();
-   foreach my $token (@tokens) {
-   if ($token =~ /^[,;]$/) {
-   # if buffer still contains undeterminated strings
-   # append it at the end of @address or @phrase
-   if ($end_of_addr_seen) {
-   push @phrase, @buffer;
-   } else {
-   push @address, @buffer;
-   }
-
-   my $str_phrase = join ' ', @phrase;
-   my $str_address = join '', @address;
-   my $str_comment = join ' ', @comment;
-
-   # quote are necessary if phrase contains
-   # special characters
-   if ($str_phrase =~ /[][()<>:;@\\,.\000-\037\177]/) {
-   $str_phrase =~ s/(^|[^\\])"/$1/g;
-   $str_phrase = qq["$str_phrase"];
-   }
-
-   # add "<>" around the address if necessary
-   if ($str_address ne "" && $str_phrase ne "") {
-   $str_address = qq[<$str_address>];
-   }
-
-   my $str_mailbox = "$str_phrase $str_address 
$str_comment";
-   $str_mailbox =~ s/^\s*|\s*$//g;
-   push @addr_list, $str_mailbox if ($str_mailbox);
-
-   @phrase = @address = @comment = @buffer = ();
-   $end_of_addr_seen = 0;
-   } elsif ($token =~ /^\(/) {
-   push @comment, $token;
-   } elsif ($token eq "<") {
-   push @phrase, (splice @address), (splice @buffer);
-   } elsif ($token eq ">") {
-   $end_of_addr_seen = 1;
-   push @address, (splice @buffer);
-   } elsif ($token eq "@" && !$end_of_addr_seen) {
-   push @address, (splice @buffer), "@";
-   } else {
-   push @buffer, $token;
-   }
-   }
-
-   return @addr_list;
-}
-
 =item hash_object ( TYPE, FILENAME )
 
 Compute the SHA1 object id of the given C considering it is
diff --git a/t/t9000-addresses.sh b/t/t9000-addresses.sh
deleted file mode 100755
index a1ebef6..000
--- a/t/t9000-addresses.sh
+++ /dev/null
@@ -1,27 +0,0 @@
-#!/bin/sh
-
-test_description='compare address parsing with and without Mail::Address'
-. ./test-lib.sh
-
-if ! test_have_prereq PERL; then
-   skip_all='skipping perl interface tests, perl not available'
-   test_done
-fi
-
-perl -MTest::More -e 0 2>/dev/null || {
-   skip_all="Perl Test::More unavailable, skipping test"
-   test_done
-}
-
-perl -MMail::Address -e 0 2>/dev/null || {
-   skip_all="Perl Mail::Address unavailable, skipping test"
-   test_done
-}
-
-test_external_has_tap=1
-
-test_external_without_stderr \
-   'Perl address parsing function' \
-   perl "$TEST_DIRECTORY"/t9000/test.pl
-
-test_done
diff --git a/t/t9000/test.pl b/t/t9000/test.pl
deleted file mode 100755
index dfeaa9c..000
--- a/t/t9000/test.pl
+++ /dev/null
@@ -1,67 +0,0 @@
-#!/usr/bin/perl
-use lib (split(/:/, $ENV{GITPERLLIB}));
-
-use 5.008;
-use warnings;
-use strict;
-
-use Test::More qw(no_plan);
-use Mail::Address;
-
-BEGIN { use_ok('Git') }
-
-my @success_list = (q[Jane],
-   q[j...@example.com],
-   q[],
-   q[Jane ],
-   q[Jane Doe ],