This is an archive of the discontinued Mercurial Phabricator instance.

narrow: don't hexify paths and double-hexify known nodes on wire (BC)
ClosedPublic

Authored by martinvonz on Sep 14 2019, 1:11 PM.

Download Raw Diff

Details

Reviewers

durin42
pulkit

Group Reviewers

hg-reviewers

Commits

rHGc2676b5a9f59: narrow: don't hexify paths and double-hexify known nodes on wire (BC)

Summary

It isn't obvious, but wireprototypes.encodelist() is meant only for
binary nodeids. So when we used it for encoding hex nodeids and paths,
the encoded result was surprising and hard to read.

This patch changes the encoding to make the list of paths a
comma-separated list and the list of common nodes to be a
encodelist()-encoded list of binary nodeids (so the result is just
singly-hexified nodeids).

This is clearly a breaking change, but the feature is experimental and
we're not aware of anyone running a server using this command yet.

Diff Detail

Repository

rHG Mercurial

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Sep 14 2019, 1:11 PM

Herald added a reviewer: durin42. · View Herald TranscriptSep 14 2019, 1:11 PM

Herald added a reviewer: hg-reviewers. · View Herald Transcript

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

martinvonz updated this revision to Diff 16544.Sep 14 2019, 1:14 PM

Hmm, I just noticed that we separate these paths by commas (not nulls) in other places, so I'll make this consistent with that. I guess that means we don't support paths containing commas for includes or excludes, but fixing that is a separate problem.

martinvonz edited the summary of this revision. (Show Details)Sep 16 2019, 12:49 AM

martinvonz updated this revision to Diff 16553.

This is clearly a breaking change, but the feature is experimental and
we're not aware of anyone running a server using this command yet.

@idlsoft and their company does use narrow extension. @idlsoft can you upgrade server and client at the same time?

@idlsoft and their company does use narrow extension. @idlsoft can you upgrade server and client at the same time?

I did that a little while ago to move to 5.0. It was not fun. It's server, teamcity, clients, docker images.
What operations does this affect? Regular push/pull/clone or only tracked --add-include?

In D6851#100666, @idlsoft wrote:

@idlsoft and their company does use narrow extension. @idlsoft can you upgrade server and client at the same time?

I did that a little while ago to move to 5.0. It was not fun. It's server, teamcity, clients, docker images.
What operations does this affect? Regular push/pull/clone or only tracked --add-include?

Just tracked --add-include. A workaround to simplify the upgrade would be to change wireprototypes.SUPPORTED_ELLIPSESCAP to be (ELLIPSESCAP1, ) on the server from now until all clients have upgraded. But that may still be annoying and error-prone for you to deal with. @pulkit, I suppose we should just add a exp-narrow-2 capability to deal with this? It doesn't seem fair to make @idlsoft deal with it.

In D6851#100667, @martinvonz wrote:

In D6851#100666, @idlsoft wrote:

@idlsoft and their company does use narrow extension. @idlsoft can you upgrade server and client at the same time?

I did that a little while ago to move to 5.0. It was not fun. It's server, teamcity, clients, docker images.
What operations does this affect? Regular push/pull/clone or only tracked --add-include?

Just tracked --add-include. A workaround to simplify the upgrade would be to change wireprototypes.SUPPORTED_ELLIPSESCAP to be (ELLIPSESCAP1, ) on the server from now until all clients have upgraded. But that may still be annoying and error-prone for you to deal with. @pulkit, I suppose we should just add a exp-narrow-2 capability to deal with this? It doesn't seem fair to make @idlsoft deal with it.

Sounds like a good idea!

Just tracked --add-include. A workaround to simplify the upgrade would be to change wireprototypes.SUPPORTED_ELLIPSESCAP to be (ELLIPSESCAP1, ) on the server from now until all clients have upgraded. But that may still be annoying and error-prone for you to deal with. @pulkit, I suppose we should just add a exp-narrow-2 capability to deal with this? It doesn't seem fair to make @idlsoft deal with it.

Sounds like a good idea!

If it's just tracked --add-include then it's not a big deal, it won't disrupt regular flow.
If backward compatibility doesn't complicate the code - great, if not - don't worry about it.

In D6851#100690, @idlsoft wrote:

