Re: [PATCH] git-p4: Use -m when running p4 changes
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-p4: Use -m when running p4 changes
On 15/04/15 04:47, Lex Spoon wrote: From 9cc607667a20317c837afd90d50c078da659b72f Mon Sep 17 00:00:00 2001 From: Lex Spoon 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 diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh new file mode 100755 index 000..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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-p4: Use -m when running p4 changes
Lex Spoon writes:
> From 9cc607667a20317c837afd90d50c078da659b72f Mon Sep 17 00:00:00 2001
> From: Lex Spoon
> 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
> ---
> 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 000..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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-p4: Use -m when running p4 changes
>From 9cc607667a20317c837afd90d50c078da659b72f Mon Sep 17 00:00:00 2001
From: Lex Spoon
Date: Sat, 11 Apr 2015 10:01:15 -0400
Subject: [PATCH] git-p4: Use -m when running p4 changes
Signed-off-by: Lex Spoon
---
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
Re: [PATCH] git-p4: Use -m when running p4 changes
(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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-p4: Use -m when running p4 changes
On 11 April 2015 at 16:17, Lex Spoon wrote:
>
>
> Signed-off-by: Lex Spoon
> ---
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
