This is an archive of the discontinued Mercurial Phabricator instance.

Differential D160

undo: basic single repo undo
ClosedPublic

Authored by felixmerk on Jul 20 2017, 6:20 PM.
Tags
None
Subscribers
stash

Details

Reviewers
stash
Group Reviewers
Restricted Project
Commits
rFBHGX0ad0bc61a332: undo: basic single repo undo
Summary

Add the ability to undo changes within one 'localbranch'. In the future, this
will probably become default behavior. Undo -b changecontext will undo changes
belonging to the local branch identified by the changecontext (see revset
localbranch). Based on obs markers we can identify connected commits that we
in turn reveal or hide outside of this localbranch. This undo allows users to
work on multiple branches independantly.

Currently, indexing for undoes is still on the repo level. This isn't good
from a ux experience. Next up would be localbranch level indexing logic and
redo logic.

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

felixmerk created this revision.Jul 20 2017, 6:20 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 20 2017, 6:20 PM
felixmerk updated this revision to Diff 385.Jul 21 2017, 7:08 PM
stash accepted this revision.Jul 25 2017, 1:28 PM
stash added a subscriber: stash.

This LGTM, I'll let you decide what to do with bookmark movements.
Also consider renaming variables to make it clear whethere it's a binnode or hash node

hgext3rd/undo.py
526–533

I'm not sure if your code handles this case, but bookmark can have '\n' inside.

537–551

Do I understand correctly that if bookmark was moved from some commit outside of localbranch back to commit in localbranch then this bookmark will just be deleted? Is that what we want? I think it's better to return this bookmark to the previous location

557–560

It's unclear for me when do you need to use bin(...) and when you don't. I usually try to use names like hexnode or binnode to make it clear. No need to change it in this diff, but would be cool to do it in the separate diff

This revision is now accepted and ready to land.Jul 25 2017, 1:28 PM
felixmerk added inline comments.Jul 25 2017, 2:29 PM
hgext3rd/undo.py
526–533

I'm under the impression that bookmarks can not have '\n' inside. I ran the following using python to illustrate this:

>>> subprocess.check_call(['hg', 'bookmark', 'abc\ndef'])
abort: '\n' cannot be used in a name

537–551

That does make more sense behavior wise. I'll make the change.

557–560

Good call

stash added inline comments.Jul 25 2017, 5:32 PM
hgext3rd/undo.py
526–533

Oh, ok, for some reason I thought it's possible. Thanks for checking!

felixmerk updated this revision to Diff 404.Jul 25 2017, 6:17 PM
felixmerk updated this revision to Diff 405.Jul 25 2017, 6:30 PM
felixmerk updated this revision to Diff 418.Jul 26 2017, 6:39 PM
felixmerk updated this revision to Diff 421.Jul 26 2017, 8:27 PM
felixmerk updated this revision to Diff 463.Jul 31 2017, 6:04 PM
felixmerk updated this revision to Diff 476.Aug 1 2017, 2:20 PM
felixmerk updated this revision to Diff 508.Aug 2 2017, 4:31 PM
Closed by commit rFBHGX0ad0bc61a332: undo: basic single repo undo (authored by felixmerk). · Explain WhyAug 4 2017, 9:19 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext3rd/undo.py (61 lines)
Mtests/test-undo.t (92 lines)

Diff 539

hgext3rd/undo.py

