This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2668

rebase: introduce support for automatically rebasing orphan changes
ClosedPublic

Authored by durin42 on Mar 4 2018, 3:37 PM.

Details

Reviewers
indygreg
krbullock
Group Reviewers
hg-reviewers
Commits
rHG92213f6745ed: rebase: introduce support for automatically rebasing orphan changes
Summary

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

We've considered some alternatives here:

  • This change, but with --auto as the flag name. We're hedging our bets on this a little in this change so that if this ends up being the wrong direction we haven't burned the valauble --auto name on rebase.
  • --destination auto: I've got reservations about the discoverability of this, and we don't currently have a good story for a revset alias of sorts that changes behavior depending on the context in which it's used.
  • A "rebase presets" feature, where we could use the currently-an-error positional argument space for the rebase command to define presets, so that users could define a 'linearize' preset that specifies --revision='orphan()-obsolete()' and --dest=_destautoorphanrebase(SRC).

Personally, I find the third option somewhat appealing, but am
hesitant to "spend" the functionality space of positional arguments to
the rebase command. We should revisit the way we expose this
functionality sometime in the 4.7 cycle once we've had a chance to vet
the implementation of the functionality.

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

durin42 created this revision.Mar 4 2018, 3:37 PM
durin42 updated this revision to Diff 6611.Mar 4 2018, 3:39 PM
indygreg requested changes to this revision.Mar 4 2018, 5:08 PM
indygreg added a subscriber: indygreg.

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

hgext/rebase.py
111

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

135

This should use the node.nullrev constant.

674

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.

809

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

This revision now requires changes to proceed.Mar 4 2018, 5:08 PM
quark added a subscriber: quark.Mar 4 2018, 11:37 PM

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.

durin42 marked an inline comment as done.Apr 9 2018, 4:27 PM
In 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...

hgext/rebase.py
674

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?

durin42 updated this revision to Diff 7914.Apr 9 2018, 4:34 PM
durin42 added a subscriber: spectral.Apr 9 2018, 4:49 PM
In 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.

quark added a subscriber: mbthomas.EditedApr 9 2018, 7:29 PM
In 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.

UPDATE: For the revset namespace issue, I think we can define something like rebase.defaultdest, rebase.auto and modify revset resolution layer to think about namespaces. And then pass namespaces to scmutil.rev* from the rebase command.

durin42 edited the summary of this revision. (Show Details)Apr 16 2018, 10:00 PM
durin42 updated this revision to Diff 8361.
krbullock requested changes to this revision.Apr 18 2018, 2:40 PM
krbullock added a subscriber: krbullock.

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.

hgext/rebase.py
812

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.

This revision now requires changes to proceed.Apr 18 2018, 2:40 PM
durin42 updated this revision to Diff 8390.Apr 18 2018, 2:50 PM
durin42 marked an inline comment as done.Apr 18 2018, 2:50 PM

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.

krbullock accepted this revision.Apr 18 2018, 3:00 PM
Closed by commit rHG92213f6745ed: rebase: introduce support for automatically rebasing orphan changes (authored by durin42). · Explain WhyApr 18 2018, 3:30 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/rebase.py (33 lines)
Mmercurial/destutil.py (33 lines)
Mtests/test-rebase-obsolete.t (27 lines)

Diff 8392

hgext/rebase.py

# # XXX: Currently private because I expect the signature to change. # # XXX: Currently private because I expect the signature to change.
# # XXX: - bailing out in case of ambiguity vs returning all data. # # XXX: - bailing out in case of ambiguity vs returning all data.
# i18n: "_rebasedefaultdest" is a keyword # i18n: "_rebasedefaultdest" is a keyword
sourceset = None sourceset = None
if x is not None: if x is not None:
sourceset = revset.getset(repo, smartset.fullreposet(repo), x) sourceset = revset.getset(repo, smartset.fullreposet(repo), x)
return subset & smartset.baseset([_destrebase(repo, sourceset)]) return subset & smartset.baseset([_destrebase(repo, sourceset)])
@revsetpredicate('_destautoorphanrebase')
indygregUnsubmitted
Not Done

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

