( )⚙ D51 undo: bare bones undo without tests

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D51

undo: bare bones undo without tests
ClosedPublic

Authored by felixmerk on Jul 11 2017, 5:56 PM.
Tags
None
Subscribers
stash, quark

Details

Reviewers
quark
Group Reviewers
Restricted Project
Commits
rFBHGX11039bbf64f1: undo: bare bones undo without tests
Summary

Adds undo command that changes working copy parent, bookmarks and
visible draft commits to a previous repo state.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

quark added a reviewer: Restricted Project.Jul 11 2017, 10:03 PM
quark requested changes to this revision.
quark added a subscriber: quark.
quark added inline comments.
hgext3rd/undo.py
35–38

This is an easy mistake :-p, but:

Importing inhibit will expose its functions. But whether inhibit is enabled or not, whether its uisetup runs or not is not guaranteed. If it's disabled (so its uisetup doesn't run), its API won't work.

The correct way to do is to get the inhibit module from mercurial.extensions.find:

from mercurial import (
    extensions,
)


try:
    inhibit = extensions.find('inhibit')
except KeyError:
    raise error.Abort(_('undo requires inhibit to work properly'))
else:
    inhibit.revive ...
321

nit: remove the space before p.

334

repo is guaranteed not None by Mercurial command handling so this if can be removed.

341

Insert a blank line before this function.

353–360

While commands.bookmark works, it's a high level API for running commands. In extension code, it's better to avoid calling other commands. repo._bookmarks has __setitem__ and __delitem__ (see bookmarks.py) which fit better in this case. Also search for recordchange for how to make transaction aware of the bookmark change.

366

Similar to bookmarks, it's better to use lower level APIs since this is an hg extension. Things might be interesting are:

  • cmdutil.bailifchanged # abort if working copy is not clean
  • merge.update / hg.clean / hg.update / hg.updatetotally # low-level update functions
369–370

Nice use of existing revsets!

374

repo.changectx(r) could be just repo[r]. The latter is more common.

381–387

It is a bit strange that hidecommits only takes a single commit. And ui is not used in revealcommits.

They could be made consistent:

def hidecommits(repo, ctxs):
    ....

def revealcommits(repo, ctxs):
    ....

hidecommits(repo, repo.set(...))
revealcommits(repo, repo.set(...))
This revision now requires changes to proceed.Jul 11 2017, 10:03 PM
stash added a subscriber: stash.Jul 12 2017, 8:46 AM
stash added inline comments.
hgext3rd/undo.py
323

Nit 'at least'

felixmerk edited edge metadata.Jul 13 2017, 12:41 AM
felixmerk updated this revision to Diff 105.
felixmerk updated this revision to Diff 210.Jul 17 2017, 3:09 PM
quark requested changes to this revision.Jul 18 2017, 8:47 PM

Almost there.

hgext3rd/undo.py
353–357

nit: could be simplified repo._bookmarks.clear().

359

nit: could be simplified to if mark:

389–391

nit: need re-indent (press == in vim)

393–394

Since the revision to revive might be hidden, I think you might want unfiltered repo here.

nit: could be slightly simplified to ctxs = repo.unfiltered().set(rev) (ctxs is more common in hg codebase than contexts). scmutil.revrange is to resolve multiple revsets while there is just one single revset here.

This revision now requires changes to proceed.Jul 18 2017, 8:47 PM
felixmerk added inline comments.Jul 19 2017, 5:19 PM
hgext3rd/undo.py
393–394

The repo is unfiltered when this is called

felixmerk edited edge metadata.Jul 19 2017, 5:31 PM
felixmerk updated this revision to Diff 314.
quark accepted this revision.Jul 19 2017, 5:40 PM

I see that's line 340. Might worth a comment on _undoto saying it expects an unfiltered repo.

This revision is now accepted and ready to land.Jul 19 2017, 5:40 PM
felixmerk updated this revision to Diff 322.Jul 19 2017, 8:45 PM
Closed by commit rFBHGX11039bbf64f1: undo: bare bones undo without tests (authored by felixmerk). · Explain WhyJul 19 2017, 10:23 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext3rd/undo.py (85 lines)
Mtests/test-undo.t (1 line)

Diff 331

hgext3rd/undo.py

# undo.py: records data in revlog for future undo functionality # undo.py: records data in revlog for future undo functionality
# #
# Copyright 2017 Facebook, Inc. # Copyright 2017 Facebook, Inc.
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
from mercurial.i18n import _ from mercurial.i18n import _
from mercurial import ( from mercurial import (
cmdutil,
dispatch, dispatch,
error, error,
extensions, extensions,
localrepo, localrepo,
lock as lockmod, lock as lockmod,
obsolete,
registrar, registrar,
revlog, revlog,
revset, revset,
revsetlang, revsetlang,
smartset, smartset,
transaction, transaction,
util, util,
) )
from mercurial.node import ( from mercurial.node import (
bin, bin,
hex, hex,
nullid, nullid,
) )
cmdtable = {} cmdtable = {}
command = registrar.command(cmdtable) command = registrar.command(cmdtable)
# Setup # Setup
quarkUnsubmitted
Not Done

