[MERGE] Packs. Kthxbye.

Ian Clatworthy ian.clatworthy at internode.on.net
Thu Oct 18 12:43:30 BST 2007


Ian Clatworthy wrote:

> So, to let you know where I'm up to overall:
> 
> * yet to read the new tests added to test_repository.py

Here's my review of the test_repository.py changes. Comments below.

> +    def test_disk_layout(self):
> +        format = self.get_format()
> +        repo = self.make_repository('.', format=format)
> +        # in case of side effects of locking.
> +        repo.lock_write()
> +        repo.unlock()

Can these last 3 lines go? They don't make any sense to me.

> +        # we want:
> +        # format 'Bazaar Experimental'
> +        # lock: is a directory
> +        # inventory.weave == empty_weave
> +        # empty revision-store directory
> +        # empty weaves directory

The stuff after the format line in this comment looks wrong. At a
minimum, s/^empty/no/.

> +    def check_databases(self, t):
> +        """check knit content for a repository."""
> +        # check conversion worked
> +        self.assertHasNoKndx(t, 'inventory')
> +        self.assertHasNoKnit(t, 'inventory')
> +        self.assertHasNoKndx(t, 'revisions')
> +        self.assertHasNoKnit(t, 'revisions')
> +        self.assertHasNoKndx(t, 'signatures')
> +        self.assertHasNoKnit(t, 'signatures')
> +        self.assertFalse(t.has('knits'))
> +        # revision-indexes file-container directory
> +        self.assertEqual([],
> +            list(GraphIndex(t, 'pack-names', None).iter_all_entries()))
> +        self.assertTrue(S_ISDIR(t.stat('packs').st_mode))
> +        self.assertTrue(S_ISDIR(t.stat('upload').st_mode))

Can we add this?

           self.assertTrue(S_ISDIR(t.stat('indices').st_mode))
           self.assertTrue(S_ISDIR(t.stat('obsolete_packs').st_mode))

> +    def test_shared_disk_layout(self):
> +        format = self.get_format()
> +        repo = self.make_repository('.', shared=True, format=format)
> +        # we want:
> +        # format 'Bazaar-NG Knit Repository Format 1'
> +        # lock: is a directory
> +        # inventory.weave == empty_weave
> +        # empty revision-store directory
> +        # empty weaves directory
> +        # a 'shared-storage' marker file.

Remove or update (preferred) this comment.

> +    def test_shared_no_tree_disk_layout(self):
> +        format = self.get_format()
> +        repo = self.make_repository('.', shared=True, format=format)
> +        repo.set_make_working_trees(False)
> +        # we want:
> +        # format 'Bazaar-NG Knit Repository Format 1'
> +        # lock ''
> +        # inventory.weave == empty_weave
> +        # empty revision-store directory
> +        # empty weaves directory
> +        # a 'shared-storage' marker file.

Remove or update this comment. If update (preferred), then add

  # a 'no-working-trees' marker

> +
> +    def test_adding_revision_creates_pack_indices(self):
> +        format = self.get_format()
> +        tree = self.make_branch_and_tree('.', format=format)
> +        trans = tree.branch.repository.bzrdir.get_repository_transport(None)
> +        self.assertEqual([],
> +            list(GraphIndex(trans, 'pack-names', None).iter_all_entries()))
> +        tree.commit('foobarbaz')
> +        index = GraphIndex(trans, 'pack-names', None)
> +        self.assertEqual(1, len(list(index.iter_all_entries())))
> +        node = list(index.iter_all_entries())[0]
> +        name = node[1][0]
> +        # the pack sizes should be listed in the index
> +        pack_value = node[2]
> +        sizes = [int(digits) for digits in pack_value.split(' ')]

A few things here:

1. it would be slightly more readable if you did this:
   index_items = list(index.iter_all_entries())
   self.assertEqual(1, len(index_items))
   node = index_items[0]

2. After getting the name, can you add an assert about it?
   e.g. length of 20? All chars alphanumeric?

3. After getting the sizes, how about
   self.assertEqual(4, len(sizes))

