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

From: Junio C Hamano <gitster@pobox.com>
Date: May 7, 2015, 22:16 UTC
Message-ID: <xmqqtwvom3t4.fsf@gitster.dls.corp.google.com>
In-Reply-To: <554BE15B.3040303@diamand.org>

Luke Diamand <luke@diamand.org> writes:

Show 14 quoted lines

> On 07/05/15 22:16, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> i.e. the shell at the beginning of system sees "$customEditor"
>> (including the double quotes) as a quoted variable, expand the
>> environment variable as exported, and treat it as the path to
>> the program.  Again untested but I think
>>
>> 	P4EDITOR="\"\$customEditor\"" &&
>
> Or will this work?
>
> -	P4EDITOR="$TRASH_DIRECTORY/ed.sh" &&
> +	P4EDITOR="\"$TRASH_DIRECTORY/ed.sh\"" &&

I wasn't sure TRASH_DIRECTORY was exported; as long as it is (and it seems to be, from lib-test-functions.sh), that should be sufficient.

Show 9 quoted lines

> I'm still a bit worried about what will happen to Windows users with
> this change though. I think the following avoids breaking Windows
> clients,...
>
> -	system([editor, template_file])
> +       system(shlex.split(editor) + [template_file])
>
> I've not tested it on anything other than Linux so far, so best not to
> merge yet!

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".

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])
 
         # If the file was not saved, prompt to see if this patch should
         # be skipped.  But skip this verification step if configured so.