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

[Stanislau Hlebik]

Calling prepushkey and pushkey hook from `bookmarks` part seems a bit hacky and my current local implementation is not very pleasant. I'll try to come up with smth better


On 10/5/16, 5:59 PM, "Stanislau Hlebik" <stash at fb.com> wrote:

    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