[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