> +        # attribute on the repository to allow a different pack distribution
> +        # and max packs policy - so we are hecking the policy is honoured

s/hecking/checking/. Unless hecking is some word I need to learn more
about. :-)

> +        # there should be 1 packs:

s/packs/pack/

> +                # both r1 and r2 have open write groups with data in them
> +                # created while the other's write group was open.
> +                # Commit both which requires a merge to the pack-names.
> +                try:
> +                    r1.commit_write_group()
> +                except:
> +                    r2.abort_write_group()
> +                    raise
> +                r2.commit_write_group()

Why do you abort the write group for r2 in the except clause and not r1?
Shouldn't we be aborting the write group for both?

> +                # Now both repositories should now about both names

s/now/know/

> +                # Now both repositories should now about just one name.

s/now/know/

> +    def test_lock_write_does_not_physically_lock(self):
> +        repo = self.make_repository('.', format=self.get_format())
> +        repo.lock_write()
> +        self.addCleanup(repo.unlock)
> +        self.assertFalse(repo.get_physical_lock_status())
> +
> +    def prepare_for_break_lock(self):

This method needed a comment. For example (taken from test_break_lock.py):

   # we want a UI factory that accepts canned input for the tests:
   # while SilentUIFactory still accepts stdin, we need to customise
   # ours

> +    def test_break_lock_breaks_physical_lock(self):
> +        repo = self.make_repository('.', format=self.get_format())
> +        repo._packs.lock_names()
> +        repo2 = repository.Repository.open('.')
> +        self.assertTrue(repo.get_physical_lock_status())
> +        self.prepare_for_break_lock()
> +        repo2.break_lock()
> +        self.assertFalse(repo.get_physical_lock_status())
> +
> +    def test_broken_physical_locks_error_on_release_names_lock(self):
> +        repo = self.make_repository('.', format=self.get_format())
> +        repo._packs.lock_names()
> +        self.assertTrue(repo.get_physical_lock_status())
> +        repo2 = repository.Repository.open('.')
> +        self.prepare_for_break_lock()
> +        repo2.break_lock()
> +        self.assertRaises(errors.LockBroken, repo._packs.release_names)

These two tests are very similar. How about just having one or making
the 2nd one simply this?

    def test_broken_physical_locks_error_on_release_names_lock(self):
        self.test_break_lock_breaks_physical_lock()
        self.assertRaises(errors.LockBroken, repo._packs.release_names)

> +        """The maximum pack count is geared from the number of revisions."""

s/geared from/a function of/

> +        # no revisions - one pack, so that we can have a revision free repo
> +        # without it blowing up
> +        self.assertEqual(1, packs._max_pack_count(0))

I'm yet to read most of pack_repo.py code but I can't see why this
should return 1 instead of zero. What's the logic here? BTW, the comment
here in the test is better than the one in the code (but my brain is
still too small to get it.)

> +        # rev count - 2010 -> 2x1000 + 1x10 (3)
> +        pack_operations = packs.plan_autopack_combinations(
> +            existing_packs, [1000, 1000, 10])
> +        self.assertEqual([[2, ["single2", "single1"]], [0, []]], pack_operations)

Why is the [0,[]] sentinel value needed on the end of this list? The
return value isn't documented and it seems like the code applying the
list of operations just ignores it rather than doing anything with it.

> +    def test_get_pack_by_name(self):
...
> +        name = packs.names()[0]
...
> +        self.assertEqual(pack_repo.ExistingPack(packs._pack_transport,
> +                packs.names()[0], rev_index, inv_index, txt_index, sig_index),
> +            pack_1)

Can just reuse the 'name' variable here to improve readability.

In summary, a heaps of tests there and very well written. I need to
think about what tests over and above these we need still. I'm happy to
do that as a separate exercise and not block this merge though.

To date, my goal has been to confirm that the code outside pack_repo.py
was production quality. IMO, it is, give or take the changes suggested
today. I'll look at pack_repo.py tomorrow. It sounds like jam (and
poolie?) are looking at that as well so between us it should be
adequately covered RSN.

Ian C.



More information about the bazaar mailing list