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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mar 26, 2012, 01:11 UTC
Message-ID: <20120326011148.GA4428@burratino>
In-Reply-To: <1332678696-4001-1-git-send-email-bwalton@artsci.utoronto.ca>

Hi Ben,

Good catch.  A few comments:

Ben Walton wrote:

Show 8 quoted lines

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

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