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  wrote:
> On 08/06/15 18:18, Junio C Hamano wrote:
>>
>> Lex Spoon  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 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


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

2015-06-08 Thread Junio C Hamano
Lex Spoon  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-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


[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 
---
 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/f