[PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[Ben Walton]

During the testing of the 1.7.10 rc series on Solaris for OpenCSW, it
was discovered that t7006-pager was failing due to finding a bad "sh"
in PATH after a call to execvp("sh", ...).  This call was setup by
run_command.c:prepare_shell_cmd.

The PATH in use at the time saw /opt/csw/bin given precedence to
traditional Solaris paths such as /usr/bin and /usr/xpg4/bin.  A
package named schilyutils (Joerg Schilling's utilities) was installed
on the build system and it delivered a modified version of the
traditional Solaris /usr/bin/sh as /opt/csw/bin/sh.  This version of
sh suffers from many of the same problems as /usr/bin/sh.

The command-specific pager test failed due to the broken "sh" handling
^ as a pipe character.  It tried to fork two processes when it
encountered "sed s/^/foo:/" as the pager command.  This problem was
entirely dependent on the PATH of the user at runtime.

Possible fixes for this issue are:

1. Use the standard system() or popen() which both launch a POSIX
   shell on Solaris as long as _POSIX_SOURCE is defined.

2. The git wrapper could prepend SANE_TOOL_PATH to PATH thus forcing
   all unqualified commands run to use the known good tools on the
   system.

3. The run_command.c:prepare_shell_command() could use the same
   SHELL_PATH that is in the #! line of all all scripts and not rely
   on PATH to find the sh to run.

Option 1 would preclude opening a bidirectional pipe to a filter
script and would also break git for Windows as cmd.exe is spawned from
system() (cf. v1.7.5-rc0~144^2, "alias: use run_command api to execute
aliases, 2011-01-07).

Option 2 is not friendly to users as it would negate their ability to
use tools of their choice in many cases.  Alternately, injecting
SANE_TOOL_PATH such that it takes precedence over /bin and /usr/bin
(and anything with lower precedence than those paths) as
git-sh-setup.sh does would not solve the problem either as the user
environment could still allow a bad sh to be found.  (Many OpenCSW
users will have /opt/csw/bin leading their PATH and some subset would
have schilyutils installed.)

Option 3 allows us to use a known good shell while still honouring the
users' PATH for the utilities being run.  Thus, it solves the problem
while not negatively impacting either users or git's ability to run
external commands in convenient ways.  Essentially, the shell is a
special case of tool that should not rely on SANE_TOOL_PATH and must
be called explicitly.

With this patch applied, any code path leading to
run_command.c:prepare_shell_cmd can count on using the same sane shell
that all shell scripts in the git suite use.  Both the build system
and run_command.c will default this shell to /bin/sh unless
overridden.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 Makefile      |    2 ++
 run-command.c |    6 +++++-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index be1957a..dea1f15 100644
--- a/Makefile
+++ b/Makefile
@@ -1913,6 +1913,8 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
+run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH_SQ)"'
+
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
 	ln git$X $@ 2>/dev/null || \
diff --git a/run-command.c b/run-command.c
index 1db8abf..2af3e0f 100644
--- a/run-command.c
+++ b/run-command.c
@@ -4,6 +4,10 @@
 #include "sigchain.h"
 #include "argv-array.h"
 
+#ifndef SHELL_PATH
+# define SHELL_PATH "/bin/sh"
+#endif
+
 struct child_to_clean {
 	pid_t pid;
 	struct child_to_clean *next;
@@ -90,7 +94,7 @@ static const char **prepare_shell_cmd(const char **argv)
 		die("BUG: shell command is empty");
 
 	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
-		nargv[nargc++] = "sh";
+		nargv[nargc++] = SHELL_PATH;
 		nargv[nargc++] = "-c";
 
 		if (argc < 2)
-- 
1.7.5.4

Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[Jonathan Nieder]

Ben Walton wrote:

> During the testing of the 1.7.10 rc series on Solaris for OpenCSW, it
> was discovered that t7006-pager was failing
[...]
> --- a/run-command.c
> +++ b/run-command.c
> @@ -4,6 +4,10 @@
>  #include "sigchain.h"
>  #include "argv-array.h"
>  
> +#ifndef SHELL_PATH
> +# define SHELL_PATH "/bin/sh"
> +#endif
> +
>  struct child_to_clean {
>  	pid_t pid;
>  	struct child_to_clean *next;
> @@ -90,7 +94,7 @@ static const char **prepare_shell_cmd(const char **argv)
>  		die("BUG: shell command is empty");
>  
>  	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
> -		nargv[nargc++] = "sh";
> +		nargv[nargc++] = SHELL_PATH;

The underlying problem is an old one.  Thanks for fixing it.

For what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[...]
> Essentially, the shell is a
> special case of tool that should not rely on SANE_TOOL_PATH and must
> be called explicitly.

It is probably annoying to hear me say this, but:

The above doesn't tell me _why_ it is a special case and that on
Solaris users have been burned by "sh" being the original Bourne shell
or a temperamental version of ksh so SHELL_PATH usually points to bash
or ksh93 instead.

I trust the reader enough to fill in the blank, though, so I think the
patch is ok as is.

Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Ben Walton wrote:
>
> The underlying problem is an old one.  Thanks for fixing it.
>
> For what it's worth,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks, all of you who participated in the review process.

Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[Jonathan Nieder]

Ben Walton wrote:

> --- a/Makefile
> +++ b/Makefile
> @@ -1913,6 +1913,8 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
>  	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
>  	'-DGIT_INFO_PATH="$(infodir_SQ)"'
>  
> +run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH_SQ)"'

Sorry I forgot to send this before.  Quick tweaks:

 - let assembler listings (from "make run-command.s") and sparse checks
   reflect the SHELL_PATH setting, too

 - the entire -DFOO="bar" argument is surrounded with single quotes
   in other EXTRA_CPPFLAGS settings, so let this one follow that
   pattern to avoid standing out

diff --git i/Makefile w/Makefile
index dea1f157..a3791139 100644
--- i/Makefile
+++ w/Makefile
@@ -1913,7 +1913,8 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
-run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH_SQ)"'
+run-command.sp run-command.s run-command.o: EXTRA_CPPFLAGS = \
+	'-DSHELL_PATH="$(SHELL_PATH_SQ)"'
 
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \

Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Sorry I forgot to send this before.  Quick tweaks:
>
>  - let assembler listings (from "make run-command.s") and sparse checks
>    reflect the SHELL_PATH setting, too
>
>  - the entire -DFOO="bar" argument is surrounded with single quotes
>    in other EXTRA_CPPFLAGS settings, so let this one follow that
>    pattern to avoid standing out
>
> diff --git i/Makefile w/Makefile
> index dea1f157..a3791139 100644
> --- i/Makefile
> +++ w/Makefile
> @@ -1913,7 +1913,8 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
>  	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
>  	'-DGIT_INFO_PATH="$(infodir_SQ)"'
>  
> -run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH_SQ)"'
> +run-command.sp run-command.s run-command.o: EXTRA_CPPFLAGS = \
> +	'-DSHELL_PATH="$(SHELL_PATH_SQ)"'
>  
>  $(BUILT_INS): git$X
>  	$(QUIET_BUILT_IN)$(RM) $@ && \

