[PATCH 0/3] git-p4: Search for parent commit on branch creation

[PATCH 0/3] git-p4: Search for parent commit on branch creation

[Vitor Antunes]

This patch was originally sent as a RFC and was improved since then
according to feedback given by Pete Wyckoff. At the time it required
calling fast-import with the --force argument. To avoid this the new
version tracks the temporary branches created during post-processing and
removes them at the end of the import process.

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

 contrib/fast-import/git-p4 |   39 ++++++++++++++++++++-
 t/t9801-git-p4-branch.sh   |   83 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 121 insertions(+), 1 deletions(-)

-- 
1.7.8.3

[PATCH 1/3] git-p4: Add checkpoint() task

[Vitor Antunes]

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

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 3e1aa27..417d119 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1450,6 +1450,14 @@ class P4Sync(Command, P4UserMap):
                    .replace("%25", "%")
         return path
 
+    # 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
+
     def extractFilesFromCommit(self, commit):
         self.cloneExclude = [re.sub(r"\.\.\.$", "", path)
                              for path in self.cloneExclude]
-- 
1.7.8.3

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

[Vitor Antunes]

To find out which is its parent the commit of the new branch is applied
sequentially to each blob of the parent branch from the newest to the
oldest. The first blob which results in a zero diff is considered the
parent commit. If none is found, then the commit is applied to the top
of the parent branch.

A fast-import "checkpoint" call is required for each comparison because
diff-tree is only able to work with blobs on disk. But most of these
commits will not be part of the final imported tree, making fast-import
fail. To avoid this, the temporary branches are tracked and then removed
at the end of the import process.

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

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 417d119..e2f9165 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1429,6 +1429,8 @@ class P4Sync(Command, P4UserMap):
         self.cloneExclude = []
         self.useClientSpec = False
         self.clientSpecDirs = None
+        self.tempBranches = []
+        self.tempBranchLocation = "git-p4-tmp"
 
         if gitConfig("git-p4.syncFromOrigin") == "false":
             self.syncWithOrigin = False
@@ -2012,7 +2014,28 @@ 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.checkpoint()
+                            for blob in read_pipe_lines("git rev-list --reverse --no-merges %s" % parent):
+                                blob = blob.strip()
+                                tempBranch = self.tempBranchLocation + os.sep + "%d-%s" % (change, blob)
+                                if self.verbose:
+                                    print "Creating temporary branch: " + tempBranch
+                                self.commit(description, filesForCommit, tempBranch, [branchPrefix], blob)
+                                self.tempBranches.append(tempBranch)
+                                self.checkpoint()
+                                if len( read_pipe("git diff-tree %s %s" % (blob, tempBranch)) ) == 0:
+                                    parentFound = 1
+                                    if self.verbose:
+                                        print "Found parent of %s in commit %s" % (branch, blob)
+                                    break
+                        if parentFound:
+                            self.commit(description, filesForCommit, branch, [branchPrefix], blob)
+                        else:
+                            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,
@@ -2347,6 +2370,12 @@ class P4Sync(Command, P4UserMap):
         self.gitOutput.close()
         self.gitError.close()
 
+        # Cleanup temporary branches created during import
+        if self.tempBranches != []:
+            for branch in self.tempBranches:
+                os.remove(".git" + os.sep + branch)
+            os.rmdir(".git" + os.sep + self.tempBranchLocation)
+
         return True
 
 class P4Rebase(Command):
-- 
1.7.8.3

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

[Pete Wyckoff]

vitor.hda@gmail.com wrote on Mon, 16 Jan 2012 00:39 +0000:
> To find out which is its parent the commit of the new branch is applied
> sequentially to each blob of the parent branch from the newest to the
> oldest. The first blob which results in a zero diff is considered the
> parent commit. If none is found, then the commit is applied to the top
> of the parent branch.
> 
> A fast-import "checkpoint" call is required for each comparison because
> diff-tree is only able to work with blobs on disk. But most of these
> commits will not be part of the final imported tree, making fast-import
> fail. To avoid this, the temporary branches are tracked and then removed
> at the end of the import process.

This looks much better without the need for "--force".  It'll be
great to fix this major branch detection problem.  Can you make a
couple of further minor changes?

> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> @@ -2012,7 +2014,28 @@ class P4Sync(Command, P4UserMap):
> -                        self.commit(description, filesForCommit, branch, [branchPrefix], parent)
> +                        parentFound = 0
> +                        if len(parent) > 0:
> +                            self.checkpoint()
> +                            for blob in read_pipe_lines("git rev-list --reverse --no-merges %s" % parent):
> +                                blob = blob.strip()
> +                                tempBranch = self.tempBranchLocation + os.sep + "%d-%s" % (change, blob)
> +                                if self.verbose:
> +                                    print "Creating temporary branch: " + tempBranch
> +                                self.commit(description, filesForCommit, tempBranch, [branchPrefix], blob)
> +                                self.tempBranches.append(tempBranch)
> +                                self.checkpoint()
> +                                if len( read_pipe("git diff-tree %s %s" % (blob, tempBranch)) ) == 0:
> +                                    parentFound = 1
> +                                    if self.verbose:
> +                                        print "Found parent of %s in commit %s" % (branch, blob)
> +                                    break
> +                        if parentFound:
> +                            self.commit(description, filesForCommit, branch, [branchPrefix], blob)
> +                        else:
> +                            if self.verbose:
> +                                print "Parent of %s not found. Committing into head of %s" % (branch, parent)
> +                            self.commit(description, filesForCommit, branch, [branchPrefix], parent)

1.  Move the tempBranch commit outside of the "for blob" loop.
    It can have no parent, and the diff-tree will still tell you
    if you found the same contents.  Instead of a ref for
    each blob inspected for each change, you'll just have one ref
    per change.  Only one checkpoint() after the tempBranch
    commit should be needed.

2.  Nit.  parentFound is boolean, use True/False, not 1/0.

> @@ -2347,6 +2370,12 @@ class P4Sync(Command, P4UserMap):
> +        # Cleanup temporary branches created during import
> +        if self.tempBranches != []:
> +            for branch in self.tempBranches:
> +                os.remove(".git" + os.sep + branch)
> +            os.rmdir(".git" + os.sep + self.tempBranchLocation)
> +

3.  Deleting refs should probably use "git update-ref -d"
    just in case GIT_DIR is not ".git".  I think you could just
    leave the "git-p4-tmp" directory around, but use
    os.environ["GIT_DIR"] instead of ".git" if you want to
    delete it.

4.  Paths are best manipulated with os.path.join(dir, file), to handle
    weirdnesses like drive letters.

Eventually if the fast-import protocol learns to delete the refs
it creates, we can clean up a bit more nicely.  I think there was
agreement this was a good idea, just needs someone to do it
sometime.

		-- Pete

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

[Vitor Antunes]

On Mon, Jan 16, 2012 at 6:57 PM, Pete Wyckoff <pw@padd.com> wrote:
> This looks much better without the need for "--force".  It'll be
> great to fix this major branch detection problem.  Can you make a
> couple of further minor changes?

Of course I can :)

>> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
>> @@ -2012,7 +2014,28 @@ class P4Sync(Command, P4UserMap):
>> -                        self.commit(description, filesForCommit, branch, [branchPrefix], parent)
>> +                        parentFound = 0
>> +                        if len(parent) > 0:
>> +                            self.checkpoint()
>> +                            for blob in read_pipe_lines("git rev-list --reverse --no-merges %s" % parent):
>> +                                blob = blob.strip()
>> +                                tempBranch = self.tempBranchLocation + os.sep + "%d-%s" % (change, blob)
>> +                                if self.verbose:
>> +                                    print "Creating temporary branch: " + tempBranch
>> +                                self.commit(description, filesForCommit, tempBranch, [branchPrefix], blob)
>> +                                self.tempBranches.append(tempBranch)
>> +                                self.checkpoint()
>> +                                if len( read_pipe("git diff-tree %s %s" % (blob, tempBranch)) ) == 0:
>> +                                    parentFound = 1
>> +                                    if self.verbose:
>> +                                        print "Found parent of %s in commit %s" % (branch, blob)
>> +                                    break
>> +                        if parentFound:
>> +                            self.commit(description, filesForCommit, branch, [branchPrefix], blob)
>> +                        else:
>> +                            if self.verbose:
>> +                                print "Parent of %s not found. Committing into head of %s" % (branch, parent)
>> +                            self.commit(description, filesForCommit, branch, [branchPrefix], parent)
>
> 1.  Move the tempBranch commit outside of the "for blob" loop.
>    It can have no parent, and the diff-tree will still tell you
>    if you found the same contents.  Instead of a ref for
>    each blob inspected for each change, you'll just have one ref
>    per change.  Only one checkpoint() after the tempBranch
>    commit should be needed.

