This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3204

wireproto: port pushkey command to wire protocol version 2
ClosedPublic

Authored by indygreg on Apr 9 2018, 2:58 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGbe5d4749edc0: wireproto: port pushkey command to wire protocol version 2
Summary

It doesn't do output redirection yet. And I'd love to generally overhaul
the pushkey protocol for wire protocol version 2. But this will be a bit
of effort. Let's do it as a follow-up.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Apr 9 2018, 2:58 PM
durin42 added a subscriber: durin42.Apr 9 2018, 6:27 PM
durin42 added inline comments.
tests/test-wireproto-command-pushkey.t
1

You can't fuse this with an existing test to avoid overhead of creating a repo etc? The testsuite has started growing precipitously again with a lot of narrowly-focussed tests, and I wonder if we should try and be a little more unified about having a single big "test wireprotov2".t file...

indygreg added inline comments.Apr 9 2018, 7:21 PM
tests/test-wireproto-command-pushkey.t
1

And at the same time having giant test files makes it difficult to refactor an existing test file because tests below change due to differences above. You end up doing all kinds of strips, etc to undo changes so you don't have to change the entire test file.

Also, is there evidence that repo creation overhead is a problem? We're not doing fsync() as part of repo creation. I'm finding it difficult to believe that repo creation or .t file existence is a big deal. I understand the theoretically concern. But for developers (who presumably have reasonable I/O on development machines), I'd expect Python + hg process startup overhead to dwarf all other performance concerns.

durin42 added inline comments.Apr 11 2018, 12:28 PM
tests/test-wireproto-command-pushkey.t
1

We're going to go ahead with this as written now, but everyone should be aware that we're approaching a crisis of sorts when it comes to test execution time, especially on non-Linux platforms. We've had people reporting -j8 runs on macOS taking well over an hour.

I hope to find some time to tackle this in the next cycle.

Closed by commit rHGbe5d4749edc0: wireproto: port pushkey command to wire protocol version 2 (authored by indygreg). · Explain WhyApr 11 2018, 12:34 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 7908

mercurial/help/internals/wireprotocol.txt

The command receives the following arguments: The command receives the following arguments:
namespace namespace
(bytestring) Pushkey namespace to query. (bytestring) Pushkey namespace to query.
The response is a map with bytestring keys and values. The response is a map with bytestring keys and values.
TODO consider using binary to represent nodes in certain pushkey namespaces. TODO consider using binary to represent nodes in certain pushkey namespaces.
pushkey
-------
Set a value using the ``pushkey`` protocol.
The command receives the following arguments:
namespace
(bytestring) Pushkey namespace to operate on.
key
(bytestring) The pushkey key to set.
old
(bytestring) Old value for this key.
new
(bytestring) New value for this key.
TODO consider using binary to represent nodes is certain pushkey namespaces.
TODO better define response type and meaning.

mercurial/wireproto.py

