This is an archive of the discontinued Mercurial Phabricator instance.

Differential D186

undo: localbrach fancy indexes
ClosedPublic

Authored by felixmerk on Jul 26 2017, 2:42 PM.
Tags
None
Subscribers
quark, stash

Details

Reviewers
quark
stash
Group Reviewers
Restricted Project
Commits
rFBHGX3a71483bbf1d: undo: localbrach fancy indexes
Summary

Adds smart indexing to localbranch undoes. This eliminates the mental burden of
the user of keeping track in what order and when changes where made. Drawback
is that it raises the computational complexity from the number of draft
commits to draft commits times how many repo states you are undoing. Common use
case should be to undo a small number of changes, so this is acceptable.

hg undo -b changectx finds the closest pertinent change and undoes that. -b
with -n is not supported. The reason it is not supported is that local branch
level undoes are delta specific and not state specific, which in essence means
that running hg undo -b # twice is not guaranteed to give you the same resault
as running hg undo -b # -n 2. We could run -b multiple times for a -n, but the
cost of a -b increases with a larger -n (which isn't the case for a standard
undo), and abort handling would be more complicated (I'm not sure how well we
can roll back a tree of obs markers?). In essence -n with -b is not that
usefull a feature (bc perf limits) and would potentially require a decently large
amount of time to implement correctly.

Also adds redo -b since this command now makes a lot more sense when we use the
smarter indexing. Adds logic to seperate undoes and redoes in different
branches. You can perform relative undoes and redoes within the latest scope,
while undoes and redoes within a new scope will start at the present (absolute)
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

felixmerk created this revision.Jul 26 2017, 2:42 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 26 2017, 2:42 PM
felixmerk retitled this revision from undo: localbrach fancing indexes to undo: localbrach fancy indexes and redos.Jul 26 2017, 2:42 PM
felixmerk retitled this revision from undo: localbrach fancy indexes and redos to undo: localbrach fancing indexes.Jul 26 2017, 6:39 PM
felixmerk added a child revision: D188: undo: clean up bin/hex.
felixmerk updated this revision to Diff 422.Jul 26 2017, 8:27 PM
stash added a subscriber: stash.Jul 27 2017, 7:31 AM

sorry, didn't have time to look more closely, so just nits

hgext3rd/undo.py
211

Does it make sense to make os.path.join("undolog", "redonode") a module-level constant?

Also it will break clients if they have undolog/redonode in old format i.e. just str(hexnode). That should be fine because undo is a relatively new feature

214

os.path.join('undolog', 'redonode')

640–642

Do we need this if at all?

641

What does iden mean here?

tests/test-undo.t
661

It would be nice to run hg log -r . -T {node} to print the node

felixmerk added inline comments.Jul 27 2017, 2:23 PM
hgext3rd/undo.py
211

Jun gave me the impression that repo.vfs.write handles OS specific file paths, so os.path.join is at best redundant or at worst possibly breaking things. But having file locations as module-level constants does make sense.

I believe this would only break clients if they run undo or redo right after updating (since otherwise the file gets overwritten). Considering the small number of clients using undo this shouldn't be a problem.

640–642

No, I can merge it into a != and an and with the elif.

641

identify (the branch)

felixmerk updated this revision to Diff 428.Jul 27 2017, 2:29 PM
felixmerk marked 5 inline comments as done.Jul 27 2017, 2:29 PM
quark requested changes to this revision.EditedJul 27 2017, 6:40 PM
quark added a subscriber: quark.

I don't think --index works well with branch undo using the current implementation. Consider the following case:

  $ cat >> $HGRCPATH <<EOF
  > [extensions]
  > undo = $TESTDIR/../hgext3rd/undo.py
  > drawdag=$RUNTESTDIR/drawdag.py
  > inhibit=$TESTDIR/../hgext3rd/inhibit.py
  > rebase=
  > [experimental]
  > evolution=createmarkers
  > EOF

  $ hg init repo
  $ cd repo
  $ hg debugdrawdag <<'EOS'
  > G F
  > | |
  > | E
  > |/
  > D C
  > | |
  > | B
  > |/
  > A
  > EOS

  $ hg phase --public G
  $ rm .hg/localtags
  $ hg rebase -s 'desc(F)' -d 'desc(G)'
  rebasing 6:b5e305ce895c "F" (tip)
  $ hg rebase -s 'desc(C)' -d tip
  rebasing 3:26805aba1e60 "C"

#testcases expected unexpected

