This is an archive of the discontinued Mercurial Phabricator instance.

Differential D55

undo: add --keep to maintian working copy changes
ClosedPublic

Authored by felixmerk on Jul 11 2017, 8:00 PM.
Tags
None
Subscribers
quark, phillco

Details

Reviewers
quark
Group Reviewers
Restricted Project
Commits
rFBHGX720575ccec6b: undo: add --keep to maintian working copy changes
Summary

Allow user to keep working copy changes after undo, mimicking unamend and
uncommit behavior. As discussed at the team lunch, this allows undo to remain
ignorant of what command the user has run while conforming to user expectations.
In the future this could be done in a smarter way where we properly restore hg
status dynamically.

Diff Detail

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

Event Timeline

felixmerk created this revision.Jul 11 2017, 8:00 PM
phillco added a subscriber: phillco.Jul 12 2017, 3:35 PM
phillco added inline comments.
hgext3rd/undo.py
383–384

Should we describe the downsides of this flag too?

384

specifically :)

385

exist

386

working copy

felixmerk updated this revision to Diff 108.Jul 13 2017, 12:41 AM
felixmerk added a reviewer: Restricted Project.Jul 13 2017, 12:43 AM
felixmerk updated this revision to Diff 109.Jul 13 2017, 2:09 AM
felixmerk added inline comments.Jul 13 2017, 2:12 AM
hgext3rd/undo.py
383–384

Yes. The major downside is that it makes redo non-trivial for the user as they need to clean up the working copy beforehand. There's also potentially some performance cost to --keep in the case of a large working copy.

felixmerk updated this revision to Diff 317.Jul 19 2017, 5:31 PM
quark added a subscriber: quark.Jul 19 2017, 6:26 PM
quark added inline comments.
hgext3rd/undo.py
496

Why is the obsolete.createmarkers vs revealcommits difference? I think the behavior should be the same regardless.

felixmerk added inline comments.Jul 19 2017, 6:34 PM
hgext3rd/undo.py
496

Fundamentally, its because there isn't a connection between two (arbitrary) working copy parents. With the --keep flag though, the user is heavily implying that there is a very strong connection between the two. Granted, the user can use --keep for really "weird" operations, but I think the obs marker is allowed to reflect that.

That being said, I can remove line 491 (revealcommits), since update will not fail to hidden commit, and that commit should be revealed later anyway.

quark accepted this revision.Jul 19 2017, 6:50 PM
quark added inline comments.
hgext3rd/undo.py
496

I noticed these lines are removed in the next diff. So the end result looks good to me.

I think line 491 is still necessary since visibility and obsolesce are different. We need to make the changeset "not obsoleted", hence the revive part. With fbamend, update could "unhide" the commit, but it won't "unobsolete" a commit.

