[merge][rfc] small inventory apply_delta patch

Martin Pool mbp at canonical.com
Tue Nov 11 07:04:59 GMT 2008


This patch goes a bit towards making BranchBuilder work with
CHKInventory, but this particular small change is more conceptually
interesting than useful.

I have some concern that in changing the internals of Inventory we'll
hit some aliasing bugs, and history shows that they can be serious in
impact, intermittently hit, and somewhat hard to track down.  Python
is not well suited to using pure/immutable objects everywhere, but I
do think it's important we be very clear about how and when these
objects can be reused or mutated.

In this patch: MutableTree like most working trees has the concept of
also making available a cached copy of the parent trees, and these are
updated by commit through this method.  As you can see it gets the
inventory from the old basis, updates it, and stores it back.

I would say this is not a good place to use mutation of the inventory.
 After this is done, the mutated inventory is presumably still
referenced by the object referenced by basis, and it seems strange or
wrong to modify something held by that.  This is unlikely to be a
problem in practice because this base implementation is rarely used in
practice and anyhow it may be unlikely someone retains a reference to
that object.  But still, we want to move towards apis that are hard to
use wrongly and avoid relying on "probably that object's
unreferenced."  So I'd say in this case, constructing a new one is
better.

-- 
Martin <http://launchpad.net/~mbp/>

=== modified file 'bzrlib/mutabletree.py'
--- bzrlib/mutabletree.py	2008-10-03 23:42:56 +0000
+++ bzrlib/mutabletree.py	2008-11-11 06:44:03 +0000
@@ -516,10 +516,11 @@
         # WorkingTree classes for optimised versions for specific format trees.
         basis = self.basis_tree()
         basis.lock_read()
-        inventory = basis.inventory
-        basis.unlock()
-        inventory.apply_delta(delta)
-        rev_tree = RevisionTree(self.branch.repository, inventory, new_revid)
+        try:
+            new_inventory = basis.inventory.create_by_apply_delta(delta)
+        finally:
+            basis.unlock()
+        rev_tree = RevisionTree(self.branch.repository,
new_inventory, new_revid)
         self.set_parent_trees([(new_revid, rev_tree)])



More information about the bazaar mailing list