Just tracked --add-include. A workaround to simplify the upgrade would be to change wireprototypes.SUPPORTED_ELLIPSESCAP to be (ELLIPSESCAP1, ) on the server from now until all clients have upgraded. But that may still be annoying and error-prone for you to deal with. @pulkit, I suppose we should just add a exp-narrow-2 capability to deal with this? It doesn't seem fair to make @idlsoft deal with it.

Sounds like a good idea!

If it's just tracked --add-include then it's not a big deal, it won't disrupt regular flow.
If backward compatibility doesn't complicate the code - great, if not - don't worry about it.

It does seem to be more work than I hoped at first, so it would be very much appreciated if you can live with hg tracked --add-include (and --remove-exclude) not working while you transition server and clients.

In D6851#100690, @idlsoft wrote:

Just tracked --add-include. A workaround to simplify the upgrade would be to change wireprototypes.SUPPORTED_ELLIPSESCAP to be (ELLIPSESCAP1, ) on the server from now until all clients have upgraded. But that may still be annoying and error-prone for you to deal with. @pulkit, I suppose we should just add a exp-narrow-2 capability to deal with this? It doesn't seem fair to make @idlsoft deal with it.

Sounds like a good idea!

If it's just tracked --add-include then it's not a big deal, it won't disrupt regular flow.
If backward compatibility doesn't complicate the code - great, if not - don't worry about it.

Great, thanks!

This revision is now accepted and ready to land.Sep 17 2019, 1:45 PM

martinvonz added a commit: rHGc2676b5a9f59: narrow: don't hexify paths and double-hexify known nodes on wire (BC).Sep 17 2019, 3:57 PM

