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

 [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

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

[Pete Wyckoff]

Signed-off-by: Pete Wyckoff <pw@padd.com>
Acked-by: Luke Diamand <luke@diamand.org>
---
 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

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

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

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