Patchwork [08,of,10,V10] pull: use `bookmarks` bundle2 part

login
register
mail settings
Submitter Stanislau Hlebik
Date Nov. 20, 2016, 12:14 p.m.
Message ID <2ac3e9d5983f18f94a1d.1479644041@dev1918.lla1.facebook.com>
Download mbox | patch
Permalink /patch/17648/
State Accepted
Headers show

Comments

Stanislau Hlebik - Nov. 20, 2016, 12:14 p.m.
# HG changeset patch
# User Stanislau Hlebik <stash@fb.com>
# Date 1479373181 28800
#      Thu Nov 17 00:59:41 2016 -0800
# Node ID 2ac3e9d5983f18f94a1df84317d1d2f1bd9b88b8
# Parent  5af41d2c5226c36d5a1f999ff3d99d8694ae68b9
pull: use `bookmarks` bundle2 part

Apply changes from `bookmarks` part.
Gregory Szorc - Nov. 22, 2016, 3:48 a.m.
On Sun, Nov 20, 2016 at 4:14 AM, Stanislau Hlebik <stash@fb.com> wrote:

> # HG changeset patch
> # User Stanislau Hlebik <stash@fb.com>
> # Date 1479373181 28800
> #      Thu Nov 17 00:59:41 2016 -0800
> # Node ID 2ac3e9d5983f18f94a1df84317d1d2f1bd9b88b8
> # Parent  5af41d2c5226c36d5a1f999ff3d99d8694ae68b9
> pull: use `bookmarks` bundle2 part
>
> Apply changes from `bookmarks` part.
>
> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -1335,9 +1335,13 @@
>      kwargs['cg'] = pullop.fetch
>      if 'listkeys' in pullop.remotebundle2caps:
>          kwargs['listkeys'] = ['phases']
> -        if pullop.remotebookmarks is None:
> -            # make sure to always includes bookmark data when migrating
> -            # `hg incoming --bundle` to using this function.
> +
> +    if pullop.remotebookmarks is None:
> +        # make sure to always includes bookmark data when migrating
> +        # `hg incoming --bundle` to using this function.
> +        if 'bookmarks' in pullop.remotebundle2caps:
> +            kwargs['bookmarks'] = True
> +        elif 'listkeys' in pullop.remotebundle2caps:
>

This is already inside an `if 'listkeys' in pullop.remotebundle2caps:`
block, so this can simply be "else:".


>              kwargs['listkeys'].append('bookmarks')
>
>      # If this is a full pull / clone and the server supports the clone
> bundles
> @@ -1365,10 +1369,23 @@
>      _pullbundle2extraprepare(pullop, kwargs)
>      bundle = pullop.remote.getbundle('pull', **kwargs)
>      try:
> -        op = bundle2.processbundle(pullop.repo, bundle,
> pullop.gettransaction)
> +        bundleopbehavior = {
> +            'processbookmarksmode': 'diverge',
> +            'explicitbookmarks': pullop.explicitbookmarks,
> +            'remotepath': pullop.remote.url(),
> +        }
> +        op = bundle2.bundleoperation(pullop.repo, pullop.gettransaction,
> +                                     behavior=bundleopbehavior)
> +        op = bundle2.processbundle(pullop.repo, bundle,
> +                                   pullop.gettransaction, op=op)
>

The reuse of "op" here reads a bit weird. Can you please rename one of the
variables?


>      except error.BundleValueError as exc:
>          raise error.Abort(_('missing support for %s') % exc)
>
> +    # `bookmarks` part was in bundle and it was applied to the repo. No
> need to
> +    # apply bookmarks one more time
> +    if 'bookmarks' in kwargs and kwargs['bookmarks']:
> +        pullop.stepsdone.add('bookmarks')
> +
>

This feels a bit weird to me because we're assuming that sending the
request for bookmarks means that we received a bookmarks part. If you look
at similar code, you'll find that we update pullops.stepsdone in the part
handler for a bundle2 part. And I guess the reason we don't do things that
way for bookmarks is because we're processing bookmarks immediately as a
@parthandler (from the previous patch) and that doesn't have an "op"
argument to update. This raises another concern, which is that the previous
patch reorders /may/ reorder the application of bookmarks. Before,
bookmarks were in a listkeys and we processed them *after* phases. Now, it
appears that we may apply bookmarks *before* phases, as the @parthandler
for listkeys defers their application. I'm not sure if this matters. But it
is definitely something Pierre-Yves should take a look at.


>      if pullop.fetch:
>          results = [cg['return'] for cg in op.records['changegroup']]
>          pullop.cgresult = changegroup.combineresults(results)
>

Patch

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -1335,9 +1335,13 @@ 
     kwargs['cg'] = pullop.fetch
     if 'listkeys' in pullop.remotebundle2caps:
         kwargs['listkeys'] = ['phases']
-        if pullop.remotebookmarks is None:
-            # make sure to always includes bookmark data when migrating
-            # `hg incoming --bundle` to using this function.
+
+    if pullop.remotebookmarks is None:
+        # make sure to always includes bookmark data when migrating
+        # `hg incoming --bundle` to using this function.
+        if 'bookmarks' in pullop.remotebundle2caps:
+            kwargs['bookmarks'] = True
+        elif 'listkeys' in pullop.remotebundle2caps:
             kwargs['listkeys'].append('bookmarks')
 
     # If this is a full pull / clone and the server supports the clone bundles
@@ -1365,10 +1369,23 @@ 
     _pullbundle2extraprepare(pullop, kwargs)
     bundle = pullop.remote.getbundle('pull', **kwargs)
     try:
-        op = bundle2.processbundle(pullop.repo, bundle, pullop.gettransaction)
+        bundleopbehavior = {
+            'processbookmarksmode': 'diverge',
+            'explicitbookmarks': pullop.explicitbookmarks,
+            'remotepath': pullop.remote.url(),
+        }
+        op = bundle2.bundleoperation(pullop.repo, pullop.gettransaction,
+                                     behavior=bundleopbehavior)
+        op = bundle2.processbundle(pullop.repo, bundle,
+                                   pullop.gettransaction, op=op)
     except error.BundleValueError as exc:
         raise error.Abort(_('missing support for %s') % exc)
 
+    # `bookmarks` part was in bundle and it was applied to the repo. No need to
+    # apply bookmarks one more time
+    if 'bookmarks' in kwargs and kwargs['bookmarks']:
+        pullop.stepsdone.add('bookmarks')
+
     if pullop.fetch:
         results = [cg['return'] for cg in op.records['changegroup']]
         pullop.cgresult = changegroup.combineresults(results)