Closed by commit rHGc2676b5a9f59: narrow: don't hexify paths and double-hexify known nodes on wire (BC) (authored by martinvonz). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			hgext/narrow/narrowcommands.py (2 lines)
M			hgext/narrow/narrowwirepeer.py (19 lines)
M			relnotes/next (4 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	16543		Sep 14 2019, 1:11 PM	★	★
Diff 2	16544		Sep 14 2019, 1:14 PM	★	★
Diff 3	16553		Sep 16 2019, 12:49 AM	★	★
Diff 4	16569	rHGc2676b5a9f597155506cb750a3ba307db7316b32	Sep 13 2019, 12:22 AM	★	★

Diff 16569

hgext/narrow/narrowcommands.py

	with ds.parentchange():			with ds.parentchange():
	ds.setparents(node.nullid, node.nullid)			ds.setparents(node.nullid, node.nullid)
	if isoldellipses:			if isoldellipses:
	with wrappedextraprepare:			with wrappedextraprepare:
	exchange.pull(repo, remote, heads=common)			exchange.pull(repo, remote, heads=common)
	else:			else:
	known = []			known = []
	if ellipsesremote:			if ellipsesremote:
	known = [node.hex(ctx.node()) for ctx in			known = [ctx.node() for ctx in
	repo.set('::%ln', common)			repo.set('::%ln', common)
	if ctx.node() != node.nullid]			if ctx.node() != node.nullid]
	with remote.commandexecutor() as e:			with remote.commandexecutor() as e:
	bundle = e.callcommand('narrow_widen', {			bundle = e.callcommand('narrow_widen', {
	'oldincludes': oldincludes,			'oldincludes': oldincludes,
	'oldexcludes': oldexcludes,			'oldexcludes': oldexcludes,
	'newincludes': newincludes,			'newincludes': newincludes,
	'newexcludes': newexcludes,			'newexcludes': newexcludes,

hgext/narrow/narrowwirepeer.py

	# narrowwirepeer.py - passes narrow spec with unbundle command			# narrowwirepeer.py - passes narrow spec with unbundle command
	#			#
	# Copyright 2017 Google, Inc.			# Copyright 2017 Google, Inc.
	#			#
	# 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

	from mercurial import (			from mercurial import (
	bundle2,			bundle2,
	error,			error,
	extensions,			extensions,
	hg,			hg,
	narrowspec,			narrowspec,
	node as nodemod,
	pycompat,			pycompat,
	wireprototypes,			wireprototypes,
	wireprotov1peer,			wireprotov1peer,
	wireprotov1server,			wireprotov1server,
	)			)

	from . import narrowbundle2			from . import narrowbundle2

	commonheads: list of heads which are common between the server and client			commonheads: list of heads which are common between the server and client
	cgversion(maybe): the changegroup version to produce			cgversion(maybe): the changegroup version to produce
	known: list of nodes which are known on the client (used in ellipses cases)			known: list of nodes which are known on the client (used in ellipses cases)
	ellipses: whether to send ellipses data or not			ellipses: whether to send ellipses data or not
	"""			"""

	preferuncompressed = False			preferuncompressed = False
	try:			try:
	oldincludes = wireprototypes.decodelist(oldincludes)			def splitpaths(data):
	newincludes = wireprototypes.decodelist(newincludes)			# work around ''.split(',') => ['']
	oldexcludes = wireprototypes.decodelist(oldexcludes)			return data.split(b',') if data else []
	newexcludes = wireprototypes.decodelist(newexcludes)			oldincludes = splitpaths(oldincludes)
				newincludes = splitpaths(newincludes)
				oldexcludes = splitpaths(oldexcludes)
				newexcludes = splitpaths(newexcludes)
	# validate the patterns			# validate the patterns
	narrowspec.validatepatterns(set(oldincludes))			narrowspec.validatepatterns(set(oldincludes))
	narrowspec.validatepatterns(set(newincludes))			narrowspec.validatepatterns(set(newincludes))
	narrowspec.validatepatterns(set(oldexcludes))			narrowspec.validatepatterns(set(oldexcludes))
	narrowspec.validatepatterns(set(newexcludes))			narrowspec.validatepatterns(set(newexcludes))

	common = wireprototypes.decodelist(commonheads)			common = wireprototypes.decodelist(commonheads)
	known = wireprototypes.decodelist(known)			known = wireprototypes.decodelist(known)
	known = {nodemod.bin(n) for n in known}
	if ellipses == '0':			if ellipses == '0':
	ellipses = False			ellipses = False
	else:			else:
	ellipses = bool(ellipses)			ellipses = bool(ellipses)
	cgversion = cgversion			cgversion = cgversion

	bundler = bundle2.bundle20(repo.ui)			bundler = bundle2.bundle20(repo.ui)
	if not ellipses:			if not ellipses:
	bundler.addpart(bundle2.bundlepart('error:abort', manargs, advargs))			bundler.addpart(bundle2.bundlepart('error:abort', manargs, advargs))
	preferuncompressed = True			preferuncompressed = True

	chunks = bundler.getchunks()			chunks = bundler.getchunks()
	return wireprototypes.streamres(gen=chunks,			return wireprototypes.streamres(gen=chunks,
	prefer_uncompressed=preferuncompressed)			prefer_uncompressed=preferuncompressed)

	def peernarrowwiden(remote, **kwargs):			def peernarrowwiden(remote, **kwargs):
	for ch in (r'oldincludes', r'newincludes', r'oldexcludes', r'newexcludes',			for ch in (r'commonheads', r'known'):
	r'commonheads', r'known'):
	kwargs[ch] = wireprototypes.encodelist(kwargs[ch])			kwargs[ch] = wireprototypes.encodelist(kwargs[ch])

				for ch in (r'oldincludes', r'newincludes', r'oldexcludes', r'newexcludes'):
				kwargs[ch] = b','.join(kwargs[ch])

	kwargs[r'ellipses'] = '%i' % bool(kwargs[r'ellipses'])			kwargs[r'ellipses'] = '%i' % bool(kwargs[r'ellipses'])
	f = remote._callcompressable('narrow_widen', **kwargs)			f = remote._callcompressable('narrow_widen', **kwargs)
	return bundle2.getunbundler(remote.ui, f)			return bundle2.getunbundler(remote.ui, f)

relnotes/next

	== Backwards Compatibility Changes ==			== Backwards Compatibility Changes ==

	* A shell that supports `$(command)`` syntax for command substitution is now			* A shell that supports `$(command)`` syntax for command substitution is now
	required for running the test suite. The test runner normally uses			required for running the test suite. The test runner normally uses
	`sh`, so if that is a shell that doesn't support `$(command)` syntax,			`sh`, so if that is a shell that doesn't support `$(command)` syntax,
	you can override it by setting `$HGTEST_SHELL` or by passing it to			you can override it by setting `$HGTEST_SHELL` or by passing it to
	`run-tests.py --shell <shell>`.			`run-tests.py --shell <shell>`.

				* The (experimental) narrow extension's wire protocol changed. If
				you're using it, you'll need to make sure to upgrade server and
				client at the same time.

	== Internal API Changes ==			== Internal API Changes ==