{"thread":{"id":"39582","subject":"[PATCHv3 0/4] git-p4: fixing --changes-block-size handling","startedAt":"2015-06-10T07:30:55Z","lastAt":"2015-06-12T19:03:14Z","messageCount":6,"participants":["Luke Diamand","Lex Spoon"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"263439","messageId":"1433921459-16555-1-git-send-email-luke@diamand.org","threadId":"39582","inReplyTo":null,"subject":"[PATCHv3 0/4] git-p4: fixing --changes-block-size handling","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-06-10T07:30:55Z","receivedAt":"2015-06-10T07:30:55Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"This series of patches teaches git-p4 to break up calls to the\nP4 server into smaller chunks, to avoid hitting the maxresults and\nmaxscanrows server-side limits.\n\nThe previous iteration of this series didn't handle non-integer\nP4 revision ranges (e.g. //depot/...@2014/1/1,2015/1/1).\n\nThis version now breaks up the commit into blocks iff the revision\nrange specified is an integer range: @all, M,N or N,#head. If the\nrange is non-numeric then it falls back to relying on Perforce to\nparse the revisions. In this mode it is no longer possible to fetch\nchanges in blocks (and requests to do so will be rejected).\n\nAnother alternative would be to turn the date (or whatever) revisions\ninto numeric revision numbers, but there doesn't seem to be a simple\nway to do this. The best I can come up with is to get the changes\n\"around\" the date in question (perhaps binary-chopping from years down\nto seconds to find a range that works?) and then take the lowest\nrevision supplied. But that's quite a bit more complex.\n\nLuke Diamand (4):\n git-p4: additional testing of --changes-block-size\n git-p4: test with limited p4 server results\n git-p4: add tests for non-numeric revision range\n git-p4: fixing --changes-block-size handling\n\n git-p4.py | 85 ++++++++++++++++++++++++++++++++++++-------------\n t/t9800-git-p4-basic.sh | 38 ++++++++++++++++++++++\n t/t9818-git-p4-block.sh | 73 ++++++++++++++++++++++++++++++++++++------\n 3 files changed, 165 insertions(+), 31 deletions(-)\n\n-- \n2.4.1.502.gb11c5ab\n"},{"id":"263442","messageId":"1433921459-16555-2-git-send-email-luke@diamand.org","threadId":"39582","inReplyTo":"1433921459-16555-1-git-send-email-luke@diamand.org","subject":"[PATCHv3 1/4] git-p4: additional testing of --changes-block-size","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-06-10T07:30:56Z","receivedAt":"2015-06-10T07:30:56Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Add additional tests of some corner-cases of the\n--changes-block-size git-p4 parameter.\n\nAlso reduce the number of p4 changes created during the\ntests, so that they complete faster.\n\nSigned-off-by: Luke Diamand \nAcked-by: Lex Spoon \n---\n t/t9818-git-p4-block.sh | 56 +++++++++++++++++++++++++++++++++++++++++--------\n 1 file changed, 47 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh\nindex 153b20a..79765a4 100755\n--- a/t/t9818-git-p4-block.sh\n+++ b/t/t9818-git-p4-block.sh\n@@ -8,18 +8,21 @@ test_expect_success 'start p4d' '\n \tstart_p4d\n '\n \n-test_expect_success 'Create a repo with ~100 changes' '\n+test_expect_success 'Create a repo with many changes' '\n \t(\n-\t\tcd \"$cli\" &&\n+\t\tclient_view \"//depot/included/... //client/included/...\" \\\n+\t\t\t \"//depot/excluded/... //client/excluded/...\" &&\n+\t\tmkdir -p \"$cli/included\" \"$cli/excluded\" &&\n+\t\tcd \"$cli/included\" &&\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\tfor i in $(test_seq 0 5)\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\tfor j in $(test_seq 0 5)\n \t\t\tdo\n \t\t\t\tp4 edit file.txt &&\n \t\t\t\techo $i$j >file.txt &&\n@@ -30,33 +33,68 @@ test_expect_success 'Create a repo with ~100 changes' '\n '\n \n test_expect_success 'Clone the repo' '\n-\tgit p4 clone --dest=\"$git\" --changes-block-size=10 --verbose //depot@all\n+\tgit p4 clone --dest=\"$git\" --changes-block-size=7 --verbose //depot/included@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+\ttest_write_lines outer5.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+\techo 55 >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+\twc -l log &&\n+\ttest_line_count = 43 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+\techo 53 >expected &&\n \ttest_cmp expected \"$git/file.txt\"\n '\n \n+# Test git-p4 sync, with some files outside the client specification.\n+\n+p4_add_file() {\n+\t(cd \"$cli\" &&\n+\t\t>$1 &&\n+\t\tp4 add $1 &&\n+\t\tp4 submit -d \"Added a file\" $1\n+\t)\n+}\n+\n+test_expect_success 'Add some more files' '\n+\tfor i in $(test_seq 0 10)\n+\tdo\n+\t\tp4_add_file \"included/x$i\" &&\n+\t\tp4_add_file \"excluded/x$i\"\n+\tdone &&\n+\tfor i in $(test_seq 0 10)\n+\tdo\n+\t\tp4_add_file \"excluded/y$i\"\n+\tdone\n+'\n+\n+# This should pick up the 10 new files in \"included\", but not be confused\n+# by the additional files in \"excluded\"\n+test_expect_success 'Syncing files' '\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit p4 sync --changes-block-size=7 &&\n+\t\tgit checkout p4/master &&\n+\t\tls -l x* > log &&\n+\t\ttest_line_count = 11 log\n+\t)\n+'\n+\n test_expect_success 'kill p4d' '\n \tkill_p4d\n '\n-- \n2.4.1.502.gb11c5ab\n"},{"id":"263441","messageId":"1433921459-16555-3-git-send-email-luke@diamand.org","threadId":"39582","inReplyTo":"1433921459-16555-1-git-send-email-luke@diamand.org","subject":"[PATCHv3 2/4] git-p4: test with limited p4 server results","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-06-10T07:30:57Z","receivedAt":"2015-06-10T07:30:57Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Change the --changes-block-size git-p4 test to use an account with\nlimited \"maxresults\" and \"maxscanrows\" values.\n\nThese conditions are applied in the server *before* the \"-m maxchanges\"\nparameter to \"p4 changes\" is applied, and so the strategy that git-p4\nuses for limiting the number of changes does not work. As a result,\nthe tests all fail.\n\nNote that \"maxscanrows\" is set quite high, as it appears to not only\nlimit results from \"p4 changes\", but *also* limits results from\n\"p4 print\". Files that have more than \"maxscanrows\" changes seem\n(experimentally) to be impossible to print. There's no good way to\nwork around this.\n\nSigned-off-by: Luke Diamand \nAcked-by: Lex Spoon \n---\n t/t9818-git-p4-block.sh | 29 +++++++++++++++++++++++------\n 1 file changed, 23 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh\nindex 79765a4..aae1121 100755\n--- a/t/t9818-git-p4-block.sh\n+++ b/t/t9818-git-p4-block.sh\n@@ -8,6 +8,19 @@ test_expect_success 'start p4d' '\n \tstart_p4d\n '\n \n+create_restricted_group() {\n+\tp4 group -i <<-EOF\n+\tGroup: restricted\n+\tMaxResults: 7\n+\tMaxScanRows: 40\n+\tUsers: author\n+\tEOF\n+}\n+\n+test_expect_success 'Create group with limited maxrows' '\n+\tcreate_restricted_group\n+'\n+\n test_expect_success 'Create a repo with many changes' '\n \t(\n \t\tclient_view \"//depot/included/... //client/included/...\" \\\n@@ -32,11 +45,15 @@ test_expect_success 'Create a repo with many changes' '\n \t)\n '\n \n-test_expect_success 'Clone the repo' '\n+test_expect_success 'Default user cannot fetch changes' '\n+\t! p4 changes -m 1 //depot/...\n+'\n+\n+test_expect_failure 'Clone the repo' '\n \tgit p4 clone --dest=\"$git\" --changes-block-size=7 --verbose //depot/included@all\n '\n \n-test_expect_success 'All files are present' '\n+test_expect_failure '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 >>expected &&\n@@ -44,18 +61,18 @@ test_expect_success 'All files are present' '\n \ttest_cmp expected current\n '\n \n-test_expect_success 'file.txt is correct' '\n+test_expect_failure 'file.txt is correct' '\n \techo 55 >expected &&\n \ttest_cmp expected \"$git/file.txt\"\n '\n \n-test_expect_success 'Correct number of commits' '\n+test_expect_failure 'Correct number of commits' '\n \t(cd \"$git\" && git log --oneline) >log &&\n \twc -l log &&\n \ttest_line_count = 43 log\n '\n \n-test_expect_success 'Previous version of file.txt is correct' '\n+test_expect_failure 'Previous version of file.txt is correct' '\n \t(cd \"$git\" && git checkout HEAD^^) &&\n \techo 53 >expected &&\n \ttest_cmp expected \"$git/file.txt\"\n@@ -85,7 +102,7 @@ test_expect_success 'Add some more files' '\n \n # This should pick up the 10 new files in \"included\", but not be confused\n # by the additional files in \"excluded\"\n-test_expect_success 'Syncing files' '\n+test_expect_failure 'Syncing files' '\n \t(\n \t\tcd \"$git\" &&\n \t\tgit p4 sync --changes-block-size=7 &&\n-- \n2.4.1.502.gb11c5ab\n"},{"id":"263440","messageId":"1433921459-16555-4-git-send-email-luke@diamand.org","threadId":"39582","inReplyTo":"1433921459-16555-1-git-send-email-luke@diamand.org","subject":"[PATCHv3 3/4] git-p4: add tests for non-numeric revision range","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-06-10T07:30:58Z","receivedAt":"2015-06-10T07:30:58Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Test that git-p4 can handle a sync with a non-numeric revision\nrange (e.g. a date).\n\nSigned-off-by: Luke Diamand \n---\n t/t9800-git-p4-basic.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 38 insertions(+)\n\ndiff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\nindex 5b56212..90d41ed 100755\n--- a/t/t9800-git-p4-basic.sh\n+++ b/t/t9800-git-p4-basic.sh\n@@ -131,6 +131,44 @@ test_expect_success 'clone two dirs, @all, conflicting files' '\n \t)\n '\n \n+revision_ranges=\"2000/01/01,#head \\\n+\t\t 1,2080/01/01 \\\n+\t\t 2000/01/01,2080/01/01 \\\n+\t\t 2000/01/01,1000 \\\n+\t\t 1,1000\"\n+\n+test_expect_success 'clone using non-numeric revision ranges' '\n+\ttest_when_finished cleanup_git &&\n+\tfor r in $revision_ranges\n+\tdo\n+\t\trm -fr \"$git\" &&\n+\t\ttest ! -d \"$git\" &&\n+\t\tgit p4 clone --dest=\"$git\" //depot@$r &&\n+\t\t(\n+\t\t\tcd \"$git\" &&\n+\t\t\tgit ls-files >lines &&\n+\t\t\ttest_line_count = 6 lines\n+\t\t)\n+\tdone\n+'\n+\n+test_expect_success 'clone with date range, excluding some changes' '\n+\ttest_when_finished cleanup_git &&\n+\tbefore=$(date +%Y/%m/%d:%H:%M:%S) &&\n+\tsleep 2 &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\t:>date_range_test &&\n+\t\tp4 add date_range_test &&\n+\t\tp4 submit -d \"Adding file\"\n+\t) &&\n+\tgit p4 clone --dest=\"$git\" //depot@1,$before &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\ttest_path_is_missing date_range_test\n+\t)\n+'\n+\n test_expect_success 'exit when p4 fails to produce marshaled output' '\n \tmkdir badp4dir &&\n \ttest_when_finished \"rm badp4dir/p4 && rmdir badp4dir\" &&\n-- \n2.4.1.502.gb11c5ab\n"},{"id":"263443","messageId":"1433921459-16555-5-git-send-email-luke@diamand.org","threadId":"39582","inReplyTo":"1433921459-16555-1-git-send-email-luke@diamand.org","subject":"[PATCHv3 4/4] git-p4: fixing --changes-block-size handling","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-06-10T07:30:59Z","receivedAt":"2015-06-10T07:30:59Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"The --changes-block-size handling was intended to help when\na user has a limited \"maxscanrows\" (see \"p4 group\"). It used\n\"p4 changes -m $maxchanges\" to limit the number of results.\n\nUnfortunately, it turns out that the \"maxscanrows\" and \"maxresults\"\nlimits are actually applied *before* the \"-m maxchanges\" parameter\nis considered (experimentally).\n\nFix the block-size handling so that it gets blocks of changes\nlimited by revision number ($Start..$Start+$N, etc). This limits\nthe number of results early enough that both sets of tests pass.\n\nNote that many other Perforce operations can fail for the same\nreason (p4 print, p4 files, etc) and it's probably not possible\nto workaround this. In the real world, this is probably not\nusually a problem.\n\nSigned-off-by: Luke Diamand \n---\n git-p4.py | 85 ++++++++++++++++++++++++++++++++++++-------------\n t/t9818-git-p4-block.sh | 12 +++----\n 2 files changed, 69 insertions(+), 28 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 26ad4bc..073f87b 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -43,6 +43,9 @@ verbose = False\n # Only labels/tags matching this will be imported/exported\n defaultLabelRegexp = r'[a-zA-Z0-9_\\-.]+$'\n \n+# Grab changes in blocks of this many revisions, unless otherwise requested\n+defaultBlockSize = 512\n+\n def p4_build_cmd(cmd):\n \"\"\"Build a suitable p4 command line.\n \n@@ -249,6 +252,10 @@ def p4_reopen(type, f):\n def p4_move(src, dest):\n p4_system([\"move\", \"-k\", wildcard_encode(src), wildcard_encode(dest)])\n \n+def p4_last_change():\n+ results = p4CmdList([\"changes\", \"-m\", \"1\"])\n+ return int(results[0]['change'])\n+\n def p4_describe(change):\n \"\"\"Make sure it returns a valid result by checking for\n the presence of field \"time\". Return a dict of the\n@@ -742,43 +749,77 @@ 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, block_size):\n+\n+def p4ParseNumericChangeRange(parts):\n+ changeStart = int(parts[0][1:])\n+ if parts[1] == '#head':\n+ changeEnd = p4_last_change()\n+ else:\n+ changeEnd = int(parts[1])\n+\n+ return (changeStart, changeEnd)\n+\n+def chooseBlockSize(blockSize):\n+ if blockSize:\n+ return blockSize\n+ else:\n+ return defaultBlockSize\n+\n+def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):\n assert depotPaths\n- assert block_size\n \n- # Parse the change range into start and end\n+ # Parse the change range into start and end. Try to find integer\n+ # revision ranges as these can be broken up into blocks to avoid\n+ # hitting server-side limits (maxrows, maxscanresults). But if\n+ # that doesn't work, fall back to using the raw revision specifier\n+ # strings, without using block mode.\n+\n if changeRange is None or changeRange == '':\n- changeStart = '@1'\n- changeEnd = '#head'\n+ changeStart = 1\n+ changeEnd = p4_last_change()\n+ block_size = chooseBlockSize(requestedBlockSize)\n else:\n parts = changeRange.split(',')\n assert len(parts) == 2\n- changeStart = parts[0]\n- changeEnd = parts[1]\n+ try:\n+ (changeStart, changeEnd) = p4ParseNumericChangeRange(parts)\n+ block_size = chooseBlockSize(requestedBlockSize)\n+ except:\n+ changeStart = parts[0][1:]\n+ changeEnd = parts[1]\n+ if requestedBlockSize:\n+ die(\"cannot use --changes-block-size with non-numeric revisions\")\n+ block_size = None\n \n # Accumulate change numbers in a dictionary to avoid duplicates\n changes = {}\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+ # into a MaxResults/MaxScanRows error from the server.\n+\n+ while True:\n cmd = ['changes']\n- cmd += ['-m', str(block_size)]\n- cmd += [\"%s...%s,%s\" % (p, start, end)]\n+\n+ if block_size:\n+ end = min(changeEnd, changeStart + block_size)\n+ revisionRange = \"%d,%d\" % (changeStart, end)\n+ else:\n+ revisionRange = \"%s,%s\" % (changeStart, changeEnd)\n+\n+ cmd += [\"%s...@%s\" % (p, revisionRange)]\n+\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+ if not block_size:\n+ break\n+\n+ if end >= changeEnd:\n+ break\n+\n+ changeStart = end + 1\n \n changelist = changes.keys()\n changelist.sort()\n@@ -1974,7 +2015,7 @@ class P4Sync(Command, P4UserMap):\n self.syncWithOrigin = True\n self.importIntoRemotes = True\n self.maxChanges = \"\"\n- self.changes_block_size = 500\n+ self.changes_block_size = None\n self.keepRepoPath = False\n self.depotPaths = None\n self.p4BranchesInGit = []\ndiff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh\nindex aae1121..3b3ae1f 100755\n--- a/t/t9818-git-p4-block.sh\n+++ b/t/t9818-git-p4-block.sh\n@@ -49,11 +49,11 @@ test_expect_success 'Default user cannot fetch changes' '\n \t! p4 changes -m 1 //depot/...\n '\n \n-test_expect_failure 'Clone the repo' '\n+test_expect_success 'Clone the repo' '\n \tgit p4 clone --dest=\"$git\" --changes-block-size=7 --verbose //depot/included@all\n '\n \n-test_expect_failure 'All files are present' '\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 >>expected &&\n@@ -61,18 +61,18 @@ test_expect_failure 'All files are present' '\n \ttest_cmp expected current\n '\n \n-test_expect_failure 'file.txt is correct' '\n+test_expect_success 'file.txt is correct' '\n \techo 55 >expected &&\n \ttest_cmp expected \"$git/file.txt\"\n '\n \n-test_expect_failure 'Correct number of commits' '\n+test_expect_success 'Correct number of commits' '\n \t(cd \"$git\" && git log --oneline) >log &&\n \twc -l log &&\n \ttest_line_count = 43 log\n '\n \n-test_expect_failure 'Previous version of file.txt is correct' '\n+test_expect_success 'Previous version of file.txt is correct' '\n \t(cd \"$git\" && git checkout HEAD^^) &&\n \techo 53 >expected &&\n \ttest_cmp expected \"$git/file.txt\"\n@@ -102,7 +102,7 @@ test_expect_success 'Add some more files' '\n \n # This should pick up the 10 new files in \"included\", but not be confused\n # by the additional files in \"excluded\"\n-test_expect_failure 'Syncing files' '\n+test_expect_success 'Syncing files' '\n \t(\n \t\tcd \"$git\" &&\n \t\tgit p4 sync --changes-block-size=7 &&\n-- \n2.4.1.502.gb11c5ab\n"},{"id":"263690","messageId":"CALM2SnYaNUqaYAs+FKZ2NBG1b3sEwoTwoRueFS=yR6naixwPZg@mail.gmail.com","threadId":"39582","inReplyTo":"1433921459-16555-1-git-send-email-luke@diamand.org","subject":"Re: [PATCHv3 0/4] git-p4: fixing --changes-block-size handling","fromName":"Lex Spoon","fromEmail":"lex@lexspoon.org","sentAt":"2015-06-12T19:03:14Z","receivedAt":"2015-06-12T19:03:14Z","isPatch":false,"sender":{"key":"lex@lexspoon.org","avatar":"https://avatars.githubusercontent.com/u/274906?v=4"},"body":"The latest patch series LGTM. It's a pity about the more complicated\nstructure with two different ways to query the changes list, but it\ndoes look hard to make it any simpler.\n\nLex Spoon\n"}]}