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

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

[Fredrik Medley]

On Windows, when Git is installed under "C:\Program Files\Git", SHELL_PATH
will include a space. Fix "git rebase --interactive --exec" so that it
works with spaces in SHELL_PATH.

Signed-off-by: Fredrik Medley <fredrik.medley@gmail.com>
---
 git-rebase--interactive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 30edb17..b938a6d 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -610,7 +610,7 @@ do_next () {
 		read -r command rest < "$todo"
 		mark_action_done
 		printf 'Executing: %s\n' "$rest"
-		${SHELL:-@SHELL_PATH@} -c "$rest" # Actual execution
+		"${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution
 		status=$?
 		# Run in subshell because require_clean_work_tree can die.
 		dirty=f
-- 
2.6.2.468.gf34be46.dirty

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

[Jeff King]

On Fri, Nov 13, 2015 at 07:03:19AM +0100, Fredrik Medley wrote:

> On Windows, when Git is installed under "C:\Program Files\Git", SHELL_PATH
> will include a space. Fix "git rebase --interactive --exec" so that it
> works with spaces in SHELL_PATH.
> 
> Signed-off-by: Fredrik Medley <fredrik.medley@gmail.com>
> ---
>  git-rebase--interactive.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 30edb17..b938a6d 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -610,7 +610,7 @@ do_next () {
>  		read -r command rest < "$todo"
>  		mark_action_done
>  		printf 'Executing: %s\n' "$rest"
> -		${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

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

[Fredrik Medley]

2015-11-13 7:25 GMT+01:00 Jeff King <peff@peff.net>:
> On Fri, Nov 13, 2015 at 07:03:19AM +0100, Fredrik Medley wrote:
>
>> On Windows, when Git is installed under "C:\Program Files\Git", SHELL_PATH
>> will include a space. Fix "git rebase --interactive --exec" so that it
>> works with spaces in SHELL_PATH.
>>
>> Signed-off-by: Fredrik Medley <fredrik.medley@gmail.com>
>> ---
>>  git-rebase--interactive.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index 30edb17..b938a6d 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -610,7 +610,7 @@ do_next () {
>>               read -r command rest < "$todo"
>>               mark_action_done
>>               printf 'Executing: %s\n' "$rest"
>> -             ${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.

I assume the Windows compilation does not fail in building this. I've
never tried building git for Windows, though.

/Fredrik

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

[Jeff King]

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.

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 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.

-Peff

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

[Fredrik Medley]

2015-11-13 23:27 GMT+01:00 Jeff King <peff@peff.net>:
> 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

>
> 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.

>
> 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

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

[Jeff King]

On Fri, Nov 13, 2015 at 11:47:40PM +0100, Fredrik Medley wrote:

> > 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

It looks like bash on your system may simply be filling in its absolute
path into the $SHELL variable. This seems like an interesting bash-ism:

  $ unset SHELL
  $ dash -c 'echo $SHELL'

  $ bash -c 'echo $SHELL'
  /bin/bash

If $SHELL is already set and exported, it will not override it, but in
your case $SHELL is probably local to each individual bash invocation.

So that at least kind-of makes sense. It's possible somebody could be
doing something clever with $SHELL, but I kind of doubt it. If they want
to do something clever, it is much easier to put it directly on the exec
line, and have the normal $SHELL run their clever thing.

-Peff

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

[Johannes Schindelin]

Hi Peff,

On Fri, 13 Nov 2015, Jeff King wrote:

> It's possible somebody could be doing something clever with $SHELL, but
> I kind of doubt it. If they want to do something clever, it is much
> easier to put it directly on the exec line, and have the normal $SHELL
> run their clever thing.

To clarify the Git for Windows scenario: SHELL_PATH is indeed set to
`/bin/sh`, but reportedly it is converted into a full Windows path when we
leave the POSIX emulation layer, i.e. when `git.exe` is called (which has
*no* idea about POSIX paths, or at least next to none).

The reason is, of course, that regular Windows programs would not know
what to do with the path /bin/sh.

The problem arises when we re-enter the POSIX realm, i.e. when we run a
script (such as `git-rebase`): the path is not converted back!

There are already tickets on Git for Windows' bug tracker where the case
is made that vim has serious problems with that space in the environment
variable, so the long-term fix is really to teach the POSIX emulation
layer (read: MSys2) to convert SHELL_PATH back into a POSIX path when
appropriate.

This is on my plate, but there are quite a couple more urgent tasks I need
to take care of, so please do not hold your breath (unless you are into
that kind of thing).

Ciao,
Dscho

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

[Jeff King]

On Mon, Nov 16, 2015 at 05:01:03PM +0100, Johannes Schindelin wrote:

> To clarify the Git for Windows scenario: SHELL_PATH is indeed set to
> `/bin/sh`, but reportedly it is converted into a full Windows path when we
> leave the POSIX emulation layer, i.e. when `git.exe` is called (which has
> *no* idea about POSIX paths, or at least next to none).
> 
> The reason is, of course, that regular Windows programs would not know
> what to do with the path /bin/sh.
> 
> The problem arises when we re-enter the POSIX realm, i.e. when we run a
> script (such as `git-rebase`): the path is not converted back!

Ah, thanks for this explanation. The whole thing seems much more clear
to me now.

Even if later versions give us back the POSIX name, I think Fredrik's
patch is a sane solution (and arguably how it should have been written
in the first place, even leaving aside this issue).

-Peff

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

[Matthieu Moy]

Fredrik Medley <fredrik.medley@gmail.com> writes:

> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -610,7 +610,7 @@ do_next () {
>  		read -r command rest < "$todo"
>  		mark_action_done
>  		printf 'Executing: %s\n' "$rest"
> -		${SHELL:-@SHELL_PATH@} -c "$rest" # Actual execution
> +		"${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution

Looks good to me. Don't know why I didn't add these double quotes when I
introduced this line. Thanks for fixing it.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/