Patchwork [Bug,5256] New: inhibit callback from strip can crash in getnode()

login
register
mail settings
Submitter mercurial-bugs@selenic.com
Date May 30, 2016, 3:19 p.m.
Message ID <bug-5256-285@https.bz.mercurial-scm.org/>
Download mbox | patch
Permalink /patch/15264/
State Not Applicable
Headers show

Comments

mercurial-bugs@selenic.com - May 30, 2016, 3:19 p.m.
https://bz.mercurial-scm.org/show_bug.cgi?id=5256

            Bug ID: 5256
           Summary: inhibit callback from strip can crash in getnode()
           Product: Mercurial
           Version: unspecified
          Hardware: PC
                OS: Windows
            Status: UNCONFIRMED
          Severity: feature
          Priority: wish
         Component: strip
          Assignee: bugzilla@selenic.com
          Reporter: timeless@gmail.com
                CC: mercurial-devel@selenic.com

I'm using a slightly modified obsolete.py, and evolve's test-inhibit.t is
failing.

Here are the modifications which I believe contribute:
                 break

But while my changes may trigger this fault, I believe the fault lies in strip,
strip and the phase cache don't seem to play well enough together.

+    File "hgext/strip.py", line 75, in strip
+      repair.strip(ui, repo, revs, backup)
+    File "mercurial/repair.py", line 205, in strip
+      tr.close()
+    File "mercurial/transaction.py", line 44, in _active
+      return func(self, *args, **kwds)
+    File "mercurial/transaction.py", line 490, in close
+      self._postclosecallback[cat](self)
+    File "evolve-main/hgext/inhibit.py", line 208, in inhibitposttransaction
+      visibleobsolete = repo.revs('obsolete() - hidden()')
+    File "mercurial/localrepo.py", line 559, in revs
+      return m(self)
+    File "mercurial/revset.py", line 2341, in mfunc
+      result = getset(repo, subset, tree)
+    File "mercurial/revset.py", line 330, in getset
+      s = methods[x[0]](repo, subset, *x[1:])
+    File "mercurial/revset.py", line 395, in differenceset
+      return getset(repo, subset, x) - getset(repo, subset, y)
+    File "mercurial/revset.py", line 330, in getset
+      s = methods[x[0]](repo, subset, *x[1:])
+    File "mercurial/revset.py", line 418, in func
+      return symbols[a[1]](repo, subset, b)
+    File "mercurial/revset.py", line 1392, in obsolete
+      obsoletes = obsmod.getrevs(repo, 'obsolete')
+    File "mercurial/obsolete.py", line 1097, in getrevs
+      repo.obsstore.caches[name] = cachefuncs[name](repo)
+    File "evolve-main/hgext/inhibit.py", line 250, in _computeobsoleteset
+      obs = obsfunc(repo)
+    File "mercurial/obsolete.py", line 1125, in _computeobsoleteset
+      if getnode(r) in repo.obsstore.successors:
+    File "mercurial/changelog.py", line 337, in node
+      return super(changelog, self).node(rev)
+    File "mercurial/revlog.py", line 377, in node
+      return self.index[rev][7]
+  IndexError: revlog index out of range
+  [1]


@cachefor('obsolete')
def _computeobsoleteset(repo):
    """the set of obsolete revisions"""
    obs = set()
    getnode = repo.changelog.node
    notpublic = repo.revs("not public()")
    for r in notpublic:
        if getnode(r) in repo.obsstore.successors:
            obs.add(r)
    return obs

stepping through, at the notpublic assignment...

>>> repo.unfiltered().revs("all()")
<spanset+ 0:17>
>>> repo.unfiltered().revs("not public()")
<baseset+ [1, 2, 3, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19]> 

@predicate('_notpublic', safe=True)
def _notpublic(repo, subset, x):
    getargs(x, 0, 0, "_notpublic takes no arguments")
    repo._phasecache.loadphaserevs(repo) # ensure phase's sets are loaded
    if repo._phasecache._phasesets:
        s = set()
        for u in repo._phasecache._phasesets[1:]:
            s.update(u)
        s = baseset(s - repo.changelog.filteredrevs)
        s.sort()
        return subset & s
    else:
        phase = repo._phasecache.phase
        target = phases.public
        condition = lambda r: phase(repo, r) != target
        return subset.filter(condition, condrepr=('<phase %r>', target),
                             cache=False)

>>> repo._phasecache._phasesets[1:]
[set([1, 2, 3, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19]), set([])]

So, the phasecache "knows" about revisions that don't exist anymore.

We could adjust _notpublic and any similar friends who look directly at the
phasecache to limit what they talk about to things in all(), but I'm not sure
that's the right thing to do.

def strip(ui, repo, nodelist, backup=True, topic='backup'):
...
    repo = repo.unfiltered()
    repo.destroying() #<- first notification that bad things are happening
(phase cache flushes pending writes, but doesn't discard current state)
...
        lock = tr = None
        try:
            lock = repo.lock()
            tr = repo.transaction('repair')
            bm.recordchange(tr)
            tr.close() #<- crash in here
        finally:
            tr.release()
            lock.release()
...
    repo.destroyed() #<- phase cache is invalidated here

Patch

diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
--- a/mercurial/obsolete.py
+++ b/mercurial/obsolete.py
@@ -1167,11 +1167,13 @@  def _computebumpedset(repo):
         # We only evaluate mutable, non-obsolete revision
         node = ctx.node()
         # (future) A cache of precursors may worth if split is very common
-        for pnode in allprecursors(repo.obsstore, [node],
+        for pnode in allsuccessors(repo.obsstore,
+                        allprecursors(repo.obsstore, [node],
+                                      ignoreflags=bumpedfix),
                                    ignoreflags=bumpedfix):
             prev = torev(pnode) # unfiltered! but so is phasecache
             if (prev is not None) and (phase(repo, prev) <= public):
-                # we have a public precursors
+                # we have a public precursor
                 bumped.add(rev)
                 break # Next draft!
     return bumped
@@ -1194,7 +1196,10 @@  def _computedivergentset(repo):
             seen.add(prec)
             if prec not in newermap:
                 successorssets(repo, prec, newermap)
-            newer = [n for n in newermap[prec] if n]
+            newer = [n for n in newermap[prec]]
+            newer = [[x for x in n if not repo.set('public() & %ld' %
repo[x])]
+                     for n in newer]
+            newer = [n for n in newer if n]
             if len(newer) > 1:
                 divergent.add(ctx.rev())