indygreg: Let's add this to `destutil.py` instead of adding more code to `rebase.py`.
def _revsetdestautoorphanrebase(repo, subset, x):
"""automatic rebase destination for a single orphan 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 = destutil.orphanpossibledestination(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): def _ctxdesc(ctx):
"""short description for a context""" """short description for a context"""
desc = '%d:%s "%s"' % (ctx.rev(), ctx, desc = '%d:%s "%s"' % (ctx.rev(), ctx,
ctx.description().split('\n', 1)[0]) ctx.description().split('\n', 1)[0])
repo = ctx.repo() repo = ctx.repo()
names = [] names = []
indygregUnsubmitted
Not Done

This should use the node.nullrev constant.

indygreg: This should use the `node.nullrev` constant.
for nsname, ns in repo.names.iteritems(): for nsname, ns in repo.names.iteritems():
if nsname == 'branches': if nsname == 'branches':
continue continue
names.extend(ns.names(repo, ctx.node())) names.extend(ns.names(repo, ctx.node()))
if names: if names:
desc += ' (%s)' % ' '.join(names) desc += ' (%s)' % ' '.join(names)
return desc return desc
('l', 'logfile', '', ('l', 'logfile', '',
_('read collapse commit message from file'), _('FILE')), _('read collapse commit message from file'), _('FILE')),
('k', 'keep', False, _('keep original changesets')), ('k', 'keep', False, _('keep original changesets')),
('', 'keepbranches', False, _('keep original branch names')), ('', 'keepbranches', False, _('keep original branch names')),
('D', 'detach', False, _('(DEPRECATED)')), ('D', 'detach', False, _('(DEPRECATED)')),
('i', 'interactive', False, _('(DEPRECATED)')), ('i', 'interactive', False, _('(DEPRECATED)')),
('t', 'tool', '', _('specify merge tool')), ('t', 'tool', '', _('specify merge tool')),
('c', 'continue', False, _('continue an interrupted rebase')), ('c', 'continue', False, _('continue an interrupted rebase')),
('a', 'abort', False, _('abort an interrupted rebase'))] + ('a', 'abort', False, _('abort an interrupted rebase')),
('', 'auto-orphans', '', _('automatically rebase orphan revisions '
indygregUnsubmitted
Not Done

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.

indygreg: The fact that `auto` takes an argument makes it... not very //auto//. I have a slight…
durin42AuthorUnsubmitted
Not Done

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?

durin42: I don't follow. It automatically rebases the specified set of revisions - that is, it…
'in the specified revset (EXPERIMENTAL)')),
] +
cmdutil.formatteropts, cmdutil.formatteropts,
_('[-s REV | -b REV] [-d REV] [OPTION]')) _('[-s REV | -b REV] [-d REV] [OPTION]'))
def rebase(ui, repo, **opts): def rebase(ui, repo, **opts):
"""move changeset (and descendants) to a different branch """move changeset (and descendants) to a different branch
Rebase uses repeated merging to graft changesets from one part of Rebase uses repeated merging to graft changesets from one part of
history (the source) onto another (the destination). This can be history (the source) onto another (the destination). This can be
useful for linearizing *local* changes relative to a master useful for linearizing *local* changes relative to a master
inmemory = ui.configbool('rebase', 'experimental.inmemory') inmemory = ui.configbool('rebase', 'experimental.inmemory')
if (opts.get('continue') or opts.get('abort') or if (opts.get('continue') or opts.get('abort') or
repo.currenttransaction() is not None): repo.currenttransaction() is not None):
# in-memory rebase is not compatible with resuming rebases. # in-memory rebase is not compatible with resuming rebases.
# (Or if it is run within a transaction, since the restart logic can # (Or if it is run within a transaction, since the restart logic can
# fail the entire transaction.) # fail the entire transaction.)
inmemory = False inmemory = False
if opts.get('auto_orphans'):
for key in opts:
indygregUnsubmitted
Done

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

indygreg: An allow list is better. But I could let this slide.
if key != 'auto_orphans' and opts.get(key):
raise error.Abort(_('--auto-orphans is incompatible with %s') %
('--' + key))
krbullockUnsubmitted
Done

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.

krbullock: Would be nice to see test coverage of this... it looks to me like we could end up spitting out…
userrevs = list(repo.revs(opts.get('auto_orphans')))
opts['rev'] = [revsetlang.formatspec('%ld and orphan()', userrevs)]
opts['dest'] = '_destautoorphanrebase(SRC)'
if inmemory: if inmemory:
try: try:
# in-memory merge doesn't support conflicts, so if we hit any, abort # in-memory merge doesn't support conflicts, so if we hit any, abort
# and re-run as an on-disk merge. # and re-run as an on-disk merge.
overrides = {('rebase', 'singletransaction'): True} overrides = {('rebase', 'singletransaction'): True}
with ui.configoverride(overrides, 'rebase'): with ui.configoverride(overrides, 'rebase'):
return _origrebase(ui, repo, inmemory=inmemory, **opts) return _origrebase(ui, repo, inmemory=inmemory, **opts)
except error.InMemoryMergeConflictsError: except error.InMemoryMergeConflictsError:

mercurial/destutil.py

from . import ( from . import (
bookmarks, bookmarks,
error, error,
obsutil, obsutil,
scmutil, scmutil,
stack stack
) )
def orphanpossibledestination(repo, rev):
"""Return all changesets that may be a new parent for orphan `rev`.
This function works fine on non-orphan revisions, it's just silly
because there's no destination implied by obsolete markers, so
it'll return nothing.
"""
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
def _destupdateobs(repo, clean): def _destupdateobs(repo, clean):
"""decide of an update destination from obsolescence markers""" """decide of an update destination from obsolescence markers"""
node = None node = None
wc = repo[None] wc = repo[None]
p1 = wc.p1() p1 = wc.p1()
movemark = None movemark = None
if p1.obsolete() and not p1.children(): if p1.obsolete() and not p1.children():

tests/test-rebase-obsolete.t

| o 6:eea13746799a G | o 6:eea13746799a G
|/| |/|
o | 5:24b6387c8c8c F o | 5:24b6387c8c8c F
| | | |
| o 4:9520eea781bc E | o 4:9520eea781bc E
|/ |/
o 0:cd010b8cd998 A o 0:cd010b8cd998 A
$ cd ..
$ cp -R hidden stabilize
$ cd stabilize
$ hg rebase --auto-orphans '0::' -d 10
abort: --auto-orphans is incompatible with --dest
[255]
$ hg rebase --auto-orphans '0::'
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 Test multiple root handling
------------------------------------ ------------------------------------
$ hg rebase --dest 4 --rev '7+11+9' $ hg rebase --dest 4 --rev '7+11+9'
rebasing 9:cf44d2f5a9f4 "D" rebasing 9:cf44d2f5a9f4 "D"
rebasing 7:02de42196ebe "H" rebasing 7:02de42196ebe "H"
rebasing 11:0d8f238b634c "C" (tip) rebasing 11:0d8f238b634c "C" (tip)