Re: [PATCH] rebase-i-exec: Allow space in SHELL_PATH

From: Fredrik Medley <fredrik.medley@gmail.com>
Date: Nov 13, 2015, 22:47 UTC
Message-ID: <CABA5-zmmKeeDDhSdan9sTppRVeDNuPiR8WX=P4Umdbq=ELafqw@mail.gmail.com>
In-Reply-To: <20151113222748.GA14830@sigill.intra.peff.net>

2015-11-13 23:27 GMT+01:00 Jeff King <peff@peff.net>:

Show 27 quoted lines

> On Fri, Nov 13, 2015 at 04:25:18PM +0100, Fredrik Medley wrote:
>
>> >> -             ${SHELL:-@SHELL_PATH@} -c "$rest" # Actual execution
>> >> +             "${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution
>> >
>> > I think this is the right thing to do (at least I could not think of a
>> > case that would be harmed by it, and it certainly fixes your case). It
>> > looks like filter-branch would need a similar fix?
>> >
>> > I think this still isn't resilient to weird meta-characters in the
>> > @SHELL_PATH@, but as this is a build-time option, I think it's OK to let
>> > people who do
>> >
>> >   make SHELL_PATH='}"; rm -rf /'
>> >
>> > hang themselves.
>> >
>> > -Peff
>>
>> Okay, that's what @SHELL_PATH@ stands for. I just read the result
>> in the Windows installation that is something like ${SHELL:-/bin/sh}.
>> The shell script processor then replaces /bin/sh with
>> C:\Program Files\...\bin\sh.
>
> Hmm. Now I'm a bit puzzled. It sounds like the installed file does have
> @SHELL_PATH@ set to /bin/sh, which is normal. And presumably the setting
> containing space is coming from the $SHELL environment variable.

I wrote the email on another computer. Checking the system where I reinstalled Git for Windows yesterday shows ${SHELL:-/bin/sh} and SHELL=/usr/bin/bash When running "git rebase --interactive HEAD^ --exec 'echo $SHELL'", I get mingw64/libexec/git-core/git-rebase--interactive: line 613: C:/Program: No such file or directory

Fixing the double quotes, "git rebase --interactive HEAD^ --exec 'echo $SHELL'" shows C:/Program Files/Git/usr/bin/bash

Show 18 quoted lines

>
> My original "I could not think of a case harmed by it" was because
> @SHELL_PATH@ also gets used on the shebang line at the beginning of the
> script. And that does not really handle command names with arguments
> well (you get one argument, and it better not have spaces in it). Of
> course, it _also_ does not handle command names with spaces in them,
> either (and there's no room for quoting there).
>
> So anything exotic pretty much has to be coming in from $SHELL, and my
> mention of filter-branch is not interesting; it just uses @SHELL_PATH@.
>
> So what are people putting in $SHELL? Obviously a shell path with a
> space in it wants the quotes. Do people do more exotic things like:
>
>   SHELL="my-shell --options"
>
> (I think a plausible option might be "--posix" for some shells).  That
> would break with your patch.

I agree, tricky case. Just to accept the fact that it won't work.

Show 6 quoted lines

>
> I dunno. We usually try to err on the side of the status quo, so as to
> avoid breaking existing users. But I find the idea of $SHELL with
> options reasonably unlikely, so I think your patch is the right
> direction. I wish I understood better who was setting $SHELL and why,
> though.

I don't understand where $SHELL is being set neither.

>
> -Peff

/Fredrik