This is an archive of the discontinued Mercurial Phabricator instance.

narrow: the first version of narrow_widen wireprotocol command
ClosedPublic

Authored by pulkit on Sep 30 2018, 1:49 PM.

Details

Summary

This patch introduces a wireprotocol command narrow_widen() which will be used
to widen a narrow copy using hg tracked command provided by narrow extension.

The wireprotocol command takes the old and new includes and excludes, common
heads, changegroup version, known revs, and a boolean ellipses and generates a
bundle2 of the required data and send it. The clients receives the bundle2
and applies that.

A bundle2 instead of changegroup because in future we might want to add more
things to send while widening. Thanks for martinvonz for the suggestion.

I am not sure whether we need changegroup version as an argument to the command
as I *think* narrow needs changegroup3 already.

The tests shows that we don't exchange phase data now while widening which is
nice. Also we don't check for pushkeys, rbc-cache, bookmarks etc.

This does not support ellipses cases for now but will be supported in future
patches. Since we send bundle2, it won't be hard to plug the ellipses logic in
here.

The existing code for widening a non-ellipses case is also dropped in this
patch.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit created this revision.Sep 30 2018, 1:49 PM
pulkit updated this revision to Diff 11520.Oct 1 2018, 12:02 PM

From a wire protocol perspective, this looks good.

hgext/narrow/narrowwirepeer.py
66–69

I think we'll want to use narrowspec.validatepatterns() to ensure the patterns are of the appropriate type.

72–79

Do we need to call narrowspec.restrictpatterns() anywhere in here?

80–82

I /thought/ we had a way to get a stream of chunks out of a changegroup. i.e. we shouldn't need to go through the util.chunkbuffer() / cg.read() dance. But I could be wrong: the changegroup APIs are a bit wonky.

pulkit edited the summary of this revision. (Show Details)Oct 2 2018, 10:19 AM
pulkit updated this revision to Diff 11561.
pulkit added inline comments.Oct 2 2018, 10:25 AM
hgext/narrow/narrowwirepeer.py
72–79

I am not sure, @martinvonz what do you think?

pulkit edited the summary of this revision. (Show Details)Oct 2 2018, 1:18 PM
pulkit retitled this revision from [RFC] narrow: the first version of narrow_widen wireprotocol command to narrow: the first version of narrow_widen wireprotocol command.
pulkit updated this revision to Diff 11572.

I am not sure whether we need changegroup version as an argument to the command
as I *think* narrow needs changegroup3 already.

Ellipses need changegroup3 (for the revision flag) and treemanifests also do (for the extra room for directory manifests), but narrow itself shouldn't need it. We can probably still assume it's cg3 because there's probably no reason to use an older version for a new command (and we can add the extra parameter later if we need to support a future cg4).

hgext/narrow/narrowcommands.py
293–295

As I said on IRC, I think we (Google) would appreciate it if this could remain here for a while to give us more time to migrate our custom server to the new wire protocol command. I was thinking we could just fall back to the getbundle command if narrow_widen isn't supported. That probably means changing the capability to "exp-narrow-2" now and call getbundle if "exp-narrow-1" is supported but "exp-narrow-2" isn't. That shouldn't be very difficult, but I don't think we've done kind of thing before.

Actually, you can ignore all that because our server has the ellipses capability, so it will not get here anyway :) We should instead think about such a migration path when we add support for ellipses to narrow_widen.

316

Should we call this something like "heads" instead? It's the set of common heads between the server and client and "common" makes sense together with "heads" in the getbundle call, but it doesn't seem to make as much sense here.

321–324

It looks like this doesn't need to be in in the commandexecutor block and the transaction probably doesn't need to span the wire protocol request. IOW, I think you can drop repo.transaction()from line 293 and instead put that togetgher this with this with-block.

hgext/narrow/narrowwirepeer.py
51

Should we just put this in core from the beginning?

72–79

I assume the reason for that would be when pulling from a narrow clone? I think we have mostly ignored that case so far. I'm fine with just not caring about it for a while more :)

81–84

