[PATCH] git-send-email: fix get_maintainer.pl regression

[PATCH] git-send-email: fix get_maintainer.pl regression

[Alex Bennée]

Getting rid of Mail::Address regressed behaviour with common
get_maintainer scripts such as the Linux kernel. Fix the missed corner
case and add a test for it.

Fixes: cc9075067776ebd34cc08f31bf78bb05f12fd879

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 perl/Git.pm           |  3 +++
 t/t9000/test.pl       |  4 +++-
 t/t9001-send-email.sh | 21 +++++++++++++++++++++
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index ffa09ace9..9b17de1cc 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -936,6 +936,9 @@ sub parse_mailboxes {
 			$end_of_addr_seen = 0;
 		} elsif ($token =~ /^\(/) {
 			push @comment, $token;
+		} elsif ($token =~ /^\)/) {
+		        my $nested_comment = pop @comment;
+			push @comment, "$nested_comment$token";
 		} elsif ($token eq "<") {
 			push @phrase, (splice @address), (splice @buffer);
 		} elsif ($token eq ">") {
diff --git a/t/t9000/test.pl b/t/t9000/test.pl
index dfeaa9c65..f10be50cd 100755
--- a/t/t9000/test.pl
+++ b/t/t9000/test.pl
@@ -35,7 +35,9 @@ my @success_list = (q[Jane],
 	q['Jane 'Doe' <jdoe@example.com>],
 	q[Jane@:;\.,()<>Doe <jdoe@example.com>],
 	q[Jane <jdoe@example.com> Doe],
-	q[<jdoe@example.com> Jane Doe]);
+	q[<jdoe@example.com> Jane Doe],
+	q[jdoe@example.com (open list:for thing (foo/bar))],
+    );
 
 my @known_failure_list = (q[Jane\ Doe <jdoe@example.com>],
 	q["Doe, Ja"ne <jdoe@example.com>],
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 4d261c2a9..0bcd7ab96 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -172,6 +172,27 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '
 	test_cmp expected-cc commandline1
 '
 
+test_expect_success $PREREQ 'setup get_mainter script for cc trailer' "
+cat >expected-cc-script.sh <<-EOF && chmod +x expected-cc-script.sh
+#!/bin/sh
+echo 'One Person <one@example.com> (supporter:THIS (FOO/bar))'
+echo 'Two Person <two@example.com> (maintainer:THIS THING)'
+echo 'Third List <three@example.com> (moderated list:THIS THING (FOO/bar))'
+echo '<four@example.com> (moderated list:FOR THING)'
+echo 'five@example.com (open list:FOR THING (FOO/bar))'
+echo 'six@example.com (open list)'
+EOF
+"
+
+test_expect_success $PREREQ 'cc trailer with get_maintainer output' '
+	test_commit cc-trailer &&
+	clean_fake_sendmail &&
+	git send-email -1 --to=recipient@example.com \
+		--cc-cmd="$(pwd)/expected-cc-script.sh" \
+		--smtp-server="$(pwd)/fake.sendmail" &&
+	test_cmp expected-cc commandline1
+'
+
 test_expect_success $PREREQ 'setup expect' "
 cat >expected-show-all-headers <<\EOF
 0001-Second.patch
-- 
2.15.0

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> Getting rid of Mail::Address regressed behaviour with common
> get_maintainer scripts such as the Linux kernel. Fix the missed corner
> case and add a test for it.
>
<snip>
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 4d261c2a9..0bcd7ab96 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -172,6 +172,27 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '
>  	test_cmp expected-cc commandline1
>  '
>
> +test_expect_success $PREREQ 'setup get_mainter script for cc trailer' "
> +cat >expected-cc-script.sh <<-EOF && chmod +x expected-cc-script.sh
> +#!/bin/sh
> +echo 'One Person <one@example.com> (supporter:THIS (FOO/bar))'
> +echo 'Two Person <two@example.com> (maintainer:THIS THING)'
> +echo 'Third List <three@example.com> (moderated list:THIS THING (FOO/bar))'
> +echo '<four@example.com> (moderated list:FOR THING)'
> +echo 'five@example.com (open list:FOR THING (FOO/bar))'
> +echo 'six@example.com (open list)'
> +EOF
> +"
> +
> +test_expect_success $PREREQ 'cc trailer with get_maintainer output' '
> +	test_commit cc-trailer &&
> +	clean_fake_sendmail &&
> +	git send-email -1 --to=recipient@example.com \
> +		--cc-cmd="$(pwd)/expected-cc-script.sh" \
> +		--smtp-server="$(pwd)/fake.sendmail" &&
> +	test_cmp expected-cc commandline1
> +'
> +

OK I'm afraid I don't fully understand the test harness as this breaks a
bunch of other tests. If anyone can offer some pointers on how to fix
I'd be grateful.

In the meantime I know the core change works because I tested with:

#+name: send-patches-dry-run
#+begin_src sh :results output
# temp workaround
export PERL5LIB=/home/alex/src/git.git/perl/
git send-email --confirm=never --dry-run --quiet ${mailto} ${series}.patches/*
#+end_src

When I sent my last set of kernel patches to the list (the workflow that
was broken before by the cc9075067776ebd34cc08f31bf78bb05f12fd879 change
landing via my git stable PPA).

--
Alex Bennée

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Eric Sunshine]

On Thu, Nov 16, 2017 at 10:48 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
> Getting rid of Mail::Address regressed behaviour with common
> get_maintainer scripts such as the Linux kernel. Fix the missed corner
> case and add a test for it.

It is not at all clear, based upon this text, what this is fixing.
When you re-roll, please provide a description of the regression in
sufficient detail for readers to easily understand the problem and the
solution.

More below...

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> diff --git a/t/t9000/test.pl b/t/t9000/test.pl
> @@ -35,7 +35,9 @@ my @success_list = (q[Jane],
>         q['Jane 'Doe' <jdoe@example.com>],
>         q[Jane@:;\.,()<>Doe <jdoe@example.com>],
>         q[Jane <jdoe@example.com> Doe],
> -       q[<jdoe@example.com> Jane Doe]);
> +       q[<jdoe@example.com> Jane Doe],
> +       q[jdoe@example.com (open list:for thing (foo/bar))],
> +    );

Style nit: The dangling ");" introduced by this change differs from
the other list(s) in this file which have the closing ");" on the same
line as the last item in the list.

> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> +test_expect_success $PREREQ 'setup get_mainter script for cc trailer' "
> +cat >expected-cc-script.sh <<-EOF && chmod +x expected-cc-script.sh
> +#!/bin/sh
> +echo 'One Person <one@example.com> (supporter:THIS (FOO/bar))'
> +echo 'Two Person <two@example.com> (maintainer:THIS THING)'
> +echo 'Third List <three@example.com> (moderated list:THIS THING (FOO/bar))'
> +echo '<four@example.com> (moderated list:FOR THING)'
> +echo 'five@example.com (open list:FOR THING (FOO/bar))'
> +echo 'six@example.com (open list)'
> +EOF
> +"

Use write_script() to create the script:

--- 8< ---
write_script expected-cc-script.sh <<-\EOF &&
echo '...'
...
EOF
--- 8< ---

No need for #!/bin/sh or chmod, both of which are handled by
write_script(). In fact, you could fold this into the following test
(since there doesn't seem a good reason for it to be separate).

> +test_expect_success $PREREQ 'cc trailer with get_maintainer output' '
> +       test_commit cc-trailer &&
> +       clean_fake_sendmail &&
> +       git send-email -1 --to=recipient@example.com \
> +               --cc-cmd="$(pwd)/expected-cc-script.sh" \
> +               --smtp-server="$(pwd)/fake.sendmail" &&
> +       test_cmp expected-cc commandline1
> +'
> OK I'm afraid I don't fully understand the test harness as this breaks a
> bunch of other tests. If anyone can offer some pointers on how to fix
> I'd be grateful.

There are several problems:

* test_commit bombs because there is already a tag named "cc-trailer"
created by an earlier test. Fix this by choosing a different name for
the commit. Even better would be to avoid making the commit in the
first place since it doesn't appear to be relevant to the test, and
the test works fine without it.

* The directory in which the expected-cc-script.sh is created contains
a space; this is intentional to catch bugs in tests and Git itself. In
this case, your test is exposing what might be considered a bug in
git-send-email itself, in which it invokes the --cc-cmd as "/path/with
space/expected-cc-script.sh", which is interpreted as trying to invoke
program "/path/with" with argument "space/expected-cc-script.sh". One
fix (which you could submit as a preparatory patch, making this a
2-patch series) would be this:

--- 8< ---
diff --git a/git-send-email.perl b/git-send-email.perl
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1724,7 +1724,7 @@ sub recipients_cmd {
    my ($prefix, $what, $cmd, $file) = @_;

     my @addresses = ();
-    open my $fh, "-|", "$cmd \Q$file\E"
+   open my $fh, "-|", "\Q$cmd\E \Q$file\E"
     or die sprintf(__("(%s) Could not execute '%s'"), $prefix, $cmd);
     while (my $address = <$fh>) {
          $address =~ s/^\s*//g;
--- 8< ---

However, it's possible that might break existing users who rely on
--cc-cmd="myscript --option arg" working. It's not clear which
behavior is correct.

* Subsequent tests fail because you've added a commit which they are
not expecting. If you look at tests earlier in the file, you will see
that they deal with this by removing the added commit (via "git reset
--hard HEAD^") by the time the test finishes. However, as noted above,
this new test doesn't actually need to make this commit in the first
place.

So, with fixes, your test might look like this:

--- 8< ---
test_expect_success $PREREQ 'cc trailer with get_maintainer output' '
    test_commit cc-trailer-get-maintainer &&
    test_when_finished "git reset --hard HEAD^" &&
    clean_fake_sendmail &&
    git send-email -1 --to=recipient@example.com \
        --cc-cmd="$(pwd)/expected-cc-script.sh" \
        --smtp-server="$(pwd)/fake.sendmail" &&
    test_cmp expected-cc commandline1
'
--- 8< ---

Or, if you drop the unnecessary test_commit():

--- 8< ---
test_expect_success $PREREQ 'cc trailer with get_maintainer output' '
    clean_fake_sendmail &&
    git send-email -1 --to=recipient@example.com \
        --cc-cmd="$(pwd)/expected-cc-script.sh" \
        --smtp-server="$(pwd)/fake.sendmail" &&
    test_cmp expected-cc commandline1
'
--- 8< ---

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Alex Bennée]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, Nov 16, 2017 at 10:48 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>> Getting rid of Mail::Address regressed behaviour with common
>> get_maintainer scripts such as the Linux kernel. Fix the missed corner
>> case and add a test for it.
>
> It is not at all clear, based upon this text, what this is fixing.
> When you re-roll, please provide a description of the regression in
> sufficient detail for readers to easily understand the problem and the
> solution.

How about:

Since the removal of Mail::Address from git-send-email certain addresses
common in MAINTAINERS now fail to get correctly parsed by
Git::parse_mailboxes. Specifically the patterns with embedded
parenthesis fail, for example for MAINTAINERS:

  KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm)
  L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
  L:	kvmarm@lists.cs.columbia.edu

Is returned by get_maintainers.pl as:

  linux-arm-kernel@lists.infradead.org (moderated list:KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm))
  kvmarm@lists.cs.columbia.edu (open list:KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm))

