Download Raw Diff

Details

Reviewers

indygreg
krbullock

Group Reviewers

hg-reviewers

Commits

rHG92213f6745ed: rebase: introduce support for automatically rebasing orphan changes

Summary

_destautorebase(SRC) is based on the _destrestack(SRC) revset from
fbamend. The supporting _possibledestination function is extracted
from evolve, with minor cleanups.

We've considered some alternatives here:

This change, but with --auto as the flag name. We're hedging our bets on this a little in this change so that if this ends up being the wrong direction we haven't burned the valauble --auto name on rebase.
--destination auto: I've got reservations about the discoverability of this, and we don't currently have a good story for a revset alias of sorts that changes behavior depending on the context in which it's used.
A "rebase presets" feature, where we could use the currently-an-error positional argument space for the rebase command to define presets, so that users could define a 'linearize' preset that specifies --revision='orphan()-obsolete()' and --dest=_destautoorphanrebase(SRC).

Personally, I find the third option somewhat appealing, but am
hesitant to "spend" the functionality space of positional arguments to
the rebase command. We should revisit the way we expose this
functionality sometime in the 4.7 cycle once we've had a chance to vet
the implementation of the functionality.

Diff Detail

Repository

rHG Mercurial

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

durin42 created this revision.Mar 4 2018, 3:37 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptMar 4 2018, 3:37 PM

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

durin42 updated this revision to Diff 6611.Mar 4 2018, 3:39 PM

I'm +1 on the feature. We should bikeshed the naming.

hgext/rebase.py
123	Let's add this to `destutil.py` instead of adding more code to `rebase.py`.
147	This should use the `node.nullrev` constant.
702	The fact that `auto` takes an argument makes it... not very auto. I have a slight preference for `--orphans <rev>`. All the other arguments that take revsets are nouns describing what will be operated on, what the revisions are being used for, etc.
837	An allow list is better. But I could let this slide.

This revision now requires changes to proceed.Mar 4 2018, 5:08 PM

My initial idea was to allow -r 'orphan()-obsolete()' -d auto. That can be actually done without any code change by defining auto as a complex revset.

That idea derived an idea of allowing omitting arguments.

rebase.autodest = ... # when -r or -s is provided
rebase.autosrc = ...  # when -d is provided
rebase.autosrcdest = ..., ... # when nothing is provided

So people can run hg rebase, hg rebase -r orphan()-obsolete().

I have also thought about "rebase presets", like:

rebase.preset.linearize.src = orphan() - obsolete()
rebase.preset.linearize.dest = (a complex revset using SRC, ALLSRC)
rebase.preset.master.src = ". % @"
rebase.preset.master.dest = @

If preset name is the unnamed argument, then people can use rebase master, rebase linearize.

In D2668#43338, @quark wrote:

My initial idea was to allow -r 'orphan()-obsolete()' -d auto. That can be actually done without any code change by defining auto as a complex revset.
That idea derived an idea of allowing omitting arguments.

This is all very cool, but I worry about how we define auto as a rebase-only scoped revset. How do we make that discoverable?

I like the way --auto (or some other name) is discoverable in hg help rebase. Losing that inside the long-form prose of the help text (which is the only place I can think of to document a magic rebase-specific revset?) seems like a bummer to me...

hgext/rebase.py
702	I don't follow. It automatically rebases the specified set of revisions - that is, it automatically picks the destination. Does that make sense? (I don't know what you think could be more auto here?) I honestly find it very confusing to write `hg rebase --orphans 'orphan()'`. Any thoughts about that?

durin42 updated this revision to Diff 7914.Apr 9 2018, 4:34 PM

In D2668#51423, @durin42 wrote:

I like the way --auto (or some other name) is discoverable in hg help rebase. Losing that inside the long-form prose of the help text (which is the only place I can think of to document a magic rebase-specific revset?) seems like a bummer to me...

@spectral had the idea of making --auto imply a default of -r orphan(), but you could override -r if you want. That'd make hg evolve --any --all become hg rebase --auto, if people like that more.

In D2668#51423, @durin42 wrote:

I like the way --auto (or some other name) is discoverable in hg help rebase. Losing that inside the long-form prose of the help text (which is the only place I can think of to document a magic rebase-specific revset?) seems like a bummer to me...

