D4615: wireprotov2: declare command arguments richly

indygreg (Gregory Szorc) phabricator at mercurial-scm.org
Mon Sep 17 21:24:42 UTC 2018


indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Previously, we declared command arguments with an example of
  their value. After this commit, we declare command arguments
  as a dict of metadata. This allows us to define the value
  type, whether the argument is required, and provide default
  values. This in turn allows us to have nice things, such as
  less boilerplate code in individual commands for validating
  input and assigning default values. It should also make command
  behavior more consistent as a result.
  
  Test output changed slightly because I realized that the "fields"
  argument wasn't being consistently defined as a set. Oops!
  
  Other test output changed because of slight differences in code
  performing type validation.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/wireprotov2server.py
  tests/test-http-protocol.t
  tests/test-wireproto-command-capabilities.t
  tests/test-wireproto-command-filedata.t
  tests/test-wireproto-command-manifestdata.t

CHANGE DETAILS

diff --git a/tests/test-wireproto-command-manifestdata.t b/tests/test-wireproto-command-manifestdata.t
--- a/tests/test-wireproto-command-manifestdata.t
+++ b/tests/test-wireproto-command-manifestdata.t
@@ -65,14 +65,14 @@
   s>     Content-Type: application/mercurial-exp-framing-0005\r\n
   s>     Transfer-Encoding: chunked\r\n
   s>     \r\n
-  s>     45\r\n
-  s>     =\x00\x00\x01\x00\x02\x012
-  s>     \xa2Eerror\xa1GmessageX\x1enodes argument must be definedFstatusEerror
+  s>     4e\r\n
+  s>     F\x00\x00\x01\x00\x02\x012
+  s>     \xa2Eerror\xa1GmessageX\'missing required arguments: nodes, treeFstatusEerror
   s>     \r\n
-  received frame(size=61; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos)
+  received frame(size=70; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos)
   s>     0\r\n
   s>     \r\n
-  abort: nodes argument must be defined!
+  abort: missing required arguments: nodes, tree!
   [255]
 
   $ sendhttpv2peer << EOF
@@ -97,14 +97,14 @@
   s>     Content-Type: application/mercurial-exp-framing-0005\r\n
   s>     Transfer-Encoding: chunked\r\n
   s>     \r\n
-  s>     44\r\n
-  s>     <\x00\x00\x01\x00\x02\x012
-  s>     \xa2Eerror\xa1GmessageX\x1dtree argument must be definedFstatusEerror
+  s>     47\r\n
+  s>     ?\x00\x00\x01\x00\x02\x012
+  s>     \xa2Eerror\xa1GmessageX missing required arguments: treeFstatusEerror
   s>     \r\n
-  received frame(size=60; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos)
+  received frame(size=63; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos)
   s>     0\r\n
   s>     \r\n
-  abort: tree argument must be defined!
+  abort: missing required arguments: tree!
   [255]
 
 Unknown node is an error
diff --git a/tests/test-wireproto-command-filedata.t b/tests/test-wireproto-command-filedata.t
--- a/tests/test-wireproto-command-filedata.t
+++ b/tests/test-wireproto-command-filedata.t
@@ -69,14 +69,14 @@
   s>     Content-Type: application/mercurial-exp-framing-0005\r\n
   s>     Transfer-Encoding: chunked\r\n
   s>     \r\n
-  s>     45\r\n
-  s>     =\x00\x00\x01\x00\x02\x012
-  s>     \xa2Eerror\xa1GmessageX\x1enodes argument must be definedFstatusEerror
+  s>     4e\r\n
+  s>     F\x00\x00\x01\x00\x02\x012
+  s>     \xa2Eerror\xa1GmessageX\'missing required arguments: nodes, pathFstatusEerror
   s>     \r\n
-  received frame(size=61; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos)
+  received frame(size=70; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos)
   s>     0\r\n
   s>     \r\n
-  abort: nodes argument must be defined!
+  abort: missing required arguments: nodes, path!
   [255]
 
   $ sendhttpv2peer << EOF
@@ -101,14 +101,14 @@
   s>     Content-Type: application/mercurial-exp-framing-0005\r\n
   s>     Transfer-Encoding: chunked\r\n
   s>     \r\n
