Patchwork D3204: wireproto: port pushkey command to wire protocol version 2

login
register
mail settings
Submitter phabricator
Date April 9, 2018, 6:58 p.m.
Message ID <differential-rev-PHID-DREV-z7xdcka52y3tbjlhjvoq-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/30606/
State Superseded
Headers show

Comments

phabricator - April 9, 2018, 6:58 p.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

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

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/help/internals/wireprotocol.txt
  mercurial/wireproto.py
  tests/test-wireproto-command-capabilities.t
  tests/test-wireproto-command-pushkey.t

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - April 9, 2018, 10:27 p.m.
durin42 added inline comments.

INLINE COMMENTS

> test-wireproto-command-pushkey.t:1
> +  $ . $TESTDIR/wireprotohelpers.sh
> +

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

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - April 9, 2018, 11:21 p.m.
indygreg added inline comments.

INLINE COMMENTS

> durin42 wrote in 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...

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.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - April 11, 2018, 4:28 p.m.
durin42 added inline comments.

INLINE COMMENTS

> indygreg wrote in 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.

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.

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/tests/test-wireproto-command-pushkey.t b/tests/test-wireproto-command-pushkey.t
new file mode 100644
--- /dev/null
+++ b/tests/test-wireproto-command-pushkey.t
@@ -0,0 +1,89 @@ 
+  $ . $TESTDIR/wireprotohelpers.sh
+
+  $ 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
diff --git a/tests/test-wireproto-command-capabilities.t b/tests/test-wireproto-command-capabilities.t
--- a/tests/test-wireproto-command-capabilities.t
+++ b/tests/test-wireproto-command-capabilities.t
@@ -30,11 +30,11 @@ 
   s>     \r\n
   s>     *\r\n (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
   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: [{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
diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
--- a/mercurial/wireproto.py
+++ b/mercurial/wireproto.py
@@ -1073,7 +1073,8 @@ 
         proto._protocaps = set(caps.split(' '))
     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):
     # compatibility with pre-1.8 clients which were accidentally
     # sending raw binary nodes rather than utf-8-encoded hex
@@ -1304,3 +1305,21 @@ 
             for k, v in keys.iteritems()}
 
     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)
diff --git a/mercurial/help/internals/wireprotocol.txt b/mercurial/help/internals/wireprotocol.txt
--- a/mercurial/help/internals/wireprotocol.txt
+++ b/mercurial/help/internals/wireprotocol.txt
@@ -1767,3 +1767,22 @@ 
 The response is a map with bytestring keys and values.
 
 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.