We recently had some internal discussion about discovery. @mbthomas mentioned that help text is a way of discovery and help text is not only about command line flags. i.e. there could be some examples in help text. I think that might be actually better not only because it keeps CLI clean, but also because a flag has limited (half of a line) space to explain things while an example could have multi-line explanation.

UPDATE: For the revset namespace issue, I think we can define something like rebase.defaultdest, rebase.auto and modify revset resolution layer to think about namespaces. And then pass namespaces to scmutil.rev* from the rebase command.

durin42 edited the summary of this revision. (Show Details)Apr 16 2018, 10:00 PM

durin42 updated this revision to Diff 8361.

Looks like a couple unresolved comments on _possibledestination, plus one nitpick. I think we can clean those things up easily and then land this though.

hgext/rebase.py
840	Would be nice to see test coverage of this... it looks to me like we could end up spitting out an error like `incompatible with --foo_bar` (with an underscore) accidentally. That isn't itself a problem now, but could be in the future. We should at least test the mutual exclusion with other options.

This revision now requires changes to proceed.Apr 18 2018, 2:40 PM

durin42 updated this revision to Diff 8390.Apr 18 2018, 2:50 PM

I'm not in love with the function name in destutil, but it's the best I've got. Open to suggestions, or we can change it in the future.

krbullock accepted this revision.Apr 18 2018, 3:00 PM

Closed by commit rHG92213f6745ed: rebase: introduce support for automatically rebasing orphan changes (authored by durin42). · Explain WhyApr 18 2018, 3:30 PM

This revision was automatically updated to reflect the committed changes.

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	6610		Mar 4 2018, 3:37 PM	★	★
Diff 2	6611		Mar 4 2018, 3:39 PM	★	★
Diff 3	7914		Apr 9 2018, 4:34 PM	★	★
Diff 4	8361		Apr 16 2018, 10:00 PM	★	★
Diff 5	8390		Apr 18 2018, 2:49 PM	★	★
Diff 6	8392	rHG92213f6745ed6f2c50feca9a2261b6f33a9a32fa	Mar 4 2018, 3:29 PM	★	★

Diff 6610

