Re: [PATCH] git-p4: Use -m when running p4 changes

Re: [PATCH] git-p4: Use -m when running p4 changes

[Luke Diamand]

On 11 April 2015 at 16:17, Lex Spoon <lex@lexspoon.org> wrote:
>
>
> Signed-off-by: Lex Spoon <lex@lexspoon.org>
> ---
> This patch addresses a problem I am running into with a client. I am
> attempting to mirror their Perforce repository into Git, and on certain
> branches their Perforce server is responding with an error about "too many
> rows scanned". This change has git-p4 use the "-m" option to return just 500
> changes at a time, thus avoiding the problem.

Thanks - that's a problem I also occasionally hit, and it definitely
needs fixing.

Your fix is quite nice - I started out thinking this should be easy,
but it's not!

A test case addition would be good if you can though - otherwise it's
certain to break at some point in the future. Would you have time to
add that?

Thanks!
Luke


>
> I have tested this on a small test repository (2000 revisions) and it
> appears to work fine. I have also run all the t98* tests; those print a
> number of yellow "not ok" results but no red ones. I presume this is the
> expected test behavior?

Yes.

>
> I considered making the block size configurable, but it seems unlikely
> anyone will strongly benefit from changing it. 500 is large enough that it
> should only take a modest number of iterations to scan the full changes
> list, but it's small enough that any reasonable Perforce server should allow
> the request.

Might be useful when making test harnesses though :-)


>
> This patch is also available on GitHub:
> https://github.com/lexspoon/git/tree/p4-sync-batches
>
>  git-p4.py | 40 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 33 insertions(+), 7 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 549022e..ce1447b 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -742,15 +742,41 @@ def originP4BranchesExist():
>
>  def p4ChangesForPaths(depotPaths, changeRange):
>      assert depotPaths
> -    cmd = ['changes']
> -    for p in depotPaths:
> -        cmd += ["%s...%s" % (p, changeRange)]
> -    output = p4_read_pipe_lines(cmd)
>
> +    # Parse the change range into start and end
> +    if changeRange is None or changeRange == '':
> +        changeStart = '@1'
> +        changeEnd = '#head'
> +    else:
> +        parts = changeRange.split(',')
> +        assert len(parts) == 2
> +        changeStart = parts[0]
> +        changeEnd = parts[1]
> +
> +    # Accumulate change numbers in a dictionary to avoid duplicates
>      changes = {}
> -    for line in output:
> -        changeNum = int(line.split(" ")[1])
> -        changes[changeNum] = True
> +
> +    for p in depotPaths:
> +        # Retrieve changes a block at a time, to prevent running
> +        # into a MaxScanRows error from the server.
> +        block_size = 500
> +        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)]
> +            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
>
>      changelist = changes.keys()
>      changelist.sort()
> --
> 1.9.1
>

Re: [PATCH] git-p4: Use -m when running p4 changes

[Lex Spoon]

(resending with accidental HTML removed)


Great, I'm glad it looks like a good approach!

I'll add a test case for it.... and to support the test case, an
option for the block size. I guess the block-size option will go on
"sync", "clone", and "fetch". Alternatively, maybe someone has a
better suggestion of how to configure the block size.

Lex Spoon

Re: [PATCH] git-p4: Use -m when running p4 changes

[Lex Spoon]

>From 9cc607667a20317c837afd90d50c078da659b72f Mon Sep 17 00:00:00 2001
From: Lex Spoon <lex@lexspoon.org>
Date: Sat, 11 Apr 2015 10:01:15 -0400
Subject: [PATCH] git-p4: Use -m when running p4 changes

Signed-off-by: Lex Spoon <lex@lexspoon.org>
---
Updated to include a test case

 git-p4.py               | 51 ++++++++++++++++++++++++++++++---------
 t/t9818-git-p4-block.sh | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+), 11 deletions(-)
 create mode 100755 t/t9818-git-p4-block.sh

diff --git a/git-p4.py b/git-p4.py
index 549022e..2fc8d9c 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -740,17 +740,43 @@ 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):
+def p4ChangesForPaths(depotPaths, changeRange, block_size):
     assert depotPaths
-    cmd = ['changes']
-    for p in depotPaths:
-        cmd += ["%s...%s" % (p, changeRange)]
-    output = p4_read_pipe_lines(cmd)
+    assert block_size
+
+    # Parse the change range into start and end
+    if changeRange is None or changeRange == '':
+        changeStart = '@1'
+        changeEnd = '#head'
+    else:
+        parts = changeRange.split(',')
+        assert len(parts) == 2
+        changeStart = parts[0]
+        changeEnd = parts[1]

+    # Accumulate change numbers in a dictionary to avoid duplicates
     changes = {}
-    for line in output:
-        changeNum = int(line.split(" ")[1])
-        changes[changeNum] = True
+
+    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)]
+            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

     changelist = changes.keys()
     changelist.sort()
