* [PATCH 0/2] Re: Bug: git-p4 edit_template() and P4EDITOR w/options
@ 2015-05-07 17:24 Luke Diamand
2015-05-07 17:25 ` [PATCH 1/2] git-p4: add failing test for P4EDITOR handling Luke Diamand
2015-05-07 17:25 ` [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR Luke Diamand
0 siblings, 2 replies; 13+ messages in thread
From: Luke Diamand @ 2015-05-07 17:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Chris Lasell, 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] git-p4: add failing test for P4EDITOR handling
2015-05-07 17:24 [PATCH 0/2] Re: Bug: git-p4 edit_template() and P4EDITOR w/options Luke Diamand
@ 2015-05-07 17:25 ` Luke Diamand
2015-05-07 17:25 ` [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR Luke Diamand
1 sibling, 0 replies; 13+ messages in thread
From: Luke Diamand @ 2015-05-07 17:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Chris Lasell, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR
2015-05-07 17:24 [PATCH 0/2] Re: Bug: git-p4 edit_template() and P4EDITOR w/options Luke Diamand
2015-05-07 17:25 ` [PATCH 1/2] git-p4: add failing test for P4EDITOR handling Luke Diamand
@ 2015-05-07 17:25 ` Luke Diamand
2015-05-07 18:11 ` Junio C Hamano
1 sibling, 1 reply; 13+ messages in thread
From: Luke Diamand @ 2015-05-07 17:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Chris Lasell, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR
2015-05-07 17:25 ` [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR Luke Diamand
@ 2015-05-07 18:11 ` Junio C Hamano
2015-05-07 19:27 ` Luke Diamand
0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2015-05-07 18:11 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, Jonathan Nieder, Chris Lasell
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 &&
> (
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR
2015-05-07 18:11 ` Junio C Hamano
@ 2015-05-07 19:27 ` Luke Diamand
2015-05-07 20:16 ` Junio C Hamano
0 siblings, 1 reply; 13+ messages in thread
From: Luke Diamand @ 2015-05-07 19:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder, Chris Lasell
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 &&
>> (
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR
2015-05-07 19:27 ` Luke Diamand
@ 2015-05-07 20:16 ` Junio C Hamano
2015-05-07 20:42 ` Luke Diamand
0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2015-05-07 20:16 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, Jonathan Nieder, Chris Lasell
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR
2015-05-07 20:16 ` Junio C Hamano
@ 2015-05-07 20:42 ` Luke Diamand
2015-05-07 21:06 ` Junio C Hamano
0 siblings, 1 reply; 13+ messages in thread
From: Luke Diamand @ 2015-05-07 20:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder, Chris Lasell
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".
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR
2015-05-07 20:42 ` Luke Diamand
@ 2015-05-07 21:06 ` Junio C Hamano
2015-05-07 21:16 ` Junio C Hamano
0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2015-05-07 21:06 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, Jonathan Nieder, Chris Lasell
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR
2015-05-07 21:06 ` Junio C Hamano
@ 2015-05-07 21:16 ` Junio C Hamano
2015-05-07 22:04 ` Luke Diamand
0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2015-05-07 21:16 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, Jonathan Nieder, Chris Lasell
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR
2015-05-07 21:16 ` Junio C Hamano
@ 2015-05-07 22:04 ` Luke Diamand
2015-05-07 22:16 ` Junio C Hamano
0 siblings, 1 reply; 13+ messages in thread
From: Luke Diamand @ 2015-05-07 22:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder, Chris Lasell
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR
2015-05-07 22:04 ` Luke Diamand
@ 2015-05-07 22:16 ` Junio C Hamano
2015-05-24 9:28 ` Luke Diamand
0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2015-05-07 22:16 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, Jonathan Nieder, Chris Lasell
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.
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR
2015-05-07 22:16 ` Junio C Hamano
@ 2015-05-24 9:28 ` Luke Diamand
2015-05-26 20:21 ` Junio C Hamano
0 siblings, 1 reply; 13+ messages in thread
From: Luke Diamand @ 2015-05-24 9:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder, Chris Lasell
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR
2015-05-24 9:28 ` Luke Diamand
@ 2015-05-26 20:21 ` Junio C Hamano
0 siblings, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2015-05-26 20:21 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, Jonathan Nieder, Chris Lasell
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-05-26 20:21 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-07 17:24 [PATCH 0/2] Re: Bug: git-p4 edit_template() and P4EDITOR w/options Luke Diamand
2015-05-07 17:25 ` [PATCH 1/2] git-p4: add failing test for P4EDITOR handling Luke Diamand
2015-05-07 17:25 ` [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR Luke Diamand
2015-05-07 18:11 ` Junio C Hamano
2015-05-07 19:27 ` Luke Diamand
2015-05-07 20:16 ` Junio C Hamano
2015-05-07 20:42 ` Luke Diamand
2015-05-07 21:06 ` Junio C Hamano
2015-05-07 21:16 ` Junio C Hamano
2015-05-07 22:04 ` Luke Diamand
2015-05-07 22:16 ` Junio C Hamano
2015-05-24 9:28 ` Luke Diamand
2015-05-26 20:21 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).