# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1513906792 28800
# Node ID 67b28d657f622232b7339e4bb9654c8d3e387e20
# Parent  8410a978c6502edd079c377f1c270207f1e37457
sqliterevmap: break ".hashes()" cycle in a safer way

The `fromsvn()` revset implementation could cause weakref error when using
sqliterevmap like:

  File "hgsubversion/util.py", line 357, in <lambda>
    return subset.filter(lambda r: tonode(r) in hashes)
  File "hgsubversion/maps.py", line 542, in __contains__
    return self.get(key) != None
  File "hgsubversion/maps.py", line 533, in get
    for row in self.revmap._query(
  ReferenceError: weakly-referenced object no longer exists

Basically the seemingly harmless assignment could break surprisingly:

  # dangerous: `hashes` does not have a reference of `meta.revmap` and may
  # become unavailable after `meta`, `revmap` being released by refcount.
  hashes = meta.revmap.hashes()

The above syntax is nice to support while avoiding cycles is also nice.
This patch removes `revmap._hashes` so the revmap no longer owns a reference
of a `ReverseRevMap` object so the `ReverseRevMap` object no longer needs to
use weakref for `self.revmap`.

This could actually be caught by `comprehensive/test_sqlite_revmap.py`.
I was not careful enough verifying the "fromsvn()" patch.

diff --git a/hgsubversion/maps.py b/hgsubversion/maps.py
--- a/hgsubversion/maps.py
+++ b/hgsubversion/maps.py
@@ -523,9 +523,9 @@ class SqliteRevMap(collections.MutableMa
     class ReverseRevMap(object):
         # collections.Mapping is not suitable since we don't want 2/3 of
         # its required interfaces: __iter__, __len__.
-        def __init__(self, revmap):
-            self.revmap = weakref.proxy(revmap)
-            self._cache = {}
+        def __init__(self, revmap, cache):
+            self.revmap = revmap
+            self._cache = cache
 
         def get(self, key, default=None):
             if key not in self._cache:
@@ -565,18 +565,17 @@ class SqliteRevMap(collections.MutableMa
         self._lastpulledpath = lastpulled_path
 
         self._db = None
-        self._hashes = None
         self._sqlitepragmas = sqlitepragmas
         self.firstpulled = 0
         self._updatefirstlastpulled()
         # __iter__ is expensive and thus disabled by default
         # it should only be enabled for testing
         self._allowiter = False
+        # used by self.hashes(), {hash: (rev, branch)}
+        self._hashescache = {}
 
     def hashes(self):
-        if self._hashes is None:
-            self._hashes = self.ReverseRevMap(self)
-        return self._hashes
+        return self.ReverseRevMap(self, self._hashescache)
 
     def branchedits(self, branch, revnum):
         return [((r[0], r[1] or None), bytes(r[2])) for r in
@@ -609,7 +608,7 @@ class SqliteRevMap(collections.MutableMa
         hgutil.unlinkpath(self._dbpath, ignoremissing=True)
         hgutil.unlinkpath(self._rowcountpath, ignoremissing=True)
         self._db = None
-        self._hashes = None
+        self._hashescache = {}
         self._firstpull = None
         self._lastpull = None
 
@@ -646,15 +645,15 @@ class SqliteRevMap(collections.MutableMa
             self.firstpulled = revnum
         if revnum > self.lastpulled or not self.lastpulled:
             self.lastpulled = revnum
-        if self._hashes is not None:
-            self._hashes._cache[binha] = key
+        if self._hashescache:
+            self._hashescache[binha] = key
 
     def __delitem__(self, key):
         for row in self._querybykey('DELETE', key):
             if self.rowcount > 0:
                 self.rowcount -= 1
             return
-        # For performance reason, self._hashes is not updated
+        # For performance reason, self._hashescache is not updated
         raise KeyError(key)
 
     @contextlib.contextmanager