[PATCH 0/5] git-p4 filetype handling

[PATCH 0/5] git-p4 filetype handling

[Pete Wyckoff]

Here's a series that includes some changes to make
git-p4 handle p4 filetypes better.  This work was
inspired by Chris Li, and includes some refactoring
of the git-p4 tests that grew out of looking at
Vitor's branch changes.

It could use review for generic test beauty, as well
as for the git-p4 filetype specifics in the code.

 contrib/fast-import/git-p4 |   91 +++++++++++++-----
 t/lib-git-p4.sh            |   55 +++++++++++
 t/t9800-git-p4.sh          |  108 ++--------------------
 t/t9801-git-p4-branch.sh   |  221 ++++++++++++++++++++++++++++++++++++++++++++
 t/t9802-git-p4-filetype.sh |  107 +++++++++++++++++++++
 5 files changed, 457 insertions(+), 125 deletions(-)

Thanks,

		-- Pete

[PATCH 1/5] git-p4 tests: refactor, split out common functions

[Pete Wyckoff]

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

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

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/lib-git-p4.sh          |   55 ++++++++++++
 t/t9800-git-p4.sh        |  108 ++---------------------
 t/t9801-git-p4-branch.sh |  221 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 283 insertions(+), 101 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..dbc1499
--- /dev/null
+++ b/t/lib-git-p4.sh
@@ -0,0 +1,55 @@
+#
+# Library code for git-p4 tests
+#
+
+. ./test-lib.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
+export P4CLIENT=client
+
+db="$TRASH_DIRECTORY/db"
+cli="$TRASH_DIRECTORY/cli"
+git="$TRASH_DIRECTORY/git"
+
+start_p4d()
+{
+	mkdir -p "$db" &&
+	p4d -q -d -r "$db" -p $P4DPORT &&
+	mkdir -p "$cli" &&
+	mkdir -p "$git" &&
+	cd "$cli" &&
+	p4 client -i <<-EOF
+	Client: client
+	Description: client
+	Root: $cli
+	View: //depot/... //client/...
+	EOF
+}
+
+kill_p4d()
+{
+	pid=`pgrep -f p4d` &&
+	test -n "$pid" &&
+	for i in {1..5} ; do
+	    test_debug "ps wl `echo $pid`" &&
+	    kill $pid 2>/dev/null &&
+	    pgrep -f p4d >/dev/null || break &&
+	    sleep 0.2
+	done &&
+	rm -rf "$db" &&
+	rm -rf "$cli"
+}
+
+cleanup_git() {
+	cd "$TRASH_DIRECTORY" &&
+	rm -rf "$git" &&
+	mkdir "$git"
+}
diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index 01ba041..bb89b63 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -2,40 +2,16 @@
 
 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' '
+	kill_p4d || : &&
+	start_p4d &&
+	cd "$TRASH_DIRECTORY"
 '
 
 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" &&
@@ -45,12 +21,6 @@ test_expect_success 'add p4 files' '
 	cd "$TRASH_DIRECTORY"
 '
 
-cleanup_git() {
-	cd "$TRASH_DIRECTORY" &&
-	rm -rf "$git" &&
-	mkdir "$git"
-}
-
 test_expect_success 'basic git-p4 clone' '
 	"$GITP4" clone --dest="$git" //depot &&
 	test_when_finished cleanup_git &&
@@ -405,72 +375,8 @@ test_expect_success 'detect copies' '
 	p4 filelog //depot/file13 | 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"
-'
-
-# 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
-'
-
-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..0b6c1d1
--- /dev/null
+++ b/t/t9801-git-p4-branch.sh
@@ -0,0 +1,221 @@
+#!/bin/sh
+
+test_description='git-p4 p4 branching tests'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	kill_p4d || : &&
+	start_p4d &&
+	cd "$TRASH_DIRECTORY"
+'
+
+#
+# 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" &&
+
+	cd "$TRASH_DIRECTORY"
+'
+
+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 &&
+	cd "$TRASH_DIRECTORY"
+'
+
+#
+# 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" &&
+	cd "$TRASH_DIRECTORY"
+'
+
+# 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.6.3

