[MERGE] Packs. Kthxbye.

[Robert Collins]

On Thu, 2007-10-18 at 21:43 +1000, Ian Clatworthy wrote:
> 
> Can these last 3 lines go? They don't make any sense to me.

I don't think so, though I may be wrong.


> > +        # 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/.

Gone.


> Can we add this?
> 
>            self.assertTrue(S_ISDIR(t.stat('indices').st_mode))
>            self.assertTrue(S_ISDIR(t.stat('obsolete_packs').st_mode))

Done.

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

Done

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

Made clearer and accurate.

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

Done.

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

Hmmm, this is better to test at the NewPack layer I'd say.

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

The zip assertion will error if its wrong.

> > +        # 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/

done and done

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

Hmm, good catch, done.

> > +                # Now both repositories should now about both names
> 
> s/now/know/
> 
> > +                # Now both repositories should now about just one
> name.
> 
> s/now/know/

done

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

I've added 'Setup the global ui factory state so that a break-lock
method call will find usable input in the input stream.

> > +    def test_break_lock_breaks_physical_lock(self):

> > +    def
> test_broken_physical_locks_error_on_release_names_lock(self):


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

They test different things, its clearer to have two tests. One tests
that break_lock() works for the caller of it, the other tests that
concurrent open locks error when they were open across the break
occuring.


> > +        """The maximum pack count is geared from the number of
> revisions."""
> 
> s/geared from/a function of/

done

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

If you have a repository with no revisions, just e.g.a  file text, the
file text needs a pack to live in.

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

Its not needed, its a side effect of the logic to create the plan, and I
didn't see any point in specially removing it because its not harmful.

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

Done.

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

Thanks,
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/fbdc0c39/attachment.pgp