This is an archive of the discontinued Mercurial Phabricator instance.

Differential D728

rebase: move working parent movement logic to scmutil.cleanupnodes
AbandonedPublic

Authored by durin42 on Sep 18 2017, 4:20 PM.

Details

Reviewers
durham
quark
Group Reviewers
hg-reviewers
Summary

Moving working parent is a common requirement that could be shared in other
places. This patch makes it possible.

This would make rebase more solid since the bookmark movement and working
parent movement is strongly guaranteed to share same rules.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Sep 18 2017, 4:20 PM
quark edited the summary of this revision. (Show Details)Sep 18 2017, 4:32 PM
pulkit added a subscriber: pulkit.Sep 18 2017, 4:34 PM

How about having a new upper level function which handles all these things and also call scmutil.cleanupnodes? Maybe we can add phases logic in that function also afterwards.

quark added a comment.EditedSep 18 2017, 4:40 PM
In D728#12181, @pulkit wrote:

How about having a new upper level function which handles all these things and also call scmutil.cleanupnodes? Maybe we can add phases logic in that function also afterwards.

It seems that you want a copyphase=True|False flag which could be added to cleanupnodes.

For working copy movement, it shares the logic with bookmark movement so it's unlikely to be a separate method.

I'm wondering if hg.cleanupnodes would be a better place than scmutil since it's getting more complex (and it also solves import cycles).

pulkit added a comment.Sep 18 2017, 4:48 PM
In D728#12194, @quark wrote:
In D728#12181, @pulkit wrote:

How about having a new upper level function which handles all these things and also call scmutil.cleanupnodes? Maybe we can add phases logic in that function also afterwards.

From these I mean bookmark+wdir.

It seems that you want a copyphase=True|False flag which could be added to cleanupnodes.
For working copy movement, it shares the logic with bookmark movement so it's unlikely to be a separate method.
I'm wondering if hg.cleanupnodes would be a better place than scmutil since it's getting more complex (and it also solves import cycles).

