bzrlib.tests.per_working_tree requires trees to have an inventory

[Jelmer Vernooij]

bzrlib.tests.per_working_tree requires trees to have an inventory. This makes it impossible to test hg, svn or git trees.

[John A Meinel (jameinel)]

Since the trees expose "iter_entries_by_dir()" why not give them an _get_inventory method rather than all of these clumsy workarounds?

[Jelmer Vernooij (jelmer)]

On Thu, 2011-04-21 at 17:04 +0200, John Arbash Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> 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).

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

Cheers,

Jelmer

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/21/2011 05:14 PM, Jelmer Vernooij wrote:
> On Thu, 2011-04-21 at 17:04 +0200, John Arbash Meinel wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> 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.

I don't think we need _write_inventory, read_working_inventory can just
do the above, etc.

>
> 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).
>
> Cheers,
>
> Jelmer

It seems you are adding a lot more places than just that, but I'll admit
I'm not looking directly at the code.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2wfmgACgkQJdeBCYSNAAM9OACcDNX8EkG9F1GaXNk7WKdbfFd4
MEIAoIqQVdNYc4qfW2padetzNpa2S+TZ
=IAY7
-----END PGP SIGNATURE-----

[Jelmer Vernooij (jelmer)]

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