Page MenuHomePhabricator

rebase: enable multidest by default
ClosedPublic

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

Details

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
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.

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

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.

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:

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
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

(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.

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
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)
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

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.

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.

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
This revision was automatically updated to reflect the committed changes.