[PATCH 06 of 10 V9] bundle2: add `bookmarks` part handler

[Gregory Szorc]

On Sun, Nov 13, 2016 at 2:30 AM, Stanislau Hlebik <stash at fb.com> wrote:

> # HG changeset patch
> # User Stanislau Hlebik <stash at fb.com>
> # Date 1479032456 28800
> #      Sun Nov 13 02:20:56 2016 -0800
> # Branch stable
> # Node ID bf21586f26e5a41f7d8bf342d4b4c16d71dbc6d2
> # Parent  5cdf21805bb97ba99199c8779e3ad91137061c07
> 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
> @@ -155,6 +155,7 @@
>
>  from .i18n import _
>  from . import (
> +    bookmarks as bookmod,
>      changegroup,
>      error,
>      obsolete,
> @@ -287,13 +288,21 @@
>      * a way to construct a bundle response when applicable.
>      """
>
> -    def __init__(self, repo, transactiongetter, captureoutput=True):
> +    def __init__(self, repo, transactiongetter, captureoutput=True,
> +                 input=None):
> +        """
> +        `input` is a dictionary that is passed to part handlers to tweak
> +        their behaviour
> +        """
>          self.repo = repo
>          self.ui = repo.ui
>          self.records = unbundlerecords()
>          self.gettransaction = transactiongetter
>          self.reply = None
>          self.captureoutput = captureoutput
> +        if input is None:
> +            input = {}
> +        self.input = input
>

"input" shadows a Python built-in function. And because of the security
implications of input() (it is evil, don't ever use it), every time I see
"input" as a standalone symbol name I grow paranoid. Could this be renamed
to something else like "purpose" or "behavior?"

Also, you can write this as: self.foo = foo or {} (differentiating between
None and a falsy value isn't interesting here).


>
>  class TransactionUnavailable(RuntimeError):
>      pass
> @@ -1624,3 +1633,32 @@
>
>      cache.write()
>      op.ui.debug('applied %i hgtags fnodes cache entries\n' % count)
> +
> + at parthandler('bookmarks')
> +def handlebookmarks(op, inpart):
> +    """Processes bookmarks part.
> +
> +    `processbookmarksmode` determines how remote bookmarks are handled.
> They are
> +    either ignored ('ignore' mode), diverged ('diverge' mode) or applied
> +    ('apply' mode). 'ignore' mode is used to get bookmarks and process
> them
> +    later, 'diverge' mode is used to process bookmarks during pull,
> 'apply'
> +    mode is used during push.
> +    """
>

The fact that you had to introduce a behavior variable to determine how to
treat a bundle2 part feels a bit strange to me. Particularly since we
already have so much other data in bundle2 and haven't had a need for this
yet. This sets off alarm bells that something may not be quite right with
the design.

I want to say that the bundle part type/data alone should determine
behavior. i.e. "if you see X do Y." But this new variable now means "if you
see X, do A, B, or C depending on Y." If course, to hold to "if X do Y" you
either need multiple bundle part types or a bundle parameter. Perhaps the
latter is warranted? But if we encoded this as part of the bundle data,
that has implications for e.g. `hg unbundle`. So you can make an argument
that the bundle should consist of "dumb" data and the bundle applier should
have the smarts about what to do with the data it sees (this is what you've
implemented).

I'm curious what others feel about this.


> +
> +    bookmarks = {}
> +    bookmarks = bookmod.decodebookmarks(inpart.read())
> +    processbookmarksmode = op.input.get('processbookmarksmode', 'ignore')
>

For future reference, introducing the code that populates a variable before
introducing code that reads it will make a reviewer's life easier. As this
is written, a reviewer has to look at future parts or take it on faith that
the "input" argument will be populated properly. This isn't a huge deal.
It's a skill you'll learn as you write more micro-commits :)


> +    if processbookmarksmode == 'apply':
> +        for bookmark, node in bookmarks.items():
> +            if node:
> +                op.repo._bookmarks[bookmark] = node
> +            else:
> +                del op.repo._bookmarks[bookmark]
>

I'm trying to think of a scenario where we could get KeyError here due to a
missing key. I want to say it could happen. But I'm not positive of that.
This certainly feels like the kind of thing where you want to surround a
del x[k] with an "except KeyError" since lacking a key that will be deleted
feels like it should be harmless.


> +        op.repo._bookmarks.recordchange(op.gettransaction())
> +    elif processbookmarksmode == 'diverge':
> +        remotepath = op.input.get('remotepath', '')
> +        explicitbookmarks = op.input.get('explicitbookmarks', ())
> +        bookmod.updatefromremote(op.ui, op.repo, bookmarks,
> +                                 remotepath, op.gettransaction,
> +                                 explicit=explicitbookmarks)
> +    op.records.add('bookmarks', bookmarks)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20161115/2f540252/attachment.html>