{"thread":{"id":"26382","subject":"[PATCH] Support different branch layouts in git-p4","startedAt":"2011-02-01T22:59:52Z","lastAt":"2011-02-10T13:43:06Z","messageCount":7,"participants":["Ian Wienand","Tor Arvid Lund","Pete Wyckoff"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"160236","messageId":"4D489068.2040704@vmware.com","threadId":"26382","inReplyTo":null,"subject":"[PATCH] Support different branch layouts in git-p4","fromName":"Ian Wienand","fromEmail":"ianw@vmware.com","sentAt":"2011-02-01T22:59:52Z","receivedAt":"2011-02-01T22:59:52Z","isPatch":true,"sender":{"key":"ianw@vmware.com","avatar":null},"body":"Hi,\n\nI think the addition to the git-p4.txt in the diff explains the\nreasoning behind the patch best.  In short, we have a repository\nlayout\n\n//depot/foo/branch\n//depot/moo/branch\n\nwhere we require projects 'foo' and 'moo' to be alongside each other.\nWe can do this with p4 views, but currently have to have 'foo' and\n'moo' in separate git repos.\n\nThis just munges the incoming paths to either put the branch as the\ntop level directory, or just remove it entirely if you don't need it.\n\nI've tested it locally, but I don't really have a wide variety of p4\nenvironments to expose it too.\n\n-i\n\nSigned-off-by: Ian Wienand <ianw@vmware.com>\n---\n contrib/fast-import/git-p4     |   35 +++++++++++++++++++++++-\n contrib/fast-import/git-p4.txt |   58 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 92 insertions(+), 1 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 04ce7e3..4bd40f8 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -848,6 +848,10 @@ class P4Sync(Command):\n                 optparse.make_option(\"--max-changes\", dest=\"maxChanges\"),\n                 optparse.make_option(\"--keep-path\", dest=\"keepRepoPath\", action='store_true',\n                                      help=\"Keep entire BRANCH/DIR/SUBDIR prefix during import\"),\n+                optparse.make_option(\"--branch-path\", dest=\"branchPath\", type='choice',\n+                                     choices=('none', 'first'),\n+                                     default=None,\n+                                     help=\"Remove the branch dir (none) or move it above project dir (first)\"),\n                 optparse.make_option(\"--use-client-spec\", dest=\"useClientSpec\", action='store_true',\n                                      help=\"Only sync files that are included in the Perforce Client Spec\")\n         ]\n@@ -917,6 +921,20 @@ class P4Sync(Command):\n             if path.startswith(p):\n                 path = path[len(p):]\n \n+        # reorg to move/remove branch from the output filename -- kind\n+        # of like how you can set your view in your p4 client\n+        if self.keepRepoPath and self.branchPath == 'first':\n+            # move the second element first, so what was was\n+            # \"//depot/proj/branch/file\" becomes \"branch/proj/file\".\n+            path = re.sub(\"^([^/]+/)([^/]+/)\", r'\\2\\1', path)\n+        elif self.keepRepoPath and self.branchPath == 'none':\n+            # remove the second element, so what was\n+            # \"//depot/proj/branch/file\" becomes \"proj/file\"\n+            path = re.sub(\"^([^/]+/)([^/]+/)\", r'\\2', path)\n+        elif self.branchPath:\n+            sys.stderr.write(\"branchPath without keepRepoPath?\")\n+            sys.exit(1)\n+\n         return path\n \n     def splitFilesIntoBranches(self, commit):\n@@ -940,7 +958,6 @@ class P4Sync(Command):\n             relPath = self.stripRepoPath(path, self.depotPaths)\n \n             for branch in self.knownBranches.keys():\n-\n                 # add a trailing slash so that a commit into qt/4.2foo doesn't end up in qt/4.2\n                 if relPath.startswith(branch + \"/\"):\n                     if branch not in branches:\n@@ -1283,12 +1300,24 @@ class P4Sync(Command):\n         if self.keepRepoPath:\n             option_keys['keepRepoPath'] = 1\n \n+        # since we're just saving the dict keys, append the branchPath\n+        # option to the key\n+        if self.branchPath:\n+            option_keys['branchPath_%s' % self.branchPath] = 1\n+\n         d[\"options\"] = ' '.join(sorted(option_keys.keys()))\n \n     def readOptions(self, d):\n         self.keepRepoPath = (d.has_key('options')\n                              and ('keepRepoPath' in d['options']))\n \n+        # restore the branchpath option; is one of \"none\" and \"first\"\n+        if (d.has_key('options')):\n+            if ('branchPath_none' in d['options']):\n+                self.branchPath = 'none'\n+            elif ('branchPath_first' in d['options']):\n+                self.branchPath = 'first'\n+\n     def gitRefForBranch(self, branch):\n         if branch == \"main\":\n             return self.refPrefix + \"master\"\n@@ -1775,6 +1804,10 @@ class P4Clone(P4Sync):\n             sys.stderr.write(\"Must specify destination for --keep-path\\n\")\n             sys.exit(1)\n \n+        if self.branchPath and not self.keepRepoPath:\n+            sys.stderr.write(\"Must specify --keep-path for --branch-path\\n\")\n+            sys.exit(1)\n+\n         depotPaths = args\n \n         if not self.cloneDestination and len(depotPaths) > 1:\ndiff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt\nindex 49b3359..669c63c 100644\n--- a/contrib/fast-import/git-p4.txt\n+++ b/contrib/fast-import/git-p4.txt\n@@ -191,6 +191,64 @@ git-p4.useclientspec\n \n   git config [--global] git-p4.useclientspec false\n \n+Dealing with different repository layouts\n+=========================================\n+\n+Perforce clients can map views of projects and branches in different\n+ways which your build system may rely on.  Say your code is organised\n+as two projects \"foo\" and \"moo\" which have a common branch\n+\n+//depot/foo/branch/...\n+//depot/moo/branch/...\n+\n+and you require both \"foo\" and \"moo\" projects in your git repository,\n+there are several options.\n+\n+Firstly, you could simply clone each project as a completely separate\n+git tree.  However, if the two projects are dependent on each other\n+this can be annoying for both sync -- you must remember to sync both\n+\"foo\" and \"moo\" to keep everything consistent -- and submit -- a\n+change that should logically be a single changeset across \"foo\" and\n+\"moo\" will have to be broken up (breaking bisection too).\n+\n+Another option is to simply specify multiple depots\n+\n+ git p4 sync //depot/foo/branch //depot/moo/branch\n+\n+which will import \"foo\" and \"moo\" into the same directory.\n+\n+To keep the projects separate, the --keep-path option used as\n+\n+ git p4 sync --keep-path --destination /tmp/boo/ //depot/foo/branch //depot/moo/branch\n+\n+will create a layout of\n+\n+ /tmp/boo/foo/branch/...\n+ /tmp/boo/moo/branch/...\n+\n+However, some build systems may rely on p4's ability to specify\n+destinations for views in your client.  The --branch-path flag, which\n+requires the --keep-path flag, allows two additional layout options.\n+\n+ git p4 sync --keep-path --destination /tmp/boo --branch-path=none //depot/foo/branch //depot/moo/branch\n+\n+will remove the branch name entirely, leaving you with a directory\n+that looks like\n+\n+ /tmp/boo/foo/...\n+ /tmp/boo/moo/...\n+\n+and\n+\n+ git p4 sync --keep-path --destination /tmp/boo --branch-path=first //depot/foo/branch //depot/moo/branch\n+\n+will give you each of the projects under a directory named for their\n+common branch\n+\n+ /tmp/boo/branch/foo/...\n+ /tmp/boo/branch/moo/...\n+\n+\n Implementation Details...\n =========================\n \n-- \n1.7.2.3\n"},{"id":"160416","messageId":"AANLkTi=ozDk9SqYaYWKHXSjVChV-93-88F_LUCwfSiDc@mail.gmail.com","threadId":"26382","inReplyTo":"4D489068.2040704@vmware.com","subject":"Re: [PATCH] Support different branch layouts in git-p4","fromName":"Tor Arvid Lund","fromEmail":"torarvid@gmail.com","sentAt":"2011-02-05T00:37:54Z","receivedAt":"2011-02-05T00:37:54Z","isPatch":true,"sender":{"key":"torarvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/439758?v=4"},"body":"On Tue, Feb 1, 2011 at 11:59 PM, Ian Wienand <ianw@vmware.com> wrote:\n> Hi,\n>\n> I think the addition to the git-p4.txt in the diff explains the\n> reasoning behind the patch best.  In short, we have a repository\n> layout\n>\n> //depot/foo/branch\n> //depot/moo/branch\n>\n> where we require projects 'foo' and 'moo' to be alongside each other.\n> We can do this with p4 views, but currently have to have 'foo' and\n> 'moo' in separate git repos.\n>\n> This just munges the incoming paths to either put the branch as the\n> top level directory, or just remove it entirely if you don't need it.\n\nHi, Ian! We haven't met, but thank you for the patch, and for trying\nto help make git better.\n\nNow, I have to say that I don't particularly like it, and here's why I\nwill vote against this patch:\n\nFor starters, I don't think that I like git-p4 being taught to solve\nproblems that seem to be caused by a poor/unfortunate perforce layout.\nEspecially since *your* type of poor perforce layout will probably be\npoor in a very different way from the next guy with a poor layout :)\nFor instance, you have hard-coded that you replace the first and\nsecond directory name... It is very easy to imagine people having\ndeeper trees than that...\n\nBut then I started thinking about it a bit more... It was me who added\nthe --use-client-spec option back in the day. The support for that\nstuff really should be better than what I made at the time. The\nclient-spec format contains lines like\n\n//depot/...   //local-root/...\n-//depot/dontcare/...   //local-root/dontcare/...\n//depot/foo/branch/...   //local-root/branch/foo/...\n//depot/moo/branch/...   //local-root/branch/moo/...\n\nPlease observe that the two last lines look like what I think *your*\nclient-spec should look like. This would map the foo/branch and\nmoo/branch in the perforce depot to branch/foo and branch/moo on the\nclient side.\n\nOf course, today this will not work with git-p4 clone. The\n--use-client-spec option, as I implemented it, simply filters out all\nthat stuff that matches the pattern lines that starts with \"-\". So the\nnames of all files will match the patterns on the left-hand side in\nthe client-spec. A solution which I think would work well for\neveryone, is if files would be placed according to the right-hand\npatterns in the client-spec.\n\nThat should be a much more elegant and generic solution. Whatcha\nthink? If you want to take a whack at hacking that into place, I will\nhelp guide the way if needed (if others are not opposed to such an\nidea) :)\n\n    -- Tor Arvid\n\n> I've tested it locally, but I don't really have a wide variety of p4\n> environments to expose it too.\n>\n> -i\n>\n> Signed-off-by: Ian Wienand <ianw@vmware.com>\n> ---\n>  contrib/fast-import/git-p4     |   35 +++++++++++++++++++++++-\n>  contrib/fast-import/git-p4.txt |   58 ++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 92 insertions(+), 1 deletions(-)\n>\n> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\n> index 04ce7e3..4bd40f8 100755\n> --- a/contrib/fast-import/git-p4\n> +++ b/contrib/fast-import/git-p4\n> @@ -848,6 +848,10 @@ class P4Sync(Command):\n>                 optparse.make_option(\"--max-changes\", dest=\"maxChanges\"),\n>                 optparse.make_option(\"--keep-path\", dest=\"keepRepoPath\", action='store_true',\n>                                      help=\"Keep entire BRANCH/DIR/SUBDIR prefix during import\"),\n> +                optparse.make_option(\"--branch-path\", dest=\"branchPath\", type='choice',\n> +                                     choices=('none', 'first'),\n> +                                     default=None,\n> +                                     help=\"Remove the branch dir (none) or move it above project dir (first)\"),\n>                 optparse.make_option(\"--use-client-spec\", dest=\"useClientSpec\", action='store_true',\n>                                      help=\"Only sync files that are included in the Perforce Client Spec\")\n>         ]\n> @@ -917,6 +921,20 @@ class P4Sync(Command):\n>             if path.startswith(p):\n>                 path = path[len(p):]\n>\n> +        # reorg to move/remove branch from the output filename -- kind\n> +        # of like how you can set your view in your p4 client\n> +        if self.keepRepoPath and self.branchPath == 'first':\n> +            # move the second element first, so what was was\n> +            # \"//depot/proj/branch/file\" becomes \"branch/proj/file\".\n> +            path = re.sub(\"^([^/]+/)([^/]+/)\", r'\\2\\1', path)\n> +        elif self.keepRepoPath and self.branchPath == 'none':\n> +            # remove the second element, so what was\n> +            # \"//depot/proj/branch/file\" becomes \"proj/file\"\n> +            path = re.sub(\"^([^/]+/)([^/]+/)\", r'\\2', path)\n> +        elif self.branchPath:\n> +            sys.stderr.write(\"branchPath without keepRepoPath?\")\n> +            sys.exit(1)\n> +\n>         return path\n>\n>     def splitFilesIntoBranches(self, commit):\n> @@ -940,7 +958,6 @@ class P4Sync(Command):\n>             relPath = self.stripRepoPath(path, self.depotPaths)\n>\n>             for branch in self.knownBranches.keys():\n> -\n>                 # add a trailing slash so that a commit into qt/4.2foo doesn't end up in qt/4.2\n>                 if relPath.startswith(branch + \"/\"):\n>                     if branch not in branches:\n> @@ -1283,12 +1300,24 @@ class P4Sync(Command):\n>         if self.keepRepoPath:\n>             option_keys['keepRepoPath'] = 1\n>\n> +        # since we're just saving the dict keys, append the branchPath\n> +        # option to the key\n> +        if self.branchPath:\n> +            option_keys['branchPath_%s' % self.branchPath] = 1\n> +\n>         d[\"options\"] = ' '.join(sorted(option_keys.keys()))\n>\n>     def readOptions(self, d):\n>         self.keepRepoPath = (d.has_key('options')\n>                              and ('keepRepoPath' in d['options']))\n>\n> +        # restore the branchpath option; is one of \"none\" and \"first\"\n> +        if (d.has_key('options')):\n> +            if ('branchPath_none' in d['options']):\n> +                self.branchPath = 'none'\n> +            elif ('branchPath_first' in d['options']):\n> +                self.branchPath = 'first'\n> +\n>     def gitRefForBranch(self, branch):\n>         if branch == \"main\":\n>             return self.refPrefix + \"master\"\n> @@ -1775,6 +1804,10 @@ class P4Clone(P4Sync):\n>             sys.stderr.write(\"Must specify destination for --keep-path\\n\")\n>             sys.exit(1)\n>\n> +        if self.branchPath and not self.keepRepoPath:\n> +            sys.stderr.write(\"Must specify --keep-path for --branch-path\\n\")\n> +            sys.exit(1)\n> +\n>         depotPaths = args\n>\n>         if not self.cloneDestination and len(depotPaths) > 1:\n> diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt\n> index 49b3359..669c63c 100644\n> --- a/contrib/fast-import/git-p4.txt\n> +++ b/contrib/fast-import/git-p4.txt\n> @@ -191,6 +191,64 @@ git-p4.useclientspec\n>\n>   git config [--global] git-p4.useclientspec false\n>\n> +Dealing with different repository layouts\n> +=========================================\n> +\n> +Perforce clients can map views of projects and branches in different\n> +ways which your build system may rely on.  Say your code is organised\n> +as two projects \"foo\" and \"moo\" which have a common branch\n> +\n> +//depot/foo/branch/...\n> +//depot/moo/branch/...\n> +\n> +and you require both \"foo\" and \"moo\" projects in your git repository,\n> +there are several options.\n> +\n> +Firstly, you could simply clone each project as a completely separate\n> +git tree.  However, if the two projects are dependent on each other\n> +this can be annoying for both sync -- you must remember to sync both\n> +\"foo\" and \"moo\" to keep everything consistent -- and submit -- a\n> +change that should logically be a single changeset across \"foo\" and\n> +\"moo\" will have to be broken up (breaking bisection too).\n> +\n> +Another option is to simply specify multiple depots\n> +\n> + git p4 sync //depot/foo/branch //depot/moo/branch\n> +\n> +which will import \"foo\" and \"moo\" into the same directory.\n> +\n> +To keep the projects separate, the --keep-path option used as\n> +\n> + git p4 sync --keep-path --destination /tmp/boo/ //depot/foo/branch //depot/moo/branch\n> +\n> +will create a layout of\n> +\n> + /tmp/boo/foo/branch/...\n> + /tmp/boo/moo/branch/...\n> +\n> +However, some build systems may rely on p4's ability to specify\n> +destinations for views in your client.  The --branch-path flag, which\n> +requires the --keep-path flag, allows two additional layout options.\n> +\n> + git p4 sync --keep-path --destination /tmp/boo --branch-path=none //depot/foo/branch //depot/moo/branch\n> +\n> +will remove the branch name entirely, leaving you with a directory\n> +that looks like\n> +\n> + /tmp/boo/foo/...\n> + /tmp/boo/moo/...\n> +\n> +and\n> +\n> + git p4 sync --keep-path --destination /tmp/boo --branch-path=first //depot/foo/branch //depot/moo/branch\n> +\n> +will give you each of the projects under a directory named for their\n> +common branch\n> +\n> + /tmp/boo/branch/foo/...\n> + /tmp/boo/branch/moo/...\n> +\n> +\n>  Implementation Details...\n>  =========================\n>\n> --\n> 1.7.2.3\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n>\n"},{"id":"160563","messageId":"4D4F3738.7010603@vmware.com","threadId":"26382","inReplyTo":"AANLkTi=ozDk9SqYaYWKHXSjVChV-93-88F_LUCwfSiDc@mail.gmail.com","subject":"Re: [PATCH] Support different branch layouts in git-p4","fromName":"Ian Wienand","fromEmail":"ianw@vmware.com","sentAt":"2011-02-07T00:05:12Z","receivedAt":"2011-02-07T00:05:12Z","isPatch":true,"sender":{"key":"ianw@vmware.com","avatar":null},"body":"Thanks for taking a look\n\nOn 04/02/11 16:37, Tor Arvid Lund wrote:\n> For starters, I don't think that I like git-p4 being taught to solve\n> problems that seem to be caused by a poor/unfortunate perforce layout.\n\nI do think this //depot/project/branch type layout is pretty typical,\nalthough I admit I don't have a lot of experience with alternative p4\nsetups.\n\n> A solution which I think would work well for everyone, is if files\n> would be placed according to the right-hand patterns in the\n> client-spec.\n\nI did consider this at first.  My only issue is that it is a bit\nconfusing to use the client spec for filtering (and in this case\nre-writing), but not for actually selecting the depots to clone, which\nI still need to replicate on the command line.  However that is a much\nlarger change.\n\nWhat do you think of this one?\n\nIn this case, my client view is\n\n//depot/project/branch/...  //client/branch/project/...\n//depot/project2/branch/...  //client/branch/project2/...\n\nand my git directory layout ends up as\n\nbranch/project/...\nbranch/project2/...\n\n-i\n\n---\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 04ce7e3..eb9620c 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -878,6 +878,7 @@ class P4Sync(Command):\n         self.cloneExclude = []\n         self.useClientSpec = False\n         self.clientSpecDirs = []\n+        self.clientName = None\n \n         if gitConfig(\"git-p4.syncFromOrigin\") == \"false\":\n             self.syncWithOrigin = False\n@@ -910,6 +911,22 @@ class P4Sync(Command):\n         return files\n \n     def stripRepoPath(self, path, prefixes):\n+        if self.useClientSpec:\n+\n+            # if using the client spec, we use the output directory\n+            # specified in the client.  For example, a view\n+            #   //depot/foo/branch/... //client/branch/foo/...\n+            # will end up putting all foo/branch files into\n+            #  branch/foo/\n+            for val in self.clientSpecDirs:\n+                if path.startswith(val[0]):\n+                    # replace the depot path with the client path\n+                    path = path.replace(val[0], val[1][1])\n+                    # now strip out the client (//client/...)\n+                    path = re.sub(\"^(//[^/]+/)\", '', path)\n+                    # the rest is all path\n+                    return path\n+\n         if self.keepRepoPath:\n             prefixes = [re.sub(\"^(//[^/]+/).*\", r'\\1', prefixes[0])]\n \n@@ -1032,7 +1049,7 @@ class P4Sync(Command):\n             includeFile = True\n             for val in self.clientSpecDirs:\n                 if f['path'].startswith(val[0]):\n-                    if val[1] <= 0:\n+                    if val[1][0] <= 0:\n                         includeFile = False\n                     break\n \n@@ -1474,20 +1491,36 @@ class P4Sync(Command):\n         temp = {}\n         for entry in specList:\n             for k,v in entry.iteritems():\n+                if k.startswith(\"Client\"):\n+                    self.clientName = v\n+            \n                 if k.startswith(\"View\"):\n                     if v.startswith('\"'):\n                         start = 1\n                     else:\n                         start = 0\n                     index = v.find(\"...\")\n+\n+                    # save the \"client view\"; i.e the RHS of the view\n+                    # line that tells the client where to put the\n+                    # files for this view.\n+                    cv = v[index+4:] # +4 to remove previous '... '\n+                    cv_index = cv.find(\"...\")\n+                    cv=cv[:cv_index]\n+\n+                    # now save the view; +index means included, -index\n+                    # means it should be filtered out.\n                     v = v[start:index]\n                     if v.startswith(\"-\"):\n                         v = v[1:]\n-                        temp[v] = -len(v)\n+                        include = -len(v)\n                     else:\n-                        temp[v] = len(v)\n+                        include = len(v)\n+\n+                    temp[v] = (include, cv)\n+\n         self.clientSpecDirs = temp.items()\n-        self.clientSpecDirs.sort( lambda x, y: abs( y[1] ) - abs( x[1] ) )\n+        self.clientSpecDirs.sort( lambda x, y: abs( y[1][0] ) - abs( x[1][0] ) )\n \n     def run(self, args):\n         self.depotPaths = []\ndiff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt\nindex 49b3359..e09da44 100644\n--- a/contrib/fast-import/git-p4.txt\n+++ b/contrib/fast-import/git-p4.txt\n@@ -191,6 +191,11 @@ git-p4.useclientspec\n \n   git config [--global] git-p4.useclientspec false\n \n+The P4CLIENT environment variable should be correctly set for p4 to be\n+able to find the relevant client.  This client spec will be used to\n+both filter the files cloned by git and set the directory layout as\n+specified in the client (this implies --keep-path style semantics).\n+\n Implementation Details...\n =========================\n \n"},{"id":"160655","messageId":"AANLkTimGKc4MTwb=AnZ_Bv1EGS7yfgrFupxBOVVSm4s8@mail.gmail.com","threadId":"26382","inReplyTo":"4D4F3738.7010603@vmware.com","subject":"Re: [PATCH] Support different branch layouts in git-p4","fromName":"Tor Arvid Lund","fromEmail":"torarvid@gmail.com","sentAt":"2011-02-07T23:27:12Z","receivedAt":"2011-02-07T23:27:12Z","isPatch":true,"sender":{"key":"torarvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/439758?v=4"},"body":"On Mon, Feb 7, 2011 at 1:05 AM, Ian Wienand <ianw@vmware.com> wrote:\n> On 04/02/11 16:37, Tor Arvid Lund wrote:\n>> For starters, I don't think that I like git-p4 being taught to solve\n>> problems that seem to be caused by a poor/unfortunate perforce layout.\n>\n> I do think this //depot/project/branch type layout is pretty typical,\n> although I admit I don't have a lot of experience with alternative p4\n> setups.\n\nYou may be right, although I suspect that\n//depot/department/project/branch may be equally typical. At my\n$dayjob, we have gone through several \"reorganizations\" of the\nperforce layouts. I'm the guy that never likes any of them ;)\n\n>> A solution which I think would work well for everyone, is if files\n>> would be placed according to the right-hand patterns in the\n>> client-spec.\n>\n> I did consider this at first.  My only issue is that it is a bit\n> confusing to use the client spec for filtering (and in this case\n> re-writing), but not for actually selecting the depots to clone, which\n> I still need to replicate on the command line.  However that is a much\n> larger change.\n>\n> What do you think of this one?\n\nIn general, me thinks me likes it :-)\n\n(and it turned out much smaller than I would have originally guessed)\n\nI should probably mention that I haven't tested your patch at all. I\nwill have a pretty rough week at work, so it would be great if anyone\nelse feels like chiming in on this one... But I have some quick\nobservations below:\n\n> ---\n>\n> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\n> index 04ce7e3..eb9620c 100755\n> --- a/contrib/fast-import/git-p4\n> +++ b/contrib/fast-import/git-p4\n> @@ -878,6 +878,7 @@ class P4Sync(Command):\n>         self.cloneExclude = []\n>         self.useClientSpec = False\n>         self.clientSpecDirs = []\n> +        self.clientName = None\n>\n>         if gitConfig(\"git-p4.syncFromOrigin\") == \"false\":\n>             self.syncWithOrigin = False\n> @@ -910,6 +911,22 @@ class P4Sync(Command):\n>         return files\n>\n>     def stripRepoPath(self, path, prefixes):\n> +        if self.useClientSpec:\n> +\n> +            # if using the client spec, we use the output directory\n> +            # specified in the client.  For example, a view\n> +            #   //depot/foo/branch/... //client/branch/foo/...\n> +            # will end up putting all foo/branch files into\n> +            #  branch/foo/\n> +            for val in self.clientSpecDirs:\n> +                if path.startswith(val[0]):\n> +                    # replace the depot path with the client path\n> +                    path = path.replace(val[0], val[1][1])\n> +                    # now strip out the client (//client/...)\n> +                    path = re.sub(\"^(//[^/]+/)\", '', path)\n> +                    # the rest is all path\n> +                    return path\n> +\n>         if self.keepRepoPath:\n>             prefixes = [re.sub(\"^(//[^/]+/).*\", r'\\1', prefixes[0])]\n>\n> @@ -1032,7 +1049,7 @@ class P4Sync(Command):\n>             includeFile = True\n>             for val in self.clientSpecDirs:\n>                 if f['path'].startswith(val[0]):\n> -                    if val[1] <= 0:\n> +                    if val[1][0] <= 0:\n>                         includeFile = False\n>                     break\n>\n> @@ -1474,20 +1491,36 @@ class P4Sync(Command):\n>         temp = {}\n>         for entry in specList:\n>             for k,v in entry.iteritems():\n> +                if k.startswith(\"Client\"):\n> +                    self.clientName = v\n> +\n>                 if k.startswith(\"View\"):\n>                     if v.startswith('\"'):\n>                         start = 1\n>                     else:\n>                         start = 0\n>                     index = v.find(\"...\")\n> +\n> +                    # save the \"client view\"; i.e the RHS of the view\n> +                    # line that tells the client where to put the\n> +                    # files for this view.\n> +                    cv = v[index+4:] # +4 to remove previous '... '\n\nThis feels less robust than what we might want. Isn't the format of a\nclient-spec line either:\n\n-?//depot/path[/...]\\s+//client/path[/...]\\n\n\nor\n\n-?\"//depot/path with spaces/path[/...]\"\\s+\"//client/path with spaces/path[/...]\"\n\n.. where -? means an optional '-' char, and \\s+ is\n'whatever-length-and-kind-of-whitespace'. I'm just guessing from\nmemory regarding these patterns, but assuming that the section\nseparator is exactly the string '... ' seems risky, no? :)\n\n> +                    cv_index = cv.find(\"...\")\n> +                    cv=cv[:cv_index]\n\nWhat if a line doesn't end with \"...\" ? Maybe add an \"if cv_index >= 0\"\n\n> +\n> +                    # now save the view; +index means included, -index\n> +                    # means it should be filtered out.\n>                     v = v[start:index]\n>                     if v.startswith(\"-\"):\n>                         v = v[1:]\n> -                        temp[v] = -len(v)\n> +                        include = -len(v)\n>                     else:\n> -                        temp[v] = len(v)\n> +                        include = len(v)\n> +\n> +                    temp[v] = (include, cv)\n> +\n>         self.clientSpecDirs = temp.items()\n> -        self.clientSpecDirs.sort( lambda x, y: abs( y[1] ) - abs( x[1] ) )\n> +        self.clientSpecDirs.sort( lambda x, y: abs( y[1][0] ) - abs( x[1][0] ) )\n>\n>     def run(self, args):\n>         self.depotPaths = []\n> diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt\n> index 49b3359..e09da44 100644\n> --- a/contrib/fast-import/git-p4.txt\n> +++ b/contrib/fast-import/git-p4.txt\n> @@ -191,6 +191,11 @@ git-p4.useclientspec\n>\n>   git config [--global] git-p4.useclientspec false\n>\n> +The P4CLIENT environment variable should be correctly set for p4 to be\n> +able to find the relevant client.  This client spec will be used to\n> +both filter the files cloned by git and set the directory layout as\n> +specified in the client (this implies --keep-path style semantics).\n> +\n>  Implementation Details...\n>  =========================\n>\n>\n\nAight. I need some sleep now. Nice work so far, Ian! :)\n\n    -- Tor Arvid\n"},{"id":"160671","messageId":"20110208012208.GA28329@arf.padd.com","threadId":"26382","inReplyTo":"4D4F3738.7010603@vmware.com","subject":"Re: [PATCH] Support different branch layouts in git-p4","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2011-02-08T01:22:08Z","receivedAt":"2011-02-08T01:22:08Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"ianw@vmware.com wrote on Sun, 06 Feb 2011 16:05 -0800:\n> I did consider this at first.  My only issue is that it is a bit\n> confusing to use the client spec for filtering (and in this case\n> re-writing), but not for actually selecting the depots to clone, which\n> I still need to replicate on the command line.  However that is a much\n> larger change.\n> \n> What do you think of this one?\n> \n> In this case, my client view is\n> \n> //depot/project/branch/...  //client/branch/project/...\n> //depot/project2/branch/...  //client/branch/project2/...\n> \n> and my git directory layout ends up as\n> \n> branch/project/...\n> branch/project2/...\n\nWe had such terrible p4 mappings too, before the last\nrearrangement put us into a single-line view spec.  I think\nit would help others to include such support, though.\n\nBack then, I hacked together similar code to deal with the\nannoyance.  My code was not pretty and not complete, either.\n\nIf you look at \"p4 help views\", they have lots of oddities\nthat in theory should be accounted for here.  It doesn't\neven mention the thing about quotes, but obviously that is\nsupported.  Wildcards ... and * can appear multiple\ntimes.  And %%[1-9] can be used to reorder the path.  Also the\norder of lines matters, and \"+\" can be used to merge entries.\nWhew.\n\nIn practice, I think you get most everything we care about.  A\nfew comments below, beyond the bits that Tor Arvid caught.\n\n\t\t-- Pete\n\n> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\n> index 04ce7e3..eb9620c 100755\n> --- a/contrib/fast-import/git-p4\n> +++ b/contrib/fast-import/git-p4\n> @@ -878,6 +878,7 @@ class P4Sync(Command):\n>          self.cloneExclude = []\n>          self.useClientSpec = False\n>          self.clientSpecDirs = []\n> +        self.clientName = None\n\nUnused.\n\n>          if gitConfig(\"git-p4.syncFromOrigin\") == \"false\":\n>              self.syncWithOrigin = False\n> @@ -910,6 +911,22 @@ class P4Sync(Command):\n>          return files\n>  \n>      def stripRepoPath(self, path, prefixes):\n> +        if self.useClientSpec:\n> +\n> +            # if using the client spec, we use the output directory\n> +            # specified in the client.  For example, a view\n> +            #   //depot/foo/branch/... //client/branch/foo/...\n> +            # will end up putting all foo/branch files into\n> +            #  branch/foo/\n> +            for val in self.clientSpecDirs:\n> +                if path.startswith(val[0]):\n> +                    # replace the depot path with the client path\n> +                    path = path.replace(val[0], val[1][1])\n> +                    # now strip out the client (//client/...)\n> +                    path = re.sub(\"^(//[^/]+/)\", '', path)\n> +                    # the rest is all path\n> +                    return path\n\nThat's clever.  Better than having to remember Client: and build\n//client/ out of it.  You could do this down in getClientSpec()\nso that val[1] starts with the git-relative path.\n\n>          if self.keepRepoPath:\n>              prefixes = [re.sub(\"^(//[^/]+/).*\", r'\\1', prefixes[0])]\n>  \n> @@ -1032,7 +1049,7 @@ class P4Sync(Command):\n>              includeFile = True\n>              for val in self.clientSpecDirs:\n>                  if f['path'].startswith(val[0]):\n> -                    if val[1] <= 0:\n> +                    if val[1][0] <= 0:\n>                          includeFile = False\n>                      break\n>  \n> @@ -1474,20 +1491,36 @@ class P4Sync(Command):\n>          temp = {}\n>          for entry in specList:\n>              for k,v in entry.iteritems():\n> +                if k.startswith(\"Client\"):\n> +                    self.clientName = v\n\nOh maybe here is where you thought you would need client, but\ndon't.\n\n>                  if k.startswith(\"View\"):\n>                      if v.startswith('\"'):\n>                          start = 1\n>                      else:\n>                          start = 0\n>                      index = v.find(\"...\")\n> +\n> +                    # save the \"client view\"; i.e the RHS of the view\n> +                    # line that tells the client where to put the\n> +                    # files for this view.\n> +                    cv = v[index+4:] # +4 to remove previous '... '\n> +                    cv_index = cv.find(\"...\")\n> +                    cv=cv[:cv_index]\n> +\n> +                    # now save the view; +index means included, -index\n> +                    # means it should be filtered out.\n>                      v = v[start:index]\n>                      if v.startswith(\"-\"):\n>                          v = v[1:]\n> -                        temp[v] = -len(v)\n> +                        include = -len(v)\n>                      else:\n> -                        temp[v] = len(v)\n> +                        include = len(v)\n> +\n> +                    temp[v] = (include, cv)\n> +\n>          self.clientSpecDirs = temp.items()\n> -        self.clientSpecDirs.sort( lambda x, y: abs( y[1] ) - abs( x[1] ) )\n> +        self.clientSpecDirs.sort( lambda x, y: abs( y[1][0] ) - abs( x[1][0] ) )\n"},{"id":"160732","messageId":"4D520E2B.2080200@vmware.com","threadId":"26382","inReplyTo":"AANLkTimGKc4MTwb=AnZ_Bv1EGS7yfgrFupxBOVVSm4s8@mail.gmail.com","subject":"Re: [PATCH] Support different branch layouts in git-p4","fromName":"Ian Wienand","fromEmail":"ianw@vmware.com","sentAt":"2011-02-09T03:46:51Z","receivedAt":"2011-02-09T03:46:51Z","isPatch":true,"sender":{"key":"ianw@vmware.com","avatar":null},"body":"Thanks for the review\n\nOn 07/02/11 15:27, Tor Arvid Lund wrote:\n> I'm just guessing from memory regarding these patterns, but assuming\n> that the section separator is exactly the string '... ' seems risky,\n> no? :)\n\nGood point.  If we go past the end of the depot ...'s, then the rest\nof the line strip()ed should just be the client I guess.\n \n> What if a line doesn't end with \"...\" ? Maybe add an \"if cv_index>= 0\"\n\nI think it would mess us up.  I put in a failure case for this.\n\nOn 07/02/11 17:22, Pete Wyckoff wrote:\n> If you look at \"p4 help views\", they have lots of oddities\n> that in theory should be accounted for here.  It doesn't\n> even mention the thing about quotes, but obviously that is\n> supported.  Wildcards ... and * can appear multiple\n> times.  And %%[1-9] can be used to reorder the path.  Also the\n> order of lines matters, and \"+\" can be used to merge entries.\n> Whew.\n\nThose %%'s would also mess us up, I put in an escape hatch for that\ntoo.\n\nI'm pretty sure this covers the majority of cases; if people have\nreally weird clients I guess they're going to have to do some more\nwork to get git-p4 to recognise it properly.  Personally, I struggle\nto see why it is a feature to have every user re-organsing their views\nof the tree -- it seems to move a lot of uncaptured state to the\nclient side.  anyway...\n\nSo here's another version; I agree some testing would be good as I've\nonly run it locally on //depot/proj/branch clients\n\n-i\n\n---\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 04ce7e3..3304f36 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -910,6 +910,22 @@ class P4Sync(Command):\n         return files\n \n     def stripRepoPath(self, path, prefixes):\n+        if self.useClientSpec:\n+\n+            # if using the client spec, we use the output directory\n+            # specified in the client.  For example, a view\n+            #   //depot/foo/branch/... //client/branch/foo/...\n+            # will end up putting all foo/branch files into\n+            #  branch/foo/\n+            for val in self.clientSpecDirs:\n+                if path.startswith(val[0]):\n+                    # replace the depot path with the client path\n+                    path = path.replace(val[0], val[1][1])\n+                    # now strip out the client (//client/...)\n+                    path = re.sub(\"^(//[^/]+/)\", '', path)\n+                    # the rest is all path\n+                    return path\n+\n         if self.keepRepoPath:\n             prefixes = [re.sub(\"^(//[^/]+/).*\", r'\\1', prefixes[0])]\n \n@@ -1032,7 +1048,7 @@ class P4Sync(Command):\n             includeFile = True\n             for val in self.clientSpecDirs:\n                 if f['path'].startswith(val[0]):\n-                    if val[1] <= 0:\n+                    if val[1][0] <= 0:\n                         includeFile = False\n                     break\n \n@@ -1475,19 +1491,45 @@ class P4Sync(Command):\n         for entry in specList:\n             for k,v in entry.iteritems():\n                 if k.startswith(\"View\"):\n+\n+                    # p4 has these %%1 to %%9 arguments in specs to\n+                    # reorder paths; which we can't handle (yet :)\n+                    if re.match('\\%\\%d', v) != None:\n+                        print \"Sorry, can't handle %%n arguments in client specs\"\n+                        sys.exit(1)\n+\n                     if v.startswith('\"'):\n                         start = 1\n                     else:\n                         start = 0\n                     index = v.find(\"...\")\n+\n+                    # save the \"client view\"; i.e the RHS of the view\n+                    # line that tells the client where to put the\n+                    # files for this view.\n+                    cv = v[index+3:].strip() # +3 to remove previous '...'\n+\n+                    # if the client view doesn't end with a\n+                    # ... wildcard, then we're going to mess up the\n+                    # output directory, so fail gracefully.\n+                    if not cv.endswith('...'):\n+                        print 'Sorry, client view in \"%s\" needs to end with wildcard' % (k)\n+                        sys.exit(1)\n+                    cv=cv[:-3]\n+\n+                    # now save the view; +index means included, -index\n+                    # means it should be filtered out.\n                     v = v[start:index]\n                     if v.startswith(\"-\"):\n                         v = v[1:]\n-                        temp[v] = -len(v)\n+                        include = -len(v)\n                     else:\n-                        temp[v] = len(v)\n+                        include = len(v)\n+\n+                    temp[v] = (include, cv)\n+\n         self.clientSpecDirs = temp.items()\n-        self.clientSpecDirs.sort( lambda x, y: abs( y[1] ) - abs( x[1] ) )\n+        self.clientSpecDirs.sort( lambda x, y: abs( y[1][0] ) - abs( x[1][0] ) )\n \n     def run(self, args):\n         self.depotPaths = []\ndiff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt\nindex 49b3359..e09da44 100644\n--- a/contrib/fast-import/git-p4.txt\n+++ b/contrib/fast-import/git-p4.txt\n@@ -191,6 +191,11 @@ git-p4.useclientspec\n \n   git config [--global] git-p4.useclientspec false\n \n+The P4CLIENT environment variable should be correctly set for p4 to be\n+able to find the relevant client.  This client spec will be used to\n+both filter the files cloned by git and set the directory layout as\n+specified in the client (this implies --keep-path style semantics).\n+\n Implementation Details...\n =========================\n \n"},{"id":"160823","messageId":"20110210134306.GA4078@arf.padd.com","threadId":"26382","inReplyTo":"4D520E2B.2080200@vmware.com","subject":"Re: [PATCH] Support different branch layouts in git-p4","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2011-02-10T13:43:06Z","receivedAt":"2011-02-10T13:43:06Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"ianw@vmware.com wrote on Tue, 08 Feb 2011 19:46 -0800:\n> So here's another version; I agree some testing would be good as I've\n> only run it locally on //depot/proj/branch clients\n\nThis is good.  Thanks for fixing it up.  One last pedantic whine\nfrom me.  Fix the regex for the error case:\n\n    arf$ python\n    >>> import re\n    >>> re.match('\\%\\%d', \"%%3\")\n    >>> re.match(r'%%\\d', \"%%3\")\n    >>> <_sre.SRE_Match object at 0x1ec4168>\n\n\n\t\t-- Pete\n\n> ---\n> \n> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\n> index 04ce7e3..3304f36 100755\n> --- a/contrib/fast-import/git-p4\n> +++ b/contrib/fast-import/git-p4\n> @@ -910,6 +910,22 @@ class P4Sync(Command):\n>          return files\n>  \n>      def stripRepoPath(self, path, prefixes):\n> +        if self.useClientSpec:\n> +\n> +            # if using the client spec, we use the output directory\n> +            # specified in the client.  For example, a view\n> +            #   //depot/foo/branch/... //client/branch/foo/...\n> +            # will end up putting all foo/branch files into\n> +            #  branch/foo/\n> +            for val in self.clientSpecDirs:\n> +                if path.startswith(val[0]):\n> +                    # replace the depot path with the client path\n> +                    path = path.replace(val[0], val[1][1])\n> +                    # now strip out the client (//client/...)\n> +                    path = re.sub(\"^(//[^/]+/)\", '', path)\n> +                    # the rest is all path\n> +                    return path\n> +\n>          if self.keepRepoPath:\n>              prefixes = [re.sub(\"^(//[^/]+/).*\", r'\\1', prefixes[0])]\n>  \n> @@ -1032,7 +1048,7 @@ class P4Sync(Command):\n>              includeFile = True\n>              for val in self.clientSpecDirs:\n>                  if f['path'].startswith(val[0]):\n> -                    if val[1] <= 0:\n> +                    if val[1][0] <= 0:\n>                          includeFile = False\n>                      break\n>  \n> @@ -1475,19 +1491,45 @@ class P4Sync(Command):\n>          for entry in specList:\n>              for k,v in entry.iteritems():\n>                  if k.startswith(\"View\"):\n> +\n> +                    # p4 has these %%1 to %%9 arguments in specs to\n> +                    # reorder paths; which we can't handle (yet :)\n> +                    if re.match('\\%\\%d', v) != None:\n> +                        print \"Sorry, can't handle %%n arguments in client specs\"\n> +                        sys.exit(1)\n> +\n>                      if v.startswith('\"'):\n>                          start = 1\n>                      else:\n>                          start = 0\n>                      index = v.find(\"...\")\n> +\n> +                    # save the \"client view\"; i.e the RHS of the view\n> +                    # line that tells the client where to put the\n> +                    # files for this view.\n> +                    cv = v[index+3:].strip() # +3 to remove previous '...'\n> +\n> +                    # if the client view doesn't end with a\n> +                    # ... wildcard, then we're going to mess up the\n> +                    # output directory, so fail gracefully.\n> +                    if not cv.endswith('...'):\n> +                        print 'Sorry, client view in \"%s\" needs to end with wildcard' % (k)\n> +                        sys.exit(1)\n> +                    cv=cv[:-3]\n> +\n> +                    # now save the view; +index means included, -index\n> +                    # means it should be filtered out.\n>                      v = v[start:index]\n>                      if v.startswith(\"-\"):\n>                          v = v[1:]\n> -                        temp[v] = -len(v)\n> +                        include = -len(v)\n>                      else:\n> -                        temp[v] = len(v)\n> +                        include = len(v)\n> +\n> +                    temp[v] = (include, cv)\n> +\n>          self.clientSpecDirs = temp.items()\n> -        self.clientSpecDirs.sort( lambda x, y: abs( y[1] ) - abs( x[1] ) )\n> +        self.clientSpecDirs.sort( lambda x, y: abs( y[1][0] ) - abs( x[1][0] ) )\n>  \n>      def run(self, args):\n>          self.depotPaths = []\n> diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt\n> index 49b3359..e09da44 100644\n> --- a/contrib/fast-import/git-p4.txt\n> +++ b/contrib/fast-import/git-p4.txt\n> @@ -191,6 +191,11 @@ git-p4.useclientspec\n>  \n>    git config [--global] git-p4.useclientspec false\n>  \n> +The P4CLIENT environment variable should be correctly set for p4 to be\n> +able to find the relevant client.  This client spec will be used to\n> +both filter the files cloned by git and set the directory layout as\n> +specified in the client (this implies --keep-path style semantics).\n> +\n>  Implementation Details...\n>  =========================\n>  \n> \n"}]}