@wireprotocommand('protocaps', 'caps', permission='pull', @wireprotocommand('protocaps', 'caps', permission='pull',
transportpolicy=POLICY_V1_ONLY) transportpolicy=POLICY_V1_ONLY)
def protocaps(repo, proto, caps): def protocaps(repo, proto, caps):
if proto.name == wireprototypes.SSHV1: if proto.name == wireprototypes.SSHV1:
proto._protocaps = set(caps.split(' ')) proto._protocaps = set(caps.split(' '))
return wireprototypes.bytesresponse('OK') return wireprototypes.bytesresponse('OK')
@wireprotocommand('pushkey', 'namespace key old new', permission='push') @wireprotocommand('pushkey', 'namespace key old new', permission='push',
transportpolicy=POLICY_V1_ONLY)
def pushkey(repo, proto, namespace, key, old, new): def pushkey(repo, proto, namespace, key, old, new):
# compatibility with pre-1.8 clients which were accidentally # compatibility with pre-1.8 clients which were accidentally
# sending raw binary nodes rather than utf-8-encoded hex # sending raw binary nodes rather than utf-8-encoded hex
if len(new) == 20 and stringutil.escapestr(new) != new: if len(new) == 20 and stringutil.escapestr(new) != new:
# looks like it could be a binary node # looks like it could be a binary node
try: try:
new.decode('utf-8') new.decode('utf-8')
new = encoding.tolocal(new) # but cleanly decodes as UTF-8 new = encoding.tolocal(new) # but cleanly decodes as UTF-8
permission='pull', permission='pull',
transportpolicy=POLICY_V2_ONLY) transportpolicy=POLICY_V2_ONLY)
def listkeysv2(repo, proto, namespace=None): def listkeysv2(repo, proto, namespace=None):
keys = repo.listkeys(encoding.tolocal(namespace)) keys = repo.listkeys(encoding.tolocal(namespace))
keys = {encoding.fromlocal(k): encoding.fromlocal(v) keys = {encoding.fromlocal(k): encoding.fromlocal(v)
for k, v in keys.iteritems()} for k, v in keys.iteritems()}
return wireprototypes.cborresponse(keys) return wireprototypes.cborresponse(keys)
@wireprotocommand('pushkey',
args={
'namespace': b'ns',
'key': b'key',
'old': b'old',
'new': b'new',
},
permission='push',
transportpolicy=POLICY_V2_ONLY)
def pushkeyv2(repo, proto, namespace, key, old, new):
# TODO handle ui output redirection
r = repo.pushkey(encoding.tolocal(namespace),
encoding.tolocal(key),
encoding.tolocal(old),
encoding.tolocal(new))
return wireprototypes.cborresponse(r)

tests/test-wireproto-command-capabilities.t

