Merge lp:~jameinel/bzr/1.19-bug-402778 into lp:~bzr/bzr/trunk-old

[John A Meinel]

[John A Meinel (jameinel)]

This fixes 2 bugs found as part of bug #402778

InterDifferingSerializer was failing when the target was a stacked branch. It had 2 primary bugs

1) The inner loop was using the wrong variable, which meant that it was generating a delta for one revision, and then inserting it as the delta for a different revision.

2) It was trying to fill in parent inventories for all parents, even if some parents where ghosts in the source.

I updated the test that Andrew added. I had to add content to the revisions, because otherwise you can't tell whether the added inventories are valid. And I added a ghost parent, to make sure that we don't try to fill in ghosts that we don't have.

[Robert Collins (lifeless)]

On Tue, 2009-08-11 at 17:35 +0000, John A Meinel wrote:
> @@ -3816,10 +3818,13 @@
> parent_ids.difference_update(revision_ids)
> parent_ids.discard(_mod_revision.NULL_REVISION)
> parent_map = self.source.get_parent_map(parent_ids)
> - for parent_tree in self.source.revision_trees(parent_ids):
> - basis_id, delta = self._get_delta_for_revision(tree, parent_ids, basis_id, cache)
> + # we iterate over parent_map and not parent_ids because we don't
> + # want to try copying any revision which is a ghost
> + for parent_tree in self.source.revision_trees(parent_map.keys()):

This might be slightly cheaper as
+ for parent_tree in self.source.revision_trees(parent_map):

I'd prefer to see separate tests for ghosts and inventory validity.

so tweak:, however that is expressed...
 review +1

review: Approve

[John A Meinel (jameinel)]

This is a follow up from Robert's review.

(original)
1) This fixes InterDifferingSerializer to properly transmit parent inventories for stacked branches. (Rather than passing the last-tree inventory and claim it was the parent inventory.)
2) Also fixes IDS to ignore ghost parents when filling in parent inventories.

(updated)
1) Changes test_pack_repository => per_pack_repository. Andrew mentioned this, I liked the change.

2) Adds a conversion to 2a format to the inter-repository tests. Which shows that even with the original fix, things were still broken for 2a formats.

3) Adds a direct test that "Repository.pack()" preserves all inventory texts, even when we don't have a revision associated with it.

I'm pretty sure this closes 2+ bugs marked as critical for 2.0.

[Robert Collins (lifeless)]

 review: +1

The use of vf.keys() in the gc pack code - might want to check that
reconcile() doesn't use revision selectors - as in that case you will
copy too many inventories [if all tests are passing this probably isn't
the case]

-Rob

review: Approve

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Robert Collins wrote:
> Review: Approve
> review: +1
>
> The use of vf.keys() in the gc pack code - might want to check that
> reconcile() doesn't use revision selectors - as in that case you will
> copy too many inventories [if all tests are passing this probably isn't
> the case]
>
> -Rob
>

Reconcile has its own implementation of _copy_inventory_texts:

    def _copy_inventory_texts(self):
        source_vf, target_vf = self._build_vfs('inventory', True, True)
        self._copy_stream(source_vf, target_vf, self.revision_keys,
                          'inventories', self._get_filtered_inv_stream, 2)
        if source_vf.keys() != self.revision_keys:
            self._data_changed = True

Which is saying "I'm going to prune down to the minimum, and if the
source's number of keys is different than the minimum, then things have
changed."

This means that reconcile on stacked branches is probably slightly wrong.

But anyway, this change obviously doesn't effect reconcile :).

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkqDKfUACgkQJdeBCYSNAAOuKQCeKD4PtKpnw6Ro0AZaWv/w0LPp
6woAoI/3kDA39hojP67PXDb715bJXIKU
=T66k
-----END PGP SIGNATURE-----

[Robert Collins (lifeless)]

On Wed, 2009-08-12 at 20:48 +0000, John A Meinel wrote:

> Which is saying "I'm going to prune down to the minimum, and if the
> source's number of keys is different than the minimum, then things have
> changed."
>
> This means that reconcile on stacked branches is probably slightly wrong.
>
> But anyway, this change obviously doesn't effect reconcile :).

Ok. So reconcile needs to keep the parent inventories too. I smell
another branch coming up :).

