[MERGE] Packs. Kthxbye.

Ian Clatworthy ian.clatworthy at internode.on.net
Fri Oct 19 07:00:47 BST 2007


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?

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.

The docstring for plan_autompack_combinations needs the return value
documented. Also parma -> param.

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?

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.

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?

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

Ian C.



More information about the bazaar mailing list