This is an archive of the discontinued Mercurial Phabricator instance.

Differential D53

undo: adds -f flag and gap edge case handling
ClosedPublic

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

Details

Reviewers
quark
Group Reviewers
Restricted Project
Commits
rFBHGXfae05e8c1972: undo: adds -f flag and gap edge case handling
Summary

Prevents users from accidentally crossing gaps in their history and adds an
overwride. In most cases undoing across a gap isn't desired as the connection
between the pre and post states is unkown. While undoing across a gap is not
destructive or inherently risky, it is inherently confusing to the user since
we don't know how many and what commands where run. Furthermore, there is no
guarantee that the pre and post states of a gap are actually connected in any
sane way.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

felixmerk created this revision.Jul 11 2017, 5:56 PM
felixmerk added a reviewer: Restricted Project.Jul 11 2017, 6:04 PM
stash added a subscriber: stash.Jul 12 2017, 9:37 AM
stash added inline comments.
hgext3rd/undo.py
85

This function is introduced in parent diff is not used there. Can you move this function in the parent diff?

felixmerk updated this revision to Diff 107.Jul 13 2017, 12:41 AM
felixmerk added inline comments.Jul 13 2017, 12:46 AM
hgext3rd/undo.py
85

Good catch. I'll actually move the call to the parent diff since thats where it belongs.

felixmerk updated this revision to Diff 212.Jul 17 2017, 3:09 PM
felixmerk updated this revision to Diff 316.Jul 19 2017, 5:31 PM
quark accepted this revision.Jul 19 2017, 6:19 PM
quark added a subscriber: quark.

The feature looks good. Some end-user facing text could be improved. But we can revisit them later.

hgext3rd/undo.py
365

What does "in memory" mean here?

402

The text is shown to end-users (hg undo -h). I think implementation details like "a blank command" might be unfriendly to them.

I'd just focus on what end-users care about and keep it short, like:

If the undo extension was turned off then turned on, you might lose the ability to undo to certain states.
This revision is now accepted and ready to land.Jul 19 2017, 6:19 PM
felixmerk updated this revision to Diff 324.Jul 19 2017, 8:45 PM
Closed by commit rFBHGXfae05e8c1972: undo: adds -f flag and gap edge case handling (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 (44 lines)
Mtests/test-undo.t (15 lines)

Diff 324

hgext3rd/undo.py

# This wrapper executes whenever a command is run. # This wrapper executes whenever a command is run.
# Some commands (eg hg sl) don't actually modify anything # Some commands (eg hg sl) don't actually modify anything
# ie can't be undone, but the command doesn't know this. # ie can't be undone, but the command doesn't know this.
command = fullargs command = fullargs
# Check wether undolog is consistent # Check wether undolog is consistent
# ie check wether the undo ext was # ie check wether the undo ext was
# off before this command # off before this command
safelog(repo, [""]) changes = safelog(repo, [""])
if changes:
_recordnewgap(repo)
result = orig(lui, repo, cmd, fullargs, *args) result = orig(lui, repo, cmd, fullargs, *args)
# record changes to repo # record changes to repo
safelog(repo, command) safelog(repo, command)
return result return result
# Write: Log control # Write: Log control
def safelog(repo, command): def safelog(repo, command):
'''boilerplate for log command '''boilerplate for log command
input: input:
repo: mercurial.localrepo repo: mercurial.localrepo
command: list of strings, first is string of command run command: list of strings, first is string of command run
output: bool output: bool
True if changes have been recorded, False otherwise True if changes have been recorded, False otherwise
''' '''
changes = False changes = False
if repo is not None:# some hg commands don't require repo if repo is not None:# some hg commands don't require repo
# undolog specific lock # undolog specific lock
# allows running command during other commands when # allows running command during other commands when
# otherwise legal. Could cause weird undolog states, # otherwise legal. Could cause weird undolog states,
# which gap handling generally covers. # which gap handling generally covers.
repo.vfs.makedirs('undolog') repo.vfs.makedirs('undolog')
stashUnsubmitted
Not Done

This function is introduced in parent diff is not used there. Can you move this function in the parent diff?

stash: This function is introduced in parent diff is not used there. Can you move this function in the…
felixmerkAuthorUnsubmitted
Not Done

Good catch. I'll actually move the call to the parent diff since thats where it belongs.

felixmerk: Good catch. I'll actually move the call to the parent diff since thats where it belongs.
with lockmod.lock(repo.vfs, "undolog/lock", desc="undolog"): with lockmod.lock(repo.vfs, "undolog/lock", desc="undolog"):
# developer config: undo._duringundologlock # developer config: undo._duringundologlock
if repo.ui.configbool('undo', '_duringundologlock'): if repo.ui.configbool('undo', '_duringundologlock'):
repo.hook("duringundologlock") repo.hook("duringundologlock")
tr = lighttransaction(repo) tr = lighttransaction(repo)
with tr: with tr:
changes = log(repo.filtered('visible'), command, tr) changes = log(repo.filtered('visible'), command, tr)
if changes and not ("undo" == command[0] or "redo" == if changes and not ("undo" == command[0] or "redo" ==
rlog = _getrevlog(repo, 'index.i') rlog = _getrevlog(repo, 'index.i')
hexnode = hex(rlog.node(_invertindex(rlog, reverseindex))) hexnode = hex(rlog.node(_invertindex(rlog, reverseindex)))
return repo.svfs.write("undolog/redonode", str(hexnode)) return repo.svfs.write("undolog/redonode", str(hexnode))
def _delundoredo(repo): def _delundoredo(repo):
path = 'undolog' + '/' + 'redonode' path = 'undolog' + '/' + 'redonode'
repo.svfs.tryunlink(path) repo.svfs.tryunlink(path)
def _recordnewgap(repo, absoluteindex=None):
path = os.path.join('undolog', 'gap')
if absoluteindex is None:
rlog = _getrevlog(repo, 'index.i')
repo.svfs.write(path, str(len(rlog) - 1))
else:
repo.svfs.write(path, str(absoluteindex))
# Read # Read
def _readindex(repo, reverseindex, prefetchedrevlog=None): def _readindex(repo, reverseindex, prefetchedrevlog=None):
if prefetchedrevlog is None: if prefetchedrevlog is None:
rlog = _getrevlog(repo, 'index.i') rlog = _getrevlog(repo, 'index.i')
else: else:
rlog = prefetchedrevlog rlog = prefetchedrevlog
index = _invertindex(rlog, reverseindex) index = _invertindex(rlog, reverseindex)
if index < 0 or index > len(rlog) - 1: if index < 0 or index > len(rlog) - 1:
raise IndexError raise IndexError
chunk = rlog.revision(index) chunk = rlog.revision(index)
indexdict = {} indexdict = {}
for row in chunk.split("\n"): for row in chunk.split("\n"):
kvpair = row.split(' ', 1) kvpair = row.split(' ', 1)
if kvpair[0]: if kvpair[0]:
indexdict[kvpair[0]] = kvpair[1] indexdict[kvpair[0]] = kvpair[1]
return indexdict return indexdict
def _readnode(repo, filename, hexnode): def _readnode(repo, filename, hexnode):
rlog = _getrevlog(repo, filename) rlog = _getrevlog(repo, filename)
return rlog.revision(bin(hexnode)) return rlog.revision(bin(hexnode))
def _gapcheck(repo, reverseindex):
rlog = _getrevlog(repo, 'index.i')
absoluteindex = _invertindex(rlog, reverseindex)
path = os.path.join('undolog', "gap")
try:
result = absoluteindex >= int(repo.svfs.read(path))
except IOError:
# recreate file
repo.ui.debug("failed to read gap file in %s, attempting recreation\n"
% path)
rlog = _getrevlog(repo, 'index.i')
i = 0
while i < (len(rlog)):
indexdict = _readindex(repo, i, rlog)
if "" == _readnode(repo, "command.i", indexdict["command"]):
break
i += 1
# defaults to before oldest command
_recordnewgap(repo, _invertindex(rlog, i))
result = absoluteindex >= _invertindex(rlog, i)
finally:
return result
# Visualize # Visualize
"""debug commands and instrumentation for the undo extension """debug commands and instrumentation for the undo extension
Adds the `debugundohistory` and `debugundosmartlog` commands to visualize Adds the `debugundohistory` and `debugundosmartlog` commands to visualize
operational history and to give a preview of how undo will behave. operational history and to give a preview of how undo will behave.
""" """
_('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: # Undo:
@command('undo', [ @command('undo', [
('n', 'index', 1, _("how many steps to undo back")), ('n', 'index', 1, _("how many steps to undo back")),
('f', 'force', False, _("undo across missing undo history (ADVANCED)")),
quarkUnsubmitted
Not Done

What does "in memory" mean here?

quark: What does "in memory" mean here?
('a', 'absolute', False, _("absolute based on command index instead of " ('a', 'absolute', False, _("absolute based on command index instead of "
"relative undo")), "relative undo")),
]) ])
def undo(ui, repo, *args, **opts): def undo(ui, repo, *args, **opts):
"""perform an undo """perform an undo
Undoes an undoable command. An undoable command is one that changed at Undoes an undoable command. An undoable command is one that changed at
least one of the following three: bookmarks, working copy parent or least one of the following three: bookmarks, working copy parent or
states. Note that as soon as you execute a different undoable command, states. Note that as soon as you execute a different undoable command,
which isn't hg undo or hg redo, any new undos or redos will be relative to which isn't hg undo or hg redo, any new undos or redos will be relative to
the state after this command. When using --index with relative undos, the state after this command. When using --index with relative undos,
this is equivalent to running index many undos, except for leaving your this is equivalent to running index many undos, except for leaving your
repo state history (hg debugundohistory) less cluttered. repo state history (hg debugundohistory) less cluttered.
Undo states are also distinct repo states and can thereby be inspected using Undo states are also distinct repo states and can thereby be inspected using
debugundohistory and specifically jumped to using undo --index --absolute. debugundohistory and specifically jumped to using undo --index --absolute.
If the undo extension was turned off and on again, you might loose the
ability to undo to certain repo states. Undoing to repo states before the
quarkUnsubmitted
Not Done

The text is shown to end-users (hg undo -h). I think implementation details like "a blank command" might be unfriendly to them.

I'd just focus on what end-users care about and keep it short, like:

If the undo extension was turned off then turned on, you might lose the ability to undo to certain states.
quark: The text is shown to end-users (`hg undo -h`). I think implementation details like "a blank…
missing ones can be forced, but isn't advised unless its known how the
before and after states are connected.
""" """
reverseindex = opts.get("index") reverseindex = opts.get("index")
relativeundo = not opts.get("absolute") relativeundo = not opts.get("absolute")
with repo.wlock(), repo.lock(), repo.transaction("undo"): with repo.wlock(), repo.lock(), repo.transaction("undo"):
cmdutil.checkunfinished(repo) cmdutil.checkunfinished(repo)
cmdutil.bailifchanged(repo) cmdutil.bailifchanged(repo)
repo = repo.unfiltered() repo = repo.unfiltered()
if relativeundo: if relativeundo:
reverseindex = _computerelative(repo, reverseindex) reverseindex = _computerelative(repo, reverseindex)
if not (opts.get("force") or _gapcheck(repo, reverseindex)):
raise error.Abort(_("attempted risky undo across"
" missing history"))
_undoto(ui, repo, reverseindex) _undoto(ui, repo, reverseindex)
# store undo data # store undo data
# for absolute undos, think of this as a reset # for absolute undos, think of this as a reset
# for relative undos, think of this as an update # for relative undos, think of this as an update
_logundoredoindex(repo, repo.currenttransaction(), reverseindex) _logundoredoindex(repo, repo.currenttransaction(), reverseindex)
@command('redo', [ @command('redo', [
('n', 'index', 1, _("how many commands to redo")), ('n', 'index', 1, _("how many commands to redo")),

tests/test-undo.t

| cmiss | cmiss
~ ~
$ hg undo -a $ hg undo -a
0 files updated, 0 files merged, 0 files removed, 0 files unresolved 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg log -G -T compact -l1 $ hg log -G -T compact -l1
@ 9[tip][master] 1dafc0b43612 1970-01-01 00:00 +0000 test @ 9[tip][master] 1dafc0b43612 1970-01-01 00:00 +0000 test
| cmiss | cmiss
~ ~
hg undo --force tests
$ hg debugundohistory -l 18
18: undo
19: ci -ma5
20: -- gap in log --
21: commit -m words
22: update master
$ hg undo -a -n 25
abort: attempted risky undo across missing history
[255]
$ hg undo -a -n 25 -f
1 files updated, 0 files merged, 1 files removed, 0 files unresolved
$ hg undo -a
2 files updated, 0 files merged, 0 files removed, 0 files unresolved