[MERGE] Use slots consistently in InventoryEntry hierarchy -- regenerated against new head. Seeking another reviewer.

Jan Hudec bulb at ucw.cz
Tue May 30 22:18:50 BST 2006


On Wed, May 24, 2006 at 10:41:17 +0200, Jan Hudec wrote:
> On Mon, May 22, 2006 at 08:25:47 -0500, John Arbash Meinel wrote:
> > Jan Hudec wrote:
> > > Hello,
> > > 
> > > Here is an updated version of the patch. It is again pullable from
> > > http://drak.ucw.cz/~bulb/bzr/bzr/inventory-slots
> > > 
> > > This patch updates InventoryEntry and all descendants to consistenly use
> > > __slots__.
> > > 
> > >  * Only attributes that are applicable for each kind are defined for it.
> > >  * Attempt to set invalid attribute. eg. symlink_target for a file, raises
> > >    AttributeError.
> > >  * Tests now actually test those errors.
> > >  * RootEntry.__eq__ was updated to make a super(...).__eq__ call. The old
> > >    version /did/ work correctly, since no attributes except children may ever
> > >    differ, but this is more future-proof---proplist will need to be compared
> > >    when properties go in.
> > >  * Removed the useless assert from xml5. However I left the reading of
> > >    attributes in _pack_entry check ie.kind rather than doing try/except or
> > >    getattr, since it's more explicit at what attributes should be present
> > >    where.
> > >  * Converted the kind attribute to constant readonly property() so it can't
> > >    be changed by mistake.
> > >  * I have kept the text_id property, which is only used when converting
> > >    version 4 branches. Maybe that code can be removed, but I'd prefer doing
> > >    that as a separate merge.
> > > 
> > > Attaching the patch for review.
> > > 
> > 
> > +1 after re-reviewing it. Do we need another reviewer?
> 
> Sorry, I was busy with anothe things yesterday and did not get to mail.
> Thanks for review. Now I need one more reviewer.
> 
> I don't think it got too much out of date, but I'll try to regenerate it as
> soon as I get some hacking time.

I have regenerated it against new head now that the PatienceDiff passes
tests. The patch did not change except for line numbers and attaching blank
lines differently due to difflib/PatienceDiff.

It is still seeking another reviewer. The branch name quoted above is still
valid.

-- 
						 Jan 'Bulb' Hudec <bulb at ucw.cz>
-------------- next part --------------
=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	
+++ bzrlib/bzrdir.py	
@@ -1603,7 +1603,8 @@
                 # if this fails, its a ghost ?
                 assert old_revision in self.converted_revs 
         self.snapshot_ie(previous_entries, ie, w, rev_id)
-        del ie.text_id
+        if ie.kind == 'file':
+            del ie.text_id
         assert getattr(ie, 'revision', None) is not None
 
     def snapshot_ie(self, previous_revisions, ie, w, rev_id):

=== modified file 'bzrlib/inventory.py'
--- bzrlib/inventory.py	
+++ bzrlib/inventory.py	
@@ -122,9 +122,7 @@
     RENAMED = 'renamed'
     MODIFIED_AND_RENAMED = 'modified and renamed'
     
