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

["Alex Bennée" <alex.bennee@linaro.org>]

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