I was suggesting a function like hg.updatenodes(mapping, wdir, phase) which handles (phase+wdir+bookmark) and calls smcutil.cleanupnodes. That sounds cleaner but we need to extract the bookmark logic and maybe we cannot do that. :(
I was also concerned about it getting more complex. I was also going to add a formatter argument to it.

quark added a comment.EditedSep 18 2017, 5:00 PM

From these I mean bookmark+wdir.
I was suggesting a function like hg.updatenodes(mapping, wdir, phase) which handles (phase+wdir+bookmark) and calls smcutil.cleanupnodes. That sounds cleaner but we need to extract the bookmark logic and maybe we cannot do that. :(

Suppose there is a higher-level hg.updatenodes calling today's scmutil.cleanupnodes, what's the use-case of calling scmutil.cleanupnodes directly? If no, I'd prefer less APIs to make it harder for extension developers to make mistake.

I was also concerned about it getting more complex. I was also going to add a formatter argument to it.

I think formatter should be a separate method. Suppose rebase can output more than just "changes", ex. the user runs rebase -T '{changes} {rebasesource} {rebasedest}, it's not cleanupnodes to resolve the template. What you need is something to register changes as part of the template function context so the template engine knows how to resolve the name changes.

durham requested changes to this revision.Sep 26 2017, 8:10 AM
durham added a subscriber: durham.
durham added inline comments.
mercurial/scmutil.py
603

This seems like a bit of a bizarre feature for an api about cleaning up nodes. Maybe cleanup nodes should return a mapping of before-to-after and the caller can use that to perform an update if they need to? I'd rather have APIs that can be composed, than add new permutations to existing APIs.

This revision now requires changes to proceed.Sep 26 2017, 8:10 AM
quark added inline comments.Sep 26 2017, 1:03 PM
mercurial/scmutil.py
603

The function is to make it easier to do common tasks: obsolete/strip, move bookmarks, move working parent. Since we have bookmark movement already. It makes sense to me to also have working parent movement.

I agree the API could be split, so bookmark and working copy movement could be used independently. But cleanupnodes is intended to be the composed function.

durin42 added a reviewer: quark.Oct 14 2017, 1:00 AM
durin42 commandeered this revision.

Let's reopen this if/when we return to it.

durin42 abandoned this revision.Oct 14 2017, 3:21 AM

Let's reopen this if/when we return to it.

Revision Contents
Changeset List

PathPackages
Mhgext/rebase.py (21 lines)
Mmercurial/scmutil.py (24 lines)
Mtests/test-rebase-conflicts.t (1 line)

Diff 1870

hgext/rebase.py

bookmarks, bookmarks,
cmdutil, cmdutil,
commands, commands,
copies, copies,
destutil, destutil,
dirstateguard, dirstateguard,
error, error,
extensions, extensions,
hg,
lock, lock,
merge as mergemod, merge as mergemod,
mergeutil, mergeutil,
obsolete, obsolete,
obsutil, obsutil,
patch, patch,
phases, phases,
registrar, registrar,
if newnode is not None: if newnode is not None:
newrev = repo[newnode].rev() newrev = repo[newnode].rev()
for oldrev in self.state.iterkeys(): for oldrev in self.state.iterkeys():
self.state[oldrev] = newrev self.state[oldrev] = newrev
if 'qtip' in repo.tags(): if 'qtip' in repo.tags():
updatemq(repo, self.state, self.skipped, **opts) updatemq(repo, self.state, self.skipped, **opts)
# restore original working directory
# (we do this before stripping)
newwd = self.state.get(self.originalwd, self.originalwd)
if newwd < 0:
# original directory is a parent of rebase set root or ignored
newwd = self.originalwd
if newwd not in [c.rev() for c in repo[None].parents()]:
ui.note(_("update back to initial working directory parent\n"))
hg.updaterepo(repo, newwd, False)
collapsedas = None collapsedas = None
if not self.keepf: 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,
collapsedas, self.keepf) collapsedas, self.keepf, self.originalwd)
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)
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): keepf=False, originalwd=None):
"""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 If `keepf` is not True, the rebase has --keep set and no nodes should be
removed (but bookmarks still need to be moved). removed (but bookmarks still need to be moved).
If `originalwd` is not None, working parent will be moved.
""" """
tonode = repo.changelog.node tonode = repo.changelog.node
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) newnode = collapsedas or tonode(newrev)
value = {'move': newnode} value = {'move': newnode}
if not keepf: if not keepf:
if rev in skipped: if rev in skipped:
succs = () succs = ()
else: else:
succs = (newnode,) succs = (newnode,)
value['new'] = succs value['new'] = succs
mapping[tonode(rev)] = value mapping[tonode(rev)] = value
scmutil.cleanupnodes(repo, mapping, 'rebase') if originalwd is not None:
originalwd = repo[originalwd].node()
scmutil.cleanupnodes(repo, mapping, 'rebase', workingparent=originalwd)
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, mapping, operation): def cleanupnodes(repo, mapping, operation, workingparent=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, moving bookmarks, and
(we might also want to move working directory parent in the future) optionally moving working parent.
mapping is {oldnode: {'new': [newnode], 'move': newnode}}, where 'new' mapping is {oldnode: {'new': [newnode], 'move': newnode}}, where 'new'
decide obsolete markers, and 'move' decide bookmark move, which could be decide obsolete markers, and 'move' decide bookmark move, which could be
different from new. If 'move' is not provided, it will be calculated 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 automatically from 'new'. If 'new' is not set, the oldnode will not be
removed or obsoleted. removed or obsoleted.
To make the API easier to use, mapping could also be in the following To make the API easier to use, mapping could also be in the following
forms: forms:
- {oldnode: [newnode]}, equal to {oldnode: {'new': [newnode]}} - {oldnode: [newnode]}, equal to {oldnode: {'new': [newnode]}}
- {oldnode}, equal to {oldnode: []} - {oldnode}, equal to {oldnode: []}
If workingparent is not None, it's the desired node that needs to update
to. The node may be adjusted using the bookmark movement logic. Uncommitted
changes will be discarded, the caller is responsible for checking the
cleanness of working copy.
durhamUnsubmitted
Not Done

This seems like a bit of a bizarre feature for an api about cleaning up nodes. Maybe cleanup nodes should return a mapping of before-to-after and the caller can use that to perform an update if they need to? I'd rather have APIs that can be composed, than add new permutations to existing APIs.

durham: This seems like a bit of a bizarre feature for an api about cleaning up nodes. Maybe cleanup…
quarkUnsubmitted
Not Done

The function is to make it easier to do common tasks: obsolete/strip, move bookmarks, move working parent. Since we have bookmark movement already. It makes sense to me to also have working parent movement.

I agree the API could be split, so bookmark and working copy movement could be used independently. But cleanupnodes is intended to be the composed function.

quark: The function is to make it easier to do common tasks: obsolete/strip, move bookmarks, move…
""" """
if not mapping: if not mapping:
return return
# translate mapping's other forms # translate mapping's other forms
if not util.safehasattr(mapping, 'items'): if not util.safehasattr(mapping, 'items'):
mapping = {n: {'new': ()} 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()): 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()} mapping = {n: {'new': newnodes} for n, newnodes in mapping.items()}
bmarkwp = '' # an invalid bookmark name to indicate working parent
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 v in mapping.values() for n in v.get('new', ())] allnewnodes = [n for v in mapping.values() for n in v.get('new', ())]
for oldnode, value in mapping.items(): for oldnode, value in mapping.items():
oldbmarks = repo.nodebookmarks(oldnode) oldbmarks = repo.nodebookmarks(oldnode)
if workingparent is not None and oldnode == workingparent:
oldbmarks = oldbmarks + [bmarkwp]
if not oldbmarks: if not oldbmarks:
continue continue
newnode = value.get('move') newnode = value.get('move')
from . import bookmarks # avoid import cycle from . import bookmarks # avoid import cycle
if not newnode: if not newnode:
newnodes = value.get('new') newnodes = value.get('new')
if newnodes is None: if newnodes is None:
continue continue
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:
if name is bmarkwp: # working parent adjustment
workingparent = newnode
continue
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)
# Working parent movement
if workingparent and [workingparent] != [r.node()
for r in repo[None].parents()]:
from . import hg # avoid import cycle
hg.updaterepo(repo, workingparent, False)
# 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.

tests/test-rebase-conflicts.t

getting f1.txt getting f1.txt
committing files: committing files:
f1.txt f1.txt
committing manifest committing manifest
committing changelog committing changelog
updating the branch cache updating the branch cache
rebased as 2a7f09cac94c rebased as 2a7f09cac94c
rebase merging completed rebase merging completed
update back to initial working directory parent
resolving manifests resolving manifests
branchmerge: False, force: False, partial: False branchmerge: False, force: False, partial: False
ancestor: 2a7f09cac94c, local: 2a7f09cac94c+, remote: d79e2059b5c0 ancestor: 2a7f09cac94c, local: 2a7f09cac94c+, remote: d79e2059b5c0
f1.txt: other deleted -> r f1.txt: other deleted -> r
removing f1.txt removing f1.txt
f2.txt: remote created -> g f2.txt: remote created -> g
getting f2.txt getting f2.txt
2 changesets found 2 changesets found