[MERGE] Use slots consistently in InventoryEntry hierarchy
John A Meinel
john at arbash-meinel.com
Thu May 11 18:59:30 BST 2006
Jan Hudec wrote:
> Hello All,
>
> Earlier I complained about inconsistent usage of __slots__ in InventoryEntry
> (http://thread.gmane.org/gmane.comp.version-control.bazaar-ng.general/11220/)
> I have given it a look today and created a patch that adds __slots__ in all
> InventoryEntry subclasses. I have also moved the attributes to the classes
> for which they actually have a meaning (eg. no children in file). I have
> fixed the code so that all tests pass.
>
> The change can be merged from http://drak.ucw.cz/~bulb/bzr/bzr/inventory-slots
> Attaching a diff for review.
>
> Regards,
>
> Jan
>
>
>
> ------------------------------------------------------------------------
>
> === modified file 'bzrlib/bzrdir.py'
> --- bzrlib/bzrdir.py
> +++ bzrlib/bzrdir.py
> @@ -1575,7 +1575,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):
>
We haven't used 'text_id' in quite a while. Are there places that
actually use it? Otherwise, I would be in favor of just ignoring it.
> === modified file 'bzrlib/inventory.py'
> --- bzrlib/inventory.py
> +++ bzrlib/inventory.py
> @@ -114,9 +114,7 @@
> 'src/wibble/wibble.c'
> """
>
> - __slots__ = ['text_sha1', 'text_size', 'file_id', 'name', 'kind',
> - 'text_id', 'parent_id', 'children', 'executable',
> - 'revision']
> + __slots__ = ['file_id', 'name', 'kind', '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,
> @@ -259,7 +257,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
> @@ -274,18 +272,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."""
> @@ -316,9 +310,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):
> @@ -439,14 +431,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):
> @@ -486,6 +473,8 @@
>
> class RootEntry(InventoryEntry):
>
> + __slots__ = ['children']
> +
> def _check(self, checker, rev_id, tree):
> """See InventoryEntry._check"""
>
> @@ -503,15 +492,18 @@
> return (self.file_id == other.file_id) \
> and (self.children == other.children)
>
> + def sorted_children(self):
> + l = self.children.items()
> + l.sort()
> + return l
You need to update the __eq__ entry, so that it will check 'children'
when it is checking the rest of the stuff. Just like you did for
InventoryDirectory.
I could also see making Root a child of Directory, but I'm not settled
on that.
>
> 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)
> @@ -542,9 +534,20 @@
> """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"""
> @@ -625,6 +628,10 @@
> 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
Again, no need for text_id. Unless we have code that supports upgrading
the old old branch format into later formats. But since we haven't
supported writing the format for a long time, I would still argue that
we should strip out any support for it, now that we have had enough time
to deprecate it.
>
> def kind_character(self):
> """See InventoryEntry.kind_character."""
> @@ -675,7 +682,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)
> @@ -689,6 +695,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."""
> @@ -697,9 +711,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))
> @@ -739,6 +750,7 @@
> 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."""
> @@ -773,6 +785,11 @@
> if self.symlink_target != previous_ie.symlink_target:
> compatible = False
> return compatible
> +
> + def __eq__(self, other):
> + return (super(InventoryLink, self).__eq__(other)
> + and (self.symlink_target == other.symlink_target)
> + )
>
>
> class Inventory(object):
>
> === 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'
> + #left.text_sha1 = 123
> + #left.executable = True
> + #left.symlink_target='foo'
> right = inventory.InventoryDirectory('123', 'hello.c', ROOT_ID)
> - right.text_sha1 = 321
> - right.symlink_target='bar'
> + #right.text_sha1 = 321
> + #right.symlink_target='bar'
> self.assertEqual((False, False), left.detect_changes(right))
> self.assertEqual((False, False), right.detect_changes(left))
>
> @@ -109,11 +109,11 @@
>
> def test_symlink_detect_changes(self):
> left = inventory.InventoryLink('123', 'hello.c', ROOT_ID)
> - left.text_sha1 = 123
> - left.executable = True
> + #left.text_sha1 = 123
> + #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
> @@ -50,16 +50,23 @@
> 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':
> + assert isinstance(ie, InventoryFile)
I think we try to avoid assert isinstance, preferring to use duck-typing
as much as possible.
You could have also just changed the line to:
v = getattr(ie, f, None)
Since that will return None when the attribute doesn't exist.
> + 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
>
I like the work, just needs some refinements (or at least discussion).
John
=:->
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 249 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060511/8559665a/attachment.pgp
More information about the bazaar
mailing list