Eric Sunshine <sunsh...@sunshineco.com> writes:
> On Fri, Jan 5, 2018 at 1:36 PM, Matthieu Moy <g...@matthieu-moy.fr> wrote:
>> From: Alex Bennée <alex.ben...@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 <g...@matthieu-moy.fr>
>> Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
>> Signed-off-by: Matthieu Moy <g...@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 <o...@example.com> (supporter:THIS (FOO/bar))'
>> + echo 'Two Person <t...@example.com> (maintainer:THIS THING)'
>> + echo 'Third List <th...@example.com> (moderated list:THIS THING
>> (FOO/bar))'
>> + echo '<f...@example.com> (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/
