{"thread":{"id":"30211","subject":"[PATCHv2 0/2] git p4 - import/export of git/p4 tags and labels","startedAt":"2012-04-11T15:21:22Z","lastAt":"2012-04-21T17:59:35Z","messageCount":9,"participants":["Luke Diamand","Junio C Hamano","Pete Wyckoff"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"188967","messageId":"1334157684-31402-1-git-send-email-luke@diamand.org","threadId":"30211","inReplyTo":null,"subject":"[PATCHv2 0/2] git p4 - import/export of git/p4 tags and labels","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2012-04-11T15:21:22Z","receivedAt":"2012-04-11T15:21:22Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"This is an updated version of the previous patch for importing p4 labels\nand exporting git tags.\n\nUnlike the previous version, this one exports tags to p4 during 'submit'.\nBecause of the rebasing that goes on with git-p4, this is no worse\nthan doing it during the sync phase - to be exported a git tag has\nto be on one of the commits that has come from p4.\n\nI have also renamed the --export-tags option to --export-labels, as\nit just seemed confusing (so now is export-labels, import-labels, etc).\n\nThis version is against Pete Wyckoff's patch series for making git-p4\na top-level program.\n\nThere is also a small fix to the t9800 test case to help it work\nwhen P4EDITOR is defined.\n\nLuke Diamand (2):\n  Fixing script editor checks\n  git p4: import/export of labels to/from p4\n\n Documentation/git-p4.txt       |   43 ++++++-\n git-p4.py                      |  263 ++++++++++++++++++++++++++++++++++------\n t/t9800-git-p4-basic.sh        |    4 +-\n t/t9811-git-p4-label-import.sh |  202 ++++++++++++++++++++++++++++++\n 4 files changed, 471 insertions(+), 41 deletions(-)\n create mode 100755 t/t9811-git-p4-label-import.sh\n\n-- \n1.7.10.rc0.161.gdb6eba\n"},{"id":"188968","messageId":"1334157684-31402-2-git-send-email-luke@diamand.org","threadId":"30211","inReplyTo":"1334157684-31402-1-git-send-email-luke@diamand.org","subject":"[PATCHv2 1/2] git p4: Fixing script editor checks","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2012-04-11T15:21:23Z","receivedAt":"2012-04-11T15:21:23Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"If P4EDITOR is defined, the tests will fail when \"git p4\" starts an\neditor.\n\nSigned-off-by: Luke Diamand <luke@diamand.org>\n---\n t/t9800-git-p4-basic.sh |    4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\nindex 13be144..b2f0869 100755\n--- a/t/t9800-git-p4-basic.sh\n+++ b/t/t9800-git-p4-basic.sh\n@@ -335,7 +335,7 @@ test_expect_success 'detect renames' '\n \ttest_when_finished cleanup_git &&\n \t(\n \t\tcd \"$git\" &&\n-\t\tgit config git-p4.skipSubmitEditCheck true &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n \n \t\tgit mv file1 file4 &&\n \t\tgit commit -a -m \"Rename file1 to file4\" &&\n@@ -394,7 +394,7 @@ test_expect_success 'detect copies' '\n \ttest_when_finished cleanup_git &&\n \t(\n \t\tcd \"$git\" &&\n-\t\tgit config git-p4.skipSubmitEditCheck true &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n \n \t\tcp file2 file8 &&\n \t\tgit add file8 &&\n-- \n1.7.10.rc0.161.gdb6eba\n"},{"id":"188969","messageId":"1334157684-31402-3-git-send-email-luke@diamand.org","threadId":"30211","inReplyTo":"1334157684-31402-1-git-send-email-luke@diamand.org","subject":"[PATCHv2 2/2] git p4: import/export of labels to/from p4","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2012-04-11T15:21:24Z","receivedAt":"2012-04-11T15:21:24Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"The existing label import code looks at each commit being\nimported, and then checks for labels at that commit. This\ndoesn't work in the real world though because it will drop\nlabels applied on changelists that have already been imported,\na common pattern.\n\nThis change adds a new --import-labels option. With this option,\nat the end of the sync, git p4 gets sets of labels in p4 and git,\nand then creates a git tag for each missing p4 label.\n\nThis means that tags created on older changelists are\nstill imported.\n\nTags that could not be imported are added to an ignore\nlist.\n\nThe same sets of git and p4 tags and labels can also be used to\nderive a list of git tags to export to p4. This is enabled with\n--export-labels in 'git p4 submit'.\n\nSigned-off-by: Luke Diamand <luke@diamand.org>\n---\n Documentation/git-p4.txt       |   43 ++++++-\n git-p4.py                      |  262 ++++++++++++++++++++++++++++++++++------\n t/t9811-git-p4-label-import.sh |  202 +++++++++++++++++++++++++++++++\n 3 files changed, 468 insertions(+), 39 deletions(-)\n create mode 100755 t/t9811-git-p4-label-import.sh\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex 3fac413..612d51d 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -198,7 +198,11 @@ git repository:\n \n --detect-labels::\n \tQuery p4 for labels associated with the depot paths, and add\n-\tthem as tags in git.\n+\tthem as tags in git. Limited usefulness as only imports labels\n+\tassociated with new changelists. Deprecated.\n+\n+--import-labels::\n+\tImport labels from p4 into git.\n \n --import-local::\n \tBy default, p4 branches are stored in 'refs/remotes/p4/',\n@@ -263,6 +267,16 @@ These options can be used to modify 'git p4 submit' behavior.\n \tRe-author p4 changes before submitting to p4.  This option\n \trequires p4 admin privileges.\n \n+--export-labels:\n+\tExport tags from git as p4 labels. Tags found in git are applied\n+\tto the perforce working directory.\n+\n+Rebase options\n+~~~~~~~~~~~~~~\n+These options can be used to modify 'git p4 rebase' behavior.\n+\n+--import-labels::\n+\tImport p4 labels.\n \n DEPOT PATH SYNTAX\n -----------------\n@@ -427,6 +441,18 @@ git-p4.branchList::\n \tenabled.  Each entry should be a pair of branch names separated\n \tby a colon (:).  This example declares that both branchA and\n \tbranchB were created from main:\n+\n+git-p4.ignoredP4Labels::\n+\tList of p4 labels to ignore. This is built automatically as\n+\tunimportable labels are discovered.\n+\n+git-p4.importLabels::\n+\tImport p4 labels into git, as per --import-labels.\n+\n+git-p4.validLabelRegexp::\n+\tOnly p4 labels matching this regular expression will be imported. The\n+\tdefault value is '[A-Z0-9_\\-.]+$'.\n+\n -------------\n git config       git-p4.branchList main:branchA\n git config --add git-p4.branchList main:branchB\n@@ -481,10 +507,17 @@ git-p4.skipUserNameCheck::\n \tsubmission regardless.\n \n git-p4.attemptRCSCleanup:\n-    If enabled, 'git p4 submit' will attempt to cleanup RCS keywords\n-    ($Header$, etc). These would otherwise cause merge conflicts and prevent\n-    the submit going ahead. This option should be considered experimental at\n-    present.\n+\tIf enabled, 'git p4 submit' will attempt to cleanup RCS keywords\n+\t($Header$, etc). These would otherwise cause merge conflicts and prevent\n+\tthe submit going ahead. This option should be considered experimental at\n+\tpresent.\n+\n+git-p4.exportLabels::\n+\tExport git tags to p4 labels, as per --export-labels.\n+\n+git-p4.validLabelRegexp::\n+\tOnly p4 labels matching this regular expression will be exported. The\n+\tdefault value is '[A-Z0-9_\\-.]+$'.\n \n IMPLEMENTATION DETAILS\n ----------------------\ndiff --git a/git-p4.py b/git-p4.py\nindex f910d5a..b35223e 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -14,6 +14,8 @@ import re, shutil\n \n verbose = False\n \n+# Only labels/tags matching this will be imported/exported\n+defaultLabelRegexp = r'[A-Z0-9_\\-.]+$'\n \n def p4_build_cmd(cmd):\n     \"\"\"Build a suitable p4 command line.\n@@ -253,6 +255,26 @@ def getP4OpenedType(file):\n     else:\n         die(\"Could not determine file type for %s (result: '%s')\" % (file, result))\n \n+# Return the set of all p4 labels\n+def getP4Labels(depotPaths):\n+    labels = set()\n+    if isinstance(depotPaths,basestring):\n+        depotPaths = [depotPaths]\n+\n+    for l in p4CmdList([\"labels\"] + [\"%s...\" % p for p in depotPaths]):\n+        label = l['label']\n+        labels.add(label)\n+\n+    return labels\n+\n+# Return the set of all git tags\n+def getGitTags():\n+    gitTags = set()\n+    for line in read_pipe_lines([\"git\", \"tag\"]):\n+        tag = line.strip()\n+        gitTags.add(tag)\n+    return gitTags\n+\n def diffTreePattern():\n     # This is a simple generator for the diff tree regex pattern. This could be\n     # a class variable if this and parseDiffTreeEntry were a part of a class.\n@@ -792,6 +814,7 @@ class P4Submit(Command, P4UserMap):\n                 optparse.make_option(\"-M\", dest=\"detectRenames\", action=\"store_true\"),\n                 # preserve the user, requires relevant p4 permissions\n                 optparse.make_option(\"--preserve-user\", dest=\"preserveUser\", action=\"store_true\"),\n+                optparse.make_option(\"--export-labels\", dest=\"exportLabels\", action=\"store_true\"),\n         ]\n         self.description = \"Submit changes from git to the perforce depot.\"\n         self.usage += \" [name of git branch to submit into perforce depot]\"\n@@ -801,6 +824,7 @@ 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.exportLabels = False\n \n     def check(self):\n         if len(p4CmdList(\"opened ...\")) > 0:\n@@ -1228,6 +1252,73 @@ class P4Submit(Command, P4UserMap):\n                    + \"Please review/edit and then use p4 submit -i < %s to submit directly!\"\n                    % (fileName, fileName))\n \n+    # Export git tags as p4 labels. Create a p4 label and then tag\n+    # with that.\n+    def exportGitTags(self, gitTags):\n+        validTagRegexp = gitConfig(\"git-p4.validTagRegexp\")\n+        if len(validTagRegexp) == 0:\n+            validTagRegexp = defaultLabelRegexp\n+        m = re.compile(validTagRegexp)\n+        commit_re = re.compile(r'\\s*\\[git-p4:.*change = (\\d+)\\s*\\]')\n+\n+        for name in gitTags:\n+\n+            if not m.match(name):\n+                if verbose:\n+                    print \"tag %s does not match regexp %s\" % (name, validTagRegexp)\n+                continue\n+\n+            # Get the p4 commit this corresponds to\n+            changelist = None\n+            for l in read_pipe_lines([\"git\", \"log\", \"--max-count=1\", name]):\n+                match = commit_re.match(l)\n+                if match:\n+                    changelist = match.group(1)\n+\n+            if not changelist:\n+                # a tag pointing to something not sent to p4; ignore\n+                if verbose:\n+                    print \"git tag %s does not give a p4 commit\" % name\n+                continue\n+\n+            # Get the tag details.\n+            inHeader = True\n+            isAnnotated = False\n+            body = []\n+            for l in read_pipe_lines([\"git\", \"cat-file\", \"-p\", name]):\n+                l = l.strip()\n+                if inHeader:\n+                    if re.match(r'tag\\s+', l):\n+                        isAnnotated = True\n+                    elif re.match(r'\\s*$', l):\n+                        inHeader = False\n+                        continue\n+                else:\n+                    body.append(l)\n+\n+            if not isAnnotated:\n+                body = [\"lightweight tag imported by git p4\\n\"]\n+\n+            # Create the label - use the same view as the client spec we are using\n+            clientSpec = getClientSpec()\n+\n+            labelTemplate  = \"Label: %s\\n\" % name\n+            labelTemplate += \"Description:\\n\"\n+            for b in body:\n+                labelTemplate += \"\\t\" + b + \"\\n\"\n+            labelTemplate += \"View:\\n\"\n+            for mapping in clientSpec.mappings:\n+                labelTemplate += \"\\t%s\\n\" % mapping.depot_side.path\n+\n+            p4_write_pipe([\"label\", \"-i\"], labelTemplate)\n+\n+            # Use the label\n+            p4_system([\"tag\", \"-l\", name] +\n+                      [\"%s@%s\" % (mapping.depot_side.path, changelist) for mapping in clientSpec.mappings])\n+\n+            if verbose:\n+                print \"created p4 label for tag %s\" % name\n+\n     def run(self, args):\n         if len(args) == 0:\n             self.master = currentGitBranch()\n@@ -1317,6 +1408,16 @@ class P4Submit(Command, P4UserMap):\n             rebase = P4Rebase()\n             rebase.rebase()\n \n+        if gitConfig(\"git-p4.exportLabels\", \"--bool\") == \"true\":\n+            self.exportLabels = true\n+\n+        if self.exportLabels:\n+            p4Labels = getP4Labels(self.depotPath)\n+            gitTags = getGitTags()\n+\n+            missingGitTags = gitTags - p4Labels\n+            self.exportGitTags(missingGitTags)\n+\n         return True\n \n class View(object):\n@@ -1544,6 +1645,7 @@ class P4Sync(Command, P4UserMap):\n                 optparse.make_option(\"--changesfile\", dest=\"changesFile\"),\n                 optparse.make_option(\"--silent\", dest=\"silent\", action=\"store_true\"),\n                 optparse.make_option(\"--detect-labels\", dest=\"detectLabels\", action=\"store_true\"),\n+                optparse.make_option(\"--import-labels\", dest=\"importLabels\", action=\"store_true\"),\n                 optparse.make_option(\"--verbose\", dest=\"verbose\", action=\"store_true\"),\n                 optparse.make_option(\"--import-local\", dest=\"importIntoRemotes\", action=\"store_false\",\n                                      help=\"Import into refs/heads/ , not refs/remotes\"),\n@@ -1568,6 +1670,7 @@ class P4Sync(Command, P4UserMap):\n         self.branch = \"\"\n         self.detectBranches = False\n         self.detectLabels = False\n+        self.importLabels = False\n         self.changesFile = \"\"\n         self.syncWithOrigin = True\n         self.verbose = False\n@@ -1829,6 +1932,38 @@ class P4Sync(Command, P4UserMap):\n         else:\n             return \"%s <a@b>\" % userid\n \n+    # Stream a p4 tag\n+    def streamTag(self, gitStream, labelName, labelDetails, commit, epoch):\n+        if verbose:\n+            print \"writing tag %s for commit %s\" % (labelName, commit)\n+        gitStream.write(\"tag %s\\n\" % labelName)\n+        gitStream.write(\"from %s\\n\" % commit)\n+\n+        if labelDetails.has_key('Owner'):\n+            owner = labelDetails[\"Owner\"]\n+        else:\n+            owner = None\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+            email = self.make_email(self.p4UserId())\n+        tagger = \"%s %s %s\" % (email, epoch, self.tz)\n+\n+        gitStream.write(\"tagger %s\\n\" % tagger)\n+\n+        print \"labelDetails=\",labelDetails\n+        if labelDetails.has_key('Description'):\n+            description = labelDetails['Description']\n+        else:\n+            description = 'Label from git p4'\n+\n+        gitStream.write(\"data %d\\n\" % len(description))\n+        gitStream.write(description)\n+        gitStream.write(\"\\n\")\n+\n     def commit(self, details, files, branch, branchPrefixes, parent = \"\"):\n         epoch = details[\"time\"]\n         author = details[\"user\"]\n@@ -1893,25 +2028,7 @@ class P4Sync(Command, P4UserMap):\n                     cleanedFiles[info[\"depotFile\"]] = info[\"rev\"]\n \n                 if cleanedFiles == labelRevisions:\n-                    self.gitStream.write(\"tag tag_%s\\n\" % labelDetails[\"label\"])\n-                    self.gitStream.write(\"from %s\\n\" % branch)\n-\n-                    owner = labelDetails[\"Owner\"]\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-                        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-                    self.gitStream.write(\"data %d\\n\" % len(description))\n-                    self.gitStream.write(description)\n-                    self.gitStream.write(\"\\n\")\n+                    self.streamTag(self.gitStream, 'tag_%s' % labelDetails['label'], labelDetails, branch, epoch)\n \n                 else:\n                     if not self.silent:\n@@ -1923,6 +2040,7 @@ class P4Sync(Command, P4UserMap):\n                     print (\"Tag %s does not match with change %s: file count is different.\"\n                            % (labelDetails[\"label\"], change))\n \n+    # Build a dictionary of changelists and labels, for \"detect-labels\" option.\n     def getLabels(self):\n         self.labels = {}\n \n@@ -1949,6 +2067,69 @@ class P4Sync(Command, P4UserMap):\n         if self.verbose:\n             print \"Label changes: %s\" % self.labels.keys()\n \n+    # Import p4 labels as git tags. A direct mapping does not\n+    # exist, so assume that if all the files are at the same revision\n+    # then we can use that, or it's something more complicated we should\n+    # just ignore.\n+    def importP4Labels(self, stream, p4Labels):\n+        if verbose:\n+            print \"import p4 labels: \" + ' '.join(p4Labels)\n+\n+        ignoredP4Labels = gitConfigList(\"git-p4.ignoredP4Labels\")\n+        validLabelRegexp = gitConfig(\"git-p4.validLabelRegexp\")\n+        if len(validLabelRegexp) == 0:\n+            validLabelRegexp = defaultLabelRegexp\n+        m = re.compile(validLabelRegexp)\n+\n+        for name in p4Labels:\n+            commitFound = False\n+\n+            if not m.match(name):\n+                if verbose:\n+                    print \"label %s does not match regexp %s\" % (name,validLabelRegexp)\n+                continue\n+\n+            if name in ignoredP4Labels:\n+                continue\n+\n+            labelDetails = p4CmdList(['label', \"-o\", name])[0]\n+\n+            # get the most recent changelist for each file in this label\n+            change = p4Cmd([\"changes\", \"-m\", \"1\"] + [\"%s...@%s\" % (p, name)\n+                                for p in self.depotPaths])\n+\n+            if change.has_key('change'):\n+                # find the corresponding git commit; take the oldest commit\n+                changelist = int(change['change'])\n+                gitCommit = read_pipe([\"git\", \"rev-list\", \"--max-count=1\",\n+                     \"--reverse\", \":/\\[git-p4:.*change = %d\\]\" % changelist])\n+                if len(gitCommit) == 0:\n+                    print \"could not find git commit for changelist %d\" % changelist\n+                else:\n+                    gitCommit = gitCommit.strip()\n+                    commitFound = True\n+                    # Convert from p4 time format\n+                    try:\n+                        tmwhen = time.strptime(labelDetails['Update'], \"%Y/%m/%d %H:%M:%S\")\n+                    except ValueError:\n+                        print \"Could not convert label time %s\" % labelDetail['Update']\n+                        tmwhen = 1\n+\n+                    when = int(time.mktime(tmwhen))\n+                    self.streamTag(stream, name, labelDetails, gitCommit, when)\n+                    if verbose:\n+                        print \"p4 label %s mapped to git commit %s\" % (name, gitCommit)\n+            else:\n+                if verbose:\n+                    print \"Label %s has no changelists - possibly deleted?\" % name\n+\n+            if not commitFound:\n+                # We can't import this label; don't try again as it will get very\n+                # expensive repeatedly fetching all the files for labels that will\n+                # never be imported. If the label is moved in the future, the\n+                # ignore will need to be removed manually.\n+                system([\"git\", \"config\", \"--add\", \"git-p4.ignoredP4Labels\", name])\n+\n     def guessProjectName(self):\n         for p in self.depotPaths:\n             if p.endswith(\"/\"):\n@@ -2425,7 +2606,6 @@ class P4Sync(Command, P4UserMap):\n \n         self.depotPaths = newPaths\n \n-\n         self.loadUserMapFromCache()\n         self.labels = {}\n         if self.detectLabels:\n@@ -2489,22 +2669,31 @@ class P4Sync(Command, P4UserMap):\n             if len(changes) == 0:\n                 if not self.silent:\n                     print \"No changes to import!\"\n-                return True\n+            else:\n+                if not self.silent and not self.detectBranches:\n+                    print \"Import destination: %s\" % self.branch\n \n-            if not self.silent and not self.detectBranches:\n-                print \"Import destination: %s\" % self.branch\n+                self.updatedBranches = set()\n \n-            self.updatedBranches = set()\n+                self.importChanges(changes)\n \n-            self.importChanges(changes)\n+                if not self.silent:\n+                    print \"\"\n+                    if len(self.updatedBranches) > 0:\n+                        sys.stdout.write(\"Updated branches: \")\n+                        for b in self.updatedBranches:\n+                            sys.stdout.write(\"%s \" % b)\n+                        sys.stdout.write(\"\\n\")\n \n-            if not self.silent:\n-                print \"\"\n-                if len(self.updatedBranches) > 0:\n-                    sys.stdout.write(\"Updated branches: \")\n-                    for b in self.updatedBranches:\n-                        sys.stdout.write(\"%s \" % b)\n-                    sys.stdout.write(\"\\n\")\n+        if gitConfig(\"git-p4.importLabels\", \"--bool\") == \"true\":\n+            self.importLabels = true\n+\n+        if self.importLabels:\n+            p4Labels = getP4Labels(self.depotPaths)\n+            gitTags = getGitTags()\n+\n+            missingP4Labels = p4Labels - gitTags\n+            self.importP4Labels(self.gitStream, missingP4Labels)\n \n         self.gitStream.close()\n         if importProcess.wait() != 0:\n@@ -2523,13 +2712,18 @@ class P4Sync(Command, P4UserMap):\n class P4Rebase(Command):\n     def __init__(self):\n         Command.__init__(self)\n-        self.options = [ ]\n+        self.options = [\n+                optparse.make_option(\"--import-labels\", dest=\"importLabels\", action=\"store_true\"),\n+                optparse.make_option(\"--verbose\", dest=\"verbose\", action=\"store_true\"),\n+        ]\n+        self.verbose = False\n+        self.importLabels = False\n         self.description = (\"Fetches the latest revision from perforce and \"\n                             + \"rebases the current work (branch) against it\")\n-        self.verbose = False\n \n     def run(self, args):\n         sync = P4Sync()\n+        sync.importLabels = self.importLabels\n         sync.run([])\n \n         return self.rebase()\ndiff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\nnew file mode 100755\nindex 0000000..85d6049\n--- /dev/null\n+++ b/t/t9811-git-p4-label-import.sh\n@@ -0,0 +1,202 @@\n+#!/bin/sh\n+\n+test_description='git 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 import tests.\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+\t\tp4 tag -l this_tag_will_be_skipped 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: TAG_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 TAG_LONG_LABEL ... &&\n+\n+\t\tp4 labels ... &&\n+\n+\t\tgit p4 clone --dest=\"$git\" //depot@all &&\n+\t\tcd \"$git\" &&\n+\t\tgit config git-p4.validLabelRegexp \".*TAG.*\" &&\n+\t\tgit p4 sync --import-labels --verbose &&\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_F1_ONLY &&\n+\t\t! test -f f2 &&\n+\t\tgit checkout 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+# Test some label corner cases:\n+#\n+# - two tags on the same file; both should be available\n+# - a tag that is only on one file; this kind of tag\n+#   cannot be imported (at least not easily).\n+\n+test_expect_success 'two labels on the same changelist' '\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tmkdir -p main &&\n+\n+\t\tp4 edit main/f1 main/f2 &&\n+\t\techo \"hello world\" >main/f1 &&\n+\t\techo \"not in the tag\" >main/f2 &&\n+\t\tp4 submit -d \"main/f[12]: testing two labels\" &&\n+\n+\t\tp4 tag -l TAG_F1_1 main/... &&\n+\t\tp4 tag -l TAG_F1_2 main/... &&\n+\n+\t\tp4 labels ... &&\n+\n+\t\tgit p4 clone --dest=\"$git\" //depot@all &&\n+\t\tcd \"$git\" &&\n+\t\tgit p4 sync --import-labels &&\n+\n+\t\tgit tag | grep TAG_F1 &&\n+\t\tgit tag | grep -q TAG_F1_1 &&\n+\t\tgit tag | grep -q TAG_F1_2 &&\n+\n+\t\tcd main &&\n+\n+\t\tgit checkout TAG_F1_1 &&\n+\t\tls &&\n+\t\ttest -f f1 &&\n+\n+\t\tgit checkout TAG_F1_2 &&\n+\t\tls &&\n+\t\ttest -f f1\n+\t)\n+'\n+\n+# Export some git tags to p4\n+test_expect_success 'export git tags to p4' '\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit tag -m \"A tag created in git:xyzzy\" GIT_TAG_1 &&\n+\t\techo \"hello world\" >main/f10 &&\n+\t\tgit add main/f10 &&\n+\t\tgit commit -m \"Adding file for export test\" &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n+\t\tgit p4 submit &&\n+\t\tgit tag -m \"Another git tag\" GIT_TAG_2 &&\n+\t\tgit tag LIGHTWEIGHT_TAG &&\n+\t\tgit p4 rebase --import-labels --verbose &&\n+\t\tgit p4 submit --export-labels --verbose\n+\t) &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tp4 sync ... &&\n+\t\tp4 labels ... | grep GIT_TAG_1 &&\n+\t\tp4 labels ... | grep GIT_TAG_2 &&\n+\t\tp4 labels ... | grep LIGHTWEIGHT_TAG &&\n+\t\tp4 label -o GIT_TAG_1 | grep \"tag created in git:xyzzy\" &&\n+\t\tp4 sync ...@GIT_TAG_1 &&\n+\t\t! test -f main/f10\n+\t\tp4 sync ...@GIT_TAG_2 &&\n+\t\ttest -f main/f10\n+\t)\n+'\n+\n+# Export a tag from git where an affected file is deleted later on\n+# Need to create git tags after rebase, since only then can the\n+# git commits be mapped to p4 changelists.\n+test_expect_success 'export git tags to p4 with deletion' '\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit p4 sync --import-labels &&\n+\t\techo \"deleted file\" >main/deleted_file &&\n+\t\tgit add main/deleted_file &&\n+\t\tgit commit -m \"create deleted file\" &&\n+\t\tgit rm main/deleted_file &&\n+\t\techo \"new file\" >main/f11 &&\n+\t\tgit add main/f11 &&\n+\t\tgit commit -m \"delete the deleted file\" &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n+\t\tgit p4 submit &&\n+\t\tgit p4 rebase --import-labels --verbose &&\n+\t\tgit tag -m \"tag on deleted file\" GIT_TAG_ON_DELETED HEAD~1 &&\n+\t\tgit tag -m \"tag after deletion\" GIT_TAG_AFTER_DELETION HEAD &&\n+\t\tgit p4 submit --export-labels --verbose\n+\t) &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tp4 sync ... &&\n+\t\tp4 sync ...@GIT_TAG_ON_DELETED &&\n+\t\ttest -f main/deleted_file &&\n+\t\tp4 sync ...@GIT_TAG_AFTER_DELETION &&\n+\t\t! test -f main/deleted_file &&\n+\t\techo \"checking label contents\" &&\n+\t\tp4 label -o GIT_TAG_ON_DELETED | grep \"tag on deleted file\"\n+\t)\n+'\n+\n+# Create a tag in git that cannot be exported to p4\n+test_expect_success 'tag that cannot be exported' '\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit checkout -b a_branch &&\n+\t\techo \"hello\" >main/f12 &&\n+\t\tgit add main/f12 &&\n+\t\tgit commit -m \"adding f12\" &&\n+\t\tgit tag -m \"tag on a_branch\" GIT_TAG_ON_A_BRANCH &&\n+\t\tgit checkout master &&\n+\t\tgit p4 submit --export-labels\n+\t) &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tp4 sync ... &&\n+\t\t!(p4 labels | grep GIT_TAG_ON_A_BRANCH)\n+\t)\n+'\n+\n+test_expect_success 'kill p4d' '\n+\tkill_p4d\n+'\n+\n+test_done\n-- \n1.7.10.rc0.161.gdb6eba\n"},{"id":"188997","messageId":"7vpqbekx7h.fsf@alter.siamese.dyndns.org","threadId":"30211","inReplyTo":"1334157684-31402-2-git-send-email-luke@diamand.org","subject":"Re: [PATCHv2 1/2] git p4: Fixing script editor checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-11T17:14:10Z","receivedAt":"2012-04-11T17:14:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand <luke@diamand.org> writes:\n\n> If P4EDITOR is defined, the tests will fail when \"git p4\" starts an\n> editor.\n\nIs that a problem specific to tests, or should \"git p4\" itself unset that\nenvironment?  If it is a problem specific to tests, would it be a better\nfix to add \"P4EDITOR=:\" like we do for EDITOR in t/test-lib.sh?\n\n>\n> Signed-off-by: Luke Diamand <luke@diamand.org>\n> ---\n>  t/t9800-git-p4-basic.sh |    4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\n> index 13be144..b2f0869 100755\n> --- a/t/t9800-git-p4-basic.sh\n> +++ b/t/t9800-git-p4-basic.sh\n> @@ -335,7 +335,7 @@ test_expect_success 'detect renames' '\n>  \ttest_when_finished cleanup_git &&\n>  \t(\n>  \t\tcd \"$git\" &&\n> -\t\tgit config git-p4.skipSubmitEditCheck true &&\n> +\t\tgit config git-p4.skipSubmitEdit true &&\n>  \n>  \t\tgit mv file1 file4 &&\n>  \t\tgit commit -a -m \"Rename file1 to file4\" &&\n> @@ -394,7 +394,7 @@ test_expect_success 'detect copies' '\n>  \ttest_when_finished cleanup_git &&\n>  \t(\n>  \t\tcd \"$git\" &&\n> -\t\tgit config git-p4.skipSubmitEditCheck true &&\n> +\t\tgit config git-p4.skipSubmitEdit true &&\n>  \n>  \t\tcp file2 file8 &&\n>  \t\tgit add file8 &&\n"},{"id":"189015","messageId":"CAE5ih7_X=4gewga8fMzaEvowsJsA1Ta9PQ2bGixx5eVvykputA@mail.gmail.com","threadId":"30211","inReplyTo":"7vpqbekx7h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 1/2] git p4: Fixing script editor checks","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2012-04-11T18:51:45Z","receivedAt":"2012-04-11T18:51:45Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On Wed, Apr 11, 2012 at 7:14 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Luke Diamand <luke@diamand.org> writes:\n>\n>> If P4EDITOR is defined, the tests will fail when \"git p4\" starts an\n>> editor.\n>\n> Is that a problem specific to tests, or should \"git p4\" itself unset that\n> environment?  If it is a problem specific to tests, would it be a better\n> fix to add \"P4EDITOR=:\" like we do for EDITOR in t/test-lib.sh?\n\nYes and no - git-p4.py will run $P4EDITOR if it is set, even if it's\njust empty. So it would need a small fix to check for an empty string.\nI can submit a suitable patch.\n"},{"id":"189019","messageId":"7vpqbejdf4.fsf@alter.siamese.dyndns.org","threadId":"30211","inReplyTo":"CAE5ih7_X=4gewga8fMzaEvowsJsA1Ta9PQ2bGixx5eVvykputA@mail.gmail.com","subject":"Re: [PATCHv2 1/2] git p4: Fixing script editor checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-11T19:06:55Z","receivedAt":"2012-04-11T19:06:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand <luke@diamand.org> writes:\n\n> On Wed, Apr 11, 2012 at 7:14 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Luke Diamand <luke@diamand.org> writes:\n>>\n>>> If P4EDITOR is defined, the tests will fail when \"git p4\" starts an\n>>> editor.\n>>\n>> Is that a problem specific to tests, or should \"git p4\" itself unset that\n>> environment? If it is a problem specific to tests, would it be a better\n>> fix to add \"P4EDITOR=:\" like we do for EDITOR in t/test-lib.sh?\n>\n> Yes and no - git-p4.py will run $P4EDITOR if it is set, even if it's\n> just empty. So it would need a small fix to check for an empty string.\n> I can submit a suitable patch.\n\nHow does real \"p4\" run $P4EDITOR?  For example, if you have:\n\n\tP4EDITOR='vi -e'\n\ndoes it start \"vi\" in its \"ex\" mode, implying that it behaves as if the\ninvocation were like this in a hypothetical implementation of \"p4\" as a\nshell script:\n\n\t#!/bin/sh\n        ... do some stuff p4 does ...\n\t$P4EDITOR file-to-be-edited\n\nWhatever it does needs to be emulated by the code in git-p4.py that runs\nit, as that is the way the end users expect, and if it turns out to be\nlike the above, setting P4EDITOR to ':' just like we do to EDITOR in\nt/test-lib.sh should be the right thing to do.  The always-true command\ncolon will leave the file-to-be-edited unmodified and report success.\n\nSimilarly, what happens with \"p4\" when $P4EDITOR is set to an empty\nstring?  If it ignores and falls back to the other usual way to find your\npreferred editor, you should emulate that as well in git-p4.py.\n\nThanks.\n"},{"id":"189022","messageId":"CAE5ih7-49ynPFrHDn_a0fiskSjfovS4qX2eHh9W3G08HaHHRkw@mail.gmail.com","threadId":"30211","inReplyTo":"7vpqbejdf4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 1/2] git p4: Fixing script editor checks","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2012-04-11T19:24:40Z","receivedAt":"2012-04-11T19:24:40Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On Wed, Apr 11, 2012 at 9:06 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Luke Diamand <luke@diamand.org> writes:\n>\n>> On Wed, Apr 11, 2012 at 7:14 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Luke Diamand <luke@diamand.org> writes:\n>>>\n>>>> If P4EDITOR is defined, the tests will fail when \"git p4\" starts an\n>>>> editor.\n>>>\n>>> Is that a problem specific to tests, or should \"git p4\" itself unset that\n>>> environment? If it is a problem specific to tests, would it be a better\n>>> fix to add \"P4EDITOR=:\" like we do for EDITOR in t/test-lib.sh?\n>>\n>> Yes and no - git-p4.py will run $P4EDITOR if it is set, even if it's\n>> just empty. So it would need a small fix to check for an empty string.\n>> I can submit a suitable patch.\n>\n> How does real \"p4\" run $P4EDITOR?  For example, if you have:\n>\n>        P4EDITOR='vi -e'\n>\n> does it start \"vi\" in its \"ex\" mode, implying that it behaves as if the\n> invocation were like this in a hypothetical implementation of \"p4\" as a\n> shell script:\n>\n>        #!/bin/sh\n>        ... do some stuff p4 does ...\n>        $P4EDITOR file-to-be-edited\n>\n> Whatever it does needs to be emulated by the code in git-p4.py that runs\n> it, as that is the way the end users expect, and if it turns out to be\n> like the above, setting P4EDITOR to ':' just like we do to EDITOR in\n> t/test-lib.sh should be the right thing to do.  The always-true command\n> colon will leave the file-to-be-edited unmodified and report success.\n>\n> Similarly, what happens with \"p4\" when $P4EDITOR is set to an empty\n> string?  If it ignores and falls back to the other usual way to find your\n> preferred editor, you should emulate that as well in git-p4.py.\n>\n> Thanks.\n\n\"vi -e\" => vi in ex mode\n\":\" => always-true-command\n\"\" => falls back to $EDITOR, and then to vi(UNIX or Win32 if $SHELL is\nset) or notepad(Win32).\n"},{"id":"189621","messageId":"20120418113422.GB19994@padd.com","threadId":"30211","inReplyTo":"1334157684-31402-3-git-send-email-luke@diamand.org","subject":"Re: [PATCHv2 2/2] git p4: import/export of labels to/from p4","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2012-04-18T11:34:22Z","receivedAt":"2012-04-18T11:34:22Z","isPatch":false,"sender":{"key":"pw@padd.com","avatar":null},"body":"luke@diamand.org wrote on Wed, 11 Apr 2012 17:21 +0200:\n> The existing label import code looks at each commit being\n> imported, and then checks for labels at that commit. This\n> doesn't work in the real world though because it will drop\n> labels applied on changelists that have already been imported,\n> a common pattern.\n> \n> This change adds a new --import-labels option. With this option,\n> at the end of the sync, git p4 gets sets of labels in p4 and git,\n> and then creates a git tag for each missing p4 label.\n> \n> This means that tags created on older changelists are\n> still imported.\n> \n> Tags that could not be imported are added to an ignore\n> list.\n> \n> The same sets of git and p4 tags and labels can also be used to\n> derive a list of git tags to export to p4. This is enabled with\n> --export-labels in 'git p4 submit'.\n\nThis is a good approach.  Here's some late review comments.\n\n> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n\n> +git-p4.validLabelRegexp::\n> +\tOnly p4 labels matching this regular expression will be imported. The\n> +\tdefault value is '[A-Z0-9_\\-.]+$'.\n> +\n\nand\n\n> +git-p4.validLabelRegexp::\n> +\tOnly p4 labels matching this regular expression will be exported. The\n> +\tdefault value is '[A-Z0-9_\\-.]+$'.\n\nThis one wants to be validTagRegexp.  Or you could combine them\nboth into one.  Why no small a-z characters?\n\n> diff --git a/git-p4.py b/git-p4.py\n\n> +            # Get the p4 commit this corresponds to\n> +            changelist = None\n> +            for l in read_pipe_lines([\"git\", \"log\", \"--max-count=1\", name]):\n> +                match = commit_re.match(l)\n> +                if match:\n> +                    changelist = match.group(1)\n\nWe have extractLogMessageFromGitCommit and extractSettingsGitLog\nto grep out the \"git-p4.. change\" tag.  They're not beautiful,\nbut we should reuse them, in case this mechanism of connecting\nchanges to commits ever changes.\n\n> +            # Get the tag details.\n> +            inHeader = True\n> +            isAnnotated = False\n> +            body = []\n> +            for l in read_pipe_lines([\"git\", \"cat-file\", \"-p\", name]):\n> +                l = l.strip()\n> +                if inHeader:\n> +                    if re.match(r'tag\\s+', l):\n> +                        isAnnotated = True\n> +                    elif re.match(r'\\s*$', l):\n> +                        inHeader = False\n> +                        continue\n> +                else:\n> +                    body.append(l)\n> +\n> +            if not isAnnotated:\n> +                body = [\"lightweight tag imported by git p4\\n\"]\n\nThe manual parsing, just to get the text in the tag (or ref),\nseems a bit awkward.  I looked at \"git show --pretty=format:%B\"\nas a way to get just the text, and \"git cat-file -t ref\" to get\nthe tag/ref difference.  But no easy replacement.\n\n> +            if change.has_key('change'):\n> +                # find the corresponding git commit; take the oldest commit\n> +                changelist = int(change['change'])\n> +                gitCommit = read_pipe([\"git\", \"rev-list\", \"--max-count=1\",\n> +                     \"--reverse\", \":/\\[git-p4:.*change = %d\\]\" % changelist])\n> +                if len(gitCommit) == 0:\n> +                    print \"could not find git commit for changelist %d\" % changelist\n> +                else:\n> +                    gitCommit = gitCommit.strip()\n> +                    commitFound = True\n> +                    # Convert from p4 time format\n> +                    try:\n> +                        tmwhen = time.strptime(labelDetails['Update'], \"%Y/%m/%d %H:%M:%S\")\n> +                    except ValueError:\n> +                        print \"Could not convert label time %s\" % labelDetail['Update']\n> +                        tmwhen = 1\n> +\n> +                    when = int(time.mktime(tmwhen))\n> +                    self.streamTag(stream, name, labelDetails, gitCommit, when)\n> +                    if verbose:\n> +                        print \"p4 label %s mapped to git commit %s\" % (name, gitCommit)\n\nNice, even the icky but required p4 time parsing.  We don't have\na common function to go from change -> commit yet.\n\n>  class P4Rebase(Command):\n>      def __init__(self):\n>          Command.__init__(self)\n> -        self.options = [ ]\n> +        self.options = [\n> +                optparse.make_option(\"--import-labels\", dest=\"importLabels\", action=\"store_true\"),\n> +                optparse.make_option(\"--verbose\", dest=\"verbose\", action=\"store_true\"),\n> +        ]\n> +        self.verbose = False\n> +        self.importLabels = False\n>          self.description = (\"Fetches the latest revision from perforce and \"\n>                              + \"rebases the current work (branch) against it\")\n> -        self.verbose = False\n\nAll commands should have a --verbose.  Could you move the\n\"--verbose\" description in the man page up into \"General\noptions\"?  Since it means the same thing in all the commands,\nroughly.\n\n>      def run(self, args):\n>          sync = P4Sync()\n> +        sync.importLabels = self.importLabels\n>          sync.run([])\n\nBut no \"sync.verbose = self.verbose\"?  I wonder if P4Rebase should\ninherit from P4Sync, like Clone does.  But that is a bit obscure\nto me already.  Probably these all want to be split out into a\nbunch of functions; there isn't a lot of state tracking happening\nin the classes, and only one instance of each per invocation.\n\n> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\n> new file mode 100755\n> index 0000000..85d6049\n> --- /dev/null\n> +++ b/t/t9811-git-p4-label-import.sh\n> @@ -0,0 +1,202 @@\n> +#!/bin/sh\n> +\n> +test_description='git p4 label tests'\n\nThis whole set of tests can go it the existing t9804, right?  It\nseems that the first few of these are duplicates of what is\nalready in there.\n\nNice test coverage, and you fixed the broken one in t9804.\n\n\t\t-- Pete\n"},{"id":"189810","messageId":"4F92F587.90305@diamand.org","threadId":"30211","inReplyTo":"20120418113422.GB19994@padd.com","subject":"Re: [PATCHv2 2/2] git p4: import/export of labels to/from p4","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2012-04-21T17:59:35Z","receivedAt":"2012-04-21T17:59:35Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 18/04/12 12:34, Pete Wyckoff wrote:\n> luke@diamand.org wrote on Wed, 11 Apr 2012 17:21 +0200:\n>> The existing label import code looks at each commit being\n>> imported, and then checks for labels at that commit. This\n>> doesn't work in the real world though because it will drop\n>> labels applied on changelists that have already been imported,\n>> a common pattern.\n>>\n>> This change adds a new --import-labels option. With this option,\n>> at the end of the sync, git p4 gets sets of labels in p4 and git,\n>> and then creates a git tag for each missing p4 label.\n>>\n>> This means that tags created on older changelists are\n>> still imported.\n>>\n>> Tags that could not be imported are added to an ignore\n>> list.\n>>\n>> The same sets of git and p4 tags and labels can also be used to\n>> derive a list of git tags to export to p4. This is enabled with\n>> --export-labels in 'git p4 submit'.\n>\n> This is a good approach.  Here's some late review comments.\n>\n>> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n>\n>> +git-p4.validLabelRegexp::\n>> +\tOnly p4 labels matching this regular expression will be imported. The\n>> +\tdefault value is '[A-Z0-9_\\-.]+$'.\n>> +\n>\n> and\n>\n>> +git-p4.validLabelRegexp::\n>> +\tOnly p4 labels matching this regular expression will be exported. The\n>> +\tdefault value is '[A-Z0-9_\\-.]+$'.\n>\n> This one wants to be validTagRegexp.  Or you could combine them\n> both into one.\n\nI'll change to \"validLabelExportRegexp\" and \"validLabelImportRegexp\". I \ndon't want to use the word \"tag\" as I think that gets confusing. The \noriginal \"detect-labels\" uses the p4 naming, and right or wrong, I guess \nwe should stick with that choice.\n\n >  Why no small a-z characters?\n\nBeing very lazy, this happens to work well for me P\n\n>\n>> diff --git a/git-p4.py b/git-p4.py\n>\n>> +            # Get the p4 commit this corresponds to\n>> +            changelist = None\n>> +            for l in read_pipe_lines([\"git\", \"log\", \"--max-count=1\", name]):\n>> +                match = commit_re.match(l)\n>> +                if match:\n>> +                    changelist = match.group(1)\n>\n> We have extractLogMessageFromGitCommit and extractSettingsGitLog\n> to grep out the \"git-p4.. change\" tag.  They're not beautiful,\n> but we should reuse them, in case this mechanism of connecting\n> changes to commits ever changes.\n\nok.\n\n>\n>> +            # Get the tag details.\n>> +            inHeader = True\n>> +            isAnnotated = False\n>> +            body = []\n>> +            for l in read_pipe_lines([\"git\", \"cat-file\", \"-p\", name]):\n>> +                l = l.strip()\n>> +                if inHeader:\n>> +                    if re.match(r'tag\\s+', l):\n>> +                        isAnnotated = True\n>> +                    elif re.match(r'\\s*$', l):\n>> +                        inHeader = False\n>> +                        continue\n>> +                else:\n>> +                    body.append(l)\n>> +\n>> +            if not isAnnotated:\n>> +                body = [\"lightweight tag imported by git p4\\n\"]\n>\n> The manual parsing, just to get the text in the tag (or ref),\n> seems a bit awkward.  I looked at \"git show --pretty=format:%B\"\n> as a way to get just the text, and \"git cat-file -t ref\" to get\n> the tag/ref difference.  But no easy replacement.\n\nYes, slightly surprising that there's no way to do this other than \nparsing the output of git cat-file.\n\n>\n>> +            if change.has_key('change'):\n>> +                # find the corresponding git commit; take the oldest commit\n>> +                changelist = int(change['change'])\n>> +                gitCommit = read_pipe([\"git\", \"rev-list\", \"--max-count=1\",\n>> +                     \"--reverse\", \":/\\[git-p4:.*change = %d\\]\" % changelist])\n>> +                if len(gitCommit) == 0:\n>> +                    print \"could not find git commit for changelist %d\" % changelist\n>> +                else:\n>> +                    gitCommit = gitCommit.strip()\n>> +                    commitFound = True\n>> +                    # Convert from p4 time format\n>> +                    try:\n>> +                        tmwhen = time.strptime(labelDetails['Update'], \"%Y/%m/%d %H:%M:%S\")\n>> +                    except ValueError:\n>> +                        print \"Could not convert label time %s\" % labelDetail['Update']\n>> +                        tmwhen = 1\n>> +\n>> +                    when = int(time.mktime(tmwhen))\n>> +                    self.streamTag(stream, name, labelDetails, gitCommit, when)\n>> +                    if verbose:\n>> +                        print \"p4 label %s mapped to git commit %s\" % (name, gitCommit)\n>\n> Nice, even the icky but required p4 time parsing.  We don't have\n> a common function to go from change ->  commit yet.\n>\n>>   class P4Rebase(Command):\n>>       def __init__(self):\n>>           Command.__init__(self)\n>> -        self.options = [ ]\n>> +        self.options = [\n>> +                optparse.make_option(\"--import-labels\", dest=\"importLabels\", action=\"store_true\"),\n>> +                optparse.make_option(\"--verbose\", dest=\"verbose\", action=\"store_true\"),\n>> +        ]\n>> +        self.verbose = False\n>> +        self.importLabels = False\n>>           self.description = (\"Fetches the latest revision from perforce and \"\n>>                               + \"rebases the current work (branch) against it\")\n>> -        self.verbose = False\n>\n> All commands should have a --verbose.  Could you move the\n> \"--verbose\" description in the man page up into \"General\n> options\"?  Since it means the same thing in all the commands,\n> roughly.\n\nOK.\n\n>\n>>       def run(self, args):\n>>           sync = P4Sync()\n>> +        sync.importLabels = self.importLabels\n>>           sync.run([])\n>\n> But no \"sync.verbose = self.verbose\"?  I wonder if P4Rebase should\n> inherit from P4Sync, like Clone does.  But that is a bit obscure\n> to me already.  Probably these all want to be split out into a\n> bunch of functions; there isn't a lot of state tracking happening\n> in the classes, and only one instance of each per invocation.\n\nOr push the 'verbose' up into the Command base class?\n\n>\n>> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\n>> new file mode 100755\n>> index 0000000..85d6049\n>> --- /dev/null\n>> +++ b/t/t9811-git-p4-label-import.sh\n>> @@ -0,0 +1,202 @@\n>> +#!/bin/sh\n>> +\n>> +test_description='git p4 label tests'\n>\n> This whole set of tests can go it the existing t9804, right?  It\n> seems that the first few of these are duplicates of what is\n> already in there.\n>\n> Nice test coverage, and you fixed the broken one in t9804.\n>\n> \t\t-- Pete\n\nOK, I'll put these in t9804.\n\nThanks!\nLuke\n"}]}