This is an archive of the discontinued Mercurial Phabricator instance.

Differential D469

rebase: initial support for multiple destinations
ClosedPublic

Authored by quark on Aug 22 2017, 1:16 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG5e83a8fe6bc4: rebase: initial support for multiple destinations
Summary

This patch defines SRC (a single source revision) and ALLSRC (all source
revisions) to be valid names in --dest revset if --src or --rev is
used. So destination could be defined differently according to source
revisions. The names are capitalized to make it clear they are "dynamically
defined", distinguishable from normal revsets (Thanks Augie for the
suggestion).

This is useful, for example, -r 'orphan()' -d 'calc-dest(SRC)' to solve
instability, which seems to be a highly wanted feature.

The feature is not completed, namely if -d overlaps with -r, things
could go wrong. A later patch will handle that case.

The feature is also gated by experimental.rebase.multidest config option
which is default off.

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.Aug 22 2017, 1:16 AM
martinvonz added a subscriber: martinvonz.Aug 29 2017, 2:53 AM
martinvonz added inline comments.
hgext/rebase.py
822

could pass ALLSRC in the fast-path version too, no? because the concern is not that that calcularing "allsrc" is costly, i assume, but that running the revset over all the revisions is

824

anyrevs() doesn't seem to make much sense here. Why not let localrepo.revs() and localrepo.set() accept "user" and "localalias" arguments too? I realize it's more code, but it seem to make more sense. Unless this is just temporary and you're planning on passing a longer list here later.

824

The destination revset is resolved earlier than I had expected. Or at least I expected it to be evaluated later, *as well*, I suppose.

Let's say my revset is "successors(parents(SRC))" (i.e. a naive attempt at "hg evolve") and my graph looks like this:

o A'
| o C
| o B
| x A
|/
o

If I then do "hg rebase -s B -d <as above>", it seems like only B would be rebased. Do you have plans for making it so C gets rebased too?

tests/test-rebase-dest.t
295

Where is "map" defined?

quark added inline comments.Aug 29 2017, 9:09 AM
hgext/rebase.py
822

Good advice! I was focused on SRC and didn't consider the use-case where ALLSRC is used but not SRC. Will make fast path use it too.

824

repo.revs docstring has:

Revset aliases from the configuration are not expanded. To expand
user aliases, consider calling ``scmutil.revrange()`` or
``repo.anyrevs([expr], user=True)``.

I think it makes sense to expend them. So anyrevs is used. I guess that's also why any is part of the method name.

IIUC, most user-provided revsets are expected to be consumed by scmutil.revrange, which uses repo.anyrev. So anyrev sounds like the right choice here.

For the example, see the test cases in the next patch. You need a more complex revset.

Defer resolving revsets could result in a much more complex patch (state file format, the order of obsmarker creation and bookmark movement need change). Performance may decline since we need to re-resolve and re-sort everything after every DAG change. I guess there are also more corner cases to handle (ex. -d tip may behave undesirably).

Note that even if you'd like to defer resolving destinations, it still need to know destinations first to decide which changesets to begin with (see the next patch). So the code here is still necessary.

I'd like we start simple.

tests/test-rebase-dest.t
295

Line 89.

quark edited the summary of this revision. (Show Details)Aug 29 2017, 9:40 PM
quark updated this revision to Diff 1418.
Closed by commit rHG5e83a8fe6bc4: rebase: initial support for multiple destinations (authored by quark). · Explain WhyAug 30 2017, 12:53 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/rebase.py (58 lines)
Mmercurial/configitems.py (3 lines)
Mmercurial/scmutil.py (8 lines)
Mtests/test-rebase-dest.t (234 lines)

Diff 1426

hgext/rebase.py

