[RFC] Inventory refactoring (brisbane)

Ian Clatworthy ian.clatworthy at internode.on.net
Mon Apr 6 09:03:57 BST 2009


This is the part of brisbane-core that I'm most concerned
about landing. Unlike nearly every other change in that
branch, this one can potentially break existing users on
existing formats and/or break plugins. Here's why ...

bzr.dev has one Inventory class with a bunch of methods.
brisbane-core has 2 Inventory classes: Inventory and
CHKInventory, both based on a new abstract class,
CommonInventory. Many of the methods in the old Inventory
class now live in CommonInventory so they are part of
the CHKInventory API. But a pile of methods aren't:

1. Unlike Inventories, CHKInventories are immutable so
   methods like add/remove_recursive_id/rename aren't
   permitted by design.

2. Several of the "read" methods on Inventory haven't been
   moved up to CommonInventory and haven't been re-implemented
   in CHKInventory yet.

Methods in the latter class are (from my notes a few weeks back):

* get_file_kind
* get_child
* get_idpath
* has_filename

Inventory also has custom versions of these and maybe CHKInventory
ought to have them as well?

* __eq__
* __ne__
* __hash__
* __repr__

So I think we need to either add get_file_kind, etc. to CommonInventory
and/or CHKInventory *or* deprecate them. Otherwise, we run the risk of
some code somewhere doing something like

  revtree.inventory.xxx()

and xxx() not existing because this is a CHK repo, not a pack one.

So, part of the problem here is that many of the "read" methods
on Inventory do *not* have test coverage yet. We need to add that
and then use test multiplication to ensure they work on both
Inventory and CHKInventory objects.

As this is arguably the critical path for merging brisbane-core
and I'm offline tomorrow for medical reasons, can someone else
please consider this problem, and if they agree with me, write
these missing tests ASAP?

Ian C.

PS: I'm happy with the code that is there and I've spent a lot of
time reviewing and tweaking it in recent weeks. The issue is the
stuff which is *not* there.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bris-inventory.diff
Type: text/x-diff
Size: 76552 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090406/65a965d4/attachment-0001.bin 


More information about the bazaar mailing list