Re: [PATCH] git-send-email: fix get_maintainer.pl regression

From: Philip Oakley <philipoakley@iee.org>
Date: Nov 21, 2017, 00:07 UTC
Message-ID: <D810DA8B202742B38343EBB44BD54A1D@PhilipOakley>
In-Reply-To: <CAPig+cSbRrGnyDkunMFiFXbWRMAsGyuAL-0FpP1QTtjSUSY2Hg@mail.gmail.com>

From: "Eric Sunshine" <sunshine@sunshineco.com>
On Sat, Nov 18, 2017 at 9:54 PM, Eric Sunshine <sunshine@sunshineco.com>
wrote:

Show 38 quoted lines

> On Thu, Nov 16, 2017 at 10:48 AM, Alex Bennée <alex.bennee@linaro.org>
> wrote:
>> +test_expect_success $PREREQ 'cc trailer with get_maintainer output' '
>> +       [...]
>> +       git send-email -1 --to=recipient@example.com \
>> +               --cc-cmd="$(pwd)/expected-cc-script.sh" \
>> +       [...]
>> +'
>> OK I'm afraid I don't fully understand the test harness as this breaks a
>> bunch of other tests. If anyone can offer some pointers on how to fix
>> I'd be grateful.
>
> There are several problems:
> [...]
> * The directory in which the expected-cc-script.sh is created contains
> a space; this is intentional to catch bugs in tests and Git itself. In
> this case, your test is exposing what might be considered a bug in
> git-send-email itself, in which it invokes the --cc-cmd as "/path/with
> space/expected-cc-script.sh", which is interpreted as trying to invoke
> program "/path/with" with argument "space/expected-cc-script.sh". One
> > fix (which you could submit as a preparatory patch, making this a
> > 2-patch series) would be this:
> >
> > --- 8< ---
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > @@ -1724,7 +1724,7 @@ sub recipients_cmd {
> > -    open my $fh, "-|", "$cmd \Q$file\E"
> > +   open my $fh, "-|", "\Q$cmd\E \Q$file\E"
> > --- 8< ---
> >
> > However, it's possible that might break existing users who rely on
> > --cc-cmd="myscript --option arg" working. It's not clear which
> > behavior is correct.
>
> The more I think about this, the less I consider this a bug in
> git-send-email. As noted, people might legitimately use a complex
> command (--cc-cmd="myscript--option arg"), so changing git-send-email
> to treat cc-cmd as an atomic string seems like a bad idea.

A while back I proposed some documentation updates https://public-inbox.org/git/1437416790-5792-1-git-send-email-philipoakley@iee.org/ regarding what is (should be) allowed in the cc-cmd etc., and at the time Junio suggested that possible existing uses of the current code would be abuses. I didn't pursue it further, but it may be useful guidance here as to potential real world command lines..

Show 11 quoted lines

>
> Assuming no changes to git-send-email, to get your test working, you
> could try to figure out how to quote the script's path you're
> specifying with --cc-cmd, however, even easier would be to drop $(pwd)
> altogether. That is, instead of:
>
>     --cc-cmd="$(pwd)/expected-cc-script.sh"
>
> just use:
>
>     --cc-cmd=./expected-cc-script.sh