-Rob

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Robert Collins wrote:
> On Wed, 2009-08-12 at 20:48 +0000, John A Meinel wrote:
>
>> Which is saying "I'm going to prune down to the minimum, and if the
>> source's number of keys is different than the minimum, then things have
>> changed."
>>
>> This means that reconcile on stacked branches is probably slightly wrong.
>>
>> But anyway, this change obviously doesn't effect reconcile :).
>
> Ok. So reconcile needs to keep the parent inventories too. I smell
> another branch coming up :).
>
> -Rob
>

Note that in that case --1.9 format repos are probably also vulnerable,
as I'm pretty sure Packer uses "self._revision_keys" directly, and
doesn't try to also include parent inventories.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkqDMI8ACgkQJdeBCYSNAANyHQCeJ4DVIM/tuX1KbvnHDxMwQJrU
jMcAnAvAjV202VihITKOng43e4tYHqiz
=ExZL
-----END PGP SIGNATURE-----

[Robert Collins (lifeless)]

On Wed, 2009-08-12 at 21:15 +0000, John A Meinel wrote:
>
> > Ok. So reconcile needs to keep the parent inventories too. I smell
> > another branch coming up :).
> >
> > -Rob
> >
>
> Note that in that case --1.9 format repos are probably also
> vulnerable,
> as I'm pretty sure Packer uses "self._revision_keys" directly, and
> doesn't try to also include parent inventories.

