( )⚙ D8925 revert: remove dangerous `parents` argument from `cmdutil.revert()`

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8925

revert: remove dangerous `parents` argument from `cmdutil.revert()`
ClosedPublic

Authored by martinvonz on Aug 11 2020, 1:23 AM.

Details

Reviewers
durin42
pulkit
Group Reviewers
hg-reviewers
Commits
rHG8c466bcb0879: revert: remove dangerous `parents` argument from `cmdutil.revert()`
Summary

As we found out the hard way (thanks to spectral@ for figuring it
out!), cmdutil.revert()'s parents argument must be
repo.dirstate.parents() or things may go wrong. We had an extension
that passed in the target commit as the first parent. The hg split
command from the evolve extension seems to have made the same mistake,
but I haven't looked carefully.

The problem is that cmdutil._performrevert() calls
dirstate.normal() on reverted files if the commit to revert to
equals the first parent. So if you pass in ctx=foo and
parents=(foo.node(), nullid), then dirstate.normal() will be
called for the revert files, even though they might not be clean in
the working copy.

There doesn't seem to be any reason, other than a tiny performance
benefit, to passing the parents around instead of looking them up
again in cmdutil._performrevert(), so that's what this patch does.

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

martinvonz created this revision.Aug 11 2020, 1:23 AM
pulkit accepted this revision.Aug 11 2020, 5:41 AM
This revision is now accepted and ready to land.Aug 11 2020, 5:41 AM

Revision Contents
Changeset List

Diff 22401

hgext/histedit.py

kwargs['extra'] = extra kwargs['extra'] = extra
return repo.commit(**kwargs) return repo.commit(**kwargs)
return commitfunc return commitfunc
def applychanges(ui, repo, ctx, opts): def applychanges(ui, repo, ctx, opts):
"""Merge changeset from ctx (only) in the current working directory""" """Merge changeset from ctx (only) in the current working directory"""
wcpar = repo.dirstate.p1() if ctx.p1().node() == repo.dirstate.p1():
if ctx.p1().node() == wcpar:
# edits are "in place" we do not need to make any merge, # edits are "in place" we do not need to make any merge,
# just applies changes on parent for editing # just applies changes on parent for editing
ui.pushbuffer() ui.pushbuffer()
cmdutil.revert(ui, repo, ctx, (wcpar, node.nullid), all=True) cmdutil.revert(ui, repo, ctx, all=True)
stats = mergemod.updateresult(0, 0, 0, 0) stats = mergemod.updateresult(0, 0, 0, 0)
ui.popbuffer() ui.popbuffer()
else: else:
try: try:
# ui.forcemerge is an internal variable, do not document # ui.forcemerge is an internal variable, do not document
repo.ui.setconfig( repo.ui.setconfig(
b'ui', b'forcemerge', opts.get(b'tool', b''), b'histedit' b'ui', b'forcemerge', opts.get(b'tool', b''), b'histedit'
) )

hgext/largefiles/overrides.py

# track of the largefiles that are being reverted so we only pull down # track of the largefiles that are being reverted so we only pull down
# the necessary largefiles. # the necessary largefiles.
# #
# Standins are only updated (to match the hash of largefiles) before # Standins are only updated (to match the hash of largefiles) before
# commits. Update the standins then run the original revert, changing # commits. Update the standins then run the original revert, changing
# the matcher to hit standins instead of largefiles. Based on the # the matcher to hit standins instead of largefiles. Based on the
# resulting standins update the largefiles. # resulting standins update the largefiles.
@eh.wrapfunction(cmdutil, b'revert') @eh.wrapfunction(cmdutil, b'revert')
def overriderevert(orig, ui, repo, ctx, parents, *pats, **opts): def overriderevert(orig, ui, repo, ctx, *pats, **opts):
# Because we put the standins in a bad state (by updating them) # Because we put the standins in a bad state (by updating them)
# and then return them to a correct state we need to lock to # and then return them to a correct state we need to lock to
# prevent others from changing them in their incorrect state. # prevent others from changing them in their incorrect state.
with repo.wlock(): with repo.wlock():
lfdirstate = lfutil.openlfdirstate(ui, repo) lfdirstate = lfutil.openlfdirstate(ui, repo)
s = lfutil.lfdirstatestatus(lfdirstate, repo) s = lfutil.lfdirstatestatus(lfdirstate, repo)
lfdirstate.write() lfdirstate.write()
for lfile in s.modified: for lfile in s.modified:
if lfile is not None: if lfile is not None:
return origmatchfn(lfile) and (f in ctx or f in mctx) return origmatchfn(lfile) and (f in ctx or f in mctx)
return origmatchfn(f) return origmatchfn(f)
m.matchfn = matchfn m.matchfn = matchfn
return m return m
with extensions.wrappedfunction(scmutil, b'match', overridematch): with extensions.wrappedfunction(scmutil, b'match', overridematch):
orig(ui, repo, ctx, parents, *pats, **opts) orig(ui, repo, ctx, *pats, **opts)
newstandins = lfutil.getstandinsstate(repo) newstandins = lfutil.getstandinsstate(repo)
filelist = lfutil.getlfilestoupdate(oldstandins, newstandins) filelist = lfutil.getlfilestoupdate(oldstandins, newstandins)
# lfdirstate should be 'normallookup'-ed for updated files, # lfdirstate should be 'normallookup'-ed for updated files,
# because reverting doesn't touch dirstate for 'normal' files # because reverting doesn't touch dirstate for 'normal' files
# when target revision is explicitly specified: in such case, # when target revision is explicitly specified: in such case,
# 'n' and valid timestamp in dirstate doesn't ensure 'clean' # 'n' and valid timestamp in dirstate doesn't ensure 'clean'
# of target (standin) file. # of target (standin) file.

