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. :)
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html