Re: [PATCH 2/2] git-p4: Improve branch support
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
Vitor Antunes