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

[Augie Fackler]

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 at durin42.com> wrote:
> > # HG changeset patch
> > # User Augie Fackler <augie at 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.