{"thread":{"id":"39071","subject":"Re: [PATCH] git-p4: Use -m when running p4 changes","startedAt":"2015-04-14T11:25:32Z","lastAt":"2015-04-20T19:17:21Z","messageCount":15,"participants":["Luke Diamand","Lex Spoon","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"259340","messageId":"CAE5ih79UcJfuhzgTdTPy2K51sa6--4bvaVaKL3nsUcC2kq4Ffg@mail.gmail.com","threadId":"39071","inReplyTo":"CALM2SnY62u3OXJOMSqSfghH_NYwZhzSedm3-wcde-dQCX6eB9Q@mail.gmail.com","subject":"Re: [PATCH] git-p4: Use -m when running p4 changes","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-04-14T11:25:32Z","receivedAt":"2015-04-14T11:25:32Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 11 April 2015 at 16:17, Lex Spoon <lex@lexspoon.org> wrote:\n>\n>\n> Signed-off-by: Lex Spoon <lex@lexspoon.org>\n> ---\n> This patch addresses a problem I am running into with a client. I am\n> attempting to mirror their Perforce repository into Git, and on certain\n> branches their Perforce server is responding with an error about \"too many\n> rows scanned\". This change has git-p4 use the \"-m\" option to return just 500\n> changes at a time, thus avoiding the problem.\n\nThanks - that's a problem I also occasionally hit, and it definitely\nneeds fixing.\n\nYour fix is quite nice - I started out thinking this should be easy,\nbut it's not!\n\nA test case addition would be good if you can though - otherwise it's\ncertain to break at some point in the future. Would you have time to\nadd that?\n\nThanks!\nLuke\n\n\n>\n> I have tested this on a small test repository (2000 revisions) and it\n> appears to work fine. I have also run all the t98* tests; those print a\n> number of yellow \"not ok\" results but no red ones. I presume this is the\n> expected test behavior?\n\nYes.\n\n>\n> I considered making the block size configurable, but it seems unlikely\n> anyone will strongly benefit from changing it. 500 is large enough that it\n> should only take a modest number of iterations to scan the full changes\n> list, but it's small enough that any reasonable Perforce server should allow\n> the request.\n\nMight be useful when making test harnesses though :-)\n\n\n>\n> This patch is also available on GitHub:\n> https://github.com/lexspoon/git/tree/p4-sync-batches\n>\n>  git-p4.py | 40 +++++++++++++++++++++++++++++++++-------\n>  1 file changed, 33 insertions(+), 7 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 549022e..ce1447b 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -742,15 +742,41 @@ def originP4BranchesExist():\n>\n>  def p4ChangesForPaths(depotPaths, changeRange):\n>      assert depotPaths\n> -    cmd = ['changes']\n> -    for p in depotPaths:\n> -        cmd += [\"%s...%s\" % (p, changeRange)]\n> -    output = p4_read_pipe_lines(cmd)\n>\n> +    # Parse the change range into start and end\n> +    if changeRange is None or changeRange == '':\n> +        changeStart = '@1'\n> +        changeEnd = '#head'\n> +    else:\n> +        parts = changeRange.split(',')\n> +        assert len(parts) == 2\n> +        changeStart = parts[0]\n> +        changeEnd = parts[1]\n> +\n> +    # Accumulate change numbers in a dictionary to avoid duplicates\n>      changes = {}\n> -    for line in output:\n> -        changeNum = int(line.split(\" \")[1])\n> -        changes[changeNum] = True\n> +\n> +    for p in depotPaths:\n> +        # Retrieve changes a block at a time, to prevent running\n> +        # into a MaxScanRows error from the server.\n> +        block_size = 500\n> +        start = changeStart\n> +        end = changeEnd\n> +        get_another_block = True\n> +        while get_another_block:\n> +            new_changes = []\n> +            cmd = ['changes']\n> +            cmd += ['-m', str(block_size)]\n> +            cmd += [\"%s...%s,%s\" % (p, start, end)]\n> +            for line in p4_read_pipe_lines(cmd):\n> +                changeNum = int(line.split(\" \")[1])\n> +                new_changes.append(changeNum)\n> +                changes[changeNum] = True\n> +            if len(new_changes) == block_size:\n> +                get_another_block = True\n> +                end = '@' + str(min(new_changes))\n> +            else:\n> +                get_another_block = False\n>\n>      changelist = changes.keys()\n>      changelist.sort()\n> --\n> 1.9.1\n>\n"},{"id":"259369","messageId":"CALM2SnY1a3FO34gTu3W98N5UaC3oGhZM-AP9aHzRpFGATi31MQ@mail.gmail.com","threadId":"39071","inReplyTo":"CALM2SnY=ZcSMSXk6Ks0uU65gPX5vC8QKG+iSrQxd3X7N=sw+Ww@mail.gmail.com","subject":"Re: [PATCH] git-p4: Use -m when running p4 changes","fromName":"Lex Spoon","fromEmail":"lex@lexspoon.org","sentAt":"2015-04-14T19:48:29Z","receivedAt":"2015-04-14T19:48:29Z","isPatch":true,"sender":{"key":"lex@lexspoon.org","avatar":"https://avatars.githubusercontent.com/u/274906?v=4"},"body":"(resending with accidental HTML removed)\n\n\nGreat, I'm glad it looks like a good approach!\n\nI'll add a test case for it.... and to support the test case, an\noption for the block size. I guess the block-size option will go on\n\"sync\", \"clone\", and \"fetch\". Alternatively, maybe someone has a\nbetter suggestion of how to configure the block size.\n\nLex Spoon\n"},{"id":"259389","messageId":"CALM2SnafBHz8YeWtUtQDUgLBP_s9AiJy=9UC6XveqP0zrYMEqA@mail.gmail.com","threadId":"39071","inReplyTo":"CAE5ih79UcJfuhzgTdTPy2K51sa6--4bvaVaKL3nsUcC2kq4Ffg@mail.gmail.com","subject":"Re: [PATCH] git-p4: Use -m when running p4 changes","fromName":"Lex Spoon","fromEmail":"lex@lexspoon.org","sentAt":"2015-04-15T03:47:40Z","receivedAt":"2015-04-15T03:47:40Z","isPatch":true,"sender":{"key":"lex@lexspoon.org","avatar":"https://avatars.githubusercontent.com/u/274906?v=4"},"body":">From 9cc607667a20317c837afd90d50c078da659b72f Mon Sep 17 00:00:00 2001\nFrom: Lex Spoon <lex@lexspoon.org>\nDate: Sat, 11 Apr 2015 10:01:15 -0400\nSubject: [PATCH] git-p4: Use -m when running p4 changes\n\nSigned-off-by: Lex Spoon <lex@lexspoon.org>\n---\nUpdated to include a test case\n\n git-p4.py               | 51 ++++++++++++++++++++++++++++++---------\n t/t9818-git-p4-block.sh | 64 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 104 insertions(+), 11 deletions(-)\n create mode 100755 t/t9818-git-p4-block.sh\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 549022e..2fc8d9c 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -740,17 +740,43 @@ def\ncreateOrUpdateBranchesFromOrigin(localRefPrefix = \"refs/remotes/p4/\",\nsilent\n def originP4BranchesExist():\n         return gitBranchExists(\"origin\") or\ngitBranchExists(\"origin/p4\") or gitBranchExists(\"origin/p4/master\")\n\n-def p4ChangesForPaths(depotPaths, changeRange):\n+def p4ChangesForPaths(depotPaths, changeRange, block_size):\n     assert depotPaths\n-    cmd = ['changes']\n-    for p in depotPaths:\n-        cmd += [\"%s...%s\" % (p, changeRange)]\n-    output = p4_read_pipe_lines(cmd)\n+    assert block_size\n+\n+    # Parse the change range into start and end\n+    if changeRange is None or changeRange == '':\n+        changeStart = '@1'\n+        changeEnd = '#head'\n+    else:\n+        parts = changeRange.split(',')\n+        assert len(parts) == 2\n+        changeStart = parts[0]\n+        changeEnd = parts[1]\n\n+    # Accumulate change numbers in a dictionary to avoid duplicates\n     changes = {}\n-    for line in output:\n-        changeNum = int(line.split(\" \")[1])\n-        changes[changeNum] = True\n+\n+    for p in depotPaths:\n+        # Retrieve changes a block at a time, to prevent running\n+        # into a MaxScanRows error from the server.\n+        start = changeStart\n+        end = changeEnd\n+        get_another_block = True\n+        while get_another_block:\n+            new_changes = []\n+            cmd = ['changes']\n+            cmd += ['-m', str(block_size)]\n+            cmd += [\"%s...%s,%s\" % (p, start, end)]\n+            for line in p4_read_pipe_lines(cmd):\n+                changeNum = int(line.split(\" \")[1])\n+                new_changes.append(changeNum)\n+                changes[changeNum] = True\n+            if len(new_changes) == block_size:\n+                get_another_block = True\n+                end = '@' + str(min(new_changes))\n+            else:\n+                get_another_block = False\n\n     changelist = changes.keys()\n     changelist.sort()\n@@ -1912,6 +1938,8 @@ class P4Sync(Command, P4UserMap):\n                 optparse.make_option(\"--import-local\",\ndest=\"importIntoRemotes\", action=\"store_false\",\n                                      help=\"Import into refs/heads/ ,\nnot refs/remotes\"),\n                 optparse.make_option(\"--max-changes\", dest=\"maxChanges\"),\n+                optparse.make_option(\"--changes-block-size\",\ndest=\"changes_block_size\", type=\"int\",\n+                                     help=\"Block size for calling p4 changes\"),\n                 optparse.make_option(\"--keep-path\",\ndest=\"keepRepoPath\", action='store_true',\n                                      help=\"Keep entire\nBRANCH/DIR/SUBDIR prefix during import\"),\n                 optparse.make_option(\"--use-client-spec\",\ndest=\"useClientSpec\", action='store_true',\n@@ -1940,6 +1968,7 @@ class P4Sync(Command, P4UserMap):\n         self.syncWithOrigin = True\n         self.importIntoRemotes = True\n         self.maxChanges = \"\"\n+        self.changes_block_size = 500\n         self.keepRepoPath = False\n         self.depotPaths = None\n         self.p4BranchesInGit = []\n@@ -2578,7 +2607,7 @@ class P4Sync(Command, P4UserMap):\n\n         return \"\"\n\n-    def importNewBranch(self, branch, maxChange):\n+    def importNewBranch(self, branch, maxChange, changes_block_size):\n         # make fast-import flush all changes to disk and update the\nrefs using the checkpoint\n         # command so that we can try to find the branch parent in the\ngit history\n         self.gitStream.write(\"checkpoint\\n\\n\");\n@@ -2586,7 +2615,7 @@ class P4Sync(Command, P4UserMap):\n         branchPrefix = self.depotPaths[0] + branch + \"/\"\n         range = \"@1,%s\" % maxChange\n         #print \"prefix\" + branchPrefix\n-        changes = p4ChangesForPaths([branchPrefix], range)\n+        changes = p4ChangesForPaths([branchPrefix], range, changes_block_size)\n         if len(changes) <= 0:\n             return False\n         firstChange = changes[0]\n@@ -3002,7 +3031,7 @@ class P4Sync(Command, P4UserMap):\n                 if self.verbose:\n                     print \"Getting p4 changes for %s...%s\" % (',\n'.join(self.depotPaths),\n                                                               self.changeRange)\n-                changes = p4ChangesForPaths(self.depotPaths, self.changeRange)\n+                changes = p4ChangesForPaths(self.depotPaths,\nself.changeRange, self.changes_block_size)\n\n                 if len(self.maxChanges) > 0:\n                     changes = changes[:min(int(self.maxChanges), len(changes))]\ndiff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh\nnew file mode 100755\nindex 0000000..73e545d\n--- /dev/null\n+++ b/t/t9818-git-p4-block.sh\n@@ -0,0 +1,64 @@\n+#!/bin/sh\n+\n+test_description='git p4 fetching changes in multiple blocks'\n+\n+. ./lib-git-p4.sh\n+\n+test_expect_success 'start p4d' '\n+ start_p4d\n+'\n+\n+test_expect_success 'Create a repo with 100 changes' '\n+ (\n+ cd \"$cli\" &&\n+ touch file.txt &&\n+ p4 add file.txt &&\n+ p4 submit -d \"Add file.txt\" &&\n+ for i in 0 1 2 3 4 5 6 7 8 9\n+ do\n+ touch outer$i.txt &&\n+ p4 add outer$i.txt &&\n+ p4 submit -d \"Adding outer$i.txt\" &&\n+ for j in 0 1 2 3 4 5 6 7 8 9\n+ do\n+ p4 edit file.txt &&\n+ echo $i$j > file.txt &&\n+ p4 submit -d \"Commit $i$j\"\n+ done\n+ done\n+ )\n+'\n+\n+test_expect_success 'Clone the repo' '\n+ git p4 clone --dest=\"$git\" --changes-block-size=10 --verbose //depot@all\n+'\n+\n+test_expect_success 'All files are present' '\n+ echo file.txt >expected &&\n+ test_write_lines outer0.txt outer1.txt outer2.txt outer3.txt\nouter4.txt >>expected &&\n+ test_write_lines outer5.txt outer6.txt outer7.txt outer8.txt\nouter9.txt >>expected &&\n+ ls \"$git\" >current &&\n+ test_cmp expected current\n+'\n+\n+test_expect_success 'file.txt is correct' '\n+ echo 99 >expected &&\n+ test_cmp expected \"$git/file.txt\"\n+'\n+\n+test_expect_success 'Correct number of commits' '\n+ (cd \"$git\"; git log --oneline) >log &&\n+ test_line_count = 111 log\n+'\n+\n+test_expect_success 'Previous version of file.txt is correct' '\n+ (cd \"$git\"; git checkout HEAD^^) &&\n+ echo 97 >expected &&\n+ test_cmp expected \"$git/file.txt\"\n+'\n+\n+test_expect_success 'kill p4d' '\n+ kill_p4d\n+'\n+\n+test_done\n-- \n1.9.1\n"},{"id":"259534","messageId":"xmqqfv7zj40w.fsf@gitster.dls.corp.google.com","threadId":"39071","inReplyTo":"CALM2SnafBHz8YeWtUtQDUgLBP_s9AiJy=9UC6XveqP0zrYMEqA@mail.gmail.com","subject":"Re: [PATCH] git-p4: Use -m when running p4 changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-16T18:56:31Z","receivedAt":"2015-04-16T18:56:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lex Spoon <lex@lexspoon.org> writes:\n\n> From 9cc607667a20317c837afd90d50c078da659b72f Mon Sep 17 00:00:00 2001\n> From: Lex Spoon <lex@lexspoon.org>\n> Date: Sat, 11 Apr 2015 10:01:15 -0400\n> Subject: [PATCH] git-p4: Use -m when running p4 changes\n\nAll of the above is duplicate and shouldn't be added to the message;\nthe recipient can pick them up from the e-mail headers.\n\nPlease explain what this change intends to do (e.g. Is it a fix?  If\nso, what is broken without this change?  Is it an enhancement?  If\nso, what cannot be done without this change, and how and why is the\nnew thing the change enables a good thing?), and why it is a good\nidea to use \"-m\" to realize that objective.\n\n> Signed-off-by: Lex Spoon <lex@lexspoon.org>\n> ---\n> Updated to include a test case\n>\n>  git-p4.py               | 51 ++++++++++++++++++++++++++++++---------\n>  t/t9818-git-p4-block.sh | 64 +++++++++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 104 insertions(+), 11 deletions(-)\n>  create mode 100755 t/t9818-git-p4-block.sh\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index 549022e..2fc8d9c 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -740,17 +740,43 @@ def\n> createOrUpdateBranchesFromOrigin(localRefPrefix = \"refs/remotes/p4/\",\n> silent\n>  def originP4BranchesExist():\n>          return gitBranchExists(\"origin\") or\n> gitBranchExists(\"origin/p4\") or gitBranchExists(\"origin/p4/master\")\n\nIt appears that the patch is severely linewrapped.\n\n> diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh\n> new file mode 100755\n> index 0000000..73e545d\n> --- /dev/null\n> +++ b/t/t9818-git-p4-block.sh\n> @@ -0,0 +1,64 @@\n> +#!/bin/sh\n> +\n> +test_description='git p4 fetching changes in multiple blocks'\n> +\n> +. ./lib-git-p4.sh\n> +\n> +test_expect_success 'start p4d' '\n> + start_p4d\n> +'\n\nWe do not do one-SP indent.  Indent with tab instead.\n\n> +\n> +test_expect_success 'Create a repo with 100 changes' '\n> + (\n> + cd \"$cli\" &&\n> + touch file.txt &&\n\nDo not use \"touch\" when the only thing you are interested in is that\nthe file exists and you do not care about its timestamp.  I.e. say\n\n    >file.txt &&\n\ninstead.\n\n> + p4 add file.txt &&\n> + p4 submit -d \"Add file.txt\" &&\n> + for i in 0 1 2 3 4 5 6 7 8 9\n> + do\n> + touch outer$i.txt &&\n> + p4 add outer$i.txt &&\n> + p4 submit -d \"Adding outer$i.txt\" &&\n> + for j in 0 1 2 3 4 5 6 7 8 9\n> + do\n> + p4 edit file.txt &&\n> + echo $i$j > file.txt &&\n> + p4 submit -d \"Commit $i$j\"\n> + done\n> + done\n> + )\n\nWhat happens when any of these commands in the &&-chain fails?\n\n\t(\n        \tcd \"$cli\" &&\n                >file.txt &&\n\t\tp4 ... &&\n                for i in $(test_seq ...)\n                do\n                \t>\"outer$i.txt\" &&\n                        p4 ... &&\n\t\t\tfor j in $(test_seq ...)\n\t\t\tdo\n\t\t\t\tp4 ... &&\n\t\t\t\tp4 ... || exit\n\t\t\tdone\n\t\tdone\n\t)\n\nor something like that, perhaps?\n"},{"id":"259546","messageId":"55304290.9070907@diamand.org","threadId":"39071","inReplyTo":"CALM2SnafBHz8YeWtUtQDUgLBP_s9AiJy=9UC6XveqP0zrYMEqA@mail.gmail.com","subject":"Re: [PATCH] git-p4: Use -m when running p4 changes","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-04-16T23:15:28Z","receivedAt":"2015-04-16T23:15:28Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 15/04/15 04:47, Lex Spoon wrote:\n>  From 9cc607667a20317c837afd90d50c078da659b72f Mon Sep 17 00:00:00 2001\n> From: Lex Spoon <lex@lexspoon.org>\n> Date: Sat, 11 Apr 2015 10:01:15 -0400\n> Subject: [PATCH] git-p4: Use -m when running p4 changes\n\nThis patch didn't want to apply for me, I'm not quite sure why but \npossibly it's become scrambled? Either that or I'm doing it wrong! If \nyou use git send-email it should Just Work.\n\nAs an aside could you post reworked versions of patches with a subject \nline of [PATCH v2], [PATCH v3], etc, so reviewers can keep track of \nwhat's going on?\n\nNote to other reviewers: the existing git-p4 has a --max-changes option \nfor 'sync', but this doesn't do the same thing at all. It doesn't limit \nthe number of changes requested from the server, it just limits the \nnumber of changes pulled down, after the p4 server has supplied those \nchanges. This confused me at first!\n\nLex - I should have mentioned this before, but would you be able to add \nsome documentation to Documentation/git-p4.txt to explain what your new \noption does? It would help to distinguish between your option and the \nexisting --max-changes option.\n\nI've put a few remarks below in your shell script; there are a few minor \nissues that could do with being tidied up.\n\nThanks!\nLuke\n\n<snip>\n\n> diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh\n> new file mode 100755\n> index 0000000..73e545d\n> --- /dev/null\n> +++ b/t/t9818-git-p4-block.sh\n> @@ -0,0 +1,64 @@\n> +#!/bin/sh\n> +\n> +test_description='git p4 fetching changes in multiple blocks'\n> +\n> +. ./lib-git-p4.sh\n> +\n> +test_expect_success 'start p4d' '\n> + start_p4d\n> +'\n> +\n> +test_expect_success 'Create a repo with 100 changes' '\n> + (\n> + cd \"$cli\" &&\n\nThis doesn't look like enough indentation. The tests normally get a hard \ntab indent at each level.\n\n> + touch file.txt &&\n> + p4 add file.txt &&\n> + p4 submit -d \"Add file.txt\" &&\n> + for i in 0 1 2 3 4 5 6 7 8 9\n> + do\n> + touch outer$i.txt &&\n> + p4 add outer$i.txt &&\n> + p4 submit -d \"Adding outer$i.txt\" &&\n> + for j in 0 1 2 3 4 5 6 7 8 9\n> + do\n> + p4 edit file.txt &&\n> + echo $i$j > file.txt &&\n\nPlease put the file argument immediately after the redirection, i.e.\n\n    echo $i$j >file.txt &&\n\n(Which you've done below in fact).\n\n> + p4 submit -d \"Commit $i$j\"\n> + done\n> + done\n> + )\n> +'\n> +\n> +test_expect_success 'Clone the repo' '\n> + git p4 clone --dest=\"$git\" --changes-block-size=10 --verbose //depot@all\n> +'\n> +\n> +test_expect_success 'All files are present' '\n> + echo file.txt >expected &&\n> + test_write_lines outer0.txt outer1.txt outer2.txt outer3.txt\n> outer4.txt >>expected &&\n> + test_write_lines outer5.txt outer6.txt outer7.txt outer8.txt\n> outer9.txt >>expected &&\n> + ls \"$git\" >current &&\n> + test_cmp expected current\n> +'\n> +\n> +test_expect_success 'file.txt is correct' '\n> + echo 99 >expected &&\n> + test_cmp expected \"$git/file.txt\"\n> +'\n> +\n> +test_expect_success 'Correct number of commits' '\n> + (cd \"$git\"; git log --oneline) >log &&\n\nUse \"&&\" rather than \";\"\n\n> + test_line_count = 111 log\n> +'\n> +\n> +test_expect_success 'Previous version of file.txt is correct' '\n> + (cd \"$git\"; git checkout HEAD^^) &&\n\nAs above.\n\n> + echo 97 >expected &&\n> + test_cmp expected \"$git/file.txt\"\n> +'\n> +\n> +test_expect_success 'kill p4d' '\n> + kill_p4d\n> +'\n> +\n> +test_done\n>\n\nLooks good other than that (+Junio's comments).\n\nThanks!\nLuke\n"},{"id":"259560","messageId":"CALM2SnZmCJ2nVqPyLiepF1zJH=S0BzCTM=-L6hnn8Vnrb+prCw@mail.gmail.com","threadId":"39071","inReplyTo":"55304290.9070907@diamand.org","subject":"Re: [PATCH] git-p4: Use -m when running p4 changes","fromName":"Lex Spoon","fromEmail":"lex@lexspoon.org","sentAt":"2015-04-17T13:20:21Z","receivedAt":"2015-04-17T13:20:21Z","isPatch":true,"sender":{"key":"lex@lexspoon.org","avatar":"https://avatars.githubusercontent.com/u/274906?v=4"},"body":"Thanks, all. I will update the patch as requested and resend a [PATCH\nv3]. This time without the redundant headers. I will also make an\nextra effort to make sure that the raw tabs do not get converted to\nspaces this time. Oof, I am really out of practice at programming with\nraw tabs, much less getting them to make it through email software.\nThank you for your patience.\n\ntest_seq is a neat utility. Also, I don't know why I didn't think to\nupdate the document page. Certainly it needs to be updated.\n\n\nLex Spoon\n"},{"id":"259607","messageId":"1429312285-13552-1-git-send-email-lex@lexspoon.org","threadId":"39071","inReplyTo":"CALM2SnZmCJ2nVqPyLiepF1zJH=S0BzCTM=-L6hnn8Vnrb+prCw@mail.gmail.com","subject":"[PATCH v3] git-p4: Use -m when running p4 changes","fromName":"Lex Spoon","fromEmail":"lex@lexspoon.org","sentAt":"2015-04-17T23:11:25Z","receivedAt":"2015-04-17T23:11:25Z","isPatch":true,"sender":{"key":"lex@lexspoon.org","avatar":"https://avatars.githubusercontent.com/u/274906?v=4"},"body":"Simply running \"p4 changes\" on a large branch can\nresult in a \"too many rows scanned\" error from the\nPerforce server. It is better to use a sequence\nof smaller calls to \"p4 changes\", using the \"-m\"\noption to limit the size of each call.\n\nSigned-off-by: Lex Spoon <lex@lexspoon.org>\nReviewed-by: Junio C Hamano <gitster@pobox.com>\nReviewed-by: Luke Diamand <luke@diamand.org>\n---\nUpdated as suggested:\n- documentation added\n- avoided touch(1)\n- used test_seq\n- used || exit for test commands inside for loops\n- more tabs\n- fewer line breaks\n- expanded commit message\n\n Documentation/git-p4.txt | 17 ++++++++++---\n git-p4.py                | 54 +++++++++++++++++++++++++++++++---------\n t/t9818-git-p4-block.sh  | 64 ++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 120 insertions(+), 15 deletions(-)\n create mode 100755 t/t9818-git-p4-block.sh\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex a1664b9..82aa5d6 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -225,9 +225,20 @@ Git repository:\n \tthey can find the p4 branches in refs/heads.\n \n --max-changes <n>::\n-\tLimit the number of imported changes to 'n'.  Useful to\n-\tlimit the amount of history when using the '@all' p4 revision\n-\tspecifier.\n+\tImport at most 'n' changes, rather than the entire range of\n+\tchanges included in the given revision specifier. A typical\n+\tusage would be use '@all' as the revision specifier, but then\n+\tto use '--max-changes 1000' to import only the last 1000\n+\trevisions rather than the entire revision history.\n+\n+--changes-block-size <n>::\n+\tThe internal block size to use when converting a revision\n+\tspecifier such as '@all' into a list of specific change\n+\tnumbers. Instead of using a single call to 'p4 changes' to\n+\tfind the full list of changes for the conversion, there are a\n+\tsequence of calls to 'p4 changes -m', each of which requests\n+\tone block of changes of the given size. The default block size\n+\tis 500, which should usually be suitable.\n \n --keep-path::\n \tThe mapping of file names from the p4 depot path to Git, by\ndiff --git a/git-p4.py b/git-p4.py\nindex 549022e..1fba3aa 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -740,17 +740,43 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = \"refs/remotes/p4/\", silent\n def originP4BranchesExist():\n         return gitBranchExists(\"origin\") or gitBranchExists(\"origin/p4\") or gitBranchExists(\"origin/p4/master\")\n \n-def p4ChangesForPaths(depotPaths, changeRange):\n+def p4ChangesForPaths(depotPaths, changeRange, block_size):\n     assert depotPaths\n-    cmd = ['changes']\n-    for p in depotPaths:\n-        cmd += [\"%s...%s\" % (p, changeRange)]\n-    output = p4_read_pipe_lines(cmd)\n+    assert block_size\n+\n+    # Parse the change range into start and end\n+    if changeRange is None or changeRange == '':\n+        changeStart = '@1'\n+        changeEnd = '#head'\n+    else:\n+        parts = changeRange.split(',')\n+        assert len(parts) == 2\n+        changeStart = parts[0]\n+        changeEnd = parts[1]\n \n+    # Accumulate change numbers in a dictionary to avoid duplicates\n     changes = {}\n-    for line in output:\n-        changeNum = int(line.split(\" \")[1])\n-        changes[changeNum] = True\n+\n+    for p in depotPaths:\n+        # Retrieve changes a block at a time, to prevent running\n+        # into a MaxScanRows error from the server.\n+        start = changeStart\n+        end = changeEnd\n+        get_another_block = True\n+        while get_another_block:\n+            new_changes = []\n+            cmd = ['changes']\n+            cmd += ['-m', str(block_size)]\n+            cmd += [\"%s...%s,%s\" % (p, start, end)]\n+            for line in p4_read_pipe_lines(cmd):\n+                changeNum = int(line.split(\" \")[1])\n+                new_changes.append(changeNum)\n+                changes[changeNum] = True\n+            if len(new_changes) == block_size:\n+                get_another_block = True\n+                end = '@' + str(min(new_changes))\n+            else:\n+                get_another_block = False\n \n     changelist = changes.keys()\n     changelist.sort()\n@@ -1911,7 +1937,10 @@ class P4Sync(Command, P4UserMap):\n                 optparse.make_option(\"--import-labels\", dest=\"importLabels\", action=\"store_true\"),\n                 optparse.make_option(\"--import-local\", dest=\"importIntoRemotes\", action=\"store_false\",\n                                      help=\"Import into refs/heads/ , not refs/remotes\"),\n-                optparse.make_option(\"--max-changes\", dest=\"maxChanges\"),\n+                optparse.make_option(\"--max-changes\", dest=\"maxChanges\",\n+                                     help=\"Maximum number of changes to import\"),\n+                optparse.make_option(\"--changes-block-size\", dest=\"changes_block_size\", type=\"int\",\n+                                     help=\"Internal block size to use when iteratively calling p4 changes\"),\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(\"--use-client-spec\", dest=\"useClientSpec\", action='store_true',\n@@ -1940,6 +1969,7 @@ class P4Sync(Command, P4UserMap):\n         self.syncWithOrigin = True\n         self.importIntoRemotes = True\n         self.maxChanges = \"\"\n+        self.changes_block_size = 500\n         self.keepRepoPath = False\n         self.depotPaths = None\n         self.p4BranchesInGit = []\n@@ -2578,7 +2608,7 @@ class P4Sync(Command, P4UserMap):\n \n         return \"\"\n \n-    def importNewBranch(self, branch, maxChange):\n+    def importNewBranch(self, branch, maxChange, changes_block_size):\n         # make fast-import flush all changes to disk and update the refs using the checkpoint\n         # command so that we can try to find the branch parent in the git history\n         self.gitStream.write(\"checkpoint\\n\\n\");\n@@ -2586,7 +2616,7 @@ class P4Sync(Command, P4UserMap):\n         branchPrefix = self.depotPaths[0] + branch + \"/\"\n         range = \"@1,%s\" % maxChange\n         #print \"prefix\" + branchPrefix\n-        changes = p4ChangesForPaths([branchPrefix], range)\n+        changes = p4ChangesForPaths([branchPrefix], range, changes_block_size)\n         if len(changes) <= 0:\n             return False\n         firstChange = changes[0]\n@@ -3002,7 +3032,7 @@ class P4Sync(Command, P4UserMap):\n                 if self.verbose:\n                     print \"Getting p4 changes for %s...%s\" % (', '.join(self.depotPaths),\n                                                               self.changeRange)\n-                changes = p4ChangesForPaths(self.depotPaths, self.changeRange)\n+                changes = p4ChangesForPaths(self.depotPaths, self.changeRange, self.changes_block_size)\n \n                 if len(self.maxChanges) > 0:\n                     changes = changes[:min(int(self.maxChanges), len(changes))]\ndiff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh\nnew file mode 100755\nindex 0000000..153b20a\n--- /dev/null\n+++ b/t/t9818-git-p4-block.sh\n@@ -0,0 +1,64 @@\n+#!/bin/sh\n+\n+test_description='git p4 fetching changes in multiple blocks'\n+\n+. ./lib-git-p4.sh\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'Create a repo with ~100 changes' '\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\t>file.txt &&\n+\t\tp4 add file.txt &&\n+\t\tp4 submit -d \"Add file.txt\" &&\n+\t\tfor i in $(test_seq 0 9)\n+\t\tdo\n+\t\t\t>outer$i.txt &&\n+\t\t\tp4 add outer$i.txt &&\n+\t\t\tp4 submit -d \"Adding outer$i.txt\" &&\n+\t\t\tfor j in $(test_seq 0 9)\n+\t\t\tdo\n+\t\t\t\tp4 edit file.txt &&\n+\t\t\t\techo $i$j >file.txt &&\n+\t\t\t\tp4 submit -d \"Commit $i$j\" || exit\n+\t\t\tdone || exit\n+\t\tdone\n+\t)\n+'\n+\n+test_expect_success 'Clone the repo' '\n+\tgit p4 clone --dest=\"$git\" --changes-block-size=10 --verbose //depot@all\n+'\n+\n+test_expect_success 'All files are present' '\n+\techo file.txt >expected &&\n+\ttest_write_lines outer0.txt outer1.txt outer2.txt outer3.txt outer4.txt >>expected &&\n+\ttest_write_lines outer5.txt outer6.txt outer7.txt outer8.txt outer9.txt >>expected &&\n+\tls \"$git\" >current &&\n+\ttest_cmp expected current\n+'\n+\n+test_expect_success 'file.txt is correct' '\n+\techo 99 >expected &&\n+\ttest_cmp expected \"$git/file.txt\"\n+'\n+\n+test_expect_success 'Correct number of commits' '\n+\t(cd \"$git\" && git log --oneline) >log &&\n+\ttest_line_count = 111 log\n+'\n+\n+test_expect_success 'Previous version of file.txt is correct' '\n+\t(cd \"$git\" && git checkout HEAD^^) &&\n+\techo 97 >expected &&\n+\ttest_cmp expected \"$git/file.txt\"\n+'\n+\n+test_expect_success 'kill p4d' '\n+\tkill_p4d\n+'\n+\n+test_done\n-- \n1.9.1\n"},{"id":"259669","messageId":"5534CC83.2000304@diamand.org","threadId":"39071","inReplyTo":"1429312285-13552-1-git-send-email-lex@lexspoon.org","subject":"Re: [PATCH v3] git-p4: Use -m when running p4 changes","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-04-20T09:53:07Z","receivedAt":"2015-04-20T09:53:07Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 18/04/15 00:11, Lex Spoon wrote:\n> Simply running \"p4 changes\" on a large branch can\n> result in a \"too many rows scanned\" error from the\n> Perforce server. It is better to use a sequence\n> of smaller calls to \"p4 changes\", using the \"-m\"\n> option to limit the size of each call.\n>\n> Signed-off-by: Lex Spoon <lex@lexspoon.org>\n> Reviewed-by: Junio C Hamano <gitster@pobox.com>\n> Reviewed-by: Luke Diamand <luke@diamand.org>\n\nI could be wrong about this, but it looks like importNewBranches() is \ntaking an extra argument, but that isn't reflected in the place where it \ngets called. I think it just got missed.\n\nAs a result, t9801-git-p4-branch.sh fails with this error:\n\nImporting revision 3 (37%)\n     Importing new branch depot/branch1\nTraceback (most recent call last):\n   File \"/home/lgd/git/git/git-p4\", line 3327, in <module>\n     main()\n   File \"/home/lgd/git/git/git-p4\", line 3321, in main\n     if not cmd.run(args):\n   File \"/home/lgd/git/git/git-p4\", line 3195, in run\n     if not P4Sync.run(self, depotPaths):\n   File \"/home/lgd/git/git/git-p4\", line 3057, in run\n     self.importChanges(changes)\n   File \"/home/lgd/git/git/git-p4\", line 2692, in importChanges\n     if self.importNewBranch(branch, change - 1):\nTypeError: importNewBranch() takes exactly 4 arguments (3 given)\nrm: cannot remove `/home/lgd/git/git/t/trash \ndirectory.t9801-git-p4-branch/git/.git/objects/pack': Directory not empty\nnot ok 8 - import depot, branch detection, branchList branch definition\n\n\nThanks!\nLuke\n\n\n> ---\n> Updated as suggested:\n> - documentation added\n> - avoided touch(1)\n> - used test_seq\n> - used || exit for test commands inside for loops\n> - more tabs\n> - fewer line breaks\n> - expanded commit message\n>\n>   Documentation/git-p4.txt | 17 ++++++++++---\n>   git-p4.py                | 54 +++++++++++++++++++++++++++++++---------\n>   t/t9818-git-p4-block.sh  | 64 ++++++++++++++++++++++++++++++++++++++++++++++++\n>   3 files changed, 120 insertions(+), 15 deletions(-)\n>   create mode 100755 t/t9818-git-p4-block.sh\n>\n> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n> index a1664b9..82aa5d6 100644\n> --- a/Documentation/git-p4.txt\n> +++ b/Documentation/git-p4.txt\n> @@ -225,9 +225,20 @@ Git repository:\n>   \tthey can find the p4 branches in refs/heads.\n>\n>   --max-changes <n>::\n> -\tLimit the number of imported changes to 'n'.  Useful to\n> -\tlimit the amount of history when using the '@all' p4 revision\n> -\tspecifier.\n> +\tImport at most 'n' changes, rather than the entire range of\n> +\tchanges included in the given revision specifier. A typical\n> +\tusage would be use '@all' as the revision specifier, but then\n> +\tto use '--max-changes 1000' to import only the last 1000\n> +\trevisions rather than the entire revision history.\n> +\n> +--changes-block-size <n>::\n> +\tThe internal block size to use when converting a revision\n> +\tspecifier such as '@all' into a list of specific change\n> +\tnumbers. Instead of using a single call to 'p4 changes' to\n> +\tfind the full list of changes for the conversion, there are a\n> +\tsequence of calls to 'p4 changes -m', each of which requests\n> +\tone block of changes of the given size. The default block size\n> +\tis 500, which should usually be suitable.\n>\n>   --keep-path::\n>   \tThe mapping of file names from the p4 depot path to Git, by\n> diff --git a/git-p4.py b/git-p4.py\n> index 549022e..1fba3aa 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -740,17 +740,43 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = \"refs/remotes/p4/\", silent\n>   def originP4BranchesExist():\n>           return gitBranchExists(\"origin\") or gitBranchExists(\"origin/p4\") or gitBranchExists(\"origin/p4/master\")\n>\n> -def p4ChangesForPaths(depotPaths, changeRange):\n> +def p4ChangesForPaths(depotPaths, changeRange, block_size):\n>       assert depotPaths\n> -    cmd = ['changes']\n> -    for p in depotPaths:\n> -        cmd += [\"%s...%s\" % (p, changeRange)]\n> -    output = p4_read_pipe_lines(cmd)\n> +    assert block_size\n> +\n> +    # Parse the change range into start and end\n> +    if changeRange is None or changeRange == '':\n> +        changeStart = '@1'\n> +        changeEnd = '#head'\n> +    else:\n> +        parts = changeRange.split(',')\n> +        assert len(parts) == 2\n> +        changeStart = parts[0]\n> +        changeEnd = parts[1]\n>\n> +    # Accumulate change numbers in a dictionary to avoid duplicates\n>       changes = {}\n> -    for line in output:\n> -        changeNum = int(line.split(\" \")[1])\n> -        changes[changeNum] = True\n> +\n> +    for p in depotPaths:\n> +        # Retrieve changes a block at a time, to prevent running\n> +        # into a MaxScanRows error from the server.\n> +        start = changeStart\n> +        end = changeEnd\n> +        get_another_block = True\n> +        while get_another_block:\n> +            new_changes = []\n> +            cmd = ['changes']\n> +            cmd += ['-m', str(block_size)]\n> +            cmd += [\"%s...%s,%s\" % (p, start, end)]\n> +            for line in p4_read_pipe_lines(cmd):\n> +                changeNum = int(line.split(\" \")[1])\n> +                new_changes.append(changeNum)\n> +                changes[changeNum] = True\n> +            if len(new_changes) == block_size:\n> +                get_another_block = True\n> +                end = '@' + str(min(new_changes))\n> +            else:\n> +                get_another_block = False\n>\n>       changelist = changes.keys()\n>       changelist.sort()\n> @@ -1911,7 +1937,10 @@ class P4Sync(Command, P4UserMap):\n>                   optparse.make_option(\"--import-labels\", dest=\"importLabels\", action=\"store_true\"),\n>                   optparse.make_option(\"--import-local\", dest=\"importIntoRemotes\", action=\"store_false\",\n>                                        help=\"Import into refs/heads/ , not refs/remotes\"),\n> -                optparse.make_option(\"--max-changes\", dest=\"maxChanges\"),\n> +                optparse.make_option(\"--max-changes\", dest=\"maxChanges\",\n> +                                     help=\"Maximum number of changes to import\"),\n> +                optparse.make_option(\"--changes-block-size\", dest=\"changes_block_size\", type=\"int\",\n> +                                     help=\"Internal block size to use when iteratively calling p4 changes\"),\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(\"--use-client-spec\", dest=\"useClientSpec\", action='store_true',\n> @@ -1940,6 +1969,7 @@ class P4Sync(Command, P4UserMap):\n>           self.syncWithOrigin = True\n>           self.importIntoRemotes = True\n>           self.maxChanges = \"\"\n> +        self.changes_block_size = 500\n>           self.keepRepoPath = False\n>           self.depotPaths = None\n>           self.p4BranchesInGit = []\n> @@ -2578,7 +2608,7 @@ class P4Sync(Command, P4UserMap):\n>\n>           return \"\"\n>\n> -    def importNewBranch(self, branch, maxChange):\n> +    def importNewBranch(self, branch, maxChange, changes_block_size):\n>           # make fast-import flush all changes to disk and update the refs using the checkpoint\n>           # command so that we can try to find the branch parent in the git history\n>           self.gitStream.write(\"checkpoint\\n\\n\");\n> @@ -2586,7 +2616,7 @@ class P4Sync(Command, P4UserMap):\n>           branchPrefix = self.depotPaths[0] + branch + \"/\"\n>           range = \"@1,%s\" % maxChange\n>           #print \"prefix\" + branchPrefix\n> -        changes = p4ChangesForPaths([branchPrefix], range)\n> +        changes = p4ChangesForPaths([branchPrefix], range, changes_block_size)\n>           if len(changes) <= 0:\n>               return False\n>           firstChange = changes[0]\n> @@ -3002,7 +3032,7 @@ class P4Sync(Command, P4UserMap):\n>                   if self.verbose:\n>                       print \"Getting p4 changes for %s...%s\" % (', '.join(self.depotPaths),\n>                                                                 self.changeRange)\n> -                changes = p4ChangesForPaths(self.depotPaths, self.changeRange)\n> +                changes = p4ChangesForPaths(self.depotPaths, self.changeRange, self.changes_block_size)\n>\n>                   if len(self.maxChanges) > 0:\n>                       changes = changes[:min(int(self.maxChanges), len(changes))]\n> diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh\n> new file mode 100755\n> index 0000000..153b20a\n> --- /dev/null\n> +++ b/t/t9818-git-p4-block.sh\n> @@ -0,0 +1,64 @@\n> +#!/bin/sh\n> +\n> +test_description='git p4 fetching changes in multiple blocks'\n> +\n> +. ./lib-git-p4.sh\n> +\n> +test_expect_success 'start p4d' '\n> +\tstart_p4d\n> +'\n> +\n> +test_expect_success 'Create a repo with ~100 changes' '\n> +\t(\n> +\t\tcd \"$cli\" &&\n> +\t\t>file.txt &&\n> +\t\tp4 add file.txt &&\n> +\t\tp4 submit -d \"Add file.txt\" &&\n> +\t\tfor i in $(test_seq 0 9)\n> +\t\tdo\n> +\t\t\t>outer$i.txt &&\n> +\t\t\tp4 add outer$i.txt &&\n> +\t\t\tp4 submit -d \"Adding outer$i.txt\" &&\n> +\t\t\tfor j in $(test_seq 0 9)\n> +\t\t\tdo\n> +\t\t\t\tp4 edit file.txt &&\n> +\t\t\t\techo $i$j >file.txt &&\n> +\t\t\t\tp4 submit -d \"Commit $i$j\" || exit\n> +\t\t\tdone || exit\n> +\t\tdone\n> +\t)\n> +'\n> +\n> +test_expect_success 'Clone the repo' '\n> +\tgit p4 clone --dest=\"$git\" --changes-block-size=10 --verbose //depot@all\n> +'\n> +\n> +test_expect_success 'All files are present' '\n> +\techo file.txt >expected &&\n> +\ttest_write_lines outer0.txt outer1.txt outer2.txt outer3.txt outer4.txt >>expected &&\n> +\ttest_write_lines outer5.txt outer6.txt outer7.txt outer8.txt outer9.txt >>expected &&\n> +\tls \"$git\" >current &&\n> +\ttest_cmp expected current\n> +'\n> +\n> +test_expect_success 'file.txt is correct' '\n> +\techo 99 >expected &&\n> +\ttest_cmp expected \"$git/file.txt\"\n> +'\n> +\n> +test_expect_success 'Correct number of commits' '\n> +\t(cd \"$git\" && git log --oneline) >log &&\n> +\ttest_line_count = 111 log\n> +'\n> +\n> +test_expect_success 'Previous version of file.txt is correct' '\n> +\t(cd \"$git\" && git checkout HEAD^^) &&\n> +\techo 97 >expected &&\n> +\ttest_cmp expected \"$git/file.txt\"\n> +'\n> +\n> +test_expect_success 'kill p4d' '\n> +\tkill_p4d\n> +'\n> +\n> +test_done\n>\n"},{"id":"259675","messageId":"CALM2SnY4GZDSYOjLmDqdq9SgGGywRO2A3XU3639E_0JAh-2P5A@mail.gmail.com","threadId":"39071","inReplyTo":"5534CC83.2000304@diamand.org","subject":"Re: [PATCH v3] git-p4: Use -m when running p4 changes","fromName":"Lex Spoon","fromEmail":"lex@lexspoon.org","sentAt":"2015-04-20T14:30:27Z","receivedAt":"2015-04-20T14:30:27Z","isPatch":true,"sender":{"key":"lex@lexspoon.org","avatar":"https://avatars.githubusercontent.com/u/274906?v=4"},"body":"On Mon, Apr 20, 2015 at 5:53 AM, Luke Diamand <luke@diamand.org> wrote:\n> I could be wrong about this, but it looks like importNewBranches() is taking\n> an extra argument, but that isn't reflected in the place where it gets\n> called. I think it just got missed.\n>\n> As a result, t9801-git-p4-branch.sh fails with this error:\n\nOh dear, definitely. The argument can in fact be dropped, because it's\nalready already available via a field of the same object. I post an\nupdate with that change.  -Lex\n"},{"id":"259676","messageId":"1429542020-11121-1-git-send-email-lex@lexspoon.org","threadId":"39071","inReplyTo":"CALM2SnY4GZDSYOjLmDqdq9SgGGywRO2A3XU3639E_0JAh-2P5A@mail.gmail.com","subject":"[PATCH v4] git-p4: Use -m when running p4 changes","fromName":"Lex Spoon","fromEmail":"lex@lexspoon.org","sentAt":"2015-04-20T15:00:20Z","receivedAt":"2015-04-20T15:00:20Z","isPatch":true,"sender":{"key":"lex@lexspoon.org","avatar":"https://avatars.githubusercontent.com/u/274906?v=4"},"body":"Simply running \"p4 changes\" on a large branch can\nresult in a \"too many rows scanned\" error from the\nPerforce server. It is better to use a sequence\nof smaller calls to \"p4 changes\", using the \"-m\"\noption to limit the size of each call.\n\nSigned-off-by: Lex Spoon <lex@lexspoon.org>\nReviewed-by: Junio C Hamano <gitster@pobox.com>\nReviewed-by: Luke Diamand <luke@diamand.org>\n---\nUpdated to avoid the crash Luke pointed out.\nAll t98* tests pass now except for t9814,\nwhich is already failing on master for some reason.\n\n Documentation/git-p4.txt | 17 ++++++++++---\n git-p4.py                | 52 ++++++++++++++++++++++++++++++---------\n t/t9818-git-p4-block.sh  | 64 ++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 119 insertions(+), 14 deletions(-)\n create mode 100755 t/t9818-git-p4-block.sh\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex a1664b9..82aa5d6 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -225,9 +225,20 @@ Git repository:\n \tthey can find the p4 branches in refs/heads.\n \n --max-changes <n>::\n-\tLimit the number of imported changes to 'n'.  Useful to\n-\tlimit the amount of history when using the '@all' p4 revision\n-\tspecifier.\n+\tImport at most 'n' changes, rather than the entire range of\n+\tchanges included in the given revision specifier. A typical\n+\tusage would be use '@all' as the revision specifier, but then\n+\tto use '--max-changes 1000' to import only the last 1000\n+\trevisions rather than the entire revision history.\n+\n+--changes-block-size <n>::\n+\tThe internal block size to use when converting a revision\n+\tspecifier such as '@all' into a list of specific change\n+\tnumbers. Instead of using a single call to 'p4 changes' to\n+\tfind the full list of changes for the conversion, there are a\n+\tsequence of calls to 'p4 changes -m', each of which requests\n+\tone block of changes of the given size. The default block size\n+\tis 500, which should usually be suitable.\n \n --keep-path::\n \tThe mapping of file names from the p4 depot path to Git, by\ndiff --git a/git-p4.py b/git-p4.py\nindex 549022e..e28033f 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -740,17 +740,43 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = \"refs/remotes/p4/\", silent\n def originP4BranchesExist():\n         return gitBranchExists(\"origin\") or gitBranchExists(\"origin/p4\") or gitBranchExists(\"origin/p4/master\")\n \n-def p4ChangesForPaths(depotPaths, changeRange):\n+def p4ChangesForPaths(depotPaths, changeRange, block_size):\n     assert depotPaths\n-    cmd = ['changes']\n-    for p in depotPaths:\n-        cmd += [\"%s...%s\" % (p, changeRange)]\n-    output = p4_read_pipe_lines(cmd)\n+    assert block_size\n+\n+    # Parse the change range into start and end\n+    if changeRange is None or changeRange == '':\n+        changeStart = '@1'\n+        changeEnd = '#head'\n+    else:\n+        parts = changeRange.split(',')\n+        assert len(parts) == 2\n+        changeStart = parts[0]\n+        changeEnd = parts[1]\n \n+    # Accumulate change numbers in a dictionary to avoid duplicates\n     changes = {}\n-    for line in output:\n-        changeNum = int(line.split(\" \")[1])\n-        changes[changeNum] = True\n+\n+    for p in depotPaths:\n+        # Retrieve changes a block at a time, to prevent running\n+        # into a MaxScanRows error from the server.\n+        start = changeStart\n+        end = changeEnd\n+        get_another_block = True\n+        while get_another_block:\n+            new_changes = []\n+            cmd = ['changes']\n+            cmd += ['-m', str(block_size)]\n+            cmd += [\"%s...%s,%s\" % (p, start, end)]\n+            for line in p4_read_pipe_lines(cmd):\n+                changeNum = int(line.split(\" \")[1])\n+                new_changes.append(changeNum)\n+                changes[changeNum] = True\n+            if len(new_changes) == block_size:\n+                get_another_block = True\n+                end = '@' + str(min(new_changes))\n+            else:\n+                get_another_block = False\n \n     changelist = changes.keys()\n     changelist.sort()\n@@ -1911,7 +1937,10 @@ class P4Sync(Command, P4UserMap):\n                 optparse.make_option(\"--import-labels\", dest=\"importLabels\", action=\"store_true\"),\n                 optparse.make_option(\"--import-local\", dest=\"importIntoRemotes\", action=\"store_false\",\n                                      help=\"Import into refs/heads/ , not refs/remotes\"),\n-                optparse.make_option(\"--max-changes\", dest=\"maxChanges\"),\n+                optparse.make_option(\"--max-changes\", dest=\"maxChanges\",\n+                                     help=\"Maximum number of changes to import\"),\n+                optparse.make_option(\"--changes-block-size\", dest=\"changes_block_size\", type=\"int\",\n+                                     help=\"Internal block size to use when iteratively calling p4 changes\"),\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(\"--use-client-spec\", dest=\"useClientSpec\", action='store_true',\n@@ -1940,6 +1969,7 @@ class P4Sync(Command, P4UserMap):\n         self.syncWithOrigin = True\n         self.importIntoRemotes = True\n         self.maxChanges = \"\"\n+        self.changes_block_size = 500\n         self.keepRepoPath = False\n         self.depotPaths = None\n         self.p4BranchesInGit = []\n@@ -2586,7 +2616,7 @@ class P4Sync(Command, P4UserMap):\n         branchPrefix = self.depotPaths[0] + branch + \"/\"\n         range = \"@1,%s\" % maxChange\n         #print \"prefix\" + branchPrefix\n-        changes = p4ChangesForPaths([branchPrefix], range)\n+        changes = p4ChangesForPaths([branchPrefix], range, self.changes_block_size)\n         if len(changes) <= 0:\n             return False\n         firstChange = changes[0]\n@@ -3002,7 +3032,7 @@ class P4Sync(Command, P4UserMap):\n                 if self.verbose:\n                     print \"Getting p4 changes for %s...%s\" % (', '.join(self.depotPaths),\n                                                               self.changeRange)\n-                changes = p4ChangesForPaths(self.depotPaths, self.changeRange)\n+                changes = p4ChangesForPaths(self.depotPaths, self.changeRange, self.changes_block_size)\n \n                 if len(self.maxChanges) > 0:\n                     changes = changes[:min(int(self.maxChanges), len(changes))]\ndiff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh\nnew file mode 100755\nindex 0000000..153b20a\n--- /dev/null\n+++ b/t/t9818-git-p4-block.sh\n@@ -0,0 +1,64 @@\n+#!/bin/sh\n+\n+test_description='git p4 fetching changes in multiple blocks'\n+\n+. ./lib-git-p4.sh\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'Create a repo with ~100 changes' '\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\t>file.txt &&\n+\t\tp4 add file.txt &&\n+\t\tp4 submit -d \"Add file.txt\" &&\n+\t\tfor i in $(test_seq 0 9)\n+\t\tdo\n+\t\t\t>outer$i.txt &&\n+\t\t\tp4 add outer$i.txt &&\n+\t\t\tp4 submit -d \"Adding outer$i.txt\" &&\n+\t\t\tfor j in $(test_seq 0 9)\n+\t\t\tdo\n+\t\t\t\tp4 edit file.txt &&\n+\t\t\t\techo $i$j >file.txt &&\n+\t\t\t\tp4 submit -d \"Commit $i$j\" || exit\n+\t\t\tdone || exit\n+\t\tdone\n+\t)\n+'\n+\n+test_expect_success 'Clone the repo' '\n+\tgit p4 clone --dest=\"$git\" --changes-block-size=10 --verbose //depot@all\n+'\n+\n+test_expect_success 'All files are present' '\n+\techo file.txt >expected &&\n+\ttest_write_lines outer0.txt outer1.txt outer2.txt outer3.txt outer4.txt >>expected &&\n+\ttest_write_lines outer5.txt outer6.txt outer7.txt outer8.txt outer9.txt >>expected &&\n+\tls \"$git\" >current &&\n+\ttest_cmp expected current\n+'\n+\n+test_expect_success 'file.txt is correct' '\n+\techo 99 >expected &&\n+\ttest_cmp expected \"$git/file.txt\"\n+'\n+\n+test_expect_success 'Correct number of commits' '\n+\t(cd \"$git\" && git log --oneline) >log &&\n+\ttest_line_count = 111 log\n+'\n+\n+test_expect_success 'Previous version of file.txt is correct' '\n+\t(cd \"$git\" && git checkout HEAD^^) &&\n+\techo 97 >expected &&\n+\ttest_cmp expected \"$git/file.txt\"\n+'\n+\n+test_expect_success 'kill p4d' '\n+\tkill_p4d\n+'\n+\n+test_done\n-- \n1.9.1\n"},{"id":"259677","messageId":"CAE5ih79BLm1LbZersZeOxShq=W4X5xaPHE1cDwctA5cJOSLRJA@mail.gmail.com","threadId":"39071","inReplyTo":"1429542020-11121-1-git-send-email-lex@lexspoon.org","subject":"Re: [PATCH v4] git-p4: Use -m when running p4 changes","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-04-20T15:15:08Z","receivedAt":"2015-04-20T15:15:08Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Sorry - could you resubmit your patch (PATCHv4 it will be) with this\nchange squashed in please? It will make life much easier, especially\nfor Junio!\n\nThanks!\nLuke\n\n\nOn 20 April 2015 at 16:00, Lex Spoon <lex@lexspoon.org> wrote:\n> Simply running \"p4 changes\" on a large branch can\n> result in a \"too many rows scanned\" error from the\n> Perforce server. It is better to use a sequence\n> of smaller calls to \"p4 changes\", using the \"-m\"\n> option to limit the size of each call.\n>\n> Signed-off-by: Lex Spoon <lex@lexspoon.org>\n> Reviewed-by: Junio C Hamano <gitster@pobox.com>\n> Reviewed-by: Luke Diamand <luke@diamand.org>\n> ---\n> Updated to avoid the crash Luke pointed out.\n> All t98* tests pass now except for t9814,\n> which is already failing on master for some reason.\n>\n>  Documentation/git-p4.txt | 17 ++++++++++---\n>  git-p4.py                | 52 ++++++++++++++++++++++++++++++---------\n>  t/t9818-git-p4-block.sh  | 64 ++++++++++++++++++++++++++++++++++++++++++++++++\n>  3 files changed, 119 insertions(+), 14 deletions(-)\n>  create mode 100755 t/t9818-git-p4-block.sh\n>\n> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n> index a1664b9..82aa5d6 100644\n> --- a/Documentation/git-p4.txt\n> +++ b/Documentation/git-p4.txt\n> @@ -225,9 +225,20 @@ Git repository:\n>         they can find the p4 branches in refs/heads.\n>\n>  --max-changes <n>::\n> -       Limit the number of imported changes to 'n'.  Useful to\n> -       limit the amount of history when using the '@all' p4 revision\n> -       specifier.\n> +       Import at most 'n' changes, rather than the entire range of\n> +       changes included in the given revision specifier. A typical\n> +       usage would be use '@all' as the revision specifier, but then\n> +       to use '--max-changes 1000' to import only the last 1000\n> +       revisions rather than the entire revision history.\n> +\n> +--changes-block-size <n>::\n> +       The internal block size to use when converting a revision\n> +       specifier such as '@all' into a list of specific change\n> +       numbers. Instead of using a single call to 'p4 changes' to\n> +       find the full list of changes for the conversion, there are a\n> +       sequence of calls to 'p4 changes -m', each of which requests\n> +       one block of changes of the given size. The default block size\n> +       is 500, which should usually be suitable.\n>\n>  --keep-path::\n>         The mapping of file names from the p4 depot path to Git, by\n> diff --git a/git-p4.py b/git-p4.py\n> index 549022e..e28033f 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -740,17 +740,43 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = \"refs/remotes/p4/\", silent\n>  def originP4BranchesExist():\n>          return gitBranchExists(\"origin\") or gitBranchExists(\"origin/p4\") or gitBranchExists(\"origin/p4/master\")\n>\n> -def p4ChangesForPaths(depotPaths, changeRange):\n> +def p4ChangesForPaths(depotPaths, changeRange, block_size):\n>      assert depotPaths\n> -    cmd = ['changes']\n> -    for p in depotPaths:\n> -        cmd += [\"%s...%s\" % (p, changeRange)]\n> -    output = p4_read_pipe_lines(cmd)\n> +    assert block_size\n> +\n> +    # Parse the change range into start and end\n> +    if changeRange is None or changeRange == '':\n> +        changeStart = '@1'\n> +        changeEnd = '#head'\n> +    else:\n> +        parts = changeRange.split(',')\n> +        assert len(parts) == 2\n> +        changeStart = parts[0]\n> +        changeEnd = parts[1]\n>\n> +    # Accumulate change numbers in a dictionary to avoid duplicates\n>      changes = {}\n> -    for line in output:\n> -        changeNum = int(line.split(\" \")[1])\n> -        changes[changeNum] = True\n> +\n> +    for p in depotPaths:\n> +        # Retrieve changes a block at a time, to prevent running\n> +        # into a MaxScanRows error from the server.\n> +        start = changeStart\n> +        end = changeEnd\n> +        get_another_block = True\n> +        while get_another_block:\n> +            new_changes = []\n> +            cmd = ['changes']\n> +            cmd += ['-m', str(block_size)]\n> +            cmd += [\"%s...%s,%s\" % (p, start, end)]\n> +            for line in p4_read_pipe_lines(cmd):\n> +                changeNum = int(line.split(\" \")[1])\n> +                new_changes.append(changeNum)\n> +                changes[changeNum] = True\n> +            if len(new_changes) == block_size:\n> +                get_another_block = True\n> +                end = '@' + str(min(new_changes))\n> +            else:\n> +                get_another_block = False\n>\n>      changelist = changes.keys()\n>      changelist.sort()\n> @@ -1911,7 +1937,10 @@ class P4Sync(Command, P4UserMap):\n>                  optparse.make_option(\"--import-labels\", dest=\"importLabels\", action=\"store_true\"),\n>                  optparse.make_option(\"--import-local\", dest=\"importIntoRemotes\", action=\"store_false\",\n>                                       help=\"Import into refs/heads/ , not refs/remotes\"),\n> -                optparse.make_option(\"--max-changes\", dest=\"maxChanges\"),\n> +                optparse.make_option(\"--max-changes\", dest=\"maxChanges\",\n> +                                     help=\"Maximum number of changes to import\"),\n> +                optparse.make_option(\"--changes-block-size\", dest=\"changes_block_size\", type=\"int\",\n> +                                     help=\"Internal block size to use when iteratively calling p4 changes\"),\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(\"--use-client-spec\", dest=\"useClientSpec\", action='store_true',\n> @@ -1940,6 +1969,7 @@ class P4Sync(Command, P4UserMap):\n>          self.syncWithOrigin = True\n>          self.importIntoRemotes = True\n>          self.maxChanges = \"\"\n> +        self.changes_block_size = 500\n>          self.keepRepoPath = False\n>          self.depotPaths = None\n>          self.p4BranchesInGit = []\n> @@ -2586,7 +2616,7 @@ class P4Sync(Command, P4UserMap):\n>          branchPrefix = self.depotPaths[0] + branch + \"/\"\n>          range = \"@1,%s\" % maxChange\n>          #print \"prefix\" + branchPrefix\n> -        changes = p4ChangesForPaths([branchPrefix], range)\n> +        changes = p4ChangesForPaths([branchPrefix], range, self.changes_block_size)\n>          if len(changes) <= 0:\n>              return False\n>          firstChange = changes[0]\n> @@ -3002,7 +3032,7 @@ class P4Sync(Command, P4UserMap):\n>                  if self.verbose:\n>                      print \"Getting p4 changes for %s...%s\" % (', '.join(self.depotPaths),\n>                                                                self.changeRange)\n> -                changes = p4ChangesForPaths(self.depotPaths, self.changeRange)\n> +                changes = p4ChangesForPaths(self.depotPaths, self.changeRange, self.changes_block_size)\n>\n>                  if len(self.maxChanges) > 0:\n>                      changes = changes[:min(int(self.maxChanges), len(changes))]\n> diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh\n> new file mode 100755\n> index 0000000..153b20a\n> --- /dev/null\n> +++ b/t/t9818-git-p4-block.sh\n> @@ -0,0 +1,64 @@\n> +#!/bin/sh\n> +\n> +test_description='git p4 fetching changes in multiple blocks'\n> +\n> +. ./lib-git-p4.sh\n> +\n> +test_expect_success 'start p4d' '\n> +       start_p4d\n> +'\n> +\n> +test_expect_success 'Create a repo with ~100 changes' '\n> +       (\n> +               cd \"$cli\" &&\n> +               >file.txt &&\n> +               p4 add file.txt &&\n> +               p4 submit -d \"Add file.txt\" &&\n> +               for i in $(test_seq 0 9)\n> +               do\n> +                       >outer$i.txt &&\n> +                       p4 add outer$i.txt &&\n> +                       p4 submit -d \"Adding outer$i.txt\" &&\n> +                       for j in $(test_seq 0 9)\n> +                       do\n> +                               p4 edit file.txt &&\n> +                               echo $i$j >file.txt &&\n> +                               p4 submit -d \"Commit $i$j\" || exit\n> +                       done || exit\n> +               done\n> +       )\n> +'\n> +\n> +test_expect_success 'Clone the repo' '\n> +       git p4 clone --dest=\"$git\" --changes-block-size=10 --verbose //depot@all\n> +'\n> +\n> +test_expect_success 'All files are present' '\n> +       echo file.txt >expected &&\n> +       test_write_lines outer0.txt outer1.txt outer2.txt outer3.txt outer4.txt >>expected &&\n> +       test_write_lines outer5.txt outer6.txt outer7.txt outer8.txt outer9.txt >>expected &&\n> +       ls \"$git\" >current &&\n> +       test_cmp expected current\n> +'\n> +\n> +test_expect_success 'file.txt is correct' '\n> +       echo 99 >expected &&\n> +       test_cmp expected \"$git/file.txt\"\n> +'\n> +\n> +test_expect_success 'Correct number of commits' '\n> +       (cd \"$git\" && git log --oneline) >log &&\n> +       test_line_count = 111 log\n> +'\n> +\n> +test_expect_success 'Previous version of file.txt is correct' '\n> +       (cd \"$git\" && git checkout HEAD^^) &&\n> +       echo 97 >expected &&\n> +       test_cmp expected \"$git/file.txt\"\n> +'\n> +\n> +test_expect_success 'kill p4d' '\n> +       kill_p4d\n> +'\n> +\n> +test_done\n> --\n> 1.9.1\n>\n"},{"id":"259678","messageId":"CALM2Snaih=r_CAACVodbgZiLqSUvJr_yPXsipEdR2WZs+utaZg@mail.gmail.com","threadId":"39071","inReplyTo":"CAE5ih79BLm1LbZersZeOxShq=W4X5xaPHE1cDwctA5cJOSLRJA@mail.gmail.com","subject":"Re: [PATCH v4] git-p4: Use -m when running p4 changes","fromName":"Lex Spoon","fromEmail":"lex@lexspoon.org","sentAt":"2015-04-20T15:25:35Z","receivedAt":"2015-04-20T15:25:35Z","isPatch":true,"sender":{"key":"lex@lexspoon.org","avatar":"https://avatars.githubusercontent.com/u/274906?v=4"},"body":"On Mon, Apr 20, 2015 at 11:15 AM, Luke Diamand <luke@diamand.org> wrote:\n> Sorry - could you resubmit your patch (PATCHv4 it will be) with this\n> change squashed in please? It will make life much easier, especially\n> for Junio!\n\nThe message you just responded is already the squashed version. It's a\nsingle patch that includes all changes so far discussed. The subject\nline says \"PATCH v4\", although since it's in the same thread, not all\nemail clients will show the subject change.\n\nLet me know if I can do more to make the process go smoothly.\n\nLex Spoon\n"},{"id":"259686","messageId":"xmqq7ft6n0rn.fsf@gitster.dls.corp.google.com","threadId":"39071","inReplyTo":"CAE5ih79BLm1LbZersZeOxShq=W4X5xaPHE1cDwctA5cJOSLRJA@mail.gmail.com","subject":"Re: [PATCH v4] git-p4: Use -m when running p4 changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-20T17:54:36Z","receivedAt":"2015-04-20T17:54:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand <luke@diamand.org> writes:\n\n> Sorry - could you resubmit your patch (PATCHv4 it will be) with this\n> change squashed in please? It will make life much easier, especially\n> for Junio!\n\nThanks for caring, but this seems to be a full patch to replace v3.\n\nIt was sent with your Reviewed-by already in, but I'd tentatively\nremove that line while queuing it to 'pu' and ask you to double\ncheck if the patch makes sense (and after your \"yes, it does\", I'd\nadd the Reviewed-by back).\n\nThanks.\n\n>\n> Thanks!\n> Luke\n>\n>\n> On 20 April 2015 at 16:00, Lex Spoon <lex@lexspoon.org> wrote:\n>> Simply running \"p4 changes\" on a large branch can\n>> result in a \"too many rows scanned\" error from the\n>> Perforce server. It is better to use a sequence\n>> of smaller calls to \"p4 changes\", using the \"-m\"\n>> option to limit the size of each call.\n>>\n>> Signed-off-by: Lex Spoon <lex@lexspoon.org>\n>> Reviewed-by: Junio C Hamano <gitster@pobox.com>\n>> Reviewed-by: Luke Diamand <luke@diamand.org>\n>> ---\n>> Updated to avoid the crash Luke pointed out.\n>> All t98* tests pass now except for t9814,\n>> which is already failing on master for some reason.\n>>\n>>  Documentation/git-p4.txt | 17 ++++++++++---\n>>  git-p4.py                | 52 ++++++++++++++++++++++++++++++---------\n>>  t/t9818-git-p4-block.sh  | 64 ++++++++++++++++++++++++++++++++++++++++++++++++\n>>  3 files changed, 119 insertions(+), 14 deletions(-)\n>>  create mode 100755 t/t9818-git-p4-block.sh\n>>\n>> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n>> index a1664b9..82aa5d6 100644\n>> --- a/Documentation/git-p4.txt\n>> +++ b/Documentation/git-p4.txt\n>> @@ -225,9 +225,20 @@ Git repository:\n>>         they can find the p4 branches in refs/heads.\n>>\n>>  --max-changes <n>::\n>> -       Limit the number of imported changes to 'n'.  Useful to\n>> -       limit the amount of history when using the '@all' p4 revision\n>> -       specifier.\n>> +       Import at most 'n' changes, rather than the entire range of\n>> +       changes included in the given revision specifier. A typical\n>> +       usage would be use '@all' as the revision specifier, but then\n>> +       to use '--max-changes 1000' to import only the last 1000\n>> +       revisions rather than the entire revision history.\n>> +\n>> +--changes-block-size <n>::\n>> +       The internal block size to use when converting a revision\n>> +       specifier such as '@all' into a list of specific change\n>> +       numbers. Instead of using a single call to 'p4 changes' to\n>> +       find the full list of changes for the conversion, there are a\n>> +       sequence of calls to 'p4 changes -m', each of which requests\n>> +       one block of changes of the given size. The default block size\n>> +       is 500, which should usually be suitable.\n>>\n>>  --keep-path::\n>>         The mapping of file names from the p4 depot path to Git, by\n>> diff --git a/git-p4.py b/git-p4.py\n>> index 549022e..e28033f 100755\n>> --- a/git-p4.py\n>> +++ b/git-p4.py\n>> @@ -740,17 +740,43 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = \"refs/remotes/p4/\", silent\n>>  def originP4BranchesExist():\n>>          return gitBranchExists(\"origin\") or gitBranchExists(\"origin/p4\") or gitBranchExists(\"origin/p4/master\")\n>>\n>> -def p4ChangesForPaths(depotPaths, changeRange):\n>> +def p4ChangesForPaths(depotPaths, changeRange, block_size):\n>>      assert depotPaths\n>> -    cmd = ['changes']\n>> -    for p in depotPaths:\n>> -        cmd += [\"%s...%s\" % (p, changeRange)]\n>> -    output = p4_read_pipe_lines(cmd)\n>> +    assert block_size\n>> +\n>> +    # Parse the change range into start and end\n>> +    if changeRange is None or changeRange == '':\n>> +        changeStart = '@1'\n>> +        changeEnd = '#head'\n>> +    else:\n>> +        parts = changeRange.split(',')\n>> +        assert len(parts) == 2\n>> +        changeStart = parts[0]\n>> +        changeEnd = parts[1]\n>>\n>> +    # Accumulate change numbers in a dictionary to avoid duplicates\n>>      changes = {}\n>> -    for line in output:\n>> -        changeNum = int(line.split(\" \")[1])\n>> -        changes[changeNum] = True\n>> +\n>> +    for p in depotPaths:\n>> +        # Retrieve changes a block at a time, to prevent running\n>> +        # into a MaxScanRows error from the server.\n>> +        start = changeStart\n>> +        end = changeEnd\n>> +        get_another_block = True\n>> +        while get_another_block:\n>> +            new_changes = []\n>> +            cmd = ['changes']\n>> +            cmd += ['-m', str(block_size)]\n>> +            cmd += [\"%s...%s,%s\" % (p, start, end)]\n>> +            for line in p4_read_pipe_lines(cmd):\n>> +                changeNum = int(line.split(\" \")[1])\n>> +                new_changes.append(changeNum)\n>> +                changes[changeNum] = True\n>> +            if len(new_changes) == block_size:\n>> +                get_another_block = True\n>> +                end = '@' + str(min(new_changes))\n>> +            else:\n>> +                get_another_block = False\n>>\n>>      changelist = changes.keys()\n>>      changelist.sort()\n>> @@ -1911,7 +1937,10 @@ class P4Sync(Command, P4UserMap):\n>>                  optparse.make_option(\"--import-labels\", dest=\"importLabels\", action=\"store_true\"),\n>>                  optparse.make_option(\"--import-local\", dest=\"importIntoRemotes\", action=\"store_false\",\n>>                                       help=\"Import into refs/heads/ , not refs/remotes\"),\n>> -                optparse.make_option(\"--max-changes\", dest=\"maxChanges\"),\n>> +                optparse.make_option(\"--max-changes\", dest=\"maxChanges\",\n>> +                                     help=\"Maximum number of changes to import\"),\n>> +                optparse.make_option(\"--changes-block-size\", dest=\"changes_block_size\", type=\"int\",\n>> +                                     help=\"Internal block size to use when iteratively calling p4 changes\"),\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(\"--use-client-spec\", dest=\"useClientSpec\", action='store_true',\n>> @@ -1940,6 +1969,7 @@ class P4Sync(Command, P4UserMap):\n>>          self.syncWithOrigin = True\n>>          self.importIntoRemotes = True\n>>          self.maxChanges = \"\"\n>> +        self.changes_block_size = 500\n>>          self.keepRepoPath = False\n>>          self.depotPaths = None\n>>          self.p4BranchesInGit = []\n>> @@ -2586,7 +2616,7 @@ class P4Sync(Command, P4UserMap):\n>>          branchPrefix = self.depotPaths[0] + branch + \"/\"\n>>          range = \"@1,%s\" % maxChange\n>>          #print \"prefix\" + branchPrefix\n>> -        changes = p4ChangesForPaths([branchPrefix], range)\n>> +        changes = p4ChangesForPaths([branchPrefix], range, self.changes_block_size)\n>>          if len(changes) <= 0:\n>>              return False\n>>          firstChange = changes[0]\n>> @@ -3002,7 +3032,7 @@ class P4Sync(Command, P4UserMap):\n>>                  if self.verbose:\n>>                      print \"Getting p4 changes for %s...%s\" % (', '.join(self.depotPaths),\n>>                                                                self.changeRange)\n>> -                changes = p4ChangesForPaths(self.depotPaths, self.changeRange)\n>> +                changes = p4ChangesForPaths(self.depotPaths, self.changeRange, self.changes_block_size)\n>>\n>>                  if len(self.maxChanges) > 0:\n>>                      changes = changes[:min(int(self.maxChanges), len(changes))]\n>> diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh\n>> new file mode 100755\n>> index 0000000..153b20a\n>> --- /dev/null\n>> +++ b/t/t9818-git-p4-block.sh\n>> @@ -0,0 +1,64 @@\n>> +#!/bin/sh\n>> +\n>> +test_description='git p4 fetching changes in multiple blocks'\n>> +\n>> +. ./lib-git-p4.sh\n>> +\n>> +test_expect_success 'start p4d' '\n>> +       start_p4d\n>> +'\n>> +\n>> +test_expect_success 'Create a repo with ~100 changes' '\n>> +       (\n>> +               cd \"$cli\" &&\n>> +               >file.txt &&\n>> +               p4 add file.txt &&\n>> +               p4 submit -d \"Add file.txt\" &&\n>> +               for i in $(test_seq 0 9)\n>> +               do\n>> +                       >outer$i.txt &&\n>> +                       p4 add outer$i.txt &&\n>> +                       p4 submit -d \"Adding outer$i.txt\" &&\n>> +                       for j in $(test_seq 0 9)\n>> +                       do\n>> +                               p4 edit file.txt &&\n>> +                               echo $i$j >file.txt &&\n>> +                               p4 submit -d \"Commit $i$j\" || exit\n>> +                       done || exit\n>> +               done\n>> +       )\n>> +'\n>> +\n>> +test_expect_success 'Clone the repo' '\n>> +       git p4 clone --dest=\"$git\" --changes-block-size=10 --verbose //depot@all\n>> +'\n>> +\n>> +test_expect_success 'All files are present' '\n>> +       echo file.txt >expected &&\n>> +       test_write_lines outer0.txt outer1.txt outer2.txt outer3.txt outer4.txt >>expected &&\n>> +       test_write_lines outer5.txt outer6.txt outer7.txt outer8.txt outer9.txt >>expected &&\n>> +       ls \"$git\" >current &&\n>> +       test_cmp expected current\n>> +'\n>> +\n>> +test_expect_success 'file.txt is correct' '\n>> +       echo 99 >expected &&\n>> +       test_cmp expected \"$git/file.txt\"\n>> +'\n>> +\n>> +test_expect_success 'Correct number of commits' '\n>> +       (cd \"$git\" && git log --oneline) >log &&\n>> +       test_line_count = 111 log\n>> +'\n>> +\n>> +test_expect_success 'Previous version of file.txt is correct' '\n>> +       (cd \"$git\" && git checkout HEAD^^) &&\n>> +       echo 97 >expected &&\n>> +       test_cmp expected \"$git/file.txt\"\n>> +'\n>> +\n>> +test_expect_success 'kill p4d' '\n>> +       kill_p4d\n>> +'\n>> +\n>> +test_done\n>> --\n>> 1.9.1\n>>\n"},{"id":"259687","messageId":"xmqq383un0b5.fsf@gitster.dls.corp.google.com","threadId":"39071","inReplyTo":"xmqq7ft6n0rn.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4] git-p4: Use -m when running p4 changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-20T18:04:30Z","receivedAt":"2015-04-20T18:04:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Luke Diamand <luke@diamand.org> writes:\n>\n>> Sorry - could you resubmit your patch (PATCHv4 it will be) with this\n>> change squashed in please? It will make life much easier, especially\n>> for Junio!\n>\n> Thanks for caring, but this seems to be a full patch to replace v3.\n>\n> It was sent with your Reviewed-by already in, but I'd tentatively\n> remove that line while queuing it to 'pu' and ask you to double\n> check if the patch makes sense (and after your \"yes, it does\", I'd\n> add the Reviewed-by back).\n>\n> Thanks.\n\nJust to make it easier to see, the interdiff between v3 and v4 looks\nlike this:\n\n git-p4.py | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 1fba3aa..e28033f 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -2608,7 +2608,7 @@ class P4Sync(Command, P4UserMap):\n \n         return \"\"\n \n-    def importNewBranch(self, branch, maxChange, changes_block_size):\n+    def importNewBranch(self, branch, maxChange):\n         # make fast-import flush all changes to disk and update the refs using the checkpoint\n         # command so that we can try to find the branch parent in the git history\n         self.gitStream.write(\"checkpoint\\n\\n\");\n@@ -2616,7 +2616,7 @@ class P4Sync(Command, P4UserMap):\n         branchPrefix = self.depotPaths[0] + branch + \"/\"\n         range = \"@1,%s\" % maxChange\n         #print \"prefix\" + branchPrefix\n-        changes = p4ChangesForPaths([branchPrefix], range, changes_block_size)\n+        changes = p4ChangesForPaths([branchPrefix], range, self.changes_block_size)\n         if len(changes) <= 0:\n             return False\n         firstChange = changes[0]\n"},{"id":"259696","messageId":"553550C1.2030807@diamand.org","threadId":"39071","inReplyTo":"CALM2Snaih=r_CAACVodbgZiLqSUvJr_yPXsipEdR2WZs+utaZg@mail.gmail.com","subject":"Re: [PATCH v4] git-p4: Use -m when running p4 changes","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-04-20T19:17:21Z","receivedAt":"2015-04-20T19:17:21Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 20/04/15 16:25, Lex Spoon wrote:\n> On Mon, Apr 20, 2015 at 11:15 AM, Luke Diamand <luke@diamand.org> wrote:\n>> Sorry - could you resubmit your patch (PATCHv4 it will be) with this\n>> change squashed in please? It will make life much easier, especially\n>> for Junio!\n>\n> The message you just responded is already the squashed version. It's a\n> single patch that includes all changes so far discussed. The subject\n> line says \"PATCH v4\", although since it's in the same thread, not all\n> email clients will show the subject change.\n\nNot sure how I missed that! It looks good, now, Ack!\n\nThanks!\nLuke\n"}]}