#if expected

Running "undo" twice works:

  $ hg undo -b 7
  $ hg log -G -T '{rev} {desc} {phase}' -r 'sort(all(), topo)'
  o  7 F draft
  |
  o  5 G public
  |
  | o  4 E draft
  |/
  o  2 D public
  |
  | o  3 C draft
  | |
  | o  1 B draft
  |/
  o  0 A public
  
  $ hg undo -b 7
  $ hg log -G -T '{rev} {desc} {phase}' -r 'sort(all(), topo)'
  o  6 F draft
  |
  o  4 E draft
  |
  | o  5 G public
  |/
  o  2 D public
  |
  | o  3 C draft
  | |
  | o  1 B draft
  |/
  o  0 A public
  
#else

"--index 2" produces wrong result - "C" will be missed:

  $ hg undo -b 7 --index 2
  $ hg log -G -T '{rev} {desc} {phase}' -r 'sort(all(), topo)'
  o  6 F draft
  |
  o  4 E draft
  |
  | o  5 G public
  |/
  o  2 D public
  |
  | o  1 B draft
  |/
  o  0 A public
  
#endif

I think implementing local branch --index correctly could be very tricky and could easily be wrong for corner cases. So I'd vote to make it simple - only support 1-step undo for the local branch case and disallow --index to be used together with --branch.

hgext3rd/undo.py
211

@stash I think / is preferred over os.path.join. Mercurial internal always uses /. os.path.join is only useful when interacting with filesystem directly (i.e. bypassing the vfs layer).

214

See the above explanation why / is better.

624

Might also handle IndexError here so people with the old (or corrupted) redonode state file would not crash the code here.

626

Since our transaction is not safe, might handle error.RevlogError too.

661

nit: reverseindex

690

nit: need space

This revision now requires changes to proceed.Jul 27 2017, 6:40 PM
felixmerk edited edge metadata.Jul 31 2017, 6:04 PM
felixmerk updated this revision to Diff 464.
felixmerk updated this revision to Diff 467.Jul 31 2017, 6:15 PM
stash added a comment.Aug 1 2017, 10:39 AM

The logic also handles --index fine.

Please update the summary, this is no longer true

stash requested changes to this revision.Aug 1 2017, 11:03 AM

The logic to handle -b correctly is quite complicated. Personally I'm not convinced that increased code complexity is worth new feature, but I maybe wrong here.

hgext3rd/undo.py
497–498

Since this option is experimental than this is probably fine, but as a user I'd have had a hard time understanding what single branch undo means. Better description would be great.

512–515

It's not clear for me why we don't allow --branch and --index in undo, but allow in redo. Can you explain and preferably add a comment? Also you don't test hg redo -b ... --index ....

644–645

Nit: this formatting is wrong:

rootdelta = revsetlang.formatspec(
      'roots(_localbranch(%s)) - roots(_localbranch(%s))', branch, oldbranch)
654–657

If redo accepts either --index or --branch then this logic can be simplified

This revision now requires changes to proceed.Aug 1 2017, 11:03 AM
felixmerk added a comment.Aug 1 2017, 2:16 PM

