threads / discuss / 39364

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

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

## tl;dr

11 messages between May 19, 2015 and May 24, 2015.

replies: 10people: 2as markdown or json

Luke Diamand· May 19, 2015, 06:40 UTC · lore

Hi!

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.

The first version (somewhat subtly) does this; the second doesn't.

I put the extra check with "$git/touched" in just to check that P4EDITOR was being invoked at all, but I guess it's not strictly necessary. I wondered about doing this:

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

But it's possibly getting a bit obscure. I guess it could be OK with a comment.

Could it go back to the original version, or is there some other way to achieve a similar effect?

Thanks! Luke

Junio C Hamano· May 19, 2015, 15:44 UTC · re: Luke Diamand · lore

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

Luke Diamand <luke@diamand.org> writes:

Show 11 quoted lines

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

Luke Diamand· May 19, 2015, 22:23 UTC · re: Junio C Hamano · lore

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

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

Luke Diamand· May 19, 2015, 22:23 UTC · re: Luke Diamand · lore

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

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 <luke@diamand.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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

Junio C Hamano· May 20, 2015, 19:54 UTC · re: Luke Diamand · lore

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

Luke Diamand <luke@diamand.org> writes:

Show 6 quoted lines

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

Oops? I assume that the one before the comment should go and this one is

	test_expect_failure 'Editor with an option' '

or something.

Show 15 quoted lines

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

Junio C Hamano· May 20, 2015, 20:56 UTC · re: Junio C Hamano · lore

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

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

Show 15 quoted lines

> Luke Diamand <luke@diamand.org> writes:
>
>> +
>> +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 &&
>
> Oops?  I assume that the one before the comment should go and this
> one is
>
> 	test_expect_failure 'Editor with an option' '
>
> or something.

I'll queue the three patches, each of them followed with its own SQUASH commit. Could you sanity check them? If everything looks OK then I'll just squash them and that way we can save back-and-forth.

Thanks.

Luke Diamand· May 20, 2015, 21:43 UTC · re: Junio C Hamano · lore

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

On 20/05/15 21:56, Junio C Hamano wrote:

Show 21 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Luke Diamand <luke@diamand.org> writes:
>>
>>> +
>>> +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 &&
>>
>> Oops?  I assume that the one before the comment should go and this
>> one is
>>
>> 	test_expect_failure 'Editor with an option' '
>>
>> or something.
>
> I'll queue the three patches, each of them followed with its own
> SQUASH commit.  Could you sanity check them?  If everything looks OK
> then I'll just squash them and that way we can save back-and-forth.

That would be great, thanks!

Luke Diamand· May 19, 2015, 22:23 UTC · re: Luke Diamand · lore

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

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 <jrnieder@gmail.com>
Signed-off-by: Luke Diamand <luke@diamand.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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

Luke Diamand· May 19, 2015, 22:23 UTC · re: Luke Diamand · lore

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

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 <luke@diamand.org>
---
 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

Luke Diamand· May 19, 2015, 22:36 UTC · re: Luke Diamand · lore

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

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

Show 41 quoted lines

>
> Use test-chmtime instead, which is designed for this.
>
> Signed-off-by: Luke Diamand <luke@diamand.org>
> ---
>   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.

Show 8 quoted lines

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

Show 36 quoted lines

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

Junio C Hamano· May 24, 2015, 18:56 UTC · re: Luke Diamand · lore

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

Luke Diamand <luke@diamand.org> writes:

Show 11 quoted lines

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

I think this hunk is wrong; we need to either change && to \ to make it a single logical line that exports three environment variables only to "git p4 commit --preserve-user", or insert "export P4EDITOR P4USER P4PASSWD &&" between these two lines.

The latter seems to be what the remainder of the test is doing, so I'd fix this up to mimick them.

Sorry for not catching it in the earlier review.

Thanks.

Show 9 quoted lines

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