Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR

[Luke Diamand <luke@diamand.org>]

On 07/05/15 23:16, Junio C Hamano wrote:
> Luke Diamand <luke@diamand.org> writes:
>

[Resurrecting old thread]

>
> Looking at run-command.c, GIT_WINDOES_NATIVE and POSIX seems to use
> pretty much the same construct, except that they use SHELL_PATH
> instead of "sh".

I think the state of git on Windows is a bit shaky (I'm happy to be 
proved wrong of course), but I think the only seriously active port is 
the msys one.

That, as far as I can tell, uses an msys version of 'sh', so it will be 
perfectly happy with the "sh -c ..." construct.

There may be a native windows port in existence, but I can't find how to 
build this, and I assume it's going to need Visual Studio, which makes 
it a lot more complex to get going.

The code you were looking at in run-command.c says this:

#ifndef GIT_WINDOWS_NATIVE
		nargv[nargc++] = SHELL_PATH;  <<<<< !GIT_WINDOWS_NATIVE
#else
		nargv[nargc++] = "sh";        <<<<< GIT_WINDOWS_NATIVE
#endif
		nargv[nargc++] = "-c";

To me, that seems to imply that for GIT_WINDOWS_NATIVE, we take the 
*second* branch and use "sh", so again, the the code as it stands will 
be fine. msysgit uses that path.

(The next line, trying to use "-c" has no chance of working if Cmd is 
being used).


>
> So something like this may be sufficient, perhaps?
>
>   Makefile  | 1 +
>   git-p4.py | 2 +-
>   2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 20058f1..fda44bf 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1776,6 +1776,7 @@ $(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS
>   $(SCRIPT_PYTHON_GEN): % : %.py
>   	$(QUIET_GEN)$(RM) $@ $@+ && \
>   	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
> +	    -e 's|SHELL_PATH|$(SHELL_PATH_SQ)|g' \
>   	    $< >$@+ && \
>   	chmod +x $@+ && \
>   	mv $@+ $@
> diff --git a/git-p4.py b/git-p4.py
> index de06046..eb6d4b1 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1220,7 +1220,7 @@ class P4Submit(Command, P4UserMap):
>               editor = os.environ.get("P4EDITOR")
>           else:
>               editor = read_pipe("git var GIT_EDITOR").strip()
> -        system(["sh", "-c", ('%s "$@"' % editor), editor, template_file])
> +        system(['''SHELL_PATH''', "-c", ('%s "$@"' % editor), editor, template_file])

This seems to be expanded to '''sh''' which doesn't then work at all. I 
didn't take the time to investigate further though.

>
>           # If the file was not saved, prompt to see if this patch should
>           # be skipped.  But skip this verification step if configured so.

I don't think we need to do anything. msysgit works fine with the origin 
"sh", "-c", ... code.

Thanks!
Luke