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

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

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.

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

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.

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.

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.

Should we just put this in core from the beginning?

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

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?

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.

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

Did that, thanks!

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.

I am not sure, I just copied from getbundle() handling: https://www.mercurial-scm.org/repo/hg/file/1a4c1a3cc3f5/mercurial/wireprotov1server.py#l404

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.

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.

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.

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.

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.

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

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?

I will try to follow-up before the freeze.

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 may want to catch Exception.

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