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

[Martin Pool]

Martin Pool has voted comment.
Status is now: Semi-approved
Comment:
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 
we're
storing.

+      delta. To enable this functionality call 
``builder.recording_deletes``

This name is odd; it sounds more like you're querying it than
setting it.  Why not just the imperative '.record_deletes()'?  But if 
the
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
correct.

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, 
especially
if they are public.

--
Martin


For details, see: 
http://bundlebuggy.aaronbentley.com/project/bzr/request/%3C1223873271.9015.266.camel%40lifeless-64%3E
Project: Bazaar