obsolete, obsolete,
obsutil, obsutil,
patch, patch,
phases, phases,
registrar, registrar,
repair, repair,
repoview, repoview,
revset, revset,
revsetlang,
scmutil, scmutil,
smartset, smartset,
util, util,
) )
release = lock.release release = lock.release
templateopts = cmdutil.templateopts templateopts = cmdutil.templateopts
cmdutil.checkunfinished(repo) cmdutil.checkunfinished(repo)
cmdutil.bailifchanged(repo) cmdutil.bailifchanged(repo)
if ui.configbool('commands', 'rebase.requiredest') and not destf: if ui.configbool('commands', 'rebase.requiredest') and not destf:
raise error.Abort(_('you must specify a destination'), raise error.Abort(_('you must specify a destination'),
hint=_('use: hg rebase -d REV')) hint=_('use: hg rebase -d REV'))
if destf: dest = None
dest = scmutil.revsingle(repo, destf)
if revf: if revf:
rebaseset = scmutil.revrange(repo, revf) rebaseset = scmutil.revrange(repo, revf)
if not rebaseset: if not rebaseset:
ui.status(_('empty "rev" revision set - nothing to rebase\n')) ui.status(_('empty "rev" revision set - nothing to rebase\n'))
return None return None
elif srcf: elif srcf:
src = scmutil.revrange(repo, [srcf]) src = scmutil.revrange(repo, [srcf])
if not src: if not src:
ui.status(_('empty "source" revision set - nothing to rebase\n')) ui.status(_('empty "source" revision set - nothing to rebase\n'))
return None return None
rebaseset = repo.revs('(%ld)::', src) rebaseset = repo.revs('(%ld)::', src)
assert rebaseset assert rebaseset
else: else:
base = scmutil.revrange(repo, [basef or '.']) base = scmutil.revrange(repo, [basef or '.'])
if not base: if not base:
ui.status(_('empty "base" revision set - ' ui.status(_('empty "base" revision set - '
"can't compute rebase set\n")) "can't compute rebase set\n"))
return None return None
if not destf: if destf:
# --base does not support multiple destinations
dest = scmutil.revsingle(repo, destf)
else:
dest = repo[_destrebase(repo, base, destspace=destspace)] dest = repo[_destrebase(repo, base, destspace=destspace)]
destf = str(dest) destf = str(dest)
roots = [] # selected children of branching points roots = [] # selected children of branching points
bpbase = {} # {branchingpoint: [origbase]} bpbase = {} # {branchingpoint: [origbase]}
for b in base: # group bases by branching points for b in base: # group bases by branching points
bp = repo.revs('ancestor(%d, %d)', b, dest).first() bp = repo.revs('ancestor(%d, %d)', b, dest).first()
bpbase[bp] = bpbase.get(bp, []) + [b] bpbase[bp] = bpbase.get(bp, []) + [b]
ui.status(_('nothing to rebase from %s to %s\n') % ui.status(_('nothing to rebase from %s to %s\n') %
('+'.join(str(repo[r]) for r in base), dest)) ('+'.join(str(repo[r]) for r in base), dest))
return None return None
if not destf: if not destf:
dest = repo[_destrebase(repo, rebaseset, destspace=destspace)] dest = repo[_destrebase(repo, rebaseset, destspace=destspace)]
destf = str(dest) destf = str(dest)
# assign dest to each rev in rebaseset allsrc = revsetlang.formatspec('%ld', rebaseset)
alias = {'ALLSRC': allsrc}
if dest is None:
try:
# 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
martinvonzUnsubmitted
Not Done

could pass ALLSRC in the fast-path version too, no? because the concern is not that that calcularing "allsrc" is costly, i assume, but that running the revset over all the revisions is

martinvonz: could pass ALLSRC in the fast-path version too, no? because the concern is not that that…
quarkAuthorUnsubmitted
Not Done

Good advice! I was focused on SRC and didn't consider the use-case where ALLSRC is used but not SRC. Will make fast path use it too.

quark: Good advice! I was focused on `SRC` and didn't consider the use-case where `ALLSRC` is used but…
destmap = {}
for r in rebaseset:
martinvonzUnsubmitted
Not Done

anyrevs() doesn't seem to make much sense here. Why not let localrepo.revs() and localrepo.set() accept "user" and "localalias" arguments too? I realize it's more code, but it seem to make more sense. Unless this is just temporary and you're planning on passing a longer list here later.

martinvonz: anyrevs() doesn't seem to make much sense here. Why not let localrepo.revs() and localrepo.set…
quarkAuthorUnsubmitted
Not Done

repo.revs docstring has:

Revset aliases from the configuration are not expanded. To expand
user aliases, consider calling ``scmutil.revrange()`` or
``repo.anyrevs([expr], user=True)``.

I think it makes sense to expend them. So anyrevs is used. I guess that's also why any is part of the method name.

IIUC, most user-provided revsets are expected to be consumed by scmutil.revrange, which uses repo.anyrev. So anyrev sounds like the right choice here.

For the example, see the test cases in the next patch. You need a more complex revset.

Defer resolving revsets could result in a much more complex patch (state file format, the order of obsmarker creation and bookmark movement need change). Performance may decline since we need to re-resolve and re-sort everything after every DAG change. I guess there are also more corner cases to handle (ex. -d tip may behave undesirably).

