[PATCHv1 0/3] git-p4: fixing --changes-block-size support

[PATCHv1 0/3] git-p4: fixing --changes-block-size support

[Luke Diamand]

We recently added support to git-p4 to limit the number of changes it
would try to import at a time. That was to help clients who were being
limited by the "maxscanrows" limit. This used the "-m maxchanges"
argument to "p4 changes" to limit the number of results returned to
git-p4.

Unfortunately it turns out that in practice, the server limits the
number of results returned *before* the "-m maxchanges" argument is
considered. Even supplying a "-m 1" argument doesn't help.

This affects both the "maxscanrows" and "maxresults" group options.

This set of patches updates the t9818 git-p4 tests to show the problem,
and then adds a fix which works by iterating over the changes in batches
(as at present) but using a revision range to limit the number of changes,
rather than "-m $BATCHSIZE".

That means it will in most cases require more transactions with the server,
but usually the effect will be small.

Along the way I also found that "p4 print" can fail if you have a file
with too many changes in it, but there's unfortunately no way to workaround
this. It's fairly unlikely to ever happen in practice.

I think I've covered everything in this fix, but it's possible that there
are still bugs to be uncovered; I find the way that these limits interact
somewhat tricky to understand.

Thanks,
Luke

Luke Diamand (3):
  git-p4: additional testing of --changes-block-size
  git-p4: test with limited p4 server results
  git-p4: fixing --changes-block-size handling

 git-p4.py               | 48 +++++++++++++++++++++++---------
 t/t9818-git-p4-block.sh | 73 +++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 99 insertions(+), 22 deletions(-)

-- 
2.3.4.48.g223ab37

[PATCHv1 1/3] git-p4: additional testing of --changes-block-size

[Luke Diamand]

Add additional tests of some corner-cases of the
--changes-block-size git-p4 parameter.

Also reduce the number of p4 changes created during the
tests, so that they complete faster.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 t/t9818-git-p4-block.sh | 56 +++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 47 insertions(+), 9 deletions(-)

diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
index 153b20a..79765a4 100755
--- a/t/t9818-git-p4-block.sh
+++ b/t/t9818-git-p4-block.sh
@@ -8,18 +8,21 @@ test_expect_success 'start p4d' '
 	start_p4d
 '
 
