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
>
> +@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
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
