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

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

[Luke Diamand]

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

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

[Junio C Hamano]

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

[Luke Diamand]

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 1/3] git-p4: add failing test for P4EDITOR handling

[Luke Diamand]

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

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

[Junio C Hamano]

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.

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

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

[Junio C Hamano]

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.

Thanks.

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

[Luke Diamand]

On 20/05/15 21:56, Junio C Hamano wrote:
> 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!

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

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

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

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

[Luke Diamand]

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

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

[Luke Diamand]

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


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

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

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

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

[Junio C Hamano]

Luke Diamand <luke@diamand.org> writes:

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

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