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

From: Jeff King <peff@peff.net>
Date: Mar 28, 2012, 04:22 UTC
Message-ID: <20120328042215.GB30251@sigill.intra.peff.net>
In-Reply-To: <1332901381-sup-4406@pinkfloyd.chass.utoronto.ca>

On Tue, Mar 27, 2012 at 10:46:15PM -0400, Ben Walton wrote:

Show 11 quoted lines

> > > +#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.
> 
> Not when using the build system, but as Hannes mentioned, there is
> potential for this to be used outside of the default build system, so
> I think having the fallback is a good defensive option.  Should it
> maybe be set to /bin/sh though to be more consistent with system()?

Yeah, I think making it "/bin/sh" is better. It's more obvious to people reading the code as part of git (since even though it is a repetition of the default from the Makefile, at least it is the _same_ default), and if it does get reused, it's probably a better default.

Show 7 quoted lines

> Given the rest of the discussion that happened, I think I understand
> that my patch is actually ok with the following caveats:
> 
> 1. My commit message still needs work.
> 2. Possibly change the default setting of the SHELL_PATH macro from
>    "sh" to "/bin/sh"
> 3. Use the _SQ variant of SHELL_PATH.

Yeah, I think so (not that I am the final word, but that at least matches my perception of the discussion so far, too).

> (The tracking of changes for SHELL_PATH is considered too heavy for
> now when the other _PATH items aren't tracked the same way.  This
> might make a nice separate patch series though, using the idea from
> the kernel where individual commands are tracked.)

Agreed.

-Peff