In defense of --branch:
While I concur that the command (at least currently) is obtuse to users (I'll try to improve the documentation) and the code that goes along with it is somewhat complex, I think its a very powerful feature. As a feature is allows you to undo changes within a much smaller scope than the entire repo and lays the ground work for undoes with arbitrary sets of commits. Probably only power users will use it as a command line tool and as such the impact is limited (although it does simplify things greatly in certain cases). However, as a nuclide tool (you select a local branch and then hit undo) I think the feature would be very easy to use and very powerful. That being said, I don't believe I'll have enough time to implement that level of nuclide support (will see), but that is where I see this feature having impact and where the future of undo might lie.

felixmerk edited edge metadata.Aug 1 2017, 2:20 PM
felixmerk edited the summary of this revision. (Show Details)
felixmerk retitled this revision from undo: localbrach fancing indexes to undo: localbrach fancy indexes.
felixmerk updated this revision to Diff 477.
stash requested changes to this revision.Aug 2 2017, 5:19 AM
stash added inline comments.
hgext3rd/undo.py
517–523

BTW, undo and redo code implementations are quite similar and potentially it can be unified. But probably not worth doing right now.

521–523

Is there a reason why branch is not recorded here? As far as I understand if our repo in STATE1 and we do:

hg undo -b HASH 
hg undo -b HASH 
hg redo -b HASH 
hg redo -b HASH

it may result in STATE2 != STATE1. I think that's not what we want, isn't it?
I'm right then please add a test for this case.

680–681

Nit:

alphaworkingcopyparent = _readnode(repo, "workingparent.i",
                                   nodedict["workingparent"])
689

Nit:

incrementalindex += direction
699–701

This comment is quite unclear. Can you elaborate?

702–705

Am I right about the priorities here:

((olddraft(%d) + olddraft(%d)) - (olddraft(%d) and olddraft(%d))) and _localbranch(%s)

?
If yes then please add the parens

BTW, the fact that - and and have the same priorities seems a bit weird for me

710

Nit: missing space

if done: # perf boost
712–713

It can be moved outside of while loop

726

Why break here? What if a few bookmarks were moved at once, for example, during histedit?

729

Typo:

intereSting
This revision now requires changes to proceed.Aug 2 2017, 5:19 AM
felixmerk added inline comments.Aug 2 2017, 2:32 PM
hgext3rd/undo.py
726

Find next delta finds the next command that changed something within a limited scope ("localbranch"). At this point we don't care wether one bookmark or a million changed, we are looking for at least one thing (working copy parent, bookmark, draft commits) to have changed. Find next delta returns the index at which the next delta occurred.

stash added inline comments.Aug 2 2017, 4:09 PM
hgext3rd/undo.py
726

That makes sense, thank you for explanation. Probably worth adding a comment though

felixmerk edited edge metadata.Aug 2 2017, 4:31 PM
felixmerk updated this revision to Diff 509.
stash accepted this revision.Aug 3 2017, 5:18 AM

I don't want to block you, especially since this feature is experimental. But consider the cases below.

hgext3rd/undo.py
621

Nit: try: #

661

Not sure if we need a default value for direction

707–710

Let's say we have a following situation:

We had a stack

   o - B
    |
   @  - A (draft)
  /
o
...

Then we stripped commit B

   @  - A (draft)
  /
o
...

Then we run hg undo -b A.

Here _localbranch(A) returns A, olddraft(-1) returns (A, B), olddraft(0) returns A. Then we have ((A) + (A, B)) - ((A) & (A, B)) & (A) = ((A, B) - (A) ) & (A) = (). So undo won't undo the strip. Is it expected?

732–733

I've just tested it and looks like there is a difference between D160 and this diff.

Let's say we have a stack of commits

   o
    |
   @  - A (draft)
  /
o
|
o - B (public)

Then we checkout B, and then we run hg undo -b A. In D160 it would just update to A, but with current changes it will move backwards.

felixmerk added inline comments.Aug 3 2017, 1:35 PM
hgext3rd/undo.py
621

Wow, I'll make sure I stop doing this. I guess I'm too used not leaving trailing white spaces, but leaving a space before the hashtag is definitely cleaner.

707–710

This isn't expected but this isn't what happens. localbranch() includes hidden commits. I'll add a test.

732–733

Yes, this was intended. Moving the working copy parent from or to a branch doesn't really 'feel' like a change to this branch. Moving a working copy parent within a branch on the other hand does. This is an implementation detail (documented on line 729). I'm fine going either way with this.

quark accepted this revision.Aug 3 2017, 1:45 PM
quark added inline comments.
hgext3rd/undo.py
521–523

Ideally we want redo to precisely revert what undo does. An alternative is to make redo accept zero flags and just read the operation log, undoing the latest undo command (so redo will not have branch concept and performs a whole-repo undo so the state must match).

Per discussion yesterday, the current design allows things like:

hg undo -n 5
hg redo

to be equivalent to

hg undo -n 4

The alternative approach will make redo undoes 5 steps, which is different but may also be explainable since redo does not have -n.

Another UX difference is if a user runs hg undo -b NAME and that NAME gets disappeared as a side effect of undo, currently, the user might lose a way to redo. So a redo without any flags seems easier to use here.

@stash Do you have preference or some tricky cases in mind that worth considering?

I'm fine with the code as-is. We can revisit it before enabling it to all users.

This revision is now accepted and ready to land.Aug 3 2017, 1:45 PM
felixmerk updated this revision to Diff 527.Aug 3 2017, 1:58 PM
Closed by commit rFBHGX3a71483bbf1d: undo: localbrach fancy indexes (authored by felixmerk). · Explain WhyAug 4 2017, 9:19 AM
This revision was automatically updated to reflect the committed changes.
stash added a comment.Aug 7 2017, 11:19 AM

I actually like the idea about redo not taking any parameters, I think it will be less surprising for users.

Revision Contents
Changeset List

PathPackages
Mhgext3rd/undo.py (164 lines)
Mtests/test-undo.t (73 lines)

Diff 541

hgext3rd/undo.py

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.
try: try:
try: try:
repo.vfs.makedirs('undolog') repo.vfs.makedirs('undolog')
except OSError: except OSError:
repo.ui.debug("can't make undolog folder in .hg") repo.ui.debug("can't make undolog folder in .hg")
return changes return changes
with lockmod.lock(repo.vfs, "undolog/lock", desc="undolog", with lockmod.lock(repo.vfs, "undolog/lock", desc="undolog",
timeout=2): timeout=2):
# 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" ==
command[0]): command[0]):
_delundoredo(repo) _delundoredo(repo)
except error.LockUnavailable:# no write permissions except error.LockUnavailable: # no write permissions
repo.ui.debug("undolog lacks write permission\n") repo.ui.debug("undolog lacks write permission\n")
except error.LockHeld:# timeout, not fatal: don't abort actual command except error.LockHeld: # timeout, not fatal: don't abort actual command
# TODO: log to Scuba. This shouldn't happen too often as it will # TODO: log to Scuba. This shouldn't happen too often as it will
# create gaps in the undo log # create gaps in the undo log
repo.ui.debug("undolog lock timeout\n") repo.ui.debug("undolog lock timeout\n")
return changes return changes
def lighttransaction(repo): def lighttransaction(repo):
# full fledged transactions have two serious issues: # full fledged transactions have two serious issues:
# 1. they may cause infite loops through hooks # 1. they may cause infite loops through hooks
def _logworkingparent(repo, tr): def _logworkingparent(repo, tr):
revstring = repo['.'].hex() revstring = repo['.'].hex()
return writelog(repo, tr, "workingparent.i", revstring) return writelog(repo, tr, "workingparent.i", revstring)
def _logindex(repo, tr, nodes): def _logindex(repo, tr, nodes):
revstring = "\n".join(sorted('%s %s' % (k, v) for k, v in nodes.items())) revstring = "\n".join(sorted('%s %s' % (k, v) for k, v in nodes.items()))
return writelog(repo, tr, "index.i", revstring) return writelog(repo, tr, "index.i", revstring)
def _logundoredoindex(repo, tr, reverseindex): def _logundoredoindex(repo, tr, reverseindex, branch=""):
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.vfs.write("undolog/redonode", str(hexnode)) return repo.vfs.write("undolog/redonode", str(hexnode) + "\0" + branch)
stashUnsubmitted
Not Done

