Re: [PATCH 2/3] git-p4: Search for parent commit on branch creation
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?
Show 32 quoted lines
>> 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 :)
Show 13 quoted lines
>> @@ -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
Vitor Antunes