hgext/mq.py

tobackup=tobackup, tobackup=tobackup,
keepchanges=keepchanges, keepchanges=keepchanges,
) )
except AbortNoCleanup: except AbortNoCleanup:
raise raise
except: # re-raises except: # re-raises
self.ui.warn(_(b'cleaning up working directory...\n')) self.ui.warn(_(b'cleaning up working directory...\n'))
cmdutil.revert( cmdutil.revert(
self.ui, self.ui, repo, repo[b'.'], no_backup=True,
repo,
repo[b'.'],
repo.dirstate.parents(),
no_backup=True,
) )
# only remove unknown files that we know we touched or # only remove unknown files that we know we touched or
# created while patching # created while patching
for f in all_files: for f in all_files:
if f not in repo.dirstate: if f not in repo.dirstate:
repo.wvfs.unlinkpath(f, ignoremissing=True) repo.wvfs.unlinkpath(f, ignoremissing=True)
self.ui.warn(_(b'done\n')) self.ui.warn(_(b'done\n'))
raise raise

mercurial/cmdutil.py

elif repo.ui.verbose: elif repo.ui.verbose:
repo.ui.write(_(b'committed changeset %d:%s\n') % (ctx.rev(), ctx)) repo.ui.write(_(b'committed changeset %d:%s\n') % (ctx.rev(), ctx))
def postcommitstatus(repo, pats, opts): def postcommitstatus(repo, pats, opts):
return repo.status(match=scmutil.match(repo[None], pats, opts)) return repo.status(match=scmutil.match(repo[None], pats, opts))
def revert(ui, repo, ctx, parents, *pats, **opts): def revert(ui, repo, ctx, *pats, **opts):
opts = pycompat.byteskwargs(opts) opts = pycompat.byteskwargs(opts)
parent, p2 = parents parent, p2 = repo.dirstate.parents()
node = ctx.node() node = ctx.node()
mf = ctx.manifest() mf = ctx.manifest()
if node == p2: if node == p2:
parent = p2 parent = p2
# need all matching names in dirstate and manifest of target rev, # need all matching names in dirstate and manifest of target rev,
# so have to walk both. do not print errors if files exist in one # so have to walk both. do not print errors if files exist in one
repo, [f for sublist in oplist for f in sublist] repo, [f for sublist in oplist for f in sublist]
) )
prefetch( prefetch(
repo, [(ctx.rev(), matchfiles)], repo, [(ctx.rev(), matchfiles)],
) )
match = scmutil.match(repo[None], pats) match = scmutil.match(repo[None], pats)
_performrevert( _performrevert(
repo, repo,
parents,
ctx, ctx,
names, names,
uipathfn, uipathfn,
actions, actions,
match, match,
interactive, interactive,
tobackup, tobackup,
) )
raise error.Abort( raise error.Abort(
b"subrepository '%s' does not exist in %s!" b"subrepository '%s' does not exist in %s!"
% (sub, short(ctx.node())) % (sub, short(ctx.node()))
) )
def _performrevert( def _performrevert(
repo, repo,
parents,
ctx, ctx,
names, names,
uipathfn, uipathfn,
actions, actions,
match, match,
interactive=False, interactive=False,
tobackup=None, tobackup=None,
): ):
"""function that actually perform all the actions computed for revert """function that actually perform all the actions computed for revert
This is an independent function to let extension to plug in and react to This is an independent function to let extension to plug in and react to
the imminent revert. the imminent revert.
Make sure you have the working directory locked when calling this function. Make sure you have the working directory locked when calling this function.
""" """
parent, p2 = parents parent, p2 = repo.dirstate.parents()
node = ctx.node() node = ctx.node()
excluded_files = [] excluded_files = []
def checkout(f): def checkout(f):
fc = ctx[f] fc = ctx[f]
repo.wwrite(f, fc.data(), fc.flags()) repo.wwrite(f, fc.data(), fc.flags())
def doremove(f): def doremove(f):

