[PATCH 2 of 3 V2] bundle2: add bookmarks part handler

[Pierre-Yves David]

On 08/23/2016 12:22 PM, Stanislau Hlebik wrote:
> # HG changeset patch
> # User Stanislau Hlebik <stash at fb.com>
> # Date 1471881013 25200
> #      Mon Aug 22 08:50:13 2016 -0700
> # Node ID 6cad044c2ea002d3a4c1e45666e89c99b7fbdd0f
> # Parent  a7c8796d3636837bc90c2bc3712a0da5e44ebe75
> bundle2: add bookmarks part handler

This changeset actually do a bit more:

1) add a part handler
2) add a part generator on the getbundle side

But (3) "using the new argument to fetch bookmark is missing.

splitting (1), (2) and (3) in different changesets is probably sensible, 
but grouping (1) and (2) without (3) is strange.

In addition (3) seems to be missing from this series.

> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -1249,6 +1249,7 @@
>                  'digests': tuple(sorted(util.DIGESTS.keys())),
>                  'remote-changegroup': ('http', 'https'),
>                  'hgtagsfnodes': (),
> +                'bookmarks': (),
>                 }
>
>  def getrepocaps(repo, allowpushback=False):
> @@ -1609,3 +1610,7 @@
>
>      cache.write()
>      op.ui.debug('applied %i hgtags fnodes cache entries\n' % count)
> +
> + at parthandler('bookmarks')
> +def handlebookmarks(op, inpart):
> +    op.records.add('bookmarks', pushkey.decodekeys(inpart.read()))

Why are we calling puskey here? One of the point of having a bookmark 
dedicated part is to avoid having to feedle with the pushkey protocol 
and encoding here.

> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -1650,6 +1650,17 @@
>      if chunks:
>          bundler.newpart('hgtagsfnodes', data=''.join(chunks))
>
> + at getbundle2partsgenerator('bookmarks')
> +def _getbundlebookmarkspart(bundler, repo, source, bundlecaps=None,
> +                            b2caps=None, heads=None, common=None,
> +                            **kwargs):
> +    if not (kwargs.get('bookmarks') and 'bookmarks' in b2caps):
> +        return

We might consider complaining if bookmarks are requested but we don't 
recognise a way to transmit them in b2caps.

> +    bookmarks = repo.listkeys(namespace='bookmarks')
> +    encodedbookmarks = pushkey.encodekeys(bookmarks.items())
> +    bundler.newpart('bookmarks', data=encodedbookmarks)

Same feedback about not using pushkey here.

> +
>  def check_heads(repo, their_heads, context):
>      """check if the heads of a repo have been modified
>
> diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
> --- a/mercurial/wireproto.py
> +++ b/mercurial/wireproto.py
> @@ -204,7 +204,8 @@
>               'bundlecaps': 'scsv',
>               'listkeys': 'csv',
>               'cg': 'boolean',
> -             'cbattempted': 'boolean'}
> +             'cbattempted': 'boolean',
> +             'bookmarks': 'boolean'}
>
>  # client side

-- 
Pierre-Yves David