{"thread":{"id":"26514","subject":"[PATCH 0/2] git-p4: Improvements to rename and copy detection","startedAt":"2011-02-15T23:49:47Z","lastAt":"2011-02-18T00:51:00Z","messageCount":6,"participants":["Vitor Antunes","Pete Wyckoff"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"161238","messageId":"1297813789-3831-1-git-send-email-vitor.hda@gmail.com","threadId":"26514","inReplyTo":null,"subject":"[PATCH 0/2] git-p4: Improvements to rename and copy detection","fromName":"Vitor Antunes","fromEmail":"vitor.hda@gmail.com","sentAt":"2011-02-15T23:49:47Z","receivedAt":"2011-02-15T23:49:47Z","isPatch":true,"sender":{"key":"vitor.hda@gmail.com","avatar":"https://avatars.githubusercontent.com/u/606876?v=4"},"body":"The new version of these patches should, hopefully, contain all the corrections\nthat were discussing previously. Please check them and confirm everything.\n\nIn the meanwhile, I would kindly ask you to review the third patch I sent with\nthe subject \"git-p4: Improve branch support\". Regarding the specific point of\nbranches, I've had a discussion about branch origin detection in the thread\n\"git-p4: Corrected typo\" to which I would like to have your opinion.\n\nThanks in advance for your time.\n\nVitor Antunes (2):\n git-p4: Add copy detection support\n git-p4: Improve branch support\n\n contrib/fast-import/git-p4 | 43 +++++++++++++++++++++++++++++++++++++++----\n 1 files changed, 39 insertions(+), 4 deletions(-)\n\n-- \n1.7.2.3\n"},{"id":"161239","messageId":"1297813789-3831-2-git-send-email-vitor.hda@gmail.com","threadId":"26514","inReplyTo":"1297813789-3831-1-git-send-email-vitor.hda@gmail.com","subject":"[PATCH 1/2] git-p4: Add copy detection support","fromName":"Vitor Antunes","fromEmail":"vitor.hda@gmail.com","sentAt":"2011-02-15T23:49:48Z","receivedAt":"2011-02-15T23:49:48Z","isPatch":true,"sender":{"key":"vitor.hda@gmail.com","avatar":"https://avatars.githubusercontent.com/u/606876?v=4"},"body":"Add new config options:\n git-p4.detectCopies - Enable copy detection.\n git-p4.detectCopiesHarder - Find copies harder.\nThe detectCopies option should be set to a true/false value.\nThe detectCopiesHarder option should be set to true/false value.\nP4Submit can now process diff-tree C status and integrate files accordingly.\n---\n contrib/fast-import/git-p4 | 16 ++++++++++++++++\n 1 files changed, 16 insertions(+), 0 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex b0da28a..1b1fc76 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -623,6 +623,12 @@ class P4Submit(Command):\n else:\n diffOpts = \"\"\n \n+ if gitConfig(\"git-p4.detectCopies\").lower() == \"true\":\n+ diffOpts += \" -C\"\n+\n+ if gitConfig(\"git-p4.detectCopiesHarder\").lower() == \"true\":\n+ diffOpts += \" --find-copies-harder\"\n+\n diff = read_pipe_lines(\"git diff-tree -r %s \\\"%s^\\\" \\\"%s\\\"\" % (diffOpts, id, id))\n filesToAdd = set()\n filesToDelete = set()\n@@ -646,6 +652,16 @@ class P4Submit(Command):\n filesToDelete.add(path)\n if path in filesToAdd:\n filesToAdd.remove(path)\n+ elif modifier == \"C\":\n+ src, dest = diff['src'], diff['dst']\n+ p4_system(\"integrate -Dt \\\"%s\\\" \\\"%s\\\"\" % (src, dest))\n+ if diff['src_sha1'] != diff['dst_sha1']:\n+ p4_system(\"edit \\\"%s\\\"\" % (dest))\n+ if isModeExecChanged(diff['src_mode'], diff['dst_mode']):\n+ p4_system(\"edit \\\"%s\\\"\" % (dest))\n+ filesToChangeExecBit[dest] = diff['dst_mode']\n+ os.unlink(dest)\n+ editedFiles.add(dest)\n elif modifier == \"R\":\n src, dest = diff['src'], diff['dst']\n p4_system(\"integrate -Dt \\\"%s\\\" \\\"%s\\\"\" % (src, dest))\n-- \n1.7.2.3\n"},{"id":"161240","messageId":"1297813789-3831-3-git-send-email-vitor.hda@gmail.com","threadId":"26514","inReplyTo":"1297813789-3831-1-git-send-email-vitor.hda@gmail.com","subject":"[PATCH 2/2] git-p4: Improve branch support","fromName":"Vitor Antunes","fromEmail":"vitor.hda@gmail.com","sentAt":"2011-02-15T23:49:49Z","receivedAt":"2011-02-15T23:49:49Z","isPatch":true,"sender":{"key":"vitor.hda@gmail.com","avatar":"https://avatars.githubusercontent.com/u/606876?v=4"},"body":"Add new config option branchUser to allow filtering P4 branch list by user.\nAllow defining the branch list through branchList config option.\nCorrect base branch directory detection to use '/' as the split character.\n\nSigned-off-by: Vitor Antunes \n---\n contrib/fast-import/git-p4 | 27 +++++++++++++++++++++++----\n 1 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 1b1fc76..93a0b6e 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -338,6 +338,11 @@ def gitConfig(key):\n _gitConfig[key] = read_pipe(\"git config %s\" % key, ignore_error=True).strip()\n return _gitConfig[key]\n \n+def gitConfigList(key):\n+ if not _gitConfig.has_key(key):\n+ _gitConfig[key] = read_pipe(\"git config --get-all %s\" % key, ignore_error=True).strip().split(os.linesep)\n+ return _gitConfig[key]\n+\n def p4BranchesInGit(branchesAreInRemotes = True):\n branches = {}\n \n@@ -1272,7 +1277,13 @@ class P4Sync(Command):\n def getBranchMapping(self):\n lostAndFoundBranches = set()\n \n- for info in p4CmdList(\"branches\"):\n+ user = gitConfig(\"git-p4.branchUser\")\n+ if len(user) > 0:\n+ command = \"branches -u %s\" % user\n+ else:\n+ command = \"branches\"\n+\n+ for info in p4CmdList(command):\n details = p4Cmd(\"branch -o %s\" % info[\"branch\"])\n viewIdx = 0\n while details.has_key(\"View%s\" % viewIdx):\n@@ -1305,6 +1316,12 @@ class P4Sync(Command):\n for branch in lostAndFoundBranches:\n self.knownBranches[branch] = branch\n \n+ configBranches = gitConfigList(\"git-p4.branchList\")\n+ for branch in configBranches:\n+ if branch:\n+ (source, destination) = branch.split(\":\")\n+ self.knownBranches[destination] = source\n+\n def getBranchMappingFromGitBranches(self):\n branches = p4BranchesInGit(self.importIntoRemotes)\n for branch in branches.keys():\n@@ -1626,12 +1643,14 @@ class P4Sync(Command):\n else:\n paths = []\n for (prev, cur) in zip(self.previousDepotPaths, depotPaths):\n- for i in range(0, min(len(cur), len(prev))):\n- if cur[i] <> prev[i]:\n+ prev_list = prev.split(\"/\")\n+ cur_list = cur.split(\"/\")\n+ for i in range(0, min(len(cur_list), len(prev_list))):\n+ if cur_list[i] <> prev_list[i]:\n i = i - 1\n break\n \n- paths.append (cur[:i + 1])\n+ paths.append (\"/\".join(cur_list[:i + 1]))\n \n self.previousDepotPaths = paths\n \n-- \n1.7.2.3\n"},{"id":"161434","messageId":"20110217181657.GA20627@honk.padd.com","threadId":"26514","inReplyTo":"1297813789-3831-2-git-send-email-vitor.hda@gmail.com","subject":"Re: [PATCH 1/2] git-p4: Add copy detection support","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2011-02-17T18:16:57Z","receivedAt":"2011-02-17T18:16:57Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"vitor.hda@gmail.com wrote on Tue, 15 Feb 2011 23:49 +0000:\n> Add new config options:\n> git-p4.detectCopies - Enable copy detection.\n> git-p4.detectCopiesHarder - Find copies harder.\n> The detectCopies option should be set to a true/false value.\n> The detectCopiesHarder option should be set to true/false value.\n> P4Submit can now process diff-tree C status and integrate files accordingly.\n\nNeeds sign-off.\nAcked-by: Pete Wyckoff \n"},{"id":"161436","messageId":"20110217184234.GB20627@honk.padd.com","threadId":"26514","inReplyTo":"1297813789-3831-3-git-send-email-vitor.hda@gmail.com","subject":"Re: [PATCH 2/2] git-p4: Improve branch support","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2011-02-17T18:42:34Z","receivedAt":"2011-02-17T18:42:34Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"vitor.hda@gmail.com wrote on Tue, 15 Feb 2011 23:49 +0000:\n> Add new config option branchUser to allow filtering P4 branch list by user.\n> Allow defining the branch list through branchList config option.\n> Correct base branch directory detection to use '/' as the split character.\n\nI've only been avoiding reading this one because I don't use\nbranches, and am having a hard time following what the point\nof the change is. Maybe someone else will notice that this is\na good change straightaway, and save you the bother of explaining\nit to me.\n\nThe whole detectBranches option seems a bit spotty, given its\nlack of documentation and numerous shady \"## HACK\" and \"## FIXME\"\ncomments. So it certainly does appreciate your attention.\n\nAs far as I can tell, a branch is just a mapping between two\nareas of the repo, and all you can do with one is to feed it\nto \"p4 integrate\" instead of giving a single explicit src/dest.\n\n> @@ -1272,7 +1277,13 @@ class P4Sync(Command):\n> def getBranchMapping(self):\n> lostAndFoundBranches = set()\n> \n> - for info in p4CmdList(\"branches\"):\n> + user = gitConfig(\"git-p4.branchUser\")\n> + if len(user) > 0:\n> + command = \"branches -u %s\" % user\n> + else:\n> + command = \"branches\"\n> +\n> + for info in p4CmdList(command):\n\nThis part is for branches -u, to limit the auto-detected braches\nto just those created by a given user.\n\n> @@ -1305,6 +1316,12 @@ class P4Sync(Command):\n> for branch in lostAndFoundBranches:\n> self.knownBranches[branch] = branch\n> \n> + configBranches = gitConfigList(\"git-p4.branchList\")\n> + for branch in configBranches:\n> + if branch:\n> + (source, destination) = branch.split(\":\")\n> + self.knownBranches[destination] = source\n> +\n\nThis bit adds more branches, if you have put them in .git/config\nseparated by a :. Presumably things that are directories in the\ndepot but do _not_ have a branch? I don't get it. What does\nyour depot look like and what do you stick in branchList?\n\n> branches = p4BranchesInGit(self.importIntoRemotes)\n> for branch in branches.keys():\n> @@ -1626,12 +1643,14 @@ class P4Sync(Command):\n> else:\n> paths = []\n> for (prev, cur) in zip(self.previousDepotPaths, depotPaths):\n> - for i in range(0, min(len(cur), len(prev))):\n> - if cur[i] <> prev[i]:\n> + prev_list = prev.split(\"/\")\n> + cur_list = cur.split(\"/\")\n> + for i in range(0, min(len(cur_list), len(prev_list))):\n> + if cur_list[i] <> prev_list[i]:\n> i = i - 1\n> break\n> \n> - paths.append (cur[:i + 1])\n> + paths.append (\"/\".join(cur_list[:i + 1]))\n\nThis was a bug? I guess somehow this is adapting the existing\ndepot paths to include new ones that you discovered with \"p4\nbranches\". And now you are comparing the path elements instead\nof going a character at a time. Maybe to catch //depot/ab vs\n//depot/ac ?\n\nIn other words, add some comments or add something to the commit\ntext. It is encouraged to split up a patch into the smallest\nlogical chenk if the changes are all independent. Maybe these\nthree are related through the theme of auto-branch detection.\n\n\t\t-- Pete\n"},{"id":"161481","messageId":"AANLkTinXHOUNhi163WFGYLgQx4QhO=duOGExhtYr-chK@mail.gmail.com","threadId":"26514","inReplyTo":"20110217184234.GB20627@honk.padd.com","subject":"Re: [PATCH 2/2] git-p4: Improve branch support","fromName":"Vitor Antunes","fromEmail":"vitor.hda@gmail.com","sentAt":"2011-02-18T00:51:00Z","receivedAt":"2011-02-18T00:51:00Z","isPatch":true,"sender":{"key":"vitor.hda@gmail.com","avatar":"https://avatars.githubusercontent.com/u/606876?v=4"},"body":"Hi Pete,\n\nFirst thing I just noticed is that I wanted to send the \"Improve\nrename detection\" patch and not this one.\nAnd now, after I sent it I understood that I still don't know how to\nuse the format-patch correctly because I sent it as a 1/3 set of\npatches... :/\nSorry to all the mailing list about this.\n\nOn Thu, Feb 17, 2011 at 6:42 PM, Pete Wyckoff wrote:\n> vitor.hda@gmail.com wrote on Tue, 15 Feb 2011 23:49 +0000:\n>> Add new config option branchUser to allow filtering P4 branch list by user.\n>> Allow defining the branch list through branchList config option.\n>> Correct base branch directory detection to use '/' as the split character.\n>\n> I've only been avoiding reading this one because I don't use\n> branches, and am having a hard time following what the point\n> of the change is.  Maybe someone else will notice that this is\n> a good change straightaway, and save you the bother of explaining\n> it to me.\n>\n> The whole detectBranches option seems a bit spotty, given its\n> lack of documentation and numerous shady \"## HACK\" and \"## FIXME\"\n> comments.  So it certainly does appreciate your attention.\n\nI also noticed those comments in the code, but until now I did not\nfind any wrong behavior on how the script works with branches. But\nthen again, in our depot we don't do many complex operations.\nI still want to add an improvement on how it detects the branch's\norigin commit. If while doing that I see something else I can improve\nI will do so :)\n\n> As far as I can tell, a branch is just a mapping between two\n> areas of the repo, and all you can do with one is to feed it\n> to \"p4 integrate\" instead of giving a single explicit src/dest.\n\nIn P4 branches work like in Subversion - they are simple links to the\norigin data, which are created with the integrate command. But we can\nalso define a branch specification. That way we can simply refer to\nthat branch for the integrate to know where to update the data\nfrom/to. git-p4 was using the branch specs to search for branches.\n\n>> @@ -1272,7 +1277,13 @@ class P4Sync(Command):\n>>      def getBranchMapping(self):\n>>          lostAndFoundBranches = set()\n>>\n>> -        for info in p4CmdList(\"branches\"):\n>> +        user = gitConfig(\"git-p4.branchUser\")\n>> +        if len(user) > 0:\n>> +            command = \"branches -u %s\" % user\n>> +        else:\n>> +            command = \"branches\"\n>> +\n>> +        for info in p4CmdList(command):\n>\n> This part is for branches -u, to limit the auto-detected braches\n> to just those created by a given user.\n\nYes, as simple as that!\nWhen you have a worldwide server an ocean away with thousands of\nbranch specifications, you really don't want to go through all of\nthem... ;)\n\n>> @@ -1305,6 +1316,12 @@ class P4Sync(Command):\n>>          for branch in lostAndFoundBranches:\n>>              self.knownBranches[branch] = branch\n>>\n>> +        configBranches = gitConfigList(\"git-p4.branchList\")\n>> +        for branch in configBranches:\n>> +            if branch:\n>> +                (source, destination) = branch.split(\":\")\n>> +                self.knownBranches[destination] = source\n>> +\n>\n> This bit adds more branches, if you have put them in .git/config\n> separated by a :.  Presumably things that are directories in the\n> depot but do _not_ have a branch?  I don't get it.  What does\n> your depot look like and what do you stick in branchList?\n\nAssume\n\n//depot/project\n\nas the main branch. And\n\n//depot/project_featureA\n//depot/project_featureB\n\nas feature development branches. So, you could have a list like:\nproject:project_featureA\nproject:project_featureB\n\nOf course, if project_featureB originated from the first dev branch,\nyou would need to define it like:\n\nproject_featureA:project_featureB\n\nThis was to avoid using P4 branch specifications. In my team we don't\nuse them that much and since I was only creating branch specs to use\ngit-p4, I kinda of thought it would be better do keep that\nconfiguration within git.\n\n>>          branches = p4BranchesInGit(self.importIntoRemotes)\n>>          for branch in branches.keys():\n>> @@ -1626,12 +1643,14 @@ class P4Sync(Command):\n>>                      else:\n>>                          paths = []\n>>                          for (prev, cur) in zip(self.previousDepotPaths, depotPaths):\n>> -                            for i in range(0, min(len(cur), len(prev))):\n>> -                                if cur[i] <> prev[i]:\n>> +                            prev_list = prev.split(\"/\")\n>> +                            cur_list = cur.split(\"/\")\n>> +                            for i in range(0, min(len(cur_list), len(prev_list))):\n>> +                                if cur_list[i] <> prev_list[i]:\n>>                                      i = i - 1\n>>                                      break\n>>\n>> -                            paths.append (cur[:i + 1])\n>> +                            paths.append (\"/\".join(cur_list[:i + 1]))\n>\n> This was a bug?  I guess somehow this is adapting the existing\n> depot paths to include new ones that you discovered with \"p4\n> branches\".  And now you are comparing the path elements instead\n> of going a character at a time.  Maybe to catch //depot/ab vs\n> //depot/ac ?\n\nExactly. The example I used above was created to also show that.\n\n> In other words, add some comments or add something to the commit\n> text.  It is encouraged to split up a patch into the smallest\n> logical chenk if the changes are all independent.  Maybe these\n> three are related through the theme of auto-branch detection.\n\nUnderstood. I think this patch can easily be split into 3 different\nlogical chunks.\nI'll add some comments and probably see if I can include some\ndescriptions/examples in git-p4.txt. I just have to find some free\ntime first :)\n\nThank you very much for your feedback and help.\n\n-- \nVitor Antunes\n"}]}