But the parse_mailboxes code mangles the address, appending the trailing
parenthesis to the email address to the address part causing it to fail validation:

   error: unable to extract a valid address from: linux-arm-kernel@lists.infradead.org) (moderated list:KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm)
   error: unable to extract a valid address from: kvmarm@lists.cs.columbia.edu) (open list:KERNEL VIRTUAL MACHINE FOR ARM (KVM/arm)

As this is a common pattern which was handled by Mail::Address I've
fixed the regression by explicitly capturing a trailing bracket and
appending it to the comment token.

>
> More below...
>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>> diff --git a/t/t9000/test.pl b/t/t9000/test.pl
>> @@ -35,7 +35,9 @@ my @success_list = (q[Jane],
>>         q['Jane 'Doe' <jdoe@example.com>],
>>         q[Jane@:;\.,()<>Doe <jdoe@example.com>],
>>         q[Jane <jdoe@example.com> Doe],
>> -       q[<jdoe@example.com> Jane Doe]);
>> +       q[<jdoe@example.com> Jane Doe],
>> +       q[jdoe@example.com (open list:for thing (foo/bar))],
>> +    );
>
> Style nit: The dangling ");" introduced by this change differs from
> the other list(s) in this file which have the closing ");" on the same
> line as the last item in the list.
>
>> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
>> +test_expect_success $PREREQ 'setup get_mainter script for cc trailer' "
>> +cat >expected-cc-script.sh <<-EOF && chmod +x expected-cc-script.sh
>> +#!/bin/sh
>> +echo 'One Person <one@example.com> (supporter:THIS (FOO/bar))'
>> +echo 'Two Person <two@example.com> (maintainer:THIS THING)'
>> +echo 'Third List <three@example.com> (moderated list:THIS THING (FOO/bar))'
>> +echo '<four@example.com> (moderated list:FOR THING)'
>> +echo 'five@example.com (open list:FOR THING (FOO/bar))'
>> +echo 'six@example.com (open list)'
>> +EOF
>> +"
>
> Use write_script() to create the script:

Thanks for the pointers, I'll fix it up.

I missed the existence of write_script.sh while I scanned through
t/README, perhaps a stanza in in "Test harness library":

 - write_script <name> <<-\EOF && <rest of test>
   echo '...'
   ...
   EOF

   The write_script helper takes care of ensuring the created helper
   script has the right shebang and is executable.

?

>
> --- 8< ---
> write_script expected-cc-script.sh <<-\EOF &&
> echo '...'
> ...
> EOF
> --- 8< ---
>
> No need for #!/bin/sh or chmod, both of which are handled by
> write_script(). In fact, you could fold this into the following test
> (since there doesn't seem a good reason for it to be separate).
>
>> +test_expect_success $PREREQ 'cc trailer with get_maintainer output' '
>> +       test_commit cc-trailer &&
>> +       clean_fake_sendmail &&
>> +       git send-email -1 --to=recipient@example.com \
>> +               --cc-cmd="$(pwd)/expected-cc-script.sh" \
>> +               --smtp-server="$(pwd)/fake.sendmail" &&
>> +       test_cmp expected-cc commandline1
>> +'
>> OK I'm afraid I don't fully understand the test harness as this breaks a
>> bunch of other tests. If anyone can offer some pointers on how to fix
>> I'd be grateful.
>
> There are several problems:
>
> * test_commit bombs because there is already a tag named "cc-trailer"
> created by an earlier test. Fix this by choosing a different name for
> the commit. Even better would be to avoid making the commit in the
> first place since it doesn't appear to be relevant to the test, and
> the test works fine without it.

Ahh that makes sense. Again perhaps in the t/README "Keep in mind:"

 - All the tests in a given test file run sequentially and share
   repository state. This means you should take care not to break
   assumptions of later tests as to which commits exist in the test
   repository.

?

>
> * The directory in which the expected-cc-script.sh is created contains
> a space; this is intentional to catch bugs in tests and Git itself. In
> this case, your test is exposing what might be considered a bug in
> git-send-email itself, in which it invokes the --cc-cmd as "/path/with
> space/expected-cc-script.sh", which is interpreted as trying to invoke
> program "/path/with" with argument "space/expected-cc-script.sh". One
> fix (which you could submit as a preparatory patch, making this a
> 2-patch series) would be this:
>
> --- 8< ---
> diff --git a/git-send-email.perl b/git-send-email.perl
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1724,7 +1724,7 @@ sub recipients_cmd {
>     my ($prefix, $what, $cmd, $file) = @_;
>
>      my @addresses = ();
> -    open my $fh, "-|", "$cmd \Q$file\E"
> +   open my $fh, "-|", "\Q$cmd\E \Q$file\E"
>      or die sprintf(__("(%s) Could not execute '%s'"), $prefix, $cmd);
>      while (my $address = <$fh>) {
>           $address =~ s/^\s*//g;
> --- 8< ---
>
> However, it's possible that might break existing users who rely on
> --cc-cmd="myscript --option arg" working. It's not clear which
> behavior is correct.
>
> * Subsequent tests fail because you've added a commit which they are
> not expecting. If you look at tests earlier in the file, you will see
> that they deal with this by removing the added commit (via "git reset
> --hard HEAD^") by the time the test finishes. However, as noted above,
> this new test doesn't actually need to make this commit in the first
> place.
>
> So, with fixes, your test might look like this:
>
> --- 8< ---
> test_expect_success $PREREQ 'cc trailer with get_maintainer output' '
>     test_commit cc-trailer-get-maintainer &&
>     test_when_finished "git reset --hard HEAD^" &&
>     clean_fake_sendmail &&
>     git send-email -1 --to=recipient@example.com \
>         --cc-cmd="$(pwd)/expected-cc-script.sh" \
>         --smtp-server="$(pwd)/fake.sendmail" &&
>     test_cmp expected-cc commandline1
> '
> --- 8< ---
>
> Or, if you drop the unnecessary test_commit():
>
> --- 8< ---
> test_expect_success $PREREQ 'cc trailer with get_maintainer output' '
>     clean_fake_sendmail &&
>     git send-email -1 --to=recipient@example.com \
>         --cc-cmd="$(pwd)/expected-cc-script.sh" \
>         --smtp-server="$(pwd)/fake.sendmail" &&
>     test_cmp expected-cc commandline1
> '
> --- 8< ---

Thanks for the comments. v2 being rolled ;-)

--
Alex Bennée

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Eric Sunshine]

On Mon, Nov 20, 2017 at 5:44 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>> It is not at all clear, based upon this text, what this is fixing.
>> When you re-roll, please provide a description of the regression in
>> sufficient detail for readers to easily understand the problem and the
>> solution.
>
> How about:
>
> Since the removal of Mail::Address from git-send-email certain addresses
> common in MAINTAINERS now fail to get correctly parsed by
> Git::parse_mailboxes. Specifically the patterns with embedded
> parenthesis fail, for example for MAINTAINERS:
> [...snip...]

Thanks, that explanation makes the problem quite clear. It also
allowed me to examine the fix with a more critical eye, which led to
several additional comments and observations (sent in my previous
email).

>> Use write_script() to create the script:
>
> Thanks for the pointers, I'll fix it up.
>
> I missed the existence of write_script.sh while I scanned through
> t/README, perhaps a stanza in in "Test harness library":
>
>  - write_script <name> <<-\EOF && <rest of test>
>    echo '...'
>    ...
>    EOF
>
>    The write_script helper takes care of ensuring the created helper
>    script has the right shebang and is executable.
> ?

Mentioning write_script() in the "Test harness library" section might
indeed be a good idea.

> Ahh that makes sense. Again perhaps in the t/README "Keep in mind:"
>
>  - All the tests in a given test file run sequentially and share
>    repository state. This means you should take care not to break
>    assumptions of later tests as to which commits exist in the test
>    repository.
> ?

Maybe, maybe not. Ideally, tests should be as self-contained as
possible. In practice, of course, this isn't always practical or even
possible (and there is a lot of old test code which is heavily
dependent upon state left over from earlier tests). Perfectly
self-contained tests (or self-contained _sets_ of tests) could
theoretically be run in parallel, so, in general, we'd like to
encourage people to tend toward self-contained, thus the above snippet
may be going in the wrong direction.

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Eric Sunshine]

On Sat, Nov 18, 2017 at 9:54 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Nov 16, 2017 at 10:48 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>> +test_expect_success $PREREQ 'cc trailer with get_maintainer output' '
>> +       [...]
>> +       git send-email -1 --to=recipient@example.com \
>> +               --cc-cmd="$(pwd)/expected-cc-script.sh" \
>> +       [...]
>> +'
>> OK I'm afraid I don't fully understand the test harness as this breaks a
>> bunch of other tests. If anyone can offer some pointers on how to fix
>> I'd be grateful.
>
> There are several problems:
> [...]
> * The directory in which the expected-cc-script.sh is created contains
> a space; this is intentional to catch bugs in tests and Git itself. In
> this case, your test is exposing what might be considered a bug in
> git-send-email itself, in which it invokes the --cc-cmd as "/path/with
> space/expected-cc-script.sh", which is interpreted as trying to invoke
> program "/path/with" with argument "space/expected-cc-script.sh". One
> fix (which you could submit as a preparatory patch, making this a
> 2-patch series) would be this:
>
> --- 8< ---
> diff --git a/git-send-email.perl b/git-send-email.perl
> @@ -1724,7 +1724,7 @@ sub recipients_cmd {
> -    open my $fh, "-|", "$cmd \Q$file\E"
> +   open my $fh, "-|", "\Q$cmd\E \Q$file\E"
> --- 8< ---
>
> However, it's possible that might break existing users who rely on
> --cc-cmd="myscript --option arg" working. It's not clear which
> behavior is correct.

The more I think about this, the less I consider this a bug in
git-send-email. As noted, people might legitimately use a complex
command (--cc-cmd="myscript--option arg"), so changing git-send-email
to treat cc-cmd as an atomic string seems like a bad idea.

Assuming no changes to git-send-email, to get your test working, you
could try to figure out how to quote the script's path you're
specifying with --cc-cmd, however, even easier would be to drop $(pwd)
altogether. That is, instead of:

    --cc-cmd="$(pwd)/expected-cc-script.sh"

just use:

    --cc-cmd=./expected-cc-script.sh

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Philip Oakley]

From: "Eric Sunshine" <sunshine@sunshineco.com>
On Sat, Nov 18, 2017 at 9:54 PM, Eric Sunshine <sunshine@sunshineco.com>
wrote:
> On Thu, Nov 16, 2017 at 10:48 AM, Alex Bennée <alex.bennee@linaro.org>
> wrote:
>> +test_expect_success $PREREQ 'cc trailer with get_maintainer output' '
>> +       [...]
>> +       git send-email -1 --to=recipient@example.com \
>> +               --cc-cmd="$(pwd)/expected-cc-script.sh" \
>> +       [...]
>> +'
>> OK I'm afraid I don't fully understand the test harness as this breaks a
>> bunch of other tests. If anyone can offer some pointers on how to fix
>> I'd be grateful.
>
> There are several problems:
> [...]
> * The directory in which the expected-cc-script.sh is created contains
> a space; this is intentional to catch bugs in tests and Git itself. In
> this case, your test is exposing what might be considered a bug in
> git-send-email itself, in which it invokes the --cc-cmd as "/path/with
> space/expected-cc-script.sh", which is interpreted as trying to invoke
> program "/path/with" with argument "space/expected-cc-script.sh". One
> > fix (which you could submit as a preparatory patch, making this a
> > 2-patch series) would be this:
> >
> > --- 8< ---
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > @@ -1724,7 +1724,7 @@ sub recipients_cmd {
> > -    open my $fh, "-|", "$cmd \Q$file\E"
> > +   open my $fh, "-|", "\Q$cmd\E \Q$file\E"
> > --- 8< ---
> >
> > However, it's possible that might break existing users who rely on
> > --cc-cmd="myscript --option arg" working. It's not clear which
> > behavior is correct.
>
> The more I think about this, the less I consider this a bug in
> git-send-email. As noted, people might legitimately use a complex
> command (--cc-cmd="myscript--option arg"), so changing git-send-email
> to treat cc-cmd as an atomic string seems like a bad idea.

A while back I proposed some documentation updates
https://public-inbox.org/git/1437416790-5792-1-git-send-email-philipoakley@iee.org/
regarding what is (should be) allowed in the cc-cmd etc., and at the time
Junio suggested that possible existing uses of the current code would be
abuses. I didn't pursue it further, but it may be useful guidance here as to
potential real world command lines..

>
> Assuming no changes to git-send-email, to get your test working, you
> could try to figure out how to quote the script's path you're
> specifying with --cc-cmd, however, even easier would be to drop $(pwd)
> altogether. That is, instead of:
>
>     --cc-cmd="$(pwd)/expected-cc-script.sh"
>
> just use:
>
>     --cc-cmd=./expected-cc-script.sh

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Eric Sunshine]

On Mon, Nov 20, 2017 at 7:07 PM, Philip Oakley <philipoakley@iee.org> wrote:
> From: "Eric Sunshine" <sunshine@sunshineco.com>
> On Sat, Nov 18, 2017 at 9:54 PM, Eric Sunshine <sunshine@sunshineco.com>
> wrote:
>> > --- 8< ---
>> > diff --git a/git-send-email.perl b/git-send-email.perl
>> > @@ -1724,7 +1724,7 @@ sub recipients_cmd {
>> > -    open my $fh, "-|", "$cmd \Q$file\E"
>> > +   open my $fh, "-|", "\Q$cmd\E \Q$file\E"
>> > --- 8< ---
>> >
>> > However, it's possible that might break existing users who rely on
>> > --cc-cmd="myscript --option arg" working. It's not clear which
>> > behavior is correct.
>>
>> The more I think about this, the less I consider this a bug in
>> git-send-email. As noted, people might legitimately use a complex
>> command (--cc-cmd="myscript--option arg"), so changing git-send-email
>> to treat cc-cmd as an atomic string seems like a bad idea.
>
> A while back I proposed some documentation updates
> https://public-inbox.org/git/1437416790-5792-1-git-send-email-philipoakley@iee.org/
> regarding what is (should be) allowed in the cc-cmd etc., and at the time
> Junio suggested that possible existing uses of the current code would be
> abuses. I didn't pursue it further, but it may be useful guidance here as to
> potential real world command lines..

Thanks for the link. I had forgotten that discussion, but re-reading
it brought most of it back. Given how the discussion ended -- with
valid use being that --cc-cmd names a program which accepts a single
argument -- then the above patch to recipients_cmd() might indeed be
desirable.

And, the documentation is still lacking, saying nothing about that
single argument passed to the cc-cmd...

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> The more I think about this, the less I consider this a bug in
> git-send-email. As noted, people might legitimately use a complex
> command (--cc-cmd="myscript--option arg"), so changing git-send-email
> to treat cc-cmd as an atomic string seems like a bad idea.

I concur.  Thanks for thinking this through.

> Assuming no changes to git-send-email, to get your test working, you
> could try to figure out how to quote the script's path you're
> specifying with --cc-cmd, however, even easier would be to drop $(pwd)
> altogether. That is, instead of:
>
>     --cc-cmd="$(pwd)/expected-cc-script.sh"
>
> just use:
>
>     --cc-cmd=./expected-cc-script.sh

Sounds sensible.

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Eric Sunshine]

A few more comments/observations...

On Thu, Nov 16, 2017 at 10:48 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
> diff --git a/perl/Git.pm b/perl/Git.pm
> @@ -936,6 +936,9 @@ sub parse_mailboxes {
>                         $end_of_addr_seen = 0;
>                 } elsif ($token =~ /^\(/) {
>                         push @comment, $token;
> +               } elsif ($token =~ /^\)/) {
> +                       my $nested_comment = pop @comment;
> +                       push @comment, "$nested_comment$token";

Due to the way tokenization works, it looks like you will only ever
see a ")" as a single character. That suggests that you should be
using ($token eq ")"), as is done for "<" and ">", rather than ($token
=~ /^\)/).

What happens if there is text before the final closing ')'? For
instance, "foo@bar (bibble (bobble) smoo)" or "...)smoo)". The result
is that "smoo" ends up tacked onto the end of the email address
("foo@barsmoo") rather than incorporated into the comment, as
intended.

What happens if you encounter a ")" but haven't yet encountered an
opening "(" (that is, @comment is empty)? For example, "foo@bar )". In
that case, it unconditionally pops from the empty array, which seems
iffy at best. It might be nice to see this case taken into
consideration explicitly.

I also was wondering if it would make more sense to take advantage of
Perl's ability to match nested expressions (??{$nested}), however,
that feature apparently was added in 5.10, and Git.pm only requires
5.8, so perhaps not (unless we want to bump the requirement higher).

Aside from those observations, it looks like the tokenizer in this
function is broken. For any input with the address enclosed in "<" and
">", the comment is lost entirely; it doesn't even end up in the
@tokens array. Since you're already fixing bugs/regressions in this
code, perhaps that's something you'd like to tackle as well in a
separate patch? ("No" is an acceptable answer, of course.)

>                 } elsif ($token eq "<") {
>                         push @phrase, (splice @address), (splice @buffer);
>                 } elsif ($token eq ">") {

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Alex Bennée]

Eric Sunshine <sunshine@sunshineco.com> writes:

> A few more comments/observations...
>
> On Thu, Nov 16, 2017 at 10:48 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>> diff --git a/perl/Git.pm b/perl/Git.pm
>> @@ -936,6 +936,9 @@ sub parse_mailboxes {
>>                         $end_of_addr_seen = 0;
>>                 } elsif ($token =~ /^\(/) {
>>                         push @comment, $token;
>> +               } elsif ($token =~ /^\)/) {
>> +                       my $nested_comment = pop @comment;
>> +                       push @comment, "$nested_comment$token";
>
> Due to the way tokenization works, it looks like you will only ever
> see a ")" as a single character. That suggests that you should be
> using ($token eq ")"), as is done for "<" and ">", rather than ($token
> =~ /^\)/).
>
> What happens if there is text before the final closing ')'? For
> instance, "foo@bar (bibble (bobble) smoo)" or "...)smoo)". The result
> is that "smoo" ends up tacked onto the end of the email address
> ("foo@barsmoo") rather than incorporated into the comment, as
> intended.
>
> What happens if you encounter a ")" but haven't yet encountered an
> opening "(" (that is, @comment is empty)? For example, "foo@bar )". In
> that case, it unconditionally pops from the empty array, which seems
> iffy at best. It might be nice to see this case taken into
> consideration explicitly.

Yeah I was only really aiming for the current regression but I'm sure it
could be more solid. I do note that my @known_failure_list in test.pl
has a bunch of other cases that need fixing up.

> I also was wondering if it would make more sense to take advantage of
> Perl's ability to match nested expressions (??{$nested}), however,
> that feature apparently was added in 5.10, and Git.pm only requires
> 5.8, so perhaps not (unless we want to bump the requirement higher).

Hmm that might be a case of abusing regex to do something better suited
to a proper tokenizer.

>
> Aside from those observations, it looks like the tokenizer in this
> function is broken. For any input with the address enclosed in "<" and
> ">", the comment is lost entirely; it doesn't even end up in the
> @tokens array. Since you're already fixing bugs/regressions in this
> code, perhaps that's something you'd like to tackle as well in a
> separate patch? ("No" is an acceptable answer, of course.)
>
>>                 } elsif ($token eq "<") {
>>                         push @phrase, (splice @address), (splice @buffer);
>>                 } elsif ($token eq ">") {

I can have a go but my perl-fu has weakened somewhat since I stopped
having to maintain perl code for a living. It's almost as though my
brain was glad to dump the knowledge ;-)