@@ -1912,6 +1938,8 @@ class P4Sync(Command, P4UserMap):
                 optparse.make_option("--import-local",
dest="importIntoRemotes", action="store_false",
                                      help="Import into refs/heads/ ,
not refs/remotes"),
                 optparse.make_option("--max-changes", dest="maxChanges"),
+                optparse.make_option("--changes-block-size",
dest="changes_block_size", type="int",
+                                     help="Block size for calling p4 changes"),
                 optparse.make_option("--keep-path",
dest="keepRepoPath", action='store_true',
                                      help="Keep entire
BRANCH/DIR/SUBDIR prefix during import"),
                 optparse.make_option("--use-client-spec",
dest="useClientSpec", action='store_true',
@@ -1940,6 +1968,7 @@ class P4Sync(Command, P4UserMap):
         self.syncWithOrigin = True
         self.importIntoRemotes = True
         self.maxChanges = ""
+        self.changes_block_size = 500
         self.keepRepoPath = False
         self.depotPaths = None
         self.p4BranchesInGit = []
@@ -2578,7 +2607,7 @@ class P4Sync(Command, P4UserMap):

         return ""

-    def importNewBranch(self, branch, maxChange):
+    def importNewBranch(self, branch, maxChange, changes_block_size):
         # make fast-import flush all changes to disk and update the
refs using the checkpoint
         # command so that we can try to find the branch parent in the
git history
         self.gitStream.write("checkpoint\n\n");
@@ -2586,7 +2615,7 @@ class P4Sync(Command, P4UserMap):
         branchPrefix = self.depotPaths[0] + branch + "/"
         range = "@1,%s" % maxChange
         #print "prefix" + branchPrefix
-        changes = p4ChangesForPaths([branchPrefix], range)
+        changes = p4ChangesForPaths([branchPrefix], range, changes_block_size)
         if len(changes) <= 0:
             return False
         firstChange = changes[0]
@@ -3002,7 +3031,7 @@ class P4Sync(Command, P4UserMap):
                 if self.verbose:
                     print "Getting p4 changes for %s...%s" % (',
'.join(self.depotPaths),
                                                               self.changeRange)
-                changes = p4ChangesForPaths(self.depotPaths, self.changeRange)
+                changes = p4ChangesForPaths(self.depotPaths,
self.changeRange, self.changes_block_size)

                 if len(self.maxChanges) > 0:
                     changes = changes[:min(int(self.maxChanges), len(changes))]
diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
new file mode 100755
index 0000000..73e545d
--- /dev/null
+++ b/t/t9818-git-p4-block.sh
@@ -0,0 +1,64 @@
+#!/bin/sh
+
+test_description='git p4 fetching changes in multiple blocks'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+ start_p4d
+'
+
+test_expect_success 'Create a repo with 100 changes' '
+ (
+ cd "$cli" &&
+ touch file.txt &&
+ p4 add file.txt &&
+ p4 submit -d "Add file.txt" &&
+ for i in 0 1 2 3 4 5 6 7 8 9
+ do
+ touch outer$i.txt &&
+ p4 add outer$i.txt &&
+ p4 submit -d "Adding outer$i.txt" &&
+ for j in 0 1 2 3 4 5 6 7 8 9
+ do
+ p4 edit file.txt &&
+ echo $i$j > file.txt &&
+ p4 submit -d "Commit $i$j"
+ done
+ done
+ )
+'
+
+test_expect_success 'Clone the repo' '
+ git p4 clone --dest="$git" --changes-block-size=10 --verbose //depot@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 &&
+ ls "$git" >current &&
+ test_cmp expected current
+'
+
+test_expect_success 'file.txt is correct' '
+ echo 99 >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
+'
+
+test_expect_success 'Previous version of file.txt is correct' '
+ (cd "$git"; git checkout HEAD^^) &&
+ echo 97 >expected &&
+ test_cmp expected "$git/file.txt"
+'
+
+test_expect_success 'kill p4d' '
+ kill_p4d
+'
+
+test_done
-- 
1.9.1

Re: [PATCH] git-p4: Use -m when running p4 changes

[Junio C Hamano]

Lex Spoon <lex@lexspoon.org> writes:

> From 9cc607667a20317c837afd90d50c078da659b72f Mon Sep 17 00:00:00 2001
> From: Lex Spoon <lex@lexspoon.org>
> Date: Sat, 11 Apr 2015 10:01:15 -0400
> Subject: [PATCH] git-p4: Use -m when running p4 changes

All of the above is duplicate and shouldn't be added to the message;
the recipient can pick them up from the e-mail headers.

Please explain what this change intends to do (e.g. Is it a fix?  If
so, what is broken without this change?  Is it an enhancement?  If
so, what cannot be done without this change, and how and why is the
new thing the change enables a good thing?), and why it is a good
idea to use "-m" to realize that objective.

> Signed-off-by: Lex Spoon <lex@lexspoon.org>
> ---
> Updated to include a test case
>
>  git-p4.py               | 51 ++++++++++++++++++++++++++++++---------
>  t/t9818-git-p4-block.sh | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 104 insertions(+), 11 deletions(-)
>  create mode 100755 t/t9818-git-p4-block.sh
>
> diff --git a/git-p4.py b/git-p4.py
> index 549022e..2fc8d9c 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -740,17 +740,43 @@ def
> createOrUpdateBranchesFromOrigin(localRefPrefix = "refs/remotes/p4/",
> silent
>  def originP4BranchesExist():
>          return gitBranchExists("origin") or
> gitBranchExists("origin/p4") or gitBranchExists("origin/p4/master")

It appears that the patch is severely linewrapped.

> diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
> new file mode 100755
> index 0000000..73e545d
> --- /dev/null
> +++ b/t/t9818-git-p4-block.sh
> @@ -0,0 +1,64 @@
> +#!/bin/sh
> +
> +test_description='git p4 fetching changes in multiple blocks'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> + start_p4d
> +'

We do not do one-SP indent.  Indent with tab instead.

> +
> +test_expect_success 'Create a repo with 100 changes' '
> + (
> + cd "$cli" &&
> + touch file.txt &&

Do not use "touch" when the only thing you are interested in is that
the file exists and you do not care about its timestamp.  I.e. say

    >file.txt &&

instead.

> + p4 add file.txt &&
> + p4 submit -d "Add file.txt" &&
> + for i in 0 1 2 3 4 5 6 7 8 9
> + do
> + touch outer$i.txt &&
> + p4 add outer$i.txt &&
> + p4 submit -d "Adding outer$i.txt" &&
> + for j in 0 1 2 3 4 5 6 7 8 9
> + do
> + p4 edit file.txt &&
> + echo $i$j > file.txt &&
> + p4 submit -d "Commit $i$j"
> + done
> + done
> + )

What happens when any of these commands in the &&-chain fails?

	(
        	cd "$cli" &&
                >file.txt &&
		p4 ... &&
                for i in $(test_seq ...)
                do
                	>"outer$i.txt" &&
                        p4 ... &&
			for j in $(test_seq ...)
			do
				p4 ... &&
				p4 ... || exit
			done
		done
	)

or something like that, perhaps?

Re: [PATCH] git-p4: Use -m when running p4 changes

[Luke Diamand]

On 15/04/15 04:47, Lex Spoon wrote:
>  From 9cc607667a20317c837afd90d50c078da659b72f Mon Sep 17 00:00:00 2001
> From: Lex Spoon <lex@lexspoon.org>
> Date: Sat, 11 Apr 2015 10:01:15 -0400
> Subject: [PATCH] git-p4: Use -m when running p4 changes

This patch didn't want to apply for me, I'm not quite sure why but 
possibly it's become scrambled? Either that or I'm doing it wrong! If 
you use git send-email it should Just Work.

As an aside could you post reworked versions of patches with a subject 
line of [PATCH v2], [PATCH v3], etc, so reviewers can keep track of 
what's going on?

Note to other reviewers: the existing git-p4 has a --max-changes option 
for 'sync', but this doesn't do the same thing at all. It doesn't limit 
the number of changes requested from the server, it just limits the 
number of changes pulled down, after the p4 server has supplied those 
changes. This confused me at first!

Lex - I should have mentioned this before, but would you be able to add 
some documentation to Documentation/git-p4.txt to explain what your new 
option does? It would help to distinguish between your option and the 
existing --max-changes option.

I've put a few remarks below in your shell script; there are a few minor 
issues that could do with being tidied up.

Thanks!
Luke

<snip>

> diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
> new file mode 100755
> index 0000000..73e545d
> --- /dev/null
> +++ b/t/t9818-git-p4-block.sh
> @@ -0,0 +1,64 @@
> +#!/bin/sh
> +
> +test_description='git p4 fetching changes in multiple blocks'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> + start_p4d
> +'
> +
> +test_expect_success 'Create a repo with 100 changes' '
> + (
> + cd "$cli" &&

This doesn't look like enough indentation. The tests normally get a hard 
tab indent at each level.

> + touch file.txt &&
> + p4 add file.txt &&
> + p4 submit -d "Add file.txt" &&
> + for i in 0 1 2 3 4 5 6 7 8 9
> + do
> + touch outer$i.txt &&
> + p4 add outer$i.txt &&
> + p4 submit -d "Adding outer$i.txt" &&
> + for j in 0 1 2 3 4 5 6 7 8 9
> + do
> + p4 edit file.txt &&
> + echo $i$j > file.txt &&

Please put the file argument immediately after the redirection, i.e.

    echo $i$j >file.txt &&

(Which you've done below in fact).

> + p4 submit -d "Commit $i$j"
> + done
> + done
> + )
> +'
> +
> +test_expect_success 'Clone the repo' '
> + git p4 clone --dest="$git" --changes-block-size=10 --verbose //depot@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 &&
> + ls "$git" >current &&
> + test_cmp expected current
> +'
> +
> +test_expect_success 'file.txt is correct' '
> + echo 99 >expected &&
> + test_cmp expected "$git/file.txt"
> +'
> +
> +test_expect_success 'Correct number of commits' '
> + (cd "$git"; git log --oneline) >log &&

Use "&&" rather than ";"

> + test_line_count = 111 log
> +'
> +
> +test_expect_success 'Previous version of file.txt is correct' '
> + (cd "$git"; git checkout HEAD^^) &&

As above.

> + echo 97 >expected &&
> + test_cmp expected "$git/file.txt"
> +'
> +
> +test_expect_success 'kill p4d' '
> + kill_p4d
> +'
> +
> +test_done
>

Looks good other than that (+Junio's comments).

Thanks!
Luke

Re: [PATCH] git-p4: Use -m when running p4 changes

[Lex Spoon]

Thanks, all. I will update the patch as requested and resend a [PATCH
v3]. This time without the redundant headers. I will also make an
extra effort to make sure that the raw tabs do not get converted to
spaces this time. Oof, I am really out of practice at programming with
raw tabs, much less getting them to make it through email software.
Thank you for your patience.

test_seq is a neat utility. Also, I don't know why I didn't think to
update the document page. Certainly it needs to be updated.


Lex Spoon

[PATCH v3] git-p4: Use -m when running p4 changes

[Lex Spoon]

Simply running "p4 changes" on a large branch can
result in a "too many rows scanned" error from the
Perforce server. It is better to use a sequence
of smaller calls to "p4 changes", using the "-m"
option to limit the size of each call.

Signed-off-by: Lex Spoon <lex@lexspoon.org>
Reviewed-by: Junio C Hamano <gitster@pobox.com>
Reviewed-by: Luke Diamand <luke@diamand.org>
---
Updated as suggested:
- documentation added
- avoided touch(1)
- used test_seq
- used || exit for test commands inside for loops
- more tabs
- fewer line breaks
- expanded commit message

 Documentation/git-p4.txt | 17 ++++++++++---
 git-p4.py                | 54 +++++++++++++++++++++++++++++++---------
 t/t9818-git-p4-block.sh  | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 120 insertions(+), 15 deletions(-)
 create mode 100755 t/t9818-git-p4-block.sh

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index a1664b9..82aa5d6 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -225,9 +225,20 @@ Git repository:
 	they can find the p4 branches in refs/heads.
 
 --max-changes <n>::
-	Limit the number of imported changes to 'n'.  Useful to
-	limit the amount of history when using the '@all' p4 revision
-	specifier.
+	Import at most 'n' changes, rather than the entire range of
+	changes included in the given revision specifier. A typical
+	usage would be use '@all' as the revision specifier, but then
+	to use '--max-changes 1000' to import only the last 1000
+	revisions rather than the entire revision history.
+
+--changes-block-size <n>::
+	The internal block size to use when converting a revision
+	specifier such as '@all' into a list of specific change
+	numbers. Instead of using a single call to 'p4 changes' to
+	find the full list of changes for the conversion, there are a
+	sequence of calls to 'p4 changes -m', each of which requests
+	one block of changes of the given size. The default block size
+	is 500, which should usually be suitable.
 
 --keep-path::
 	The mapping of file names from the p4 depot path to Git, by
diff --git a/git-p4.py b/git-p4.py
index 549022e..1fba3aa 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -740,17 +740,43 @@ 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):
+def p4ChangesForPaths(depotPaths, changeRange, block_size):
     assert depotPaths
-    cmd = ['changes']
-    for p in depotPaths:
-        cmd += ["%s...%s" % (p, changeRange)]
-    output = p4_read_pipe_lines(cmd)
+    assert block_size
+
+    # Parse the change range into start and end
+    if changeRange is None or changeRange == '':
+        changeStart = '@1'
+        changeEnd = '#head'
+    else:
+        parts = changeRange.split(',')
+        assert len(parts) == 2
+        changeStart = parts[0]
+        changeEnd = parts[1]
 
+    # Accumulate change numbers in a dictionary to avoid duplicates
     changes = {}
-    for line in output:
-        changeNum = int(line.split(" ")[1])
-        changes[changeNum] = True
+
+    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)]
+            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
 
     changelist = changes.keys()
     changelist.sort()
@@ -1911,7 +1937,10 @@ class P4Sync(Command, P4UserMap):
                 optparse.make_option("--import-labels", dest="importLabels", action="store_true"),
                 optparse.make_option("--import-local", dest="importIntoRemotes", action="store_false",
                                      help="Import into refs/heads/ , not refs/remotes"),
-                optparse.make_option("--max-changes", dest="maxChanges"),
+                optparse.make_option("--max-changes", dest="maxChanges",
+                                     help="Maximum number of changes to import"),
+                optparse.make_option("--changes-block-size", dest="changes_block_size", type="int",
+                                     help="Internal block size to use when iteratively calling p4 changes"),
                 optparse.make_option("--keep-path", dest="keepRepoPath", action='store_true',
                                      help="Keep entire BRANCH/DIR/SUBDIR prefix during import"),
                 optparse.make_option("--use-client-spec", dest="useClientSpec", action='store_true',
@@ -1940,6 +1969,7 @@ class P4Sync(Command, P4UserMap):
         self.syncWithOrigin = True
         self.importIntoRemotes = True
         self.maxChanges = ""
+        self.changes_block_size = 500
         self.keepRepoPath = False
         self.depotPaths = None
         self.p4BranchesInGit = []
@@ -2578,7 +2608,7 @@ class P4Sync(Command, P4UserMap):
 
         return ""
 
-    def importNewBranch(self, branch, maxChange):
+    def importNewBranch(self, branch, maxChange, changes_block_size):
         # make fast-import flush all changes to disk and update the refs using the checkpoint
         # command so that we can try to find the branch parent in the git history
         self.gitStream.write("checkpoint\n\n");
@@ -2586,7 +2616,7 @@ class P4Sync(Command, P4UserMap):
         branchPrefix = self.depotPaths[0] + branch + "/"
         range = "@1,%s" % maxChange
         #print "prefix" + branchPrefix
-        changes = p4ChangesForPaths([branchPrefix], range)
+        changes = p4ChangesForPaths([branchPrefix], range, changes_block_size)
         if len(changes) <= 0:
             return False
         firstChange = changes[0]
@@ -3002,7 +3032,7 @@ class P4Sync(Command, P4UserMap):
                 if self.verbose:
                     print "Getting p4 changes for %s...%s" % (', '.join(self.depotPaths),
                                                               self.changeRange)
-                changes = p4ChangesForPaths(self.depotPaths, self.changeRange)
+                changes = p4ChangesForPaths(self.depotPaths, self.changeRange, self.changes_block_size)
 
                 if len(self.maxChanges) > 0:
                     changes = changes[:min(int(self.maxChanges), len(changes))]
diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
new file mode 100755
index 0000000..153b20a
--- /dev/null
+++ b/t/t9818-git-p4-block.sh
@@ -0,0 +1,64 @@
+#!/bin/sh
+
+test_description='git p4 fetching changes in multiple blocks'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+test_expect_success 'Create a repo with ~100 changes' '
+	(
+		cd "$cli" &&
+		>file.txt &&
+		p4 add file.txt &&
+		p4 submit -d "Add file.txt" &&
+		for i in $(test_seq 0 9)
+		do
+			>outer$i.txt &&
+			p4 add outer$i.txt &&
+			p4 submit -d "Adding outer$i.txt" &&
+			for j in $(test_seq 0 9)
+			do
+				p4 edit file.txt &&
+				echo $i$j >file.txt &&
+				p4 submit -d "Commit $i$j" || exit
+			done || exit
+		done
+	)
+'
+
+test_expect_success 'Clone the repo' '
+	git p4 clone --dest="$git" --changes-block-size=10 --verbose //depot@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 &&
+	ls "$git" >current &&
+	test_cmp expected current
+'
+
+test_expect_success 'file.txt is correct' '
+	echo 99 >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
+'
+
+test_expect_success 'Previous version of file.txt is correct' '
+	(cd "$git" && git checkout HEAD^^) &&
+	echo 97 >expected &&
+	test_cmp expected "$git/file.txt"
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.9.1

Re: [PATCH v3] git-p4: Use -m when running p4 changes

[Luke Diamand]

On 18/04/15 00:11, Lex Spoon wrote:
> Simply running "p4 changes" on a large branch can
> result in a "too many rows scanned" error from the
> Perforce server. It is better to use a sequence
> of smaller calls to "p4 changes", using the "-m"
> option to limit the size of each call.
>
> Signed-off-by: Lex Spoon <lex@lexspoon.org>
> Reviewed-by: Junio C Hamano <gitster@pobox.com>
> Reviewed-by: Luke Diamand <luke@diamand.org>

I could be wrong about this, but it looks like importNewBranches() is 
taking an extra argument, but that isn't reflected in the place where it 
gets called. I think it just got missed.

As a result, t9801-git-p4-branch.sh fails with this error:

Importing revision 3 (37%)
     Importing new branch depot/branch1
Traceback (most recent call last):
   File "/home/lgd/git/git/git-p4", line 3327, in <module>
     main()
   File "/home/lgd/git/git/git-p4", line 3321, in main
     if not cmd.run(args):
   File "/home/lgd/git/git/git-p4", line 3195, in run
     if not P4Sync.run(self, depotPaths):
   File "/home/lgd/git/git/git-p4", line 3057, in run
     self.importChanges(changes)
   File "/home/lgd/git/git/git-p4", line 2692, in importChanges
     if self.importNewBranch(branch, change - 1):
TypeError: importNewBranch() takes exactly 4 arguments (3 given)
rm: cannot remove `/home/lgd/git/git/t/trash 
directory.t9801-git-p4-branch/git/.git/objects/pack': Directory not empty
not ok 8 - import depot, branch detection, branchList branch definition


Thanks!
Luke


> ---
> Updated as suggested:
> - documentation added
> - avoided touch(1)
> - used test_seq
> - used || exit for test commands inside for loops
> - more tabs
> - fewer line breaks
> - expanded commit message
>
>   Documentation/git-p4.txt | 17 ++++++++++---
>   git-p4.py                | 54 +++++++++++++++++++++++++++++++---------
>   t/t9818-git-p4-block.sh  | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 120 insertions(+), 15 deletions(-)
>   create mode 100755 t/t9818-git-p4-block.sh
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index a1664b9..82aa5d6 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -225,9 +225,20 @@ Git repository:
>   	they can find the p4 branches in refs/heads.
>
>   --max-changes <n>::
> -	Limit the number of imported changes to 'n'.  Useful to
> -	limit the amount of history when using the '@all' p4 revision
> -	specifier.
> +	Import at most 'n' changes, rather than the entire range of
> +	changes included in the given revision specifier. A typical
> +	usage would be use '@all' as the revision specifier, but then
> +	to use '--max-changes 1000' to import only the last 1000
> +	revisions rather than the entire revision history.
> +
> +--changes-block-size <n>::
> +	The internal block size to use when converting a revision
> +	specifier such as '@all' into a list of specific change
> +	numbers. Instead of using a single call to 'p4 changes' to
> +	find the full list of changes for the conversion, there are a
> +	sequence of calls to 'p4 changes -m', each of which requests
> +	one block of changes of the given size. The default block size
> +	is 500, which should usually be suitable.
>
>   --keep-path::
>   	The mapping of file names from the p4 depot path to Git, by
> diff --git a/git-p4.py b/git-p4.py
> index 549022e..1fba3aa 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -740,17 +740,43 @@ 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):
> +def p4ChangesForPaths(depotPaths, changeRange, block_size):
>       assert depotPaths
> -    cmd = ['changes']
> -    for p in depotPaths:
> -        cmd += ["%s...%s" % (p, changeRange)]
> -    output = p4_read_pipe_lines(cmd)
> +    assert block_size
> +
> +    # Parse the change range into start and end
> +    if changeRange is None or changeRange == '':
> +        changeStart = '@1'
> +        changeEnd = '#head'
> +    else:
> +        parts = changeRange.split(',')
> +        assert len(parts) == 2
> +        changeStart = parts[0]
> +        changeEnd = parts[1]
>
> +    # Accumulate change numbers in a dictionary to avoid duplicates
>       changes = {}
> -    for line in output:
> -        changeNum = int(line.split(" ")[1])
> -        changes[changeNum] = True
> +
> +    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)]
> +            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
>
>       changelist = changes.keys()
>       changelist.sort()
> @@ -1911,7 +1937,10 @@ class P4Sync(Command, P4UserMap):
>                   optparse.make_option("--import-labels", dest="importLabels", action="store_true"),
>                   optparse.make_option("--import-local", dest="importIntoRemotes", action="store_false",
>                                        help="Import into refs/heads/ , not refs/remotes"),
> -                optparse.make_option("--max-changes", dest="maxChanges"),
> +                optparse.make_option("--max-changes", dest="maxChanges",
> +                                     help="Maximum number of changes to import"),
> +                optparse.make_option("--changes-block-size", dest="changes_block_size", type="int",
> +                                     help="Internal block size to use when iteratively calling p4 changes"),
>                   optparse.make_option("--keep-path", dest="keepRepoPath", action='store_true',
>                                        help="Keep entire BRANCH/DIR/SUBDIR prefix during import"),
>                   optparse.make_option("--use-client-spec", dest="useClientSpec", action='store_true',
> @@ -1940,6 +1969,7 @@ class P4Sync(Command, P4UserMap):
>           self.syncWithOrigin = True
>           self.importIntoRemotes = True
>           self.maxChanges = ""
> +        self.changes_block_size = 500
>           self.keepRepoPath = False
>           self.depotPaths = None
>           self.p4BranchesInGit = []
> @@ -2578,7 +2608,7 @@ class P4Sync(Command, P4UserMap):
>
>           return ""
>
> -    def importNewBranch(self, branch, maxChange):
> +    def importNewBranch(self, branch, maxChange, changes_block_size):
>           # make fast-import flush all changes to disk and update the refs using the checkpoint
>           # command so that we can try to find the branch parent in the git history
>           self.gitStream.write("checkpoint\n\n");
> @@ -2586,7 +2616,7 @@ class P4Sync(Command, P4UserMap):
>           branchPrefix = self.depotPaths[0] + branch + "/"
>           range = "@1,%s" % maxChange
>           #print "prefix" + branchPrefix
> -        changes = p4ChangesForPaths([branchPrefix], range)
> +        changes = p4ChangesForPaths([branchPrefix], range, changes_block_size)
>           if len(changes) <= 0:
>               return False
>           firstChange = changes[0]
> @@ -3002,7 +3032,7 @@ class P4Sync(Command, P4UserMap):
>                   if self.verbose:
>                       print "Getting p4 changes for %s...%s" % (', '.join(self.depotPaths),
>                                                                 self.changeRange)
> -                changes = p4ChangesForPaths(self.depotPaths, self.changeRange)
> +                changes = p4ChangesForPaths(self.depotPaths, self.changeRange, self.changes_block_size)
>
>                   if len(self.maxChanges) > 0:
>                       changes = changes[:min(int(self.maxChanges), len(changes))]
> diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
> new file mode 100755
> index 0000000..153b20a
> --- /dev/null
> +++ b/t/t9818-git-p4-block.sh
> @@ -0,0 +1,64 @@
> +#!/bin/sh
> +
> +test_description='git p4 fetching changes in multiple blocks'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> +	start_p4d
> +'
> +
> +test_expect_success 'Create a repo with ~100 changes' '
> +	(
> +		cd "$cli" &&
> +		>file.txt &&
> +		p4 add file.txt &&
> +		p4 submit -d "Add file.txt" &&
> +		for i in $(test_seq 0 9)
> +		do
> +			>outer$i.txt &&
> +			p4 add outer$i.txt &&
> +			p4 submit -d "Adding outer$i.txt" &&
> +			for j in $(test_seq 0 9)
> +			do
> +				p4 edit file.txt &&
> +				echo $i$j >file.txt &&
> +				p4 submit -d "Commit $i$j" || exit
> +			done || exit
> +		done
> +	)
> +'
> +
> +test_expect_success 'Clone the repo' '
> +	git p4 clone --dest="$git" --changes-block-size=10 --verbose //depot@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 &&
> +	ls "$git" >current &&
> +	test_cmp expected current
> +'
> +
> +test_expect_success 'file.txt is correct' '
> +	echo 99 >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
> +'
> +
> +test_expect_success 'Previous version of file.txt is correct' '
> +	(cd "$git" && git checkout HEAD^^) &&
> +	echo 97 >expected &&
> +	test_cmp expected "$git/file.txt"
> +'
> +
> +test_expect_success 'kill p4d' '
> +	kill_p4d
> +'
> +
> +test_done
>

Re: [PATCH v3] git-p4: Use -m when running p4 changes

[Lex Spoon]

On Mon, Apr 20, 2015 at 5:53 AM, Luke Diamand <luke@diamand.org> wrote:
> I could be wrong about this, but it looks like importNewBranches() is taking
> an extra argument, but that isn't reflected in the place where it gets
> called. I think it just got missed.
>
> As a result, t9801-git-p4-branch.sh fails with this error:

Oh dear, definitely. The argument can in fact be dropped, because it's
already already available via a field of the same object. I post an
update with that change.  -Lex

[PATCH v4] git-p4: Use -m when running p4 changes

[Lex Spoon]

Simply running "p4 changes" on a large branch can
result in a "too many rows scanned" error from the
Perforce server. It is better to use a sequence
of smaller calls to "p4 changes", using the "-m"
option to limit the size of each call.

Signed-off-by: Lex Spoon <lex@lexspoon.org>
Reviewed-by: Junio C Hamano <gitster@pobox.com>
Reviewed-by: Luke Diamand <luke@diamand.org>
---
Updated to avoid the crash Luke pointed out.
All t98* tests pass now except for t9814,
which is already failing on master for some reason.

 Documentation/git-p4.txt | 17 ++++++++++---
 git-p4.py                | 52 ++++++++++++++++++++++++++++++---------
 t/t9818-git-p4-block.sh  | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 119 insertions(+), 14 deletions(-)
 create mode 100755 t/t9818-git-p4-block.sh

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index a1664b9..82aa5d6 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -225,9 +225,20 @@ Git repository:
 	they can find the p4 branches in refs/heads.
 
 --max-changes <n>::
-	Limit the number of imported changes to 'n'.  Useful to
-	limit the amount of history when using the '@all' p4 revision
-	specifier.
+	Import at most 'n' changes, rather than the entire range of
+	changes included in the given revision specifier. A typical
+	usage would be use '@all' as the revision specifier, but then
+	to use '--max-changes 1000' to import only the last 1000
+	revisions rather than the entire revision history.
+
+--changes-block-size <n>::
+	The internal block size to use when converting a revision
+	specifier such as '@all' into a list of specific change
+	numbers. Instead of using a single call to 'p4 changes' to
+	find the full list of changes for the conversion, there are a
+	sequence of calls to 'p4 changes -m', each of which requests
+	one block of changes of the given size. The default block size
+	is 500, which should usually be suitable.
 
 --keep-path::
 	The mapping of file names from the p4 depot path to Git, by
diff --git a/git-p4.py b/git-p4.py
index 549022e..e28033f 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -740,17 +740,43 @@ 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):
+def p4ChangesForPaths(depotPaths, changeRange, block_size):
     assert depotPaths
-    cmd = ['changes']
-    for p in depotPaths:
-        cmd += ["%s...%s" % (p, changeRange)]
-    output = p4_read_pipe_lines(cmd)
+    assert block_size
+
+    # Parse the change range into start and end
+    if changeRange is None or changeRange == '':
+        changeStart = '@1'
+        changeEnd = '#head'
+    else:
+        parts = changeRange.split(',')
+        assert len(parts) == 2
+        changeStart = parts[0]
+        changeEnd = parts[1]
 
+    # Accumulate change numbers in a dictionary to avoid duplicates
     changes = {}
-    for line in output:
-        changeNum = int(line.split(" ")[1])
-        changes[changeNum] = True
+
+    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)]
+            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
 
     changelist = changes.keys()
     changelist.sort()
@@ -1911,7 +1937,10 @@ class P4Sync(Command, P4UserMap):
                 optparse.make_option("--import-labels", dest="importLabels", action="store_true"),
                 optparse.make_option("--import-local", dest="importIntoRemotes", action="store_false",
                                      help="Import into refs/heads/ , not refs/remotes"),
-                optparse.make_option("--max-changes", dest="maxChanges"),
+                optparse.make_option("--max-changes", dest="maxChanges",
+                                     help="Maximum number of changes to import"),
+                optparse.make_option("--changes-block-size", dest="changes_block_size", type="int",
+                                     help="Internal block size to use when iteratively calling p4 changes"),
                 optparse.make_option("--keep-path", dest="keepRepoPath", action='store_true',
                                      help="Keep entire BRANCH/DIR/SUBDIR prefix during import"),
                 optparse.make_option("--use-client-spec", dest="useClientSpec", action='store_true',
@@ -1940,6 +1969,7 @@ class P4Sync(Command, P4UserMap):
         self.syncWithOrigin = True
         self.importIntoRemotes = True
         self.maxChanges = ""
+        self.changes_block_size = 500
         self.keepRepoPath = False
         self.depotPaths = None
         self.p4BranchesInGit = []
@@ -2586,7 +2616,7 @@ class P4Sync(Command, P4UserMap):
         branchPrefix = self.depotPaths[0] + branch + "/"
         range = "@1,%s" % maxChange
         #print "prefix" + branchPrefix
-        changes = p4ChangesForPaths([branchPrefix], range)
+        changes = p4ChangesForPaths([branchPrefix], range, self.changes_block_size)
         if len(changes) <= 0:
             return False
         firstChange = changes[0]
@@ -3002,7 +3032,7 @@ class P4Sync(Command, P4UserMap):
                 if self.verbose:
                     print "Getting p4 changes for %s...%s" % (', '.join(self.depotPaths),
                                                               self.changeRange)
-                changes = p4ChangesForPaths(self.depotPaths, self.changeRange)
+                changes = p4ChangesForPaths(self.depotPaths, self.changeRange, self.changes_block_size)
 
                 if len(self.maxChanges) > 0:
                     changes = changes[:min(int(self.maxChanges), len(changes))]
diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
new file mode 100755
index 0000000..153b20a
--- /dev/null
+++ b/t/t9818-git-p4-block.sh
@@ -0,0 +1,64 @@
+#!/bin/sh
+
+test_description='git p4 fetching changes in multiple blocks'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+test_expect_success 'Create a repo with ~100 changes' '
+	(
+		cd "$cli" &&
+		>file.txt &&
+		p4 add file.txt &&
+		p4 submit -d "Add file.txt" &&
+		for i in $(test_seq 0 9)
+		do
+			>outer$i.txt &&
+			p4 add outer$i.txt &&
+			p4 submit -d "Adding outer$i.txt" &&
+			for j in $(test_seq 0 9)
+			do
+				p4 edit file.txt &&
+				echo $i$j >file.txt &&
+				p4 submit -d "Commit $i$j" || exit
+			done || exit
+		done
+	)
+'
+
+test_expect_success 'Clone the repo' '
+	git p4 clone --dest="$git" --changes-block-size=10 --verbose //depot@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 &&
+	ls "$git" >current &&
+	test_cmp expected current
+'
+
+test_expect_success 'file.txt is correct' '
+	echo 99 >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
+'
+
+test_expect_success 'Previous version of file.txt is correct' '
+	(cd "$git" && git checkout HEAD^^) &&
+	echo 97 >expected &&
+	test_cmp expected "$git/file.txt"
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.9.1

Re: [PATCH v4] git-p4: Use -m when running p4 changes

[Luke Diamand]

Sorry - could you resubmit your patch (PATCHv4 it will be) with this
change squashed in please? It will make life much easier, especially
for Junio!

Thanks!
Luke


On 20 April 2015 at 16:00, Lex Spoon <lex@lexspoon.org> wrote:
> Simply running "p4 changes" on a large branch can
> result in a "too many rows scanned" error from the
> Perforce server. It is better to use a sequence
> of smaller calls to "p4 changes", using the "-m"
> option to limit the size of each call.
>
> Signed-off-by: Lex Spoon <lex@lexspoon.org>
> Reviewed-by: Junio C Hamano <gitster@pobox.com>
> Reviewed-by: Luke Diamand <luke@diamand.org>
> ---
> Updated to avoid the crash Luke pointed out.
> All t98* tests pass now except for t9814,
> which is already failing on master for some reason.
>
>  Documentation/git-p4.txt | 17 ++++++++++---
>  git-p4.py                | 52 ++++++++++++++++++++++++++++++---------
>  t/t9818-git-p4-block.sh  | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 119 insertions(+), 14 deletions(-)
>  create mode 100755 t/t9818-git-p4-block.sh
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index a1664b9..82aa5d6 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -225,9 +225,20 @@ Git repository:
>         they can find the p4 branches in refs/heads.
>
>  --max-changes <n>::
> -       Limit the number of imported changes to 'n'.  Useful to
> -       limit the amount of history when using the '@all' p4 revision
> -       specifier.
> +       Import at most 'n' changes, rather than the entire range of
> +       changes included in the given revision specifier. A typical
> +       usage would be use '@all' as the revision specifier, but then
> +       to use '--max-changes 1000' to import only the last 1000
> +       revisions rather than the entire revision history.
> +
> +--changes-block-size <n>::
> +       The internal block size to use when converting a revision
> +       specifier such as '@all' into a list of specific change
> +       numbers. Instead of using a single call to 'p4 changes' to
> +       find the full list of changes for the conversion, there are a
> +       sequence of calls to 'p4 changes -m', each of which requests
> +       one block of changes of the given size. The default block size
> +       is 500, which should usually be suitable.
>
>  --keep-path::
>         The mapping of file names from the p4 depot path to Git, by
> diff --git a/git-p4.py b/git-p4.py
> index 549022e..e28033f 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -740,17 +740,43 @@ 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):
> +def p4ChangesForPaths(depotPaths, changeRange, block_size):
>      assert depotPaths
> -    cmd = ['changes']
> -    for p in depotPaths:
> -        cmd += ["%s...%s" % (p, changeRange)]
> -    output = p4_read_pipe_lines(cmd)
> +    assert block_size
> +
> +    # Parse the change range into start and end
> +    if changeRange is None or changeRange == '':
> +        changeStart = '@1'
> +        changeEnd = '#head'
> +    else:
> +        parts = changeRange.split(',')
> +        assert len(parts) == 2
> +        changeStart = parts[0]
> +        changeEnd = parts[1]
>
> +    # Accumulate change numbers in a dictionary to avoid duplicates
>      changes = {}
> -    for line in output:
> -        changeNum = int(line.split(" ")[1])
> -        changes[changeNum] = True
> +
> +    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)]
> +            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
>
>      changelist = changes.keys()
>      changelist.sort()
> @@ -1911,7 +1937,10 @@ class P4Sync(Command, P4UserMap):
>                  optparse.make_option("--import-labels", dest="importLabels", action="store_true"),
>                  optparse.make_option("--import-local", dest="importIntoRemotes", action="store_false",
>                                       help="Import into refs/heads/ , not refs/remotes"),
> -                optparse.make_option("--max-changes", dest="maxChanges"),
> +                optparse.make_option("--max-changes", dest="maxChanges",
> +                                     help="Maximum number of changes to import"),
> +                optparse.make_option("--changes-block-size", dest="changes_block_size", type="int",
> +                                     help="Internal block size to use when iteratively calling p4 changes"),
>                  optparse.make_option("--keep-path", dest="keepRepoPath", action='store_true',
>                                       help="Keep entire BRANCH/DIR/SUBDIR prefix during import"),
>                  optparse.make_option("--use-client-spec", dest="useClientSpec", action='store_true',
> @@ -1940,6 +1969,7 @@ class P4Sync(Command, P4UserMap):
>          self.syncWithOrigin = True
>          self.importIntoRemotes = True
>          self.maxChanges = ""
> +        self.changes_block_size = 500
>          self.keepRepoPath = False
>          self.depotPaths = None
>          self.p4BranchesInGit = []
> @@ -2586,7 +2616,7 @@ class P4Sync(Command, P4UserMap):
>          branchPrefix = self.depotPaths[0] + branch + "/"
>          range = "@1,%s" % maxChange
>          #print "prefix" + branchPrefix
> -        changes = p4ChangesForPaths([branchPrefix], range)
> +        changes = p4ChangesForPaths([branchPrefix], range, self.changes_block_size)
>          if len(changes) <= 0:
>              return False
>          firstChange = changes[0]
> @@ -3002,7 +3032,7 @@ class P4Sync(Command, P4UserMap):
>                  if self.verbose:
>                      print "Getting p4 changes for %s...%s" % (', '.join(self.depotPaths),
>                                                                self.changeRange)
> -                changes = p4ChangesForPaths(self.depotPaths, self.changeRange)
> +                changes = p4ChangesForPaths(self.depotPaths, self.changeRange, self.changes_block_size)
>
>                  if len(self.maxChanges) > 0:
>                      changes = changes[:min(int(self.maxChanges), len(changes))]
> diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
> new file mode 100755
> index 0000000..153b20a
> --- /dev/null
> +++ b/t/t9818-git-p4-block.sh
> @@ -0,0 +1,64 @@
> +#!/bin/sh
> +
> +test_description='git p4 fetching changes in multiple blocks'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> +       start_p4d
> +'
> +
> +test_expect_success 'Create a repo with ~100 changes' '
> +       (
> +               cd "$cli" &&
> +               >file.txt &&
> +               p4 add file.txt &&
> +               p4 submit -d "Add file.txt" &&
> +               for i in $(test_seq 0 9)
> +               do
> +                       >outer$i.txt &&
> +                       p4 add outer$i.txt &&
> +                       p4 submit -d "Adding outer$i.txt" &&
> +                       for j in $(test_seq 0 9)
> +                       do
> +                               p4 edit file.txt &&
> +                               echo $i$j >file.txt &&
> +                               p4 submit -d "Commit $i$j" || exit
> +                       done || exit
> +               done
> +       )
> +'
> +
> +test_expect_success 'Clone the repo' '
> +       git p4 clone --dest="$git" --changes-block-size=10 --verbose //depot@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 &&
> +       ls "$git" >current &&
> +       test_cmp expected current
> +'
> +
> +test_expect_success 'file.txt is correct' '
> +       echo 99 >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
> +'
> +
> +test_expect_success 'Previous version of file.txt is correct' '
> +       (cd "$git" && git checkout HEAD^^) &&
> +       echo 97 >expected &&
> +       test_cmp expected "$git/file.txt"
> +'
> +
> +test_expect_success 'kill p4d' '
> +       kill_p4d
> +'
> +
> +test_done
> --
> 1.9.1
>

Re: [PATCH v4] git-p4: Use -m when running p4 changes

[Lex Spoon]

On Mon, Apr 20, 2015 at 11:15 AM, Luke Diamand <luke@diamand.org> wrote:
> Sorry - could you resubmit your patch (PATCHv4 it will be) with this
> change squashed in please? It will make life much easier, especially
> for Junio!

The message you just responded is already the squashed version. It's a
single patch that includes all changes so far discussed. The subject
line says "PATCH v4", although since it's in the same thread, not all
email clients will show the subject change.

Let me know if I can do more to make the process go smoothly.

Lex Spoon

Re: [PATCH v4] git-p4: Use -m when running p4 changes

[Luke Diamand]

On 20/04/15 16:25, Lex Spoon wrote:
> On Mon, Apr 20, 2015 at 11:15 AM, Luke Diamand <luke@diamand.org> wrote:
>> Sorry - could you resubmit your patch (PATCHv4 it will be) with this
>> change squashed in please? It will make life much easier, especially
>> for Junio!
>
> The message you just responded is already the squashed version. It's a
> single patch that includes all changes so far discussed. The subject
> line says "PATCH v4", although since it's in the same thread, not all
> email clients will show the subject change.

Not sure how I missed that! It looks good, now, Ack!

Thanks!
Luke

Re: [PATCH v4] git-p4: Use -m when running p4 changes

[Junio C Hamano]

Luke Diamand <luke@diamand.org> writes:

> Sorry - could you resubmit your patch (PATCHv4 it will be) with this
> change squashed in please? It will make life much easier, especially
> for Junio!

Thanks for caring, but this seems to be a full patch to replace v3.

It was sent with your Reviewed-by already in, but I'd tentatively
remove that line while queuing it to 'pu' and ask you to double
check if the patch makes sense (and after your "yes, it does", I'd
add the Reviewed-by back).

Thanks.

>
> Thanks!
> Luke
>
>
> On 20 April 2015 at 16:00, Lex Spoon <lex@lexspoon.org> wrote:
>> Simply running "p4 changes" on a large branch can
>> result in a "too many rows scanned" error from the
>> Perforce server. It is better to use a sequence
>> of smaller calls to "p4 changes", using the "-m"
>> option to limit the size of each call.
>>
>> Signed-off-by: Lex Spoon <lex@lexspoon.org>
>> Reviewed-by: Junio C Hamano <gitster@pobox.com>
>> Reviewed-by: Luke Diamand <luke@diamand.org>
>> ---
>> Updated to avoid the crash Luke pointed out.
>> All t98* tests pass now except for t9814,
>> which is already failing on master for some reason.
>>
>>  Documentation/git-p4.txt | 17 ++++++++++---
>>  git-p4.py                | 52 ++++++++++++++++++++++++++++++---------
>>  t/t9818-git-p4-block.sh  | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 119 insertions(+), 14 deletions(-)
>>  create mode 100755 t/t9818-git-p4-block.sh
>>
>> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
>> index a1664b9..82aa5d6 100644
>> --- a/Documentation/git-p4.txt
>> +++ b/Documentation/git-p4.txt
>> @@ -225,9 +225,20 @@ Git repository:
>>         they can find the p4 branches in refs/heads.
>>
>>  --max-changes <n>::
>> -       Limit the number of imported changes to 'n'.  Useful to
>> -       limit the amount of history when using the '@all' p4 revision
>> -       specifier.
>> +       Import at most 'n' changes, rather than the entire range of
>> +       changes included in the given revision specifier. A typical
>> +       usage would be use '@all' as the revision specifier, but then
>> +       to use '--max-changes 1000' to import only the last 1000
>> +       revisions rather than the entire revision history.
>> +
>> +--changes-block-size <n>::
>> +       The internal block size to use when converting a revision
>> +       specifier such as '@all' into a list of specific change
>> +       numbers. Instead of using a single call to 'p4 changes' to
>> +       find the full list of changes for the conversion, there are a
>> +       sequence of calls to 'p4 changes -m', each of which requests
>> +       one block of changes of the given size. The default block size
>> +       is 500, which should usually be suitable.
>>
>>  --keep-path::
>>         The mapping of file names from the p4 depot path to Git, by
>> diff --git a/git-p4.py b/git-p4.py
>> index 549022e..e28033f 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -740,17 +740,43 @@ 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):
>> +def p4ChangesForPaths(depotPaths, changeRange, block_size):
>>      assert depotPaths
>> -    cmd = ['changes']
>> -    for p in depotPaths:
>> -        cmd += ["%s...%s" % (p, changeRange)]
>> -    output = p4_read_pipe_lines(cmd)
>> +    assert block_size
>> +
>> +    # Parse the change range into start and end
>> +    if changeRange is None or changeRange == '':
>> +        changeStart = '@1'
>> +        changeEnd = '#head'
>> +    else:
>> +        parts = changeRange.split(',')
>> +        assert len(parts) == 2
>> +        changeStart = parts[0]
>> +        changeEnd = parts[1]
>>
>> +    # Accumulate change numbers in a dictionary to avoid duplicates
>>      changes = {}
>> -    for line in output:
>> -        changeNum = int(line.split(" ")[1])
>> -        changes[changeNum] = True
>> +
>> +    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)]
>> +            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
>>
>>      changelist = changes.keys()
>>      changelist.sort()
>> @@ -1911,7 +1937,10 @@ class P4Sync(Command, P4UserMap):
>>                  optparse.make_option("--import-labels", dest="importLabels", action="store_true"),
>>                  optparse.make_option("--import-local", dest="importIntoRemotes", action="store_false",
>>                                       help="Import into refs/heads/ , not refs/remotes"),
>> -                optparse.make_option("--max-changes", dest="maxChanges"),
>> +                optparse.make_option("--max-changes", dest="maxChanges",
>> +                                     help="Maximum number of changes to import"),
>> +                optparse.make_option("--changes-block-size", dest="changes_block_size", type="int",
>> +                                     help="Internal block size to use when iteratively calling p4 changes"),
>>                  optparse.make_option("--keep-path", dest="keepRepoPath", action='store_true',
>>                                       help="Keep entire BRANCH/DIR/SUBDIR prefix during import"),
>>                  optparse.make_option("--use-client-spec", dest="useClientSpec", action='store_true',
>> @@ -1940,6 +1969,7 @@ class P4Sync(Command, P4UserMap):
>>          self.syncWithOrigin = True
>>          self.importIntoRemotes = True
>>          self.maxChanges = ""
>> +        self.changes_block_size = 500
>>          self.keepRepoPath = False
>>          self.depotPaths = None
>>          self.p4BranchesInGit = []
>> @@ -2586,7 +2616,7 @@ class P4Sync(Command, P4UserMap):
>>          branchPrefix = self.depotPaths[0] + branch + "/"
>>          range = "@1,%s" % maxChange
>>          #print "prefix" + branchPrefix
>> -        changes = p4ChangesForPaths([branchPrefix], range)
>> +        changes = p4ChangesForPaths([branchPrefix], range, self.changes_block_size)
>>          if len(changes) <= 0:
>>              return False
>>          firstChange = changes[0]
>> @@ -3002,7 +3032,7 @@ class P4Sync(Command, P4UserMap):
>>                  if self.verbose:
>>                      print "Getting p4 changes for %s...%s" % (', '.join(self.depotPaths),
>>                                                                self.changeRange)
>> -                changes = p4ChangesForPaths(self.depotPaths, self.changeRange)
>> +                changes = p4ChangesForPaths(self.depotPaths, self.changeRange, self.changes_block_size)
>>
>>                  if len(self.maxChanges) > 0:
>>                      changes = changes[:min(int(self.maxChanges), len(changes))]
>> diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
>> new file mode 100755
>> index 0000000..153b20a
>> --- /dev/null
>> +++ b/t/t9818-git-p4-block.sh
>> @@ -0,0 +1,64 @@
>> +#!/bin/sh
>> +
>> +test_description='git p4 fetching changes in multiple blocks'
>> +
>> +. ./lib-git-p4.sh
>> +
>> +test_expect_success 'start p4d' '
>> +       start_p4d
>> +'
>> +
>> +test_expect_success 'Create a repo with ~100 changes' '
>> +       (
>> +               cd "$cli" &&
>> +               >file.txt &&
>> +               p4 add file.txt &&
>> +               p4 submit -d "Add file.txt" &&
>> +               for i in $(test_seq 0 9)
>> +               do
>> +                       >outer$i.txt &&
>> +                       p4 add outer$i.txt &&
>> +                       p4 submit -d "Adding outer$i.txt" &&
>> +                       for j in $(test_seq 0 9)
>> +                       do
>> +                               p4 edit file.txt &&
>> +                               echo $i$j >file.txt &&
>> +                               p4 submit -d "Commit $i$j" || exit
>> +                       done || exit
>> +               done
>> +       )
>> +'
>> +
>> +test_expect_success 'Clone the repo' '
>> +       git p4 clone --dest="$git" --changes-block-size=10 --verbose //depot@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 &&
>> +       ls "$git" >current &&
>> +       test_cmp expected current
>> +'
>> +
>> +test_expect_success 'file.txt is correct' '
>> +       echo 99 >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
>> +'
>> +
>> +test_expect_success 'Previous version of file.txt is correct' '
>> +       (cd "$git" && git checkout HEAD^^) &&
>> +       echo 97 >expected &&
>> +       test_cmp expected "$git/file.txt"
>> +'
>> +
>> +test_expect_success 'kill p4d' '
>> +       kill_p4d
>> +'
>> +
>> +test_done
>> --
>> 1.9.1
>>

Re: [PATCH v4] git-p4: Use -m when running p4 changes

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Luke Diamand <luke@diamand.org> writes:
>
>> Sorry - could you resubmit your patch (PATCHv4 it will be) with this
>> change squashed in please? It will make life much easier, especially
>> for Junio!
>
> Thanks for caring, but this seems to be a full patch to replace v3.
>
> It was sent with your Reviewed-by already in, but I'd tentatively
> remove that line while queuing it to 'pu' and ask you to double
> check if the patch makes sense (and after your "yes, it does", I'd
> add the Reviewed-by back).
>
> Thanks.

Just to make it easier to see, the interdiff between v3 and v4 looks
like this:

 git-p4.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 1fba3aa..e28033f 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2608,7 +2608,7 @@ class P4Sync(Command, P4UserMap):
 
         return ""
 
-    def importNewBranch(self, branch, maxChange, changes_block_size):
+    def importNewBranch(self, branch, maxChange):
         # make fast-import flush all changes to disk and update the refs using the checkpoint
         # command so that we can try to find the branch parent in the git history
         self.gitStream.write("checkpoint\n\n");
@@ -2616,7 +2616,7 @@ class P4Sync(Command, P4UserMap):
         branchPrefix = self.depotPaths[0] + branch + "/"
         range = "@1,%s" % maxChange
         #print "prefix" + branchPrefix
-        changes = p4ChangesForPaths([branchPrefix], range, changes_block_size)
+        changes = p4ChangesForPaths([branchPrefix], range, self.changes_block_size)
         if len(changes) <= 0:
             return False
         firstChange = changes[0]