This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1952

bundlespec: move computing the bundle contentops in parsebundlespec
ClosedPublic

Authored by lothiraldan on Jan 31 2018, 11:25 AM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHG6c7a6b04b274: bundlespec: move computing the bundle contentops in parsebundlespec
Summary

We will introduce a new bundlespec for stream bundle which will influence the
contentops.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

lothiraldan created this revision.Jan 31 2018, 11:25 AM
lothiraldan updated this revision to Diff 5151.Feb 2 2018, 4:30 AM
indygreg requested changes to this revision.Feb 20 2018, 11:48 PM
indygreg added a subscriber: indygreg.

I like where this is going.

It's worth noting that at some point this will reinvent the capabilities mechanism of bundle2. Over the wire protocol today, the client submits its bundle2 capabilities and the server emits parts based on the peer's advertised bundle2 feature support. If you squint, this looks a lot like content options. Have you given any consideration to merging the two concepts and having e.g. a bundlespec map to a pre-defined set of bundle2 capabilities?

mercurial/exchange.py
215

version here does not refer to the changegroup version but rather the bundlespec version.

IMO the changegroup version should be implied by the bundlespec version.

This revision now requires changes to proceed.Feb 20 2018, 11:48 PM
indygreg added a comment.Mar 19 2018, 4:26 PM

I took a bit of a deeper look and found a few more things.

Again, I like the direction of this patch. I think our goal for bundle2 generation should be to derive a set of options for its parts up front as quick as possible and pass that data structure into the lower-level bundle2 generation mechanism.

mercurial/exchange.py
55–57

I *really* like this and this is the direction we need to go. i.e. a bundlespec base version should imply a set of options for generation of the bundle.

111–112

This comment needs updated.

217

At the point we're returning yet another element, it might be worth converting the return into an attr-derived class. We're starting to use those a bit to define data structures that are too large for tuples but don't yet merit a normal class.

lothiraldan added a comment.Mar 26 2018, 4:24 PM
In D1952#38770, @indygreg wrote:

I like where this is going.
It's worth noting that at some point this will reinvent the capabilities mechanism of bundle2. Over the wire protocol today, the client submits its bundle2 capabilities and the server emits parts based on the peer's advertised bundle2 feature support. If you squint, this looks a lot like content options. Have you given any consideration to merging the two concepts and having e.g. a bundlespec map to a pre-defined set of bundle2 capabilities?

Both concepts seem intimately intricated but I was afraid of the complexity of the series that actually merge them and all the potential backward-incompatible implications. Do you have a specific design in mind we could iterate on?

I will update this series according to your feedbacks.

lothiraldan updated this revision to Diff 7374.Mar 30 2018, 8:55 AM
lothiraldan marked 4 inline comments as done.Mar 30 2018, 9:01 AM
indygreg accepted this revision.Mar 30 2018, 1:04 PM

Thanks for following up. This looks great!

This revision is now accepted and ready to land.Mar 30 2018, 1:04 PM
Closed by commit rHG6c7a6b04b274: bundlespec: move computing the bundle contentops in parsebundlespec (authored by lothiraldan). · Explain WhyMar 30 2018, 1:11 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 7374

hgext/lfs/__init__.py

wrapfunction(hg, 'clone', wrapper.hgclone) wrapfunction(hg, 'clone', wrapper.hgclone)
wrapfunction(hg, 'postshare', wrapper.hgpostshare) wrapfunction(hg, 'postshare', wrapper.hgpostshare)
scmutil.fileprefetchhooks.add('lfs', wrapper._prefetchfiles) scmutil.fileprefetchhooks.add('lfs', wrapper._prefetchfiles)
# Make bundle choose changegroup3 instead of changegroup2. This affects # Make bundle choose changegroup3 instead of changegroup2. This affects
# "hg bundle" command. Note: it does not cover all bundle formats like # "hg bundle" command. Note: it does not cover all bundle formats like
# "packed1". Using "packed1" with lfs will likely cause trouble. # "packed1". Using "packed1" with lfs will likely cause trouble.
names = [k for k, v in exchange._bundlespeccgversions.items() if v == '02'] exchange._bundlespeccontentopts["v2"]["cg.version"] = "03"
for k in names:
exchange._bundlespeccgversions[k] = '03'
# bundlerepo uses "vfsmod.readonlyvfs(othervfs)", we need to make sure lfs # bundlerepo uses "vfsmod.readonlyvfs(othervfs)", we need to make sure lfs
# options and blob stores are passed from othervfs to the new readonlyvfs. # options and blob stores are passed from othervfs to the new readonlyvfs.
wrapfunction(vfsmod.readonlyvfs, '__init__', wrapper.vfsinit) wrapfunction(vfsmod.readonlyvfs, '__init__', wrapper.vfsinit)
# when writing a bundle via "hg bundle" command, upload related LFS blobs # when writing a bundle via "hg bundle" command, upload related LFS blobs
wrapfunction(bundle2, 'writenewbundle', wrapper.writenewbundle) wrapfunction(bundle2, 'writenewbundle', wrapper.writenewbundle)

