# [PATCH 0/2] git-p4: Improvements to rename and copy detection 6 messages from 2011-02-15 to 2011-02-18. Participants: Vitor Antunes, Pete Wyckoff. Thread: https://gitlist.dev/t/26514 ## Vitor Antunes, 2011-02-15 23:49 Subject: [PATCH 0/2] git-p4: Improvements to rename and copy detection Message-ID: <1297813789-3831-1-git-send-email-vitor.hda@gmail.com> URL: https://gitlist.dev/e/1297813789-3831-1-git-send-email-vitor.hda%40gmail.com ``` The new version of these patches should, hopefully, contain all the corrections that were discussing previously. Please check them and confirm everything. In the meanwhile, I would kindly ask you to review the third patch I sent with the subject "git-p4: Improve branch support". Regarding the specific point of branches, I've had a discussion about branch origin detection in the thread "git-p4: Corrected typo" to which I would like to have your opinion. Thanks in advance for your time. Vitor Antunes (2): git-p4: Add copy detection support git-p4: Improve branch support contrib/fast-import/git-p4 | 43 +++++++++++++++++++++++++++++++++++++++---- 1 files changed, 39 insertions(+), 4 deletions(-) -- 1.7.2.3 ``` ## Vitor Antunes, 2011-02-15 23:49 Subject: [PATCH 1/2] git-p4: Add copy detection support Message-ID: <1297813789-3831-2-git-send-email-vitor.hda@gmail.com> URL: https://gitlist.dev/e/1297813789-3831-2-git-send-email-vitor.hda%40gmail.com In-Reply-To: <1297813789-3831-1-git-send-email-vitor.hda@gmail.com> ``` Add new config options: git-p4.detectCopies - Enable copy detection. git-p4.detectCopiesHarder - Find copies harder. The detectCopies option should be set to a true/false value. The detectCopiesHarder option should be set to true/false value. P4Submit can now process diff-tree C status and integrate files accordingly. --- contrib/fast-import/git-p4 | 16 ++++++++++++++++ 1 files changed, 16 insertions(+), 0 deletions(-) diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4 index b0da28a..1b1fc76 100755 --- a/contrib/fast-import/git-p4 +++ b/contrib/fast-import/git-p4 @@ -623,6 +623,12 @@ class P4Submit(Command): else: diffOpts = "" + if gitConfig("git-p4.detectCopies").lower() == "true": + diffOpts += " -C" + + if gitConfig("git-p4.detectCopiesHarder").lower() == "true": + diffOpts += " --find-copies-harder" + diff = read_pipe_lines("git diff-tree -r %s \"%s^\" \"%s\"" % (diffOpts, id, id)) filesToAdd = set() filesToDelete = set() @@ -646,6 +652,16 @@ class P4Submit(Command): filesToDelete.add(path) if path in filesToAdd: filesToAdd.remove(path) + elif modifier == "C": + src, dest = diff['src'], diff['dst'] + p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest)) + if diff['src_sha1'] != diff['dst_sha1']: + p4_system("edit \"%s\"" % (dest)) + if isModeExecChanged(diff['src_mode'], diff['dst_mode']): + p4_system("edit \"%s\"" % (dest)) + filesToChangeExecBit[dest] = diff['dst_mode'] + os.unlink(dest) + editedFiles.add(dest) elif modifier == "R": src, dest = diff['src'], diff['dst'] p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest)) -- 1.7.2.3 ``` ## Vitor Antunes, 2011-02-15 23:49 Subject: [PATCH 2/2] git-p4: Improve branch support Message-ID: <1297813789-3831-3-git-send-email-vitor.hda@gmail.com> URL: https://gitlist.dev/e/1297813789-3831-3-git-send-email-vitor.hda%40gmail.com In-Reply-To: <1297813789-3831-1-git-send-email-vitor.hda@gmail.com> ``` 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. Signed-off-by: Vitor Antunes --- contrib/fast-import/git-p4 | 27 +++++++++++++++++++++++---- 1 files changed, 23 insertions(+), 4 deletions(-) diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4 index 1b1fc76..93a0b6e 100755 --- a/contrib/fast-import/git-p4 +++ b/contrib/fast-import/git-p4 @@ -338,6 +338,11 @@ def gitConfig(key): _gitConfig[key] = read_pipe("git config %s" % key, ignore_error=True).strip() return _gitConfig[key] +def gitConfigList(key): + if not _gitConfig.has_key(key): + _gitConfig[key] = read_pipe("git config --get-all %s" % key, ignore_error=True).strip().split(os.linesep) + return _gitConfig[key] + def p4BranchesInGit(branchesAreInRemotes = True): 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): details = p4Cmd("branch -o %s" % info["branch"]) viewIdx = 0 while details.has_key("View%s" % viewIdx): @@ -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 + def getBranchMappingFromGitBranches(self): 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])) self.previousDepotPaths = paths -- 1.7.2.3 ``` ## Pete Wyckoff, 2011-02-17 18:16 Subject: Re: [PATCH 1/2] git-p4: Add copy detection support Message-ID: <20110217181657.GA20627@honk.padd.com> URL: https://gitlist.dev/e/20110217181657.GA20627%40honk.padd.com In-Reply-To: <1297813789-3831-2-git-send-email-vitor.hda@gmail.com> ``` vitor.hda@gmail.com wrote on Tue, 15 Feb 2011 23:49 +0000: > Add new config options: > git-p4.detectCopies - Enable copy detection. > git-p4.detectCopiesHarder - Find copies harder. > The detectCopies option should be set to a true/false value. > The detectCopiesHarder option should be set to true/false value. > P4Submit can now process diff-tree C status and integrate files accordingly. Needs sign-off. Acked-by: Pete Wyckoff ``` ## Pete Wyckoff, 2011-02-17 18:42 Subject: Re: [PATCH 2/2] git-p4: Improve branch support Message-ID: <20110217184234.GB20627@honk.padd.com> URL: https://gitlist.dev/e/20110217184234.GB20627%40honk.padd.com In-Reply-To: <1297813789-3831-3-git-send-email-vitor.hda@gmail.com> ``` 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. 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. > @@ -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. > @@ -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? > 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 ? 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. -- Pete ``` ## Vitor Antunes, 2011-02-18 00:51 Subject: Re: [PATCH 2/2] git-p4: Improve branch support Message-ID: URL: https://gitlist.dev/e/AANLkTinXHOUNhi163WFGYLgQx4QhO%3DduOGExhtYr-chK%40mail.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 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 ```