Comment 4 for bug 767212

On Thu, 2011-04-21 at 20:58 +0200, John Arbash Meinel wrote:
> On 04/21/2011 05:14 PM, Jelmer Vernooij wrote:
> > On Thu, 2011-04-21 at 17:04 +0200, John Arbash Meinel wrote:
> >> On 04/21/2011 03:18 PM, John A Meinel wrote:
> >>> Since the trees expose "iter_entries_by_dir()" why not give them an
> >>> _get_inventory method rather than all of these clumsy workarounds?
> >> Sending this to you directly, since otherwise it probably gets lost in
> >> the 100s of bug spam I just generated.
> > Thanks.
> >
> > all trees have iter_entries_by_dir(), but only some actually have an
> > inventory and the related methods (get_canonical_inventory_path,
> > _write_inventory, read_working_inventory, etc).
>
> But if you have iter_entries_by_dir, you would have:
>
> byid = dict((ie.file_id, ie) for _, ie in self.iter_entries_by_dir())
> inv = Inventory()
> inv._byid = byid
> inv.root = X
>
> Or something not to far removed from that.
Doesn't that require InventoryDirectory.children to be populated ?

A concern is also that if the foreign plugins provide .inventory it can
get used accidentally and cause performance to degrade unexpectedly.

> I don't think we need _write_inventory, read_working_inventory can just
> do the above, etc.
Perhaps we can compromise by just providing read_working_inventory()
rather than .inventory ?

> > A _get_inventory() method on tree would require a fair amount of extra
> > work for foreign trees. It would allow us to keep check_inventory_shape
> > vs check_tree_shape, but would not prevent any of the places where
> > TestNotApplicable is raised (all for _write_inventory,
> > get_canonical_inventory_path, etc).
> It seems you are adding a lot more places than just that, but I'll admit
> I'm not looking directly at the code.
I've proposed a seperate MP without the check_tree_shape() changes, this
should make it a bit easier to review.

Cheers,

Jelmer