Re: [PATCH 2/2] git-p4: Improve branch support

From: Vitor Antunes <vitor.hda@gmail.com>
Date: Feb 18, 2011, 00:51 UTC
Message-ID: <AANLkTinXHOUNhi163WFGYLgQx4QhO=duOGExhtYr-chK@mail.gmail.com>
In-Reply-To: <20110217184234.GB20627@honk.padd.com>

Hi Pete,

First thing I just noticed is that I wanted to send the "Improve rename detection" patch and not this one. And now, after I sent it I understood that I still don't know how to use the format-patch correctly because I sent it as a 1/3 set of patches... :/ Sorry to all the mailing list about this.

On Thu, Feb 17, 2011 at 6:42 PM, Pete Wyckoff <pw@padd.com> wrote:

Show 14 quoted lines

> vitor.hda@gmail.com wrote on Tue, 15 Feb 2011 23:49 +0000:
>> Add new config option branchUser to allow filtering P4 branch list by user.
>> Allow defining the branch list through branchList config option.
>> Correct base branch directory detection to use '/' as the split character.
>
> I've only been avoiding reading this one because I don't use
> branches, and am having a hard time following what the point
> of the change is.  Maybe someone else will notice that this is
> a good change straightaway, and save you the bother of explaining
> it to me.
>
> The whole detectBranches option seems a bit spotty, given its
> lack of documentation and numerous shady "## HACK" and "## FIXME"
> comments.  So it certainly does appreciate your attention.

I also noticed those comments in the code, but until now I did not find any wrong behavior on how the script works with branches. But then again, in our depot we don't do many complex operations. I still want to add an improvement on how it detects the branch's origin commit. If while doing that I see something else I can improve I will do so :)

> As far as I can tell, a branch is just a mapping between two
> areas of the repo, and all you can do with one is to feed it
> to "p4 integrate" instead of giving a single explicit src/dest.

In P4 branches work like in Subversion - they are simple links to the origin data, which are created with the integrate command. But we can also define a branch specification. That way we can simply refer to that branch for the integrate to know where to update the data from/to. git-p4 was using the branch specs to search for branches.

Show 15 quoted lines

>> @@ -1272,7 +1277,13 @@ class P4Sync(Command):
>>      def getBranchMapping(self):
>>          lostAndFoundBranches = set()
>>
>> -        for info in p4CmdList("branches"):
>> +        user = gitConfig("git-p4.branchUser")
>> +        if len(user) > 0:
>> +            command = "branches -u %s" % user
>> +        else:
>> +            command = "branches"
>> +
>> +        for info in p4CmdList(command):
>
> This part is for branches -u, to limit the auto-detected braches
> to just those created by a given user.

Yes, as simple as that! When you have a worldwide server an ocean away with thousands of branch specifications, you really don't want to go through all of them... ;)

Show 15 quoted lines

>> @@ -1305,6 +1316,12 @@ class P4Sync(Command):
>>          for branch in lostAndFoundBranches:
>>              self.knownBranches[branch] = branch
>>
>> +        configBranches = gitConfigList("git-p4.branchList")
>> +        for branch in configBranches:
>> +            if branch:
>> +                (source, destination) = branch.split(":")
>> +                self.knownBranches[destination] = source
>> +
>
> This bit adds more branches, if you have put them in .git/config
> separated by a :.  Presumably things that are directories in the
> depot but do _not_ have a branch?  I don't get it.  What does
> your depot look like and what do you stick in branchList?

Assume

//depot/project

as the main branch. And

//depot/project_featureA //depot/project_featureB

as feature development branches. So, you could have a list like: project:project_featureA project:project_featureB

Of course, if project_featureB originated from the first dev branch, you would need to define it like:

project_featureA:project_featureB

This was to avoid using P4 branch specifications. In my team we don't use them that much and since I was only creating branch specs to use git-p4, I kinda of thought it would be better do keep that configuration within git.

Show 23 quoted lines

>>          branches = p4BranchesInGit(self.importIntoRemotes)
>>          for branch in branches.keys():
>> @@ -1626,12 +1643,14 @@ class P4Sync(Command):
>>                      else:
>>                          paths = []
>>                          for (prev, cur) in zip(self.previousDepotPaths, depotPaths):
>> -                            for i in range(0, min(len(cur), len(prev))):
>> -                                if cur[i] <> prev[i]:
>> +                            prev_list = prev.split("/")
>> +                            cur_list = cur.split("/")
>> +                            for i in range(0, min(len(cur_list), len(prev_list))):
>> +                                if cur_list[i] <> prev_list[i]:
>>                                      i = i - 1
>>                                      break
>>
>> -                            paths.append (cur[:i + 1])
>> +                            paths.append ("/".join(cur_list[:i + 1]))
>
> This was a bug?  I guess somehow this is adapting the existing
> depot paths to include new ones that you discovered with "p4
> branches".  And now you are comparing the path elements instead
> of going a character at a time.  Maybe to catch //depot/ab vs
> //depot/ac ?

Exactly. The example I used above was created to also show that.

> In other words, add some comments or add something to the commit
> text.  It is encouraged to split up a patch into the smallest
> logical chenk if the changes are all independent.  Maybe these
> three are related through the theme of auto-branch detection.

Understood. I think this patch can easily be split into 3 different logical chunks. I'll add some comments and probably see if I can include some descriptions/examples in git-p4.txt. I just have to find some free time first :)

Thank you very much for your feedback and help.

-- 
Vitor Antunes