[PATCH/RFC 0/2] Search for parent commit in new branch import

[PATCH/RFC 0/2] Search for parent commit in new branch import

[Vitor Antunes]

This is mostly a RFC/RFH (Request For Help;) regarding the code I used on this
patch. I think that this is not a clean solution, but it shows the algorithm I
want to implement. The worst points are:

1. Relies on a sleep() to avoid a race condition. I'm using 0.1s, but it still
   fails sometimes, so it needs to be bigger.
2. Requires the --force option of fast-import.

Test case is included.

Thanks in advance to everyone who contributes.

Vitor Antunes (2):
  git-p4: Search for parent commit on branch creation
  git-p4: Add complex test case for branch import

 contrib/fast-import/git-p4 |   22 +++++++++++-
 t/t9800-git-p4.sh          |   77 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+), 2 deletions(-)

-- 
1.7.5.4

[PATCH/RFC 1/2] git-p4: Search for parent commit on branch creation

[Vitor Antunes]

The new commit is applied sequentially to each blob of the parent branch. The
first blob which results in a zero diff with the new commit is considered the
parent commit. If none is found, then the commit is applied to the top of the
parent branch.
This algorithm requires a checkpoint and a time.sleep() to be able to use
diff-tree, making it slower than what would be desirable. It also requires using
--force argument of fast-import, leaving lots of trash commits when finished.

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 contrib/fast-import/git-p4 |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index f622a38..0bced6d 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1671,7 +1671,25 @@ class P4Sync(Command, P4UserMap):
                             parent = self.initialParents[branch]
                             del self.initialParents[branch]
 
