This is an archive of the discontinued Mercurial Phabricator instance.

Differential D727

rebase: move bookmarks with --keep (issue5682)
ClosedPublic

Authored by quark on Sep 18 2017, 2:58 PM.

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG2f427b57bf90: rebase: move bookmarks with --keep (issue5682)
Summary

This is a regression caused by 3b7cb3d17137. We have documented the behavior
in rebase help:

Rebase will destroy original commits unless you use "--keep". It will
also move your bookmarks (even if you do).

So let's restore the old behavior.

It is done by changing scmutil.cleanupnodes to accept more information so
a node could have different "movement destination" from "successors". It
also helps simplifying the callsite as a side effect - the special bookmark
movement logic in rebase is removed.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Sep 18 2017, 2:58 PM
quark updated this revision to Diff 1868.Sep 18 2017, 3:03 PM
quark added inline comments.Sep 18 2017, 3:04 PM
mercurial/scmutil.py
624–646

This is unchanged if viewed with whitespace ignored: https://phab.mercurial-scm.org/D727?whitespace=ignore-all

martinvonz added a subscriber: martinvonz.Sep 20 2017, 2:47 AM

I'm tired, so I don't think I reviewed this properly, but seemed to make sense.

hgext/rebase.py
547–550

Can combine to one condition now

1522

drop "not", right?

martinvonz added inline comments.Sep 20 2017, 12:57 PM
mercurial/scmutil.py
588–603

This felt to me like it should be two separate maps, so I sent D748-D750. Let me know how that looks to you.

martinvonz requested changes to this revision.Sep 20 2017, 12:57 PM
This revision now requires changes to proceed.Sep 20 2017, 12:57 PM
quark added a subscriber: simonfar.Sep 21 2017, 6:33 PM
In D727#12457, @martinvonz wrote:

I'm tired, so I don't think I reviewed this properly, but seemed to make sense.

Take your time! Nowadays none of my upstream patches are urgent (for fb) - for important fixes, they are already committed internally as hotfixes. Kudos to @simonfar and Kostia who make hotfixes very easy to work with.

Closed by commit rHG2f427b57bf90: rebase: move bookmarks with --keep (issue5682) (authored by quark). · Explain WhySep 30 2017, 10:28 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 1868

hgext/rebase.py

newwd = self.state.get(self.originalwd, self.originalwd) newwd = self.state.get(self.originalwd, self.originalwd)
if newwd < 0: if newwd < 0:
# original directory is a parent of rebase set root or ignored # original directory is a parent of rebase set root or ignored
newwd = self.originalwd newwd = self.originalwd
if newwd not in [c.rev() for c in repo[None].parents()]: if newwd not in [c.rev() for c in repo[None].parents()]:
ui.note(_("update back to initial working directory parent\n")) ui.note(_("update back to initial working directory parent\n"))
hg.updaterepo(repo, newwd, False) hg.updaterepo(repo, newwd, False)
if not self.keepf:
collapsedas = None collapsedas = None
if not self.keepf:
if self.collapsef: if self.collapsef:
collapsedas = newnode collapsedas = newnode
clearrebased(ui, repo, self.destmap, self.state, self.skipped, clearrebased(ui, repo, self.destmap, self.state, self.skipped,
martinvonzUnsubmitted
Not Done

Can combine to one condition now

martinvonz: Can combine to one condition now
collapsedas) collapsedas, self.keepf)
clearstatus(repo) clearstatus(repo)
clearcollapsemsg(repo) clearcollapsemsg(repo)
ui.note(_("rebase completed\n")) ui.note(_("rebase completed\n"))
util.unlinkpath(repo.sjoin('undo'), ignoremissing=True) util.unlinkpath(repo.sjoin('undo'), ignoremissing=True)
if self.skipped: if self.skipped:
skippedlen = len(self.skipped) skippedlen = len(self.skipped)
return None return None
for rev in sorted(state): for rev in sorted(state):
parents = [p for p in repo.changelog.parentrevs(rev) if p != nullrev] parents = [p for p in repo.changelog.parentrevs(rev) if p != nullrev]
# if all parents of this revision are done, then so is this revision # if all parents of this revision are done, then so is this revision
if parents and all((state.get(p) == p for p in parents)): if parents and all((state.get(p) == p for p in parents)):
state[rev] = rev state[rev] = rev
return originalwd, destmap, state return originalwd, destmap, state
def clearrebased(ui, repo, destmap, state, skipped, collapsedas=None): def clearrebased(ui, repo, destmap, state, skipped, collapsedas=None,
keepf=False):
"""dispose of rebased revision at the end of the rebase """dispose of rebased revision at the end of the rebase
If `collapsedas` is not None, the rebase was a collapse whose result if the If `collapsedas` is not None, the rebase was a collapse whose result if the
`collapsedas` node.""" `collapsedas` node.
If `keepf` is not True, the rebase has --keep set and no nodes should be
martinvonzUnsubmitted
Not Done

drop "not", right?

martinvonz: drop "not", right?
removed (but bookmarks still need to be moved).
"""
tonode = repo.changelog.node tonode = repo.changelog.node
# Move bookmark of skipped nodes to destination. This cannot be handled
# by scmutil.cleanupnodes since it will treat rev as removed (no successor)
# and move bookmark backwards.
bmchanges = [(name, tonode(state[rev]))
for rev in skipped
for name in repo.nodebookmarks(tonode(rev))]
if bmchanges:
with repo.transaction('rebase') as tr:
repo._bookmarks.applychanges(repo, tr, bmchanges)
mapping = {} mapping = {}
for rev, newrev in sorted(state.items()): for rev, newrev in sorted(state.items()):
if newrev >= 0 and newrev != rev: if newrev >= 0 and newrev != rev:
newnode = collapsedas or tonode(newrev)
value = {'move': newnode}
if not keepf:
if rev in skipped: if rev in skipped:
succs = () succs = ()
elif collapsedas is not None:
succs = (collapsedas,)
else: else:
succs = (tonode(newrev),) succs = (newnode,)
mapping[tonode(rev)] = succs value['new'] = succs
mapping[tonode(rev)] = value
scmutil.cleanupnodes(repo, mapping, 'rebase') scmutil.cleanupnodes(repo, mapping, 'rebase')
def pullrebase(orig, ui, repo, *args, **opts): def pullrebase(orig, ui, repo, *args, **opts):
'Call rebase after pull if the latter has been invoked with --rebase' 'Call rebase after pull if the latter has been invoked with --rebase'
ret = None ret = None
if opts.get('rebase'): if opts.get('rebase'):
if ui.configbool('commands', 'rebase.requiredest'): if ui.configbool('commands', 'rebase.requiredest'):
msg = _('rebase destination required by configuration') msg = _('rebase destination required by configuration')

