Patchwork [6,of,6,V2] histedit: migrate to scmutil.cleanupnodes (BC)

login
register
mail settings
Submitter Jun Wu
Date July 8, 2017, 2:11 a.m.
Message ID <5844ab9ea01f0fec2a98.1499479889@x1c>
Download mbox | patch
Permalink /patch/22123/
State Superseded
Headers show

Comments

Jun Wu - July 8, 2017, 2:11 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1498515971 25200
#      Mon Jun 26 15:26:11 2017 -0700
# Node ID 5844ab9ea01f0fec2a9885bb03918d34b534d374
# Parent  8beac6dfe217dfabf51de371e04c024ad30406dd
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 5844ab9ea01f
histedit: migrate to scmutil.cleanupnodes (BC)

This is marked as BC because the strip backup file name has changed.

Aside from exciting code removals, there are some output changes explained
below:

  - "histedit: moving bookmarks X from Y to Z" no longer gets printed with
    --verbose. This is more consistent with other commands, like "commit"
    won't show bookmark movement message. The bookmark movement message is
    still available via --debug.
  - There is only one backup file per command invocation, instead of two.
    It is cleaner to just put all removed nodes in one file.
  - There are some debugobsolete ordering changes. That's just some
    implementation detail of cleanupnodes. It should not affect any end-user
    experience.
via Mercurial-devel - July 8, 2017, 9:13 p.m.
On Fri, Jul 7, 2017 at 7:11 PM, Jun Wu <quark@fb.com> wrote:
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -1542,44 +1549,19 @@ def processreplacement(state):
>      return final, tmpnodes, new, newtopmost
>
> -def movebookmarks(ui, repo, mapping, oldtopmost, newtopmost):
> -    """Move bookmark from old to newly created node"""
> -    if not mapping:
> -        # if nothing got rewritten there is not purpose for this function
> +def movebookmarks(repo, oldtopmost, newtopmost):
> +    """Move bookmark from oldtopmost to newly created topmost.
> +
> +    Most of the bookmark movement logic is handled by scmutil.cleanupnodes,
> +    only handle the special case here: topmost bookmark movement.
> +    """
> +    if not oldtopmost or not newtopmost:
>          return
> -    moves = []
> -    for bk, old in sorted(repo._bookmarks.iteritems()):
> -        if old == oldtopmost:
> -            # special case ensure bookmark stay on tip.
> -            #
> -            # This is arguably a feature and we may only want that for the
> -            # active bookmark. But the behavior is kept compatible with the old
> -            # version for now.
> -            moves.append((bk, newtopmost))
> -            continue
> -        base = old
> -        new = mapping.get(base, None)
> -        if new is None:
> -            continue
> -        while not new:
> -            # base is killed, trying with parent
> -            base = repo[base].p1().node()
> -            new = mapping.get(base, (base,))
> -            # nothing to move
> -        moves.append((bk, new[-1]))
> -    if moves:
> -        lock = tr = None
> -        try:
> -            lock = repo.lock()
> -            tr = repo.transaction('histedit')
> +    oldbmarks = repo.nodebookmarks(oldtopmost)
> +    if oldbmarks:
> +        with repo.transaction('histedit') as tr:
>              marks = repo._bookmarks
> -            for mark, new in moves:
> -                old = marks[mark]
> -                ui.note(_('histedit: moving bookmarks %s from %s to %s\n')
> -                        % (mark, node.short(old), node.short(new)))
> -                marks[mark] = new
> +            for name in oldbmarks:
> +                marks[name] = newtopmost
>              marks.recordchange(tr)
> -            tr.close()
> -        finally:
> -            release(tr, lock)

The locking was lost here. AFAICT, that has been unnecessary ever
since the transaction was introduced in 1168499e5266 (histedit: make
use of bookmarks.recordchange instead of bookmarks.write, 2015-11-20).
Removing the unnecessary locking and switching to the context manager
syntax seems unrelated to switching to cleanupnodes, so could you move
that out in the next version as well?