-                        self.commit(description, filesForCommit, branch, [branchPrefix], parent)
+                        parentFound = 0
+                        if len(parent) > 0:
+                            self.gitStream.write("checkpoint\n\n")
+                            time.sleep(0.1)
+                            for blob in read_pipe_lines("git rev-list --reverse --no-merges %s" % parent):
+                                blob = blob.strip()
+                                self.gitStream.write("reset %s\nfrom %s\n" % (branch, blob))
+                                self.commit(description, filesForCommit, branch, [branchPrefix], blob)
+                                self.gitStream.write("checkpoint\n\n")
+                                time.sleep(0.1)
+                                if len( read_pipe("git diff-tree %s %s" % (blob, branch)) ) == 0:
+                                    parentFound = 1
+                                    if self.verbose:
+                                        print "Found parent of %s in commit %s" % (branch, blob)
+                                    break
+                        if not parentFound:
+                            if self.verbose:
+                                print "Parent of %s not found. Committing into head of %s" % (branch, parent)
+                            self.commit(description, filesForCommit, branch, [branchPrefix], parent)
                 else:
                     files = self.extractFilesFromCommit(description)
                     self.commit(description, files, self.branch, self.depotPaths,
@@ -1948,7 +1966,7 @@ class P4Sync(Command, P4UserMap):
 
         self.tz = "%+03d%02d" % (- time.timezone / 3600, ((- time.timezone % 3600) / 60))
 
-        importProcess = subprocess.Popen(["git", "fast-import"],
+        importProcess = subprocess.Popen(["git", "fast-import", "--force"],
                                          stdin=subprocess.PIPE, stdout=subprocess.PIPE,
                                          stderr=subprocess.PIPE);
         self.gitOutput = importProcess.stdout
-- 
1.7.5.4

[PATCH/RFC 2/2] git-p4: Add complex test case for branch import

[Vitor Antunes]

Check if branches created from old changelists are correctly imported.

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 t/t9800-git-p4.sh |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 77 insertions(+), 0 deletions(-)

diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index 15704d2..9f757d7 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -454,6 +454,83 @@ test_expect_success 'git-p4 clone simple branches' '
 	rm -rf "$git" && mkdir "$git"
 '
 
+# Create a complex branch structure in P4 depot to check if they are correctly
+# cloned. The branches are created from older changelists to check if git-p4 is
+# able to correctly detect them.
+# The final expected structure is:
+# `branch1
+# | `- file1
+# | `- file2 (updated)
+# | `- file3
+# `branch2
+# | `- file1
+# | `- file2
+# `branch3
+# | `- file1
+# | `- file2 (updated)
+# | `- file3
+# `branch4
+# | `- file1
+# | `- file2
+# `branch5
+#   `- file1
+#   `- file2
+#   `- file3
+test_expect_success 'add p4 complex branches' '
+	cd "$cli" &&
+	changelist=$(p4 changes -m1 //depot/... | cut -d \  -f 2) &&
+	changelist=$(($changelist - 5)) &&
+	p4 integrate //depot/branch1/...@$changelist //depot/branch4/... &&
+	p4 submit -d "branch4" &&
+	changelist=$(($changelist + 2)) &&
+	p4 integrate //depot/branch1/...@$changelist //depot/branch5/... &&
+	p4 submit -d "branch5" &&
+	cd "$TRASH_DIRECTORY"
+'
+
+# Configure branches through git-config and clone them. git-p4 will only be able
+# to clone the original structure if it is able to detect the origin changelist
+# of each branch.
+test_expect_success 'git-p4 clone complex branches' '
+	git init "$git" &&
+	cd "$git" &&
+	git config git-p4.branchList branch1:branch2 &&
+	git config --add git-p4.branchList branch1:branch3 &&
+	git config --add git-p4.branchList branch1:branch4 &&
+	git config --add git-p4.branchList branch1:branch5 &&
+	cd "$TRASH_DIRECTORY" &&
+	"$GITP4" clone --dest="$git" --detect-branches //depot@all &&
+	cd "$git" &&
+	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 &&
+	git reset --hard p4/depot/branch4 &&
+	test -f file1 &&
+	test -f file2 &&
+	test \! -f file3 &&
+	! grep -q update file2 &&
+	git reset --hard p4/depot/branch5 &&
+	test -f file1 &&
+	test -f file2 &&
+	test -f file3 &&
+	! grep -q update file2 &&
+	cd "$TRASH_DIRECTORY" &&
+	rm -rf "$git" && mkdir "$git"
+'
+
 test_expect_success 'shutdown' '
 	pid=`pgrep -f p4d` &&
 	test -n "$pid" &&
-- 
1.7.5.4

Re: [PATCH/RFC 2/2] git-p4: Add complex test case for branch import

[Pete Wyckoff]

vitor.hda@gmail.com wrote on Fri, 19 Aug 2011 01:09 +0100:
> Check if branches created from old changelists are correctly imported.
> 
> Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>

Style edits.

		-- Pete



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

>From 5d1103a97b60570423782fd3873a2b413ddb1d5b Mon Sep 17 00:00:00 2001
From: Pete Wyckoff <pw@padd.com>
Date: Sat, 20 Aug 2011 14:31:49 -0400
Subject: [PATCH 3/4] git-p4: complex branch test edits

A few simplifications and style edits here too.

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

diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index 5bcb8b4..cf3a70e 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -489,11 +489,11 @@ test_expect_success 'git-p4 clone simple branches' '
 #   `- file3
 test_expect_success 'add p4 complex branches' '
 	cd "$cli" &&
-	changelist=$(p4 changes -m1 //depot/... | cut -d \  -f 2) &&
-	changelist=$(($changelist - 5)) &&
+	changelist=$(p4 changes -m1 //depot/... | cut -d" " -f2) &&
+	changelist=$((changelist - 5)) &&
 	p4 integrate //depot/branch1/...@$changelist //depot/branch4/... &&
 	p4 submit -d "branch4" &&
-	changelist=$(($changelist + 2)) &&
+	changelist=$((changelist + 2)) &&
 	p4 integrate //depot/branch1/...@$changelist //depot/branch5/... &&
 	p4 submit -d "branch5" &&
 	cd "$TRASH_DIRECTORY"
@@ -503,15 +503,14 @@ test_expect_success 'add p4 complex branches' '
 # to clone the original structure if it is able to detect the origin changelist
 # of each branch.
 test_expect_success 'git-p4 clone complex branches' '
-	git init "$git" &&
+	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 &&
 	git config --add git-p4.branchList branch1:branch4 &&
 	git config --add git-p4.branchList branch1:branch5 &&
-	cd "$TRASH_DIRECTORY" &&
-	"$GITP4" clone --dest="$git" --detect-branches //depot@all &&
-	cd "$git" &&
+	"$GITP4" clone --dest=. --detect-branches //depot@all &&
 	git log --all --graph --decorate --stat &&
 	git reset --hard p4/depot/branch1 &&
 	test -f file1 &&
@@ -521,7 +520,7 @@ test_expect_success 'git-p4 clone complex branches' '
 	git reset --hard p4/depot/branch2 &&
 	test -f file1 &&
 	test -f file2 &&
-	test \! -f file3 &&
+	test ! -f file3 &&
 	! grep -q update file2 &&
 	git reset --hard p4/depot/branch3 &&
 	test -f file1 &&
@@ -531,15 +530,13 @@ test_expect_success 'git-p4 clone complex branches' '
 	git reset --hard p4/depot/branch4 &&
 	test -f file1 &&
 	test -f file2 &&
-	test \! -f file3 &&
+	test ! -f file3 &&
 	! grep -q update file2 &&
 	git reset --hard p4/depot/branch5 &&
 	test -f file1 &&
 	test -f file2 &&
 	test -f file3 &&
-	! grep -q update file2 &&
-	cd "$TRASH_DIRECTORY" &&
-	rm -rf "$git" && mkdir "$git"
+	! grep -q update file2
 '
 
 test_expect_success 'shutdown' '
-- 
1.7.5.4

Re: [PATCH/RFC 2/2] git-p4: Add complex test case for branch import

[Vitor Antunes]

On Sat, 20 Aug 2011 15:12:03 -0400
Pete Wyckoff <pw@padd.com> wrote:

> Style edits.
> 
> 		-- Pete

I still need some help on the code part of this RFC patch. I really do
not like the way it is implemented. Do you see other possible ways of
achieving the same algorithm?

Thanks,
-- 
Vitor Antunes

Re: [PATCH/RFC 2/2] git-p4: Add complex test case for branch import

[Pete Wyckoff]

vitor.hda@gmail.com wrote on Mon, 22 Aug 2011 01:13 +0100:
> I still need some help on the code part of this RFC patch. I really do
> not like the way it is implemented. Do you see other possible ways of
> achieving the same algorithm?

I think I understand the problem now, and have a small test case
to see what's going wrong.  Your solution is definitely
sub-optimal.  :)  Is there anything that can be done with
--import-marks and --export-marks?

Could you guess at the branch point using p4?  I.e. for each file
in the new branch, grab the change that caused its most recent
revision (the #xx part).  Pick the max of those changes.  In git,
find the commit whith "change = ..." that you just discovered.
That's the parent.

Even this won't be perfect, and I'm not even sure if it's less
icky than diff-tree.

		-- Pete

Re: [PATCH/RFC 2/2] git-p4: Add complex test case for branch import

[Vitor Antunes]

On Tue, Aug 23, 2011 at 3:27 AM, Pete Wyckoff <pw@padd.com> wrote:
> I think I understand the problem now, and have a small test case
> to see what's going wrong.  Your solution is definitely
> sub-optimal.  :)  Is there anything that can be done with
> --import-marks and --export-marks?

Don't know if you agree, but I think it would be better to keep git-p4
from storing extra files in the filesystem. And, as far as I understood,
these two options require extra files.
But I don't think marks would help us that much. We already have
something similar: the changelist number in the commit log. The big
problem is that P4 does not store a pointer to the previous changelist.

> Could you guess at the branch point using p4?  I.e. for each file
> in the new branch, grab the change that caused its most recent
> revision (the #xx part).  Pick the max of those changes.  In git,
> find the commit whith "change = ..." that you just discovered.
> That's the parent.

Yes, I think this would work. Nevertheless, I would prefer not to do so
much pooling on the P4 server because when we're in a remote location in
regards to the P4 proxy server this kind of command becomes rather slow.
And if the branch has many files this may become cumbersome.

> Even this won't be perfect, and I'm not even sure if it's less
> icky than diff-tree.

From what I can see, the two possible solutions would be:

a) Running a "filelog" on all files. This may become slower when the
   branch has lots of files. There's also the problem in needing to
   contact the P4 proxy server every time.

b) Using diff-tree. This will be slower if the original changelist is
   very far from the latest one, but has the advantage of being run
   locally. It has the disadvantage of not having an apparent
   compatibility with fast-import.

In my personal point of view, diff-tree solution still looks better, if
it weren't for the fast-import issue. Now if we could just figure out
how to overcome this issue...

-- 
Vitor Antunes

Re: [PATCH/RFC 2/2] git-p4: Add complex test case for branch import

[Pete Wyckoff]

vitor.hda@gmail.com wrote on Tue, 23 Aug 2011 23:02 +0100:
> On Tue, Aug 23, 2011 at 3:27 AM, Pete Wyckoff <pw@padd.com> wrote:
> > I think I understand the problem now, and have a small test case
> > to see what's going wrong.  Your solution is definitely
> > sub-optimal.  :)  Is there anything that can be done with
> > --import-marks and --export-marks?
> 
> Don't know if you agree, but I think it would be better to keep git-p4
> from storing extra files in the filesystem. And, as far as I understood,
> these two options require extra files.
> But I don't think marks would help us that much. We already have
> something similar: the changelist number in the commit log. The big
> problem is that P4 does not store a pointer to the previous changelist.

Agreed.

> > Could you guess at the branch point using p4?  I.e. for each file
> > in the new branch, grab the change that caused its most recent
> > revision (the #xx part).  Pick the max of those changes.  In git,
> > find the commit whith "change = ..." that you just discovered.
> > That's the parent.
> 
> Yes, I think this would work. Nevertheless, I would prefer not to do so
> much pooling on the P4 server because when we're in a remote location in
> regards to the P4 proxy server this kind of command becomes rather slow.
> And if the branch has many files this may become cumbersome.
> 
> > Even this won't be perfect, and I'm not even sure if it's less
> > icky than diff-tree.
> 
> From what I can see, the two possible solutions would be:
> 
> a) Running a "filelog" on all files. This may become slower when the
>    branch has lots of files. There's also the problem in needing to
>    contact the P4 proxy server every time.
> 
> b) Using diff-tree. This will be slower if the original changelist is
>    very far from the latest one, but has the advantage of being run
>    locally. It has the disadvantage of not having an apparent
>    compatibility with fast-import.
> 
> In my personal point of view, diff-tree solution still looks better, if
> it weren't for the fast-import issue. Now if we could just figure out
> how to overcome this issue...

Nice analysis.  P4 can indeed be the bottleneck, even on near
servers if there are lots of files.  Diff-tree seems like the
better choice to find the branch point.  I don't understand fully
why the way we are using fast-import causes these problems.  Will
look at that next.

		-- Pete

Re: [PATCH/RFC 2/2] git-p4: Add complex test case for branch import

[Vitor Antunes]

On Wed, Aug 24, 2011 at 12:10 AM, Pete Wyckoff <pw@padd.com> wrote:
> Nice analysis.  P4 can indeed be the bottleneck, even on near
> servers if there are lots of files.  Diff-tree seems like the
> better choice to find the branch point.  I don't understand fully
> why the way we are using fast-import causes these problems.  Will
> look at that next.

I came up with some ideas during sleep:

1. When doing a "checkpoint" it would make sense to have some kind of
   feedback loop to know when fast-import completes it. In fact, I've
   just looked at fast-import man page and the "progress" command seems
   to do exactly that! Myabe we should crease a specific function for
   "checkpoint" that would call also "progress" instead of calling
   os.sleep().

2. In order to avoid needing to use "--force" it would be nice to have
   some form of "drop" command in fast-import that would allows us to
   actively drop an older commit after calling "reset" (this could even
   be an option of "reset"). This way fast-import would not find
   dangling commits in the end of the import.  Maybe there's already
   some sort of command to achieve this in fast-import...? We could
   probably ask to one of its maintainers.

-- 
Vitor Antunes

Re: [PATCH/RFC 2/2] git-p4: Add complex test case for branch import

[Vitor Antunes]

On Wed, Aug 24, 2011 at 11:46 AM, Vitor Antunes <vitor.hda@gmail.com> wrote:
> 1. When doing a "checkpoint" it would make sense to have some kind of
>   feedback loop to know when fast-import completes it. In fact, I've
>   just looked at fast-import man page and the "progress" command seems
>   to do exactly that! Myabe we should crease a specific function for
>   "checkpoint" that would call also "progress" instead of calling
>   os.sleep().

    # Force a checkpoint in fast-import and wait for it to finish
    def checkpoint(self):
        self.gitStream.write("checkpoint\n\n")
        self.gitStream.write("progress checkpoint\n\n")
        out = self.gitOutput.readline()
        if self.verbose:
            print "checkpoint finished: " + out

This seems to work! :)

