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

From: Vitor Antunes <vitor.hda@gmail.com>
Date: Jan 23, 2012, 13:49 UTC
Message-ID: <CAOpHH-Xy2DnoyL_sYjz8umbOg3Hd7iirUzUnfLmYdcg7Oiw6fQ@mail.gmail.com>
In-Reply-To: <7v7h0ld58z.fsf@alter.siamese.dyndns.org>

Hi Junio,

On Sat, Jan 21, 2012 at 4:55 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

> Vitor Antunes <vitor.hda@gmail.com> writes:
>
>> 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>
>> ---
>
> It might make sense to squash 1/3 with this patch; the definition alone
> without any actual user would not be very useful and won't help hunting
> for breakages, if exists, in the future.

Makes sense. WIll correct as you suggest in v3.

Show 23 quoted lines

>> @@ -2012,7 +2014,27 @@ class P4Sync(Command, P4UserMap):
>>                              parent = self.initialParents[branch]
>>                              del self.initialParents[branch]
>>
>> -                        self.commit(description, filesForCommit, branch, [branchPrefix], parent)
>> +                        parentFound = False
>> +                        if len(parent) > 0:
>> +                            tempBranch = os.path.join(self.tempBranchLocation, "%d" % (change))
>> +                            if self.verbose:
>> +                                print "Creating temporary branch: " + tempBranch
>> +                            self.commit(description, filesForCommit, tempBranch, [branchPrefix])
>> +                            self.tempBranches.append(tempBranch)
>> +                            self.checkpoint()
>> +                            for blob in read_pipe_lines("git rev-list --reverse --no-merges %s" % parent):
>> +                                blob = blob.strip()
>> +                                if len( read_pipe("git diff-tree %s %s" % (blob, tempBranch)) ) == 0:
>> +                                    parentFound = True
>> +                                    if self.verbose:
>> +                                        print "Found parent of %s in commit %s" % (branch, blob)
>
> ... also this looks excessively deeply nested, which is a sign that it
> might be a good idea to refactor this part into a separate helper function
> or something.

I have to agree with you. Since v3 is required, I'll also include this fix along.

Thanks, Vitor