This is an archive of the discontinued Mercurial Phabricator instance.

bundle: add the possibility to bundle bookmarks (issue5792)
Needs RevisionPublic

Authored by lothiraldan on Mar 16 2018, 9:21 AM.

Download Raw Diff

Details

Reviewers

indygreg
baymax

Group Reviewers

hg-reviewers

Summary

Also take the wlock when unbundling. It shouldn't have a big impact on
performance.

Diff Detail

Repository

rHG Mercurial

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

lothiraldan created this revision.Mar 16 2018, 9:21 AM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptMar 16 2018, 9:21 AM

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

pulkit added a subscriber: pulkit.Mar 16 2018, 9:27 AM

pulkit added inline comments.

mercurial/configitems.py
422	Any reason why we have all these config option as experimental? We talked at sprint about moving things out of experimental and we agreed on `experimental.bundle-phases`. This one sounds similar.

lothiraldan added a subscriber: indygreg.Mar 16 2018, 9:49 AM

lothiraldan added inline comments.

mercurial/configitems.py
422	I was afraid we might have to delete this option once we have the discussion about bundle spec format with @indygreg. If that would be the case, I would feel more comfortable deleting an experimental option. If we agreed on moving bundle-phases out of experimental, I guess we can do the same for bundle-bookmarks. @indygreg Do you think we would still have this option around once we have new-style bundle spec?

martinvonz added a subscriber: martinvonz.Mar 16 2018, 12:45 PM

martinvonz added inline comments.

tests/test-bundle-bookmarks.t
37–63	Can we have a similar test case where we create divergence? Create a fork in the graph in the debugdrawdag call above. Let's say you have commit F that branches off of B, then do something like this: $ hg bundle --all bundle $ hg strip --no-backup C $ hg bookmarks -f -r F D1 $ hg unbundle -q bundle $ hg log -G -T '{desc} {bookmarks}\n' o E \| o D D1 D2 \| o C \| \| o F D1@1 \|/ o B \| o A A1 I don't know if "@1" is the appropriate divergence marker, but I can't think of a better one. I haven't even looked at your code to try to guess what it would be in practice (if it would work at all).

What's the status of this patch? Is it still reviewable? If so, let's get a rebased version submitted, just in case things have changed.

This revision now requires changes to proceed.Apr 12 2018, 12:16 PM

In D2880#52559, @indygreg wrote:

What's the status of this patch? Is it still reviewable? If so, let's get a rebased version submitted, just in case things have changed.

I need to update the patch according to @martinvoz comment.

I was also waiting on your comment on https://phab.mercurial-scm.org/D2880#46263, do you think we will keep the option once the bundlespec system is revamped? If yes, we can make the config option a non-experimental one. But if it will be dropped soon, I think keeping it experimental until it's dropped is preferable. What do you think?

lothiraldan updated this revision to Diff 8263.Apr 14 2018, 5:38 AM

lothiraldan added inline comments.Apr 14 2018, 5:40 AM

tests/test-bundle-bookmarks.t
37–63	I have added such test and indeed this case is not handled right now. I'm not aware of how Mercurial handle this case when exchanging bookmarks, I will need to take a look at the code to find out. What is the correct behavior here? Change the existing bookmark or the bookmark from the bundle?

martinvonz added inline comments.Apr 14 2018, 12:41 PM

tests/test-bundle-bookmarks.t
37–63	I think it should be the same thing as pulling from another repo. Try putting the same changes in another repo and pulling from it without giving the path a name (nothing in [paths] config). I haven't checked what suffix will be used on the divergent bookmark, but it probably makes sense to use the same here (perhaps it's just going to be "D1@1" as I guessed before).

lothiraldan added inline comments.Apr 17 2018, 3:41 AM

tests/test-bundle-bookmarks.t
37–63	I tried adding such scenarios but failed to produce a divergence scenario, no idea why. I don't think I will have time to debug the issue before the freeze, so let's skip this changeset for 4.6

Just a reminder that this has not been queued yet.

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Jan 24 2020, 12:32 PM

Revision Contents
Changeset List

		Path
M		mercurial/bundle2.py (8 lines)
M		mercurial/commands.py (4 lines)
M		mercurial/configitems.py (3 lines)
M		mercurial/debugcommands.py (12 lines)
A	M	tests/test-bundle-bookmarks.t (80 lines)

Diff 7077

