From: Vitor Antunes <vitor.hda@gmail.com>
Date: Mon, 16 Jan 2012 23:41:39 GMT
Subject: Re: [PATCH 2/3] git-p4: Search for parent commit on branch creation
Message-ID: <CAOpHH-UkyK-c_AHUOPbQQmW9cQQypDvirMR0Jb7vTGSQF7RZpw@mail.gmail.com>
In-Reply-To: <20120116185738.GA21996@padd.com>

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