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

Eric Sunshine [off-list ref] writes:

On Fri, Jan 5, 2018 at 1:36 PM, Matthieu Moy [off-list ref] wrote:

quoted

From: Alex Bennée <redacted>

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 [off-list ref]
Signed-off-by: Alex Bennée <redacted>
Signed-off-by: Matthieu Moy <redacted>
---

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/