Re: git-p4: test t9820-git-p4-editor-handling.sh failing

6 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: git-p4: test t9820-git-p4-editor-handling.sh failing

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:04:49

Luke Diamand [off-list ref] writes:

The test I put in recently for multi-word editor handling in git-p4,
t9820-git-p4-editor-handling.sh, has started failing.

It looks like the reason is the change to it that goes:

-	P4EDITOR="touch \"$git/touched\"" git p4 submit &&
+	P4EDITOR=": >\"$git/touched\"" git p4 submit &&

The problem is that git-p4 invokes $P4EDITOR passing it the name of
the submit template. After it returns, it checks that the editor has
actually updated the file's modification time.

Sorry, that was an unwarranted and unnecessary amend.  Didn't
realize that touch was trying to affect two files.

But "touch" is not quite right, either.

Unlike human sitting in front of keyboard, our fake editor types
very fast and wallclock time may not change between the time when
"git p4" prepares the file to be edited and the fake editor returns.

Is it really *only* the modification time that is checked?  If our
fake editor adds one blank line and return very fast without
changing the modification time, doesn't the caller notice that (and
if not, shouldn't it be fixed to do so [*1*])?

If you absolutely need to change the timestamp to work around the
caller if it only checks the timestamp and does not notice the size
or contents are different, then test-chmtime would be the right
thing to use in the test suite to do this portably, something like.

    P4EDITOR=": >\"$git/touched\" && test-chmtime +5"

perhaps.

Thanks.

[Footnote]

*1* Yeah, I just checked.  It does check only mtime and wants the
    editor to spend at least one second to edit, which is silly X-<.

[PATCHv3 0/3] git-p4: fix handling of multi-word P4EDITOR

From: Luke Diamand <hidden>
Date: 2016-06-15 23:04:50

Junio noticed that my new test was using touch, which is a bit
racetastic because git-p4 wants P4EDITOR to spend at least one
second editing the submit template or it will fail.

This updates the test to use test-chmtime, and also fixes the
other git-p4 tests in the same way.

Luke Diamand (3):
  git-p4: add failing test for P4EDITOR handling
  git-p4: fix handling of multi-word P4EDITOR
  git-p4: tests: use test-chmtime in place of touch

 git-p4.py                          |  2 +-
 t/t9803-git-p4-shell-metachars.sh  |  4 ++--
 t/t9805-git-p4-skip-submit-edit.sh |  4 ++--
 t/t9813-git-p4-preserve-users.sh   |  7 ++++---
 t/t9820-git-p4-editor-handling.sh  | 38 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 47 insertions(+), 8 deletions(-)
 create mode 100755 t/t9820-git-p4-editor-handling.sh

-- 
2.4.1.502.gb11c5ab

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

From: Luke Diamand <hidden>
Date: 2016-06-15 23:04:50

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.

This also adjusts t9805, which relied on the previous behaviour.

Suggested-by: Jonathan Nieder <redacted>
Signed-off-by: Luke Diamand <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 git-p4.py                          | 2 +-
 t/t9805-git-p4-skip-submit-edit.sh | 2 +-
 t/t9820-git-p4-editor-handling.sh  | 1 -
 3 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index ff132b2..de06046 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([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/t9805-git-p4-skip-submit-edit.sh b/t/t9805-git-p4-skip-submit-edit.sh
index 8931188..5fbf904 100755
--- a/t/t9805-git-p4-skip-submit-edit.sh
+++ b/t/t9805-git-p4-skip-submit-edit.sh

@@ -90,7 +90,7 @@ test_expect_success 'no config, edited' '
 		cd "$git" &&
 		echo line >>file1 &&
 		git commit -a -m "change 5" &&
-		P4EDITOR="$TRASH_DIRECTORY/ed.sh" &&
+		P4EDITOR="\"$TRASH_DIRECTORY/ed.sh\"" &&
 		export P4EDITOR &&
 		git p4 submit &&
 		p4 changes //depot/... >wc &&

diff --git a/t/t9820-git-p4-editor-handling.sh b/t/t9820-git-p4-editor-handling.sh
index af924bb..7026ef9 100755
--- a/t/t9820-git-p4-editor-handling.sh
+++ b/t/t9820-git-p4-editor-handling.sh

@@ -17,7 +17,6 @@ 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' '

-- 
2.4.1.502.gb11c5ab

[PATCHv3 1/3] git-p4: add failing test for P4EDITOR handling

From: Luke Diamand <hidden>
Date: 2016-06-15 23:04:50

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

Signed-off-by: Luke Diamand <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 t/t9820-git-p4-editor-handling.sh | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 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..af924bb
--- /dev/null
+++ b/t/t9820-git-p4-editor-handling.sh

@@ -0,0 +1,39 @@
+#!/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.
+test_expect_success 'EDITOR has options' '
+	git p4 clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		echo change >file1 &&
+		git commit -m "change" file1 &&
+		P4EDITOR=": >\"$git/touched\" && test-chmtime +5" git p4 submit &&
+		test_path_is_file "$git/touched"
+	)
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done

-- 
2.4.1.502.gb11c5ab

[PATCHv3 3/3] git-p4: tests: use test-chmtime in place of touch

From: Luke Diamand <hidden>
Date: 2016-06-15 23:04:50

Using "touch" for P4EDITOR means that the tests can be a bit
racy, since git-p4 checks the timestamp has been updated and
fails if the timestamp is not updated.

Use test-chmtime instead, which is designed for this.

Signed-off-by: Luke Diamand <redacted>
---
 t/t9803-git-p4-shell-metachars.sh  | 4 ++--
 t/t9805-git-p4-skip-submit-edit.sh | 2 +-
 t/t9813-git-p4-preserve-users.sh   | 7 ++++---
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/t/t9803-git-p4-shell-metachars.sh b/t/t9803-git-p4-shell-metachars.sh
index fbacff3..d950c7d 100755
--- a/t/t9803-git-p4-shell-metachars.sh
+++ b/t/t9803-git-p4-shell-metachars.sh

@@ -28,7 +28,7 @@ test_expect_success 'shell metachars in filenames' '
 		echo f2 >"file with spaces" &&
 		git add "file with spaces" &&
 		git commit -m "add files" &&
-		P4EDITOR=touch git p4 submit
+		P4EDITOR="test-chmtime +5" git p4 submit
 	) &&
 	(
 		cd "$cli" &&

@@ -47,7 +47,7 @@ test_expect_success 'deleting with shell metachars' '
 		git rm foo\$bar &&
 		git rm file\ with\ spaces &&
 		git commit -m "remove files" &&
-		P4EDITOR=touch git p4 submit
+		P4EDITOR="test-chmtime +5" git p4 submit
 	) &&
 	(
 		cd "$cli" &&

diff --git a/t/t9805-git-p4-skip-submit-edit.sh b/t/t9805-git-p4-skip-submit-edit.sh
index 5fbf904..dc8fc0c 100755
--- a/t/t9805-git-p4-skip-submit-edit.sh
+++ b/t/t9805-git-p4-skip-submit-edit.sh

@@ -17,7 +17,6 @@ test_expect_success 'init depot' '
 	)
 '
 