querystring = revsetlang.formatspec('((::%ld) & draft())::', revs) querystring = revsetlang.formatspec('((::%ld) & draft())::', revs)
return subset & smartset.baseset(repo.revs(querystring)) return subset & smartset.baseset(repo.revs(querystring))
# Undo: # Undo:
@command('undo', [ @command('undo', [
('a', 'absolute', False, _("absolute based on command index instead of " ('a', 'absolute', False, _("absolute based on command index instead of "
"relative undo")), "relative undo")),
('b', 'branch', "", _("local branch undo, accepts commit hash "
"(ADVANCED)")),
('f', 'force', False, _("undo across missing undo history (ADVANCED)")), ('f', 'force', False, _("undo across missing undo history (ADVANCED)")),
('k', 'keep', False, _("keep working copy changes")), ('k', 'keep', False, _("keep working copy changes")),
('n', 'index', 1, _("how many steps to undo back")), ('n', 'index', 1, _("how many steps to undo back")),
]) ])
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
so you can't simply redo without cleaning your working copy. Secondly, so you can't simply redo without cleaning your working copy. Secondly,
the operation may be slow if your working copy is large. If unsure, the operation may be slow if your working copy is large. If unsure,
its generally easier try undo without --keep first and redo if you want its generally easier try undo without --keep first and redo if you want
to change this. to change this.
""" """
reverseindex = opts.get("index") reverseindex = opts.get("index")
relativeundo = not opts.get("absolute") relativeundo = not opts.get("absolute")
keep = opts.get("keep") keep = opts.get("keep")
branch = opts.get("branch")
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)): if not (opts.get("force") or _gapcheck(repo, reverseindex)):
raise error.Abort(_("attempted risky undo across" raise error.Abort(_("attempted risky undo across"
" missing history")) " missing history"))
_undoto(ui, repo, reverseindex, keep=keep) _undoto(ui, repo, reverseindex, keep=keep, branch=branch)
# 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")),
]) ])
with repo.wlock(), repo.lock(), repo.transaction("redo"): with repo.wlock(), repo.lock(), repo.transaction("redo"):
cmdutil.checkunfinished(repo) cmdutil.checkunfinished(repo)
cmdutil.bailifchanged(repo) cmdutil.bailifchanged(repo)
repo = repo.unfiltered() repo = repo.unfiltered()
_undoto(ui, repo, reverseindex) _undoto(ui, repo, reverseindex)
_logundoredoindex(repo, repo.currenttransaction(), reverseindex) _logundoredoindex(repo, repo.currenttransaction(), reverseindex)
def _undoto(ui, repo, reverseindex, keep=False): def _undoto(ui, repo, reverseindex, keep=False, branch=None):
# undo to specific reverseindex # undo to specific reverseindex
# requires inhibit extension # requires inhibit extension
# branch is a changectx hash (potentially short form)
# which identifies its branch via localbranch revset
if repo != repo.unfiltered(): if repo != repo.unfiltered():
raise error.ProgrammingError(_("_undoto expects unfilterd repo")) raise error.ProgrammingError(_("_undoto expects unfilterd repo"))
try: try:
nodedict = _readindex(repo, reverseindex) nodedict = _readindex(repo, reverseindex)
except IndexError: except IndexError:
raise error.Abort(_("index out of bounds")) raise error.Abort(_("index out of bounds"))
# bookmarks # bookmarks
bookstring = _readnode(repo, "bookmarks.i", nodedict["bookmarks"]) bookstring = _readnode(repo, "bookmarks.i", nodedict["bookmarks"])
booklist = bookstring.split("\n") booklist = bookstring.split("\n")
if branch:
revs = repo.revs(revsetlang.formatspec('_localbranch(%s)', branch))
tonode = repo.changelog.node
branchcommits = [tonode(x) for x in revs]
else:
branchcommits = False
stashUnsubmitted
Not Done

I'm not sure if your code handles this case, but bookmark can have '\n' inside.

stash: I'm not sure if your code handles this case, but bookmark can have '\n' inside.
felixmerkAuthorUnsubmitted
Not Done

I'm under the impression that bookmarks can not have '\n' inside. I ran the following using python to illustrate this:

>>> subprocess.check_call(['hg', 'bookmark', 'abc\ndef'])
abort: '\n' cannot be used in a name

felixmerk: I'm under the impression that bookmarks can not have '\n' inside. I ran the following using…
stashUnsubmitted
Not Done

Oh, ok, for some reason I thought it's possible. Thanks for checking!

stash: Oh, ok, for some reason I thought it's possible. Thanks for checking!
# copy implementation for bookmarks # copy implementation for bookmarks
itercopy = [] itercopy = []
for mark in repo._bookmarks.iteritems(): for mark in repo._bookmarks.iteritems():
itercopy.append(mark) itercopy.append(mark)
bmchanges = [(mark[0], None) for mark in itercopy] bmremove = []
repo._bookmarks.applychanges(repo, repo.currenttransaction(), bmchanges) for mark in itercopy:
if not branchcommits or mark[1] in branchcommits:
bmremove.append((mark[0], None))
repo._bookmarks.applychanges(repo, repo.currenttransaction(), bmremove)
bmchanges = [] bmchanges = []
for mark in booklist: for mark in booklist:
if mark: if mark:
kv = mark.rsplit(" ", 1) kv = mark.rsplit(" ", 1)
if not branchcommits or\
bin(kv[1]) in branchcommits or\
(kv[0], None) in bmremove:
bmchanges.append((kv[0], bin(kv[1]))) bmchanges.append((kv[0], bin(kv[1])))
repo._bookmarks.applychanges(repo, repo.currenttransaction(), bmchanges) repo._bookmarks.applychanges(repo, repo.currenttransaction(), bmchanges)
stashUnsubmitted
Not Done

