This is an archive of the discontinued Mercurial Phabricator instance.

Differential D108

undo: meaningful obs markers for undos
ClosedPublic

Authored by felixmerk on Jul 17 2017, 3:09 PM.
Tags
None
Subscribers
quark

Details

Reviewers
quark
Group Reviewers
Restricted Project
Commits
rFBHGX80f01f00d59a: undo: meaningful obs markers for undos
Summary

Makes obs markers for undos more meaningful. When an undo performs a strict one
to one change, the written obs marker reflects that and the user even gets a
helpful ui message in smartlog. One to many undos do not support such obs
markers (currently) because we can't tell splits and divergence apart easily.

Diff Detail

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

Event Timeline

felixmerk created this revision.Jul 17 2017, 3:09 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 17 2017, 3:09 PM
felixmerk updated this revision to Diff 318.Jul 19 2017, 5:31 PM
quark requested changes to this revision.Jul 19 2017, 6:57 PM
quark added a subscriber: quark.

Function looks good. But there are some easy wins here.

hgext3rd/undo.py
549–558

This is O(N * M) where N = len(related), M = len(destinations). Use set operations to make it something like O(M * log N):

related = set(obsutil.allprecursors(unfi.obsstore, [ctx.node()]))
related.update(obsutil.allsuccessors(unfi.obsstore, [ctx.node()]))
related.intersection_update(x.node() for x in showctxs)

(note: ctxs is more common than ctxts in the existing code base)

tests/test-undo.t
338

It's better to show more information about other nodes, like:

-T "{node|short} {if(undosuccessors, label('sl.undo', '(Undone as {join(undosuccessors% \'{shortest(undosuccessor, 6)}\', ', ')})'))}"

This will show where 1dafc0 is.

This revision now requires changes to proceed.Jul 19 2017, 6:57 PM
felixmerk edited edge metadata.Jul 19 2017, 8:45 PM
felixmerk updated this revision to Diff 326.
quark accepted this revision.Jul 19 2017, 8:46 PM
This revision is now accepted and ready to land.Jul 19 2017, 8:46 PM
Closed by commit rFBHGX80f01f00d59a: undo: meaningful obs markers for undos (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/smartlog.py (6 lines)
Mhgext3rd/undo.py (52 lines)
Mtests/test-smartlog.t (4 lines)
Mtests/test-undo.t (86 lines)

Diff 318

hgext3rd/smartlog.py

@templatekeyword('histeditsuccessors') @templatekeyword('histeditsuccessors')
def histeditsuccessors(repo, ctx, **args): def histeditsuccessors(repo, ctx, **args):
"""Return all of the node's successors created as a result of """Return all of the node's successors created as a result of
histedit histedit
""" """
asnodes = list(modifysuccessors(ctx, 'histedit')) asnodes = list(modifysuccessors(ctx, 'histedit'))
return templatekw.showlist('histeditsuccessor', asnodes, args) return templatekw.showlist('histeditsuccessor', asnodes, args)
@templatekeyword('undosuccessors')
def undosuccessors(repo, ctx, **args):
"""Return all of the node's successors created as a result of undo"""
asnodes = list(modifysuccessors(ctx, 'undo'))
return templatekw.showlist('undosuccessor', asnodes, args)
def successormarkers(ctx): def successormarkers(ctx):
for data in ctx.repo().obsstore.successors.get(ctx.node(), ()): for data in ctx.repo().obsstore.successors.get(ctx.node(), ()):
yield obsutil.marker(ctx.repo(), data) yield obsutil.marker(ctx.repo(), data)
def modifysuccessors(ctx, operation): def modifysuccessors(ctx, operation):
"""Return all of the node's successors which were created as a result """Return all of the node's successors which were created as a result
of a given modification operation""" of a given modification operation"""
for m in successormarkers(ctx): for m in successormarkers(ctx):

hgext3rd/undo.py

cmdutil, cmdutil,
dispatch, dispatch,
error, error,
extensions, extensions,
hg, hg,
localrepo, localrepo,
lock as lockmod, lock as lockmod,
obsolete, obsolete,
obsutil,
registrar, registrar,
revlog, revlog,
revset, revset,
revsetlang, revsetlang,
smartset, smartset,
transaction, transaction,
util, util,
) )
kv = mark.rsplit(" ", 1) kv = mark.rsplit(" ", 1)
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)
# 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) #revealcommits(repo, workingcopyparent)
hg.updatetotally(ui, repo, workingcopyparent, workingcopyparent, hg.updatetotally(ui, repo, workingcopyparent, workingcopyparent,
clean=False, updatecheck='abort') clean=False, updatecheck='abort')
else: else:
# keeps working copy files # keeps working copy files
curctx = repo['.']
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()
dirstate = repo.dirstate dirstate = repo.dirstate
diff = precctxmanifest.diff(wctxmanifest) diff = precctxmanifest.diff(wctxmanifest)
changedfiles.extend(diff.iterkeys()) changedfiles.extend(diff.iterkeys())
with dirstate.parentchange(): with dirstate.parentchange():
dirstate.rebuild(precnode, precctxmanifest, changedfiles) dirstate.rebuild(precnode, precctxmanifest, changedfiles)
# we want added and removed files to be shown # we want added and removed files to be shown
# properly, not with ? and ! prefixes # properly, not with ? and ! prefixes
for filename, data in diff.iteritems(): for filename, data in diff.iteritems():
if data[0][0] is None: if data[0][0] is None:
dirstate.add(filename) dirstate.add(filename)
if data[1][0] is None: if data[1][0] is None:
dirstate.remove(filename) dirstate.remove(filename)
obsolete.createmarkers(repo, [(curctx, (precctx,))])
# visible changesets # visible changesets
addedrevs = revsetlang.formatspec('olddraft(0) - olddraft(%d)', addedrevs = revsetlang.formatspec('olddraft(0) - olddraft(%d)',
reverseindex) reverseindex)
hidecommits(repo, addedrevs)
removedrevs = revsetlang.formatspec('olddraft(%d) - olddraft(0)', removedrevs = revsetlang.formatspec('olddraft(%d) - olddraft(0)',
reverseindex) reverseindex)
smarthide(repo, addedrevs, removedrevs)
revealcommits(repo, removedrevs) revealcommits(repo, removedrevs)
def _computerelative(repo, reverseindex): def _computerelative(repo, reverseindex):
# allows for relative undos using # allows for relative undos using
# redo.i storage # redo.i storage
try: try:
hexnode = repo.svfs.read("undolog/redo.i") hexnode = repo.svfs.read("undolog/redo.i")
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):
'''hides changecontexts and reveals some commits
def hidecommits(repo, rev): tries to connect related hides and shows with obs marker
ctxts = repo.set(rev) when reasonable and correct
for commit in ctxts: '''
obsolete.createmarkers(repo, [[commit,[]]]) hidectxts = repo.set(revhide)
showctxts = repo.set(revshow)
for ctx in hidectxts:
unfi = repo.unfiltered()
related = []
for i in obsutil.allprecursors(unfi.obsstore, [ctx.node()]):
related.append(i)
for i in obsutil.allsuccessors(unfi.obsstore, [ctx.node()]):
related.append(i)
destinations = []
for x in showctxts:
if x.node() in related:
destinations.append(repo[x.node()])
quarkUnsubmitted
Not Done