Actually, I do not think this is sufficient, and it happens that you and I
are the best position to realize it ;-).

Look at what is done in the Makefile for DEFAULT_EDITOR and DEFAULT_PAGER,
and compare with what the above is doing, and think why the EDITOR/PAGER
needs to have another level of quoting.

Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jonathan Nieder <jrnieder@gmail.com> writes:
> ...
>> -run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH_SQ)"'
>> +run-command.sp run-command.s run-command.o: EXTRA_CPPFLAGS = \
>> +	'-DSHELL_PATH="$(SHELL_PATH_SQ)"'
>>  
>>  $(BUILT_INS): git$X
>>  	$(QUIET_BUILT_IN)$(RM) $@ && \
>
> Actually, I do not think this is sufficient, and it happens that you and I
> are in the best position to realize it ;-).
>
> Look at what is done in the Makefile for DEFAULT_EDITOR and DEFAULT_PAGER,
> and compare with what the above is doing, and think why the EDITOR/PAGER
> needs to have another level of quoting.

In other words, something like this squashed into Ben's patch...

diff --git i/Makefile w/Makefile
index dea1f15..abee43e 100644
--- i/Makefile
+++ w/Makefile
@@ -1849,6 +1849,13 @@ DEFAULT_PAGER_CQ_SQ = $(subst ','\'',$(DEFAULT_PAGER_CQ))
 BASIC_CFLAGS += -DDEFAULT_PAGER='$(DEFAULT_PAGER_CQ_SQ)'
 endif
 
