This is an archive of the discontinued Mercurial Phabricator instance.

Differential D231

httppeer: add support for httppostargs when we're sending a file
ClosedPublic

Authored by durin42 on Aug 4 2017, 5:05 PM.

Download Raw Diff

Details

Reviewers

martinvonz

Group Reviewers

hg-reviewers

Commits

rHG3c91cc0c5fde: httppeer: add support for httppostargs when we're sending a file

Summary

This is probably only used in the 'unbundle' command, but the code
ended up being cleaner to make it generic and treat *all* httppostargs
with a non-args request body as though they were file-like in
nature. It also means we get test coverage more or less for free. A
previous version of this change didn't use io.BytesIO, and it was a
lot more complicated.

This also fixes a server-side bug, so anyone using httppostargs should
update all of their servers to this revision or later *before* this
gets to their clients, otherwise servers will hang trying to over-read
the POST body.

Diff Detail

Repository

rHG Mercurial

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

durin42 created this revision.Aug 4 2017, 5:05 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptAug 4 2017, 5:05 PM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

martinvonz added a subscriber: martinvonz.Aug 5 2017, 1:04 AM

martinvonz added inline comments.

mercurial/httppeer.py
95	I'm guessing 'fileprepender' here and a few placed below is the old name for '_multifile' (so please update)
96	It doesn't matter much since it's just a programming error if it happens, but how will these arguments to ValueError be rendered?
105	i read that negative amount means the same thing (read all). do we care to follow that contract here?
111	nit: i think this can be "if got <= amt" to avoid an unnecessary 0-length read the next time _multifile.read() (and it does look like that will be the normal case, that the reader will read exactly the size of the first "file" first)
125	also need to set self._index=0, no?
189–190	Nit: to reduce indentation, it looks like you can change the condition above to "if postargsok and args" instead (because bool(strargs) == bool(args) AFAICT)

durin42 marked 3 inline comments as done.Aug 8 2017, 10:41 AM

durin42 updated this revision to Diff 627.

durin42 added inline comments.Aug 8 2017, 10:41 AM

mercurial/httppeer.py
96	ValueError: ('_multifile only supports file objects that have a length but this one does not:', <type 'file'>, <open file 'README', mode 'r' at 0x110ff0b70>) It ain't pretty, but it's got all the information we need.
111	No, because consider these file buffers: 'foo', 'bar' if we read(2), we'll pull 2 bytes off the first buffer, which is not yet exhausted, but got == amt.

durin42 updated this revision to Diff 745.Aug 10 2017, 4:04 PM

martinvonz accepted this revision.Aug 15 2017, 4:31 PM

This revision is now accepted and ready to land.Aug 15 2017, 4:31 PM

Closed by commit rHG3c91cc0c5fde: httppeer: add support for httppostargs when we're sending a file (authored by durin42). · Explain WhyAug 15 2017, 4:55 PM

This revision was automatically updated to reflect the committed changes.

yuja added a subscriber: yuja.Aug 21 2017, 9:21 AM

yuja added inline comments.

mercurial/httppeer.py
105	Just nits: `read(0)` should return an empty string. `None <= 0` is TypeError on Python 3.
199	Does this mean `data` must have `length` attribute if it isn't a string? A plain file object has no such attribute.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/hgweb/protocol.py (3 lines)
M			mercurial/httppeer.py (62 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	559		Aug 4 2017, 5:05 PM	★	★
Diff 2	627		Aug 8 2017, 10:41 AM	★	★
Diff 3	745		Aug 10 2017, 4:04 PM	★	★
Diff 4	949	rHG3c91cc0c5fde70a43703787c4d2934b8dd3f2704	Jul 26 2017, 5:58 PM	★	★

	Status	Author	Revision
	Closed	durin42	D231 httppeer: add support for httppostargs when we're sending a file
	Closed	durin42	D230 http: add a test of actually pushing with httppostargs

Diff 745

mercurial/hgweb/protocol.py

	self.req.read(postlen), keep_blank_values=True))			self.req.read(postlen), keep_blank_values=True))
	return args			return args

	argvalue = decodevaluefromheaders(self.req, 'X-HgArg')			argvalue = decodevaluefromheaders(self.req, 'X-HgArg')
	args.update(cgi.parse_qs(argvalue, keep_blank_values=True))			args.update(cgi.parse_qs(argvalue, keep_blank_values=True))
	return args			return args
	def getfile(self, fp):			def getfile(self, fp):
	length = int(self.req.env['CONTENT_LENGTH'])			length = int(self.req.env['CONTENT_LENGTH'])
				# If httppostargs is used, we need to read Content-Length
				# minus the amount that was consumed by args.
				length -= int(self.req.env.get('HTTP_X_HGARGS_POST', 0))
	for s in util.filechunkiter(self.req, limit=length):			for s in util.filechunkiter(self.req, limit=length):
	fp.write(s)			fp.write(s)
	def redirect(self):			def redirect(self):
	self.oldio = self.ui.fout, self.ui.ferr			self.oldio = self.ui.fout, self.ui.ferr
	self.ui.ferr = self.ui.fout = stringio()			self.ui.ferr = self.ui.fout = stringio()
	def restore(self):			def restore(self):
	val = self.ui.fout.getvalue()			val = self.ui.fout.getvalue()
	self.ui.ferr, self.ui.fout = self.oldio			self.ui.ferr, self.ui.fout = self.oldio

