Patchwork D1944: wireproto: provide accessors for client capabilities

login
register
mail settings
Submitter phabricator
Date Jan. 28, 2018, 1:17 a.m.
Message ID <differential-rev-PHID-DREV-zfpmhcakrvx6i5sttczu-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/27073/
State Superseded
Headers show

Comments

phabricator - Jan. 28, 2018, 1:17 a.m.
joerg.sonnenberger created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION 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.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/hgweb/protocol.py
  mercurial/sshpeer.py
  mercurial/sshserver.py
  mercurial/wireproto.py
  tests/test-ssh-bundle1.t
  tests/test-ssh.t

CHANGE DETAILS




To: joerg.sonnenberger, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 28, 2018, 2:01 a.m.
joerg.sonnenberger added inline comments.

INLINE COMMENTS

> sshserver.py:74
> +
> +    def do_protocaps(self):
> +        """ssh-specific command for sending the client capabilities

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.

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 28, 2018, 6:36 a.m.
mharbison72 added inline comments.

INLINE COMMENTS

> sshserver.py:74
> +
> +    def do_protocaps(self):
> +        """ssh-specific command for sending the client capabilities

See https://phab.mercurial-scm.org/rHGbf2db35a6fe7a02b69eaa0e35708cf0044d73510 for an example of # no-check-commit.

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers
Cc: mharbison72, mercurial-devel
phabricator - Jan. 28, 2018, 11:49 a.m.
joerg.sonnenberger added inline comments.

INLINE COMMENTS

> mharbison72 wrote in sshserver.py:74
> See https://phab.mercurial-scm.org/rHGbf2db35a6fe7a02b69eaa0e35708cf0044d73510 for an example of # no-check-commit.

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.

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers
Cc: mharbison72, mercurial-devel
phabricator - Jan. 31, 2018, 12:47 a.m.
indygreg added a comment.


  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.

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers
Cc: indygreg, mharbison72, mercurial-devel
phabricator - April 4, 2018, 10:55 p.m.
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
phabricator - April 6, 2018, 5:26 p.m.
indygreg accepted this revision.
indygreg added a comment.
This revision is now accepted and ready to land.


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

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/tests/test-ssh.t b/tests/test-ssh.t
--- a/tests/test-ssh.t
+++ b/tests/test-ssh.t
@@ -486,9 +486,12 @@ 
   devel-peer-request: between
   devel-peer-request:   pairs: 81 bytes
   sending between command
-  remote: 384
-  remote: capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch streamreqs=generaldelta,revlogv1 $USUAL_BUNDLE2_CAPS_SERVER$ unbundle=HG10GZ,HG10BZ,HG10UN
+  remote: 394
+  remote: capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch streamreqs=generaldelta,revlogv1 $USUAL_BUNDLE2_CAPS_SERVER$ unbundle=HG10GZ,HG10BZ,HG10UN protocaps
   remote: 1
+  devel-peer-request: protocaps
+  devel-peer-request:   caps: 25 bytes
+  sending protocaps command
   query 1; heads
   devel-peer-request: batch
   devel-peer-request:   cmds: 141 bytes
