{"thread":{"id":"30055","subject":"[PATCH 2/2] Makefile: Set EXTRA_CPPFLAGS during the compilation of run-command","startedAt":"2012-03-25T12:31:34Z","lastAt":"2012-03-30T06:32:16Z","messageCount":29,"participants":["Ben Walton","Jonathan Nieder","Jeff King","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"187675","messageId":"1332678696-4001-1-git-send-email-bwalton@artsci.utoronto.ca","threadId":"30055","inReplyTo":null,"subject":"[PATCH 0/2] Make run-command.c honour SHELL_PATH","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-03-25T12:31:34Z","receivedAt":"2012-03-25T12:31:34Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Hi Jeff and Junio,\n\n[Others touched this file too, but it appears Jeff wrote the affected\nfunctionality.]\n\nI hit a glitch with t7006-pager while testing the 1.7.10 rc1/rc2\nbuilds for OpenCSW/Solaris that turned out to be a problem with the\nway run-command.c:prepare_shell_cmd was setting up external\nutilities.  It was hard coded to fork 'sh -c' instead of honouring the\nSHELL_PATH as set at build time.\n\nIn this case, the failing test was t7006-pager:command-specific\npager.  That test (and some subsequent ones) were setting the pager\ncommand used by git log to \"sed s/^/foo:/ >actual\" which is fine in a\nPOSIX-compliant sh, but not in Solaris' sh.  If the user PATH at\nruntime happened to allow the broken system sh used instead of a sane\nsh, the ^ is interpreted the same as[1] | and this caused sed to fail\nwith incomplete s/ command and a \"command not found: /foo:\" from the\nother forked process.\n\nTo mitigate this, the following patches introduce the macro SHELL_PATH\nfor use in run-command.c, defaulting to \"sh\" to preserve the current\nbehaviour and then cause the build system to provide the SHELL_PATH as\nset by the builder.  This means that all processed forked by\nrun-command will use the same interpreter as the shell scripts in the\ngit suite.\n\nI considered implementing a dynamically generated .h file for this,\nsimilar to common-cmds.h, but thought that was overkill at the current\ntime.  If you think that (or something else) is a better fit for the\nchange, let me know and I'll make the required adjustments.\n\nThanks\n-Ben\n\n[1] http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/cmd/sh/cmd.c#184\n\n\nBen Walton (2):\n  run-command.c: Define SHELL_PATH macro for use in prepare_shell_cmd\n  Makefile: Set EXTRA_CPPFLAGS during the compilation of run-command\n\n Makefile      |    2 ++\n run-command.c |    6 +++++-\n 2 files changed, 7 insertions(+), 1 deletions(-)\n\n-- \n1.7.5.4\n"},{"id":"187674","messageId":"1332678696-4001-2-git-send-email-bwalton@artsci.utoronto.ca","threadId":"30055","inReplyTo":"1332678696-4001-1-git-send-email-bwalton@artsci.utoronto.ca","subject":"[PATCH 1/2] run-command.c: Define SHELL_PATH macro for use in prepare_shell_cmd","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-03-25T12:31:35Z","receivedAt":"2012-03-25T12:31:35Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"The shell spawned in run-command.c:prepare_shell_cmd was hard coded to\n'sh'.  Instead, make this a macro named SHELL_PATH so that it can be\noverridden by the build system.  Use 'sh' as the default to preserve\noriginal behaviour and ensure that a value is always set.\n\nThis avoids a situation where some commands were spawned using a\ndifferent shell than the one configured at build time.  Previously, it\nwas possible for things to be executed by a non-POSIX shell depending\non the user's PATH.\n\nSigned-off-by: Ben Walton <bwalton@artsci.utoronto.ca>\n---\n run-command.c |    6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 1db8abf..f005a31 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -4,6 +4,10 @@\n #include \"sigchain.h\"\n #include \"argv-array.h\"\n \n+#ifndef SHELL_PATH\n+# define SHELL_PATH \"sh\"\n+#endif\n+\n struct child_to_clean {\n \tpid_t pid;\n \tstruct child_to_clean *next;\n@@ -90,7 +94,7 @@ static const char **prepare_shell_cmd(const char **argv)\n \t\tdie(\"BUG: shell command is empty\");\n \n \tif (strcspn(argv[0], \"|&;<>()$`\\\\\\\"' \\t\\n*?[#~=%\") != strlen(argv[0])) {\n-\t\tnargv[nargc++] = \"sh\";\n+\t\tnargv[nargc++] = SHELL_PATH;\n \t\tnargv[nargc++] = \"-c\";\n \n \t\tif (argc < 2)\n-- \n1.7.5.4\n"},{"id":"187673","messageId":"1332678696-4001-3-git-send-email-bwalton@artsci.utoronto.ca","threadId":"30055","inReplyTo":"1332678696-4001-1-git-send-email-bwalton@artsci.utoronto.ca","subject":"[PATCH 2/2] Makefile: Set EXTRA_CPPFLAGS during the compilation of run-command","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-03-25T12:31:36Z","receivedAt":"2012-03-25T12:31:36Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"When building run-command.o, we want to pass in a define for\nSHELL_PATH to ensure that the shell configured at build time is used\nat run time when spawning external commands.\n\nSigned-off-by: Ben Walton <bwalton@artsci.utoronto.ca>\n---\n Makefile |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex be1957a..344e2e0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1913,6 +1913,8 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \\\n \t'-DGIT_MAN_PATH=\"$(mandir_SQ)\"' \\\n \t'-DGIT_INFO_PATH=\"$(infodir_SQ)\"'\n \n+run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='\"$(SHELL_PATH)\"'\n+\n $(BUILT_INS): git$X\n \t$(QUIET_BUILT_IN)$(RM) $@ && \\\n \tln git$X $@ 2>/dev/null || \\\n-- \n1.7.5.4\n"},{"id":"187697","messageId":"20120326011148.GA4428@burratino","threadId":"30055","inReplyTo":"1332678696-4001-1-git-send-email-bwalton@artsci.utoronto.ca","subject":"Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-26T01:11:48Z","receivedAt":"2012-03-26T01:11:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Ben,\n\nGood catch.  A few comments:\n\nBen Walton wrote:\n\n> In this case, the failing test was t7006-pager:command-specific\n> pager.  That test (and some subsequent ones) were setting the pager\n> command used by git log to \"sed s/^/foo:/ >actual\" which is fine in a\n> POSIX-compliant sh, but not in Solaris' sh.  If the user PATH at\n> runtime happened to allow the broken system sh used instead of a sane\n> sh, the ^ is interpreted the same as[1] | and this caused sed to fail\n> with incomplete s/ command and a \"command not found: /foo:\" from the\n> other forked process.\n\nWhen I first read the corresponding patch without reading this cover\nletter I was mystified.  Until I saw the above paragraph, I did not\neven see what problem was being solved.  The above paragraph should\nprobably be part of the commit message.\n\nOk, on to the proposed solution. ;-)\n\nMy first reaction was to suspect the series is solving the problem in\nthe wrong place.  The core of the bug might be t7006 itself, which\nassumes that the shell used to interpret the GIT_PAGER setting is a\nPOSIX-style shell rather than an ancient Bourne shell or cmd.exe.\nIn the far long term, we should probably skip this test on some\nplatforms using an appropriate test prerequisite.\n\nTo put it another way, the RUN_USING_SHELL magic is just supposed to\nbe a more featureful way to do what system() normally does.  What\nshell does system() use on Solaris?\n\nA second reaction was to wonder why the usual fixup from\nv1.6.4-rc0~66^2~1 (Makefile: insert SANE_TOOL_PATH to PATH before /bin\nor /usr/bin, 2009-07-08) didn't apply.  Should the git wrapper prepend\nthe same magic to $PATH that git-sh-setup.sh does to make the behavior\nof scripted and unscripted commands a little more consistent?\n\nA third reaction was that git_pager in the sh-setup library uses the\neval builtin, so we are already assuming that GIT_PAGER is appropriate\ninput for a POSIX-style shell.  So maybe the approach you've adopted\nis appropriate after all, at least in the short term while we require\na POSIX-style shell elsewhere in git.\n\nA few added words in the commit message could save the next reader\nfrom going through so long a thought process before seeing why what\nthe patch does is the right thing to do.\n\nA more minor comment: patch 1/2 was even more mysterious.  Combining\nthe two patches would be enough to avoid confusion there.  I haven't\nchecked the makefile changes and interaction with GIT-CFLAGS carefully\nyet and hope to do so in the next round.\n\nThanks for working on this.\n\nSincerely,\nJonathan\n"},{"id":"187720","messageId":"1332768900-sup-4533@pinkfloyd.chass.utoronto.ca","threadId":"30055","inReplyTo":"20120326011148.GA4428@burratino","subject":"Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-03-26T13:38:45Z","receivedAt":"2012-03-26T13:38:45Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Excerpts from Jonathan Nieder's message of Sun Mar 25 21:11:48 -0400 2012:\n\nHi Jonathan,\n\n> > In this case, the failing test was t7006-pager:command-specific\n> > pager.  That test (and some subsequent ones) were setting the pager\n> > command used by git log to \"sed s/^/foo:/ >actual\" which is fine in a\n> > POSIX-compliant sh, but not in Solaris' sh.  If the user PATH at\n> > runtime happened to allow the broken system sh used instead of a sane\n> > sh, the ^ is interpreted the same as[1] | and this caused sed to fail\n> > with incomplete s/ command and a \"command not found: /foo:\" from the\n> > other forked process.\n> \n> When I first read the corresponding patch without reading this cover\n> letter I was mystified.  Until I saw the above paragraph, I did not\n> even see what problem was being solved.  The above paragraph should\n> probably be part of the commit message.\n\nOk, that's fair.  I thought the commit message was self-contained\nenough to justify the change, but point taken. :)  I'll redo the\ncommit log and squash the two patches as per your later points.\n\n> My first reaction was to suspect the series is solving the problem\n> in the wrong place.  The core of the bug might be t7006 itself,\n> which assumes that the shell used to interpret the GIT_PAGER setting\n> is a POSIX-style shell rather than an ancient Bourne shell or\n> cmd.exe.  In the far long term, we should probably skip this test on\n> some platforms using an appropriate test prerequisite.\n\nI started looking through t7006 first as well, thinking that some\nnon-POSIX syntax had slipped in... This raises a good point though.\nI'd come at this thinking that things _should_ be able to rely on a\nPOSIX shell (or a modern standard, anyway) for things like this.  Is\nthere any sort of defined expectation for this?\n\n>From my point of view, I'd expect the command to be spawned under\nsomething sane on the current platform.  I haven't looked for this\nspecifically, but this code path would still try to fork 'sh' on\nWindows as far as my understanding of it goes.  I don't use git there,\nso I don't know if part of the installation sets up a usable sh\ntoo.\n\n> To put it another way, the RUN_USING_SHELL magic is just supposed to\n> be a more featureful way to do what system() normally does.  What\n> shell does system() use on Solaris?\n\nThis depends on some macros.  The default is /usr/bin/sh, but if\nvarious XOPEN-type macros are set, it will use /usr/xpg4/bin/sh.\n\n> A second reaction was to wonder why the usual fixup from\n> v1.6.4-rc0~66^2~1 (Makefile: insert SANE_TOOL_PATH to PATH before\n> /bin or /usr/bin, 2009-07-08) didn't apply.  Should the git wrapper\n> prepend the same magic to $PATH that git-sh-setup.sh does to make\n> the behavior of scripted and unscripted commands a little more\n> consistent?\n\nI did some poking at this before creating my patch as that is what I'd\nexpected too.  It would likely be a good idea, but in my case, even\nthat wouldn't help.\n\nI add /opt/csw/gnu:/opt/csw/bin:/usr/xpg4/bin to the SANE_TOOL_PATH\nbut set the SHELL_PATH to /opt/csw/bin/bash.  Without my patch, but\nwith SANE_TOOL_PATH honoured, I'd still see /opt/csw/bin/sh forked and\nthat sh is crippled too.\n\nSo that leads me to think that if we're going to fork a shell, it\nshould be one that we know to be good...if the builder has provided\nthat value.  I think you agree with this based on your next comment.\n\n> A third reaction was that git_pager in the sh-setup library uses the\n> eval builtin, so we are already assuming that GIT_PAGER is\n> appropriate input for a POSIX-style shell.  So maybe the approach\n> you've adopted is appropriate after all, at least in the short term\n> while we require a POSIX-style shell elsewhere in git.\n\nI'm unclear what you're meaning by this.  Are you implying that the\nrequirement for a POSIX-style shell should be relaxes to the point\nwhere things don't rely on that base set of functionality?\n\n> A few added words in the commit message could save the next reader\n> from going through so long a thought process before seeing why what\n> the patch does is the right thing to do.\n\nOk, I can re-do the commit message to incorporate notes about the\nbroken text and some of your rationale above.\n\n> A more minor comment: patch 1/2 was even more mysterious.  Combining\n> the two patches would be enough to avoid confusion there.  I haven't\n> checked the makefile changes and interaction with GIT-CFLAGS\n> carefully yet and hope to do so in the next round.\n\nTo me they were two logically separate changes, so I split them.  I\ndon't mind squashing it together though.\n\nI'll resubmit later today.\n\nThanks\n-Ben\n--\nBen Walton\nSystems Programmer - CHASS\nUniversity of Toronto\nC:416.407.5610 | W:416.978.4302\n"},{"id":"187750","messageId":"20120326175843.GF7942@sigill.intra.peff.net","threadId":"30055","inReplyTo":"1332678696-4001-1-git-send-email-bwalton@artsci.utoronto.ca","subject":"Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-26T17:58:43Z","receivedAt":"2012-03-26T17:58:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 25, 2012 at 08:31:34AM -0400, Ben Walton wrote:\n\n> [Others touched this file too, but it appears Jeff wrote the affected\n> functionality.]\n\nSort of. I was just refactoring what was happening elsewhere in the\ncode, where individual callsites were sitting \"sh -c\" manually into the\nargv list. So I didn't actually give any thought to whether SHELL_PATH\nshould be used. :)\n\n> I hit a glitch with t7006-pager while testing the 1.7.10 rc1/rc2\n> builds for OpenCSW/Solaris that turned out to be a problem with the\n> way run-command.c:prepare_shell_cmd was setting up external\n> utilities.  It was hard coded to fork 'sh -c' instead of honouring the\n> SHELL_PATH as set at build time.\n> \n> In this case, the failing test was t7006-pager:command-specific\n> pager.  That test (and some subsequent ones) were setting the pager\n> command used by git log to \"sed s/^/foo:/ >actual\" which is fine in a\n> POSIX-compliant sh, but not in Solaris' sh.  If the user PATH at\n> runtime happened to allow the broken system sh used instead of a sane\n> sh, the ^ is interpreted the same as[1] | and this caused sed to fail\n> with incomplete s/ command and a \"command not found: /foo:\" from the\n> other forked process.\n\nThe original intent of SHELL_PATH is to give the full path, so it could\nbe used in places where PATH lookup is not an option (i.e., on\n#!-lines). Whereas the run-command use_shell option looks up \"sh\" in the\nPATH, so the \"right\" thing is to have your PATH set up to put a sane\n\"sh\" near the front.\n\nSo in that sense, your patch is unnecessary. On the other hand, it is\nvery unlikely to harm anyone[1], and it reduces the number of things the\nuser has to make sure are set up properly, so I think overall it's an\nimprovement, even if there is already another way to fix the problem.\n\n-Peff\n\n[1] I can't imagine anybody will be upset that we would use SHELL_PATH\n    instead of \"sh\". It could produce a difference of behavior if you\n    had a sane \"sh\" in your PATH, but SHELL_PATH points to the absolute\n    path of some other shell. But if your SHELL_PATH and the \"sh\" in\n    your PATH are so wildly divergent that you notice the difference, I\n    think you may have bigger problems.\n"},{"id":"187753","messageId":"20120326180802.GG7942@sigill.intra.peff.net","threadId":"30055","inReplyTo":"20120326011148.GA4428@burratino","subject":"Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-26T18:08:02Z","receivedAt":"2012-03-26T18:08:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 25, 2012 at 08:11:48PM -0500, Jonathan Nieder wrote:\n\n> My first reaction was to suspect the series is solving the problem in\n> the wrong place.  The core of the bug might be t7006 itself, which\n> assumes that the shell used to interpret the GIT_PAGER setting is a\n> POSIX-style shell rather than an ancient Bourne shell or cmd.exe.\n> In the far long term, we should probably skip this test on some\n> platforms using an appropriate test prerequisite.\n\nI don't think that's an unreasonable assumption. Solaris /bin/sh is\nreally a total piece of junk, and we have already gone to lengths to let\nusers avoid it. We have SHELL_PATH, but we also have SANE_TOOL_PATH.\nThis is a case that should have been caught by SANE_TOOL_PATH, but\nwasn't. So I think the problem is worth fixing, and just lets Solaris\npeople enjoy the same reasonable assumption about having a working shell\nthat other systems do.\n\n> A second reaction was to wonder why the usual fixup from\n> v1.6.4-rc0~66^2~1 (Makefile: insert SANE_TOOL_PATH to PATH before /bin\n> or /usr/bin, 2009-07-08) didn't apply.  Should the git wrapper prepend\n> the same magic to $PATH that git-sh-setup.sh does to make the behavior\n> of scripted and unscripted commands a little more consistent?\n\nI think that would be an OK solution, too. Though I wonder if using\nSHELL_PATH isn't simply easier for the user to get right (I seem to\nrecall people finding SANE_TOOL_PATH confusing to set up in the past,\nbut I have not personally used it myself).\n\n> A more minor comment: patch 1/2 was even more mysterious.  Combining\n> the two patches would be enough to avoid confusion there.  I haven't\n> checked the makefile changes and interaction with GIT-CFLAGS carefully\n> yet and hope to do so in the next round.\n\nI think they would be easier to understand squashed, too. If there were\nmany users of the new macro to be converted individually, I would say it\nmight make sense to introduce it in one commit, then convert each class\nof callsites separately. But here there is really only one user, so\nseeing the application can help understand the rationale for the\ndefinition.\n\n-Peff\n"},{"id":"187754","messageId":"20120326181238.GH7942@sigill.intra.peff.net","threadId":"30055","inReplyTo":"1332768900-sup-4533@pinkfloyd.chass.utoronto.ca","subject":"Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-26T18:12:38Z","receivedAt":"2012-03-26T18:12:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 26, 2012 at 09:38:45AM -0400, Ben Walton wrote:\n\n> > A second reaction was to wonder why the usual fixup from\n> > v1.6.4-rc0~66^2~1 (Makefile: insert SANE_TOOL_PATH to PATH before\n> > /bin or /usr/bin, 2009-07-08) didn't apply.  Should the git wrapper\n> > prepend the same magic to $PATH that git-sh-setup.sh does to make\n> > the behavior of scripted and unscripted commands a little more\n> > consistent?\n> \n> I did some poking at this before creating my patch as that is what I'd\n> expected too.  It would likely be a good idea, but in my case, even\n> that wouldn't help.\n> \n> I add /opt/csw/gnu:/opt/csw/bin:/usr/xpg4/bin to the SANE_TOOL_PATH\n> but set the SHELL_PATH to /opt/csw/bin/bash.  Without my patch, but\n> with SANE_TOOL_PATH honoured, I'd still see /opt/csw/bin/sh forked and\n> that sh is crippled too.\n> \n> So that leads me to think that if we're going to fork a shell, it\n> should be one that we know to be good...if the builder has provided\n> that value.  I think you agree with this based on your next comment.\n\nWhat is /opt/csw/bin/sh? Is it a symlink to bash (which would mean bash\nrunning in POSIX mode)? Or is it a lightweight shell like ash? Or\nsomething else?\n\nThe point of SHELL_PATH is to provide a POSIX shell.  Generally, bash\nbehavior is a superset of POSIX, so you will not run into any\nincompatibilities by running things with bash. But do be aware that you\nare slightly incompatible with the rest of the world (so things that\nwork for you, for example, might not work for people using git with the\nstock \"shell is /bin/sh\" configuration).\n\n-Peff\n"},{"id":"187755","messageId":"20120326181720.GG7599@burratino","threadId":"30055","inReplyTo":"1332768900-sup-4533@pinkfloyd.chass.utoronto.ca","subject":"Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-26T18:17:20Z","receivedAt":"2012-03-26T18:17:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ben Walton wrote:\n> Excerpts from Jonathan Nieder's message of Sun Mar 25 21:11:48 -0400 2012:\n\n[...]\n>>                                          at least in the short term\n>> while we require a POSIX-style shell elsewhere in git.\n>\n> I'm unclear what you're meaning by this.  Are you implying that the\n> requirement for a POSIX-style shell should be relaxes to the point\n> where things don't rely on that base set of functionality?\n\nCurrently git uses bash on Windows by necessity.  I suspect we could\nmake a lot of people happy by dropping that dependency some day and\nusing cmd.exe or something like powershell in places the user provides\na script.\n\nContext: [1]\n\nOf course that's far in the future and not certain at all. ;-)\n\nCiao,\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/164656/focus=164716\n"},{"id":"187757","messageId":"1332785666-sup-5513@pinkfloyd.chass.utoronto.ca","threadId":"30055","inReplyTo":"20120326181238.GH7942@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-03-26T18:19:54Z","receivedAt":"2012-03-26T18:19:54Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Excerpts from Jeff King's message of Mon Mar 26 14:12:38 -0400 2012:\n\n> What is /opt/csw/bin/sh? Is it a symlink to bash (which would mean\n> bash running in POSIX mode)? Or is it a lightweight shell like ash?\n> Or something else?\n\nThis is some version of the traditional solaris /bin/sh packaged as\npart of schilyutils (Joerg Schilling's utilities).  I'm really not\nsure why he packages it or what the differences from /bin/sh\nare...Asking him is on my todo list though.  I only just learned it\nexisted when I hit this glitch.\n\n> The point of SHELL_PATH is to provide a POSIX shell.  Generally,\n> bash behavior is a superset of POSIX, so you will not run into any\n> incompatibilities by running things with bash. But do be aware that\n> you are slightly incompatible with the rest of the world (so things\n> that work for you, for example, might not work for people using git\n> with the stock \"shell is /bin/sh\" configuration).\n\nThis was a conscious choice I made early in the packaging of git for\nOpenCSW.  The idea was that we had a shell that was also under our\npackaging control and 'sane' at the same time.  I realize it's a\nsuperset of the POSIX functionality.  It's something I should maybe\nchange, but I'd need to stage it slowly so as to avoid breakages.\n\nThanks\n-Ben\n--\nBen Walton\nSystems Programmer - CHASS\nUniversity of Toronto\nC:416.407.5610 | W:416.978.4302\n"},{"id":"187758","messageId":"20120326182427.GA10333@sigill.intra.peff.net","threadId":"30055","inReplyTo":"1332785666-sup-5513@pinkfloyd.chass.utoronto.ca","subject":"Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-26T18:24:27Z","receivedAt":"2012-03-26T18:24:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 26, 2012 at 02:19:54PM -0400, Ben Walton wrote:\n\n> Excerpts from Jeff King's message of Mon Mar 26 14:12:38 -0400 2012:\n> \n> > What is /opt/csw/bin/sh? Is it a symlink to bash (which would mean\n> > bash running in POSIX mode)? Or is it a lightweight shell like ash?\n> > Or something else?\n> \n> This is some version of the traditional solaris /bin/sh packaged as\n> part of schilyutils (Joerg Schilling's utilities).  I'm really not\n> sure why he packages it or what the differences from /bin/sh\n> are...Asking him is on my todo list though.  I only just learned it\n> existed when I hit this glitch.\n\nOK, that's gross. I would argue that your SANE_TOOL_PATH is not great,\nthen, because the \"sh\" is not sane. But that being said, this is exactly\nthe sort of thing I was talking about in my first message, which is that\nSANE_TOOL_PATH is hard to get right. If we can make things Just Work by\nusing SHELL_PATH, I don't see that as a bad thing.\n\n> > The point of SHELL_PATH is to provide a POSIX shell.  Generally,\n> > bash behavior is a superset of POSIX, so you will not run into any\n> > incompatibilities by running things with bash. But do be aware that\n> > you are slightly incompatible with the rest of the world (so things\n> > that work for you, for example, might not work for people using git\n> > with the stock \"shell is /bin/sh\" configuration).\n> \n> This was a conscious choice I made early in the packaging of git for\n> OpenCSW.  The idea was that we had a shell that was also under our\n> packaging control and 'sane' at the same time.  I realize it's a\n> superset of the POSIX functionality.  It's something I should maybe\n> change, but I'd need to stage it slowly so as to avoid breakages.\n\nIt sounds like using bash is probably the least bad thing to do, and\ndoing anything else would not be worth the complexity. You could use\n\"bash -posix\" (or an sh-symlink to bash), but in practice I doubt it is\nreally hurting anybody.\n\n-Peff\n"},{"id":"187803","messageId":"1332816078-26829-1-git-send-email-bwalton@artsci.utoronto.ca","threadId":"30055","inReplyTo":"20120326182427.GA10333@sigill.intra.peff.net","subject":"[PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-03-27T02:41:18Z","receivedAt":"2012-03-27T02:41:18Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"The shell spawned in run-command.c:prepare_shell_cmd was hard coded to\n\"sh.\"  This was breaking \"t7006-pager:command-specific pager\" when the\n\"sh\" found in the PATH happened to be a broken one such as Solaris'\n/usr/bin/sh.  (The breakage in this case was due to ^ being\ninterpreted the same as | which was seeing two processes forked\ninstead of a single sed process.)\n\nTo allow the build system to supply a value that is deemed sane, thus\nremoving variation injected by the local user environment, define a\nmacro named SHELL_PATH (to match the variable of the same name in the\nbuild system).  Use this macro instead of the literal \"sh\" when\npreparing a set of arguments to be forked.  Set \"sh\" as the default\nvalue of this macro if none is set through other means to preserve the\noriginal behaviour.\n\nThe Makefile now sets EXTRA_CPPFLAGS to a value that includes a\ndefinition of SHELL_PATH when building run_command.c.  This sees the\nsane shell passed to run_command.c and used when forking external\ncommands.\n\nLeveraging SANE_TOOL_PATH was considered here but had downsides that\nmay have made it an incomplete solution.  For example, some builds may\nuse bash as the shell and not want sh used at all.\n\nSigned-off-by: Ben Walton <bwalton@artsci.utoronto.ca>\n---\n Makefile      |    2 ++\n run-command.c |    6 +++++-\n 2 files changed, 7 insertions(+), 1 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex be1957a..344e2e0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1913,6 +1913,8 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \\\n \t'-DGIT_MAN_PATH=\"$(mandir_SQ)\"' \\\n \t'-DGIT_INFO_PATH=\"$(infodir_SQ)\"'\n \n+run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='\"$(SHELL_PATH)\"'\n+\n $(BUILT_INS): git$X\n \t$(QUIET_BUILT_IN)$(RM) $@ && \\\n \tln git$X $@ 2>/dev/null || \\\ndiff --git a/run-command.c b/run-command.c\nindex 1db8abf..f005a31 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -4,6 +4,10 @@\n #include \"sigchain.h\"\n #include \"argv-array.h\"\n \n+#ifndef SHELL_PATH\n+# define SHELL_PATH \"sh\"\n+#endif\n+\n struct child_to_clean {\n \tpid_t pid;\n \tstruct child_to_clean *next;\n@@ -90,7 +94,7 @@ static const char **prepare_shell_cmd(const char **argv)\n \t\tdie(\"BUG: shell command is empty\");\n \n \tif (strcspn(argv[0], \"|&;<>()$`\\\\\\\"' \\t\\n*?[#~=%\") != strlen(argv[0])) {\n-\t\tnargv[nargc++] = \"sh\";\n+\t\tnargv[nargc++] = SHELL_PATH;\n \t\tnargv[nargc++] = \"-c\";\n \n \t\tif (argc < 2)\n-- \n1.7.5.4\n"},{"id":"187804","messageId":"1332816105-sup-9131@pinkfloyd.chass.utoronto.ca","threadId":"30055","inReplyTo":"20120326182427.GA10333@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-03-27T02:45:41Z","receivedAt":"2012-03-27T02:45:41Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Excerpts from Jeff King's message of Mon Mar 26 14:24:27 -0400 2012:\n\nHi Jeff,\n\n> OK, that's gross. I would argue that your SANE_TOOL_PATH is not\n> great, then, because the \"sh\" is not sane. But that being said, this\n> is exactly the sort of thing I was talking about in my first\n> message, which is that SANE_TOOL_PATH is hard to get right. If we\n> can make things Just Work by using SHELL_PATH, I don't see that as a\n> bad thing.\n\nOk, thanks for the great feedback.  I just sent an updated version of\nthe patch with the squash done and what I hope is a better commit\nmessage.\n\n> It sounds like using bash is probably the least bad thing to do, and\n> doing anything else would not be worth the complexity. You could use\n> \"bash -posix\" (or an sh-symlink to bash), but in practice I doubt it\n> is really hurting anybody.\n\nI'd need to jump through some hoops to do an sh-symlink that was\nwithin our packaging standards, although it's not impossible.  Using\n\"bash -posix\" is likely a better path for me to travel.  I'll still\nneed to plan out the change with lots of notice for users in case\nthere have been bashism's used in local scripts, but as you say it's\nnot likely hurting anyone in practice.\n\nThanks\n-Ben\n--\nBen Walton\nSystems Programmer - CHASS\nUniversity of Toronto\nC:416.407.5610 | W:416.978.4302\n"},{"id":"187808","messageId":"20120327032917.GB17338@sigill.intra.peff.net","threadId":"30055","inReplyTo":"1332816078-26829-1-git-send-email-bwalton@artsci.utoronto.ca","subject":"Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-27T03:29:17Z","receivedAt":"2012-03-27T03:29:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 26, 2012 at 10:41:18PM -0400, Ben Walton wrote:\n\n> diff --git a/Makefile b/Makefile\n> index be1957a..344e2e0 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1913,6 +1913,8 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \\\n>  \t'-DGIT_MAN_PATH=\"$(mandir_SQ)\"' \\\n>  \t'-DGIT_INFO_PATH=\"$(infodir_SQ)\"'\n>  \n> +run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='\"$(SHELL_PATH)\"'\n> +\n\nThis should be $(SHELL_PATH_SQ), no?\n\n> diff --git a/run-command.c b/run-command.c\n> index 1db8abf..f005a31 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -4,6 +4,10 @@\n>  #include \"sigchain.h\"\n>  #include \"argv-array.h\"\n>  \n> +#ifndef SHELL_PATH\n> +# define SHELL_PATH \"sh\"\n> +#endif\n\nDoes this default ever kick in? The Makefile defaults SHELL_PATH to\n/bin/sh, so we will always end up with at least that.\n\nDoing so at least makes us consistent across builds, but I wonder if we\nshould leave it as \"sh\" on systems that do not set SHELL_PATH manually.\nExecuting \"sh\" via the PATH is the normal system() thing to do. I doubt\nmany people have an \"sh\" in their PATH ahead of /bin/sh, but if they do,\nwe are changing the behavior for them for no good reason.\n\nThe whole SHELL_PATH and SANE_TOOL_PATH mess is about helping people on\nless-abled systems, and I do not mind bending the usual conventions to\nmake things more convenient on those systems (e.g., by not doing the\nPATH lookup of the shell name). But it would be nice if that bending did\nnot affect people on more mainstream systems.\n\n-Peff\n"},{"id":"187809","messageId":"20120327033420.GC17338@sigill.intra.peff.net","threadId":"30055","inReplyTo":"20120327032917.GB17338@sigill.intra.peff.net","subject":"Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-27T03:34:20Z","receivedAt":"2012-03-27T03:34:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 26, 2012 at 11:29:17PM -0400, Jeff King wrote:\n\n> > +#ifndef SHELL_PATH\n> > +# define SHELL_PATH \"sh\"\n> > +#endif\n> \n> Does this default ever kick in? The Makefile defaults SHELL_PATH to\n> /bin/sh, so we will always end up with at least that.\n> \n> Doing so at least makes us consistent across builds, but I wonder if we\n> should leave it as \"sh\" on systems that do not set SHELL_PATH manually.\n> Executing \"sh\" via the PATH is the normal system() thing to do. I doubt\n> many people have an \"sh\" in their PATH ahead of /bin/sh, but if they do,\n> we are changing the behavior for them for no good reason.\n> \n> The whole SHELL_PATH and SANE_TOOL_PATH mess is about helping people on\n> less-abled systems, and I do not mind bending the usual conventions to\n> make things more convenient on those systems (e.g., by not doing the\n> PATH lookup of the shell name). But it would be nice if that bending did\n> not affect people on more mainstream systems.\n\nI think you could do that pretty easily with this in the Makefile:\n\ndiff --git a/Makefile b/Makefile\nindex be1957a..fcd6896 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -528,6 +528,9 @@ BINDIR_PROGRAMS_NO_X += git-cvsserver\n # Set paths to tools early so that they can be used for version tests.\n ifndef SHELL_PATH\n \tSHELL_PATH = /bin/sh\n+\tSHELL_NAME = sh\n+else\n+\tSHELL_NAME = $(SHELL_PATH)\n endif\n ifndef PERL_PATH\n \tPERL_PATH = /usr/bin/perl\n\nand then just pass and use $SHELL_NAME in run-command.c (I guess you'd\nhave to make a $SHELL_NAME_SQ, too).\n\nOne other option not discussed: what if you always just used the\nbasename of $SHELL_PATH? That would fix your problem (it would do \"bash\n-c\" instead of \"sh -c\"), while keeping the same PATH-lookup semantics\nthat exist now. The only setup it wouldn't help is somebody who uses\n/some/other/sh, then still has /bin in their PATH before /some/other.\n\n-Peff\n"},{"id":"187812","messageId":"20120327042620.GA22547@burratino","threadId":"30055","inReplyTo":"1332816078-26829-1-git-send-email-bwalton@artsci.utoronto.ca","subject":"Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-27T04:26:20Z","receivedAt":"2012-03-27T04:26:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi again,\n\nBen Walton wrote:\n\n> The shell spawned in run-command.c:prepare_shell_cmd was hard coded to\n> \"sh.\"  This was breaking \"t7006-pager:command-specific pager\" when the\n> \"sh\" found in the PATH happened to be a broken one such as Solaris'\n> /usr/bin/sh.  (The breakage in this case was due to ^ being\n> interpreted the same as | which was seeing two processes forked\n> instead of a single sed process.)\n\nBetter.  But the above is still focusing on what you changed rather\nthan the intended impact and potential fallout.  Remember, the commit\nlog is where most of git's design documents are stored.\n\nIsn't it something more like this?\n\n\tEver since <date or version>, the \"command-specific pager\" test\n\tfrom t7006-pager has not been passing on Solaris with the\n\tdefault value of PATH.\n\n\tThe cause: that test (and some subsequent ones) sets the pager\n\tcommand used by git log to \"sed s/^/foo:/ >actual\" which is fine\n\tin a POSIX-compliant sh, but not in Solaris' sh.  The\n\trun_command machinery uses plain \"sh\" as a shell and if the user\n\tPATH at runtime happens to put the ancient system sh before any\n\tmodern POSIX-style sh, the ^ is interpreted the same as | and\n\tchaos results.\n\n\tOr to put it another way, t7006 expects that the shell used to\n\tinterpret GIT_PAGER is a POSIX-style shell and run_command's\n\tshell feature fails to deliver.\n\n\tTo fix this:\n\n\ta. instead of launching a shell on its own, run_command could\n\t   call system() or popen().  This launches a POSIX shell on\n\t   Solaris as long as _POSIX_SOURCE is defined.\n\n\tb. the git wrapper could prepend SANE_TOOL_PATH to $PATH for\n\t   builtin commands for consistency with scripted commands that\n\t   do that using git-sh-setup.  If we're lucky, the first \"sh\"\n\t   command found in the new PATH would be a suitable shell,\n\t   though git's current scripts do not rely on that.\n\n\tc. the run_command machinery could use the same SHELL_PATH\n\t   shell that is used by the git filter-branch script and in\n\t   all script's #! lines.\n\n\tHowever:\n\n\t- (a) means losing the ability to open a bidirectional pipe to a\n\t  filter script.  It would also break git for Windows, where\n\t  system() uses cmd.exe instead of a POSIX-style shell (see\n\t  v1.7.5-rc0~144^2, \"alias: use run_command api to execute\n\t  aliases, 2011-01-07).\n\n\t- On Solaris systems with schilyutils in /opt/csw/bin at the\n\t  beginning of SANE_TOOL_PATH, (b) finds /opt/csw/bin/sh which\n\t  is another ancient Bourne-style shell with the same problem.\n\n\tSo let's go with (c).\n\n\tAfter this patch, $GIT_PAGER is interpreted by the same shell\n\tin scripted commands which use the git_pager function that uses\n\t\"eval\" and builtins which use the run_command machinery to\n\tlaunch the interpreter specified as SHELL_PATH when git was built.\n\nJust my two cents,\nJonathan\n"},{"id":"187813","messageId":"20120327044957.GB22547@burratino","threadId":"30055","inReplyTo":"20120327042620.GA22547@burratino","subject":"Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-27T04:49:57Z","receivedAt":"2012-03-27T04:49:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n[...]\n> \tThe cause: that test (and some subsequent ones) sets the pager\n\n\tThe cause: that test and some subsequent ones set the pager ...\n\n(The text has subject/verb agreement problems if the reader chooses to\nfollow the parentheses.  Removing parens, s/sets/set to spare the reader\nthe choice.)\n\n> \tcommand used by git log to \"sed s/^/foo:/ >actual\" which is fine\n> \tin a POSIX-compliant sh, but not in Solaris' sh.  The\n\n\t                   ... to \"sed s/^/foo:/ >actual\", which is fine\n\tin a POSIX-compliant sh but not in Solaris's sh.\n\n(missing comma before \"which\"; extra comma before \"but\").\n\n[...]\n> \ta. instead of launching a shell on its own, run_command could\n> \t   call system() or popen().  This launches a POSIX shell on\n> \t   Solaris as long as _POSIX_SOURCE is defined.\n\nGit uses _XOPEN_SOURCE so it's clearer to say that instead.\n\n[...]\n> \tc. the run_command machinery could use the same SHELL_PATH\n> \t   shell that is used by the git filter-branch script and in\n> \t   all script's #! lines.\n\nPunctuation: script's should be scripts'.\n\nSorry for the noise.\n"},{"id":"187814","messageId":"20120327050109.GC22547@burratino","threadId":"30055","inReplyTo":"20120327032917.GB17338@sigill.intra.peff.net","subject":"Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-27T05:01:10Z","receivedAt":"2012-03-27T05:01:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Does this default ever kick in? The Makefile defaults SHELL_PATH to\n> /bin/sh, so we will always end up with at least that.\n>\n> Doing so at least makes us consistent across builds, but I wonder if we\n> should leave it as \"sh\" on systems that do not set SHELL_PATH manually.\n> Executing \"sh\" via the PATH is the normal system() thing to do.\n\nIt's more common for system() to default to /bin/sh.\n\nI noticed the Makefile already doesn't do this sort of thing for\nmandir and htmldir, but do you think changes to SHELL_PATH should be\ntracked to force a rebuild when it changes?\n\n\tGIT-SHELL-PATH: FORCE\n\t\t@FLAGS='$(SHELL_PATH_SQ)'; \\\n\t\t    if test x\"$$FLAGS\" != x\"`cat GIT-SHELL-PATH 2>/dev/null`\" ; then \\\n\t\t\techo 1>&2 \"    * new shell path\"; \\\n\t\t\techo \"$$FLAGS\" >GIT-SHELL-PATH; \\\n\t\t    fi\n\n\t$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh GIT-SHELL-PATH\n\t\t...\n\n\t$(SCRIPT_LIB) : % : %.sh GIT-SHELL-PATH\n\t\t...\n\n\trun_command.sp run_command.o: GIT-SHELL-PATH\n\texec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \\\n\t\t'-DSHELL_PATH=\"$(SHELL_PATH_SQ)\"'\n"},{"id":"187816","messageId":"20120327051200.GA20897@sigill.intra.peff.net","threadId":"30055","inReplyTo":"20120327050109.GC22547@burratino","subject":"Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-27T05:12:00Z","receivedAt":"2012-03-27T05:12:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 27, 2012 at 12:01:10AM -0500, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > Does this default ever kick in? The Makefile defaults SHELL_PATH to\n> > /bin/sh, so we will always end up with at least that.\n> >\n> > Doing so at least makes us consistent across builds, but I wonder if we\n> > should leave it as \"sh\" on systems that do not set SHELL_PATH manually.\n> > Executing \"sh\" via the PATH is the normal system() thing to do.\n> \n> It's more common for system() to default to /bin/sh.\n\nHmm, you're right. I think I was fooled by the fact that it puts \"sh\"\ninto argv[0]. It doesn't seem to actually do a PATH lookup, though (at\nleast not on my system).\n\nIn that case, Ben's patch is actually making us _more_ normal and\nlike system(), which is probably a slight benefit (although it's still a\nbehavior change from what we've been doing).\n\n> I noticed the Makefile already doesn't do this sort of thing for\n> mandir and htmldir, but do you think changes to SHELL_PATH should be\n> tracked to force a rebuild when it changes?\n\nYeah, that would be nice.\n\nIn general, I wish adding these sorts of dependencies wasn't so manual\nand painful. I'm not sure of a good solution short of totally retooling\nour build system, though.\n\n-Peff\n"},{"id":"187823","messageId":"20120327055348.GA23186@burratino","threadId":"30055","inReplyTo":"20120327051200.GA20897@sigill.intra.peff.net","subject":"Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-27T05:53:48Z","receivedAt":"2012-03-27T05:53:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Tue, Mar 27, 2012 at 12:01:10AM -0500, Jonathan Nieder wrote:\n\n>> I noticed the Makefile already doesn't do this sort of thing for\n>> mandir and htmldir, but do you think changes to SHELL_PATH should be\n>> tracked to force a rebuild when it changes?\n>\n> Yeah, that would be nice.\n\nThanks.  On second thought, without the corresponding tracking for\nPERL_PATH, USE_GETTEXT_SCHEME, localedir, etc, it doesn't seem worth\nit.\n\n> In general, I wish adding these sorts of dependencies wasn't so manual\n> and painful. I'm not sure of a good solution short of totally retooling\n> our build system, though.\n\nThe Linux kernel build system has an interesting solution to this.  It\nkeeps track of the command line used to build each file and forces a\nrebuild when that command changes.\n\nCiao,\nJonathan\n"},{"id":"187827","messageId":"4F715CDA.2010500@viscovery.net","threadId":"30055","inReplyTo":"20120327032917.GB17338@sigill.intra.peff.net","subject":"Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-03-27T06:23:22Z","receivedAt":"2012-03-27T06:23:22Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/27/2012 5:29, schrieb Jeff King:\n> On Mon, Mar 26, 2012 at 10:41:18PM -0400, Ben Walton wrote:\n>> diff --git a/run-command.c b/run-command.c\n>> index 1db8abf..f005a31 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -4,6 +4,10 @@\n>>  #include \"sigchain.h\"\n>>  #include \"argv-array.h\"\n>>  \n>> +#ifndef SHELL_PATH\n>> +# define SHELL_PATH \"sh\"\n>> +#endif\n> \n> Does this default ever kick in? The Makefile defaults SHELL_PATH to\n> /bin/sh, so we will always end up with at least that.\n\nPeople do use this code outside git. Providing a fallback here would help\nthem.\n\n-- Hannes\n"},{"id":"187909","messageId":"1332901381-sup-4406@pinkfloyd.chass.utoronto.ca","threadId":"30055","inReplyTo":"20120327032917.GB17338@sigill.intra.peff.net","subject":"Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-03-28T02:46:15Z","receivedAt":"2012-03-28T02:46:15Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Excerpts from Jeff King's message of Mon Mar 26 23:29:17 -0400 2012:\n\nHi Jeff,\n\n> > +run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='\"$(SHELL_PATH)\"'\n> > +\n> \n> This should be $(SHELL_PATH_SQ), no?\n\nYes, you're right, it should be.\n\n> > +#ifndef SHELL_PATH\n> > +# define SHELL_PATH \"sh\"\n> > +#endif\n> \n> Does this default ever kick in? The Makefile defaults SHELL_PATH to\n> /bin/sh, so we will always end up with at least that.\n\nNot when using the build system, but as Hannes mentioned, there is\npotential for this to be used outside of the default build system, so\nI think having the fallback is a good defensive option.  Should it\nmaybe be set to /bin/sh though to be more consistent with system()?\n\n> The whole SHELL_PATH and SANE_TOOL_PATH mess is about helping people\n> on less-abled systems, and I do not mind bending the usual\n> conventions to make things more convenient on those systems (e.g.,\n> by not doing the PATH lookup of the shell name). But it would be\n> nice if that bending did not affect people on more mainstream\n> systems.\n\nGiven the rest of the discussion that happened, I think I understand\nthat my patch is actually ok with the following caveats:\n\n1. My commit message still needs work.\n2. Possibly change the default setting of the SHELL_PATH macro from\n   \"sh\" to \"/bin/sh\"\n3. Use the _SQ variant of SHELL_PATH.\n\n(The tracking of changes for SHELL_PATH is considered too heavy for\nnow when the other _PATH items aren't tracked the same way.  This\nmight make a nice separate patch series though, using the idea from\nthe kernel where individual commands are tracked.)\n\nDid I miss anything else?\n\nThanks\n-Ben\n--\nBen Walton\nSystems Programmer - CHASS\nUniversity of Toronto\nC:416.407.5610 | W:416.978.4302\n"},{"id":"187910","messageId":"20120328042215.GB30251@sigill.intra.peff.net","threadId":"30055","inReplyTo":"1332901381-sup-4406@pinkfloyd.chass.utoronto.ca","subject":"Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-28T04:22:15Z","receivedAt":"2012-03-28T04:22:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 27, 2012 at 10:46:15PM -0400, Ben Walton wrote:\n\n> > > +#ifndef SHELL_PATH\n> > > +# define SHELL_PATH \"sh\"\n> > > +#endif\n> > \n> > Does this default ever kick in? The Makefile defaults SHELL_PATH to\n> > /bin/sh, so we will always end up with at least that.\n> \n> Not when using the build system, but as Hannes mentioned, there is\n> potential for this to be used outside of the default build system, so\n> I think having the fallback is a good defensive option.  Should it\n> maybe be set to /bin/sh though to be more consistent with system()?\n\nYeah, I think making it \"/bin/sh\" is better. It's more obvious to people\nreading the code as part of git (since even though it is a repetition of\nthe default from the Makefile, at least it is the _same_ default), and\nif it does get reused, it's probably a better default.\n\n> Given the rest of the discussion that happened, I think I understand\n> that my patch is actually ok with the following caveats:\n> \n> 1. My commit message still needs work.\n> 2. Possibly change the default setting of the SHELL_PATH macro from\n>    \"sh\" to \"/bin/sh\"\n> 3. Use the _SQ variant of SHELL_PATH.\n\nYeah, I think so (not that I am the final word, but that at least\nmatches my perception of the discussion so far, too).\n\n> (The tracking of changes for SHELL_PATH is considered too heavy for\n> now when the other _PATH items aren't tracked the same way.  This\n> might make a nice separate patch series though, using the idea from\n> the kernel where individual commands are tracked.)\n\nAgreed.\n\n-Peff\n"},{"id":"188025","messageId":"1332977191-29069-1-git-send-email-bwalton@artsci.utoronto.ca","threadId":"30055","inReplyTo":"20120328042215.GB30251@sigill.intra.peff.net","subject":"[PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-03-28T23:26:31Z","receivedAt":"2012-03-28T23:26:31Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"During the testing of the 1.7.10 rc series on Solaris for OpenCSW, it\nwas discovered that t7006-pager was failing due to finding a bad \"sh\"\nin PATH after a call to execvp(\"sh\", ...).  This call was setup by\nrun_command.c:prepare_shell_cmd.\n\nThe SANE_TOOL_PATH in use at the time was lead by /opt/csw/gnu and\n/opt/csw/bin as used by OpenCSW packages so that OpenCSW packaged GNU\ntools are found instead of system versions.  A package named\nschilyutils (Joerg Schilling's utilities) was installed on the build\nsystem and it provided a version of the traditional Solaris\n/usr/bin/sh, as /opt/csw/bin/sh.  This version of \"sh\" contains many\nof the same problems as the traditional Solaris /usr/bin/sh.\n\nThe command-specific pager test failed due to the broken \"sh\" handling\n^ as a pipe character.  It tried to fork two processes when it\nencountered \"sed s/^/foo:/\" as the pager command.  This problem was\nentirely dependent on the PATH of the user at runtime.\n\nPossible fixes for this issue are:\n\n1. Use the standard system() or popen() which both launch a POSIX\n   shell on Solaris as long as _POSIX_SOURCE is defined.\n\n2. The git wrapper could prepend SANE_TOOL_PATH to PATH for\n   consistency with builtin commands.\n\n3. The run_command.c:prepare_shell_command() could use the same\n   SHELL_PATH that is in the #! line of all all scripts.\n\nOption 1 would preclude opening a bidirectional pipe to a filter\nscript and would also break git for Windows as cmd.exe is spawned from\nsystem() (cf. v1.7.5-rc0~144^2, \"alias: use run_command api to execute\naliases, 2011-01-07).\n\nOption 2 is voided by the same example that turned up this issue.\nSANE_TOOL_PATH might also include 'insane' tools.\n\nOption 3 is the best choice at this time.\n\nAfter this patch, $GIT_PAGER is interpreted by the same shell in\nscripted commands which use the git_pager function that uses \"eval\"\nand builtins which use the run_command machinery.\n\nThe default shell used by commands will be /bin/sh if not overridden\nby the build system.  (This allows for use of this code without the\nbuild system, which was noted during the discussion as a good\nquality.[1]) The build always system will pass the value of\nSHELL_PATH, which default to /bin/sh as well.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/193866/focus=194018\n\nSigned-off-by: Ben Walton <bwalton@artsci.utoronto.ca>\n---\n Makefile      |    2 ++\n run-command.c |    6 +++++-\n 2 files changed, 7 insertions(+), 1 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex be1957a..dea1f15 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1913,6 +1913,8 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \\\n \t'-DGIT_MAN_PATH=\"$(mandir_SQ)\"' \\\n \t'-DGIT_INFO_PATH=\"$(infodir_SQ)\"'\n \n+run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='\"$(SHELL_PATH_SQ)\"'\n+\n $(BUILT_INS): git$X\n \t$(QUIET_BUILT_IN)$(RM) $@ && \\\n \tln git$X $@ 2>/dev/null || \\\ndiff --git a/run-command.c b/run-command.c\nindex 1db8abf..2af3e0f 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -4,6 +4,10 @@\n #include \"sigchain.h\"\n #include \"argv-array.h\"\n \n+#ifndef SHELL_PATH\n+# define SHELL_PATH \"/bin/sh\"\n+#endif\n+\n struct child_to_clean {\n \tpid_t pid;\n \tstruct child_to_clean *next;\n@@ -90,7 +94,7 @@ static const char **prepare_shell_cmd(const char **argv)\n \t\tdie(\"BUG: shell command is empty\");\n \n \tif (strcspn(argv[0], \"|&;<>()$`\\\\\\\"' \\t\\n*?[#~=%\") != strlen(argv[0])) {\n-\t\tnargv[nargc++] = \"sh\";\n+\t\tnargv[nargc++] = SHELL_PATH;\n \t\tnargv[nargc++] = \"-c\";\n \n \t\tif (argc < 2)\n-- \n1.7.5.4\n"},{"id":"188026","messageId":"1332977205-sup-469@pinkfloyd.chass.utoronto.ca","threadId":"30055","inReplyTo":"20120328042215.GB30251@sigill.intra.peff.net","subject":"Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-03-28T23:28:03Z","receivedAt":"2012-03-28T23:28:03Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Excerpts from Jeff King's message of Wed Mar 28 00:22:15 -0400 2012:\n\n> Yeah, I think so (not that I am the final word, but that at least\n> matches my perception of the discussion so far, too).\n\nOk, I just sent the updated patch.  Thanks to everyone that has\nprovided feedback on this so far.\n\nThanks\n-Ben\n--\nBen Walton\nSystems Programmer - CHASS\nUniversity of Toronto\nC:416.407.5610 | W:416.978.4302\n"},{"id":"188030","messageId":"7v8vikrr5l.fsf@alter.siamese.dyndns.org","threadId":"30055","inReplyTo":"1332977191-29069-1-git-send-email-bwalton@artsci.utoronto.ca","subject":"Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-29T04:02:14Z","receivedAt":"2012-03-29T04:02:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Walton <bwalton@artsci.utoronto.ca> writes:\n\n> Possible fixes for this issue are:\n> ...\n> 2. The git wrapper could prepend SANE_TOOL_PATH to PATH for\n>    consistency with builtin commands.\n>\n> 3. The run_command.c:prepare_shell_command() could use the same\n>    SHELL_PATH that is in the #! line of all all scripts.\n> ...\n> Option 2 is voided by the same example that turned up this issue.\n> SANE_TOOL_PATH might also include 'insane' tools.\n>\n> Option 3 is the best choice at this time.\n\nThis line of reasoning makes me feel uneasy.\n\nThe approach may allow a directory that has \"insane\" tools in it to be on\nthe SANE_TOOL_PATH for *this* particular codepath, but at the same time,\nit encourages people to build Git with such a broken SANE_TOOL_PATH.\n\nAren't you exposing your users to potential breakage in other parts of the\nsystem, caused by broken tools on SANE_TOOL_PATH that is not shell, by\ndoing so?  Worse yet, there is no escape hatch similar to SHELL_PATH for\nsuch tools.\n\nWill queue on 'pu' for now, but I am not convinced.\n"},{"id":"188038","messageId":"20120329060936.GB2026@burratino","threadId":"30055","inReplyTo":"7v8vikrr5l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-29T06:09:36Z","receivedAt":"2012-03-29T06:09:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Ben Walton <bwalton@artsci.utoronto.ca> writes:\n\n>> Possible fixes for this issue are:\n>> ...\n>> 2. The git wrapper could prepend SANE_TOOL_PATH to PATH for\n>>    consistency with builtin commands.\n>>\n>> 3. The run_command.c:prepare_shell_command() could use the same\n>>    SHELL_PATH that is in the #! line of all all scripts.\n>> ...\n>> Option 2 is voided by the same example that turned up this issue.\n>> SANE_TOOL_PATH might also include 'insane' tools.\n[...]\n> This line of reasoning makes me feel uneasy.\n\nI think the missing detail in the above explanation is that \"sh\" is a\nspecial case.  Even though POSIX and good sense would require \"sh\" to\nname a reasonable shell just like \"grep\" needs to name a reasonable\ngrep, on Solaris we have never been able to count on an \"sh\" for git's\nneeds and common practice seems to be to give up and just use bash or\nksh.\n\nSee v1.5.5-rc0~5^2~3 (filter-branch: use $SHELL_PATH instead of 'sh',\n2008-03-12), for example.\n"},{"id":"188105","messageId":"20120329230043.GB27829@burratino","threadId":"30055","inReplyTo":"1332977191-29069-1-git-send-email-bwalton@artsci.utoronto.ca","subject":"Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-29T23:00:43Z","receivedAt":"2012-03-29T23:00:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ben Walton wrote:\n\n[...]\n> 2. The git wrapper could prepend SANE_TOOL_PATH to PATH for\n>    consistency with builtin commands.\n\nThe phrase \"builtin commands\" left me confused for a moment ---\nbusybox-style builtins and standalone commands in C both don't get the\nPATH fixup.\n\nI think you meant scripted commands (i.e., commands that get the PATH\nfixup by sourcing git-sh-setup).\n\n> 3. The run_command.c:prepare_shell_command() could use the same\n>    SHELL_PATH that is in the #! line of all all scripts.\n>\n> Option 1 would preclude opening a bidirectional pipe to a filter\n> script and would also break git for Windows as cmd.exe is spawned from\n> system() (cf. v1.7.5-rc0~144^2, \"alias: use run_command api to execute\n> aliases, 2011-01-07).\n>\n> Option 2 is voided by the same example that turned up this issue.\n> SANE_TOOL_PATH might also include 'insane' tools.\n\nAs Junio mentioned, the only insane tool that SANE_TOOL_PATH is\nallowed to point to is sh.  I guess the note on insane tools was meant\nas a wistful observation, but it didn't come through clearly.\n\nMaybe a reference to v1.5.5-rc0~5^2~3 (filter-branch: use $SHELL_PATH\ninstead of 'sh', 2008-03-12) would make the motivation behind the\n\"don't trust sh\" principle clearer.\n\n> Option 3 is the best choice\n\nMakes sense to me and the patch looks good.  Thanks for your\nperseverance.\n\nCiao,\nJonathan\n"},{"id":"188129","messageId":"20120330063216.GC7220@burratino","threadId":"30055","inReplyTo":"1333073831-sup-5734@pinkfloyd.chass.utoronto.ca","subject":"Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-30T06:32:16Z","receivedAt":"2012-03-30T06:32:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ben Walton wrote:\n\n> The issue really comes down to the fact the current code allows a\n> user's environment at runtime to negatively affect the ability to run\n> commands that would otherwise be fine.\n[...]\n> The way that SANE_TOOL_PATH is injected into PATH for shell scripts\n> only sees it given priority over /bin and /usr/bin (and things\n> following them).  If this were mimicked for consistency in the\n> non-script parts of git, users would still be able to have paths with\n> 'insane' tools given precedence.\n\nOh, that's what you meant.\n\nGit could unconditionally override or prefix the PATH with some value\ndetermined at build time, but we deliberately gave users more control\nthan that.  git-rebase.sh et al make use of various tools from the\nusual Unix toolset, expecting to find them on the PATH.  If the user\nsets the PATH to point to the Plan 9 tools, say, then these scripts\nwill break.  Is that a problem?\n\nMore importantly: is it the same problem your patch fixes?\n\nIf \"yes\", then that is problematic.  If the SANE_TOOL_PATH facility is\nbroken, we should be fixing or eliminating it, not piling workarounds\non top.  If we want to say 'sh' is special, we should mean it.\n\n[My personal answers are \"no, give them rope\" and \"no, they are\ndifferent problems\", so I'm not too worried. ;-)]\n\nThanks for some food for thought.\nJonathan\n"}]}