This is O(N * M) where N = len(related), M = len(destinations). Use set operations to make it something like O(M * log N):

related = set(obsutil.allprecursors(unfi.obsstore, [ctx.node()]))
related.update(obsutil.allsuccessors(unfi.obsstore, [ctx.node()]))
related.intersection_update(x.node() for x in showctxs)

(note: ctxs is more common than ctxts in the existing code base)

quark: This is `O(N * M)` where N = len(related), M = len(destinations). Use `set` operations to make…
# two primary objectives:
# 1. correct divergence/nondivergence
# 2. correct visibility of changesets for the user
# secondary objectives:
# 3. usefull ui message in hg sl: "Undone to"
# Design choices:
# 1-to-1 correspondence is easy
# 1-to-many correspondence is hard:
# it's either divergent A to B, A to C
# or split A to B,C
# because of undo we don't know which
# without complex logic
# Solution: provide helpfull ui message for
# common and easy case (1 to 1), use simplest
# correct solution for complex edge case
if len(destinations) == 1:
hidecommits(repo, ctx, destinations)
else:
hidecommits(repo, ctx, [])
def hidecommits(repo, curctx, precctxs):
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:
raise error.Abort(_('undo requires inhibit to work properly')) raise error.Abort(_('undo requires inhibit to work properly'))
else: else:
ctxts = repo.set(rev) ctxts = repo.set(rev)

tests/test-smartlog.t

-- --
splitsuccessors splitsuccessors
Return all of the node's successors created as a result of Return all of the node's successors created as a result of
split split
-- --
successorssets successorssets
Returns a string of sets of successors for a changectx Returns a string of sets of successors for a changectx
--
undosuccessors
Return all of the node's successors created as a result of
undo

tests/test-undo.t