I guess we could maintain a nesting count and a current token type and
use that to more intelligently direct the nested portions to the
appropriate bits. Maybe Matthieu or Remi (CC'ed) might want to chime in
on other options?

--
Alex Bennée

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Thomas Adam]

On Tue, Nov 21, 2017 at 08:46:59PM +0000, Alex Bennée wrote:
> 
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > Aside from those observations, it looks like the tokenizer in this
> > function is broken. For any input with the address enclosed in "<" and
> > ">", the comment is lost entirely; it doesn't even end up in the
> > @tokens array. Since you're already fixing bugs/regressions in this
> > code, perhaps that's something you'd like to tackle as well in a
> > separate patch? ("No" is an acceptable answer, of course.)
> >
> >>                 } elsif ($token eq "<") {
> >>                         push @phrase, (splice @address), (splice @buffer);
> >>                 } elsif ($token eq ">") {
> 
> I can have a go but my perl-fu has weakened somewhat since I stopped
> having to maintain perl code for a living. It's almost as though my
> brain was glad to dump the knowledge ;-)
> 
> I guess we could maintain a nesting count and a current token type and
> use that to more intelligently direct the nested portions to the
> appropriate bits. Maybe Matthieu or Remi (CC'ed) might want to chime in
> on other options?

Trying to come up with a reinvention of regexps for email addresses is asking
for trouble, not to mention a crappy rod for your own back.  Don't do that.
This is why people use Mail::Address.

https://metacpan.org/pod/distribution/MailTools/lib/Mail/Address.pod

-- Thomas Adam

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Junio C Hamano]