mercurial/scmutil.py

return self._revcontains(self._torev(node)) return self._revcontains(self._torev(node))
def cleanupnodes(repo, mapping, operation): def cleanupnodes(repo, mapping, operation):
"""do common cleanups when old nodes are replaced by new nodes """do common cleanups when old nodes are replaced by new nodes
That includes writing obsmarkers or stripping nodes, and moving bookmarks. That includes writing obsmarkers or stripping nodes, and moving bookmarks.
(we might also want to move working directory parent in the future) (we might also want to move working directory parent in the future)
mapping is {oldnode: [newnode]} or a iterable of nodes if they do not have mapping is {oldnode: {'new': [newnode], 'move': newnode}}, where 'new'
replacements. operation is a string, like "rebase". decide obsolete markers, and 'move' decide bookmark move, which could be
different from new. If 'move' is not provided, it will be calculated
automatically from 'new'. If 'new' is not set, the oldnode will not be
removed or obsoleted.
To make the API easier to use, mapping could also be in the following
forms:
- {oldnode: [newnode]}, equal to {oldnode: {'new': [newnode]}}
- {oldnode}, equal to {oldnode: []}
""" """
if not mapping:
return
# translate {oldnode} form
martinvonzUnsubmitted
Not Done

This felt to me like it should be two separate maps, so I sent D748-D750. Let me know how that looks to you.