mercurial/bundle2.py

part.addparam('version', cg.version) part.addparam('version', cg.version)
if 'clcount' in cg.extras: if 'clcount' in cg.extras:
part.addparam('nbchanges', '%d' % cg.extras['clcount'], part.addparam('nbchanges', '%d' % cg.extras['clcount'],
mandatory=False) mandatory=False)
if opts.get('phases') and repo.revs('%ln and secret()', if opts.get('phases') and repo.revs('%ln and secret()',
outgoing.missingheads): outgoing.missingheads):
part.addparam('targetphase', '%d' % phases.secret, mandatory=False) part.addparam('targetphase', '%d' % phases.secret, mandatory=False)
if opts.get('tagsfnodescache', True):
addparttagsfnodescache(repo, bundler, outgoing) addparttagsfnodescache(repo, bundler, outgoing)
if opts.get('revbranchcache', True):
addpartrevbranchcache(repo, bundler, outgoing) addpartrevbranchcache(repo, bundler, outgoing)
if opts.get('obsolescence', False): if opts.get('obsolescence', False):
obsmarkers = repo.obsstore.relevantmarkers(outgoing.missing) obsmarkers = repo.obsstore.relevantmarkers(outgoing.missing)
buildobsmarkerspart(bundler, obsmarkers) buildobsmarkerspart(bundler, obsmarkers)
if opts.get('phases', False): if opts.get('phases', False):
headsbyphase = phases.subsetphaseheads(repo, outgoing.missing) headsbyphase = phases.subsetphaseheads(repo, outgoing.missing)
phasedata = phases.binaryencode(headsbyphase) phasedata = phases.binaryencode(headsbyphase)

mercurial/commands.py