mercurial/commands.py

if stats.unresolvedcount: if stats.unresolvedcount:
repo.ui.status( repo.ui.status(
_(b"use 'hg resolve' to retry unresolved file merges\n") _(b"use 'hg resolve' to retry unresolved file merges\n")
) )
return 1 return 1
else: else:
hg.clean(repo, node, show_stats=False) hg.clean(repo, node, show_stats=False)
repo.dirstate.setbranch(branch) repo.dirstate.setbranch(branch)
cmdutil.revert(ui, repo, rctx, repo.dirstate.parents()) cmdutil.revert(ui, repo, rctx)
if opts.get(b'no_commit'): if opts.get(b'no_commit'):
msg = _(b"changeset %s backed out, don't forget to commit.\n") msg = _(b"changeset %s backed out, don't forget to commit.\n")
ui.status(msg % short(node)) ui.status(msg % short(node))
return 0 return 0
def commitfunc(ui, repo, message, match, opts): def commitfunc(ui, repo, message, match, opts):
editform = b'backout' editform = b'backout'
% ctx.rev() % ctx.rev()
) )
elif dirty: elif dirty:
hint = _(b"uncommitted changes, use --all to discard all changes") hint = _(b"uncommitted changes, use --all to discard all changes")
else: else:
hint = _(b"use --all to revert all files") hint = _(b"use --all to revert all files")
raise error.Abort(msg, hint=hint) raise error.Abort(msg, hint=hint)
return cmdutil.revert( return cmdutil.revert(ui, repo, ctx, *pats, **pycompat.strkwargs(opts))
ui, repo, ctx, (parent, p2), *pats, **pycompat.strkwargs(opts)
)
@command( @command(
b'rollback', b'rollback',
dryrunopts + [(b'f', b'force', False, _(b'ignore safety measures'))], dryrunopts + [(b'f', b'force', False, _(b'ignore safety measures'))],
helpcategory=command.CATEGORY_MAINTENANCE, helpcategory=command.CATEGORY_MAINTENANCE,
) )
def rollback(ui, repo, **opts): def rollback(ui, repo, **opts):

mercurial/shelve.py

def mergefiles(ui, repo, wctx, shelvectx): def mergefiles(ui, repo, wctx, shelvectx):
"""updates to wctx and merges the changes from shelvectx into the """updates to wctx and merges the changes from shelvectx into the
dirstate.""" dirstate."""
with ui.configoverride({(b'ui', b'quiet'): True}): with ui.configoverride({(b'ui', b'quiet'): True}):
hg.update(repo, wctx.node()) hg.update(repo, wctx.node())
ui.pushbuffer(True) ui.pushbuffer(True)
cmdutil.revert(ui, repo, shelvectx, repo.dirstate.parents()) cmdutil.revert(ui, repo, shelvectx)
ui.popbuffer() ui.popbuffer()
def restorebranch(ui, repo, branchtorestore): def restorebranch(ui, repo, branchtorestore):
if branchtorestore and branchtorestore != repo.dirstate.branch(): if branchtorestore and branchtorestore != repo.dirstate.branch():
repo.dirstate.setbranch(branchtorestore) repo.dirstate.setbranch(branchtorestore)
ui.status( ui.status(
_(b'marked working directory as branch %s\n') % branchtorestore _(b'marked working directory as branch %s\n') % branchtorestore

mercurial/subrepo.py

self.filerevert(*pats, **opts) self.filerevert(*pats, **opts)
# Update the repo to the revision specified in the given substate # Update the repo to the revision specified in the given substate
if not opts.get('dry_run'): if not opts.get('dry_run'):
self.get(substate, overwrite=True) self.get(substate, overwrite=True)
def filerevert(self, *pats, **opts): def filerevert(self, *pats, **opts):
ctx = self._repo[opts['rev']] ctx = self._repo[opts['rev']]
parents = self._repo.dirstate.parents()
if opts.get('all'): if opts.get('all'):
pats = [b'set:modified()'] pats = [b'set:modified()']
else: else:
pats = [] pats = []
cmdutil.revert(self.ui, self._repo, ctx, parents, *pats, **opts) cmdutil.revert(self.ui, self._repo, ctx, *pats, **opts)
def shortid(self, revid): def shortid(self, revid):
return revid[:12] return revid[:12]
@annotatesubrepoerror @annotatesubrepoerror
def unshare(self): def unshare(self):
# subrepo inherently violates our import layering rules # subrepo inherently violates our import layering rules
# because it wants to make repo objects from deep inside the stack # because it wants to make repo objects from deep inside the stack