Patchwork D2080: wireprotoserver: split ssh protocol handler and server

login
register
mail settings
Submitter phabricator
Date Feb. 8, 2018, 12:27 a.m.
Message ID <differential-rev-PHID-DREV-akjubx45jfy4v47xml22-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/27448/
State Superseded
Headers show

Comments

phabricator - Feb. 8, 2018, 12:27 a.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  We want to formalize the interface for protocol handlers. Today,
  server functionality (which is domain specific) is interleaved
  with protocol handling functionality (which conforms to a generic
  interface) in the sshserver class.
  
  This commit splits the ssh protocol handling code out of the
  sshserver class.
  
  The state of the new code isn't great in terms of purity: there
  are still some private attribute accesses from sshserver into
  sshprotocolhandler that shouldn't be there. This will likely
  require some interface changes to address. I'm not yet sure how
  to reconcile things. But this split seems strictly better in
  terms of isolating the wire protocol interface from the rest of
  the code.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/wireprotoserver.py
  tests/sshprotoext.py
  tests/test-sshserver.py

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel

Patch

diff --git a/tests/test-sshserver.py b/tests/test-sshserver.py
--- a/tests/test-sshserver.py
+++ b/tests/test-sshserver.py
@@ -24,7 +24,7 @@ 
     def assertparse(self, cmd, input, expected):
         server = mockserver(input)
         _func, spec = wireproto.commands[cmd]
-        self.assertEqual(server.getargs(spec), expected)
+        self.assertEqual(server._proto.getargs(spec), expected)
 
 def mockserver(inbytes):
     ui = mockui(inbytes)
diff --git a/tests/sshprotoext.py b/tests/sshprotoext.py
--- a/tests/sshprotoext.py
+++ b/tests/sshprotoext.py
@@ -45,11 +45,11 @@ 
         l = self._fin.readline()
         assert l == b'hello\n'
         # Respond to unknown commands with an empty reply.
-        self._sendresponse(b'')
+        self._proto._sendresponse(b'')
         l = self._fin.readline()
         assert l == b'between\n'
-        rsp = wireproto.dispatch(self._repo, self, b'between')
-        self._handlers[rsp.__class__](self, rsp)
+        rsp = wireproto.dispatch(self._repo, self._proto, b'between')
+        self._proto._handlers[rsp.__class__](self._proto, rsp)
 
         super(prehelloserver, self).serve_forever()
 
@@ -73,7 +73,7 @@ 
 
         # Send the upgrade response.
         self._fout.write(b'upgraded %s %s\n' % (token, name))
-        servercaps = wireproto.capabilities(self._repo, self)
+        servercaps = wireproto.capabilities(self._repo, self._proto)
         rsp = b'capabilities: %s' % servercaps
         self._fout.write(b'%d\n' % len(rsp))
         self._fout.write(rsp)
diff --git a/mercurial/wireprotoserver.py b/mercurial/wireprotoserver.py
--- a/mercurial/wireprotoserver.py
+++ b/mercurial/wireprotoserver.py
@@ -336,19 +336,11 @@ 
 
     return ''
 
-class sshserver(baseprotocolhandler):
-    def __init__(self, ui, repo):
+class sshprotocolhandler(baseprotocolhandler):
+    def __init__(self, ui, fin, fout):
         self._ui = ui
-        self._repo = repo
-        self._fin = ui.fin
-        self._fout = ui.fout
-
-        hook.redirect(True)
-        ui.fout = repo.ui.fout = ui.ferr
-
-        # Prevent insertion/deletion of CRs
-        util.setbinary(self._fin)
-        util.setbinary(self._fout)
+        self._fin = fin
+        self._fout = fout
 
     @property
     def name(self):
@@ -409,11 +401,6 @@ 
         self._fout.write('\n')
         self._fout.flush()
 
-    def serve_forever(self):
-        while self.serve_one():
-            pass
-        sys.exit(0)
-
     _handlers = {
         str: _sendresponse,
         wireproto.streamres: _sendstream,
@@ -423,15 +410,38 @@ 
         wireproto.ooberror: _sendooberror,
     }
 
-    def serve_one(self):
-        cmd = self._fin.readline()[:-1]
-        if cmd and wireproto.commands.commandavailable(cmd, self):
-            rsp = wireproto.dispatch(self._repo, self, cmd)
-            self._handlers[rsp.__class__](self, rsp)
-        elif cmd:
-            self._sendresponse("")
-        return cmd != ''
-
     def _client(self):
         client = encoding.environ.get('SSH_CLIENT', '').split(' ', 1)[0]
         return 'remote:ssh:' + client
+
+class sshserver(object):
+    def __init__(self, ui, repo):
+        self._ui = ui
+        self._repo = repo
+        self._fin = ui.fin
+        self._fout = ui.fout
+
+        hook.redirect(True)
+        ui.fout = repo.ui.fout = ui.ferr
+
+        # Prevent insertion/deletion of CRs
+        util.setbinary(self._fin)
+        util.setbinary(self._fout)
+
+        self._proto = sshprotocolhandler(self._ui, self._fin, self._fout)
+
+    def serve_forever(self):
+        while self.serve_one():
+            pass
+        sys.exit(0)
+
+    def serve_one(self):
+        # TODO improve boundary between transport layer and protocol handler.
+        cmd = self._fin.readline()[:-1]
+        if cmd and wireproto.commands.commandavailable(cmd, self._proto):
+            rsp = wireproto.dispatch(self._repo, self._proto, cmd)
+            self._proto._handlers[rsp.__class__](self._proto, rsp)
+        elif cmd:
+            self._proto._sendresponse("")
+
+        return cmd != ''