Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH

From: Jeff King <peff@peff.net>
Date: Mar 26, 2012, 17:58 UTC
Message-ID: <20120326175843.GF7942@sigill.intra.peff.net>
In-Reply-To: <1332678696-4001-1-git-send-email-bwalton@artsci.utoronto.ca>

On Sun, Mar 25, 2012 at 08:31:34AM -0400, Ben Walton wrote:

> [Others touched this file too, but it appears Jeff wrote the affected
> functionality.]

Sort of. I was just refactoring what was happening elsewhere in the code, where individual callsites were sitting "sh -c" manually into the argv list. So I didn't actually give any thought to whether SHELL_PATH should be used. :)

Show 14 quoted lines

> I hit a glitch with t7006-pager while testing the 1.7.10 rc1/rc2
> builds for OpenCSW/Solaris that turned out to be a problem with the
> way run-command.c:prepare_shell_cmd was setting up external
> utilities.  It was hard coded to fork 'sh -c' instead of honouring the
> SHELL_PATH as set at build time.
> 
> In this case, the failing test was t7006-pager:command-specific
> pager.  That test (and some subsequent ones) were setting the pager
> command used by git log to "sed s/^/foo:/ >actual" which is fine in a
> POSIX-compliant sh, but not in Solaris' sh.  If the user PATH at
> runtime happened to allow the broken system sh used instead of a sane
> sh, the ^ is interpreted the same as[1] | and this caused sed to fail
> with incomplete s/ command and a "command not found: /foo:" from the
> other forked process.

The original intent of SHELL_PATH is to give the full path, so it could be used in places where PATH lookup is not an option (i.e., on #!-lines). Whereas the run-command use_shell option looks up "sh" in the PATH, so the "right" thing is to have your PATH set up to put a sane "sh" near the front.

So in that sense, your patch is unnecessary. On the other hand, it is very unlikely to harm anyone[1], and it reduces the number of things the user has to make sure are set up properly, so I think overall it's an improvement, even if there is already another way to fix the problem.

-Peff

[1] I can't imagine anybody will be upset that we would use SHELL_PATH
    instead of "sh". It could produce a difference of behavior if you
    had a sane "sh" in your PATH, but SHELL_PATH points to the absolute
    path of some other shell. But if your SHELL_PATH and the "sh" in
    your PATH are so wildly divergent that you notice the difference, I
    think you may have bigger problems.