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

[Jeff King <peff@peff.net>]

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

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

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

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

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