On Sun, Nov 20, 2016 at 4:14 AM, Stanislau Hlebik <st...@fb.com> wrote:
> # HG changeset patch > # User Stanislau Hlebik <st...@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) >
_______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel