[MERGE REVIEW] Use InventoryDirectory to represent tree root

[Robert Collins]

On Mon, 2006-08-07 at 14:32 -0400, Aaron Bentley wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Robert Collins wrote:
> 
> > ^^
> > This seems to be a place where giving the root entry a 'revision' value
> > - synthetic that is - would simplify the code.
> 
> Agreed, but I didn't do this, because I was trying to do one step at a time.

Sure. I note this because sometimes doing it one step at a time
highlights steps that should be done first.

> > @@ -1580,6 +1580,8 @@
> >                 rev_id)
> >          parent_invs = map(self._load_updated_inventory,
> > present_parents)
> >          for file_id in inv:
> > +            if inv.is_root(file_id):
> > +                continue
> > 
> > ^^ if we switched to iter_entries here, we would not need to call
> > is_root. 
> 
> Doesn't iter_entries now yield the root entry?

Yes, but its always first, so it can be skipped trivially. Processing
the tree in order may also help with performance by creating the files
on disk in the same order we'll access them for status etc.

> > This might be simpler and more efficient in a different style:
> > for instance:
> >     # will do more work than the minimum, but is trivial to write :)
> >     dir_cnt = len(work_inv.walkdirs()) - 1
> > or:
> >     #should be about the cheapest:
> >     dir_cnt = len(file_id for file_id in work_inv
> >         if work_inv[file_id].kind == 'directory') - 1
> > or:
> >     # bit more readable:
> >     dirs = [file_id for file_id in work_inv if work_inv[file_id].kind ==
> > 'directory']
> >     dir_cnt = len(dirs) - 1
> 
> These all assume that a tree has one root directory, which is not true
> in nested-trees, where this came from.

I dont quite follow. In nested trees AIUI there is still only one root
for a given tree. It might have had a different root in the past, but
right now there is only one node that has the path '' and type
InventoryDirectory. Nested child trees have type InventoryChildTree or
whatever - because they different from directories - they may not have
children, and they have a tree revision-id value that we need to access.

> > In inventory.py:
> > +    def is_root(self, file_id):
> > +        return self.root is not None and file_id == self.root.file_id
> > +
> >  
> > self.root is a property - its never None, so theres no need to check for
> > that.
> 
> It is sometimes None in nested-trees, where this came from.

I can understand that - but at this point we haven't gotten that part of
the code reviewed and agreed to. I think we're [you and I] still in
discussion about that. I'm not going -1 this bit of code here because
its basically harmless, but I would prefer the check for None not come
in until the root is allowed to be None.

> > We dont use textinv.py at all - perhaps we should delete it?
> > 
> > Thats the last use of is_root I see in the patch other than the tests
> > for it - it looks to me like we may not need it at all. What do you
> > think?
> 
> I think that it depends on whether the origin revision's tree has a root
> id or not.  However, these changes don't force us to take either path.

Then I'd prefer that we implement the change to use InventoryDirectory
to represent the tree root without having is_root at this point - if we
needed it when the origin revision's changes come in, we can add it
then.

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: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060808/e032b4e1/attachment.pgp