$ cat >> $HGRCPATH <<EOF $ cat >> $HGRCPATH <<EOF
> [extensions] > [extensions]
> undo = $TESTDIR/../hgext3rd/undo.py > undo = $TESTDIR/../hgext3rd/undo.py
> inhibit=$TESTDIR/../hgext3rd/inhibit.py > inhibit=$TESTDIR/../hgext3rd/inhibit.py
> [undo] > [undo]
> _duringundologlock=1 > _duringundologlock=1
> [experimental] > [experimental]
> evolution=createmarkers > evolution=createmarkers
> [ui]
> interactive = true
> EOF > EOF
Build up a repo Build up a repo
$ hg init repo $ hg init repo
$ cd repo $ cd repo
Test data store Test data store
$ touch kfl1 && hg add kfl1 $ touch kfl1 && hg add kfl1
$ hg st $ hg st
A kfl1 A kfl1
$ hg commit --amend $ hg commit --amend
$ hg st $ hg st
$ hg undo --keep $ hg undo --keep
$ hg st $ hg st
A kfl1 A kfl1
$ hg commit --amend
hg undo informative obsmarkers
check 1 to 1 undos have informative obsmarker
check 1 to many undos (generally a redo of split or divergence) do not connect
the changesets with obsmarkers as we do not differentiate between split and
divergence cases in undo. The original split/divergence obsmarkers suffice for
checking split/divergence.
$ cat >> $HGRCPATH <<EOF
> [extensions]
> smartlog = $TESTDIR/../hgext3rd/smartlog.py
> tweakdefaults = $TESTDIR/../hgext3rd/tweakdefaults.py
> fbamend = $TESTDIR/../hgext3rd/fbamend/
> EOF
$ hg undo
0 files updated, 0 files merged, 1 files removed, 0 files unresolved
$ hg sl --all --hidden -T "{if(undosuccessors, node)}\n{if(undosuccessors, label('sl.undo', '(Undone as {join(undosuccessors% \'{shortest(undosuccessor, 6)}\', ', ')})'))}"
quarkUnsubmitted
Not Done

It's better to show more information about other nodes, like:

-T "{node|short} {if(undosuccessors, label('sl.undo', '(Undone as {join(undosuccessors% \'{shortest(undosuccessor, 6)}\', ', ')})'))}"

This will show where 1dafc0 is.

quark: It's better to show more information about other nodes, like: -T "{node|short} {if…
x
|
| x
|/
| x
|/
| x
|/
@
:
: x c9476255bc2a68672c844021397838ff4eeefcda
:/ (Undone as 1dafc0)
: x
:/
: o
:/
: x
: |
: x
:/
: o
:/
o
|
~
$ echo "a" >> newa && echo "b" >> newb && hg add newa newb && hg ci -m "newfiles"
$ hg split --quiet << EOF
> y
> y
> n
> y
> EOF
diff --git a/newa b/newa
new file mode 100644
examine changes to 'newa'? [Ynesfdaq?] y
@@ -0,0 +1,1 @@
+a
record change 1/2 to 'newa'? [Ynesfdaq?] y
diff --git a/newb b/newb
new file mode 100644
examine changes to 'newb'? [Ynesfdaq?] n
Done splitting? [yN] y
$ hg debugobsolete | tail -5
db3723da827c373768d500ab4e3a9c59a78314a6 0 {1dafc0b436123cab96f82a8e9e8d1d42c0301aaa} (Thu Jan 01 00:00:00 1970 +0000) {'user': 'test'}
c9476255bc2a68672c844021397838ff4eeefcda 1dafc0b436123cab96f82a8e9e8d1d42c0301aaa 0 (Thu Jan 01 00:00:04 1970 +0000) {'user': 'test'}
1dafc0b436123cab96f82a8e9e8d1d42c0301aaa c9476255bc2a68672c844021397838ff4eeefcda 0 (Thu Jan 01 00:00:05 1970 +0000) {'user': 'test'}
c9476255bc2a68672c844021397838ff4eeefcda 1dafc0b436123cab96f82a8e9e8d1d42c0301aaa 0 (Thu Jan 01 00:00:06 1970 +0000) {'operation': 'undo', 'user': 'test'}
f86734247df6db66a810e549cc938a72cd5c6d1a d0fdb9510dbf78c1a7e62c3e6628ff1f978f87ea 75f63379f12bf02d40fe7444587ad67be9ae81b8 0 (Thu Jan 01 00:00:00 1970 +0000) {'operation': 'split', 'user': 'test'}
$ hg undo
0 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg redo
0 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg debugobsolete | tail -5
d0fdb9510dbf78c1a7e62c3e6628ff1f978f87ea f86734247df6db66a810e549cc938a72cd5c6d1a 0 (Thu Jan 01 00:00:01 1970 +0000) {'operation': 'undo', 'user': 'test'}
75f63379f12bf02d40fe7444587ad67be9ae81b8 0 {d0fdb9510dbf78c1a7e62c3e6628ff1f978f87ea} (Thu Jan 01 00:00:01 1970 +0000) {'operation': 'undo', 'user': 'test'}
f86734247df6db66a810e549cc938a72cd5c6d1a 0 {1dafc0b436123cab96f82a8e9e8d1d42c0301aaa} (Thu Jan 01 00:00:02 1970 +0000) {'operation': 'undo', 'user': 'test'}
d0fdb9510dbf78c1a7e62c3e6628ff1f978f87ea d0fdb9510dbf78c1a7e62c3e6628ff1f978f87ea 0 (Thu Jan 01 00:00:01 1970 +0000) {'operation': 'revive', 'user': 'test'}
75f63379f12bf02d40fe7444587ad67be9ae81b8 75f63379f12bf02d40fe7444587ad67be9ae81b8 0 (Thu Jan 01 00:00:01 1970 +0000) {'operation': 'revive', 'user': 'test'}
$ cat >> $HGRCPATH <<EOF
> [extensions]
> smartlog =!
> tweakdefaults =!
> fbamend =!
> EOF