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

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