{"thread":{"id":"29378","subject":"[PATCHv3 0/4] git-p4: small fixes to branches and labels","startedAt":"2012-01-16T23:14:45Z","lastAt":"2012-01-19T09:12:16Z","messageCount":7,"participants":["Luke Diamand","Pete Wyckoff"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"182647","messageId":"1326755689-3344-1-git-send-email-luke@diamand.org","threadId":"29378","inReplyTo":null,"subject":"[PATCHv3 0/4] git-p4: small fixes to branches and labels","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2012-01-16T23:14:45Z","receivedAt":"2012-01-16T23:14:45Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"This is the third version of some small fixes to git-p4 branch and\nlabel handling.\n\nIt was suggested for the earlier version that the author handling\ncould be simplified to use the 'author' variable. The code can\nbe simplified, but the author is the wrong value to use - it is\njust the author of the commit, not the tag. Use the creator of the\nlabel, or, if that does not exist (\"p4 tag ...\"), the p4 user.\n\nThis change does not fix the other problems with git-p4 labels:\n\n- two p4 labels on the same changelist will fall over\n- labels must match exactly the list of files imported\n- you can't import a label without a p4 commit\n\nLuke Diamand (4):\n  git-p4: handle p4 branches and labels containing shell chars\n  git-p4: cope with labels with empty descriptions\n  git-p4: importing labels should cope with missing owner\n  git-p4: add test for p4 labels\n\n contrib/fast-import/git-p4        |   79 ++++++++++++++++++++----------------\n t/t9803-git-p4-shell-metachars.sh |   48 ++++++++++++++++++++++\n t/t9804-git-p4-label.sh           |   73 ++++++++++++++++++++++++++++++++++\n 3 files changed, 165 insertions(+), 35 deletions(-)\n create mode 100755 t/t9804-git-p4-label.sh\n\n-- \n1.7.8.rc1.209.geac91.dirty\n"},{"id":"182648","messageId":"1326755689-3344-2-git-send-email-luke@diamand.org","threadId":"29378","inReplyTo":"1326755689-3344-1-git-send-email-luke@diamand.org","subject":"[PATCH 1/4] git-p4: handle p4 branches and labels containing shell chars","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2012-01-16T23:14:46Z","receivedAt":"2012-01-16T23:14:46Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Don't use shell expansion when detecting branches, as it will\nfail if the branch name contains a shell metachar. Similarly\nfor labels.\n\nAdd additional test for branches with shell metachars.\n\nSigned-off-by: Luke Diamand <luke@diamand.org>\n---\n contrib/fast-import/git-p4        |    8 +++---\n t/t9803-git-p4-shell-metachars.sh |   48 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 3e1aa27..822e6f1 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -799,7 +799,7 @@ class P4Submit(Command, P4UserMap):\n     def canChangeChangelists(self):\n         # check to see if we have p4 admin or super-user permissions, either of\n         # which are required to modify changelists.\n-        results = p4CmdList(\"protects %s\" % self.depotPath)\n+        results = p4CmdList([\"protects\", self.depotPath])\n         for r in results:\n             if r.has_key('perm'):\n                 if r['perm'] == 'admin':\n@@ -1758,7 +1758,7 @@ class P4Sync(Command, P4UserMap):\n     def getLabels(self):\n         self.labels = {}\n \n-        l = p4CmdList(\"labels %s...\" % ' '.join (self.depotPaths))\n+        l = p4CmdList([\"labels\", \"%s...\" % ' '.join (self.depotPaths)])\n         if len(l) > 0 and not self.silent:\n             print \"Finding files belonging to labels in %s\" % `self.depotPaths`\n \n@@ -1800,7 +1800,7 @@ class P4Sync(Command, P4UserMap):\n             command = \"branches\"\n \n         for info in p4CmdList(command):\n-            details = p4Cmd(\"branch -o %s\" % info[\"branch\"])\n+            details = p4Cmd([\"branch\", \"-o\", info[\"branch\"]])\n             viewIdx = 0\n             while details.has_key(\"View%s\" % viewIdx):\n                 paths = details[\"View%s\" % viewIdx].split(\" \")\n@@ -1938,7 +1938,7 @@ class P4Sync(Command, P4UserMap):\n         sourceRef = self.gitRefForBranch(sourceBranch)\n         #print \"source \" + sourceBranch\n \n-        branchParentChange = int(p4Cmd(\"changes -m 1 %s...@1,%s\" % (sourceDepotPath, firstChange))[\"change\"])\n+        branchParentChange = int(p4Cmd([\"changes\", \"-m\", \"1\", \"%s...@1,%s\" % (sourceDepotPath, firstChange)])[\"change\"])\n         #print \"branch parent: %s\" % branchParentChange\n         gitParent = self.gitCommitByP4Change(sourceRef, branchParentChange)\n         if len(gitParent) > 0:\ndiff --git a/t/t9803-git-p4-shell-metachars.sh b/t/t9803-git-p4-shell-metachars.sh\nindex db04375..db67020 100755\n--- a/t/t9803-git-p4-shell-metachars.sh\n+++ b/t/t9803-git-p4-shell-metachars.sh\n@@ -57,6 +57,54 @@ test_expect_success 'deleting with shell metachars' '\n \t)\n '\n \n+# Create a branch with a shell metachar in its name\n+#\n+# 1. //depot/main\n+# 2. //depot/branch$3\n+\n+test_expect_success 'branch with shell char' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_create_repo \"$git\" &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\n+\t\tmkdir -p main &&\n+\n+\t\techo f1 >main/f1 &&\n+\t\tp4 add main/f1 &&\n+\t\tp4 submit -d \"main/f1\" &&\n+\n+\t\tp4 integrate //depot/main/... //depot/branch\\$3/... &&\n+\t\tp4 submit -d \"integrate main to branch\\$3\" &&\n+\n+\t\techo f1 >branch\\$3/shell_char_branch_file &&\n+\t\tp4 add branch\\$3/shell_char_branch_file &&\n+\t\tp4 submit -d \"branch\\$3/shell_char_branch_file\" &&\n+\n+\t\tp4 branch -i <<-EOF &&\n+\t\tBranch: branch\\$3\n+\t\tView: //depot/main/... //depot/branch\\$3/...\n+\t\tEOF\n+\n+\t\tp4 edit main/f1 &&\n+\t\techo \"a change\" >> main/f1 &&\n+\t\tp4 submit -d \"a change\" main/f1 &&\n+\n+\t\tp4 integrate -b branch\\$3 &&\n+\t\tp4 resolve -am branch\\$3/... &&\n+\t\tp4 submit -d \"integrate main to branch\\$3\" &&\n+\n+\t\tcd \"$git\" &&\n+\n+\t\tgit config git-p4.branchList main:branch\\$3 &&\n+\t\t\"$GITP4\" clone --dest=. --detect-branches //depot@all &&\n+\t\tgit log --all --graph --decorate --stat &&\n+\t\tgit reset --hard p4/depot/branch\\$3 &&\n+\t\ttest -f shell_char_branch_file &&\n+\t\ttest -f f1\n+\t)\n+'\n+\n test_expect_success 'kill p4d' '\n \tkill_p4d\n '\n-- \n1.7.8.rc1.209.geac91.dirty\n"},{"id":"182651","messageId":"1326755689-3344-3-git-send-email-luke@diamand.org","threadId":"29378","inReplyTo":"1326755689-3344-1-git-send-email-luke@diamand.org","subject":"[PATCH 2/4] git-p4: cope with labels with empty descriptions","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2012-01-16T23:14:47Z","receivedAt":"2012-01-16T23:14:47Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Use an explicit length for the data in a label, rather\nthan EOT, so that labels with empty descriptions are\npassed through correctly.\n\nSigned-off-by: Luke Diamand <luke@diamand.org>\n---\n contrib/fast-import/git-p4 |    8 +++++---\n 1 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 822e6f1..f7707f2 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -1741,9 +1741,11 @@ class P4Sync(Command, P4UserMap):\n                     else:\n                         tagger = \"%s <a@b> %s %s\" % (owner, epoch, self.tz)\n                     self.gitStream.write(\"tagger %s\\n\" % tagger)\n-                    self.gitStream.write(\"data <<EOT\\n\")\n-                    self.gitStream.write(labelDetails[\"Description\"])\n-                    self.gitStream.write(\"EOT\\n\\n\")\n+\n+                    description = labelDetails[\"Description\"]\n+                    self.gitStream.write(\"data %d\\n\" % len(description))\n+                    self.gitStream.write(description)\n+                    self.gitStream.write(\"\\n\")\n \n                 else:\n                     if not self.silent:\n-- \n1.7.8.rc1.209.geac91.dirty\n"},{"id":"182649","messageId":"1326755689-3344-4-git-send-email-luke@diamand.org","threadId":"29378","inReplyTo":"1326755689-3344-1-git-send-email-luke@diamand.org","subject":"[PATCH 3/4] git-p4: importing labels should cope with missing owner","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2012-01-16T23:14:48Z","receivedAt":"2012-01-16T23:14:48Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"In p4, the Owner field is optional. If it is missing,\nconstruct something sensible rather than crashing.\n\nSigned-off-by: Luke Diamand <luke@diamand.org>\n---\n contrib/fast-import/git-p4 |   63 ++++++++++++++++++++++++-------------------\n 1 files changed, 35 insertions(+), 28 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex f7707f2..efb2dad 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -563,6 +563,26 @@ class Command:\n class P4UserMap:\n     def __init__(self):\n         self.userMapFromPerforceServer = False\n+        self.myP4UserId = None\n+\n+    def p4UserId(self):\n+        if self.myP4UserId:\n+            return self.myP4UserId\n+\n+        results = p4CmdList(\"user -o\")\n+        for r in results:\n+            if r.has_key('User'):\n+                self.myP4UserId = r['User']\n+                return r['User']\n+        die(\"Could not find your p4 user id\")\n+\n+    def p4UserIsMe(self, p4User):\n+        # return True if the given p4 user is actually me\n+        me = self.p4UserId()\n+        if not p4User or p4User != me:\n+            return False\n+        else:\n+            return True\n \n     def getUserCacheFilename(self):\n         home = os.environ.get(\"HOME\", os.environ.get(\"USERPROFILE\"))\n@@ -700,7 +720,6 @@ class P4Submit(Command, P4UserMap):\n         self.verbose = False\n         self.preserveUser = gitConfig(\"git-p4.preserveUser\").lower() == \"true\"\n         self.isWindows = (platform.system() == \"Windows\")\n-        self.myP4UserId = None\n \n     def check(self):\n         if len(p4CmdList(\"opened ...\")) > 0:\n@@ -808,25 +827,6 @@ class P4Submit(Command, P4UserMap):\n                     return 1\n         return 0\n \n-    def p4UserId(self):\n-        if self.myP4UserId:\n-            return self.myP4UserId\n-\n-        results = p4CmdList(\"user -o\")\n-        for r in results:\n-            if r.has_key('User'):\n-                self.myP4UserId = r['User']\n-                return r['User']\n-        die(\"Could not find your p4 user id\")\n-\n-    def p4UserIsMe(self, p4User):\n-        # return True if the given p4 user is actually me\n-        me = self.p4UserId()\n-        if not p4User or p4User != me:\n-            return False\n-        else:\n-            return True\n-\n     def prepareSubmitTemplate(self):\n         # remove lines in the Files section that show changes to files outside the depot path we're committing into\n         template = \"\"\n@@ -1664,6 +1664,12 @@ class P4Sync(Command, P4UserMap):\n             if self.stream_file.has_key('depotFile'):\n                 self.streamOneP4File(self.stream_file, self.stream_contents)\n \n+    def make_email(self, userid):\n+        if userid in self.users:\n+            return self.users[userid]\n+        else:\n+            return \"%s <a@b>\" % userid\n+\n     def commit(self, details, files, branch, branchPrefixes, parent = \"\"):\n         epoch = details[\"time\"]\n         author = details[\"user\"]\n@@ -1687,10 +1693,7 @@ class P4Sync(Command, P4UserMap):\n         committer = \"\"\n         if author not in self.users:\n             self.getUserMapFromPerforceServer()\n-        if author in self.users:\n-            committer = \"%s %s %s\" % (self.users[author], epoch, self.tz)\n-        else:\n-            committer = \"%s <a@b> %s %s\" % (author, epoch, self.tz)\n+        committer = \"%s %s %s\" % (self.make_email(author), epoch, self.tz)\n \n         self.gitStream.write(\"committer %s\\n\" % committer)\n \n@@ -1735,11 +1738,15 @@ class P4Sync(Command, P4UserMap):\n                     self.gitStream.write(\"from %s\\n\" % branch)\n \n                     owner = labelDetails[\"Owner\"]\n-                    tagger = \"\"\n-                    if author in self.users:\n-                        tagger = \"%s %s %s\" % (self.users[owner], epoch, self.tz)\n+\n+                    # Try to use the owner of the p4 label, or failing that,\n+                    # the current p4 user id.\n+                    if owner:\n+                        email = self.make_email(owner)\n                     else:\n-                        tagger = \"%s <a@b> %s %s\" % (owner, epoch, self.tz)\n+                        email = self.make_email(self.p4UserId())\n+                    tagger = \"%s %s %s\" % (email, epoch, self.tz)\n+\n                     self.gitStream.write(\"tagger %s\\n\" % tagger)\n \n                     description = labelDetails[\"Description\"]\n-- \n1.7.8.rc1.209.geac91.dirty\n"},{"id":"182650","messageId":"1326755689-3344-5-git-send-email-luke@diamand.org","threadId":"29378","inReplyTo":"1326755689-3344-1-git-send-email-luke@diamand.org","subject":"[PATCH 4/4] git-p4: add test for p4 labels","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2012-01-16T23:14:49Z","receivedAt":"2012-01-16T23:14:49Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Add basic test of p4 label import. Checks label import and\nimport with shell metachars; labels with different length\ndescriptions.\n\nSigned-off-by: Luke Diamand <luke@diamand.org>\n---\n t/t9804-git-p4-label.sh |   73 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 73 insertions(+), 0 deletions(-)\n create mode 100755 t/t9804-git-p4-label.sh\n\ndiff --git a/t/t9804-git-p4-label.sh b/t/t9804-git-p4-label.sh\nnew file mode 100755\nindex 0000000..eba3521\n--- /dev/null\n+++ b/t/t9804-git-p4-label.sh\n@@ -0,0 +1,73 @@\n+test_description='git-p4 p4 label tests'\n+\n+. ./lib-git-p4.sh\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+# Basic p4 label tests.\n+#\n+# Note: can't have more than one label per commit - others\n+# are silently discarded.\n+#\n+test_expect_success 'basic p4 labels' '\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tmkdir -p main &&\n+\n+\t\techo f1 >main/f1 &&\n+\t\tp4 add main/f1 &&\n+\t\tp4 submit -d \"main/f1\" &&\n+\n+\t\techo f2 >main/f2 &&\n+\t\tp4 add main/f2 &&\n+\t\tp4 submit -d \"main/f2\" &&\n+\n+\t\techo f3 >main/file_with_\\$metachar &&\n+\t\tp4 add main/file_with_\\$metachar &&\n+\t\tp4 submit -d \"file with metachar\" &&\n+\n+\t\tp4 tag -l tag_f1_only main/f1 &&\n+\t\tp4 tag -l tag_with\\$_shell_char main/... &&\n+\n+\t\techo f4 >main/f4 &&\n+\t\tp4 add main/f4 &&\n+\t\tp4 submit -d \"main/f4\" &&\n+\n+\t\tp4 label -i <<-EOF &&\n+\t\tLabel: long_label\n+\t\tDescription:\n+\t\t   A Label first line\n+\t\t   A Label second line\n+\t\tView:\t//depot/...\n+\t\tEOF\n+\n+\t\tp4 tag -l long_label ... &&\n+\n+\t\tp4 labels ... &&\n+\n+\t\tcd \"$git\" &&\n+\t\tpwd &&\n+\t\t\"$GITP4\" clone --dest=. --detect-labels //depot@all &&\n+\n+\t\tgit tag &&\n+\t\tgit tag >taglist &&\n+\t\ttest_line_count = 3 taglist &&\n+\n+\t\tcd main &&\n+\t\tgit checkout tag_tag_f1_only &&\n+\t\t! test -f f2 &&\n+\t\tgit checkout tag_tag_with\\$_shell_char &&\n+\t\ttest -f f1 && test -f f2 && test -f file_with_\\$metachar &&\n+\n+\t\tgit show tag_long_label | grep -q \"A Label second line\"\n+\t)\n+'\n+\n+test_expect_success 'kill p4d' '\n+\tkill_p4d\n+'\n+\n+test_done\n-- \n1.7.8.rc1.209.geac91.dirty\n"},{"id":"182702","messageId":"20120117223949.GA811@padd.com","threadId":"29378","inReplyTo":"1326755689-3344-2-git-send-email-luke@diamand.org","subject":"Re: [PATCH 1/4] git-p4: handle p4 branches and labels containing shell chars","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2012-01-17T22:39:49Z","receivedAt":"2012-01-17T22:39:49Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"luke@diamand.org wrote on Mon, 16 Jan 2012 23:14 +0000:\n> Don't use shell expansion when detecting branches, as it will\n> fail if the branch name contains a shell metachar. Similarly\n> for labels.\n> \n> Add additional test for branches with shell metachars.\n\nNice.  There will be a fixup on a command in Vitor's series,\ndepending on which goes first.  He'll have a couple of\nun-listified read_pipe{,_lines} that we should treat similarly.\n\n> @@ -1758,7 +1758,7 @@ class P4Sync(Command, P4UserMap):\n>      def getLabels(self):\n>          self.labels = {}\n>  \n> -        l = p4CmdList(\"labels %s...\" % ' '.join (self.depotPaths))\n> +        l = p4CmdList([\"labels\", \"%s...\" % ' '.join (self.depotPaths)])\n>          if len(l) > 0 and not self.silent:\n>              print \"Finding files belonging to labels in %s\" % `self.depotPaths`\n\nI suspect the command \"p4\" \"labels\" \"//depot/foo/...  //depot/bar/...\"\nmight confuse p4, but haven't tested.  Maybe tuck each one in its\nown argument?\n\n    [\"labels\"] + [\"%s...\" % p for p in self.depotPaths]\n\nWhat happened to your failing test?  It's fun to keep the broken\nones around to inspire others to fix them.\n\n\t\t-- Pete\n"},{"id":"182770","messageId":"4F17DE70.2010204@diamand.org","threadId":"29378","inReplyTo":"20120117223949.GA811@padd.com","subject":"Re: [PATCH 1/4] git-p4: handle p4 branches and labels containing shell chars","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2012-01-19T09:12:16Z","receivedAt":"2012-01-19T09:12:16Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 17/01/12 22:39, Pete Wyckoff wrote:\n> luke@diamand.org wrote on Mon, 16 Jan 2012 23:14 +0000:\n>> Don't use shell expansion when detecting branches, as it will\n>> fail if the branch name contains a shell metachar. Similarly\n>> for labels.\n>>\n>> Add additional test for branches with shell metachars.\n>\n> Nice.  There will be a fixup on a command in Vitor's series,\n> depending on which goes first.  He'll have a couple of\n> un-listified read_pipe{,_lines} that we should treat similarly.\n>\n>> @@ -1758,7 +1758,7 @@ class P4Sync(Command, P4UserMap):\n>>       def getLabels(self):\n>>           self.labels = {}\n>>\n>> -        l = p4CmdList(\"labels %s...\" % ' '.join (self.depotPaths))\n>> +        l = p4CmdList([\"labels\", \"%s...\" % ' '.join (self.depotPaths)])\n>>           if len(l)>  0 and not self.silent:\n>>               print \"Finding files belonging to labels in %s\" % `self.depotPaths`\n>\n> I suspect the command \"p4\" \"labels\" \"//depot/foo/...  //depot/bar/...\"\n> might confuse p4, but haven't tested.  Maybe tuck each one in its\n> own argument?\n>\n>      [\"labels\"] + [\"%s...\" % p for p in self.depotPaths]\n\nYes, you're right. I'll resubmit. I suspect the previous code was \nactually broken as well as you end up with the \"...\" only on the last depot.\n\n>\n> What happened to your failing test?  It's fun to keep the broken\n> ones around to inspire others to fix them.\n\nI'll send that one through as well. I actually had a fix, but it needs \nto be reworked now. I'm reluctant to try to do much more on this though \nunless someone can tell me how the --detect-labels code can ever really \nwork.\n\ni.e. if you do:\n\n  p4 submit\n  git-p4 rebase --detect-labels\n  p4 tag LABEL_A\n  git-p4 rebase --detect-labels\n\nAt this point, LABEL_A won't show up in git until the _next_ p4 commit.\n\nIt's slowly working it's way towards the top of my todo list though!\n\nRegards!\nLuke\n"}]}