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.sz...@gmail.com>
Date: Tuesday, November 22, 2016 at 3:48 AM
To: Stanislau Hlebik <st...@fb.com>
Cc: mercurial-devel <mercurial-devel@mercurial-scm.org>, Pierre-Yves David
<pierre-yves.da...@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
<st...@fb.com<mailto:st...@fb.com>> wrote:
# HG changeset patch
# User Stanislau Hlebik <st...@fb.com<mailto: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:".
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)
--
Pierre-Yves David
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