Thomas Adam <thomas@xteddy.org> writes:

> Trying to come up with a reinvention of regexps for email addresses is asking
> for trouble, not to mention a crappy rod for your own back.  Don't do that.
> This is why people use Mail::Address.
>
> https://metacpan.org/pod/distribution/MailTools/lib/Mail/Address.pod

Now we are coming back to cc907506 ("send-email: don't use
Mail::Address, even if available", 2017-08-23).  It argues

    * Having this optional Mail::Address makes it tempting to anwser "please
      install Mail::Address" to users instead of fixing our own code. We've
      reached the stage where bugs in our parser should be fixed, not worked
      around.

but if it costs us maintaining our substitute that much, it seems to
me that depending on Mail::Address is not just tempting but may be a
sensible way forward.

Was there any reason why Mail::Address was _inadequate_?  I know we
had trouble with random garbage that are *not* addresses people put
on the in-body CC: trailer in the past, but I do not recall if they
are something Mail::Address would give worse result and we need our
workaround (hence our own substitute), or Mail::Address would handle
them just fine.

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Alex Bennée]

Junio C Hamano <gitster@pobox.com> writes:

> Thomas Adam <thomas@xteddy.org> writes:
>
>> Trying to come up with a reinvention of regexps for email addresses is asking
>> for trouble, not to mention a crappy rod for your own back.  Don't do that.
>> This is why people use Mail::Address.
>>
>> https://metacpan.org/pod/distribution/MailTools/lib/Mail/Address.pod
>
> Now we are coming back to cc907506 ("send-email: don't use
> Mail::Address, even if available", 2017-08-23).  It argues
>
>     * Having this optional Mail::Address makes it tempting to anwser "please
>       install Mail::Address" to users instead of fixing our own code. We've
>       reached the stage where bugs in our parser should be fixed, not worked
>       around.
>
> but if it costs us maintaining our substitute that much, it seems to
> me that depending on Mail::Address is not just tempting but may be a
> sensible way forward.
>
> Was there any reason why Mail::Address was _inadequate_?  I know we
> had trouble with random garbage that are *not* addresses people put
> on the in-body CC: trailer in the past, but I do not recall if they
> are something Mail::Address would give worse result and we need our
> workaround (hence our own substitute), or Mail::Address would handle
> them just fine.

Ping?

So have we come to a consensus about the best solution here?

I'm perfectly happy to send a reversion patch because to be honest
hacking on a bunch of perl to handle special mail cases is not my idea
of fun spare time hacking ;-)

I guess the full solution is to make Mail::Address a hard dependency?

--
Alex Bennée

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Thomas Adam]

On Mon, Dec 11, 2017 at 05:13:53PM +0000, Alex Bennée wrote:
> So have we come to a consensus about the best solution here?
> 
> I'm perfectly happy to send a reversion patch because to be honest
> hacking on a bunch of perl to handle special mail cases is not my idea
> of fun spare time hacking ;-)
> 
> I guess the full solution is to make Mail::Address a hard dependency?

This is what I was suggesting, and then as a follow-up, addressing the point
that there's a bunch of require() hacks to also get around needing
hard-dependencies.

-- Thomas Adam

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Ævar Arnfjörð Bjarmason]

On Mon, Dec 11, 2017 at 6:26 PM, Thomas Adam <thomas@xteddy.org> wrote:
> On Mon, Dec 11, 2017 at 05:13:53PM +0000, Alex Bennée wrote:
>> So have we come to a consensus about the best solution here?
>>
>> I'm perfectly happy to send a reversion patch because to be honest
>> hacking on a bunch of perl to handle special mail cases is not my idea
>> of fun spare time hacking ;-)
>>
>> I guess the full solution is to make Mail::Address a hard dependency?
>
> This is what I was suggesting, and then as a follow-up, addressing the point
> that there's a bunch of require() hacks to also get around needing
> hard-dependencies.

