{"thread":{"id":"34623","subject":"[PATCH v2] git-p4: Ask \"p4\" to interpret View setting","startedAt":"2013-08-06T06:45:29Z","lastAt":"2013-08-29T22:40:37Z","messageCount":9,"participants":["kazuki saitoh","Pete Wyckoff"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"224652","messageId":"CACGba4zdA=3tBE9UR=i9P9kNAL1HUc3UwSHbYeq4s9fwaN4=Mw@mail.gmail.com","threadId":"34623","inReplyTo":null,"subject":"[PATCH v2] git-p4: Ask \"p4\" to interpret View setting","fromName":"kazuki saitoh","fromEmail":"ksaitoh560@gmail.com","sentAt":"2013-08-06T06:45:29Z","receivedAt":"2013-08-06T06:45:29Z","isPatch":true,"sender":{"key":"ksaitoh560@gmail.com","avatar":"https://avatars.githubusercontent.com/u/217076?v=4"},"body":"In Perforce, View setting of p4 client can describe\n  -//depot/project/files/*.xls //client/project/files/*.xls\nto exclude Excel files.\nBut \"git p4 --use-client-spec\" cannot support '*'.\n\nIn git-p4.py, \"map_in_client\" method analyzes View setting and return\nclient file path.\nSo I modify the method to just ask p4.\n\n\n> Let me play with this for a bit.  I wonder about the performance\n> aspects of doing a \"p4 fstat\" for every file.  Would it be\n> possible to do one or a few batch queries higher up somewhere?\nTo reduce p4 access, it cache result of asking \"client path\".\nAnd addition, \"fstat\" depends on sync status, so modify to use \"p4\nwhere\" instead of \"fstat\".\n\n\n\nSigned-off-by: KazukiSaitoh <ksaitoh560@gmail.com>\n---\n git-p4.py                     | 53 ++++++-----------------------------\n t/lib-git-p4.sh               |  1 +\n t/t9807-git-p4-submit.sh      |  2 +-\n t/t9809-git-p4-client-view.sh | 65 +++++++++++++++++++++++++++++++++++--------\n 4 files changed, 64 insertions(+), 57 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 31e71ff..8ec8eb4 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1819,15 +1819,6 @@ class View(object):\n                variables.\"\"\"\n\n             self.ends_triple_dot = False\n-            # There are three wildcards allowed in p4 views\n-            # (see \"p4 help views\").  This code knows how to\n-            # handle \"...\" (only at the end), but cannot deal with\n-            # \"%%n\" or \"*\".  Only check the depot_side, as p4 should\n-            # validate that the client_side matches too.\n-            if re.search(r'%%[1-9]', self.path):\n-                die(\"Can't handle %%n wildcards in view: %s\" % self.path)\n-            if self.path.find(\"*\") >= 0:\n-                die(\"Can't handle * wildcards in view: %s\" % self.path)\n             triple_dot_index = self.path.find(\"...\")\n             if triple_dot_index >= 0:\n                 if triple_dot_index != len(self.path) - 3:\n@@ -1903,6 +1894,7 @@ class View(object):\n     #\n     def __init__(self):\n         self.mappings = []\n+        self.client_spec_path_cache = {}  # Caching result of p4\nquery, use for \"--use-client-spec\".\n\n     def append(self, view_line):\n         \"\"\"Parse a view line, splitting it into depot and client\n@@ -1965,44 +1957,17 @@ class View(object):\n            depot file should live.  Returns \"\" if the file should\n            not be mapped in the client.\"\"\"\n\n-        paths_filled = []\n-        client_path = \"\"\n-\n-        # look at later entries first\n-        for m in self.mappings[::-1]:\n-\n-            # see where will this path end up in the client\n-            p = m.map_depot_to_client(depot_path)\n-\n-            if p == \"\":\n-                # Depot path does not belong in client.  Must remember\n-                # this, as previous items should not cause files to\n-                # exist in this path either.  Remember that the list is\n-                # being walked from the end, which has higher precedence.\n-                # Overlap mappings do not exclude previous mappings.\n-                if not m.overlay:\n-                    paths_filled.append(m.client_side)\n+        if self.client_spec_path_cache.has_key(depot_path):\n+            return self.client_spec_path_cache[depot_path]\n\n-            else:\n-                # This mapping matched; no need to search any further.\n-                # But, the mapping could be rejected if the client path\n-                # has already been claimed by an earlier mapping (i.e.\n-                # one later in the list, which we are walking backwards).\n-                already_mapped_in_client = False\n-                for f in paths_filled:\n-                    # this is View.Path.match\n-                    if f.match(p):\n-                        already_mapped_in_client = True\n-                        break\n-                if not already_mapped_in_client:\n-                    # Include this file, unless it is from a line that\n-                    # explicitly said to exclude it.\n-                    if not m.exclude:\n-                        client_path = p\n-\n-                # a match, even if rejected, always stops the search\n+        client_path = \"\"\n+        where_result = p4CmdList(['where', depot_path])\n+        for res in where_result:\n+            if res[\"code\"] != \"error\" and not res.has_key(\"unmap\"):\n+                client_path = res[\"path\"].replace(getClientRoot()+\"/\", \"\")\n                 break\n\n+        self.client_spec_path_cache[depot_path] = client_path\n         return client_path\n\n class P4Sync(Command, P4UserMap):\ndiff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh\nindex 2098b9b..0d631dc 100644\n--- a/t/lib-git-p4.sh\n+++ b/t/lib-git-p4.sh\n@@ -48,6 +48,7 @@ P4DPORT=$((10669 + ($testid - $git_p4_test_start)))\n P4PORT=localhost:$P4DPORT\n P4CLIENT=client\n P4EDITOR=:\n+P4CHARSET=\"\"\n export P4PORT P4CLIENT P4EDITOR\n\n db=\"$TRASH_DIRECTORY/db\"\ndiff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh\nindex 1fb7bc7..4caf36e 100755\n--- a/t/t9807-git-p4-submit.sh\n+++ b/t/t9807-git-p4-submit.sh\n@@ -17,7 +17,7 @@ test_expect_success 'init depot' '\n  )\n '\n\n-test_expect_failure 'is_cli_file_writeable function' '\n+test_expect_success 'is_cli_file_writeable function' '\n  (\n  cd \"$cli\" &&\n  echo a >a &&\ndiff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh\nindex 77f6349..160fd9a 100755\n--- a/t/t9809-git-p4-client-view.sh\n+++ b/t/t9809-git-p4-client-view.sh\n@@ -75,18 +75,6 @@ test_expect_success 'init depot' '\n  )\n '\n\n-# double % for printf\n-test_expect_success 'unsupported view wildcard %%n' '\n- client_view \"//depot/%%%%1/sub/... //client/sub/%%%%1/...\" &&\n- test_when_finished cleanup_git &&\n- test_must_fail git p4 clone --use-client-spec --dest=\"$git\" //depot\n-'\n-\n-test_expect_success 'unsupported view wildcard *' '\n- client_view \"//depot/*/bar/... //client/*/bar/...\" &&\n- test_when_finished cleanup_git &&\n- test_must_fail git p4 clone --use-client-spec --dest=\"$git\" //depot\n-'\n\n test_expect_success 'wildcard ... only supported at end of spec 1' '\n  client_view \"//depot/.../file11 //client/.../file11\" &&\n@@ -836,6 +824,59 @@ test_expect_success 'quotes on both sides' '\n  git_verify \"cdir 1/file11\" \"cdir 1/file12\"\n '\n\n+#\n+# //depot\n+#   - dir1\n+#     - file11\n+#     - file12\n+#     - noneed_file11.junk\n+#     - noneed_file12.junk\n+#   - dir2\n+#     - file21\n+#     - file22\n+#     - noneed_file21.junk\n+#     - noneed_file22.junk\n+#\n+test_expect_success 'wildcard * setup' '\n+ client_view \"//depot/... //client/...\" &&\n+ (\n+ p4 sync &&\n+ cd \"$cli\" &&\n+ rm files &&\n+ p4 delete \"//depot/...\" &&\n+ p4 submit -d \"delete all files\" &&\n+ init_depot &&\n+\n+ cd \"$cli\" &&\n+ p4 sync &&\n+\n+ echo dir1/noneed_file11.junk >dir1/noneed_file11.junk &&\n+ p4 add dir1/noneed_file11.junk &&\n+ p4 submit -d \"dir1/noneed_file11.junk\" &&\n+\n+ echo dir1/noneed_file12.junk >dir1/noneed_file12.junk &&\n+ p4 add dir1/noneed_file12.junk &&\n+ p4 submit -d \"dir1/noneed_file12.junk\" &&\n+\n+ echo dir2/noneed_file21.junk >dir2/noneed_file21.junk &&\n+ p4 add dir2/noneed_file21.junk &&\n+ p4 submit -d \"dir2/noneed_file21.junk\" &&\n+\n+ echo dir2/noneed_file22.junk >dir2/noneed_file22.junk &&\n+ p4 add dir2/noneed_file22.junk &&\n+ p4 submit -d \"dir2/noneed_file22.junk\"\n+ )\n+'\n+test_expect_success 'view wildcard *' '\n+ client_view \"//depot/... //client/...\" \\\n+ \"-//depot/dir1/*.junk //client/dir1/*.junk\" \\\n+ \"-//depot/dir2/*.junk //client/dir2/*.junk\" &&\n+ files=\"dir1/file11 dir1/file12 dir2/file21 dir2/file22\" &&\n+ client_verify $files &&\n+ git p4 clone --use-client-spec --dest=\"$git\" //depot &&\n+ git_verify $files\n+'\n+\n test_expect_success 'kill p4d' '\n  kill_p4d\n '\n-- \n1.8.4-rc1\n"},{"id":"225030","messageId":"20130810201123.GA31706@padd.com","threadId":"34623","inReplyTo":"CACGba4zdA=3tBE9UR=i9P9kNAL1HUc3UwSHbYeq4s9fwaN4=Mw@mail.gmail.com","subject":"Re: [PATCH v2] git-p4: Ask \"p4\" to interpret View setting","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-08-10T20:11:23Z","receivedAt":"2013-08-10T20:11:23Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"ksaitoh560@gmail.com wrote on Tue, 06 Aug 2013 15:45 +0900:\n> In Perforce, View setting of p4 client can describe\n>   -//depot/project/files/*.xls //client/project/files/*.xls\n> to exclude Excel files.\n> But \"git p4 --use-client-spec\" cannot support '*'.\n> \n> In git-p4.py, \"map_in_client\" method analyzes View setting and return\n> client file path.\n> So I modify the method to just ask p4.\n> \n> \n> > Let me play with this for a bit.  I wonder about the performance\n> > aspects of doing a \"p4 fstat\" for every file.  Would it be\n> > possible to do one or a few batch queries higher up somewhere?\n> To reduce p4 access, it cache result of asking \"client path\".\n> And addition, \"fstat\" depends on sync status, so modify to use \"p4\n> where\" instead of \"fstat\".\n\nI played around with your patch a bit, ending up with this\nteensy series.\n\nI redid the code to use clientFile, not path, as that\nwill work better with AltRoots.  Also I simplified your\ntest and added a couple more for the now-supported wildcards.\nAnd deleted a bunch of newly dead code.\n\nMy only concern is in the commit message, about performance.  A\nchange that has lots of files in it will cause many roundtrips to\np4d to do \"p4 where\" on each.  When the files don't have much\nedited content, this new approach will make the import take twice\nas long, I'll guess.  Do you have a big repository where you\ncould test that?\n\nTell me what you think.\n\n\t\t-- Pete\n"},{"id":"225031","messageId":"1376165713-26170-1-git-send-email-pw@padd.com","threadId":"34623","inReplyTo":"20130810201123.GA31706@padd.com","subject":"[PATCH 1/2] git p4 test: sanitize P4CHARSET","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-08-10T20:15:12Z","receivedAt":"2013-08-10T20:15:12Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"From: kazuki saitoh <ksaitoh560@gmail.com>\n\nIn the tests, p4d is started without using \"internationalized\nmode\".  Make sure this environment variable is unset, otherwise\na mis-matched user setting would break the tests.  The error\nmessage would be \"Unicode clients require a unicode enabled server.\"\n\n[pw: use unset, add commit text]\n\nSigned-off-by: Kazuki Saitoh <ksaitoh560@gmail.com>\nSigned-off-by: Pete Wyckoff <pw@padd.com>\n---\n t/lib-git-p4.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh\nindex 2098b9b..ccd918e 100644\n--- a/t/lib-git-p4.sh\n+++ b/t/lib-git-p4.sh\n@@ -48,7 +48,8 @@ P4DPORT=$((10669 + ($testid - $git_p4_test_start)))\n P4PORT=localhost:$P4DPORT\n P4CLIENT=client\n P4EDITOR=:\n-export P4PORT P4CLIENT P4EDITOR\n+unset P4CHARSET\n+export P4PORT P4CLIENT P4EDITOR P4CHARSET\n \n db=\"$TRASH_DIRECTORY/db\"\n cli=\"$TRASH_DIRECTORY/cli\"\n-- \n1.8.4.rc2.88.ga5463da\n"},{"id":"225032","messageId":"1376165713-26170-2-git-send-email-pw@padd.com","threadId":"34623","inReplyTo":"20130810201123.GA31706@padd.com","subject":"[PATCH 2/2] git p4: implement view spec wildcards with \"p4 where\"","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-08-10T20:15:13Z","receivedAt":"2013-08-10T20:15:13Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"From: kazuki saitoh <ksaitoh560@gmail.com>\n\nCurrently, git p4 does not support many of the view\nwildcards, such as * and %%n.  It only knows the\ncommon ... mapping, and exclusions.\n\nRedo the entire wildcard code around the idea of\ndirectly querying the p4 server for the mapping.  For each\nunique file, invoke \"p4 where //depot/path\" and use\nthe client mapping to decide where the file goes in git.\n\nThis simplifies a lot of code, and adds support for all\nwildcards supported by p4.  The downside is that each file\ntriggers a query to the p4 server, possibly making high file\ncount changes very slow.  If it turns out to be a problem,\nwe might consider resurrecting the old wildcard code just\nfor use on the \"easy\" cases it understands.\n\n[pw: redo code and tests]\n\nSigned-off-by: Kazuki Saitoh <ksaitoh560@gmail.com>\nSigned-off-by: Pete Wyckoff <pw@padd.com>\n---\n git-p4.py                     | 204 +++++++++---------------------------------\n t/t9809-git-p4-client-view.sh |  88 ++++++++++++------\n 2 files changed, 103 insertions(+), 189 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 31e71ff..40522f7 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -780,11 +780,14 @@ def getClientSpec():\n     # dictionary of all client parameters\n     entry = specList[0]\n \n+    # the //client/ name\n+    client_name = entry[\"Client\"]\n+\n     # just the keys that start with \"View\"\n     view_keys = [ k for k in entry.keys() if k.startswith(\"View\") ]\n \n     # hold this new View\n-    view = View()\n+    view = View(client_name)\n \n     # append the lines, in order, to the view\n     for view_num in range(len(view_keys)):\n@@ -1555,8 +1558,8 @@ class P4Submit(Command, P4UserMap):\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+            for depot_side in clientSpec.mappings:\n+                labelTemplate += \"\\t%s\\n\" % depot_side\n \n             if self.dry_run:\n                 print \"Would create p4 label %s for tag\" % name\n@@ -1568,7 +1571,7 @@ class P4Submit(Command, P4UserMap):\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+                          [\"%s@%s\" % (depot_side, changelist) for depot_side in clientSpec.mappings])\n \n                 if verbose:\n                     print \"created p4 label for tag %s\" % name\n@@ -1796,117 +1799,16 @@ class View(object):\n     \"\"\"Represent a p4 view (\"p4 help views\"), and map files in a\n        repo according to the view.\"\"\"\n \n-    class Path(object):\n-        \"\"\"A depot or client path, possibly containing wildcards.\n-           The only one supported is ... at the end, currently.\n-           Initialize with the full path, with //depot or //client.\"\"\"\n-\n-        def __init__(self, path, is_depot):\n-            self.path = path\n-            self.is_depot = is_depot\n-            self.find_wildcards()\n-            # remember the prefix bit, useful for relative mappings\n-            m = re.match(\"(//[^/]+/)\", self.path)\n-            if not m:\n-                die(\"Path %s does not start with //prefix/\" % self.path)\n-            prefix = m.group(1)\n-            if not self.is_depot:\n-                # strip //client/ on client paths\n-                self.path = self.path[len(prefix):]\n-\n-        def find_wildcards(self):\n-            \"\"\"Make sure wildcards are valid, and set up internal\n-               variables.\"\"\"\n-\n-            self.ends_triple_dot = False\n-            # There are three wildcards allowed in p4 views\n-            # (see \"p4 help views\").  This code knows how to\n-            # handle \"...\" (only at the end), but cannot deal with\n-            # \"%%n\" or \"*\".  Only check the depot_side, as p4 should\n-            # validate that the client_side matches too.\n-            if re.search(r'%%[1-9]', self.path):\n-                die(\"Can't handle %%n wildcards in view: %s\" % self.path)\n-            if self.path.find(\"*\") >= 0:\n-                die(\"Can't handle * wildcards in view: %s\" % self.path)\n-            triple_dot_index = self.path.find(\"...\")\n-            if triple_dot_index >= 0:\n-                if triple_dot_index != len(self.path) - 3:\n-                    die(\"Can handle only single ... wildcard, at end: %s\" %\n-                        self.path)\n-                self.ends_triple_dot = True\n-\n-        def ensure_compatible(self, other_path):\n-            \"\"\"Make sure the wildcards agree.\"\"\"\n-            if self.ends_triple_dot != other_path.ends_triple_dot:\n-                 die(\"Both paths must end with ... if either does;\\n\" +\n-                     \"paths: %s %s\" % (self.path, other_path.path))\n-\n-        def match_wildcards(self, test_path):\n-            \"\"\"See if this test_path matches us, and fill in the value\n-               of the wildcards if so.  Returns a tuple of\n-               (True|False, wildcards[]).  For now, only the ... at end\n-               is supported, so at most one wildcard.\"\"\"\n-            if self.ends_triple_dot:\n-                dotless = self.path[:-3]\n-                if test_path.startswith(dotless):\n-                    wildcard = test_path[len(dotless):]\n-                    return (True, [ wildcard ])\n-            else:\n-                if test_path == self.path:\n-                    return (True, [])\n-            return (False, [])\n-\n-        def match(self, test_path):\n-            \"\"\"Just return if it matches; don't bother with the wildcards.\"\"\"\n-            b, _ = self.match_wildcards(test_path)\n-            return b\n-\n-        def fill_in_wildcards(self, wildcards):\n-            \"\"\"Return the relative path, with the wildcards filled in\n-               if there are any.\"\"\"\n-            if self.ends_triple_dot:\n-                return self.path[:-3] + wildcards[0]\n-            else:\n-                return self.path\n-\n-    class Mapping(object):\n-        def __init__(self, depot_side, client_side, overlay, exclude):\n-            # depot_side is without the trailing /... if it had one\n-            self.depot_side = View.Path(depot_side, is_depot=True)\n-            self.client_side = View.Path(client_side, is_depot=False)\n-            self.overlay = overlay  # started with \"+\"\n-            self.exclude = exclude  # started with \"-\"\n-            assert not (self.overlay and self.exclude)\n-            self.depot_side.ensure_compatible(self.client_side)\n-\n-        def __str__(self):\n-            c = \" \"\n-            if self.overlay:\n-                c = \"+\"\n-            if self.exclude:\n-                c = \"-\"\n-            return \"View.Mapping: %s%s -> %s\" % \\\n-                   (c, self.depot_side.path, self.client_side.path)\n-\n-        def map_depot_to_client(self, depot_path):\n-            \"\"\"Calculate the client path if using this mapping on the\n-               given depot path; does not consider the effect of other\n-               mappings in a view.  Even excluded mappings are returned.\"\"\"\n-            matches, wildcards = self.depot_side.match_wildcards(depot_path)\n-            if not matches:\n-                return \"\"\n-            client_path = self.client_side.fill_in_wildcards(wildcards)\n-            return client_path\n-\n-    #\n-    # View methods\n-    #\n-    def __init__(self):\n+    def __init__(self, client_name):\n         self.mappings = []\n+        self.client_prefix = \"//%s/\" % client_name\n+        # cache results of \"p4 where\" to lookup client file locations\n+        self.client_spec_path_cache = {}\n \n     def append(self, view_line):\n         \"\"\"Parse a view line, splitting it into depot and client\n-           sides.  Append to self.mappings, preserving order.\"\"\"\n+           sides.  Append to self.mappings, preserving order.  This\n+           is only needed for tag creation.\"\"\"\n \n         # Split the view line into exactly two words.  P4 enforces\n         # structure on these lines that simplifies this quite a bit.\n@@ -1934,75 +1836,49 @@ class View(object):\n             depot_side = view_line[0:space_index]\n             rhs_index = space_index + 1\n \n-        if view_line[rhs_index] == '\"':\n-            # Second word is double quoted.  Make sure there is a\n-            # double quote at the end too.\n-            if not view_line.endswith('\"'):\n-                die(\"View line with rhs quote should end with one: %s\" %\n-                    view_line)\n-            # skip the quotes\n-            client_side = view_line[rhs_index+1:-1]\n-        else:\n-            client_side = view_line[rhs_index:]\n-\n         # prefix + means overlay on previous mapping\n-        overlay = False\n         if depot_side.startswith(\"+\"):\n-            overlay = True\n             depot_side = depot_side[1:]\n \n-        # prefix - means exclude this path\n+        # prefix - means exclude this path, leave out of mappings\n         exclude = False\n         if depot_side.startswith(\"-\"):\n             exclude = True\n             depot_side = depot_side[1:]\n \n-        m = View.Mapping(depot_side, client_side, overlay, exclude)\n-        self.mappings.append(m)\n+        if not exclude:\n+            self.mappings.append(depot_side)\n \n     def map_in_client(self, depot_path):\n         \"\"\"Return the relative location in the client where this\n            depot file should live.  Returns \"\" if the file should\n            not be mapped in the client.\"\"\"\n \n-        paths_filled = []\n-        client_path = \"\"\n-\n-        # look at later entries first\n-        for m in self.mappings[::-1]:\n-\n-            # see where will this path end up in the client\n-            p = m.map_depot_to_client(depot_path)\n-\n-            if p == \"\":\n-                # Depot path does not belong in client.  Must remember\n-                # this, as previous items should not cause files to\n-                # exist in this path either.  Remember that the list is\n-                # being walked from the end, which has higher precedence.\n-                # Overlap mappings do not exclude previous mappings.\n-                if not m.overlay:\n-                    paths_filled.append(m.client_side)\n-\n-            else:\n-                # This mapping matched; no need to search any further.\n-                # But, the mapping could be rejected if the client path\n-                # has already been claimed by an earlier mapping (i.e.\n-                # one later in the list, which we are walking backwards).\n-                already_mapped_in_client = False\n-                for f in paths_filled:\n-                    # this is View.Path.match\n-                    if f.match(p):\n-                        already_mapped_in_client = True\n-                        break\n-                if not already_mapped_in_client:\n-                    # Include this file, unless it is from a line that\n-                    # explicitly said to exclude it.\n-                    if not m.exclude:\n-                        client_path = p\n-\n-                # a match, even if rejected, always stops the search\n-                break\n+        if depot_path in self.client_spec_path_cache:\n+            return self.client_spec_path_cache[depot_path]\n \n+        where_result = p4CmdList(['where', depot_path])\n+        if len(where_result) == 0:\n+            die(\"No result from 'p4 where %s'\" % depot_path)\n+        client_path = \"\"\n+        for res in where_result:\n+            if \"code\" in res and res[\"code\"] == \"error\":\n+                # assume error is \"... file(s) not in client view\"\n+                client_path = \"\"\n+                continue\n+            if \"clientFile\" not in res:\n+                die(\"No clientFile from 'p4 where %s'\" % depot_path)\n+            if \"unmap\" in res:\n+                # it will list all of them, but only one not unmap-ped\n+                continue\n+            # chop off //client/ part to make it relative\n+            clientFile = res[\"clientFile\"]\n+            if not clientFile.startswith(self.client_prefix):\n+                die(\"No prefix '%s' on clientFile '%s'\" %\n+                    (self.client_prefix, clientFile))\n+            client_path = clientFile[len(self.client_prefix):]\n+\n+        self.client_spec_path_cache[depot_path] = client_path\n         return client_path\n \n class P4Sync(Command, P4UserMap):\ndiff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh\nindex 77f6349..2ebc6e0 100755\n--- a/t/t9809-git-p4-client-view.sh\n+++ b/t/t9809-git-p4-client-view.sh\n@@ -75,31 +75,6 @@ test_expect_success 'init depot' '\n \t)\n '\n \n-# double % for printf\n-test_expect_success 'unsupported view wildcard %%n' '\n-\tclient_view \"//depot/%%%%1/sub/... //client/sub/%%%%1/...\" &&\n-\ttest_when_finished cleanup_git &&\n-\ttest_must_fail git p4 clone --use-client-spec --dest=\"$git\" //depot\n-'\n-\n-test_expect_success 'unsupported view wildcard *' '\n-\tclient_view \"//depot/*/bar/... //client/*/bar/...\" &&\n-\ttest_when_finished cleanup_git &&\n-\ttest_must_fail git p4 clone --use-client-spec --dest=\"$git\" //depot\n-'\n-\n-test_expect_success 'wildcard ... only supported at end of spec 1' '\n-\tclient_view \"//depot/.../file11 //client/.../file11\" &&\n-\ttest_when_finished cleanup_git &&\n-\ttest_must_fail git p4 clone --use-client-spec --dest=\"$git\" //depot\n-'\n-\n-test_expect_success 'wildcard ... only supported at end of spec 2' '\n-\tclient_view \"//depot/.../a/... //client/.../a/...\" &&\n-\ttest_when_finished cleanup_git &&\n-\ttest_must_fail git p4 clone --use-client-spec --dest=\"$git\" //depot\n-'\n-\n test_expect_success 'basic map' '\n \tclient_view \"//depot/dir1/... //client/cli1/...\" &&\n \tfiles=\"cli1/file11 cli1/file12\" &&\n@@ -793,6 +768,69 @@ test_expect_success 'overlay sync swap: cleanup' '\n \t)\n '\n \n+# add a .junk file, make sure it isn't mapped\n+test_expect_success 'wildcard * setup' '\n+\tclient_view \"//depot/... //client/...\" &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tp4 sync &&\n+\t\techo dir1/file13.junk >dir1/file13.junk &&\n+\t\tp4 add dir1/file13.junk &&\n+\t\tp4 submit -d dir1/file13.junk\n+\t)\n+'\n+\n+test_expect_success 'wildcard * base case' '\n+\tclient_view \"//depot/... //client/...\" &&\n+\tfiles=\"dir1/file11 dir1/file12 dir1/file13.junk\n+\t       dir2/file21 dir2/file22\" &&\n+\tclient_verify $files &&\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --use-client-spec --dest=\"$git\" //depot &&\n+\tgit_verify $files\n+'\n+\n+test_expect_success 'wildcard * excludes junk file' '\n+\tclient_view \"//depot/... //client/...\" \\\n+\t\t    \"-//depot/dir1/*.junk //client/dir1/*.junk\" &&\n+\tfiles=\"dir1/file11 dir1/file12\n+\t       dir2/file21 dir2/file22\" &&\n+\tclient_verify $files &&\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --use-client-spec --dest=\"$git\" //depot &&\n+\tgit_verify $files\n+'\n+\n+test_expect_success 'wildcard * cleanup' '\n+\tclient_view \"//depot/... //client/...\" &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tp4 sync &&\n+\t\tp4 delete dir1/file13.junk &&\n+\t\tp4 submit -d \"remove dir1/file13.junk\"\n+\t)\n+'\n+\n+test_expect_success 'wildcard ... in middle' '\n+\tclient_view \"//depot/.../file11 //client/.../file11\" &&\n+\tfiles=\"dir1/file11\" &&\n+\tclient_verify $files &&\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --use-client-spec --dest=\"$git\" //depot &&\n+\tgit_verify $files\n+'\n+\n+test_expect_success 'wildcard %% mapping' '\n+\tclient_view \"//depot/%%1/... //client/map-%%1/...\" &&\n+\tfiles=\"map-dir1/file11 map-dir1/file12\n+\t       map-dir2/file21 map-dir2/file22\" &&\n+\tclient_verify $files &&\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --use-client-spec --dest=\"$git\" //depot &&\n+\tgit_verify $files\n+'\n+\n+\n #\n # Rename directories to test quoting in depot-side mappings\n # //depot\n-- \n1.8.4.rc2.88.ga5463da\n"},{"id":"225173","messageId":"CACGba4wbqyHzXDCQxG31EKawfc-D4jpVYqbB4GdmK4hM=Oi4mw@mail.gmail.com","threadId":"34623","inReplyTo":"20130810201123.GA31706@padd.com","subject":"Re: [PATCH v2] git-p4: Ask \"p4\" to interpret View setting","fromName":"kazuki saitoh","fromEmail":"ksaitoh560@gmail.com","sentAt":"2013-08-14T00:59:48Z","receivedAt":"2013-08-14T00:59:48Z","isPatch":true,"sender":{"key":"ksaitoh560@gmail.com","avatar":"https://avatars.githubusercontent.com/u/217076?v=4"},"body":"> My only concern is in the commit message, about performance.  A\n> change that has lots of files in it will cause many roundtrips to\n> p4d to do \"p4 where\" on each.  When the files don't have much\n> edited content, this new approach will make the import take twice\n> as long, I'll guess.  Do you have a big repository where you\n> could test that?\n\nI measured performance of \"git p4 clone  --use-client-spec\" with a\nrepository it has 1925 files, 50MB.\n  Original:    8.05s user 32.02s system 15% cpu 4:25.34 total\n  Apply patch:    9.02s user 53.19s system 14% cpu 6:56.41 total\n\nIt is acceptable in my situation, but looks quite slow...\n\nThen I implemented one batch query version\n   7.92s user 33.03s system 15% cpu 4:25.59 total\n\nIt is same as original\n\nMy additional patch is below.\nI investigate call graph (attached rough sketch) and\nimplement batch query in \"commit()\" and \"splitFilesIntoBranches()\".\nIn addition, modified \"map_in_client\" to just search cache value.\n\nCould you accept?\n\n\nSubject: [PATCH] git p4: Implement as one batch \"p4 where\" query to interpret\n view spec\n\nQuery for each file is decrese performance.\nSo I implement query to get client file path as one batch query.\nThe query must called before use client path (map_in_client() ).\n\nResult of performance measurement, about 40% speed up\n\nSigned-off-by: KazukiSaitoh <ksaitoh560@gmail.com>\n---\n git-p4.py | 70 ++++++++++++++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 51 insertions(+), 19 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 40522f7..8cbee24 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1849,37 +1849,46 @@ class View(object):\n         if not exclude:\n             self.mappings.append(depot_side)\n\n-    def map_in_client(self, depot_path):\n-        \"\"\"Return the relative location in the client where this\n-           depot file should live.  Returns \"\" if the file should\n-           not be mapped in the client.\"\"\"\n+    def convert_client_path(self, clientFile):\n+        # chop off //client/ part to make it relative\n+        if not clientFile.startswith(self.client_prefix):\n+            die(\"No prefix '%s' on clientFile '%s'\" %\n+                (self.client_prefix, clientFile))\n+        return clientFile[len(self.client_prefix):]\n\n-        if depot_path in self.client_spec_path_cache:\n-            return self.client_spec_path_cache[depot_path]\n+    def update_client_spec_path_cache(self, files):\n+        fileArgs = [f for f in files if f not in self.client_spec_path_cache]\n\n-        where_result = p4CmdList(['where', depot_path])\n-        if len(where_result) == 0:\n-            die(\"No result from 'p4 where %s'\" % depot_path)\n-        client_path = \"\"\n+        if len(fileArgs) == 0:\n+            return  # All files in cache\n+\n+        where_result = p4CmdList([\"-x\", \"-\", \"where\"], stdin=fileArgs)\n         for res in where_result:\n             if \"code\" in res and res[\"code\"] == \"error\":\n                 # assume error is \"... file(s) not in client view\"\n-                client_path = \"\"\n                 continue\n             if \"clientFile\" not in res:\n                 die(\"No clientFile from 'p4 where %s'\" % depot_path)\n             if \"unmap\" in res:\n                 # it will list all of them, but only one not unmap-ped\n                 continue\n-            # chop off //client/ part to make it relative\n-            clientFile = res[\"clientFile\"]\n-            if not clientFile.startswith(self.client_prefix):\n-                die(\"No prefix '%s' on clientFile '%s'\" %\n-                    (self.client_prefix, clientFile))\n-            client_path = clientFile[len(self.client_prefix):]\n+            self.client_spec_path_cache[res['depotFile']] =\nself.convert_client_path(res[\"clientFile\"])\n+\n+        # not found files or unmap files set to \"\"\n+        for depotFile in fileArgs:\n+            if depotFile not in self.client_spec_path_cache:\n+                self.client_spec_path_cache[depotFile] = \"\"\n+\n+    def map_in_client(self, depot_path):\n+        \"\"\"Return the relative location in the client where this\n+           depot file should live.  Returns \"\" if the file should\n+           not be mapped in the client.\"\"\"\n+\n+        if depot_path in self.client_spec_path_cache:\n+            return self.client_spec_path_cache[depot_path]\n\n-        self.client_spec_path_cache[depot_path] = client_path\n-        return client_path\n+        die( \"Error: %s is not found in client spec path\" % depot_path )\n+        return \"\"\n\n class P4Sync(Command, P4UserMap):\n     delete_actions = ( \"delete\", \"move/delete\", \"purge\" )\n@@ -2006,6 +2015,22 @@ class P4Sync(Command, P4UserMap):\n         \"\"\"Look at each depotFile in the commit to figure out to what\n            branch it belongs.\"\"\"\n\n+        # create file list and get client paths by one batch \"p4 where\" query\n+        if self.clientSpecDirs:\n+            fnum = 0\n+            file_list  = []\n+            while commit.has_key(\"depotFile%s\" % fnum):\n+                path =  commit[\"depotFile%s\" % fnum]\n+                found = [p for p in self.depotPaths\n+                         if p4PathStartsWith(path, p)]\n+                if not found:\n+                    fnum = fnum + 1\n+                    continue\n+\n+                file_list.append(path)\n+                fnum = fnum + 1\n+            self.clientSpecDirs.update_client_spec_path_cache(file_list)\n+\n         branches = {}\n         fnum = 0\n         while commit.has_key(\"depotFile%s\" % fnum):\n@@ -2255,6 +2280,13 @@ class P4Sync(Command, P4UserMap):\n             else:\n                 sys.stderr.write(\"Ignoring file outside of prefix:\n%s\\n\" % f['path'])\n\n+        # get client paths by one batch \"p4 where\" query\n+        if self.clientSpecDirs:\n+            file_list = []\n+            for f in files:\n+                file_list.append(f['path'])\n+            self.clientSpecDirs.update_client_spec_path_cache(file_list)\n+\n         self.gitStream.write(\"commit %s\\n\" % branch)\n #        gitStream.write(\"mark :%s\\n\" % details[\"change\"])\n         self.committedChanges.add(int(details[\"change\"]))\n-- \n1.8.4-rc2\n"},{"id":"225304","messageId":"20130816012420.GA20985@padd.com","threadId":"34623","inReplyTo":"CACGba4wbqyHzXDCQxG31EKawfc-D4jpVYqbB4GdmK4hM=Oi4mw@mail.gmail.com","subject":"Re: [PATCH v2] git-p4: Ask \"p4\" to interpret View setting","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-08-16T01:24:20Z","receivedAt":"2013-08-16T01:24:20Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"ksaitoh560@gmail.com wrote on Wed, 14 Aug 2013 09:59 +0900:\n> > My only concern is in the commit message, about performance.  A\n> > change that has lots of files in it will cause many roundtrips to\n> > p4d to do \"p4 where\" on each.  When the files don't have much\n> > edited content, this new approach will make the import take twice\n> > as long, I'll guess.  Do you have a big repository where you\n> > could test that?\n> \n> I measured performance of \"git p4 clone  --use-client-spec\" with a\n> repository it has 1925 files, 50MB.\n>   Original:    8.05s user 32.02s system 15% cpu 4:25.34 total\n>   Apply patch:    9.02s user 53.19s system 14% cpu 6:56.41 total\n> \n> It is acceptable in my situation, but looks quite slow...\n> \n> Then I implemented one batch query version\n>    7.92s user 33.03s system 15% cpu 4:25.59 total\n> \n> It is same as original\n> \n> My additional patch is below.\n> I investigate call graph (attached rough sketch) and\n> implement batch query in \"commit()\" and \"splitFilesIntoBranches()\".\n> In addition, modified \"map_in_client\" to just search cache value.\n> \n> Could you accept?\n\nThis looks good.  I've started my own performance testing\non a few-hundred-thousand file repo to confirm your findings.\n\nIf it seems to work out, we can clean up the patch.  Otherwise\nmaybe need to think about having both implementations and use\nthe by-hand one for \"...\".  I don't like that approach.  Let's\nhope it's not needed.\n\n\t\t-- Pete\n\n> Subject: [PATCH] git p4: Implement as one batch \"p4 where\" query to interpret\n>  view spec\n> \n> Query for each file is decrese performance.\n> So I implement query to get client file path as one batch query.\n> The query must called before use client path (map_in_client() ).\n> \n> Result of performance measurement, about 40% speed up\n> \n> Signed-off-by: KazukiSaitoh <ksaitoh560@gmail.com>\n> ---\n>  git-p4.py | 70 ++++++++++++++++++++++++++++++++++++++++++++++-----------------\n>  1 file changed, 51 insertions(+), 19 deletions(-)\n> \n> diff --git a/git-p4.py b/git-p4.py\n> index 40522f7..8cbee24 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1849,37 +1849,46 @@ class View(object):\n>          if not exclude:\n>              self.mappings.append(depot_side)\n> \n> -    def map_in_client(self, depot_path):\n> -        \"\"\"Return the relative location in the client where this\n> -           depot file should live.  Returns \"\" if the file should\n> -           not be mapped in the client.\"\"\"\n> +    def convert_client_path(self, clientFile):\n> +        # chop off //client/ part to make it relative\n> +        if not clientFile.startswith(self.client_prefix):\n> +            die(\"No prefix '%s' on clientFile '%s'\" %\n> +                (self.client_prefix, clientFile))\n> +        return clientFile[len(self.client_prefix):]\n> \n> -        if depot_path in self.client_spec_path_cache:\n> -            return self.client_spec_path_cache[depot_path]\n> +    def update_client_spec_path_cache(self, files):\n> +        fileArgs = [f for f in files if f not in self.client_spec_path_cache]\n> \n> -        where_result = p4CmdList(['where', depot_path])\n> -        if len(where_result) == 0:\n> -            die(\"No result from 'p4 where %s'\" % depot_path)\n> -        client_path = \"\"\n> +        if len(fileArgs) == 0:\n> +            return  # All files in cache\n> +\n> +        where_result = p4CmdList([\"-x\", \"-\", \"where\"], stdin=fileArgs)\n>          for res in where_result:\n>              if \"code\" in res and res[\"code\"] == \"error\":\n>                  # assume error is \"... file(s) not in client view\"\n> -                client_path = \"\"\n>                  continue\n>              if \"clientFile\" not in res:\n>                  die(\"No clientFile from 'p4 where %s'\" % depot_path)\n>              if \"unmap\" in res:\n>                  # it will list all of them, but only one not unmap-ped\n>                  continue\n> -            # chop off //client/ part to make it relative\n> -            clientFile = res[\"clientFile\"]\n> -            if not clientFile.startswith(self.client_prefix):\n> -                die(\"No prefix '%s' on clientFile '%s'\" %\n> -                    (self.client_prefix, clientFile))\n> -            client_path = clientFile[len(self.client_prefix):]\n> +            self.client_spec_path_cache[res['depotFile']] =\n> self.convert_client_path(res[\"clientFile\"])\n> +\n> +        # not found files or unmap files set to \"\"\n> +        for depotFile in fileArgs:\n> +            if depotFile not in self.client_spec_path_cache:\n> +                self.client_spec_path_cache[depotFile] = \"\"\n> +\n> +    def map_in_client(self, depot_path):\n> +        \"\"\"Return the relative location in the client where this\n> +           depot file should live.  Returns \"\" if the file should\n> +           not be mapped in the client.\"\"\"\n> +\n> +        if depot_path in self.client_spec_path_cache:\n> +            return self.client_spec_path_cache[depot_path]\n> \n> -        self.client_spec_path_cache[depot_path] = client_path\n> -        return client_path\n> +        die( \"Error: %s is not found in client spec path\" % depot_path )\n> +        return \"\"\n> \n>  class P4Sync(Command, P4UserMap):\n>      delete_actions = ( \"delete\", \"move/delete\", \"purge\" )\n> @@ -2006,6 +2015,22 @@ class P4Sync(Command, P4UserMap):\n>          \"\"\"Look at each depotFile in the commit to figure out to what\n>             branch it belongs.\"\"\"\n> \n> +        # create file list and get client paths by one batch \"p4 where\" query\n> +        if self.clientSpecDirs:\n> +            fnum = 0\n> +            file_list  = []\n> +            while commit.has_key(\"depotFile%s\" % fnum):\n> +                path =  commit[\"depotFile%s\" % fnum]\n> +                found = [p for p in self.depotPaths\n> +                         if p4PathStartsWith(path, p)]\n> +                if not found:\n> +                    fnum = fnum + 1\n> +                    continue\n> +\n> +                file_list.append(path)\n> +                fnum = fnum + 1\n> +            self.clientSpecDirs.update_client_spec_path_cache(file_list)\n> +\n>          branches = {}\n>          fnum = 0\n>          while commit.has_key(\"depotFile%s\" % fnum):\n> @@ -2255,6 +2280,13 @@ class P4Sync(Command, P4UserMap):\n>              else:\n>                  sys.stderr.write(\"Ignoring file outside of prefix:\n> %s\\n\" % f['path'])\n> \n> +        # get client paths by one batch \"p4 where\" query\n> +        if self.clientSpecDirs:\n> +            file_list = []\n> +            for f in files:\n> +                file_list.append(f['path'])\n> +            self.clientSpecDirs.update_client_spec_path_cache(file_list)\n> +\n>          self.gitStream.write(\"commit %s\\n\" % branch)\n>  #        gitStream.write(\"mark :%s\\n\" % details[\"change\"])\n>          self.committedChanges.add(int(details[\"change\"]))\n> -- \n> 1.8.4-rc2\n"},{"id":"225802","messageId":"20130825022944.GA16027@padd.com","threadId":"34623","inReplyTo":"20130816012420.GA20985@padd.com","subject":"Re: [PATCH v2] git-p4: Ask \"p4\" to interpret View setting","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-08-25T02:29:44Z","receivedAt":"2013-08-25T02:29:44Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"pw@padd.com wrote on Thu, 15 Aug 2013 21:24 -0400:\n> ksaitoh560@gmail.com wrote on Wed, 14 Aug 2013 09:59 +0900:\n> > > My only concern is in the commit message, about performance.  A\n> > > change that has lots of files in it will cause many roundtrips to\n> > > p4d to do \"p4 where\" on each.  When the files don't have much\n> > > edited content, this new approach will make the import take twice\n> > > as long, I'll guess.  Do you have a big repository where you\n> > > could test that?\n> > \n> > I measured performance of \"git p4 clone  --use-client-spec\" with a\n> > repository it has 1925 files, 50MB.\n> >   Original:    8.05s user 32.02s system 15% cpu 4:25.34 total\n> >   Apply patch:    9.02s user 53.19s system 14% cpu 6:56.41 total\n> > \n> > It is acceptable in my situation, but looks quite slow...\n> > \n> > Then I implemented one batch query version\n> >    7.92s user 33.03s system 15% cpu 4:25.59 total\n> > \n> > It is same as original\n> > \n> > My additional patch is below.\n> > I investigate call graph (attached rough sketch) and\n> > implement batch query in \"commit()\" and \"splitFilesIntoBranches()\".\n> > In addition, modified \"map_in_client\" to just search cache value.\n> > \n> > Could you accept?\n> \n> This looks good.  I've started my own performance testing\n> on a few-hundred-thousand file repo to confirm your findings.\n> \n> If it seems to work out, we can clean up the patch.  Otherwise\n> maybe need to think about having both implementations and use\n> the by-hand one for \"...\".  I don't like that approach.  Let's\n> hope it's not needed.\n\nI tried with a few repos:\n\nSmall repo, single-commit clone:\n\n    Current:     0m0.35s user 0m0.30s sys 0m11.52s elapsed 5.69 %CPU\n    No batching: 0m0.66s user 0m0.77s sys 0m34.42s elapsed 4.17 %CPU\n    Batching:    0m0.28s user 0m0.29s sys 0m10.85s elapsed 5.27 %CPU\n\nBig repo, single-commit clone:\n\n    Current:     6m21.38s user 1m35.36s sys 19m44.83s elapsed 40.23 %CPU\n    No batching: 1m53.13s user 24m34.35s sys 146m13.80s elapsed 18.09 %CPU (*)\n    Batching:    6m22.01s user 1m44.23s sys 21m19.73s elapsed 37.99 %CPU\n\n    The \"no batching\" run died with an unrelated p4 timeout.\n\nBig repo, 1000 incremental changes:\n\n    Current:     0m13.43s user 0m19.82s sys 11m12.58s elapsed 4.94 %CPU\n    No batching: 0m20.29s user 0m39.94s sys 38m44.69s elapsed 2.59 %CPU (*)\n    Batching:    0m16.15s user 0m26.60s sys 13m55.69s elapsed 5.11 %CPU\n\n    The \"no batching\" run died at 28% of the way through.\n\nThere is probably a 20%-ish slowdown in my environment with this\napproach.  But given that the timescale for these operations is\nmeasured in the tens of minutes, I don't think a couple more matters\ntoo much to anybody.\n\nThe attractiveness of the simplicity and increased client spec feature\ncoverage weighs in its favor.  Let's go ahead and inflict this on the\nworld and see what they think.\n\nDo you have an updated patch?  Want to take some time to clean up and\nresubmit the entire series?  The batching should be incorporated with\nthe last 2/2 that I sent out.\n\n\t\t-- Pete\n"},{"id":"225947","messageId":"CACGba4wqSYf+qg21C7-0Y1r+ZafvggEVQrPu3nMdTr5PdtEOXQ@mail.gmail.com","threadId":"34623","inReplyTo":"20130825022944.GA16027@padd.com","subject":"Re: [PATCH v2] git-p4: Ask \"p4\" to interpret View setting","fromName":"kazuki saitoh","fromEmail":"ksaitoh560@gmail.com","sentAt":"2013-08-27T02:43:38Z","receivedAt":"2013-08-27T02:43:38Z","isPatch":true,"sender":{"key":"ksaitoh560@gmail.com","avatar":"https://avatars.githubusercontent.com/u/217076?v=4"},"body":"> Do you have an updated patch?  Want to take some time to clean up and\n> resubmit the entire series?  The batching should be incorporated with\n> the last 2/2 that I sent out.\n\nI don't have other update.\nI'm satisfied because able to want to do and it became better than my\noriginal modification thanks to your cooperation.\n(> a few-hundred-thousand file repo\nI didn't think that it work with so HUGE repo.)\n\nHow should I do?\nShould I create one patch mail that incorporated your sent one?\nOr nothing to do?\n\n\n2013/8/25 Pete Wyckoff <pw@padd.com>:\n> pw@padd.com wrote on Thu, 15 Aug 2013 21:24 -0400:\n>> ksaitoh560@gmail.com wrote on Wed, 14 Aug 2013 09:59 +0900:\n>> > > My only concern is in the commit message, about performance.  A\n>> > > change that has lots of files in it will cause many roundtrips to\n>> > > p4d to do \"p4 where\" on each.  When the files don't have much\n>> > > edited content, this new approach will make the import take twice\n>> > > as long, I'll guess.  Do you have a big repository where you\n>> > > could test that?\n>> >\n>> > I measured performance of \"git p4 clone  --use-client-spec\" with a\n>> > repository it has 1925 files, 50MB.\n>> >   Original:    8.05s user 32.02s system 15% cpu 4:25.34 total\n>> >   Apply patch:    9.02s user 53.19s system 14% cpu 6:56.41 total\n>> >\n>> > It is acceptable in my situation, but looks quite slow...\n>> >\n>> > Then I implemented one batch query version\n>> >    7.92s user 33.03s system 15% cpu 4:25.59 total\n>> >\n>> > It is same as original\n>> >\n>> > My additional patch is below.\n>> > I investigate call graph (attached rough sketch) and\n>> > implement batch query in \"commit()\" and \"splitFilesIntoBranches()\".\n>> > In addition, modified \"map_in_client\" to just search cache value.\n>> >\n>> > Could you accept?\n>>\n>> This looks good.  I've started my own performance testing\n>> on a few-hundred-thousand file repo to confirm your findings.\n>>\n>> If it seems to work out, we can clean up the patch.  Otherwise\n>> maybe need to think about having both implementations and use\n>> the by-hand one for \"...\".  I don't like that approach.  Let's\n>> hope it's not needed.\n>\n> I tried with a few repos:\n>\n> Small repo, single-commit clone:\n>\n>     Current:     0m0.35s user 0m0.30s sys 0m11.52s elapsed 5.69 %CPU\n>     No batching: 0m0.66s user 0m0.77s sys 0m34.42s elapsed 4.17 %CPU\n>     Batching:    0m0.28s user 0m0.29s sys 0m10.85s elapsed 5.27 %CPU\n>\n> Big repo, single-commit clone:\n>\n>     Current:     6m21.38s user 1m35.36s sys 19m44.83s elapsed 40.23 %CPU\n>     No batching: 1m53.13s user 24m34.35s sys 146m13.80s elapsed 18.09 %CPU (*)\n>     Batching:    6m22.01s user 1m44.23s sys 21m19.73s elapsed 37.99 %CPU\n>\n>     The \"no batching\" run died with an unrelated p4 timeout.\n>\n> Big repo, 1000 incremental changes:\n>\n>     Current:     0m13.43s user 0m19.82s sys 11m12.58s elapsed 4.94 %CPU\n>     No batching: 0m20.29s user 0m39.94s sys 38m44.69s elapsed 2.59 %CPU (*)\n>     Batching:    0m16.15s user 0m26.60s sys 13m55.69s elapsed 5.11 %CPU\n>\n>     The \"no batching\" run died at 28% of the way through.\n>\n> There is probably a 20%-ish slowdown in my environment with this\n> approach.  But given that the timescale for these operations is\n> measured in the tens of minutes, I don't think a couple more matters\n> too much to anybody.\n>\n> The attractiveness of the simplicity and increased client spec feature\n> coverage weighs in its favor.  Let's go ahead and inflict this on the\n> world and see what they think.\n>\n> Do you have an updated patch?  Want to take some time to clean up and\n> resubmit the entire series?  The batching should be incorporated with\n> the last 2/2 that I sent out.\n>\n>                 -- Pete\n"},{"id":"226278","messageId":"20130829224037.GA25879@padd.com","threadId":"34623","inReplyTo":"CACGba4wqSYf+qg21C7-0Y1r+ZafvggEVQrPu3nMdTr5PdtEOXQ@mail.gmail.com","subject":"Re: [PATCH v2] git-p4: Ask \"p4\" to interpret View setting","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-08-29T22:40:37Z","receivedAt":"2013-08-29T22:40:37Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"ksaitoh560@gmail.com wrote on Tue, 27 Aug 2013 11:43 +0900:\n> > Do you have an updated patch?  Want to take some time to clean up and\n> > resubmit the entire series?  The batching should be incorporated with\n> > the last 2/2 that I sent out.\n> \n> I don't have other update.\n> I'm satisfied because able to want to do and it became better than my\n> original modification thanks to your cooperation.\n> (> a few-hundred-thousand file repo\n> I didn't think that it work with so HUGE repo.)\n> \n> How should I do?\n> Should I create one patch mail that incorporated your sent one?\n> Or nothing to do?\n\nIt would be good if you could fold the one I sent in with yours,\nand clean up any stylistic issues as you go.\n\nI'll play with it a bit more, then send on to Junio for\nthe next release.\n\nThanks, this is a good addition!\n\n\t\t-- Pete\n\n\n> 2013/8/25 Pete Wyckoff <pw@padd.com>:\n> > pw@padd.com wrote on Thu, 15 Aug 2013 21:24 -0400:\n> >> ksaitoh560@gmail.com wrote on Wed, 14 Aug 2013 09:59 +0900:\n> >> > > My only concern is in the commit message, about performance.  A\n> >> > > change that has lots of files in it will cause many roundtrips to\n> >> > > p4d to do \"p4 where\" on each.  When the files don't have much\n> >> > > edited content, this new approach will make the import take twice\n> >> > > as long, I'll guess.  Do you have a big repository where you\n> >> > > could test that?\n> >> >\n> >> > I measured performance of \"git p4 clone  --use-client-spec\" with a\n> >> > repository it has 1925 files, 50MB.\n> >> >   Original:    8.05s user 32.02s system 15% cpu 4:25.34 total\n> >> >   Apply patch:    9.02s user 53.19s system 14% cpu 6:56.41 total\n> >> >\n> >> > It is acceptable in my situation, but looks quite slow...\n> >> >\n> >> > Then I implemented one batch query version\n> >> >    7.92s user 33.03s system 15% cpu 4:25.59 total\n> >> >\n> >> > It is same as original\n> >> >\n> >> > My additional patch is below.\n> >> > I investigate call graph (attached rough sketch) and\n> >> > implement batch query in \"commit()\" and \"splitFilesIntoBranches()\".\n> >> > In addition, modified \"map_in_client\" to just search cache value.\n> >> >\n> >> > Could you accept?\n> >>\n> >> This looks good.  I've started my own performance testing\n> >> on a few-hundred-thousand file repo to confirm your findings.\n> >>\n> >> If it seems to work out, we can clean up the patch.  Otherwise\n> >> maybe need to think about having both implementations and use\n> >> the by-hand one for \"...\".  I don't like that approach.  Let's\n> >> hope it's not needed.\n> >\n> > I tried with a few repos:\n> >\n> > Small repo, single-commit clone:\n> >\n> >     Current:     0m0.35s user 0m0.30s sys 0m11.52s elapsed 5.69 %CPU\n> >     No batching: 0m0.66s user 0m0.77s sys 0m34.42s elapsed 4.17 %CPU\n> >     Batching:    0m0.28s user 0m0.29s sys 0m10.85s elapsed 5.27 %CPU\n> >\n> > Big repo, single-commit clone:\n> >\n> >     Current:     6m21.38s user 1m35.36s sys 19m44.83s elapsed 40.23 %CPU\n> >     No batching: 1m53.13s user 24m34.35s sys 146m13.80s elapsed 18.09 %CPU (*)\n> >     Batching:    6m22.01s user 1m44.23s sys 21m19.73s elapsed 37.99 %CPU\n> >\n> >     The \"no batching\" run died with an unrelated p4 timeout.\n> >\n> > Big repo, 1000 incremental changes:\n> >\n> >     Current:     0m13.43s user 0m19.82s sys 11m12.58s elapsed 4.94 %CPU\n> >     No batching: 0m20.29s user 0m39.94s sys 38m44.69s elapsed 2.59 %CPU (*)\n> >     Batching:    0m16.15s user 0m26.60s sys 13m55.69s elapsed 5.11 %CPU\n> >\n> >     The \"no batching\" run died at 28% of the way through.\n> >\n> > There is probably a 20%-ish slowdown in my environment with this\n> > approach.  But given that the timescale for these operations is\n> > measured in the tens of minutes, I don't think a couple more matters\n> > too much to anybody.\n> >\n> > The attractiveness of the simplicity and increased client spec feature\n> > coverage weighs in its favor.  Let's go ahead and inflict this on the\n> > world and see what they think.\n> >\n> > Do you have an updated patch?  Want to take some time to clean up and\n> > resubmit the entire series?  The batching should be incorporated with\n> > the last 2/2 that I sent out.\n> >\n> >                 -- Pete\n> \n"}]}