mercurial/httppeer.py

	# httppeer.py - HTTP repository proxy classes for mercurial			# httppeer.py - HTTP repository proxy classes for mercurial
	#			#
	# Copyright 2005, 2006 Matt Mackall <mpm@selenic.com>			# Copyright 2005, 2006 Matt Mackall <mpm@selenic.com>
	# Copyright 2006 Vadim Gelfer <vadim.gelfer@gmail.com>			# Copyright 2006 Vadim Gelfer <vadim.gelfer@gmail.com>
	#			#
	# This software may be used and distributed according to the terms of the			# This software may be used and distributed according to the terms of the
	# GNU General Public License version 2 or any later version.			# GNU General Public License version 2 or any later version.

	from __future__ import absolute_import			from __future__ import absolute_import

	import errno			import errno
				import io
	import os			import os
	import socket			import socket
	import struct			import struct
	import tempfile			import tempfile

	from .i18n import _			from .i18n import _
	from .node import nullid			from .node import nullid
	from . import (			from . import (
	raise error.PeerTransportError(			raise error.PeerTransportError(
	_('HTTP request error (%s)') % e,			_('HTTP request error (%s)') % e,
	hint=_('this may be an intermittent network failure; '			hint=_('this may be an intermittent network failure; '
	'if the error persists, consider contacting the '			'if the error persists, consider contacting the '
	'network or server operator'))			'network or server operator'))

	resp.__class__ = readerproxy			resp.__class__ = readerproxy

				class _multifile(object):
				def __init__(self, *fileobjs):
				for f in fileobjs:
				if not util.safehasattr(f, 'length'):
				raise ValueError(
				'_multifile only supports file objects that '
				martinvonzUnsubmitted Done I'm guessing 'fileprepender' here and a few placed below is the old name for '_multifile' (so please update) martinvonz: I'm guessing 'fileprepender' here and a few placed below is the old name for '_multifile' (so…
				'have a length but this one does not:', type(f), f)
				martinvonzUnsubmitted Not Done It doesn't matter much since it's just a programming error if it happens, but how will these arguments to ValueError be rendered? martinvonz: It doesn't matter much since it's just a programming error if it happens, but how will these…
				durin42AuthorUnsubmitted Not Done ValueError: ('_multifile only supports file objects that have a length but this one does not:', <type 'file'>, <open file 'README', mode 'r' at 0x110ff0b70>) It ain't pretty, but it's got all the information we need. durin42: ValueError: ('_multifile only supports file objects that have a length but this one does not:'…
				self._fileobjs = fileobjs
				self._index = 0

				@property
				def length(self):
				return sum(f.length for f in self._fileobjs)

				def read(self, amt=None):
				if amt <= 0:
				martinvonzUnsubmitted Not Done i read that negative amount means the same thing (read all). do we care to follow that contract here? martinvonz: i read that negative amount means the same thing (read all). do we care to follow that contract…
				yujaUnsubmitted Not Done Just nits: `read(0)` should return an empty string. `None <= 0` is TypeError on Python 3. yuja: Just nits: * `read(0)` should return an empty string. * `None <= 0` is TypeError on Python 3.
				return ''.join(f.read() for f in self._fileobjs)
				parts = []
				while amt and self._index < len(self._fileobjs):
				parts.append(self._fileobjs[self._index].read(amt))
				got = len(parts[-1])
				if got < amt:
				martinvonzUnsubmitted Not Done nit: i think this can be "if got <= amt" to avoid an unnecessary 0-length read the next time _multifile.read() (and it does look like that will be the normal case, that the reader will read exactly the size of the first "file" first) martinvonz: nit: i think this can be "if got <= amt" to avoid an unnecessary 0-length read the next time…
				durin42AuthorUnsubmitted Not Done No, because consider these file buffers: 'foo', 'bar' if we read(2), we'll pull 2 bytes off the first buffer, which is not yet exhausted, but got == amt. durin42: No, because consider these file buffers: 'foo', 'bar' if we read(2), we'll pull 2 bytes off…
				self._index += 1
				amt -= got
				return ''.join(parts)

				def seek(self, offset, whence=os.SEEK_SET):
				if whence != os.SEEK_SET:
				raise NotImplementedError(
				'_multifile does not support anything other'
				' than os.SEEK_SET for whence on seek()')
				if offset != 0:
				raise NotImplementedError(
				'_multifile only supports seeking to start, but that '
				'could be fixed if you need it')
				for f in self._fileobjs:
				martinvonzUnsubmitted Done also need to set self._index=0, no? martinvonz: also need to set self._index=0, no?
				f.seek(0)
				self._index = 0

	class httppeer(wireproto.wirepeer):			class httppeer(wireproto.wirepeer):
	def __init__(self, ui, path):			def __init__(self, ui, path):
	self._path = path			self._path = path
	self._caps = None			self._caps = None
	self._urlopener = None			self._urlopener = None
	self._requestbuilder = None			self._requestbuilder = None
	u = util.url(path)			u = util.url(path)
	if u.query or u.fragment:			if u.query or u.fragment:
	self.ui.debug("sending %s command\n" % cmd)			self.ui.debug("sending %s command\n" % cmd)
	q = [('cmd', cmd)]			q = [('cmd', cmd)]
	headersize = 0			headersize = 0
	varyheaders = []			varyheaders = []
	# Important: don't use self.capable() here or else you end up			# Important: don't use self.capable() here or else you end up
	# with infinite recursion when trying to look up capabilities			# with infinite recursion when trying to look up capabilities
	# for the first time.			# for the first time.
	postargsok = self._caps is not None and 'httppostargs' in self._caps			postargsok = self._caps is not None and 'httppostargs' in self._caps
	# TODO: support for httppostargs when data is a file-like			if postargsok and args:
	# object rather than a basestring
	canmungedata = not data or isinstance(data, basestring)
	if postargsok and canmungedata:
	strargs = urlreq.urlencode(sorted(args.items()))			strargs = urlreq.urlencode(sorted(args.items()))
	if strargs:
	if not data:			if not data:
				martinvonzUnsubmitted Done Nit: to reduce indentation, it looks like you can change the condition above to "if postargsok and args" instead (because bool(strargs) == bool(args) AFAICT) martinvonz: Nit: to reduce indentation, it looks like you can change the condition above to "if postargsok…
	data = strargs			data = strargs
	elif isinstance(data, basestring):			else:
	data = strargs + data			if isinstance(data, basestring):
				i = io.BytesIO(data)
				i.length = len(data)
				data = i
				argsio = io.BytesIO(strargs)
				argsio.length = len(strargs)
				data = _multifile(argsio, data)
				yujaUnsubmitted Not Done Does this mean `data` must have `length` attribute if it isn't a string? A plain file object has no such attribute. yuja: Does this mean `data` must have `length` attribute if it isn't a string? A plain file object…
	headers['X-HgArgs-Post'] = len(strargs)			headers['X-HgArgs-Post'] = len(strargs)
	else:			else:
	if len(args) > 0:			if len(args) > 0:
	httpheader = self.capable('httpheader')			httpheader = self.capable('httpheader')
	if httpheader:			if httpheader:
	headersize = int(httpheader.split(',', 1)[0])			headersize = int(httpheader.split(',', 1)[0])
	if headersize > 0:			if headersize > 0:
	# The headers can typically carry more data than the URL.			# The headers can typically carry more data than the URL.
	encargs = urlreq.urlencode(sorted(args.items()))			encargs = urlreq.urlencode(sorted(args.items()))