This is an archive of the discontinued Mercurial Phabricator instance.

wireproto: provide accessors for client capabilities
ClosedPublic

Authored by joerg.sonnenberger on Jan 27 2018, 8:16 PM.

Details

Summary

For HTTP, this refactors the existing logic, including the parsing of
the compression engine capability.

For SSH, this adds a ssh-only capability "protocaps" and a command for
informing the server on what the client supports. Since SSH is stateful,
keep track of the capabilities in the server instance.

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

mercurial/sshserver.py
74 ↗(On Diff #5011)

So this function triggers the underscore function name check, even if the support for this kind of naming is pre-existing. Since the old instances are no longer relevant, renaming the callback would be an option too.

mharbison72 added inline comments.
mercurial/sshserver.py
74 ↗(On Diff #5011)

See bf2db35a6fe7 for an example of # no-check-commit.

mercurial/sshserver.py
74 ↗(On Diff #5011)

Thanks, that answers half of the question. This is an internal interface contract, it could be adjusted as well or replaced with an explicit dictionary. I have no preference for what is cleaner.

I've already started work on a ground-up rewrite of the wire protocol. I hope to start sending patches on Thursday when the 4.6 release window opens.

I hate to spew "stop energy," but I'm hesitant to accept any new additions to the existing wire protocol given a new protocol will soon emerge. The feature in this patch is useful. And I will try to work it into my patches. At the very least, I'll keep it in the back of my mind as I'm designing things. The new wire protocol won't be set in stone from day 0 and we should be able to change it throughout the 4.6 cycle. So there will be plenty of time for bikeshedding and enhancements in the weeks ahead.

indygreg requested changes to this revision.Apr 4 2018, 6:55 PM

I'm OK with the general approach. But this requires a handful of changes before it can be accepted.

For protocol version 2, I plan to send client capabilities as part of the command request. Now that we are using CBOR for command requests, it will be trivial to add client capabilities to the request. We will redundantly send capabilities as part of multiple requests. But since we can have an active compression context in use across command requests, the wire overhead will be negligible. So I'm not worried about the overhead. I care more about making server-side command handlers stateless, as that will make it easier to implement alternate servers.

mercurial/help/internals/wireprotocol.txt
391–393

This should be in the SSH Version 1 Transport section below, because I don't intent to carry this stateful feature forward to protocol version 2.

Also, the new capability should be documented in the capabilities section in this document.

mercurial/wireproto.py
875–877

The protocol handler class in wireprotoserver.py now has an addcapabilities() that should be used for adding transport-specific capabilities. Please use it.

1042

Please define this as transportpolicy=POLICY_V1_ONLY.

Also, please add documentation for the new command to wireprotocol.txt.

1044–1045

The transport filtering isn't necessary if this is implemented differently. Yes, we could expose the command to HTTP. It shouldn't matter.

Also, please set this on proto instead because it is the most appropriate place to define this. The protocol handler's lifetime is per connection for SSH and per-request for HTTP. The repository instance can outlive the HTTP request and the HTTP/SSH connection and it therefore isn't an appropriate place.

mercurial/wireprotoserver.py
594

Please remove this, as we won't carry this implementation forward to version 2.

610–613

And have this return an empty set for now.

This revision now requires changes to proceed.Apr 4 2018, 6:55 PM
joerg.sonnenberger marked 6 inline comments as done.Apr 5 2018, 7:20 AM
indygreg accepted this revision.Apr 6 2018, 1:26 PM

This looks great! It adds some much needed parity between the existing SSH and HTTP transports.

This revision is now accepted and ready to land.Apr 6 2018, 1:26 PM
This revision was automatically updated to reflect the committed changes.