Patchwork D2668: rebase: introduce support for automatically rebasing orphan changes

login
register
mail settings
Submitter phabricator
Date March 4, 2018, 8:37 p.m.
Message ID <differential-rev-PHID-DREV-kv7tqkxl7moy7qy46w5t-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/28991/
State Superseded
Headers show

Comments

phabricator - March 4, 2018, 8:37 p.m.
durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

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

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/rebase.py
  tests/test-rebase-obsolete.t

CHANGE DETAILS




To: durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - March 4, 2018, 10:08 p.m.
indygreg requested changes to this revision.
indygreg added a comment.
This revision now requires changes to proceed.


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

INLINE COMMENTS

> rebase.py:123
>  
> +def _possibledestination(repo, rev):
> +    """Return all changesets that may be a new parent for `rev`."""

Let's add this to `destutil.py` instead of adding more code to `rebase.py`.

> rebase.py:147
> +                    dr = torev(n)
> +                    if dr != -1:
> +                        dest.add(dr)

This should use the `node.nullrev` constant.

> rebase.py:702
> +    ('a', 'abort', False, _('abort an interrupted rebase')),
> +    ('', 'auto', '', _('automatically rebase orphan revisions '
> +                       'in the specified revset (EXPERIMENTAL)')),

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.

> rebase.py:837
> +    if opts.get('auto'):
> +        for disallowed in 'rev', 'source', 'base', 'dest':
> +            if opts.get(disallowed):

An allow list is better. But I could let this slide.

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - March 5, 2018, 4:37 a.m.
quark added a comment.


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

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers, indygreg
Cc: quark, indygreg, mercurial-devel
phabricator - April 9, 2018, 8:27 p.m.
durin42 marked an inline comment as done.
durin42 added a comment.


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

INLINE COMMENTS

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

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?

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers, indygreg
Cc: quark, indygreg, mercurial-devel
phabricator - April 9, 2018, 8:49 p.m.
durin42 added a subscriber: spectral.
durin42 added a comment.


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

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers, indygreg
Cc: spectral, quark, indygreg, mercurial-devel
phabricator - April 9, 2018, 11:29 p.m.
quark added a subscriber: mbthomas.
quark added a comment.


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

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers, indygreg
Cc: mbthomas, spectral, quark, indygreg, mercurial-devel
phabricator - April 18, 2018, 6:40 p.m.
krbullock requested changes to this revision.
krbullock added a comment.
This revision now requires changes to proceed.


  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.

INLINE COMMENTS

> rebase.py:840
> +                raise error.Abort(_('--auto-orphans is incompatible with %s') %
> +                                  ('--' + key))
> +        userrevs = list(repo.revs(opts.get('auto_orphans')))

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.

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers, indygreg, krbullock
Cc: krbullock, mbthomas, spectral, quark, indygreg, mercurial-devel
phabricator - April 18, 2018, 6:50 p.m.
durin42 marked an inline comment as done.
durin42 added a comment.


  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.

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers, indygreg, krbullock
Cc: krbullock, mbthomas, spectral, quark, indygreg, mercurial-devel

Patch

diff --git a/tests/test-rebase-obsolete.t b/tests/test-rebase-obsolete.t
--- a/tests/test-rebase-obsolete.t
+++ b/tests/test-rebase-obsolete.t
@@ -482,7 +482,31 @@ 
   |/
   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
 ------------------------------------
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -120,6 +120,53 @@ 
         sourceset = revset.getset(repo, smartset.fullreposet(repo), x)
     return subset & smartset.baseset([_destrebase(repo, sourceset)])
 
+def _possibledestination(repo, rev):
+    """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:
+                        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):
     """short description for a context"""
     desc = '%d:%s "%s"' % (ctx.rev(), ctx,
@@ -651,7 +698,10 @@ 
     ('i', 'interactive', False, _('(DEPRECATED)')),
     ('t', 'tool', '', _('specify merge tool')),
     ('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 '
+                       'in the specified revset (EXPERIMENTAL)')),
+     ] +
     cmdutil.formatteropts,
     _('[-s REV | -b REV] [-d REV] [OPTION]'))
 def rebase(ui, repo, **opts):
@@ -783,6 +833,15 @@ 
         # fail the entire transaction.)
         inmemory = False
 
+    if opts.get('auto'):
+        for disallowed in 'rev', 'source', 'base', 'dest':
+            if opts.get(disallowed):
+                raise error.Abort(_('--auto is incompatible with %s') %
+                                  ('--' + disallowed))
+        userrevs = list(repo.revs(opts.get('auto')))
+        opts['rev'] = [revsetlang.formatspec('%ld', userrevs)]
+        opts['dest'] = '_destautorebase(SRC)'
+
     if inmemory:
         try:
             # in-memory merge doesn't support conflicts, so if we hit any, abort