Patchwork [5,of,5,postargs] http: support sending hgargs via POST body instead of in GET or headers

login
register
mail settings
Submitter Augie Fackler
Date March 11, 2016, 4:55 p.m.
Message ID <6782893562a73dc56f41.1457715316@arthedain.pit.corp.google.com>
Download mbox | patch
Permalink /patch/13797/
State Superseded
Commit fd2acc5046f67680d7f264392c8ba070e48ab58b
Delegated to: Martin von Zweigbergk
Headers show

Comments

Augie Fackler - March 11, 2016, 4:55 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1457714220 18000
#      Fri Mar 11 11:37:00 2016 -0500
# Node ID 6782893562a73dc56f4184c3967775178c5bdfb4
# Parent  efcc8e438753cddf5af209ae812f77c2affead07
# EXP-Topic batch
http: support sending hgargs via POST body instead of in GET or headers

narrowhg (for its narrow spec) and remotefilelog (for its large batch
requests) would like to be able to make requests with argument sets so
absurdly large that they blow out total request size limit on some
http servers. As a workaround, support stuffing args at the start
of the POST body.

We will probably want to leave this behavior off by default in servers
forever, because it makes the old "POSTs are only for writes"
assumption wrong, which might break some of the simpler authentication
configurations.
Gregory Szorc - March 11, 2016, 6 p.m.
On Fri, Mar 11, 2016 at 8:55 AM, Augie Fackler <raf@durin42.com> wrote:

> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1457714220 18000
> #      Fri Mar 11 11:37:00 2016 -0500
> # Node ID 6782893562a73dc56f4184c3967775178c5bdfb4
> # Parent  efcc8e438753cddf5af209ae812f77c2affead07
> # EXP-Topic batch
> http: support sending hgargs via POST body instead of in GET or headers
>
> narrowhg (for its narrow spec) and remotefilelog (for its large batch
> requests) would like to be able to make requests with argument sets so
> absurdly large that they blow out total request size limit on some
> http servers. As a workaround, support stuffing args at the start
> of the POST body.
>
> We will probably want to leave this behavior off by default in servers
> forever, because it makes the old "POSTs are only for writes"
> assumption wrong, which might break some of the simpler authentication
> configurations.
>

This patch and series look correct.

I do wish we had a larger discussion about future directions of the
protocol. e.g. can we use the Upgrade header so we can employ WebSockets,
HTTP/2 or a custom protocol that gets us bi-directional streaming, which
would be very beneficial for things like more efficient discovery.

That being said, this is behind an experimental flag and I'm OK with that.
Why stop progress when we have no better alternatives.


