[MERGE] Delegate basis inventory calculation during commit to the CommitBuilder object.
robertc at robertcollins.net
Tue Oct 28 04:32:18 GMT 2008
On Mon, 2008-10-27 at 22:29 -0400, Martin Pool wrote:
> 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.
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
> 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,
> if they are public.
Sure, I'll add one.
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20081028/adbaa4ac/attachment-0001.pgp
More information about the bazaar