martinvonz: This felt to me like it should be two separate maps, so I sent D748-D750. Let me know how that…
if not util.safehasattr(mapping, 'items'): if not util.safehasattr(mapping, 'items'):
mapping = {n: () for n in mapping} mapping = {n: {'new': ()} for n in mapping}
elif all('new' not in v and 'move' not in v for v in mapping.values()):
mapping = {n: {'new': newnodes} for n, newnodes in mapping.items()}
with repo.transaction('cleanup') as tr: with repo.transaction('cleanup') as tr:
# Move bookmarks # Move bookmarks
bmarks = repo._bookmarks bmarks = repo._bookmarks
bmarkchanges = [] bmarkchanges = []
allnewnodes = [n for ns in mapping.values() for n in ns] allnewnodes = [n for v in mapping.values() for n in v.get('new', ())]
for oldnode, newnodes in mapping.items(): for oldnode, value in mapping.items():
oldbmarks = repo.nodebookmarks(oldnode) oldbmarks = repo.nodebookmarks(oldnode)
if not oldbmarks: if not oldbmarks:
continue continue
newnode = value.get('move')
from . import bookmarks # avoid import cycle from . import bookmarks # avoid import cycle
if not newnode:
newnodes = value.get('new')
if newnodes is None:
continue
if len(newnodes) > 1: if len(newnodes) > 1:
# usually a split, take the one with biggest rev number # usually a split, take the one with biggest rev number
newnode = next(repo.set('max(%ln)', newnodes)).node() newnode = next(repo.set('max(%ln)', newnodes)).node()
elif len(newnodes) == 0: elif len(newnodes) == 0:
# move bookmark backwards # move bookmark backwards
roots = list(repo.set('max((::%n) - %ln)', oldnode, roots = list(repo.set('max((::%n) - %ln)', oldnode,
list(mapping))) list(mapping)))
if roots: if roots:
newnode = roots[0].node() newnode = roots[0].node()
else: else:
newnode = nullid newnode = nullid
else: else:
newnode = newnodes[0] newnode = newnodes[0]
repo.ui.debug('moving bookmarks %r from %s to %s\n' % repo.ui.debug('moving bookmarks %r from %s to %s\n' %
(oldbmarks, hex(oldnode), hex(newnode))) (oldbmarks, hex(oldnode), hex(newnode)))
# Delete divergent bookmarks being parents of related newnodes # Delete divergent bookmarks being parents of related newnodes
deleterevs = repo.revs('parents(roots(%ln & (::%n))) - parents(%n)', deleterevs = repo.revs('parents(roots(%ln & (::%n))) - parents(%n)',
allnewnodes, newnode, oldnode) allnewnodes, newnode, oldnode)
deletenodes = _containsnode(repo, deleterevs) deletenodes = _containsnode(repo, deleterevs)
for name in oldbmarks: for name in oldbmarks:
bmarkchanges.append((name, newnode)) bmarkchanges.append((name, newnode))
for b in bookmarks.divergent2delete(repo, deletenodes, name): for b in bookmarks.divergent2delete(repo, deletenodes, name):
bmarkchanges.append((b, None)) bmarkchanges.append((b, None))
quarkAuthorUnsubmitted
Not Done

This is unchanged if viewed with whitespace ignored: https://phab.mercurial-scm.org/D727?whitespace=ignore-all

quark: This is unchanged if viewed with whitespace ignored: https://phab.mercurial-scm.org/D727?
if bmarkchanges: if bmarkchanges:
bmarks.applychanges(repo, tr, bmarkchanges) bmarks.applychanges(repo, tr, bmarkchanges)
# Obsolete or strip nodes # Obsolete or strip nodes
if obsolete.isenabled(repo, obsolete.createmarkersopt): if obsolete.isenabled(repo, obsolete.createmarkersopt):
# If a node is already obsoleted, and we want to obsolete it # If a node is already obsoleted, and we want to obsolete it
# without a successor, skip that obssolete request since it's # without a successor, skip that obssolete request since it's
# unnecessary. That's the "if s or not isobs(n)" check below. # unnecessary. That's the "if s or not isobs(n)" check below.
# Also sort the node in topology order, that might be useful for # Also sort the node in topology order, that might be useful for
# some obsstore logic. # some obsstore logic.
# NOTE: the filtering and sorting might belong to createmarkers. # NOTE: the filtering and sorting might belong to createmarkers.
# Unfiltered repo is needed since nodes in mapping might be hidden. # Unfiltered repo is needed since nodes in mapping might be hidden.
unfi = repo.unfiltered() unfi = repo.unfiltered()
isobs = unfi.obsstore.successors.__contains__ isobs = unfi.obsstore.successors.__contains__
torev = unfi.changelog.rev torev = unfi.changelog.rev
sortfunc = lambda ns: torev(ns[0]) sortfunc = lambda ns: torev(ns[0])
rels = [(unfi[n], tuple(unfi[m] for m in s)) rels = [(unfi[n], tuple(unfi[m] for m in v['new']))
for n, s in sorted(mapping.items(), key=sortfunc) for n, v in sorted(mapping.items(), key=sortfunc)
if s or not isobs(n)] if 'new' in v and (v['new'] or not isobs(n))]
if rels:
obsolete.createmarkers(repo, rels, operation=operation) obsolete.createmarkers(repo, rels, operation=operation)
else: else:
from . import repair # avoid import cycle from . import repair # avoid import cycle
repair.delayedstrip(repo.ui, repo, list(mapping), operation) tostrip = [n for n, v in mapping.items() if 'new' in v]
if tostrip:
repair.delayedstrip(repo.ui, repo, tostrip, operation)
def addremove(repo, matcher, prefix, opts=None, dry_run=None, similarity=None): def addremove(repo, matcher, prefix, opts=None, dry_run=None, similarity=None):
if opts is None: if opts is None:
opts = {} opts = {}
m = matcher m = matcher
if dry_run is None: if dry_run is None:
dry_run = opts.get('dry_run') dry_run = opts.get('dry_run')
if similarity is None: if similarity is None:

tests/test-rebase-bookmarks.t