> 2. In order to avoid needing to use "--force" it would be nice to have
>   some form of "drop" command in fast-import that would allows us to
>   actively drop an older commit after calling "reset" (this could even
>   be an option of "reset"). This way fast-import would not find
>   dangling commits in the end of the import.  Maybe there's already
>   some sort of command to achieve this in fast-import...? We could
>   probably ask to one of its maintainers.

Just missing point 2 now.

-- 
Vitor Antunes

Re: [PATCH/RFC 2/2] git-p4: Add complex test case for branch import

[Pete Wyckoff]

vitor.hda@gmail.com wrote on Wed, 24 Aug 2011 22:23 +0100:
> On Wed, Aug 24, 2011 at 11:46 AM, Vitor Antunes <vitor.hda@gmail.com> wrote:
> > 1. When doing a "checkpoint" it would make sense to have some kind of
> >   feedback loop to know when fast-import completes it. In fact, I've
> >   just looked at fast-import man page and the "progress" command seems
> >   to do exactly that! Myabe we should crease a specific function for
> >   "checkpoint" that would call also "progress" instead of calling
> >   os.sleep().
> 
>     # Force a checkpoint in fast-import and wait for it to finish
>     def checkpoint(self):
>         self.gitStream.write("checkpoint\n\n")
>         self.gitStream.write("progress checkpoint\n\n")
>         out = self.gitOutput.readline()
>         if self.verbose:
>             print "checkpoint finished: " + out
> 
> This seems to work! :)

