{"thread":{"id":"39550","subject":"[PATCHv1 0/3] git-p4: fixing --changes-block-size support","startedAt":"2015-06-07T10:21:42Z","lastAt":"2015-06-08T22:32:46Z","messageCount":18,"participants":["Luke Diamand","Lex Spoon","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"263140","messageId":"1433672505-11940-1-git-send-email-luke@diamand.org","threadId":"39550","inReplyTo":null,"subject":"[PATCHv1 0/3] git-p4: fixing --changes-block-size support","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-06-07T10:21:42Z","receivedAt":"2015-06-07T10:21:42Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"We recently added support to git-p4 to limit the number of changes it\nwould try to import at a time. That was to help clients who were being\nlimited by the \"maxscanrows\" limit. This used the \"-m maxchanges\"\nargument to \"p4 changes\" to limit the number of results returned to\ngit-p4.\n\nUnfortunately it turns out that in practice, the server limits the\nnumber of results returned *before* the \"-m maxchanges\" argument is\nconsidered. Even supplying a \"-m 1\" argument doesn't help.\n\nThis affects both the \"maxscanrows\" and \"maxresults\" group options.\n\nThis set of patches updates the t9818 git-p4 tests to show the problem,\nand then adds a fix which works by iterating over the changes in batches\n(as at present) but using a revision range to limit the number of changes,\nrather than \"-m $BATCHSIZE\".\n\nThat means it will in most cases require more transactions with the server,\nbut usually the effect will be small.\n\nAlong the way I also found that \"p4 print\" can fail if you have a file\nwith too many changes in it, but there's unfortunately no way to workaround\nthis. It's fairly unlikely to ever happen in practice.\n\nI think I've covered everything in this fix, but it's possible that there\nare still bugs to be uncovered; I find the way that these limits interact\nsomewhat tricky to understand.\n\nThanks,\nLuke\n\nLuke Diamand (3):\n git-p4: additional testing of --changes-block-size\n git-p4: test with limited p4 server results\n git-p4: fixing --changes-block-size handling\n\n git-p4.py | 48 +++++++++++++++++++++++---------\n t/t9818-git-p4-block.sh | 73 +++++++++++++++++++++++++++++++++++++++++++------\n 2 files changed, 99 insertions(+), 22 deletions(-)\n\n-- \n2.3.4.48.g223ab37\n"},{"id":"263141","messageId":"1433672505-11940-2-git-send-email-luke@diamand.org","threadId":"39550","inReplyTo":"1433672505-11940-1-git-send-email-luke@diamand.org","subject":"[PATCHv1 1/3] git-p4: additional testing of --changes-block-size","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-06-07T10:21:43Z","receivedAt":"2015-06-07T10:21:43Z","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 \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.3.4.48.g223ab37\n"},{"id":"263143","messageId":"1433672505-11940-3-git-send-email-luke@diamand.org","threadId":"39550","inReplyTo":"1433672505-11940-1-git-send-email-luke@diamand.org","subject":"[PATCHv1 2/3] git-p4: test with limited p4 server results","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-06-07T10:21:44Z","receivedAt":"2015-06-07T10:21:44Z","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 \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.3.4.48.g223ab37\n"},{"id":"263142","messageId":"1433672505-11940-4-git-send-email-luke@diamand.org","threadId":"39550","inReplyTo":"1433672505-11940-1-git-send-email-luke@diamand.org","subject":"[PATCHv1 3/3] git-p4: fixing --changes-block-size handling","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-06-07T10:21:45Z","receivedAt":"2015-06-07T10:21:45Z","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\nIf the --changes-block-size option is not in use, then the code\nnaturally falls back to the original scheme and gets as many changes\nas possible.\n\nUnfortunately, it also turns out that \"p4 print\" can fail on\nfiles with more changes than \"maxscanrows\". This fix is unable to\nworkaround this problem, although in the real world this shouldn't\nnormally happen.\n\nSigned-off-by: Luke Diamand \n---\n git-p4.py | 48 +++++++++++++++++++++++++++++++++++-------------\n t/t9818-git-p4-block.sh | 12 ++++++------\n 2 files changed, 41 insertions(+), 19 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 26ad4bc..0e29b75 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -744,41 +744,63 @@ def originP4BranchesExist():\n \n def p4ChangesForPaths(depotPaths, changeRange, block_size):\n assert depotPaths\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+ changeStart = 1\n+ changeEnd = None\n else:\n parts = changeRange.split(',')\n assert len(parts) == 2\n- changeStart = parts[0]\n- changeEnd = parts[1]\n+ changeStart = int(parts[0][1:])\n+ if parts[1] == '#head':\n+ changeEnd = None\n+ else:\n+ changeEnd = int(parts[1])\n \n # Accumulate change numbers in a dictionary to avoid duplicates\n changes = {}\n \n+ # We need the most recent change list number if we're operating in\n+ # batch mode. For whatever reason, clients with limited MaxResults\n+ # can get this for the entire depot, but not for individual bits of\n+ # the depot.\n+ if block_size:\n+ results = p4CmdList([\"changes\", \"-m\", \"1\"])\n+ mostRecentCommit = int(results[0]['change'])\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+\n+ if block_size:\n+ end = changeStart + block_size # only fetch a few at a time\n+ else:\n+ end = changeEnd # fetch as many as possible\n+\n+ if end:\n+ endStr = str(end)\n+ else:\n+ endStr = '#head'\n+\n+ cmd += [\"%s...@%d,%s\" % (p, changeStart, endStr)]\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+\n+ if not block_size:\n+ # Not batched, so nothing more to do\n get_another_block = False\n+ elif end >= mostRecentCommit:\n+ get_another_block = False\n+ else:\n+ changeStart = end + 1\n \n changelist = changes.keys()\n changelist.sort()\n@@ -1974,7 +1996,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.3.4.48.g223ab37\n"},{"id":"263152","messageId":"CALM2Sna_sdD_95MO3EbF0+QSpB9W1K8Rv3-TNOmnovWG57gh7g@mail.gmail.com","threadId":"39550","inReplyTo":"1433672505-11940-1-git-send-email-luke@diamand.org","subject":"Re: [PATCHv1 0/3] git-p4: fixing --changes-block-size support","fromName":"Lex Spoon","fromEmail":"lex@lexspoon.org","sentAt":"2015-06-07T16:01:09Z","receivedAt":"2015-06-07T16:01:09Z","isPatch":false,"sender":{"key":"lex@lexspoon.org","avatar":"https://avatars.githubusercontent.com/u/274906?v=4"},"body":"Great work.\n\nFor curiosity's sake, the -m solution has been observed to work on at\nleast one Perforce installation. However clearly it doesn't work on\nothers, so the batch ranges approach looks like it will be better.\n\nBased on what has been seen so far, the Perforce maxscanrows setting\nmust be applying the low-level database queries that Perforce uses\ninternally in its implementation. That makes the precise effect on\nexternal queries rather hard to predict. It likely also depends on the\nversion of Perforce.\n\nLex Spoon\n"},{"id":"263151","messageId":"CALM2SnZ7o1P8+NadEKWVuXD+ajHDdiBeM8xrLfnnKuwHGGjJbA@mail.gmail.com","threadId":"39550","inReplyTo":"1433672505-11940-2-git-send-email-luke@diamand.org","subject":"Re: [PATCHv1 1/3] git-p4: additional testing of --changes-block-size","fromName":"Lex Spoon","fromEmail":"lex@lexspoon.org","sentAt":"2015-06-07T16:06:05Z","receivedAt":"2015-06-07T16:06:05Z","isPatch":false,"sender":{"key":"lex@lexspoon.org","avatar":"https://avatars.githubusercontent.com/u/274906?v=4"},"body":"I'll add in reviews since I touched similar code, but I don't know\nwhether it's sufficient given I don't know the code very well.\n\nAnyway, these tests LGTM. Having a smaller test repository is fine,\nand the new tests for files outside the client spec are a great idea.\n-Lex\n"},{"id":"263153","messageId":"CALM2SnZk1vHCxg-47=tktzpvULw9+SaKQNBNNoFre=aMPO-cUg@mail.gmail.com","threadId":"39550","inReplyTo":"1433672505-11940-3-git-send-email-luke@diamand.org","subject":"Re: [PATCHv1 2/3] git-p4: test with limited p4 server results","fromName":"Lex Spoon","fromEmail":"lex@lexspoon.org","sentAt":"2015-06-07T16:11:06Z","receivedAt":"2015-06-07T16:11:06Z","isPatch":false,"sender":{"key":"lex@lexspoon.org","avatar":"https://avatars.githubusercontent.com/u/274906?v=4"},"body":"LGTM. That's great adding a user with the appropriate restrictions on\nit to really exercise the functionality. -Lex\n"},{"id":"263154","messageId":"CALM2SnbVY1baAONo3o2gb2NS+rDSsyhkPffP5EJZKU1MDA7q9w@mail.gmail.com","threadId":"39550","inReplyTo":"1433672505-11940-4-git-send-email-luke@diamand.org","subject":"Re: [PATCHv1 3/3] git-p4: fixing --changes-block-size handling","fromName":"Lex Spoon","fromEmail":"lex@lexspoon.org","sentAt":"2015-06-07T16:33:20Z","receivedAt":"2015-06-07T16:33:20Z","isPatch":false,"sender":{"key":"lex@lexspoon.org","avatar":"https://avatars.githubusercontent.com/u/274906?v=4"},"body":"The implementation looks fine, especially given the test cases that\nback it up. I am only curious why the block size is set to a default\nof None. To put it as contcretely as possible: is there any expected\nconfiguration where None would work but 500 would not? We know there\nare many cases of the other way around, and those cases are going to\nsend users to StackOverflow to find the right workaround.\n\nDropping the option would also simplify the code in several places.\nThe complex logic around get_another_block could be removed, and\ninstead there could be a loop from start to mostRecentCommit by\nblock_size. Several places that check \"if not block_size\" could just\nchoose the other branch.\n\nLex Spoon\n"},{"id":"263155","messageId":"55747846.3060305@diamand.org","threadId":"39550","inReplyTo":"CALM2Sna_sdD_95MO3EbF0+QSpB9W1K8Rv3-TNOmnovWG57gh7g@mail.gmail.com","subject":"Re: [PATCHv1 0/3] git-p4: fixing --changes-block-size support","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-06-07T16:58:46Z","receivedAt":"2015-06-07T16:58:46Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 07/06/15 17:01, Lex Spoon wrote:\n> Great work.\n\nThanks! I actually found the problem in my day job, so it was very handy \nhaving all the infrastructure already in place!\n>\n> For curiosity's sake, the -m solution has been observed to work on at\n> least one Perforce installation. However clearly it doesn't work on\n> others, so the batch ranges approach looks like it will be better.\n\nYes, I can easily imagine that it's changed from one version to the \nnext. I tried going back to a 2014.2 server which still had the same \nproblem (with maxresults), but my investigations were not very exhaustive!\n\n>\n> Based on what has been seen so far, the Perforce maxscanrows setting\n> must be applying the low-level database queries that Perforce uses\n> internally in its implementation. That makes the precise effect on\n> external queries rather hard to predict. It likely also depends on the\n> version of Perforce.\n\nIndeed. All sorts of things can cause it to fail; I've seen it reject \n\"p4 files\" and \"p4 print\", albeit with artificially low maxscanrows and \nmaxresults values. I think this means there's no way to ever make it \nreliably work for all possible sizes of depot and values of \nmaxscanrows/maxresults.\n\nLuke\n"},{"id":"263156","messageId":"55747A05.3070704@diamand.org","threadId":"39550","inReplyTo":"CALM2SnbVY1baAONo3o2gb2NS+rDSsyhkPffP5EJZKU1MDA7q9w@mail.gmail.com","subject":"Re: [PATCHv1 3/3] git-p4: fixing --changes-block-size handling","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-06-07T17:06:13Z","receivedAt":"2015-06-07T17:06:13Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 07/06/15 17:33, Lex Spoon wrote:\n> The implementation looks fine, especially given the test cases that\n> back it up. I am only curious why the block size is set to a default\n> of None. To put it as contcretely as possible: is there any expected\n> configuration where None would work but 500 would not? We know there\n> are many cases of the other way around, and those cases are going to\n> send users to StackOverflow to find the right workaround.\n\nI think it was just caution: it's pretty easy to make it fall back to \nthe old non-batched scheme, so if it turns out that there *is* a \nproblem, fewer people will hit the problem and we're less likely to have \na paper-bag release.\n\n>\n> Dropping the option would also simplify the code in several places.\n> The complex logic around get_another_block could be removed, and\n> instead there could be a loop from start to mostRecentCommit by\n> block_size. Several places that check \"if not block_size\" could just\n> choose the other branch.\n\nFair point. I'll give it a go and see what happens.\n\n(Plus 500 is a very unnatural number, chosen just because we still place \nsome kind of significance on a chance evolutionary accident that gave \nour ape ancestors 5 digits on each hand :-)\n\nLuke\n"},{"id":"263161","messageId":"1433712905-7508-1-git-send-email-luke@diamand.org","threadId":"39550","inReplyTo":"55747A05.3070704@diamand.org","subject":"[PATCHv2 0/3] git-p4: fixing --changes-block-size handling","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-06-07T21:35:02Z","receivedAt":"2015-06-07T21:35:02Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Updated per Lex's suggestion, so that git-p4 always uses the block mode,\nand takes advantage of this to simplify the loop. This exposed a bug\nin the termination condition.\n\nOne thing to note: 'git p4 sync' claims to support arbitrary p4 revision\nspecifications. I need to check that this is tested and hasn't been broken\nby these changes.\n\nLuke\n\nLuke Diamand (3):\n git-p4: additional testing of --changes-block-size\n git-p4: test with limited p4 server results\n git-p4: fixing --changes-block-size handling\n\n git-p4.py | 45 ++++++++++++++++++------------\n t/t9818-git-p4-block.sh | 73 +++++++++++++++++++++++++++++++++++++++++++------\n 2 files changed, 92 insertions(+), 26 deletions(-)\n\n-- \n2.4.1.502.gb11c5ab\n"},{"id":"263160","messageId":"1433712905-7508-2-git-send-email-luke@diamand.org","threadId":"39550","inReplyTo":"1433712905-7508-1-git-send-email-luke@diamand.org","subject":"[PATCHv2 1/3] git-p4: additional testing of --changes-block-size","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-06-07T21:35:03Z","receivedAt":"2015-06-07T21:35:03Z","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":"263162","messageId":"1433712905-7508-3-git-send-email-luke@diamand.org","threadId":"39550","inReplyTo":"1433712905-7508-1-git-send-email-luke@diamand.org","subject":"[PATCHv2 2/3] git-p4: test with limited p4 server results","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-06-07T21:35:04Z","receivedAt":"2015-06-07T21:35:04Z","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":"263163","messageId":"1433712905-7508-4-git-send-email-luke@diamand.org","threadId":"39550","inReplyTo":"1433712905-7508-1-git-send-email-luke@diamand.org","subject":"[PATCHv2 3/3] git-p4: fixing --changes-block-size handling","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-06-07T21:35:05Z","receivedAt":"2015-06-07T21:35:05Z","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 | 45 ++++++++++++++++++++++++++++-----------------\n t/t9818-git-p4-block.sh | 12 ++++++------\n 2 files changed, 34 insertions(+), 23 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 26ad4bc..4be0037 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -249,6 +249,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@@ -746,39 +750,46 @@ def p4ChangesForPaths(depotPaths, changeRange, block_size):\n assert depotPaths\n assert block_size\n \n+ # We need the most recent change list number since we can't just\n+ # use #head in block mode.\n+ lastChange = p4_last_change()\n+\n # Parse the change range into start and end\n if changeRange is None or changeRange == '':\n- changeStart = '@1'\n- changeEnd = '#head'\n+ changeStart = 1\n+ changeEnd = lastChange\n else:\n parts = changeRange.split(',')\n assert len(parts) == 2\n- changeStart = parts[0]\n- changeEnd = parts[1]\n+ changeStart = int(parts[0][1:])\n+ if parts[1] == '#head':\n+ changeEnd = lastChange\n+ else:\n+ changeEnd = int(parts[1])\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+ end = min(changeEnd, changeStart + block_size)\n+\n cmd = ['changes']\n- cmd += ['-m', str(block_size)]\n- cmd += [\"%s...%s,%s\" % (p, start, end)]\n+ cmd += [\"%s...@%d,%d\" % (p, changeStart, end)]\n+\n+ new_changes = []\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 end >= changeEnd:\n+ break\n+\n+ changeStart = end + 1\n \n changelist = changes.keys()\n changelist.sort()\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":"263164","messageId":"CALM2SnZShkETQoQuNc8e0GsPWzODQACzwjh1qCGeajiN+5sjaw@mail.gmail.com","threadId":"39550","inReplyTo":"1433712905-7508-4-git-send-email-luke@diamand.org","subject":"Re: [PATCHv2 3/3] git-p4: fixing --changes-block-size handling","fromName":"Lex Spoon","fromEmail":"lex@lexspoon.org","sentAt":"2015-06-07T22:58:15Z","receivedAt":"2015-06-07T22:58:15Z","isPatch":false,"sender":{"key":"lex@lexspoon.org","avatar":"https://avatars.githubusercontent.com/u/274906?v=4"},"body":"Unless I am reading something wrong, the \"new_changes\" variable could\nbe dropped now. It was needed for the -m version for detecting the\nsmallest change number that was returned. Otherwise it looks good to\nme.\n"},{"id":"263238","messageId":"xmqqk2vecho1.fsf@gitster.dls.corp.google.com","threadId":"39550","inReplyTo":"CALM2SnZShkETQoQuNc8e0GsPWzODQACzwjh1qCGeajiN+5sjaw@mail.gmail.com","subject":"Re: [PATCHv2 3/3] git-p4: fixing --changes-block-size handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-08T16:02:22Z","receivedAt":"2015-06-08T16:02:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lex Spoon writes:\n\n> Unless I am reading something wrong, the \"new_changes\" variable could\n> be dropped now. It was needed for the -m version for detecting the\n> smallest change number that was returned. Otherwise it looks good to\n> me.\n\nMeaning that I should squash this in to 3/3, right?\n\n\n\ndiff --git a/git-p4.py b/git-p4.py\nindex f201f52..7009766 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -780,10 +780,8 @@ def p4ChangesForPaths(depotPaths, changeRange, block_size):\n cmd = ['changes']\n cmd += [\"%s...@%d,%d\" % (p, changeStart, end)]\n \n- new_changes = []\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 \n if end >= changeEnd:\n-- \n2.4.3-495-gcb7a0d9\n"},{"id":"263247","messageId":"CALM2Snb9nxT9_shhsGBJ6nuSOninKbKC-+GXKwmd=rkuLuLXzw@mail.gmail.com","threadId":"39550","inReplyTo":"xmqqk2vecho1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv2 3/3] git-p4: fixing --changes-block-size handling","fromName":"Lex Spoon","fromEmail":"lex@lexspoon.org","sentAt":"2015-06-08T16:36:48Z","receivedAt":"2015-06-08T16:36:48Z","isPatch":false,"sender":{"key":"lex@lexspoon.org","avatar":"https://avatars.githubusercontent.com/u/274906?v=4"},"body":"Precisely, Junio, that's what I had in mind. The patch with the two\nlines deleted LGTM.\n"},{"id":"263319","messageId":"CAPc5daUFM+46tdPPmRTRu17UvDbyCGrhx_BDEnSepBMgS2r4ag@mail.gmail.com","threadId":"39550","inReplyTo":"5575E264.6040601@diamand.org","subject":"Re: [PATCHv2 3/3] git-p4: fixing --changes-block-size handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-08T22:32:46Z","receivedAt":"2015-06-08T22:32:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Mon, Jun 8, 2015 at 11:43 AM, Luke Diamand wrote:\n> On 08/06/15 18:18, Junio C Hamano wrote:\n>>\n>> Lex Spoon writes:\n>>\n>>> Precisely, Junio, that's what I had in mind. The patch with the two\n>>> lines deleted LGTM.\n>>\n>>\n>> Thanks, will do.\n>\n>\n> I don't think we're quite there yet unfortunately.\n>\n> The current version of git-p4 will let you do things like:\n>\n> $ git p4 clone //depot@1,2015/05/31\n>\n> i.e. get all the revisions between revision 1 and the end of last month.\n>\n> Because my change tries to batch up the revisions, it fails when presented\n> with this.\n>\n> There aren't any test cases for this, but it's documented (briefly) in the\n> manual page.\n>\n> I think that although the current code looks really nice and clean, it's\n> going to have to pick up a bit more complexity to handle non-numerical\n> revisions. I don't think it's possible to do batching at the same time.\n>\n> It shouldn't be too hard though; I'll look at it later this week.\n\n[jch: adding git@ back]\n\nThanks.\n"}]}