I don't know what the right move is here, but just saying that this
could also be built on top of my "Git::Error" wrapper which I added in
"Makefile: replace perl/Makefile.PL with simple make rules" which is
currently cooking.

I.e. we'd just ship a copy of Email::Valid and Mail::Address in
perl/Git/FromCPAN/, use a wrapper to load them, and then we wouldn't
need to if/else this at the code level, just always use the module,
and it would work even on core perl.

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Thomas Adam]

Hi,

On Mon, Dec 11, 2017 at 08:46:46PM +0100, Ævar Arnfjörð Bjarmason wrote:
> I.e. we'd just ship a copy of Email::Valid and Mail::Address in
> perl/Git/FromCPAN/, use a wrapper to load them, and then we wouldn't
> need to if/else this at the code level, just always use the module,
> and it would work even on core perl.

I disagree with the premise of this, Ævar.  As soon as you go down this route,
it increases maintenance to ensure we keep up to date with what's on CPAN for
a tiny edge-case which I don't believe exists.

You may as well just use App::FatPacker.

We're talking about package maintenance here -- and as I said before, there's
plenty of it around.  For those distributions which ship Git (and hence also
package git-send-email), the dependencies are already there, too.  I just
cannot see this being a problem in relying on non-core perl modules.  Every
perl program does this, and they don't go down this route of having copies of
various CPAN modules just in case.  So why should we?  We're not a special
snowflake.

-- Thomas Adam

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 12 2017, Thomas Adam jotted:

> Hi,
>
> On Mon, Dec 11, 2017 at 08:46:46PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> I.e. we'd just ship a copy of Email::Valid and Mail::Address in
>> perl/Git/FromCPAN/, use a wrapper to load them, and then we wouldn't
>> need to if/else this at the code level, just always use the module,
>> and it would work even on core perl.
>
> I disagree with the premise of this, Ævar.  As soon as you go down this route,
> it increases maintenance to ensure we keep up to date with what's on CPAN for
> a tiny edge-case which I don't believe exists.
>
> You may as well just use App::FatPacker.
>
> We're talking about package maintenance here -- and as I said before, there's
> plenty of it around.  For those distributions which ship Git (and hence also
> package git-send-email), the dependencies are already there, too.  I just
> cannot see this being a problem in relying on non-core perl modules.  Every
> perl program does this, and they don't go down this route of having copies of
> various CPAN modules just in case.  So why should we?  We're not a special
> snowflake.

Something like FatPacker wouldn't make sense in this case, we're not
packing stuff into an archive, but just dropping them during 'make
install', but yes, it's the same idea of shipping our dependencies with
us.

I wouldn't argue for doing this from first principles, in general I
think we're way too conservative about adding dependencies to git.git,
but the general consensus on-list is to do that carefully, that's why we
have all this stuff in contrib/, and why we're depending on perl core
only.

Users or packagers of git don't care what's normal for perl programs, to
them the fact that git-send-email is written in perl is an
implementation detail.

