{"thread":{"id":"39269","subject":"[PATCH 0/2] Re: Bug: git-p4 edit_template() and P4EDITOR w/options","startedAt":"2015-05-07T17:24:59Z","lastAt":"2015-05-26T20:21:01Z","messageCount":13,"participants":["Luke Diamand","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"260752","messageId":"1431019501-30807-1-git-send-email-luke@diamand.org","threadId":"39269","inReplyTo":null,"subject":"[PATCH 0/2] Re: Bug: git-p4 edit_template() and P4EDITOR w/options","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-05-07T17:24:59Z","receivedAt":"2015-05-07T17:24:59Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"This adds a test case, and Jonathan's fix, for the git-p4 edit_template\nproblem found by Chris.\n\nLuke Diamand (2):\n  git-p4: add failing test for P4EDITOR handling\n  git-p4: fix handling of multi-word P4EDITOR\n\n git-p4.py                         |  2 +-\n t/t9820-git-p4-editor-handling.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 39 insertions(+), 1 deletion(-)\n create mode 100755 t/t9820-git-p4-editor-handling.sh\n\n-- \n2.4.0.rc3.380.g8e2ddc7\n"},{"id":"260753","messageId":"1431019501-30807-2-git-send-email-luke@diamand.org","threadId":"39269","inReplyTo":"1431019501-30807-1-git-send-email-luke@diamand.org","subject":"[PATCH 1/2] git-p4: add failing test for P4EDITOR handling","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-05-07T17:25:00Z","receivedAt":"2015-05-07T17:25:00Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Add test case that git-p4 handles a setting of P4EDITOR\nthat takes arguments, e.g. \"gvim -f\"\n\nSigned-off-by: Luke Diamand <luke@diamand.org>\n---\n t/t9820-git-p4-editor-handling.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 38 insertions(+)\n create mode 100755 t/t9820-git-p4-editor-handling.sh\n\ndiff --git a/t/t9820-git-p4-editor-handling.sh b/t/t9820-git-p4-editor-handling.sh\nnew file mode 100755\nindex 0000000..e0a3c52\n--- /dev/null\n+++ b/t/t9820-git-p4-editor-handling.sh\n@@ -0,0 +1,38 @@\n+#!/bin/sh\n+\n+test_description='git p4 handling of EDITOR'\n+\n+. ./lib-git-p4.sh\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'init depot' '\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\techo file1 >file1 &&\n+\t\tp4 add file1 &&\n+\t\tp4 submit -d \"file1\"\n+\t)\n+'\n+\n+test_expect_failure 'EDITOR has options' '\n+# Check that the P4EDITOR argument can be given command-line\n+# options, which git-p4 will then pass through to the shell.\n+\tgit p4 clone --dest=\"$git\" //depot &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\techo change >file1 &&\n+\t\tgit commit -m \"change\" file1 &&\n+\t\tP4EDITOR=\"touch \\\"$git/touched\\\"\" git p4 submit &&\n+\t\ttest_path_is_file \"$git/touched\"\n+\t)\n+'\n+\n+test_expect_success 'kill p4d' '\n+\tkill_p4d\n+'\n+\n+test_done\n-- \n2.4.0.rc3.380.g8e2ddc7\n"},{"id":"260754","messageId":"1431019501-30807-3-git-send-email-luke@diamand.org","threadId":"39269","inReplyTo":"1431019501-30807-1-git-send-email-luke@diamand.org","subject":"[PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-05-07T17:25:01Z","receivedAt":"2015-05-07T17:25:01Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"This teaches git-p4 to pass the P4EDITOR variable to the\nshell for expansion, so that any command-line arguments are\ncorrectly handled. Without this, git-p4 can only launch the\neditor if P4EDITOR is solely the path to the binary, without\nany arguments.\n\nSuggested-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Luke Diamand <luke@diamand.org>\n---\n git-p4.py                         | 2 +-\n t/t9820-git-p4-editor-handling.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 41a77e6..ca6bb95 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1248,7 +1248,7 @@ class P4Submit(Command, P4UserMap):\n             editor = os.environ.get(\"P4EDITOR\")\n         else:\n             editor = read_pipe(\"git var GIT_EDITOR\").strip()\n-        system([editor, template_file])\n+        system([\"sh\", \"-c\", ('%s \"$@\"' % editor), editor, template_file])\n \n         # If the file was not saved, prompt to see if this patch should\n         # be skipped.  But skip this verification step if configured so.\ndiff --git a/t/t9820-git-p4-editor-handling.sh b/t/t9820-git-p4-editor-handling.sh\nindex e0a3c52..c178bd7 100755\n--- a/t/t9820-git-p4-editor-handling.sh\n+++ b/t/t9820-git-p4-editor-handling.sh\n@@ -17,9 +17,9 @@ test_expect_success 'init depot' '\n \t)\n '\n \n-test_expect_failure 'EDITOR has options' '\n # Check that the P4EDITOR argument can be given command-line\n # options, which git-p4 will then pass through to the shell.\n+test_expect_success 'EDITOR has options' '\n \tgit p4 clone --dest=\"$git\" //depot &&\n \ttest_when_finished cleanup_git &&\n \t(\n-- \n2.4.0.rc3.380.g8e2ddc7\n"},{"id":"260761","messageId":"xmqqr3qsp8a0.fsf@gitster.dls.corp.google.com","threadId":"39269","inReplyTo":"1431019501-30807-3-git-send-email-luke@diamand.org","subject":"Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-07T18:11:35Z","receivedAt":"2015-05-07T18:11:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand <luke@diamand.org> writes:\n\n> This teaches git-p4 to pass the P4EDITOR variable to the\n> shell for expansion, so that any command-line arguments are\n> correctly handled. Without this, git-p4 can only launch the\n> editor if P4EDITOR is solely the path to the binary, without\n> any arguments.\n\nDoes the real Perforce, when spawning P4EDITOR, behave the same way?\n\nIf not, this patch would be breaking not just the expectation of\nexisting git-p4 users (which we cannot avoid and which we are\nwilling to do) but also breaking things for people who _will_ come\nto us in the future, expecting that\n\n\texport P4EDITOR=\"/Users/me/My Programs/my-editor\"\n\nto work as they expect.  If they already have to do\n\n\texport P4EDITOR=\"'/Users/me/My Programs/my-editor'\"\n\nthen there is no problem, but because I am not a P4EDITOR user, I am\nmerely double checking.\n\n>\n> Suggested-by: Jonathan Nieder <jrnieder@gmail.com>\n> Signed-off-by: Luke Diamand <luke@diamand.org>\n> ---\n>  git-p4.py                         | 2 +-\n>  t/t9820-git-p4-editor-handling.sh | 2 +-\n>  2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 41a77e6..ca6bb95 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1248,7 +1248,7 @@ class P4Submit(Command, P4UserMap):\n>              editor = os.environ.get(\"P4EDITOR\")\n>          else:\n>              editor = read_pipe(\"git var GIT_EDITOR\").strip()\n> -        system([editor, template_file])\n> +        system([\"sh\", \"-c\", ('%s \"$@\"' % editor), editor, template_file])\n>  \n>          # If the file was not saved, prompt to see if this patch should\n>          # be skipped.  But skip this verification step if configured so.\n> diff --git a/t/t9820-git-p4-editor-handling.sh b/t/t9820-git-p4-editor-handling.sh\n> index e0a3c52..c178bd7 100755\n> --- a/t/t9820-git-p4-editor-handling.sh\n> +++ b/t/t9820-git-p4-editor-handling.sh\n> @@ -17,9 +17,9 @@ test_expect_success 'init depot' '\n>  \t)\n>  '\n>  \n> -test_expect_failure 'EDITOR has options' '\n>  # Check that the P4EDITOR argument can be given command-line\n>  # options, which git-p4 will then pass through to the shell.\n> +test_expect_success 'EDITOR has options' '\n>  \tgit p4 clone --dest=\"$git\" //depot &&\n>  \ttest_when_finished cleanup_git &&\n>  \t(\n"},{"id":"260778","messageId":"554BBCBE.1020408@diamand.org","threadId":"39269","inReplyTo":"xmqqr3qsp8a0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-05-07T19:27:58Z","receivedAt":"2015-05-07T19:27:58Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 07/05/15 19:11, Junio C Hamano wrote:\n> Luke Diamand <luke@diamand.org> writes:\n>\n>\n> Does the real Perforce, when spawning P4EDITOR, behave the same way?\n>\n> If not, this patch would be breaking not just the expectation of\n> existing git-p4 users (which we cannot avoid and which we are\n> willing to do) but also breaking things for people who _will_ come\n> to us in the future, expecting that\n>\n> \texport P4EDITOR=\"/Users/me/My Programs/my-editor\"\n>\n> to work as they expect.  If they already have to do\n>\n> \texport P4EDITOR=\"'/Users/me/My Programs/my-editor'\"\n>\n> then there is no problem, but because I am not a P4EDITOR user, I am\n> merely double checking.\n\nOn Linux:\n\n$ export P4EDITOR=\"gvim -f\"\n$ p4 submit\n-- works as you would hope --\n\n$ export P4EDITOR=\"/home/lgd/My Programs/editor.sh\"\n$ p4 submit\nsh: 1: /home/lgd/My: not found\n\n$ export P4EDITOR=\"/home/lgd/My\\ Programs/editor.sh\"\n-- works fine --\n\nI don't know what happens on Windows. But I think the \"sh -c\" code would \nbreak on that platform, whereas the current code works fine (*)\n\nLuke\n\n\n(*) I should probably get a Windows test environment going somehow so I \ncan check this stuff.\n\n>\n>>\n>> Suggested-by: Jonathan Nieder <jrnieder@gmail.com>\n>> Signed-off-by: Luke Diamand <luke@diamand.org>\n>> ---\n>>   git-p4.py                         | 2 +-\n>>   t/t9820-git-p4-editor-handling.sh | 2 +-\n>>   2 files changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/git-p4.py b/git-p4.py\n>> index 41a77e6..ca6bb95 100755\n>> --- a/git-p4.py\n>> +++ b/git-p4.py\n>> @@ -1248,7 +1248,7 @@ class P4Submit(Command, P4UserMap):\n>>               editor = os.environ.get(\"P4EDITOR\")\n>>           else:\n>>               editor = read_pipe(\"git var GIT_EDITOR\").strip()\n>> -        system([editor, template_file])\n>> +        system([\"sh\", \"-c\", ('%s \"$@\"' % editor), editor, template_file])\n>>\n>>           # If the file was not saved, prompt to see if this patch should\n>>           # be skipped.  But skip this verification step if configured so.\n>> diff --git a/t/t9820-git-p4-editor-handling.sh b/t/t9820-git-p4-editor-handling.sh\n>> index e0a3c52..c178bd7 100755\n>> --- a/t/t9820-git-p4-editor-handling.sh\n>> +++ b/t/t9820-git-p4-editor-handling.sh\n>> @@ -17,9 +17,9 @@ test_expect_success 'init depot' '\n>>   \t)\n>>   '\n>>\n>> -test_expect_failure 'EDITOR has options' '\n>>   # Check that the P4EDITOR argument can be given command-line\n>>   # options, which git-p4 will then pass through to the shell.\n>> +test_expect_success 'EDITOR has options' '\n>>   \tgit p4 clone --dest=\"$git\" //depot &&\n>>   \ttest_when_finished cleanup_git &&\n>>   \t(\n"},{"id":"260780","messageId":"xmqqlhh0nny1.fsf@gitster.dls.corp.google.com","threadId":"39269","inReplyTo":"554BBCBE.1020408@diamand.org","subject":"Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-07T20:16:06Z","receivedAt":"2015-05-07T20:16:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand <luke@diamand.org> writes:\n\n> On Linux:\n>\n> $ export P4EDITOR=\"gvim -f\"\n> $ p4 submit\n> -- works as you would hope --\n>\n> $ export P4EDITOR=\"/home/lgd/My Programs/editor.sh\"\n> $ p4 submit\n> sh: 1: /home/lgd/My: not found\n>\n> $ export P4EDITOR=\"/home/lgd/My\\ Programs/editor.sh\"\n> -- works fine --\n\nGood.  That is exactly I wanted to see.\n\nThanks.\n"},{"id":"260781","messageId":"554BCE25.5070308@diamand.org","threadId":"39269","inReplyTo":"xmqqlhh0nny1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-05-07T20:42:13Z","receivedAt":"2015-05-07T20:42:13Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 07/05/15 21:16, Junio C Hamano wrote:\n> Luke Diamand <luke@diamand.org> writes:\n>\n>> On Linux:\n>>\n>> $ export P4EDITOR=\"gvim -f\"\n>> $ p4 submit\n>> -- works as you would hope --\n>>\n>> $ export P4EDITOR=\"/home/lgd/My Programs/editor.sh\"\n>> $ p4 submit\n>> sh: 1: /home/lgd/My: not found\n>>\n>> $ export P4EDITOR=\"/home/lgd/My\\ Programs/editor.sh\"\n>> -- works fine --\n>\n> Good.  That is exactly I wanted to see.\n\nTest case t9805-git-p4-skip-submit-edit.sh fails with that change. It \nsets P4EDITOR to \"$TRASH_DIRECTORY/ed.sh\".\n"},{"id":"260783","messageId":"xmqqa8xgnlme.fsf@gitster.dls.corp.google.com","threadId":"39269","inReplyTo":"554BCE25.5070308@diamand.org","subject":"Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-07T21:06:17Z","receivedAt":"2015-05-07T21:06:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand <luke@diamand.org> writes:\n\n> On 07/05/15 21:16, Junio C Hamano wrote:\n>> Luke Diamand <luke@diamand.org> writes:\n>>\n>>> On Linux:\n>>>\n>>> $ export P4EDITOR=\"gvim -f\"\n>>> $ p4 submit\n>>> -- works as you would hope --\n>>>\n>>> $ export P4EDITOR=\"/home/lgd/My Programs/editor.sh\"\n>>> $ p4 submit\n>>> sh: 1: /home/lgd/My: not found\n>>>\n>>> $ export P4EDITOR=\"/home/lgd/My\\ Programs/editor.sh\"\n>>> -- works fine --\n>>\n>> Good.  That is exactly I wanted to see.\n>\n> Test case t9805-git-p4-skip-submit-edit.sh fails with that change. It\n> sets P4EDITOR to \"$TRASH_DIRECTORY/ed.sh\".\n\n;-)\n\nPerhaps something like this would work (totally untested)?\n\ndiff --git a/t/t9805-git-p4-skip-submit-edit.sh b/t/t9805-git-p4-skip-submit-edit.sh\nindex 8931188..a47b44b 100755\n--- a/t/t9805-git-p4-skip-submit-edit.sh\n+++ b/t/t9805-git-p4-skip-submit-edit.sh\n@@ -90,8 +90,9 @@ test_expect_success 'no config, edited' '\n \t\tcd \"$git\" &&\n \t\techo line >>file1 &&\n \t\tgit commit -a -m \"change 5\" &&\n-\t\tP4EDITOR=\"$TRASH_DIRECTORY/ed.sh\" &&\n-\t\texport P4EDITOR &&\n+\t\tcustomEditor=\"$TRASH_DIRECTORY/ed.sh\" &&\n+\t\tP4EDITOR=\"\\$customEditor\" &&\n+\t\texport customEditor P4EDITOR &&\n \t\tgit p4 submit &&\n \t\tp4 changes //depot/... >wc &&\n \t\ttest_line_count = 5 wc\n"},{"id":"260785","messageId":"xmqq3838nl5l.fsf@gitster.dls.corp.google.com","threadId":"39269","inReplyTo":"xmqqa8xgnlme.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-07T21:16:22Z","receivedAt":"2015-05-07T21:16:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Perhaps something like this would work (totally untested)?\n>\n> diff --git a/t/t9805-git-p4-skip-submit-edit.sh b/t/t9805-git-p4-skip-submit-edit.sh\n> index 8931188..a47b44b 100755\n> --- a/t/t9805-git-p4-skip-submit-edit.sh\n> +++ b/t/t9805-git-p4-skip-submit-edit.sh\n> @@ -90,8 +90,9 @@ test_expect_success 'no config, edited' '\n>  \t\tcd \"$git\" &&\n>  \t\techo line >>file1 &&\n>  \t\tgit commit -a -m \"change 5\" &&\n> -\t\tP4EDITOR=\"$TRASH_DIRECTORY/ed.sh\" &&\n> -\t\texport P4EDITOR &&\n> +\t\tcustomEditor=\"$TRASH_DIRECTORY/ed.sh\" &&\n> +\t\tP4EDITOR=\"\\$customEditor\" &&\n\nSorry, this inside should be quoted a bit more so that the executed\ncommand becomes\n\n       sh -c '\"$customEditor\" \"$@\"'\n\ni.e. the shell at the beginning of system sees \"$customEditor\"\n(including the double quotes) as a quoted variable, expand the\nenvironment variable as exported, and treat it as the path to\nthe program.  Again untested but I think\n\n\tP4EDITOR=\"\\\"\\$customEditor\\\"\" &&\n\nshould do the work.\n\n> +\t\texport customEditor P4EDITOR &&\n>  \t\tgit p4 submit &&\n>  \t\tp4 changes //depot/... >wc &&\n>  \t\ttest_line_count = 5 wc\n"},{"id":"260787","messageId":"554BE15B.3040303@diamand.org","threadId":"39269","inReplyTo":"xmqq3838nl5l.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-05-07T22:04:11Z","receivedAt":"2015-05-07T22:04:11Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 07/05/15 22:16, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> i.e. the shell at the beginning of system sees \"$customEditor\"\n> (including the double quotes) as a quoted variable, expand the\n> environment variable as exported, and treat it as the path to\n> the program.  Again untested but I think\n>\n> \tP4EDITOR=\"\\\"\\$customEditor\\\"\" &&\n\nOr will this work?\n\n-\tP4EDITOR=\"$TRASH_DIRECTORY/ed.sh\" &&\n+\tP4EDITOR=\"\\\"$TRASH_DIRECTORY/ed.sh\\\"\" &&\n\nI'm still a bit worried about what will happen to Windows users with \nthis change though. I think the following avoids breaking Windows \nclients, but I'm not sure if it's the right way to go:\n\n-\tsystem([editor, template_file])\n+       system(shlex.split(editor) + [template_file])\n\nI've not tested it on anything other than Linux so far, so best not to \nmerge yet!\n\nLuke\n\n>\n> should do the work.\n>\n>> +\t\texport customEditor P4EDITOR &&\n>>   \t\tgit p4 submit &&\n>>   \t\tp4 changes //depot/... >wc &&\n>>   \t\ttest_line_count = 5 wc\n"},{"id":"260788","messageId":"xmqqtwvom3t4.fsf@gitster.dls.corp.google.com","threadId":"39269","inReplyTo":"554BE15B.3040303@diamand.org","subject":"Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-07T22:16:23Z","receivedAt":"2015-05-07T22:16:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand <luke@diamand.org> writes:\n\n> On 07/05/15 22:16, Junio C Hamano wrote:\n>> Junio C Hamano <gitster@pobox.com> writes:\n>>\n>> i.e. the shell at the beginning of system sees \"$customEditor\"\n>> (including the double quotes) as a quoted variable, expand the\n>> environment variable as exported, and treat it as the path to\n>> the program.  Again untested but I think\n>>\n>> \tP4EDITOR=\"\\\"\\$customEditor\\\"\" &&\n>\n> Or will this work?\n>\n> -\tP4EDITOR=\"$TRASH_DIRECTORY/ed.sh\" &&\n> +\tP4EDITOR=\"\\\"$TRASH_DIRECTORY/ed.sh\\\"\" &&\n\nI wasn't sure TRASH_DIRECTORY was exported; as long as it is (and it\nseems to be, from lib-test-functions.sh), that should be sufficient.\n\n> I'm still a bit worried about what will happen to Windows users with\n> this change though. I think the following avoids breaking Windows\n> clients,...\n>\n> -\tsystem([editor, template_file])\n> +       system(shlex.split(editor) + [template_file])\n>\n> I've not tested it on anything other than Linux so far, so best not to\n> merge yet!\n\nLooking at run-command.c, GIT_WINDOES_NATIVE and POSIX seems to use\npretty much the same construct, except that they use SHELL_PATH\ninstead of \"sh\".\n\nSo something like this may be sufficient, perhaps?\n\n Makefile  | 1 +\n git-p4.py | 2 +-\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 20058f1..fda44bf 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1776,6 +1776,7 @@ $(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS\n $(SCRIPT_PYTHON_GEN): % : %.py\n \t$(QUIET_GEN)$(RM) $@ $@+ && \\\n \tsed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \\\n+\t    -e 's|SHELL_PATH|$(SHELL_PATH_SQ)|g' \\\n \t    $< >$@+ && \\\n \tchmod +x $@+ && \\\n \tmv $@+ $@\ndiff --git a/git-p4.py b/git-p4.py\nindex de06046..eb6d4b1 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1220,7 +1220,7 @@ class P4Submit(Command, P4UserMap):\n             editor = os.environ.get(\"P4EDITOR\")\n         else:\n             editor = read_pipe(\"git var GIT_EDITOR\").strip()\n-        system([\"sh\", \"-c\", ('%s \"$@\"' % editor), editor, template_file])\n+        system(['''SHELL_PATH''', \"-c\", ('%s \"$@\"' % editor), editor, template_file])\n \n         # If the file was not saved, prompt to see if this patch should\n         # be skipped.  But skip this verification step if configured so.\n"},{"id":"262015","messageId":"556199A2.8090802@diamand.org","threadId":"39269","inReplyTo":"xmqqtwvom3t4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-05-24T09:28:02Z","receivedAt":"2015-05-24T09:28:02Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 07/05/15 23:16, Junio C Hamano wrote:\n> Luke Diamand <luke@diamand.org> writes:\n>\n\n[Resurrecting old thread]\n\n>\n> Looking at run-command.c, GIT_WINDOES_NATIVE and POSIX seems to use\n> pretty much the same construct, except that they use SHELL_PATH\n> instead of \"sh\".\n\nI think the state of git on Windows is a bit shaky (I'm happy to be \nproved wrong of course), but I think the only seriously active port is \nthe msys one.\n\nThat, as far as I can tell, uses an msys version of 'sh', so it will be \nperfectly happy with the \"sh -c ...\" construct.\n\nThere may be a native windows port in existence, but I can't find how to \nbuild this, and I assume it's going to need Visual Studio, which makes \nit a lot more complex to get going.\n\nThe code you were looking at in run-command.c says this:\n\n#ifndef GIT_WINDOWS_NATIVE\n\t\tnargv[nargc++] = SHELL_PATH;  <<<<< !GIT_WINDOWS_NATIVE\n#else\n\t\tnargv[nargc++] = \"sh\";        <<<<< GIT_WINDOWS_NATIVE\n#endif\n\t\tnargv[nargc++] = \"-c\";\n\nTo me, that seems to imply that for GIT_WINDOWS_NATIVE, we take the \n*second* branch and use \"sh\", so again, the the code as it stands will \nbe fine. msysgit uses that path.\n\n(The next line, trying to use \"-c\" has no chance of working if Cmd is \nbeing used).\n\n\n>\n> So something like this may be sufficient, perhaps?\n>\n>   Makefile  | 1 +\n>   git-p4.py | 2 +-\n>   2 files changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 20058f1..fda44bf 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1776,6 +1776,7 @@ $(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS\n>   $(SCRIPT_PYTHON_GEN): % : %.py\n>   \t$(QUIET_GEN)$(RM) $@ $@+ && \\\n>   \tsed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \\\n> +\t    -e 's|SHELL_PATH|$(SHELL_PATH_SQ)|g' \\\n>   \t    $< >$@+ && \\\n>   \tchmod +x $@+ && \\\n>   \tmv $@+ $@\n> diff --git a/git-p4.py b/git-p4.py\n> index de06046..eb6d4b1 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1220,7 +1220,7 @@ class P4Submit(Command, P4UserMap):\n>               editor = os.environ.get(\"P4EDITOR\")\n>           else:\n>               editor = read_pipe(\"git var GIT_EDITOR\").strip()\n> -        system([\"sh\", \"-c\", ('%s \"$@\"' % editor), editor, template_file])\n> +        system(['''SHELL_PATH''', \"-c\", ('%s \"$@\"' % editor), editor, template_file])\n\nThis seems to be expanded to '''sh''' which doesn't then work at all. I \ndidn't take the time to investigate further though.\n\n>\n>           # If the file was not saved, prompt to see if this patch should\n>           # be skipped.  But skip this verification step if configured so.\n\nI don't think we need to do anything. msysgit works fine with the origin \n\"sh\", \"-c\", ... code.\n\nThanks!\nLuke\n"},{"id":"262157","messageId":"xmqqpp5njdjm.fsf@gitster.dls.corp.google.com","threadId":"39269","inReplyTo":"556199A2.8090802@diamand.org","subject":"Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-26T20:21:01Z","receivedAt":"2015-05-26T20:21:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand <luke@diamand.org> writes:\n\n> On 07/05/15 23:16, Junio C Hamano wrote:\n>> Luke Diamand <luke@diamand.org> writes:\n>>\n>\n> [Resurrecting old thread]\n> ...\n>\n> To me, that seems to imply that for GIT_WINDOWS_NATIVE, we take the\n> *second* branch and use \"sh\", so again, the the code as it stands will\n> be fine. msysgit uses that path.\n> ...\n>\n> I don't think we need to do anything. msysgit works fine with the\n> origin \"sh\", \"-c\", ... code.\n\nThanks.  Let's merge what is in 'pu' down to 'next' then.\n"}]}