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

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

[Luke Diamand]

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

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

[Luke Diamand]

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

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

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

[Luke Diamand]

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

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

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

[Junio C Hamano]

Luke Diamand <luke@diamand.org> writes:

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

>
> 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 &&
>  	(

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

[Luke Diamand]

On 07/05/15 19:11, Junio C Hamano wrote:
> 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.

>
>>
>> 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 &&
>>   	(

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

[Junio C Hamano]

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.

Thanks.

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

[Luke Diamand]

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

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

[Junio C Hamano]

Luke Diamand <luke@diamand.org> writes:

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

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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

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

[Luke Diamand]

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

>
> should do the work.
>
>> +		export customEditor P4EDITOR &&
>>   		git p4 submit &&
>>   		p4 changes //depot/... >wc &&
>>   		test_line_count = 5 wc

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

[Junio C Hamano]

Luke Diamand <luke@diamand.org> writes:

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

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

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

[Luke Diamand]

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

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

[Junio C Hamano]

Luke Diamand <luke@diamand.org> writes:

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