You're right. Completely oversaw that. Will improve the code
accordingly.

> 2.  Nit.  parentFound is boolean, use True/False, not 1/0.

That was not a nice thing to do... thanks for noticing :)

>> @@ -2347,6 +2370,12 @@ class P4Sync(Command, P4UserMap):
>> +        # Cleanup temporary branches created during import
>> +        if self.tempBranches != []:
>> +            for branch in self.tempBranches:
>> +                os.remove(".git" + os.sep + branch)
>> +            os.rmdir(".git" + os.sep + self.tempBranchLocation)
>> +
>
> 3.  Deleting refs should probably use "git update-ref -d"
>    just in case GIT_DIR is not ".git".  I think you could just
>    leave the "git-p4-tmp" directory around, but use
>    os.environ["GIT_DIR"] instead of ".git" if you want to
>    delete it.

Will use os.environ.get, which can be configured to return ".git" if
$GIT_DIR is not defined. Is this ok?

> 4.  Paths are best manipulated with os.path.join(dir, file), to handle
>    weirdnesses like drive letters.

Perfect. I was completely unaware of that method. Thanks for the tip.

> Eventually if the fast-import protocol learns to delete the refs
> it creates, we can clean up a bit more nicely.  I think there was
> agreement this was a good idea, just needs someone to do it
> sometime.

One can always hope ;)

Thanks,
Vitor

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

[Vitor Antunes]

On Mon, Jan 16, 2012 at 11:41 PM, Vitor Antunes <vitor.hda@gmail.com> wrote:
> On Mon, Jan 16, 2012 at 6:57 PM, Pete Wyckoff <pw@padd.com> wrote:
>> 1.  Move the tempBranch commit outside of the "for blob" loop.
>>    It can have no parent, and the diff-tree will still tell you
>>    if you found the same contents.  Instead of a ref for
>>    each blob inspected for each change, you'll just have one ref
>>    per change.  Only one checkpoint() after the tempBranch
>>    commit should be needed.
>
> You're right. Completely oversaw that. Will improve the code
> accordingly.

Apparently I did not oversee it. Assume you have added a new file to
HEAD of parent branch, but you branched from a previous commit. When the
new branch is committed over HEAD the new file will, incorrectly, be
part of it and diff-tree will not work as expected.

I should avoid taking 6 months to submit a patch to avoid forgetting why
I did what I did :)

Vitor

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

[Pete Wyckoff]

vitor.hda@gmail.com wrote on Tue, 17 Jan 2012 00:10 +0000:
> On Mon, Jan 16, 2012 at 11:41 PM, Vitor Antunes <vitor.hda@gmail.com> wrote:
> > On Mon, Jan 16, 2012 at 6:57 PM, Pete Wyckoff <pw@padd.com> wrote:
> >> 1.  Move the tempBranch commit outside of the "for blob" loop.
> >>    It can have no parent, and the diff-tree will still tell you
> >>    if you found the same contents.  Instead of a ref for
> >>    each blob inspected for each change, you'll just have one ref
> >>    per change.  Only one checkpoint() after the tempBranch
> >>    commit should be needed.
> >
> > You're right. Completely oversaw that. Will improve the code
> > accordingly.
> 
> Apparently I did not oversee it. Assume you have added a new file to
> HEAD of parent branch, but you branched from a previous commit. When the
> new branch is committed over HEAD the new file will, incorrectly, be
> part of it and diff-tree will not work as expected.

I don't get it.  This algorithm works on the fact that a "branch"
in p4 creates a new change that looks exactly like a previous
change.

The git-p4 sync step, when it detects a branch, starts by saving
the change in a commit with parent = null, so it is its own new
branch, an orphan, with no parents.

