[PATCH v3 0/6] git-p4 tests, filetypes, shell metacharacters

[PATCH v3 0/6] git-p4 tests, filetypes, shell metacharacters

[Pete Wyckoff]

This series of six patches has been reviewed on the mailing list
and should be ready for inclusion.  Changes from v2 to v3 are
shell style and compatibility issues helpfully pointed out by
Stefano.

1    - Clean up git-p4 tests, including Junio review comments

2..4 - Handle p4 filetypes better, including Chris's suggested
       utf16 fix

5    - Fix p4 keyword parsing

6    - From Luke: avoid using the shell, so filenames with metacharacters
       are not errantly expanded

Luke Diamand (1):
      git-p4: handle files with shell metacharacters

Pete Wyckoff (5):
      git-p4 tests: refactor and cleanup
      git-p4: handle utf16 filetype properly
      git-p4: recognize all p4 filetypes
      git-p4: stop ignoring apple filetype
      git-p4: keyword flattening fixes

 contrib/fast-import/git-p4     |  287 ++++++++++++++--------
 t/lib-git-p4.sh                |   74 ++++++
 t/t9800-git-p4.sh              |  544 +++++++++++++++++-----------------------
 t/t9801-git-p4-branch.sh       |  233 +++++++++++++++++
 t/t9802-git-p4-filetype.sh     |  108 ++++++++
 t/t9803-git-shell-metachars.sh |   64 +++++
 6 files changed, 903 insertions(+), 407 deletions(-)
 create mode 100644 t/lib-git-p4.sh
 create mode 100755 t/t9801-git-p4-branch.sh
 create mode 100755 t/t9802-git-p4-filetype.sh
 create mode 100755 t/t9803-git-shell-metachars.sh

[PATCH 1/6] git-p4 tests: refactor and cleanup

[Pete Wyckoff]

Introduce a library for functions that are common to
multiple git-p4 test files.

Be a bit more clever about starting and stopping p4d.
Specify a unique port number for each test, so that
tests can run in parallel.  Start p4d not in daemon mode,
and save the pid, to be able to kill it cleanly later.
Never kill p4d at startup; always shutdown cleanly.

Handle directory changes better.  Always chdir inside
a subshell, and remove any post-test directory changes.

Clean up whitespace, and use test_cmp and test_must_fail
more consistently.

Separate the tests related to detecting p4 branches
into their own file, and add a few more.

Acked-by: Luke Diamand <luke@diamand.org>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/lib-git-p4.sh          |   74 +++++++
 t/t9800-git-p4.sh        |  544 ++++++++++++++++++++--------------------------
 t/t9801-git-p4-branch.sh |  233 ++++++++++++++++++++
 3 files changed, 543 insertions(+), 308 deletions(-)
 create mode 100644 t/lib-git-p4.sh
 create mode 100755 t/t9801-git-p4-branch.sh

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
new file mode 100644
index 0000000..a870f9a
--- /dev/null
+++ b/t/lib-git-p4.sh
@@ -0,0 +1,74 @@
+#
+# Library code for git-p4 tests
+#
+
+. ./test-lib.sh
+
+if ! test_have_prereq PYTHON; then
+	skip_all='skipping git-p4 tests; python not available'
+	test_done
+fi
+( p4 -h && p4d -h ) >/dev/null 2>&1 || {
+	skip_all='skipping git-p4 tests; no p4 or p4d'
+	test_done
+}
+
+GITP4="$GIT_BUILD_DIR/contrib/fast-import/git-p4"
+
+# Try to pick a unique port: guess a large number, then hope
+# no more than one of each test is running.
+#
+# This does not handle the case where somebody else is running the
+# same tests and has chosen the same ports.
+testid=${this_test#t}
+git_p4_test_start=9800
+P4DPORT=$((10669 + ($testid - $git_p4_test_start)))
+
+export P4PORT=localhost:$P4DPORT
+export P4CLIENT=client
+
+db="$TRASH_DIRECTORY/db"
+cli="$TRASH_DIRECTORY/cli"
+git="$TRASH_DIRECTORY/git"
+pidfile="$TRASH_DIRECTORY/p4d.pid"
+
+start_p4d() {
+	mkdir -p "$db" "$cli" "$git" &&
+	(
+		p4d -q -r "$db" -p $P4DPORT &
+		echo $! >"$pidfile"
+	) &&
+	for i in 1 2 3 4 5 ; do
+		p4 info >/dev/null 2>&1 && break || true &&
+		echo waiting for p4d to start &&
+		sleep 1
+	done &&
+	# complain if it never started
+	p4 info >/dev/null &&
+	(
+		cd "$cli" &&
+		p4 client -i <<-EOF
+		Client: client
+		Description: client
+		Root: $cli
+		View: //depot/... //client/...
+		EOF
+	)
+}
+
+kill_p4d() {
+	pid=$(cat "$pidfile")
+	# it had better exist for the first kill
+	kill $pid &&
+	for i in 1 2 3 4 5 ; do
+		kill $pid >/dev/null 2>&1 || break
+		sleep 1
+	done &&
+	# complain if it would not die
+	test_must_fail kill $pid >/dev/null 2>&1 &&
+	rm -rf "$db" "$cli" "$pidfile"
+}
+
+cleanup_git() {
+	rm -rf "$git"
+}
diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index 01ba041..8fbed66 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -2,76 +2,51 @@
 
 test_description='git-p4 tests'
 
-. ./test-lib.sh
+. ./lib-git-p4.sh
 
-( p4 -h && p4d -h ) >/dev/null 2>&1 || {
-	skip_all='skipping git-p4 tests; no p4 or p4d'
-	test_done
-}
-
-GITP4=$GIT_BUILD_DIR/contrib/fast-import/git-p4
-P4DPORT=10669
-
-export P4PORT=localhost:$P4DPORT
-
-db="$TRASH_DIRECTORY/db"
-cli="$TRASH_DIRECTORY/cli"
-git="$TRASH_DIRECTORY/git"
-
-test_debug 'echo p4d -q -d -r "$db" -p $P4DPORT'
-test_expect_success setup '
-	mkdir -p "$db" &&
-	p4d -q -d -r "$db" -p $P4DPORT &&
-	mkdir -p "$cli" &&
-	mkdir -p "$git" &&
-	export P4PORT=localhost:$P4DPORT
+test_expect_success 'start p4d' '
+	start_p4d
 '
 
 test_expect_success 'add p4 files' '
-	cd "$cli" &&
-	p4 client -i <<-EOF &&
-	Client: client
-	Description: client
-	Root: $cli
-	View: //depot/... //client/...
-	EOF
-	export P4CLIENT=client &&
-	echo file1 >file1 &&
-	p4 add file1 &&
-	p4 submit -d "file1" &&
-	echo file2 >file2 &&
-	p4 add file2 &&
-	p4 submit -d "file2" &&
-	cd "$TRASH_DIRECTORY"
+	(
+		cd "$cli" &&
+		echo file1 >file1 &&
+		p4 add file1 &&
+		p4 submit -d "file1" &&
+		echo file2 >file2 &&
+		p4 add file2 &&
+		p4 submit -d "file2"
+	)
 '
 
-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 "$git" &&
+		git log --oneline >lines &&
+		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 "$git" &&
+		git log --oneline >lines &&
+		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
+	(
+		cd "$git" &&
+		test_must_fail "$GITP4" sync
+	)
 '
 
 #
@@ -81,17 +56,19 @@ 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 &&
-	test_line_count = 2 lines
+	(
+		cd "$git" &&
+		test_commit head &&
+		"$GITP4" sync --branch=refs/remotes/p4/depot //depot@all &&
+		git log --oneline p4/depot >lines &&
+		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" &&
+	mkdir "$badp4dir" &&
+	test_when_finished "rm \"$badp4dir/p4\" && rmdir \"$badp4dir\"" &&
 	cat >"$badp4dir"/p4 <<-EOF &&
 	#!$SHELL_PATH
 	exit 1
@@ -103,61 +80,61 @@ test_expect_success 'exit when p4 fails to produce marshaled output' '
 '
 
 test_expect_success 'add p4 files with wildcards in the names' '
-	cd "$cli" &&
-	echo file-wild-hash >file-wild#hash &&
-	echo file-wild-star >file-wild\*star &&
-	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 "$cli" &&
+		echo file-wild-hash >file-wild#hash &&
+		echo file-wild-star >file-wild\*star &&
+		echo file-wild-at >file-wild@at &&
+		echo file-wild-percent >file-wild%percent &&
+		p4 add -f file-wild* &&
+		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 "$git" &&
+		test -f file-wild#hash &&
+		test -f file-wild\*star &&
+		test -f file-wild@at &&
+		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 "$git" &&
+		test ! -d .git &&
+		bare=`git config --get core.bare` &&
+		test "$bare" = true
+	)
 '
 
 p4_add_user() {
-    name=$1
-    fullname=$2
-    p4 user -f -i <<EOF &&
-User: $name
-Email: $name@localhost
-FullName: $fullname
-EOF
-    p4 passwd -P secret $name
+	name=$1 fullname=$2 &&
+	p4 user -f -i <<-EOF &&
+	User: $name
+	Email: $name@localhost
+	FullName: $fullname
+	EOF
+	p4 passwd -P secret $name
 }
 
 p4_grant_admin() {
-    name=$1
-    p4 protect -o |\
-	awk "{print}END{print \"    admin user $name * //depot/...\"}" |\
-	p4 protect -i
+	name=$1 &&
+	{
+		p4 protect -o &&
+		echo "    admin user $name * //depot/..."
+	} | p4 protect -i
 }
 
 p4_check_commit_author() {
-    file=$1
-    user=$2
-    if p4 changes -m 1 //depot/$file | grep $user > /dev/null ; then
-	return 0
-    else
-	echo "file $file not modified by user $user" 1>&2
-	return 1
-    fi
+	file=$1 user=$2 &&
+	p4 changes -m 1 //depot/$file | grep -q $user
 }
 
 make_change_by_user() {
@@ -174,15 +151,17 @@ test_expect_success 'preserve users' '
 	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 &&
-	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 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 "$git" &&
+		echo "username: a change by alice" >>file1 &&
+		echo "username: a change by bob" >>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 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
+	)
 '
 
 # Test username support, submitting as bob, who lacks admin rights. Should
@@ -190,32 +169,37 @@ test_expect_success 'preserve users' '
 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 &&
-	! git diff --exit-code HEAD..p4/master > /dev/null
+	(
+		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 test_must_fail "$GITP4" commit --preserve-user &&
+		test_must_fail git diff --exit-code HEAD..p4/master
+	)
 '
 
 # 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 &&
-	git commit --author "Bob <bob@localhost>" -m "preserve: a change by bob" 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 &&
-	! git diff --exit-code HEAD..p4/master > /dev/null &&
-	echo "$0: repeat with allowMissingP4Users enabled" &&
-	git config git-p4.allowMissingP4Users true &&
-	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 "$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 &&
+		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 test_must_fail "$GITP4" commit --preserve-user &&
+		test_must_fail git diff --exit-code HEAD..p4/master &&
+
+		echo "$0: repeat with allowMissingP4Users enabled" &&
+		git config git-p4.allowMissingP4Users true &&
+		git config git-p4.preserveUser true &&
+		P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit &&
+		git diff --exit-code HEAD..p4/master &&
+		p4_check_commit_author file1 alice
+	)
 '
 
 # If we're *not* using --preserve-user, git-p4 should warn if we're submitting
@@ -225,28 +209,30 @@ test_expect_success 'preserve user where author is unknown to p4' '
 test_expect_success 'not preserving user with mixed authorship' '
 	"$GITP4" clone --dest="$git" //depot &&
 	test_when_finished cleanup_git &&
