patch, 2 partsMakefile: Set EXTRA_CPPFLAGS during the compilation of run-command

Hi Jeff and Junio,

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

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.

To mitigate this, the following patches introduce the macro SHELL_PATH for use in run-command.c, defaulting to "sh" to preserve the current behaviour and then cause the build system to provide the SHELL_PATH as set by the builder. This means that all processed forked by run-command will use the same interpreter as the shell scripts in the git suite.

I considered implementing a dynamically generated .h file for this, similar to common-cmds.h, but thought that was overkill at the current time. If you think that (or something else) is a better fit for the change, let me know and I'll make the required adjustments.

Thanks -Ben

[1] http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/cmd/sh/cmd.c#184

Ben Walton (2):
  run-command.c: Define SHELL_PATH macro for use in prepare_shell_cmd
  Makefile: Set EXTRA_CPPFLAGS during the compilation of run-command

 Makefile      |    2 ++
 run-command.c |    6 +++++-
 2 files changed, 7 insertions(+), 1 deletions(-)

The shell spawned in run-command.c:prepare_shell_cmd was hard coded to 'sh'. Instead, make this a macro named SHELL_PATH so that it can be overridden by the build system. Use 'sh' as the default to preserve original behaviour and ensure that a value is always set.

This avoids a situation where some commands were spawned using a different shell than the one configured at build time. Previously, it was possible for things to be executed by a non-POSIX shell depending on the user's PATH.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 run-command.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

When building run-command.o, we want to pass in a define for SHELL_PATH to ensure that the shell configured at build time is used at run time when spawning external commands.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 Makefile |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Hi Ben,

Good catch.  A few comments:

Ben Walton wrote:

When I first read the corresponding patch without reading this cover letter I was mystified. Until I saw the above paragraph, I did not even see what problem was being solved. The above paragraph should probably be part of the commit message.

Ok, on to the proposed solution. ;-)

My first reaction was to suspect the series is solving the problem in the wrong place. The core of the bug might be t7006 itself, which assumes that the shell used to interpret the GIT_PAGER setting is a POSIX-style shell rather than an ancient Bourne shell or cmd.exe. In the far long term, we should probably skip this test on some platforms using an appropriate test prerequisite.

To put it another way, the RUN_USING_SHELL magic is just supposed to be a more featureful way to do what system() normally does. What shell does system() use on Solaris?

A second reaction was to wonder why the usual fixup from v1.6.4-rc0~66^2~1 (Makefile: insert SANE_TOOL_PATH to PATH before /bin or /usr/bin, 2009-07-08) didn't apply. Should the git wrapper prepend the same magic to $PATH that git-sh-setup.sh does to make the behavior of scripted and unscripted commands a little more consistent?

A third reaction was that git_pager in the sh-setup library uses the eval builtin, so we are already assuming that GIT_PAGER is appropriate input for a POSIX-style shell. So maybe the approach you've adopted is appropriate after all, at least in the short term while we require a POSIX-style shell elsewhere in git.

A few added words in the commit message could save the next reader from going through so long a thought process before seeing why what the patch does is the right thing to do.

A more minor comment: patch 1/2 was even more mysterious. Combining the two patches would be enough to avoid confusion there. I haven't checked the makefile changes and interaction with GIT-CFLAGS carefully yet and hope to do so in the next round.

Thanks for working on this.

Sincerely, Jonathan

Excerpts from Jonathan Nieder's message of Sun Mar 25 21:11:48 -0400 2012:

Hi Jonathan,

Ok, that's fair. I thought the commit message was self-contained enough to justify the change, but point taken. :) I'll redo the commit log and squash the two patches as per your later points.

I started looking through t7006 first as well, thinking that some non-POSIX syntax had slipped in... This raises a good point though. I'd come at this thinking that things _should_ be able to rely on a POSIX shell (or a modern standard, anyway) for things like this. Is there any sort of defined expectation for this?

something sane on the current platform. I haven't looked for this specifically, but this code path would still try to fork 'sh' on Windows as far as my understanding of it goes. I don't use git there, so I don't know if part of the installation sets up a usable sh too.

This depends on some macros. The default is /usr/bin/sh, but if various XOPEN-type macros are set, it will use /usr/xpg4/bin/sh.

I did some poking at this before creating my patch as that is what I'd expected too. It would likely be a good idea, but in my case, even that wouldn't help.