-test_expect_success 'Create a repo with ~100 changes' '
+test_expect_success 'Create a repo with many changes' '
 	(
-		cd "$cli" &&
+		client_view "//depot/included/... //client/included/..." \
+			    "//depot/excluded/... //client/excluded/..." &&
+		mkdir -p "$cli/included" "$cli/excluded" &&
+		cd "$cli/included" &&
 		>file.txt &&
 		p4 add file.txt &&
 		p4 submit -d "Add file.txt" &&
-		for i in $(test_seq 0 9)
+		for i in $(test_seq 0 5)
 		do
 			>outer$i.txt &&
 			p4 add outer$i.txt &&
 			p4 submit -d "Adding outer$i.txt" &&
-			for j in $(test_seq 0 9)
+			for j in $(test_seq 0 5)
 			do
 				p4 edit file.txt &&
 				echo $i$j >file.txt &&
@@ -30,33 +33,68 @@ test_expect_success 'Create a repo with ~100 changes' '
 '
 
 test_expect_success 'Clone the repo' '
-	git p4 clone --dest="$git" --changes-block-size=10 --verbose //depot@all
+	git p4 clone --dest="$git" --changes-block-size=7 --verbose //depot/included@all
 '
 
 test_expect_success 'All files are present' '
 	echo file.txt >expected &&
 	test_write_lines outer0.txt outer1.txt outer2.txt outer3.txt outer4.txt >>expected &&
-	test_write_lines outer5.txt outer6.txt outer7.txt outer8.txt outer9.txt >>expected &&
+	test_write_lines outer5.txt >>expected &&
 	ls "$git" >current &&
 	test_cmp expected current
 '
 
 test_expect_success 'file.txt is correct' '
-	echo 99 >expected &&
+	echo 55 >expected &&
 	test_cmp expected "$git/file.txt"
 '
 
 test_expect_success 'Correct number of commits' '
 	(cd "$git" && git log --oneline) >log &&
-	test_line_count = 111 log
+	wc -l log &&
+	test_line_count = 43 log
 '
 
 test_expect_success 'Previous version of file.txt is correct' '
 	(cd "$git" && git checkout HEAD^^) &&
-	echo 97 >expected &&
+	echo 53 >expected &&
 	test_cmp expected "$git/file.txt"
 '
 
+# Test git-p4 sync, with some files outside the client specification.
+
+p4_add_file() {
+	(cd "$cli" &&
+		>$1 &&
+		p4 add $1 &&
+		p4 submit -d "Added a file" $1
+	)
+}
+
+test_expect_success 'Add some more files' '
+	for i in $(test_seq 0 10)
+	do
+		p4_add_file "included/x$i" &&
+		p4_add_file "excluded/x$i"
+	done &&
+	for i in $(test_seq 0 10)
+	do
+		p4_add_file "excluded/y$i"
+	done
+'
+
+# This should pick up the 10 new files in "included", but not be confused
+# by the additional files in "excluded"
+test_expect_success 'Syncing files' '
+	(
+		cd "$git" &&
+		git p4 sync --changes-block-size=7 &&
+		git checkout p4/master &&
+		ls -l x* > log &&
+		test_line_count = 11 log
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
2.3.4.48.g223ab37

Re: [PATCHv1 1/3] git-p4: additional testing of --changes-block-size

[Lex Spoon]

I'll add in reviews since I touched similar code, but I don't know
whether it's sufficient given I don't know the code very well.

Anyway, these tests LGTM. Having a smaller test repository is fine,
and the new tests for files outside the client spec are a great idea.
-Lex

[PATCHv1 2/3] git-p4: test with limited p4 server results

[Luke Diamand]

Change the --changes-block-size git-p4 test to use an account with
limited "maxresults" and "maxscanrows" values.

These conditions are applied in the server *before* the "-m maxchanges"
parameter to "p4 changes" is applied, and so the strategy that git-p4
uses for limiting the number of changes does not work. As a result,
the tests all fail.

Note that "maxscanrows" is set quite high, as it appears to not only
limit results from "p4 changes", but *also* limits results from
"p4 print". Files that have more than "maxscanrows" changes seem
(experimentally) to be impossible to print. There's no good way to
work around this.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 t/t9818-git-p4-block.sh | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
index 79765a4..aae1121 100755
--- a/t/t9818-git-p4-block.sh
+++ b/t/t9818-git-p4-block.sh
@@ -8,6 +8,19 @@ test_expect_success 'start p4d' '
 	start_p4d
 '
 
+create_restricted_group() {
+	p4 group -i <<-EOF
+	Group: restricted
+	MaxResults: 7
+	MaxScanRows: 40
+	Users: author
+	EOF
+}
+
+test_expect_success 'Create group with limited maxrows' '
+	create_restricted_group
+'
+
 test_expect_success 'Create a repo with many changes' '
 	(
 		client_view "//depot/included/... //client/included/..." \
@@ -32,11 +45,15 @@ test_expect_success 'Create a repo with many changes' '
 	)
 '
 
-test_expect_success 'Clone the repo' '
+test_expect_success 'Default user cannot fetch changes' '
+	! p4 changes -m 1 //depot/...
+'
+
+test_expect_failure 'Clone the repo' '
 	git p4 clone --dest="$git" --changes-block-size=7 --verbose //depot/included@all
 '
 
-test_expect_success 'All files are present' '
+test_expect_failure 'All files are present' '
 	echo file.txt >expected &&
 	test_write_lines outer0.txt outer1.txt outer2.txt outer3.txt outer4.txt >>expected &&
 	test_write_lines outer5.txt >>expected &&
@@ -44,18 +61,18 @@ test_expect_success 'All files are present' '
 	test_cmp expected current
 '
 
-test_expect_success 'file.txt is correct' '
+test_expect_failure 'file.txt is correct' '
 	echo 55 >expected &&
 	test_cmp expected "$git/file.txt"
 '
 
-test_expect_success 'Correct number of commits' '
+test_expect_failure 'Correct number of commits' '
 	(cd "$git" && git log --oneline) >log &&
 	wc -l log &&
 	test_line_count = 43 log
 '
 
-test_expect_success 'Previous version of file.txt is correct' '
+test_expect_failure 'Previous version of file.txt is correct' '
 	(cd "$git" && git checkout HEAD^^) &&
 	echo 53 >expected &&
 	test_cmp expected "$git/file.txt"
@@ -85,7 +102,7 @@ test_expect_success 'Add some more files' '
 
 # This should pick up the 10 new files in "included", but not be confused
 # by the additional files in "excluded"
-test_expect_success 'Syncing files' '
+test_expect_failure 'Syncing files' '
 	(
 		cd "$git" &&
 		git p4 sync --changes-block-size=7 &&
-- 
2.3.4.48.g223ab37

Re: [PATCHv1 2/3] git-p4: test with limited p4 server results

[Lex Spoon]

LGTM. That's great adding a user with the appropriate restrictions on
it to really exercise the functionality.  -Lex

[PATCHv1 3/3] git-p4: fixing --changes-block-size handling

[Luke Diamand]

The --changes-block-size handling was intended to help when
a user has a limited "maxscanrows" (see "p4 group"). It used
"p4 changes -m $maxchanges" to limit the number of results.

Unfortunately, it turns out that the "maxscanrows" and "maxresults"
limits are actually applied *before* the "-m maxchanges" parameter
is considered (experimentally).

Fix the block-size handling so that it gets blocks of changes
limited by revision number ($Start..$Start+$N, etc). This limits
the number of results early enough that both sets of tests pass.

If the --changes-block-size option is not in use, then the code
naturally falls back to the original scheme and gets as many changes
as possible.

Unfortunately, it also turns out that "p4 print" can fail on
files with more changes than "maxscanrows". This fix is unable to
workaround this problem, although in the real world this shouldn't
normally happen.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py               | 48 +++++++++++++++++++++++++++++++++++-------------
 t/t9818-git-p4-block.sh | 12 ++++++------
 2 files changed, 41 insertions(+), 19 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 26ad4bc..0e29b75 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -744,41 +744,63 @@ def originP4BranchesExist():
 
 def p4ChangesForPaths(depotPaths, changeRange, block_size):
     assert depotPaths
-    assert block_size
 
     # Parse the change range into start and end
     if changeRange is None or changeRange == '':
-        changeStart = '@1'
-        changeEnd = '#head'
+        changeStart = 1
+        changeEnd = None
     else:
         parts = changeRange.split(',')
         assert len(parts) == 2
-        changeStart = parts[0]
-        changeEnd = parts[1]
+        changeStart = int(parts[0][1:])
+        if parts[1] == '#head':
+            changeEnd = None
+        else:
+            changeEnd = int(parts[1])
 
     # Accumulate change numbers in a dictionary to avoid duplicates
     changes = {}
 
+    # We need the most recent change list number if we're operating in
+    # batch mode. For whatever reason, clients with limited MaxResults
+    # can get this for the entire depot, but not for individual bits of
+    # the depot.
+    if block_size:
+        results = p4CmdList(["changes", "-m", "1"])
+        mostRecentCommit = int(results[0]['change'])
+
     for p in depotPaths:
         # Retrieve changes a block at a time, to prevent running
         # into a MaxScanRows error from the server.
         start = changeStart
-        end = changeEnd
         get_another_block = True
         while get_another_block:
             new_changes = []
             cmd = ['changes']
-            cmd += ['-m', str(block_size)]
-            cmd += ["%s...%s,%s" % (p, start, end)]
+
+            if block_size:
+                end = changeStart + block_size    # only fetch a few at a time
+            else:
+                end = changeEnd             # fetch as many as possible
+
+            if end:
+                endStr = str(end)
+            else:
+                endStr = '#head'
+
+            cmd += ["%s...@%d,%s" % (p, changeStart, endStr)]
             for line in p4_read_pipe_lines(cmd):
                 changeNum = int(line.split(" ")[1])
                 new_changes.append(changeNum)
                 changes[changeNum] = True
-            if len(new_changes) == block_size:
-                get_another_block = True
-                end = '@' + str(min(new_changes))
-            else:
+
+            if not block_size:
+                # Not batched, so nothing more to do
                 get_another_block = False
+            elif end >= mostRecentCommit:
+                get_another_block = False
+            else:
+                changeStart = end + 1
 
     changelist = changes.keys()
     changelist.sort()
@@ -1974,7 +1996,7 @@ class P4Sync(Command, P4UserMap):
         self.syncWithOrigin = True
         self.importIntoRemotes = True
         self.maxChanges = ""
-        self.changes_block_size = 500
+        self.changes_block_size = None
         self.keepRepoPath = False
         self.depotPaths = None
         self.p4BranchesInGit = []
diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
index aae1121..3b3ae1f 100755
--- a/t/t9818-git-p4-block.sh
+++ b/t/t9818-git-p4-block.sh
@@ -49,11 +49,11 @@ test_expect_success 'Default user cannot fetch changes' '
 	! p4 changes -m 1 //depot/...
 '
 
-test_expect_failure 'Clone the repo' '
+test_expect_success 'Clone the repo' '
 	git p4 clone --dest="$git" --changes-block-size=7 --verbose //depot/included@all
 '
 
-test_expect_failure 'All files are present' '
+test_expect_success 'All files are present' '
 	echo file.txt >expected &&
 	test_write_lines outer0.txt outer1.txt outer2.txt outer3.txt outer4.txt >>expected &&
 	test_write_lines outer5.txt >>expected &&
@@ -61,18 +61,18 @@ test_expect_failure 'All files are present' '
 	test_cmp expected current
 '
 
-test_expect_failure 'file.txt is correct' '
+test_expect_success 'file.txt is correct' '
 	echo 55 >expected &&
 	test_cmp expected "$git/file.txt"
 '
 
-test_expect_failure 'Correct number of commits' '
+test_expect_success 'Correct number of commits' '
 	(cd "$git" && git log --oneline) >log &&
 	wc -l log &&
 	test_line_count = 43 log
 '
 
-test_expect_failure 'Previous version of file.txt is correct' '
+test_expect_success 'Previous version of file.txt is correct' '
 	(cd "$git" && git checkout HEAD^^) &&
 	echo 53 >expected &&
 	test_cmp expected "$git/file.txt"
@@ -102,7 +102,7 @@ test_expect_success 'Add some more files' '
 
 # This should pick up the 10 new files in "included", but not be confused
 # by the additional files in "excluded"
-test_expect_failure 'Syncing files' '
+test_expect_success 'Syncing files' '
 	(
 		cd "$git" &&
 		git p4 sync --changes-block-size=7 &&
-- 
2.3.4.48.g223ab37

Re: [PATCHv1 3/3] git-p4: fixing --changes-block-size handling

[Lex Spoon]

The implementation looks fine, especially given the test cases that
back it up. I am only curious why the block size is set to a default
of None. To put it as contcretely as possible: is there any expected
configuration where None would work but 500 would not? We know there
are many cases of the other way around, and those cases are going to
send users to StackOverflow to find the right workaround.

Dropping the option would also simplify the code in several places.
The complex logic around get_another_block could be removed, and
instead there could be a loop from start to mostRecentCommit by
block_size. Several places that check "if not block_size" could just
choose the other branch.

Lex Spoon

Re: [PATCHv1 3/3] git-p4: fixing --changes-block-size handling

[Luke Diamand]

On 07/06/15 17:33, Lex Spoon wrote:
> The implementation looks fine, especially given the test cases that
> back it up. I am only curious why the block size is set to a default
> of None. To put it as contcretely as possible: is there any expected
> configuration where None would work but 500 would not? We know there
> are many cases of the other way around, and those cases are going to
> send users to StackOverflow to find the right workaround.

I think it was just caution: it's pretty easy to make it fall back to 
the old non-batched scheme, so if it turns out that there *is* a 
problem, fewer people will hit the problem and we're less likely to have 
a paper-bag release.

>
> Dropping the option would also simplify the code in several places.
> The complex logic around get_another_block could be removed, and
> instead there could be a loop from start to mostRecentCommit by
> block_size. Several places that check "if not block_size" could just
> choose the other branch.

Fair point. I'll give it a go and see what happens.

(Plus 500 is a very unnatural number, chosen just because we still place 
some kind of significance on a chance evolutionary accident that gave 
our ape ancestors 5 digits on each hand :-)

Luke

[PATCHv2 0/3] git-p4: fixing --changes-block-size handling

[Luke Diamand]

Updated per Lex's suggestion, so that git-p4 always uses the block mode,
and takes advantage of this to simplify the loop. This exposed a bug
in the termination condition.

One thing to note: 'git p4 sync' claims to support arbitrary p4 revision
specifications. I need to check that this is tested and hasn't been broken
by these changes.

Luke

Luke Diamand (3):
  git-p4: additional testing of --changes-block-size
  git-p4: test with limited p4 server results
  git-p4: fixing --changes-block-size handling

 git-p4.py               | 45 ++++++++++++++++++------------
 t/t9818-git-p4-block.sh | 73 +++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 92 insertions(+), 26 deletions(-)

-- 
2.4.1.502.gb11c5ab

[PATCHv2 1/3] git-p4: additional testing of --changes-block-size

[Luke Diamand]

Add additional tests of some corner-cases of the
--changes-block-size git-p4 parameter.

Also reduce the number of p4 changes created during the
tests, so that they complete faster.

Signed-off-by: Luke Diamand <luke@diamand.org>
Acked-by: Lex Spoon <lex@lexspoon.org>
---
 t/t9818-git-p4-block.sh | 56 +++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 47 insertions(+), 9 deletions(-)

diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
index 153b20a..79765a4 100755
--- a/t/t9818-git-p4-block.sh
+++ b/t/t9818-git-p4-block.sh
@@ -8,18 +8,21 @@ test_expect_success 'start p4d' '
 	start_p4d
 '
 
-test_expect_success 'Create a repo with ~100 changes' '
+test_expect_success 'Create a repo with many changes' '
 	(
-		cd "$cli" &&
+		client_view "//depot/included/... //client/included/..." \
+			    "//depot/excluded/... //client/excluded/..." &&
+		mkdir -p "$cli/included" "$cli/excluded" &&
+		cd "$cli/included" &&
 		>file.txt &&
 		p4 add file.txt &&
 		p4 submit -d "Add file.txt" &&
-		for i in $(test_seq 0 9)
+		for i in $(test_seq 0 5)
 		do
 			>outer$i.txt &&
 			p4 add outer$i.txt &&
 			p4 submit -d "Adding outer$i.txt" &&
-			for j in $(test_seq 0 9)
+			for j in $(test_seq 0 5)
 			do
 				p4 edit file.txt &&
 				echo $i$j >file.txt &&
@@ -30,33 +33,68 @@ test_expect_success 'Create a repo with ~100 changes' '
 '
 
 test_expect_success 'Clone the repo' '
-	git p4 clone --dest="$git" --changes-block-size=10 --verbose //depot@all
+	git p4 clone --dest="$git" --changes-block-size=7 --verbose //depot/included@all
 '
 
 test_expect_success 'All files are present' '
 	echo file.txt >expected &&
 	test_write_lines outer0.txt outer1.txt outer2.txt outer3.txt outer4.txt >>expected &&
-	test_write_lines outer5.txt outer6.txt outer7.txt outer8.txt outer9.txt >>expected &&
+	test_write_lines outer5.txt >>expected &&
 	ls "$git" >current &&
 	test_cmp expected current
 '
 
 test_expect_success 'file.txt is correct' '
-	echo 99 >expected &&
+	echo 55 >expected &&
 	test_cmp expected "$git/file.txt"
 '
 
 test_expect_success 'Correct number of commits' '
 	(cd "$git" && git log --oneline) >log &&
-	test_line_count = 111 log
+	wc -l log &&
+	test_line_count = 43 log
 '
 
 test_expect_success 'Previous version of file.txt is correct' '
 	(cd "$git" && git checkout HEAD^^) &&
-	echo 97 >expected &&
+	echo 53 >expected &&
 	test_cmp expected "$git/file.txt"
 '
 
+# Test git-p4 sync, with some files outside the client specification.
+
+p4_add_file() {
+	(cd "$cli" &&
+		>$1 &&
+		p4 add $1 &&
+		p4 submit -d "Added a file" $1
+	)
+}
+
+test_expect_success 'Add some more files' '
+	for i in $(test_seq 0 10)
+	do
+		p4_add_file "included/x$i" &&
+		p4_add_file "excluded/x$i"
+	done &&
+	for i in $(test_seq 0 10)
+	do
+		p4_add_file "excluded/y$i"
+	done
+'
+
+# This should pick up the 10 new files in "included", but not be confused
+# by the additional files in "excluded"
+test_expect_success 'Syncing files' '
+	(
+		cd "$git" &&
+		git p4 sync --changes-block-size=7 &&
+		git checkout p4/master &&
+		ls -l x* > log &&
+		test_line_count = 11 log
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
2.4.1.502.gb11c5ab

[PATCHv2 2/3] git-p4: test with limited p4 server results

[Luke Diamand]

Change the --changes-block-size git-p4 test to use an account with
limited "maxresults" and "maxscanrows" values.

These conditions are applied in the server *before* the "-m maxchanges"
parameter to "p4 changes" is applied, and so the strategy that git-p4
uses for limiting the number of changes does not work. As a result,
the tests all fail.

Note that "maxscanrows" is set quite high, as it appears to not only
limit results from "p4 changes", but *also* limits results from
"p4 print". Files that have more than "maxscanrows" changes seem
(experimentally) to be impossible to print. There's no good way to
work around this.

Signed-off-by: Luke Diamand <luke@diamand.org>
Acked-by: Lex Spoon <lex@lexspoon.org>
---
 t/t9818-git-p4-block.sh | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
index 79765a4..aae1121 100755
--- a/t/t9818-git-p4-block.sh
+++ b/t/t9818-git-p4-block.sh
@@ -8,6 +8,19 @@ test_expect_success 'start p4d' '
 	start_p4d
 '
 
+create_restricted_group() {
+	p4 group -i <<-EOF
+	Group: restricted
+	MaxResults: 7
+	MaxScanRows: 40
+	Users: author
+	EOF
+}
+
+test_expect_success 'Create group with limited maxrows' '
+	create_restricted_group
+'
+
 test_expect_success 'Create a repo with many changes' '
 	(
 		client_view "//depot/included/... //client/included/..." \
@@ -32,11 +45,15 @@ test_expect_success 'Create a repo with many changes' '
 	)
 '
 
-test_expect_success 'Clone the repo' '
+test_expect_success 'Default user cannot fetch changes' '
+	! p4 changes -m 1 //depot/...
+'
+
+test_expect_failure 'Clone the repo' '
 	git p4 clone --dest="$git" --changes-block-size=7 --verbose //depot/included@all
 '
 
-test_expect_success 'All files are present' '
+test_expect_failure 'All files are present' '
 	echo file.txt >expected &&
 	test_write_lines outer0.txt outer1.txt outer2.txt outer3.txt outer4.txt >>expected &&
 	test_write_lines outer5.txt >>expected &&
@@ -44,18 +61,18 @@ test_expect_success 'All files are present' '
 	test_cmp expected current
 '
 
-test_expect_success 'file.txt is correct' '
+test_expect_failure 'file.txt is correct' '
 	echo 55 >expected &&
 	test_cmp expected "$git/file.txt"
 '
 
-test_expect_success 'Correct number of commits' '
+test_expect_failure 'Correct number of commits' '
 	(cd "$git" && git log --oneline) >log &&
 	wc -l log &&
 	test_line_count = 43 log
 '
 
-test_expect_success 'Previous version of file.txt is correct' '
+test_expect_failure 'Previous version of file.txt is correct' '
 	(cd "$git" && git checkout HEAD^^) &&
 	echo 53 >expected &&
 	test_cmp expected "$git/file.txt"
@@ -85,7 +102,7 @@ test_expect_success 'Add some more files' '
 
 # This should pick up the 10 new files in "included", but not be confused
 # by the additional files in "excluded"
-test_expect_success 'Syncing files' '
+test_expect_failure 'Syncing files' '
 	(
 		cd "$git" &&
 		git p4 sync --changes-block-size=7 &&
-- 
2.4.1.502.gb11c5ab

[PATCHv2 3/3] git-p4: fixing --changes-block-size handling

[Luke Diamand]

The --changes-block-size handling was intended to help when
a user has a limited "maxscanrows" (see "p4 group"). It used
"p4 changes -m $maxchanges" to limit the number of results.

Unfortunately, it turns out that the "maxscanrows" and "maxresults"
limits are actually applied *before* the "-m maxchanges" parameter
is considered (experimentally).

Fix the block-size handling so that it gets blocks of changes
limited by revision number ($Start..$Start+$N, etc). This limits
the number of results early enough that both sets of tests pass.

Note that many other Perforce operations can fail for the same
reason (p4 print, p4 files, etc) and it's probably not possible
to workaround this. In the real world, this is probably not
usually a problem.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py               | 45 ++++++++++++++++++++++++++++-----------------
 t/t9818-git-p4-block.sh | 12 ++++++------
 2 files changed, 34 insertions(+), 23 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 26ad4bc..4be0037 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -249,6 +249,10 @@ def p4_reopen(type, f):
 def p4_move(src, dest):
     p4_system(["move", "-k", wildcard_encode(src), wildcard_encode(dest)])
 
+def p4_last_change():
+    results = p4CmdList(["changes", "-m", "1"])
+    return int(results[0]['change'])
+
 def p4_describe(change):
     """Make sure it returns a valid result by checking for
        the presence of field "time".  Return a dict of the
@@ -746,39 +750,46 @@ def p4ChangesForPaths(depotPaths, changeRange, block_size):
     assert depotPaths
     assert block_size
 
+    # We need the most recent change list number since we can't just
+    # use #head in block mode.
+    lastChange = p4_last_change()
+
     # Parse the change range into start and end
     if changeRange is None or changeRange == '':
-        changeStart = '@1'
-        changeEnd = '#head'
+        changeStart = 1
+        changeEnd = lastChange
     else:
         parts = changeRange.split(',')
         assert len(parts) == 2
-        changeStart = parts[0]
-        changeEnd = parts[1]
+        changeStart = int(parts[0][1:])
+        if parts[1] == '#head':
+            changeEnd = lastChange
+        else:
+            changeEnd = int(parts[1])
 
     # Accumulate change numbers in a dictionary to avoid duplicates
     changes = {}
 
     for p in depotPaths:
         # Retrieve changes a block at a time, to prevent running
-        # into a MaxScanRows error from the server.
-        start = changeStart
-        end = changeEnd
-        get_another_block = True
-        while get_another_block:
-            new_changes = []
+        # into a MaxResults/MaxScanRows error from the server.
+
+        while True:
+            end = min(changeEnd, changeStart + block_size)
+
             cmd = ['changes']
-            cmd += ['-m', str(block_size)]
-            cmd += ["%s...%s,%s" % (p, start, end)]
+            cmd += ["%s...@%d,%d" % (p, changeStart, end)]
+
+            new_changes = []
             for line in p4_read_pipe_lines(cmd):
                 changeNum = int(line.split(" ")[1])
                 new_changes.append(changeNum)
                 changes[changeNum] = True
-            if len(new_changes) == block_size:
-                get_another_block = True
-                end = '@' + str(min(new_changes))
-            else:
-                get_another_block = False
+
+            if end >= changeEnd:
+                break
+
+            changeStart = end + 1
 
     changelist = changes.keys()
     changelist.sort()
diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
index aae1121..3b3ae1f 100755
--- a/t/t9818-git-p4-block.sh
+++ b/t/t9818-git-p4-block.sh
@@ -49,11 +49,11 @@ test_expect_success 'Default user cannot fetch changes' '
 	! p4 changes -m 1 //depot/...
 '
 
-test_expect_failure 'Clone the repo' '
+test_expect_success 'Clone the repo' '
 	git p4 clone --dest="$git" --changes-block-size=7 --verbose //depot/included@all
 '
 
-test_expect_failure 'All files are present' '
+test_expect_success 'All files are present' '
 	echo file.txt >expected &&
 	test_write_lines outer0.txt outer1.txt outer2.txt outer3.txt outer4.txt >>expected &&
 	test_write_lines outer5.txt >>expected &&
@@ -61,18 +61,18 @@ test_expect_failure 'All files are present' '
 	test_cmp expected current
 '
 
-test_expect_failure 'file.txt is correct' '
+test_expect_success 'file.txt is correct' '
 	echo 55 >expected &&
 	test_cmp expected "$git/file.txt"
 '
 
-test_expect_failure 'Correct number of commits' '
+test_expect_success 'Correct number of commits' '
 	(cd "$git" && git log --oneline) >log &&
 	wc -l log &&
 	test_line_count = 43 log
 '
 
-test_expect_failure 'Previous version of file.txt is correct' '
+test_expect_success 'Previous version of file.txt is correct' '
 	(cd "$git" && git checkout HEAD^^) &&
 	echo 53 >expected &&
 	test_cmp expected "$git/file.txt"
@@ -102,7 +102,7 @@ test_expect_success 'Add some more files' '
 
 # This should pick up the 10 new files in "included", but not be confused
 # by the additional files in "excluded"
-test_expect_failure 'Syncing files' '
+test_expect_success 'Syncing files' '
 	(
 		cd "$git" &&
 		git p4 sync --changes-block-size=7 &&
-- 
2.4.1.502.gb11c5ab

Re: [PATCHv2 3/3] git-p4: fixing --changes-block-size handling

[Lex Spoon]

Unless I am reading something wrong, the "new_changes" variable could
be dropped now. It was needed for the -m version for detecting the
smallest change number that was returned. Otherwise it looks good to
me.

Re: [PATCHv2 3/3] git-p4: fixing --changes-block-size handling

[Junio C Hamano]

Lex Spoon <lex@lexspoon.org> writes:

> Unless I am reading something wrong, the "new_changes" variable could
> be dropped now. It was needed for the -m version for detecting the
> smallest change number that was returned. Otherwise it looks good to
> me.

Meaning that I should squash this in to 3/3, right?



diff --git a/git-p4.py b/git-p4.py
index f201f52..7009766 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -780,10 +780,8 @@ def p4ChangesForPaths(depotPaths, changeRange, block_size):
             cmd = ['changes']
             cmd += ["%s...@%d,%d" % (p, changeStart, end)]
 
-            new_changes = []
             for line in p4_read_pipe_lines(cmd):
                 changeNum = int(line.split(" ")[1])
-                new_changes.append(changeNum)
                 changes[changeNum] = True
 
             if end >= changeEnd:
-- 
2.4.3-495-gcb7a0d9

Re: [PATCHv2 3/3] git-p4: fixing --changes-block-size handling

[Lex Spoon]

Precisely, Junio, that's what I had in mind. The patch with the two
lines deleted LGTM.

Re: [PATCHv2 3/3] git-p4: fixing --changes-block-size handling

[Junio C Hamano]

On Mon, Jun 8, 2015 at 11:43 AM, Luke Diamand <luke@diamand.org> wrote:
> On 08/06/15 18:18, Junio C Hamano wrote:
>>
>> Lex Spoon <lex@lexspoon.org> writes:
>>
>>> Precisely, Junio, that's what I had in mind. The patch with the two
>>> lines deleted LGTM.
>>
>>
>> Thanks, will do.
>
>
> I don't think we're quite there yet unfortunately.
>
> The current version of git-p4 will let you do things like:
>
> $ git p4 clone //depot@1,2015/05/31
>
> i.e. get all the revisions between revision 1 and the end of last month.
>
> Because my change tries to batch up the revisions, it fails when presented
> with this.
>
> There aren't any test cases for this, but it's documented (briefly) in the
> manual page.
>
> I think that although the current code looks really nice and clean, it's
> going to have to pick up a bit more complexity to handle non-numerical
> revisions. I don't think it's possible to do batching at the same time.
>
> It shouldn't be too hard though; I'll look at it later this week.

[jch: adding git@ back]

Thanks.

Re: [PATCHv1 0/3] git-p4: fixing --changes-block-size support

[Lex Spoon]

Great work.

For curiosity's sake, the -m solution has been observed to work on at
least one Perforce installation. However clearly it doesn't work on
others, so the batch ranges approach looks like it will be better.

Based on what has been seen so far, the Perforce maxscanrows setting
must be applying the low-level database queries that Perforce uses
internally in its implementation. That makes the precise effect on
external queries rather hard to predict. It likely also depends on the
version of Perforce.

Lex Spoon

Re: [PATCHv1 0/3] git-p4: fixing --changes-block-size support

[Luke Diamand]

On 07/06/15 17:01, Lex Spoon wrote:
> Great work.

Thanks! I actually found the problem in my day job, so it was very handy 
having all the infrastructure already in place!
>
> For curiosity's sake, the -m solution has been observed to work on at
> least one Perforce installation. However clearly it doesn't work on
> others, so the batch ranges approach looks like it will be better.

Yes, I can easily imagine that it's changed from one version to the 
next. I tried going back to a 2014.2 server which still had the same 
problem (with maxresults), but my investigations were not very exhaustive!

>
> Based on what has been seen so far, the Perforce maxscanrows setting
> must be applying the low-level database queries that Perforce uses
> internally in its implementation. That makes the precise effect on
> external queries rather hard to predict. It likely also depends on the
> version of Perforce.

Indeed. All sorts of things can cause it to fail; I've seen it reject 
"p4 files" and "p4 print", albeit with artificially low maxscanrows and 
maxresults values. I think this means there's no way to ever make it 
reliably work for all possible sizes of depot and values of 
maxscanrows/maxresults.

Luke