[PATCH 6 of 9 V6] bundle2: add `bookmarks` part handler

[Pierre-Yves David]

On 10/11/2016 06:25 PM, Stanislau Hlebik wrote:
> # HG changeset patch
> # User Stanislau Hlebik <stash at fb.com>
> # Date 1476197535 25200
> #      Tue Oct 11 07:52:15 2016 -0700
> # Node ID b9e71247c1b68ce1fac7dd69cf26d106f88f9372
> # Parent  f781756b8de11a6f3e7dd5fd6354e9778defd8c3
> bundle2: add `bookmarks` part handler
>
> Applies bookmarks part to the local repo. `processbookmarksmode` determines
> how remote bookmarks are handled. They are either ignored ('ignore' mode),
> diverged ('diverge' mode) or applied ('apply' mode).
>
> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -154,7 +154,9 @@
>  import sys
>
>  from .i18n import _
> +from .node import hex
>  from . import (
> +    bookmarks as bookmod,
>      changegroup,
>      error,
>      obsolete,
> @@ -287,13 +289,18 @@
>      * a way to construct a bundle response when applicable.
>      """
>
> -    def __init__(self, repo, transactiongetter, captureoutput=True):
> +    def __init__(self, repo, transactiongetter, captureoutput=True,
> +                 processbookmarksmode='ignore', explicitbookmarks=(),
> +                 remotepath=''):
>          self.repo = repo
>          self.ui = repo.ui
>          self.records = unbundlerecords()
>          self.gettransaction = transactiongetter
>          self.reply = None
>          self.captureoutput = captureoutput
> +        self.processbookmarksmode = processbookmarksmode
> +        self.explicitbookmarks = explicitbookmarks
> +        self.remotepath = remotepath

Hum, now that we see this change in practice, it seems a bit odd. The 
unbundle operation do not have business specific logic yet 
(pull/pushoperation does but they are higher level).

The best way here is probably to introduce a "input" attribut on the 
object (or "config" but the name is meh, or anything else good).

That input should probably be a simple dictionnary where higher order 
logic will be able to stick data to be used by part handler.

What do you think ?

>
>  class TransactionUnavailable(RuntimeError):
>      pass
> @@ -1620,3 +1627,28 @@
>
>      cache.write()
>      op.ui.debug('applied %i hgtags fnodes cache entries\n' % count)
> +
> + at parthandler('bookmarks')
> +def handlebookmarks(op, inpart):
> +    """Applies bookmarks to the local repo.

"Applies" does not seems to be the best wording as some of the process 
mode do not apply anything.

> +
> +    `processbookmarksmode` determines how remote bookmarks are handled. They are
> +    either ignored ('ignore' mode), diverged ('diverge' mode) or applied
> +    ('apply' mode).
> +    """

We should probably explain what use case each more is aimed at.

> +
> +    bookmarks = {}
> +    bookmarks = bookmod.decodebookmarks(inpart.read())
> +    if op.processbookmarksmode == 'apply':
> +        for bookmark, node in bookmarks.items():
> +            if node:
> +                op.repo._bookmarks[bookmark] = node
> +            else:
> +                del op.repo._bookmarks[bookmark]
> +        op.repo._bookmarks.recordchange(op.gettransaction())

What is your plan regarding hooks execution here?


> +    elif op.processbookmarksmode == 'diverge':
> +        bookmod.updatefromremote(op.ui, op.repo, bookmarks,
> +                                 op.remotepath, op.gettransaction,
> +                                 explicit=op.explicitbookmarks,
> +                                 srchex=hex)
> +    op.records.add('bookmarks', bookmarks)

How do you handle cases where the part exist multiple time in the bundle ?


cheers

-- 
Pierre-Yves David