The maintenance burden of just shipping some CPAN module as a fallback
is trivial, for example we've shipped Error.pm since 2006-ish, and until
I sent a patch this month nobody had touched it since 2013.

It's certainly much easier than maintaining a bunch of if/else code
ourselves, or maintaining our own stuff purely because we don't want to
force people to package perl dependencies for git.

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Alex Bennée]

Thomas Adam <thomas@xteddy.org> writes:

> Hi,
>
> On Mon, Dec 11, 2017 at 08:46:46PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> I.e. we'd just ship a copy of Email::Valid and Mail::Address in
>> perl/Git/FromCPAN/, use a wrapper to load them, and then we wouldn't
>> need to if/else this at the code level, just always use the module,
>> and it would work even on core perl.
>
> I disagree with the premise of this, Ævar.  As soon as you go down this route,
> it increases maintenance to ensure we keep up to date with what's on CPAN for
> a tiny edge-case which I don't believe exists.
>
> You may as well just use App::FatPacker.
>
> We're talking about package maintenance here -- and as I said before, there's
> plenty of it around.  For those distributions which ship Git (and hence also
> package git-send-email), the dependencies are already there, too.  I just
> cannot see this being a problem in relying on non-core perl modules.  Every
> perl program does this, and they don't go down this route of having copies of
> various CPAN modules just in case.  So why should we?  We're not a special
> snowflake.

I less bothered my the potentially shipping a git specific copy than
ensuring the packagers pick up the dependency when they do their builds.
Do we already have a mechanism for testing for non-core perl modules
during the "configure" phase of git?

--
Alex Bennée

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 12 2017, Alex Bennée jotted:

> Thomas Adam <thomas@xteddy.org> writes:
>
>> Hi,
>>
>> On Mon, Dec 11, 2017 at 08:46:46PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>> I.e. we'd just ship a copy of Email::Valid and Mail::Address in
>>> perl/Git/FromCPAN/, use a wrapper to load them, and then we wouldn't
>>> need to if/else this at the code level, just always use the module,
>>> and it would work even on core perl.
>>
>> I disagree with the premise of this, Ævar.  As soon as you go down this route,
>> it increases maintenance to ensure we keep up to date with what's on CPAN for
>> a tiny edge-case which I don't believe exists.
>>
>> You may as well just use App::FatPacker.
>>
>> We're talking about package maintenance here -- and as I said before, there's
>> plenty of it around.  For those distributions which ship Git (and hence also
>> package git-send-email), the dependencies are already there, too.  I just
>> cannot see this being a problem in relying on non-core perl modules.  Every
>> perl program does this, and they don't go down this route of having copies of
>> various CPAN modules just in case.  So why should we?  We're not a special
>> snowflake.
>
> I less bothered my the potentially shipping a git specific copy than
> ensuring the packagers pick up the dependency when they do their builds.
> Do we already have a mechanism for testing for non-core perl modules
> during the "configure" phase of git?

Current git.git master does two things:

 * For Error.pm we test at build time. See `git grep Error --
   'perl/Make*'`. If you don't have Error.pm when you build we'll ship
   an old copy of it, and use that forever even if it's installed from
   CPAN afterwards.

 * For Mail::Address, Net::Domain etc. we don't ship the CPAN module,
   but some fallback code. We test at runtime, see `git grep
   eval.*require`. If you install the package from CPAN we'll start
   using it at your next invocation.

My "Makefile: replace perl/Makefile.PL with simple make rules" currently
cooking in pu changes that so that:

 * We always at runtime test for the system CPAN module.

 * In the case of Error.pm we happen to ship a fallback, in the case of
   Mail::Address etc. we don't and have fallback code, but we could also
   just ship a copy and remove the fallback code.

This makes more sense, we always "dynamically link" as it were, we'll
just change the target to (a presumably newer) system module in the case
of Error.pm if it's found on the system, otherwise use our fallback.

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> My "Makefile: replace perl/Makefile.PL with simple make rules" currently
> cooking in pu changes that so that:
>
>  * We always at runtime test for the system CPAN module.
>
>  * In the case of Error.pm we happen to ship a fallback, in the case of
>    Mail::Address etc. we don't and have fallback code, but we could also
>    just ship a copy and remove the fallback code.
>
> This makes more sense, we always "dynamically link" as it were, we'll
> just change the target to (a presumably newer) system module in the case
> of Error.pm if it's found on the system, otherwise use our fallback.

"When to fallback" aside, I think the above makes sense for the
send-email simply because we would be replacing "our own" fallback
we may need to maintain forever with something with an upstream that
we do not have to worry too much about.

A tangent; I thought I heard that use of Error.pm is strongly
discouraged several years ago---am I mistaken, or if I am not,
perhaps we should start looking into updating the users?

Thanks.

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 12 2017, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> My "Makefile: replace perl/Makefile.PL with simple make rules" currently
>> cooking in pu changes that so that:
>>
>>  * We always at runtime test for the system CPAN module.
>>
>>  * In the case of Error.pm we happen to ship a fallback, in the case of
>>    Mail::Address etc. we don't and have fallback code, but we could also
>>    just ship a copy and remove the fallback code.
>>
>> This makes more sense, we always "dynamically link" as it were, we'll
>> just change the target to (a presumably newer) system module in the case
>> of Error.pm if it's found on the system, otherwise use our fallback.
>
> "When to fallback" aside, I think the above makes sense for the
> send-email simply because we would be replacing "our own" fallback
> we may need to maintain forever with something with an upstream that
> we do not have to worry too much about.

I'll see about submitting something that replaces the fallback with just
using the CPAN modules + bundling them once the Makefile patch has
cooked to master.

> A tangent; I thought I heard that use of Error.pm is strongly
> discouraged several years ago---am I mistaken, or if I am not,
> perhaps we should start looking into updating the users?

I'm not a fan of it, 41c01693ac ("git-svn: handle merge-base failures",
2010-01-06) shows how you can do that rather simply with just perl's
built-in exceptions.

My TODO list of "perl stuff in git" is now:

 - Get my Makefile.PL thing through
 - Make sure Dan Jacques's relocatable stuff is OK wrt perl on top of that
 - Upgrade the required version from 5.8 to 5.10
 - Update Error.pm itself, our copy is ancient
 - Add more stuff to Git::FromCPAN + remove fallbacks

I could add "rip out Error.pm" to that, it looks rather easy, however
given previous discussion about me needing to build a manpage from
Git.pm I understand that Git.pm is used by code outside of Git itself.

Ripping out Error.pm for our few internal callers is one thing, trying
to maintain bugwards compatibility with how it throws exceptions for
users expecting Error.pm objects is another. I think at that point it's
easier to just stay with Error.pm.

