Merge lp:~jameinel/bzr/repack-missing-inventories-437003 into lp:bzr/2.0

[John A Meinel]

Fix bug #437003. Don't abort an auto-pack if an inventory is in the repository, just not in the subset of packs we are handling.

This change was done against the 2.0 branch, so I'm proposing it first there, and we can 'merge up'. Though I'll be happy to skip and just propose the merge against newer branches if we prefer. (I'm not 100% sure how we decided the NEWS entries should go.)

Anyway, the proposed fix is pretty simple, and probably applicable all the way through the stack. When GCCHKPacker sees a missing inventory, it falls back to self._pack_collection.repo.inventories._index, to see if the missing inventory isn't in the repository at all. Note that I don't use self._pack_collection.repo.inventories because that would include fallbacks.

The test is pretty ugly. It involves a lot of poking at the exact state of the repository to set up the exact condition. I do a little more poking to then trigger a real failure (remove the pack file which contains the A inventory.)

The test also isn't a permutation test. It might apply vs most Pack based repositories, but I can't guarantee it.

I've also proven that the fix applies to bzr 2.4 without a problem (see lp:///~jameinel/bzr/2.4-repack-missing-inventories-437003). So we should be able to land it on any release we want. The only thing that has problems is the NEWS entries.

[Vincent Ladeuil (vila)]

77 + def make_branch_with_disjoint_inventory_and_revision(self):

This test is quite hard to understand (because of the bug it's addressing, not the way you wrote it), I wouldn't mind more comments on the individual steps and the associated assertions. (On a second read, I got it, so you may ignore this remark).

113 + else:
114 + self.fail('Could not find pack containing A\'s inventory')

a comment or a clearer message that THIS-SHOULD-NEVER-HAPPEN will do ;)

136 + (repo, rev_a_pack_name, inv_a_pack_name, rev_c_pack_name
137 + ) = self.make_branch_with_disjoint_inventory_and_revision()

Nice indentation !

Overall: nice TDD there ;)

I'm ok to land this as is.

review: Approve

[John A Meinel (jameinel)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-12-16 09:01:41 +0000
+++ NEWS	2011-03-08 12:02:22 +0000
@@ -28,6 +28,10 @@
 * Avoid UnicodeDecodeError in ``bzr add`` with multiple files under a non-ascii
   path on windows from symlink support addition. (Martin [gz], #686611)
 
+* Avoid spurious ValueErrors when autopacking a subset of the repository,
+  and seeing a revision without its related inventory
+  (John Arbash Meinel, #437003)
+
 Improvements
 ************
 
 
=== modified file 'bzrlib/repofmt/groupcompress_repo.py'
--- bzrlib/repofmt/groupcompress_repo.py	2010-01-21 16:59:21 +0000
+++ bzrlib/repofmt/groupcompress_repo.py	2011-03-08 12:02:22 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2008, 2009, 2010 Canonical Ltd
+# Copyright (C) 2008-2011 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
@@ -421,9 +421,18 @@
         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,))
+            # Go back to the original repo, to see if these are really missing
+            # https://bugs.launchpad.net/bzr/+bug/437003
+            # If we are packing a subset of the repo, it is fine to just have
+            # the data in another Pack file, which is not included in this pack
+            # operation.
+            inv_index = self._pack_collection.repo.inventories._index
+            pmap = inv_index.get_parent_map(missing_inventories)
+            really_missing = missing_inventories.difference(pmap)
+            if really_missing:
+                missing_inventories = sorted(really_missing)
+                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)
 
 
=== modified file 'bzrlib/tests/test_repository.py'
--- bzrlib/tests/test_repository.py	2010-01-21 19:24:26 +0000
+++ bzrlib/tests/test_repository.py	2011-03-08 12:02:22 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006-2010 Canonical Ltd
+# Copyright (C) 2006-2011 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
@@ -1564,6 +1564,106 @@
         self.assertTrue(new_pack.signature_index._optimize_for_size)
 
 
