threads / patch / 39269

patch, 2 partsRe: Bug: git-p4 edit_template() and P4EDITOR w/options

Subject: [PATCH 0/2] Re: Bug: git-p4 edit_template() and P4EDITOR w/options

## tl;dr

13 messages between May 7, 2015 and May 26, 2015. Diffs are folded; open one to read it.

replies: 12people: 2as markdown or json

Luke Diamand· May 7, 2015, 17:24 UTC · lore

This adds a test case, and Jonathan's fix, for the git-p4 edit_template problem found by Chris.

Luke Diamand (2):
  git-p4: add failing test for P4EDITOR handling
  git-p4: fix handling of multi-word P4EDITOR
 git-p4.py                         |  2 +-
 t/t9820-git-p4-editor-handling.sh | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 1 deletion(-)
 create mode 100755 t/t9820-git-p4-editor-handling.sh
-- 
2.4.0.rc3.380.g8e2ddc7
Luke Diamand· May 7, 2015, 17:25 UTC · re: Luke Diamand · lore

[PATCH 1/2] git-p4: add failing test for P4EDITOR handling

Add test case that git-p4 handles a setting of P4EDITOR that takes arguments, e.g. "gvim -f"

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 t/t9820-git-p4-editor-handling.sh | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100755 t/t9820-git-p4-editor-handling.sh
Show changes to t/t9820-git-p4-editor-handling.sh +38 −0
diff --git a/t/t9820-git-p4-editor-handling.sh b/t/t9820-git-p4-editor-handling.sh
new file mode 100755
index 0000000..e0a3c52
--- /dev/null
+++ b/t/t9820-git-p4-editor-handling.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+
+test_description='git p4 handling of EDITOR'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+test_expect_success 'init depot' '
+	(
+		cd "$cli" &&
+		echo file1 >file1 &&
+		p4 add file1 &&
+		p4 submit -d "file1"
+	)
+'
+
+test_expect_failure 'EDITOR has options' '
+# Check that the P4EDITOR argument can be given command-line
+# options, which git-p4 will then pass through to the shell.
+	git p4 clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		echo change >file1 &&
+		git commit -m "change" file1 &&
+		P4EDITOR="touch \"$git/touched\"" git p4 submit &&
+		test_path_is_file "$git/touched"
+	)
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done

-- 
2.4.0.rc3.380.g8e2ddc7
Luke Diamand· May 7, 2015, 17:25 UTC · re: Luke Diamand · lore

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

This teaches git-p4 to pass the P4EDITOR variable to the shell for expansion, so that any command-line arguments are correctly handled. Without this, git-p4 can only launch the editor if P4EDITOR is solely the path to the binary, without any arguments.

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py                         | 2 +-
 t/t9820-git-p4-editor-handling.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
Show changes to 2 files +2 −2

git-p4.py, t/t9820-git-p4-editor-handling.sh

diff --git a/git-p4.py b/git-p4.py
index 41a77e6..ca6bb95 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1248,7 +1248,7 @@ class P4Submit(Command, P4UserMap):
             editor = os.environ.get("P4EDITOR")
         else:
             editor = read_pipe("git var GIT_EDITOR").strip()