Probably easier to stay with it either way, don't poke sleeping dragons
and all that, it's working code, even if we wouldn't write it like that
today the churn probably isn't worth it.

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Ripping out Error.pm for our few internal callers is one thing, trying
> to maintain bugwards compatibility with how it throws exceptions for
> users expecting Error.pm objects is another. I think at that point it's
> easier to just stay with Error.pm.
>
> Probably easier to stay with it either way, don't poke sleeping dragons
> and all that, it's working code, even if we wouldn't write it like that
> today the churn probably isn't worth it.

OK, I'm inclined to defer to your judgment.

THanks.

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Was there any reason why Mail::Address was _inadequate_?

I think the main reason was that Mail::Address is not a standard perl
module, and not relying on it avoided one external dependency. AFAIK, we
don't really have a good way to deal with Perl dependencies, so having a
strong requirement on Mail::Address will probably end up in a runtime
error for users who compile Git from source (people using a packaged
version have their package manager to deal with this).

> I know we had trouble with random garbage that are *not* addresses
> people put on the in-body CC: trailer in the past, but I do not recall
> if they are something Mail::Address would give worse result and we
> need our workaround (hence our own substitute), or Mail::Address would
> handle them just fine.

For a long time, we used Mail::Address if it was available and I don't
think we had issues with it.

My point in cc907506 ("send-email: don't use Mail::Address, even if
available", 2017-08-23) was not that Mail::Address was bad, but that
changing our behavior depending on whether it was there or not was
really bad. For example, the issue dealt with in this thread probably
always existed, but it was present only for *some* users.

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

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Alex Bennée]

Matthieu Moy <Matthieu.Moy@univ-lyon1.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Was there any reason why Mail::Address was _inadequate_?
>
> I think the main reason was that Mail::Address is not a standard perl
> module, and not relying on it avoided one external dependency. AFAIK, we
> don't really have a good way to deal with Perl dependencies, so having a
> strong requirement on Mail::Address will probably end up in a runtime
> error for users who compile Git from source (people using a packaged
> version have their package manager to deal with this).
>
>> I know we had trouble with random garbage that are *not* addresses
>> people put on the in-body CC: trailer in the past, but I do not recall
>> if they are something Mail::Address would give worse result and we
>> need our workaround (hence our own substitute), or Mail::Address would
>> handle them just fine.
>
> For a long time, we used Mail::Address if it was available and I don't
> think we had issues with it.
>
> My point in cc907506 ("send-email: don't use Mail::Address, even if
> available", 2017-08-23) was not that Mail::Address was bad, but that
> changing our behavior depending on whether it was there or not was
> really bad. For example, the issue dealt with in this thread probably
> always existed, but it was present only for *some* users.

So I just did a little digging on my systems to illustrate the point. My
work machine is Ubuntu, so has a packaged git via PPA. It depends on
libmailtools-perl which includes the perl module and:

  apt-cache rdepends  libmailtools-perl | wc -l
  45

So for binary packaged systems it's not a big thing - libmailtools-perl
seems to be quite widely relied on.

On my system at home, running Gentoo, while requiring "perl" doesn't
explicitly pull in the Mail::Address dependency. As a result the git
send-email stanza I was running at work manages to corrupt the
addresses. If I manually install dev-perl/MailTools of course it
silently starts working again.

Good job I never usually send patches from my home machine ;-)

My hacky guess about GIT's perl use calls is:

   find . -iname "*.perl" -or -iname "*.pm" -or -iname "*.pl" | xargs grep -h  "use .*::" | sort | uniq | wc -l
   88

So that is about 88 perl modules used in the code base. How many of them
are not part of the core perl distribution?

Should the solution be to just make Mail::Address a hard dependency and
not have the fallback?

--
Alex Bennée

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Thomas Adam]

On Wed, Nov 22, 2017 at 09:05:41AM +0000, Alex Bennée wrote:
> My hacky guess about GIT's perl use calls is:
> 
>    find . -iname "*.perl" -or -iname "*.pm" -or -iname "*.pl" | xargs grep -h  "use .*::" | sort | uniq | wc -l
>    88

So let us concentrate just on git-send-email.perl for now.  In the
Module::Extract::Use module (which happens to be what corelist uses), there's
an example script called 'extract_modules' which will statically analyse a
perl file and tell you the following information:

% perl extract_modules ./git-send-email.perl
Modules required by ./git-send-email.perl:
- Authen::SASL
- Cwd (first released with Perl 5)
- Email::Valid
- Error
- File::Spec::Functions (first released with Perl 5.00504)
- File::Temp (first released with Perl 5.006001)
- Getopt::Long (first released with Perl 5)
- Git
- Git::I18N
- IO::Socket::SSL
- MIME::Base64 (first released with Perl 5.007003)
- MIME::QuotedPrint (first released with Perl 5.007003)
- Net::Domain (first released with Perl 5.007003)
- Net::SMTP (first released with Perl 5.007003)
- Net::SMTP::SSL
- POSIX (first released with Perl 5)
- Sys::Hostname (first released with Perl 5)
- Term::ANSIColor (first released with Perl 5.006)
- Term::ReadLine (first released with Perl 5.002)
- Text::ParseWords (first released with Perl 5)
- strict (first released with Perl 5)
- warnings (first released with Perl 5.006)

Therefore, we have the following modules which are not standard:

- Email::Valid
- Error
- Git
- Git::I18N
- IO::Socket::SSL
- NET::SMTP::SSL

Looking at the code for git-send-email.perl, it seems most of those are
eval()d at the point they're needed, which seems in many cases to be fallback
responses to something we've written, or a means of ensuring we don't need to
explicitly handle the case of it not being present at run-time.

> Should the solution be to just make Mail::Address a hard dependency and
> not have the fallback?

This seems like a slight on ensuring a running script which may or may not
have additional functionality depending on which modules are installed.  Given
the pretty good state of packaging across those platforms which Git runs on, I
would argue we're now in a much better position to explicitly check for
non-core modules at BEGIN{} time, and moan loudly if they're not installed.

-- Thomas Adam

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@univ-lyon1.fr> writes:

> My point in cc907506 ("send-email: don't use Mail::Address, even if
> available", 2017-08-23) was not that Mail::Address was bad, but that
> changing our behavior depending on whether it was there or not was
> really bad. For example, the issue dealt with in this thread probably
> always existed, but it was present only for *some* users.

Yes, I understand and agree with you that having a single
implementation would make things simpler for both users and bug
wranglers than two sperate implementaions with two different sets of
glitches.

I am just wondering if we picked the right one back then, given this
thread; that's all.