This is an easy mistake :-p, but:

Importing inhibit will expose its functions. But whether inhibit is enabled or not, whether its uisetup runs or not is not guaranteed. If it's disabled (so its uisetup doesn't run), its API won't work.

The correct way to do is to get the inhibit module from mercurial.extensions.find:

from mercurial import (
    extensions,
)


try:
    inhibit = extensions.find('inhibit')
except KeyError:
    raise error.Abort(_('undo requires inhibit to work properly'))
else:
    inhibit.revive ...
quark: This is an easy mistake :-p, but: Importing `inhibit` will expose its functions. But whether…
def extsetup(ui): def extsetup(ui):
extensions.wrapfunction(dispatch, 'runcommand', _runcommandwrapper) extensions.wrapfunction(dispatch, 'runcommand', _runcommandwrapper)
# undo has its own locking, whitelist itself to bypass repo lock audit # undo has its own locking, whitelist itself to bypass repo lock audit
localrepo.localrepository._wlockfreeprefix.add('undolog/') localrepo.localrepository._wlockfreeprefix.add('undolog/')
# Wrappers # Wrappers
Note: this revset may include hidden commits Note: this revset may include hidden commits
""" """
args = revset.getargsdict(x, 'olddraftrevset', 'reverseindex') args = revset.getargsdict(x, 'olddraftrevset', 'reverseindex')
reverseindex = revsetlang.getinteger(args.get('reverseindex'), reverseindex = revsetlang.getinteger(args.get('reverseindex'),
_('index must be a positive integer'), 1) _('index must be a positive integer'), 1)
revs = _getolddrafts(repo, reverseindex) revs = _getolddrafts(repo, reverseindex)
return smartset.baseset(revs) return smartset.baseset(revs)
# Undo:
@command('undo', [
('n', 'index', 1, _("how many steps to undo back")),
])
def undo(ui, repo, *args, **opts):
"""perform an undo
quarkUnsubmitted
Not Done

nit: remove the space before p.

quark: nit: remove the space before `p`.
Undoes an undoable command. An undoable command is one that changed at
stashUnsubmitted
Not Done

Nit 'at least'

stash: Nit 'at least'
least one of the following three: bookmarks, working copy parent or
changesets. Note that this specifically does not include commands like log.
It will include update if update changes the working copy parent (you update
to a changeset that isn't the current one). Note that commands that edit
public repos can't be undone (specifically push).
Undo does not preserve the working copy changes.
"""
reverseindex = opts.get("index")
quarkUnsubmitted
Not Done

repo is guaranteed not None by Mercurial command handling so this if can be removed.

quark: `repo` is guaranteed not None by Mercurial command handling so this `if` can be removed.
with repo.wlock(), repo.lock(), repo.transaction("undo"):
cmdutil.checkunfinished(repo)
cmdutil.bailifchanged(repo)
repo = repo.unfiltered()
_undoto(ui, repo, reverseindex)
def _undoto(ui, repo, reverseindex):
quarkUnsubmitted
Not Done

Insert a blank line before this function.

quark: Insert a blank line before this function.
# undo to specific reverseindex
# requires inhibit extension
if repo != repo.unfilterd:
raise error.ProgrammingError(_("_undoto expects unfilterd repo"))
try:
nodedict = _readindex(repo, reverseindex)
except IndexError:
raise error.Abort(_("index out of bounds"))
# bookmarks
bookstring = _readnode(repo, "bookmarks.i", nodedict["bookmarks"])
booklist = bookstring.split("\n")
# copy implementation for bookmarks
itercopy = []
for mark in repo._bookmarks.iteritems():
itercopy.append(mark)
quarkUnsubmitted
Not Done

nit: could be simplified repo._bookmarks.clear().

quark: nit: could be simplified `repo._bookmarks.clear()`.
bmchanges = [(mark[0], None) for mark in itercopy]
repo._bookmarks.applychanges(repo, repo.currenttransaction(), bmchanges)
quarkUnsubmitted
Not Done

nit: could be simplified to if mark:

quark: nit: could be simplified to `if mark:`
bmchanges = []
quarkUnsubmitted
Not Done

While commands.bookmark works, it's a high level API for running commands. In extension code, it's better to avoid calling other commands. repo._bookmarks has __setitem__ and __delitem__ (see bookmarks.py) which fit better in this case. Also search for recordchange for how to make transaction aware of the bookmark change.

