[MERGE] Delegate basis inventory calculation during commit to the CommitBuilder object.

Martin Pool mbp at canonical.com
Tue Oct 28 02:29:57 GMT 2008

Martin Pool has voted comment.
Status is now: Semi-approved
Thanks for the nice clear cover letter.

+    * New method ``bzrlib.repository.Repository.add_inventory_delta``
+      allows adding an inventory via an inventory delta, which can be
+      more efficient for some repository types. (Robert Collins)

I think this would be clearer as 'add_inventory_by_delta' (like
update_basis_by_delta) as it's not actually an inventory delta that 

+      delta. To enable this functionality call 

This name is odd; it sounds more like you're querying it than
setting it.  Why not just the imperative '.record_deletes()'?  But if 
point is to record an inventory delta, and recording deletes is just a
detail of doing it, why not '.record_inventory_delta()'?

In fact why not just set it as a constructor option, as the object is
constructed from the branch just above where you set it.

It looks like you always accumulate into the CommitBuilder.basis_delta,
even if recording_deletes has not been called, and so there's always
apparently a delta there but sometimes it's incorrect.  That seems
dangerous.  One option would be to just require that callers must supply
deletion information, and e.g. removing the api that old code used.
Alternatively, don't generate an inventory delta unless it's know to be

Will new code ever want to not pass deletion information?  Is this all
just for compatibility?

+        self.basis_delta = []

Please add a comment or :ivar: when you add instance variables, 
if they are public.


For details, see: 
Project: Bazaar

More information about the bazaar mailing list