git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ben Walton <bwalton@artsci.utoronto.ca>
Date
Mar 26, 2012, 13:38 UTC
Message-ID
<1332768900-sup-4533@pinkfloyd.chass.utoronto.ca>
In-Reply-To
<20120326011148.GA4428@burratino>
Excerpts from Jonathan Nieder's message of Sun Mar 25 21:11:48 -0400 2012:
Hi Jonathan,
Show 13 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, 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.

Show 6 quoted lines
> 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.

Show 6 quoted lines
> 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.

Show 5 quoted lines
> 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

Previous: Jonathan NiederNext: Jeff King
Message 5 of 29 in “Make run-command.c honour SHELL_PATH”
  1. 0/2 Make run-command.c honour SHELL_PATHBen Walton, Mar 25, 2012
  2. 1/2 run-command.c: Define SHELL_PATH macro for use in prepare_shell_cmdBen Walton, Mar 25, 2012
  3. 2/2 Makefile: Set EXTRA_CPPFLAGS during the compilation of run-commandBen Walton, Mar 25, 2012
  4. Jonathan NiederMar 26, 2012
  5. Ben WaltonMar 26, 2012
  6. Jeff KingMar 26, 2012
  7. Ben WaltonMar 26, 2012
  8. Jeff KingMar 26, 2012
  9. Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmdBen Walton, Mar 27, 2012
  10. Jeff KingMar 27, 2012
  11. Jeff KingMar 27, 2012
  12. Jonathan NiederMar 27, 2012
  13. Jeff KingMar 27, 2012
  14. Jonathan NiederMar 27, 2012
  15. Johannes SixtMar 27, 2012
  16. Ben WaltonMar 28, 2012
  17. Jeff KingMar 28, 2012
  18. Use SHELL_PATH from build system in run_command.c:prepare_shell_cmdBen Walton, Mar 28, 2012
  19. Junio C HamanoMar 29, 2012
  20. Jonathan NiederMar 29, 2012
  21. Jonathan NiederMar 29, 2012
  22. Ben WaltonMar 28, 2012
  23. Jonathan NiederMar 27, 2012
  24. Jonathan NiederMar 27, 2012
  25. Ben WaltonMar 27, 2012
  26. Jonathan NiederMar 26, 2012
  27. Jeff KingMar 26, 2012
  28. Jeff KingMar 26, 2012
  29. Jonathan NiederMar 30, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.