Do I understand correctly that if bookmark was moved from some commit outside of localbranch back to commit in localbranch then this bookmark will just be deleted? Is that what we want? I think it's better to return this bookmark to the previous location

stash: Do I understand correctly that if bookmark was moved from some commit outside of localbranch…
felixmerkAuthorUnsubmitted
Not Done

That does make more sense behavior wise. I'll make the change.

felixmerk: That does make more sense behavior wise. I'll make the change.
# working copy parent # working copy parent
workingcopyparent = _readnode(repo, "workingparent.i", workingcopyparent = _readnode(repo, "workingparent.i",
nodedict["workingparent"]) nodedict["workingparent"])
if not keep: if not keep:
#revealcommits(repo, workingcopyparent) if not branchcommits or bin(workingcopyparent) in branchcommits:
hg.updatetotally(ui, repo, workingcopyparent, workingcopyparent, hg.updatetotally(ui, repo, workingcopyparent, workingcopyparent,
clean=False, updatecheck='abort') clean=False, updatecheck='abort')
else: elif not branchcommits or bin(workingcopyparent) in branchcommits:
stashUnsubmitted
Not Done

It's unclear for me when do you need to use bin(...) and when you don't. I usually try to use names like hexnode or binnode to make it clear. No need to change it in this diff, but would be cool to do it in the separate diff

stash: It's unclear for me when do you need to use `bin(...)` and when you don't. I usually try to use…
felixmerkAuthorUnsubmitted
Not Done

Good call

felixmerk: Good call
# keeps working copy files # keeps working copy files
precnode = bin(workingcopyparent) precnode = bin(workingcopyparent)
precctx = repo[precnode] precctx = repo[precnode]
changedfiles = [] changedfiles = []
wctx = repo[None] wctx = repo[None]
wctxmanifest = wctx.manifest() wctxmanifest = wctx.manifest()
precctxmanifest = precctx.manifest() precctxmanifest = precctx.manifest()
if data[1][0] is None: if data[1][0] is None:
dirstate.remove(filename) dirstate.remove(filename)
# visible changesets # visible changesets
addedrevs = revsetlang.formatspec('olddraft(0) - olddraft(%d)', addedrevs = revsetlang.formatspec('olddraft(0) - olddraft(%d)',
reverseindex) reverseindex)
removedrevs = revsetlang.formatspec('olddraft(%d) - olddraft(0)', removedrevs = revsetlang.formatspec('olddraft(%d) - olddraft(0)',
reverseindex) reverseindex)
if not branch:
smarthide(repo, addedrevs, removedrevs) smarthide(repo, addedrevs, removedrevs)
revealcommits(repo, removedrevs) revealcommits(repo, removedrevs)
else:
localadds = revsetlang.formatspec('(olddraft(0) - olddraft(%d)) and'
' _localbranch(%s)',
reverseindex, branch)
localremoves = revsetlang.formatspec('(olddraft(%d) - olddraft(0)) and'
' _localbranch(%s)',
reverseindex, branch)
smarthide(repo, localadds, removedrevs)
smarthide(repo, addedrevs, localremoves, local=True)
revealcommits(repo, localremoves)
def _computerelative(repo, reverseindex): def _computerelative(repo, reverseindex):
# allows for relative undos using # allows for relative undos using
# redonode storage # redonode storage
try: try:
hexnode = repo.vfs.read("undolog/redonode") hexnode = repo.vfs.read("undolog/redonode")
rlog = _getrevlog(repo, 'index.i') rlog = _getrevlog(repo, 'index.i')
rev = rlog.rev(bin(hexnode)) rev = rlog.rev(bin(hexnode))
reverseindex = _invertindex(rlog, rev) + reverseindex reverseindex = _invertindex(rlog, rev) + reverseindex
except IOError: except IOError:
# return input index # return input index
pass pass
return reverseindex return reverseindex
# hide and reveal commits # hide and reveal commits
def smarthide(repo, revhide, revshow): def smarthide(repo, revhide, revshow, local=False):
'''hides changecontexts and reveals some commits '''hides changecontexts and reveals some commits
tries to connect related hides and shows with obs marker tries to connect related hides and shows with obs marker
when reasonable and correct when reasonable and correct
use local to not hide revhides without corresponding revshows
''' '''
hidectxs = repo.set(revhide) hidectxs = repo.set(revhide)
showctxs = repo.set(revshow) showctxs = repo.set(revshow)
for ctx in hidectxs: for ctx in hidectxs:
unfi = repo.unfiltered() unfi = repo.unfiltered()
related = [] related = []
related = set(obsutil.allprecursors(unfi.obsstore, [ctx.node()])) related = set(obsutil.allprecursors(unfi.obsstore, [ctx.node()]))
related.update(obsutil.allsuccessors(unfi.obsstore, [ctx.node()])) related.update(obsutil.allsuccessors(unfi.obsstore, [ctx.node()]))
# or split A to B,C # or split A to B,C
# because of undo we don't know which # because of undo we don't know which
# without complex logic # without complex logic
# Solution: provide helpfull ui message for # Solution: provide helpfull ui message for
# common and easy case (1 to 1), use simplest # common and easy case (1 to 1), use simplest
# correct solution for complex edge case # correct solution for complex edge case
if len(destinations) == 1: if len(destinations) == 1:
hidecommits(repo, ctx, destinations) hidecommits(repo, ctx, destinations)
else: elif len(destinations) > 1:# split
hidecommits(repo, ctx, [])
elif len(destinations) == 0:
if not local:
hidecommits(repo, ctx, []) hidecommits(repo, ctx, [])
def hidecommits(repo, curctx, precctxs): def hidecommits(repo, curctx, precctxs):
obsolete.createmarkers(repo, [(curctx, precctxs)], operation='undo') obsolete.createmarkers(repo, [(curctx, precctxs)], operation='undo')
def revealcommits(repo, rev): def revealcommits(repo, rev):
try: try:
inhibit = extensions.find('inhibit') inhibit = extensions.find('inhibit')
except KeyError: except KeyError:

tests/test-undo.t

summary: newfiles summary: newfiles
changeset: 18:75f63379f12b changeset: 18:75f63379f12b
tag: tip tag: tip
user: test user: test
date: Thu Jan 01 00:00:00 1970 +0000 date: Thu Jan 01 00:00:00 1970 +0000
summary: newfiles summary: newfiles
localbranch undos
Make changes on two branches (old and new)
Undo only changes in one branch (old)
Includes commit and book changes
$ hg book "oldbook"
$ touch oldbranch
$ hg add oldbranch && hg ci -moldbranch
$ hg update null
0 files updated, 0 files merged, 8 files removed, 0 files unresolved
(leaving bookmark oldbook)
$ touch newbranch
$ hg add newbranch && hg ci -mnewbranch
created new head
$ hg book "newbook"
$ hg log -l 2
changeset: 20:805791ba4bcd
bookmark: newbook
tag: tip
parent: -1:000000000000
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: newbranch
changeset: 19:7b0ef4f2a1ae
bookmark: oldbook
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: oldbranch
$ hg up 75f63379f12b
7 files updated, 0 files merged, 1 files removed, 0 files unresolved
(leaving bookmark newbook)
$ hg undo -b 75f63379f12b -n 6
0 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg log -l 2
changeset: 20:805791ba4bcd
bookmark: newbook
tag: tip
parent: -1:000000000000
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: newbranch
changeset: 18:75f63379f12b
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: newfiles
Check rebase local undos of rebases
Make sure bookmarks and commits are not lost
and commits are not duplicated
$ cat >> $HGRCPATH <<EOF
> [extensions]
> rebase =
> EOF
$ hg rebase -s 8057 -d 75f6
rebasing 20:805791ba4bcd "newbranch" (tip newbook)
$ hg log -l 2
changeset: 21:35324a911c0d
bookmark: newbook
tag: tip
parent: 18:75f63379f12b
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: newbranch
changeset: 18:75f63379f12b
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: newfiles
$ hg undo -b 3532
0 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg log -l 2
changeset: 20:805791ba4bcd
bookmark: newbook
tag: tip
parent: -1:000000000000
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: newbranch
changeset: 18:75f63379f12b
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: newfiles
$ cat >> $HGRCPATH <<EOF
> [extensions]
> rebase = !
> EOF