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

[Matthieu Moy <git@matthieu-moy.fr>]

Eric Sunshine <sunshine@sunshineco.com> writes:

> 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

Indeed, removed.

> 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)/").

Added a note in the commit message.

> An alternative would be to insert a cleanup/modernization
> patch before this one which changes all the "$(pwd)/" to "./",

For --smtp-server, doing so results in a failing tests. I didn't
investigate on why.

> 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)/").

Also, the existing code is written like this: --cc-cmd is always
relative, --stmp-server is always absolute, including when they're used
in the same command:

test_suppress_self () {
[...]
	git send-email --from="$1 <$2>" \
		--to=nobody@example.com \
		--cc-cmd=./cccmd-sed \
		--suppress-cc=self \
		--smtp-server="$(pwd)/fake.sendmail" \

Thanks for your careful review,

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