{"thread":{"id":"39364","subject":"git-p4: test t9820-git-p4-editor-handling.sh failing","startedAt":"2015-05-19T06:40:55Z","lastAt":"2015-05-24T18:56:03Z","messageCount":11,"participants":["Luke Diamand","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"261537","messageId":"555ADAF7.5020001@diamand.org","threadId":"39364","inReplyTo":null,"subject":"git-p4: test t9820-git-p4-editor-handling.sh failing","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-05-19T06:40:55Z","receivedAt":"2015-05-19T06:40:55Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Hi!\n\nThe test I put in recently for multi-word editor handling in git-p4, \nt9820-git-p4-editor-handling.sh, has started failing.\n\nIt looks like the reason is the change to it that goes:\n\n-\tP4EDITOR=\"touch \\\"$git/touched\\\"\" git p4 submit &&\n+\tP4EDITOR=\": >\\\"$git/touched\\\"\" git p4 submit &&\n\nThe problem is that git-p4 invokes $P4EDITOR passing it the name of the \nsubmit template. After it returns, it checks that the editor has \nactually updated the file's modification time.\n\nThe first version (somewhat subtly) does this; the second doesn't.\n\nI put the extra check with \"$git/touched\" in just to check that P4EDITOR \nwas being invoked at all, but I guess it's not strictly necessary. I \nwondered about doing this:\n\n+\tP4EDITOR=\": >\\\"$git/touched\\\" && touch\" git p4 submit &&\n\nBut it's possibly getting a bit obscure. I guess it could be OK with a \ncomment.\n\nCould it go back to the original version, or is there some other way to \nachieve a similar effect?\n\nThanks!\nLuke\n"},{"id":"261557","messageId":"xmqqa8x0a7wq.fsf@gitster.dls.corp.google.com","threadId":"39364","inReplyTo":"555ADAF7.5020001@diamand.org","subject":"Re: git-p4: test t9820-git-p4-editor-handling.sh failing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-19T15:44:53Z","receivedAt":"2015-05-19T15:44:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand writes:\n\n> The test I put in recently for multi-word editor handling in git-p4,\n> t9820-git-p4-editor-handling.sh, has started failing.\n>\n> It looks like the reason is the change to it that goes:\n>\n> -\tP4EDITOR=\"touch \\\"$git/touched\\\"\" git p4 submit &&\n> +\tP4EDITOR=\": >\\\"$git/touched\\\"\" git p4 submit &&\n>\n> The problem is that git-p4 invokes $P4EDITOR passing it the name of\n> the submit template. After it returns, it checks that the editor has\n> actually updated the file's modification time.\n\nSorry, that was an unwarranted and unnecessary amend. Didn't\nrealize that touch was trying to affect two files.\n\nBut \"touch\" is not quite right, either.\n\nUnlike human sitting in front of keyboard, our fake editor types\nvery fast and wallclock time may not change between the time when\n\"git p4\" prepares the file to be edited and the fake editor returns.\n\nIs it really *only* the modification time that is checked? If our\nfake editor adds one blank line and return very fast without\nchanging the modification time, doesn't the caller notice that (and\nif not, shouldn't it be fixed to do so [*1*])?\n\nIf you absolutely need to change the timestamp to work around the\ncaller if it only checks the timestamp and does not notice the size\nor contents are different, then test-chmtime would be the right\nthing to use in the test suite to do this portably, something like.\n\n P4EDITOR=\": >\\\"$git/touched\\\" && test-chmtime +5\"\n\nperhaps.\n\nThanks.\n\n[Footnote]\n\n*1* Yeah, I just checked. It does check only mtime and wants the\n editor to spend at least one second to edit, which is silly X-<.\n"},{"id":"261616","messageId":"1432074198-13806-1-git-send-email-luke@diamand.org","threadId":"39364","inReplyTo":"xmqqa8x0a7wq.fsf@gitster.dls.corp.google.com","subject":"[PATCHv3 0/3] git-p4: fix handling of multi-word P4EDITOR","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-05-19T22:23:15Z","receivedAt":"2015-05-19T22:23:15Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Junio noticed that my new test was using touch, which is a bit\nracetastic because git-p4 wants P4EDITOR to spend at least one\nsecond editing the submit template or it will fail.\n\nThis updates the test to use test-chmtime, and also fixes the\nother git-p4 tests in the same way.\n\nLuke Diamand (3):\n git-p4: add failing test for P4EDITOR handling\n git-p4: fix handling of multi-word P4EDITOR\n git-p4: tests: use test-chmtime in place of touch\n\n git-p4.py | 2 +-\n t/t9803-git-p4-shell-metachars.sh | 4 ++--\n t/t9805-git-p4-skip-submit-edit.sh | 4 ++--\n t/t9813-git-p4-preserve-users.sh | 7 ++++---\n t/t9820-git-p4-editor-handling.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 5 files changed, 47 insertions(+), 8 deletions(-)\n create mode 100755 t/t9820-git-p4-editor-handling.sh\n\n-- \n2.4.1.502.gb11c5ab\n"},{"id":"261618","messageId":"1432074198-13806-2-git-send-email-luke@diamand.org","threadId":"39364","inReplyTo":"1432074198-13806-1-git-send-email-luke@diamand.org","subject":"[PATCHv3 1/3] git-p4: add failing test for P4EDITOR handling","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-05-19T22:23:16Z","receivedAt":"2015-05-19T22:23:16Z","isPatch":false,"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\". This currently fails.\n\nSigned-off-by: Luke Diamand \nSigned-off-by: Junio C Hamano \n---\n t/t9820-git-p4-editor-handling.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 1 file changed, 39 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..af924bb\n--- /dev/null\n+++ b/t/t9820-git-p4-editor-handling.sh\n@@ -0,0 +1,39 @@\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+test_expect_success 'EDITOR has options' '\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=\": >\\\"$git/touched\\\" && test-chmtime +5\" 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.1.502.gb11c5ab\n"},{"id":"261617","messageId":"1432074198-13806-3-git-send-email-luke@diamand.org","threadId":"39364","inReplyTo":"1432074198-13806-1-git-send-email-luke@diamand.org","subject":"[PATCHv3 2/3] git-p4: fix handling of multi-word P4EDITOR","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-05-19T22:23:17Z","receivedAt":"2015-05-19T22:23:17Z","isPatch":false,"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\nThis also adjusts t9805, which relied on the previous behaviour.\n\nSuggested-by: Jonathan Nieder \nSigned-off-by: Luke Diamand \nSigned-off-by: Junio C Hamano \n---\n git-p4.py | 2 +-\n t/t9805-git-p4-skip-submit-edit.sh | 2 +-\n t/t9820-git-p4-editor-handling.sh | 1 -\n 3 files changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex ff132b2..de06046 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([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/t9805-git-p4-skip-submit-edit.sh b/t/t9805-git-p4-skip-submit-edit.sh\nindex 8931188..5fbf904 100755\n--- a/t/t9805-git-p4-skip-submit-edit.sh\n+++ b/t/t9805-git-p4-skip-submit-edit.sh\n@@ -90,7 +90,7 @@ 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\tP4EDITOR=\"\\\"$TRASH_DIRECTORY/ed.sh\\\"\" &&\n \t\texport P4EDITOR &&\n \t\tgit p4 submit &&\n \t\tp4 changes //depot/... >wc &&\ndiff --git a/t/t9820-git-p4-editor-handling.sh b/t/t9820-git-p4-editor-handling.sh\nindex af924bb..7026ef9 100755\n--- a/t/t9820-git-p4-editor-handling.sh\n+++ b/t/t9820-git-p4-editor-handling.sh\n@@ -17,7 +17,6 @@ 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-- \n2.4.1.502.gb11c5ab\n"},{"id":"261619","messageId":"1432074198-13806-4-git-send-email-luke@diamand.org","threadId":"39364","inReplyTo":"1432074198-13806-1-git-send-email-luke@diamand.org","subject":"[PATCHv3 3/3] git-p4: tests: use test-chmtime in place of touch","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-05-19T22:23:18Z","receivedAt":"2015-05-19T22:23:18Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Using \"touch\" for P4EDITOR means that the tests can be a bit\nracy, since git-p4 checks the timestamp has been updated and\nfails if the timestamp is not updated.\n\nUse test-chmtime instead, which is designed for this.\n\nSigned-off-by: Luke Diamand \n---\n t/t9803-git-p4-shell-metachars.sh | 4 ++--\n t/t9805-git-p4-skip-submit-edit.sh | 2 +-\n t/t9813-git-p4-preserve-users.sh | 7 ++++---\n 3 files changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t9803-git-p4-shell-metachars.sh b/t/t9803-git-p4-shell-metachars.sh\nindex fbacff3..d950c7d 100755\n--- a/t/t9803-git-p4-shell-metachars.sh\n+++ b/t/t9803-git-p4-shell-metachars.sh\n@@ -28,7 +28,7 @@ test_expect_success 'shell metachars in filenames' '\n \t\techo f2 >\"file with spaces\" &&\n \t\tgit add \"file with spaces\" &&\n \t\tgit commit -m \"add files\" &&\n-\t\tP4EDITOR=touch git p4 submit\n+\t\tP4EDITOR=\"test-chmtime +5\" git p4 submit\n \t) &&\n \t(\n \t\tcd \"$cli\" &&\n@@ -47,7 +47,7 @@ test_expect_success 'deleting with shell metachars' '\n \t\tgit rm foo\\$bar &&\n \t\tgit rm file\\ with\\ spaces &&\n \t\tgit commit -m \"remove files\" &&\n-\t\tP4EDITOR=touch git p4 submit\n+\t\tP4EDITOR=\"test-chmtime +5\" git p4 submit\n \t) &&\n \t(\n \t\tcd \"$cli\" &&\ndiff --git a/t/t9805-git-p4-skip-submit-edit.sh b/t/t9805-git-p4-skip-submit-edit.sh\nindex 5fbf904..dc8fc0c 100755\n--- a/t/t9805-git-p4-skip-submit-edit.sh\n+++ b/t/t9805-git-p4-skip-submit-edit.sh\n@@ -17,7 +17,6 @@ test_expect_success 'init depot' '\n \t)\n '\n \n-# this works because P4EDITOR is set to true\n test_expect_success 'no config, unedited, say yes' '\n \tgit p4 clone --dest=\"$git\" //depot &&\n \ttest_when_finished cleanup_git &&\n@@ -25,6 +24,7 @@ test_expect_success 'no config, unedited, say yes' '\n \t\tcd \"$git\" &&\n \t\techo line >>file1 &&\n \t\tgit commit -a -m \"change 2\" &&\n+\t\tP4EDITOR=\"test-chmtime +5\" &&\n \t\techo y | git p4 submit &&\n \t\tp4 changes //depot/... >wc &&\n \t\ttest_line_count = 2 wc\ndiff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh\nindex 166b840..fe65788 100755\n--- a/t/t9813-git-p4-preserve-users.sh\n+++ b/t/t9813-git-p4-preserve-users.sh\n@@ -53,7 +53,8 @@ test_expect_success 'preserve users' '\n \t\tgit commit --author \"Alice \" -m \"a change by alice\" file1 &&\n \t\tgit commit --author \"Bob \" -m \"a change by bob\" file2 &&\n \t\tgit config git-p4.skipSubmitEditCheck true &&\n-\t\tP4EDITOR=touch P4USER=alice P4PASSWD=secret git p4 commit --preserve-user &&\n+\t\tP4EDITOR=\"test-chmtime +5\" P4USER=alice P4PASSWD=secret &&\n+\t\tgit p4 commit --preserve-user &&\n \t\tp4_check_commit_author file1 alice &&\n \t\tp4_check_commit_author file2 bob\n \t)\n@@ -69,7 +70,7 @@ test_expect_success 'refuse to preserve users without perms' '\n \t\tgit config git-p4.skipSubmitEditCheck true &&\n \t\techo \"username-noperms: a change by alice\" >>file1 &&\n \t\tgit commit --author \"Alice \" -m \"perms: a change by alice\" file1 &&\n-\t\tP4EDITOR=touch P4USER=bob P4PASSWD=secret &&\n+\t\tP4EDITOR=\"test-chmtime +5\" P4USER=bob P4PASSWD=secret &&\n \t\texport P4EDITOR P4USER P4PASSWD &&\n \t\ttest_must_fail git p4 commit --preserve-user &&\n \t\t! git diff --exit-code HEAD..p4/master\n@@ -87,7 +88,7 @@ test_expect_success 'preserve user where author is unknown to p4' '\n \t\tgit commit --author \"Bob \" -m \"preserve: a change by bob\" file1 &&\n \t\techo \"username-unknown: a change by charlie\" >>file1 &&\n \t\tgit commit --author \"Charlie \" -m \"preserve: a change by charlie\" file1 &&\n-\t\tP4EDITOR=touch P4USER=alice P4PASSWD=secret &&\n+\t\tP4EDITOR=\"test-chmtime +5\" P4USER=alice P4PASSWD=secret &&\n \t\texport P4EDITOR P4USER P4PASSWD &&\n \t\ttest_must_fail git p4 commit --preserve-user &&\n \t\t! git diff --exit-code HEAD..p4/master &&\n-- \n2.4.1.502.gb11c5ab\n"},{"id":"261624","messageId":"555BBAF0.7070008@diamand.org","threadId":"39364","inReplyTo":"1432074198-13806-4-git-send-email-luke@diamand.org","subject":"Re: [PATCHv3 3/3] git-p4: tests: use test-chmtime in place of touch","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-05-19T22:36:32Z","receivedAt":"2015-05-19T22:36:32Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 19/05/15 23:23, Luke Diamand wrote:\n> Using \"touch\" for P4EDITOR means that the tests can be a bit\n> racy, since git-p4 checks the timestamp has been updated and\n> fails if the timestamp is not updated.\n\nJunio - this one is incorrect where it changes t9805 around the test \ncalled \"no config, unedited, say yes\" (it didn't need changing at all).\n\nI can resend all three, or just the last one. Please let me know which \nis easier.\n\nThanks\nLuke\n\n\n>\n> Use test-chmtime instead, which is designed for this.\n>\n> Signed-off-by: Luke Diamand \n> ---\n> t/t9803-git-p4-shell-metachars.sh | 4 ++--\n> t/t9805-git-p4-skip-submit-edit.sh | 2 +-\n> t/t9813-git-p4-preserve-users.sh | 7 ++++---\n> 3 files changed, 7 insertions(+), 6 deletions(-)\n>\n> diff --git a/t/t9803-git-p4-shell-metachars.sh b/t/t9803-git-p4-shell-metachars.sh\n> index fbacff3..d950c7d 100755\n> --- a/t/t9803-git-p4-shell-metachars.sh\n> +++ b/t/t9803-git-p4-shell-metachars.sh\n> @@ -28,7 +28,7 @@ test_expect_success 'shell metachars in filenames' '\n> \t\techo f2 >\"file with spaces\" &&\n> \t\tgit add \"file with spaces\" &&\n> \t\tgit commit -m \"add files\" &&\n> -\t\tP4EDITOR=touch git p4 submit\n> +\t\tP4EDITOR=\"test-chmtime +5\" git p4 submit\n> \t) &&\n> \t(\n> \t\tcd \"$cli\" &&\n> @@ -47,7 +47,7 @@ test_expect_success 'deleting with shell metachars' '\n> \t\tgit rm foo\\$bar &&\n> \t\tgit rm file\\ with\\ spaces &&\n> \t\tgit commit -m \"remove files\" &&\n> -\t\tP4EDITOR=touch git p4 submit\n> +\t\tP4EDITOR=\"test-chmtime +5\" git p4 submit\n> \t) &&\n> \t(\n> \t\tcd \"$cli\" &&\n> diff --git a/t/t9805-git-p4-skip-submit-edit.sh b/t/t9805-git-p4-skip-submit-edit.sh\n> index 5fbf904..dc8fc0c 100755\n> --- a/t/t9805-git-p4-skip-submit-edit.sh\n> +++ b/t/t9805-git-p4-skip-submit-edit.sh\n> @@ -17,7 +17,6 @@ test_expect_success 'init depot' '\n> \t)\n> '\n>\n> -# this works because P4EDITOR is set to true\n\nThe above line shouldn't be removed, but it's worth noting that it gets \nset to true in lib-git-p4.sh.\n\n> test_expect_success 'no config, unedited, say yes' '\n> \tgit p4 clone --dest=\"$git\" //depot &&\n> \ttest_when_finished cleanup_git &&\n> @@ -25,6 +24,7 @@ test_expect_success 'no config, unedited, say yes' '\n> \t\tcd \"$git\" &&\n> \t\techo line >>file1 &&\n> \t\tgit commit -a -m \"change 2\" &&\n> +\t\tP4EDITOR=\"test-chmtime +5\" &&\n\nThe above line should not be added.\n\n> \t\techo y | git p4 submit &&\n> \t\tp4 changes //depot/... >wc &&\n> \t\ttest_line_count = 2 wc\n> diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh\n> index 166b840..fe65788 100755\n> --- a/t/t9813-git-p4-preserve-users.sh\n> +++ b/t/t9813-git-p4-preserve-users.sh\n> @@ -53,7 +53,8 @@ test_expect_success 'preserve users' '\n> \t\tgit commit --author \"Alice \" -m \"a change by alice\" file1 &&\n> \t\tgit commit --author \"Bob \" -m \"a change by bob\" file2 &&\n> \t\tgit config git-p4.skipSubmitEditCheck true &&\n> -\t\tP4EDITOR=touch P4USER=alice P4PASSWD=secret git p4 commit --preserve-user &&\n> +\t\tP4EDITOR=\"test-chmtime +5\" P4USER=alice P4PASSWD=secret &&\n> +\t\tgit p4 commit --preserve-user &&\n> \t\tp4_check_commit_author file1 alice &&\n> \t\tp4_check_commit_author file2 bob\n> \t)\n> @@ -69,7 +70,7 @@ test_expect_success 'refuse to preserve users without perms' '\n> \t\tgit config git-p4.skipSubmitEditCheck true &&\n> \t\techo \"username-noperms: a change by alice\" >>file1 &&\n> \t\tgit commit --author \"Alice \" -m \"perms: a change by alice\" file1 &&\n> -\t\tP4EDITOR=touch P4USER=bob P4PASSWD=secret &&\n> +\t\tP4EDITOR=\"test-chmtime +5\" P4USER=bob P4PASSWD=secret &&\n> \t\texport P4EDITOR P4USER P4PASSWD &&\n> \t\ttest_must_fail git p4 commit --preserve-user &&\n> \t\t! git diff --exit-code HEAD..p4/master\n> @@ -87,7 +88,7 @@ test_expect_success 'preserve user where author is unknown to p4' '\n> \t\tgit commit --author \"Bob \" -m \"preserve: a change by bob\" file1 &&\n> \t\techo \"username-unknown: a change by charlie\" >>file1 &&\n> \t\tgit commit --author \"Charlie \" -m \"preserve: a change by charlie\" file1 &&\n> -\t\tP4EDITOR=touch P4USER=alice P4PASSWD=secret &&\n> +\t\tP4EDITOR=\"test-chmtime +5\" P4USER=alice P4PASSWD=secret &&\n> \t\texport P4EDITOR P4USER P4PASSWD &&\n> \t\ttest_must_fail git p4 commit --preserve-user &&\n> \t\t! git diff --exit-code HEAD..p4/master &&\n>\n"},{"id":"261705","messageId":"xmqq617nnhxk.fsf@gitster.dls.corp.google.com","threadId":"39364","inReplyTo":"1432074198-13806-2-git-send-email-luke@diamand.org","subject":"Re: [PATCHv3 1/3] git-p4: add failing test for P4EDITOR handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-20T19:54:31Z","receivedAt":"2015-05-20T19:54:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand writes:\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\nOops? I assume that the one before the comment should go and this\none is\n\n\ttest_expect_failure 'Editor with an option' '\n\nor something.\n\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=\": >\\\"$git/touched\\\" && test-chmtime +5\" 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"},{"id":"261713","messageId":"xmqqd21vm0hw.fsf@gitster.dls.corp.google.com","threadId":"39364","inReplyTo":"xmqq617nnhxk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv3 1/3] git-p4: add failing test for P4EDITOR handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-20T20:56:27Z","receivedAt":"2015-05-20T20:56:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Luke Diamand writes:\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>\n> Oops? I assume that the one before the comment should go and this\n> one is\n>\n> \ttest_expect_failure 'Editor with an option' '\n>\n> or something.\n\nI'll queue the three patches, each of them followed with its own\nSQUASH commit. Could you sanity check them? If everything looks OK\nthen I'll just squash them and that way we can save back-and-forth.\n\nThanks.\n"},{"id":"261724","messageId":"555D000F.105@diamand.org","threadId":"39364","inReplyTo":"xmqqd21vm0hw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv3 1/3] git-p4: add failing test for P4EDITOR handling","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-05-20T21:43:43Z","receivedAt":"2015-05-20T21:43:43Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 20/05/15 21:56, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Luke Diamand writes:\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>>\n>> Oops? I assume that the one before the comment should go and this\n>> one is\n>>\n>> \ttest_expect_failure 'Editor with an option' '\n>>\n>> or something.\n>\n> I'll queue the three patches, each of them followed with its own\n> SQUASH commit. Could you sanity check them? If everything looks OK\n> then I'll just squash them and that way we can save back-and-forth.\n\nThat would be great, thanks!\n"},{"id":"262017","messageId":"xmqqy4kdby9o.fsf@gitster.dls.corp.google.com","threadId":"39364","inReplyTo":"1432074198-13806-4-git-send-email-luke@diamand.org","subject":"Re: [PATCHv3 3/3] git-p4: tests: use test-chmtime in place of touch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-24T18:56:03Z","receivedAt":"2015-05-24T18:56:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand writes:\n\n> diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh\n> index 166b840..fe65788 100755\n> --- a/t/t9813-git-p4-preserve-users.sh\n> +++ b/t/t9813-git-p4-preserve-users.sh\n> @@ -53,7 +53,8 @@ test_expect_success 'preserve users' '\n> \t\tgit commit --author \"Alice \" -m \"a change by alice\" file1 &&\n> \t\tgit commit --author \"Bob \" -m \"a change by bob\" file2 &&\n> \t\tgit config git-p4.skipSubmitEditCheck true &&\n> -\t\tP4EDITOR=touch P4USER=alice P4PASSWD=secret git p4 commit --preserve-user &&\n> +\t\tP4EDITOR=\"test-chmtime +5\" P4USER=alice P4PASSWD=secret &&\n> +\t\tgit p4 commit --preserve-user &&\n\nI think this hunk is wrong; we need to either change && to \\ to make\nit a single logical line that exports three environment variables\nonly to \"git p4 commit --preserve-user\", or insert \"export P4EDITOR\nP4USER P4PASSWD &&\" between these two lines.\n\nThe latter seems to be what the remainder of the test is doing, so\nI'd fix this up to mimick them.\n\nSorry for not catching it in the earlier review.\n\nThanks.\n\n> @@ -69,7 +70,7 @@ test_expect_success 'refuse to preserve users without perms' '\n> \t\tgit config git-p4.skipSubmitEditCheck true &&\n> \t\techo \"username-noperms: a change by alice\" >>file1 &&\n> \t\tgit commit --author \"Alice \" -m \"perms: a change by alice\" file1 &&\n> -\t\tP4EDITOR=touch P4USER=bob P4PASSWD=secret &&\n> +\t\tP4EDITOR=\"test-chmtime +5\" P4USER=bob P4PASSWD=secret &&\n> \t\texport P4EDITOR P4USER P4PASSWD &&\n> \t\ttest_must_fail git p4 commit --preserve-user &&\n> \t\t! git diff --exit-code HEAD..p4/master\n"}]}