Does it make sense to make os.path.join("undolog", "redonode") a module-level constant?

Also it will break clients if they have undolog/redonode in old format i.e. just str(hexnode). That should be fine because undo is a relatively new feature

stash: Does it make sense to make `os.path.join("undolog", "redonode")` a module-level constant? Also…
felixmerkAuthorUnsubmitted
Not Done

Jun gave me the impression that repo.vfs.write handles OS specific file paths, so os.path.join is at best redundant or at worst possibly breaking things. But having file locations as module-level constants does make sense.

I believe this would only break clients if they run undo or redo right after updating (since otherwise the file gets overwritten). Considering the small number of clients using undo this shouldn't be a problem.

felixmerk: Jun gave me the impression that repo.vfs.write handles OS specific file paths, so os.path.join…
quarkUnsubmitted
Not Done

@stash I think / is preferred over os.path.join. Mercurial internal always uses /. os.path.join is only useful when interacting with filesystem directly (i.e. bypassing the vfs layer).

quark: @stash I think `/` is preferred over `os.path.join`. Mercurial internal always uses `/`. `os.
def _delundoredo(repo): def _delundoredo(repo):
path = 'undolog' + '/' + 'redonode' path = 'undolog' + '/' + 'redonode'
stashUnsubmitted
Not Done

os.path.join('undolog', 'redonode')

stash: `os.path.join('undolog', 'redonode')`
quarkUnsubmitted
Not Done

See the above explanation why / is better.

quark: See the above explanation why `/` is better.
repo.vfs.tryunlink(path) repo.vfs.tryunlink(path)
def _recordnewgap(repo, absoluteindex=None): def _recordnewgap(repo, absoluteindex=None):
path = 'undolog' + '/' + 'gap' path = 'undolog' + '/' + 'gap'
if absoluteindex is None: if absoluteindex is None:
rlog = _getrevlog(repo, 'index.i') rlog = _getrevlog(repo, 'index.i')
repo.vfs.write(path, str(len(rlog) - 1)) repo.vfs.write(path, str(len(rlog) - 1))
else: else:
unamend and hg uncommit. Specifically, files that exsist currently that unamend and hg uncommit. Specifically, files that exsist currently that
don't exist at the repo state we are undoing to will remain in your don't exist at the repo state we are undoing to will remain in your
working copy but not in your changeset. Maintaining your working copy working copy but not in your changeset. Maintaining your working copy
has primarily two downsides: firstly your new working copy won't be clean has primarily two downsides: firstly your new working copy won't be clean
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.
Branch limits the scope of an undo to a group of local (draft) changectxs,
identified by any one member of this group.
""" """
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") branch = opts.get("branch")
if branch and reverseindex != 1:
raise error.Abort(_("--branch with --index not supported"))
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: reverseindex = _computerelative(repo, reverseindex,
reverseindex = _computerelative(repo, reverseindex) absolute=not relativeundo,
branch=branch)
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, branch=branch) _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, branch)
@command('redo', [ @command('redo', [
('b', 'branch', "", _("single branch undo, accepts commit hash "
"(ADVANCED)")),
stashUnsubmitted
Not Done

Since this option is experimental than this is probably fine, but as a user I'd have had a hard time understanding what single branch undo means. Better description would be great.

stash: Since this option is experimental than this is probably fine, but as a user I'd have had a hard…
('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.
""" """
branch = opts.get("branch")
reverseindex = -1 * abs(opts.get("index")) reverseindex = -1 * abs(opts.get("index"))
reverseindex = _computerelative(repo, reverseindex)
if branch and reverseindex != -1:
raise error.Abort(_("--branch with --index not supported"))
stashUnsubmitted
Not Done

It's not clear for me why we don't allow --branch and --index in undo, but allow in redo. Can you explain and preferably add a comment? Also you don't test hg redo -b ... --index ....

stash: It's not clear for me why we don't allow `--branch` and `--index` in undo, but allow in 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) reverseindex = _computerelative(repo, reverseindex, branch=branch)
_logundoredoindex(repo, repo.currenttransaction(), reverseindex) _undoto(ui, repo, reverseindex, branch=branch)
_logundoredoindex(repo, repo.currenttransaction(), reverseindex, branch)
stashUnsubmitted
Not Done

Is there a reason why branch is not recorded here? As far as I understand if our repo in STATE1 and we do:

hg undo -b HASH 
hg undo -b HASH 
hg redo -b HASH 
hg redo -b HASH

it may result in STATE2 != STATE1. I think that's not what we want, isn't it?
I'm right then please add a test for this case.

stash: Is there a reason why `branch` is not recorded here? As far as I understand if our repo in…
quarkUnsubmitted
Not Done

Ideally we want redo to precisely revert what undo does. An alternative is to make redo accept zero flags and just read the operation log, undoing the latest undo command (so redo will not have branch concept and performs a whole-repo undo so the state must match).

Per discussion yesterday, the current design allows things like:

hg undo -n 5
hg redo

to be equivalent to

hg undo -n 4

The alternative approach will make redo undoes 5 steps, which is different but may also be explainable since redo does not have -n.

Another UX difference is if a user runs hg undo -b NAME and that NAME gets disappeared as a side effect of undo, currently, the user might lose a way to redo. So a redo without any flags seems easier to use here.

@stash Do you have preference or some tricky cases in mind that worth considering?

I'm fine with the code as-is. We can revisit it before enabling it to all users.

quark: Ideally we want `redo` to precisely revert what `undo` does. An alternative is to make `redo`…
stashUnsubmitted
Not Done

BTW, undo and redo code implementations are quite similar and potentially it can be unified. But probably not worth doing right now.

stash: BTW, undo and redo code implementations are quite similar and potentially it can be unified.
def _undoto(ui, repo, reverseindex, keep=False, branch=None): 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) # branch is a changectx hash (potentially short form)
# which identifies its branch via localbranch revset # 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"))
reverseindex, branch) reverseindex, branch)
localremoves = revsetlang.formatspec('(olddraft(%d) - olddraft(0)) and' localremoves = revsetlang.formatspec('(olddraft(%d) - olddraft(0)) and'
' _localbranch(%s)', ' _localbranch(%s)',
reverseindex, branch) reverseindex, branch)
smarthide(repo, localadds, removedrevs) smarthide(repo, localadds, removedrevs)
smarthide(repo, addedrevs, localremoves, local=True) smarthide(repo, addedrevs, localremoves, local=True)
revealcommits(repo, localremoves) revealcommits(repo, localremoves)
def _computerelative(repo, reverseindex): def _computerelative(repo, reverseindex, absolute=False, branch=""):
# allows for relative undos using # allows for relative undos using
# redonode storage # redonode storage
# allows for branch undos using
# findnextdelta logic
if not absolute:
try: # attempt to get relative shift
stashUnsubmitted
Not Done

Nit: try: #

stash: Nit: `try: #`
felixmerkAuthorUnsubmitted
Not Done

