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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11 April 2015 at 16:17, Lex Spoon <lex@xxxxxxxxxxxx> wrote:
>
>
> Signed-off-by: Lex Spoon <lex@xxxxxxxxxxxx>
> ---
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]