+class TestGCCHKPacker(TestCaseWithTransport):
+
+    def make_abc_branch(self):
+        builder = self.make_branch_builder('source')
+        builder.start_series()
+        builder.build_snapshot('A', None, [
+            ('add', ('', 'root-id', 'directory', None)),
+            ('add', ('file', 'file-id', 'file', 'content\n')),
+            ])
+        builder.build_snapshot('B', ['A'], [
+            ('add', ('dir', 'dir-id', 'directory', None))])
+        builder.build_snapshot('C', ['B'], [
+            ('modify', ('file-id', 'new content\n'))])
+        builder.finish_series()
+        return builder.get_branch()
+
+    def make_branch_with_disjoint_inventory_and_revision(self):
+        """a repo with separate packs for a revisions Revision and Inventory.
+
+        There will be one pack file that holds the Revision content, and one
+        for the Inventory content.
+
+        :return: (repository,
+                  pack_name_with_rev_A_Revision,
+                  pack_name_with_rev_A_Inventory,
+                  pack_name_with_rev_C_content)
+        """
+        b_source = self.make_abc_branch()
+        b_base = b_source.bzrdir.sprout('base', revision_id='A').open_branch()
+        b_stacked = b_base.bzrdir.sprout('stacked', stacked=True).open_branch()
+        b_stacked.lock_write()
+        self.addCleanup(b_stacked.unlock)
+        b_stacked.fetch(b_source, 'B')
+        # Now re-open the stacked repo directly (no fallbacks) so that we can
+        # fill in the A rev.
+        repo_not_stacked = b_stacked.bzrdir.open_repository()
+        repo_not_stacked.lock_write()
+        self.addCleanup(repo_not_stacked.unlock)
+        # Now we should have a pack file with A's inventory, but not its
+        # Revision
+        self.assertEqual([('A',), ('B',)],
+                         sorted(repo_not_stacked.inventories.keys()))
+        self.assertEqual([('B',)],
+                         sorted(repo_not_stacked.revisions.keys()))
+        stacked_pack_names = repo_not_stacked._pack_collection.names()
+        # We have a couple names here, figure out which has A's inventory
+        for name in stacked_pack_names:
+            pack = repo_not_stacked._pack_collection.get_pack_by_name(name)
+            keys = [n[1] for n in pack.inventory_index.iter_all_entries()]
+            if ('A',) in keys:
+                inv_a_pack_name = name
+                break
+        else:
+            self.fail('Could not find pack containing A\'s inventory')
+        repo_not_stacked.fetch(b_source.repository, 'A')
+        self.assertEqual([('A',), ('B',)],
+                         sorted(repo_not_stacked.revisions.keys()))
+        new_pack_names = set(repo_not_stacked._pack_collection.names())
+        rev_a_pack_names = new_pack_names.difference(stacked_pack_names)
+        self.assertEqual(1, len(rev_a_pack_names))
+        rev_a_pack_name = list(rev_a_pack_names)[0]
+        # Now fetch 'C', so we have a couple pack files to join
+        repo_not_stacked.fetch(b_source.repository, 'C')
+        rev_c_pack_names = set(repo_not_stacked._pack_collection.names())
+        rev_c_pack_names = rev_c_pack_names.difference(new_pack_names)
+        self.assertEqual(1, len(rev_c_pack_names))
+        rev_c_pack_name = list(rev_c_pack_names)[0]
+        return (repo_not_stacked, rev_a_pack_name, inv_a_pack_name,
+                rev_c_pack_name)
+
+    def test_pack_with_distant_inventories(self):
+        # See https://bugs.launchpad.net/bzr/+bug/437003
+        # When repacking, it is possible to have an inventory in a different
+        # pack file than the associated revision. An autopack can then come
+        # along, and miss that inventory, and complain.
+        (repo, rev_a_pack_name, inv_a_pack_name, rev_c_pack_name
+         ) = self.make_branch_with_disjoint_inventory_and_revision()
+        a_pack = repo._pack_collection.get_pack_by_name(rev_a_pack_name)
+        c_pack = repo._pack_collection.get_pack_by_name(rev_c_pack_name)
+        packer = groupcompress_repo.GCCHKPacker(repo._pack_collection,
+                    [a_pack, c_pack], '.test-pack')
+        # This would raise ValueError in bug #437003, but should not raise an
+        # error once fixed.
+        packer.pack()
+
+    def test_pack_with_missing_inventory(self):
+        # Similar to test_pack_with_missing_inventory, but this time, we force
+        # the A inventory to actually be gone from the repository.
+        (repo, rev_a_pack_name, inv_a_pack_name, rev_c_pack_name
+         ) = self.make_branch_with_disjoint_inventory_and_revision()
+        inv_a_pack = repo._pack_collection.get_pack_by_name(inv_a_pack_name)
+        repo._pack_collection._remove_pack_from_memory(inv_a_pack)
+        packer = groupcompress_repo.GCCHKPacker(repo._pack_collection,
+            repo._pack_collection.all_packs(), '.test-pack')
+        e = self.assertRaises(ValueError, packer.pack)
+        packer.new_pack.abort()
+        self.assertContainsRe(str(e),
+            r"We are missing inventories for revisions: .*'A'")
+
+
 class TestCrossFormatPacks(TestCaseWithTransport):
 
     def log_pack(self, hint=None):