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

[Eric Sunshine <sunshine@sunshineco.com>]

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< ---