This is an archive of the discontinued Mercurial Phabricator instance.

rebase: enable multidest by default
ClosedPublic

Authored by quark on Oct 13 2017, 5:08 PM.

Download Raw Diff

Details

Reviewers

durin42
dlax

Group Reviewers

hg-reviewers

Commits

rHG3398603c5621: rebase: enable multidest by default
rHGd2055c13f1f4: rebase: enable multidest by default

Summary

This was intended to be done by D470. But there was a minor documentation
issue. The feature is quite usable now so it gets formally documented and
enabled.

There is no behavior change for people not using the SRC or ALLSRC in
rebase destination revset.

.. feature:: Rebase with different destination per source revision

Previously, rebase only supports one unique destination. Now ``SRC`` and
``ALLSRC`` can be used in rebase destination revset to precisely define
destination per each individual source revision.

For example, the following command could move some orphaned changesets to
reasonable new places so they become no longer orphaned::

  hg rebase
    -r 'orphan()-obsolete()'
    -d 'max((successors(max(roots(ALLSRC) & ::SRC)^)-obsolete())::)'

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

quark created this revision.Oct 13 2017, 5:08 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptOct 13 2017, 5:08 PM

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

I'm a fan. Will give it to Monday for any objections.

In D1063#17789, @durin42 wrote:

I'm a fan. Will give it to Monday for any objections.

I'm against. I'd like to see that the feature is indeed powerful enough for handling the intended use cases before we turn it on. Once it's on, we can't go back. The intended use case I'm aware of is to mimic "hg restack", IIUC. Maybe you have already switched over to using this (multi-dest rebase) internally at FB and seen that it does work well?

What's the advantage of turning it on? It seems like very few users would use it directly anyway. Won't you provide aliases instead and those aliases could then also enable the feature?

In D1063#17791, @martinvonz wrote:

In D1063#17789, @durin42 wrote:

I'm a fan. Will give it to Monday for any objections.

I'm against. I'd like to see that the feature is indeed powerful enough for handling the intended use cases before we turn it on. Once it's on, we can't go back. The intended use case I'm aware of is to mimic "hg restack", IIUC. Maybe you have already switched over to using this (multi-dest rebase) internally at FB and seen that it does work well?

Define "powerful enough for handling the intended use cases" precisely?

What's the advantage of turning it on? It seems like very few users would use it directly anyway. Won't you provide aliases instead and those aliases could then also enable the feature?

Aliases cannot use --config.

In D1063#17791, @martinvonz wrote:

In D1063#17789, @durin42 wrote:

I'm a fan. Will give it to Monday for any objections.

I'm against. I'd like to see that the feature is indeed powerful enough for handling the intended use cases before we turn it on. Once it's on, we can't go back. The intended use case I'm aware of is to mimic "hg restack", IIUC. Maybe you have already switched over to using this (multi-dest rebase) internally at FB and seen that it does work well?
What's the advantage of turning it on? It seems like very few users would use it directly anyway. Won't you provide aliases instead and those aliases could then also enable the feature?

In D1063#17931, @quark wrote:

In D1063#17791, @martinvonz wrote:

In D1063#17789, @durin42 wrote:

I'm a fan. Will give it to Monday for any objections.

I'm against. I'd like to see that the feature is indeed powerful enough for handling the intended use cases before we turn it on. Once it's on, we can't go back. The intended use case I'm aware of is to mimic "hg restack", IIUC. Maybe you have already switched over to using this (multi-dest rebase) internally at FB and seen that it does work well?

Define "powerful enough for handling the intended use cases" precisely?

I think what @martinvonz is getting at here is that we'd like to have some confidence that this is the right interface, by satisfying ourselves that this has been at least somewhat fieldtested at FB, if not anywhere else. Have you been doing so, and has it helped hg restack or whatever be simpler to implement?