Re: [PATCH 1/5] git-p4 tests: refactor, split out common functions

[Junio C Hamano]

Pete Wyckoff <pw@padd.com> writes:

> Introduce a library for functions that are common to
> multiple git-p4 test files.
>
> Separate the tests related to detecting p4 branches
> into their own file, and add a few more.
>
> Signed-off-by: Pete Wyckoff <pw@padd.com>
> ---
>  t/lib-git-p4.sh          |   55 ++++++++++++
>  t/t9800-git-p4.sh        |  108 ++---------------------
>  t/t9801-git-p4-branch.sh |  221 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 283 insertions(+), 101 deletions(-)
>  create mode 100644 t/lib-git-p4.sh
>  create mode 100755 t/t9801-git-p4-branch.sh

I take that you meant "coding style" by "generic test beauty" in the cover
letter, so here are some minor nitpicks.

> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> new file mode 100644
> index 0000000..dbc1499
> --- /dev/null
> +++ b/t/lib-git-p4.sh
> @@ -0,0 +1,55 @@
> +#
> +# Library code for git-p4 tests
> +#
> +
> +. ./test-lib.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

What happens when tests are run in parallel (make -j or prove --jobs) and
9800 and 9801 are run at the same time?

> +export P4PORT=localhost:$P4DPORT
> +export P4CLIENT=client
> +
> +db="$TRASH_DIRECTORY/db"
> +cli="$TRASH_DIRECTORY/cli"
> +git="$TRASH_DIRECTORY/git"
> +
> +start_p4d()
> +{

Prevalent style in t/ and scripted part of Git in general is to begin a
shell function like this, with SP on both sides of () and opening brace
on the same line.

	start_p4d () {

> +	mkdir -p "$db" &&
> +	p4d -q -d -r "$db" -p $P4DPORT &&
> +	mkdir -p "$cli" &&
> +	mkdir -p "$git" &&
> +	cd "$cli" &&
> +	p4 client -i <<-EOF
> +	Client: client
> +	Description: client
> +	Root: $cli
> +	View: //depot/... //client/...
> +	EOF
> +}
> +
> +kill_p4d()
> +{
> +	pid=`pgrep -f p4d` &&
> +	test -n "$pid" &&

It is unfortunate that you have to use pgrep. I am unfamiliar with p4, but
do you have any control how p4d is started during this test? If the first
use of client automagically starts p4d without your control, that would be
harder to arrange, but the point I am getting at is that if you know when
you start p4d yourself and that is the only p4d process you use, you
should be keep its pid in $TRASH_DIRECTORY somewhere and replace these
with

	pid=$(cat "$TRASH_DIRECTORY/p4d_pid") &&
        kill -0 "$pid"

to see if that daemon is still alive. 

You call kill_p4d at the very beginning of t9800; what instance of p4d are
you trying to kill? Who could have started it? For you to be able to kill
(and nobody sane would be running the test suite as "root", I hope), it
would be your process, but would it be possible that you are doing some
other important thing using p4 that is not related to git-p4 development
or testing at all, perhaps listening to a port different from 10669? Would
it be necessary to kill that other p4d to run these tests in a predicatable
and reproducible environment?

I would very much more prefer if at the very beginning you started p4d at
the port assigned for the test and fail the test if it cannot start for
whatever reason. Perhaps the reason you cannot start a p4d is because a
stale p4d instance is hanging around from previous round of test, and if
that is the case, then that is the bug we need to fix in the _previous_
test, not something we want to sweep under the rug by killing it during
this round of test.

> +	for i in {1..5} ; do

That {1..5} does not pass POSIX shells, such as /bin/dash.

	for i in 1 2 3 4 5
	do
		...

> +	    test_debug "ps wl `echo $pid`" &&
> +	    kill $pid 2>/dev/null &&
> +	    pgrep -f p4d >/dev/null || break &&

You are saying "all of these things would hold true if we attempt to kill
it and it still is alive" with the chain of "&&" up to that last pgrep,
and then with "||", you say "otherwise we do not have to keep sending the
signal to it anymore". But the extra "&&" after "break" is unneeded and
misleading.

> +	    sleep 0.2

That 0.2 does not look like a non-negative decimal interger like POSIX
wants to have.

> +	done &&
> +	rm -rf "$db" &&
> +	rm -rf "$cli"
> +}
> +
> +cleanup_git() {
> +	cd "$TRASH_DIRECTORY" &&
> +	rm -rf "$git" &&
> +	mkdir "$git"
> +}
> diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
> index 01ba041..bb89b63 100755
> --- a/t/t9800-git-p4.sh
> +++ b/t/t9800-git-p4.sh
> @@ -2,40 +2,16 @@
>  
>  test_description='git-p4 tests'
>  
> -. ./test-lib.sh
> +. ./lib-git-p4.sh
>  
> +test_expect_success 'start p4d' '
> +	kill_p4d || : &&
> +	start_p4d &&
> +	cd "$TRASH_DIRECTORY"
>  '

Don't "chdir" around in the test, and worse yet hide some "cd" in helper
functions. The seemingly unnecessary "cd $TRASH_DIRECTORY" at the end,
which may not even happen if start_p4d fails, is because start_p4d has a
hidden "cd" somewhere (which in turn may or may not run depending on where
in the && chain you have a failure).

One way to keep the test cleaner is to do the helper functions like the
following, so that the callers do not have to worry about where they end
up with:

	start_p4d () {
		mkdir -p "$db" "$cli" "$git" &&
                p4d -q -d -r "$db" -p "$P4DPORT" &&
		(
			cd "$cli" &&
			p4 client -i <<-EOF
			...
			EOF
		)
	}

>  test_expect_success 'add p4 files' '
>  	cd "$cli" &&
>  	echo file1 >file1 &&
>  	p4 add file1 &&
>  	p4 submit -d "file1" &&
>  ...
>  	cd "$TRASH_DIRECTORY"
>  '

The same issue here.

	test_expect_success 'add p4 files' '
		(
			cd "$cli" &&
			echo file1 >file1 &&
			...
		)
	'

Re: [PATCH 1/5] git-p4 tests: refactor, split out common functions

[Pete Wyckoff]

gitster@pobox.com wrote on Sun, 18 Sep 2011 14:48 -0700:
> Pete Wyckoff <pw@padd.com> writes:
> 
> > Introduce a library for functions that are common to
> > multiple git-p4 test files.
> >
> > Separate the tests related to detecting p4 branches
> > into their own file, and add a few more.
> >
> > Signed-off-by: Pete Wyckoff <pw@padd.com>
> > ---
> >  t/lib-git-p4.sh          |   55 ++++++++++++
> >  t/t9800-git-p4.sh        |  108 ++---------------------
> >  t/t9801-git-p4-branch.sh |  221 ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 283 insertions(+), 101 deletions(-)
> >  create mode 100644 t/lib-git-p4.sh
> >  create mode 100755 t/t9801-git-p4-branch.sh
> 
> I take that you meant "coding style" by "generic test beauty" in the cover
> letter, so here are some minor nitpicks.

Definitely appreciated.

> > diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> > new file mode 100644
> > index 0000000..dbc1499
> > --- /dev/null
> > +++ b/t/lib-git-p4.sh
> > @@ -0,0 +1,55 @@
> > +#
> > +# Library code for git-p4 tests
> > +#
> > +
> > +. ./test-lib.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
> 
> What happens when tests are run in parallel (make -j or prove --jobs) and
> 9800 and 9801 are run at the same time?

Hard... part of the p4d pain below.

> > +export P4PORT=localhost:$P4DPORT
> > +export P4CLIENT=client
> > +
> > +db="$TRASH_DIRECTORY/db"
> > +cli="$TRASH_DIRECTORY/cli"
> > +git="$TRASH_DIRECTORY/git"
> > +
> > +start_p4d()
> > +{
> 
> Prevalent style in t/ and scripted part of Git in general is to begin a
> shell function like this, with SP on both sides of () and opening brace
> on the same line.

Thanks, fixed.

> 	start_p4d () {
> 
> > +	mkdir -p "$db" &&
> > +	p4d -q -d -r "$db" -p $P4DPORT &&
> > +	mkdir -p "$cli" &&
> > +	mkdir -p "$git" &&
> > +	cd "$cli" &&
> > +	p4 client -i <<-EOF
> > +	Client: client
> > +	Description: client
> > +	Root: $cli
> > +	View: //depot/... //client/...
> > +	EOF
> > +}
> > +
> > +kill_p4d()
> > +{
> > +	pid=`pgrep -f p4d` &&
> > +	test -n "$pid" &&
> 
> It is unfortunate that you have to use pgrep. I am unfamiliar with p4, but
> do you have any control how p4d is started during this test? If the first
> use of client automagically starts p4d without your control, that would be
> harder to arrange, but the point I am getting at is that if you know when
> you start p4d yourself and that is the only p4d process you use, you
> should be keep its pid in $TRASH_DIRECTORY somewhere and replace these
> with
> 
> 	pid=$(cat "$TRASH_DIRECTORY/p4d_pid") &&
>         kill -0 "$pid"
> 
> to see if that daemon is still alive. 
> 
> You call kill_p4d at the very beginning of t9800; what instance of p4d are
> you trying to kill? Who could have started it? For you to be able to kill
> (and nobody sane would be running the test suite as "root", I hope), it
> would be your process, but would it be possible that you are doing some
> other important thing using p4 that is not related to git-p4 development
> or testing at all, perhaps listening to a port different from 10669? Would
> it be necessary to kill that other p4d to run these tests in a predicatable
> and reproducible environment?
> 
> I would very much more prefer if at the very beginning you started p4d at
> the port assigned for the test and fail the test if it cannot start for
> whatever reason. Perhaps the reason you cannot start a p4d is because a
> stale p4d instance is hanging around from previous round of test, and if
> that is the case, then that is the bug we need to fix in the _previous_
> test, not something we want to sweep under the rug by killing it during
> this round of test.

Yes, this is all very shoddy, and I suspect that I don't need the
initial kill_p4d as long as the other tests fail nicely.

But your point about killing the _wrong_ p4d is good.  But p4d
isn't friendly like other daemons that will drop a pid file for
help in future cleanup.  I can run it without the "-d", in which
case it does not daemonize.  Then I should be able to use shell
job control to kill off the one I started, hopefully.

Finding an unused port is also a pain.  I'll see if p4d produces
parseable error messages that can indicate a taken port.
Otherwise, hardcoding a port per test is a good idea, and will
fix the parallel-run problem.  Hopefully no "production" p4d
also happens to be running at the same random port with this
approach.

> > +	for i in {1..5} ; do
> 
> That {1..5} does not pass POSIX shells, such as /bin/dash.
> 
> 	for i in 1 2 3 4 5
> 	do
> 		...

Ah.  Or `seq`, but not sure its existence can be counted on.
Fixed as you suggest for this teensy list.

> > +	    test_debug "ps wl `echo $pid`" &&
> > +	    kill $pid 2>/dev/null &&
> > +	    pgrep -f p4d >/dev/null || break &&
> 
> You are saying "all of these things would hold true if we attempt to kill
> it and it still is alive" with the chain of "&&" up to that last pgrep,
> and then with "||", you say "otherwise we do not have to keep sending the
> signal to it anymore". But the extra "&&" after "break" is unneeded and
> misleading.

Yes, that is icky.  I'll send this instead, next time (unless I
can fix the p4d issues):

	kill $pid 2>/dev/null &&
	if pgrep -f p4d >/dev/null ; then
	    break
	fi &&

> > +	    sleep 0.2
> 
> That 0.2 does not look like a non-negative decimal interger like POSIX
> wants to have.

Switched to 1.  Optimization of 0.2 isn't important.

> > +test_expect_success 'start p4d' '
> > +	kill_p4d || : &&
> > +	start_p4d &&
> > +	cd "$TRASH_DIRECTORY"
> >  '
> 
> Don't "chdir" around in the test, and worse yet hide some "cd" in helper
> functions. The seemingly unnecessary "cd $TRASH_DIRECTORY" at the end,
> which may not even happen if start_p4d fails, is because start_p4d has a
> hidden "cd" somewhere (which in turn may or may not run depending on where
> in the && chain you have a failure).
> 
> One way to keep the test cleaner is to do the helper functions like the
> following, so that the callers do not have to worry about where they end
> up with:
> 
> 	start_p4d () {
> 		mkdir -p "$db" "$cli" "$git" &&
>                 p4d -q -d -r "$db" -p "$P4DPORT" &&
> 		(
> 			cd "$cli" &&
> 			p4 client -i <<-EOF
> 			...
> 			EOF
> 		)
> 	}

I see; chdir in a subshell.  Will rework these to remove the
silly post-test cd, and put other cds inside shells.

> >  test_expect_success 'add p4 files' '
> >  	cd "$cli" &&
> >  	echo file1 >file1 &&
> >  	p4 add file1 &&
> >  	p4 submit -d "file1" &&
> >  ...
> >  	cd "$TRASH_DIRECTORY"
> >  '
> 
> The same issue here.
> 
> 	test_expect_success 'add p4 files' '
> 		(
> 			cd "$cli" &&
> 			echo file1 >file1 &&
> 			...
> 		)
> 	'

Thanks for the useful critique.  I'll think through the difficult
issues with p4d, and resubmit the whole series post-release.

		-- Pete

Re: [PATCH 1/5] git-p4 tests: refactor, split out common functions

[Junio C Hamano]

Pete Wyckoff <pw@padd.com> writes:

> Yes, this is all very shoddy, and I suspect that I don't need the
> initial kill_p4d as long as the other tests fail nicely.
>
> But your point about killing the _wrong_ p4d is good.  But p4d
> isn't friendly like other daemons that will drop a pid file for
> help in future cleanup.  I can run it without the "-d", in which
> case it does not daemonize.  Then I should be able to use shell
> job control to kill off the one I started, hopefully.

Yeah, something like

	: start
	p4d &
        echo $! >$pidfile

        : stop
	while kill -0 $pidfile
        do
	        kill $pidfile
                sleep 1
	done

> Finding an unused port is also a pain.

I think the way t55xx series assign LIB_HTTPD_PORT is an acceptable
compromise.

Re: [PATCH 1/5] git-p4 tests: refactor, split out common functions

[Junio C Hamano]

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

> Yeah, something like
>
> 	: start
> 	p4d &
>         echo $! >$pidfile

Obviously I should have written:

	: stop
        pid=$(cat "$pidfile")
	while kill -0 $pid
	do
		kill $pid
		sleep 1
	done

Sorry for the noise.

>         : stop
> 	while kill -0 $pidfile
>         do
> 	        kill $pidfile
>                 sleep 1
> 	done
>
>> Finding an unused port is also a pain.
>
> I think the way t55xx series assign LIB_HTTPD_PORT is an acceptable
> compromise.

[PATCH 2/5] 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
intstead 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>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4 |   11 +++++
 t/t9802-git-p4-filetype.sh |  107 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 118 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..f112eaa
--- /dev/null
+++ b/t/t9802-git-p4-filetype.sh
@@ -0,0 +1,107 @@
+#!/bin/sh
+
+test_description='git-p4 p4 filetype tests'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	kill_p4d || : &&
+	start_p4d &&
+	cd "$TRASH_DIRECTORY"
+'
+
+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 &&
+	cd "$TRASH_DIRECTORY"
+'
+
+test_expect_success 'utf-16 file test' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot@all &&
+	cd "$git" &&
+
+	cmp "$cli/f-ascii" f-ascii &&
+	cmp "$cli/f-ascii-as-utf16" f-ascii-as-utf16 &&
+	cmp "$cli/f-utf16" f-utf16 &&
+	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 &&
+	cd "$TRASH_DIRECTORY"
+'
+
+ko_smush() {
+	cat >smush.py <<-EOF &&
+	import re, sys
+	sys.stdout.write(re.sub(r'(?i)\\\$(Id|Header):[^$]*\\\$', r'$\1$', sys.stdin.read()))
+	EOF
+	python smush.py < "$1"
+}
+
+k_smush() {
+	cat >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
+	python smush.py < "$1"
+}
+
+test_expect_success 'keyword file test' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot@all &&
+	cd "$git" &&
+
+	# text, ensure unexpanded
+	k_smush "$cli/k-text-k" > cli-k-text-k-smush &&
+	cmp cli-k-text-k-smush k-text-k &&
+	ko_smush "$cli/k-text-ko" > cli-k-text-ko-smush &&
+	cmp cli-k-text-ko-smush k-text-ko &&
+
+	# utf16, even though p4 expands keywords, git-p4 does not
+	# try to undo that
+	cmp "$cli/k-utf16-k" k-utf16-k &&
+	cmp "$cli/k-utf16-ko" k-utf16-ko
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.7.6.3

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

[Luke Diamand]

On 18/09/11 02:28, Pete Wyckoff wrote:
> 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
> intstead read the contents directly.

"intstead" - should be "instead", or perhaps "int32_tstead".



>
> 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>
> Signed-off-by: Pete Wyckoff<pw@padd.com>
> ---
>   contrib/fast-import/git-p4 |   11 +++++
>   t/t9802-git-p4-filetype.sh |  107 ++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 118 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..f112eaa
> --- /dev/null
> +++ b/t/t9802-git-p4-filetype.sh
> @@ -0,0 +1,107 @@
> +#!/bin/sh
> +
> +test_description='git-p4 p4 filetype tests'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> +	kill_p4d || :&&
> +	start_p4d&&
> +	cd "$TRASH_DIRECTORY"
> +'
> +
> +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&&
> +	cd "$TRASH_DIRECTORY"
> +'
> +
> +test_expect_success 'utf-16 file test' '
> +	test_when_finished cleanup_git&&
> +	"$GITP4" clone --dest="$git" //depot@all&&
> +	cd "$git"&&
> +
> +	cmp "$cli/f-ascii" f-ascii&&
> +	cmp "$cli/f-ascii-as-utf16" f-ascii-as-utf16&&
> +	cmp "$cli/f-utf16" f-utf16&&
> +	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&&
> +	cd "$TRASH_DIRECTORY"
> +'
> +
> +ko_smush() {
> +	cat>smush.py<<-EOF&&
> +	import re, sys
> +	sys.stdout.write(re.sub(r'(?i)\\\$(Id|Header):[^$]*\\\$', r'$\1$', sys.stdin.read()))
> +	EOF
> +	python smush.py<  "$1"
> +}
> +
> +k_smush() {
> +	cat>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
> +	python smush.py<  "$1"
> +}
> +
> +test_expect_success 'keyword file test' '
> +	test_when_finished cleanup_git&&
> +	"$GITP4" clone --dest="$git" //depot@all&&
> +	cd "$git"&&
> +
> +	# text, ensure unexpanded
> +	k_smush "$cli/k-text-k">  cli-k-text-k-smush&&
> +	cmp cli-k-text-k-smush k-text-k&&
> +	ko_smush "$cli/k-text-ko">  cli-k-text-ko-smush&&
> +	cmp cli-k-text-ko-smush k-text-ko&&
> +
> +	# utf16, even though p4 expands keywords, git-p4 does not
> +	# try to undo that
> +	cmp "$cli/k-utf16-k" k-utf16-k&&
> +	cmp "$cli/k-utf16-ko" k-utf16-ko
> +'
> +
> +test_expect_success 'kill p4d' '
> +	kill_p4d
> +'
> +
> +test_done

[PATCH 2/5] 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
intstead 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>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4 |   11 +++++
 t/t9802-git-p4-filetype.sh |  107 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 118 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..f112eaa
--- /dev/null
+++ b/t/t9802-git-p4-filetype.sh
@@ -0,0 +1,107 @@
+#!/bin/sh
+
+test_description='git-p4 p4 filetype tests'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	kill_p4d || : &&
+	start_p4d &&
+	cd "$TRASH_DIRECTORY"
+'
+
+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 &&
+	cd "$TRASH_DIRECTORY"
+'
+
+test_expect_success 'utf-16 file test' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot@all &&
+	cd "$git" &&
+
+	cmp "$cli/f-ascii" f-ascii &&
+	cmp "$cli/f-ascii-as-utf16" f-ascii-as-utf16 &&
+	cmp "$cli/f-utf16" f-utf16 &&
+	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 &&
+	cd "$TRASH_DIRECTORY"
+'
+
+ko_smush() {
+	cat >smush.py <<-EOF &&
+	import re, sys
+	sys.stdout.write(re.sub(r'(?i)\\\$(Id|Header):[^$]*\\\$', r'$\1$', sys.stdin.read()))
+	EOF
+	python smush.py < "$1"
+}
+
+k_smush() {
+	cat >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
+	python smush.py < "$1"
+}
+
+test_expect_success 'keyword file test' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot@all &&
+	cd "$git" &&
+
+	# text, ensure unexpanded
+	k_smush "$cli/k-text-k" > cli-k-text-k-smush &&
+	cmp cli-k-text-k-smush k-text-k &&
+	ko_smush "$cli/k-text-ko" > cli-k-text-ko-smush &&
+	cmp cli-k-text-ko-smush k-text-ko &&
+
+	# utf16, even though p4 expands keywords, git-p4 does not
+	# try to undo that
+	cmp "$cli/k-utf16-k" k-utf16-k &&
+	cmp "$cli/k-utf16-ko" k-utf16-ko
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.7.6.3

[PATCH 3/5] 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.

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..931cf5f 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.6.3

[PATCH 4/5] 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.

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 931cf5f..eee288d 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.6.3

[PATCH 5/5] 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 $.

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 eee288d..782b891 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.6.3

Re: [PATCH 0/5] git-p4 filetype handling

[Luke Diamand]

On 18/09/11 02:26, Pete Wyckoff wrote:
> Here's a series that includes some changes to make
> git-p4 handle p4 filetypes better.  This work was
> inspired by Chris Li, and includes some refactoring
> of the git-p4 tests that grew out of looking at
> Vitor's branch changes.
>
> It could use review for generic test beauty, as well
> as for the git-p4 filetype specifics in the code.

Modulo Junio's other comments, this looks sensible to me. I tried it on 
a sizeable tree and it didn't scramble anything (but that will only have 
tested the 'normal' filetypes).

Do we use $(some_command) rather than `some_command` now?

A few other small comments to follow.

Regards!
Luke


>
>   contrib/fast-import/git-p4 |   91 +++++++++++++-----
>   t/lib-git-p4.sh            |   55 +++++++++++
>   t/t9800-git-p4.sh          |  108 ++--------------------
>   t/t9801-git-p4-branch.sh   |  221 ++++++++++++++++++++++++++++++++++++++++++++
>   t/t9802-git-p4-filetype.sh |  107 +++++++++++++++++++++
>   5 files changed, 457 insertions(+), 125 deletions(-)
>
> Thanks,
>
> 		-- Pete
>