Re: [PATCH v2 3/3] send-email: add test for Linux's get_maintainer.pl
Eric Sunshinewrites: > On Fri, Jan 5, 2018 at 1:36 PM, Matthieu Moy wrote: >> From: Alex Bennée >> >> 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 >> Signed-off-by: Alex Bennée >> Signed-off-by: Matthieu Moy >> --- >> 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 (supporter:THIS (FOO/bar))' >> + echo 'Two Person (maintainer:THIS THING)' >> + echo 'Third List (moderated list:THIS THING >> (FOO/bar))' >> + echo ' (moderated list:FOR THING)' >> + echo 'f...@example.com (open list:FOR THING (FOO/bar))' >> + echo 's...@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=recipi...@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=nob...@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/
Re: [PATCH v2 3/3] send-email: add test for Linux's get_maintainer.pl
On Fri, Jan 5, 2018 at 1:36 PM, Matthieu Moywrote: > From: Alex Bennée > > 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 > Signed-off-by: Alex Bennée > Signed-off-by: Matthieu Moy > --- > 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 (supporter:THIS (FOO/bar))' > + echo 'Two Person (maintainer:THIS THING)' > + echo 'Third List (moderated list:THIS THING > (FOO/bar))' > + echo ' (moderated list:FOR THING)' > + echo 'f...@example.com (open list:FOR THING (FOO/bar))' > + echo 's...@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=recipi...@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
[PATCH v2 3/3] send-email: add test for Linux's get_maintainer.pl
From: Alex BennéeWe 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 Signed-off-by: Alex Bennée Signed-off-by: Matthieu Moy --- Change since v1: fixed proposed by Eric Sunshine and pointed out by Alex Bennée. Eric pointed out that using --cc-cmd=$(pwd)/expected-cc-script.sh did not work because $(pwd) had spaces in it, but I already turned it into ./expected-cc-script.sh. t/t9001-send-email.sh | 20 1 file changed, 20 insertions(+) diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 4d261c2..d13d8c3 100755 --- 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_cmp expected-cc commandline1 ' +test_expect_success $PREREQ 'setup fake get_maintainer.pl script for cc trailer' " + write_script expected-cc-script.sh <<-EOF && + echo 'One Person (supporter:THIS (FOO/bar))' + echo 'Two Person (maintainer:THIS THING)' + echo 'Third List (moderated list:THIS THING (FOO/bar))' + echo ' (moderated list:FOR THING)' + echo 'f...@example.com (open list:FOR THING (FOO/bar))' + echo 's...@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=recipi...@example.com \ + --cc-cmd="./expected-cc-script.sh" \ + --smtp-server="$(pwd)/fake.sendmail" && + test_cmp expected-cc commandline1 +' + test_expect_success $PREREQ 'setup expect' " cat >expected-show-all-headers <<\EOF 0001-Second.patch -- 2.7.4