-# this works because P4EDITOR is set to true
 test_expect_success 'no config, unedited, say yes' '
 	git p4 clone --dest="$git" //depot &&
 	test_when_finished cleanup_git &&

@@ -25,6 +24,7 @@ test_expect_success 'no config, unedited, say yes' '
 		cd "$git" &&
 		echo line >>file1 &&
 		git commit -a -m "change 2" &&
+		P4EDITOR="test-chmtime +5" &&
 		echo y | git p4 submit &&
 		p4 changes //depot/... >wc &&
 		test_line_count = 2 wc

diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
index 166b840..fe65788 100755
--- a/t/t9813-git-p4-preserve-users.sh
+++ b/t/t9813-git-p4-preserve-users.sh

@@ -53,7 +53,8 @@ test_expect_success 'preserve users' '
 		git commit --author "Alice <alice@example.com>" -m "a change by alice" file1 &&
 		git commit --author "Bob <bob@example.com>" -m "a change by bob" file2 &&
 		git config git-p4.skipSubmitEditCheck true &&
-		P4EDITOR=touch P4USER=alice P4PASSWD=secret git p4 commit --preserve-user &&
+		P4EDITOR="test-chmtime +5" P4USER=alice P4PASSWD=secret &&
+		git p4 commit --preserve-user &&
 		p4_check_commit_author file1 alice &&
 		p4_check_commit_author file2 bob
 	)

@@ -69,7 +70,7 @@ test_expect_success 'refuse to preserve users without perms' '
 		git config git-p4.skipSubmitEditCheck true &&
 		echo "username-noperms: a change by alice" >>file1 &&
 		git commit --author "Alice <alice@example.com>" -m "perms: a change by alice" file1 &&
-		P4EDITOR=touch P4USER=bob P4PASSWD=secret &&
+		P4EDITOR="test-chmtime +5" P4USER=bob P4PASSWD=secret &&
 		export P4EDITOR P4USER P4PASSWD &&
 		test_must_fail git p4 commit --preserve-user &&
 		! git diff --exit-code HEAD..p4/master

@@ -87,7 +88,7 @@ test_expect_success 'preserve user where author is unknown to p4' '
 		git commit --author "Bob <bob@example.com>" -m "preserve: a change by bob" file1 &&
 		echo "username-unknown: a change by charlie" >>file1 &&
 		git commit --author "Charlie <charlie@example.com>" -m "preserve: a change by charlie" file1 &&
-		P4EDITOR=touch P4USER=alice P4PASSWD=secret &&
+		P4EDITOR="test-chmtime +5" P4USER=alice P4PASSWD=secret &&
 		export P4EDITOR P4USER P4PASSWD &&
 		test_must_fail git p4 commit --preserve-user &&
 		! git diff --exit-code HEAD..p4/master &&

-- 
2.4.1.502.gb11c5ab

Re: [PATCHv3 3/3] git-p4: tests: use test-chmtime in place of touch

From: Luke Diamand <hidden>
Date: 2016-06-15 23:04:50

On 19/05/15 23:23, Luke Diamand wrote:

Using "touch" for P4EDITOR means that the tests can be a bit
racy, since git-p4 checks the timestamp has been updated and
fails if the timestamp is not updated.

Junio - this one is incorrect where it changes t9805 around the test 
called "no config, unedited, say yes" (it didn't need changing at all).

I can resend all three, or just the last one. Please let me know which 
is easier.

Thanks
Luke

quoted hunk

Use test-chmtime instead, which is designed for this.

Signed-off-by: Luke Diamand <redacted>
---
  t/t9803-git-p4-shell-metachars.sh  | 4 ++--
  t/t9805-git-p4-skip-submit-edit.sh | 2 +-
  t/t9813-git-p4-preserve-users.sh   | 7 ++++---
  3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/t/t9803-git-p4-shell-metachars.sh b/t/t9803-git-p4-shell-metachars.sh
index fbacff3..d950c7d 100755
--- a/t/t9803-git-p4-shell-metachars.sh
+++ b/t/t9803-git-p4-shell-metachars.sh

@@ -28,7 +28,7 @@ test_expect_success 'shell metachars in filenames' '
  		echo f2 >"file with spaces" &&
  		git add "file with spaces" &&
  		git commit -m "add files" &&
-		P4EDITOR=touch git p4 submit
+		P4EDITOR="test-chmtime +5" git p4 submit
  	) &&
  	(
  		cd "$cli" &&

@@ -47,7 +47,7 @@ test_expect_success 'deleting with shell metachars' '
  		git rm foo\$bar &&
  		git rm file\ with\ spaces &&
  		git commit -m "remove files" &&
-		P4EDITOR=touch git p4 submit
+		P4EDITOR="test-chmtime +5" git p4 submit
  	) &&
  	(
  		cd "$cli" &&

diff --git a/t/t9805-git-p4-skip-submit-edit.sh b/t/t9805-git-p4-skip-submit-edit.sh
index 5fbf904..dc8fc0c 100755
--- a/t/t9805-git-p4-skip-submit-edit.sh
+++ b/t/t9805-git-p4-skip-submit-edit.sh

@@ -17,7 +17,6 @@ test_expect_success 'init depot' '
  	)
  '

-# this works because P4EDITOR is set to true

The above line shouldn't be removed, but it's worth noting that it gets 
set to true in lib-git-p4.sh.

quoted hunk

  test_expect_success 'no config, unedited, say yes' '
  	git p4 clone --dest="$git" //depot &&
  	test_when_finished cleanup_git &&

@@ -25,6 +24,7 @@ test_expect_success 'no config, unedited, say yes' '
  		cd "$git" &&
  		echo line >>file1 &&
  		git commit -a -m "change 2" &&
+		P4EDITOR="test-chmtime +5" &&

The above line should not be added.

quoted hunk

  		echo y | git p4 submit &&
  		p4 changes //depot/... >wc &&
  		test_line_count = 2 wc

diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
index 166b840..fe65788 100755
--- a/t/t9813-git-p4-preserve-users.sh
+++ b/t/t9813-git-p4-preserve-users.sh

@@ -53,7 +53,8 @@ test_expect_success 'preserve users' '
  		git commit --author "Alice <alice@example.com>" -m "a change by alice" file1 &&
  		git commit --author "Bob <bob@example.com>" -m "a change by bob" file2 &&
  		git config git-p4.skipSubmitEditCheck true &&
-		P4EDITOR=touch P4USER=alice P4PASSWD=secret git p4 commit --preserve-user &&
+		P4EDITOR="test-chmtime +5" P4USER=alice P4PASSWD=secret &&
+		git p4 commit --preserve-user &&
  		p4_check_commit_author file1 alice &&
  		p4_check_commit_author file2 bob
  	)

@@ -69,7 +70,7 @@ test_expect_success 'refuse to preserve users without perms' '
  		git config git-p4.skipSubmitEditCheck true &&
  		echo "username-noperms: a change by alice" >>file1 &&
  		git commit --author "Alice <alice@example.com>" -m "perms: a change by alice" file1 &&
-		P4EDITOR=touch P4USER=bob P4PASSWD=secret &&
+		P4EDITOR="test-chmtime +5" P4USER=bob P4PASSWD=secret &&
  		export P4EDITOR P4USER P4PASSWD &&
  		test_must_fail git p4 commit --preserve-user &&
  		! git diff --exit-code HEAD..p4/master

@@ -87,7 +88,7 @@ test_expect_success 'preserve user where author is unknown to p4' '
  		git commit --author "Bob <bob@example.com>" -m "preserve: a change by bob" file1 &&
  		echo "username-unknown: a change by charlie" >>file1 &&
  		git commit --author "Charlie <charlie@example.com>" -m "preserve: a change by charlie" file1 &&
-		P4EDITOR=touch P4USER=alice P4PASSWD=secret &&
+		P4EDITOR="test-chmtime +5" P4USER=alice P4PASSWD=secret &&
  		export P4EDITOR P4USER P4PASSWD &&
  		test_must_fail git p4 commit --preserve-user &&
  		! git diff --exit-code HEAD..p4/master &&

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help