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

[Robert Collins]

On Mon, 2008-10-27 at 22:29 -0400, Martin Pool wrote:
> 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.

I agree.

> +      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.

I was preserving API compatibility, as there was no real reason to break
it. As far as the name goes, well - it's not recording a delta so much
as constructing one as it goes; I don't really like the current name,
but the ones you've proposed don't grab me either :(.

> 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?

Yes. Is it dangerous? I don't think so; we don't use the delta in the
CommitBuilder unless we've been told that we will know about deletes.

> +        self.basis_delta = []
> 
> Please add a comment or :ivar: when you add instance variables, 
> especially
> if they are public.

Sure, I'll add one.

-Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20081028/adbaa4ac/attachment-0001.pgp