[MERGE] Packs. Kthxbye.

Robert Collins robertc at robertcollins.net
Fri Oct 19 07:28:41 BST 2007


On Fri, 2007-10-19 at 16:00 +1000, Ian Clatworthy wrote:
> Robert Collins wrote:
> > I think the moment has come to get the pack repository format reviewed.
> 
> Some *initial* comments on RepositoryPackCollection. Apologies if some
> of these were buried in earlier reviews.
> 
> The retention of obsolete packs is a known issue and I'm pretty sure
> you've raised a bug on it. Perhaps a simple limit on the number of
> obsolete packs would be sufficient initially, e.g. if there are more
> than N, delete the oldest (or largest?) ones. Is there value as well in
> putting obsolete indices into ../obsolete_indices/ instead of
> ../obsolete_packs?

I don't think so. Yes there is a bug, tagged with 'packs'. Rather than
speculate on different fixes - there is a bug. Lets either decide 'must
fix before merge', or 'there is a bug and thats ok'.

> It would be good if related methods were more closely positioned in the
> source code, e.g. lock_names/unlock_names,
> add_pack_to_memory/remove_pack_from_memory. You normal do this well and
> it does help when reviewing, e.g. making sure all attributes that are
> initialised in one spot get cleared in the other.

I deliberately aim for alphabetical method ordering; which is why they
are not close, and why I'd resist putting them close together.

> The code for pack would be slightly more readable if total_rev_count was
> calculated and then the code did
> 
>   self.execute_pack_operations([[total_rev_count, self.app_packs()]])
> 
> pack_distribution isn't used in that method either?

pack_distribution is used to calculate the pack operations, not to
execute them. I'm not sure without checking the code whether your
suggesting is an improvement or not.

> It might be good to add some logging when collisions are detected, e.g.
> allocate. I'm thinking about diagnosing weird bugs and having something
> in bzr.log that might help us realise something unusual happened (and
> perhaps that's the cause or a symptom of the real problem).
> 
> In reset, self.repo._inventory_knit stands out as not being cleared.
> Perhaps there isn't one.

There isn't.

> In _packs_list_to_..., s/lsit/list/ in the docstring. Also a better
> explanation needed for index_attribute.
> 
> In commit_write_group, you might want to set self._new_pack to None in
> the else clause after the abort() call?

Yes, good idea.

> So, nothing serious there except obsolete_pack/index handling. The rest
> are minor grumbles and not blockers IMO.

Cool.

-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/20071019/70b4fb3f/attachment-0001.pgp 


More information about the bazaar mailing list