| indygreg |
| hg-reviewers |
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.
| Lint Skipped |
| Unit Tests Skipped |
| mercurial/sshserver.py | ||
|---|---|---|
| 74 | 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. | |
| mercurial/sshserver.py | ||
|---|---|---|
| 74 | See bf2db35a6fe7 for an example of # no-check-commit. | |
| mercurial/sshserver.py | ||
|---|---|---|
| 74 | 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. | |
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.
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.
| mercurial/help/internals/wireprotocol.txt | ||
|---|---|---|
| 391–393 ↗ | (On Diff #7505) | 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. |
| mercurial/wireproto.py | ||
| 783–785 | The protocol handler class in wireprotoserver.py now has an addcapabilities() that should be used for adding transport-specific capabilities. Please use it. | |
| 953 | Please define this as transportpolicy=POLICY_V1_ONLY. Also, please add documentation for the new command to wireprotocol.txt. | |
| 955–956 | 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. | |
| mercurial/wireprotoserver.py | ||
| 586 ↗ | (On Diff #7505) | Please remove this, as we won't carry this implementation forward to version 2. |
| 603–606 ↗ | (On Diff #7505) | And have this return an empty set for now. |
This looks great! It adds some much needed parity between the existing SSH and HTTP transports.
| Path | Packages | |||
|---|---|---|---|---|
| M | mercurial/hgweb/protocol.py (14 lines) | |||
| M | mercurial/sshpeer.py (21 lines) | |||
| M | mercurial/sshserver.py (15 lines) | |||
| M | mercurial/wireproto.py (14 lines) | |||
| M | tests/test-ssh-bundle1.t (5 lines) | |||
| M | tests/test-ssh.t (7 lines) |
| return ''.join(chunks) | return ''.join(chunks) | ||||
| class webproto(wireproto.abstractserverproto): | class webproto(wireproto.abstractserverproto): | ||||
| def __init__(self, req, ui): | def __init__(self, req, ui): | ||||
| self.req = req | self.req = req | ||||
| self.response = '' | self.response = '' | ||||
| self.ui = ui | self.ui = ui | ||||
| self.name = 'http' | self.name = 'http' | ||||
| self._protocaps = None | |||||
| def getargs(self, args): | def getargs(self, args): | ||||
| knownargs = self._args() | knownargs = self._args() | ||||
| data = {} | data = {} | ||||
| keys = args.split() | keys = args.split() | ||||
| for k in keys: | for k in keys: | ||||
| if k == '*': | if k == '*': | ||||
| star = {} | star = {} | ||||
| return args | return args | ||||
| def getfile(self, fp): | def getfile(self, fp): | ||||
| length = int(self.req.env[r'CONTENT_LENGTH']) | length = int(self.req.env[r'CONTENT_LENGTH']) | ||||
| # If httppostargs is used, we need to read Content-Length | # If httppostargs is used, we need to read Content-Length | ||||
| # minus the amount that was consumed by args. | # minus the amount that was consumed by args. | ||||
| length -= int(self.req.env.get(r'HTTP_X_HGARGS_POST', 0)) | length -= int(self.req.env.get(r'HTTP_X_HGARGS_POST', 0)) | ||||
| for s in util.filechunkiter(self.req, limit=length): | for s in util.filechunkiter(self.req, limit=length): | ||||
| fp.write(s) | 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): | def redirect(self): | ||||
| self.oldio = self.ui.fout, self.ui.ferr | self.oldio = self.ui.fout, self.ui.ferr | ||||
| self.ui.ferr = self.ui.fout = stringio() | self.ui.ferr = self.ui.fout = stringio() | ||||
| def restore(self): | def restore(self): | ||||
| val = self.ui.fout.getvalue() | val = self.ui.fout.getvalue() | ||||
| self.ui.ferr, self.ui.fout = self.oldio | self.ui.ferr, self.ui.fout = self.oldio | ||||
| return val | return val | ||||
| def _client(self): | def _client(self): | ||||
| return 'remote:%s:%s:%s' % ( | return 'remote:%s:%s:%s' % ( | ||||
| self.req.env.get('wsgi.url_scheme') or 'http', | self.req.env.get('wsgi.url_scheme') or 'http', | ||||
| urlreq.quote(self.req.env.get('REMOTE_HOST', '')), | urlreq.quote(self.req.env.get('REMOTE_HOST', '')), | ||||
| urlreq.quote(self.req.env.get('REMOTE_USER', ''))) | urlreq.quote(self.req.env.get('REMOTE_USER', ''))) | ||||
| def responsetype(self, prefer_uncompressed): | def responsetype(self, prefer_uncompressed): | ||||
| """Determine the appropriate response type and compression settings. | """Determine the appropriate response type and compression settings. | ||||
| Returns a tuple of (mediatype, compengine, engineopts). | Returns a tuple of (mediatype, compengine, engineopts). | ||||
| """ | """ | ||||
| # Determine the response media type and compression engine based | # Determine the response media type and compression engine based | ||||
| # on the request parameters. | # on the request parameters. | ||||
| protocaps = decodevaluefromheaders(self.req, r'X-HgProto').split(' ') | protocaps = self.getprotocaps() | ||||
| if '0.2' in protocaps: | if '0.2' in protocaps: | ||||
| # All clients are expected to support uncompressed data. | # All clients are expected to support uncompressed data. | ||||
| if prefer_uncompressed: | if prefer_uncompressed: | ||||
| return HGTYPE2, util._noopengine(), {} | return HGTYPE2, util._noopengine(), {} | ||||
| # Default as defined by wire protocol spec. | # Default as defined by wire protocol spec. | ||||
| compformats = ['zlib', 'none'] | compformats = self.getcompressionsupport() | ||||
| for cap in protocaps: | |||||
| if cap.startswith('comp='): | |||||
| compformats = cap[5:].split(',') | |||||
| break | |||||
| # Now find an agreed upon compression format. | # Now find an agreed upon compression format. | ||||
| for engine in wireproto.supportedcompengines(self.ui, self, | for engine in wireproto.supportedcompengines(self.ui, self, | ||||
| util.SERVERROLE): | util.SERVERROLE): | ||||
| if engine.wireprotosupport().name in compformats: | if engine.wireprotosupport().name in compformats: | ||||
| opts = {} | opts = {} | ||||
| level = self.ui.configint('server', | level = self.ui.configint('server', | ||||
| '%slevel' % engine.name()) | '%slevel' % engine.name()) | ||||
| # sshpeer.py - ssh repository proxy class for mercurial | # sshpeer.py - ssh repository proxy class for mercurial | ||||
| # | # | ||||
| # Copyright 2005, 2006 Matt Mackall <mpm@selenic.com> | # Copyright 2005, 2006 Matt Mackall <mpm@selenic.com> | ||||
| # | # | ||||
| # This software may be used and distributed according to the terms of the | # This software may be used and distributed according to the terms of the | ||||
| # GNU General Public License version 2 or any later version. | # GNU General Public License version 2 or any later version. | ||||
| from __future__ import absolute_import | from __future__ import absolute_import | ||||
| import re | import re | ||||
| from .i18n import _ | from .i18n import _ | ||||
| from . import ( | from . import ( | ||||
| error, | error, | ||||
| pycompat, | pycompat, | ||||
| sshserver, | |||||
| util, | util, | ||||
| wireproto, | wireproto, | ||||
| ) | ) | ||||
| def _serverquote(s): | def _serverquote(s): | ||||
| """quote a string for the remote shell ... which we assume is sh""" | """quote a string for the remote shell ... which we assume is sh""" | ||||
| if not s: | if not s: | ||||
| return s | return s | ||||
| # Begin of _basewirecommands interface. | # Begin of _basewirecommands interface. | ||||
| def capabilities(self): | def capabilities(self): | ||||
| return self._caps | return self._caps | ||||
| # End of _basewirecommands interface. | # 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): | def _validaterepo(self, sshcmd, args, remotecmd, sshenv=None): | ||||
| # cleanup up previous run | # cleanup up previous run | ||||
| self._cleanup() | self._cleanup() | ||||
| cmd = '%s %s %s' % (sshcmd, args, | cmd = '%s %s %s' % (sshcmd, args, | ||||
| util.shellquote("%s -R %s serve --stdio" % | util.shellquote("%s -R %s serve --stdio" % | ||||
| (_serverquote(remotecmd), _serverquote(self._path)))) | (_serverquote(remotecmd), _serverquote(self._path)))) | ||||
| self.ui.debug('running %s\n' % cmd) | self.ui.debug('running %s\n' % cmd) | ||||
| badresponse() | badresponse() | ||||
| self._caps = set() | self._caps = set() | ||||
| for l in reversed(lines): | for l in reversed(lines): | ||||
| if l.startswith("capabilities:"): | if l.startswith("capabilities:"): | ||||
| self._caps.update(l[:-1].split(":")[1].split()) | self._caps.update(l[:-1].split(":")[1].split()) | ||||
| break | break | ||||
| if 'protocaps' in self._caps: | |||||
| try: | |||||
| self._call("protocaps", | |||||
| caps=' '.join(self._clientcapabilities())) | |||||
| except IOError: | |||||
| badresponse() | |||||
| def _readerr(self): | def _readerr(self): | ||||
| _forwardoutput(self.ui, self._pipee) | _forwardoutput(self.ui, self._pipee) | ||||
| def _abort(self, exception): | def _abort(self, exception): | ||||
| self._cleanup() | self._cleanup() | ||||
| raise exception | raise exception | ||||
| def _cleanup(self): | def _cleanup(self): | ||||
| for key, value in sorted(args.items()): | for key, value in sorted(args.items()): | ||||
| if not isinstance(value, dict): | if not isinstance(value, dict): | ||||
| dbg(line % ' %s: %d bytes' % (key, len(value))) | dbg(line % ' %s: %d bytes' % (key, len(value))) | ||||
| else: | else: | ||||
| for dk, dv in sorted(value.items()): | for dk, dv in sorted(value.items()): | ||||
| dbg(line % ' %s-%s: %d' % (key, dk, len(dv))) | dbg(line % ' %s-%s: %d' % (key, dk, len(dv))) | ||||
| self.ui.debug("sending %s command\n" % cmd) | self.ui.debug("sending %s command\n" % cmd) | ||||
| self._pipeo.write("%s\n" % cmd) | self._pipeo.write("%s\n" % cmd) | ||||
| if cmd in wireproto.commands: | |||||
| _func, names = wireproto.commands[cmd] | _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() | keys = names.split() | ||||
| wireargs = {} | wireargs = {} | ||||
| for k in keys: | for k in keys: | ||||
| if k == '*': | if k == '*': | ||||
| wireargs['*'] = args | wireargs['*'] = args | ||||
| break | break | ||||
| else: | else: | ||||
| wireargs[k] = args[k] | wireargs[k] = args[k] | ||||
| class sshserver(wireproto.abstractserverproto): | class sshserver(wireproto.abstractserverproto): | ||||
| def __init__(self, ui, repo): | def __init__(self, ui, repo): | ||||
| self.ui = ui | self.ui = ui | ||||
| self.repo = repo | self.repo = repo | ||||
| self.lock = None | self.lock = None | ||||
| self.fin = ui.fin | self.fin = ui.fin | ||||
| self.fout = ui.fout | self.fout = ui.fout | ||||
| self.name = 'ssh' | self.name = 'ssh' | ||||
| self._protocaps = [] | |||||
| hook.redirect(True) | hook.redirect(True) | ||||
| ui.fout = repo.ui.fout = ui.ferr | ui.fout = repo.ui.fout = ui.ferr | ||||
| # Prevent insertion/deletion of CRs | # Prevent insertion/deletion of CRs | ||||
| util.setbinary(self.fin) | util.setbinary(self.fin) | ||||
| util.setbinary(self.fout) | util.setbinary(self.fout) | ||||
| def getfile(self, fpout): | def getfile(self, fpout): | ||||
| self.sendresponse('') | self.sendresponse('') | ||||
| count = int(self.fin.readline()) | count = int(self.fin.readline()) | ||||
| while count: | while count: | ||||
| fpout.write(self.fin.read(count)) | fpout.write(self.fin.read(count)) | ||||
| count = int(self.fin.readline()) | count = int(self.fin.readline()) | ||||
| def getprotocaps(self): | |||||
| """return the wireprotocol capabilities of the current request""" | |||||
| return self._protocaps | |||||
| def do_protocaps(self): | |||||
joerg.sonnenberger: So this function triggers the underscore function name check, even if the support for this kind… | |||||
mharbison72Unsubmitted Not Done See bf2db35a6fe7 for an example of # no-check-commit. mharbison72: See bf2db35a6fe7 for an example of # no-check-commit. | |||||
joerg.sonnenbergerAuthorUnsubmitted Not Done 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. joerg.sonnenberger: Thanks, that answers half of the question. This is an internal interface contract, it could be… | |||||
| """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): | def redirect(self): | ||||
| pass | pass | ||||
| def sendresponse(self, v): | def sendresponse(self, v): | ||||
| self.fout.write("%d\n" % len(v)) | self.fout.write("%d\n" % len(v)) | ||||
| self.fout.write(v) | self.fout.write(v) | ||||
| self.fout.flush() | self.fout.flush() | ||||
| debug output | debug output | ||||
| $ hg pull --debug ssh://user@dummy/remote | $ hg pull --debug ssh://user@dummy/remote | ||||
| pulling from ssh://user@dummy/remote | pulling from ssh://user@dummy/remote | ||||
| running .* ".*/dummyssh" ['"]user@dummy['"] ('|")hg -R remote serve --stdio('|") (re) | running .* ".*/dummyssh" ['"]user@dummy['"] ('|")hg -R remote serve --stdio('|") (re) | ||||
| sending hello command | sending hello command | ||||
| sending between command | sending between command | ||||
| remote: 384 | remote: 394 | ||||
| remote: capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch streamreqs=generaldelta,revlogv1 $USUAL_BUNDLE2_CAPS_SERVER$ unbundle=HG10GZ,HG10BZ,HG10UN | remote: capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch streamreqs=generaldelta,revlogv1 $USUAL_BUNDLE2_CAPS_SERVER$ unbundle=HG10GZ,HG10BZ,HG10UN protocaps | ||||
| remote: 1 | remote: 1 | ||||
| sending protocaps command | |||||
| preparing listkeys for "bookmarks" | preparing listkeys for "bookmarks" | ||||
| sending listkeys command | sending listkeys command | ||||
| received listkey for "bookmarks": 45 bytes | received listkey for "bookmarks": 45 bytes | ||||
| query 1; heads | query 1; heads | ||||
| sending batch command | sending batch command | ||||
| searching for changes | searching for changes | ||||
| all remote heads known locally | all remote heads known locally | ||||
| no changes found | no changes found | ||||
| $ hg pull --debug ssh://user@dummy/remote --config devel.debug.peer-request=yes | $ hg pull --debug ssh://user@dummy/remote --config devel.debug.peer-request=yes | ||||
| pulling from ssh://user@dummy/remote | pulling from ssh://user@dummy/remote | ||||
| running .* ".*/dummyssh" ['"]user@dummy['"] ('|")hg -R remote serve --stdio('|") (re) | running .* ".*/dummyssh" ['"]user@dummy['"] ('|")hg -R remote serve --stdio('|") (re) | ||||
| devel-peer-request: hello | devel-peer-request: hello | ||||
| sending hello command | sending hello command | ||||
| devel-peer-request: between | devel-peer-request: between | ||||
| devel-peer-request: pairs: 81 bytes | devel-peer-request: pairs: 81 bytes | ||||
| sending between command | sending between command | ||||
| remote: 384 | remote: 394 | ||||
| remote: capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch streamreqs=generaldelta,revlogv1 $USUAL_BUNDLE2_CAPS_SERVER$ unbundle=HG10GZ,HG10BZ,HG10UN | remote: capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch streamreqs=generaldelta,revlogv1 $USUAL_BUNDLE2_CAPS_SERVER$ unbundle=HG10GZ,HG10BZ,HG10UN protocaps | ||||
| remote: 1 | remote: 1 | ||||
| devel-peer-request: protocaps | |||||
| devel-peer-request: caps: 25 bytes | |||||
| sending protocaps command | |||||
| query 1; heads | query 1; heads | ||||
| devel-peer-request: batch | devel-peer-request: batch | ||||
| devel-peer-request: cmds: 141 bytes | devel-peer-request: cmds: 141 bytes | ||||
| sending batch command | sending batch command | ||||
| searching for changes | searching for changes | ||||
| all remote heads known locally | all remote heads known locally | ||||
| no changes found | no changes found | ||||
| devel-peer-request: getbundle | devel-peer-request: getbundle | ||||
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.