git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Johannes Sixt <j.sixt@viscovery.net>
To: Ben Walton <bwalton@artsci.utoronto.ca>
Cc: peff@peff.net, jrnieder@gmail.com, gitster@pobox.com,
	git@vger.kernel.org
Subject: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows
Date: Tue, 17 Apr 2012 09:03:21 +0200	[thread overview]
Message-ID: <4F8D15B9.70803@viscovery.net> (raw)
In-Reply-To: <1333157601-6458-1-git-send-email-bwalton@artsci.utoronto.ca>

From: Johannes Sixt <j6t@kdbg.org>

The recent change to use SHELL_PATH instead of "sh" to spawn shell commands
is not suited for Windows:

- The default setting, "/bin/sh", does not work when git has to run the
  shell because it is a POSIX style path, but not a proper Windows style
  path.

- If it worked, it would hard-code a position in the files system where
  the shell is expected, making git (more precisely, the POSIX toolset that
  is needed alongside git) non-relocatable. But we cannot sacrifice
  relocatability on Windows.

- Apart from that, even though the Makefile leaves SHELL_PATH set to
  "/bin/sh" for the Windows builds, the build system passes a mangled path
  to the compiler, and something like "D:/Src/msysgit/bin/sh" is used,
  which is doubly bad because it points to where /bin/sh resolves to on
  the system where git was built.

- Finally, the system's CreateProcess() function that is used under
  mingw.c's hood does not work with forward slashes and cannot find the
  shell.

Undo the earlier change on Windows.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Am 3/31/2012 3:33, schrieb Ben Walton:
> +#ifndef SHELL_PATH
> +# define SHELL_PATH "/bin/sh"
> +#endif
> +
>  struct child_to_clean {
>  	pid_t pid;
>  	struct child_to_clean *next;
> @@ -90,7 +94,7 @@ static const char **prepare_shell_cmd(const char **argv)
>  		die("BUG: shell command is empty");
>  
>  	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
> -		nargv[nargc++] = "sh";
> +		nargv[nargc++] = SHELL_PATH;
>  		nargv[nargc++] = "-c";
>  
>  		if (argc < 2)

This looked so obviously correct, that I carelessly did not test the
change. But given that the first two points addressed in the commit
message above are actually dealbreakers, I should have noticed this issue
much earlier.

-- Hannes

 run-command.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/run-command.c b/run-command.c
index 2af3e0f..e4edede 100644
--- a/run-command.c
+++ b/run-command.c
@@ -94,7 +94,11 @@ static const char **prepare_shell_cmd(const char **argv)
 		die("BUG: shell command is empty");
 
 	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
+#ifndef WIN32
 		nargv[nargc++] = SHELL_PATH;
+#else
+		nargv[nargc++] = "sh";
+#endif
 		nargv[nargc++] = "-c";
 
 		if (argc < 2)
-- 
1.7.10.1370.ga6f62.dirty

  parent reply	other threads:[~2012-04-17  7:03 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <7vvclmoit6.fsf@alter.siamese.dyndns.org>
2012-03-31  1:33 ` [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd Ben Walton
2012-03-31  3:48   ` Jonathan Nieder
2012-03-31  5:38     ` Junio C Hamano
2012-03-31  5:55   ` Jonathan Nieder
2012-03-31 17:49     ` Junio C Hamano
2012-03-31 18:04       ` Junio C Hamano
2012-04-17  7:03   ` Johannes Sixt [this message]
2012-04-17 13:45     ` [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows Ben Walton
2012-04-17 14:00       ` Johannes Sixt
2012-04-17 14:04         ` Ben Walton
2012-04-17 22:14     ` Jeff King
2012-04-18  5:39       ` Johannes Sixt
2012-04-18  7:27         ` Jeff King
2012-04-18 16:30         ` Junio C Hamano
2012-04-19  5:36           ` Johannes Sixt
2012-04-19  5:49             ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4F8D15B9.70803@viscovery.net \
    --to=j.sixt@viscovery.net \
    --cc=bwalton@artsci.utoronto.ca \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).