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

login
register
mail settings
Submitter Stanislau Hlebik
Date Nov. 22, 2016, 10:14 a.m.
Message ID <C8263504-BDF0-4BCF-BBE3-C09591C8ECCF@fb.com>
Download mbox | patch
Permalink /patch/17690/
State Superseded
Headers show

Comments

Stanislau Hlebik - Nov. 22, 2016, 10:14 a.m.
From: Gregory Szorc <gregory.szorc@gmail.com>

Date: Tuesday, November 22, 2016 at 3:48 AM
To: Stanislau Hlebik <stash@fb.com>
Cc: mercurial-devel <mercurial-devel@mercurial-scm.org>, Pierre-Yves David <pierre-yves.david@ens-lyon.org>
Subject: Re: [PATCH 08 of 10 V10] pull: use `bookmarks` bundle2 part

On Sun, Nov 20, 2016 at 4:14 AM, Stanislau Hlebik <stash@fb.com<mailto:stash@fb.com>> wrote:
# HG changeset patch
# User Stanislau Hlebik <stash@fb.com<mailto: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.

No, it’s not inside `if 'listkeys' in pullop.remotebundle2caps:`, it’s in different block so I have to use elif

             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?

Ok, I’ll rename first op to bundleop.

     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.

I think changing of order should not affect the end result, but let’s wait for Pierre-Yves.
Meanwhile I’ll send new series without first 3 patches (they were queued) and with your comments fixed


     if pullop.fetch:
         results = [cg['return'] for cg in op.records['changegroup']]
         pullop.cgresult = changegroup.combineresults(results)
Pierre-Yves David - Dec. 3, 2016, 2:47 a.m.
Sorry for the long delay, I've been far too busy in November,

There is a couple thing that you could do to help getting a lower 
latency on such review. (actual answer to actual question are available 
inline)

[CCing me on patch series]

Do not automatically CC me on all your series (especially the 10 patches 
ones). Otherwise part I really should be looking at get drown into other 
works.

   I've a special email folder for "stuff on Mercurial-devel that I'm a 
direct recipient to". This usually get a handful of email per day in it 
and help me to keep low latency on active discussion. The low amount of 
email there means the reply to my question will stand out even if tens 
or hundred of emails have stacked in my mail mercurial-devel folder. In 
addition, these are email with ongoing discussion for most of them will 
have a couple of focused questions that I can be processed quickly. 
CCing me on new series defeat these two properties: the traffic in that 
folder get too high for me to handle it without a proper chunk of time 
and getting new patches in there means I needs to do a full review of 
the content instead of replying to  simple question.

   So do not worry and do not CC me on resent. I'll spot resend when I 
scan through patchwork and look at them, especially if they are close to 
completion.

[Getting my attention on specific questions]

That said, if a discussion get to a point were people want my attention 
on a specific details, feel free to CC me so that I can spot the 
question (this will only work if this is not drown as a reply of a 
series I'm not CC one in the first place). In addition you can reply to 
some specific patch in your series and CC me if you thing some specific 
patches/hunks especially needs my attention.

[send smaller series]

It is recommended to send series of 6 patches or less. In the case of 
this series, there is some intermediate group that can be identified. 
eg: patches 1-3 (they got push independently),  patches 4-5 (move to 
binary encoding internally) and patches 6-10 (bundle2 part).
Having smaller series:
  * will make them easier to pick up for reviewers,
  * reduce overall traffic on the list because later patch in the series 
are likely to get impacted by the review of the earlier one,
  * avoid getting to V11 for your series (as each bits, can start at V1) 
This make it simpler for other to follow the feedback a series received 
in the past.

[upgrade your email client]

I'm not sure what you are using now, but it seems unable to provide a 
text version with proper quoting. (see how Greg and your answer get 
mixed). Using an email that actually can do email would help other 
people to follow the review discussion.

On 11/22/2016 11:14 AM, Stanislau Hlebik wrote:
> From: Gregory Szorc <gregory.szorc@gmail.com>
> Date: Tuesday, November 22, 2016 at 3:48 AM
> To: Stanislau Hlebik <stash@fb.com>
> Cc: mercurial-devel <mercurial-devel@mercurial-scm.org>, Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> Subject: Re: [PATCH 08 of 10 V10] pull: use `bookmarks` bundle2 part
>
> On Sun, Nov 20, 2016 at 4:14 AM, Stanislau Hlebik <stash@fb.com<mailto:stash@fb.com>> wrote:
> # HG changeset patch
> # User Stanislau Hlebik <stash@fb.com<mailto: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:".
>
> No, it’s not inside `if 'listkeys' in pullop.remotebundle2caps:`, it’s in different block so I have to use elif
>
>              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?
>
> Ok, I’ll rename first op to bundleop.

Actually, the op in input and the op in output are the same object. So 
keeping the same name seems fine.

The 'op' argument in input is optional and the processbundle function 
will take care of creating the 'op' object itself if not provided (which 
is why it get returned)

>      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 the server advertise bookmark part and request bookmark data through 
that mean we should consider that step should be consider 'done' when we 
request the bookmark. We have to trust the server to properly reply to 
our getbundle request. And we don't need anything other way to fetch 
bookmark to get into play. See how it is done for changegroup and 
obsmarker a bit before the call to _pullbundle2extraprepare

> I think changing of order should not affect the end result, but let’s wait for Pierre-Yves.
> Meanwhile I’ll send new series without first 3 patches (they were queued) and with your comments fixed

Yes, I don't think we have a requirement on the order of change beside 
the fact we need the changeset to exist before referencing them with 
bookmark or phase.

In particular sensible hooking should happen at the time the transaction 
is closed and all data is available.

That said, this does not guarantee there is a zero change of breakage, 
but if anything break this is a case were I think we should sanitize 
things instead of keeping part doomed to fail.

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


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