[MERGE] Use slots consistently in InventoryEntry hierarchy -- update
Jan Hudec
bulb at ucw.cz
Thu Jun 1 20:55:50 BST 2006
On Thu, Jun 01, 2006 at 08:28:06 +0200, Jan Hudec wrote:
> On Thu, Jun 01, 2006 at 13:33:43 +1000, Robert Collins wrote:
> > On Tue, 2006-05-30 at 23:18 +0200, Jan Hudec wrote:
> >
> >
> > > 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.
> >
> > +1 with one further change: the kind = lambda idiom is less clear than a
> > regular method based declaration:
> >
> > @property
> > def kind(self):
> > return 'symlink'
> >
> > Etc.
> >
> > unless there is a significant performance difference, please use that
> > form for properties.
> >
> > That said, why is this a property and not an attribute ? If its to avoid
> > each type having a kind field, then just have a 'kind' slot in the base
> > class, and add a 'kind=None' keyword parameter to the base __init__ :
> > that will allow kind to be an attribute, and remove the duplicate effort
> > of having a property in each class. It will also allow the base __init__
> > to check it is given a kind and assert if it wasn't.
>
> It was an attribute. But there is no point to have it as an instance
> attribute when it's only determined by the type. I made property to make
> sure it's read-only, but class attribute will probably do better.
>
> I will also add class attributes for the type specific attributes (eg.
> text_sha1 or executable) to make the interface less type-dependent and
> repost.
Ok. I have tried to update the patch to address this and Aaron's concerns.
The 'kind' is now a normal class attribute and so are all the other
attributes specific to some InventoryEntry subclass. So the serializers can
safely read all of them and get None for the unapplicable ones, so they are
kind-agnostic again (I actually reverted them to the old code except
replacing the 'something != None' constructs with proer 'something is not
None'.
Please review. The original version got +1, -0, +1. I am attaching both the
complete diff from mainline (ok, 3 revisions back at this point) and a diff
of just the updates. Please use
http://drak.ucw.cz/~bulb/bzr/bzr/inventory-slots for merging.
--
Jan 'Bulb' Hudec <bulb at ucw.cz>
-------------- next part --------------
=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py
+++ bzrlib/bzrdir.py
@@ -1603,7 +1603,13 @@
# 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
+ # Defining text_id for other kinds than text seems like an overkill
+ # to me, especially since it's only ever used for conversion from
+ # version 4 repositories, and del won't work on a class attribute as
+ # is the approach with all the other kind-specifc InventoryEntry
+ # attributes.
+ 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,15 @@
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']
+
+ # Attributes valid only for some of the subclass. This is a fastest way
+ # to allow unconditional read access for them.
+ children = None
+ executable = None
+ symlink_target = None
+ text_sha1 = None
+ text_size = None
def _add_text_to_weave(self, new_lines, parents, weave_store, transaction):
versionedfile = weave_store.get_weave_or_empty(self.file_id,
@@ -267,7 +273,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 +288,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."""
@@ -458,14 +460,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 +502,33 @@
class RootEntry(InventoryEntry):
+ __slots__ = ['children']
+
def _check(self, checker, rev_id, tree):
"""See InventoryEntry._check"""
+ kind = '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)
+ )
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 +537,11 @@
# others are added
return other
+ kind = '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 +560,16 @@
"""See InventoryEntry._put_on_disk."""
os.mkdir(fullpath)
+ def __eq__(self, other):
+ return (super(InventoryDirectory, self).__eq__(other)
+ and (self.children == other.children)
+ )
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 = '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 = '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,16 @@
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.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,12 +49,12 @@
e.set('file_id', ie.file_id)
e.set('kind', ie.kind)
- if ie.text_size != None:
+ if ie.text_size is not 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:
+ if v is not None:
e.set(f, v)
# to be conservative, we don't externalize the root pointers
=== modified file 'bzrlib/xml5.py'
--- bzrlib/xml5.py
+++ bzrlib/xml5.py
@@ -51,12 +51,12 @@
e.set('name', ie.name)
e.set('file_id', ie.file_id)
- if ie.text_size != None:
+ if ie.text_size is not None:
e.set('text_size', '%d' % ie.text_size)
for f in ['text_sha1', 'revision', 'symlink_target']:
v = getattr(ie, f)
- if v != None:
+ if v is not None:
e.set(f, v)
if ie.executable:
-------------- next part --------------
=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py
+++ bzrlib/bzrdir.py
@@ -1603,6 +1603,11 @@
# if this fails, its a ghost ?
assert old_revision in self.converted_revs
self.snapshot_ie(previous_entries, ie, w, rev_id)
+ # Defining text_id for other kinds than text seems like an overkill
+ # to me, especially since it's only ever used for conversion from
+ # version 4 repositories, and del won't work on a class attribute as
+ # is the approach with all the other kind-specifc InventoryEntry
+ # attributes.
if ie.kind == 'file':
del ie.text_id
assert getattr(ie, 'revision', None) is not None
=== modified file 'bzrlib/inventory.py'
--- bzrlib/inventory.py
+++ bzrlib/inventory.py
@@ -124,6 +124,14 @@
__slots__ = ['file_id', 'name', 'parent_id', 'revision']
+ # Attributes valid only for some of the subclass. This is a fastest way
+ # to allow unconditional read access for them.
+ children = None
+ executable = None
+ symlink_target = None
+ text_sha1 = None
+ text_size = None
+
def _add_text_to_weave(self, new_lines, parents, weave_store, transaction):
versionedfile = weave_store.get_weave_or_empty(self.file_id,
transaction)
@@ -318,7 +326,9 @@
raise BzrError("don't know how to export {%s} of kind %r" % (self.file_id, self.kind))
def sorted_children(self):
- return []
+ l = self.children.items()
+ l.sort()
+ return l
@staticmethod
def versionable_kind(kind):
@@ -497,7 +507,7 @@
def _check(self, checker, rev_id, tree):
"""See InventoryEntry._check"""
- kind = property(lambda self: 'root_directory')
+ kind = 'root_directory'
def __init__(self, file_id):
self.file_id = file_id
@@ -511,11 +521,6 @@
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."""
@@ -532,7 +537,7 @@
# others are added
return other
- kind = property(lambda self: 'directory')
+ kind = 'directory'
def __init__(self, file_id, name, parent_id):
super(InventoryDirectory, self).__init__(file_id, name, parent_id)
@@ -560,11 +565,6 @@
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."""
@@ -646,7 +646,7 @@
"""See InventoryEntry.has_text."""
return True
- kind = property(lambda self: 'file')
+ kind = 'file'
def __init__(self, file_id, name, parent_id):
super(InventoryFile, self).__init__(file_id, name, parent_id)
@@ -769,7 +769,7 @@
else:
print >>output_to, '=== target is %r' % self.symlink_target
- kind = property(lambda self: 'symlink')
+ kind = 'symlink'
def __init__(self, file_id, name, parent_id):
super(InventoryLink, self).__init__(file_id, name, parent_id)
=== modified file 'bzrlib/tests/test_inv.py'
--- bzrlib/tests/test_inv.py
+++ bzrlib/tests/test_inv.py
@@ -114,7 +114,6 @@
self.assertRaises(AttributeError, setattr, left, 'executable', True)
left.symlink_target='foo'
right = inventory.InventoryLink('123', 'hello.c', ROOT_ID)
- #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,18 +49,13 @@
e.set('file_id', ie.file_id)
e.set('kind', ie.kind)
- 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)
+ if ie.text_size is not None:
+ e.set('text_size', '%d' % ie.text_size)
+
+ for f in ['text_id', 'text_sha1', 'symlink_target']:
+ v = getattr(ie, f)
+ if v is not None:
+ e.set(f, v)
# 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, InventoryFile
+from bzrlib.inventory import ROOT_ID, Inventory, InventoryEntry
import bzrlib.inventory as inventory
from bzrlib.revision import Revision
from bzrlib.errors import BzrError
@@ -51,22 +51,16 @@
e.set('name', ie.name)
e.set('file_id', ie.file_id)
- 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)
+ if ie.text_size is not None:
+ e.set('text_size', '%d' % ie.text_size)
+
+ for f in ['text_sha1', 'revision', 'symlink_target']:
+ v = getattr(ie, f)
+ if v is not None:
+ e.set(f, v)
+
+ if ie.executable:
+ e.set('executable', 'yes')
# 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/20060601/75a8c849/attachment.pgp
More information about the bazaar
mailing list