( )⚙ D1063 rebase: enable multidest by default

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1063

rebase: enable multidest by default
ClosedPublic

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

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
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Oct 13 2017, 5:08 PM
durin42 accepted this revision as: durin42.Oct 14 2017, 12:36 AM
durin42 added a subscriber: durin42.

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

martinvonz added a subscriber: martinvonz.Oct 14 2017, 12:44 AM
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?

quark added a comment.EditedOct 14 2017, 5:33 AM
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.

durin42 added a comment.Oct 16 2017, 4:01 PM
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.)

durin42 added a comment.Oct 16 2017, 4:01 PM

(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
quark added a comment.EditedOct 16 2017, 5:21 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.

dlax added a comment.Oct 17 2017, 3:02 AM
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
645

Maybe this could be in a .. container:: verbose block since it's an advanced feature?

698

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.

quark added a comment.Oct 17 2017, 1:53 PM

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
698

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
645

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)
durin42 requested changes to this revision.Oct 18 2017, 5:05 PM

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
martinvonz added a comment.Oct 18 2017, 5:17 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).

quark requested review of this revision.Nov 9 2017, 3:47 PM

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

martinvonz added a comment.Nov 9 2017, 4:17 PM
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 added a comment.Nov 22 2017, 12:30 PM

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

durin42 accepted this revision as: durin42.Dec 5 2017, 5:37 PM

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

PathPackages
Mhgext/rebase.py (13 lines)
Mmercurial/configitems.py (3 lines)
Mtests/test-rebase-dest.t (1 line)

Diff 2702

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', 'revertalternateinteractivemode', coreconfigitem('experimental', 'revertalternateinteractivemode',
default=True, default=True,
) )
coreconfigitem('experimental', 'revlogv2', coreconfigitem('experimental', 'revlogv2',
default=None, default=None,
) )
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
> stabilization=all > stabilization=all
> 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