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

login
register
mail settings
Submitter Stanislau Hlebik
Date Nov. 13, 2016, 10:30 a.m.
Message ID <bf21586f26e5a41f7d8b.1479033017@dev1918.lla1.facebook.com>
Download mbox | patch
Permalink /patch/17537/
State Changes Requested
Headers show

Comments

Stanislau Hlebik - Nov. 13, 2016, 10:30 a.m.
# HG changeset patch
# User Stanislau Hlebik <stash@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).
Gregory Szorc - Nov. 16, 2016, 4:46 a.m.
On Sun, Nov 13, 2016 at 2:30 AM, Stanislau Hlebik <stash@fb.com> wrote:

> # HG changeset patch
> # User Stanislau Hlebik <stash@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)
> +
> +@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@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

Patch

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
 
 class TransactionUnavailable(RuntimeError):
     pass
@@ -1624,3 +1633,32 @@ 
 
     cache.write()
     op.ui.debug('applied %i hgtags fnodes cache entries\n' % count)
+
+@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.
+    """
+
+    bookmarks = {}
+    bookmarks = bookmod.decodebookmarks(inpart.read())
+    processbookmarksmode = op.input.get('processbookmarksmode', 'ignore')
+    if 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())
+    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)