[PATCH 3 of 8 V5] bundle2: add `bookmarks` part handler

[Pierre-Yves David]

On 09/16/2016 01:10 PM, Stanislau Hlebik wrote:
> # HG changeset patch
> # User Stanislau Hlebik <stash at fb.com>
> # Date 1473954996 25200
> #      Thu Sep 15 08:56:36 2016 -0700
> # Node ID 3456cba8a4879e74f3b928df321ce7e7c695fb4c
> # Parent  f3fb030f0e4601561ac94137c7481694407db7b7
> bundle2: add `bookmarks` part handler
>
> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -154,8 +154,12 @@
>  import sys
>
>  from .i18n import _
> +from .node import (
> +    bin,
> +)
>  from . import (
>      changegroup,
> +    encoding,
>      error,
>      obsolete,
>      pushkey,
> @@ -1614,3 +1618,20 @@
>
>      cache.write()
>      op.ui.debug('applied %i hgtags fnodes cache entries\n' % count)
> +
> + at parthandler('bookmarks')
> +def handlebookmarks(op, inpart):


Please add some docstring to this part.

Greg, do you think we should also update the technical documentation 
specification of each part or should the docstring take care of this ?

> +    dec = encoding.tolocal
> +    bookmarks = {}
> +    for bookmarknode in inpart.read().splitlines():
> +        book, node = bookmarknode.rsplit(' ', 1)

We should probably stick on binary encoding for bookmark. This would 
avoid the need to encode/decode the hex and probably be more extensible 
in the future.

> +        bookmarks[dec(book)] = node
> +    if op.applybookmarks:
> +        for bookmark, node in bookmarks.items():
> +            if node:
> +                op.repo._bookmarks[bookmark] = bin(node)
> +            else:
> +                del op.repo._bookmarks[bookmark]
> +        op.repo._bookmarks.recordchange(op.gettransaction())

You wan to call gettransaction before doing any change. The pushrebase 
extension delay the locking until the transaction is actually fetched 
and this would expose you to a race here.

As you pointed on IRC, this is no longer trigger the pushkey hooks for 
bookmarks. This highlight the fact that:

(1) We should most probably introduces some hook dedicated to bookmark
(2) unfortunatly, we probably need to trigger the pushkey hook even if 
we are not using pushkey for BC reason :-(


> +    else:
> +        op.records.add('bookmarks', bookmarks)

We want to add record in all cases. The record are here to help other 
part of the push (eg: handler, hooks) to understand and collaborate with 
what happened.

Cheers,

-- 
Pierre-Yves David