That's brilliant.  And not even hacky.  :)

> > 2. In order to avoid needing to use "--force" it would be nice to have
> >   some form of "drop" command in fast-import that would allows us to
> >   actively drop an older commit after calling "reset" (this could even
> >   be an option of "reset"). This way fast-import would not find
> >   dangling commits in the end of the import.  Maybe there's already
> >   some sort of command to achieve this in fast-import...? We could
> >   probably ask to one of its maintainers.
> 
> Just missing point 2 now.

How about this.  Create the commit onto a fake branch name,
something involving the changeid would be okay.  Remember this
so we can delete it later.  For a parent, you can use anything,
so the top of the parent branch would be okay.  Checkpoint.

Use rev-parse like you do.  But go from the most recent commit on
back, hoping that the branch was created recently.  I suspect
this is the case for those using "git-p4 sync" with
detect-branches to discover new ones periodically.

For each ref, diff-tree like you do.  When it is found, generate
the commit again, with its real branch name and parent now.

When it exits, remember to delete the temp branches.  Would be
nice to do this even on error exit, but that's harder.  Maybe
the fake branch name should be something like
"refs/git-p4-temp/change-30214" instead of refs/heads/... to
indicate they are special.  GC will commit the commits
eventually.


The other thought I had was to use "blob" with a mark name on
each file.  Use --export-marks=...  Checkpoint.  Read the blob
ids corresponding to files on the branch.  Then iterate back the
revs and do "git ls-tree -r ref^{tree}".  Compare its blobs to
the new ones until a matching commit is found.  Finally assemble
the blobs into a commit; no need to write them again.  This seems
harder.