Note that even if you'd like to defer resolving destinations, it still need to know destinations first to decide which changesets to begin with (see the next patch). So the code here is still necessary.

I'd like we start simple.

quark: `repo.revs` docstring has: Revset aliases from the configuration are not expanded. To…
martinvonzUnsubmitted
Not Done

The destination revset is resolved earlier than I had expected. Or at least I expected it to be evaluated later, *as well*, I suppose.

Let's say my revset is "successors(parents(SRC))" (i.e. a naive attempt at "hg evolve") and my graph looks like this:

o A'
| o C
| o B
| x A
|/
o

If I then do "hg rebase -s B -d <as above>", it seems like only B would be rebased. Do you have plans for making it so C gets rebased too?

martinvonz: The destination revset is resolved earlier than I had expected. Or at least I expected it to be…
alias['SRC'] = revsetlang.formatspec('%d', r)
# use repo.anyrevs instead of scmutil.revsingle because we
# don't want to abort if destset is empty.
destset = repo.anyrevs([destf], user=True, localalias=alias)
size = len(destset)
if size == 1:
destmap[r] = destset.first()
elif size == 0:
ui.note(_('skipping %s - empty destination\n') % repo[r])
else:
raise error.Abort(_('rebase destination for %s is not '
'unique') % repo[r])
if dest is not None:
# single-dest case: assign dest to each rev in rebaseset
destrev = dest.rev() destrev = dest.rev()
destmap = {r: destrev for r in rebaseset} # {srcrev: destrev} destmap = {r: destrev for r in rebaseset} # {srcrev: destrev}
if not destmap:
ui.status(_('nothing to rebase - empty destination\n'))
return None
return destmap return destmap
def externalparent(repo, state, destancestors): def externalparent(repo, state, destancestors):
"""Return the revision that should be used as the second parent """Return the revision that should be used as the second parent
when the revisions in state is collapsed on top of destancestors. when the revisions in state is collapsed on top of destancestors.
Abort if there is more than one parent. Abort if there is more than one parent.
""" """
parents = set() parents = set()
def adjustdest(repo, rev, destmap, state): def adjustdest(repo, rev, destmap, state):
"""adjust rebase destination given the current rebase state """adjust rebase destination given the current rebase state
rev is what is being rebased. Return a list of two revs, which are the rev is what is being rebased. Return a list of two revs, which are the
adjusted destinations for rev's p1 and p2, respectively. If a parent is adjusted destinations for rev's p1 and p2, respectively. If a parent is
nullrev, return dest without adjustment for it. nullrev, return dest without adjustment for it.
For example, when doing rebase -r B+E -d F, rebase will first move B to B1, For example, when doing rebasing B+E to F, C to G, rebase will first move B
and E's destination will be adjusted from F to B1. to B1, and E's destination will be adjusted from F to B1.
B1 <- written during rebasing B B1 <- written during rebasing B
| |
F <- original destination of B, E F <- original destination of B, E
| |
| E <- rev, which is being rebased | E <- rev, which is being rebased
| | | |
| D <- prev, one parent of rev being checked | D <- prev, one parent of rev being checked
| | | |
| x <- skipped, ex. no successor or successor in (::dest) | x <- skipped, ex. no successor or successor in (::dest)
| | | |
| C | C <- rebased as C', different destination
| | | |
| B <- rebased as B1 | B <- rebased as B1 C'
|/ |/ |
A A G <- destination of C, different
Another example about merge changeset, rebase -r C+G+H -d K, rebase will Another example about merge changeset, rebase -r C+G+H -d K, rebase will
first move C to C1, G to G1, and when it's checking H, the adjusted first move C to C1, G to G1, and when it's checking H, the adjusted
destinations will be [C1, G1]. destinations will be [C1, G1].
H C1 G1 H C1 G1
/| | / /| | /
F G |/ F G |/

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,

mercurial/scmutil.py

