This is an archive of the discontinued Mercurial Phabricator instance.

Having a cborutil.decodeone() function which can make sure we only have one dcoded value and using that fn instead of decodeall()[0]
A cborutil.encode() function to use where we don't want to do streamencode() and use that in place of b''.join() pattern
Checking for an empty string being passed in cborutil.decodeall()[0] or cborutil.decodeone()

What do you think?

Closed by commit rHGcdb56f295b03: httppeer: use our CBOR decoder (authored by indygreg). · Explain WhySep 5 2018, 8:23 AM

This revision was automatically updated to reflect the committed changes.

In D4465#68517, @pulkit wrote:

Queued the cbor patches. I think following can improve the code more:

Having a cborutil.decodeone() function which can make sure we only have one dcoded value and using that fn instead of decodeall()[0]

A cborutil.encode() function to use where we don't want to do streamencode() and use that in place of b''.join() pattern

Checking for an empty string being passed in cborutil.decodeall()[0] or cborutil.decodeone()

What do you think?

I agree with all these suggestions. I also think we should teach the sansiodecoder to decode at most one item and use that functionality to build cborutil.decodeone().

FWIW one concern with going down that path is buffer over-read: if we're trying to decode a CBOR value from a stream without advancing past the end of the value, that can be difficult to do without feeding in data in small chunks. But it is doable.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/httppeer.py (8 lines)

Commit	Parents	Author	Summary	Date
		Gregory Szorc		Aug 28 2018, 6:10 PM

Status	Author	Revision
Closed	indygreg	D4492 wireprotov2: let clients drive delta behavior
Closed	indygreg	D4491 exchangev2: fetch file revisions
Closed	indygreg	D4490 wireprotov2: define and implement "filedata" command
Closed	indygreg	D4489 exchangev2: fetch manifest revisions
Closed	indygreg	D4488 wireprotov2: define and implement "manifestdata" command
Closed	indygreg	D4487 wireprotov2: add TODOs around extending changesetdata fields
Closed	indygreg	D4486 exchangev2: fetch and apply bookmarks
Closed	indygreg	D4485 wireprotov2: add bookmarks to "changesetdata" command
Closed	indygreg	D4484 exchangev2: fetch and apply phases data
Closed	indygreg	D4483 wireprotov2: add phases to "changesetdata" command
Closed	indygreg	D4482 exchangev2: fetch changeset revisions
Closed	indygreg	D4481 wireprotov2: define and implement "changesetdata" command
Closed	indygreg	D4480 exchangev2: start to implement pull with wire protocol v2
Closed	indygreg	D4436 httppeer: expose capabilities for each command
Closed	indygreg	D4474 wireprotov2peer: stream decoded responses
Closed	indygreg	D4473 wireprotoframing: buffer emitted data to reduce frame count
Closed	indygreg	D4472 wireprotov2: implement commands as a generator of objects
Closed	indygreg	D4471 state: use our CBOR module
Closed	indygreg	D4470 wireprotov2peer: use our CBOR decoder
Closed	indygreg	D4469 debugcommands: use our CBOR decoder
Closed	indygreg	D4468 wireprotov2server: use our CBOR encoder
Closed	indygreg	D4467 wireprotoserver: use our CBOR encoder
Closed	indygreg	D4466 wireprotoframing: use our CBOR module
Closed	indygreg	D4465 httppeer: use our CBOR decoder
Closed	indygreg	D4443 internals: extract frame-based protocol docs to own document
Closed	indygreg	D4442 internals: extract wire protocol version 2 commands to standalone doc
Closed	indygreg	D4441 wireprotov2peer: properly format errors
Closed	indygreg	D4440 wireprotov2peer: report exceptions in frame handling against request future
Closed	indygreg	D4439 wireprotov2peer: split responsedata handling into separate function
Closed	indygreg	D4438 httppeer: add TODO about delayed handling of ^C
Closed	indygreg	D4437 httppeer: log commands for version 2 peer
Closed	indygreg	D4435 wireprotov2peer: add TODO about streaming responses
Closed	indygreg	D4434 cborutil: add a buffering decoder
Closed	indygreg	D4433 cborutil: remove readindefinitebytestringtoiter()
Closed	indygreg	D4414 cborutil: implement sans I/O decoder
Closed	indygreg	D4413 tests: remove use of string in CBOR test
Closed	indygreg	D4412 internals: document CBOR utilization

Diff 10762

mercurial/httppeer.py

	import errno			import errno
	import io			import io
	import os			import os
	import socket			import socket
	import struct			import struct
	import weakref			import weakref

	from .i18n import _			from .i18n import _
	from .thirdparty import (
	cbor,
	)
	from . import (			from . import (
	bundle2,			bundle2,
	error,			error,
	httpconnection,			httpconnection,
	pycompat,			pycompat,
	repository,			repository,
	statichttprepo,			statichttprepo,
	url as urlmod,			url as urlmod,
	util,			util,
	wireprotoframing,			wireprotoframing,
	wireprototypes,			wireprototypes,
	wireprotov1peer,			wireprotov1peer,
	wireprotov2peer,			wireprotov2peer,
	wireprotov2server,			wireprotov2server,
	)			)
	from .utils import (			from .utils import (
				cborutil,
	interfaceutil,			interfaceutil,
	stringutil,			stringutil,
	)			)

	httplib = util.httplib			httplib = util.httplib
	urlerr = util.urlerr			urlerr = util.urlerr
	urlreq = util.urlreq			urlreq = util.urlreq

	resp.close()			resp.close()

	if not ct.startswith('application/mercurial-'):			if not ct.startswith('application/mercurial-'):
	raise error.ProgrammingError('unexpected content-type: %s' % ct)			raise error.ProgrammingError('unexpected content-type: %s' % ct)

	if advertisev2:			if advertisev2:
	if ct == 'application/mercurial-cbor':			if ct == 'application/mercurial-cbor':
	try:			try:
	info = cbor.loads(rawdata)			info = cborutil.decodeall(rawdata)[0]
	except cbor.CBORDecodeError:			except cborutil.CBORDecodeError:
	raise error.Abort(_('error decoding CBOR from remote server'),			raise error.Abort(_('error decoding CBOR from remote server'),
	hint=_('try again and consider contacting '			hint=_('try again and consider contacting '
	'the server operator'))			'the server operator'))

	# We got a legacy response. That's fine.			# We got a legacy response. That's fine.
	elif ct in ('application/mercurial-0.1', 'application/mercurial-0.2'):			elif ct in ('application/mercurial-0.1', 'application/mercurial-0.2'):
	info = {			info = {
	'v1capabilities': set(rawdata.split())			'v1capabilities': set(rawdata.split())

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	10762		Sep 4 2018, 2:28 PM	★	★
Diff 2	10782	rHGcdb56f295b031ace8a12bc5a08a6e8a4e2da6a66	Aug 28 2018, 6:10 PM	★	★

This is an archive of the discontinued Mercurial Phabricator instance.

httppeer: use our CBOR decoderClosedPublic

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 10762

mercurial/httppeer.py

httppeer: use our CBOR decoder
ClosedPublic

Revision Contents
Changeset List