{"thread":{"id":"28880","subject":"[RFC/PATCH 0/4] git-p4: small fixes to branches and labels; tests","startedAt":"2011-11-07T21:36:35Z","lastAt":"2011-11-07T22:34:23Z","messageCount":6,"participants":["Luke Diamand","Pete Wyckoff"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"179081","messageId":"1320701799-26071-1-git-send-email-luke@diamand.org","threadId":"28880","inReplyTo":null,"subject":"[RFC/PATCH 0/4] git-p4: small fixes to branches and labels; tests","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2011-11-07T21:36:35Z","receivedAt":"2011-11-07T21:36:35Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"This is a small set of patches to git-p4 to fix a couple of issues with\nbranches and labels.\n\nFirstly, I've added the fixes needed so that branches and labels can\ncontain shell metacharacters (missed from the previous series). Added\na test case for this.\n\nIn adding the test case for labels I also found a few other small bugs\nin the label handling:\n\n - labels missing a description or \"EOT\" in their text cause problems;\n - labels without an owner cause problems.\n\nI also noticed, but did not fix, that you can't have more than one label\nper commit (the others are silently dropped) and the documentation for\nbranch import could be improved.\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 |   61 ++++++++++++++++++++-----------------\n t/t9801-git-p4-branch.sh   |   48 +++++++++++++++++++++++++++++\n t/t9804-git-p4-label.sh    |   73 ++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 154 insertions(+), 28 deletions(-)\n create mode 100755 t/t9804-git-p4-label.sh\n\n-- \n1.7.7.295.g34dd4\n"},{"id":"179082","messageId":"1320701799-26071-2-git-send-email-luke@diamand.org","threadId":"28880","inReplyTo":"1320701799-26071-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":"2011-11-07T21:36:36Z","receivedAt":"2011-11-07T21:36:36Z","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---\n contrib/fast-import/git-p4 |    8 +++---\n t/t9801-git-p4-branch.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 b975d67..bcac6ec 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -793,7 +793,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@@ -1528,7 +1528,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@@ -1570,7 +1570,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@@ -1708,7 +1708,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/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh\nindex a25f18d..bd08dff 100755\n--- a/t/t9801-git-p4-branch.sh\n+++ b/t/t9801-git-p4-branch.sh\n@@ -226,6 +226,54 @@ test_expect_success 'git-p4 clone simple branches' '\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.7.295.g34dd4\n"},{"id":"179083","messageId":"1320701799-26071-3-git-send-email-luke@diamand.org","threadId":"28880","inReplyTo":"1320701799-26071-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":"2011-11-07T21:36:37Z","receivedAt":"2011-11-07T21:36:37Z","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---\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 bcac6ec..02f0f54 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -1511,9 +1511,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.7.295.g34dd4\n"},{"id":"179085","messageId":"1320701799-26071-4-git-send-email-luke@diamand.org","threadId":"28880","inReplyTo":"1320701799-26071-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":"2011-11-07T21:36:38Z","receivedAt":"2011-11-07T21:36:38Z","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---\n contrib/fast-import/git-p4 |   45 +++++++++++++++++++++++--------------------\n 1 files changed, 24 insertions(+), 21 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 02f0f54..d97f927 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -553,6 +553,27 @@ class Command:\n     def __init__(self):\n         self.usage = \"usage: %prog [options]\"\n         self.needsGit = True\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 \n class P4UserMap:\n     def __init__(self):\n@@ -694,7 +715,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@@ -802,25 +822,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@@ -1506,7 +1507,9 @@ class P4Sync(Command, P4UserMap):\n \n                     owner = labelDetails[\"Owner\"]\n                     tagger = \"\"\n-                    if author in self.users:\n+                    if not owner:\n+                        tagger = \"%s <a@b> %s %s\" % (self.p4UserId(), epoch, self.tz)\n+                    elif author in self.users:\n                         tagger = \"%s %s %s\" % (self.users[owner], epoch, self.tz)\n                     else:\n                         tagger = \"%s <a@b> %s %s\" % (owner, epoch, self.tz)\n-- \n1.7.7.295.g34dd4\n"},{"id":"179084","messageId":"1320701799-26071-5-git-send-email-luke@diamand.org","threadId":"28880","inReplyTo":"1320701799-26071-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":"2011-11-07T21:36:39Z","receivedAt":"2011-11-07T21:36:39Z","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---\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.7.295.g34dd4\n"},{"id":"179092","messageId":"20111107223423.GA19362@arf.padd.com","threadId":"28880","inReplyTo":"1320701799-26071-4-git-send-email-luke@diamand.org","subject":"Re: [PATCH 3/4] git-p4: importing labels should cope with missing owner","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2011-11-07T22:34:23Z","receivedAt":"2011-11-07T22:34:23Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"luke@diamand.org wrote on Mon, 07 Nov 2011 21:36 +0000:\n> In p4, the Owner field is optional. If it is missing,\n> construct something sensible rather than crashing.\n\nNice.  One question:\n\n>                      owner = labelDetails[\"Owner\"]\n>                      tagger = \"\"\n> -                    if author in self.users:\n> +                    if not owner:\n> +                        tagger = \"%s <a@b> %s %s\" % (self.p4UserId(), epoch, self.tz)\n> +                    elif author in self.users:\n>                          tagger = \"%s %s %s\" % (self.users[owner], epoch, self.tz)\n>                      else:\n>                          tagger = \"%s <a@b> %s %s\" % (owner, epoch, self.tz)\n\nWe trust p4 always returns a key \"Owner\", but it could\nbe an empty string?  I'm okay with that, if that's how it works.\n\nShould be \"author in self.users\"?  I'm looking at the\ncreation of the committer string above, and guessing\nthat we try to use owner, and if that doesn't exist, try\nto use author just like as above, so something like:\n\n    if owner:\n\tif owner in self.users:\n\t    email = self.users[owner]\n\telse:\n\t    email = \"%s <a@b>\" % owner\n    else:\n\tif author in self.users:\n\t    email = self.users[author]\n\telse:\n\t    email = \"%s <a@b>\" % author\n\nI could misunderstand this.\n\nMaybe that lookup in self.users[] and <a@b> default wants to\nhave a function since it will be repeated three times now.\n\nAck to the whole series.\n\n\t\t-- Pete\n"}]}