[PATCH 0/4] git-p4 test cleanup, commit time change

[PATCH 0/4] git-p4 test cleanup, commit time change

[Pete Wyckoff]

Here's a small patch series to clean up the git-p4 tests a bit,
make them work on a 2-user eval p4d, and to fix a problem with
the timestamp on the import commit.

Luke, if you can take a look and ack these, I'd appreciate
the review since I had to adjust your --preserveUser tests.
And if you're looking at that one, might as well look at them
all.  :)

I'd welcome review from anyone with interest, and will plan
to submit these to Junio in a week or so.

		-- Pete

[PATCH 1/4] git-p4: use test_when_finished in tests

[Pete Wyckoff]

Cleanup nicely when tests fail.  This avoids many duplicated
lines in the tests, and adds cleanup in a couple of tests that
did not have it.  When one fails, now all the rest will not
fail too.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9800-git-p4.sh |  103 +++++++++++++++++++++++++----------------------------
 1 files changed, 49 insertions(+), 54 deletions(-)

diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index 33b0127..aec3ba1 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -45,29 +45,33 @@ test_expect_success 'add p4 files' '
 	cd "$TRASH_DIRECTORY"
 '
 
+cleanup_git() {
+	cd "$TRASH_DIRECTORY" &&
+	rm -rf "$git" &&
+	mkdir "$git"
+}
+
 test_expect_success 'basic git-p4 clone' '
 	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	git log --oneline >lines &&
-	test_line_count = 1 lines &&
-	cd .. &&
-	rm -rf "$git" && mkdir "$git"
+	test_line_count = 1 lines
 '
 
 test_expect_success 'git-p4 clone @all' '
 	"$GITP4" clone --dest="$git" //depot@all &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	git log --oneline >lines &&
-	test_line_count = 2 lines &&
-	cd .. &&
-	rm -rf "$git" && mkdir "$git"
+	test_line_count = 2 lines
 '
 
 test_expect_success 'git-p4 sync uninitialized repo' '
 	test_create_repo "$git" &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
-	test_must_fail "$GITP4" sync &&
-	rm -rf "$git" && mkdir "$git"
+	test_must_fail "$GITP4" sync
 '
 
 #
@@ -76,19 +80,18 @@ test_expect_success 'git-p4 sync uninitialized repo' '
 #
 test_expect_success 'git-p4 sync new branch' '
 	test_create_repo "$git" &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	test_commit head &&
 	"$GITP4" sync --branch=refs/remotes/p4/depot //depot@all &&
 	git log --oneline p4/depot >lines &&
-	cat lines &&
-	test_line_count = 2 lines &&
-	cd .. &&
-	rm -rf "$git" && mkdir "$git"
+	test_line_count = 2 lines
 '
 
 test_expect_success 'exit when p4 fails to produce marshaled output' '
 	badp4dir="$TRASH_DIRECTORY/badp4dir" &&
 	mkdir -p "$badp4dir" &&
+	test_when_finished "rm -rf $badp4dir" &&
 	cat >"$badp4dir"/p4 <<-EOF &&
 	#!$SHELL_PATH
 	exit 1
@@ -106,29 +109,26 @@ test_expect_success 'add p4 files with wildcards in the names' '
 	echo file-wild-at >file-wild@at &&
 	echo file-wild-percent >file-wild%percent &&
 	p4 add -f file-wild* &&
-	p4 submit -d "file wildcards" &&
-	cd "$TRASH_DIRECTORY"
+	p4 submit -d "file wildcards"
 '
 
 test_expect_success 'wildcard files git-p4 clone' '
 	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	test -f file-wild#hash &&
 	test -f file-wild\*star &&
 	test -f file-wild@at &&
-	test -f file-wild%percent &&
-	cd "$TRASH_DIRECTORY" &&
-	rm -rf "$git" && mkdir "$git"
+	test -f file-wild%percent
 '
 
 test_expect_success 'clone bare' '
 	"$GITP4" clone --dest="$git" --bare //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	test ! -d .git &&
 	bare=`git config --get core.bare` &&