quark: While `commands.bookmark` works, it's a high level API for running commands. In extension code…
for mark in booklist:
if mark:
kv = mark.rsplit(" ", 1)
bmchanges.append((kv[0], bin(kv[1])))
repo._bookmarks.applychanges(repo, repo.currenttransaction(), bmchanges)
quarkUnsubmitted
Not Done

Similar to bookmarks, it's better to use lower level APIs since this is an hg extension. Things might be interesting are:

  • cmdutil.bailifchanged # abort if working copy is not clean
  • merge.update / hg.clean / hg.update / hg.updatetotally # low-level update functions
quark: Similar to bookmarks, it's better to use lower level APIs since this is an hg extension. Things…
# working copy parent
workingcopyparent = _readnode(repo, "workingparent.i",
nodedict["workingparent"])
revealcommits(repo, workingcopyparent)
quarkUnsubmitted
Not Done

Nice use of existing revsets!

quark: Nice use of existing revsets!
commands.update(ui, repo, rev=workingcopyparent)
# visible changesets
addedrevs = revsetlang.formatspec('olddraft(0) - olddraft(%d)',
quarkUnsubmitted
Not Done

repo.changectx(r) could be just repo[r]. The latter is more common.

quark: `repo.changectx(r)` could be just `repo[r]`. The latter is more common.
reverseindex)
hidecommits(repo, addedrevs)
removedrevs = revsetlang.formatspec('olddraft(%d) - olddraft(0)',
reverseindex)
revealcommits(repo, removedrevs)
# hide and reveal commits
def hidecommits(repo, rev):
ctxts = repo.set(rev)
for commit in ctxts:
obsolete.createmarkers(repo, [[commit,[]]])
quarkUnsubmitted
Not Done

It is a bit strange that hidecommits only takes a single commit. And ui is not used in revealcommits.

They could be made consistent:

def hidecommits(repo, ctxs):
    ....

def revealcommits(repo, ctxs):
    ....

hidecommits(repo, repo.set(...))
revealcommits(repo, repo.set(...))
quark: It is a bit strange that `hidecommits` only takes a single commit. And `ui` is not used in…
def revealcommits(repo, rev):
try:
inhibit = extensions.find('inhibit')
quarkUnsubmitted
Not Done

nit: need re-indent (press == in vim)

quark: nit: need re-indent (press `==` in vim)
except KeyError:
raise error.Abort(_('undo requires inhibit to work properly'))
else:
quarkUnsubmitted
Not Done

Since the revision to revive might be hidden, I think you might want unfiltered repo here.

nit: could be slightly simplified to ctxs = repo.unfiltered().set(rev) (ctxs is more common in hg codebase than contexts). scmutil.revrange is to resolve multiple revsets while there is just one single revset here.

quark: Since the revision to revive might be hidden, I think you might want unfiltered repo here. nit…
felixmerkAuthorUnsubmitted
Not Done

The repo is unfiltered when this is called

felixmerk: The repo is unfiltered when this is called
ctxts = repo.set(rev)
inhibit.revive(ctxts)
# Tools # Tools
def _invertindex(rlog, indexorreverseindex): def _invertindex(rlog, indexorreverseindex):
return len(rlog) - 1 - indexorreverseindex return len(rlog) - 1 - indexorreverseindex
def _getrevlog(repo, filename): def _getrevlog(repo, filename):
path = 'undolog/' + filename path = 'undolog/' + filename
return revlog.revlog(repo.vfs, path) return revlog.revlog(repo.vfs, path)

tests/test-undo.t

$ hg debugdata .hg/undolog/index.i 1 $ hg debugdata .hg/undolog/index.i 1
bookmarks 8153d44860d076e9c328951c8f36cf8daebe695a bookmarks 8153d44860d076e9c328951c8f36cf8daebe695a
command 440cdcef588f9a594c5530a5b6dede39a96d930d command 440cdcef588f9a594c5530a5b6dede39a96d930d
date * (glob) date * (glob)
draftheads b80de5d138758541c5f05265ad144ab9fa86d1db draftheads b80de5d138758541c5f05265ad144ab9fa86d1db
workingparent fcb754f6a51eaf982f66d0637b39f3d2e6b520d5 (no-eol) workingparent fcb754f6a51eaf982f66d0637b39f3d2e6b520d5 (no-eol)
$ touch a3 && hg add a3 $ touch a3 && hg add a3
$ hg commit --amend $ hg commit --amend
saved backup bundle to $TESTTMP/repo/.hg/strip-backup/db92053d5c83-e25f6bc1-amend.hg (glob)
$ hg debugdata .hg/undolog/command.i 11 $ hg debugdata .hg/undolog/command.i 11
commit\x00--amend (no-eol) (esc) commit\x00--amend (no-eol) (esc)
Test debugundohistory Test debugundohistory
$ hg debugundohistory -l $ hg debugundohistory -l
0: commit --amend 0: commit --amend
1: ci -md 1: ci -md
2: book feature2 2: book feature2