This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6851

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

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

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
martinvonz updated this revision to Diff 16544.Sep 14 2019, 1:14 PM
martinvonz added a comment.Sep 16 2019, 12:49 AM

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.
pulkit added subscribers: idlsoft, pulkit.Sep 16 2019, 1:05 PM

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 added a comment.Sep 16 2019, 1:31 PM

@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?

martinvonz added a comment.Sep 16 2019, 1:39 PM
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.

pulkit added a comment.Sep 16 2019, 1:46 PM
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!

idlsoft added a comment.Sep 16 2019, 2:03 PM

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.

martinvonz added a comment.Sep 16 2019, 2:14 PM
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.

pulkit accepted this revision.Sep 17 2019, 1:45 PM
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

Revision Contents
Changeset List

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 ==