def intrev(ctx): def intrev(ctx):
"""Return integer for a given basectx that can be used in comparison or """Return integer for a given basectx that can be used in comparison or
arithmetic operation""" arithmetic operation"""
rev = ctx.rev() rev = ctx.rev()
if rev is None: if rev is None:
return wdirrev return wdirrev
return rev return rev
def revsingle(repo, revspec, default='.'): def revsingle(repo, revspec, default='.', localalias=None):
if not revspec and revspec != 0: if not revspec and revspec != 0:
return repo[default] return repo[default]
l = revrange(repo, [revspec]) l = revrange(repo, [revspec], localalias=localalias)
if not l: if not l:
raise error.Abort(_('empty revision set')) raise error.Abort(_('empty revision set'))
return repo[l.last()] return repo[l.last()]
def _pairspec(revspec): def _pairspec(revspec):
tree = revsetlang.parse(revspec) tree = revsetlang.parse(revspec)
return tree and tree[0] in ('range', 'rangepre', 'rangepost', 'rangeall') return tree and tree[0] in ('range', 'rangepre', 'rangepost', 'rangeall')
raise error.Abort(_('empty revision on one side of range')) raise error.Abort(_('empty revision on one side of range'))
# if top-level is range expression, the result must always be a pair # if top-level is range expression, the result must always be a pair
if first == second and len(revs) == 1 and not _pairspec(revs[0]): if first == second and len(revs) == 1 and not _pairspec(revs[0]):
return repo.lookup(first), None return repo.lookup(first), None
return repo.lookup(first), repo.lookup(second) return repo.lookup(first), repo.lookup(second)
def revrange(repo, specs): def revrange(repo, specs, localalias=None):
"""Execute 1 to many revsets and return the union. """Execute 1 to many revsets and return the union.
This is the preferred mechanism for executing revsets using user-specified This is the preferred mechanism for executing revsets using user-specified
config options, such as revset aliases. config options, such as revset aliases.
The revsets specified by ``specs`` will be executed via a chained ``OR`` The revsets specified by ``specs`` will be executed via a chained ``OR``
expression. If ``specs`` is empty, an empty result is returned. expression. If ``specs`` is empty, an empty result is returned.
Returns a ``revset.abstractsmartset`` which is a list-like interface over Returns a ``revset.abstractsmartset`` which is a list-like interface over
integer revisions. integer revisions.
""" """
allspecs = [] allspecs = []
for spec in specs: for spec in specs:
if isinstance(spec, int): if isinstance(spec, int):
spec = revsetlang.formatspec('rev(%d)', spec) spec = revsetlang.formatspec('rev(%d)', spec)
allspecs.append(spec) allspecs.append(spec)
return repo.anyrevs(allspecs, user=True) return repo.anyrevs(allspecs, user=True, localalias=localalias)
def meaningfulparents(repo, ctx): def meaningfulparents(repo, ctx):
"""Return list of meaningful (or all if debug) parentrevs for rev. """Return list of meaningful (or all if debug) parentrevs for rev.
For merges (two non-nullrev revisions) both parents are meaningful. For merges (two non-nullrev revisions) both parents are meaningful.
Otherwise the first parent revision is considered meaningful if it Otherwise the first parent revision is considered meaningful if it
is not the preceding revision. is not the preceding revision.
""" """

tests/test-rebase-dest.t