+ifdef SHELL_PATH
+SHELL_PATH_CQ = "$(subst ",\",$(subst \,\\,$(SHELL_PATH)))"
+SHELL_PATH_CQ_SQ = $(subst ','\'',$(SHELL_PATH_CQ))
+
+BASIC_CFLAGS += -DSHELL_PATH='$(SHELL_PATH_CQ_SQ)'
+endif
+
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
 
@@ -1913,8 +1920,6 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
-run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH_SQ)"'
-
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
 	ln git$X $@ 2>/dev/null || \

[PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Johannes Sixt]

From: Johannes Sixt <j6t@kdbg.org>

The recent change to use SHELL_PATH instead of "sh" to spawn shell commands
is not suited for Windows:

- The default setting, "/bin/sh", does not work when git has to run the
  shell because it is a POSIX style path, but not a proper Windows style
  path.

- If it worked, it would hard-code a position in the files system where
  the shell is expected, making git (more precisely, the POSIX toolset that
  is needed alongside git) non-relocatable. But we cannot sacrifice
  relocatability on Windows.

- Apart from that, even though the Makefile leaves SHELL_PATH set to
  "/bin/sh" for the Windows builds, the build system passes a mangled path
  to the compiler, and something like "D:/Src/msysgit/bin/sh" is used,
  which is doubly bad because it points to where /bin/sh resolves to on
  the system where git was built.

- Finally, the system's CreateProcess() function that is used under
  mingw.c's hood does not work with forward slashes and cannot find the
  shell.

Undo the earlier change on Windows.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Am 3/31/2012 3:33, schrieb Ben Walton:
> +#ifndef SHELL_PATH
> +# define SHELL_PATH "/bin/sh"
> +#endif
> +
>  struct child_to_clean {
>  	pid_t pid;
>  	struct child_to_clean *next;
> @@ -90,7 +94,7 @@ static const char **prepare_shell_cmd(const char **argv)
>  		die("BUG: shell command is empty");
>  
>  	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
> -		nargv[nargc++] = "sh";
> +		nargv[nargc++] = SHELL_PATH;
>  		nargv[nargc++] = "-c";
>  
>  		if (argc < 2)

This looked so obviously correct, that I carelessly did not test the
change. But given that the first two points addressed in the commit
message above are actually dealbreakers, I should have noticed this issue
much earlier.

-- Hannes

 run-command.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/run-command.c b/run-command.c
index 2af3e0f..e4edede 100644
--- a/run-command.c
+++ b/run-command.c
@@ -94,7 +94,11 @@ static const char **prepare_shell_cmd(const char **argv)
 		die("BUG: shell command is empty");
 
 	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
+#ifndef WIN32
 		nargv[nargc++] = SHELL_PATH;
+#else
+		nargv[nargc++] = "sh";
+#endif
 		nargv[nargc++] = "-c";
 
 		if (argc < 2)
-- 
1.7.10.1370.ga6f62.dirty

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Ben Walton]

Excerpts from Johannes Sixt's message of Tue Apr 17 03:03:21 -0400 2012:

Hi Hannes,