I add /opt/csw/gnu:/opt/csw/bin:/usr/xpg4/bin to the SANE_TOOL_PATH but set the SHELL_PATH to /opt/csw/bin/bash. Without my patch, but with SANE_TOOL_PATH honoured, I'd still see /opt/csw/bin/sh forked and that sh is crippled too.

So that leads me to think that if we're going to fork a shell, it should be one that we know to be good...if the builder has provided that value. I think you agree with this based on your next comment.

I'm unclear what you're meaning by this. Are you implying that the requirement for a POSIX-style shell should be relaxes to the point where things don't rely on that base set of functionality?

Ok, I can re-do the commit message to incorporate notes about the broken text and some of your rationale above.

To me they were two logically separate changes, so I split them. I don't mind squashing it together though.

I'll resubmit later today.

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

On Mon, Mar 26, 2012 at 09:38:45AM -0400, Ben Walton wrote:

What is /opt/csw/bin/sh? Is it a symlink to bash (which would mean bash running in POSIX mode)? Or is it a lightweight shell like ash? Or something else?

The point of SHELL_PATH is to provide a POSIX shell. Generally, bash behavior is a superset of POSIX, so you will not run into any incompatibilities by running things with bash. But do be aware that you are slightly incompatible with the rest of the world (so things that work for you, for example, might not work for people using git with the stock "shell is /bin/sh" configuration).

-Peff

Excerpts from Jeff King's message of Mon Mar 26 14:12:38 -0400 2012:

