[MERGE] Update to bzr.dev.

[Andrew Bennetts]

More dribs and drabs for you.

Robert Collins wrote:
> === modified file 'bzrlib/tests/repository_implementations/test_fetch.py'
[...]
>          rev1_tree = knit3_repo.revision_tree('rev1')
> -        lines = rev1_tree.get_file_lines(rev1_tree.get_root_id())
> +        rev1_tree.lock_read()
> +        try:
> +            lines = rev1_tree.get_file_lines(rev1_tree.get_root_id())
> +        finally:
> +            rev1_tree.unlock()

This seems like a slightly surprising API change.  But it also seems reasonable
enough.

> === modified file 'bzrlib/tests/repository_implementations/test_reconcile.py'
[...]
>      def checkNoBackupInventory(self, aBzrDir):
>          """Check that there is no backup inventory in aBzrDir."""
>          repo = aBzrDir.open_repository()
> -        self.assertRaises(errors.NoSuchFile,
> -                          repo.control_weaves.get_weave,
> -                          'inventory.backup',
> -                          repo.get_transaction())
> +        # Remote repository, and possibly others, do not have 
> +        # _transport.
> +        if getattr(repo, '_transport', None) is not None:
> +            for path in repo._transport.list_dir('.'):
> +                self.assertFalse('inventory.backup' in path)

Hmm, this is a small reduction in test coverage.  I guess this is the most
reasonable thing to do about that though, but it's a shame.

> === modified file 'bzrlib/tests/repository_implementations/test_repository.py'
[...]
> +    def test_attribute_revision_store_basics(self):
> +        """Test the basic behaviour of the revisions attribute."""

This is followed by 24 lines of test code.  A brief summary of the behaviours
that are being exercised would be nice, e.g. “The 'keys' and 'get_parent_map'
attributes reflect the revisions committed to this repository.”

> +    def test_attribute_text_store_basics(self):
> +        """Test the basic behaviour of the text store."""

Same again: except there are 47 lines of test code here.  Actually, I think
there needs to be some comments in this method.  The intent of lots of this test
method is opaque.  I could confidently guess the purpose of all of
test_attribute_revision_store_basics after only a minute of looking at it, but
this one is much less clear to me.

[...]
> +                parents = {root_commit:()}

PEP 8 (implicitly) says there should be a space after colons in dicts.  (It says
the example of “spam(ham[1], {eggs: 2})” is good and does not have any
extraneous whitespace, and this is also the predominant style in the standard
library.)

[...]
> +    def test_exposed_versioned_files_are_marked_dirty(self):
> +        repo = self.make_repository('.')
> +        repo.lock_write()
> +        signatures = repo.signatures
> +        revisions = repo.revisions
> +        inventories = repo.inventories
> +        repo.unlock()
> +        self.assertRaises(errors.ObjectNotLocked,
> +            signatures.keys)
> +        self.assertRaises(errors.ObjectNotLocked,
> +            revisions.keys)
> +        self.assertRaises(errors.ObjectNotLocked,
> +            inventories.keys)

These lines don't need to be wrapped.

> === modified file 'bzrlib/tests/repository_implementations/test_statistics.py'

> === modified file 'bzrlib/tests/test_bundle.py'
[...]
> -        root_vf = repo.weave_store.get_weave(inv.root.file_id,
> -                                             repo.get_transaction())
> -        self.assertEqual(root_vf.versions(), ['rev1', 'rev2'])
> +        root_id = inv.root.file_id
> +        repo.lock_read()
> +        self.addCleanup(repo.unlock)
> +        self.assertEqual({(root_id, 'rev1'):(),
> +            (root_id, 'rev2'):((root_id, 'rev1'),)},
> +            repo.texts.get_parent_map([(root_id, 'rev1'), (root_id, 'rev2')]))

This sort of change disappoints a bit.  “self.assertEqual(root_vf.versions(),
['rev1', 'rev2'])” is a much more direct and clear statement of intent than this
new assertion.

In this case, I think it's fairly easy to improve:

        root_rev1 = (root_id, 'rev1')
        root_rev2 = (root_id, 'rev2')
        self.assertEqual({root_rev1: (), root_rev2: (root_rev1,)},
            repo.texts.get_parent_map([root_rev1, root_rev2]))

Although, I'm not sure why you chose to use get_parent_map when the more direct
translation of the old code would be:

        self.assertEqual(set([(root_id, 'rev1'), (root_id, 'rev2')]),
            set(repo.texts.keys()))

> === modified file 'bzrlib/tests/test_fetch.py'
[...]
> -        # Make sure fetch retrieved only what we requested
> -        self.assertTrue('rev1' in root_knit)
> -        self.assertTrue('rev2' not in root_knit)
> +        repo.lock_read()
> +        try:
> +            # Make sure fetch retrieved only what we requested
> +            self.assertEqual({('tree-root', 'rev1'):()},
> +                repo.texts.get_parent_map(
> +                    [('tree-root', 'rev1'), ('tree-root', 'rev2')]))

Wouldn't checking “repo.texts.keys()” be a more direct (and clearer) way to
assert this?  We don't care about the details of the parent relationships at
this point, just which keys exist.

> +            # Make sure fetch retrieved only what we requested
> +            self.assertEqual({('tree-root', 'rev2'):(('tree-root', 'rev1'),)},
> +                repo.texts.get_parent_map([('tree-root', 'rev2')]))

Ditto.

> @@ -286,9 +293,7 @@
>          # twice?
>          self.assertEqual(1, self._count_log_matches('/ce/id.kndx', http_logs))
>          self.assertEqual(1, self._count_log_matches('/ce/id.knit', http_logs))
> -        # XXX: This *should* be '1', but more intrusive fetch changes are
> -        # needed to drop this back to 1.
> -        self.assertEqual(2, self._count_log_matches('inventory.kndx', http_logs))
> +        self.assertEqual(1, self._count_log_matches('inventory.kndx', http_logs))

Hooray :)

-Andrew.