s> HTTP/1.1 200 OK\r\n s> HTTP/1.1 200 OK\r\n
s> Server: testing stub value\r\n s> Server: testing stub value\r\n
s> Date: $HTTP_DATE$\r\n s> Date: $HTTP_DATE$\r\n
s> Content-Type: application/mercurial-exp-framing-0003\r\n s> Content-Type: application/mercurial-exp-framing-0003\r\n
s> Transfer-Encoding: chunked\r\n s> Transfer-Encoding: chunked\r\n
s> \r\n s> \r\n
s> *\r\n (glob) s> *\r\n (glob)
s> *\x00\x01\x00\x02\x01F (glob) s> *\x00\x01\x00\x02\x01F (glob)
s> \xa2Hcommands\xa9Eheads\xa2Dargs\xa1Jpubliconly\xf4Kpermissions\x81DpullEknown\xa2Dargs\xa1Enodes\x81HdeadbeefKpermissions\x81DpullFlookup\xa2Dargs\xa1CkeyFlegacyKpermissions\x81DpullGpushkey\xa2Dargs\xa4CkeyFlegacyCnewFlegacyColdFlegacyInamespaceFlegacyKpermissions\x81DpushHlistkeys\xa2Dargs\xa1InamespaceBnsKpermissions\x81DpullHunbundle\xa2Dargs\xa1EheadsFlegacyKpermissions\x81DpushIbranchmap\xa2Dargs\xa0Kpermissions\x81DpullIgetbundle\xa2Dargs\xa1A*FlegacyKpermissions\x81DpullLcapabilities\xa2Dargs\xa0Kpermissions\x81DpullKcompression\x82\xa1DnameDzstd\xa1DnameDzlib s> \xa2Hcommands\xa9Eheads\xa2Dargs\xa1Jpubliconly\xf4Kpermissions\x81DpullEknown\xa2Dargs\xa1Enodes\x81HdeadbeefKpermissions\x81DpullFlookup\xa2Dargs\xa1CkeyFlegacyKpermissions\x81DpullGpushkey\xa2Dargs\xa4CkeyCkeyCnewCnewColdColdInamespaceBnsKpermissions\x81DpullHlistkeys\xa2Dargs\xa1InamespaceBnsKpermissions\x81DpullHunbundle\xa2Dargs\xa1EheadsFlegacyKpermissions\x81DpushIbranchmap\xa2Dargs\xa0Kpermissions\x81DpullIgetbundle\xa2Dargs\xa1A*FlegacyKpermissions\x81DpullLcapabilities\xa2Dargs\xa0Kpermissions\x81DpullKcompression\x82\xa1DnameDzstd\xa1DnameDzlib
s> \r\n s> \r\n
received frame(size=*; request=1; stream=2; streamflags=stream-begin; type=bytes-response; flags=eos|cbor) (glob) received frame(size=*; request=1; stream=2; streamflags=stream-begin; type=bytes-response; flags=eos|cbor) (glob)
s> 0\r\n s> 0\r\n
s> \r\n s> \r\n
response: [{b'commands': {b'branchmap': {b'args': {}, b'permissions': [b'pull']}, b'capabilities': {b'args': {}, b'permissions': [b'pull']}, b'getbundle': {b'args': {b'*': b'legacy'}, b'permissions': [b'pull']}, b'heads': {b'args': {b'publiconly': False}, b'permissions': [b'pull']}, b'known': {b'args': {b'nodes': [b'deadbeef']}, b'permissions': [b'pull']}, b'listkeys': {b'args': {b'namespace': b'ns'}, b'permissions': [b'pull']}, b'lookup': {b'args': {b'key': b'legacy'}, b'permissions': [b'pull']}, b'pushkey': {b'args': {b'key': b'legacy', b'namespace': b'legacy', b'new': b'legacy', b'old': b'legacy'}, b'permissions': [b'push']}, b'unbundle': {b'args': {b'heads': b'legacy'}, b'permissions': [b'push']}}, b'compression': [{b'name': b'zstd'}, {b'name': b'zlib'}]}] response: [{b'commands': {b'branchmap': {b'args': {}, b'permissions': [b'pull']}, b'capabilities': {b'args': {}, b'permissions': [b'pull']}, b'getbundle': {b'args': {b'*': b'legacy'}, b'permissions': [b'pull']}, b'heads': {b'args': {b'publiconly': False}, b'permissions': [b'pull']}, b'known': {b'args': {b'nodes': [b'deadbeef']}, b'permissions': [b'pull']}, b'listkeys': {b'args': {b'namespace': b'ns'}, b'permissions': [b'pull']}, b'lookup': {b'args': {b'key': b'legacy'}, b'permissions': [b'pull']}, b'pushkey': {b'args': {b'key': b'key', b'namespace': b'ns', b'new': b'new', b'old': b'old'}, b'permissions': [b'pull']}, b'unbundle': {b'args': {b'heads': b'legacy'}, b'permissions': [b'push']}}, b'compression': [{b'name': b'zstd'}, {b'name': b'zlib'}]}]
$ cat error.log $ cat error.log

tests/test-wireproto-command-pushkey.t

  • This file was added.
$ . $TESTDIR/wireprotohelpers.sh
durin42Unsubmitted
Not Done

You can't fuse this with an existing test to avoid overhead of creating a repo etc? The testsuite has started growing precipitously again with a lot of narrowly-focussed tests, and I wonder if we should try and be a little more unified about having a single big "test wireprotov2".t file...

durin42: You can't fuse this with an existing test to avoid overhead of creating a repo etc? The…
indygregAuthorUnsubmitted
Not Done

And at the same time having giant test files makes it difficult to refactor an existing test file because tests below change due to differences above. You end up doing all kinds of strips, etc to undo changes so you don't have to change the entire test file.

Also, is there evidence that repo creation overhead is a problem? We're not doing fsync() as part of repo creation. I'm finding it difficult to believe that repo creation or .t file existence is a big deal. I understand the theoretically concern. But for developers (who presumably have reasonable I/O on development machines), I'd expect Python + hg process startup overhead to dwarf all other performance concerns.

