On Fri, Jan 5, 2018 at 1:36 PM, Matthieu Moy <[email protected]> wrote: > From: Alex Bennée <[email protected]> > > 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 <[email protected]> > Signed-off-by: Alex Bennée <[email protected]> > Signed-off-by: Matthieu Moy <[email protected]> > --- > 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 <[email protected]> (supporter:THIS (FOO/bar))' > + echo 'Two Person <[email protected]> (maintainer:THIS THING)' > + echo 'Third List <[email protected]> (moderated list:THIS THING > (FOO/bar))' > + echo '<[email protected]> (moderated list:FOR THING)' > + echo '[email protected] (open list:FOR THING (FOO/bar))' > + echo '[email protected] (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 [email protected] \ > + --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