-    __slots__ = ['text_sha1', 'text_size', 'file_id', 'name', 'kind',
-                 'text_id', 'parent_id', 'children', 'executable', 
-                 'revision']
+    __slots__ = ['file_id', 'name', 'parent_id', 'revision']
 
     def _add_text_to_weave(self, new_lines, parents, weave_store, transaction):
         versionedfile = weave_store.get_weave_or_empty(self.file_id,
@@ -267,7 +265,7 @@
         """
         return False
 
-    def __init__(self, file_id, name, parent_id, text_id=None):
+    def __init__(self, file_id, name, parent_id):
         """Create an InventoryEntry
         
         The filename must be a single component, relative to the
@@ -282,18 +280,14 @@
         Traceback (most recent call last):
         InvalidEntryName: Invalid entry name: src/hello.c
         """
+        assert self.__class__ != InventoryEntry # Must be subclassed...
         assert isinstance(name, basestring), name
         if '/' in name or '\\' in name:
             raise InvalidEntryName(name=name)
-        self.executable = False
         self.revision = None
-        self.text_sha1 = None
-        self.text_size = None
         self.file_id = file_id
         self.name = name
-        self.text_id = text_id
         self.parent_id = parent_id
-        self.symlink_target = None
 
     def kind_character(self):
         """Return a short kind indicator useful for appending to names."""
@@ -324,9 +318,7 @@
         raise BzrError("don't know how to export {%s} of kind %r" % (self.file_id, self.kind))
 
     def sorted_children(self):
-        l = self.children.items()
-        l.sort()
-        return l
+        return []
 
     @staticmethod
     def versionable_kind(kind):
@@ -458,14 +450,9 @@
 
         return ((self.file_id == other.file_id)
                 and (self.name == other.name)
-                and (other.symlink_target == self.symlink_target)
-                and (self.text_sha1 == other.text_sha1)
-                and (self.text_size == other.text_size)
-                and (self.text_id == other.text_id)
                 and (self.parent_id == other.parent_id)
                 and (self.kind == other.kind)
                 and (self.revision == other.revision)
-                and (self.executable == other.executable)
                 )
 
     def __ne__(self, other):
@@ -505,32 +492,38 @@
 
 class RootEntry(InventoryEntry):
 
+    __slots__ = ['children']
+
     def _check(self, checker, rev_id, tree):
         """See InventoryEntry._check"""
 
+    kind = property(lambda self: 'root_directory')
+
     def __init__(self, file_id):
         self.file_id = file_id
         self.children = {}
-        self.kind = 'root_directory'
         self.parent_id = None
         self.name = u''
+        self.revision = None
 
     def __eq__(self, other):
-        if not isinstance(other, RootEntry):
-            return NotImplemented
-        
-        return (self.file_id == other.file_id) \
-               and (self.children == other.children)
+        return (super(RootEntry, self).__eq__(other)
+                and (self.children == other.children)
+                )
+
+    def sorted_children(self):
+        l = self.children.items()
+        l.sort()
+        return l
 
 
 class InventoryDirectory(InventoryEntry):
     """A directory in an inventory."""
 
+    __slots__ = ['children']
+
     def _check(self, checker, rev_id, tree):
         """See InventoryEntry._check"""
-        if self.text_sha1 != None or self.text_size != None or self.text_id != None:
-            raise BzrCheckError('directory {%s} has text in revision {%s}'
-                                % (self.file_id, rev_id))
 
     def copy(self):
         other = InventoryDirectory(self.file_id, self.name, self.parent_id)
@@ -539,10 +532,11 @@
         # others are added
         return other
 
+    kind = property(lambda self: 'directory')
+
     def __init__(self, file_id, name, parent_id):
         super(InventoryDirectory, self).__init__(file_id, name, parent_id)
         self.children = {}
-        self.kind = 'directory'
 
     def kind_character(self):
         """See InventoryEntry.kind_character."""
@@ -561,10 +555,21 @@
         """See InventoryEntry._put_on_disk."""
         os.mkdir(fullpath)
 
+    def __eq__(self, other):
+        return (super(InventoryDirectory, self).__eq__(other)
+                and (self.children == other.children)
+                )
+
+    def sorted_children(self):
+        l = self.children.items()
+        l.sort()
+        return l
 
 class InventoryFile(InventoryEntry):
     """A file in an inventory."""
 
+    __slots__ = ['text_sha1', 'text_size', 'text_id', 'executable']
+
     def _check(self, checker, tree_revision_id, tree):
         """See InventoryEntry._check"""
         t = (self.file_id, self.revision)
@@ -641,9 +646,14 @@
         """See InventoryEntry.has_text."""
         return True
 
+    kind = property(lambda self: 'file')
+
     def __init__(self, file_id, name, parent_id):
         super(InventoryFile, self).__init__(file_id, name, parent_id)
-        self.kind = 'file'
+        self.executable = False
+        self.text_sha1 = None
+        self.text_size = None
+        self.text_id = None
 
     def kind_character(self):
         """See InventoryEntry.kind_character."""
@@ -694,7 +704,6 @@
             self.text_sha1 = sha_strings(new_lines)
             self.text_size = sum(map(len, new_lines))
 
-
     def _unchanged(self, previous_ie):
         """See InventoryEntry._unchanged."""
         compatible = super(InventoryFile, self)._unchanged(previous_ie)
@@ -708,6 +717,14 @@
             compatible = False
         return compatible
 
+    def __eq__(self, other):
+        return (super(InventoryFile, self).__eq__(other)
+                and (self.text_sha1 == other.text_sha1)
+                and (self.text_size == other.text_size)
+                and (self.text_id == other.text_id)
+                and (self.executable == other.executable)
+                )
+
 
 class InventoryLink(InventoryEntry):
     """A file in an inventory."""
@@ -716,9 +733,6 @@
 
     def _check(self, checker, rev_id, tree):
         """See InventoryEntry._check"""
-        if self.text_sha1 != None or self.text_size != None or self.text_id != None:
-            raise BzrCheckError('symlink {%s} has text in revision {%s}'
-                    % (self.file_id, rev_id))
         if self.symlink_target == None:
             raise BzrCheckError('symlink {%s} has no target in revision {%s}'
                     % (self.file_id, rev_id))
@@ -755,9 +769,11 @@
             else:
                 print >>output_to, '=== target is %r' % self.symlink_target
 
+    kind = property(lambda self: 'symlink')
+
     def __init__(self, file_id, name, parent_id):
         super(InventoryLink, self).__init__(file_id, name, parent_id)
-        self.kind = 'symlink'
+        self.symlink_target = None
 
     def kind_character(self):
         """See InventoryEntry.kind_character."""
@@ -793,6 +809,11 @@
             compatible = False
         return compatible
 
+    def __eq__(self, other):
+        return (super(InventoryLink, self).__eq__(other)
+                and (self.symlink_target == other.symlink_target)
+                )
+
 
 class Inventory(object):
     """Inventory of versioned files in a tree.

=== modified file 'bzrlib/tests/test_inv.py'
--- bzrlib/tests/test_inv.py	
+++ bzrlib/tests/test_inv.py	
@@ -84,12 +84,12 @@
 
     def test_dir_detect_changes(self):
         left = inventory.InventoryDirectory('123', 'hello.c', ROOT_ID)
-        left.text_sha1 = 123
-        left.executable = True
-        left.symlink_target='foo'
+        self.assertRaises(AttributeError, setattr, left, 'text_sha1', 123)
+        self.assertRaises(AttributeError, setattr, left, 'executable', True)
+        self.assertRaises(AttributeError, setattr, left, 'symlink_target', 'foo')
         right = inventory.InventoryDirectory('123', 'hello.c', ROOT_ID)
-        right.text_sha1 = 321
-        right.symlink_target='bar'
+        self.assertRaises(AttributeError, setattr, right, 'text_sha1', 321)
+        self.assertRaises(AttributeError, setattr, right, 'symlink_target', 'bar')
         self.assertEqual((False, False), left.detect_changes(right))
         self.assertEqual((False, False), right.detect_changes(left))
 
@@ -104,16 +104,17 @@
         self.assertEqual((False, True), left.detect_changes(right))
         self.assertEqual((False, True), right.detect_changes(left))
         right.text_sha1 = 321
+        self.assertRaises(AttributeError, setattr, left, 'symlink_target', 123)
         self.assertEqual((True, True), left.detect_changes(right))
         self.assertEqual((True, True), right.detect_changes(left))
 
     def test_symlink_detect_changes(self):
         left = inventory.InventoryLink('123', 'hello.c', ROOT_ID)
-        left.text_sha1 = 123
-        left.executable = True
+        self.assertRaises(AttributeError, setattr, left, 'text_sha1', 123)
+        self.assertRaises(AttributeError, setattr, left, 'executable', True)
         left.symlink_target='foo'
         right = inventory.InventoryLink('123', 'hello.c', ROOT_ID)
-        right.text_sha1 = 321
+        #right.text_sha1 = 321
         right.symlink_target='foo'
         self.assertEqual((False, False), left.detect_changes(right))
         self.assertEqual((False, False), right.detect_changes(left))

=== modified file 'bzrlib/xml4.py'
--- bzrlib/xml4.py	
+++ bzrlib/xml4.py	
@@ -49,13 +49,18 @@
         e.set('file_id', ie.file_id)
         e.set('kind', ie.kind)
 
-        if ie.text_size != None:
-            e.set('text_size', '%d' % ie.text_size)
-
-        for f in ['text_id', 'text_sha1', 'symlink_target']:
-            v = getattr(ie, f)
-            if v != None:
-                e.set(f, v)
+        if ie.kind == 'file':
+            if ie.text_size != None:
+                e.set('text_size', '%d' % ie.text_size)
+
+            for f in ['text_id', 'text_sha1']:
+                v = getattr(ie, f)
+                if v != None:
+                    e.set(f, v)
+
+        elif ie.kind == 'symlink':
+            if ie.symlink_target != None:
+                e.set('symlink_target', ie.symlink_target)
 
         # to be conservative, we don't externalize the root pointers
         # for now, leaving them as null in the xml form.  in a future

=== modified file 'bzrlib/xml5.py'
--- bzrlib/xml5.py	
+++ bzrlib/xml5.py	
@@ -14,7 +14,7 @@
 
 
 from bzrlib.xml_serializer import ElementTree, SubElement, Element, Serializer
-from bzrlib.inventory import ROOT_ID, Inventory, InventoryEntry
+from bzrlib.inventory import ROOT_ID, Inventory, InventoryEntry, InventoryFile
 import bzrlib.inventory as inventory
 from bzrlib.revision import Revision        
 from bzrlib.errors import BzrError
@@ -51,16 +51,22 @@
         e.set('name', ie.name)
         e.set('file_id', ie.file_id)
 
-        if ie.text_size != None:
-            e.set('text_size', '%d' % ie.text_size)
-
-        for f in ['text_sha1', 'revision', 'symlink_target']:
-            v = getattr(ie, f)
-            if v != None:
-                e.set(f, v)
-
-        if ie.executable:
-            e.set('executable', 'yes')
+        if ie.kind == 'file':
+            if ie.text_size != None:
+                e.set('text_size', '%d' % ie.text_size)
+
+            if ie.text_sha1 != None:
+                e.set('text_sha1', ie.text_sha1)
+
+            if ie.executable:
+                e.set('executable', 'yes')
+
+        elif ie.kind == 'symlink':
+            if ie.symlink_target != None:
+                e.set('symlink_target', ie.symlink_target)
+
+        if ie.revision != None:
+            e.set('revision', ie.revision)
 
         # to be conservative, we don't externalize the root pointers
         # for now, leaving them as null in the xml form.  in a future

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060530/27ec509b/attachment.pgp 


More information about the bazaar mailing list