exactly :(. Want me to file the bug?

=== modified file 'NEWS'
--- NEWS	2009-08-12 02:59:14 +0000
+++ NEWS	2009-08-12 21:35:09 +0000
@@ -27,6 +27,11 @@
 * Further tweaks to handling of ``bzr add`` messages about ignored files.
   (Jason Spashett, #76616)
 
+* Properly handle fetching into a stacked branch while converting the
+  data, especially when there are also ghosts. The code was filling in
+  parent inventories incorrectly, and also not handling when one of the
+  parents was a ghost. (John Arbash Meinel, #402778, #412198)
+
 Improvements
 ************
 

=== modified file 'bzrlib/repofmt/groupcompress_repo.py'
--- bzrlib/repofmt/groupcompress_repo.py	2009-08-04 04:36:34 +0000
+++ bzrlib/repofmt/groupcompress_repo.py	2009-08-12 21:35:09 +0000
@@ -410,7 +410,18 @@
 
     def _copy_inventory_texts(self):
         source_vf, target_vf = self._build_vfs('inventory', True, True)
-        self._copy_stream(source_vf, target_vf, self.revision_keys,
+        # It is not sufficient to just use self.revision_keys, as stacked
+        # repositories can have more inventories than they have revisions.
+        # One alternative would be to do something with
+        # get_parent_map(self.revision_keys), but that shouldn't be any faster
+        # than this.
+        inventory_keys = source_vf.keys()
+        missing_inventories = set(self.revision_keys).difference(inventory_keys)
+        if missing_inventories:
+            missing_inventories = sorted(missing_inventories)
+            raise ValueError('We are missing inventories for revisions: %s'
+                % (missing_inventories,))
+        self._copy_stream(source_vf, target_vf, inventory_keys,
                           'inventories', self._get_filtered_inv_stream, 2)
 
     def _copy_chk_texts(self):
@@ -1110,7 +1121,7 @@
 
 class RepositoryFormat2a(RepositoryFormatCHK2):
     """A CHK repository that uses the bencode revision serializer.
-    
+
     This is the same as RepositoryFormatCHK2 but with a public name.
     """
 

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py	2009-08-11 02:45:36 +0000
+++ bzrlib/repository.py	2009-08-12 21:35:09 +0000
@@ -3812,6 +3812,8 @@
             # for the new revisions that we are about to insert.  We do this
             # before adding the revisions so that no revision is added until
             # all the inventories it may depend on are added.
+            # Note that this is overzealous, as we may have fetched these in an
+            # earlier batch.
             parent_ids = set()
             revision_ids = set()
             for revision in pending_revisions:
@@ -3820,10 +3822,13 @@
             parent_ids.difference_update(revision_ids)
             parent_ids.discard(_mod_revision.NULL_REVISION)
             parent_map = self.source.get_parent_map(parent_ids)
-            for parent_tree in self.source.revision_trees(parent_ids):
-                basis_id, delta = self._get_delta_for_revision(tree, parent_ids, basis_id, cache)
+            # we iterate over parent_map and not parent_ids because we don't
+            # want to try copying any revision which is a ghost
+            for parent_tree in self.source.revision_trees(parent_map):
                 current_revision_id = parent_tree.get_revision_id()
                 parents_parents = parent_map[current_revision_id]
+                basis_id, delta = self._get_delta_for_revision(parent_tree,
+                    parents_parents, basis_id, cache)
                 self.target.add_inventory_by_delta(
                     basis_id, delta, current_revision_id, parents_parents)
         # insert signatures and revisions

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2009-08-04 11:40:59 +0000
+++ bzrlib/tests/__init__.py	2009-08-12 21:35:09 +0000
@@ -3386,6 +3386,7 @@
                    'bzrlib.tests.per_lock',
                    'bzrlib.tests.per_transport',
                    'bzrlib.tests.per_tree',
+                   'bzrlib.tests.per_pack_repository',
                    'bzrlib.tests.per_repository',
                    'bzrlib.tests.per_repository_chk',
                    'bzrlib.tests.per_repository_reference',
@@ -3480,7 +3481,6 @@
                    'bzrlib.tests.test_osutils',
                    'bzrlib.tests.test_osutils_encodings',
                    'bzrlib.tests.test_pack',
-                   'bzrlib.tests.test_pack_repository',
                    'bzrlib.tests.test_patch',
                    'bzrlib.tests.test_patches',
                    'bzrlib.tests.test_permissions',

=== modified file 'bzrlib/tests/per_interrepository/__init__.py'
--- bzrlib/tests/per_interrepository/__init__.py	2009-07-10 06:46:10 +0000
+++ bzrlib/tests/per_interrepository/__init__.py	2009-08-12 21:35:09 +0000
@@ -68,7 +68,12 @@
 
 def default_test_list():
     """Generate the default list of interrepo permutations to test."""
-    from bzrlib.repofmt import knitrepo, pack_repo, weaverepo
+    from bzrlib.repofmt import (
+        groupcompress_repo,
+        knitrepo,
+        pack_repo,
+        weaverepo,
+        )
     result = []
     # test the default InterRepository between format 6 and the current
     # default format.
@@ -111,6 +116,9 @@
     result.append((InterDifferingSerializer,
                    pack_repo.RepositoryFormatKnitPack1(),
                    pack_repo.RepositoryFormatKnitPack6RichRoot()))
+    result.append((InterDifferingSerializer,
+                   pack_repo.RepositoryFormatKnitPack6RichRoot(),
+                   groupcompress_repo.RepositoryFormat2a()))
     return result
 
 

=== modified file 'bzrlib/tests/per_interrepository/test_fetch.py'
--- bzrlib/tests/per_interrepository/test_fetch.py	2009-07-10 06:46:10 +0000
+++ bzrlib/tests/per_interrepository/test_fetch.py	2009-08-12 21:35:09 +0000
@@ -132,17 +132,23 @@
         altered by all revisions it contains, which means that it needs both
         the inventory for any revision it has, and the inventories of all that
         revision's parents.
+
+        However, we should also skip any revisions which are ghosts in the
+        parents.
         """
-        to_repo = self.make_to_repository('to')
-        if not to_repo._format.supports_external_lookups:
+        if not self.repository_format_to.supports_external_lookups:
             raise TestNotApplicable("Need stacking support in the target.")
         builder = self.make_branch_builder('branch')
         builder.start_series()
         builder.build_snapshot('base', None, [
-            ('add', ('', 'root-id', 'directory', ''))])
-        builder.build_snapshot('left', ['base'], [])
-        builder.build_snapshot('right', ['base'], [])
-        builder.build_snapshot('merge', ['left', 'right'], [])
+            ('add', ('', 'root-id', 'directory', '')),
+            ('add', ('file', 'file-id', 'file', 'content\n'))])
+        builder.build_snapshot('left', ['base'], [
+            ('modify', ('file-id', 'left content\n'))])
+        builder.build_snapshot('right', ['base'], [
+            ('modify', ('file-id', 'right content\n'))])
+        builder.build_snapshot('merge', ['left', 'right'], [
+            ('modify', ('file-id', 'left and right content\n'))])
         builder.finish_series()
         branch = builder.get_branch()
         repo = self.make_to_repository('trunk')
@@ -161,6 +167,57 @@
         self.assertEqual(
             set([('left',), ('right',), ('merge',)]),
             unstacked_repo.inventories.keys())
+        # And the basis inventories have been copied correctly
+        trunk.lock_read()
+        self.addCleanup(trunk.unlock)
+        left_tree, right_tree = trunk.repository.revision_trees(
+            ['left', 'right'])
+        stacked_branch.lock_read()
+        self.addCleanup(stacked_branch.unlock)
+        (stacked_left_tree,
+         stacked_right_tree) = stacked_branch.repository.revision_trees(
+            ['left', 'right'])
+        self.assertEqual(left_tree.inventory, stacked_left_tree.inventory)
+        self.assertEqual(right_tree.inventory, stacked_right_tree.inventory)
+
+    def test_fetch_across_stacking_boundary_ignores_ghost(self):
+        if not self.repository_format_to.supports_external_lookups:
+            raise TestNotApplicable("Need stacking support in the target.")
+        to_repo = self.make_to_repository('to')
+        builder = self.make_branch_builder('branch')
+        builder.start_series()
+        builder.build_snapshot('base', None, [
+            ('add', ('', 'root-id', 'directory', '')),
+            ('add', ('file', 'file-id', 'file', 'content\n'))])
+        builder.build_snapshot('second', ['base'], [
+            ('modify', ('file-id', 'second content\n'))])
+        builder.build_snapshot('third', ['second', 'ghost'], [
+            ('modify', ('file-id', 'third content\n'))])
+        builder.finish_series()
+        branch = builder.get_branch()
+        repo = self.make_to_repository('trunk')
+        trunk = repo.bzrdir.create_branch()
+        trunk.repository.fetch(branch.repository, 'second')
+        repo = self.make_to_repository('stacked')
+        stacked_branch = repo.bzrdir.create_branch()
+        stacked_branch.set_stacked_on_url(trunk.base)
+        stacked_branch.repository.fetch(branch.repository, 'third')
+        unstacked_repo = stacked_branch.bzrdir.open_repository()
+        unstacked_repo.lock_read()
+        self.addCleanup(unstacked_repo.unlock)
+        self.assertFalse(unstacked_repo.has_revision('second'))
+        self.assertFalse(unstacked_repo.has_revision('ghost'))
+        self.assertEqual(
+            set([('second',), ('third',)]),
+            unstacked_repo.inventories.keys())
+        # And the basis inventories have been copied correctly
+        trunk.lock_read()
+        self.addCleanup(trunk.unlock)
+        second_tree = trunk.repository.revision_tree('second')
+        stacked_branch.lock_read()
+        self.addCleanup(stacked_branch.unlock)
+        stacked_second_tree = stacked_branch.repository.revision_tree('second')
+        self.assertEqual(second_tree.inventory, stacked_second_tree.inventory)
 
     def test_fetch_missing_basis_text(self):
         """If fetching a delta, we should die if a basis is not present."""
@@ -276,8 +333,12 @@
         to_repo = self.make_to_repository('to')
         to_repo.fetch(from_tree.branch.repository)
         recorded_inv_sha1 = to_repo.get_inventory_sha1('foo-id')
-        xml = to_repo.get_inventory_xml('foo-id')
-        computed_inv_sha1 = osutils.sha_string(xml)
+        to_repo.lock_read()
+        self.addCleanup(to_repo.unlock)
+        stream = to_repo.inventories.get_record_stream([('foo-id',)],
+                                                       'unordered', True)
+        bytes = stream.next().get_bytes_as('fulltext')
+        computed_inv_sha1 = osutils.sha_string(bytes)
         self.assertEqual(computed_inv_sha1, recorded_inv_sha1)
 
 

=== renamed file 'bzrlib/tests/test_pack_repository.py' => 'bzrlib/tests/per_pack_repository.py'
--- bzrlib/tests/test_pack_repository.py	2009-08-11 05:26:57 +0000
+++ bzrlib/tests/per_pack_repository.py	2009-08-12 21:35:09 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2008 Canonical Ltd
+# Copyright (C) 2008, 2009 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -42,7 +42,7 @@
     pack_repo,
     groupcompress_repo,
     )
-from bzrlib.repofmt.groupcompress_repo import RepositoryFormatCHK1
+from bzrlib.repofmt.groupcompress_repo import RepositoryFormat2a
 from bzrlib.smart import (
     client,
     server,
@@ -84,7 +84,7 @@
         """Packs reuse deltas."""
         format = self.get_format()
         repo = self.make_repository('.', format=format)
-        if isinstance(format.repository_format, RepositoryFormatCHK1):
+        if isinstance(format.repository_format, RepositoryFormat2a):
             # TODO: This is currently a workaround. CHK format repositories
             #       ignore the 'deltas' flag, but during conversions, we can't
             #       do unordered delta fetches. Remove this clause once we
@@ -295,6 +295,41 @@
         self.assertEqual(1, len(list(index.iter_all_entries())))
         self.assertEqual(2, len(tree.branch.repository.all_revision_ids()))
 
+    def test_pack_preserves_all_inventories(self):
+        # This is related to bug:
+        #   https://bugs.launchpad.net/bzr/+bug/412198
+        # Stacked repositories need to keep the inventory for parents, even
+        # after a pack operation. However, it is harder to test that, then just
+        # test that all inventory texts are preserved.
+        format = self.get_format()
+        builder = self.make_branch_builder('source', format=format)
+        builder.start_series()
+        builder.build_snapshot('A-id', None, [
+            ('add', ('', 'root-id', 'directory', None))])
+        builder.build_snapshot('B-id', None, [
+            ('add', ('file', 'file-id', 'file', 'B content\n'))])
+        builder.build_snapshot('C-id', None, [
+            ('modify', ('file-id', 'C content\n'))])
+        builder.finish_series()
+        b = builder.get_branch()
+        b.lock_read()
+        self.addCleanup(b.unlock)
+        repo = self.make_repository('repo', shared=True, format=format)
+        repo.lock_write()
+        self.addCleanup(repo.unlock)
+        repo.fetch(b.repository, revision_id='B-id')
+        inv = b.repository.iter_inventories(['C-id']).next()
+        repo.start_write_group()
+        repo.add_inventory('C-id', inv, ['B-id'])
+        repo.commit_write_group()
+        self.assertEqual([('A-id',), ('B-id',), ('C-id',)],
+                         sorted(repo.inventories.keys()))
+        repo.pack()
+        self.assertEqual([('A-id',), ('B-id',), ('C-id',)],
+                         sorted(repo.inventories.keys()))
+        # Content should be preserved as well
+        self.assertEqual(inv, repo.iter_inventories(['C-id']).next())
+
     def test_pack_layout(self):
         # Test that the ordering of revisions in pack repositories is
         # tip->ancestor
@@ -311,7 +346,7 @@
         # revision access tends to be tip->ancestor, so ordering that way on
         # disk is a good idea.
         for _1, key, val, refs in pack.revision_index.iter_all_entries():
-            if type(format.repository_format) is RepositoryFormatCHK1:
+            if type(format.repository_format) is RepositoryFormat2a:
                 # group_start, group_len, internal_start, internal_len
                 pos = map(int, val.split())
             else:
@@ -589,7 +624,7 @@
 
     def make_write_ready_repo(self):
         format = self.get_format()
-        if isinstance(format.repository_format, RepositoryFormatCHK1):
+        if isinstance(format.repository_format, RepositoryFormat2a):
             raise TestNotApplicable("No missing compression parents")
         repo = self.make_repository('.', format=format)
         repo.lock_write()
@@ -808,7 +843,7 @@
                 matching_format_name = 'pack-0.92-subtree'
             else:
                 if repo._format.supports_chks:
-                    matching_format_name = 'development6-rich-root'
+                    matching_format_name = '2a'
                 else:
                     matching_format_name = 'rich-root-pack'
             mismatching_format_name = 'pack-0.92'
@@ -841,7 +876,7 @@
         else:
             if repo.supports_rich_root():
                 if repo._format.supports_chks:
-                    matching_format_name = 'development6-rich-root'
+                    matching_format_name = '2a'
                 else:
                     matching_format_name = 'rich-root-pack'
                 mismatching_format_name = 'pack-0.92-subtree'
@@ -1062,9 +1097,9 @@
                   "(bzr 1.9)\n",
               format_supports_external_lookups=True,
               index_class=BTreeGraphIndex),
-         dict(format_name='development6-rich-root',
-              format_string='Bazaar development format - group compression '
-                  'and chk inventory (needs bzr.dev from 1.14)\n',
+         dict(format_name='2a',
+              format_string="Bazaar repository format 2a "
+                "(needs bzr 1.16 or later)\n",
               format_supports_external_lookups=True,
               index_class=BTreeGraphIndex),
          ]