From 755113b90374e76aba321ff6c6f3f9969ebe2011 Mon Sep 17 00:00:00 2001 From: Luke Diamand Date: Sun, 7 Jun 2015 22:35:03 +0100 Subject: git-p4: additional testing of --changes-block-size 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 Acked-by: Lex Spoon Signed-off-by: Junio C Hamano --- 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 153b20afbd..79765a4a4e 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 ' -- cgit v1.2.1 From 6ba9e2c1ba91ac34d1fb5d0fe659eb6611ddc903 Mon Sep 17 00:00:00 2001 From: Luke Diamand Date: Sun, 7 Jun 2015 22:35:04 +0100 Subject: git-p4: test with limited p4 server results 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 Acked-by: Lex Spoon Signed-off-by: Junio C Hamano --- 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 79765a4a4e..aae1121b93 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 && -- cgit v1.2.1 From eceafffbec97385eb65d7ba0c365ee028aaf1aad Mon Sep 17 00:00:00 2001 From: Luke Diamand Date: Wed, 10 Jun 2015 08:30:58 +0100 Subject: git-p4: add tests for non-numeric revision range Test that git-p4 can handle a sync with a non-numeric revision range (e.g. a date). Signed-off-by: Luke Diamand Signed-off-by: Junio C Hamano --- 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 5b562122a1..90d41ed954 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" && -- cgit v1.2.1 From 1051ef00636357061d72bcf673da86054fb14a12 Mon Sep 17 00:00:00 2001 From: Luke Diamand Date: Wed, 10 Jun 2015 08:30:59 +0100 Subject: git-p4: fixing --changes-block-size handling 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 Signed-off-by: Junio C Hamano --- 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 ca6bb95c57..73f05f0a88 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 aae1121b93..3b3ae1f59a 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 && -- cgit v1.2.1