{"thread":{"id":"40789","subject":"[PATCH] rebase-i-exec: Allow space in SHELL_PATH","startedAt":"2015-11-13T06:03:19Z","lastAt":"2015-11-16T16:34:05Z","messageCount":9,"participants":["Fredrik Medley","Jeff King","Matthieu Moy","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"273267","messageId":"1447394599-16077-1-git-send-email-fredrik.medley@gmail.com","threadId":"40789","inReplyTo":null,"subject":"[PATCH] rebase-i-exec: Allow space in SHELL_PATH","fromName":"Fredrik Medley","fromEmail":"fredrik.medley@gmail.com","sentAt":"2015-11-13T06:03:19Z","receivedAt":"2015-11-13T06:03:19Z","isPatch":true,"sender":{"key":"fredrik.medley@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5401799?v=4"},"body":"On Windows, when Git is installed under \"C:\\Program Files\\Git\", SHELL_PATH\nwill include a space. Fix \"git rebase --interactive --exec\" so that it\nworks with spaces in SHELL_PATH.\n\nSigned-off-by: Fredrik Medley <fredrik.medley@gmail.com>\n---\n git-rebase--interactive.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 30edb17..b938a6d 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -610,7 +610,7 @@ do_next () {\n \t\tread -r command rest < \"$todo\"\n \t\tmark_action_done\n \t\tprintf 'Executing: %s\\n' \"$rest\"\n-\t\t${SHELL:-@SHELL_PATH@} -c \"$rest\" # Actual execution\n+\t\t\"${SHELL:-@SHELL_PATH@}\" -c \"$rest\" # Actual execution\n \t\tstatus=$?\n \t\t# Run in subshell because require_clean_work_tree can die.\n \t\tdirty=f\n-- \n2.6.2.468.gf34be46.dirty\n"},{"id":"273270","messageId":"20151113062534.GE32157@sigill.intra.peff.net","threadId":"40789","inReplyTo":"1447394599-16077-1-git-send-email-fredrik.medley@gmail.com","subject":"Re: [PATCH] rebase-i-exec: Allow space in SHELL_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-13T06:25:35Z","receivedAt":"2015-11-13T06:25:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 13, 2015 at 07:03:19AM +0100, Fredrik Medley wrote:\n\n> On Windows, when Git is installed under \"C:\\Program Files\\Git\", SHELL_PATH\n> will include a space. Fix \"git rebase --interactive --exec\" so that it\n> works with spaces in SHELL_PATH.\n> \n> Signed-off-by: Fredrik Medley <fredrik.medley@gmail.com>\n> ---\n>  git-rebase--interactive.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index 30edb17..b938a6d 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -610,7 +610,7 @@ do_next () {\n>  \t\tread -r command rest < \"$todo\"\n>  \t\tmark_action_done\n>  \t\tprintf 'Executing: %s\\n' \"$rest\"\n> -\t\t${SHELL:-@SHELL_PATH@} -c \"$rest\" # Actual execution\n> +\t\t\"${SHELL:-@SHELL_PATH@}\" -c \"$rest\" # Actual execution\n\nI think this is the right thing to do (at least I could not think of a\ncase that would be harmed by it, and it certainly fixes your case). It\nlooks like filter-branch would need a similar fix?\n\nI think this still isn't resilient to weird meta-characters in the\n@SHELL_PATH@, but as this is a build-time option, I think it's OK to let\npeople who do\n\n  make SHELL_PATH='}\"; rm -rf /'\n\nhang themselves.\n\n-Peff\n"},{"id":"273279","messageId":"CABA5-zk+RVBxfmuLyK8CcCFUpMXEzbHRKeWWV2SKsJqjnG-nfA@mail.gmail.com","threadId":"40789","inReplyTo":"20151113062534.GE32157@sigill.intra.peff.net","subject":"Re: [PATCH] rebase-i-exec: Allow space in SHELL_PATH","fromName":"Fredrik Medley","fromEmail":"fredrik.medley@gmail.com","sentAt":"2015-11-13T15:25:18Z","receivedAt":"2015-11-13T15:25:18Z","isPatch":true,"sender":{"key":"fredrik.medley@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5401799?v=4"},"body":"2015-11-13 7:25 GMT+01:00 Jeff King <peff@peff.net>:\n> On Fri, Nov 13, 2015 at 07:03:19AM +0100, Fredrik Medley wrote:\n>\n>> On Windows, when Git is installed under \"C:\\Program Files\\Git\", SHELL_PATH\n>> will include a space. Fix \"git rebase --interactive --exec\" so that it\n>> works with spaces in SHELL_PATH.\n>>\n>> Signed-off-by: Fredrik Medley <fredrik.medley@gmail.com>\n>> ---\n>>  git-rebase--interactive.sh | 2 +-\n>>  1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n>> index 30edb17..b938a6d 100644\n>> --- a/git-rebase--interactive.sh\n>> +++ b/git-rebase--interactive.sh\n>> @@ -610,7 +610,7 @@ do_next () {\n>>               read -r command rest < \"$todo\"\n>>               mark_action_done\n>>               printf 'Executing: %s\\n' \"$rest\"\n>> -             ${SHELL:-@SHELL_PATH@} -c \"$rest\" # Actual execution\n>> +             \"${SHELL:-@SHELL_PATH@}\" -c \"$rest\" # Actual execution\n>\n> I think this is the right thing to do (at least I could not think of a\n> case that would be harmed by it, and it certainly fixes your case). It\n> looks like filter-branch would need a similar fix?\n>\n> I think this still isn't resilient to weird meta-characters in the\n> @SHELL_PATH@, but as this is a build-time option, I think it's OK to let\n> people who do\n>\n>   make SHELL_PATH='}\"; rm -rf /'\n>\n> hang themselves.\n>\n> -Peff\n\nOkay, that's what @SHELL_PATH@ stands for. I just read the result\nin the Windows installation that is something like ${SHELL:-/bin/sh}.\nThe shell script processor then replaces /bin/sh with\nC:\\Program Files\\...\\bin\\sh.\n\nI assume the Windows compilation does not fail in building this. I've\nnever tried building git for Windows, though.\n\n/Fredrik\n"},{"id":"273284","messageId":"vpqio55j0zd.fsf@anie.imag.fr","threadId":"40789","inReplyTo":"1447394599-16077-1-git-send-email-fredrik.medley@gmail.com","subject":"Re: [PATCH] rebase-i-exec: Allow space in SHELL_PATH","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-11-13T16:52:22Z","receivedAt":"2015-11-13T16:52:22Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Fredrik Medley <fredrik.medley@gmail.com> writes:\n\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -610,7 +610,7 @@ do_next () {\n>  \t\tread -r command rest < \"$todo\"\n>  \t\tmark_action_done\n>  \t\tprintf 'Executing: %s\\n' \"$rest\"\n> -\t\t${SHELL:-@SHELL_PATH@} -c \"$rest\" # Actual execution\n> +\t\t\"${SHELL:-@SHELL_PATH@}\" -c \"$rest\" # Actual execution\n\nLooks good to me. Don't know why I didn't add these double quotes when I\nintroduced this line. Thanks for fixing it.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"273293","messageId":"20151113222748.GA14830@sigill.intra.peff.net","threadId":"40789","inReplyTo":"CABA5-zk+RVBxfmuLyK8CcCFUpMXEzbHRKeWWV2SKsJqjnG-nfA@mail.gmail.com","subject":"Re: [PATCH] rebase-i-exec: Allow space in SHELL_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-13T22:27:49Z","receivedAt":"2015-11-13T22:27:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 13, 2015 at 04:25:18PM +0100, Fredrik Medley wrote:\n\n> >> -             ${SHELL:-@SHELL_PATH@} -c \"$rest\" # Actual execution\n> >> +             \"${SHELL:-@SHELL_PATH@}\" -c \"$rest\" # Actual execution\n> >\n> > I think this is the right thing to do (at least I could not think of a\n> > case that would be harmed by it, and it certainly fixes your case). It\n> > looks like filter-branch would need a similar fix?\n> >\n> > I think this still isn't resilient to weird meta-characters in the\n> > @SHELL_PATH@, but as this is a build-time option, I think it's OK to let\n> > people who do\n> >\n> >   make SHELL_PATH='}\"; rm -rf /'\n> >\n> > hang themselves.\n> >\n> > -Peff\n> \n> Okay, that's what @SHELL_PATH@ stands for. I just read the result\n> in the Windows installation that is something like ${SHELL:-/bin/sh}.\n> The shell script processor then replaces /bin/sh with\n> C:\\Program Files\\...\\bin\\sh.\n\nHmm. Now I'm a bit puzzled. It sounds like the installed file does have\n@SHELL_PATH@ set to /bin/sh, which is normal. And presumably the setting\ncontaining space is coming from the $SHELL environment variable.\n\nMy original \"I could not think of a case harmed by it\" was because\n@SHELL_PATH@ also gets used on the shebang line at the beginning of the\nscript. And that does not really handle command names with arguments\nwell (you get one argument, and it better not have spaces in it). Of\ncourse, it _also_ does not handle command names with spaces in them,\neither (and there's no room for quoting there).\n\nSo anything exotic pretty much has to be coming in from $SHELL, and my\nmention of filter-branch is not interesting; it just uses @SHELL_PATH@.\n\nSo what are people putting in $SHELL? Obviously a shell path with a\nspace in it wants the quotes. Do people do more exotic things like:\n\n  SHELL=\"my-shell --options\"\n\n(I think a plausible option might be \"--posix\" for some shells).  That\nwould break with your patch.\n\nI dunno. We usually try to err on the side of the status quo, so as to\navoid breaking existing users. But I find the idea of $SHELL with\noptions reasonably unlikely, so I think your patch is the right\ndirection. I wish I understood better who was setting $SHELL and why,\nthough.\n\n-Peff\n"},{"id":"273295","messageId":"CABA5-zmmKeeDDhSdan9sTppRVeDNuPiR8WX=P4Umdbq=ELafqw@mail.gmail.com","threadId":"40789","inReplyTo":"20151113222748.GA14830@sigill.intra.peff.net","subject":"Re: [PATCH] rebase-i-exec: Allow space in SHELL_PATH","fromName":"Fredrik Medley","fromEmail":"fredrik.medley@gmail.com","sentAt":"2015-11-13T22:47:40Z","receivedAt":"2015-11-13T22:47:40Z","isPatch":true,"sender":{"key":"fredrik.medley@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5401799?v=4"},"body":"2015-11-13 23:27 GMT+01:00 Jeff King <peff@peff.net>:\n> On Fri, Nov 13, 2015 at 04:25:18PM +0100, Fredrik Medley wrote:\n>\n>> >> -             ${SHELL:-@SHELL_PATH@} -c \"$rest\" # Actual execution\n>> >> +             \"${SHELL:-@SHELL_PATH@}\" -c \"$rest\" # Actual execution\n>> >\n>> > I think this is the right thing to do (at least I could not think of a\n>> > case that would be harmed by it, and it certainly fixes your case). It\n>> > looks like filter-branch would need a similar fix?\n>> >\n>> > I think this still isn't resilient to weird meta-characters in the\n>> > @SHELL_PATH@, but as this is a build-time option, I think it's OK to let\n>> > people who do\n>> >\n>> >   make SHELL_PATH='}\"; rm -rf /'\n>> >\n>> > hang themselves.\n>> >\n>> > -Peff\n>>\n>> Okay, that's what @SHELL_PATH@ stands for. I just read the result\n>> in the Windows installation that is something like ${SHELL:-/bin/sh}.\n>> The shell script processor then replaces /bin/sh with\n>> C:\\Program Files\\...\\bin\\sh.\n>\n> Hmm. Now I'm a bit puzzled. It sounds like the installed file does have\n> @SHELL_PATH@ set to /bin/sh, which is normal. And presumably the setting\n> containing space is coming from the $SHELL environment variable.\nI wrote the email on another computer. Checking the system where I reinstalled\nGit for Windows yesterday shows ${SHELL:-/bin/sh} and SHELL=/usr/bin/bash\nWhen running \"git rebase --interactive HEAD^ --exec 'echo $SHELL'\", I get\nmingw64/libexec/git-core/git-rebase--interactive: line 613:\nC:/Program: No such file or directory\n\nFixing the double quotes, \"git rebase --interactive HEAD^ --exec 'echo\n$SHELL'\" shows\nC:/Program Files/Git/usr/bin/bash\n\n>\n> My original \"I could not think of a case harmed by it\" was because\n> @SHELL_PATH@ also gets used on the shebang line at the beginning of the\n> script. And that does not really handle command names with arguments\n> well (you get one argument, and it better not have spaces in it). Of\n> course, it _also_ does not handle command names with spaces in them,\n> either (and there's no room for quoting there).\n>\n> So anything exotic pretty much has to be coming in from $SHELL, and my\n> mention of filter-branch is not interesting; it just uses @SHELL_PATH@.\n>\n> So what are people putting in $SHELL? Obviously a shell path with a\n> space in it wants the quotes. Do people do more exotic things like:\n>\n>   SHELL=\"my-shell --options\"\n>\n> (I think a plausible option might be \"--posix\" for some shells).  That\n> would break with your patch.\nI agree, tricky case. Just to accept the fact that it won't work.\n\n>\n> I dunno. We usually try to err on the side of the status quo, so as to\n> avoid breaking existing users. But I find the idea of $SHELL with\n> options reasonably unlikely, so I think your patch is the right\n> direction. I wish I understood better who was setting $SHELL and why,\n> though.\nI don't understand where $SHELL is being set neither.\n\n>\n> -Peff\n\n/Fredrik\n"},{"id":"273296","messageId":"20151113230933.GA16173@sigill.intra.peff.net","threadId":"40789","inReplyTo":"CABA5-zmmKeeDDhSdan9sTppRVeDNuPiR8WX=P4Umdbq=ELafqw@mail.gmail.com","subject":"Re: [PATCH] rebase-i-exec: Allow space in SHELL_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-13T23:09:33Z","receivedAt":"2015-11-13T23:09:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 13, 2015 at 11:47:40PM +0100, Fredrik Medley wrote:\n\n> > Hmm. Now I'm a bit puzzled. It sounds like the installed file does have\n> > @SHELL_PATH@ set to /bin/sh, which is normal. And presumably the setting\n> > containing space is coming from the $SHELL environment variable.\n> I wrote the email on another computer. Checking the system where I reinstalled\n> Git for Windows yesterday shows ${SHELL:-/bin/sh} and SHELL=/usr/bin/bash\n> When running \"git rebase --interactive HEAD^ --exec 'echo $SHELL'\", I get\n> mingw64/libexec/git-core/git-rebase--interactive: line 613:\n> C:/Program: No such file or directory\n> \n> Fixing the double quotes, \"git rebase --interactive HEAD^ --exec 'echo\n> $SHELL'\" shows\n> C:/Program Files/Git/usr/bin/bash\n\nIt looks like bash on your system may simply be filling in its absolute\npath into the $SHELL variable. This seems like an interesting bash-ism:\n\n  $ unset SHELL\n  $ dash -c 'echo $SHELL'\n\n  $ bash -c 'echo $SHELL'\n  /bin/bash\n\nIf $SHELL is already set and exported, it will not override it, but in\nyour case $SHELL is probably local to each individual bash invocation.\n\nSo that at least kind-of makes sense. It's possible somebody could be\ndoing something clever with $SHELL, but I kind of doubt it. If they want\nto do something clever, it is much easier to put it directly on the exec\nline, and have the normal $SHELL run their clever thing.\n\n-Peff\n"},{"id":"273371","messageId":"alpine.DEB.1.00.1511161656480.1686@s15462909.onlinehome-server.info","threadId":"40789","inReplyTo":"20151113230933.GA16173@sigill.intra.peff.net","subject":"Re: [PATCH] rebase-i-exec: Allow space in SHELL_PATH","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-11-16T16:01:03Z","receivedAt":"2015-11-16T16:01:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 13 Nov 2015, Jeff King wrote:\n\n> It's possible somebody could be doing something clever with $SHELL, but\n> I kind of doubt it. If they want to do something clever, it is much\n> easier to put it directly on the exec line, and have the normal $SHELL\n> run their clever thing.\n\nTo clarify the Git for Windows scenario: SHELL_PATH is indeed set to\n`/bin/sh`, but reportedly it is converted into a full Windows path when we\nleave the POSIX emulation layer, i.e. when `git.exe` is called (which has\n*no* idea about POSIX paths, or at least next to none).\n\nThe reason is, of course, that regular Windows programs would not know\nwhat to do with the path /bin/sh.\n\nThe problem arises when we re-enter the POSIX realm, i.e. when we run a\nscript (such as `git-rebase`): the path is not converted back!\n\nThere are already tickets on Git for Windows' bug tracker where the case\nis made that vim has serious problems with that space in the environment\nvariable, so the long-term fix is really to teach the POSIX emulation\nlayer (read: MSys2) to convert SHELL_PATH back into a POSIX path when\nappropriate.\n\nThis is on my plate, but there are quite a couple more urgent tasks I need\nto take care of, so please do not hold your breath (unless you are into\nthat kind of thing).\n\nCiao,\nDscho\n"},{"id":"273373","messageId":"20151116163404.GB15046@sigill.intra.peff.net","threadId":"40789","inReplyTo":"alpine.DEB.1.00.1511161656480.1686@s15462909.onlinehome-server.info","subject":"Re: [PATCH] rebase-i-exec: Allow space in SHELL_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-16T16:34:05Z","receivedAt":"2015-11-16T16:34:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 16, 2015 at 05:01:03PM +0100, Johannes Schindelin wrote:\n\n> To clarify the Git for Windows scenario: SHELL_PATH is indeed set to\n> `/bin/sh`, but reportedly it is converted into a full Windows path when we\n> leave the POSIX emulation layer, i.e. when `git.exe` is called (which has\n> *no* idea about POSIX paths, or at least next to none).\n> \n> The reason is, of course, that regular Windows programs would not know\n> what to do with the path /bin/sh.\n> \n> The problem arises when we re-enter the POSIX realm, i.e. when we run a\n> script (such as `git-rebase`): the path is not converted back!\n\nAh, thanks for this explanation. The whole thing seems much more clear\nto me now.\n\nEven if later versions give us back the POSIX name, I think Fredrik's\npatch is a sane solution (and arguably how it should have been written\nin the first place, even leaving aside this issue).\n\n-Peff\n"}]}