D1944: wireproto: provide accessors for client capabilities

[indygreg (Gregory Szorc)]

indygreg requested changes to this revision.
indygreg added a comment.
This revision now requires changes to proceed.


  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.

INLINE COMMENTS

> wireprotocol.txt:391-393
> +If the server announces support for the ``protocaps`` capability, the client
> +should issue a ``protocaps`` command after the initial handshake to annonunce
> +its own capabilities. The client capabilities are persistent.

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.

> wireproto.py:845-847
> +    if proto.name in (wireprototypes.SSHV1, wireprototypes.SSHV2):
> +        # Advertise support for the ssh-only protocaps command
> +        caps.append('protocaps')

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

> wireproto.py:1015
>  
> + at wireprotocommand('protocaps', 'caps', permission='pull')
> +def protocaps(repo, proto, caps):

Please define this as `transportpolicy=POLICY_V1_ONLY`.

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

> wireproto.py:1017-1018
> +def protocaps(repo, proto, caps):
> +    if proto.name in (wireprototypes.SSHV1, wireprototypes.SSHV2):
> +        repo._protocaps = set(caps.split(' '))
> +    return bytesresponse('OK')

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.

> wireprotoserver.py:586
>          self._args = args
> +        self._protocaps = None
>  

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

> wireprotoserver.py:603-606
> +        if self._protocaps is None:
> +            value = decodevaluefromheaders(self._req, r'X-HgProto')
> +            self._protocaps = set(value.split(' '))
> +        return self._protocaps

And have this return an empty set for now.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1944

To: joerg.sonnenberger, #hg-reviewers, indygreg
Cc: indygreg, mharbison72, mercurial-devel