-	cd "$git" &&
-	git config git-p4.skipSubmitEditCheck true &&
-	p4_add_user derek Derek &&
+	(
+		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 Derek derek@localhost &&
+		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |\
+		grep "git author derek@localhost does not match" &&
 
-	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 Charlie charlie@localhost &&
+		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |\
+		grep "git author charlie@localhost does not match" &&
 
-	make_change_by_user usernamefile3 alice alice@localhost &&
-	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
-	! grep "git author.*does not match" actual &&
+		make_change_by_user usernamefile3 alice alice@localhost &&
+		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" |\
+		test_must_fail grep "git author.*does not match" &&
 
-	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 &&
+		git config git-p4.skipUserNameCheck true &&
+		make_change_by_user usernamefile3 Charlie charlie@localhost &&
+		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |\
+		test_must_fail grep "git author.*does not match" &&
 
-	p4_check_commit_author usernamefile3 alice
+		p4_check_commit_author usernamefile3 alice
+	)
 '
 
 marshal_dump() {
@@ -263,10 +249,12 @@ test_expect_success 'initial import time from top change 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
+	(
+		cd "$git" &&
+		gittime=$(git show -s --raw --pretty=format:%at HEAD) &&
+		echo $p4time $gittime &&
+		test $p4time = $gittime
+	)
 '
 
 # Rename a file and confirm that rename is not detected in P4.
@@ -279,47 +267,49 @@ test_expect_success 'initial import time from top change time' '
 test_expect_success 'detect renames' '
 	"$GITP4" clone --dest="$git" //depot@all &&
 	test_when_finished cleanup_git &&
-	cd "$git" &&
-	git config git-p4.skipSubmitEditCheck true &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEditCheck true &&
 
-	git mv file1 file4 &&
-	git commit -a -m "Rename file1 to file4" &&
-	git diff-tree -r -M HEAD &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file4 &&
-	! p4 filelog //depot/file4 | grep -q "branch from" &&
+		git mv file1 file4 &&
+		git commit -a -m "Rename file1 to file4" &&
+		git diff-tree -r -M HEAD &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file4 &&
+		p4 filelog //depot/file4 | test_must_fail grep -q "branch from" &&
 
-	git mv file4 file5 &&
-	git commit -a -m "Rename file4 to file5" &&
-	git diff-tree -r -M HEAD &&
-	git config git-p4.detectRenames true &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file5 &&
-	p4 filelog //depot/file5 | grep -q "branch from //depot/file4" &&
+		git mv file4 file5 &&
+		git commit -a -m "Rename file4 to file5" &&
+		git diff-tree -r -M HEAD &&
+		git config git-p4.detectRenames true &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file5 &&
+		p4 filelog //depot/file5 | grep -q "branch from //depot/file4" &&
 
-	git mv file5 file6 &&
-	echo update >>file6 &&
-	git add file6 &&
-	git commit -a -m "Rename file5 to file6 with changes" &&
-	git diff-tree -r -M HEAD &&
-	level=$(git diff-tree -r -M HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/R0*//") &&
-	test -n "$level" && test "$level" -gt 0 && test "$level" -lt 98 &&
-	git config git-p4.detectRenames $((level + 2)) &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file6 &&
-	! p4 filelog //depot/file6 | grep -q "branch from" &&
+		git mv file5 file6 &&
+		echo update >>file6 &&
+		git add file6 &&
+		git commit -a -m "Rename file5 to file6 with changes" &&
+		git diff-tree -r -M HEAD &&
+		level=$(git diff-tree -r -M HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/R0*//") &&
+		test -n "$level" && test "$level" -gt 0 && test "$level" -lt 98 &&
+		git config git-p4.detectRenames $(($level + 2)) &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file6 &&
+		p4 filelog //depot/file6 | test_must_fail grep -q "branch from" &&
 
-	git mv file6 file7 &&
-	echo update >>file7 &&
-	git add file7 &&
-	git commit -a -m "Rename file6 to file7 with changes" &&
-	git diff-tree -r -M HEAD &&
-	level=$(git diff-tree -r -M HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/R0*//") &&
-	test -n "$level" && test "$level" -gt 2 && test "$level" -lt 100 &&
-	git config git-p4.detectRenames $((level - 2)) &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file7 &&
-	p4 filelog //depot/file7 | grep -q "branch from //depot/file6"
+		git mv file6 file7 &&
+		echo update >>file7 &&
+		git add file7 &&
+		git commit -a -m "Rename file6 to file7 with changes" &&
+		git diff-tree -r -M HEAD &&
+		level=$(git diff-tree -r -M HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/R0*//") &&
+		test -n "$level" && test "$level" -gt 2 && test "$level" -lt 100 &&
+		git config git-p4.detectRenames $(($level - 2)) &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file7 &&
+		p4 filelog //depot/file7 | grep -q "branch from //depot/file6"
+	)
 '
 
 # Copy a file and confirm that copy is not detected in P4.
@@ -336,141 +326,79 @@ test_expect_success 'detect renames' '
 test_expect_success 'detect copies' '
 	"$GITP4" clone --dest="$git" //depot@all &&
 	test_when_finished cleanup_git &&
-	cd "$git" &&
-	git config git-p4.skipSubmitEditCheck true &&
-
-	cp file2 file8 &&
-	git add file8 &&
-	git commit -a -m "Copy file2 to file8" &&
-	git diff-tree -r -C HEAD &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file8 &&
-	! p4 filelog //depot/file8 | grep -q "branch from" &&
-
-	cp file2 file9 &&
-	git add file9 &&
-	git commit -a -m "Copy file2 to file9" &&
-	git diff-tree -r -C HEAD &&
-	git config git-p4.detectCopies true &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file9 &&
-	! p4 filelog //depot/file9 | grep -q "branch from" &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEditCheck true &&
 
-	echo "file2" >>file2 &&
-	cp file2 file10 &&
-	git add file2 file10 &&
-	git commit -a -m "Modify and copy file2 to file10" &&
-	git diff-tree -r -C HEAD &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file10 &&
-	p4 filelog //depot/file10 | grep -q "branch from //depot/file" &&
+		cp file2 file8 &&
+		git add file8 &&
+		git commit -a -m "Copy file2 to file8" &&
+		git diff-tree -r -C HEAD &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file8 &&
+		p4 filelog //depot/file8 | test_must_fail grep -q "branch from" &&
 
-	cp file2 file11 &&
-	git add file11 &&
-	git commit -a -m "Copy file2 to file11" &&
-	git diff-tree -r -C --find-copies-harder HEAD &&
-	src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
-	test "$src" = file10 &&
-	git config git-p4.detectCopiesHarder true &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file11 &&
-	p4 filelog //depot/file11 | grep -q "branch from //depot/file" &&
+		cp file2 file9 &&
+		git add file9 &&
+		git commit -a -m "Copy file2 to file9" &&
+		git diff-tree -r -C HEAD &&
+		git config git-p4.detectCopies true &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file9 &&
+		p4 filelog //depot/file9 | test_must_fail grep -q "branch from" &&
 
-	cp file2 file12 &&
-	echo "some text" >>file12 &&
-	git add file12 &&
-	git commit -a -m "Copy file2 to file12 with changes" &&
-	git diff-tree -r -C --find-copies-harder HEAD &&
-	level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
-	test -n "$level" && test "$level" -gt 0 && test "$level" -lt 98 &&
-	src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
-	test "$src" = file10 &&
-	git config git-p4.detectCopies $((level + 2)) &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file12 &&
-	! p4 filelog //depot/file12 | grep -q "branch from" &&
+		echo "file2" >>file2 &&
+		cp file2 file10 &&
+		git add file2 file10 &&
+		git commit -a -m "Modify and copy file2 to file10" &&
+		git diff-tree -r -C HEAD &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file10 &&
+		p4 filelog //depot/file10 | grep -q "branch from //depot/file" &&
 
-	cp file2 file13 &&
-	echo "different text" >>file13 &&
-	git add file13 &&
-	git commit -a -m "Copy file2 to file13 with changes" &&
-	git diff-tree -r -C --find-copies-harder HEAD &&
-	level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
-	test -n "$level" && test "$level" -gt 2 && test "$level" -lt 100 &&
-	src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
-	test "$src" = file10 &&
-	git config git-p4.detectCopies $((level - 2)) &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file13 &&
-	p4 filelog //depot/file13 | grep -q "branch from //depot/file"
-'
+		cp file2 file11 &&
+		git add file11 &&
+		git commit -a -m "Copy file2 to file11" &&
+		git diff-tree -r -C --find-copies-harder HEAD &&
+		src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
+		test "$src" = file10 &&
+		git config git-p4.detectCopiesHarder true &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file11 &&
+		p4 filelog //depot/file11 | grep -q "branch from //depot/file" &&
 
-# Create a simple branch structure in P4 depot to check if it is correctly
-# cloned.
-test_expect_success 'add simple p4 branches' '
-	cd "$cli" &&
-	mkdir branch1 &&
-	cd branch1 &&
-	echo file1 >file1 &&
-	echo file2 >file2 &&
-	p4 add file1 file2 &&
-	p4 submit -d "branch1" &&
-	p4 integrate //depot/branch1/... //depot/branch2/... &&
-	p4 submit -d "branch2" &&
-	echo file3 >file3 &&
-	p4 add file3 &&
-	p4 submit -d "add file3 in branch1" &&
-	p4 open file2 &&
-	echo update >>file2 &&
-	p4 submit -d "update file2 in branch1" &&
-	p4 integrate //depot/branch1/... //depot/branch3/... &&
-	p4 submit -d "branch3" &&
-	cd "$TRASH_DIRECTORY"
-'
+		cp file2 file12 &&
+		echo "some text" >>file12 &&
+		git add file12 &&
+		git commit -a -m "Copy file2 to file12 with changes" &&
+		git diff-tree -r -C --find-copies-harder HEAD &&
+		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
+		test -n "$level" && test "$level" -gt 0 && test "$level" -lt 98 &&
+		src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
+		test "$src" = file10 &&
+		git config git-p4.detectCopies $(($level + 2)) &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file12 &&
+		p4 filelog //depot/file12 | test_must_fail grep -q "branch from" &&
 
-# Configure branches through git-config and clone them.
-# All files are tested to make sure branches were cloned correctly.
-# Finally, make an update to branch1 on P4 side to check if it is imported
-# correctly by git-p4.
-test_expect_success 'git-p4 clone simple branches' '
-	test_when_finished cleanup_git &&
-	test_create_repo "$git" &&
-	cd "$git" &&
-	git config git-p4.branchList branch1:branch2 &&
-	git config --add git-p4.branchList branch1:branch3 &&
-	"$GITP4" clone --dest=. --detect-branches //depot@all &&
-	git log --all --graph --decorate --stat &&
-	git reset --hard p4/depot/branch1 &&
-	test -f file1 &&
-	test -f file2 &&
-	test -f file3 &&
-	grep -q update file2 &&
-	git reset --hard p4/depot/branch2 &&
-	test -f file1 &&
-	test -f file2 &&
-	test ! -f file3 &&
-	! grep -q update file2 &&
-	git reset --hard p4/depot/branch3 &&
-	test -f file1 &&
-	test -f file2 &&
-	test -f file3 &&
-	grep -q update file2 &&
-	cd "$cli" &&
-	cd branch1 &&
-	p4 edit file2 &&
-	echo file2_ >>file2 &&
-	p4 submit -d "update file2 in branch1" &&
-	cd "$git" &&
-	git reset --hard p4/depot/branch1 &&
-	"$GITP4" rebase &&
-	grep -q file2_ file2
+		cp file2 file13 &&
+		echo "different text" >>file13 &&
+		git add file13 &&
+		git commit -a -m "Copy file2 to file13 with changes" &&
+		git diff-tree -r -C --find-copies-harder HEAD &&
+		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
+		test -n "$level" && test "$level" -gt 2 && test "$level" -lt 100 &&
+		src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
+		test "$src" = file10 &&
+		git config git-p4.detectCopies $(($level - 2)) &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file13 &&
+		p4 filelog //depot/file13 | grep -q "branch from //depot/file"
+	)
 '
 
-test_expect_success 'shutdown' '
-	pid=`pgrep -f p4d` &&
-	test -n "$pid" &&
-	test_debug "ps wl `echo $pid`" &&
-	kill $pid
+test_expect_success 'kill p4d' '
+	kill_p4d
 '
 
 test_done
diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
new file mode 100755
index 0000000..a25f18d
--- /dev/null
+++ b/t/t9801-git-p4-branch.sh
@@ -0,0 +1,233 @@
+#!/bin/sh
+
+test_description='git-p4 p4 branching tests'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+#
+# 1: //depot/main/f1
+# 2: //depot/main/f2
+# 3: integrate //depot/main/... -> //depot/branch1/...
+# 4: //depot/main/f4
+# 5: //depot/branch1/f5
+# .: named branch branch2
+# 6: integrate -b branch2
+# 7: //depot/branch2/f7
+# 8: //depot/main/f8
+#
+test_expect_success 'basic p4 branches' '
+	(
+		cd "$cli" &&
+		mkdir -p main &&
+
+		echo f1 >main/f1 &&
+		p4 add main/f1 &&
+		p4 submit -d "main/f1" &&
+
+		echo f2 >main/f2 &&
+		p4 add main/f2 &&
+		p4 submit -d "main/f2" &&
+
+		p4 integrate //depot/main/... //depot/branch1/... &&
+		p4 submit -d "integrate main to branch1" &&
+
+		echo f4 >main/f4 &&
+		p4 add main/f4 &&
+		p4 submit -d "main/f4" &&
+
+		echo f5 >branch1/f5 &&
+		p4 add branch1/f5 &&
+		p4 submit -d "branch1/f5" &&
+
+		p4 branch -i <<-EOF &&
+		Branch: branch2
+		View: //depot/main/... //depot/branch2/...
+		EOF
+
+		p4 integrate -b branch2 &&
+		p4 submit -d "integrate main to branch2" &&
+
+		echo f7 >branch2/f7 &&
+		p4 add branch2/f7 &&
+		p4 submit -d "branch2/f7" &&
+
+		echo f8 >main/f8 &&
+		p4 add main/f8 &&
+		p4 submit -d "main/f8"
+	)
+'
+
+test_expect_success 'import main, no branch detection' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot/main@all &&
+	(
+		cd "$git" &&
+		git log --oneline --graph --decorate --all &&
+		git rev-list master >wc &&
+		test_line_count = 4 wc
+	)
+'
+
+test_expect_success 'import branch1, no branch detection' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot/branch1@all &&
+	(
+		cd "$git" &&
+		git log --oneline --graph --decorate --all &&
+		git rev-list master >wc &&
+		test_line_count = 2 wc
+	)
+'
+
+test_expect_success 'import branch2, no branch detection' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot/branch2@all &&
+	(
+		cd "$git" &&
+		git log --oneline --graph --decorate --all &&
+		git rev-list master >wc &&
+		test_line_count = 2 wc
+	)
+'
+
+test_expect_success 'import depot, no branch detection' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot@all &&
+	(
+		cd "$git" &&
+		git log --oneline --graph --decorate --all &&
+		git rev-list master >wc &&
+		test_line_count = 8 wc
+	)
+'
+
+test_expect_success 'import depot, branch detection' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" --detect-branches //depot@all &&
+	(
+		cd "$git" &&
+
+		git log --oneline --graph --decorate --all &&
+
+		# 4 main commits
+		git rev-list master >wc &&
+		test_line_count = 4 wc &&
+
+		# 3 main, 1 integrate, 1 on branch2
+		git rev-list p4/depot/branch2 >wc &&
+		test_line_count = 5 wc &&
+
+		# no branch1, since no p4 branch created for it
+		test_must_fail git show-ref p4/depot/branch1
+	)
+'
+
+test_expect_success 'import depot, branch detection, branchList branch definition' '
+	test_when_finished cleanup_git &&
+	test_create_repo "$git" &&
+	(
+		cd "$git" &&
+		git config git-p4.branchList main:branch1 &&
+		"$GITP4" clone --dest=. --detect-branches //depot@all &&
+
+		git log --oneline --graph --decorate --all &&
+
+		# 4 main commits
+		git rev-list master >wc &&
+		test_line_count = 4 wc &&
+
+		# 3 main, 1 integrate, 1 on branch2
+		git rev-list p4/depot/branch2 >wc &&
+		test_line_count = 5 wc &&
+
+		# 2 main, 1 integrate, 1 on branch1
+		git rev-list p4/depot/branch1 >wc &&
+		test_line_count = 4 wc
+	)
+'
+
+test_expect_success 'restart p4d' '
+	kill_p4d &&
+	start_p4d
+'
+
+#
+# 1: //depot/branch1/file1
+#    //depot/branch1/file2
+# 2: integrate //depot/branch1/... -> //depot/branch2/...
+# 3: //depot/branch1/file3
+# 4: //depot/branch1/file2 (edit)
+# 5: integrate //depot/branch1/... -> //depot/branch3/...
+#
+## Create a simple branch structure in P4 depot.
+test_expect_success 'add simple p4 branches' '
+	(
+		cd "$cli" &&
+		mkdir branch1 &&
+		cd branch1 &&
+		echo file1 >file1 &&
+		echo file2 >file2 &&
+		p4 add file1 file2 &&
+		p4 submit -d "branch1" &&
+		p4 integrate //depot/branch1/... //depot/branch2/... &&
+		p4 submit -d "branch2" &&
+		echo file3 >file3 &&
+		p4 add file3 &&
+		p4 submit -d "add file3 in branch1" &&
+		p4 open file2 &&
+		echo update >>file2 &&
+		p4 submit -d "update file2 in branch1" &&
+		p4 integrate //depot/branch1/... //depot/branch3/... &&
+		p4 submit -d "branch3"
+	)
+'
+
+# Configure branches through git-config and clone them.
+# All files are tested to make sure branches were cloned correctly.
+# Finally, make an update to branch1 on P4 side to check if it is imported
+# correctly by git-p4.
+test_expect_success 'git-p4 clone simple branches' '
+	test_when_finished cleanup_git &&
+	test_create_repo "$git" &&
+	(
+		cd "$git" &&
+		git config git-p4.branchList branch1:branch2 &&
+		git config --add git-p4.branchList branch1:branch3 &&
+		"$GITP4" clone --dest=. --detect-branches //depot@all &&
+		git log --all --graph --decorate --stat &&
+		git reset --hard p4/depot/branch1 &&
+		test -f file1 &&
+		test -f file2 &&
+		test -f file3 &&
+		grep -q update file2 &&
+		git reset --hard p4/depot/branch2 &&
+		test -f file1 &&
+		test -f file2 &&
+		test ! -f file3 &&
+		test_must_fail grep -q update file2 &&
+		git reset --hard p4/depot/branch3 &&
+		test -f file1 &&
+		test -f file2 &&
+		test -f file3 &&
+		grep -q update file2 &&
+		cd "$cli" &&
+		cd branch1 &&
+		p4 edit file2 &&
+		echo file2_ >>file2 &&
+		p4 submit -d "update file2 in branch3" &&
+		cd "$git" &&
+		git reset --hard p4/depot/branch1 &&
+		"$GITP4" rebase &&
+		grep -q file2_ file2
+	)
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.7.7

Re: [PATCH 1/6] git-p4 tests: refactor and cleanup

[Pete Wyckoff]

pw@padd.com wrote on Sun, 16 Oct 2011 10:43 -0400:
> Introduce a library for functions that are common to
> multiple git-p4 test files.

Update, replacing bare "python" invocation with "$PYTHON_PATH".


-------------------------8<-----------------------------

>From 3a26dbf40d9aa9f8432d994d5697ad10fce8ed91 Mon Sep 17 00:00:00 2001
From: Pete Wyckoff <pw@padd.com>
Date: Mon, 22 Aug 2011 22:20:33 -0400
Subject: [PATCH 1/6] git-p4 tests: refactor and cleanup

Introduce a library for functions that are common to
multiple git-p4 test files.

Be a bit more clever about starting and stopping p4d.
Specify a unique port number for each test, so that
tests can run in parallel.  Start p4d not in daemon mode,
and save the pid, to be able to kill it cleanly later.
Never kill p4d at startup; always shutdown cleanly.

Handle directory changes better.  Always chdir inside
a subshell, and remove any post-test directory changes.

Clean up whitespace, and use test_cmp and test_must_fail
more consistently.

Separate the tests related to detecting p4 branches
into their own file, and add a few more.

Acked-by: Luke Diamand <luke@diamand.org>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/lib-git-p4.sh          |   74 +++++++
 t/t9800-git-p4.sh        |  546 ++++++++++++++++++++--------------------------
 t/t9801-git-p4-branch.sh |  233 ++++++++++++++++++++
 3 files changed, 544 insertions(+), 309 deletions(-)
 create mode 100644 t/lib-git-p4.sh
 create mode 100755 t/t9801-git-p4-branch.sh

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
new file mode 100644
index 0000000..a870f9a
--- /dev/null
+++ b/t/lib-git-p4.sh
@@ -0,0 +1,74 @@
+#
+# Library code for git-p4 tests
+#
+
+. ./test-lib.sh
+
+if ! test_have_prereq PYTHON; then
+	skip_all='skipping git-p4 tests; python not available'
+	test_done
+fi
+( p4 -h && p4d -h ) >/dev/null 2>&1 || {
+	skip_all='skipping git-p4 tests; no p4 or p4d'
+	test_done
+}
+
+GITP4="$GIT_BUILD_DIR/contrib/fast-import/git-p4"
+
+# Try to pick a unique port: guess a large number, then hope
+# no more than one of each test is running.
+#
+# This does not handle the case where somebody else is running the
+# same tests and has chosen the same ports.
+testid=${this_test#t}
+git_p4_test_start=9800
+P4DPORT=$((10669 + ($testid - $git_p4_test_start)))
+
+export P4PORT=localhost:$P4DPORT
+export P4CLIENT=client
+
+db="$TRASH_DIRECTORY/db"
+cli="$TRASH_DIRECTORY/cli"
+git="$TRASH_DIRECTORY/git"
+pidfile="$TRASH_DIRECTORY/p4d.pid"
+
+start_p4d() {
+	mkdir -p "$db" "$cli" "$git" &&
+	(
+		p4d -q -r "$db" -p $P4DPORT &
+		echo $! >"$pidfile"
+	) &&
+	for i in 1 2 3 4 5 ; do
+		p4 info >/dev/null 2>&1 && break || true &&
+		echo waiting for p4d to start &&
+		sleep 1
+	done &&
+	# complain if it never started
+	p4 info >/dev/null &&
+	(
+		cd "$cli" &&
+		p4 client -i <<-EOF
+		Client: client
+		Description: client
+		Root: $cli
+		View: //depot/... //client/...
+		EOF
+	)
+}
+
+kill_p4d() {
+	pid=$(cat "$pidfile")
+	# it had better exist for the first kill
+	kill $pid &&
+	for i in 1 2 3 4 5 ; do
+		kill $pid >/dev/null 2>&1 || break
+		sleep 1
+	done &&
+	# complain if it would not die
+	test_must_fail kill $pid >/dev/null 2>&1 &&
+	rm -rf "$db" "$cli" "$pidfile"
+}
+
+cleanup_git() {
+	rm -rf "$git"
+}
diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index 01ba041..272de3f 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -2,76 +2,51 @@
 
 test_description='git-p4 tests'
 
-. ./test-lib.sh
+. ./lib-git-p4.sh
 
-( p4 -h && p4d -h ) >/dev/null 2>&1 || {
-	skip_all='skipping git-p4 tests; no p4 or p4d'
-	test_done
-}
-
-GITP4=$GIT_BUILD_DIR/contrib/fast-import/git-p4
-P4DPORT=10669
-
-export P4PORT=localhost:$P4DPORT
-
-db="$TRASH_DIRECTORY/db"
-cli="$TRASH_DIRECTORY/cli"
-git="$TRASH_DIRECTORY/git"
-
-test_debug 'echo p4d -q -d -r "$db" -p $P4DPORT'
-test_expect_success setup '
-	mkdir -p "$db" &&
-	p4d -q -d -r "$db" -p $P4DPORT &&
-	mkdir -p "$cli" &&
-	mkdir -p "$git" &&
-	export P4PORT=localhost:$P4DPORT
+test_expect_success 'start p4d' '
+	start_p4d
 '
 
 test_expect_success 'add p4 files' '
-	cd "$cli" &&
-	p4 client -i <<-EOF &&
-	Client: client
-	Description: client
-	Root: $cli
-	View: //depot/... //client/...
-	EOF
-	export P4CLIENT=client &&
-	echo file1 >file1 &&
-	p4 add file1 &&
-	p4 submit -d "file1" &&
-	echo file2 >file2 &&
-	p4 add file2 &&
-	p4 submit -d "file2" &&
-	cd "$TRASH_DIRECTORY"
+	(
+		cd "$cli" &&
+		echo file1 >file1 &&
+		p4 add file1 &&
+		p4 submit -d "file1" &&
+		echo file2 >file2 &&
+		p4 add file2 &&
+		p4 submit -d "file2"
+	)
 '
 
-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 "$git" &&
+		git log --oneline >lines &&
+		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 "$git" &&
+		git log --oneline >lines &&
+		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
+	(
+		cd "$git" &&
+		test_must_fail "$GITP4" sync
+	)
 '
 
 #
@@ -81,17 +56,19 @@ 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 &&
-	test_line_count = 2 lines
+	(
+		cd "$git" &&
+		test_commit head &&
+		"$GITP4" sync --branch=refs/remotes/p4/depot //depot@all &&
+		git log --oneline p4/depot >lines &&
+		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" &&
+	mkdir "$badp4dir" &&
+	test_when_finished "rm \"$badp4dir/p4\" && rmdir \"$badp4dir\"" &&
 	cat >"$badp4dir"/p4 <<-EOF &&
 	#!$SHELL_PATH
 	exit 1
@@ -103,61 +80,61 @@ test_expect_success 'exit when p4 fails to produce marshaled output' '
 '
 
 test_expect_success 'add p4 files with wildcards in the names' '
-	cd "$cli" &&
-	echo file-wild-hash >file-wild#hash &&
-	echo file-wild-star >file-wild\*star &&
-	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 "$cli" &&
+		echo file-wild-hash >file-wild#hash &&
+		echo file-wild-star >file-wild\*star &&
+		echo file-wild-at >file-wild@at &&
+		echo file-wild-percent >file-wild%percent &&
+		p4 add -f file-wild* &&
+		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 "$git" &&
+		test -f file-wild#hash &&
+		test -f file-wild\*star &&
+		test -f file-wild@at &&
+		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 "$git" &&
+		test ! -d .git &&
+		bare=`git config --get core.bare` &&
+		test "$bare" = true
+	)
 '
 
 p4_add_user() {
-    name=$1
-    fullname=$2
-    p4 user -f -i <<EOF &&
-User: $name
-Email: $name@localhost
-FullName: $fullname
-EOF
-    p4 passwd -P secret $name
+	name=$1 fullname=$2 &&
+	p4 user -f -i <<-EOF &&
+	User: $name
+	Email: $name@localhost
+	FullName: $fullname
+	EOF
+	p4 passwd -P secret $name
 }
 
 p4_grant_admin() {
-    name=$1
-    p4 protect -o |\
-	awk "{print}END{print \"    admin user $name * //depot/...\"}" |\
-	p4 protect -i
+	name=$1 &&
+	{
+		p4 protect -o &&
+		echo "    admin user $name * //depot/..."
+	} | p4 protect -i
 }
 
 p4_check_commit_author() {
-    file=$1
-    user=$2
-    if p4 changes -m 1 //depot/$file | grep $user > /dev/null ; then
-	return 0
-    else
-	echo "file $file not modified by user $user" 1>&2
-	return 1
-    fi
+	file=$1 user=$2 &&
+	p4 changes -m 1 //depot/$file | grep -q $user
 }
 
 make_change_by_user() {
@@ -174,15 +151,17 @@ test_expect_success 'preserve users' '
 	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 &&
-	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 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 "$git" &&
+		echo "username: a change by alice" >>file1 &&
+		echo "username: a change by bob" >>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 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
+	)
 '
 
 # Test username support, submitting as bob, who lacks admin rights. Should
@@ -190,32 +169,37 @@ test_expect_success 'preserve users' '
 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 &&
-	! git diff --exit-code HEAD..p4/master > /dev/null
+	(
+		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 test_must_fail "$GITP4" commit --preserve-user &&
+		test_must_fail git diff --exit-code HEAD..p4/master
+	)
 '
 
 # 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 &&
-	git commit --author "Bob <bob@localhost>" -m "preserve: a change by bob" 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 &&
-	! git diff --exit-code HEAD..p4/master > /dev/null &&
-	echo "$0: repeat with allowMissingP4Users enabled" &&
-	git config git-p4.allowMissingP4Users true &&
-	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 "$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 &&
+		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 test_must_fail "$GITP4" commit --preserve-user &&
+		test_must_fail git diff --exit-code HEAD..p4/master &&
+
+		echo "$0: repeat with allowMissingP4Users enabled" &&
+		git config git-p4.allowMissingP4Users true &&
+		git config git-p4.preserveUser true &&
+		P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit &&
+		git diff --exit-code HEAD..p4/master &&
+		p4_check_commit_author file1 alice
+	)
 '
 
 # If we're *not* using --preserve-user, git-p4 should warn if we're submitting
@@ -225,33 +209,35 @@ test_expect_success 'preserve user where author is unknown to p4' '
 test_expect_success 'not preserving user with mixed authorship' '
 	"$GITP4" clone --dest="$git" //depot &&
 	test_when_finished cleanup_git &&
-	cd "$git" &&
-	git config git-p4.skipSubmitEditCheck true &&
-	p4_add_user derek Derek &&
+	(
+		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 Derek derek@localhost &&
+		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |\
+		grep "git author derek@localhost does not match" &&
 
-	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 Charlie charlie@localhost &&
+		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |\
+		grep "git author charlie@localhost does not match" &&
 
-	make_change_by_user usernamefile3 alice alice@localhost &&
-	P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit >actual &&
-	! grep "git author.*does not match" actual &&
+		make_change_by_user usernamefile3 alice alice@localhost &&
+		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" |\
+		test_must_fail grep "git author.*does not match" &&
 
-	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 &&
+		git config git-p4.skipUserNameCheck true &&
+		make_change_by_user usernamefile3 Charlie charlie@localhost &&
+		P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |\
+		test_must_fail grep "git author.*does not match" &&
 
-	p4_check_commit_author usernamefile3 alice
+		p4_check_commit_author usernamefile3 alice
+	)
 '
 
 marshal_dump() {
 	what=$1
-	python -c 'import marshal, sys; d = marshal.load(sys.stdin); print d["'$what'"]'
+	"$PYTHON_PATH" -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
@@ -263,10 +249,12 @@ test_expect_success 'initial import time from top change 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
+	(
+		cd "$git" &&
+		gittime=$(git show -s --raw --pretty=format:%at HEAD) &&
+		echo $p4time $gittime &&
+		test $p4time = $gittime
+	)
 '
 
 # Rename a file and confirm that rename is not detected in P4.
@@ -279,47 +267,49 @@ test_expect_success 'initial import time from top change time' '
 test_expect_success 'detect renames' '
 	"$GITP4" clone --dest="$git" //depot@all &&
 	test_when_finished cleanup_git &&
-	cd "$git" &&
-	git config git-p4.skipSubmitEditCheck true &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEditCheck true &&
 
-	git mv file1 file4 &&
-	git commit -a -m "Rename file1 to file4" &&
-	git diff-tree -r -M HEAD &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file4 &&
-	! p4 filelog //depot/file4 | grep -q "branch from" &&
+		git mv file1 file4 &&
+		git commit -a -m "Rename file1 to file4" &&
+		git diff-tree -r -M HEAD &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file4 &&
+		p4 filelog //depot/file4 | test_must_fail grep -q "branch from" &&
 
-	git mv file4 file5 &&
-	git commit -a -m "Rename file4 to file5" &&
-	git diff-tree -r -M HEAD &&
-	git config git-p4.detectRenames true &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file5 &&
-	p4 filelog //depot/file5 | grep -q "branch from //depot/file4" &&
+		git mv file4 file5 &&
+		git commit -a -m "Rename file4 to file5" &&
+		git diff-tree -r -M HEAD &&
+		git config git-p4.detectRenames true &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file5 &&
+		p4 filelog //depot/file5 | grep -q "branch from //depot/file4" &&
 
-	git mv file5 file6 &&
-	echo update >>file6 &&
-	git add file6 &&
-	git commit -a -m "Rename file5 to file6 with changes" &&
-	git diff-tree -r -M HEAD &&
-	level=$(git diff-tree -r -M HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/R0*//") &&
-	test -n "$level" && test "$level" -gt 0 && test "$level" -lt 98 &&
-	git config git-p4.detectRenames $((level + 2)) &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file6 &&
-	! p4 filelog //depot/file6 | grep -q "branch from" &&
+		git mv file5 file6 &&
+		echo update >>file6 &&
+		git add file6 &&
+		git commit -a -m "Rename file5 to file6 with changes" &&
+		git diff-tree -r -M HEAD &&
+		level=$(git diff-tree -r -M HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/R0*//") &&
+		test -n "$level" && test "$level" -gt 0 && test "$level" -lt 98 &&
+		git config git-p4.detectRenames $(($level + 2)) &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file6 &&
+		p4 filelog //depot/file6 | test_must_fail grep -q "branch from" &&
 
-	git mv file6 file7 &&
-	echo update >>file7 &&
-	git add file7 &&
-	git commit -a -m "Rename file6 to file7 with changes" &&
-	git diff-tree -r -M HEAD &&
-	level=$(git diff-tree -r -M HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/R0*//") &&
-	test -n "$level" && test "$level" -gt 2 && test "$level" -lt 100 &&
-	git config git-p4.detectRenames $((level - 2)) &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file7 &&
-	p4 filelog //depot/file7 | grep -q "branch from //depot/file6"
+		git mv file6 file7 &&
+		echo update >>file7 &&
+		git add file7 &&
+		git commit -a -m "Rename file6 to file7 with changes" &&
+		git diff-tree -r -M HEAD &&
+		level=$(git diff-tree -r -M HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/R0*//") &&
+		test -n "$level" && test "$level" -gt 2 && test "$level" -lt 100 &&
+		git config git-p4.detectRenames $(($level - 2)) &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file7 &&
+		p4 filelog //depot/file7 | grep -q "branch from //depot/file6"
+	)
 '
 
 # Copy a file and confirm that copy is not detected in P4.
@@ -336,141 +326,79 @@ test_expect_success 'detect renames' '
 test_expect_success 'detect copies' '
 	"$GITP4" clone --dest="$git" //depot@all &&
 	test_when_finished cleanup_git &&
-	cd "$git" &&
-	git config git-p4.skipSubmitEditCheck true &&
-
-	cp file2 file8 &&
-	git add file8 &&
-	git commit -a -m "Copy file2 to file8" &&
-	git diff-tree -r -C HEAD &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file8 &&
-	! p4 filelog //depot/file8 | grep -q "branch from" &&
-
-	cp file2 file9 &&
-	git add file9 &&
-	git commit -a -m "Copy file2 to file9" &&
-	git diff-tree -r -C HEAD &&
-	git config git-p4.detectCopies true &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file9 &&
-	! p4 filelog //depot/file9 | grep -q "branch from" &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEditCheck true &&
 
-	echo "file2" >>file2 &&
-	cp file2 file10 &&
-	git add file2 file10 &&
-	git commit -a -m "Modify and copy file2 to file10" &&
-	git diff-tree -r -C HEAD &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file10 &&
-	p4 filelog //depot/file10 | grep -q "branch from //depot/file" &&
+		cp file2 file8 &&
+		git add file8 &&
+		git commit -a -m "Copy file2 to file8" &&
+		git diff-tree -r -C HEAD &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file8 &&
+		p4 filelog //depot/file8 | test_must_fail grep -q "branch from" &&
 
-	cp file2 file11 &&
-	git add file11 &&
-	git commit -a -m "Copy file2 to file11" &&
-	git diff-tree -r -C --find-copies-harder HEAD &&
-	src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
-	test "$src" = file10 &&
-	git config git-p4.detectCopiesHarder true &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file11 &&
-	p4 filelog //depot/file11 | grep -q "branch from //depot/file" &&
+		cp file2 file9 &&
+		git add file9 &&
+		git commit -a -m "Copy file2 to file9" &&
+		git diff-tree -r -C HEAD &&
+		git config git-p4.detectCopies true &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file9 &&
+		p4 filelog //depot/file9 | test_must_fail grep -q "branch from" &&
 
-	cp file2 file12 &&
-	echo "some text" >>file12 &&
-	git add file12 &&
-	git commit -a -m "Copy file2 to file12 with changes" &&
-	git diff-tree -r -C --find-copies-harder HEAD &&
-	level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
-	test -n "$level" && test "$level" -gt 0 && test "$level" -lt 98 &&
-	src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
-	test "$src" = file10 &&
-	git config git-p4.detectCopies $((level + 2)) &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file12 &&
-	! p4 filelog //depot/file12 | grep -q "branch from" &&
+		echo "file2" >>file2 &&
+		cp file2 file10 &&
+		git add file2 file10 &&
+		git commit -a -m "Modify and copy file2 to file10" &&
+		git diff-tree -r -C HEAD &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file10 &&
+		p4 filelog //depot/file10 | grep -q "branch from //depot/file" &&
 
-	cp file2 file13 &&
-	echo "different text" >>file13 &&
-	git add file13 &&
-	git commit -a -m "Copy file2 to file13 with changes" &&
-	git diff-tree -r -C --find-copies-harder HEAD &&
-	level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
-	test -n "$level" && test "$level" -gt 2 && test "$level" -lt 100 &&
-	src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
-	test "$src" = file10 &&
-	git config git-p4.detectCopies $((level - 2)) &&
-	"$GITP4" submit &&
-	p4 filelog //depot/file13 &&
-	p4 filelog //depot/file13 | grep -q "branch from //depot/file"
-'
+		cp file2 file11 &&
+		git add file11 &&
+		git commit -a -m "Copy file2 to file11" &&
+		git diff-tree -r -C --find-copies-harder HEAD &&
+		src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
+		test "$src" = file10 &&
+		git config git-p4.detectCopiesHarder true &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file11 &&
+		p4 filelog //depot/file11 | grep -q "branch from //depot/file" &&
 
-# Create a simple branch structure in P4 depot to check if it is correctly
-# cloned.
-test_expect_success 'add simple p4 branches' '
-	cd "$cli" &&
-	mkdir branch1 &&
-	cd branch1 &&
-	echo file1 >file1 &&
-	echo file2 >file2 &&
-	p4 add file1 file2 &&
-	p4 submit -d "branch1" &&
-	p4 integrate //depot/branch1/... //depot/branch2/... &&
-	p4 submit -d "branch2" &&
-	echo file3 >file3 &&
-	p4 add file3 &&
-	p4 submit -d "add file3 in branch1" &&
-	p4 open file2 &&
-	echo update >>file2 &&
-	p4 submit -d "update file2 in branch1" &&
-	p4 integrate //depot/branch1/... //depot/branch3/... &&
-	p4 submit -d "branch3" &&
-	cd "$TRASH_DIRECTORY"
-'
+		cp file2 file12 &&
+		echo "some text" >>file12 &&
+		git add file12 &&
+		git commit -a -m "Copy file2 to file12 with changes" &&
+		git diff-tree -r -C --find-copies-harder HEAD &&
+		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
+		test -n "$level" && test "$level" -gt 0 && test "$level" -lt 98 &&
+		src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
+		test "$src" = file10 &&
+		git config git-p4.detectCopies $(($level + 2)) &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file12 &&
+		p4 filelog //depot/file12 | test_must_fail grep -q "branch from" &&
 
-# Configure branches through git-config and clone them.
-# All files are tested to make sure branches were cloned correctly.
-# Finally, make an update to branch1 on P4 side to check if it is imported
-# correctly by git-p4.
-test_expect_success 'git-p4 clone simple branches' '
-	test_when_finished cleanup_git &&
-	test_create_repo "$git" &&
-	cd "$git" &&
-	git config git-p4.branchList branch1:branch2 &&
-	git config --add git-p4.branchList branch1:branch3 &&
-	"$GITP4" clone --dest=. --detect-branches //depot@all &&
-	git log --all --graph --decorate --stat &&
-	git reset --hard p4/depot/branch1 &&
-	test -f file1 &&
-	test -f file2 &&
-	test -f file3 &&
-	grep -q update file2 &&
-	git reset --hard p4/depot/branch2 &&
-	test -f file1 &&
-	test -f file2 &&
-	test ! -f file3 &&
-	! grep -q update file2 &&
-	git reset --hard p4/depot/branch3 &&
-	test -f file1 &&
-	test -f file2 &&
-	test -f file3 &&
-	grep -q update file2 &&
-	cd "$cli" &&
-	cd branch1 &&
-	p4 edit file2 &&
-	echo file2_ >>file2 &&
-	p4 submit -d "update file2 in branch1" &&
-	cd "$git" &&
-	git reset --hard p4/depot/branch1 &&
-	"$GITP4" rebase &&
-	grep -q file2_ file2
+		cp file2 file13 &&
+		echo "different text" >>file13 &&
+		git add file13 &&
+		git commit -a -m "Copy file2 to file13 with changes" &&
+		git diff-tree -r -C --find-copies-harder HEAD &&
+		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
+		test -n "$level" && test "$level" -gt 2 && test "$level" -lt 100 &&
+		src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
+		test "$src" = file10 &&
+		git config git-p4.detectCopies $(($level - 2)) &&
+		"$GITP4" submit &&
+		p4 filelog //depot/file13 &&
+		p4 filelog //depot/file13 | grep -q "branch from //depot/file"
+	)
 '
 
-test_expect_success 'shutdown' '
-	pid=`pgrep -f p4d` &&
-	test -n "$pid" &&
-	test_debug "ps wl `echo $pid`" &&
-	kill $pid
+test_expect_success 'kill p4d' '
+	kill_p4d
 '
 
 test_done
diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
new file mode 100755
index 0000000..a25f18d
--- /dev/null
+++ b/t/t9801-git-p4-branch.sh
@@ -0,0 +1,233 @@
+#!/bin/sh
+
+test_description='git-p4 p4 branching tests'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+#
+# 1: //depot/main/f1
+# 2: //depot/main/f2
+# 3: integrate //depot/main/... -> //depot/branch1/...
+# 4: //depot/main/f4
+# 5: //depot/branch1/f5
+# .: named branch branch2
+# 6: integrate -b branch2
+# 7: //depot/branch2/f7
+# 8: //depot/main/f8
+#
+test_expect_success 'basic p4 branches' '
+	(
+		cd "$cli" &&
+		mkdir -p main &&
+
+		echo f1 >main/f1 &&
+		p4 add main/f1 &&
+		p4 submit -d "main/f1" &&
+
+		echo f2 >main/f2 &&
+		p4 add main/f2 &&
+		p4 submit -d "main/f2" &&
+
+		p4 integrate //depot/main/... //depot/branch1/... &&
+		p4 submit -d "integrate main to branch1" &&
+
+		echo f4 >main/f4 &&
+		p4 add main/f4 &&
+		p4 submit -d "main/f4" &&
+
+		echo f5 >branch1/f5 &&
+		p4 add branch1/f5 &&
+		p4 submit -d "branch1/f5" &&
+
+		p4 branch -i <<-EOF &&
+		Branch: branch2
+		View: //depot/main/... //depot/branch2/...
+		EOF
+
+		p4 integrate -b branch2 &&
+		p4 submit -d "integrate main to branch2" &&
+
+		echo f7 >branch2/f7 &&
+		p4 add branch2/f7 &&
+		p4 submit -d "branch2/f7" &&
+
+		echo f8 >main/f8 &&
+		p4 add main/f8 &&
+		p4 submit -d "main/f8"
+	)
+'
+
+test_expect_success 'import main, no branch detection' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot/main@all &&
+	(
+		cd "$git" &&
+		git log --oneline --graph --decorate --all &&
+		git rev-list master >wc &&
+		test_line_count = 4 wc
+	)
+'
+
+test_expect_success 'import branch1, no branch detection' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot/branch1@all &&
+	(
+		cd "$git" &&
+		git log --oneline --graph --decorate --all &&
+		git rev-list master >wc &&
+		test_line_count = 2 wc
+	)
+'
+
+test_expect_success 'import branch2, no branch detection' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot/branch2@all &&
+	(
+		cd "$git" &&
+		git log --oneline --graph --decorate --all &&
+		git rev-list master >wc &&
+		test_line_count = 2 wc
+	)
+'
+
+test_expect_success 'import depot, no branch detection' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot@all &&
+	(
+		cd "$git" &&
+		git log --oneline --graph --decorate --all &&
+		git rev-list master >wc &&
+		test_line_count = 8 wc
+	)
+'
+
+test_expect_success 'import depot, branch detection' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" --detect-branches //depot@all &&
+	(
+		cd "$git" &&
+
+		git log --oneline --graph --decorate --all &&
+
+		# 4 main commits
+		git rev-list master >wc &&
+		test_line_count = 4 wc &&
+
+		# 3 main, 1 integrate, 1 on branch2
+		git rev-list p4/depot/branch2 >wc &&
+		test_line_count = 5 wc &&
+
+		# no branch1, since no p4 branch created for it
+		test_must_fail git show-ref p4/depot/branch1
+	)
+'
+
+test_expect_success 'import depot, branch detection, branchList branch definition' '
+	test_when_finished cleanup_git &&
+	test_create_repo "$git" &&
+	(
+		cd "$git" &&
+		git config git-p4.branchList main:branch1 &&
+		"$GITP4" clone --dest=. --detect-branches //depot@all &&
+
+		git log --oneline --graph --decorate --all &&
+
+		# 4 main commits
+		git rev-list master >wc &&
+		test_line_count = 4 wc &&
+
+		# 3 main, 1 integrate, 1 on branch2
+		git rev-list p4/depot/branch2 >wc &&
+		test_line_count = 5 wc &&
+
+		# 2 main, 1 integrate, 1 on branch1
+		git rev-list p4/depot/branch1 >wc &&
+		test_line_count = 4 wc
+	)
+'
+
+test_expect_success 'restart p4d' '
+	kill_p4d &&
+	start_p4d
+'
+
+#
+# 1: //depot/branch1/file1
+#    //depot/branch1/file2
+# 2: integrate //depot/branch1/... -> //depot/branch2/...
+# 3: //depot/branch1/file3
+# 4: //depot/branch1/file2 (edit)
+# 5: integrate //depot/branch1/... -> //depot/branch3/...
+#
+## Create a simple branch structure in P4 depot.
+test_expect_success 'add simple p4 branches' '
+	(
+		cd "$cli" &&
+		mkdir branch1 &&
+		cd branch1 &&
+		echo file1 >file1 &&
+		echo file2 >file2 &&
+		p4 add file1 file2 &&
+		p4 submit -d "branch1" &&
+		p4 integrate //depot/branch1/... //depot/branch2/... &&
+		p4 submit -d "branch2" &&
+		echo file3 >file3 &&
+		p4 add file3 &&
+		p4 submit -d "add file3 in branch1" &&
+		p4 open file2 &&
+		echo update >>file2 &&
+		p4 submit -d "update file2 in branch1" &&
+		p4 integrate //depot/branch1/... //depot/branch3/... &&
+		p4 submit -d "branch3"
+	)
+'
+
+# Configure branches through git-config and clone them.
+# All files are tested to make sure branches were cloned correctly.
+# Finally, make an update to branch1 on P4 side to check if it is imported
+# correctly by git-p4.
+test_expect_success 'git-p4 clone simple branches' '
+	test_when_finished cleanup_git &&
+	test_create_repo "$git" &&
+	(
+		cd "$git" &&
+		git config git-p4.branchList branch1:branch2 &&
+		git config --add git-p4.branchList branch1:branch3 &&
+		"$GITP4" clone --dest=. --detect-branches //depot@all &&
+		git log --all --graph --decorate --stat &&
+		git reset --hard p4/depot/branch1 &&
+		test -f file1 &&
+		test -f file2 &&
+		test -f file3 &&
+		grep -q update file2 &&
+		git reset --hard p4/depot/branch2 &&
+		test -f file1 &&
+		test -f file2 &&
+		test ! -f file3 &&
+		test_must_fail grep -q update file2 &&
+		git reset --hard p4/depot/branch3 &&
+		test -f file1 &&
+		test -f file2 &&
+		test -f file3 &&
+		grep -q update file2 &&
+		cd "$cli" &&
+		cd branch1 &&
+		p4 edit file2 &&
+		echo file2_ >>file2 &&
+		p4 submit -d "update file2 in branch3" &&
+		cd "$git" &&
+		git reset --hard p4/depot/branch1 &&
+		"$GITP4" rebase &&
+		grep -q file2_ file2
+	)
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.7.7

[PATCH 2/6] git-p4: handle utf16 filetype properly

[Pete Wyckoff]

One of the filetypes that p4 supports is utf16.  Its behavior is
odd in this case.  The data delivered through "p4 -G print" is
not encoded in utf16, although "p4 print -o" will produce the
proper utf16-encoded file.

When dealing with this filetype, discard the data from -G, and
instead read the contents directly.

An alternate approach would be to try to encode the data in
python.  That worked for true utf16 files, but for other files
marked as utf16, p4 delivers mangled text in no recognizable encoding.

Add a test case to check utf16 handling, and +k and +ko handling.

Reported-by: Chris Li <git@chrisli.org>
Acked-by: Luke Diamand <luke@diamand.org>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4 |   11 +++++
 t/t9802-git-p4-filetype.sh |  108 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 119 insertions(+), 0 deletions(-)
 create mode 100755 t/t9802-git-p4-filetype.sh

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 2f7b270..e69caf3 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1238,6 +1238,15 @@ class P4Sync(Command, P4UserMap):
             data = ''.join(contents)
             contents = [data[:-1]]
 
+        if file['type'].startswith("utf16"):
+            # p4 delivers different text in the python output to -G
+            # than it does when using "print -o", or normal p4 client
+            # operations.  utf16 is converted to ascii or utf8, perhaps.
+            # But ascii text saved as -t utf16 is completely mangled.
+            # Invoke print -o to get the real contents.
+            text = p4_read_pipe('print -q -o - "%s"' % file['depotFile'])
+            contents = [ text ]
+
         if self.isWindows and file["type"].endswith("text"):
             mangled = []
             for data in contents:
@@ -1245,6 +1254,8 @@ class P4Sync(Command, P4UserMap):
                 mangled.append(data)
             contents = mangled
 
+        # Note that we do not try to de-mangle keywords on utf16 files,
+        # even though in theory somebody may want that.
         if file['type'] in ('text+ko', 'unicode+ko', 'binary+ko'):
             contents = map(lambda text: re.sub(r'(?i)\$(Id|Header):[^$]*\$',r'$\1$', text), contents)
         elif file['type'] in ('text+k', 'ktext', 'kxtext', 'unicode+k', 'binary+k'):
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
new file mode 100755
index 0000000..dff0e02
--- /dev/null
+++ b/t/t9802-git-p4-filetype.sh
@@ -0,0 +1,108 @@
+#!/bin/sh
+
+test_description='git-p4 p4 filetype tests'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+test_expect_success 'utf-16 file create' '
+	(
+		cd "$cli" &&
+
+		# p4 saves this verbatim
+		printf "three\nline\ntext" >f-ascii &&
+		p4 add -t text f-ascii &&
+
+		# p4 adds \377\376 header
+		cp f-ascii f-ascii-as-utf16 &&
+		p4 add -t utf16 f-ascii-as-utf16 &&
+
+		# p4 saves this exactly as iconv produced it
+		printf "three\nline\ntext" | iconv -f ascii -t utf-16 >f-utf16 &&
+		p4 add -t utf16 f-utf16 &&
+
+		# this also is unchanged
+		cp f-utf16 f-utf16-as-text &&
+		p4 add -t text f-utf16-as-text &&
+
+		p4 submit -d "f files" &&
+
+		# force update of client files
+		p4 sync -f
+	)
+'
+
+test_expect_success 'utf-16 file test' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot@all &&
+	(
+		cd "$git" &&
+
+		test_cmp "$cli/f-ascii" f-ascii &&
+		test_cmp "$cli/f-ascii-as-utf16" f-ascii-as-utf16 &&
+		test_cmp "$cli/f-utf16" f-utf16 &&
+		test_cmp "$cli/f-utf16-as-text" f-utf16-as-text
+	)
+'
+
+test_expect_success 'keyword file create' '
+	(
+		cd "$cli" &&
+
+		printf "id\n\$Id\$\n\$Author\$\ntext" >k-text-k &&
+		p4 add -t text+k k-text-k &&
+
+		cp k-text-k k-text-ko &&
+		p4 add -t text+ko k-text-ko &&
+
+		cat k-text-k | iconv -f ascii -t utf-16 >k-utf16-k &&
+		p4 add -t utf16+k k-utf16-k &&
+
+		cp k-utf16-k k-utf16-ko &&
+		p4 add -t utf16+ko k-utf16-ko &&
+
+		p4 submit -d "k files" &&
+		p4 sync -f
+	)
+'
+
+build_smush() {
+	cat >k_smush.py <<-\EOF &&
+	import re, sys
+	sys.stdout.write(re.sub(r'(?i)\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\$', r'$\1$', sys.stdin.read()))
+	EOF
+	cat >ko_smush.py <<-\EOF
+	import re, sys
+	sys.stdout.write(re.sub(r'(?i)\$(Id|Header):[^$]*\$', r'$\1$', sys.stdin.read()))
+	EOF
+}
+
+test_expect_success 'keyword file test' '
+	build_smush &&
+	test_when_finished rm -f k_smush.py ko_smush.py &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot@all &&
+	(
+		cd "$git" &&
+
+		# text, ensure unexpanded
+		python "$TRASH_DIRECTORY/k_smush.py" <"$cli/k-text-k" >cli-k-text-k-smush &&
+		test_cmp cli-k-text-k-smush k-text-k &&
+		python "$TRASH_DIRECTORY/ko_smush.py" <"$cli/k-text-ko" >cli-k-text-ko-smush &&
+		test_cmp cli-k-text-ko-smush k-text-ko &&
+
+		# utf16, even though p4 expands keywords, git-p4 does not
+		# try to undo that
+		test_cmp "$cli/k-utf16-k" k-utf16-k &&
+		test_cmp "$cli/k-utf16-ko" k-utf16-ko
+	)
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.7.7

Re: [PATCH 2/6] git-p4: handle utf16 filetype properly

[Stefano Lattarini]

Hi Pete, and thanks for taking my previous remarks into account.  I have
one more nit/question though ...

On Sunday 16 October 2011, Pete Wyckoff wrote:
> diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
> new file mode 100755
> index 0000000..dff0e02
> --- /dev/null
> +++ b/t/t9802-git-p4-filetype.sh
> @@ -0,0 +1,108 @@
> +#!/bin/sh
> +
> + [SNIP]
> +
> +		printf "three\nline\ntext" >f-ascii &&
>
With this command, the `f-ascii' file won't be newline-terminated.  Is
this intended, or the result of an oversight?  The same goes for further
similar usages in the rest f the patch.

Regards,
  Stefano

Re: [PATCH 2/6] git-p4: handle utf16 filetype properly

[Pete Wyckoff]

stefano.lattarini@gmail.com wrote on Sun, 16 Oct 2011 16:59 +0200:
> Hi Pete, and thanks for taking my previous remarks into account.  I have
> one more nit/question though ...
> 
> On Sunday 16 October 2011, Pete Wyckoff wrote:
> > diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
> > new file mode 100755
> > index 0000000..dff0e02
> > --- /dev/null
> > +++ b/t/t9802-git-p4-filetype.sh
> > @@ -0,0 +1,108 @@
> > +#!/bin/sh
> > +
> > + [SNIP]
> > +
> > +		printf "three\nline\ntext" >f-ascii &&
> >
> With this command, the `f-ascii' file won't be newline-terminated.  Is
> this intended, or the result of an oversight?  The same goes for further
> similar usages in the rest f the patch.

That is harmless.  I think an earlier version used to count the
lines, but now it is just random text.  Ditto the other places.
A trailing \n might be prettier, though, for some future debugging
of the tests.  I tested that both with and without \n pass.

		-- Pete

Re: [PATCH 2/6] git-p4: handle utf16 filetype properly

[Junio C Hamano]

Pete Wyckoff <pw@padd.com> writes:

> +build_smush() {
> +	cat >k_smush.py <<-\EOF &&
> +	import re, sys
> +	sys.stdout.write(re.sub(r'(?i)\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\$', r'$\1$', sys.stdin.read()))
> +	EOF
> +	cat >ko_smush.py <<-\EOF
> +	import re, sys
> +	sys.stdout.write(re.sub(r'(?i)\$(Id|Header):[^$]*\$', r'$\1$', sys.stdin.read()))
> +	EOF
> +}
> +
> +test_expect_success 'keyword file test' '
> +	build_smush &&
> +	test_when_finished rm -f k_smush.py ko_smush.py &&
> +	test_when_finished cleanup_git &&
> +	"$GITP4" clone --dest="$git" //depot@all &&
> +	(
> +		cd "$git" &&
> +
> +		# text, ensure unexpanded
> +		python "$TRASH_DIRECTORY/k_smush.py" <"$cli/k-text-k" >cli-k-text-k-smush &&
> +		test_cmp cli-k-text-k-smush k-text-k &&
> +		python "$TRASH_DIRECTORY/ko_smush.py" <"$cli/k-text-ko" >cli-k-text-ko-smush &&
> +		test_cmp cli-k-text-ko-smush k-text-ko &&
> +
> +		# utf16, even though p4 expands keywords, git-p4 does not
> +		# try to undo that
> +		test_cmp "$cli/k-utf16-k" k-utf16-k &&
> +		test_cmp "$cli/k-utf16-ko" k-utf16-ko
> +	)
> +'

Shouldn't this pay attention to PYTHON_PATH in any way?

Re: [PATCH 2/6] git-p4: handle utf16 filetype properly

[Pete Wyckoff]

gitster@pobox.com wrote on Sun, 16 Oct 2011 11:08 -0700:
> Pete Wyckoff <pw@padd.com> writes:
> > +		python "$TRASH_DIRECTORY/k_smush.py" <"$cli/k-text-k" >cli-k-text-k-smush &&
> 
> Shouldn't this pay attention to PYTHON_PATH in any way?

Yes.  I did not notice that the configuration variable existed.
I'll respond to 1/6 to fix that one too.  Below is the new 2/6,
please.  Includes the extra \n that Stefano noticed too.

		-- Pete

--------------------8<--------------------------
From f177a75182576251ae4c895861d1e0f1f92794fc Mon Sep 17 00:00:00 2001
From: Pete Wyckoff <pw@padd.com>
Date: Sat, 17 Sep 2011 19:16:14 -0400
Subject: [PATCH 2/6] git-p4: handle utf16 filetype properly

One of the filetypes that p4 supports is utf16.  Its behavior is
odd in this case.  The data delivered through "p4 -G print" is
not encoded in utf16, although "p4 print -o" will produce the
proper utf16-encoded file.

When dealing with this filetype, discard the data from -G, and
instead read the contents directly.

An alternate approach would be to try to encode the data in
python.  That worked for true utf16 files, but for other files
marked as utf16, p4 delivers mangled text in no recognizable encoding.

Add a test case to check utf16 handling, and +k and +ko handling.

Reported-by: Chris Li <git@chrisli.org>
Acked-by: Luke Diamand <luke@diamand.org>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4 |   11 +++++
 t/t9802-git-p4-filetype.sh |  108 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 119 insertions(+), 0 deletions(-)
 create mode 100755 t/t9802-git-p4-filetype.sh

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 2f7b270..e69caf3 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1238,6 +1238,15 @@ class P4Sync(Command, P4UserMap):
             data = ''.join(contents)
             contents = [data[:-1]]
 
+        if file['type'].startswith("utf16"):
+            # p4 delivers different text in the python output to -G
+            # than it does when using "print -o", or normal p4 client
+            # operations.  utf16 is converted to ascii or utf8, perhaps.
+            # But ascii text saved as -t utf16 is completely mangled.
+            # Invoke print -o to get the real contents.
+            text = p4_read_pipe('print -q -o - "%s"' % file['depotFile'])
+            contents = [ text ]
+
         if self.isWindows and file["type"].endswith("text"):
             mangled = []
             for data in contents:
@@ -1245,6 +1254,8 @@ class P4Sync(Command, P4UserMap):
                 mangled.append(data)
             contents = mangled
 
+        # Note that we do not try to de-mangle keywords on utf16 files,
+        # even though in theory somebody may want that.
         if file['type'] in ('text+ko', 'unicode+ko', 'binary+ko'):
             contents = map(lambda text: re.sub(r'(?i)\$(Id|Header):[^$]*\$',r'$\1$', text), contents)
         elif file['type'] in ('text+k', 'ktext', 'kxtext', 'unicode+k', 'binary+k'):
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
new file mode 100755
index 0000000..3b358ef
--- /dev/null
+++ b/t/t9802-git-p4-filetype.sh
@@ -0,0 +1,108 @@
+#!/bin/sh
+
+test_description='git-p4 p4 filetype tests'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+test_expect_success 'utf-16 file create' '
+	(
+		cd "$cli" &&
+
+		# p4 saves this verbatim
+		printf "three\nline\ntext\n" >f-ascii &&
+		p4 add -t text f-ascii &&
+
+		# p4 adds \377\376 header
+		cp f-ascii f-ascii-as-utf16 &&
+		p4 add -t utf16 f-ascii-as-utf16 &&
+
+		# p4 saves this exactly as iconv produced it
+		printf "three\nline\ntext\n" | iconv -f ascii -t utf-16 >f-utf16 &&
+		p4 add -t utf16 f-utf16 &&
+
+		# this also is unchanged
+		cp f-utf16 f-utf16-as-text &&
+		p4 add -t text f-utf16-as-text &&
+
+		p4 submit -d "f files" &&
+
+		# force update of client files
+		p4 sync -f
+	)
+'
+
+test_expect_success 'utf-16 file test' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot@all &&
+	(
+		cd "$git" &&
+
+		test_cmp "$cli/f-ascii" f-ascii &&
+		test_cmp "$cli/f-ascii-as-utf16" f-ascii-as-utf16 &&
+		test_cmp "$cli/f-utf16" f-utf16 &&
+		test_cmp "$cli/f-utf16-as-text" f-utf16-as-text
+	)
+'
+
+test_expect_success 'keyword file create' '
+	(
+		cd "$cli" &&
+
+		printf "id\n\$Id\$\n\$Author\$\ntext\n" >k-text-k &&
+		p4 add -t text+k k-text-k &&
+
+		cp k-text-k k-text-ko &&
+		p4 add -t text+ko k-text-ko &&
+
+		cat k-text-k | iconv -f ascii -t utf-16 >k-utf16-k &&
+		p4 add -t utf16+k k-utf16-k &&
+
+		cp k-utf16-k k-utf16-ko &&
+		p4 add -t utf16+ko k-utf16-ko &&
+
+		p4 submit -d "k files" &&
+		p4 sync -f
+	)
+'
+
+build_smush() {
+	cat >k_smush.py <<-\EOF &&
+	import re, sys
+	sys.stdout.write(re.sub(r'(?i)\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\$', r'$\1$', sys.stdin.read()))
+	EOF
+	cat >ko_smush.py <<-\EOF
+	import re, sys
+	sys.stdout.write(re.sub(r'(?i)\$(Id|Header):[^$]*\$', r'$\1$', sys.stdin.read()))
+	EOF
+}
+
+test_expect_success 'keyword file test' '
+	build_smush &&
+	test_when_finished rm -f k_smush.py ko_smush.py &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot@all &&
+	(
+		cd "$git" &&
+
+		# text, ensure unexpanded
+		"$PYTHON_PATH" "$TRASH_DIRECTORY/k_smush.py" <"$cli/k-text-k" >cli-k-text-k-smush &&
+		test_cmp cli-k-text-k-smush k-text-k &&
+		"$PYTHON_PATH" "$TRASH_DIRECTORY/ko_smush.py" <"$cli/k-text-ko" >cli-k-text-ko-smush &&
+		test_cmp cli-k-text-ko-smush k-text-ko &&
+
+		# utf16, even though p4 expands keywords, git-p4 does not
+		# try to undo that
+		test_cmp "$cli/k-utf16-k" k-utf16-k &&
+		test_cmp "$cli/k-utf16-ko" k-utf16-ko
+	)
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.7.7

[PATCH 3/6] git-p4: recognize all p4 filetypes

[Pete Wyckoff]

The previous code was approximate in the filetypes it recognized.
Put in the canonical list and be more careful about matching
elements of the file type.

This might change behavior in some cases, hopefully for the
better.  Windows newline mangling will now happen on all
text files.  Previously some like "text+ko" were oddly exempt.

Files with multiple combinations of modifiers, like "text+klx",
are now recognized for keyword expansion.  I expect these to be
seen only rarely.

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

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index e69caf3..0490ca5 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -118,13 +118,41 @@ def p4_system(cmd):
     real_cmd = p4_build_cmd(cmd)
     return system(real_cmd)
 
-def isP4Exec(kind):
-    """Determine if a Perforce 'kind' should have execute permission
+#
+# Canonicalize the p4 type and return a tuple of the
+# base type, plus any modifiers.  See "p4 help filetypes"
+# for a list and explanation.
+#
+def split_p4_type(p4type):
+
+    p4_filetypes_historical = {
+        "ctempobj": "binary+Sw",
+        "ctext": "text+C",
+        "cxtext": "text+Cx",
+        "ktext": "text+k",
+        "kxtext": "text+kx",
+        "ltext": "text+F",
+        "tempobj": "binary+FSw",
+        "ubinary": "binary+F",
+        "uresource": "resource+F",
+        "uxbinary": "binary+Fx",
+        "xbinary": "binary+x",
+        "xltext": "text+Fx",
+        "xtempobj": "binary+Swx",
+        "xtext": "text+x",
+        "xunicode": "unicode+x",
+        "xutf16": "utf16+x",
+    }
+    if p4type in p4_filetypes_historical:
+        p4type = p4_filetypes_historical[p4type]
+    mods = ""
+    s = p4type.split("+")
+    base = s[0]
+    mods = ""
+    if len(s) > 1:
+        mods = s[1]
+    return (base, mods)
 
-    'p4 help filetypes' gives a list of the types.  If it starts with 'x',
-    or x follows one of a few letters.  Otherwise, if there is an 'x' after
-    a plus sign, it is also executable"""
-    return (re.search(r"(^[cku]?x)|\+.*x", kind) != None)
 
 def setP4ExecBit(file, mode):
     # Reopens an already open file and changes the execute bit to match
@@ -1229,16 +1257,18 @@ class P4Sync(Command, P4UserMap):
         if verbose:
             sys.stderr.write("%s\n" % relPath)
 
-        mode = "644"
-        if isP4Exec(file["type"]):
-            mode = "755"
-        elif file["type"] == "symlink":
-            mode = "120000"
-            # p4 print on a symlink contains "target\n", so strip it off
+        (type_base, type_mods) = split_p4_type(file["type"])
+
+        git_mode = "100644"
+        if "x" in type_mods:
+            git_mode = "100755"
+        if type_base == "symlink":
+            git_mode = "120000"
+            # p4 print on a symlink contains "target\n"; remove the newline
             data = ''.join(contents)
             contents = [data[:-1]]
 
-        if file['type'].startswith("utf16"):
+        if type_base == "utf16":
             # p4 delivers different text in the python output to -G
             # than it does when using "print -o", or normal p4 client
             # operations.  utf16 is converted to ascii or utf8, perhaps.
@@ -1247,7 +1277,9 @@ class P4Sync(Command, P4UserMap):
             text = p4_read_pipe('print -q -o - "%s"' % file['depotFile'])
             contents = [ text ]
 
-        if self.isWindows and file["type"].endswith("text"):
+        # Perhaps windows wants unicode, utf16 newlines translated too;
+        # but this is not doing it.
+        if self.isWindows and type_base == "text":
             mangled = []
             for data in contents:
                 data = data.replace("\r\n", "\n")
@@ -1256,12 +1288,13 @@ class P4Sync(Command, P4UserMap):
 
         # Note that we do not try to de-mangle keywords on utf16 files,
         # even though in theory somebody may want that.
-        if file['type'] in ('text+ko', 'unicode+ko', 'binary+ko'):
-            contents = map(lambda text: re.sub(r'(?i)\$(Id|Header):[^$]*\$',r'$\1$', text), contents)
-        elif file['type'] in ('text+k', 'ktext', 'kxtext', 'unicode+k', 'binary+k'):
-            contents = map(lambda text: re.sub(r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$\n]*\$',r'$\1$', text), contents)
+        if type_base in ("text", "unicode", "binary"):
+            if "ko" in type_mods:
+                contents = map(lambda text: re.sub(r'(?i)\$(Id|Header):[^$]*\$', r'$\1$', text), contents)
+            elif "k" in type_mods:
+                contents = map(lambda text: re.sub(r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$\n]*\$', r'$\1$', text), contents)
 
-        self.gitStream.write("M %s inline %s\n" % (mode, relPath))
+        self.gitStream.write("M %s inline %s\n" % (git_mode, relPath))
 
         # total length...
         length = 0
-- 
1.7.7

[PATCH 4/6] git-p4: stop ignoring apple filetype

[Pete Wyckoff]

Currently "apple" filetype is ignored explicitly, and the file is
not even included in the git repository.  This seems wrong.
Remove this, letting it be treated like a "binary" filetype.

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

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 0490ca5..6b91595 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1247,11 +1247,6 @@ class P4Sync(Command, P4UserMap):
     # - helper for streamP4Files
 
     def streamOneP4File(self, file, contents):
-        if file["type"] == "apple":
-            print "\nfile %s is a strange apple file that forks. Ignoring" % \
-                file['depotFile']
-            return
-
         relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
         relPath = self.wildcard_decode(relPath)
         if verbose:
-- 
1.7.7

[PATCH 5/6] git-p4: keyword flattening fixes

[Pete Wyckoff]

Join the text before looking for keywords.  There is nothing to
prevent the p4 output marshaller from splitting in the middle of a
keyword, although it has never been known to happen.

Also remove the (?i) regexp modifier; perforce keywords are
documented as case-sensitive.

Remove the "\n" end-character match.  I don't know why that is
in there, and every keyword in a fairly large production p4 repository
always ends with a $.

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

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 6b91595..55b1667 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1285,9 +1285,13 @@ class P4Sync(Command, P4UserMap):
         # even though in theory somebody may want that.
         if type_base in ("text", "unicode", "binary"):
             if "ko" in type_mods:
-                contents = map(lambda text: re.sub(r'(?i)\$(Id|Header):[^$]*\$', r'$\1$', text), contents)
+                text = ''.join(contents)
+                text = re.sub(r'\$(Id|Header):[^$]*\$', r'$\1$', text)
+                contents = [ text ]
             elif "k" in type_mods:
-                contents = map(lambda text: re.sub(r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$\n]*\$', r'$\1$', text), contents)
+                text = ''.join(contents)
+                text = re.sub(r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\$', r'$\1$', text)
+                contents = [ text ]
 
         self.gitStream.write("M %s inline %s\n" % (git_mode, relPath))
 
-- 
1.7.7

[PATCH 6/6] git-p4: handle files with shell metacharacters

[Pete Wyckoff]

From: Luke Diamand <luke@diamand.org>

git-p4 used to simply pass strings into system() and popen(), and
relied on the shell doing the necessary expansion. This though meant
that shell metacharacters in file names would be corrupted - for
example files with $ or space in them.

Switch to using subprocess.Popen() and friends, and pass in explicit
arrays in the places where it matters. This then avoids needing shell
expansion.

Add trivial helper functions for some common perforce operations. Add
test case.

[pw: test cleanup]

Signed-off-by: Luke Diamand <luke@diamand.org>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4     |  200 ++++++++++++++++++++++++---------------
 t/t9803-git-shell-metachars.sh |   64 +++++++++++++
 2 files changed, 187 insertions(+), 77 deletions(-)
 create mode 100755 t/t9803-git-shell-metachars.sh

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 55b1667..f885d70 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -22,36 +22,39 @@ def p4_build_cmd(cmd):
     location. It means that hooking into the environment, or other configuration
     can be done more easily.
     """
-    real_cmd = "%s " % "p4"
+    real_cmd = ["p4"]
 
     user = gitConfig("git-p4.user")
     if len(user) > 0:
-        real_cmd += "-u %s " % user
+        real_cmd += ["-u",user]
 
     password = gitConfig("git-p4.password")
     if len(password) > 0:
-        real_cmd += "-P %s " % password
+        real_cmd += ["-P", password]
 
     port = gitConfig("git-p4.port")
     if len(port) > 0:
-        real_cmd += "-p %s " % port
+        real_cmd += ["-p", port]
 
     host = gitConfig("git-p4.host")
     if len(host) > 0:
-        real_cmd += "-h %s " % host
+        real_cmd += ["-h", host]
 
     client = gitConfig("git-p4.client")
     if len(client) > 0:
-        real_cmd += "-c %s " % client
+        real_cmd += ["-c", client]
 
-    real_cmd += "%s" % (cmd)
-    if verbose:
-        print real_cmd
+
+    if isinstance(cmd,basestring):
+        real_cmd = ' '.join(real_cmd) + ' ' + cmd
+    else:
+        real_cmd += cmd
     return real_cmd
 
 def chdir(dir):
-    if os.name == 'nt':
-        os.environ['PWD']=dir
+    # P4 uses the PWD environment variable rather than getcwd(). Since we're
+    # not using the shell, we have to set it ourselves.
+    os.environ['PWD']=dir
     os.chdir(dir)
 
 def die(msg):
@@ -61,29 +64,34 @@ def die(msg):
         sys.stderr.write(msg + "\n")
         sys.exit(1)
 
-def write_pipe(c, str):
+def write_pipe(c, stdin):
     if verbose:
-        sys.stderr.write('Writing pipe: %s\n' % c)
+        sys.stderr.write('Writing pipe: %s\n' % str(c))
 
-    pipe = os.popen(c, 'w')
-    val = pipe.write(str)
-    if pipe.close():
-        die('Command failed: %s' % c)
+    expand = isinstance(c,basestring)
+    p = subprocess.Popen(c, stdin=subprocess.PIPE, shell=expand)
+    pipe = p.stdin
+    val = pipe.write(stdin)
+    pipe.close()
+    if p.wait():
+        die('Command failed: %s' % str(c))
 
     return val
 
-def p4_write_pipe(c, str):
+def p4_write_pipe(c, stdin):
     real_cmd = p4_build_cmd(c)
-    return write_pipe(real_cmd, str)
+    return write_pipe(real_cmd, stdin)
 
 def read_pipe(c, ignore_error=False):
     if verbose:
-        sys.stderr.write('Reading pipe: %s\n' % c)
+        sys.stderr.write('Reading pipe: %s\n' % str(c))
 
-    pipe = os.popen(c, 'rb')
+    expand = isinstance(c,basestring)
+    p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
+    pipe = p.stdout
     val = pipe.read()
-    if pipe.close() and not ignore_error:
-        die('Command failed: %s' % c)
+    if p.wait() and not ignore_error:
+        die('Command failed: %s' % str(c))
 
     return val
 
@@ -93,12 +101,14 @@ def p4_read_pipe(c, ignore_error=False):
 
 def read_pipe_lines(c):
     if verbose:
-        sys.stderr.write('Reading pipe: %s\n' % c)
-    ## todo: check return status
-    pipe = os.popen(c, 'rb')
+        sys.stderr.write('Reading pipe: %s\n' % str(c))
+
+    expand = isinstance(c, basestring)
+    p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
+    pipe = p.stdout
     val = pipe.readlines()
-    if pipe.close():
-        die('Command failed: %s' % c)
+    if pipe.close() or p.wait():
+        die('Command failed: %s' % str(c))
 
     return val
 
@@ -108,15 +118,37 @@ def p4_read_pipe_lines(c):
     return read_pipe_lines(real_cmd)
 
 def system(cmd):
+    expand = isinstance(cmd,basestring)
     if verbose:
-        sys.stderr.write("executing %s\n" % cmd)
-    if os.system(cmd) != 0:
-        die("command failed: %s" % cmd)
+        sys.stderr.write("executing %s\n" % str(cmd))
+    subprocess.check_call(cmd, shell=expand)
 
 def p4_system(cmd):
     """Specifically invoke p4 as the system command. """
     real_cmd = p4_build_cmd(cmd)
-    return system(real_cmd)
+    expand = isinstance(real_cmd, basestring)
+    subprocess.check_call(real_cmd, shell=expand)
+
+def p4_integrate(src, dest):
+    p4_system(["integrate", "-Dt", src, dest])
+
+def p4_sync(path):
+    p4_system(["sync", path])
+
+def p4_add(f):
+    p4_system(["add", f])
+
+def p4_delete(f):
+    p4_system(["delete", f])
+
+def p4_edit(f):
+    p4_system(["edit", f])
+
+def p4_revert(f):
+    p4_system(["revert", f])
+
+def p4_reopen(type, file):
+    p4_system(["reopen", "-t", type, file])
 
 #
 # Canonicalize the p4 type and return a tuple of the
@@ -167,12 +199,12 @@ def setP4ExecBit(file, mode):
         if p4Type[-1] == "+":
             p4Type = p4Type[0:-1]
 
-    p4_system("reopen -t %s %s" % (p4Type, file))
+    p4_reopen(p4Type, file)
 
 def getP4OpenedType(file):
     # Returns the perforce file type for the given file.
 
-    result = p4_read_pipe("opened %s" % file)
+    result = p4_read_pipe(["opened", file])
     match = re.match(".*\((.+)\)\r?$", result)
     if match:
         return match.group(1)
@@ -228,9 +260,17 @@ def isModeExecChanged(src_mode, dst_mode):
     return isModeExec(src_mode) != isModeExec(dst_mode)
 
 def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
-    cmd = p4_build_cmd("-G %s" % (cmd))
+
+    if isinstance(cmd,basestring):
+        cmd = "-G " + cmd
+        expand = True
+    else:
+        cmd = ["-G"] + cmd
+        expand = False
+
+    cmd = p4_build_cmd(cmd)
     if verbose:
-        sys.stderr.write("Opening pipe: %s\n" % cmd)
+        sys.stderr.write("Opening pipe: %s\n" % str(cmd))
 
     # Use a temporary file to avoid deadlocks without
     # subprocess.communicate(), which would put another copy
@@ -238,11 +278,16 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
     stdin_file = None
     if stdin is not None:
         stdin_file = tempfile.TemporaryFile(prefix='p4-stdin', mode=stdin_mode)
-        stdin_file.write(stdin)
+        if isinstance(stdin,basestring):
+            stdin_file.write(stdin)
+        else:
+            for i in stdin:
+                stdin_file.write(i + '\n')
         stdin_file.flush()
         stdin_file.seek(0)
 
-    p4 = subprocess.Popen(cmd, shell=True,
+    p4 = subprocess.Popen(cmd,
+                          shell=expand,
                           stdin=stdin_file,
                           stdout=subprocess.PIPE)
 
@@ -275,7 +320,7 @@ def p4Where(depotPath):
     if not depotPath.endswith("/"):
         depotPath += "/"
     depotPath = depotPath + "..."
-    outputList = p4CmdList("where %s" % depotPath)
+    outputList = p4CmdList(["where", depotPath])
     output = None
     for entry in outputList:
         if "depotFile" in entry:
@@ -477,8 +522,10 @@ def originP4BranchesExist():
 
 def p4ChangesForPaths(depotPaths, changeRange):
     assert depotPaths
-    output = p4_read_pipe_lines("changes " + ' '.join (["%s...%s" % (p, changeRange)
-                                                        for p in depotPaths]))
+    cmd = ['changes']
+    for p in depotPaths:
+        cmd += ["%s...%s" % (p, changeRange)]
+    output = p4_read_pipe_lines(cmd)
 
     changes = {}
     for line in output:
@@ -561,7 +608,7 @@ class P4Debug(Command):
 
     def run(self, args):
         j = 0
-        for output in p4CmdList(" ".join(args)):
+        for output in p4CmdList(args):
             print 'Element: %d' % j
             j += 1
             print output
@@ -715,7 +762,7 @@ class P4Submit(Command, P4UserMap):
                 break
         if not client:
             die("could not get client spec")
-        results = p4CmdList("changes -c %s -m 1" % client)
+        results = p4CmdList(["changes", "-c", client, "-m", "1"])
         for r in results:
             if r.has_key('change'):
                 return r['change']
@@ -778,7 +825,7 @@ class P4Submit(Command, P4UserMap):
         # remove lines in the Files section that show changes to files outside the depot path we're committing into
         template = ""
         inFilesSection = False
-        for line in p4_read_pipe_lines("change -o"):
+        for line in p4_read_pipe_lines(['change', '-o']):
             if line.endswith("\r\n"):
                 line = line[:-2] + "\n"
             if inFilesSection:
@@ -835,7 +882,7 @@ class P4Submit(Command, P4UserMap):
             modifier = diff['status']
             path = diff['src']
             if modifier == "M":
-                p4_system("edit \"%s\"" % path)
+                p4_edit(path)
                 if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
                     filesToChangeExecBit[path] = diff['dst_mode']
                 editedFiles.add(path)
@@ -850,21 +897,21 @@ class P4Submit(Command, P4UserMap):
                     filesToAdd.remove(path)
             elif modifier == "C":
                 src, dest = diff['src'], diff['dst']
-                p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest))
+                p4_integrate(src, dest)
                 if diff['src_sha1'] != diff['dst_sha1']:
-                    p4_system("edit \"%s\"" % (dest))
+                    p4_edit(dest)
                 if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
-                    p4_system("edit \"%s\"" % (dest))
+                    p4_edit(dest)
                     filesToChangeExecBit[dest] = diff['dst_mode']
                 os.unlink(dest)
                 editedFiles.add(dest)
             elif modifier == "R":
                 src, dest = diff['src'], diff['dst']
-                p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest))
+                p4_integrate(src, dest)
                 if diff['src_sha1'] != diff['dst_sha1']:
-                    p4_system("edit \"%s\"" % (dest))
+                    p4_edit(dest)
                 if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
-                    p4_system("edit \"%s\"" % (dest))
+                    p4_edit(dest)
                     filesToChangeExecBit[dest] = diff['dst_mode']
                 os.unlink(dest)
                 editedFiles.add(dest)
@@ -887,9 +934,9 @@ class P4Submit(Command, P4UserMap):
             if response == "s":
                 print "Skipping! Good luck with the next patches..."
                 for f in editedFiles:
-                    p4_system("revert \"%s\"" % f);
+                    p4_revert(f)
                 for f in filesToAdd:
-                    system("rm %s" %f)
+                    os.remove(f)
                 return
             elif response == "a":
                 os.system(applyPatchCmd)
@@ -910,10 +957,10 @@ class P4Submit(Command, P4UserMap):
         system(applyPatchCmd)
 
         for f in filesToAdd:
-            p4_system("add \"%s\"" % f)
+            p4_add(f)
         for f in filesToDelete:
-            p4_system("revert \"%s\"" % f)
-            p4_system("delete \"%s\"" % f)
+            p4_revert(f)
+            p4_delete(f)
 
         # Set/clear executable bits
         for f in filesToChangeExecBit.keys():
@@ -935,7 +982,7 @@ class P4Submit(Command, P4UserMap):
                 del(os.environ["P4DIFF"])
             diff = ""
             for editedFile in editedFiles:
-                diff += p4_read_pipe("diff -du %r" % editedFile)
+                diff += p4_read_pipe(['diff', '-du', editedFile])
 
             newdiff = ""
             for newFile in filesToAdd:
@@ -987,7 +1034,7 @@ class P4Submit(Command, P4UserMap):
                 submitTemplate = message[:message.index(separatorLine)]
                 if self.isWindows:
                     submitTemplate = submitTemplate.replace("\r\n", "\n")
-                p4_write_pipe("submit -i", submitTemplate)
+                p4_write_pipe(['submit', '-i'], submitTemplate)
 
                 if self.preserveUser:
                     if p4User:
@@ -998,10 +1045,10 @@ class P4Submit(Command, P4UserMap):
 
             else:
                 for f in editedFiles:
-                    p4_system("revert \"%s\"" % f);
+                    p4_revert(f)
                 for f in filesToAdd:
-                    p4_system("revert \"%s\"" % f);
-                    system("rm %s" %f)
+                    p4_revert(f)
+                    os.remove(f)
 
             os.remove(fileName)
         else:
@@ -1054,8 +1101,7 @@ class P4Submit(Command, P4UserMap):
 
         chdir(self.clientPath)
         print "Synchronizing p4 checkout..."
-        p4_system("sync ...")
-
+        p4_sync("...")
         self.check()
 
         commits = []
@@ -1269,7 +1315,7 @@ class P4Sync(Command, P4UserMap):
             # operations.  utf16 is converted to ascii or utf8, perhaps.
             # But ascii text saved as -t utf16 is completely mangled.
             # Invoke print -o to get the real contents.
-            text = p4_read_pipe('print -q -o - "%s"' % file['depotFile'])
+            text = p4_read_pipe(['print', '-q', '-o', '-', file['depotFile']])
             contents = [ text ]
 
         # Perhaps windows wants unicode, utf16 newlines translated too;
@@ -1365,10 +1411,11 @@ class P4Sync(Command, P4UserMap):
             def streamP4FilesCbSelf(entry):
                 self.streamP4FilesCb(entry)
 
-            p4CmdList("-x - print",
-                '\n'.join(['%s#%s' % (f['path'], f['rev'])
-                                                  for f in filesToRead]),
-                cb=streamP4FilesCbSelf)
+            fileArgs = ['%s#%s' % (f['path'], f['rev']) for f in filesToRead]
+
+            p4CmdList(["-x", "-", "print"],
+                      stdin=fileArgs,
+                      cb=streamP4FilesCbSelf)
 
             # do the last chunk
             if self.stream_file.has_key('depotFile'):
@@ -1429,8 +1476,8 @@ class P4Sync(Command, P4UserMap):
             if self.verbose:
                 print "Change %s is labelled %s" % (change, labelDetails)
 
-            files = p4CmdList("files " + ' '.join (["%s...@%s" % (p, change)
-                                                    for p in branchPrefixes]))
+            files = p4CmdList(["files"] + ["%s...@%s" % (p, change)
+                                                    for p in branchPrefixes])
 
             if len(files) == len(labelRevisions):
 
@@ -1478,9 +1525,9 @@ class P4Sync(Command, P4UserMap):
             newestChange = 0
             if self.verbose:
                 print "Querying files for label %s" % label
-            for file in p4CmdList("files "
-                                  +  ' '.join (["%s...@%s" % (p, label)
-                                                for p in self.depotPaths])):
+            for file in p4CmdList(["files"] +
+                                      ["%s...@%s" % (p, label)
+                                          for p in self.depotPaths]):
                 revisions[file["depotFile"]] = file["rev"]
                 change = int(file["change"])
                 if change > newestChange:
@@ -1735,10 +1782,9 @@ class P4Sync(Command, P4UserMap):
         newestRevision = 0
 
         fileCnt = 0
-        for info in p4CmdList("files "
-                              +  ' '.join(["%s...%s"
-                                           % (p, revision)
-                                           for p in self.depotPaths])):
+        fileArgs = ["%s...%s" % (p,revision) for p in self.depotPaths]
+
+        for info in p4CmdList(["files"] + fileArgs):
 
             if 'code' in info and info['code'] == 'error':
                 sys.stderr.write("p4 returned an error: %s\n"
diff --git a/t/t9803-git-shell-metachars.sh b/t/t9803-git-shell-metachars.sh
new file mode 100755
index 0000000..db04375
--- /dev/null
+++ b/t/t9803-git-shell-metachars.sh
@@ -0,0 +1,64 @@
+#!/bin/sh
+
+test_description='git-p4 transparency to shell metachars in filenames'
+
+. ./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_success 'shell metachars in filenames' '
+	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEditCheck true &&
+		echo f1 >foo\$bar &&
+		git add foo\$bar &&
+		echo f2 >"file with spaces" &&
+		git add "file with spaces" &&
+		git commit -m "add files" &&
+		P4EDITOR=touch "$GITP4" submit
+	) &&
+	(
+		cd "$cli" &&
+		p4 sync ... &&
+		test -e "file with spaces" &&
+		test -e "foo\$bar"
+	)
+'
+
+test_expect_success 'deleting with shell metachars' '
+	"$GITP4" clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEditCheck true &&
+		git rm foo\$bar &&
+		git rm file\ with\ spaces &&
+		git commit -m "remove files" &&
+		P4EDITOR=touch "$GITP4" submit
+	) &&
+	(
+		cd "$cli" &&
+		p4 sync ... &&
+		test ! -e "file with spaces" &&
+		test ! -e foo\$bar
+	)
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.7.7

[PATCH v2 0/6] git-p4 tests, filetypes, shell metacharacters

[Pete Wyckoff]

This series of six patches has been reviewed on the mailing list
and should be ready for inclusion.

1    - Clean up git-p4 tests, including Junio review comments

2..4 - Handle p4 filetypes better, including Chris's suggested
       utf16 fix

5    - Fix p4 keyword parsing

6    - From Luke: avoid using the shell, so filenames with metacharacters
       are not errantly expanded

Luke Diamand (1):
      git-p4: handle files with shell metacharacters

Pete Wyckoff (5):
      git-p4 tests: refactor and cleanup
      git-p4: handle utf16 filetype properly
      git-p4: recognize all p4 filetypes
      git-p4: stop ignoring apple filetype
      git-p4: keyword flattening fixes

 contrib/fast-import/git-p4     |  287 +++++++++++++-------
 t/lib-git-p4.sh                |   74 +++++
 t/t9800-git-p4.sh              |  574 ++++++++++++++++++----------------------
 t/t9801-git-p4-branch.sh       |  233 ++++++++++++++++
 t/t9802-git-p4-filetype.sh     |  108 ++++++++
 t/t9803-git-shell-metachars.sh |   64 +++++
 6 files changed, 918 insertions(+), 422 deletions(-)
 create mode 100644 t/lib-git-p4.sh
 create mode 100755 t/t9801-git-p4-branch.sh
 create mode 100755 t/t9802-git-p4-filetype.sh
 create mode 100755 t/t9803-git-shell-metachars.sh

[PATCH 2/6] git-p4: handle utf16 filetype properly

[Pete Wyckoff]

One of the filetypes that p4 supports is utf16.  Its behavior is
odd in this case.  The data delivered through "p4 -G print" is
not encoded in utf16, although "p4 print -o" will produce the
proper utf16-encoded file.

When dealing with this filetype, discard the data from -G, and
instead read the contents directly.

An alternate approach would be to try to encode the data in
python.  That worked for true utf16 files, but for other files
marked as utf16, p4 delivers mangled text in no recognizable encoding.

Add a test case to check utf16 handling, and +k and +ko handling.

Reported-by: Chris Li <git@chrisli.org>
Acked-by: Luke Diamand <luke@diamand.org>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4 |   11 +++++
 t/t9802-git-p4-filetype.sh |  108 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 119 insertions(+), 0 deletions(-)
 create mode 100755 t/t9802-git-p4-filetype.sh

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 2f7b270..e69caf3 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1238,6 +1238,15 @@ class P4Sync(Command, P4UserMap):
             data = ''.join(contents)
             contents = [data[:-1]]
 
+        if file['type'].startswith("utf16"):
+            # p4 delivers different text in the python output to -G
+            # than it does when using "print -o", or normal p4 client
+            # operations.  utf16 is converted to ascii or utf8, perhaps.
+            # But ascii text saved as -t utf16 is completely mangled.
+            # Invoke print -o to get the real contents.
+            text = p4_read_pipe('print -q -o - "%s"' % file['depotFile'])
+            contents = [ text ]
+
         if self.isWindows and file["type"].endswith("text"):
             mangled = []
             for data in contents:
@@ -1245,6 +1254,8 @@ class P4Sync(Command, P4UserMap):
                 mangled.append(data)
             contents = mangled
 
+        # Note that we do not try to de-mangle keywords on utf16 files,
+        # even though in theory somebody may want that.
         if file['type'] in ('text+ko', 'unicode+ko', 'binary+ko'):
             contents = map(lambda text: re.sub(r'(?i)\$(Id|Header):[^$]*\$',r'$\1$', text), contents)
         elif file['type'] in ('text+k', 'ktext', 'kxtext', 'unicode+k', 'binary+k'):
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
new file mode 100755
index 0000000..cf07e6d
--- /dev/null
+++ b/t/t9802-git-p4-filetype.sh
@@ -0,0 +1,108 @@
+#!/bin/sh
+
+test_description='git-p4 p4 filetype tests'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+test_expect_success 'utf-16 file create' '
+	(
+		cd "$cli" &&
+
+		# p4 saves this verbatim
+		echo -e "three\nline\ntext" >f-ascii &&
+		p4 add -t text f-ascii &&
+
+		# p4 adds \377\376 header
+		cp f-ascii f-ascii-as-utf16 &&
+		p4 add -t utf16 f-ascii-as-utf16 &&
+
+		# p4 saves this exactly as iconv produced it
+		echo -e "three\nline\ntext" | iconv -f ascii -t utf-16 >f-utf16 &&
+		p4 add -t utf16 f-utf16 &&
+
+		# this also is unchanged
+		cp f-utf16 f-utf16-as-text &&
+		p4 add -t text f-utf16-as-text &&
+
+		p4 submit -d "f files" &&
+
+		# force update of client files
+		p4 sync -f
+	)
+'
+
+test_expect_success 'utf-16 file test' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot@all &&
+	(
+		cd "$git" &&
+
+		test_cmp "$cli/f-ascii" f-ascii &&
+		test_cmp "$cli/f-ascii-as-utf16" f-ascii-as-utf16 &&
+		test_cmp "$cli/f-utf16" f-utf16 &&
+		test_cmp "$cli/f-utf16-as-text" f-utf16-as-text
+	)
+'
+
+test_expect_success 'keyword file create' '
+	(
+		cd "$cli" &&
+
+		echo -e "id\n\$Id\$\n\$Author\$\ntext" >k-text-k &&
+		p4 add -t text+k k-text-k &&
+
+		cp k-text-k k-text-ko &&
+		p4 add -t text+ko k-text-ko &&
+
+		cat k-text-k | iconv -f ascii -t utf-16 >k-utf16-k &&
+		p4 add -t utf16+k k-utf16-k &&
+
+		cp k-utf16-k k-utf16-ko &&
+		p4 add -t utf16+ko k-utf16-ko &&
+
+		p4 submit -d "k files" &&
+		p4 sync -f
+	)
+'
+
+build_smush() {
+	cat >k_smush.py <<-EOF &&
+	import re, sys
+	sys.stdout.write(re.sub(r'(?i)\\\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\\\$', r'$\1$', sys.stdin.read()))
+	EOF
+	cat >ko_smush.py <<-EOF
+	import re, sys
+	sys.stdout.write(re.sub(r'(?i)\\\$(Id|Header):[^$]*\\\$', r'$\1$', sys.stdin.read()))
+	EOF
+}
+
+test_expect_success 'keyword file test' '
+	build_smush &&
+	test_when_finished rm -f k_smush.py ko_smush.py &&
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot@all &&
+	(
+		cd "$git" &&
+
+		# text, ensure unexpanded
+		python "$TRASH_DIRECTORY/k_smush.py" <"$cli/k-text-k" >cli-k-text-k-smush &&
+		test_cmp cli-k-text-k-smush k-text-k &&
+		python "$TRASH_DIRECTORY/ko_smush.py" <"$cli/k-text-ko" >cli-k-text-ko-smush &&
+		test_cmp cli-k-text-ko-smush k-text-ko &&
+
+		# utf16, even though p4 expands keywords, git-p4 does not
+		# try to undo that
+		test_cmp "$cli/k-utf16-k" k-utf16-k &&
+		test_cmp "$cli/k-utf16-ko" k-utf16-ko
+	)
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.7.6.3

Re: [PATCH 2/6] git-p4: handle utf16 filetype properly

[Stefano Lattarini]

Hello Pete.

One more "outsider nit" here ...

On Saturday 15 October 2011, Pete Wyckoff wrote:
>
> [SNIP]
> 
> Add a test case to check utf16 handling, and +k and +ko handling.
>
> [SNIP]
>
> diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
> new file mode 100755
> index 0000000..cf07e6d
> --- /dev/null
> +++ b/t/t9802-git-p4-filetype.sh
> @@ -0,0 +1,108 @@
> +#!/bin/sh
> +
> +test_description='git-p4 p4 filetype tests'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> +	start_p4d
> +'
> +
> +test_expect_success 'utf-16 file create' '
> +	(
> +		cd "$cli" &&
> +
> +		# p4 saves this verbatim
> +		echo -e "three\nline\ntext" >f-ascii &&
>
Not portable to (at least) solaris /usr/xpg4/bin/sh and /bin/ksh:

 $ /bin/ksh -c 'echo -e "three\nline\ntext"'
 -e three
 line
 text

In fact, use of options and/or escape sequences in "echo" arguments is
highly unportable (see the entry for `echo' in the "Limitations of Shell
Builtins" section of the autoconf manual); your best bet is to use printf,
which is more portable and well-behaved (at least on systems that are not
musuem pieces).

>
> [SNIP]
>
> +
> +build_smush() {
> +	cat >k_smush.py <<-EOF &&
> +	import re, sys
> +	sys.stdout.write(re.sub(r'(?i)\\\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\\\$', r'$\1$', sys.stdin.read()))
> +	EOF
>
This is a basically a stylistic nit, so fell free to disregard it completely,
but ... wouldn't it be simpler to quote the "EOF" at the here-doc beginning,
so that you don't have to escape all the backslashes in the here-doc itself?
Similarly for other later usages.

Regards,
  Stefano

Re: [PATCH 2/6] git-p4: handle utf16 filetype properly

[Pete Wyckoff]

stefano.lattarini@gmail.com wrote on Sun, 16 Oct 2011 11:52 +0200:
> Hello Pete.
> 
> One more "outsider nit" here ...
> 
> On Saturday 15 October 2011, Pete Wyckoff wrote:
> >
> > [SNIP]
> > 
> > Add a test case to check utf16 handling, and +k and +ko handling.
> >
> > [SNIP]
> >
> > diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
> > new file mode 100755
> > index 0000000..cf07e6d
> > --- /dev/null
> > +++ b/t/t9802-git-p4-filetype.sh
> > @@ -0,0 +1,108 @@
> > +#!/bin/sh
> > +
> > +test_description='git-p4 p4 filetype tests'
> > +
> > +. ./lib-git-p4.sh
> > +
> > +test_expect_success 'start p4d' '
> > +	start_p4d
> > +'
> > +
> > +test_expect_success 'utf-16 file create' '
> > +	(
> > +		cd "$cli" &&
> > +
> > +		# p4 saves this verbatim
> > +		echo -e "three\nline\ntext" >f-ascii &&
> >
> Not portable to (at least) solaris /usr/xpg4/bin/sh and /bin/ksh:
> 
>  $ /bin/ksh -c 'echo -e "three\nline\ntext"'
>  -e three
>  line
>  text
> 
> In fact, use of options and/or escape sequences in "echo" arguments is
> highly unportable (see the entry for `echo' in the "Limitations of Shell
> Builtins" section of the autoconf manual); your best bet is to use printf,
> which is more portable and well-behaved (at least on systems that are not
> musuem pieces).

Indeed.  The t/ source consistently uses printf where \n is
needed.

> >
> > [SNIP]
> >
> > +
> > +build_smush() {
> > +	cat >k_smush.py <<-EOF &&
> > +	import re, sys
> > +	sys.stdout.write(re.sub(r'(?i)\\\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\\\$', r'$\1$', sys.stdin.read()))
> > +	EOF
> >
> This is a basically a stylistic nit, so fell free to disregard it completely,
> but ... wouldn't it be simpler to quote the "EOF" at the here-doc beginning,
> so that you don't have to escape all the backslashes in the here-doc itself?
> Similarly for other later usages.

Switched to using \EOF and got rid of the extranneous \.

Thanks,

		-- Pete