[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