Wow, I'll make sure I stop doing this. I guess I'm too used not leaving trailing white spaces, but leaving a space before the hashtag is definitely cleaner.

felixmerk: Wow, I'll make sure I stop doing this. I guess I'm too used not leaving trailing white spaces…
nodebranch = repo.vfs.read("undolog/redonode").split("\0")
hexnode = nodebranch[0]
try: try:
quarkUnsubmitted
Not Done

Might also handle IndexError here so people with the old (or corrupted) redonode state file would not crash the code here.

quark: Might also handle `IndexError` here so people with the old (or corrupted) `redonode` state file…
hexnode = repo.vfs.read("undolog/redonode") oldbranch = nodebranch[1]
except IndexError:
quarkUnsubmitted
Not Done

Since our transaction is not safe, might handle error.RevlogError too.

quark: Since our transaction is not safe, might handle `error.RevlogError` too.
oldbranch = ""
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 shiftedindex = _invertindex(rlog, rev)
except IOError: except IOError, error.RevlogError:
# return input index # no shift
pass shiftedindex = 0
oldbranch = ""
else:
shiftedindex = 0
oldbranch = ""
if not branch:
reverseindex = shiftedindex + reverseindex
else:
stashUnsubmitted
Done

What does iden mean here?

stash: What does `iden` mean here?
felixmerkAuthorUnsubmitted
Done

