From: Vitor Antunes Date: Fri, 18 Feb 2011 00:51:00 GMT Subject: Re: [PATCH 2/2] git-p4: Improve branch support Message-ID: 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 wrote: > 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. >> @@ -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... ;) >> @@ -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. >>          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