-	test "$bare" = true &&
-	cd "$TRASH_DIRECTORY" &&
-	rm -rf "$git" && mkdir "$git"
+	test "$bare" = true
 '
 
 p4_add_user() {
@@ -173,6 +173,7 @@ test_expect_success 'preserve users' '
 	p4_add_user bob Bob &&
 	p4_grant_admin alice &&
 	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	echo "username: a change by alice" >> file1 &&
 	echo "username: a change by bob" >> file2 &&
@@ -181,27 +182,25 @@ test_expect_success 'preserve users' '
 	git config git-p4.skipSubmitEditCheck true &&
 	P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user &&
 	p4_check_commit_author file1 alice &&
-	p4_check_commit_author file2 bob &&
-	cd "$TRASH_DIRECTORY" &&
-	rm -rf "$git" && mkdir "$git"
+	p4_check_commit_author file2 bob
 '
 
 # Test username support, submitting as bob, who lacks admin rights. Should
 # not submit change to p4 (git diff should show deltas).
 test_expect_success 'refuse to preserve users without perms' '
 	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	echo "username-noperms: a change by alice" >> file1 &&
 	git commit --author "Alice <alice@localhost>" -m "perms: a change by alice" file1 &&
 	! P4EDITOR=touch P4USER=bob P4PASSWD=secret "$GITP4" commit --preserve-user &&
-	! git diff --exit-code HEAD..p4/master > /dev/null &&
-	cd "$TRASH_DIRECTORY" &&
-	rm -rf "$git" && mkdir "$git"
+	! git diff --exit-code HEAD..p4/master > /dev/null
 '
 
 # What happens with unknown author? Without allowMissingP4Users it should fail.
 test_expect_success 'preserve user where author is unknown to p4' '
 	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	git config git-p4.skipSubmitEditCheck true
 	echo "username-bob: a change by bob" >> file1 &&
@@ -215,9 +214,7 @@ test_expect_success 'preserve user where author is unknown to p4' '
 	git config git-p4.preserveUser true &&
 	P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit &&
 	git diff --exit-code HEAD..p4/master > /dev/null &&
-	p4_check_commit_author file1 alice &&
-	cd "$TRASH_DIRECTORY" &&
-	rm -rf "$git" && mkdir "$git"
+	p4_check_commit_author file1 alice
 '
 
 # If we're *not* using --preserve-user, git-p4 should warn if we're submitting
@@ -226,31 +223,29 @@ test_expect_success 'preserve user where author is unknown to p4' '
 # Test: warning disabled and user is the same.
 test_expect_success 'not preserving user with mixed authorship' '
 	"$GITP4" clone --dest="$git" //depot &&
-	(
-		cd "$git" &&
-		git config git-p4.skipSubmitEditCheck true &&
-		p4_add_user derek Derek &&
-
-		make_change_by_user usernamefile3 Derek derek@localhost &&
-		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
-		grep "git author derek@localhost does not match" actual &&
-
-		make_change_by_user usernamefile3 Charlie charlie@localhost &&
-		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
-		grep "git author charlie@localhost does not match" actual &&
-
-		make_change_by_user usernamefile3 alice alice@localhost &&
-		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
-		! grep "git author.*does not match" actual &&
-
-		git config git-p4.skipUserNameCheck true &&
-		make_change_by_user usernamefile3 Charlie charlie@localhost &&
-		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
-		! grep "git author.*does not match" actual &&
-
-		p4_check_commit_author usernamefile3 alice
-	) &&
-	rm -rf "$git" && mkdir "$git"
+	test_when_finished cleanup_git &&
+	cd "$git" &&
+	git config git-p4.skipSubmitEditCheck true &&
+	p4_add_user derek Derek &&
+
+	make_change_by_user usernamefile3 Derek derek@localhost &&
+	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
+	grep "git author derek@localhost does not match" actual &&
+
+	make_change_by_user usernamefile3 Charlie charlie@localhost &&
+	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
+	grep "git author charlie@localhost does not match" actual &&
+
+	make_change_by_user usernamefile3 alice alice@localhost &&
+	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
+	! grep "git author.*does not match" actual &&
+
+	git config git-p4.skipUserNameCheck true &&
+	make_change_by_user usernamefile3 Charlie charlie@localhost &&
+	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
+	! grep "git author.*does not match" actual &&
+
+	p4_check_commit_author usernamefile3 alice
 '
 
 
-- 
1.7.5.4

Re: [PATCH 1/4] git-p4: use test_when_finished in tests

[Luke Diamand]

On 31/07/11 01:38, Pete Wyckoff wrote:
> Cleanup nicely when tests fail.  This avoids many duplicated
> lines in the tests, and adds cleanup in a couple of tests that
> did not have it.  When one fails, now all the rest will not
> fail too.

Acked.

Much tidier.

>
> Signed-off-by: Pete Wyckoff<pw@padd.com>
> ---
>   t/t9800-git-p4.sh |  103 +++++++++++++++++++++++++----------------------------
>   1 files changed, 49 insertions(+), 54 deletions(-)
>
> diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
> index 33b0127..aec3ba1 100755
> --- a/t/t9800-git-p4.sh
> +++ b/t/t9800-git-p4.sh
> @@ -45,29 +45,33 @@ test_expect_success 'add p4 files' '
>   	cd "$TRASH_DIRECTORY"
>   '
>
> +cleanup_git() {
> +	cd "$TRASH_DIRECTORY"&&
> +	rm -rf "$git"&&
> +	mkdir "$git"
> +}
> +
>   test_expect_success 'basic git-p4 clone' '
>   	"$GITP4" clone --dest="$git" //depot&&
> +	test_when_finished cleanup_git&&
>   	cd "$git"&&
>   	git log --oneline>lines&&
> -	test_line_count = 1 lines&&
> -	cd ..&&
> -	rm -rf "$git"&&  mkdir "$git"
> +	test_line_count = 1 lines
>   '
>
>   test_expect_success 'git-p4 clone @all' '
>   	"$GITP4" clone --dest="$git" //depot@all&&
> +	test_when_finished cleanup_git&&
>   	cd "$git"&&
>   	git log --oneline>lines&&
> -	test_line_count = 2 lines&&
> -	cd ..&&
> -	rm -rf "$git"&&  mkdir "$git"
> +	test_line_count = 2 lines
>   '
>
>   test_expect_success 'git-p4 sync uninitialized repo' '
>   	test_create_repo "$git"&&
> +	test_when_finished cleanup_git&&
>   	cd "$git"&&
> -	test_must_fail "$GITP4" sync&&
> -	rm -rf "$git"&&  mkdir "$git"
> +	test_must_fail "$GITP4" sync
>   '
>
>   #
> @@ -76,19 +80,18 @@ test_expect_success 'git-p4 sync uninitialized repo' '
>   #
>   test_expect_success 'git-p4 sync new branch' '
>   	test_create_repo "$git"&&
> +	test_when_finished cleanup_git&&
>   	cd "$git"&&
>   	test_commit head&&
>   	"$GITP4" sync --branch=refs/remotes/p4/depot //depot@all&&
>   	git log --oneline p4/depot>lines&&
> -	cat lines&&
> -	test_line_count = 2 lines&&
> -	cd ..&&
> -	rm -rf "$git"&&  mkdir "$git"
> +	test_line_count = 2 lines
>   '
>
>   test_expect_success 'exit when p4 fails to produce marshaled output' '
>   	badp4dir="$TRASH_DIRECTORY/badp4dir"&&
>   	mkdir -p "$badp4dir"&&
> +	test_when_finished "rm -rf $badp4dir"&&
>   	cat>"$badp4dir"/p4<<-EOF&&
>   	#!$SHELL_PATH
>   	exit 1
> @@ -106,29 +109,26 @@ test_expect_success 'add p4 files with wildcards in the names' '
>   	echo file-wild-at>file-wild@at&&
>   	echo file-wild-percent>file-wild%percent&&
>   	p4 add -f file-wild*&&
> -	p4 submit -d "file wildcards"&&
> -	cd "$TRASH_DIRECTORY"
> +	p4 submit -d "file wildcards"
>   '
>
>   test_expect_success 'wildcard files git-p4 clone' '
>   	"$GITP4" clone --dest="$git" //depot&&
> +	test_when_finished cleanup_git&&
>   	cd "$git"&&
>   	test -f file-wild#hash&&
>   	test -f file-wild\*star&&
>   	test -f file-wild@at&&
> -	test -f file-wild%percent&&
> -	cd "$TRASH_DIRECTORY"&&
> -	rm -rf "$git"&&  mkdir "$git"
> +	test -f file-wild%percent
>   '
>
>   test_expect_success 'clone bare' '
>   	"$GITP4" clone --dest="$git" --bare //depot&&
> +	test_when_finished cleanup_git&&
>   	cd "$git"&&
>   	test ! -d .git&&
>   	bare=`git config --get core.bare`&&
> -	test "$bare" = true&&
> -	cd "$TRASH_DIRECTORY"&&
> -	rm -rf "$git"&&  mkdir "$git"
> +	test "$bare" = true
>   '
>
>   p4_add_user() {
> @@ -173,6 +173,7 @@ test_expect_success 'preserve users' '
>   	p4_add_user bob Bob&&
>   	p4_grant_admin alice&&
>   	"$GITP4" clone --dest="$git" //depot&&
> +	test_when_finished cleanup_git&&
>   	cd "$git"&&
>   	echo "username: a change by alice">>  file1&&
>   	echo "username: a change by bob">>  file2&&
> @@ -181,27 +182,25 @@ test_expect_success 'preserve users' '
>   	git config git-p4.skipSubmitEditCheck true&&
>   	P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user&&
>   	p4_check_commit_author file1 alice&&
> -	p4_check_commit_author file2 bob&&
> -	cd "$TRASH_DIRECTORY"&&
> -	rm -rf "$git"&&  mkdir "$git"
> +	p4_check_commit_author file2 bob
>   '
>
>   # Test username support, submitting as bob, who lacks admin rights. Should
>   # not submit change to p4 (git diff should show deltas).
>   test_expect_success 'refuse to preserve users without perms' '
>   	"$GITP4" clone --dest="$git" //depot&&
> +	test_when_finished cleanup_git&&
>   	cd "$git"&&
>   	echo "username-noperms: a change by alice">>  file1&&
>   	git commit --author "Alice<alice@localhost>" -m "perms: a change by alice" file1&&
>   	! P4EDITOR=touch P4USER=bob P4PASSWD=secret "$GITP4" commit --preserve-user&&
> -	! git diff --exit-code HEAD..p4/master>  /dev/null&&
> -	cd "$TRASH_DIRECTORY"&&
> -	rm -rf "$git"&&  mkdir "$git"
> +	! git diff --exit-code HEAD..p4/master>  /dev/null
>   '
>
>   # What happens with unknown author? Without allowMissingP4Users it should fail.
>   test_expect_success 'preserve user where author is unknown to p4' '
>   	"$GITP4" clone --dest="$git" //depot&&
> +	test_when_finished cleanup_git&&
>   	cd "$git"&&
>   	git config git-p4.skipSubmitEditCheck true
>   	echo "username-bob: a change by bob">>  file1&&
> @@ -215,9 +214,7 @@ test_expect_success 'preserve user where author is unknown to p4' '
>   	git config git-p4.preserveUser true&&
>   	P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit&&
>   	git diff --exit-code HEAD..p4/master>  /dev/null&&
> -	p4_check_commit_author file1 alice&&
> -	cd "$TRASH_DIRECTORY"&&
> -	rm -rf "$git"&&  mkdir "$git"
> +	p4_check_commit_author file1 alice
>   '
>
>   # If we're *not* using --preserve-user, git-p4 should warn if we're submitting
> @@ -226,31 +223,29 @@ test_expect_success 'preserve user where author is unknown to p4' '
>   # Test: warning disabled and user is the same.
>   test_expect_success 'not preserving user with mixed authorship' '
>   	"$GITP4" clone --dest="$git" //depot&&
> -	(
> -		cd "$git"&&
> -		git config git-p4.skipSubmitEditCheck true&&
> -		p4_add_user derek Derek&&
> -
> -		make_change_by_user usernamefile3 Derek derek@localhost&&
> -		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit>actual&&
> -		grep "git author derek@localhost does not match" actual&&
> -
> -		make_change_by_user usernamefile3 Charlie charlie@localhost&&
> -		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit>actual&&
> -		grep "git author charlie@localhost does not match" actual&&
> -
> -		make_change_by_user usernamefile3 alice alice@localhost&&
> -		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit>actual&&
> -		! grep "git author.*does not match" actual&&
> -
> -		git config git-p4.skipUserNameCheck true&&
> -		make_change_by_user usernamefile3 Charlie charlie@localhost&&
> -		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit>actual&&
> -		! grep "git author.*does not match" actual&&
> -
> -		p4_check_commit_author usernamefile3 alice
> -	)&&
> -	rm -rf "$git"&&  mkdir "$git"
> +	test_when_finished cleanup_git&&
> +	cd "$git"&&
> +	git config git-p4.skipSubmitEditCheck true&&
> +	p4_add_user derek Derek&&
> +
> +	make_change_by_user usernamefile3 Derek derek@localhost&&
> +	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit>actual&&
> +	grep "git author derek@localhost does not match" actual&&
> +
> +	make_change_by_user usernamefile3 Charlie charlie@localhost&&
> +	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit>actual&&
> +	grep "git author charlie@localhost does not match" actual&&
> +
> +	make_change_by_user usernamefile3 alice alice@localhost&&
> +	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit>actual&&
> +	! grep "git author.*does not match" actual&&
> +
> +	git config git-p4.skipUserNameCheck true&&
> +	make_change_by_user usernamefile3 Charlie charlie@localhost&&
> +	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit>actual&&
> +	! grep "git author.*does not match" actual&&
> +
> +	p4_check_commit_author usernamefile3 alice
>   '
>
>

[PATCH 2/4] git-p4: make tests work on p4d eval server

[Pete Wyckoff]

The "evaluation" license for p4d allows only two users.  One of
the users will be auto-created as super when generating the first
commit or adding the first new user.  So we cannot have both an
Alice and Bob as well as super.  Instead, adapt the tests to work
with just a single Alice user.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9800-git-p4.sh |   44 ++++++++++++++++++++++++++++++--------------
 1 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index aec3ba1..04d8413 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -149,6 +149,13 @@ p4_grant_admin() {
 	p4 protect -i
 }
 
+p4_ungrant_admin() {
+    name=$1
+    p4 protect -o |\
+	grep -v $name |\
+	p4 protect -i
+}
+
 p4_check_commit_author() {
     file=$1
     user=$2
@@ -167,22 +174,30 @@ make_change_by_user() {
 	git commit --author "$name <$email>" -m "a change by $name"
 }
 
-# Test username support, submitting as user 'alice'
+marshal_dump() {
+	what=$1
+	python -c 'import marshal, sys; d = marshal.load(sys.stdin); print d["'$what'"]'
+}
+
+# Test username support, submitting as user 'alice'.  Second user will be
+# whoever is running these tests.  Cannot avoid having p4d auto-create that
+# user, and the eval version only supports two users.
 test_expect_success 'preserve users' '
+	bobuser=$(p4 -G users | marshal_dump User) &&
+	bobemail=$(p4 -G users | marshal_dump Email) &&
 	p4_add_user alice Alice &&
-	p4_add_user bob Bob &&
 	p4_grant_admin alice &&
 	"$GITP4" clone --dest="$git" //depot &&
 	test_when_finished cleanup_git &&
 	cd "$git" &&
 	echo "username: a change by alice" >> file1 &&
-	echo "username: a change by bob" >> file2 &&
+	echo "username: a change by $bobuser" >> file2 &&
 	git commit --author "Alice <alice@localhost>" -m "a change by alice" file1 &&
-	git commit --author "Bob <bob@localhost>" -m "a change by bob" file2 &&
+	git commit --author "$bobuser <$bobemail>" -m "a change by $bobuser" file2 &&
 	git config git-p4.skipSubmitEditCheck true &&
 	P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user &&
 	p4_check_commit_author file1 alice &&
-	p4_check_commit_author file2 bob
+	p4_check_commit_author file2 $bobuser
 '
 
 # Test username support, submitting as bob, who lacks admin rights. Should
@@ -191,9 +206,10 @@ test_expect_success 'refuse to preserve users without perms' '
 	"$GITP4" clone --dest="$git" //depot &&
 	test_when_finished cleanup_git &&
 	cd "$git" &&
-	echo "username-noperms: a change by alice" >> file1 &&
-	git commit --author "Alice <alice@localhost>" -m "perms: a change by alice" file1 &&
-	! P4EDITOR=touch P4USER=bob P4PASSWD=secret "$GITP4" commit --preserve-user &&
+	p4_ungrant_admin alice &&
+	echo "username-noperms: a change by $bobuser" >> file1 &&
+	git commit --author "$bobuser <$bobemail>" -m "perms: a change by $bobuser" file1 &&
+	! P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user &&
 	! git diff --exit-code HEAD..p4/master > /dev/null
 '
 
@@ -202,9 +218,10 @@ test_expect_success 'preserve user where author is unknown to p4' '
 	"$GITP4" clone --dest="$git" //depot &&
 	test_when_finished cleanup_git &&
 	cd "$git" &&
-	git config git-p4.skipSubmitEditCheck true
-	echo "username-bob: a change by bob" >> file1 &&
-	git commit --author "Bob <bob@localhost>" -m "preserve: a change by bob" file1 &&
+	git config git-p4.skipSubmitEditCheck true &&
+	p4_grant_admin alice &&
+	echo "username-bob: a change by $bobuser" >> file1 &&
+	git commit --author "$bobuser <$bobemail>" -m "preserve: a change by $bobuser" file1 &&
 	echo "username-unknown: a change by charlie" >> file1 &&
 	git commit --author "Charlie <charlie@localhost>" -m "preserve: a change by charlie" file1 &&
 	! P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user &&
@@ -226,11 +243,10 @@ test_expect_success 'not preserving user with mixed authorship' '
 	test_when_finished cleanup_git &&
 	cd "$git" &&
 	git config git-p4.skipSubmitEditCheck true &&
-	p4_add_user derek Derek &&
 
-	make_change_by_user usernamefile3 Derek derek@localhost &&
+	make_change_by_user usernamefile3 $bobuser $bobemail &&
 	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
-	grep "git author derek@localhost does not match" actual &&
+	grep "git author $bobemail does not match" actual &&
 
 	make_change_by_user usernamefile3 Charlie charlie@localhost &&
 	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
-- 
1.7.5.4

[PATCH 3/4] git-p4: one test missing config git-p4.skipSubmitEditCheck

[Pete Wyckoff]

Add this missing line in one of the tests.  Otherwise, on fast
machines, the following git-p4 commit will complain that nobody
edited the submission message.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9800-git-p4.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index 04d8413..24a8b79 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -207,6 +207,7 @@ test_expect_success 'refuse to preserve users without perms' '
 	test_when_finished cleanup_git &&
 	cd "$git" &&
 	p4_ungrant_admin alice &&
+	git config git-p4.skipSubmitEditCheck true &&
 	echo "username-noperms: a change by $bobuser" >> file1 &&
 	git commit --author "$bobuser <$bobemail>" -m "perms: a change by $bobuser" file1 &&
 	! P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user &&
-- 
1.7.5.4

Re: [PATCH 3/4] git-p4: one test missing config git-p4.skipSubmitEditCheck

[Luke Diamand]

On 31/07/11 01:39, Pete Wyckoff wrote:
> Add this missing line in one of the tests.  Otherwise, on fast
> machines, the following git-p4 commit will complain that nobody
> edited the submission message.

Thanks for fixing this.

Acked.


>
> Signed-off-by: Pete Wyckoff<pw@padd.com>
> ---
>   t/t9800-git-p4.sh |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
> index 04d8413..24a8b79 100755
> --- a/t/t9800-git-p4.sh
> +++ b/t/t9800-git-p4.sh
> @@ -207,6 +207,7 @@ test_expect_success 'refuse to preserve users without perms' '
>   	test_when_finished cleanup_git&&
>   	cd "$git"&&
>   	p4_ungrant_admin alice&&
> +	git config git-p4.skipSubmitEditCheck true&&
>   	echo "username-noperms: a change by $bobuser">>  file1&&
>   	git commit --author "$bobuser<$bobemail>" -m "perms: a change by $bobuser" file1&&
>   	! P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user&&

[PATCH 4/4] git-p4: commit time should be most recent p4 change time

[Pete Wyckoff]

When importing a repo, the time on the initial commit had been
just "now".  But this causes problems when trying to share among
git-p4 repos that were created identically, although at different
times.  Instead, use the time in the top-most p4 change as the
time for the git import commit.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4 |   15 ++++++++++++++-
 t/t9800-git-p4.sh          |   14 ++++++++++++++
 2 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 98d2aee..6b9de9e 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1649,7 +1649,8 @@ class P4Sync(Command, P4UserMap):
     def importHeadRevision(self, revision):
         print "Doing initial import of %s from revision %s into %s" % (' '.join(self.depotPaths), revision, self.branch)
 
-        details = { "user" : "git perforce import user", "time" : int(time.time()) }
+        details = {}
+        details["user"] = "git perforce import user"
         details["desc"] = ("Initial import of %s from the state at revision %s\n"
                            % (' '.join(self.depotPaths), revision))
         details["change"] = revision
@@ -1689,6 +1690,18 @@ class P4Sync(Command, P4UserMap):
             fileCnt = fileCnt + 1
 
         details["change"] = newestRevision
+
+        # Use time from top-most change so that all git-p4 clones of
+        # the same p4 repo have the same commit SHA1s.
+        res = p4CmdList("describe -s %d" % newestRevision)
+        newestTime = None
+        for r in res:
+            if r.has_key('time'):
+                newestTime = int(r['time'])
+        if newestTime is None:
+            die("\"describe -s\" on newest change %d did not give a time")
+        details["time"] = newestTime
+
         self.updateOptionDict(details)
         try:
             self.commit(details, self.extractFilesFromCommit(details), self.branch, self.depotPaths)
diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index 24a8b79..1b5f4b2 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -265,6 +265,20 @@ test_expect_success 'not preserving user with mixed authorship' '
 	p4_check_commit_author usernamefile3 alice
 '
 
+# Sleep a bit so that the top-most p4 change did not happen "now".  Then
+# import the repo and make sure that the initial import has the same time
+# as the top-most change.
+test_expect_success 'initial import time from top change time' '
+	p4change=$(p4 -G changes -m 1 //depot/... | marshal_dump change) &&
+	p4time=$(p4 -G changes -m 1 //depot/... | marshal_dump time) &&
+	sleep 3 &&
+	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
+	cd "$git" &&
+	gittime=$(git show -s --raw --pretty=format:%at HEAD) &&
+	echo $p4time $gittime &&
+	test $p4time = $gittime
+'
 
 test_expect_success 'shutdown' '
 	pid=`pgrep -f p4d` &&
-- 
1.7.5.4

Re: [PATCH 4/4] git-p4: commit time should be most recent p4 change time

[Luke Diamand]

On 31/07/11 01:39, Pete Wyckoff wrote:
> When importing a repo, the time on the initial commit had been
> just "now".  But this causes problems when trying to share among
> git-p4 repos that were created identically, although at different
> times.  Instead, use the time in the top-most p4 change as the
> time for the git import commit.

Ack.


>
> Signed-off-by: Pete Wyckoff<pw@padd.com>
> ---
>   contrib/fast-import/git-p4 |   15 ++++++++++++++-
>   t/t9800-git-p4.sh          |   14 ++++++++++++++
>   2 files changed, 28 insertions(+), 1 deletions(-)
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index 98d2aee..6b9de9e 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -1649,7 +1649,8 @@ class P4Sync(Command, P4UserMap):
>       def importHeadRevision(self, revision):
>           print "Doing initial import of %s from revision %s into %s" % (' '.join(self.depotPaths), revision, self.branch)
>
> -        details = { "user" : "git perforce import user", "time" : int(time.time()) }
> +        details = {}
> +        details["user"] = "git perforce import user"
>           details["desc"] = ("Initial import of %s from the state at revision %s\n"
>                              % (' '.join(self.depotPaths), revision))
>           details["change"] = revision
> @@ -1689,6 +1690,18 @@ class P4Sync(Command, P4UserMap):
>               fileCnt = fileCnt + 1
>
>           details["change"] = newestRevision
> +
> +        # Use time from top-most change so that all git-p4 clones of
> +        # the same p4 repo have the same commit SHA1s.
> +        res = p4CmdList("describe -s %d" % newestRevision)
> +        newestTime = None
> +        for r in res:
> +            if r.has_key('time'):
> +                newestTime = int(r['time'])
> +        if newestTime is None:
> +            die("\"describe -s\" on newest change %d did not give a time")
> +        details["time"] = newestTime
> +
>           self.updateOptionDict(details)
>           try:
>               self.commit(details, self.extractFilesFromCommit(details), self.branch, self.depotPaths)
> diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
> index 24a8b79..1b5f4b2 100755
> --- a/t/t9800-git-p4.sh
> +++ b/t/t9800-git-p4.sh
> @@ -265,6 +265,20 @@ test_expect_success 'not preserving user with mixed authorship' '
>   	p4_check_commit_author usernamefile3 alice
>   '
>
> +# Sleep a bit so that the top-most p4 change did not happen "now".  Then
> +# import the repo and make sure that the initial import has the same time
> +# as the top-most change.
> +test_expect_success 'initial import time from top change time' '
> +	p4change=$(p4 -G changes -m 1 //depot/... | marshal_dump change)&&
> +	p4time=$(p4 -G changes -m 1 //depot/... | marshal_dump time)&&
> +	sleep 3&&
> +	"$GITP4" clone --dest="$git" //depot&&
> +	test_when_finished cleanup_git&&
> +	cd "$git"&&
> +	gittime=$(git show -s --raw --pretty=format:%at HEAD)&&
> +	echo $p4time $gittime&&
> +	test $p4time = $gittime
> +'
>
>   test_expect_success 'shutdown' '
>   	pid=`pgrep -f p4d`&&

Re: [PATCH 0/4] git-p4 test cleanup, commit time change

[Luke Diamand]

On 31/07/11 01:35, Pete Wyckoff wrote:
> Here's a small patch series to clean up the git-p4 tests a bit,
> make them work on a 2-user eval p4d, and to fix a problem with
> the timestamp on the import commit.
>
> Luke, if you can take a look and ack these, I'd appreciate
> the review since I had to adjust your --preserveUser tests.
> And if you're looking at that one, might as well look at them
> all.  :)
>
> I'd welcome review from anyone with interest, and will plan
> to submit these to Junio in a week or so.

Just looking at them now.

One thing though - are you sure the p4d demo license only allows 2 
users? I've only ever used the demo p4d for testing and it seems to work 
fine (just tried again with version P4D/LINUX26X86_64/2010.2/322263).

The web page says this:

"The free Perforce Server supports two users and five client workspaces, 
or unlimited users and up to 1,000 files. Request a 45-day evaluation 
license to support any number of users and unlimited files."

Which I assume means the test harness couldn't have more than 1000 files 
and some unspecified number of client workspaces?

I've just created 4 users by hand with no obvious problems. I'm pretty 
sure I'm not accidentally using the corporate server!

Confuzzled!

Re: [PATCH 0/4] git-p4 test cleanup, commit time change

[Pete Wyckoff]

luke@diamand.org wrote on Sun, 31 Jul 2011 10:49 +0100:
> One thing though - are you sure the p4d demo license only allows 2
> users? I've only ever used the demo p4d for testing and it seems to
> work fine (just tried again with version
> P4D/LINUX26X86_64/2010.2/322263).

You are so right.  I was using an older p4d (2009.1/222893) at
home, and didn't even think about checking for updates.

There was one line in patch 3 that should survive, just to add a
"&&".  I've added your acks for the rest and will repost to show
the current changes.

Thanks,

		-- Pete

Re: [PATCH 0/4] git-p4 test cleanup, commit time change

[Luke Diamand]

On 31/07/11 14:42, Pete Wyckoff wrote:
> luke@diamand.org wrote on Sun, 31 Jul 2011 10:49 +0100:
>> One thing though - are you sure the p4d demo license only allows 2
>> users? I've only ever used the demo p4d for testing and it seems to
>> work fine (just tried again with version
>> P4D/LINUX26X86_64/2010.2/322263).
>
> You are so right.  I was using an older p4d (2009.1/222893) at
> home, and didn't even think about checking for updates.
>
> There was one line in patch 3 that should survive, just to add a
> "&&".  I've added your acks for the rest and will repost to show
> the current changes.

All looks good to me!

Luke

[PATCHv2 0/4] git-p4 test cleanup, commit time change

[Pete Wyckoff]

Re-spun, removing all of the original patch 2 now that Luke
pointed out that a new p4d is available.

Original intro:

    Here's a small patch series to clean up the git-p4 tests a bit,
    make them work on a 2-user eval p4d, and to fix a problem with
    the timestamp on the import commit.

[PATCHv2 1/4] git-p4: use test_when_finished in tests

[Pete Wyckoff]

Cleanup nicely when tests fail.  This avoids many duplicated
lines in the tests, and adds cleanup in a couple of tests that
did not have it.  When one fails, now all the rest will not
fail too.

Signed-off-by: Pete Wyckoff <pw@padd.com>
Acked-by: Luke Diamand <luke@diamand.org>
---
 t/t9800-git-p4.sh |  103 +++++++++++++++++++++++++----------------------------
 1 files changed, 49 insertions(+), 54 deletions(-)

diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index 33b0127..aec3ba1 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -45,29 +45,33 @@ test_expect_success 'add p4 files' '
 	cd "$TRASH_DIRECTORY"
 '
 
+cleanup_git() {
+	cd "$TRASH_DIRECTORY" &&
+	rm -rf "$git" &&
+	mkdir "$git"
+}
+
 test_expect_success 'basic git-p4 clone' '
 	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	git log --oneline >lines &&
-	test_line_count = 1 lines &&
-	cd .. &&
-	rm -rf "$git" && mkdir "$git"
+	test_line_count = 1 lines
 '
 
 test_expect_success 'git-p4 clone @all' '
 	"$GITP4" clone --dest="$git" //depot@all &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	git log --oneline >lines &&
-	test_line_count = 2 lines &&
-	cd .. &&
-	rm -rf "$git" && mkdir "$git"
+	test_line_count = 2 lines
 '
 
 test_expect_success 'git-p4 sync uninitialized repo' '
 	test_create_repo "$git" &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
-	test_must_fail "$GITP4" sync &&
-	rm -rf "$git" && mkdir "$git"
+	test_must_fail "$GITP4" sync
 '
 
 #
@@ -76,19 +80,18 @@ test_expect_success 'git-p4 sync uninitialized repo' '
 #
 test_expect_success 'git-p4 sync new branch' '
 	test_create_repo "$git" &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	test_commit head &&
 	"$GITP4" sync --branch=refs/remotes/p4/depot //depot@all &&
 	git log --oneline p4/depot >lines &&
-	cat lines &&
-	test_line_count = 2 lines &&
-	cd .. &&
-	rm -rf "$git" && mkdir "$git"
+	test_line_count = 2 lines
 '
 
 test_expect_success 'exit when p4 fails to produce marshaled output' '
 	badp4dir="$TRASH_DIRECTORY/badp4dir" &&
 	mkdir -p "$badp4dir" &&
+	test_when_finished "rm -rf $badp4dir" &&
 	cat >"$badp4dir"/p4 <<-EOF &&
 	#!$SHELL_PATH
 	exit 1
@@ -106,29 +109,26 @@ test_expect_success 'add p4 files with wildcards in the names' '
 	echo file-wild-at >file-wild@at &&
 	echo file-wild-percent >file-wild%percent &&
 	p4 add -f file-wild* &&
-	p4 submit -d "file wildcards" &&
-	cd "$TRASH_DIRECTORY"
+	p4 submit -d "file wildcards"
 '
 
 test_expect_success 'wildcard files git-p4 clone' '
 	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	test -f file-wild#hash &&
 	test -f file-wild\*star &&
 	test -f file-wild@at &&
-	test -f file-wild%percent &&
-	cd "$TRASH_DIRECTORY" &&
-	rm -rf "$git" && mkdir "$git"
+	test -f file-wild%percent
 '
 
 test_expect_success 'clone bare' '
 	"$GITP4" clone --dest="$git" --bare //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	test ! -d .git &&
 	bare=`git config --get core.bare` &&
-	test "$bare" = true &&
-	cd "$TRASH_DIRECTORY" &&
-	rm -rf "$git" && mkdir "$git"
+	test "$bare" = true
 '
 
 p4_add_user() {
@@ -173,6 +173,7 @@ test_expect_success 'preserve users' '
 	p4_add_user bob Bob &&
 	p4_grant_admin alice &&
 	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	echo "username: a change by alice" >> file1 &&
 	echo "username: a change by bob" >> file2 &&
@@ -181,27 +182,25 @@ test_expect_success 'preserve users' '
 	git config git-p4.skipSubmitEditCheck true &&
 	P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user &&
 	p4_check_commit_author file1 alice &&
-	p4_check_commit_author file2 bob &&
-	cd "$TRASH_DIRECTORY" &&
-	rm -rf "$git" && mkdir "$git"
+	p4_check_commit_author file2 bob
 '
 
 # Test username support, submitting as bob, who lacks admin rights. Should
 # not submit change to p4 (git diff should show deltas).
 test_expect_success 'refuse to preserve users without perms' '
 	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	echo "username-noperms: a change by alice" >> file1 &&
 	git commit --author "Alice <alice@localhost>" -m "perms: a change by alice" file1 &&
 	! P4EDITOR=touch P4USER=bob P4PASSWD=secret "$GITP4" commit --preserve-user &&
-	! git diff --exit-code HEAD..p4/master > /dev/null &&
-	cd "$TRASH_DIRECTORY" &&
-	rm -rf "$git" && mkdir "$git"
+	! git diff --exit-code HEAD..p4/master > /dev/null
 '
 
 # What happens with unknown author? Without allowMissingP4Users it should fail.
 test_expect_success 'preserve user where author is unknown to p4' '
 	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	git config git-p4.skipSubmitEditCheck true
 	echo "username-bob: a change by bob" >> file1 &&
@@ -215,9 +214,7 @@ test_expect_success 'preserve user where author is unknown to p4' '
 	git config git-p4.preserveUser true &&
 	P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit &&
 	git diff --exit-code HEAD..p4/master > /dev/null &&
-	p4_check_commit_author file1 alice &&
-	cd "$TRASH_DIRECTORY" &&
-	rm -rf "$git" && mkdir "$git"
+	p4_check_commit_author file1 alice
 '
 
 # If we're *not* using --preserve-user, git-p4 should warn if we're submitting
@@ -226,31 +223,29 @@ test_expect_success 'preserve user where author is unknown to p4' '
 # Test: warning disabled and user is the same.
 test_expect_success 'not preserving user with mixed authorship' '
 	"$GITP4" clone --dest="$git" //depot &&
-	(
-		cd "$git" &&
-		git config git-p4.skipSubmitEditCheck true &&
-		p4_add_user derek Derek &&
-
-		make_change_by_user usernamefile3 Derek derek@localhost &&
-		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
-		grep "git author derek@localhost does not match" actual &&
-
-		make_change_by_user usernamefile3 Charlie charlie@localhost &&
-		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
-		grep "git author charlie@localhost does not match" actual &&
-
-		make_change_by_user usernamefile3 alice alice@localhost &&
-		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
-		! grep "git author.*does not match" actual &&
-
-		git config git-p4.skipUserNameCheck true &&
-		make_change_by_user usernamefile3 Charlie charlie@localhost &&
-		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
-		! grep "git author.*does not match" actual &&
-
-		p4_check_commit_author usernamefile3 alice
-	) &&
-	rm -rf "$git" && mkdir "$git"
+	test_when_finished cleanup_git &&
+	cd "$git" &&
+	git config git-p4.skipSubmitEditCheck true &&
+	p4_add_user derek Derek &&
+
+	make_change_by_user usernamefile3 Derek derek@localhost &&
+	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
+	grep "git author derek@localhost does not match" actual &&
+
+	make_change_by_user usernamefile3 Charlie charlie@localhost &&
+	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
+	grep "git author charlie@localhost does not match" actual &&
+
+	make_change_by_user usernamefile3 alice alice@localhost &&
+	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
+	! grep "git author.*does not match" actual &&
+
+	git config git-p4.skipUserNameCheck true &&
+	make_change_by_user usernamefile3 Charlie charlie@localhost &&
+	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
+	! grep "git author.*does not match" actual &&
+
+	p4_check_commit_author usernamefile3 alice
 '
 
 
-- 
1.7.5.4

[PATCHv2 2/4] git-p4: add missing && in test

[Pete Wyckoff]

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9800-git-p4.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index aec3ba1..b7eda82 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -202,7 +202,7 @@ test_expect_success 'preserve user where author is unknown to p4' '
 	"$GITP4" clone --dest="$git" //depot &&
 	test_when_finished cleanup_git &&
 	cd "$git" &&
-	git config git-p4.skipSubmitEditCheck true
+	git config git-p4.skipSubmitEditCheck true &&
 	echo "username-bob: a change by bob" >> file1 &&
 	git commit --author "Bob <bob@localhost>" -m "preserve: a change by bob" file1 &&
 	echo "username-unknown: a change by charlie" >> file1 &&
-- 
1.7.5.4

Re: [PATCHv2 2/4] git-p4: add missing && in test

[Luke Diamand]

Acked.



On 31/07/11 14:45, Pete Wyckoff wrote:
> Signed-off-by: Pete Wyckoff<pw@padd.com>
> ---
>   t/t9800-git-p4.sh |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
> index aec3ba1..b7eda82 100755
> --- a/t/t9800-git-p4.sh
> +++ b/t/t9800-git-p4.sh
> @@ -202,7 +202,7 @@ test_expect_success 'preserve user where author is unknown to p4' '
>   	"$GITP4" clone --dest="$git" //depot&&
>   	test_when_finished cleanup_git&&
>   	cd "$git"&&
> -	git config git-p4.skipSubmitEditCheck true
> +	git config git-p4.skipSubmitEditCheck true&&
>   	echo "username-bob: a change by bob">>  file1&&
>   	git commit --author "Bob<bob@localhost>" -m "preserve: a change by bob" file1&&
>   	echo "username-unknown: a change by charlie">>  file1&&

[PATCHv2 3/4] git-p4: one test missing config git-p4.skipSubmitEditCheck

[Pete Wyckoff]

Add this missing line in one of the tests.  Otherwise, on fast
machines, the following git-p4 commit will complain that nobody
edited the submission message.

Signed-off-by: Pete Wyckoff <pw@padd.com>
Acked-by: Luke Diamand <luke@diamand.org>
---
 t/t9800-git-p4.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index b7eda82..b304707 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -191,6 +191,7 @@ test_expect_success 'refuse to preserve users without perms' '
 	"$GITP4" clone --dest="$git" //depot &&
 	test_when_finished cleanup_git &&
 	cd "$git" &&
+	git config git-p4.skipSubmitEditCheck true &&
 	echo "username-noperms: a change by alice" >> file1 &&
 	git commit --author "Alice <alice@localhost>" -m "perms: a change by alice" file1 &&
 	! P4EDITOR=touch P4USER=bob P4PASSWD=secret "$GITP4" commit --preserve-user &&
-- 
1.7.5.4

[PATCHv2 4/4] git-p4: commit time should be most recent p4 change time

[Pete Wyckoff]

When importing a repo, the time on the initial commit had been
just "now".  But this causes problems when trying to share among
git-p4 repos that were created identically, although at different
times.  Instead, use the time in the top-most p4 change as the
time for the git import commit.

Signed-off-by: Pete Wyckoff <pw@padd.com>
Acked-by: Luke Diamand <luke@diamand.org>
---
 contrib/fast-import/git-p4 |   15 ++++++++++++++-
 t/t9800-git-p4.sh          |   19 +++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 98d2aee..6b9de9e 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1649,7 +1649,8 @@ class P4Sync(Command, P4UserMap):
     def importHeadRevision(self, revision):
         print "Doing initial import of %s from revision %s into %s" % (' '.join(self.depotPaths), revision, self.branch)
 
-        details = { "user" : "git perforce import user", "time" : int(time.time()) }
+        details = {}
+        details["user"] = "git perforce import user"
         details["desc"] = ("Initial import of %s from the state at revision %s\n"
                            % (' '.join(self.depotPaths), revision))
         details["change"] = revision
@@ -1689,6 +1690,18 @@ class P4Sync(Command, P4UserMap):
             fileCnt = fileCnt + 1
 
         details["change"] = newestRevision
+
+        # Use time from top-most change so that all git-p4 clones of
+        # the same p4 repo have the same commit SHA1s.
+        res = p4CmdList("describe -s %d" % newestRevision)
+        newestTime = None
+        for r in res:
+            if r.has_key('time'):
+                newestTime = int(r['time'])
+        if newestTime is None:
+            die("\"describe -s\" on newest change %d did not give a time")
+        details["time"] = newestTime
+
         self.updateOptionDict(details)
         try:
             self.commit(details, self.extractFilesFromCommit(details), self.branch, self.depotPaths)
diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index b304707..97ec975 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -249,6 +249,25 @@ test_expect_success 'not preserving user with mixed authorship' '
 	p4_check_commit_author usernamefile3 alice
 '
 
+marshal_dump() {
+	what=$1
+	python -c 'import marshal, sys; d = marshal.load(sys.stdin); print d["'$what'"]'
+}
+
+# Sleep a bit so that the top-most p4 change did not happen "now".  Then
+# import the repo and make sure that the initial import has the same time
+# as the top-most change.
+test_expect_success 'initial import time from top change time' '
+	p4change=$(p4 -G changes -m 1 //depot/... | marshal_dump change) &&
+	p4time=$(p4 -G changes -m 1 //depot/... | marshal_dump time) &&
+	sleep 3 &&
+	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
+	cd "$git" &&
+	gittime=$(git show -s --raw --pretty=format:%at HEAD) &&
+	echo $p4time $gittime &&
+	test $p4time = $gittime
+'
 
 test_expect_success 'shutdown' '
 	pid=`pgrep -f p4d` &&
-- 
1.7.5.4

[PATCH 0/4] git-p4 test cleanup, commit time change

[Pete Wyckoff]

Here's a small patch series to clean up the git-p4 tests a bit,
make them work on a 2-user eval p4d, and to fix a problem with
the timestamp on the import commit.

 [PATCH 1/4] git-p4: use test_when_finished in tests
 [PATCH 2/4] git-p4: add missing && in test
 [PATCH 3/4] git-p4: one test missing config
 [PATCH 4/4] git-p4: commit time should be most recent p4 change time

 contrib/fast-import/git-p4 |   15 +++++
 t/t9800-git-p4.sh          |  125 +++++++++++++++++++++++++--------------------
 2 files changed, 84 insertions(+), 56 deletions(-)

Luke acked them, and I've been using it for a couple of weeks.  Could you
include them in the next convenient release?  Thanks,

		-- Pete

[PATCH 1/4] git-p4: use test_when_finished in tests

[Pete Wyckoff]

Cleanup nicely when tests fail.  This avoids many duplicated
lines in the tests, and adds cleanup in a couple of tests that
did not have it.  When one fails, now all the rest will not
fail too.

Signed-off-by: Pete Wyckoff <pw@padd.com>
Acked-by: Luke Diamand <luke@diamand.org>
---
 t/t9800-git-p4.sh |  103 +++++++++++++++++++++++++----------------------------
 1 files changed, 49 insertions(+), 54 deletions(-)

diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index 33b0127..aec3ba1 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -45,29 +45,33 @@ test_expect_success 'add p4 files' '
 	cd "$TRASH_DIRECTORY"
 '
 
+cleanup_git() {
+	cd "$TRASH_DIRECTORY" &&
+	rm -rf "$git" &&
+	mkdir "$git"
+}
+
 test_expect_success 'basic git-p4 clone' '
 	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	git log --oneline >lines &&
-	test_line_count = 1 lines &&
-	cd .. &&
-	rm -rf "$git" && mkdir "$git"
+	test_line_count = 1 lines
 '
 
 test_expect_success 'git-p4 clone @all' '
 	"$GITP4" clone --dest="$git" //depot@all &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	git log --oneline >lines &&
-	test_line_count = 2 lines &&
-	cd .. &&
-	rm -rf "$git" && mkdir "$git"
+	test_line_count = 2 lines
 '
 
 test_expect_success 'git-p4 sync uninitialized repo' '
 	test_create_repo "$git" &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
-	test_must_fail "$GITP4" sync &&
-	rm -rf "$git" && mkdir "$git"
+	test_must_fail "$GITP4" sync
 '
 
 #
@@ -76,19 +80,18 @@ test_expect_success 'git-p4 sync uninitialized repo' '
 #
 test_expect_success 'git-p4 sync new branch' '
 	test_create_repo "$git" &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	test_commit head &&
 	"$GITP4" sync --branch=refs/remotes/p4/depot //depot@all &&
 	git log --oneline p4/depot >lines &&
-	cat lines &&
-	test_line_count = 2 lines &&
-	cd .. &&
-	rm -rf "$git" && mkdir "$git"
+	test_line_count = 2 lines
 '
 
 test_expect_success 'exit when p4 fails to produce marshaled output' '
 	badp4dir="$TRASH_DIRECTORY/badp4dir" &&
 	mkdir -p "$badp4dir" &&
+	test_when_finished "rm -rf $badp4dir" &&
 	cat >"$badp4dir"/p4 <<-EOF &&
 	#!$SHELL_PATH
 	exit 1
@@ -106,29 +109,26 @@ test_expect_success 'add p4 files with wildcards in the names' '
 	echo file-wild-at >file-wild@at &&
 	echo file-wild-percent >file-wild%percent &&
 	p4 add -f file-wild* &&
-	p4 submit -d "file wildcards" &&
-	cd "$TRASH_DIRECTORY"
+	p4 submit -d "file wildcards"
 '
 
 test_expect_success 'wildcard files git-p4 clone' '
 	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	test -f file-wild#hash &&
 	test -f file-wild\*star &&
 	test -f file-wild@at &&
-	test -f file-wild%percent &&
-	cd "$TRASH_DIRECTORY" &&
-	rm -rf "$git" && mkdir "$git"
+	test -f file-wild%percent
 '
 
 test_expect_success 'clone bare' '
 	"$GITP4" clone --dest="$git" --bare //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	test ! -d .git &&
 	bare=`git config --get core.bare` &&
-	test "$bare" = true &&
-	cd "$TRASH_DIRECTORY" &&
-	rm -rf "$git" && mkdir "$git"
+	test "$bare" = true
 '
 
 p4_add_user() {
@@ -173,6 +173,7 @@ test_expect_success 'preserve users' '
 	p4_add_user bob Bob &&
 	p4_grant_admin alice &&
 	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	echo "username: a change by alice" >> file1 &&
 	echo "username: a change by bob" >> file2 &&
@@ -181,27 +182,25 @@ test_expect_success 'preserve users' '
 	git config git-p4.skipSubmitEditCheck true &&
 	P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user &&
 	p4_check_commit_author file1 alice &&
-	p4_check_commit_author file2 bob &&
-	cd "$TRASH_DIRECTORY" &&
-	rm -rf "$git" && mkdir "$git"
+	p4_check_commit_author file2 bob
 '
 
 # Test username support, submitting as bob, who lacks admin rights. Should
 # not submit change to p4 (git diff should show deltas).
 test_expect_success 'refuse to preserve users without perms' '
 	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	echo "username-noperms: a change by alice" >> file1 &&
 	git commit --author "Alice <alice@localhost>" -m "perms: a change by alice" file1 &&
 	! P4EDITOR=touch P4USER=bob P4PASSWD=secret "$GITP4" commit --preserve-user &&
-	! git diff --exit-code HEAD..p4/master > /dev/null &&
-	cd "$TRASH_DIRECTORY" &&
-	rm -rf "$git" && mkdir "$git"
+	! git diff --exit-code HEAD..p4/master > /dev/null
 '
 
 # What happens with unknown author? Without allowMissingP4Users it should fail.
 test_expect_success 'preserve user where author is unknown to p4' '
 	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
 	cd "$git" &&
 	git config git-p4.skipSubmitEditCheck true
 	echo "username-bob: a change by bob" >> file1 &&
@@ -215,9 +214,7 @@ test_expect_success 'preserve user where author is unknown to p4' '
 	git config git-p4.preserveUser true &&
 	P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit &&
 	git diff --exit-code HEAD..p4/master > /dev/null &&
-	p4_check_commit_author file1 alice &&
-	cd "$TRASH_DIRECTORY" &&
-	rm -rf "$git" && mkdir "$git"
+	p4_check_commit_author file1 alice
 '
 
 # If we're *not* using --preserve-user, git-p4 should warn if we're submitting
@@ -226,31 +223,29 @@ test_expect_success 'preserve user where author is unknown to p4' '
 # Test: warning disabled and user is the same.
 test_expect_success 'not preserving user with mixed authorship' '
 	"$GITP4" clone --dest="$git" //depot &&
-	(
-		cd "$git" &&
-		git config git-p4.skipSubmitEditCheck true &&
-		p4_add_user derek Derek &&
-
-		make_change_by_user usernamefile3 Derek derek@localhost &&
-		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
-		grep "git author derek@localhost does not match" actual &&
-
-		make_change_by_user usernamefile3 Charlie charlie@localhost &&
-		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
-		grep "git author charlie@localhost does not match" actual &&
-
-		make_change_by_user usernamefile3 alice alice@localhost &&
-		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
-		! grep "git author.*does not match" actual &&
-
-		git config git-p4.skipUserNameCheck true &&
-		make_change_by_user usernamefile3 Charlie charlie@localhost &&
-		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
-		! grep "git author.*does not match" actual &&
-
-		p4_check_commit_author usernamefile3 alice
-	) &&
-	rm -rf "$git" && mkdir "$git"
+	test_when_finished cleanup_git &&
+	cd "$git" &&
+	git config git-p4.skipSubmitEditCheck true &&
+	p4_add_user derek Derek &&
+
+	make_change_by_user usernamefile3 Derek derek@localhost &&
+	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
+	grep "git author derek@localhost does not match" actual &&
+
+	make_change_by_user usernamefile3 Charlie charlie@localhost &&
+	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
+	grep "git author charlie@localhost does not match" actual &&
+
+	make_change_by_user usernamefile3 alice alice@localhost &&
+	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
+	! grep "git author.*does not match" actual &&
+
+	git config git-p4.skipUserNameCheck true &&
+	make_change_by_user usernamefile3 Charlie charlie@localhost &&
+	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
+	! grep "git author.*does not match" actual &&
+
+	p4_check_commit_author usernamefile3 alice
 '
 
 
-- 
1.7.5.4