-        system([editor, template_file])
+        system(["sh", "-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.
diff --git a/t/t9820-git-p4-editor-handling.sh b/t/t9820-git-p4-editor-handling.sh
index e0a3c52..c178bd7 100755
--- a/t/t9820-git-p4-editor-handling.sh
+++ b/t/t9820-git-p4-editor-handling.sh
@@ -17,9 +17,9 @@ test_expect_success 'init depot' '
 	)
 '
 
-test_expect_failure 'EDITOR has options' '
 # Check that the P4EDITOR argument can be given command-line
 # options, which git-p4 will then pass through to the shell.
+test_expect_success 'EDITOR has options' '
 	git p4 clone --dest="$git" //depot &&
 	test_when_finished cleanup_git &&
 	(

-- 
2.4.0.rc3.380.g8e2ddc7
Junio C Hamano· May 7, 2015, 18:11 UTC · re: Luke Diamand · lore

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

Luke Diamand <luke@diamand.org> writes:
Show 5 quoted lines
> This teaches git-p4 to pass the P4EDITOR variable to the
> shell for expansion, so that any command-line arguments are
> correctly handled. Without this, git-p4 can only launch the
> editor if P4EDITOR is solely the path to the binary, without
> any arguments.
Does the real Perforce, when spawning P4EDITOR, behave the same way?

If not, this patch would be breaking not just the expectation of existing git-p4 users (which we cannot avoid and which we are willing to do) but also breaking things for people who _will_ come to us in the future, expecting that

	export P4EDITOR="/Users/me/My Programs/my-editor"
to work as they expect.  If they already have to do
	export P4EDITOR="'/Users/me/My Programs/my-editor'"

then there is no problem, but because I am not a P4EDITOR user, I am merely double checking.

Show 36 quoted lines
>
> Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Luke Diamand <luke@diamand.org>
> ---
>  git-p4.py                         | 2 +-
>  t/t9820-git-p4-editor-handling.sh | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 41a77e6..ca6bb95 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1248,7 +1248,7 @@ class P4Submit(Command, P4UserMap):
>              editor = os.environ.get("P4EDITOR")
>          else:
>              editor = read_pipe("git var GIT_EDITOR").strip()
> -        system([editor, template_file])
> +        system(["sh", "-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.
> diff --git a/t/t9820-git-p4-editor-handling.sh b/t/t9820-git-p4-editor-handling.sh
> index e0a3c52..c178bd7 100755
> --- a/t/t9820-git-p4-editor-handling.sh
> +++ b/t/t9820-git-p4-editor-handling.sh
> @@ -17,9 +17,9 @@ test_expect_success 'init depot' '
>  	)
>  '
>  
> -test_expect_failure 'EDITOR has options' '
>  # Check that the P4EDITOR argument can be given command-line
>  # options, which git-p4 will then pass through to the shell.
> +test_expect_success 'EDITOR has options' '
>  	git p4 clone --dest="$git" //depot &&
>  	test_when_finished cleanup_git &&
>  	(
Luke Diamand· May 7, 2015, 19:27 UTC · re: Junio C Hamano · lore

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

On 07/05/15 19:11, Junio C Hamano wrote:
Show 18 quoted lines
> Luke Diamand <luke@diamand.org> writes:
>
>
> Does the real Perforce, when spawning P4EDITOR, behave the same way?
>
> If not, this patch would be breaking not just the expectation of
> existing git-p4 users (which we cannot avoid and which we are
> willing to do) but also breaking things for people who _will_ come
> to us in the future, expecting that
>
> 	export P4EDITOR="/Users/me/My Programs/my-editor"
>
> to work as they expect.  If they already have to do
>
> 	export P4EDITOR="'/Users/me/My Programs/my-editor'"
>
> then there is no problem, but because I am not a P4EDITOR user, I am
> merely double checking.
On Linux:

$ export P4EDITOR="gvim -f" $ p4 submit -- works as you would hope --

$ export P4EDITOR="/home/lgd/My Programs/editor.sh" $ p4 submit sh: 1: /home/lgd/My: not found

$ export P4EDITOR="/home/lgd/My\ Programs/editor.sh" -- works fine --

I don't know what happens on Windows. But I think the "sh -c" code would break on that platform, whereas the current code works fine (*)

Luke

(*) I should probably get a Windows test environment going somehow so I can check this stuff.

Show 37 quoted lines
>
>>
>> Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
>> Signed-off-by: Luke Diamand <luke@diamand.org>
>> ---
>>   git-p4.py                         | 2 +-
>>   t/t9820-git-p4-editor-handling.sh | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 41a77e6..ca6bb95 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -1248,7 +1248,7 @@ class P4Submit(Command, P4UserMap):
>>               editor = os.environ.get("P4EDITOR")
>>           else:
>>               editor = read_pipe("git var GIT_EDITOR").strip()
>> -        system([editor, template_file])
>> +        system(["sh", "-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.
>> diff --git a/t/t9820-git-p4-editor-handling.sh b/t/t9820-git-p4-editor-handling.sh
>> index e0a3c52..c178bd7 100755
>> --- a/t/t9820-git-p4-editor-handling.sh
>> +++ b/t/t9820-git-p4-editor-handling.sh
>> @@ -17,9 +17,9 @@ test_expect_success 'init depot' '
>>   	)
>>   '
>>
>> -test_expect_failure 'EDITOR has options' '
>>   # Check that the P4EDITOR argument can be given command-line
>>   # options, which git-p4 will then pass through to the shell.
>> +test_expect_success 'EDITOR has options' '
>>   	git p4 clone --dest="$git" //depot &&
>>   	test_when_finished cleanup_git &&
>>   	(
Junio C Hamano· May 7, 2015, 20:16 UTC · re: Luke Diamand · lore

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

Luke Diamand <luke@diamand.org> writes:
Show 12 quoted lines
> On Linux:
>
> $ export P4EDITOR="gvim -f"
> $ p4 submit
> -- works as you would hope --
>
> $ export P4EDITOR="/home/lgd/My Programs/editor.sh"
> $ p4 submit
> sh: 1: /home/lgd/My: not found
>
> $ export P4EDITOR="/home/lgd/My\ Programs/editor.sh"
> -- works fine --
Good.  That is exactly I wanted to see.
Thanks.
Luke Diamand· May 7, 2015, 20:42 UTC · re: Junio C Hamano · lore

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

On 07/05/15 21:16, Junio C Hamano wrote:
Show 16 quoted lines
> Luke Diamand <luke@diamand.org> writes:
>
>> On Linux:
>>
>> $ export P4EDITOR="gvim -f"
>> $ p4 submit
>> -- works as you would hope --
>>
>> $ export P4EDITOR="/home/lgd/My Programs/editor.sh"
>> $ p4 submit
>> sh: 1: /home/lgd/My: not found
>>
>> $ export P4EDITOR="/home/lgd/My\ Programs/editor.sh"
>> -- works fine --
>
> Good.  That is exactly I wanted to see.

Test case t9805-git-p4-skip-submit-edit.sh fails with that change. It sets P4EDITOR to "$TRASH_DIRECTORY/ed.sh".

Junio C Hamano· May 7, 2015, 21:06 UTC · re: Luke Diamand · lore

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

Luke Diamand <luke@diamand.org> writes:
Show 20 quoted lines
> On 07/05/15 21:16, Junio C Hamano wrote:
>> Luke Diamand <luke@diamand.org> writes:
>>
>>> On Linux:
>>>
>>> $ export P4EDITOR="gvim -f"
>>> $ p4 submit
>>> -- works as you would hope --
>>>
>>> $ export P4EDITOR="/home/lgd/My Programs/editor.sh"
>>> $ p4 submit
>>> sh: 1: /home/lgd/My: not found
>>>
>>> $ export P4EDITOR="/home/lgd/My\ Programs/editor.sh"
>>> -- works fine --
>>
>> Good.  That is exactly I wanted to see.
>
> Test case t9805-git-p4-skip-submit-edit.sh fails with that change. It
> sets P4EDITOR to "$TRASH_DIRECTORY/ed.sh".
;-)
Perhaps something like this would work (totally untested)?
Show changes to t/t9805-git-p4-skip-submit-edit.sh +3 −2
diff --git a/t/t9805-git-p4-skip-submit-edit.sh b/t/t9805-git-p4-skip-submit-edit.sh
index 8931188..a47b44b 100755
--- a/t/t9805-git-p4-skip-submit-edit.sh
+++ b/t/t9805-git-p4-skip-submit-edit.sh
@@ -90,8 +90,9 @@ test_expect_success 'no config, edited' '
 		cd "$git" &&
 		echo line >>file1 &&
 		git commit -a -m "change 5" &&
-		P4EDITOR="$TRASH_DIRECTORY/ed.sh" &&
-		export P4EDITOR &&
+		customEditor="$TRASH_DIRECTORY/ed.sh" &&
+		P4EDITOR="\$customEditor" &&
+		export customEditor P4EDITOR &&
 		git p4 submit &&
 		p4 changes //depot/... >wc &&
 		test_line_count = 5 wc
Junio C Hamano· May 7, 2015, 21:16 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 14 quoted lines
> Perhaps something like this would work (totally untested)?
>
> diff --git a/t/t9805-git-p4-skip-submit-edit.sh b/t/t9805-git-p4-skip-submit-edit.sh
> index 8931188..a47b44b 100755
> --- a/t/t9805-git-p4-skip-submit-edit.sh
> +++ b/t/t9805-git-p4-skip-submit-edit.sh
> @@ -90,8 +90,9 @@ test_expect_success 'no config, edited' '
>  		cd "$git" &&
>  		echo line >>file1 &&
>  		git commit -a -m "change 5" &&
> -		P4EDITOR="$TRASH_DIRECTORY/ed.sh" &&
> -		export P4EDITOR &&
> +		customEditor="$TRASH_DIRECTORY/ed.sh" &&
> +		P4EDITOR="\$customEditor" &&

Sorry, this inside should be quoted a bit more so that the executed command becomes

       sh -c '"$customEditor" "$@"'

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\"" &&
should do the work.
> +		export customEditor P4EDITOR &&
>  		git p4 submit &&
>  		p4 changes //depot/... >wc &&
>  		test_line_count = 5 wc
Luke Diamand· May 7, 2015, 22:04 UTC · re: Junio C Hamano · lore

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

On 07/05/15 22:16, Junio C Hamano wrote:
Show 8 quoted lines
> 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'm still a bit worried about what will happen to Windows users with this change though. I think the following avoids breaking Windows clients, but I'm not sure if it's the right way to go:

-	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!

Luke
Show 7 quoted lines
>
> should do the work.
>
>> +		export customEditor P4EDITOR &&
>>   		git p4 submit &&
>>   		p4 changes //depot/... >wc &&
>>   		test_line_count = 5 wc
Junio C Hamano· May 7, 2015, 22:16 UTC · re: Luke Diamand · lore

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

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(-)
Show changes to 2 files +2 −1

Makefile, git-p4.py

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.
Luke Diamand· May 24, 2015, 09:28 UTC · re: Junio C Hamano · lore

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

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

Show 29 quoted lines
>
> 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

Junio C Hamano· May 26, 2015, 20:21 UTC · re: Luke Diamand · lore

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

Luke Diamand <luke@diamand.org> writes:
Show 14 quoted lines
> On 07/05/15 23:16, Junio C Hamano wrote:
>> Luke Diamand <luke@diamand.org> writes:
>>
>
> [Resurrecting old thread]
> ...
>
> 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.
> ...
>
> I don't think we need to do anything. msysgit works fine with the
> origin "sh", "-c", ... code.
Thanks.  Let's merge what is in 'pu' down to 'next' then.

← back to recent threads