This is some version of the traditional solaris /bin/sh packaged as part of schilyutils (Joerg Schilling's utilities). I'm really not sure why he packages it or what the differences from /bin/sh are...Asking him is on my todo list though. I only just learned it existed when I hit this glitch.

This was a conscious choice I made early in the packaging of git for OpenCSW. The idea was that we had a shell that was also under our packaging control and 'sane' at the same time. I realize it's a superset of the POSIX functionality. It's something I should maybe change, but I'd need to stage it slowly so as to avoid breakages.

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

On Mon, Mar 26, 2012 at 02:19:54PM -0400, Ben Walton wrote:

OK, that's gross. I would argue that your SANE_TOOL_PATH is not great, then, because the "sh" is not sane. But that being said, this is exactly the sort of thing I was talking about in my first message, which is that SANE_TOOL_PATH is hard to get right. If we can make things Just Work by using SHELL_PATH, I don't see that as a bad thing.

It sounds like using bash is probably the least bad thing to do, and doing anything else would not be worth the complexity. You could use "bash -posix" (or an sh-symlink to bash), but in practice I doubt it is really hurting anybody.

-Peff

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

To allow the build system to supply a value that is deemed sane, thus removing variation injected by the local user environment, define a macro named SHELL_PATH (to match the variable of the same name in the build system). Use this macro instead of the literal "sh" when preparing a set of arguments to be forked. Set "sh" as the default value of this macro if none is set through other means to preserve the original behaviour.

The Makefile now sets EXTRA_CPPFLAGS to a value that includes a definition of SHELL_PATH when building run_command.c. This sees the sane shell passed to run_command.c and used when forking external commands.

Leveraging SANE_TOOL_PATH was considered here but had downsides that may have made it an incomplete solution. For example, some builds may use bash as the shell and not want sh used at all.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 Makefile      |    2 ++
 run-command.c |    6 +++++-
 2 files changed, 7 insertions(+), 1 deletions(-)

On Mon, Mar 26, 2012 at 10:41:18PM -0400, Ben Walton wrote:

This should be $(SHELL_PATH_SQ), no?

Does this default ever kick in? The Makefile defaults SHELL_PATH to /bin/sh, so we will always end up with at least that.

Doing so at least makes us consistent across builds, but I wonder if we should leave it as "sh" on systems that do not set SHELL_PATH manually. Executing "sh" via the PATH is the normal system() thing to do. I doubt many people have an "sh" in their PATH ahead of /bin/sh, but if they do, we are changing the behavior for them for no good reason.

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.

-Peff

On Mon, Mar 26, 2012 at 11:29:17PM -0400, Jeff King wrote:

I think you could do that pretty easily with this in the Makefile:

Jeff King wrote:

It's more common for system() to default to /bin/sh.

I noticed the Makefile already doesn't do this sort of thing for mandir and htmldir, but do you think changes to SHELL_PATH should be tracked to force a rebuild when it changes?

	GIT-SHELL-PATH: FORCE
		@FLAGS='$(SHELL_PATH_SQ)'; \
		    if test x"$$FLAGS" != x"`cat GIT-SHELL-PATH 2>/dev/null`" ; then \
			echo 1>&2 "    * new shell path"; \
			echo "$$FLAGS" >GIT-SHELL-PATH; \
		    fi

	$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh GIT-SHELL-PATH
		...

	$(SCRIPT_LIB) : % : %.sh GIT-SHELL-PATH
		...

	run_command.sp run_command.o: GIT-SHELL-PATH
	exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \
		'-DSHELL_PATH="$(SHELL_PATH_SQ)"'

On Tue, Mar 27, 2012 at 12:01:10AM -0500, Jonathan Nieder wrote:

Hmm, you're right. I think I was fooled by the fact that it puts "sh" into argv[0]. It doesn't seem to actually do a PATH lookup, though (at least not on my system).

In that case, Ben's patch is actually making us _more_ normal and like system(), which is probably a slight benefit (although it's still a behavior change from what we've been doing).

Yeah, that would be nice.

In general, I wish adding these sorts of dependencies wasn't so manual and painful. I'm not sure of a good solution short of totally retooling our build system, though.

-Peff

Jeff King wrote:

Thanks. On second thought, without the corresponding tracking for PERL_PATH, USE_GETTEXT_SCHEME, localedir, etc, it doesn't seem worth it.

The Linux kernel build system has an interesting solution to this. It keeps track of the command line used to build each file and forces a rebuild when that command changes.

Ciao, Jonathan

Am 3/27/2012 5:29, schrieb Jeff King:

People do use this code outside git. Providing a fallback here would help them.

-- Hannes

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

Hi Jeff,

Yes, you're right, it should be.

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

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

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

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.

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

Agreed.

-Peff

During the testing of the 1.7.10 rc series on Solaris for OpenCSW, it was discovered that t7006-pager was failing due to finding a bad "sh" in PATH after a call to execvp("sh", ...). This call was setup by run_command.c:prepare_shell_cmd.

The SANE_TOOL_PATH in use at the time was lead by /opt/csw/gnu and /opt/csw/bin as used by OpenCSW packages so that OpenCSW packaged GNU tools are found instead of system versions. A package named schilyutils (Joerg Schilling's utilities) was installed on the build system and it provided a version of the traditional Solaris /usr/bin/sh, as /opt/csw/bin/sh. This version of "sh" contains many of the same problems as the traditional Solaris /usr/bin/sh.

The command-specific pager test failed due to the broken "sh" handling ^ as a pipe character. It tried to fork two processes when it encountered "sed s/^/foo:/" as the pager command. This problem was entirely dependent on the PATH of the user at runtime.

Possible fixes for this issue are:

1. Use the standard system() or popen() which both launch a POSIX
   shell on Solaris as long as _POSIX_SOURCE is defined.

2. The git wrapper could prepend SANE_TOOL_PATH to PATH for
   consistency with builtin commands.

3. The run_command.c:prepare_shell_command() could use the same
   SHELL_PATH that is in the #! line of all all scripts.

Option 1 would preclude opening a bidirectional pipe to a filter script and would also break git for Windows as cmd.exe is spawned from system() (cf. v1.7.5-rc0~144^2, "alias: use run_command api to execute aliases, 2011-01-07).

Option 2 is voided by the same example that turned up this issue. SANE_TOOL_PATH might also include 'insane' tools.

Option 3 is the best choice at this time.

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.

The default shell used by commands will be /bin/sh if not overridden by the build system. (This allows for use of this code without the build system, which was noted during the discussion as a good quality.[1]) The build always system will pass the value of SHELL_PATH, which default to /bin/sh as well.

[1] http://thread.gmane.org/gmane.comp.version-control.git/193866/focus=194018

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 Makefile      |    2 ++
 run-command.c |    6 +++++-
 2 files changed, 7 insertions(+), 1 deletions(-)

Ben Walton <bwalton@artsci.utoronto.ca> writes:

This line of reasoning makes me feel uneasy.

The approach may allow a directory that has "insane" tools in it to be on the SANE_TOOL_PATH for *this* particular codepath, but at the same time, it encourages people to build Git with such a broken SANE_TOOL_PATH.

Aren't you exposing your users to potential breakage in other parts of the system, caused by broken tools on SANE_TOOL_PATH that is not shell, by doing so? Worse yet, there is no escape hatch similar to SHELL_PATH for such tools.

Will queue on 'pu' for now, but I am not convinced.

Junio C Hamano wrote:

[...]

I think the missing detail in the above explanation is that "sh" is a special case. Even though POSIX and good sense would require "sh" to name a reasonable shell just like "grep" needs to name a reasonable grep, on Solaris we have never been able to count on an "sh" for git's needs and common practice seems to be to give up and just use bash or ksh.

See v1.5.5-rc0~5^2~3 (filter-branch: use $SHELL_PATH instead of 'sh', 2008-03-12), for example.

Ben Walton wrote:

[...]

The phrase "builtin commands" left me confused for a moment --- busybox-style builtins and standalone commands in C both don't get the PATH fixup.

I think you meant scripted commands (i.e., commands that get the PATH fixup by sourcing git-sh-setup).

As Junio mentioned, the only insane tool that SANE_TOOL_PATH is allowed to point to is sh. I guess the note on insane tools was meant as a wistful observation, but it didn't come through clearly.

Maybe a reference to v1.5.5-rc0~5^2~3 (filter-branch: use $SHELL_PATH instead of 'sh', 2008-03-12) would make the motivation behind the "don't trust sh" principle clearer.

Makes sense to me and the patch looks good. Thanks for your perseverance.

Ciao, Jonathan

Excerpts from Jeff King's message of Wed Mar 28 00:22:15 -0400 2012:

Ok, I just sent the updated patch. Thanks to everyone that has provided feedback on this so far.

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

Hi again,

Ben Walton wrote:

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

Jonathan Nieder wrote:

[...]

	The cause: that test and some subsequent ones set the pager ...

(The text has subject/verb agreement problems if the reader chooses to follow the parentheses. Removing parens, s/sets/set to spare the reader the choice.)

	                   ... to "sed s/^/foo:/ >actual", which is fine
	in a POSIX-compliant sh but not in Solaris's sh.

(missing comma before "which"; extra comma before "but").

[...]

Git uses _XOPEN_SOURCE so it's clearer to say that instead.

[...]

Punctuation: script's should be scripts'.

Sorry for the noise.

Excerpts from Jeff King's message of Mon Mar 26 14:24:27 -0400 2012:

Hi Jeff,

Ok, thanks for the great feedback. I just sent an updated version of the patch with the squash done and what I hope is a better commit message.

I'd need to jump through some hoops to do an sh-symlink that was within our packaging standards, although it's not impossible. Using "bash -posix" is likely a better path for me to travel. I'll still need to plan out the change with lots of notice for users in case there have been bashism's used in local scripts, but as you say it's not likely hurting anyone in practice.

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

Ben Walton wrote:

[...]

Currently git uses bash on Windows by necessity. I suspect we could make a lot of people happy by dropping that dependency some day and using cmd.exe or something like powershell in places the user provides a script.

Context: [1]

Of course that's far in the future and not certain at all. ;-)

Ciao, Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/164656/focus=164716

On Sun, Mar 25, 2012 at 08:11:48PM -0500, Jonathan Nieder wrote:

I don't think that's an unreasonable assumption. Solaris /bin/sh is really a total piece of junk, and we have already gone to lengths to let users avoid it. We have SHELL_PATH, but we also have SANE_TOOL_PATH. This is a case that should have been caught by SANE_TOOL_PATH, but wasn't. So I think the problem is worth fixing, and just lets Solaris people enjoy the same reasonable assumption about having a working shell that other systems do.

I think that would be an OK solution, too. Though I wonder if using SHELL_PATH isn't simply easier for the user to get right (I seem to recall people finding SANE_TOOL_PATH confusing to set up in the past, but I have not personally used it myself).

I think they would be easier to understand squashed, too. If there were many users of the new macro to be converted individually, I would say it might make sense to introduce it in one commit, then convert each class of callsites separately. But here there is really only one user, so seeing the application can help understand the rationale for the definition.

-Peff

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

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

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.

Ben Walton wrote:

[...]

Oh, that's what you meant.

Git could unconditionally override or prefix the PATH with some value determined at build time, but we deliberately gave users more control than that. git-rebase.sh et al make use of various tools from the usual Unix toolset, expecting to find them on the PATH. If the user sets the PATH to point to the Plan 9 tools, say, then these scripts will break. Is that a problem?

More importantly: is it the same problem your patch fixes?

If "yes", then that is problematic. If the SANE_TOOL_PATH facility is broken, we should be fixing or eliminating it, not piling workarounds on top. If we want to say 'sh' is special, we should mean it.

[My personal answers are "no, give them rope" and "no, they are different problems", so I'm not too worried. ;-)]

Thanks for some food for thought. Jonathan