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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi Ben,

Good catch.  A few comments:

Ben Walton wrote:

> 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