{"thread":{"id":"30115","subject":"[PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd","startedAt":"2012-03-31T01:33:21Z","lastAt":"2012-04-19T05:49:43Z","messageCount":16,"participants":["Ben Walton","Jonathan Nieder","Junio C Hamano","Johannes Sixt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"188227","messageId":"1333157601-6458-1-git-send-email-bwalton@artsci.utoronto.ca","threadId":"30115","inReplyTo":"7vvclmoit6.fsf@alter.siamese.dyndns.org","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-31T01:33:21Z","receivedAt":"2012-03-31T01:33:21Z","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 PATH in use at the time saw /opt/csw/bin given precedence to\ntraditional Solaris paths such as /usr/bin and /usr/xpg4/bin.  A\npackage named schilyutils (Joerg Schilling's utilities) was installed\non the build system and it delivered a modified version of the\ntraditional Solaris /usr/bin/sh as /opt/csw/bin/sh.  This version of\nsh suffers from many of the same problems as /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 thus forcing\n   all unqualified commands run to use the known good tools on the\n   system.\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 and not rely\n   on PATH to find the sh to run.\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 not friendly to users as it would negate their ability to\nuse tools of their choice in many cases.  Alternately, injecting\nSANE_TOOL_PATH such that it takes precedence over /bin and /usr/bin\n(and anything with lower precedence than those paths) as\ngit-sh-setup.sh does would not solve the problem either as the user\nenvironment could still allow a bad sh to be found.  (Many OpenCSW\nusers will have /opt/csw/bin leading their PATH and some subset would\nhave schilyutils installed.)\n\nOption 3 allows us to use a known good shell while still honouring the\nusers' PATH for the utilities being run.  Thus, it solves the problem\nwhile not negatively impacting either users or git's ability to run\nexternal commands in convenient ways.  Essentially, the shell is a\nspecial case of tool that should not rely on SANE_TOOL_PATH and must\nbe called explicitly.\n\nWith this patch applied, any code path leading to\nrun_command.c:prepare_shell_cmd can count on using the same sane shell\nthat all shell scripts in the git suite use.  Both the build system\nand run_command.c will default this shell to /bin/sh unless\noverridden.\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":"188228","messageId":"20120331034802.GB2475@burratino","threadId":"30115","inReplyTo":"1333157601-6458-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-31T03:48:02Z","receivedAt":"2012-03-31T03:48:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ben Walton wrote:\n\n> During the testing of the 1.7.10 rc series on Solaris for OpenCSW, it\n> was discovered that t7006-pager was failing\n[...]\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\nThe underlying problem is an old one.  Thanks for fixing it.\n\nFor what it's worth,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\n[...]\n> Essentially, the shell is a\n> special case of tool that should not rely on SANE_TOOL_PATH and must\n> be called explicitly.\n\nIt is probably annoying to hear me say this, but:\n\nThe above doesn't tell me _why_ it is a special case and that on\nSolaris users have been burned by \"sh\" being the original Bourne shell\nor a temperamental version of ksh so SHELL_PATH usually points to bash\nor ksh93 instead.\n\nI trust the reader enough to fill in the blank, though, so I think the\npatch is ok as is.\n"},{"id":"188232","messageId":"7vd37tjpod.fsf@alter.siamese.dyndns.org","threadId":"30115","inReplyTo":"20120331034802.GB2475@burratino","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-31T05:38:10Z","receivedAt":"2012-03-31T05:38:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Ben Walton wrote:\n>\n> The underlying problem is an old one.  Thanks for fixing it.\n>\n> For what it's worth,\n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks, all of you who participated in the review process.\n"},{"id":"188234","messageId":"20120331055520.GA7939@burratino","threadId":"30115","inReplyTo":"1333157601-6458-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-31T05:55:20Z","receivedAt":"2012-03-31T05:55:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ben Walton wrote:\n\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\nSorry I forgot to send this before.  Quick tweaks:\n\n - let assembler listings (from \"make run-command.s\") and sparse checks\n   reflect the SHELL_PATH setting, too\n\n - the entire -DFOO=\"bar\" argument is surrounded with single quotes\n   in other EXTRA_CPPFLAGS settings, so let this one follow that\n   pattern to avoid standing out\n\ndiff --git i/Makefile w/Makefile\nindex dea1f157..a3791139 100644\n--- i/Makefile\n+++ w/Makefile\n@@ -1913,7 +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+run-command.sp run-command.s run-command.o: EXTRA_CPPFLAGS = \\\n+\t'-DSHELL_PATH=\"$(SHELL_PATH_SQ)\"'\n \n $(BUILT_INS): git$X\n \t$(QUIET_BUILT_IN)$(RM) $@ && \\\n"},{"id":"188256","messageId":"7v62dkk6dl.fsf@alter.siamese.dyndns.org","threadId":"30115","inReplyTo":"20120331055520.GA7939@burratino","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-31T17:49:42Z","receivedAt":"2012-03-31T17:49:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Sorry I forgot to send this before.  Quick tweaks:\n>\n>  - let assembler listings (from \"make run-command.s\") and sparse checks\n>    reflect the SHELL_PATH setting, too\n>\n>  - the entire -DFOO=\"bar\" argument is surrounded with single quotes\n>    in other EXTRA_CPPFLAGS settings, so let this one follow that\n>    pattern to avoid standing out\n>\n> diff --git i/Makefile w/Makefile\n> index dea1f157..a3791139 100644\n> --- i/Makefile\n> +++ w/Makefile\n> @@ -1913,7 +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> +run-command.sp run-command.s run-command.o: EXTRA_CPPFLAGS = \\\n> +\t'-DSHELL_PATH=\"$(SHELL_PATH_SQ)\"'\n>  \n>  $(BUILT_INS): git$X\n>  \t$(QUIET_BUILT_IN)$(RM) $@ && \\\n\nActually, I do not think this is sufficient, and it happens that you and I\nare the best position to realize it ;-).\n\nLook at what is done in the Makefile for DEFAULT_EDITOR and DEFAULT_PAGER,\nand compare with what the above is doing, and think why the EDITOR/PAGER\nneeds to have another level of quoting.\n"},{"id":"188258","messageId":"7vwr60ir3q.fsf@alter.siamese.dyndns.org","threadId":"30115","inReplyTo":"7v62dkk6dl.fsf@alter.siamese.dyndns.org","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-31T18:04:57Z","receivedAt":"2012-03-31T18:04:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n> ...\n>> -run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='\"$(SHELL_PATH_SQ)\"'\n>> +run-command.sp run-command.s run-command.o: EXTRA_CPPFLAGS = \\\n>> +\t'-DSHELL_PATH=\"$(SHELL_PATH_SQ)\"'\n>>  \n>>  $(BUILT_INS): git$X\n>>  \t$(QUIET_BUILT_IN)$(RM) $@ && \\\n>\n> Actually, I do not think this is sufficient, and it happens that you and I\n> are in the best position to realize it ;-).\n>\n> Look at what is done in the Makefile for DEFAULT_EDITOR and DEFAULT_PAGER,\n> and compare with what the above is doing, and think why the EDITOR/PAGER\n> needs to have another level of quoting.\n\nIn other words, something like this squashed into Ben's patch...\n\ndiff --git i/Makefile w/Makefile\nindex dea1f15..abee43e 100644\n--- i/Makefile\n+++ w/Makefile\n@@ -1849,6 +1849,13 @@ DEFAULT_PAGER_CQ_SQ = $(subst ','\\'',$(DEFAULT_PAGER_CQ))\n BASIC_CFLAGS += -DDEFAULT_PAGER='$(DEFAULT_PAGER_CQ_SQ)'\n endif\n \n+ifdef SHELL_PATH\n+SHELL_PATH_CQ = \"$(subst \",\\\",$(subst \\,\\\\,$(SHELL_PATH)))\"\n+SHELL_PATH_CQ_SQ = $(subst ','\\'',$(SHELL_PATH_CQ))\n+\n+BASIC_CFLAGS += -DSHELL_PATH='$(SHELL_PATH_CQ_SQ)'\n+endif\n+\n ALL_CFLAGS += $(BASIC_CFLAGS)\n ALL_LDFLAGS += $(BASIC_LDFLAGS)\n \n@@ -1913,8 +1920,6 @@ 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 || \\\n"},{"id":"189501","messageId":"4F8D15B9.70803@viscovery.net","threadId":"30115","inReplyTo":"1333157601-6458-1-git-send-email-bwalton@artsci.utoronto.ca","subject":"[PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-04-17T07:03:21Z","receivedAt":"2012-04-17T07:03:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Johannes Sixt <j6t@kdbg.org>\n\nThe recent change to use SHELL_PATH instead of \"sh\" to spawn shell commands\nis not suited for Windows:\n\n- The default setting, \"/bin/sh\", does not work when git has to run the\n  shell because it is a POSIX style path, but not a proper Windows style\n  path.\n\n- If it worked, it would hard-code a position in the files system where\n  the shell is expected, making git (more precisely, the POSIX toolset that\n  is needed alongside git) non-relocatable. But we cannot sacrifice\n  relocatability on Windows.\n\n- Apart from that, even though the Makefile leaves SHELL_PATH set to\n  \"/bin/sh\" for the Windows builds, the build system passes a mangled path\n  to the compiler, and something like \"D:/Src/msysgit/bin/sh\" is used,\n  which is doubly bad because it points to where /bin/sh resolves to on\n  the system where git was built.\n\n- Finally, the system's CreateProcess() function that is used under\n  mingw.c's hood does not work with forward slashes and cannot find the\n  shell.\n\nUndo the earlier change on Windows.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\nAm 3/31/2012 3:33, schrieb Ben Walton:\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\nThis looked so obviously correct, that I carelessly did not test the\nchange. But given that the first two points addressed in the commit\nmessage above are actually dealbreakers, I should have noticed this issue\nmuch earlier.\n\n-- Hannes\n\n run-command.c |    4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 2af3e0f..e4edede 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -94,7 +94,11 @@ 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+#ifndef WIN32\n \t\tnargv[nargc++] = SHELL_PATH;\n+#else\n+\t\tnargv[nargc++] = \"sh\";\n+#endif\n \t\tnargv[nargc++] = \"-c\";\n \n \t\tif (argc < 2)\n-- \n1.7.10.1370.ga6f62.dirty\n"},{"id":"189521","messageId":"1334670128-sup-4966@pinkfloyd.chass.utoronto.ca","threadId":"30115","inReplyTo":"4F8D15B9.70803@viscovery.net","subject":"Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-04-17T13:45:50Z","receivedAt":"2012-04-17T13:45:50Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Excerpts from Johannes Sixt's message of Tue Apr 17 03:03:21 -0400 2012:\n\nHi Hannes,\n\n> The recent change to use SHELL_PATH instead of \"sh\" to spawn shell\n> commands is not suited for Windows:\n\nAh yes, I can see how that would be undesirable on Windows.\n\n\n> +#ifndef WIN32\n>          nargv[nargc++] = SHELL_PATH;\n> +#else\n> +        nargv[nargc++] = \"sh\";\n> +#endif\n>          nargv[nargc++] = \"-c\";\n\nA minor style question: Would this conditional code not be better if\nit were up where the fallback SHELL_PATH is set?  That would leave\nthis code block #ifdef free while still avoiding the breakage on\nWindows.\n\nAdmittedly, it's a very minor point and possibly entirely a personal\npreference.\n\nThanks\n-Ben\n--\nBen Walton\nSystems Programmer - CHASS\nUniversity of Toronto\nC:416.407.5610 | W:416.978.4302\n"},{"id":"189522","messageId":"4F8D7779.9080506@viscovery.net","threadId":"30115","inReplyTo":"1334670128-sup-4966@pinkfloyd.chass.utoronto.ca","subject":"Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-04-17T14:00:25Z","receivedAt":"2012-04-17T14:00:25Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 4/17/2012 15:45, schrieb Ben Walton:\n> Excerpts from Johannes Sixt's message of Tue Apr 17 03:03:21 -0400 2012:\n>> The recent change to use SHELL_PATH instead of \"sh\" to spawn shell\n>> commands is not suited for Windows:\n> \n> Ah yes, I can see how that would be undesirable on Windows.\n> \n>> +#ifndef WIN32\n>>          nargv[nargc++] = SHELL_PATH;\n>> +#else\n>> +        nargv[nargc++] = \"sh\";\n>> +#endif\n>>          nargv[nargc++] = \"-c\";\n> \n> A minor style question: Would this conditional code not be better if\n> it were up where the fallback SHELL_PATH is set?  That would leave\n> this code block #ifdef free while still avoiding the breakage on\n> Windows.\n\nIt would require either to #define yet another new name to SHELL_PATH or\n\"sh\", or to #undef SHELL_PATH and re#define it for WIN32. Neither is very\nappealing. I personally prefer to have the conditional (if it is as short\nas this one) at the point where it matters.\n\n-- Hannes\n"},{"id":"189523","messageId":"1334671397-sup-5172@pinkfloyd.chass.utoronto.ca","threadId":"30115","inReplyTo":"4F8D7779.9080506@viscovery.net","subject":"Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-04-17T14:04:25Z","receivedAt":"2012-04-17T14:04:25Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Excerpts from Johannes Sixt's message of Tue Apr 17 10:00:25 -0400 2012:\n\n> It would require either to #define yet another new name to\n> SHELL_PATH or \"sh\", or to #undef SHELL_PATH and re#define it for\n> WIN32. Neither is very appealing. I personally prefer to have the\n> conditional (if it is as short as this one) at the point where it\n> matters.\n\nOk.  I'll defer to your more experienced judgement. :)  Thanks for\nclarifying though.\n\nThanks\n-Ben\n--\nBen Walton\nSystems Programmer - CHASS\nUniversity of Toronto\nC:416.407.5610 | W:416.978.4302\n"},{"id":"189579","messageId":"20120417221449.GC10797@sigill.intra.peff.net","threadId":"30115","inReplyTo":"4F8D15B9.70803@viscovery.net","subject":"Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-17T22:14:50Z","receivedAt":"2012-04-17T22:14:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 17, 2012 at 09:03:21AM +0200, Johannes Sixt wrote:\n\n> diff --git a/run-command.c b/run-command.c\n> index 2af3e0f..e4edede 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -94,7 +94,11 @@ 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> +#ifndef WIN32\n>  \t\tnargv[nargc++] = SHELL_PATH;\n> +#else\n> +\t\tnargv[nargc++] = \"sh\";\n> +#endif\n>  \t\tnargv[nargc++] = \"-c\";\n>  \n>  \t\tif (argc < 2)\n\nIt sounds like the real problem is not the use of a configurable shell,\nbut rather the use of an absolute path. Should you maybe try to pass the\nbasename of SHELL_PATH? Or maybe that is not even worth worrying about,\nas somebody on Windows is not going to ever set SHELL_PATH, since it is\nnot like they are working around a non-POSIX \"sh\" included with the\noperating system (which is why people on Solaris typically set\nSHELL_PATH).\n\n-Peff\n"},{"id":"189601","messageId":"4F8E539F.7030902@viscovery.net","threadId":"30115","inReplyTo":"20120417221449.GC10797@sigill.intra.peff.net","subject":"Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-04-18T05:39:43Z","receivedAt":"2012-04-18T05:39:43Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 4/18/2012 0:14, schrieb Jeff King:\n> On Tue, Apr 17, 2012 at 09:03:21AM +0200, Johannes Sixt wrote:\n> \n>> diff --git a/run-command.c b/run-command.c\n>> index 2af3e0f..e4edede 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -94,7 +94,11 @@ 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>> +#ifndef WIN32\n>>  \t\tnargv[nargc++] = SHELL_PATH;\n>> +#else\n>> +\t\tnargv[nargc++] = \"sh\";\n>> +#endif\n>>  \t\tnargv[nargc++] = \"-c\";\n>>  \n>>  \t\tif (argc < 2)\n> \n> It sounds like the real problem is not the use of a configurable shell,\n> but rather the use of an absolute path. Should you maybe try to pass the\n> basename of SHELL_PATH? Or maybe that is not even worth worrying about,\n> as somebody on Windows is not going to ever set SHELL_PATH, since it is\n> not like they are working around a non-POSIX \"sh\" included with the\n> operating system (which is why people on Solaris typically set\n> SHELL_PATH).\n\nI thought about offering a customization point, but decided that it is not\nworth the hassle: Most people download an installer, then the installer\ncan set up the PATH so that \"sh\" is not broken or something entirely\nunrelated. And those who build git themselves know sufficiently well what\nthey are doing.\n\n-- Hannes\n"},{"id":"189605","messageId":"20120418072615.GA2217@sigill.intra.peff.net","threadId":"30115","inReplyTo":"4F8E539F.7030902@viscovery.net","subject":"Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-18T07:27:50Z","receivedAt":"2012-04-18T07:27:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 18, 2012 at 07:39:43AM +0200, Johannes Sixt wrote:\n\n> > It sounds like the real problem is not the use of a configurable shell,\n> > but rather the use of an absolute path. Should you maybe try to pass the\n> > basename of SHELL_PATH? Or maybe that is not even worth worrying about,\n> > as somebody on Windows is not going to ever set SHELL_PATH, since it is\n> > not like they are working around a non-POSIX \"sh\" included with the\n> > operating system (which is why people on Solaris typically set\n> > SHELL_PATH).\n> \n> I thought about offering a customization point, but decided that it is not\n> worth the hassle: Most people download an installer, then the installer\n> can set up the PATH so that \"sh\" is not broken or something entirely\n> unrelated. And those who build git themselves know sufficiently well what\n> they are doing.\n\nOK. I'll defer your judgement on what Windows users prefer. I just\nwanted to make sure it was a conscious decision.\n\nOther than that, the patch looks obviously correct to me. Thanks, and\nsorry for inadvertently breaking git on windows (again). :)\n\n-Peff\n"},{"id":"189634","messageId":"7vobqpknoq.fsf@alter.siamese.dyndns.org","threadId":"30115","inReplyTo":"4F8E539F.7030902@viscovery.net","subject":"Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-18T16:30:13Z","receivedAt":"2012-04-18T16:30:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n>> It sounds like the real problem is not the use of a configurable shell,\n>> but rather the use of an absolute path. Should you maybe try to pass the\n>> basename of SHELL_PATH? ...\n>\n> I thought about offering a customization point, but decided that it is not\n> worth the hassle...\n\nI think you meant by the above that the installer could offer the user to\nchoose where to place the shell, and if that is the case, I agree it would\nprobably not worth it.\n\nThere is one thing I am confused about your response, though.  I thought\nthe suggestion by Peff was to build your binary with \"make SHELL_PATH=sh\"\n(not \"make SHELL_PATH=/bin/sh\").  I do not know if that works or does not\nwork (I do not see why not, though), but in either case offering a new\ncustomization point sounds like a separate issue.\n"},{"id":"189680","messageId":"4F8FA453.8050807@viscovery.net","threadId":"30115","inReplyTo":"7vobqpknoq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-04-19T05:36:19Z","receivedAt":"2012-04-19T05:36:19Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 4/18/2012 18:30, schrieb Junio C Hamano:\n> There is one thing I am confused about your response, though.  I thought\n> the suggestion by Peff was to build your binary with \"make SHELL_PATH=sh\"\n> (not \"make SHELL_PATH=/bin/sh\").  I do not know if that works or does not\n> work (I do not see why not, though), but in either case offering a new\n> customization point sounds like a separate issue.\n\nI tried this, and it does not work. (Shell scripts' shbang line would end\nup as '#!sh', and at least bash does not like to run such scripts.)\n\nThe alternative is to customize the -DSHELL_PATH=... compiler option for\nWindows, and that is primarily what I don't like: It would mean that\neither the name \"SHELL_PATH\" has different meaning in C code and the\nMakefile, or we need a new name for the C macro. (The latter would be \"a\nnew customization point\".)\n\nI learn from this conversation that I should note failed alternatives more\nfrequently in the commit message to save reviewers' time.\n\n-- Hannes\n"},{"id":"189681","messageId":"7v8vhsi83s.fsf@alter.siamese.dyndns.org","threadId":"30115","inReplyTo":"4F8FA453.8050807@viscovery.net","subject":"Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-19T05:49:43Z","receivedAt":"2012-04-19T05:49:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Am 4/18/2012 18:30, schrieb Junio C Hamano:\n>> There is one thing I am confused about your response, though.  I thought\n>> the suggestion by Peff was to build your binary with \"make SHELL_PATH=sh\"\n>> (not \"make SHELL_PATH=/bin/sh\").  I do not know if that works or does not\n>> work (I do not see why not, though), but in either case offering a new\n>> customization point sounds like a separate issue.\n>\n> I tried this, and it does not work. (Shell scripts' shbang line would end\n> up as '#!sh', and at least bash does not like to run such scripts.)\n\nAhh, sorry for missing the obvious.  I should have realized about that\none, as customizing that one was the very original motivation of the make\nvariable.\n\nIn any case, as I wrote in the \"What's cooking\", I'll fast-track the fix\nto 'master' shortly, so that Windows port does not have to carry it.\n\nThanks.\n"}]}