[PATCHv3 0/4] git-p4: fixing --changes-block-size handling

[PATCHv3 0/4] git-p4: fixing --changes-block-size handling

[Luke Diamand]

This series of patches teaches git-p4 to break up calls to the
P4 server into smaller chunks, to avoid hitting the maxresults and
maxscanrows server-side limits.

The previous iteration of this series didn't handle non-integer
P4 revision ranges (e.g. //depot/...@2014/1/1,2015/1/1).

This version now breaks up the commit into blocks iff the revision
range specified is an integer range: @all, M,N or N,#head. If the
range is non-numeric then it falls back to relying on Perforce to
parse the revisions. In this mode it is no longer possible to fetch
changes in blocks (and requests to do so will be rejected).

Another alternative would be to turn the date (or whatever) revisions
into numeric revision numbers, but there doesn't seem to be a simple
way to do this. The best I can come up with is to get the changes
"around" the date in question (perhaps binary-chopping from years down
to seconds to find a range that works?) and then take the lowest
revision supplied. But that's quite a bit more complex.

Luke Diamand (4):
  git-p4: additional testing of --changes-block-size
  git-p4: test with limited p4 server results
  git-p4: add tests for non-numeric revision range
  git-p4: fixing --changes-block-size handling

 git-p4.py               | 85 ++++++++++++++++++++++++++++++++++++-------------
 t/t9800-git-p4-basic.sh | 38 ++++++++++++++++++++++
 t/t9818-git-p4-block.sh | 73 ++++++++++++++++++++++++++++++++++++------
 3 files changed, 165 insertions(+), 31 deletions(-)

-- 
2.4.1.502.gb11c5ab

[PATCHv3 1/4] 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

[PATCHv3 2/4] 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

[PATCHv3 3/4] git-p4: add tests for non-numeric revision range

[Luke Diamand]

Test that git-p4 can handle a sync with a non-numeric revision
range (e.g. a date).

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 t/t9800-git-p4-basic.sh | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 5b56212..90d41ed 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -131,6 +131,44 @@ test_expect_success 'clone two dirs, @all, conflicting files' '
 	)
 '
 
+revision_ranges="2000/01/01,#head \
+		 1,2080/01/01 \
+		 2000/01/01,2080/01/01 \
+		 2000/01/01,1000 \
+		 1,1000"
+
+test_expect_success 'clone using non-numeric revision ranges' '
+	test_when_finished cleanup_git &&
+	for r in $revision_ranges
+	do
+		rm -fr "$git" &&
+		test ! -d "$git" &&
+		git p4 clone --dest="$git" //depot@$r &&
+		(
+			cd "$git" &&
+			git ls-files >lines &&
+			test_line_count = 6 lines
+		)
+	done
+'
+
+test_expect_success 'clone with date range, excluding some changes' '
+	test_when_finished cleanup_git &&
+	before=$(date +%Y/%m/%d:%H:%M:%S) &&
+	sleep 2 &&
+	(
+		cd "$cli" &&
+		:>date_range_test &&
+		p4 add date_range_test &&
+		p4 submit -d "Adding file"
+	) &&
+	git p4 clone --dest="$git" //depot@1,$before &&
+	(
+		cd "$git" &&
+		test_path_is_missing date_range_test
+	)
+'
+
 test_expect_success 'exit when p4 fails to produce marshaled output' '
 	mkdir badp4dir &&
 	test_when_finished "rm badp4dir/p4 && rmdir badp4dir" &&
-- 
2.4.1.502.gb11c5ab

[PATCHv3 4/4] 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               | 85 ++++++++++++++++++++++++++++++++++++-------------
 t/t9818-git-p4-block.sh | 12 +++----
 2 files changed, 69 insertions(+), 28 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 26ad4bc..073f87b 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -43,6 +43,9 @@ verbose = False
 # Only labels/tags matching this will be imported/exported
 defaultLabelRegexp = r'[a-zA-Z0-9_\-.]+$'
 
+# Grab changes in blocks of this many revisions, unless otherwise requested
+defaultBlockSize = 512
+
 def p4_build_cmd(cmd):
     """Build a suitable p4 command line.
 
@@ -249,6 +252,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
@@ -742,43 +749,77 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = "refs/remotes/p4/", silent
 def originP4BranchesExist():
         return gitBranchExists("origin") or gitBranchExists("origin/p4") or gitBranchExists("origin/p4/master")
 
-def p4ChangesForPaths(depotPaths, changeRange, block_size):
+
+def p4ParseNumericChangeRange(parts):
+    changeStart = int(parts[0][1:])
+    if parts[1] == '#head':
+        changeEnd = p4_last_change()
+    else:
+        changeEnd = int(parts[1])
+
+    return (changeStart, changeEnd)
+
+def chooseBlockSize(blockSize):
+    if blockSize:
+        return blockSize
+    else:
+        return defaultBlockSize
+
+def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):
     assert depotPaths
-    assert block_size
 
-    # Parse the change range into start and end
+    # Parse the change range into start and end. Try to find integer
+    # revision ranges as these can be broken up into blocks to avoid
+    # hitting server-side limits (maxrows, maxscanresults). But if
+    # that doesn't work, fall back to using the raw revision specifier
+    # strings, without using block mode.
+
     if changeRange is None or changeRange == '':
-        changeStart = '@1'
-        changeEnd = '#head'
+        changeStart = 1
+        changeEnd = p4_last_change()
+        block_size = chooseBlockSize(requestedBlockSize)
     else:
         parts = changeRange.split(',')
         assert len(parts) == 2
-        changeStart = parts[0]
-        changeEnd = parts[1]
+        try:
+            (changeStart, changeEnd) = p4ParseNumericChangeRange(parts)
+            block_size = chooseBlockSize(requestedBlockSize)
+        except:
+            changeStart = parts[0][1:]
+            changeEnd = parts[1]
+            if requestedBlockSize:
+                die("cannot use --changes-block-size with non-numeric revisions")
+            block_size = None
 
     # 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:
             cmd = ['changes']
-            cmd += ['-m', str(block_size)]
-            cmd += ["%s...%s,%s" % (p, start, end)]
+
+            if block_size:
+                end = min(changeEnd, changeStart + block_size)
+                revisionRange = "%d,%d" % (changeStart, end)
+            else:
+                revisionRange = "%s,%s" % (changeStart, changeEnd)
+
+            cmd += ["%s...@%s" % (p, revisionRange)]
+
             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 not block_size:
+                break
+
+            if end >= changeEnd:
+                break
+
+            changeStart = end + 1
 
     changelist = changes.keys()
     changelist.sort()
@@ -1974,7 +2015,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.4.1.502.gb11c5ab

Re: [PATCHv3 0/4] git-p4: fixing --changes-block-size handling

[Lex Spoon]

The latest patch series LGTM. It's a pity about the more complicated
structure with two different ways to query the changes list, but it
does look hard to make it any simpler.

Lex Spoon