[MERGE] Packs. Kthxbye.

Robert Collins robertc at robertcollins.net
Wed Oct 17 21:35:59 BST 2007


On Wed, 2007-10-17 at 23:44 +1000, Ian Clatworthy wrote:
> 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.

Thanks...

> 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.

I've made it 
"""A no-subtrees parameterised Pack repository.

This format was introduced in bzr.dev.
"""

and for Knit3

"""A subtrees parameterised Pack repository.

This repository format uses the xml7 serializer to get:
 - support for recording full info about the tree root
 - support for recording tree references

This format was introduced in bzr.dev.
"""

> > +    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?

I've deleted it, running tests to see what breaks now.




> > === 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.

Done.

> 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.)

It always has been able to be called with an index of None, its how the
writer is disabled.

> try/except/finally needs to be nested in Python 2.4.

A bug - was meant to be try:except:else.

-Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20071018/6edd6551/attachment.pgp 


More information about the bazaar mailing list