This revision is now accepted and ready to land.Jul 19 2017, 6:50 PM
felixmerk updated this revision to Diff 325.Jul 19 2017, 8:45 PM
Closed by commit rFBHGX720575ccec6b: undo: add --keep to maintian working copy changes (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 (58 lines)
Mtests/test-undo.t (10 lines)

Diff 73

hgext3rd/undo.py

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: # Undo:
@command('undo', [ @command('undo', [
('n', 'index', 1, _("how many steps to undo back")),
('f', 'force', False, _("undo across a gap in memory (ADVANCED)")),
('a', 'absolute', False, _("absolute based on command index instead of " ('a', 'absolute', False, _("absolute based on command index instead of "
"relative undo")), "relative undo")),
('f', 'force', False, _("undo across a gap in memory (ADVANCED)")),
('k', 'keep', False, _("keep working copy changes")),
('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 atleast Undoes an undoable command. An undoable command is one that changed atleast
one of the following three: bookmarks, working copy parent or changesets. one of the following three: bookmarks, working copy parent or changesets.
Note that this specifically does not include commands like log. It will 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 include update if update changes the working copy parent (you update to a
If you run a command with the extension on, and the recorded undo-data does If you run a command with the extension on, and the recorded undo-data does
not match the pre-command state, a blank command is recorded before your new not match the pre-command state, a blank command is recorded before your new
command is run. This can happen when you first start the extension, turn command is run. This can happen when you first start the extension, turn
the extension off and on again, corrupt/edit/delete the storage files or the the extension off and on again, corrupt/edit/delete the storage files or the
repo or another command breaks something. Undoing across this "gap" can be repo or another command breaks something. Undoing across this "gap" can be
forced, but isn't advised unless its known how the before and after states forced, but isn't advised unless its known how the before and after states
are connected. Undoing up to a gap is safe. are connected. Undoing up to a gap is safe.
Use keep to maintain working copy changes. With keep, undo mimics hg
unamend and hg uncommit. Specifcally, files that exsist currently that
phillcoUnsubmitted
Not Done

specifically :)

phillco: specifically :)
phillcoUnsubmitted
Not Done

Should we describe the downsides of this flag too?

phillco: Should we describe the downsides of this flag too?
felixmerkAuthorUnsubmitted
Not Done

Yes. The major downside is that it makes redo non-trivial for the user as they need to clean up the working copy beforehand. There's also potentially some performance cost to --keep in the case of a large working copy.

felixmerk: Yes. The major downside is that it makes redo non-trivial for the user as they need to clean…
don't exsist at the repo state we are undoing to will remain in your
phillcoUnsubmitted
Not Done

exist

phillco: exist
workingcopy but not in your changeset.
phillcoUnsubmitted
Not Done

working copy

phillco: working copy
""" """
try:
extensions.find('inhibit')
except KeyError:
hint = _("please add inhibit to the list of enabled extensions")
e = _("undo requires inhibit extension to be enabled")
raise error.Abort(e, hint=hint)
reverseindex = opts.get("index") reverseindex = opts.get("index")
relativeundo = not opts.get("absolute") relativeundo = not opts.get("absolute")
keep = opts.get("keep")
if repo is not None: if repo is not None:
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) _undoto(ui, repo, reverseindex, keep=keep)
# 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, reverseindex) _logundoredoindex(repo, reverseindex)
@command('redo', [ @command('redo', [
('n', 'index', 1, _("how many commands to redo")), ('n', 'index', 1, _("how many commands to redo")),
]) ])
def redo(ui, repo, *args, **opts): def redo(ui, repo, *args, **opts):
""" perform a redo """ perform a redo
Performs a redo. Specifically, redo moves forward a repo state relative to Performs a redo. Specifically, redo moves forward a repo state relative to
the previous undo or redo command. If you run hg undo -n 10, you can redo the previous undo or redo command. If you run hg undo -n 10, you can redo
each of the 10 repo states one by one all the way back to the state from each of the 10 repo states one by one all the way back to the state from
which you ran undo. You can use --index to redo across more states at once, which you ran undo. You can use --index to redo across more states at once,
and you can use any number of undos/redos up to the current state or back to and you can use any number of undos/redos up to the current state or back to
when the undo extension was first active. when the undo extension was first active.
""" """
try:
extensions.find('inhibit')
except KeyError:
hint = _("please add inhibit to the list of enabled extensions")
e = _("redo requires inhibit extension to be enabled")
raise error.Abort(e, hint=hint)
reverseindex = -1 * abs(opts.get("index")) reverseindex = -1 * abs(opts.get("index"))
reverseindex = _computerelative(repo, reverseindex) reverseindex = _computerelative(repo, reverseindex)
if repo is not None: if repo is not None:
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, reverseindex) _logundoredoindex(repo, reverseindex)
def _undoto(ui, repo, reverseindex): def _undoto(ui, repo, reverseindex, keep=False):
# undo to specific reverseindex # undo to specific reverseindex
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")
# 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)
for mark in itercopy: for mark in itercopy:
commands.bookmark(ui, repo, mark[0], delete=True) commands.bookmark(ui, repo, mark[0], delete=True)
for mark in booklist: for mark in booklist:
if mark is not None and mark != "": if mark is not None and mark != "":
kv = mark.rsplit(" ", 1) kv = mark.rsplit(" ", 1)
commands.bookmark(ui, repo, kv[0], rev=kv[1]) commands.bookmark(ui, repo, kv[0], rev=kv[1])
# working copy parent # working copy parent
workingcopyparent = _readnode(repo, "workingparent.i", workingcopyparent = _readnode(repo, "workingparent.i",
nodedict["workingparent"]) nodedict["workingparent"])
if not keep:
revealcommits(ui, repo, workingcopyparent) revealcommits(ui, repo, workingcopyparent)
commands.update(ui, repo, rev=workingcopyparent) commands.update(ui, repo, rev=workingcopyparent)
else:
# keeps working copy files
curctx = repo['.']
precnode = bin(workingcopyparent)
precctx = repo[precnode]
changedfiles = []
wctx = repo[None]
wctxmanifest = wctx.manifest()
precctxmanifest = precctx.manifest()
dirstate = repo.dirstate
diff = precctxmanifest.diff(wctxmanifest)
changedfiles.extend(diff.iterkeys())
with dirstate.parentchange():
dirstate.rebuild(precnode, precctxmanifest, changedfiles)
# we want added and removed files to be shown
# properly, not with ? and ! prefixes
for filename, data in diff.iteritems():
if data[0][0] is None:
dirstate.add(filename)
if data[1][0] is None:
dirstate.remove(filename)
obsolete.createmarkers(repo, [(curctx, (precctx,))])
quarkUnsubmitted
Not Done

Why is the obsolete.createmarkers vs revealcommits difference? I think the behavior should be the same regardless.

quark: Why is the `obsolete.createmarkers` vs `revealcommits` difference? I think the behavior should…
felixmerkAuthorUnsubmitted
Not Done

Fundamentally, its because there isn't a connection between two (arbitrary) working copy parents. With the --keep flag though, the user is heavily implying that there is a very strong connection between the two. Granted, the user can use --keep for really "weird" operations, but I think the obs marker is allowed to reflect that.

That being said, I can remove line 491 (revealcommits), since update will not fail to hidden commit, and that commit should be revealed later anyway.

felixmerk: Fundamentally, its because there isn't a connection between two (arbitrary) working copy…
quarkUnsubmitted
Not Done

I noticed these lines are removed in the next diff. So the end result looks good to me.

I think line 491 is still necessary since visibility and obsolesce are different. We need to make the changeset "not obsoleted", hence the revive part. With fbamend, update could "unhide" the commit, but it won't "unobsolete" a commit.

quark: I noticed these lines are removed in the next diff. So the end result looks good to me. I…
# visible changesets # visible changesets
addedrevs = revsetlang.formatspec('olddraft(0) - olddraft(%d)', addedrevs = revsetlang.formatspec('olddraft(0) - olddraft(%d)',
reverseindex) reverseindex)
revs = repo.revs(addedrevs) revs = repo.revs(addedrevs)
with repo.lock(): with repo.lock():
for r in revs: for r in revs:
hidecommits(repo, repo.changectx(r)) hidecommits(repo, repo.changectx(r))

tests/test-undo.t

17: undo 17: undo
$ hg undo -a -n 20 $ hg undo -a -n 20
abort: attempted risky undo across missing history abort: attempted risky undo across missing history
[255] [255]
$ hg undo -a -n 20 -f $ hg undo -a -n 20 -f
0 files updated, 0 files merged, 1 files removed, 0 files unresolved 0 files updated, 0 files merged, 1 files removed, 0 files unresolved
$ hg undo -a $ hg undo -a
1 files updated, 0 files merged, 0 files removed, 0 files unresolved 1 files updated, 0 files merged, 0 files removed, 0 files unresolved
hg undo --keep tests
$ touch kfl1 && hg add kfl1
$ hg st
A kfl1
$ hg commit --amend
$ hg st
$ hg undo --keep
$ hg st
A kfl1