> The recent change to use SHELL_PATH instead of "sh" to spawn shell
> commands is not suited for Windows:

Ah yes, I can see how that would be undesirable on Windows.


> +#ifndef WIN32
>          nargv[nargc++] = SHELL_PATH;
> +#else
> +        nargv[nargc++] = "sh";
> +#endif
>          nargv[nargc++] = "-c";

A minor style question: Would this conditional code not be better if
it were up where the fallback SHELL_PATH is set?  That would leave
this code block #ifdef free while still avoiding the breakage on
Windows.

Admittedly, it's a very minor point and possibly entirely a personal
preference.

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Johannes Sixt]

Am 4/17/2012 15:45, schrieb Ben Walton:
> Excerpts from Johannes Sixt's message of Tue Apr 17 03:03:21 -0400 2012:
>> The recent change to use SHELL_PATH instead of "sh" to spawn shell
>> commands is not suited for Windows:
> 
> Ah yes, I can see how that would be undesirable on Windows.
> 
>> +#ifndef WIN32
>>          nargv[nargc++] = SHELL_PATH;
>> +#else
>> +        nargv[nargc++] = "sh";
>> +#endif
>>          nargv[nargc++] = "-c";
> 
> A minor style question: Would this conditional code not be better if
> it were up where the fallback SHELL_PATH is set?  That would leave
> this code block #ifdef free while still avoiding the breakage on
> Windows.

It would require either to #define yet another new name to SHELL_PATH or
"sh", or to #undef SHELL_PATH and re#define it for WIN32. Neither is very
appealing. I personally prefer to have the conditional (if it is as short
as this one) at the point where it matters.

-- Hannes

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Ben Walton]

Excerpts from Johannes Sixt's message of Tue Apr 17 10:00:25 -0400 2012:

> It would require either to #define yet another new name to
> SHELL_PATH or "sh", or to #undef SHELL_PATH and re#define it for
> WIN32. Neither is very appealing. I personally prefer to have the
> conditional (if it is as short as this one) at the point where it
> matters.

Ok.  I'll defer to your more experienced judgement. :)  Thanks for
clarifying though.

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Jeff King]

On Tue, Apr 17, 2012 at 09:03:21AM +0200, Johannes Sixt wrote:

> diff --git a/run-command.c b/run-command.c
> index 2af3e0f..e4edede 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -94,7 +94,11 @@ static const char **prepare_shell_cmd(const char **argv)
>  		die("BUG: shell command is empty");
>  
>  	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
> +#ifndef WIN32
>  		nargv[nargc++] = SHELL_PATH;
> +#else
> +		nargv[nargc++] = "sh";
> +#endif
>  		nargv[nargc++] = "-c";
>  
>  		if (argc < 2)

It sounds like the real problem is not the use of a configurable shell,
but rather the use of an absolute path. Should you maybe try to pass the
basename of SHELL_PATH? Or maybe that is not even worth worrying about,
as somebody on Windows is not going to ever set SHELL_PATH, since it is
not like they are working around a non-POSIX "sh" included with the
operating system (which is why people on Solaris typically set
SHELL_PATH).

-Peff

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Johannes Sixt]

Am 4/18/2012 0:14, schrieb Jeff King:
> On Tue, Apr 17, 2012 at 09:03:21AM +0200, Johannes Sixt wrote:
> 
>> diff --git a/run-command.c b/run-command.c
>> index 2af3e0f..e4edede 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -94,7 +94,11 @@ static const char **prepare_shell_cmd(const char **argv)
>>  		die("BUG: shell command is empty");
>>  
>>  	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
>> +#ifndef WIN32
>>  		nargv[nargc++] = SHELL_PATH;
>> +#else
>> +		nargv[nargc++] = "sh";
>> +#endif
>>  		nargv[nargc++] = "-c";
>>  
>>  		if (argc < 2)
> 
> It sounds like the real problem is not the use of a configurable shell,
> but rather the use of an absolute path. Should you maybe try to pass the
> basename of SHELL_PATH? Or maybe that is not even worth worrying about,
> as somebody on Windows is not going to ever set SHELL_PATH, since it is
> not like they are working around a non-POSIX "sh" included with the
> operating system (which is why people on Solaris typically set
> SHELL_PATH).

