Patchwork [07,of,10,V10] exchange: getbundle `bookmarks` part generator

# HG changeset patch
# User Stanislau Hlebik <stash@fb.com>
# Date 1479373181 28800
#      Thu Nov 17 00:59:41 2016 -0800
# Node ID 5af41d2c5226c36d5a1f999ff3d99d8694ae68b9
# Parent  866281dae2407308c19c7c3109bb5501b940ee67
exchange: getbundle `bookmarks` part generator

This generator will be used during pull operation.

On Sun, Nov 20, 2016 at 4:14 AM, Stanislau Hlebik <stash@fb.com> wrote:

> # HG changeset patch
> # User Stanislau Hlebik <stash@fb.com>
> # Date 1479373181 28800
> #      Thu Nov 17 00:59:41 2016 -0800
> # Node ID 5af41d2c5226c36d5a1f999ff3d99d8694ae68b9
> # Parent  866281dae2407308c19c7c3109bb5501b940ee67
> exchange: getbundle `bookmarks` part generator
>
> This generator will be used during pull operation.
>
> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -1680,6 +1680,21 @@
>      if chunks:
>          bundler.newpart('hgtagsfnodes', data=''.join(chunks))
>
> +@getbundle2partsgenerator('bookmarks')
> +def _getbundlebookmarkspart(bundler, repo, source, bundlecaps=None,
> +                            b2caps=None, heads=None, common=None,
> +                            **kwargs):
> +    if not kwargs.get('bookmarks'):
> +        return
> +    if 'bookmarks' not in b2caps:
> +        raise ValueError(
> +            _('bookmarks are requested but client is not capable '
> +              'of receiving it'))
>

I fully support the principle of failing fast. However, AFAICT no other
@getbundle2partsgenerator raises a ValueError like this.
_getbundletagsfnodes() for example silently no-ops.

I like the idea of failing here. But at the same time, this is breaking an
apparent convention. I'd like a 2nd opinion on whether we should fail or
just no-op.


> +
> +    bookmarks = _getbookmarks(repo, **kwargs)
> +    encodedbookmarks = bookmod.encodebookmarks(bookmarks)
> +    bundler.newpart('bookmarks', data=encodedbookmarks)
> +
>  def _getbookmarks(repo, **kwargs):
>      """Returns bookmark to node mapping.
>
> diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
> --- a/mercurial/wireproto.py
> +++ b/mercurial/wireproto.py
> @@ -228,7 +228,9 @@
>               'bundlecaps': 'scsv',
>               'listkeys': 'csv',
>               'cg': 'boolean',
> -             'cbattempted': 'boolean'}
> +             'cbattempted': 'boolean',
> +             'bookmarks': 'boolean',
> +            }
>
>  # client side
>
>

On 11/22/2016 04:29 AM, Gregory Szorc wrote:
> On Sun, Nov 20, 2016 at 4:14 AM, Stanislau Hlebik <stash@fb.com
> <mailto:stash@fb.com>> wrote:
>
>     # HG changeset patch
>     # User Stanislau Hlebik <stash@fb.com <mailto:stash@fb.com>>
>     # Date 1479373181 28800
>     #      Thu Nov 17 00:59:41 2016 -0800
>     # Node ID 5af41d2c5226c36d5a1f999ff3d99d8694ae68b9
>     # Parent  866281dae2407308c19c7c3109bb5501b940ee67
>     exchange: getbundle `bookmarks` part generator
>
>     This generator will be used during pull operation.
>
>     diff --git a/mercurial/exchange.py b/mercurial/exchange.py
>     --- a/mercurial/exchange.py
>     +++ b/mercurial/exchange.py
>     @@ -1680,6 +1680,21 @@
>          if chunks:
>              bundler.newpart('hgtagsfnodes', data=''.join(chunks))
>
>     +@getbundle2partsgenerator('bookmarks')
>     +def _getbundlebookmarkspart(bundler, repo, source, bundlecaps=None,
>     +                            b2caps=None, heads=None, common=None,
>     +                            **kwargs):
>     +    if not kwargs.get('bookmarks'):
>     +        return
>     +    if 'bookmarks' not in b2caps:
>     +        raise ValueError(
>     +            _('bookmarks are requested but client is not capable '
>     +              'of receiving it'))
>
>
> I fully support the principle of failing fast. However, AFAICT no other
> @getbundle2partsgenerator raises a ValueError like this.
> _getbundletagsfnodes() for example silently no-ops.
>
> I like the idea of failing here. But at the same time, this is breaking
> an apparent convention. I'd like a 2nd opinion on whether we should fail
> or just no-op.

Failing seems to the the right behavior here. 'getbundletagsfnodes' is 
providing the client with a cache entry. So if we fail to provide it, 
the client will behave properly but have to do extra work. In addition 
the client do not explicitly request the tagsfnodes entry. It request 
changesets and we kindly provide the cache entry along if the client 
support it.

On the other hand in this case, the client is explicitly requesting 
bookmark data, if we do not provide any the client will see a valid 
reply with no bookmark and think the server has no bookmark. This lead 
to an incorrect behavior. So crashing on bogus request is the way to go 
here.

>
>
>     +
>     +    bookmarks = _getbookmarks(repo, **kwargs)
>     +    encodedbookmarks = bookmod.encodebookmarks(bookmarks)
>     +    bundler.newpart('bookmarks', data=encodedbookmarks)
>     +
>      def _getbookmarks(repo, **kwargs):
>          """Returns bookmark to node mapping.
>
>     diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
>     --- a/mercurial/wireproto.py
>     +++ b/mercurial/wireproto.py
>     @@ -228,7 +228,9 @@
>                   'bundlecaps': 'scsv',
>                   'listkeys': 'csv',
>                   'cg': 'boolean',
>     -             'cbattempted': 'boolean'}
>     +             'cbattempted': 'boolean',
>     +             'bookmarks': 'boolean',
>     +            }
>
>      # client side
>
>