> diff --git a/mercurial/hgweb/protocol.py b/mercurial/hgweb/protocol.py
> --- a/mercurial/hgweb/protocol.py
> +++ b/mercurial/hgweb/protocol.py
> @@ -45,6 +45,9 @@ class webproto(wireproto.abstractserverp
>          return [data[k] for k in keys]
>      def _args(self):
>          args = self.req.form.copy()
> +        postlen = int(self.req.env.get('HTTP_X_HGARGS_POST', 0))
> +        if postlen:
> +            return cgi.parse_qs(self.req.read(postlen))
>

Now that data is outside of headers, it feels wasteful to have to URL
encode and turn it into a query string like thing. But, it does mean you
get to reuse the existing parser, so I guess it isn't too bad. If we ever
invent a new, more efficient encoding, we can always invent a new
capability to convey that.


>          chunks = []
>          i = 1
>          while True:
> diff --git a/mercurial/httppeer.py b/mercurial/httppeer.py
> --- a/mercurial/httppeer.py
> +++ b/mercurial/httppeer.py
> @@ -97,7 +97,22 @@ class httppeer(wireproto.wirepeer):
>          self.ui.debug("sending %s command\n" % cmd)
>          q = [('cmd', cmd)]
>          headersize = 0
> -        if True:
> +        # Important: don't use self.capable() here or else you end up
> +        # with infinite recursion when trying to look up capabilities
> +        # for the first time.
> +        postargsok = self.caps is not None and 'httppostargs' in self.caps
> +        # TODO: support for httppostargs when data is a file-like
> +        # object rather than a basestring
> +        canmungedata = not data or isinstance(data, basestring)
> +        if postargsok and canmungedata:
> +            strargs = urllib.urlencode(sorted(args.items()))
> +            if strargs:
> +                if not data:
> +                    data = strargs
> +                elif isinstance(data, basestring):
> +                    data = strargs + data
> +                headers['X-HgArgs-Post'] = len(strargs)
> +        else:
>              if len(args) > 0:
>                  httpheader = self.capable('httpheader')
>                  if httpheader:
> diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
> --- a/mercurial/wireproto.py
> +++ b/mercurial/wireproto.py
> @@ -688,6 +688,8 @@ def _capabilities(repo, proto):
>      caps.append('unbundle=%s' % ','.join(changegroupmod.bundlepriority))
>      caps.append(
>          'httpheader=%d' % repo.ui.configint('server', 'maxhttpheaderlen',
> 1024))
> +    if repo.ui.configbool('experimental', 'httppostargs', False):
> +        caps.append('httppostargs')
>      return caps
>
>  # If you are writing an extension and consider wrapping this function.
> Wrap
> diff --git a/tests/test-wireproto.t b/tests/test-wireproto.t
> --- a/tests/test-wireproto.t
> +++ b/tests/test-wireproto.t
> @@ -19,7 +19,12 @@ Local:
>
>  HTTP:
>
> -  $ hg serve -R repo -p $HGPORT -d --pid-file=hg1.pid -E error.log -A
> access.log
> +  $ cat >> $HGRCPATH <<EOF
> +  > [experimental]
> +  > httppostargs = true
> +  > EOF
> +
> +  $ hg  serve -R repo -p $HGPORT -d --pid-file=hg1.pid -E error.log -A
> access.log
>    $ cat hg1.pid >> $DAEMON_PIDS
>
>    $ hg debugwireargs http://localhost:$HGPORT/ un deux trois quatre
> @@ -37,6 +42,66 @@ HTTP:
>    $ cat error.log
>    $ cat access.log
>    * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:39
> (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:39
> (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:43
> (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:43
> (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:27
> (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:27
> (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17
> (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17
> (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17
> (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17
> (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:1033
> (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:1033
> (glob)
> +
> +  $ cat >> $HGRCPATH <<EOF
> +  > [experimental]
> +  > httppostargs = false
> +  > EOF
> +
> +HTTP without args-in-POST:
> +  $ hg serve -R repo -p $HGPORT1 -d --pid-file=hg1.pid -E error.log -A
> access.log
> +  $ cat hg1.pid >> $DAEMON_PIDS
> +
> +  $ hg debugwireargs http://localhost:$HGPORT1/ un deux trois quatre
> +  un deux trois quatre None
> +  $ hg debugwireargs http://localhost:$HGPORT1/ \ un deux trois\  qu\ \
> atre
> +   un deux trois  qu  atre None
> +  $ hg debugwireargs http://localhost:$HGPORT1/ eins zwei --four vier
> +  eins zwei None vier None
> +  $ hg debugwireargs http://localhost:$HGPORT1/ eins zwei
> +  eins zwei None None None
> +  $ hg debugwireargs http://localhost:$HGPORT1/ eins zwei --five fuenf
> +  eins zwei None None None
> +  $ hg debugwireargs http://localhost:$HGPORT1/ un deux trois
> onethousandcharactersxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> +  un deux trois
> onethousandcharactersxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> None
> +  $ cat error.log
> +  $ cat access.log
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:39
> (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:39
> (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:43
> (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:43
> (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:27
> (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:27
> (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17
> (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17
> (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17
> (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17
> (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:1033
> (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:1033
> (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
>    * - - [*] "GET /?cmd=debugwireargs HTTP/1.1" 200 -
> x-hgarg-1:four=quatre&one=un&three=trois&two=deux (glob)
>    * - - [*] "GET /?cmd=debugwireargs HTTP/1.1" 200 -
> x-hgarg-1:four=quatre&one=un&three=trois&two=deux (glob)
>    * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Martin von Zweigbergk - March 11, 2016, 6:02 p.m.
On Fri, Mar 11, 2016 at 10:00 AM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> On Fri, Mar 11, 2016 at 8:55 AM, Augie Fackler <raf@durin42.com> wrote:
>>
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1457714220 18000
>> #      Fri Mar 11 11:37:00 2016 -0500
>> # Node ID 6782893562a73dc56f4184c3967775178c5bdfb4
>> # Parent  efcc8e438753cddf5af209ae812f77c2affead07
>> # EXP-Topic batch
>> http: support sending hgargs via POST body instead of in GET or headers
>>
>> narrowhg (for its narrow spec) and remotefilelog (for its large batch
>> requests) would like to be able to make requests with argument sets so
>> absurdly large that they blow out total request size limit on some
>> http servers. As a workaround, support stuffing args at the start
>> of the POST body.
>>
>> We will probably want to leave this behavior off by default in servers
>> forever, because it makes the old "POSTs are only for writes"
>> assumption wrong, which might break some of the simpler authentication
>> configurations.
>
>
> This patch and series look correct.
>
> I do wish we had a larger discussion about future directions of the
> protocol. e.g. can we use the Upgrade header so we can employ WebSockets,
> HTTP/2 or a custom protocol that gets us bi-directional streaming, which
> would be very beneficial for things like more efficient discovery.
>
> That being said, this is behind an experimental flag and I'm OK with that.
> Why stop progress when we have no better alternatives.
>
>>
>> diff --git a/mercurial/hgweb/protocol.py b/mercurial/hgweb/protocol.py
>> --- a/mercurial/hgweb/protocol.py
>> +++ b/mercurial/hgweb/protocol.py
>> @@ -45,6 +45,9 @@ class webproto(wireproto.abstractserverp
>>          return [data[k] for k in keys]
>>      def _args(self):
>>          args = self.req.form.copy()
>> +        postlen = int(self.req.env.get('HTTP_X_HGARGS_POST', 0))
>> +        if postlen:
>> +            return cgi.parse_qs(self.req.read(postlen))
>
>
> Now that data is outside of headers, it feels wasteful to have to URL encode
> and turn it into a query string like thing. But, it does mean you get to
> reuse the existing parser, so I guess it isn't too bad. If we ever invent a
> new, more efficient encoding, we can always invent a new capability to
> convey that.

I haven't reviewed the patch yet, but I did suggest
pushkey.{encode,decode}keys(). Would that be better?

>
>>
>>          chunks = []
>>          i = 1
>>          while True:
>> diff --git a/mercurial/httppeer.py b/mercurial/httppeer.py
>> --- a/mercurial/httppeer.py
>> +++ b/mercurial/httppeer.py
>> @@ -97,7 +97,22 @@ class httppeer(wireproto.wirepeer):
>>          self.ui.debug("sending %s command\n" % cmd)
>>          q = [('cmd', cmd)]
>>          headersize = 0
>> -        if True:
>> +        # Important: don't use self.capable() here or else you end up
>> +        # with infinite recursion when trying to look up capabilities
>> +        # for the first time.
>> +        postargsok = self.caps is not None and 'httppostargs' in
>> self.caps
>> +        # TODO: support for httppostargs when data is a file-like
>> +        # object rather than a basestring
>> +        canmungedata = not data or isinstance(data, basestring)
>> +        if postargsok and canmungedata:
>> +            strargs = urllib.urlencode(sorted(args.items()))
>> +            if strargs:
>> +                if not data:
>> +                    data = strargs
>> +                elif isinstance(data, basestring):
>> +                    data = strargs + data
>> +                headers['X-HgArgs-Post'] = len(strargs)
>> +        else:
>>              if len(args) > 0:
>>                  httpheader = self.capable('httpheader')
>>                  if httpheader:
>> diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
>> --- a/mercurial/wireproto.py
>> +++ b/mercurial/wireproto.py
>> @@ -688,6 +688,8 @@ def _capabilities(repo, proto):
>>      caps.append('unbundle=%s' % ','.join(changegroupmod.bundlepriority))
>>      caps.append(
>>          'httpheader=%d' % repo.ui.configint('server', 'maxhttpheaderlen',
>> 1024))
>> +    if repo.ui.configbool('experimental', 'httppostargs', False):
>> +        caps.append('httppostargs')
>>      return caps
>>
>>  # If you are writing an extension and consider wrapping this function.
>> Wrap
>> diff --git a/tests/test-wireproto.t b/tests/test-wireproto.t
>> --- a/tests/test-wireproto.t
>> +++ b/tests/test-wireproto.t
>> @@ -19,7 +19,12 @@ Local:
>>
>>  HTTP:
>>
>> -  $ hg serve -R repo -p $HGPORT -d --pid-file=hg1.pid -E error.log -A
>> access.log
>> +  $ cat >> $HGRCPATH <<EOF
>> +  > [experimental]
>> +  > httppostargs = true
>> +  > EOF
>> +
>> +  $ hg  serve -R repo -p $HGPORT -d --pid-file=hg1.pid -E error.log -A
>> access.log
>>    $ cat hg1.pid >> $DAEMON_PIDS
>>
>>    $ hg debugwireargs http://localhost:$HGPORT/ un deux trois quatre
>> @@ -37,6 +42,66 @@ HTTP:
>>    $ cat error.log
>>    $ cat access.log
>>    * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:39
>> (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:39
>> (glob)
>> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:43
>> (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:43
>> (glob)
>> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:27
>> (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:27
>> (glob)
>> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17
>> (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17
>> (glob)
>> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17
>> (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17
>> (glob)
>> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:1033
>> (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:1033
>> (glob)
>> +
>> +  $ cat >> $HGRCPATH <<EOF
>> +  > [experimental]
>> +  > httppostargs = false
>> +  > EOF
>> +
>> +HTTP without args-in-POST:
>> +  $ hg serve -R repo -p $HGPORT1 -d --pid-file=hg1.pid -E error.log -A
>> access.log
>> +  $ cat hg1.pid >> $DAEMON_PIDS
>> +
>> +  $ hg debugwireargs http://localhost:$HGPORT1/ un deux trois quatre
>> +  un deux trois quatre None
>> +  $ hg debugwireargs http://localhost:$HGPORT1/ \ un deux trois\  qu\ \
>> atre
>> +   un deux trois  qu  atre None
>> +  $ hg debugwireargs http://localhost:$HGPORT1/ eins zwei --four vier
>> +  eins zwei None vier None
>> +  $ hg debugwireargs http://localhost:$HGPORT1/ eins zwei
>> +  eins zwei None None None
>> +  $ hg debugwireargs http://localhost:$HGPORT1/ eins zwei --five fuenf
>> +  eins zwei None None None
>> +  $ hg debugwireargs http://localhost:$HGPORT1/ un deux trois
>> onethousandcharactersxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>> +  un deux trois
>> onethousandcharactersxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>> None
>> +  $ cat error.log
>> +  $ cat access.log
>> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:39
>> (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:39
>> (glob)
>> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:43
>> (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:43
>> (glob)
>> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:27
>> (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:27
>> (glob)
>> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17
>> (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17
>> (glob)
>> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17
>> (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17
>> (glob)
>> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:1033
>> (glob)
>> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:1033
>> (glob)
>> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
>>    * - - [*] "GET /?cmd=debugwireargs HTTP/1.1" 200 -
>> x-hgarg-1:four=quatre&one=un&three=trois&two=deux (glob)
>>    * - - [*] "GET /?cmd=debugwireargs HTTP/1.1" 200 -
>> x-hgarg-1:four=quatre&one=un&three=trois&two=deux (glob)
>>    * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Augie Fackler - March 11, 2016, 6:06 p.m.
> On Mar 11, 2016, at 13:02, Martin von Zweigbergk <martinvonz@google.com> wrote:
> 
>>>         args = self.req.form.copy()
>>> +        postlen = int(self.req.env.get('HTTP_X_HGARGS_POST', 0))
>>> +        if postlen:
>>> +            return cgi.parse_qs(self.req.read(postlen))
>> 
>> 
>> Now that data is outside of headers, it feels wasteful to have to URL encode
>> and turn it into a query string like thing. But, it does mean you get to
>> reuse the existing parser, so I guess it isn't too bad. If we ever invent a
>> new, more efficient encoding, we can always invent a new capability to
>> convey that.
> 
> I haven't reviewed the patch yet, but I did suggest
> pushkey.{encode,decode}keys(). Would that be better?

pushkey encoding didn't appear to have enough escaping to be workable for this case.
Augie Fackler - March 11, 2016, 7:23 p.m.
> On Mar 11, 2016, at 13:00, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> 
> We will probably want to leave this behavior off by default in servers
> forever, because it makes the old "POSTs are only for writes"
> assumption wrong, which might break some of the simpler authentication
> configurations.
> 
> This patch and series look correct.
>  
> I do wish we had a larger discussion about future directions of the protocol. e.g. can we use the Upgrade header so we can employ WebSockets, HTTP/2 or a custom protocol that gets us bi-directional streaming, which would be very beneficial for things like more efficient discovery.

I admire your ambition, but I don't think websockets is going to be viable for Google's needs in the foreseeable future, so we'll continue to want plain-old-http to be nicely performant.

I'm also skeptical that http/2 offers much value over http/1.1 for our use cases in Mercurial, but I'd be happy to be wrong.

> That being said, this is behind an experimental flag and I'm OK with that. Why stop progress when we have no better alternatives.

I'll want to consider making this just a [server] flag some day, and have the client support it by default if servers are willing.
Martin von Zweigbergk - March 14, 2016, 10:07 p.m.
On Fri, Mar 11, 2016 at 8:55 AM, Augie Fackler <raf@durin42.com> wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1457714220 18000
> #      Fri Mar 11 11:37:00 2016 -0500
> # Node ID 6782893562a73dc56f4184c3967775178c5bdfb4
> # Parent  efcc8e438753cddf5af209ae812f77c2affead07
> # EXP-Topic batch
> http: support sending hgargs via POST body instead of in GET or headers
>
> narrowhg (for its narrow spec) and remotefilelog (for its large batch
> requests) would like to be able to make requests with argument sets so
> absurdly large that they blow out total request size limit on some
> http servers. As a workaround, support stuffing args at the start
> of the POST body.
>
> We will probably want to leave this behavior off by default in servers
> forever, because it makes the old "POSTs are only for writes"
> assumption wrong, which might break some of the simpler authentication
> configurations.
>
> diff --git a/mercurial/hgweb/protocol.py b/mercurial/hgweb/protocol.py
> --- a/mercurial/hgweb/protocol.py
> +++ b/mercurial/hgweb/protocol.py
> @@ -45,6 +45,9 @@ class webproto(wireproto.abstractserverp
>          return [data[k] for k in keys]
>      def _args(self):
>          args = self.req.form.copy()

When using GET, these arguments are combined with the ones in HgArgs.
Is that just for legacy clients, or why do we not need it for POST? (I
read something about that in httppeer._callpush().)

> +        postlen = int(self.req.env.get('HTTP_X_HGARGS_POST', 0))
> +        if postlen:
> +            return cgi.parse_qs(self.req.read(postlen))

Similar question here: For GET, we pass keep_blank_values=True to
parse_qs(). Can there not be empty values from recent hg clients?

Should we document that these things only apply to old clients?

>          chunks = []
>          i = 1
>          while True:
> diff --git a/mercurial/httppeer.py b/mercurial/httppeer.py
> --- a/mercurial/httppeer.py
> +++ b/mercurial/httppeer.py
> @@ -97,7 +97,22 @@ class httppeer(wireproto.wirepeer):
>          self.ui.debug("sending %s command\n" % cmd)
>          q = [('cmd', cmd)]
>          headersize = 0
> -        if True:
> +        # Important: don't use self.capable() here or else you end up
> +        # with infinite recursion when trying to look up capabilities
> +        # for the first time.
> +        postargsok = self.caps is not None and 'httppostargs' in self.caps

Do you think "cmd != 'capabilities' and self.capable('httppostargs')"
would be better? That form seems to assume less about the ordering of
commands; it should allow POST'ing HgArgs even if 'capabilities' had
not been called before the current command (whatever that may be). I
guess it's mostly a hypothetical issue at this point (or maybe not; I
don't know the signalling well enough).

> +        # TODO: support for httppostargs when data is a file-like
> +        # object rather than a basestring

How do you imagine we (probably I) would do that? Do I make
_callpush() prepend the serialized HgArgs to the bundle file?

> +        canmungedata = not data or isinstance(data, basestring)
> +        if postargsok and canmungedata:
> +            strargs = urllib.urlencode(sorted(args.items()))
> +            if strargs:
> +                if not data:
> +                    data = strargs
> +                elif isinstance(data, basestring):
> +                    data = strargs + data
> +                headers['X-HgArgs-Post'] = len(strargs)
> +        else:
>              if len(args) > 0:
>                  httpheader = self.capable('httpheader')
>                  if httpheader:
> diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
> --- a/mercurial/wireproto.py
> +++ b/mercurial/wireproto.py
> @@ -688,6 +688,8 @@ def _capabilities(repo, proto):
>      caps.append('unbundle=%s' % ','.join(changegroupmod.bundlepriority))
>      caps.append(
>          'httpheader=%d' % repo.ui.configint('server', 'maxhttpheaderlen', 1024))
> +    if repo.ui.configbool('experimental', 'httppostargs', False):
> +        caps.append('httppostargs')
>      return caps
>
>  # If you are writing an extension and consider wrapping this function. Wrap
> diff --git a/tests/test-wireproto.t b/tests/test-wireproto.t
> --- a/tests/test-wireproto.t
> +++ b/tests/test-wireproto.t
> @@ -19,7 +19,12 @@ Local:
>
>  HTTP:
>
> -  $ hg serve -R repo -p $HGPORT -d --pid-file=hg1.pid -E error.log -A access.log
> +  $ cat >> $HGRCPATH <<EOF
> +  > [experimental]
> +  > httppostargs = true
> +  > EOF
> +
> +  $ hg  serve -R repo -p $HGPORT -d --pid-file=hg1.pid -E error.log -A access.log

nti: why not use "--config experimental.httppostargs=true" to save a
few line here not have to reset it for the next test?

>    $ cat hg1.pid >> $DAEMON_PIDS
>
>    $ hg debugwireargs http://localhost:$HGPORT/ un deux trois quatre
> @@ -37,6 +42,66 @@ HTTP:
>    $ cat error.log
>    $ cat access.log
>    * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:39 (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:39 (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:43 (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:43 (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:27 (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:27 (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17 (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17 (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17 (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17 (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:1033 (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:1033 (glob)
> +
> +  $ cat >> $HGRCPATH <<EOF
> +  > [experimental]
> +  > httppostargs = false
> +  > EOF
> +
> +HTTP without args-in-POST:
> +  $ hg serve -R repo -p $HGPORT1 -d --pid-file=hg1.pid -E error.log -A access.log
> +  $ cat hg1.pid >> $DAEMON_PIDS
> +
> +  $ hg debugwireargs http://localhost:$HGPORT1/ un deux trois quatre
> +  un deux trois quatre None
> +  $ hg debugwireargs http://localhost:$HGPORT1/ \ un deux trois\  qu\ \ atre
> +   un deux trois  qu  atre None
> +  $ hg debugwireargs http://localhost:$HGPORT1/ eins zwei --four vier
> +  eins zwei None vier None
> +  $ hg debugwireargs http://localhost:$HGPORT1/ eins zwei
> +  eins zwei None None None
> +  $ hg debugwireargs http://localhost:$HGPORT1/ eins zwei --five fuenf
> +  eins zwei None None None
> +  $ hg debugwireargs http://localhost:$HGPORT1/ un deux trois onethousandcharactersxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> +  un deux trois onethousandcharactersxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx None
> +  $ cat error.log
> +  $ cat access.log
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:39 (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:39 (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:43 (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:43 (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:27 (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:27 (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17 (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17 (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17 (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17 (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:1033 (glob)
> +  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:1033 (glob)
> +  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
>    * - - [*] "GET /?cmd=debugwireargs HTTP/1.1" 200 - x-hgarg-1:four=quatre&one=un&three=trois&two=deux (glob)
>    * - - [*] "GET /?cmd=debugwireargs HTTP/1.1" 200 - x-hgarg-1:four=quatre&one=un&three=trois&two=deux (glob)
>    * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Augie Fackler - March 15, 2016, 12:58 a.m.
Thanks for the review. I answered your question about implementing
this for unbundle, please feel encouraged to ask questions if I wasn't
clear enough.

On Mon, Mar 14, 2016 at 03:07:04PM -0700, Martin von Zweigbergk wrote:
> On Fri, Mar 11, 2016 at 8:55 AM, Augie Fackler <raf@durin42.com> wrote:
> > # HG changeset patch
> > # User Augie Fackler <augie@google.com>
> > # Date 1457714220 18000
> > #      Fri Mar 11 11:37:00 2016 -0500
> > # Node ID 6782893562a73dc56f4184c3967775178c5bdfb4
> > # Parent  efcc8e438753cddf5af209ae812f77c2affead07
> > # EXP-Topic batch
> > http: support sending hgargs via POST body instead of in GET or headers

[...]
> > diff --git a/mercurial/hgweb/protocol.py b/mercurial/hgweb/protocol.py
> > --- a/mercurial/hgweb/protocol.py
> > +++ b/mercurial/hgweb/protocol.py
> > @@ -45,6 +45,9 @@ class webproto(wireproto.abstractserverp
> >          return [data[k] for k in keys]
> >      def _args(self):
> >          args = self.req.form.copy()
>
> When using GET, these arguments are combined with the ones in HgArgs.
> Is that just for legacy clients, or why do we not need it for POST? (I
> read something about that in httppeer._callpush().)

Good catch.

>
> > +        postlen = int(self.req.env.get('HTTP_X_HGARGS_POST', 0))
> > +        if postlen:
> > +            return cgi.parse_qs(self.req.read(postlen))
>
> Similar question here: For GET, we pass keep_blank_values=True to
> parse_qs(). Can there not be empty values from recent hg clients?

Good catch.

>
> Should we document that these things only apply to old clients?
>
> >          chunks = []
> >          i = 1
> >          while True:
> > diff --git a/mercurial/httppeer.py b/mercurial/httppeer.py
> > --- a/mercurial/httppeer.py
> > +++ b/mercurial/httppeer.py
> > @@ -97,7 +97,22 @@ class httppeer(wireproto.wirepeer):
> >          self.ui.debug("sending %s command\n" % cmd)
> >          q = [('cmd', cmd)]
> >          headersize = 0
> > -        if True:
> > +        # Important: don't use self.capable() here or else you end up
> > +        # with infinite recursion when trying to look up capabilities
> > +        # for the first time.
> > +        postargsok = self.caps is not None and 'httppostargs' in self.caps
>
> Do you think "cmd != 'capabilities' and self.capable('httppostargs')"
> would be better? That form seems to assume less about the ordering of
> commands; it should allow POST'ing HgArgs even if 'capabilities' had
> not been called before the current command (whatever that may be). I
> guess it's mostly a hypothetical issue at this point (or maybe not; I
> don't know the signalling well enough).

As far as I know, that's entirely hypothetical.

>
> > +        # TODO: support for httppostargs when data is a file-like
> > +        # object rather than a basestring
>
> How do you imagine we (probably I) would do that? Do I make
> _callpush() prepend the serialized HgArgs to the bundle file?

The rough guess I had was to make a file-like type that prepended some
data to a file-like type.

class prepender(object):
  def __init__(self, prefix, fp):
    self._prefix_pointer = 0
    self._prefix = prefix
    self._fp = fp
  def read(self, amt):
    if self._prefix_pointer >= len(self._prefix):
      return self._fp.read(amt)
    ret = self._prefix[self._prefix_pointer:amt]
    self._prefix_pointer += len(ret)
    if len(ret) < amt:
      ret += self._fp.read(amt - len(ret))
    return ret

implementing prepender.seek() is left as an exercise for the
reader. Also testing, because I wrote that in my mailer and didn't
even check to see if it's valid Python.

>
> > +        canmungedata = not data or isinstance(data, basestring)
> > +        if postargsok and canmungedata:

[...]

> >  # If you are writing an extension and consider wrapping this function. Wrap
> > diff --git a/tests/test-wireproto.t b/tests/test-wireproto.t
> > --- a/tests/test-wireproto.t
> > +++ b/tests/test-wireproto.t
> > @@ -19,7 +19,12 @@ Local:
> >
> >  HTTP:
> >
> > -  $ hg serve -R repo -p $HGPORT -d --pid-file=hg1.pid -E error.log -A access.log
> > +  $ cat >> $HGRCPATH <<EOF
> > +  > [experimental]
> > +  > httppostargs = true
> > +  > EOF
> > +
> > +  $ hg  serve -R repo -p $HGPORT -d --pid-file=hg1.pid -E error.log -A access.log
>
> nit: why not use "--config experimental.httppostargs=true" to save a
> few line here not have to reset it for the next test?

Sure, fixed in the resend.

Patch

diff --git a/mercurial/hgweb/protocol.py b/mercurial/hgweb/protocol.py
--- a/mercurial/hgweb/protocol.py
+++ b/mercurial/hgweb/protocol.py
@@ -45,6 +45,9 @@  class webproto(wireproto.abstractserverp
         return [data[k] for k in keys]
     def _args(self):
         args = self.req.form.copy()
+        postlen = int(self.req.env.get('HTTP_X_HGARGS_POST', 0))
+        if postlen:
+            return cgi.parse_qs(self.req.read(postlen))
         chunks = []
         i = 1
         while True:
diff --git a/mercurial/httppeer.py b/mercurial/httppeer.py
--- a/mercurial/httppeer.py
+++ b/mercurial/httppeer.py
@@ -97,7 +97,22 @@  class httppeer(wireproto.wirepeer):
         self.ui.debug("sending %s command\n" % cmd)
         q = [('cmd', cmd)]
         headersize = 0
-        if True:
+        # Important: don't use self.capable() here or else you end up
+        # with infinite recursion when trying to look up capabilities
+        # for the first time.
+        postargsok = self.caps is not None and 'httppostargs' in self.caps
+        # TODO: support for httppostargs when data is a file-like
+        # object rather than a basestring
+        canmungedata = not data or isinstance(data, basestring)
+        if postargsok and canmungedata:
+            strargs = urllib.urlencode(sorted(args.items()))
+            if strargs:
+                if not data:
+                    data = strargs
+                elif isinstance(data, basestring):
+                    data = strargs + data
+                headers['X-HgArgs-Post'] = len(strargs)
+        else:
             if len(args) > 0:
                 httpheader = self.capable('httpheader')
                 if httpheader:
diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
--- a/mercurial/wireproto.py
+++ b/mercurial/wireproto.py
@@ -688,6 +688,8 @@  def _capabilities(repo, proto):
     caps.append('unbundle=%s' % ','.join(changegroupmod.bundlepriority))
     caps.append(
         'httpheader=%d' % repo.ui.configint('server', 'maxhttpheaderlen', 1024))
+    if repo.ui.configbool('experimental', 'httppostargs', False):
+        caps.append('httppostargs')
     return caps
 
 # If you are writing an extension and consider wrapping this function. Wrap
diff --git a/tests/test-wireproto.t b/tests/test-wireproto.t
--- a/tests/test-wireproto.t
+++ b/tests/test-wireproto.t
@@ -19,7 +19,12 @@  Local:
 
 HTTP:
 
-  $ hg serve -R repo -p $HGPORT -d --pid-file=hg1.pid -E error.log -A access.log
+  $ cat >> $HGRCPATH <<EOF
+  > [experimental]
+  > httppostargs = true
+  > EOF
+
+  $ hg  serve -R repo -p $HGPORT -d --pid-file=hg1.pid -E error.log -A access.log
   $ cat hg1.pid >> $DAEMON_PIDS
 
   $ hg debugwireargs http://localhost:$HGPORT/ un deux trois quatre
@@ -37,6 +42,66 @@  HTTP:
   $ cat error.log
   $ cat access.log
   * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:39 (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:39 (glob)
+  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:43 (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:43 (glob)
+  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:27 (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:27 (glob)
+  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17 (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17 (glob)
+  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17 (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17 (glob)
+  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:1033 (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:1033 (glob)
+
+  $ cat >> $HGRCPATH <<EOF
+  > [experimental]
+  > httppostargs = false
+  > EOF
+
+HTTP without args-in-POST:
+  $ hg serve -R repo -p $HGPORT1 -d --pid-file=hg1.pid -E error.log -A access.log
+  $ cat hg1.pid >> $DAEMON_PIDS
+
+  $ hg debugwireargs http://localhost:$HGPORT1/ un deux trois quatre
+  un deux trois quatre None
+  $ hg debugwireargs http://localhost:$HGPORT1/ \ un deux trois\  qu\ \ atre
+   un deux trois  qu  atre None
+  $ hg debugwireargs http://localhost:$HGPORT1/ eins zwei --four vier
+  eins zwei None vier None
+  $ hg debugwireargs http://localhost:$HGPORT1/ eins zwei
+  eins zwei None None None
+  $ hg debugwireargs http://localhost:$HGPORT1/ eins zwei --five fuenf
+  eins zwei None None None
+  $ hg debugwireargs http://localhost:$HGPORT1/ un deux trois onethousandcharactersxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
+  un deux trois onethousandcharactersxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx None
+  $ cat error.log
+  $ cat access.log
+  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:39 (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:39 (glob)
+  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:43 (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:43 (glob)
+  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:27 (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:27 (glob)
+  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17 (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17 (glob)
+  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17 (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:17 (glob)
+  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:1033 (glob)
+  * - - [*] "POST /?cmd=debugwireargs HTTP/1.1" 200 - x-hgargs-post:1033 (glob)
+  * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
   * - - [*] "GET /?cmd=debugwireargs HTTP/1.1" 200 - x-hgarg-1:four=quatre&one=un&three=trois&two=deux (glob)
   * - - [*] "GET /?cmd=debugwireargs HTTP/1.1" 200 - x-hgarg-1:four=quatre&one=un&three=trois&two=deux (glob)
   * - - [*] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)