I thought about offering a customization point, but decided that it is not
worth the hassle: Most people download an installer, then the installer
can set up the PATH so that "sh" is not broken or something entirely
unrelated. And those who build git themselves know sufficiently well what
they are doing.

-- Hannes

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Jeff King]

On Wed, Apr 18, 2012 at 07:39:43AM +0200, Johannes Sixt wrote:

> > It sounds like the real problem is not the use of a configurable shell,
> > but rather the use of an absolute path. Should you maybe try to pass the
> > basename of SHELL_PATH? Or maybe that is not even worth worrying about,
> > as somebody on Windows is not going to ever set SHELL_PATH, since it is
> > not like they are working around a non-POSIX "sh" included with the
> > operating system (which is why people on Solaris typically set
> > SHELL_PATH).
> 
> I thought about offering a customization point, but decided that it is not
> worth the hassle: Most people download an installer, then the installer
> can set up the PATH so that "sh" is not broken or something entirely
> unrelated. And those who build git themselves know sufficiently well what
> they are doing.

OK. I'll defer your judgement on what Windows users prefer. I just
wanted to make sure it was a conscious decision.

Other than that, the patch looks obviously correct to me. Thanks, and
sorry for inadvertently breaking git on windows (again). :)

-Peff

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

>> It sounds like the real problem is not the use of a configurable shell,
>> but rather the use of an absolute path. Should you maybe try to pass the
>> basename of SHELL_PATH? ...
>
> I thought about offering a customization point, but decided that it is not
> worth the hassle...

I think you meant by the above that the installer could offer the user to
choose where to place the shell, and if that is the case, I agree it would
probably not worth it.

There is one thing I am confused about your response, though.  I thought
the suggestion by Peff was to build your binary with "make SHELL_PATH=sh"
(not "make SHELL_PATH=/bin/sh").  I do not know if that works or does not
work (I do not see why not, though), but in either case offering a new
customization point sounds like a separate issue.

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Johannes Sixt]

Am 4/18/2012 18:30, schrieb Junio C Hamano:
> There is one thing I am confused about your response, though.  I thought
> the suggestion by Peff was to build your binary with "make SHELL_PATH=sh"
> (not "make SHELL_PATH=/bin/sh").  I do not know if that works or does not
> work (I do not see why not, though), but in either case offering a new
> customization point sounds like a separate issue.

I tried this, and it does not work. (Shell scripts' shbang line would end
up as '#!sh', and at least bash does not like to run such scripts.)

The alternative is to customize the -DSHELL_PATH=... compiler option for
Windows, and that is primarily what I don't like: It would mean that
either the name "SHELL_PATH" has different meaning in C code and the
Makefile, or we need a new name for the C macro. (The latter would be "a
new customization point".)

I learn from this conversation that I should note failed alternatives more
frequently in the commit message to save reviewers' time.

-- Hannes

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 4/18/2012 18:30, schrieb Junio C Hamano:
>> There is one thing I am confused about your response, though.  I thought
>> the suggestion by Peff was to build your binary with "make SHELL_PATH=sh"
>> (not "make SHELL_PATH=/bin/sh").  I do not know if that works or does not
>> work (I do not see why not, though), but in either case offering a new
>> customization point sounds like a separate issue.
>
> I tried this, and it does not work. (Shell scripts' shbang line would end
> up as '#!sh', and at least bash does not like to run such scripts.)

Ahh, sorry for missing the obvious.  I should have realized about that
one, as customizing that one was the very original motivation of the make
variable.

In any case, as I wrote in the "What's cooking", I'll fast-track the fix
to 'master' shortly, so that Windows port does not have to carry it.

Thanks.