What's the advantage of turning it on? It seems like very few users would use it directly anyway. Won't you provide aliases instead and those aliases could then also enable the feature?

Aliases cannot use --config.

Yes, this is a known issue, and largely orthogonal to the concern here IMO (could still be done as a shell alias.)

(Note that I'd still welcome feedback from non-BigCo contributors here - is this something we should make permanent? Have people been testing this? Etc.)

dlax added a subscriber: dlax.Oct 16 2017, 4:07 PM

Interface-wise, I'm thinking about defining BASE as max(roots(ALLSRC) & ::SRC)^ to make it easier to use. But that won't affect the SRC + ALLSRC design since it's the most flexible.

In D1063#18725, @durin42 wrote:

(Note that I'd still welcome feedback from non-BigCo contributors here - is this something we should make permanent? Have people been testing this? Etc.)

I have never tested this but it seems to me that this option enables a rather advanced behavior that would get rarely used (I cannot think of a situation where I'd need this FWIW). So having it on by default seems wrong to me.
On the other hand, as said in the commit message, you already have to used advanced names SRC or ALLSRC to see the actual effect of this option, so maybe it's already fine. Ultimately, if it's really safe, why keeping the option at all?

Anyways, I'd also welcome more feedback from people already using it, perhaps other people from FB could share their experience here?

hgext/rebase.py
650	Maybe this could be in a `.. container:: verbose` block since it's an advanced feature?
703	I'd also be very useful to have an example of how to stabilize only the current stack, but maybe this is unrelated to this changeset.

D1139 changes restack to use revsets. The resulting code is simpler, and no longer do duplicated rebases.

From a high level, this feature basically allows some user-invisible logic like rebase source and destination decision to be more user-visible as revsets, so users can investigate them by running log -r etc., which seems to be a good thing.

Ultimately, if it's really safe, why keeping the option at all?

The feature was implemented by multiple patches. I added the config so in case some of the patches did not land, the incomplete feature is not exposed. Now that all patches are landed. It's time to enable the feature by default.

hgext/rebase.py
703	This could be expressed via a complex revset if `predecessors` is in core.

quark added inline comments.Oct 17 2017, 2:03 PM

hgext/rebase.py
650	I think the "SRC" feature itself is not that "advanced" and is easy to understand. The problem is the revset for "restack" is long given the expressiveness of today's revsets. So that revset is hidden. It's possible to write short revsets, like: -d _destrebase(SRC)

I'm using "request changes" because I don't see how else I can un-do an LGTM. @martinvonz and I talked some, and he's convinced me that we should see if we can write an alias that uses multidest and successfully replaces hg stabilize in our day to day operations.

Let's revisit this early in the 4.5 cycle. Sorry. :/

This revision now requires changes to proceed.Oct 18 2017, 5:05 PM

In D1063#19864, @durin42 wrote:

I'm using "request changes" because I don't see how else I can un-do an LGTM. @martinvonz and I talked some, and he's convinced me that we should see if we can write an alias that uses multidest and successfully replaces hg stabilize in our day to day operations.
Let's revisit this early in the 4.5 cycle. Sorry. :/

Yeah, sorry about causing you inconvenience :-(

Hopefully it won't be too hard to make D1139 work without this being on by default. I'm curious to hear how it goes once you have rolled that out internally (but I'm confident it will be just fine).

Since the freeze is over. I'd like to know how to move forward.

In D1063#22439, @quark wrote:

Since the freeze is over. I'd like to know how to move forward.

Does that mean FB has now rewritten "hg restack" to use multi-destination rebase and used that in production for a while?

I want to test it out in production for us too and see if it works for us, but I won't bother until I've heard that you guys have.

@quark @martinvonz Ping

We have shipped restack based on this code path and haven't heard a problem yet.

I'm fine with this based on the reports that it's working well for FB. Anyone else have an objection?

dlax accepted this revision.Dec 6 2017, 3:16 AM

durin42 accepted this revision.Dec 7 2017, 2:53 PM

This revision is now accepted and ready to land.Dec 7 2017, 2:53 PM

Closed by commit rHGd2055c13f1f4: rebase: enable multidest by default (authored by quark). · Explain WhyDec 7 2017, 2:56 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			hgext/rebase.py (13 lines)
M			mercurial/configitems.py (6 lines)
M			tests/test-rebase-dest.t (1 line)

Diff 4174

hgext/rebase.py

	descendants.			descendants.

	3. Use ``--base`` to select a changeset; rebase will find ancestors			3. Use ``--base`` to select a changeset; rebase will find ancestors
	and their descendants which are not also ancestors of the destination.			and their descendants which are not also ancestors of the destination.

	4. If you do not specify any of ``--rev``, ``source``, or ``--base``,			4. If you do not specify any of ``--rev``, ``source``, or ``--base``,
	rebase will use ``--base .`` as above.			rebase will use ``--base .`` as above.

				If ``--source`` or ``--rev`` is used, special names ``SRC`` and ``ALLSRC``
				can be used in ``--dest``. Destination would be calculated per source
				revision with ``SRC`` substituted by that single source revision and
				``ALLSRC`` substituted by all source revisions.

				dlaxUnsubmitted Not Done Maybe this could be in a `.. container:: verbose` block since it's an advanced feature? dlax: Maybe this could be in a `.. container:: verbose` block since it's an advanced feature?
				quarkAuthorUnsubmitted Not Done I think the "SRC" feature itself is not that "advanced" and is easy to understand. The problem is the revset for "restack" is long given the expressiveness of today's revsets. So that revset is hidden. It's possible to write short revsets, like: -d _destrebase(SRC) quark: I think the "SRC" feature itself is not that "advanced" and is easy to understand. The problem…
	Rebase will destroy original changesets unless you use ``--keep``.			Rebase will destroy original changesets unless you use ``--keep``.
	It will also move your bookmarks (even if you do).			It will also move your bookmarks (even if you do).

	Some changesets may be dropped if they do not contribute changes			Some changesets may be dropped if they do not contribute changes
	(e.g. merges from the destination branch).			(e.g. merges from the destination branch).

	Unlike ``merge``, rebase will do nothing if you are at the branch tip of			Unlike ``merge``, rebase will do nothing if you are at the branch tip of
	a named branch with two heads. You will need to explicitly specify source			a named branch with two heads. You will need to explicitly specify source
	- collapse a sequence of changes into a single commit::			- collapse a sequence of changes into a single commit::

	hg rebase --collapse -r 1520:1525 -d .			hg rebase --collapse -r 1520:1525 -d .

	- move a named branch while preserving its name::			- move a named branch while preserving its name::

	hg rebase -r "branch(featureX)" -d 1.3 --keepbranches			hg rebase -r "branch(featureX)" -d 1.3 --keepbranches

				- stabilize orphaned changesets so history looks linear::

				hg rebase -r 'orphan()-obsolete()'\
				-d 'first(max((successors(max(roots(ALLSRC) & ::SRC)^)-obsolete())::) +\
				max(::((roots(ALLSRC) & ::SRC)^)-obsolete()))'
				dlaxUnsubmitted Not Done I'd also be very useful to have an example of how to stabilize only the current stack, but maybe this is unrelated to this changeset. dlax: I'd also be very useful to have an example of how to stabilize only the current stack, but…
				quarkAuthorUnsubmitted Not Done This could be expressed via a complex revset if `predecessors` is in core. quark: This could be expressed via a complex revset if `predecessors` is in core.

	Configuration Options:			Configuration Options:

	You can make rebase require a destination if you set the following config			You can make rebase require a destination if you set the following config
	option::			option::

	[commands]			[commands]
	rebase.requiredest = True			rebase.requiredest = True

	allsrc = revsetlang.formatspec('%ld', rebaseset)			allsrc = revsetlang.formatspec('%ld', rebaseset)
	alias = {'ALLSRC': allsrc}			alias = {'ALLSRC': allsrc}

	if dest is None:			if dest is None:
	try:			try:
	# fast path: try to resolve dest without SRC alias			# fast path: try to resolve dest without SRC alias
	dest = scmutil.revsingle(repo, destf, localalias=alias)			dest = scmutil.revsingle(repo, destf, localalias=alias)
	except error.RepoLookupError:			except error.RepoLookupError:
	if not ui.configbool('experimental', 'rebase.multidest'):
	raise
	# multi-dest path: resolve dest for each SRC separately			# multi-dest path: resolve dest for each SRC separately
	destmap = {}			destmap = {}
	for r in rebaseset:			for r in rebaseset:
	alias['SRC'] = revsetlang.formatspec('%d', r)			alias['SRC'] = revsetlang.formatspec('%d', r)
	# use repo.anyrevs instead of scmutil.revsingle because we			# use repo.anyrevs instead of scmutil.revsingle because we
	# don't want to abort if destset is empty.			# don't want to abort if destset is empty.
	destset = repo.anyrevs([destf], user=True, localalias=alias)			destset = repo.anyrevs([destf], user=True, localalias=alias)
	size = len(destset)			size = len(destset)

mercurial/configitems.py

	default=False,			default=False,
	)			)
	coreconfigitem('experimental', 'mergedriver',			coreconfigitem('experimental', 'mergedriver',
	default=None,			default=None,
	)			)
	coreconfigitem('experimental', 'obsmarkers-exchange-debug',			coreconfigitem('experimental', 'obsmarkers-exchange-debug',
	default=False,			default=False,
	)			)
	coreconfigitem('experimental', 'rebase.multidest',
	default=False,
	)
	coreconfigitem('experimental', 'remotenames',			coreconfigitem('experimental', 'remotenames',
	default=False,			default=False,
	)			)
				coreconfigitem('experimental', 'revertalternateinteractivemode',
				default=True,
				)
	coreconfigitem('experimental', 'revlogv2',			coreconfigitem('experimental', 'revlogv2',
	default=None,			default=None,
	)			)
	coreconfigitem('experimental', 'single-head-per-branch',			coreconfigitem('experimental', 'single-head-per-branch',
	default=False,			default=False,
	)			)
	coreconfigitem('experimental', 'spacemovesdown',			coreconfigitem('experimental', 'spacemovesdown',
	default=False,			default=False,

tests/test-rebase-dest.t

	> drawdag=$TESTDIR/drawdag.py			> drawdag=$TESTDIR/drawdag.py
	> [phases]			> [phases]
	> publish=False			> publish=False
	> [alias]			> [alias]
	> tglog = log -G --template "{rev}: {desc} {instabilities}" -r 'sort(all(), topo)'			> tglog = log -G --template "{rev}: {desc} {instabilities}" -r 'sort(all(), topo)'
	> [extensions]			> [extensions]
	> maprevset=$TESTTMP/maprevset.py			> maprevset=$TESTTMP/maprevset.py
	> [experimental]			> [experimental]
	> rebase.multidest=true
	> evolution=true			> evolution=true
	> EOF			> EOF

	$ rebasewithdag() {			$ rebasewithdag() {
	> N=`$PYTHON -c "print($N+1)"`			> N=`$PYTHON -c "print($N+1)"`
	> hg init repo$N && cd repo$N			> hg init repo$N && cd repo$N
	> hg debugdrawdag			> hg debugdrawdag
	> hg rebase "$@" > _rebasetmp			> hg rebase "$@" > _rebasetmp

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	2702		Oct 13 2017, 5:08 PM	★	★
Diff 2	4174	rHGd2055c13f1f48829be95e3392746665d03dc0fed	Oct 13 2017, 5:08 PM	★	★