Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd

From: Jeff King <peff@peff.net>
Date: Mar 27, 2012, 03:29 UTC
Message-ID: <20120327032917.GB17338@sigill.intra.peff.net>
In-Reply-To: <1332816078-26829-1-git-send-email-bwalton@artsci.utoronto.ca>

On Mon, Mar 26, 2012 at 10:41:18PM -0400, Ben Walton wrote:

Show 10 quoted lines

> diff --git a/Makefile b/Makefile
> index be1957a..344e2e0 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)"'
> +

This should be $(SHELL_PATH_SQ), no?

Show 11 quoted lines

> diff --git a/run-command.c b/run-command.c
> index 1db8abf..f005a31 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 "sh"
> +#endif

Does this default ever kick in? The Makefile defaults SHELL_PATH to /bin/sh, so we will always end up with at least that.

Doing so at least makes us consistent across builds, but I wonder if we should leave it as "sh" on systems that do not set SHELL_PATH manually. Executing "sh" via the PATH is the normal system() thing to do. I doubt many people have an "sh" in their PATH ahead of /bin/sh, but if they do, we are changing the behavior for them for no good reason.

The whole SHELL_PATH and SANE_TOOL_PATH mess is about helping people on less-abled systems, and I do not mind bending the usual conventions to make things more convenient on those systems (e.g., by not doing the PATH lookup of the shell name). But it would be nice if that bending did not affect people on more mainstream systems.

-Peff