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

[Pierre-Yves David]

On 10/05/2016 06:59 PM, Stanislau Hlebik wrote:
> What does binary encoding for bookmarks mean? Or you are talking about nodes encoding?

It means using a binary format instead of a textual one. Historically 
Mercurial have been using textual format a lot, but we have been moving 
to favoring binary ones in the later years.

A binary format for bookmarks could be:

   <20 bytes for nodes><4 (or 2?) bytes for length of 
bookmarks><bookmark name>

>
> Thanks,
> Stas
>
> On 9/16/16, 4:53 PM, "Pierre-Yves David" <pierre-yves.david at ens-lyon.org> wrote:
>
>
>
>     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
>
>

-- 
Pierre-Yves David