Merge lp:~spiv/bzr/fragile-do_merge-cleanup-517275-2.1 into lp:bzr/2.1

[Andrew Bennetts]

(andrew) Replace several fragile try/finally blocks in merge.py using bzrlib.cleanup. (#517275)

[Andrew Bennetts (spiv)]

This replaces a bunch of fragile try/finally blocks with more robust cleanups using bzrlib.cleanup.

Bug 517275 only pointed out one place causing trouble, but I took the liberty of improving most of the other try/finally blocks in the module while I was there (maybe tripling the diff, but it's still not a very large patch).

As with other patches improving cleanups, this one is largely mechanical. I also improved the docstring of the bzrlib.cleanup module slightly.

[Martin Pool (mbp)]

looks good

review: Approve

=== modified file 'NEWS'
--- NEWS	2010-02-03 14:08:55 +0000
+++ NEWS	2010-02-05 06:55:20 +0000
@@ -32,6 +32,15 @@
 * The new ``merge_file_content`` should now be ok with tests to avoid
   regressions.
   (Vincent Ladeuil, #515597)
+  
+Internals
+*********
+
+* Use ``bzrlib.cleanup`` rather than less robust ``try``/``finally``
+  blocks in several places in ``bzrlib.merge``.  This avoids masking prior
+  errors when errors like ``ImmortalPendingDeletion`` occur during cleanup
+  in ``do_merge``.
+  (Andrew Bennetts, #517275)
 
 bzr 2.1.0rc2
 ############

=== modified file 'bzrlib/cleanup.py'
--- bzrlib/cleanup.py	2010-01-13 23:16:20 +0000
+++ bzrlib/cleanup.py	2010-02-05 06:55:20 +0000
@@ -31,9 +31,9 @@
 If you want to be certain that the first, and only the first, error is raised,
 then use::
 
-    operation = OperationWithCleanups(lambda operation: do_something())
+    operation = OperationWithCleanups(do_something)
     operation.add_cleanup(cleanup_something)
-    operation.run()
+    operation.run_simple()
 
 This is more inconvenient (because you need to make every try block a
 function), but will ensure that the first error encountered is the one raised,

=== modified file 'bzrlib/merge.py'
--- bzrlib/merge.py	2010-02-01 17:29:44 +0000
+++ bzrlib/merge.py	2010-02-05 06:55:20 +0000
@@ -36,6 +36,7 @@
     ui,
     versionedfile
     )
+from bzrlib.cleanup import OperationWithCleanups
 from bzrlib.symbol_versioning import (
     deprecated_in,
     deprecated_method,
@@ -45,11 +46,10 @@
 
 def transform_tree(from_tree, to_tree, interesting_ids=None):
     from_tree.lock_tree_write()
-    try:
-        merge_inner(from_tree.branch, to_tree, from_tree, ignore_zero=True,
-                    interesting_ids=interesting_ids, this_tree=from_tree)
-    finally:
-        from_tree.unlock()
+    operation = OperationWithCleanups(merge_inner)
+    operation.add_cleanup(from_tree.unlock)
+    operation.run_simple(from_tree.branch, to_tree, from_tree,
+        ignore_zero=True, interesting_ids=interesting_ids, this_tree=from_tree)
 
 
 class MergeHooks(hooks.Hooks):
@@ -455,6 +455,7 @@
     def _add_parent(self):
         new_parents = self.this_tree.get_parent_ids() + [self.other_rev_id]
         new_parent_trees = []
+        operation = OperationWithCleanups(self.this_tree.set_parent_trees)
         for revision_id in new_parents:
             try:
                 tree = self.revision_tree(revision_id)
@@ -462,14 +463,9 @@
                 tree = None
             else:
                 tree.lock_read()
+                operation.add_cleanup(tree.unlock)
             new_parent_trees.append((revision_id, tree))
-        try:
-            self.this_tree.set_parent_trees(new_parent_trees,
-                                            allow_leftmost_as_ghost=True)
-        finally:
-            for _revision_id, tree in new_parent_trees:
-                if tree is not None:
-                    tree.unlock()
+        operation.run_simple(new_parent_trees, allow_leftmost_as_ghost=True)
 
     def set_other(self, other_revision, possible_transports=None):
         """Set the revision and tree to merge from.
@@ -626,7 +622,8 @@
                                change_reporter=self.change_reporter,
                                **kwargs)
 
-    def _do_merge_to(self, merge):
+    def _do_merge_to(self):
+        merge = self.make_merger()
         if self.other_branch is not None:
             self.other_branch.update_references(self.this_branch)
         merge.do_merge()
@@ -646,26 +643,19 @@
                     sub_tree.branch.repository.revision_tree(base_revision)
                 sub_merge.base_rev_id = base_revision
                 sub_merge.do_merge()
+        return merge
 
     def do_merge(self):
+        operation = OperationWithCleanups(self._do_merge_to)
         self.this_tree.lock_tree_write()
-        try:
-            if self.base_tree is not None:
-                self.base_tree.lock_read()
-            try:
-                if self.other_tree is not None:
-                    self.other_tree.lock_read()
-                try:
-                    merge = self.make_merger()
-                    self._do_merge_to(merge)
-                finally:
-                    if self.other_tree is not None:
-                        self.other_tree.unlock()
-            finally:
-                if self.base_tree is not None:
-                    self.base_tree.unlock()
-        finally:
-            self.this_tree.unlock()
+        operation.add_cleanup(self.this_tree.unlock)
+        if self.base_tree is not None:
+            self.base_tree.lock_read()
+            operation.add_cleanup(self.base_tree.unlock)
+        if self.other_tree is not None:
+            self.other_tree.lock_read()
+            operation.add_cleanup(self.other_tree.unlock)
+        merge = operation.run_simple()
         if len(merge.cooked_conflicts) == 0:
             if not self.ignore_zero and not trace.is_quiet():
                 trace.note("All changes applied successfully.")
@@ -765,41 +755,43 @@
             self.do_merge()
 
     def do_merge(self):
+        operation = OperationWithCleanups(self._do_merge)
+        operation.add_cleanup(self.pb.clear)
         self.this_tree.lock_tree_write()
+        operation.add_cleanup(self.this_tree.unlock)
         self.base_tree.lock_read()
+        operation.add_cleanup(self.base_tree.unlock)
         self.other_tree.lock_read()
+        operation.add_cleanup(self.other_tree.unlock)
+        operation.run()
+
+    def _do_merge(self, operation):
+        self.tt = transform.TreeTransform(self.this_tree, self.pb)
+        operation.add_cleanup(self.tt.finalize)
+        self.pp.next_phase()
+        self._compute_transform()
+        self.pp.next_phase()
+        results = self.tt.apply(no_conflicts=True)
+        self.write_modified(results)
         try:
-            self.tt = transform.TreeTransform(self.this_tree, self.pb)
-            try:
-                self.pp.next_phase()
-                self._compute_transform()
-                self.pp.next_phase()
-                results = self.tt.apply(no_conflicts=True)
-                self.write_modified(results)
-                try:
-                    self.this_tree.add_conflicts(self.cooked_conflicts)
-                except errors.UnsupportedOperation:
-                    pass
-            finally:
-                self.tt.finalize()
-        finally:
-            self.other_tree.unlock()
-            self.base_tree.unlock()
-            self.this_tree.unlock()
-            self.pb.clear()
+            self.this_tree.add_conflicts(self.cooked_conflicts)
+        except errors.UnsupportedOperation:
+            pass
 
     def make_preview_transform(self):
+        operation = OperationWithCleanups(self._make_preview_transform)
+        operation.add_cleanup(self.pb.clear)
         self.base_tree.lock_read()
+        operation.add_cleanup(self.base_tree.unlock)
         self.other_tree.lock_read()
+        operation.add_cleanup(self.other_tree.unlock)
+        return operation.run_simple()
+
+    def _make_preview_transform(self):
         self.tt = transform.TransformPreview(self.this_tree)
-        try:
-            self.pp.next_phase()
-            self._compute_transform()
-            self.pp.next_phase()
-        finally:
-            self.other_tree.unlock()
-            self.base_tree.unlock()
-            self.pb.clear()
+        self.pp.next_phase()
+        self._compute_transform()
+        self.pp.next_phase()
         return self.tt
 
     def _compute_transform(self):