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

2015-06-08 Thread Junio C Hamano
On Mon, Jun 8, 2015 at 11:43 AM, Luke Diamand l...@diamand.org wrote:
 On 08/06/15 18:18, Junio C Hamano wrote:

 Lex Spoon l...@lexspoon.org writes:

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


 Thanks, will do.


 I don't think we're quite there yet unfortunately.

 The current version of git-p4 will let you do things like:

 $ git p4 clone //depot@1,2015/05/31

 i.e. get all the revisions between revision 1 and the end of last month.

 Because my change tries to batch up the revisions, it fails when presented
 with this.

 There aren't any test cases for this, but it's documented (briefly) in the
 manual page.

 I think that although the current code looks really nice and clean, it's
 going to have to pick up a bit more complexity to handle non-numerical
 revisions. I don't think it's possible to do batching at the same time.

 It shouldn't be too hard though; I'll look at it later this week.

[jch: adding git@ back]

Thanks.
--
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: [PATCHv2 3/3] git-p4: fixing --changes-block-size handling

2015-06-08 Thread Junio C Hamano
Lex Spoon l...@lexspoon.org writes:

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

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



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

--
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: [PATCHv2 3/3] git-p4: fixing --changes-block-size handling

2015-06-08 Thread Lex Spoon
Precisely, Junio, that's what I had in mind. The patch with the two
lines deleted LGTM.
--
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


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

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

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

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

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

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

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

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

2015-06-07 Thread Lex Spoon
Unless I am reading something wrong, the new_changes variable could
be dropped now. It was needed for the -m version for detecting the
smallest change number that was returned. Otherwise it looks good to
me.
--
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