hgext/rebase.py

	# # XXX: Currently private because I expect the signature to change.			# # XXX: Currently private because I expect the signature to change.
	# # XXX: - bailing out in case of ambiguity vs returning all data.			# # XXX: - bailing out in case of ambiguity vs returning all data.
	# i18n: "_rebasedefaultdest" is a keyword			# i18n: "_rebasedefaultdest" is a keyword
	sourceset = None			sourceset = None
	if x is not None:			if x is not None:
	sourceset = revset.getset(repo, smartset.fullreposet(repo), x)			sourceset = revset.getset(repo, smartset.fullreposet(repo), x)
	return subset & smartset.baseset([_destrebase(repo, sourceset)])			return subset & smartset.baseset([_destrebase(repo, sourceset)])

				def _possibledestination(repo, rev):
				indygregUnsubmitted Not Done Let's add this to `destutil.py` instead of adding more code to `rebase.py`. indygreg: Let's add this to `destutil.py` instead of adding more code to `rebase.py`.
				"""Return all changesets that may be a new parent for `rev`."""
				tonode = repo.changelog.node
				parents = repo.changelog.parentrevs
				torev = repo.changelog.rev
				dest = set()
				tovisit = list(parents(rev))
				while tovisit:
				r = tovisit.pop()
				succsets = obsutil.successorssets(repo, tonode(r))
				if not succsets:
				# if there are no successors for r, r was probably pruned
				# and we should walk up to r's parents to try and find
				# some successors.
				tovisit.extend(parents(r))
				else:
				# We should probably pick only one destination from split
				# (case where '1 < len(ss)'), This could be the currently
				# tipmost, but the correct result is less clear when
				# results of the split have been moved such that they
				# reside on multiple branches.
				for ss in succsets:
				for n in ss:
				dr = torev(n)
				if dr != -1:
				indygregUnsubmitted Not Done This should use the `node.nullrev` constant. indygreg: This should use the `node.nullrev` constant.
				dest.add(dr)
				return dest

				@revsetpredicate('_destautorebase')
				def _revsetdestautorebase(repo, subset, x):
				"""automatic rebase destination for a single revision"""
				unfi = repo.unfiltered()
				obsoleted = unfi.revs('obsolete()')

				src = revset.getset(repo, subset, x).first()

				# Empty src or already obsoleted - Do not return a destination
				if not src or src in obsoleted:
				return smartset.baseset()
				dests = _possibledestination(repo, src)
				if len(dests) > 1:
				raise error.Abort(
				_("ambiguous automatic rebase: %r could end up on any of %r") % (
				src, dests))
				# We have zero or one destination, so we can just return here.
				return smartset.baseset(dests)

	def _ctxdesc(ctx):			def _ctxdesc(ctx):
	"""short description for a context"""			"""short description for a context"""
	desc = '%d:%s "%s"' % (ctx.rev(), ctx,			desc = '%d:%s "%s"' % (ctx.rev(), ctx,
	ctx.description().split('\n', 1)[0])			ctx.description().split('\n', 1)[0])
	repo = ctx.repo()			repo = ctx.repo()
	names = []			names = []
	for nsname, ns in repo.names.iteritems():			for nsname, ns in repo.names.iteritems():
	if nsname == 'branches':			if nsname == 'branches':
	('l', 'logfile', '',			('l', 'logfile', '',
	_('read collapse commit message from file'), _('FILE')),			_('read collapse commit message from file'), _('FILE')),
	('k', 'keep', False, _('keep original changesets')),			('k', 'keep', False, _('keep original changesets')),
	('', 'keepbranches', False, _('keep original branch names')),			('', 'keepbranches', False, _('keep original branch names')),
	('D', 'detach', False, _('(DEPRECATED)')),			('D', 'detach', False, _('(DEPRECATED)')),
	('i', 'interactive', False, _('(DEPRECATED)')),			('i', 'interactive', False, _('(DEPRECATED)')),
	('t', 'tool', '', _('specify merge tool')),			('t', 'tool', '', _('specify merge tool')),
	('c', 'continue', False, _('continue an interrupted rebase')),			('c', 'continue', False, _('continue an interrupted rebase')),
	('a', 'abort', False, _('abort an interrupted rebase'))] +			('a', 'abort', False, _('abort an interrupted rebase')),
				('', 'auto', '', _('automatically rebase orphan revisions '
				indygregUnsubmitted Not Done The fact that `auto` takes an argument makes it... not very auto. I have a slight preference for `--orphans <rev>`. All the other arguments that take revsets are nouns describing what will be operated on, what the revisions are being used for, etc. indygreg: The fact that `auto` takes an argument makes it... not very //auto//. I have a slight…
				durin42AuthorUnsubmitted Not Done I don't follow. It automatically rebases the specified set of revisions - that is, it automatically picks the destination. Does that make sense? (I don't know what you think could be more auto here?) I honestly find it very confusing to write `hg rebase --orphans 'orphan()'`. Any thoughts about that? durin42: I don't follow. It automatically rebases the specified set of revisions - that is, it…
				'in the specified revset (EXPERIMENTAL)')),
				] +
	cmdutil.formatteropts,			cmdutil.formatteropts,
	_('[-s REV \| -b REV] [-d REV] [OPTION]'))			_('[-s REV \| -b REV] [-d REV] [OPTION]'))
	def rebase(ui, repo, **opts):			def rebase(ui, repo, **opts):
	"""move changeset (and descendants) to a different branch			"""move changeset (and descendants) to a different branch

	Rebase uses repeated merging to graft changesets from one part of			Rebase uses repeated merging to graft changesets from one part of
	history (the source) onto another (the destination). This can be			history (the source) onto another (the destination). This can be
	useful for linearizing local changes relative to a master			useful for linearizing local changes relative to a master
	inmemory = ui.configbool('rebase', 'experimental.inmemory')			inmemory = ui.configbool('rebase', 'experimental.inmemory')
	if (opts.get('continue') or opts.get('abort') or			if (opts.get('continue') or opts.get('abort') or
	repo.currenttransaction() is not None):			repo.currenttransaction() is not None):
	# in-memory rebase is not compatible with resuming rebases.			# in-memory rebase is not compatible with resuming rebases.
	# (Or if it is run within a transaction, since the restart logic can			# (Or if it is run within a transaction, since the restart logic can
	# fail the entire transaction.)			# fail the entire transaction.)
	inmemory = False			inmemory = False

				if opts.get('auto'):
				for disallowed in 'rev', 'source', 'base', 'dest':
				indygregUnsubmitted Done An allow list is better. But I could let this slide. indygreg: An allow list is better. But I could let this slide.
				if opts.get(disallowed):
				raise error.Abort(_('--auto is incompatible with %s') %
				('--' + disallowed))
				krbullockUnsubmitted Done Would be nice to see test coverage of this... it looks to me like we could end up spitting out an error like `incompatible with --foo_bar` (with an underscore) accidentally. That isn't itself a problem now, but could be in the future. We should at least test the mutual exclusion with other options. krbullock: Would be nice to see test coverage of this... it looks to me like we could end up spitting out…
				userrevs = list(repo.revs(opts.get('auto')))
				opts['rev'] = [revsetlang.formatspec('%ld', userrevs)]
				opts['dest'] = '_destautorebase(SRC)'

	if inmemory:			if inmemory:
	try:			try:
	# in-memory merge doesn't support conflicts, so if we hit any, abort			# in-memory merge doesn't support conflicts, so if we hit any, abort
	# and re-run as an on-disk merge.			# and re-run as an on-disk merge.
	return _origrebase(ui, repo, inmemory=inmemory, **opts)			return _origrebase(ui, repo, inmemory=inmemory, **opts)
	except error.InMemoryMergeConflictsError:			except error.InMemoryMergeConflictsError:
	ui.warn(_('hit merge conflicts; re-running rebase without in-memory'			ui.warn(_('hit merge conflicts; re-running rebase without in-memory'
	' merge\n'))			' merge\n'))

