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

[Stanislau Hlebik]

What does binary encoding for bookmarks mean? Or you are talking about nodes encoding?

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