Here's a commit that refactors the tests and adds a bunch more
branch tests.  They all pass with your checkpoint/progress fix,
and still using --force.

		-- Pete


---------------------8<---------------
From f1e11b78cbb74fd4347df5fff64a373c3e449f68 Mon Sep 17 00:00:00 2001
From: Pete Wyckoff <pw@padd.com>
Date: Mon, 22 Aug 2011 22:20:33 -0400
Subject: [PATCH] git-p4 tests: refactor and add branch tests

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

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

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/lib-git-p4.sh          |   55 ++++++++
 t/t9800-git-p4.sh        |  182 +-------------------------
 t/t9801-git-p4-branch.sh |  326 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 388 insertions(+), 175 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 cf3a70e..684aa53 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 &&
@@ -404,146 +374,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 branch3" &&
-	cd "$git" &&
-	git reset --hard p4/depot/branch1 &&
-	"$GITP4" rebase &&
-	grep -q file2_ file2
-'
-
-# Create a complex branch structure in P4 depot to check if they are correctly
-# cloned. The branches are created from older changelists to check if git-p4 is
-# able to correctly detect them.
-# The final expected structure is:
-# `branch1
-# | `- file1
-# | `- file2 (updated)
-# | `- file3
-# `branch2
-# | `- file1
-# | `- file2
-# `branch3
-# | `- file1
-# | `- file2 (updated)
-# | `- file3
-# `branch4
-# | `- file1
-# | `- file2
-# `branch5
-#   `- file1
-#   `- file2
-#   `- file3
-test_expect_success 'add p4 complex branches' '
-	cd "$cli" &&
-	changelist=$(p4 changes -m1 //depot/... | cut -d" " -f2) &&
-	changelist=$((changelist - 5)) &&
-	p4 integrate //depot/branch1/...@$changelist //depot/branch4/... &&
-	p4 submit -d "branch4" &&
-	changelist=$((changelist + 2)) &&
-	p4 integrate //depot/branch1/...@$changelist //depot/branch5/... &&
-	p4 submit -d "branch5" &&
-	cd "$TRASH_DIRECTORY"
-'
-
-# Configure branches through git-config and clone them. git-p4 will only be able
-# to clone the original structure if it is able to detect the origin changelist
-# of each branch.
-test_expect_success 'git-p4 clone complex 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 &&
-	git config --add git-p4.branchList branch1:branch4 &&
-	git config --add git-p4.branchList branch1:branch5 &&
-	"$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 &&
-	git reset --hard p4/depot/branch4 &&
-	test -f file1 &&
-	test -f file2 &&
-	test ! -f file3 &&
-	! grep -q update file2 &&
-	git reset --hard p4/depot/branch5 &&
-	test -f file1 &&
-	test -f file2 &&
-	test -f file3 &&
-	! grep -q update 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..c71d1a5
--- /dev/null
+++ b/t/t9801-git-p4-branch.sh
@@ -0,0 +1,326 @@
+#!/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 'branch from non-head should find parent' '
+	cd "$cli" &&
+	p4 integrate //depot/main/...@1 //depot/branch3/... &&
+	p4 submit -d "integrate main@1 to branch3" &&
+
+	test_when_finished cleanup_git &&
+	test_create_repo "$git" &&
+	cd "$git" &&
+
+	git config git-p4.branchList main:branch1 &&
+	git config git-p4.branchList main:branch3 &&
+	"$GITP4" clone --dest=. --detect-branches //depot@all &&
+
+	git log --oneline --graph --decorate --all &&
+
+	branch3_parent=$(git rev-parse p4/depot/branch3^) &&
+	change1=$(git rev-parse :/"change = 1") &&
+	echo test $branch3_parent = $change1 &&
+	test $branch3_parent = $change1
+'
+
+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
+'
+
+# Create a complex branch structure in P4 depot to check if they are correctly
+# cloned. The branches are created from older changelists to check if git-p4 is
+# able to correctly detect them.
+#
+# The final expected structure is:
+# `branch1
+# | `- file1
+# | `- file2 (updated)
+# | `- file3
+# `branch2
+# | `- file1
+# | `- file2
+# `branch3
+# | `- file1
+# | `- file2 (updated)
+# | `- file3
+# `branch4
+# | `- file1
+# | `- file2
+# `branch5
+#   `- file1
+#   `- file2
+#   `- file3
+#
+# Changes 1..6 from previous tests, plus two new ones:
+# 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/...
+# 6: //depot/branch3/file2 (edit, from previous test)
+# 7: integrate //depot/branch1/...@1 -> //depot/branch4/...
+# 8: integrate //depot/branch1/...@3 -> //depot/branch5/...
+#
+test_expect_success 'add p4 complex branches' '
+	cd "$cli" &&
+	p4 integrate //depot/branch1/...@1 //depot/branch4/... &&
+	p4 submit -d "branch4 from branch1@1" &&
+	p4 integrate //depot/branch1/...@3 //depot/branch5/... &&
+	p4 submit -d "branch5 from branch1@3" &&
+	cd "$TRASH_DIRECTORY"
+'
+
+# Configure branches through git-config and clone them.  git-p4 will only be
+# able to clone the original structure if it is able to detect the origin
+# changelist of each branch.
+test_expect_success 'git-p4 clone complex 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 &&
+	git config --add git-p4.branchList branch1:branch4 &&
+	git config --add git-p4.branchList branch1:branch5 &&
+	"$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 &&
+	git reset --hard p4/depot/branch4 &&
+	test -f file1 &&
+	test -f file2 &&
+	test ! -f file3 &&
+	test_must_fail grep -q update file2 &&
+	git reset --hard p4/depot/branch5 &&
+	test -f file1 &&
+	test -f file2 &&
+	test -f file3 &&
+	test_must_fail grep -q update file2
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.7.5.4