diff --git a/tests/test-ssh-bundle1.t b/tests/test-ssh-bundle1.t
--- a/tests/test-ssh-bundle1.t
+++ b/tests/test-ssh-bundle1.t
@@ -467,9 +467,10 @@ 
   running .* ".*/dummyssh" ['"]user@dummy['"] ('|")hg -R remote serve --stdio('|") (re)
   sending hello command
   sending between command
-  remote: 384
-  remote: capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch streamreqs=generaldelta,revlogv1 $USUAL_BUNDLE2_CAPS_SERVER$ unbundle=HG10GZ,HG10BZ,HG10UN
+  remote: 394
+  remote: capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch streamreqs=generaldelta,revlogv1 $USUAL_BUNDLE2_CAPS_SERVER$ unbundle=HG10GZ,HG10BZ,HG10UN protocaps
   remote: 1
+  sending protocaps command
   preparing listkeys for "bookmarks"
   sending listkeys command
   received listkey for "bookmarks": 45 bytes
diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
--- a/mercurial/wireproto.py
+++ b/mercurial/wireproto.py
@@ -64,6 +64,17 @@ 
         """
         raise NotImplementedError()
 
+    def getprotocaps(self):
+        """return the wireprotocol capabilities of the current request"""
+        raise NotImplementedError()
+
+    def getcompressionsupport(self):
+        """return a list of compression methods supported by the client"""
+        for cap in self.getprotocaps():
+            if cap.startswith('comp='):
+                return cap[5:].split(',')
+        return ['zlib', 'none']
+
     def redirect(self):
         """may setup interception for stdout and stderr
 
@@ -799,6 +810,9 @@ 
             comptypes = ','.join(urlreq.quote(e.wireprotosupport().name)
                                  for e in compengines)
             caps.append('compression=%s' % comptypes)
+    elif proto.name == 'ssh':
+        # Advertise support for the ssh-only protocaps command
+        caps.append('protocaps')
 
     return caps
 
diff --git a/mercurial/sshserver.py b/mercurial/sshserver.py
--- a/mercurial/sshserver.py
+++ b/mercurial/sshserver.py
@@ -27,6 +27,7 @@ 
         self.fin = ui.fin
         self.fout = ui.fout
         self.name = 'ssh'
+        self._protocaps = []
 
         hook.redirect(True)
         ui.fout = repo.ui.fout = ui.ferr
@@ -66,6 +67,20 @@ 
             fpout.write(self.fin.read(count))
             count = int(self.fin.readline())
 
+    def getprotocaps(self):
+        """return the wireprotocol capabilities of the current request"""
+        return self._protocaps
+
+    def do_protocaps(self):
+        """ssh-specific command for sending the client capabilities
+
+        The ssh protocol is stateful and doesn't retransmit the wireprotocol
+        capabilities on every request.
+        """
+        self._protocaps = self.getargs(self.do_protocaps_arguments)[0]
+        self.sendresponse('OK')
+    do_protocaps_arguments = 'caps'
+
     def redirect(self):
         pass
 
diff --git a/mercurial/sshpeer.py b/mercurial/sshpeer.py
--- a/mercurial/sshpeer.py
+++ b/mercurial/sshpeer.py
@@ -13,6 +13,7 @@ 
 from . import (
     error,
     pycompat,
+    sshserver,
     util,
     wireproto,
 )
@@ -182,6 +183,13 @@ 
 
     # End of _basewirecommands interface.
 
+    def _clientcapabilities(self):
+        protoparams = []
+        comps = [e.wireprotosupport().name for e in
+                 util.compengines.supportedwireengines(util.CLIENTROLE)]
+        protoparams.append('comp=%s' % ','.join(comps))
+        return protoparams
+
     def _validaterepo(self, sshcmd, args, remotecmd, sshenv=None):
         # cleanup up previous run
         self._cleanup()
@@ -240,6 +248,12 @@ 
                 self._caps.update(l[:-1].split(":")[1].split())
                 break
 
+        if 'protocaps' in self._caps:
+            try:
+                self._call("protocaps",
+                           caps=' '.join(self._clientcapabilities()))
+            except IOError:
+                badresponse()
     def _readerr(self):
         _forwardoutput(self.ui, self._pipee)
 
@@ -296,7 +310,12 @@ 
                         dbg(line % '  %s-%s: %d' % (key, dk, len(dv)))
         self.ui.debug("sending %s command\n" % cmd)
         self._pipeo.write("%s\n" % cmd)
-        _func, names = wireproto.commands[cmd]
+        if cmd in wireproto.commands:
+            _func, names = wireproto.commands[cmd]
+        elif getattr(sshserver.sshserver, 'do_' + cmd, None):
+            names = getattr(sshserver.sshserver, 'do_%s_arguments' % cmd, None)
+        else:
+            raise KeyError(cmd)
         keys = names.split()
         wireargs = {}
         for k in keys:
diff --git a/mercurial/hgweb/protocol.py b/mercurial/hgweb/protocol.py
--- a/mercurial/hgweb/protocol.py
+++ b/mercurial/hgweb/protocol.py
@@ -52,6 +52,7 @@ 
         self.response = ''
         self.ui = ui
         self.name = 'http'
+        self._protocaps = None
 
     def getargs(self, args):
         knownargs = self._args()
@@ -88,6 +89,11 @@ 
         length -= int(self.req.env.get(r'HTTP_X_HGARGS_POST', 0))
         for s in util.filechunkiter(self.req, limit=length):
             fp.write(s)
+    def getprotocaps(self):
+        if self._protocaps is None:
+            value = decodevaluefromheaders(self.req, r'X-HgProto')
+            self._protocaps = value.split(' ')
+        return self._protocaps
     def redirect(self):
         self.oldio = self.ui.fout, self.ui.ferr
         self.ui.ferr = self.ui.fout = stringio()
@@ -109,19 +115,15 @@ 
         """
         # Determine the response media type and compression engine based
         # on the request parameters.
-        protocaps = decodevaluefromheaders(self.req, r'X-HgProto').split(' ')
+        protocaps = self.getprotocaps()
 
         if '0.2' in protocaps:
             # All clients are expected to support uncompressed data.
             if prefer_uncompressed:
                 return HGTYPE2, util._noopengine(), {}
 
             # Default as defined by wire protocol spec.
-            compformats = ['zlib', 'none']
-            for cap in protocaps:
-                if cap.startswith('comp='):
-                    compformats = cap[5:].split(',')
-                    break
+            compformats = self.getcompressionsupport()
 
             # Now find an agreed upon compression format.
             for engine in wireproto.supportedcompengines(self.ui, self,