-  s>     44\r\n
-  s>     <\x00\x00\x01\x00\x02\x012
-  s>     \xa2Eerror\xa1GmessageX\x1dpath argument must be definedFstatusEerror
+  s>     47\r\n
+  s>     ?\x00\x00\x01\x00\x02\x012
+  s>     \xa2Eerror\xa1GmessageX missing required arguments: pathFstatusEerror
   s>     \r\n
-  received frame(size=60; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos)
+  received frame(size=63; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos)
   s>     0\r\n
   s>     \r\n
-  abort: path argument must be defined!
+  abort: missing required arguments: path!
   [255]
 
 Unknown node is an error
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
@@ -212,7 +212,7 @@
   s>     Content-Type: application/mercurial-cbor\r\n
   s>     Content-Length: *\r\n (glob)
   s>     \r\n
-  s>     \xa3GapibaseDapi/Dapis\xa1Pexp-http-v2-0001\xa4Hcommands\xaaIbranchmap\xa2Dargs\xa0Kpermissions\x81DpullLcapabilities\xa2Dargs\xa0Kpermissions\x81DpullMchangesetdata\xa2Dargs\xa3Ffields\xd9\x01\x02\x82GparentsHrevisionInoderange\x82\x81J0123456...\x81Iabcdef...Enodes\x81J0123456...Kpermissions\x81DpullHfiledata\xa2Dargs\xa4Ffields\x82GparentsHrevisionKhaveparents\xf5Enodes\x81J0123456...DpathGfoo.txtKpermissions\x81DpullEheads\xa2Dargs\xa1Jpubliconly\xf4Kpermissions\x81DpullEknown\xa2Dargs\xa1Enodes\x81HdeadbeefKpermissions\x81DpullHlistkeys\xa2Dargs\xa1InamespaceBnsKpermissions\x81DpullFlookup\xa2Dargs\xa1CkeyCfooKpermissions\x81DpullLmanifestdata\xa2Dargs\xa4Ffields\x82GparentsHrevisionKhaveparents\xf5Enodes\x81J0123456...Dtree at Kpermissions\x81DpullGpushkey\xa2Dargs\xa4CkeyCkeyInamespaceBnsCnewCnewColdColdKpermissions\x81DpushKcompression\x81\xa1DnameDzlibQframingmediatypes\x81X&application/mercurial-exp-framing-0005Nrawrepoformats\x82LgeneraldeltaHrevlogv1Nv1capabilitiesY\x01\xc5batch branchmap $USUAL_BUNDLE2_CAPS_SERVER$ changegroupsubset compression=$BUNDLE2_COMPRESSIONS$ getbundle httpheader=1024 httpmediatype=0.1rx,0.1tx,0.2tx known lookup pushkey streamreqs=generaldelta,revlogv1 unbundle=HG10GZ,HG10BZ,HG10UN unbundlehash
+  s>     \xa3GapibaseDapi/Dapis\xa1Pexp-http-v2-0001\xa4Hcommands\xaaIbranchmap\xa2Dargs\xa0Kpermissions\x81DpullLcapabilities\xa2Dargs\xa0Kpermissions\x81DpullMchangesetdata\xa2Dargs\xa3Ffields\xd9\x01\x02\x82GparentsHrevisionInoderange\x82\x81J0123456...\x81Iabcdef...Enodes\x81J0123456...Kpermissions\x81DpullHfiledata\xa2Dargs\xa4Ffields\xd9\x01\x02\x82GparentsHrevisionKhaveparents\xf5Enodes\x81J0123456...DpathGfoo.txtKpermissions\x81DpullEheads\xa2Dargs\xa1Jpubliconly\xf4Kpermissions\x81DpullEknown\xa2Dargs\xa1Enodes\x81HdeadbeefKpermissions\x81DpullHlistkeys\xa2Dargs\xa1InamespaceBnsKpermissions\x81DpullFlookup\xa2Dargs\xa1CkeyCfooKpermissions\x81DpullLmanifestdata\xa2Dargs\xa4Ffields\xd9\x01\x02\x82GparentsHrevisionKhaveparents\xf5Enodes\x81J0123456...Dtree at Kpermissions\x81DpullGpushkey\xa2Dargs\xa4CkeyCkeyInamespaceBnsCnewCnewColdColdKpermissions\x81DpushKcompression\x81\xa1DnameDzlibQframingmediatypes\x81X&application/mercurial-exp-framing-0005Nrawrepoformats\x82LgeneraldeltaHrevlogv1Nv1capabilitiesY\x01\xc5batch branchmap $USUAL_BUNDLE2_CAPS_SERVER$ changegroupsubset compression=$BUNDLE2_COMPRESSIONS$ getbundle httpheader=1024 httpmediatype=0.1rx,0.1tx,0.2tx known lookup pushkey streamreqs=generaldelta,revlogv1 unbundle=HG10GZ,HG10BZ,HG10UN unbundlehash
   cbor> {
     b'apibase': b'api/',
     b'apis': {
@@ -254,10 +254,10 @@
           },
           b'filedata': {
             b'args': {
-              b'fields': [
+              b'fields': set([
                 b'parents',
                 b'revision'
-              ],
+              ]),
               b'haveparents': True,
               b'nodes': [
                 b'0123456...'
@@ -304,10 +304,10 @@
           },
           b'manifestdata': {
             b'args': {
-              b'fields': [
+              b'fields': set([
                 b'parents',
                 b'revision'
-              ],
+              ]),
               b'haveparents': True,
               b'nodes': [
                 b'0123456...'
@@ -369,7 +369,7 @@
   s>     Content-Type: application/mercurial-cbor\r\n
   s>     Content-Length: *\r\n (glob)
   s>     \r\n
-  s>     \xa3GapibaseDapi/Dapis\xa1Pexp-http-v2-0001\xa4Hcommands\xaaIbranchmap\xa2Dargs\xa0Kpermissions\x81DpullLcapabilities\xa2Dargs\xa0Kpermissions\x81DpullMchangesetdata\xa2Dargs\xa3Ffields\xd9\x01\x02\x82GparentsHrevisionInoderange\x82\x81J0123456...\x81Iabcdef...Enodes\x81J0123456...Kpermissions\x81DpullHfiledata\xa2Dargs\xa4Ffields\x82GparentsHrevisionKhaveparents\xf5Enodes\x81J0123456...DpathGfoo.txtKpermissions\x81DpullEheads\xa2Dargs\xa1Jpubliconly\xf4Kpermissions\x81DpullEknown\xa2Dargs\xa1Enodes\x81HdeadbeefKpermissions\x81DpullHlistkeys\xa2Dargs\xa1InamespaceBnsKpermissions\x81DpullFlookup\xa2Dargs\xa1CkeyCfooKpermissions\x81DpullLmanifestdata\xa2Dargs\xa4Ffields\x82GparentsHrevisionKhaveparents\xf5Enodes\x81J0123456...Dtree at Kpermissions\x81DpullGpushkey\xa2Dargs\xa4CkeyCkeyInamespaceBnsCnewCnewColdColdKpermissions\x81DpushKcompression\x81\xa1DnameDzlibQframingmediatypes\x81X&application/mercurial-exp-framing-0005Nrawrepoformats\x82LgeneraldeltaHrevlogv1Nv1capabilitiesY\x01\xc5batch branchmap $USUAL_BUNDLE2_CAPS_SERVER$ changegroupsubset compression=$BUNDLE2_COMPRESSIONS$ getbundle httpheader=1024 httpmediatype=0.1rx,0.1tx,0.2tx known lookup pushkey streamreqs=generaldelta,revlogv1 unbundle=HG10GZ,HG10BZ,HG10UN unbundlehash
+  s>     \xa3GapibaseDapi/Dapis\xa1Pexp-http-v2-0001\xa4Hcommands\xaaIbranchmap\xa2Dargs\xa0Kpermissions\x81DpullLcapabilities\xa2Dargs\xa0Kpermissions\x81DpullMchangesetdata\xa2Dargs\xa3Ffields\xd9\x01\x02\x82GparentsHrevisionInoderange\x82\x81J0123456...\x81Iabcdef...Enodes\x81J0123456...Kpermissions\x81DpullHfiledata\xa2Dargs\xa4Ffields\xd9\x01\x02\x82GparentsHrevisionKhaveparents\xf5Enodes\x81J0123456...DpathGfoo.txtKpermissions\x81DpullEheads\xa2Dargs\xa1Jpubliconly\xf4Kpermissions\x81DpullEknown\xa2Dargs\xa1Enodes\x81HdeadbeefKpermissions\x81DpullHlistkeys\xa2Dargs\xa1InamespaceBnsKpermissions\x81DpullFlookup\xa2Dargs\xa1CkeyCfooKpermissions\x81DpullLmanifestdata\xa2Dargs\xa4Ffields\xd9\x01\x02\x82GparentsHrevisionKhaveparents\xf5Enodes\x81J0123456...Dtree at Kpermissions\x81DpullGpushkey\xa2Dargs\xa4CkeyCkeyInamespaceBnsCnewCnewColdColdKpermissions\x81DpushKcompression\x81\xa1DnameDzlibQframingmediatypes\x81X&application/mercurial-exp-framing-0005Nrawrepoformats\x82LgeneraldeltaHrevlogv1Nv1capabilitiesY\x01\xc5batch branchmap $USUAL_BUNDLE2_CAPS_SERVER$ changegroupsubset compression=$BUNDLE2_COMPRESSIONS$ getbundle httpheader=1024 httpmediatype=0.1rx,0.1tx,0.2tx known lookup pushkey streamreqs=generaldelta,revlogv1 unbundle=HG10GZ,HG10BZ,HG10UN unbundlehash
   sending capabilities command
   s>     POST /api/exp-http-v2-0001/ro/capabilities HTTP/1.1\r\n
   s>     Accept-Encoding: identity\r\n
@@ -392,11 +392,11 @@
   s>     \xa1FstatusBok
   s>     \r\n
   received frame(size=11; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=continuation)
-  s>     30e\r\n
-  s>     \x06\x03\x00\x01\x00\x02\x001
-  s>     \xa4Hcommands\xaaIbranchmap\xa2Dargs\xa0Kpermissions\x81DpullLcapabilities\xa2Dargs\xa0Kpermissions\x81DpullMchangesetdata\xa2Dargs\xa3Ffields\xd9\x01\x02\x82GparentsHrevisionInoderange\x82\x81J0123456...\x81Iabcdef...Enodes\x81J0123456...Kpermissions\x81DpullHfiledata\xa2Dargs\xa4Ffields\x82GparentsHrevisionKhaveparents\xf5Enodes\x81J0123456...DpathGfoo.txtKpermissions\x81DpullEheads\xa2Dargs\xa1Jpubliconly\xf4Kpermissions\x81DpullEknown\xa2Dargs\xa1Enodes\x81HdeadbeefKpermissions\x81DpullHlistkeys\xa2Dargs\xa1InamespaceBnsKpermissions\x81DpullFlookup\xa2Dargs\xa1CkeyCfooKpermissions\x81DpullLmanifestdata\xa2Dargs\xa4Ffields\x82GparentsHrevisionKhaveparents\xf5Enodes\x81J0123456...Dtree at Kpermissions\x81DpullGpushkey\xa2Dargs\xa4CkeyCkeyInamespaceBnsCnewCnewColdColdKpermissions\x81DpushKcompression\x81\xa1DnameDzlibQframingmediatypes\x81X&application/mercurial-exp-framing-0005Nrawrepoformats\x82LgeneraldeltaHrevlogv1
+  s>     314\r\n
+  s>     \x0c\x03\x00\x01\x00\x02\x001
+  s>     \xa4Hcommands\xaaIbranchmap\xa2Dargs\xa0Kpermissions\x81DpullLcapabilities\xa2Dargs\xa0Kpermissions\x81DpullMchangesetdata\xa2Dargs\xa3Ffields\xd9\x01\x02\x82GparentsHrevisionInoderange\x82\x81J0123456...\x81Iabcdef...Enodes\x81J0123456...Kpermissions\x81DpullHfiledata\xa2Dargs\xa4Ffields\xd9\x01\x02\x82GparentsHrevisionKhaveparents\xf5Enodes\x81J0123456...DpathGfoo.txtKpermissions\x81DpullEheads\xa2Dargs\xa1Jpubliconly\xf4Kpermissions\x81DpullEknown\xa2Dargs\xa1Enodes\x81HdeadbeefKpermissions\x81DpullHlistkeys\xa2Dargs\xa1InamespaceBnsKpermissions\x81DpullFlookup\xa2Dargs\xa1CkeyCfooKpermissions\x81DpullLmanifestdata\xa2Dargs\xa4Ffields\xd9\x01\x02\x82GparentsHrevisionKhaveparents\xf5Enodes\x81J0123456...Dtree at Kpermissions\x81DpullGpushkey\xa2Dargs\xa4CkeyCkeyInamespaceBnsCnewCnewColdColdKpermissions\x81DpushKcompression\x81\xa1DnameDzlibQframingmediatypes\x81X&application/mercurial-exp-framing-0005Nrawrepoformats\x82LgeneraldeltaHrevlogv1
   s>     \r\n
-  received frame(size=774; request=1; stream=2; streamflags=; type=command-response; flags=continuation)
+  received frame(size=780; request=1; stream=2; streamflags=; type=command-response; flags=continuation)
   s>     8\r\n
   s>     \x00\x00\x00\x01\x00\x02\x002
   s>     \r\n
@@ -442,10 +442,10 @@
         },
         b'filedata': {
           b'args': {
-            b'fields': [
+            b'fields': set([
               b'parents',
               b'revision'
-            ],
+            ]),
             b'haveparents': True,
             b'nodes': [
               b'0123456...'
@@ -492,10 +492,10 @@
         },
         b'manifestdata': {
           b'args': {
-            b'fields': [
+            b'fields': set([
               b'parents',
               b'revision'
-            ],
+            ]),
             b'haveparents': True,
             b'nodes': [
               b'0123456...'
diff --git a/tests/test-http-protocol.t b/tests/test-http-protocol.t
--- a/tests/test-http-protocol.t
+++ b/tests/test-http-protocol.t
@@ -313,7 +313,7 @@
   s>     Content-Type: application/mercurial-cbor\r\n
   s>     Content-Length: *\r\n (glob)
   s>     \r\n
-  s>     \xa3GapibaseDapi/Dapis\xa1Pexp-http-v2-0001\xa4Hcommands\xaaIbranchmap\xa2Dargs\xa0Kpermissions\x81DpullLcapabilities\xa2Dargs\xa0Kpermissions\x81DpullMchangesetdata\xa2Dargs\xa3Ffields\xd9\x01\x02\x82GparentsHrevisionInoderange\x82\x81J0123456...\x81Iabcdef...Enodes\x81J0123456...Kpermissions\x81DpullHfiledata\xa2Dargs\xa4Ffields\x82GparentsHrevisionKhaveparents\xf5Enodes\x81J0123456...DpathGfoo.txtKpermissions\x81DpullEheads\xa2Dargs\xa1Jpubliconly\xf4Kpermissions\x81DpullEknown\xa2Dargs\xa1Enodes\x81HdeadbeefKpermissions\x81DpullHlistkeys\xa2Dargs\xa1InamespaceBnsKpermissions\x81DpullFlookup\xa2Dargs\xa1CkeyCfooKpermissions\x81DpullLmanifestdata\xa2Dargs\xa4Ffields\x82GparentsHrevisionKhaveparents\xf5Enodes\x81J0123456...Dtree at Kpermissions\x81DpullGpushkey\xa2Dargs\xa4CkeyCkeyInamespaceBnsCnewCnewColdColdKpermissions\x81DpushKcompression\x81\xa1DnameDzlibQframingmediatypes\x81X&application/mercurial-exp-framing-0005Nrawrepoformats\x82LgeneraldeltaHrevlogv1Nv1capabilitiesY\x01\xc5batch branchmap $USUAL_BUNDLE2_CAPS_SERVER$ changegroupsubset compression=$BUNDLE2_COMPRESSIONS$ getbundle httpheader=1024 httpmediatype=0.1rx,0.1tx,0.2tx known lookup pushkey streamreqs=generaldelta,revlogv1 unbundle=HG10GZ,HG10BZ,HG10UN unbundlehash
+  s>     \xa3GapibaseDapi/Dapis\xa1Pexp-http-v2-0001\xa4Hcommands\xaaIbranchmap\xa2Dargs\xa0Kpermissions\x81DpullLcapabilities\xa2Dargs\xa0Kpermissions\x81DpullMchangesetdata\xa2Dargs\xa3Ffields\xd9\x01\x02\x82GparentsHrevisionInoderange\x82\x81J0123456...\x81Iabcdef...Enodes\x81J0123456...Kpermissions\x81DpullHfiledata\xa2Dargs\xa4Ffields\xd9\x01\x02\x82GparentsHrevisionKhaveparents\xf5Enodes\x81J0123456...DpathGfoo.txtKpermissions\x81DpullEheads\xa2Dargs\xa1Jpubliconly\xf4Kpermissions\x81DpullEknown\xa2Dargs\xa1Enodes\x81HdeadbeefKpermissions\x81DpullHlistkeys\xa2Dargs\xa1InamespaceBnsKpermissions\x81DpullFlookup\xa2Dargs\xa1CkeyCfooKpermissions\x81DpullLmanifestdata\xa2Dargs\xa4Ffields\xd9\x01\x02\x82GparentsHrevisionKhaveparents\xf5Enodes\x81J0123456...Dtree at Kpermissions\x81DpullGpushkey\xa2Dargs\xa4CkeyCkeyInamespaceBnsCnewCnewColdColdKpermissions\x81DpushKcompression\x81\xa1DnameDzlibQframingmediatypes\x81X&application/mercurial-exp-framing-0005Nrawrepoformats\x82LgeneraldeltaHrevlogv1Nv1capabilitiesY\x01\xc5batch branchmap $USUAL_BUNDLE2_CAPS_SERVER$ changegroupsubset compression=$BUNDLE2_COMPRESSIONS$ getbundle httpheader=1024 httpmediatype=0.1rx,0.1tx,0.2tx known lookup pushkey streamreqs=generaldelta,revlogv1 unbundle=HG10GZ,HG10BZ,HG10UN unbundlehash
   sending heads command
   s>     POST /api/exp-http-v2-0001/ro/heads HTTP/1.1\r\n
   s>     Accept-Encoding: identity\r\n
diff --git a/mercurial/wireprotov2server.py b/mercurial/wireprotov2server.py
--- a/mercurial/wireprotov2server.py
+++ b/mercurial/wireprotov2server.py
@@ -311,6 +311,10 @@
         action, meta = reactor.oncommandresponsereadyobjects(
             outstream, command['requestid'], objs)
 
+    except error.WireprotoCommandError as e:
+        action, meta = reactor.oncommanderror(
+            outstream, command['requestid'], e.message, e.messageargs)
+
     except Exception as e:
         action, meta = reactor.onservererror(
             outstream, command['requestid'],
@@ -348,13 +352,39 @@
         return HTTP_WIREPROTO_V2
 
     def getargs(self, args):
+        # First look for args that were passed but aren't registered on this
+        # command.
+        extra = set(self._args) - set(args)
+        if extra:
+            raise error.WireprotoCommandError(
+                'unsupported argument to command: %s' %
+                ', '.join(sorted(extra)))
+
+        # And look for required arguments that are missing.
+        missing = {a for a in args if args[a].get('required')} - set(self._args)
+
+        if missing:
+            raise error.WireprotoCommandError(
+                'missing required arguments: %s' % ', '.join(sorted(missing)))
+
+        # Now derive the arguments to pass to the command, taking into
+        # account the arguments specified by the client.
         data = {}
-        for k, typ in args.items():
-            if k == '*':
-                raise NotImplementedError('do not support * args')
-            elif k in self._args:
-                # TODO consider validating value types.
-                data[k] = self._args[k]
+        for k, meta in sorted(args.items()):
+            # This argument wasn't passed by the client.
+            if k not in self._args:
+                data[k] = meta.get('default', lambda: None)()
+                continue
+
+            v = self._args[k]
+
+            # Sets may be expressed as lists. Silently normalize.
+            if meta['type'] == 'set' and isinstance(v, list):
+                v = set(v)
+
+            # TODO consider more/stronger type validation.
+
+            data[k] = v
 
         return data
 
@@ -404,8 +434,10 @@
     # TODO expose available changesetdata fields.
 
     for command, entry in COMMANDS.items():
+        args = {arg: meta['example'] for arg, meta in entry.args.items()}
+
         caps['commands'][command] = {
-            'args': entry.args,
+            'args': args,
             'permissions': [entry.permission],
         }
 
@@ -500,7 +532,22 @@
 
     ``name`` is the name of the wire protocol command being provided.
 
-    ``args`` is a dict of argument names to example values.
+    ``args`` is a dict defining arguments accepted by the command. Keys are
+    the argument name. Values are dicts with the following keys:
+
+       ``type``
+          The argument data type. Must be one of the following string
+          literals: ``bytes``, ``int``, ``list``, ``dict``, ``set``,
+          or ``bool``.
+
+       ``default``
+          A callable returning the default value for this argument.
+
+       ``required``
+          Bool indicating whether the argument is required.
+
+       ``example``
+          An example value for this argument.
 
     ``permission`` defines the permission type needed to run this command.
     Can be ``push`` or ``pull``. These roughly map to read-write and read-only,
@@ -529,6 +576,28 @@
         raise error.ProgrammingError('arguments for version 2 commands '
                                      'must be declared as dicts')
 
+    for arg, meta in args.items():
+        if arg == '*':
+            raise error.ProgrammingError('* argument name not allowed on '
+                                         'version 2 commands')
+
+        if not isinstance(meta, dict):
+            raise error.ProgrammingError('arguments for version 2 commands '
+                                         'must declare metadata as a dict')
+
+        if 'type' not in meta:
+            raise error.ProgrammingError('%s argument for command %s does not '
+                                         'declare type field' % (arg, name))
+
+        if meta['type'] not in ('bytes', 'int', 'list', 'dict', 'set', 'bool'):
+            raise error.ProgrammingError('%s argument for command %s has '
+                                         'illegal type: %s' % (arg, name,
+                                                               meta['type']))
+
+        if 'example' not in meta:
+            raise error.ProgrammingError('%s argument for command %s does not '
+                                         'declare example field' % (arg, name))
+
     def register(func):
         if name in COMMANDS:
             raise error.ProgrammingError('%s command already registered '
@@ -550,16 +619,25 @@
 def capabilitiesv2(repo, proto):
     yield _capabilitiesv2(repo, proto)
 
- at wireprotocommand('changesetdata',
-                  args={
-                      'noderange': [[b'0123456...'], [b'abcdef...']],
-                      'nodes': [b'0123456...'],
-                      'fields': {b'parents', b'revision'},
-                  },
-                  permission='pull')
-def changesetdata(repo, proto, noderange=None, nodes=None, fields=None):
-    fields = fields or set()
-
+ at wireprotocommand(
+    'changesetdata',
+    args={
+        'noderange': {
+            'type': 'list',
+            'example': [[b'0123456...'], [b'abcdef...']],
+        },
+        'nodes': {
+            'type': 'list',
+            'example': [b'0123456...'],
+        },
+        'fields': {
+            'type': 'set',
+            'default': set,
+            'example': {b'parents', b'revision'},
+        },
+    },
+    permission='pull')
+def changesetdata(repo, proto, noderange, nodes, fields):
     # TODO look for unknown fields and abort when they can't be serviced.
 
     if noderange is None and nodes is None:
@@ -691,24 +769,32 @@
 
     return fl
 
- at wireprotocommand('filedata',
-                  args={
-                      'haveparents': True,
-                      'nodes': [b'0123456...'],
-                      'fields': [b'parents', b'revision'],
-                      'path': b'foo.txt',
-                  },
-                  permission='pull')
-def filedata(repo, proto, haveparents=False, nodes=None, fields=None,
-             path=None):
-    fields = fields or set()
-
-    if nodes is None:
-        raise error.WireprotoCommandError('nodes argument must be defined')
-
-    if path is None:
-        raise error.WireprotoCommandError('path argument must be defined')
-
+ at wireprotocommand(
+    'filedata',
+    args={
+        'haveparents': {
+            'type': 'bool',
+            'default': lambda: False,
+            'example': True,
+        },
+        'nodes': {
+            'type': 'list',
+            'required': True,
+            'example': [b'0123456...'],
+        },
+        'fields': {
+            'type': 'set',
+            'default': set,
+            'example': {b'parents', b'revision'},
+        },
+        'path': {
+            'type': 'bytes',
+            'required': True,
+            'example': b'foo.txt',
+        }
+    },
+    permission='pull')
+def filedata(repo, proto, haveparents, nodes, fields, path):
     try:
         # Extensions may wish to access the protocol handler.
         store = getfilestore(repo, proto, path)
@@ -776,72 +862,97 @@
         except GeneratorExit:
             pass
 
- at wireprotocommand('heads',
-                  args={
-                      'publiconly': False,
-                  },
-                  permission='pull')
-def headsv2(repo, proto, publiconly=False):
+ at wireprotocommand(
+    'heads',
+    args={
+        'publiconly': {
+            'type': 'bool',
+            'default': lambda: False,
+            'example': False,
+        },
+    },
+    permission='pull')
+def headsv2(repo, proto, publiconly):
     if publiconly:
         repo = repo.filtered('immutable')
 
     yield repo.heads()
 
- at wireprotocommand('known',
-                  args={
-                      'nodes': [b'deadbeef'],
-                  },
-                  permission='pull')
-def knownv2(repo, proto, nodes=None):
-    nodes = nodes or []
+ at wireprotocommand(
+    'known',
+    args={
+        'nodes': {
+            'type': 'list',
+            'default': list,
+            'example': [b'deadbeef'],
+        },
+    },
+    permission='pull')
+def knownv2(repo, proto, nodes):
     result = b''.join(b'1' if n else b'0' for n in repo.known(nodes))
     yield result
 
- at wireprotocommand('listkeys',
-                  args={
-                      'namespace': b'ns',
-                  },
-                  permission='pull')
-def listkeysv2(repo, proto, namespace=None):
+ at wireprotocommand(
+    'listkeys',
+    args={
+        'namespace': {
+            'type': 'bytes',
+            'required': True,
+            'example': b'ns',
+        },
+    },
+    permission='pull')
+def listkeysv2(repo, proto, namespace):
     keys = repo.listkeys(encoding.tolocal(namespace))
     keys = {encoding.fromlocal(k): encoding.fromlocal(v)
             for k, v in keys.iteritems()}
 
     yield keys
 
- at wireprotocommand('lookup',
-                  args={
-                      'key': b'foo',
-                  },
-                  permission='pull')
+ at wireprotocommand(
+    'lookup',
+    args={
+        'key': {
+            'type': 'bytes',
+            'required': True,
+            'example': b'foo',
+        },
+    },
+    permission='pull')
 def lookupv2(repo, proto, key):
     key = encoding.tolocal(key)
 
     # TODO handle exception.
     node = repo.lookup(key)
 
     yield node
 
- at wireprotocommand('manifestdata',
-                  args={
-                      'nodes': [b'0123456...'],
-                      'haveparents': True,
-                      'fields': [b'parents', b'revision'],
-                      'tree': b'',
-                  },
-                  permission='pull')
-def manifestdata(repo, proto, haveparents=False, nodes=None, fields=None,
-                 tree=None):
-    fields = fields or set()
-
-    if nodes is None:
-        raise error.WireprotoCommandError(
-            'nodes argument must be defined')
-
-    if tree is None:
-        raise error.WireprotoCommandError(
-            'tree argument must be defined')
-
+ at wireprotocommand(
+    'manifestdata',
+    args={
+        'nodes': {
+            'type': 'list',
+            'required': True,
+            'example': [b'0123456...'],
+        },
+        'haveparents': {
+            'type': 'bool',
+            'default': lambda: False,
+            'example': True,
+        },
+        'fields': {
+            'type': 'set',
+            'default': set,
+            'example': {b'parents', b'revision'},
+        },
+        'tree': {
+            'type': 'bytes',
+            'required': True,
+            'example': b'',
+        },
+    },
+    permission='pull')
+def manifestdata(repo, proto, haveparents, nodes, fields, tree):
     store = repo.manifestlog.getstorage(tree)
 
     # Validate the node is known and abort on unknown revisions.
@@ -905,14 +1016,31 @@
         except GeneratorExit:
             pass
 
- at wireprotocommand('pushkey',
-                  args={
-                      'namespace': b'ns',
-                      'key': b'key',
-                      'old': b'old',
-                      'new': b'new',
-                  },
-                  permission='push')
+ at wireprotocommand(
+    'pushkey',
+    args={
+        'namespace': {
+            'type': 'bytes',
+            'required': True,
+            'example': b'ns',
+        },
+        'key': {
+            'type': 'bytes',
+            'required': True,
+            'example': b'key',
+        },
+        'old': {
+            'type': 'bytes',
+            'required': True,
+            'example': b'old',
+        },
+        'new': {
+            'type': 'bytes',
+            'required': True,
+            'example': 'new',
+        },
+    },
+    permission='push')
 def pushkeyv2(repo, proto, namespace, key, old, new):
     # TODO handle ui output redirection
     yield repo.pushkey(encoding.tolocal(namespace),



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


More information about the Mercurial-devel mailing list