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

From: Ben Walton <bwalton@artsci.utoronto.ca>
Date: Mar 28, 2012, 02:46 UTC
Message-ID: <1332901381-sup-4406@pinkfloyd.chass.utoronto.ca>
In-Reply-To: <20120327032917.GB17338@sigill.intra.peff.net>

Excerpts from Jeff King's message of Mon Mar 26 23:29:17 -0400 2012:

Hi Jeff,

> > +run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH)"'
> > +
> 
> This should be $(SHELL_PATH_SQ), no?

Yes, you're right, it should be.

Show 6 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()?

Show 6 quoted lines

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

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.

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

Did I miss anything else?

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