Now the task is to find some commit that has an identical tree to
this temporary one.  You walk back all known p4 commits to try to
find one that is the same.  It doesn't matter if any of those p4
commits have other commits on top of them.

At each step in the backward walk, the comparison is against the
unchanged orphan commit.

An ascii-art picture might help me.  Or even a test case.

> I should avoid taking 6 months to submit a patch to avoid forgetting why
> I did what I did :)

Yeah, and now you have to explain it all over to me again too.  :)

		-- Pete

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

[Vitor Antunes]

On Tue, Jan 17, 2012 at 10:18 PM, Pete Wyckoff <pw@padd.com> wrote:
> vitor.hda@gmail.com wrote on Tue, 17 Jan 2012 00:10 +0000:
>> On Mon, Jan 16, 2012 at 11:41 PM, Vitor Antunes <vitor.hda@gmail.com> wrote:
>> > On Mon, Jan 16, 2012 at 6:57 PM, Pete Wyckoff <pw@padd.com> wrote:
>> >> 1.  Move the tempBranch commit outside of the "for blob" loop.
>> >>    It can have no parent, and the diff-tree will still tell you
>> >>    if you found the same contents.  Instead of a ref for
>> >>    each blob inspected for each change, you'll just have one ref
>> >>    per change.  Only one checkpoint() after the tempBranch
>> >>    commit should be needed.
>> >
>> > You're right. Completely oversaw that. Will improve the code
>> > accordingly.
>>
>> Apparently I did not oversee it. Assume you have added a new file to
>> HEAD of parent branch, but you branched from a previous commit. When the
>> new branch is committed over HEAD the new file will, incorrectly, be
>> part of it and diff-tree will not work as expected.
>
> I don't get it.  This algorithm works on the fact that a "branch"
> in p4 creates a new change that looks exactly like a previous
> change.
>
> The git-p4 sync step, when it detects a branch, starts by saving
> the change in a commit with parent = null, so it is its own new
> branch, an orphan, with no parents.
>
> Now the task is to find some commit that has an identical tree to
> this temporary one.  You walk back all known p4 commits to try to
> find one that is the same.  It doesn't matter if any of those p4
> commits have other commits on top of them.
>
> At each step in the backward walk, the comparison is against the
> unchanged orphan commit.
>
> An ascii-art picture might help me.  Or even a test case.

I can see now that I did not understand the "no parent" in your original
reply. What you just explained makes sense. So, let me try that and I'll
update you tomorrow.

Vitor

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

[Vitor Antunes]

Check if branches created from old changelists are correctly imported.

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

diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index a25f18d..0f10e00 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -226,6 +226,89 @@ test_expect_success 'git-p4 clone simple branches' '
 	)
 '
 
+# 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 'git-p4 add complex branches' '
+	test_when_finished cleanup_git &&
+	test_create_repo "$git" &&
+	(
+		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" &&
+	(
+		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 ! -d .git/git-p4-tmp
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
1.7.8.3

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

[Pete Wyckoff]

vitor.hda@gmail.com wrote on Mon, 16 Jan 2012 00:39 +0000:
> diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
> +test_expect_success 'git-p4 add complex branches' '
> +	test_when_finished cleanup_git &&
> +	test_create_repo "$git" &&
> +	(
> +		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"
> +	)
> +'

Sorry: I think I wanted the "$"s removed from inside $((..)).
Turns out that some shells don't grok that.  The above should be:

	changelist=$(($changelist - 5)) &&

You can drop the last cd to $TRASH_DIRECTORY since you're inside
a subshell.  (Nice addition of the subshells.)

> +
> +# 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" &&
> +	(
> +		test_when_finished cleanup_git &&
> +		test_create_repo "$git" &&

These two lines can go; you already did it outside the subshell.

> +		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 &&

There are preferred functions for these tests, I learned recently:

	test_path_is_file file1 &&

> +		grep -q update file2 &&
> +		git reset --hard p4/depot/branch2 &&
> +		test -f file1 &&
> +		test -f file2 &&
> +		test ! -f file3 &&

Similarly

	test_path_is_missing 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 ! -d .git/git-p4-tmp
> +	)
> +'