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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mar 27, 2012, 04:26 UTC
Message-ID: <20120327042620.GA22547@burratino>
In-Reply-To: <1332816078-26829-1-git-send-email-bwalton@artsci.utoronto.ca>

Hi again,

Ben Walton wrote:

Show 6 quoted lines

> The shell spawned in run-command.c:prepare_shell_cmd was hard coded to
> "sh."  This was breaking "t7006-pager:command-specific pager" when the
> "sh" found in the PATH happened to be a broken one such as Solaris'
> /usr/bin/sh.  (The breakage in this case was due to ^ being
> interpreted the same as | which was seeing two processes forked
> instead of a single sed process.)

Better. But the above is still focusing on what you changed rather than the intended impact and potential fallout. Remember, the commit log is where most of git's design documents are stored.

Isn't it something more like this?

	Ever since <date or version>, the "command-specific pager" test
	from t7006-pager has not been passing on Solaris with the
	default value of PATH.

	The cause: that test (and some subsequent ones) sets 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.  The
	run_command machinery uses plain "sh" as a shell and if the user
	PATH at runtime happens to put the ancient system sh before any
	modern POSIX-style sh, the ^ is interpreted the same as | and
	chaos results.

	Or to put it another way, t7006 expects that the shell used to
	interpret GIT_PAGER is a POSIX-style shell and run_command's
	shell feature fails to deliver.

	To fix this:

	a. instead of launching a shell on its own, run_command could
	   call system() or popen().  This launches a POSIX shell on
	   Solaris as long as _POSIX_SOURCE is defined.

	b. the git wrapper could prepend SANE_TOOL_PATH to $PATH for
	   builtin commands for consistency with scripted commands that
	   do that using git-sh-setup.  If we're lucky, the first "sh"
	   command found in the new PATH would be a suitable shell,
	   though git's current scripts do not rely on that.

	c. the run_command machinery could use the same SHELL_PATH
	   shell that is used by the git filter-branch script and in
	   all script's #! lines.

	However:

	- (a) means losing the ability to open a bidirectional pipe to a
	  filter script.  It would also break git for Windows, where
	  system() uses cmd.exe instead of a POSIX-style shell (see
	  v1.7.5-rc0~144^2, "alias: use run_command api to execute
	  aliases, 2011-01-07).

	- On Solaris systems with schilyutils in /opt/csw/bin at the
	  beginning of SANE_TOOL_PATH, (b) finds /opt/csw/bin/sh which
	  is another ancient Bourne-style shell with the same problem.

	So let's go with (c).

	After this patch, $GIT_PAGER is interpreted by the same shell
	in scripted commands which use the git_pager function that uses
	"eval" and builtins which use the run_command machinery to
	launch the interpreter specified as SHELL_PATH when git was built.

Just my two cents, Jonathan