[MERGE] Packs. Kthxbye.
Ian Clatworthy
ian.clatworthy at internode.on.net
Wed Oct 17 14:44:50 BST 2007
Robert Collins wrote:
> I think the moment has come to get the pack repository format reviewed.
A big congratulations Robert. Great to see this up for review.
I'm planning to tackle this in pieces. The first bit of feedback is below.
bb:comment
Most of the code I've read so far in this patch looks fine.
> +class RepositoryFormatPack(MetaDirRepositoryFormat):
> + """Format logic for pack structured repositories.
> +
> + This repository format has:
> + - a list of packs in pack-names
> + - packs in packs/NAME.pack
> + - indices in indices/NAME.{iix,six,tix,rix}
> + - knit deltas in the packs, knit indices mapped to the indices.
Hooray!
> +class RepositoryFormatGraphKnit1(RepositoryFormatPack):
> + """Experimental pack based repository with knit1 style data.
> +
> + This repository format has:
> + - knits for file texts and inventory
> + - hash subdirectory based stores.
> + - knits for revisions and signatures
> + - uses a GraphKnitIndex for revisions.knit.
> + - TextStores for revisions and signatures.
This needs updating - it's a copy directly from RepositoryFormatKnit1.
You want to copy-and-paste from RepositoryFormatPack instead I think.
> + def __ne__(self, other):
> + return self.__class__ is not other.__class__
While this method is directly copied out of RepositoryFormatKnit1, I
don't like it. Wouldn't it to be better to:
1. Move it up the class hierarchy to RepositoryFormatPack so that it
applies to RepositoryFormatGraphKit3 as well as this class?
2. Use the normal idiom: define an __eq__ method and then
implement the __ne__ method in terms of it?
> +class RepositoryFormatGraphKnit3(RepositoryFormatPack):
> + """Experimental repository with knit3 style data.
> +
> + This repository format has:
> + - knits for file texts and inventory
> + - hash subdirectory based stores.
> + - knits for revisions and signatures
> + - uses a GraphKnitIndex for revisions.knit.
> + - TextStores for revisions and signatures.
This needs updating - it's a copy directly from RepositoryFormatKnit3.
You want to copy-and-paste from RepositoryFormatPack instead I think.
> + def get_format_string(self):
> + """See RepositoryFormat.get_format_string()."""
> + return "Bazaar Experimental subtrees\n"
> +
> + def get_format_description(self):
> + """See RepositoryFormat.get_format_description()."""
> + return "Experimental no-subtrees\n"
>
Nope - you mean "Experimental subtrees\n" in the description. :-)
> === modified file 'bzrlib/index.py'
> --- bzrlib/index.py 2007-10-15 07:56:04 +0000
> +++ bzrlib/index.py 2007-10-17 09:39:41 +0000
> @@ -267,6 +267,17 @@
> self._nodes_by_key = None
> self._size = size
>
> + def __eq__(self, other):
> + """Equal when self and otherwere created with the same parameters."""
A space is missing between other and were.
> === modified file 'bzrlib/knit.py'
> --- bzrlib/knit.py 2007-10-12 05:26:46 +0000
> +++ bzrlib/knit.py 2007-10-17 09:39:41 +0000
> @@ -1950,7 +1950,8 @@
>
> def set_writer(self, writer, index, (transport, packname)):
> """Set a writer to use for adding data."""
> - self.indices[index] = (transport, packname)
> + if index is not None:
> + self.indices[index] = (transport, packname)
> self.container_writer = writer
> self.write_index = index
So set_writer can now be called with an index of None? I probably ought
to run annotate and see which commit in your branch made this change and
why. I'm flagging it here in case it prompts Martin or you to say "That
was a temporary change." If it's here to stay, perhaps a comment
wouldn't hurt explaining when it makes sense to call set_writer without
an index. (If the index can really be None, I'll go through and check
the lookups of self.indices to confirm that None is handled correctly
there.)
> === modified file 'bzrlib/tests/repository_implementations/helpers.py'
> --- bzrlib/tests/repository_implementations/helpers.py 2007-09-24 03:45:21 +0000
> +++ bzrlib/tests/repository_implementations/helpers.py 2007-10-17 09:39:41 +0000
> + repo.lock_write()
> + repo.start_write_group()
> + try:
> + inv = inventory.Inventory(revision_id='revision-id')
> + inv.root.revision = 'revision-id'
> + repo.add_inventory('revision-id', inv, [])
> + revision = _mod_revision.Revision('revision-id',
> + committer='jrandom at example.com', timestamp=0,
> + inventory_sha1='', timezone=0, message='message', parent_ids=[])
> + repo.add_revision('revision-id',revision, inv)
> + except:
> + repo.abort_write_group()
> + repo.unlock()
> + raise
> + finally:
> + repo.commit_write_group()
> + repo.unlock()
>
try/except/finally needs to be nested in Python 2.4.
A start,
Ian C.
More information about the bazaar
mailing list