Rev 4810: (andrew) Replace several fragile try/finally blocks in merge.py using in file:///home/pqm/archives/thelove/bzr/2.1/

[Canonical.com Patch Queue Manager]

At file:///home/pqm/archives/thelove/bzr/2.1/

------------------------------------------------------------
revno: 4810 [merge]
revision-id: pqm at pqm.ubuntu.com-20100211014515-vj7wflpt4ke2qk2v
parent: pqm at pqm.ubuntu.com-20100210165740-sxhuygwhr3emarfo
parent: andrew.bennetts at canonical.com-20100205075803-35lnry4nifbnjoea
committer: Canonical.com Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: 2.1
timestamp: Thu 2010-02-11 01:45:15 +0000
message:
  (andrew) Replace several fragile try/finally blocks in merge.py using
  	bzrlib.cleanup. (#517275)
modified:
  NEWS                           NEWS-20050323055033-4e00b5db738777ff
  bzrlib/repofmt/pack_repo.py    pack_repo.py-20070813041115-gjv5ma7ktfqwsjgn-1
=== modified file 'NEWS'
--- a/NEWS	2010-02-03 14:08:55 +0000
+++ b/NEWS	2010-02-05 07:58:03 +0000
@@ -20,6 +20,13 @@
 * Fix "AttributeError in Inter1and2Helper" during fetch.
   (Martin Pool, #513432)
 
+* Ignore ``KeyError`` from ``remove_index`` during ``_abort_write_group``
+  in a pack repository, which can happen harmlessly if the abort occurs during
+  finishing the write group.  Also use ``bzrlib.cleanup`` so that any
+  other errors that occur while aborting the individual packs won't be
+  hidden by secondary failures when removing the corresponding indices.
+  (Andrew Bennetts, #423015)
+  
 * Using the ``bzrlib.chk_map`` module from within multiple threads at the
   same time was broken due to race conditions with a module level page
   cache. This shows up as a KeyError in the ``bzrlib.lru_cache`` code with
@@ -33,6 +40,14 @@
   regressions.
   (Vincent Ladeuil, #515597)
 
+API Changes
+***********
+
+* The ``remove_index`` method of
+  ``bzrlib.repofmt.pack_repo.AggregateIndex`` no longer takes a ``pack``
+  argument.  This argument was always ignored.
+  (Andrew Bennetts, #423015)
+
 bzr 2.1.0rc2
 ############
 

=== modified file 'bzrlib/repofmt/pack_repo.py'
--- a/bzrlib/repofmt/pack_repo.py	2010-01-21 21:13:09 +0000
+++ b/bzrlib/repofmt/pack_repo.py	2010-02-05 07:58:03 +0000
@@ -24,6 +24,7 @@
 
 from bzrlib import (
     chk_map,
+    cleanup,
     debug,
     graph,
     osutils,
@@ -646,11 +647,10 @@
         del self.combined_index._indices[:]
         self.add_callback = None
 
-    def remove_index(self, index, pack):
+    def remove_index(self, index):
         """Remove index from the indices used to answer queries.
 
         :param index: An index from the pack parameter.
-        :param pack: A Pack instance.
         """
         del self.index_to_pack[index]
         self.combined_index._indices.remove(index)
@@ -1840,14 +1840,22 @@
         self._remove_pack_indices(pack)
         self.packs.remove(pack)
 
-    def _remove_pack_indices(self, pack):
-        """Remove the indices for pack from the aggregated indices."""
-        self.revision_index.remove_index(pack.revision_index, pack)
-        self.inventory_index.remove_index(pack.inventory_index, pack)
-        self.text_index.remove_index(pack.text_index, pack)
-        self.signature_index.remove_index(pack.signature_index, pack)
-        if self.chk_index is not None:
-            self.chk_index.remove_index(pack.chk_index, pack)
+    def _remove_pack_indices(self, pack, ignore_missing=False):
+        """Remove the indices for pack from the aggregated indices.
+        
+        :param ignore_missing: Suppress KeyErrors from calling remove_index.
+        """
+        for index_type in Pack.index_definitions.keys():
+            attr_name = index_type + '_index'
+            aggregate_index = getattr(self, attr_name)
+            if aggregate_index is not None:
+                pack_index = getattr(pack, attr_name)
+                try:
+                    aggregate_index.remove_index(pack_index)
+                except KeyError:
+                    if ignore_missing:
+                        continue
+                    raise
 
     def reset(self):
         """Clear all cached data."""
@@ -2091,24 +2099,21 @@
         # FIXME: just drop the transient index.
         # forget what names there are
         if self._new_pack is not None:
-            try:
-                self._new_pack.abort()
-            finally:
-                # XXX: If we aborted while in the middle of finishing the write
-                # group, _remove_pack_indices can fail because the indexes are
-                # already gone.  If they're not there we shouldn't fail in this
-                # case.  -- mbp 20081113
-                self._remove_pack_indices(self._new_pack)
-                self._new_pack = None
+            operation = cleanup.OperationWithCleanups(self._new_pack.abort)
+            operation.add_cleanup(setattr, self, '_new_pack', None)
+            # If we aborted while in the middle of finishing the write
+            # group, _remove_pack_indices could fail because the indexes are
+            # already gone.  But they're not there we shouldn't fail in this
+            # case, so we pass ignore_missing=True.
+            operation.add_cleanup(self._remove_pack_indices, self._new_pack,
+                ignore_missing=True)
+            operation.run_simple()
         for resumed_pack in self._resumed_packs:
-            try:
-                resumed_pack.abort()
-            finally:
-                # See comment in previous finally block.
-                try:
-                    self._remove_pack_indices(resumed_pack)
-                except KeyError:
-                    pass
+            operation = cleanup.OperationWithCleanups(resumed_pack.abort)
+            # See comment in previous finally block.
+            operation.add_cleanup(self._remove_pack_indices, resumed_pack,
+                ignore_missing=True)
+            operation.run_simple()
         del self._resumed_packs[:]
 
     def _remove_resumed_pack_indices(self):