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
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.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
635–657

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
512–515

Can combine to one condition now

1363

drop "not", right?

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

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 2201

hgext/rebase.py

newwd = self.obsoletenotrebased[self.originalwd] newwd = self.obsoletenotrebased[self.originalwd]
elif newwd < 0: elif 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.dest, self.state, self.skipped, clearrebased(ui, repo, self.dest, 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)
state[ignored] = revignored state[ignored] = revignored
for r in obsoletenotrebased: for r in obsoletenotrebased:
if obsoletenotrebased[r] is None: if obsoletenotrebased[r] is None:
state[r] = revpruned state[r] = revpruned
else: else:
state[r] = revprecursor state[r] = revprecursor
return originalwd, dest.rev(), state return originalwd, dest.rev(), state
def clearrebased(ui, repo, dest, state, skipped, collapsedas=None): def clearrebased(ui, repo, dest, 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 replacements = {}
# by scmutil.cleanupnodes since it will treat rev as removed (no successor) moves = {}
# and move bookmark backwards.
bmchanges = [(name, tonode(max(adjustdest(repo, rev, dest, state))))
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 = {}
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:
oldnode = tonode(rev)
newnode = collapsedas or tonode(newrev)
moves[oldnode] = 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 replacements[oldnode] = succs
scmutil.cleanupnodes(repo, mapping, 'rebase') scmutil.cleanupnodes(repo, replacements, 'rebase', moves)
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')
hint = _('use hg pull followed by hg rebase -d DEST') hint = _('use hg pull followed by hg rebase -d DEST')

mercurial/scmutil.py

def __init__(self, repo, revcontainer): def __init__(self, repo, revcontainer):
self._torev = repo.changelog.rev self._torev = repo.changelog.rev
self._revcontains = revcontainer.__contains__ self._revcontains = revcontainer.__contains__
def __contains__(self, node): def __contains__(self, node):
return self._revcontains(self._torev(node)) return self._revcontains(self._torev(node))
def cleanupnodes(repo, replacements, operation): def cleanupnodes(repo, replacements, operation, moves=None):
"""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)
By default, bookmark moves are calculated automatically from 'replacements',
but 'moves' can be used to override that. Also, 'moves' may include
additional bookmark moves that should not have associated obsmarkers.
replacements is {oldnode: [newnode]} or a iterable of nodes if they do not replacements is {oldnode: [newnode]} or a iterable of nodes if they do not
have replacements. operation is a string, like "rebase". have replacements. operation is a string, like "rebase".
""" """
if not replacements and not moves:
return
# translate mapping's other forms
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(replacements, 'items'): if not util.safehasattr(replacements, 'items'):
replacements = {n: () for n in replacements} replacements = {n: () for n in replacements}
# Calculate bookmark movements # Calculate bookmark movements
if moves is None:
moves = {} moves = {}
# Unfiltered repo is needed since nodes in replacements might be hidden. # Unfiltered repo is needed since nodes in replacements might be hidden.
unfi = repo.unfiltered() unfi = repo.unfiltered()
for oldnode, newnodes in replacements.items(): for oldnode, newnodes in replacements.items():
if oldnode in moves:
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(unfi.set('max(%ln)', newnodes)).node() newnode = next(unfi.set('max(%ln)', newnodes)).node()
elif len(newnodes) == 0: elif len(newnodes) == 0:
# move bookmark backwards # move bookmark backwards
roots = list(unfi.set('max((::%n) - %ln)', oldnode, roots = list(unfi.set('max((::%n) - %ln)', oldnode,
list(replacements))) list(replacements)))
if roots: if roots:
for oldnode, newnode in moves.items(): for oldnode, newnode in moves.items():
oldbmarks = repo.nodebookmarks(oldnode) oldbmarks = repo.nodebookmarks(oldnode)
if not oldbmarks: if not oldbmarks:
continue continue
from . import bookmarks # avoid import cycle from . import bookmarks # avoid import cycle
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))
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.
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 s))
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?
for n, s in sorted(replacements.items(), key=sortfunc) for n, s in sorted(replacements.items(), key=sortfunc)
if s or not isobs(n)] if s 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(replacements), operation) tostrip = list(replacements)
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)