[PATCH 2 of 3 v2] bundle2: part handler for processing .hgtags fnodes mappings

[Pierre-Yves David]

On 06/01/2015 08:56 PM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc at gmail.com>
> # Date 1433215402 25200
> #      Mon Jun 01 20:23:22 2015 -0700
> # Node ID 3ed19563760abf1babe65f37e6b4ec6c4422ac2b
> # Parent  2c67a5830d8dfcb3c76d9a171abe18c19caf05db
> bundle2: part handler for processing .hgtags fnodes mappings
>
> .hgtags fnodes cache entries can be expensive to compute, especially
> if there are hundreds of even thousands of them. This patch implements
> support for receiving a bundle2 part that contains a mapping of
> changeset to .hgtags fnodes.
>
> An upcoming patch will teach the server to send this part, allowing
> clients to bypass having to redundantly compute these values.
>
> A number of tests changed due to the client advertising the "hgtagsfnodes"
> capability.

I've a couple of feedback below, but we are getting there.

> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -155,9 +155,9 @@ import obsolete
>   import pushkey
>   import url
>   import re
>
> -import changegroup, error
> +import changegroup, error, tags
>   from i18n import _
>
>   _pack = struct.pack
>   _unpack = struct.unpack
> @@ -1113,8 +1113,9 @@ capabilities = {'HG20': (),
>                   'listkeys': (),
>                   'pushkey': (),
>                   'digests': tuple(sorted(util.DIGESTS.keys())),
>                   'remote-changegroup': ('http', 'https'),
> +                'hgtagsfnodes': (),
>                  }
>
>   def getrepocaps(repo, allowpushback=False):
>       """return the bundle2 capabilities for a given repo
> @@ -1363,4 +1364,23 @@ def handlepushkeyreply(op, inpart):
>       """retrieve the result of a pushkey request"""
>       ret = int(inpart.params['new'])
>       partid = int(inpart.params['in-reply-to'])
>       op.records.add('obsmarkers', {'new': ret}, partid)
> +
> + at parthandler('hgtagsfnodes')
> +def handlehgtagsfnodes(op, inpart):

That function want's a docstring.

> +    cache = tags.hgtagsfnodescache(op.repo.unfiltered())
> +
> +    # Raw data is pairs of 20 byte changeset nodes and filenodes.
> +    count = 0
> +    while True:
> +        node = inpart.read(20)
> +        if len(node) < 20:
> +            break
> +        fnode = inpart.read(20)
> +        if len(fnode) < 20:
> +            break

I'm shivering at error passing silently. I think we should probably 
issue a warning (or even abort) or at least issue a debug message (but I 
would be in favor of something stronger.)

> +        cache.setfnode(node, fnode)
> +        count += 1
> +
> +    cache.write()
> +    op.repo.ui.debug('applied %i hgtags fnodes cache entries\n' % count)


-- 
Pierre-Yves David