Btw, even with histedit.singletransaction=True, _finishhistedit()
(i.e. obsmarkers and bookmarks) are done outside of the transaction.
Maybe not intended? (I'm obviously not blaming that on your patch.)

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -1172,11 +1172,18 @@  def _finishhistedit(ui, repo, state):
                         ui.debug(m % node.short(n))
 
-    safecleanupnode(ui, repo, 'temp', tmpnodes)
-
     if not state.keep:
-        if mapping:
-            movebookmarks(ui, repo, mapping, state.topmost, ntm)
-            # TODO update mq state
-        safecleanupnode(ui, repo, 'replaced', mapping)
+        movebookmarks(repo, state.topmost, ntm)
+        toclean = mapping.copy()
+    else:
+        toclean = {}
+    # tmpnodes do not have successors, add them to "toclean" dict
+    for n in tmpnodes:
+        if n not in toclean:
+            toclean[n] = ()
+    # remove entries about unknown nodes
+    nodemap = repo.unfiltered().changelog.nodemap
+    toclean = {k: v for k, v in toclean.items()
+               if k in nodemap and all(n in nodemap for n in v)}
+    scmutil.cleanupnodes(repo, toclean, 'histedit')
 
     state.clear()
@@ -1542,44 +1549,19 @@  def processreplacement(state):
     return final, tmpnodes, new, newtopmost
 
-def movebookmarks(ui, repo, mapping, oldtopmost, newtopmost):
-    """Move bookmark from old to newly created node"""
-    if not mapping:
-        # if nothing got rewritten there is not purpose for this function
+def movebookmarks(repo, oldtopmost, newtopmost):
+    """Move bookmark from oldtopmost to newly created topmost.
+
+    Most of the bookmark movement logic is handled by scmutil.cleanupnodes,
+    only handle the special case here: topmost bookmark movement.
+    """
+    if not oldtopmost or not newtopmost:
         return
-    moves = []
-    for bk, old in sorted(repo._bookmarks.iteritems()):
-        if old == oldtopmost:
-            # special case ensure bookmark stay on tip.
-            #
-            # This is arguably a feature and we may only want that for the
-            # active bookmark. But the behavior is kept compatible with the old
-            # version for now.
-            moves.append((bk, newtopmost))
-            continue
-        base = old
-        new = mapping.get(base, None)
-        if new is None:
-            continue
-        while not new:
-            # base is killed, trying with parent
-            base = repo[base].p1().node()
-            new = mapping.get(base, (base,))
-            # nothing to move
-        moves.append((bk, new[-1]))
-    if moves:
-        lock = tr = None
-        try:
-            lock = repo.lock()
-            tr = repo.transaction('histedit')
+    oldbmarks = repo.nodebookmarks(oldtopmost)
+    if oldbmarks:
+        with repo.transaction('histedit') as tr:
             marks = repo._bookmarks
-            for mark, new in moves:
-                old = marks[mark]
-                ui.note(_('histedit: moving bookmarks %s from %s to %s\n')
-                        % (mark, node.short(old), node.short(new)))
-                marks[mark] = new
+            for name in oldbmarks:
+                marks[name] = newtopmost
             marks.recordchange(tr)
-            tr.close()
-        finally:
-            release(tr, lock)
 
 def cleanupnode(ui, repo, name, nodes):
@@ -1605,32 +1587,4 @@  def cleanupnode(ui, repo, name, nodes):
             repair.strip(ui, repo, c)
 
-def safecleanupnode(ui, repo, name, nodes):
-    """strip or obsolete nodes
-
-    nodes could be either a set or dict which maps to replacements.
-    nodes could be unknown (outside the repo).
-    """
-    supportsmarkers = obsolete.isenabled(repo, obsolete.createmarkersopt)
-    if supportsmarkers:
-        if util.safehasattr(nodes, 'get'):
-            # nodes is a dict-like mapping
-            # use unfiltered repo for successors in case they are hidden
-            urepo = repo.unfiltered()
-            def getmarker(prec):
-                succs = tuple(urepo[n] for n in nodes.get(prec, ()))
-                return (repo[prec], succs)
-        else:
-            # nodes is a set-like
-            def getmarker(prec):
-                return (repo[prec], ())
-        # sort by revision number because it sound "right"
-        sortednodes = sorted([n for n in nodes if n in repo],
-                             key=repo.changelog.rev)
-        markers = [getmarker(t) for t in sortednodes]
-        if markers:
-            obsolete.createmarkers(repo, markers, operation='histedit')
-    else:
-        return cleanupnode(ui, repo, name, nodes)
-
 def stripwrapper(orig, ui, repo, nodelist, *args, **kwargs):
     if isinstance(nodelist, str):
diff --git a/tests/test-histedit-arguments.t b/tests/test-histedit-arguments.t
--- a/tests/test-histedit-arguments.t
+++ b/tests/test-histedit-arguments.t
@@ -148,5 +148,5 @@  temporarily.
 
   $ hg histedit --continue
-  saved backup bundle to $TESTTMP/foo/.hg/strip-backup/08d98a8350f3-02594089-backup.hg (glob)
+  saved backup bundle to $TESTTMP/foo/.hg/strip-backup/08d98a8350f3-02594089-histedit.hg (glob)
   $ hg log -G -T '{rev} {shortest(node)} {desc}\n' -r 2::
   @  4 f5ed five
@@ -158,5 +158,5 @@  temporarily.
   ~
 
-  $ hg unbundle -q $TESTTMP/foo/.hg/strip-backup/08d98a8350f3-02594089-backup.hg
+  $ hg unbundle -q $TESTTMP/foo/.hg/strip-backup/08d98a8350f3-02594089-histedit.hg
   $ hg strip -q -r f5ed --config extensions.strip=
   $ hg up -q 08d98a8350f3
@@ -265,6 +265,5 @@  short hash. This tests issue3893.
   HG: branch 'default'
   HG: changed alpha
-  saved backup bundle to $TESTTMP/foo/.hg/strip-backup/*-backup.hg (glob)
-  saved backup bundle to $TESTTMP/foo/.hg/strip-backup/*-backup.hg (glob)
+  saved backup bundle to $TESTTMP/foo/.hg/strip-backup/c8e68270e35a-63d8b8d8-histedit.hg (glob)
 
   $ hg update -q 2
diff --git a/tests/test-histedit-bookmark-motion.t b/tests/test-histedit-bookmark-motion.t
--- a/tests/test-histedit-bookmark-motion.t
+++ b/tests/test-histedit-bookmark-motion.t
@@ -88,12 +88,5 @@ 
   > pick 652413bf663e 5 f
   > EOF
-  saved backup bundle to $TESTTMP/r/.hg/strip-backup/96e494a2d553-3c6c5d92-backup.hg (glob)
-  histedit: moving bookmarks also-two from 177f92b77385 to b346ab9a313d
-  histedit: moving bookmarks five from 652413bf663e to cacdfd884a93
-  histedit: moving bookmarks four from e860deea161a to 59d9f330561f
-  histedit: moving bookmarks three from 055a42cdd887 to 59d9f330561f
-  histedit: moving bookmarks two from 177f92b77385 to b346ab9a313d
-  histedit: moving bookmarks will-move-backwards from d2ae7f538514 to cb9a9f314b8b
-  saved backup bundle to $TESTTMP/r/.hg/strip-backup/d2ae7f538514-48787b8d-backup.hg (glob)
+  saved backup bundle to $TESTTMP/r/.hg/strip-backup/96e494a2d553-45c027ab-histedit.hg (glob)
   $ hg log --graph
   @  changeset:   3:cacdfd884a93
@@ -149,8 +142,5 @@ 
   > pick 59d9f330561f 2 d
   > EOF
-  histedit: moving bookmarks five from cacdfd884a93 to c04e50810e4b
-  histedit: moving bookmarks four from 59d9f330561f to c04e50810e4b
-  histedit: moving bookmarks three from 59d9f330561f to c04e50810e4b
-  saved backup bundle to $TESTTMP/r/.hg/strip-backup/59d9f330561f-073008af-backup.hg (glob)
+  saved backup bundle to $TESTTMP/r/.hg/strip-backup/59d9f330561f-073008af-histedit.hg (glob)
 
 We expect 'five' to stay at tip, since the tipmost bookmark is most
diff --git a/tests/test-histedit-commute.t b/tests/test-histedit-commute.t
--- a/tests/test-histedit-commute.t
+++ b/tests/test-histedit-commute.t
@@ -418,6 +418,5 @@  Now, let's try to fold the second commit
 
   $ HGEDITOR="sh ./editor.sh" hg histedit 0
-  saved backup bundle to $TESTTMP/issue4251/.hg/strip-backup/*-backup.hg (glob)
-  saved backup bundle to $TESTTMP/issue4251/.hg/strip-backup/*-backup.hg (glob)
+  saved backup bundle to $TESTTMP/issue4251/.hg/strip-backup/*-histedit.hg (glob)
 
   $ hg --config diff.git=yes export 0
diff --git a/tests/test-histedit-edit.t b/tests/test-histedit-edit.t
--- a/tests/test-histedit-edit.t
+++ b/tests/test-histedit-edit.t
@@ -274,5 +274,5 @@  check histedit_source
   HG: branch 'default'
   HG: added f
-  saved backup bundle to $TESTTMP/r/.hg/strip-backup/b5f70786f9b0-c28d9c86-backup.hg (glob)
+  saved backup bundle to $TESTTMP/r/.hg/strip-backup/b5f70786f9b0-c28d9c86-histedit.hg (glob)
 
   $ hg status
@@ -438,5 +438,5 @@  rollback should not work after a histedi
   [1]
   $ HGEDITOR=true hg histedit --continue
-  saved backup bundle to $TESTTMP/r0/.hg/strip-backup/cb9a9f314b8b-cc5ccb0b-backup.hg (glob)
+  saved backup bundle to $TESTTMP/r0/.hg/strip-backup/cb9a9f314b8b-cc5ccb0b-histedit.hg (glob)
 
   $ hg log -G
diff --git a/tests/test-histedit-fold.t b/tests/test-histedit-fold.t
--- a/tests/test-histedit-fold.t
+++ b/tests/test-histedit-fold.t
@@ -306,5 +306,5 @@  should effectively drop the changes from
   $ hg histedit --continue
   251d831eeec5: empty changeset
-  saved backup bundle to $TESTTMP/*-backup.hg (glob)
+  saved backup bundle to $TESTTMP/fold-to-empty-test/.hg/strip-backup/888f9082bf99-daa0b8b3-histedit.hg (glob)
   $ hg logt --graph
   @  1:617f94f13c0f +4
@@ -383,6 +383,5 @@  dropped revision.
   HG: branch 'default'
   HG: changed file
-  saved backup bundle to $TESTTMP/fold-with-dropped/.hg/strip-backup/55c8d8dc79ce-4066cd98-backup.hg (glob)
-  saved backup bundle to $TESTTMP/fold-with-dropped/.hg/strip-backup/617f94f13c0f-a35700fc-backup.hg (glob)
+  saved backup bundle to $TESTTMP/fold-with-dropped/.hg/strip-backup/617f94f13c0f-3d69522c-histedit.hg (glob)
   $ hg logt -G
   @  1:10c647b2cdd5 +4
diff --git a/tests/test-histedit-obsolete.t b/tests/test-histedit-obsolete.t
--- a/tests/test-histedit-obsolete.t
+++ b/tests/test-histedit-obsolete.t
@@ -171,6 +171,4 @@  Base setup for the rest of the testing
   
   $ hg debugobsolete
-  96e494a2d553dd05902ba1cee1d94d4cb7b8faed 0 {b346ab9a313db8537ecf96fca3ca3ca984ef3bd7} (*) {'user': 'test'} (glob)
-  b558abc46d09c30f57ac31e85a8a3d64d2e906e4 0 {96e494a2d553dd05902ba1cee1d94d4cb7b8faed} (*) {'user': 'test'} (glob)
   d2ae7f538514cd87c17547b0de4cea71fe1af9fb 0 {cb9a9f314b8b07ba71012fcdbc544b5a4d82ff5b} (*) {'user': 'test'} (glob)
   177f92b773850b59254aa5e923436f921b55483b b346ab9a313db8537ecf96fca3ca3ca984ef3bd7 0 (*) {'user': 'test'} (glob)
@@ -178,4 +176,6 @@  Base setup for the rest of the testing
   e860deea161a2f77de56603b340ebbb4536308ae 59d9f330561fd6c88b1a6b32f0e45034d88db784 0 (*) {'user': 'test'} (glob)
   652413bf663ef2a641cab26574e46d5f5a64a55a cacdfd884a9321ec4e1de275ef3949fa953a1f83 0 (*) {'user': 'test'} (glob)
+  96e494a2d553dd05902ba1cee1d94d4cb7b8faed 0 {b346ab9a313db8537ecf96fca3ca3ca984ef3bd7} (*) {'user': 'test'} (glob)
+  b558abc46d09c30f57ac31e85a8a3d64d2e906e4 0 {96e494a2d553dd05902ba1cee1d94d4cb7b8faed} (*) {'user': 'test'} (glob)