tests/test-rebase-obsolete.t

	\| o 6:eea13746799a G			\| o 6:eea13746799a G
	\|/\|			\|/\|
	o \| 5:24b6387c8c8c F			o \| 5:24b6387c8c8c F
	\| \|			\| \|
	\| o 4:9520eea781bc E			\| o 4:9520eea781bc E
	\|/			\|/
	o 0:cd010b8cd998 A			o 0:cd010b8cd998 A

				$ cd ..
				$ cp -r hidden stabilize
				$ cd stabilize
				$ hg rebase --auto 'orphan()'
				rebasing 9:cf44d2f5a9f4 "D"
				$ hg log -G
				o 12:7e3935feaa68 D
				\|
				o 11:0d8f238b634c C
				\|
				o 10:7c6027df6a99 B
				\|
				@ 7:02de42196ebe H
				\|
				\| o 6:eea13746799a G
				\|/\|
				o \| 5:24b6387c8c8c F
				\| \|
				\| o 4:9520eea781bc E
				\|/
				o 0:cd010b8cd998 A


				$ cd ../hidden
				$ rm -r ../stabilize

	Test multiple root handling			Test multiple root handling
	------------------------------------			------------------------------------

	$ hg rebase --dest 4 --rev '7+11+9'			$ hg rebase --dest 4 --rev '7+11+9'
	rebasing 9:cf44d2f5a9f4 "D"			rebasing 9:cf44d2f5a9f4 "D"
	rebasing 7:02de42196ebe "H"			rebasing 7:02de42196ebe "H"
	rebasing 11:0d8f238b634c "C" (tip)			rebasing 11:0d8f238b634c "C" (tip)

			Path	Packages
M			hgext/rebase.py (61 lines)
M			tests/test-rebase-obsolete.t (24 lines)

This is an archive of the discontinued Mercurial Phabricator instance.

rebase: introduce support for automatically rebasing orphan changes
ClosedPublic

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 6610

hgext/rebase.py

tests/test-rebase-obsolete.t

This is an archive of the discontinued Mercurial Phabricator instance.

rebase: introduce support for automatically rebasing orphan changesClosedPublic

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 6610

hgext/rebase.py

tests/test-rebase-obsolete.t

rebase: introduce support for automatically rebasing orphan changes
ClosedPublic

Revision Contents
Changeset List