mercurial/bundle2.py

	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)
	bundler.newpart('phase-heads', data=phasedata)			bundler.newpart('phase-heads', data=phasedata)

				if opts.get('bookmarks', False):
				# From exchange._pushb2bookmarkspart
				data = []
				for book, new in repo._bookmarks.items():
				data.append((book, new))
				checkdata = bookmarks.binaryencode(data)
				bundler.newpart('bookmarks', data=checkdata)

	def addparttagsfnodescache(repo, bundler, outgoing):			def addparttagsfnodescache(repo, bundler, outgoing):
	# we include the tags fnode cache for the bundle changeset			# we include the tags fnode cache for the bundle changeset
	# (as an optional parts)			# (as an optional parts)
	cache = tags.hgtagsfnodescache(repo.unfiltered())			cache = tags.hgtagsfnodescache(repo.unfiltered())
	chunks = []			chunks = []

	# .hgtags fnodes are only relevant for head changesets. While we could			# .hgtags fnodes are only relevant for head changesets. While we could
	# transfer values for all known nodes, there will likely be little to			# transfer values for all known nodes, there will likely be little to

mercurial/commands.py

	compopts['level'] = complevel			compopts['level'] = complevel


	contentopts = {'cg.version': cgversion}			contentopts = {'cg.version': cgversion}
	if repo.ui.configbool('experimental', 'evolution.bundle-obsmarker'):			if repo.ui.configbool('experimental', 'evolution.bundle-obsmarker'):
	contentopts['obsolescence'] = True			contentopts['obsolescence'] = True
	if repo.ui.configbool('experimental', 'bundle-phases'):			if repo.ui.configbool('experimental', 'bundle-phases'):
	contentopts['phases'] = True			contentopts['phases'] = True
				if repo.ui.configbool('experimental', 'bundle-bookmarks'):
				contentopts['bookmarks'] = True
	bundle2.writenewbundle(ui, repo, 'bundle', fname, bversion, outgoing,			bundle2.writenewbundle(ui, repo, 'bundle', fname, bversion, outgoing,
	contentopts, compression=bcompression,			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')),
	"""apply one or more bundle files			"""apply one or more bundle files

	Apply one or more bundle files generated by :hg:`bundle`.			Apply one or more bundle files generated by :hg:`bundle`.

	Returns 0 on success, 1 if an update has unresolved files.			Returns 0 on success, 1 if an update has unresolved files.
	"""			"""
	fnames = (fname1,) + fnames			fnames = (fname1,) + fnames

	with repo.lock():			with repo.wlock(), repo.lock():
	for fname in fnames:			for fname in fnames:
	f = hg.openpath(ui, fname)			f = hg.openpath(ui, fname)
	gen = exchange.readbundle(ui, f, fname)			gen = exchange.readbundle(ui, f, fname)
	if isinstance(gen, streamclone.streamcloneapplier):			if isinstance(gen, streamclone.streamcloneapplier):
	raise error.Abort(			raise error.Abort(
	_('packed bundles cannot be applied with '			_('packed bundles cannot be applied with '
	'"hg unbundle"'),			'"hg unbundle"'),
	hint=_('use "hg debugapplystreamclonebundle"'))			hint=_('use "hg debugapplystreamclonebundle"'))

mercurial/configitems.py

	default=None,			default=None,
	)			)
	coreconfigitem('email', 'to',			coreconfigitem('email', 'to',
	default=None,			default=None,
	)			)
	coreconfigitem('experimental', 'archivemetatemplate',			coreconfigitem('experimental', 'archivemetatemplate',
	default=dynamicdefault,			default=dynamicdefault,
	)			)
				coreconfigitem('experimental', 'bundle-bookmarks',
				pulkitUnsubmitted Not Done Any reason why we have all these config option as experimental? We talked at sprint about moving things out of experimental and we agreed on `experimental.bundle-phases`. This one sounds similar. pulkit: Any reason why we have all these config option as experimental? We talked at sprint about…
				lothiraldanAuthorUnsubmitted Not Done I was afraid we might have to delete this option once we have the discussion about bundle spec format with @indygreg. If that would be the case, I would feel more comfortable deleting an experimental option. If we agreed on moving bundle-phases out of experimental, I guess we can do the same for bundle-bookmarks. @indygreg Do you think we would still have this option around once we have new-style bundle spec? lothiraldan: I was afraid we might have to delete this option once we have the discussion about bundle spec…
				default=False,
				)
	coreconfigitem('experimental', 'bundle-phases',			coreconfigitem('experimental', 'bundle-phases',
	default=False,			default=False,
	)			)
	coreconfigitem('experimental', 'bundle2-advertise',			coreconfigitem('experimental', 'bundle2-advertise',
	default=True,			default=True,
	)			)
	coreconfigitem('experimental', 'bundle2-output-capture',			coreconfigitem('experimental', 'bundle2-output-capture',
	default=False,			default=False,

mercurial/debugcommands.py

	bin,			bin,
	hex,			hex,
	nullhex,			nullhex,
	nullid,			nullid,
	nullrev,			nullrev,
	short,			short,
	)			)
	from . import (			from . import (
				bookmarks,
	bundle2,			bundle2,
	changegroup,			changegroup,
	cmdutil,			cmdutil,
	color,			color,
	context,			context,
	dagparser,			dagparser,
	dagutil,			dagutil,
	encoding,			encoding,
	"""display version and markers contained in 'data'"""			"""display version and markers contained in 'data'"""
	indent_string = ' ' * indent			indent_string = ' ' * indent
	headsbyphase = phases.binarydecode(data)			headsbyphase = phases.binarydecode(data)
	for phase in phases.allphases:			for phase in phases.allphases:
	for head in headsbyphase[phase]:			for head in headsbyphase[phase]:
	ui.write(indent_string)			ui.write(indent_string)
	ui.write('%s %s\n' % (hex(head), phases.phasenames[phase]))			ui.write('%s %s\n' % (hex(head), phases.phasenames[phase]))

				def _debugbookmarks(ui, data, indent=0):
				"""display version and markers contained in 'data'"""
				indent_string = ' ' * indent
				bm = bookmarks.binarydecode(data)
				for bookmark in sorted(bm):
				ui.write(indent_string)
				ui.write('%s: %s\n' % (bookmark[0], bookmark[1]))

	def _quasirepr(thing):			def _quasirepr(thing):
	if isinstance(thing, (dict, util.sortdict, collections.OrderedDict)):			if isinstance(thing, (dict, util.sortdict, collections.OrderedDict)):
	return '{%s}' % (			return '{%s}' % (
	b', '.join(b'%s: %s' % (k, thing[k]) for k in sorted(thing)))			b', '.join(b'%s: %s' % (k, thing[k]) for k in sorted(thing)))
	return pycompat.bytestr(repr(thing))			return pycompat.bytestr(repr(thing))

	def _debugbundle2(ui, gen, all=None, **opts):			def _debugbundle2(ui, gen, all=None, **opts):
	"""lists the contents of a bundle2"""			"""lists the contents of a bundle2"""
	if not ui.quiet:			if not ui.quiet:
	_debugchangegroup(ui, cg, all=all, indent=4, **opts)			_debugchangegroup(ui, cg, all=all, indent=4, **opts)
	if part.type == 'obsmarkers':			if part.type == 'obsmarkers':
	if not ui.quiet:			if not ui.quiet:
	_debugobsmarkers(ui, part, indent=4, **opts)			_debugobsmarkers(ui, part, indent=4, **opts)
	if part.type == 'phase-heads':			if part.type == 'phase-heads':
	if not ui.quiet:			if not ui.quiet:
	_debugphaseheads(ui, part, indent=4)			_debugphaseheads(ui, part, indent=4)
				if part.type == 'bookmarks':
				if not ui.quiet:
				_debugbookmarks(ui, part, indent=4)

	@command('debugbundle',			@command('debugbundle',
	[('a', 'all', None, _('show all details')),			[('a', 'all', None, _('show all details')),
	('', 'part-type', [], _('show only the named part type')),			('', 'part-type', [], _('show only the named part type')),
	('', 'spec', None, _('print the bundlespec of the bundle'))],			('', 'spec', None, _('print the bundlespec of the bundle'))],
	_('FILE'),			_('FILE'),
	norepo=True)			norepo=True)
	def debugbundle(ui, bundlepath, all=None, spec=None, **opts):			def debugbundle(ui, bundlepath, all=None, spec=None, **opts):

tests/test-bundle-bookmarks.t

This file was added.

				$ cat >> $HGRCPATH <<EOF
				> [experimental]
				> bundle-bookmarks=yes
				> [extensions]
				> strip=
				> drawdag=$TESTDIR/drawdag.py
				> EOF

				Set up repo with linear history
				$ hg init linear
				$ cd linear
				$ hg debugdrawdag <<'EOF'
				> E
				> \|
				> D
				> \|
				> C
				> \|
				> B
				> \|
				> A
				> EOF
				$ hg bookmarks -r A "A1"
				$ hg bookmarks -r D "D1"
				$ hg bookmarks -r D "D2"
				$ hg log -G -T '{desc} {bookmarks}\n'
				o E
				\|
				o D D1 D2
				\|
				o C
				\|
				o B
				\|
				o A A1

				Bookmarks are restored when unbundling
				$ hg bundle --all bundle
				5 changesets found
				$ hg debugbundle bundle
				Stream params: {Compression: BZ}
				changegroup -- {nbchanges: 5, version: 02}
				426bada5c67598ca65036d57d9e4b64b0c1ce7a0
				112478962961147124edd43549aedd1a335e44bf
				26805aba1e600a82e93661149f2313866a221a7b
				f585351a92f85104bff7c284233c338b10eb1df7
				9bc730a19041f9ec7cb33c626e811aa233efb18c
				bookmarks -- {}
				A1: Bk\xad\xa5\xc6u\x98\xcae\x03mW\xd9\xe4\xb6K\x0c\x1c\xe7\xa0 (esc)
				D1: \xf5\x855\x1a\x92\xf8Q\x04\xbf\xf7\xc2\x84#<3\x8b\x10\xeb\x1d\xf7 (esc)
				D2: \xf5\x855\x1a\x92\xf8Q\x04\xbf\xf7\xc2\x84#<3\x8b\x10\xeb\x1d\xf7 (esc)
				$ hg strip --no-backup C
				$ hg unbundle -q bundle
				$ hg log -G -T '{desc} {bookmarks}\n'
				o E
				\|
				o D D1 D2
				\|
				o C
				\|
				o B
				\|
				o A A1
				martinvonzUnsubmitted Not Done Can we have a similar test case where we create divergence? Create a fork in the graph in the debugdrawdag call above. Let's say you have commit F that branches off of B, then do something like this: $ hg bundle --all bundle $ hg strip --no-backup C $ hg bookmarks -f -r F D1 $ hg unbundle -q bundle $ hg log -G -T '{desc} {bookmarks}\n' o E \| o D D1 D2 \| o C \| \| o F D1@1 \|/ o B \| o A A1 I don't know if "@1" is the appropriate divergence marker, but I can't think of a better one. I haven't even looked at your code to try to guess what it would be in practice (if it would work at all). martinvonz: Can we have a similar test case where we create divergence? Create a fork in the graph in the…
				lothiraldanAuthorUnsubmitted Not Done I have added such test and indeed this case is not handled right now. I'm not aware of how Mercurial handle this case when exchanging bookmarks, I will need to take a look at the code to find out. What is the correct behavior here? Change the existing bookmark or the bookmark from the bundle? lothiraldan: I have added such test and indeed this case is not handled right now. I'm not aware of how…
				martinvonzUnsubmitted Not Done I think it should be the same thing as pulling from another repo. Try putting the same changes in another repo and pulling from it without giving the path a name (nothing in [paths] config). I haven't checked what suffix will be used on the divergent bookmark, but it probably makes sense to use the same here (perhaps it's just going to be "D1@1" as I guessed before). martinvonz: I think it should be the same thing as pulling from another repo. Try putting the same changes…
				lothiraldanAuthorUnsubmitted Not Done I tried adding such scenarios but failed to produce a divergence scenario, no idea why. I don't think I will have time to debug the issue before the freeze, so let's skip this changeset for 4.6 lothiraldan: I tried adding such scenarios but failed to produce a divergence scenario, no idea why. I…

				Bookmarks doesn't conflict with local bookmarks

				$ hg bookmarks -d A1
				$ hg bookmarks -r A "A2"
				$ hg unbundle -q bundle
				$ hg log -G -T '{desc} {bookmarks}\n'
				o E
				\|
				o D D1 D2
				\|
				o C
				\|
				o B
				\|
				o A A1 A2

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	7077		Mar 16 2018, 9:21 AM	★	★
Diff 2	8263		Apr 14 2018, 5:38 AM	★	★