Re: [PATCH v2 3/3] send-email: add test for Linux's get_maintainer.pl

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Jan 5, 2018 at 1:36 PM, Matthieu Moy <git@matthieu-moy.fr> wrote:
> From: Alex Bennée <alex.bennee@linaro.org>
>
> We had a regression that broke Linux's get_maintainer.pl. Using
> Mail::Address to parse email addresses fixed it, but let's protect
> against future regressions.
>
> Patch-edited-by: Matthieu Moy <git@matthieu-moy.fr>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Matthieu Moy <git@matthieu-moy.fr>
> ---
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> @@ -172,6 +172,26 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '
> +test_expect_success $PREREQ 'setup fake get_maintainer.pl script for cc trailer' "
> +       write_script expected-cc-script.sh <<-EOF &&
> +       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
> +       chmod +x expected-cc-script.sh
> +"
> +
> +test_expect_success $PREREQ 'cc trailer with get_maintainer.pl output' '
> +       clean_fake_sendmail &&
> +       git send-email -1 --to=recipient@example.com \
> +               --cc-cmd="./expected-cc-script.sh" \
> +               --smtp-server="$(pwd)/fake.sendmail" &&

Aside from the unnecessary (thus noisy) quotes around the --cc-cmd
value, my one concern is that someone may come along and want to
"normalize" it to --cc-cmd="$(pwd)/expected-cc-script.sh" for
consistency with the following --smtp-server line. This worry is
compounded by the commit message not explaining why these two lines
differ (one using "./" and one using "$(pwd)/"). So, at minimum, it
might be a good idea to explain why "./" is used for this one distinct
case, compared with all the others which use "$(pwd)/". An alternative
would be to insert a cleanup/modernization patch before this one which
changes all the "$(pwd)/" to "./", although you'd still want to
explain why that's being done (to wit: because --cc-cmd behavior with
spaces is not well defined). Or, perhaps this isn't an issue and my
worry is not justified (after all, the test will break if someone
changes the "./" to "$(pwd)/"). At any rate, such a concern probably
shouldn't hold up this patch.

> +       test_cmp expected-cc commandline1
> +'
> +
>  test_expect_success $PREREQ 'setup expect' "
>  cat >expected-show-all-headers <<\EOF
>  0001-Second.patch