This is an archive of the discontinued Mercurial Phabricator instance.

Differential D644

rebase: remove complex unhiding code
ClosedPublic

Authored by quark on Sep 6 2017, 7:52 PM.

Details

Reviewers
durham
Group Reviewers
hg-reviewers
Commits
rHG5d45a997d11d: rebase: remove complex unhiding code
Summary

This is similar to Martin von Zweigbergk's previous patch [1].

Previous patches are adding more .unfiltered() to the rebase code. So I
wonder: are we playing whack-a-mole regarding on unfiltered() in rebase?

Thinking about it, I believe most of the rebase code *should* just use an
unfiltered repo. The only exception is before we figuring out a
rebasestate. This patch makes it so. See added comment in code for why
that's more reasonable.

This would make the code base cleaner (not mangling the repo object),
faster (no need to invalidate caches), simpler (less LOC), less error-prone
(no need to think about what to unhide, ex. should we unhide wdir p2? how
about destinations?), and future proof (other code may change visibility in
an unexpected way, ex. directaccess may make the destination only visible
when it's in "--dest" revset tree).

[1]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-March/094277.html

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 6 2017, 7:52 PM
quark updated this revision to Diff 1644.Sep 6 2017, 7:55 PM
quark edited the summary of this revision. (Show Details)Sep 6 2017, 7:58 PM
durham added a subscriber: durham.Sep 7 2017, 8:43 PM

This sound reasonable, assuming all descendant/children computations are done before prepared is set. Could you run the fb-hgext inhibit/fbamend tests with this change? Since they might do some more interesting acrobatics around visibility changes.

durham added a comment.Sep 7 2017, 8:45 PM

I'd love to get rid of hidden filtering everywhere. Just limit knowledge of "hidden" to getting the list of visible heads, and algorithms that need to traverse children.

quark added a comment.Sep 11 2017, 1:50 PM

In theory things would be fine because fbamend and inhibit is to make things more visible, not invisible. This patch also makes things from invisible to visible so they do not conflict.

I have run ./run-tests.py --extra-config-opt=extensions.fbamend= --extra-config-opt=extensions.inhibit= --extra-config-opt=fbamend.safestrip=0 --extra-config-opt=fbamend.userestack=1 -l test-rebase-*.t and only test-rebase-obsolete.t has changed due to the "use restck" hint and an update to obsoleted wd makes it visible. They are both expected behavior differences.

durham accepted this revision.Sep 11 2017, 2:00 PM
Closed by commit rHG5d45a997d11d: rebase: remove complex unhiding code (authored by quark). · Explain WhySep 11 2017, 2:00 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/rebase.py (52 lines)

Diff 1719

hgext/rebase.py

merge as mergemod, merge as mergemod,
mergeutil, mergeutil,
obsolete, obsolete,
obsutil, obsutil,
patch, patch,
phases, phases,
registrar, registrar,
repair, repair,
repoview,
revset, revset,
revsetlang, revsetlang,
scmutil, scmutil,
smartset, smartset,
util, util,
) )
release = lock.release release = lock.release
return desc return desc
class rebaseruntime(object): class rebaseruntime(object):
"""This class is a container for rebase runtime state""" """This class is a container for rebase runtime state"""
def __init__(self, repo, ui, opts=None): def __init__(self, repo, ui, opts=None):
if opts is None: if opts is None:
opts = {} opts = {}
self.repo = repo # prepared: whether we have rebasestate prepared or not. Currently it
# decides whether "self.repo" is unfiltered or not.
# The rebasestate has explicit hash to hash instructions not depending
# on visibility. If rebasestate exists (in-memory or on-disk), use
# unfiltered repo to avoid visibility issues.
# Before knowing rebasestate (i.e. when starting a new rebase (not
# --continue or --abort)), the original repo should be used so
# visibility-dependent revsets are correct.
self.prepared = False
self._repo = repo
self.ui = ui self.ui = ui
self.opts = opts self.opts = opts
self.originalwd = None self.originalwd = None
self.external = nullrev self.external = nullrev
# Mapping between the old revision id and either what is the new rebased # Mapping between the old revision id and either what is the new rebased
# revision or what needs to be done with the old revision. The state # revision or what needs to be done with the old revision. The state
# dict will be what contains most of the rebase progress state. # dict will be what contains most of the rebase progress state.
self.state = {} self.state = {}
self.keepf = opts.get('keep', False) self.keepf = opts.get('keep', False)
self.keepbranchesf = opts.get('keepbranches', False) self.keepbranchesf = opts.get('keepbranches', False)
# keepopen is not meant for use on the command line, but by # keepopen is not meant for use on the command line, but by
# other extensions # other extensions
self.keepopen = opts.get('keepopen', False) self.keepopen = opts.get('keepopen', False)
self.obsoletenotrebased = {} self.obsoletenotrebased = {}
@property
def repo(self):
if self.prepared:
return self._repo.unfiltered()
else:
return self._repo
def storestatus(self, tr=None): def storestatus(self, tr=None):
"""Store the current status to allow recovery""" """Store the current status to allow recovery"""
if tr: if tr:
tr.addfilegenerator('rebasestate', ('rebasestate',), tr.addfilegenerator('rebasestate', ('rebasestate',),
self._writestatus, location='plain') self._writestatus, location='plain')
else: else:
with self.repo.vfs("rebasestate", "w") as f: with self.repo.vfs("rebasestate", "w") as f:
self._writestatus(f) self._writestatus(f)
else: else:
newrev = v newrev = v
destnode = repo[destmap[d]].hex() destnode = repo[destmap[d]].hex()
f.write("%s:%s:%s\n" % (oldrev, newrev, destnode)) f.write("%s:%s:%s\n" % (oldrev, newrev, destnode))
repo.ui.debug('rebase status stored\n') repo.ui.debug('rebase status stored\n')
def restorestatus(self): def restorestatus(self):
"""Restore a previously stored status""" """Restore a previously stored status"""
self.prepared = True
repo = self.repo.unfiltered() repo = self.repo.unfiltered()
keepbranches = None keepbranches = None
legacydest = None legacydest = None
collapse = False collapse = False
external = nullrev external = nullrev
activebookmark = None activebookmark = None
state = {} state = {}
destmap = {} destmap = {}
seen = set(destmap.values()) seen = set(destmap.values())
for old, new in sorted(state.items()): for old, new in sorted(state.items()):
if new != revtodo and new in seen: if new != revtodo and new in seen:
skipped.add(old) skipped.add(old)
seen.add(new) seen.add(new)
repo.ui.debug('computed skipped revs: %s\n' % repo.ui.debug('computed skipped revs: %s\n' %
(' '.join(str(r) for r in sorted(skipped)) or None)) (' '.join(str(r) for r in sorted(skipped)) or None))
repo.ui.debug('rebase status resumed\n') repo.ui.debug('rebase status resumed\n')
_setrebasesetvisibility(repo, set(state.keys()) | {originalwd})
self.originalwd = originalwd self.originalwd = originalwd
self.destmap = destmap self.destmap = destmap
self.state = state self.state = state
self.skipped = skipped self.skipped = skipped
self.collapsef = collapse self.collapsef = collapse
self.keepf = keep self.keepf = keep
self.keepbranchesf = keepbranches self.keepbranchesf = keepbranches
inclusive=True) inclusive=True)
self.external = externalparent(self.repo, self.state, destancestors) self.external = externalparent(self.repo, self.state, destancestors)
for destrev in sorted(set(destmap.values())): for destrev in sorted(set(destmap.values())):
dest = self.repo[destrev] dest = self.repo[destrev]
if dest.closesbranch() and not self.keepbranchesf: if dest.closesbranch() and not self.keepbranchesf:
self.ui.status(_('reopening closed branch head %s\n') % dest) self.ui.status(_('reopening closed branch head %s\n') % dest)
self.prepared = True
def _performrebase(self, tr): def _performrebase(self, tr):
repo, ui = self.repo, self.ui repo, ui = self.repo, self.ui
if self.keepbranchesf: if self.keepbranchesf:
# insert _savebranch at the start of extrafns so if # insert _savebranch at the start of extrafns so if
# there's a user-provided extrafn it can clobber branch if # there's a user-provided extrafn it can clobber branch if
# desired # desired
self.extrafns.insert(0, _savebranch) self.extrafns.insert(0, _savebranch)
if self.collapsef: if self.collapsef:
# Oh well, just abort like normal # Oh well, just abort like normal
collapsemsg = '' collapsemsg = ''
else: else:
raise error.Abort(_('missing .hg/last-message.txt for rebase')) raise error.Abort(_('missing .hg/last-message.txt for rebase'))
return collapsemsg return collapsemsg
def clearstatus(repo): def clearstatus(repo):
'Remove the status files' 'Remove the status files'
_clearrebasesetvisibiliy(repo)
# Make sure the active transaction won't write the state file # Make sure the active transaction won't write the state file
tr = repo.currenttransaction() tr = repo.currenttransaction()
if tr: if tr:
tr.removefilegenerator('rebasestate') tr.removefilegenerator('rebasestate')
repo.vfs.unlinkpath("rebasestate", ignoremissing=True) repo.vfs.unlinkpath("rebasestate", ignoremissing=True)
def needupdate(repo, state): def needupdate(repo, state):
'''check whether we should `update --clean` away from a merge, or if '''check whether we should `update --clean` away from a merge, or if
def buildstate(repo, destmap, collapse): def buildstate(repo, destmap, collapse):
'''Define which revisions are going to be rebased and where '''Define which revisions are going to be rebased and where
repo: repo repo: repo
destmap: {srcrev: destrev} destmap: {srcrev: destrev}
''' '''
rebaseset = destmap.keys() rebaseset = destmap.keys()
originalwd = repo['.'].rev() originalwd = repo['.'].rev()
_setrebasesetvisibility(repo, set(rebaseset) | {originalwd})
# This check isn't strictly necessary, since mq detects commits over an # This check isn't strictly necessary, since mq detects commits over an
# applied patch. But it prevents messing up the working directory when # applied patch. But it prevents messing up the working directory when
# a partially completed rebase is blocked by mq. # a partially completed rebase is blocked by mq.
if 'qtip' in repo.tags(): if 'qtip' in repo.tags():
mqapplied = set(repo[s.node].rev() for s in repo.mq.applied) mqapplied = set(repo[s.node].rev() for s in repo.mq.applied)
if set(destmap.values()) & mqapplied: if set(destmap.values()) & mqapplied:
raise error.Abort(_('cannot rebase onto an applied mq patch')) raise error.Abort(_('cannot rebase onto an applied mq patch'))
commands.update(ui, repo) commands.update(ui, repo)
else: else:
if opts.get('tool'): if opts.get('tool'):
raise error.Abort(_('--tool can only be used with --rebase')) raise error.Abort(_('--tool can only be used with --rebase'))
ret = orig(ui, repo, *args, **opts) ret = orig(ui, repo, *args, **opts)
return ret return ret
def _setrebasesetvisibility(repo, revs):
"""store the currently rebased set on the repo object
This is used by another function to prevent rebased revision to because
hidden (see issue4504)"""
repo = repo.unfiltered()
repo._rebaseset = revs
# invalidate cache if visibility changes
hiddens = repo.filteredrevcache.get('visible', set())
if revs & hiddens:
repo.invalidatevolatilesets()
def _clearrebasesetvisibiliy(repo):
"""remove rebaseset data from the repo"""
repo = repo.unfiltered()
if '_rebaseset' in vars(repo):
del repo._rebaseset
def _rebasedvisible(orig, repo):
"""ensure rebased revs stay visible (see issue4504)"""
blockers = orig(repo)
blockers.update(getattr(repo, '_rebaseset', ()))
return blockers
def _filterobsoleterevs(repo, revs): def _filterobsoleterevs(repo, revs):
"""returns a set of the obsolete revisions in revs""" """returns a set of the obsolete revisions in revs"""
return set(r for r in revs if repo[r].obsolete()) return set(r for r in revs if repo[r].obsolete())
def _computeobsoletenotrebased(repo, rebaseobsrevs, destmap): def _computeobsoletenotrebased(repo, rebaseobsrevs, destmap):
"""return a mapping obsolete => successor for all obsolete nodes to be """return a mapping obsolete => successor for all obsolete nodes to be
rebased that have a successors in the destination rebased that have a successors in the destination
entry[1].append(('t', 'tool', '', entry[1].append(('t', 'tool', '',
_("specify merge tool for rebase"))) _("specify merge tool for rebase")))
cmdutil.summaryhooks.add('rebase', summaryhook) cmdutil.summaryhooks.add('rebase', summaryhook)
cmdutil.unfinishedstates.append( cmdutil.unfinishedstates.append(
['rebasestate', False, False, _('rebase in progress'), ['rebasestate', False, False, _('rebase in progress'),
_("use 'hg rebase --continue' or 'hg rebase --abort'")]) _("use 'hg rebase --continue' or 'hg rebase --abort'")])
cmdutil.afterresolvedstates.append( cmdutil.afterresolvedstates.append(
['rebasestate', _('hg rebase --continue')]) ['rebasestate', _('hg rebase --continue')])
# ensure rebased rev are not hidden
extensions.wrapfunction(repoview, 'pinnedrevs', _rebasedvisible)