[PATCH 5 of 5 postargs] http: support sending hgargs via POST body instead of in GET or headers
raf at durin42.com
Mon Mar 14 20:58:54 EDT 2016
Thanks for the review. I answered your question about implementing
this for unbundle, please feel encouraged to ask questions if I wasn't
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().)
> > + 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).
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.
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):
ret = self._prefix[self._prefix_pointer:amt]
self._prefix_pointer += len(ret)
if len(ret) < amt:
ret += self._fp.read(amt - len(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.
More information about the Mercurial-devel