Patchwork [7,of,7] histedit: use scmutil.cleanupnodes (BC)

login
register
mail settings
Submitter Jun Wu
Date July 8, 2017, 11:51 p.m.
Message ID <61d54ce7e0610fa42934.1499557895@x1c>
Download mbox | patch
Permalink /patch/22142/
State Accepted
Headers show

Comments

Jun Wu - July 8, 2017, 11:51 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1499557831 25200
#      Sat Jul 08 16:50:31 2017 -0700
# Node ID 61d54ce7e0610fa4293436ac07263c0133437481
# Parent  d75e65724d2bbcf17fcaaad705e463e84a3de5d2
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 61d54ce7e061
histedit: use scmutil.cleanupnodes (BC)

This is marked as BC because the strip backup file name has changed.
via Mercurial-devel - July 9, 2017, 7:04 p.m.
On Sat, Jul 8, 2017 at 4:51 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
> @@ -1182,5 +1182,9 @@ def _finishhistedit(ui, repo, state):
>          mapping[n] = ()
>
> -    safecleanupnode(ui, repo, mapping)
> +    # remove entries about unknown nodes
> +    nodemap = repo.unfiltered().changelog.nodemap
> +    mapping = {k: v for k, v in mapping.items()
> +               if k in nodemap and all(n in nodemap for n in v)}

I suppose these few lines could potentially move into cleanupnodes?
IIUC, it's to prevent crashing if you had stripped nodes during
histedit. The same can be done during rebase (while stopped to resolve
conflicts) and

> +    scmutil.cleanupnodes(repo, mapping, 'histedit')
>
>      state.clear()
Jun Wu - July 9, 2017, 7:21 p.m.
Excerpts from Martin von Zweigbergk's message of 2017-07-09 12:04:11 -0700:
> On Sat, Jul 8, 2017 at 4:51 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
> > @@ -1182,5 +1182,9 @@ def _finishhistedit(ui, repo, state):
> >          mapping[n] = ()
> >
> > -    safecleanupnode(ui, repo, mapping)
> > +    # remove entries about unknown nodes
> > +    nodemap = repo.unfiltered().changelog.nodemap
> > +    mapping = {k: v for k, v in mapping.items()
> > +               if k in nodemap and all(n in nodemap for n in v)}
> 
> I suppose these few lines could potentially move into cleanupnodes?
> IIUC, it's to prevent crashing if you had stripped nodes during
> histedit. The same can be done during rebase (while stopped to resolve
> conflicts) and

The current rebase and amend do not have the "remove unknown node" logic
(might be a bug for rebase, but amend does not seem to need this). Since
it's only useful in a subset of all cases, and commands like metaedit,
split, amend, etc do not need it, I think it's cleaner to require explicit
node removal from the callsite, so when the callsite made a mistake, we can
catch it instead of ignoring it silently.

> > +    scmutil.cleanupnodes(repo, mapping, 'histedit')
> >
> >      state.clear()
via Mercurial-devel - July 9, 2017, 7:54 p.m.
On Sun, Jul 9, 2017 at 12:21 PM, Jun Wu <quark@fb.com> wrote:
> Excerpts from Martin von Zweigbergk's message of 2017-07-09 12:04:11 -0700:
>> On Sat, Jul 8, 2017 at 4:51 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
>> > @@ -1182,5 +1182,9 @@ def _finishhistedit(ui, repo, state):
>> >          mapping[n] = ()
>> >
>> > -    safecleanupnode(ui, repo, mapping)
>> > +    # remove entries about unknown nodes
>> > +    nodemap = repo.unfiltered().changelog.nodemap
>> > +    mapping = {k: v for k, v in mapping.items()
>> > +               if k in nodemap and all(n in nodemap for n in v)}
>>
>> I suppose these few lines could potentially move into cleanupnodes?
>> IIUC, it's to prevent crashing if you had stripped nodes during
>> histedit. The same can be done during rebase (while stopped to resolve
>> conflicts) and
>
> The current rebase and amend do not have the "remove unknown node" logic
> (might be a bug for rebase, but amend does not seem to need this). Since
> it's only useful in a subset of all cases, and commands like metaedit,
> split, amend, etc do not need it, I think it's cleaner to require explicit
> node removal from the callsite, so when the callsite made a mistake, we can
> catch it instead of ignoring it silently.

I agree that amend and metaedit should not need it. I could be made an
option, since at least rebase and histedit seem to need it. Anyway, we
can always deal with that when someone files a bug report.

>
>> > +    scmutil.cleanupnodes(repo, mapping, 'histedit')
>> >
>> >      state.clear()
Jun Wu - July 9, 2017, 7:59 p.m.
Excerpts from Martin von Zweigbergk's message of 2017-07-09 12:54:50 -0700:
> I agree that amend and metaedit should not need it. I could be made an
> option, since at least rebase and histedit seem to need it. Anyway, we
> can always deal with that when someone files a bug report.

An option sounds good to me. I'll send a follow-up once the number of my
pending patches decreases a bit.

> >
> >> > +    scmutil.cleanupnodes(repo, mapping, 'histedit')
> >> >
> >> >      state.clear()

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -1174,5 +1174,5 @@  def _finishhistedit(ui, repo, state):
     if not state.keep:
         if mapping:
-            movebookmarks(ui, repo, mapping, state.topmost, ntm)
+            movetopmostbookmarks(repo, state.topmost, ntm)
             # TODO update mq state
     else:
@@ -1182,5 +1182,9 @@  def _finishhistedit(ui, repo, state):
         mapping[n] = ()
 
-    safecleanupnode(ui, repo, mapping)
+    # remove entries about unknown nodes
+    nodemap = repo.unfiltered().changelog.nodemap
+    mapping = {k: v for k, v in mapping.items()
+               if k in nodemap and all(n in nodemap for n in v)}
+    scmutil.cleanupnodes(repo, mapping, 'histedit')
 
     state.clear()
@@ -1562,36 +1566,4 @@  def movetopmostbookmarks(repo, oldtopmos
             marks.recordchange(tr)
 
-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
-        return
-    movetopmostbookmarks(repo, oldtopmost, newtopmost)
-    moves = []
-    for bk, old in sorted(repo._bookmarks.iteritems()):
-        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')
-            marks = repo._bookmarks
-            for mark, new in moves:
-                old = marks[mark]
-                marks[mark] = new
-            marks.recordchange(tr)
-            tr.close()
-        finally:
-            release(tr, lock)
-
 def cleanupnode(ui, repo, nodes):
     """strip a group of nodes from the repository
@@ -1611,32 +1583,4 @@  def cleanupnode(ui, repo, nodes):
             repair.strip(ui, repo, roots)
 
-def safecleanupnode(ui, repo, 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, 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,5 +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/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,5 +88,5 @@ 
   > pick 652413bf663e 5 f
   > EOF
-  saved backup bundle to $TESTTMP/r/.hg/strip-backup/96e494a2d553-45c027ab-backup.hg (glob)
+  saved backup bundle to $TESTTMP/r/.hg/strip-backup/96e494a2d553-45c027ab-histedit.hg (glob)
   $ hg log --graph
   @  changeset:   3:cacdfd884a93
@@ -142,5 +142,5 @@ 
   > pick 59d9f330561f 2 d
   > EOF
-  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,5 +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/b0f4233702ca-4cf5af69-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,5 +383,5 @@  dropped revision.
   HG: branch 'default'
   HG: changed file
-  saved backup bundle to $TESTTMP/fold-with-dropped/.hg/strip-backup/617f94f13c0f-3d69522c-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