$ cat >> $HGRCPATH <<EOF $ cat >> $HGRCPATH <<EOF
> [extensions] > [extensions]
> rebase= > rebase=
> drawdag=$TESTDIR/drawdag.py
> >
> [phases] > [phases]
> publish=False > publish=False
> >
> [alias] > [alias]
> tglog = log -G --template "{rev}: '{desc}' bookmarks: {bookmarks}\n" > tglog = log -G --template "{rev}: '{desc}' bookmarks: {bookmarks}\n"
> EOF > EOF
$ hg ci -m bisect3 $ hg ci -m bisect3
$ hg book bisect $ hg book bisect
$ hg update -q Y $ hg update -q Y
$ hg rebase -r '"bisect"^^::"bisect"^' -r bisect -d Z $ hg rebase -r '"bisect"^^::"bisect"^' -r bisect -d Z
rebasing 5:345c90f326a4 "bisect" rebasing 5:345c90f326a4 "bisect"
rebasing 6:f677a2907404 "bisect2" rebasing 6:f677a2907404 "bisect2"
rebasing 7:325c16001345 "bisect3" (tip bisect) rebasing 7:325c16001345 "bisect3" (tip bisect)
saved backup bundle to $TESTTMP/a3/.hg/strip-backup/345c90f326a4-b4840586-rebase.hg (glob) saved backup bundle to $TESTTMP/a3/.hg/strip-backup/345c90f326a4-b4840586-rebase.hg (glob)
Bookmark and working parent get moved even if --keep is set (issue5682)
$ hg init $TESTTMP/book-keep
$ cd $TESTTMP/book-keep
$ hg debugdrawdag <<'EOS'
> B C
> |/
> A
> EOS
$ eval `hg tags -T 'hg bookmark -ir {node} {tag};\n' | grep -v tip`
$ rm .hg/localtags
$ hg up -q B
$ hg tglog
o 2: 'C' bookmarks: C
|
| @ 1: 'B' bookmarks: B
|/
o 0: 'A' bookmarks: A
$ hg rebase -r B -d C --keep
rebasing 1:112478962961 "B" (B)
$ hg tglog
@ 3: 'B' bookmarks: B
|
o 2: 'C' bookmarks: C
|
| o 1: 'B' bookmarks:
|/
o 0: 'A' bookmarks: A

tests/test-rebase-emptycommit.t

| o 3 D BOOK-D | o 3 D BOOK-D
| | | |
| o 2 C BOOK-C | o 2 C BOOK-C
| | | |
o | 1 B o | 1 B
|/ |/
o 0 A o 0 A
With --keep, bookmark should not move With --keep, bookmark should move
$ hg rebase -r 3+4 -d E --keep $ hg rebase -r 3+4 -d E --keep
rebasing 3:e7b3f00ed42e "D" (BOOK-D) rebasing 3:e7b3f00ed42e "D" (BOOK-D)
note: rebase of 3:e7b3f00ed42e created no changes to commit note: rebase of 3:e7b3f00ed42e created no changes to commit
rebasing 4:69a34c08022a "E" (BOOK-E) rebasing 4:69a34c08022a "E" (BOOK-E)
note: rebase of 4:69a34c08022a created no changes to commit note: rebase of 4:69a34c08022a created no changes to commit
$ hg log -G -T '{rev} {desc} {bookmarks}' $ hg log -G -T '{rev} {desc} {bookmarks}'
o 7 E o 7 E BOOK-D BOOK-E
| |
o 6 D o 6 D
| |
| o 5 F BOOK-F | o 5 F BOOK-F
| | | |
| o 4 E BOOK-E | o 4 E
| | | |
| o 3 D BOOK-D | o 3 D
| | | |
| o 2 C BOOK-C | o 2 C BOOK-C
| | | |
o | 1 B o | 1 B
|/ |/
o 0 A o 0 A
Move D and E back for the next test
$ hg bookmark BOOK-D -fqir 3
$ hg bookmark BOOK-E -fqir 4
Bookmark is usually an indication of a head. For changes that are introduced by Bookmark is usually an indication of a head. For changes that are introduced by
an ancestor of bookmark B, after moving B to B-NEW, the changes are ideally an ancestor of bookmark B, after moving B to B-NEW, the changes are ideally
still introduced by an ancestor of changeset on B-NEW. In the below case, still introduced by an ancestor of changeset on B-NEW. In the below case,
"BOOK-D", and "BOOK-E" include changes introduced by "C". "BOOK-D", and "BOOK-E" include changes introduced by "C".
$ hg rebase -s 2 -d E $ hg rebase -s 2 -d E
rebasing 2:dc0947a82db8 "C" (C BOOK-C) rebasing 2:dc0947a82db8 "C" (C BOOK-C)
rebasing 3:e7b3f00ed42e "D" (BOOK-D) rebasing 3:e7b3f00ed42e "D" (BOOK-D)