bundletype = opts.get('type', 'bzip2').lower() bundletype = opts.get('type', 'bzip2').lower()
try: try:
bundlespec = exchange.parsebundlespec(repo, bundletype, strict=False) bundlespec = exchange.parsebundlespec(repo, bundletype, strict=False)
except error.UnsupportedBundleSpecification as e: except error.UnsupportedBundleSpecification as e:
raise error.Abort(pycompat.bytestr(e), raise error.Abort(pycompat.bytestr(e),
hint=_("see 'hg help bundlespec' for supported " hint=_("see 'hg help bundlespec' for supported "
"values for --type")) "values for --type"))
cgversion = bundlespec.version cgversion = bundlespec.contentopts["cg.version"]
# Packed bundles are a pseudo bundle format for now. # Packed bundles are a pseudo bundle format for now.
if cgversion == 's1': if cgversion == 's1':
raise error.Abort(_('packed bundles cannot be produced by "hg bundle"'), raise error.Abort(_('packed bundles cannot be produced by "hg bundle"'),
hint=_("use 'hg debugcreatestreamclonebundle'")) hint=_("use 'hg debugcreatestreamclonebundle'"))
if opts.get('all'): if opts.get('all'):
if dest: if dest:
# This is a temporary hack to allow adjusting bundle compression # This is a temporary hack to allow adjusting bundle compression
# level without a) formalizing the bundlespec changes to declare it # level without a) formalizing the bundlespec changes to declare it
# b) introducing a command flag. # b) introducing a command flag.
compopts = {} compopts = {}
complevel = ui.configint('experimental', 'bundlecomplevel') complevel = ui.configint('experimental', 'bundlecomplevel')
if complevel is not None: if complevel is not None:
compopts['level'] = complevel compopts['level'] = complevel
# Allow overriding the bundling of obsmarker in phases through
contentopts = {'cg.version': cgversion, 'changegroup': True} # configuration while we don't have a bundle version that include them
if repo.ui.configbool('experimental', 'evolution.bundle-obsmarker'): if repo.ui.configbool('experimental', 'evolution.bundle-obsmarker'):
contentopts['obsolescence'] = True bundlespec.contentopts['obsolescence'] = True
if repo.ui.configbool('experimental', 'bundle-phases'): if repo.ui.configbool('experimental', 'bundle-phases'):
contentopts['phases'] = True bundlespec.contentopts['phases'] = True
bundle2.writenewbundle(ui, repo, 'bundle', fname, bversion, outgoing, bundle2.writenewbundle(ui, repo, 'bundle', fname, bversion, outgoing,
contentopts, compression=bcompression, bundlespec.contentopts, compression=bcompression,
compopts=compopts) compopts=compopts)
@command('cat', @command('cat',
[('o', 'output', '', [('o', 'output', '',
_('print output to file with formatted name'), _('FORMAT')), _('print output to file with formatted name'), _('FORMAT')),
('r', 'rev', '', _('print the given revision'), _('REV')), ('r', 'rev', '', _('print the given revision'), _('REV')),
('', 'decode', None, _('apply any matching decode filter')), ('', 'decode', None, _('apply any matching decode filter')),
] + walkopts + formatteropts, ] + walkopts + formatteropts,

mercurial/exchange.py

# Maps bundle version human names to changegroup versions. # Maps bundle version human names to changegroup versions.
_bundlespeccgversions = {'v1': '01', _bundlespeccgversions = {'v1': '01',
'v2': '02', 'v2': '02',
'packed1': 's1', 'packed1': 's1',
'bundle2': '02', #legacy 'bundle2': '02', #legacy
} }
# Maps bundle version with content opts to choose which part to bundle
_bundlespeccontentopts = {
'v1': {
indygregUnsubmitted
Done

I *really* like this and this is the direction we need to go. i.e. a bundlespec base version should imply a set of options for generation of the bundle.

indygreg: I *really* like this and this is the direction we need to go. i.e. a bundlespec base version…
'changegroup': True,
'cg.version': '01',
'obsolescence': False,
'phases': False,
'tagsfnodescache': False,
'revbranchcache': False
},
'v2': {
'changegroup': True,
'cg.version': '02',
'obsolescence': False,
'phases': False,
'tagsfnodescache': True,
'revbranchcache': True
},
'packed1' : {
'cg.version': 's1'
}
}
_bundlespeccontentopts['bundle2'] = _bundlespeccontentopts['v2']
# Compression engines allowed in version 1. THIS SHOULD NEVER CHANGE. # Compression engines allowed in version 1. THIS SHOULD NEVER CHANGE.
_bundlespecv1compengines = {'gzip', 'bzip2', 'none'} _bundlespecv1compengines = {'gzip', 'bzip2', 'none'}
@attr.s @attr.s
class bundlespec(object): class bundlespec(object):
compression = attr.ib() compression = attr.ib()
version = attr.ib() version = attr.ib()
params = attr.ib() params = attr.ib()
contentopts = attr.ib()
def parsebundlespec(repo, spec, strict=True, externalnames=False): def parsebundlespec(repo, spec, strict=True, externalnames=False):
"""Parse a bundle string specification into parts. """Parse a bundle string specification into parts.
Bundle specifications denote a well-defined bundle/exchange format. Bundle specifications denote a well-defined bundle/exchange format.
The content of a given specification should not change over time in The content of a given specification should not change over time in
order to ensure that bundles produced by a newer version of Mercurial are order to ensure that bundles produced by a newer version of Mercurial are
readable from an older version. readable from an older version.
The string currently has the form: The string currently has the form:
<compression>-<type>[;<parameter0>[;<parameter1>]] <compression>-<type>[;<parameter0>[;<parameter1>]]
Where <compression> is one of the supported compression formats Where <compression> is one of the supported compression formats
and <type> is (currently) a version string. A ";" can follow the type and and <type> is (currently) a version string. A ";" can follow the type and
all text afterwards is interpreted as URI encoded, ";" delimited key=value all text afterwards is interpreted as URI encoded, ";" delimited key=value
pairs. pairs.
If ``strict`` is True (the default) <compression> is required. Otherwise, If ``strict`` is True (the default) <compression> is required. Otherwise,
it is optional. it is optional.
If ``externalnames`` is False (the default), the human-centric names will If ``externalnames`` is False (the default), the human-centric names will
be converted to their internal representation. be converted to their internal representation.
Returns a bundlespec object of (compression, version, parameters). Returns a bundlespec object of (compression, version, parameters).
indygregUnsubmitted
Done

This comment needs updated.

indygreg: This comment needs updated.
Compression will be ``None`` if not in strict mode and a compression isn't Compression will be ``None`` if not in strict mode and a compression isn't
defined. defined.
An ``InvalidBundleSpecification`` is raised when the specification is An ``InvalidBundleSpecification`` is raised when the specification is
not syntactically well formed. not syntactically well formed.
An ``UnsupportedBundleSpecification`` is raised when the compression or An ``UnsupportedBundleSpecification`` is raised when the compression or
bundle type/version is not recognized. bundle type/version is not recognized.
if version == 'packed1' and 'requirements' in params: if version == 'packed1' and 'requirements' in params:
requirements = set(params['requirements'].split(',')) requirements = set(params['requirements'].split(','))
missingreqs = requirements - repo.supportedformats missingreqs = requirements - repo.supportedformats
if missingreqs: if missingreqs:
raise error.UnsupportedBundleSpecification( raise error.UnsupportedBundleSpecification(
_('missing support for repository features: %s') % _('missing support for repository features: %s') %
', '.join(sorted(missingreqs))) ', '.join(sorted(missingreqs)))
# Compute contentopts based on the version
contentopts = _bundlespeccontentopts.get(version, {}).copy()
if not externalnames: if not externalnames:
engine = util.compengines.forbundlename(compression) engine = util.compengines.forbundlename(compression)
compression = engine.bundletype()[1] compression = engine.bundletype()[1]
version = _bundlespeccgversions[version] version = _bundlespeccgversions[version]
return bundlespec(compression, version, params) return bundlespec(compression, version, params, contentopts)
indygregUnsubmitted
Done

version here does not refer to the changegroup version but rather the bundlespec version.

IMO the changegroup version should be implied by the bundlespec version.

indygreg: `version` here does not refer to the changegroup version but rather the bundlespec version.
def readbundle(ui, fh, fname, vfs=None): def readbundle(ui, fh, fname, vfs=None):
header = changegroup.readexactly(fh, 4) header = changegroup.readexactly(fh, 4)
indygregUnsubmitted
Done

At the point we're returning yet another element, it might be worth converting the return into an attr-derived class. We're starting to use those a bit to define data structures that are too large for tuples but don't yet merit a normal class.

indygreg: At the point we're returning yet another element, it might be worth converting the return into…
alg = None alg = None
if not fname: if not fname:
fname = "stream" fname = "stream"
if not header.startswith('HG') and header.startswith('\0'): if not header.startswith('HG') and header.startswith('\0'):
fh = changegroup.headerlessfixup(fh, header) fh = changegroup.headerlessfixup(fh, header)
header = "HG10" header = "HG10"
alg = 'UN' alg = 'UN'

tests/flagprocessorext.py

allsupportedversions) allsupportedversions)
# Teach revlog about our test flags # Teach revlog about our test flags
flags = [REVIDX_NOOP, REVIDX_BASE64, REVIDX_GZIP, REVIDX_FAIL] flags = [REVIDX_NOOP, REVIDX_BASE64, REVIDX_GZIP, REVIDX_FAIL]
revlog.REVIDX_KNOWN_FLAGS |= util.bitsfrom(flags) revlog.REVIDX_KNOWN_FLAGS |= util.bitsfrom(flags)
revlog.REVIDX_FLAGS_ORDER.extend(flags) revlog.REVIDX_FLAGS_ORDER.extend(flags)
# Teach exchange to use changegroup 3 # Teach exchange to use changegroup 3
for k in exchange._bundlespeccgversions.keys(): for k in exchange._bundlespeccontentopts.keys():
exchange._bundlespeccgversions[k] = b'03' exchange._bundlespeccontentopts[k]["cg.version"] = "03"
# Add wrappers for addrevision, responsible to set flags depending on the # Add wrappers for addrevision, responsible to set flags depending on the
# revision data contents. # revision data contents.
wrapfunction(filelog.filelog, 'addrevision', noopaddrevision) wrapfunction(filelog.filelog, 'addrevision', noopaddrevision)
wrapfunction(filelog.filelog, 'addrevision', b64addrevision) wrapfunction(filelog.filelog, 'addrevision', b64addrevision)
wrapfunction(filelog.filelog, 'addrevision', gzipaddrevision) wrapfunction(filelog.filelog, 'addrevision', gzipaddrevision)
wrapfunction(filelog.filelog, 'addrevision', failaddrevision) wrapfunction(filelog.filelog, 'addrevision', failaddrevision)