indygreg: And at the same time having giant test files makes it difficult to refactor an existing test…
durin42Unsubmitted
Not Done

We're going to go ahead with this as written now, but everyone should be aware that we're approaching a crisis of sorts when it comes to test execution time, especially on non-Linux platforms. We've had people reporting -j8 runs on macOS taking well over an hour.

I hope to find some time to tackle this in the next cycle.

durin42: We're going to go ahead with this as written now, but everyone should be aware that we're…
$ hg init server
$ enablehttpv2 server
$ cd server
$ cat >> .hg/hgrc << EOF
> [web]
> push_ssl = false
> allow-push = *
> EOF
$ hg debugdrawdag << EOF
> C D
> |/
> B
> |
> A
> EOF
$ hg serve -p $HGPORT -d --pid-file hg.pid -E error.log
$ cat hg.pid > $DAEMON_PIDS
pushkey for a bookmark works
$ sendhttpv2peer << EOF
> command pushkey
> namespace bookmarks
> key @
> old
> new 426bada5c67598ca65036d57d9e4b64b0c1ce7a0
> EOF
creating http peer for wire protocol version 2
sending pushkey command
s> *\r\n (glob)
s> Accept-Encoding: identity\r\n
s> accept: application/mercurial-exp-framing-0003\r\n
s> content-type: application/mercurial-exp-framing-0003\r\n
s> content-length: 105\r\n
s> host: $LOCALIP:$HGPORT\r\n (glob)
s> user-agent: Mercurial debugwireproto\r\n
s> \r\n
s> a\x00\x00\x01\x00\x01\x01\x11\xa2Dargs\xa4CkeyA@CnewX(426bada5c67598ca65036d57d9e4b64b0c1ce7a0Cold@InamespaceIbookmarksDnameGpushkey
s> makefile('rb', None)
s> HTTP/1.1 200 OK\r\n
s> Server: testing stub value\r\n
s> Date: $HTTP_DATE$\r\n
s> Content-Type: application/mercurial-exp-framing-0003\r\n
s> Transfer-Encoding: chunked\r\n
s> \r\n
s> 9\r\n
s> *\x00\x01\x00\x02\x01F (glob)
s> \xf5
s> \r\n
received frame(size=*; request=1; stream=2; streamflags=stream-begin; type=bytes-response; flags=eos|cbor) (glob)
s> 0\r\n
s> \r\n
response: []
$ sendhttpv2peer << EOF
> command listkeys
> namespace bookmarks
> EOF
creating http peer for wire protocol version 2
sending listkeys command
s> POST /api/exp-http-v2-0001/ro/listkeys HTTP/1.1\r\n
s> Accept-Encoding: identity\r\n
s> accept: application/mercurial-exp-framing-0003\r\n
s> content-type: application/mercurial-exp-framing-0003\r\n
s> content-length: 49\r\n
s> host: $LOCALIP:$HGPORT\r\n (glob)
s> user-agent: Mercurial debugwireproto\r\n
s> \r\n
s> )\x00\x00\x01\x00\x01\x01\x11\xa2Dargs\xa1InamespaceIbookmarksDnameHlistkeys
s> makefile('rb', None)
s> HTTP/1.1 200 OK\r\n
s> Server: testing stub value\r\n
s> Date: $HTTP_DATE$\r\n
s> Content-Type: application/mercurial-exp-framing-0003\r\n
s> Transfer-Encoding: chunked\r\n
s> \r\n
s> 35\r\n
s> -\x00\x00\x01\x00\x02\x01F
s> \xa1A@X(426bada5c67598ca65036d57d9e4b64b0c1ce7a0
s> \r\n
received frame(size=45; request=1; stream=2; streamflags=stream-begin; type=bytes-response; flags=eos|cbor)
s> 0\r\n
s> \r\n
response: [{b'@': b'426bada5c67598ca65036d57d9e4b64b0c1ce7a0'}]
$ cat error.log