identify (the branch)

felixmerk: identify (the branch)
# check if relative branch
stashUnsubmitted
Done

Do we need this if at all?

stash: Do we need this `if` at all?
felixmerkAuthorUnsubmitted
Done

No, I can merge it into a != and an and with the elif.

felixmerk: No, I can merge it into a != and an and with the elif.
if (branch != oldbranch) and (oldbranch != ""):
rootdelta = revsetlang.formatspec(
'roots(_localbranch(%s)) - roots(_localbranch(%s))',
stashUnsubmitted
Not Done

Nit: this formatting is wrong:

rootdelta = revsetlang.formatspec(
      'roots(_localbranch(%s)) - roots(_localbranch(%s))', branch, oldbranch)
stash: Nit: this formatting is wrong: ``` rootdelta = revsetlang.formatspec…
branch, oldbranch)
if repo.revs(rootdelta):
# different group of commits
shiftedindex = 0
# from shifted index, find reverse index # of states that change
# branch
# remember that reverseindex can be negative
sign = reverseindex / abs(reverseindex)
for count in range(abs(reverseindex)):
shiftedindex = _findnextdelta(repo, shiftedindex, branch,
direction=sign)
stashUnsubmitted
Not Done

If redo accepts either --index or --branch then this logic can be simplified

stash: If redo accepts either `--index` or `--branch` then this logic can be simplified
reverseindex = shiftedindex
return reverseindex return reverseindex
def _findnextdelta(repo, reverseindex, branch, direction):
quarkUnsubmitted
Not Done

nit: reverseindex

quark: nit: reverseindex
stashUnsubmitted
Not Done

Not sure if we need a default value for direction

stash: Not sure if we need a default value for `direction`
# finds closest repos state making changes to branch in direction
# input:
# repo: mercurial.localrepo
# reverseindex: positive int for index.i
# branch: string changectx (commit hash)
# direction: positive or negative int
# output:
# int index with next branch delta
# this is the first repo state that makes a changectx, bookmark or working
# copy parent change that effects the given branch
if 0 == direction: # no infinite cycles guarantee
raise error.ProgrammingError
repo = repo.unfiltered()
# current state
try:
nodedict = _readindex(repo, reverseindex)
except IndexError:
raise error.Abort(_("index out of bounds"))
alphaworkingcopyparent = _readnode(repo, "workingparent.i",
nodedict["workingparent"])
stashUnsubmitted
Not Done

Nit:

alphaworkingcopyparent = _readnode(repo, "workingparent.i",
                                   nodedict["workingparent"])
stash: Nit: ``` alphaworkingcopyparent = _readnode(repo, "workingparent.i"…
alphabookstring = _readnode(repo, "bookmarks.i",
nodedict["bookmarks"])
incrementalindex = reverseindex
localbranch = repo.revs(
revsetlang.formatspec("_localbranch(%s)", branch))
tonode = repo.changelog.node
hexnodes = [hex(tonode(x)) for x in localbranch]
stashUnsubmitted
Not Done

Nit:

incrementalindex += direction
stash: Nit: ``` incrementalindex += direction ```
quarkUnsubmitted
Not Done

nit: need space

quark: nit: need space
done = False
while not done:
# move index
incrementalindex += direction
# check this index
try:
nodedict = _readindex(repo, incrementalindex)
except IndexError:
raise error.Abort(_("index out of bounds"))
# check wkp, commits, bookmarks
workingcopyparent = _readnode(repo, "workingparent.i",
stashUnsubmitted
Not Done

This comment is quite unclear. Can you elaborate?

stash: This comment is quite unclear. Can you elaborate?
nodedict["workingparent"])
bookstring = _readnode(repo, "bookmarks.i", nodedict["bookmarks"])
# local changes in respect to visible changectxs
# disjunctive union of present and old = changes
stashUnsubmitted
Not Done

Am I right about the priorities here:

((olddraft(%d) + olddraft(%d)) - (olddraft(%d) and olddraft(%d))) and _localbranch(%s)

?
If yes then please add the parens

BTW, the fact that - and and have the same priorities seems a bit weird for me

stash: Am I right about the priorities here: ``` ((olddraft(%d) + olddraft(%d)) - (olddraft(%d) and…
# intersection of changes and local = localchanges
localctxchanges = revsetlang.formatspec(
'((olddraft(%d) + olddraft(%d)) -'
'(olddraft(%d) and olddraft(%d)))'
' and _localbranch(%s)',
stashUnsubmitted
Not Done

Nit: missing space

if done: # perf boost
stash: Nit: missing space ``` if done: # perf boost ```
stashUnsubmitted
Not Done

Let's say we have a following situation:

We had a stack

   o - B
    |
   @  - A (draft)
  /
o
...

Then we stripped commit B

   @  - A (draft)
  /
o
...

Then we run hg undo -b A.

Here _localbranch(A) returns A, olddraft(-1) returns (A, B), olddraft(0) returns A. Then we have ((A) + (A, B)) - ((A) & (A, B)) & (A) = ((A, B) - (A) ) & (A) = (). So undo won't undo the strip. Is it expected?

stash: Let's say we have a following situation: We had a stack ``` o - B | @ - A (draft)…
felixmerkAuthorUnsubmitted
Not Done

This isn't expected but this isn't what happens. localbranch() includes hidden commits. I'll add a test.

felixmerk: This isn't expected but this isn't what happens. localbranch() includes hidden commits. I'll…
incrementalindex, reverseindex,
incrementalindex, reverseindex,
branch)
stashUnsubmitted
Not Done

It can be moved outside of while loop

stash: It can be moved outside of `while` loop
done = done or repo.revs(localctxchanges)
if done: # perf boost
break
# bookmark changes
if alphabookstring != bookstring:
diff = set(alphabookstring.split("\n")) ^\
set(bookstring.split("\n"))
for mark in diff:
if mark:
kv = mark.rsplit(" ", 1)
# was or will the mark be in the localbranch
if kv[1] in hexnodes:
done = True
stashUnsubmitted
Not Done

Why break here? What if a few bookmarks were moved at once, for example, during histedit?

stash: Why break here? What if a few bookmarks were moved at once, for example, during histedit?
felixmerkAuthorUnsubmitted
Not Done

Find next delta finds the next command that changed something within a limited scope ("localbranch"). At this point we don't care wether one bookmark or a million changed, we are looking for at least one thing (working copy parent, bookmark, draft commits) to have changed. Find next delta returns the index at which the next delta occurred.

felixmerk: Find next delta finds the next command that changed something within a limited scope…
stashUnsubmitted
Not Done

That makes sense, thank you for explanation. Probably worth adding a comment though

stash: That makes sense, thank you for explanation. Probably worth adding a comment though
break
# working copy parent changes
stashUnsubmitted
Not Done

Typo:

intereSting
stash: Typo: ``` intereSting ```
# for workingcopyparent, only changes within the scope are interesting
if alphaworkingcopyparent != workingcopyparent:
done = done or (workingcopyparent in hexnodes and
alphaworkingcopyparent in hexnodes)
stashUnsubmitted
Not Done

I've just tested it and looks like there is a difference between D160 and this diff.

Let's say we have a stack of commits

   o
    |
   @  - A (draft)
  /
o
|
o - B (public)

Then we checkout B, and then we run hg undo -b A. In D160 it would just update to A, but with current changes it will move backwards.

stash: I've just tested it and looks like there is a difference between D160 and this diff. Let's say…
felixmerkAuthorUnsubmitted
Not Done

Yes, this was intended. Moving the working copy parent from or to a branch doesn't really 'feel' like a change to this branch. Moving a working copy parent within a branch on the other hand does. This is an implementation detail (documented on line 729). I'm fine going either way with this.

felixmerk: Yes, this was intended. Moving the working copy parent from or to a branch doesn't really…
return incrementalindex
# hide and reveal commits # hide and reveal commits
def smarthide(repo, revhide, revshow, local=False): 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 use local to not hide revhides without corresponding revshows
# 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)
elif len(destinations) > 1:# split elif len(destinations) > 1: # split
hidecommits(repo, ctx, []) hidecommits(repo, ctx, [])
elif len(destinations) == 0: elif len(destinations) == 0:
if not local: 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')

tests/test-undo.t

bookmark: oldbook bookmark: oldbook
user: test user: test
date: Thu Jan 01 00:00:00 1970 +0000 date: Thu Jan 01 00:00:00 1970 +0000
summary: oldbranch summary: oldbranch
$ hg up 75f63379f12b $ hg up 75f63379f12b
7 files updated, 0 files merged, 1 files removed, 0 files unresolved 7 files updated, 0 files merged, 1 files removed, 0 files unresolved
(leaving bookmark newbook) (leaving bookmark newbook)
$ hg undo -b 75f63379f12b -n 6 3 changes within local scope: commit, book, update
$ hg undo -b 75f63379f12b
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg undo -b 75f63379f12b
0 files updated, 0 files merged, 1 files removed, 0 files unresolved
$ hg undo -b 75f63379f12b
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 -l 2 $ hg log -l 2
changeset: 20:805791ba4bcd changeset: 20:805791ba4bcd
bookmark: newbook bookmark: newbook
tag: tip tag: tip
parent: -1:000000000000 parent: -1:000000000000
user: test user: test
date: Thu Jan 01 00:00:00 1970 +0000 date: Thu Jan 01 00:00:00 1970 +0000
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
$ cat >> $HGRCPATH <<EOF $ cat >> $HGRCPATH <<EOF
> [extensions] > [extensions]
> rebase = ! > rebase = !
> EOF > EOF
Check local redo works
Simple test should be sufficient, as per usual, redo is an undo with some index
logic
$ hg redo -b 3532
0 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ 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
$ touch a9 && hg add a9 && hg ci -m a9
stashUnsubmitted
Done

It would be nice to run hg log -r . -T {node} to print the node

stash: It would be nice to run `hg log -r . -T {node}` to print the node
created new head
$ hg log -r . -T {node}
3ee6a6880888df9e48cdc568b5e835bd3087f8cb (no-eol)
$ hg undo -b 3532
0 files updated, 0 files merged, 1 files removed, 0 files unresolved
$ hg undo -b 3532
0 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg redo -b 75f6
0 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg redo -b 75f6
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg log -l 2
changeset: 22:3ee6a6880888
tag: tip
parent: 18:75f63379f12b
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: a9
changeset: 21:35324a911c0d
bookmark: newbook
parent: 18:75f63379f12b
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: newbranch
Check local undo with facebook style strip
$ cat >> $HGRCPATH <<EOF
> [extensions]
> strip =
> fbamend =
> EOF
$ hg strip 3ee6
0 files updated, 0 files merged, 1 files removed, 0 files unresolved
working directory now at 75f63379f12b
1 changesets pruned
$ hg undo -b 3532
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg log -r . -T {node}
3ee6a6880888df9e48cdc568b5e835bd3087f8cb (no-eol)
$ cat >> $HGRCPATH <<EOF
> [extensions]
> strip =!
> fbamend =!
> EOF