I suppose we want the default to be False? Isn't ellipses = (args.get('ellipses') == '1') enough? Or is the idea to be flexible with what values we accept? Is that what other wire protocol commands do?

pulkit updated this revision to Diff 11591.Oct 3 2018, 7:55 AM
pulkit added inline comments.Oct 3 2018, 7:55 AM
hgext/narrow/narrowcommands.py
293–295

We can have a 'exp-ellipses-2' which will tell whether the server supports ellipses widening using narrow_widen() wireprotocol command or not. I think that should help in the meantime. Also will a week, or 10-15 days be enough for you? I think it will be better if can prevent releasing this compatibility because exp-ellipses-1 was introduced in this cycle only.

316

Made this commonheads which makes more sense. Thanks for the suggestion!

321–324

Did that, thanks!

hgext/narrow/narrowwirepeer.py
51

I implemented the peer initially in core only, but while implementing server side, I realized it rely on logic in narrowbundle2.py which also needs to be moved to core. Then I decided to implement it cleanly in the extension and then move it to core.

81–84

I need to specify the arguments instead of just specifying "*".

Don't forget this one.

hgext/narrow/narrowcommands.py
293–295

We don't really care whether a version is released or not. It would be nice to have a version of the client that would use the new wire protocol with ellipses if the server said it supported that but would otherwise fall back to the old getbundle-based call.

hgext/narrow/narrowwirepeer.py
51

It seems to depend only on widen_bundle, which doesn't seem to depend on anything else, so it would probably be easy to move it to core, but I won't insist.

81–84

That code was added in https://www.mercurial-scm.org/repo/hg/rev/3e7f675628ad. Maybe @indygreg can tell us if he thinks we should support the same values for a new wireprotocol command.

indygreg added inline comments.Oct 3 2018, 2:51 PM
hgext/narrow/narrowbundle2.py
329

Because of how wireprotov1server.getbundle() handles arguments, removing this argument from the definition of arguments to the getbundle wire protocol command means that existing clients attempting to send the argument to the getbundle wire protocol command will cause a server-side exception.

We need to keep this key around for BC unless we're fine breaking old clients or unless there is something that changes server capabilities in a way that prevents old clients from calling getbundle with this argument. We could ignore the argument or nerf the server to error if it is received.

hgext/narrow/narrowwirepeer.py
66

I'm pretty sure we'll want a try..except around this entire command so that we'll send a bundle2 error payload on failure. See wireprotov1server.getbundle() for inspiration. Search for use of the error:abort bundle2 part. We'll want to do something like that in the except block.

Keep in mind exceptions can occur mid stream. In that case, proper error handling is a bit harder. But a good start is to put a try..except around all the code before we return the output.

81–84

We probably want to extract the argument "parsing" from wireprotov1server.getbundle() into a standalone function so we can use it for this command.

Arguments in wire protocol version 1 are wonky :/

pulkit edited the summary of this revision. (Show Details)Oct 4 2018, 10:04 AM
pulkit updated this revision to Diff 11675.

I need to specify the arguments instead of just specifying "*".

Don't forget this one.

Did that!

hgext/narrow/narrowbundle2.py
329

This argument has not yet been a part of release. It was introduced in D4383 which was pushed in the end on August. So I don't think we need to take care of that unless someone is using hg from default branch. @martinvonz will this removal affect you? Do you have D4383 grafted to your internal mercurial?

hgext/narrow/narrowwirepeer.py
51

I will try to follow-up before the freeze.

81–84

Maybe we need to have a function which takes the values and what type to decode into and that can be used in both places. Also after specifying the args names instead of "*", we don't have a dictionary with all the arguments. We need to build one.

This looks pretty reasonable to me from a wire protocol perspective.

hgext/narrow/narrowwirepeer.py
107

This may want to catch Exception.

pulkit added inline comments.Oct 5 2018, 12:28 PM
hgext/narrow/narrowwirepeer.py
107

Okay, I will followup if this got pushed. Also I copied it from getbundle, do we want to change it there also?

This revision was automatically updated to reflect the committed changes.