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

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

Hi Jonathan,

quoted

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

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.

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?

From my point of view, I'd expect the command to be spawned under

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.

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?

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.

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?

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.

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.

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?

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.

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

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.

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