$ cd clone $ cd clone
$ echo f > f $ echo f > f
$ hg commit -qAm ff $ hg commit -qAm ff
$ hg pull --rebase $ hg pull --rebase
abort: rebase destination required by configuration abort: rebase destination required by configuration
(use hg pull followed by hg rebase -d DEST) (use hg pull followed by hg rebase -d DEST)
[255] [255]
Setup rebase with multiple destinations
$ cd $TESTTMP
$ cat >> $TESTTMP/maprevset.py <<EOF
> from __future__ import absolute_import
> from mercurial import registrar, revset, revsetlang, smartset
> revsetpredicate = registrar.revsetpredicate()
> cache = {}
> @revsetpredicate('map')
> def map(repo, subset, x):
> """(set, mapping)"""
> setarg, maparg = revsetlang.getargs(x, 2, 2, '')
> rset = revset.getset(repo, smartset.fullreposet(repo), setarg)
> mapstr = revsetlang.getstring(maparg, '')
> map = dict(a.split(':') for a in mapstr.split(','))
> rev = rset.first()
> desc = repo[rev].description()
> newdesc = map.get(desc)
> if newdesc == 'null':
> revs = [-1]
> else:
> query = revsetlang.formatspec('desc(%s)', newdesc)
> revs = repo.revs(query)
> return smartset.baseset(revs)
> EOF
$ cat >> $HGRCPATH <<EOF
> [ui]
> allowemptycommit=1
> [extensions]
> drawdag=$TESTDIR/drawdag.py
> [phases]
> publish=False
> [alias]
> tglog = log -G --template "{rev}: {desc} {instabilities}" -r 'sort(all(), topo)'
> [extensions]
> maprevset=$TESTTMP/maprevset.py
> [experimental]
> rebase.multidest=true
> stabilization=all
> EOF
$ rebasewithdag() {
> N=`$PYTHON -c "print($N+1)"`
> hg init repo$N && cd repo$N
> hg debugdrawdag
> hg rebase "$@" > _rebasetmp
> r=$?
> grep -v 'saved backup bundle' _rebasetmp
> [ $r -eq 0 ] && rm -f .hg/localtags && hg tglog
> cd ..
> return $r
> }
Destination resolves to an empty set:
$ rebasewithdag -s B -d 'SRC - SRC' <<'EOS'
> C
> |
> B
> |
> A
> EOS
nothing to rebase - empty destination
[1]
Multiple destinations and --collapse are not compatible:
$ rebasewithdag -s C+E -d 'SRC^^' --collapse <<'EOS'
> C F
> | |
> B E
> | |
> A D
> EOS
abort: --collapse does not work with multiple destinations
[255]
Multiple destinations cannot be used with --base:
$ rebasewithdag -b B+E -d 'SRC^^' --collapse <<'EOS'
> B E
> | |
> A D
> EOS
abort: unknown revision 'SRC'!
[255]
Rebase to null should work:
$ rebasewithdag -r A+C+D -d 'null' <<'EOS'
> C D
> | |
> A B
> EOS
already rebased 0:426bada5c675 "A" (A)
already rebased 2:dc0947a82db8 "C" (C)
rebasing 3:004dc1679908 "D" (D tip)
o 4: D
o 2: C
|
| o 1: B
|
o 0: A
Destination resolves to multiple changesets:
$ rebasewithdag -s B -d 'ALLSRC+SRC' <<'EOS'
> C
> |
> B
> |
> Z
> EOS
abort: rebase destination for f0a671a46792 is not unique
[255]
Destination is an ancestor of source:
$ rebasewithdag -s B -d 'SRC' <<'EOS'
> C
> |
> B
> |
> Z
> EOS
abort: source is ancestor of destination
[255]
Switch roots:
$ rebasewithdag -s 'all() - roots(all())' -d 'roots(all()) - ::SRC' <<'EOS'
> C F
> | |
> B E
> | |
> A D
> EOS
rebasing 2:112478962961 "B" (B)
rebasing 4:26805aba1e60 "C" (C)
rebasing 3:cd488e83d208 "E" (E)
rebasing 5:0069ba24938a "F" (F tip)
o 9: F
|
o 8: E
|
| o 7: C
| |
| o 6: B
| |
| o 1: D
|
o 0: A
Different destinations for merge changesets with a same root:
$ rebasewithdag -s B -d '((parents(SRC)-B-A)::) - (::ALLSRC)' <<'EOS'
> C G
> |\|
> | F
> |
> B E
> |\|
> A D
> EOS
rebasing 3:a4256619d830 "B" (B)
rebasing 6:8e139e245220 "C" (C tip)
o 8: C
|\
| o 7: B
| |\
o | | 5: G
| | |
| | o 4: E
| | |
o | | 2: F
/ /
| o 1: D
|
o 0: A
Move to a previous parent:
$ rebasewithdag -s E+F+G -d 'SRC^^' <<'EOS'
> H
> |
> D G
> |/
> C F
> |/
> B E # E will be ignored, since E^^ is empty
> |/
> A
> EOS
rebasing 4:33441538d4aa "F" (F)
rebasing 6:cf43ad9da869 "G" (G)
rebasing 7:eef94f3b5f03 "H" (H tip)
o 10: H
|
| o 5: D
|/
o 3: C
|
| o 9: G
|/
o 1: B
|
| o 8: F
|/
| o 2: E
|/
o 0: A
Source overlaps with destination (not handled well currently):
martinvonzUnsubmitted
Not Done

Where is "map" defined?

martinvonz: Where is "map" defined?
quarkAuthorUnsubmitted
Not Done

Line 89.

quark: Line 89.
$ rebasewithdag -s 'B+C+D' -d 'map(SRC, "B:C,C:D")' <<'EOS'
> B C D
> \|/
> A
> EOS
rebasing 1:112478962961 "B" (B)
rebasing 2:dc0947a82db8 "C" (C)
o 5: C
|
o 3: D
|
| o 4: B orphan
| |
| x 2: C
|/
o 0: A