This is an archive of the discontinued Mercurial Phabricator instance.

pycompat: prevent encoding or decoding values if not required
ClosedPublic

Authored by pulkit on Feb 27 2018, 4:36 AM.

Download Raw Diff

Details

Reviewers

yuja
durin42

Group Reviewers

hg-reviewers

Commits

rHGe2b87e19c6ef: pycompat: prevent encoding or decoding values if not required

Summary

pycompat.py has functions strurl and bytesurl which decodes and encodes the url
passed on Python 3 respectively. In some cases, strurl gets a url which is
already str and bytesurl gets a url which is already bytes. Let's prevent
encoding or decoding the values again if not required.

Diff Detail

Repository

rHG Mercurial

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

pulkit created this revision.Feb 27 2018, 4:36 AM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptFeb 27 2018, 4:36 AM

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

pulkit added a child revision: D2473: py3: use pycompat.strurl to convert url to str.Feb 27 2018, 4:36 AM

pulkit added a child revision: D2474: py3: use pycompat.strurl to convert url to str.Feb 27 2018, 4:47 AM

I'm 0 for this move.

I'm not a fan of making function inputs and outputs obscure because we'll
be likely to have a hard time to fix it in future. Mixing bytes and unicodes is
horrible. But if it's painful to be strict on url type, I'll say go for it.

mercurial/pycompat.py
197	Need to return an unmodified value.

This revision now requires changes to proceed.Feb 27 2018, 7:51 AM

pulkit updated this revision to Diff 6176.Feb 27 2018, 12:09 PM

I agree with @yuja here. I think we should strive to convert URLs to bytes (like most other internal types). Although I do realize that lots of stdlib code has... expectations. Is there a particular test failure you are tracking down with this fix? I've kind of got a lot of the wire protocol code paged in right now. I could potentially offer some advice.

In D2472#40379, @indygreg wrote:

I agree with @yuja here. I think we should strive to convert URLs to bytes (like most other internal types). Although I do realize that lots of stdlib code has... expectations. Is there a particular test failure you are tracking down with this fix? I've kind of got a lot of the wire protocol code paged in right now. I could potentially offer some advice.

test-http-bundle1.t failures. I think this current patch is probably the "least bad" option (barring getting pytype working, which I think would let us try and constrain some types in more principled ways) since some of these URL-relevant functions are called by both hg and urllib guts. The former wants to use bytes, and the latter can only use unicodes.

I've poked at this on and off a few times and something like this fix is all I've come up with. I'm not happy about it either.

The other option is probably to always treat URLs as unicodes, though that seems like it'll probably come with its own bag of hurt throughout the codebase.

durin42 accepted this revision.Mar 3 2018, 1:53 PM

Closed by commit rHGe2b87e19c6ef: pycompat: prevent encoding or decoding values if not required (authored by pulkit). · Explain WhyMar 3 2018, 1:58 PM

This revision was automatically updated to reflect the committed changes.

durin42 added a child revision: D2598: httpconnection: convert url to bytes in readauthforuri.Mar 3 2018, 2:30 PM

durin42 updated this revision to Diff 6449.

durin42 removed a child revision: D2598: httpconnection: convert url to bytes in readauthforuri.Mar 3 2018, 2:32 PM

Revision Contents
Changeset List

			Path	Packages
M			mercurial/pycompat.py (6 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	6152		Feb 27 2018, 4:36 AM	★	★
Diff 2	6176		Feb 27 2018, 12:08 PM	★	★
Diff 3	6439	rHGe2b87e19c6efa87f12da17350207ea6d97062cfd	Feb 26 2018, 2:03 PM	★	★
Diff 4	6449		Mar 3 2018, 2:30 PM	★	★

Status	Author	Revision
Closed	pulkit	D2480 py3: use print as a function in tests/test-hgrc.t
Closed	pulkit	D2479 py3: use '%d' to convert integers to bytes
Closed	pulkit	D2478 py3: add b'' prefixes in tests/test-extension.t
Closed	pulkit	D2477 py3: convert os.devnull to bytes using pycompat.bytestr
Closed	pulkit	D2476 py3: fix more keyword arguments handling
Closed	pulkit	D2475 py3: make sure regexes are bytes
Closed	pulkit	D2474 py3: use pycompat.strurl to convert url to str
Abandoned	durin42	D2473 py3: use pycompat.strurl to convert url to str
Closed	pulkit	D2472 pycompat: prevent encoding or decoding values if not required

Diff 6152

mercurial/pycompat.py

	such that 'sysstr(a) != sysstr(b)' for all 'a != b'.			such that 'sysstr(a) != sysstr(b)' for all 'a != b'.
	"""			"""
	if isinstance(s, builtins.str):			if isinstance(s, builtins.str):
	return s			return s
	return s.decode(u'latin-1')			return s.decode(u'latin-1')

	def strurl(url):			def strurl(url):
	"""Converts a bytes url back to str"""			"""Converts a bytes url back to str"""
				if isinstance(url, bytes):
	return url.decode(u'ascii')			return url.decode(u'ascii')

				yujaUnsubmitted Not Done Need to return an unmodified value. yuja: Need to return an unmodified value.
	def bytesurl(url):			def bytesurl(url):
	"""Converts a str url to bytes by encoding in ascii"""			"""Converts a str url to bytes by encoding in ascii"""
				if isinstance(url, str):
	return url.encode(u'ascii')			return url.encode(u'ascii')

	def raisewithtb(exc, tb):			def raisewithtb(exc, tb):
	"""Raise exception with the given traceback"""			"""Raise exception with the given traceback"""
	raise exc.with_traceback(tb)			raise exc.with_traceback(tb)

	def getdoc(obj):			def getdoc(obj):
	"""Get docstring as bytes; may be None so gettext() won't confuse it			"""Get docstring as bytes; may be None so gettext() won't confuse it
	with _('')"""			with _('')"""