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

[Gregory Szorc]

On Sun, Nov 20, 2016 at 4:14 AM, Stanislau Hlebik <stash at fb.com> wrote:

> # HG changeset patch
> # User Stanislau Hlebik <stash at 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)
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20161121/869dd878/attachment.html>