Patchwork D1063: rebase: enable multidest by default

login
register
mail settings
Submitter phabricator
Date Oct. 13, 2017, 9:08 p.m.
Message ID <differential-rev-PHID-DREV-pkncib7m3fpxtazvowa7-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/24857/
State Superseded
Headers show

Comments

phabricator - Oct. 13, 2017, 9:08 p.m.
quark created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This was intended to be done by https://phab.mercurial-scm.org/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())::)'

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1063

AFFECTED FILES
  hgext/rebase.py
  mercurial/configitems.py
  tests/test-rebase-dest.t

CHANGE DETAILS




To: quark, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 14, 2017, 4:36 a.m.
durin42 accepted this revision as: durin42.
durin42 added a comment.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1063

To: quark, #hg-reviewers, durin42
Cc: durin42, mercurial-devel
phabricator - Oct. 14, 2017, 4:44 a.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/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?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1063

To: quark, #hg-reviewers, durin42
Cc: martinvonz, durin42, mercurial-devel
phabricator - Oct. 14, 2017, 9:33 a.m.
quark added a comment.


  In https://phab.mercurial-scm.org/D1063#17791, @martinvonz wrote:
  
  > In https://phab.mercurial-scm.org/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?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1063

To: quark, #hg-reviewers, durin42
Cc: martinvonz, durin42, mercurial-devel
phabricator - Oct. 16, 2017, 8:01 p.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D1063#17791, @martinvonz wrote:
  
  > In https://phab.mercurial-scm.org/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 https://phab.mercurial-scm.org/D1063#17931, @quark wrote:
  
  > In https://phab.mercurial-scm.org/D1063#17791, @martinvonz wrote:
  >
  > > In https://phab.mercurial-scm.org/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.)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1063

To: quark, #hg-reviewers, durin42
Cc: martinvonz, durin42, mercurial-devel
phabricator - Oct. 16, 2017, 8:01 p.m.
durin42 added a comment.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1063

To: quark, #hg-reviewers, durin42
Cc: martinvonz, durin42, mercurial-devel
phabricator - Oct. 16, 2017, 9:21 p.m.
quark added a comment.


  Interface-wise, I'm thinking about defining `BASE` as `max(roots(ALLSRC) & ::SRC)^` to make it easier to use.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1063

To: quark, #hg-reviewers, durin42
Cc: dlax, martinvonz, durin42, mercurial-devel
phabricator - Oct. 17, 2017, 7:02 a.m.
dlax added a comment.


  In https://phab.mercurial-scm.org/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?

INLINE COMMENTS

> rebase.py:645
> +    ``ALLSRC`` substituted by all source revisions.
> +
>      Rebase will destroy original changesets unless you use ``--keep``.

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

> rebase.py:698
> + -d 'first(max((successors(max(roots(ALLSRC) & ::SRC)^)-obsolete())::) +\
> + max(::((roots(ALLSRC) & ::SRC)^)-obsolete()))'
> +

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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1063

To: quark, #hg-reviewers, durin42
Cc: dlax, martinvonz, durin42, mercurial-devel
phabricator - Oct. 17, 2017, 5:54 p.m.
quark added a comment.


  https://phab.mercurial-scm.org/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.

INLINE COMMENTS

> dlax wrote in rebase.py: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.

This could be expressed via a complex revset if `predecessors` is in core.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1063

To: quark, #hg-reviewers, durin42
Cc: dlax, martinvonz, durin42, mercurial-devel
phabricator - Oct. 17, 2017, 6:03 p.m.
quark added inline comments.

INLINE COMMENTS

> dlax wrote in rebase.py:645
> Maybe this could be in a `.. container:: verbose` block since it's an advanced feature?

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)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1063

To: quark, #hg-reviewers, durin42
Cc: dlax, martinvonz, durin42, mercurial-devel
phabricator - Oct. 18, 2017, 9:05 p.m.
durin42 requested changes to this revision.
durin42 added a comment.
This revision now requires changes to proceed.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1063

To: quark, #hg-reviewers, durin42
Cc: dlax, martinvonz, durin42, mercurial-devel
phabricator - Oct. 18, 2017, 9:17 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/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 https://phab.mercurial-scm.org/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).

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1063

To: quark, #hg-reviewers, durin42
Cc: dlax, martinvonz, durin42, mercurial-devel
phabricator - Nov. 9, 2017, 8:47 p.m.
quark requested review of this revision.
quark added a comment.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1063

To: quark, #hg-reviewers, durin42
Cc: dlax, martinvonz, durin42, mercurial-devel
phabricator - Nov. 9, 2017, 9:17 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1063

To: quark, #hg-reviewers, durin42
Cc: dlax, martinvonz, durin42, mercurial-devel
phabricator - Nov. 22, 2017, 5:08 p.m.
krbullock added a comment.


  @quark @martinvonz Ping

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1063

To: quark, #hg-reviewers, durin42
Cc: krbullock, dlax, martinvonz, durin42, mercurial-devel
phabricator - Nov. 22, 2017, 5:30 p.m.
quark added a comment.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1063

To: quark, #hg-reviewers, durin42
Cc: krbullock, dlax, martinvonz, durin42, mercurial-devel
phabricator - Dec. 5, 2017, 10:37 p.m.
durin42 accepted this revision as: durin42.
durin42 added a comment.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1063

To: quark, #hg-reviewers, durin42
Cc: krbullock, dlax, martinvonz, durin42, mercurial-devel

Patch

diff --git a/tests/test-rebase-dest.t b/tests/test-rebase-dest.t
--- a/tests/test-rebase-dest.t
+++ b/tests/test-rebase-dest.t
@@ -115,7 +115,6 @@ 
   > [extensions]
   > maprevset=$TESTTMP/maprevset.py
   > [experimental]
-  > rebase.multidest=true
   > stabilization=all
   > EOF
 
diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -341,9 +341,6 @@ 
 coreconfigitem('experimental', 'obsmarkers-exchange-debug',
     default=False,
 )
-coreconfigitem('experimental', 'rebase.multidest',
-    default=False,
-)
 coreconfigitem('experimental', 'revertalternateinteractivemode',
     default=True,
 )
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -638,6 +638,11 @@ 
       4. If you do not specify any of ``--rev``, ``source``, or ``--base``,
          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.
+
     Rebase will destroy original changesets unless you use ``--keep``.
     It will also move your bookmarks (even if you do).
 
@@ -686,6 +691,12 @@ 
 
           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()))'
+
     Configuration Options:
 
     You can make rebase require a destination if you set the following config
@@ -878,8 +889,6 @@ 
             # fast path: try to resolve dest without SRC alias
             dest = scmutil.revsingle(repo, destf, localalias=alias)
         except error.RepoLookupError:
-            if not ui.configbool('experimental', 'rebase.multidest'):
-                raise
             # multi-dest path: resolve dest for each SRC separately
             destmap = {}
             for r in rebaseset: