From: Ben Walton <hidden> Date: 2016-06-15 22:53:23
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(-)
--
1.7.5.4
From: Ben Walton <hidden> Date: 2016-06-15 22:53:23
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 <redacted>
---
Makefile | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
From: Ben Walton <hidden> Date: 2016-06-15 22:53:23
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 <redacted>
---
run-command.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:53:23
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
From: Ben Walton <hidden> Date: 2016-06-15 22:53:23
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
From: Jeff King <hidden> Date: 2016-06-15 22:53:23
On Sun, Mar 25, 2012 at 08:31:34AM -0400, Ben Walton wrote:
[Others touched this file too, but it appears Jeff wrote the affected
functionality.]
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. :)
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.
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.
From: Jeff King <hidden> Date: 2016-06-15 22:53:23
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
From: Jeff King <hidden> Date: 2016-06-15 22:53:23
On Mon, Mar 26, 2012 at 09:38:45AM -0400, Ben Walton wrote:
quoted
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.
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
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:53:23
Ben Walton wrote:
Excerpts from Jonathan Nieder's message of Sun Mar 25 21:11:48 -0400 2012:
[...]
quoted
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?
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
From: Ben Walton <hidden> Date: 2016-06-15 22:53:23
Excerpts from Jeff King's message of Mon Mar 26 14:12:38 -0400 2012:
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?
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.
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).
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
From: Jeff King <hidden> Date: 2016-06-15 22:53:23
On Mon, Mar 26, 2012 at 02:19:54PM -0400, Ben Walton wrote:
Excerpts from Jeff King's message of Mon Mar 26 14:12:38 -0400 2012:
quoted
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?
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.
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.
quoted
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).
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.
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
From: Ben Walton <hidden> Date: 2016-06-15 22:53:23
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 <redacted>
---
Makefile | 2 ++
run-command.c | 6 +++++-
2 files changed, 7 insertions(+), 1 deletions(-)
From: Ben Walton <hidden> Date: 2016-06-15 22:53:23
Excerpts from Jeff King's message of Mon Mar 26 14:24:27 -0400 2012:
Hi Jeff,
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.
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.
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.
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
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
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.
I think you could do that pretty easily with this in the Makefile:
@@ -528,6 +528,9 @@ BINDIR_PROGRAMS_NO_X += git-cvsserver# Set paths to tools early so that they can be used for version tests.ifndef SHELL_PATHSHELL_PATH=/bin/sh+SHELL_NAME=sh+else+SHELL_NAME=$(SHELL_PATH)endififndef PERL_PATHPERL_PATH=/usr/bin/perl
and then just pass and use $SHELL_NAME in run-command.c (I guess you'd
have to make a $SHELL_NAME_SQ, too).
One other option not discussed: what if you always just used the
basename of $SHELL_PATH? That would fix your problem (it would do "bash
-c" instead of "sh -c"), while keeping the same PATH-lookup semantics
that exist now. The only setup it wouldn't help is somebody who uses
/some/other/sh, then still has /bin in their PATH before /some/other.
-Peff
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:53:23
Hi again,
Ben Walton wrote:
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.)
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
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:53:23
Jonathan Nieder wrote:
[...]
The cause: that test (and some subsequent ones) sets the pager
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.)
command used by git log to "sed s/^/foo:/ >actual" which is fine
in a POSIX-compliant sh, but not in Solaris' sh. The
... 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").
[...]
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.
Git uses _XOPEN_SOURCE so it's clearer to say that instead.
[...]
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.
Punctuation: script's should be scripts'.
Sorry for the noise.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:53:23
Jeff King wrote:
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.
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)"'
From: Jeff King <hidden> Date: 2016-06-15 22:53:23
On Tue, Mar 27, 2012 at 12:01:10AM -0500, Jonathan Nieder wrote:
Jeff King wrote:
quoted
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.
It's more common for system() to default to /bin/sh.
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).
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?
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
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:53:24
Jeff King wrote:
On Tue, Mar 27, 2012 at 12:01:10AM -0500, Jonathan Nieder wrote:
quoted
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?
Yeah, that would be nice.
Thanks. On second thought, without the corresponding tracking for
PERL_PATH, USE_GETTEXT_SCHEME, localedir, etc, it doesn't seem worth
it.
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.
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
Does this default ever kick in? The Makefile defaults SHELL_PATH to
/bin/sh, so we will always end up with at least that.
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()?
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.
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
Does this default ever kick in? The Makefile defaults SHELL_PATH to
/bin/sh, so we will always end up with at least that.
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()?
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.
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.
Yeah, I think so (not that I am the final word, but that at least
matches my perception of the discussion so far, too).
(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.)
From: Ben Walton <hidden> Date: 2016-06-15 22:53:25
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 <redacted>
---
Makefile | 2 ++
run-command.c | 6 +++++-
2 files changed, 7 insertions(+), 1 deletions(-)
From: Ben Walton <hidden> Date: 2016-06-15 22:53:25
Excerpts from Jeff King's message of Wed Mar 28 00:22:15 -0400 2012:
Yeah, I think so (not that I am the final word, but that at least
matches my perception of the discussion so far, too).
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
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:53:25
Ben Walton wrote:
[...]
2. The git wrapper could prepend SANE_TOOL_PATH to PATH for
consistency with builtin commands.
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).
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.
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.
Option 3 is the best choice
Makes sense to me and the patch looks good. Thanks for your
perseverance.
Ciao,
Jonathan