Patchwork D2385: wireproto: document the wonky push protocol for SSH

login
register
mail settings
Submitter phabricator
Date Feb. 22, 2018, 3:23 a.m.
Message ID <differential-rev-PHID-DREV-bfxcxcg37dxp6ioay6f3-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/28228/
State Superseded
Headers show

Comments

phabricator - Feb. 22, 2018, 3:23 a.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  It took me several minutes to figure out how the "unbundle"
  protocol worked. It turns out that the SSH protocol handler
  sends an empty reply that is interpreted as "OK to send" and
  only then does the client send the bundle payload.
  
  On top of that, the response is different depending on whether
  the operation was successful or not. I nearly pulled out my hair
  deciphering this.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/sshpeer.py
  mercurial/wireprotoserver.py

CHANGE DETAILS




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

Patch

diff --git a/mercurial/wireprotoserver.py b/mercurial/wireprotoserver.py
--- a/mercurial/wireprotoserver.py
+++ b/mercurial/wireprotoserver.py
@@ -347,12 +347,16 @@ 
         return [data[k] for k in keys]
 
     def forwardpayload(self, fpout):
+        # We initially send an empty response. This tells the client it is
+        # OK to start sending data. If a client sees any other response, it
+        # interprets it as an error.
+        _sshv1respondbytes(self._fout, b'')
+
         # The file is in the form:
         #
         # <chunk size>\n<chunk>
         # ...
         # 0\n
-        _sshv1respondbytes(self._fout, b'')
         count = int(self._fin.readline())
         while count:
             fpout.write(self._fin.read(count))
diff --git a/mercurial/sshpeer.py b/mercurial/sshpeer.py
--- a/mercurial/sshpeer.py
+++ b/mercurial/sshpeer.py
@@ -466,25 +466,40 @@ 
         return self._sendrequest(cmd, args, framed=True).read()
 
     def _callpush(self, cmd, fp, **args):
+        # The server responds with an empty frame if the client should
+        # continue submitting the payload.
         r = self._call(cmd, **args)
         if r:
             return '', r
+
+        # The payload consists of frames with content followed by an empty
+        # frame.
         for d in iter(lambda: fp.read(4096), ''):
             self._writeframed(d)
         self._writeframed("", flush=True)
+
+        # In case of success, there is an empty frame and a frame containing
+        # the integer result (as a string).
+        # In case of error, there is a non-empty frame containing the error.
         r = self._readframed()
         if r:
             return '', r
         return self._readframed(), ''
 
     def _calltwowaystream(self, cmd, fp, **args):
+        # The server responds with an empty frame if the client should
+        # continue submitting the payload.
         r = self._call(cmd, **args)
         if r:
             # XXX needs to be made better
             raise error.Abort(_('unexpected remote reply: %s') % r)
+
+        # The payload consists of frames with content followed by an empty
+        # frame.
         for d in iter(lambda: fp.read(4096), ''):
             self._writeframed(d)
         self._writeframed("", flush=True)
+
         return self._pipei
 
     def _getamount(self):