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

[Stanislau Hlebik]

On Sun, Nov 13, 2016 at 2:30 AM, Stanislau Hlebik <stash at fb.com<mailto:stash at fb.com>> wrote:
# HG changeset patch
# User Stanislau Hlebik <stash at fb.com<mailto: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).

Thanks, will fix it


 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.

Having a smart unbundler was Pierre-Yves’s idea, so I think he supports it.

BTW, what implication for `hg unbundle` encoding info in bundle data has?

+
+    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 :)

Thanks

+    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.

Makes sense, I’ll catch KeyError


+        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<mailto:Mercurial-devel at mercurial-scm.org>
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel<https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DgMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=1EQ58Dmb5uX1qHujcsT1Mg&m=d8gZ8AyG1LBpY9wvHw8zriCoo1M0dw9vI71Gk4dDyD0&s=7wCM_P_8QJwfSiKX0gz7lvZa-UkrbIS6IYty80Mtt7I&e=>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20161116/f0345d9a/attachment.html>