Merge lp:~eric97/bzr/regenerate-pack-names into lp:bzr

[Eric Siegerman]

I clobbered the pack-names file in one of my shared repos. (Not
to worry; I'm 100% certain it was my screwup, *not* Bazaar's.)

This MP is the result. It adds a hidden "regenerate-pack-names"
command.

It does *not* actually overwrite .bzr/repository/pack-names, but
rather writes its new version to "pack-names.generated", for the
user to inspect and, if it looks good, rename to the live
filename. (Yeah, I know JAM's new repair-dirstate command does
the repairs in-place, but pack-names seems a far more dangerous
file to replace, so I wanted to build in an extra layer of
paranoia.)

Two issues before it's ready to merge:
  - Locking: Does this need to lock the repo? If so, read or
    write? Strictly, it should block other processes from even
    renaming new packs into packs/*.pack for the duration, but is
    that even possible? Not much point locking the
    rewrite-pack-names mutex, because it doesn't in fact write
    that file...

  - So far, I've only tested against local file access; no
    remote Transports. I wonder whether there's any point
    supporting it remotely, given that you need local shell
    access to rename the new file into service anyway.

[Martin Pool (mbp)]

Thanks for addressing that gap. I don't recall hearing of anyone else hitting that exact problem, but it's certainly good to have ways for people to recover whenever we can.

For consistency I think we should call this repair-pack-names.

I think you should hold a lock while doing this. I think the repo lock is held while this is changed. You should check.

--force doesn't seem to have any effect, and the help implies it does.

I see your point about writing it to a temporary file, but I think that just makes it more difficult for people who've got into this state. I would rather for example back up the existing file, or indeed back up the whole repo dir, or show a description of the differences. For users who are not super technical and perhaps have their tree stored on a remote file server, asking them to also find and rename the file just makes it more difficult.

regenerate_pack_names seems likely to be duplicating some knowledge about the file format that's already present.

+ (hdr, err) = self.run_bzr_error([], ['dump-btree', '--raw', path],
+ retcode=0)

It would be cleaner and faster to use an API call for these rather than starting a new copy of bzr.

+ def assertFileExists(self, path):
+ self.assertTrue(os.path.exists(path), "%s exists" % path)
+
+ def assertNotFileExists(self, path):
+ self.assertFalse(os.path.exists(path), "%s does not exist" % path)
+

There is already failIfExists and failUnlessExists in TestCase.

  - So far, I've only tested against local file access; no
    remote Transports. I wonder whether there's any point
    supporting it remotely, given that you need local shell
    access to rename the new file into service anyway.

You're not doing anything transport-specific so I don't see any reason you would need to run per-transport tests. However, it would be good to make the unit tests run against a memory transport because that will be faster and it will validate you're not doing anything accidentally local fs specific.

If you want help with anything just ask.

Thanks, Martin

review: Needs Fixing

[Eric Siegerman (eric97)]

I've set this back to Work in Progress while I address Martin's comments.

[Martin Pool (mbp)]

On 19 February 2011 06:08, Eric Siegerman <email address hidden> wrote:
> I've set this back to Work in Progress while I address Martin's comments.

If you want help, or want someone else to finish it, just ask.

Unmerged revisions

5671. By Eric Siegerman <email address hidden> on 2011-02-15

Blackbox tests now verify that the old pack-names wasn't touched.

5670. By Eric Siegerman <email address hidden> on 2011-02-15

Doc and release-note fixes.

5669. By Eric Siegerman <email address hidden> on 2011-02-15

Add a test case, and rename another one.

5668. By Eric Siegerman <email address hidden> on 2011-02-14

Implement pack_repo.RepositoryPackCollection.regenerate_pack_names().

5667. By Eric Siegerman <email address hidden> on 2011-02-14

Push the implementation down a couple more levels.
It still doesn't do anything, but checks for a bunch of error
conditions first.

5666. By Eric Siegerman <email address hidden> on 2011-02-14

A couple more doc fixes: simple typos.

5665. By Eric Siegerman <email address hidden> on 2011-02-14

Drive-by doc fix: TestCase.run_captured() doesn't exist, but .run_bzr_error() captures output,
which is why the comment in question seems to think one might want to "see also" it.

5664. By Eric Siegerman <email address hidden> on 2011-02-13

Drive-by doc fix.

5663. By Eric Siegerman <email address hidden> on 2011-02-13

Skeleton.

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2011-02-09 17:10:05 +0000
+++ bzrlib/builtins.py	2011-02-15 02:33:59 +0000
@@ -537,6 +537,38 @@
             raise errors.BzrCommandError('failed to reset the tree state'
                                          + extra)
 
+class cmd_regenerate_pack_names(Command):
+    __doc__ = """Regenerate the pack-names file.
+
+    This is not meant to be used normally, but only as a way to recover from
+    filesystem corruption, etc. This creates a new .bzr/repository/pack-names
+    file for the repository from scratch, including in it all packs in the
+    repository's packs directory.
+
+    The existing pack-names file is *not* overwritten; the new file is written
+    to .bzr/repository/pack-names.generated.  After running this command, you
+    should look over the new file, e.g. using `bzr dump-btree`; then, if it
+    looks OK, move it into place manually.
+    """
+
+    takes_options = ['directory',
+        Option('force',
+               help='Regenerate the file even if it doesn\'t appear to be'
+                    ' corrupted.'),
+    ]
+    hidden = True
+
+    def run(self, directory='.', force=False):
+        bzr_dir = bzrdir.BzrDir.open(directory)
+        repository = bzr_dir.open_repository()
+        try:
+            new_content = repository.regenerate_pack_names()
+        except NotImplementedError:
+            raise errors.BzrCommandError(directory + ': not a pack repository')
+        t = repository._transport
+        mode=repository.bzrdir._get_file_mode()
+        t.put_file('pack-names.generated', new_content, mode)
+
 
 class cmd_revno(Command):
     __doc__ = """Show current revision number.

=== modified file 'bzrlib/repofmt/pack_repo.py'
--- bzrlib/repofmt/pack_repo.py	2011-01-20 21:15:10 +0000
+++ bzrlib/repofmt/pack_repo.py	2011-02-15 02:33:59 +0000
@@ -1896,10 +1896,11 @@
     def _diff_pack_names(self):
         """Read the pack names from disk, and compare it to the one in memory.
 
-        :return: (disk_nodes, deleted_nodes, new_nodes)
-            disk_nodes    The final set of nodes that should be referenced
-            deleted_nodes Nodes which have been removed from when we started
-            new_nodes     Nodes that are newly introduced
+        :return: (disk_nodes, deleted_nodes, new_nodes, orig_disk_nodes)
+            disk_nodes      The final set of nodes that should be referenced
+            deleted_nodes   Nodes which have been removed from when we started
+            new_nodes       Nodes that are newly introduced
+            orig_disk_nodes Original set of nodes that are already referenced
         """
         # load the disk nodes across
         disk_nodes = set()
@@ -2216,6 +2217,47 @@
         for token in tokens:
             self._resume_pack(token)
 
+    def regenerate_pack_names(self):
+        """Build a new pack-names file by examining the pack and index files on disk.
+
+        Does *not* modify the existing pack-names file, but returns the contents
+        for a new one.
+
+        :return: The contents that (this method believes) should be written
+            to pack-names
+        """
+        if not self._pack_transport.listable():
+            raise Exception("can't list repository's contents")
+
+        # Paranoia: make sure we don't have *anything* cached
+        self.reset()
+
+        builder = self._index_builder_class()
+        files = self._pack_transport.list_dir('.')
+        for file in files:
+            if not file.endswith('.pack'):
+                continue
+            pack_name = file[0:-5]
+
+            sizes = [None, None, None, None]
+            if self.chk_index is not None:
+                sizes.append(None)
+            for suffix in self._suffix_offsets.keys():
+                if suffix == '.cix' and self.chk_index is None:
+                        continue
+                size_offset = self._suffix_offsets[suffix]
+                index_name = pack_name + suffix
+                size = self._index_transport.stat(index_name).st_size
+                sizes[size_offset] = str(size)
+
+            builder.add_node((pack_name,), " ".join(sizes))
+
+        # More paranoia: if anything did make it back into the cache,
+        # our caller might well be about to change things out from under it
+        self.reset()
+
+        return builder.finish()
+
 
 class KnitPackRepository(KnitRepository):
     """Repository with knit objects stored inside pack containers.
@@ -2432,6 +2474,13 @@
         packer = ReconcilePacker(collection, packs, extension, revs)
         return packer.pack(pb)
 
+    def regenerate_pack_names(self):
+        """Regenerate the pack-names file within the repository.
+
+        This is only for disaster recovery; it should not be called in day-to-day use.
+        """
+        return self._pack_collection.regenerate_pack_names()
+
     @only_raises(errors.LockNotHeld, errors.LockBroken)
     def unlock(self):
         if self._write_lock_count == 1 and self._write_group is not None:

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py	2011-02-08 15:41:44 +0000
+++ bzrlib/repository.py	2011-02-15 02:33:59 +0000
@@ -1439,7 +1439,7 @@
     def lock_write(self, token=None):
         """Lock this repository for writing.
 
-        This causes caching within the repository obejct to start accumlating
+        This causes caching within the repository object to start accumlating
         data during reads, and allows a 'write_group' to be obtained. Write
         groups must be used for actual data insertion.
 
@@ -2866,6 +2866,17 @@
         """
         raise NotImplementedError(self.revision_graph_can_have_wrong_parents)
 
+    def regenerate_pack_names(self):
+        """Build a new pack-names file by examining the pack and index files on disk.
+
+        Does *not* modify the existing pack-names file, but returns the contents
+        for a new one.
+
+        :return: The contents that (this method believes) should be written
+            to pack-names
+        """
+        raise NotImplementedError(self.regenerate_pack_names)
+
 
 def install_revision(repository, rev, revision_tree):
     """Install all revision data into a repository."""

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2011-02-11 17:12:35 +0000
+++ bzrlib/tests/__init__.py	2011-02-15 02:33:59 +0000
@@ -1896,7 +1896,7 @@
         or a functional test of the library.)
 
         This sends the stdout/stderr results into the test's log,
-        where it may be useful for debugging.  See also run_captured.
+        where it may be useful for debugging.  See also run_bzr_error.
 
         :keyword stdin: A string to be used as stdin for the command.
         :keyword retcode: The status code the command should return;

=== modified file 'bzrlib/tests/blackbox/__init__.py'
--- bzrlib/tests/blackbox/__init__.py	2011-01-27 17:45:24 +0000
+++ bzrlib/tests/blackbox/__init__.py	2011-02-15 02:33:59 +0000
@@ -99,6 +99,7 @@
                      'test_remove',
                      'test_re_sign',
                      'test_remove_tree',
+                     'test_regenerate_pack_names',
                      'test_repair_workingtree',
                      'test_resolve',
                      'test_revert',

=== added file 'bzrlib/tests/blackbox/test_regenerate_pack_names.py'
--- bzrlib/tests/blackbox/test_regenerate_pack_names.py	1970-01-01 00:00:00 +0000
+++ bzrlib/tests/blackbox/test_regenerate_pack_names.py	2011-02-15 02:33:59 +0000
@@ -0,0 +1,131 @@
+# Copyright (C) 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
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+
+import os
+from bzrlib import (
+    workingtree,
+    )
+from bzrlib.tests import TestCaseWithTransport
+
+
+class TestRegeneratePackNames(TestCaseWithTransport):
+
+    def get_btree_content(self, path):
+        """Produce a text dump of a B-Tree file.
+        
+        :return: A hybrid dump: a --raw dump of the header,
+            followed by a formatted dump of the content
+        """
+        (hdr, err) = self.run_bzr_error([], ['dump-btree', '--raw', path],
+            retcode=0)
+        self.assertEquals("", err)
+        data_offset = hdr.find("\nPage 0")
+        if data_offset != -1:
+            hdr = hdr[0:data_offset]
+
+        (out, err) = self.run_bzr_error([], ['dump-btree', path], retcode=0)
+        self.assertEquals("", err)
+        return hdr + out
+        
+    def assertFileExists(self, path):
+        self.assertTrue(os.path.exists(path), "%s exists" % path)
+
+    def assertNotFileExists(self, path):
+        self.assertFalse(os.path.exists(path), "%s does not exist" % path)
+
+    def assertFileContains(self, path, expected):
+        """Check an actual pack-names file"""
+        if expected is None:
+            self.assertNotFileExists(path)
+        else:
+            self.assertFileExists(path)
+            actual = self.get_btree_content(path)
+            self.assertEqualDiff(expected, actual)
+
+    def check_files(self, live_expected, gen_expected):
+        """Check the pack-names files after a test run.
+
+        :param live_expected: Expected content of the real pack-names file
+            (which will be its original contents unless our caller explicitly
+            modified the file).  If None, the file is expected *not* to exist.
+        :param gen_expected:  Expected content of the pack-names.generated
+            file that might have been produced from the code under test.
+            If None, the file is expected *not* to exist.
+        """
+
+        self.assertFileContains("tree/.bzr/repository/pack-names",
+            live_expected)
+        self.assertFileContains("tree/.bzr/repository/pack-names.generated",
+            gen_expected)
+
+    def make_initial_tree(self, format=None, read_pack_names=True):
+        tree = self.make_branch_and_tree('tree', format=format)
+        self.build_tree(['tree/foo', 'tree/dir/', 'tree/dir/bar'])
+        tree.add(['foo', 'dir', 'dir/bar'])
+        tree.commit('first')
+        if read_pack_names:
+            content = self.get_btree_content("tree/.bzr/repository/pack-names")
+        else:
+            content = None
+        return tree, content
+
+    # Success cases
+
+    def test_regenerate_pack_names_2a(self):
+        tree, old_pack_names = self.make_initial_tree(format="2a",
+            read_pack_names=True)
+        self.run_bzr('regenerate-pack-names -d tree')
+        self.check_files(old_pack_names, old_pack_names)
+
+    def test_regenerate_pack_names_default(self):
+        """Some day a new format might become the default.  Then,
+        this test will have its moment in the sun :-)
+        """
+        tree, old_pack_names = self.make_initial_tree()
+        self.run_bzr('regenerate-pack-names -d tree')
+        self.check_files(old_pack_names, old_pack_names)
+
+    # Error cases
+
+    def test_regenerate_pack_names_knit(self):
+        """Can't regenerate-pack-names in a repo that doesn't have one"""
+        tree, old_pack_names = self.make_initial_tree(format="knit",
+            read_pack_names=False)
+        self.run_bzr_error([": not a pack repository"],
+            'regenerate-pack-names -d tree')
+        self.check_files(None, None)
+
+    def test_regenerate_pack_names_dir_no_bzrdir(self):
+        """regenerate-pack-names should fail on a directory that has no .bzr"""
+        tree, old_pack_names = self.make_initial_tree()
+        self.run_bzr_error(['Not a branch'],
+            'regenerate-pack-names -d tree/dir')
+        self.check_files(old_pack_names, None)
+
+    def test_regenerate_pack_names_file(self):
+        """regenerate-pack-names should fail on a file"""
+        tree, old_pack_names = self.make_initial_tree()
+        self.run_bzr_error(['Not a branch'],
+            'regenerate-pack-names -d tree/foo')
+        self.check_files(old_pack_names, None)
+
+    def test_regenerate_pack_names_nonexistent(self):
+        """regenerate-pack-names should fail on a nonexistent pathname"""
+        tree, old_pack_names = self.make_initial_tree()
+        self.run_bzr_error(['Not a branch'],
+            'regenerate-pack-names -d nonex')
+        self.check_files(old_pack_names, None)

=== modified file 'bzrlib/tests/per_pack_repository.py'
--- bzrlib/tests/per_pack_repository.py	2011-01-26 19:34:58 +0000
+++ bzrlib/tests/per_pack_repository.py	2011-02-15 02:33:59 +0000
@@ -19,6 +19,10 @@
 These tests are repeated for all pack-based repository formats.
 """
 
+from os import (
+    listdir,
+    remove,
+    )
 from stat import S_ISDIR
 
 from bzrlib.btree_index import BTreeGraphIndex
@@ -53,7 +57,7 @@
 
     The following are populated from the test scenario:
 
-    :ivar format_name: Registered name fo the format to test.
+    :ivar format_name: Registered name of the format to test.
     :ivar format_string: On-disk format marker.
     :ivar format_supports_external_lookups: Boolean.
     """
@@ -710,7 +714,7 @@
 
     def _lock_write(self, write_lockable):
         """Lock write_lockable, add a cleanup and return the result.
-        
+
         :param write_lockable: An object with a lock_write method.
         :return: The result of write_lockable.lock_write().
         """
@@ -1120,6 +1124,91 @@
             self.assertEqual(2, streaming_calls)
 
 
+class TestRegeneratePackNames(TestCaseWithTransport):
+    def get_format(self):
+        return bzrdir.format_registry.make_bzrdir(self.format_name)
+
+    def read_file(self, path):
+        f = file(path, 'rb')
+        try:
+            return f.read()
+        finally:
+            f.close()
+
+        self.fail("Should never get here")
+
+    def make_initial_tree(self, num_commits):
+        """Build and populate a working tree.
+
+        :param num_commits: Number of commits to perform (for sufficiently small
+            values, i.e. until autopacking happens, this is also the number of
+            packs to create)
+        :returns: (tree, old_pack_names)
+            tree           The WorkingTree
+            old_pack_names Content of the repository's pack-names file
+        """
+        format = self.get_format()
+        tree = self.make_branch_and_tree('tree', format=format)
+        self.build_tree(['tree/foo', 'tree/dir/', 'tree/dir/bar'])
+        tree.add(['foo', 'dir', 'dir/bar'])
+        for i in range(1, num_commits + 1):
+            tree.commit("revision #%d" % i)
+        old_pack_names = self.read_file('tree/.bzr/repository/pack-names')
+        return tree, old_pack_names
+
+    def run_and_check(self, tree, live_expected, gen_expected):
+        """Run the method under test, and check the results.
+
+        :param tree: The working tree
+        :param live_expected: Expected content of the real pack-names file
+            (which will be its original contents unless our caller explicitly
+            modified the file)
+        :param gen_expected:  Expected content of the newly generated pack-names
+        """
+        # Run the code under test
+        reader = tree.branch.repository.regenerate_pack_names()
+        # Check its output...
+        gen_actual = reader.read()
+        self.assertEqualDiff(gen_expected, gen_actual)
+        # ...and the live pack-names file
+        live_actual = self.read_file('tree/.bzr/repository/pack-names')
+        self.assertEqualDiff(live_expected, live_actual)
+
+    def test_zero_packs(self):
+        tree, old_pack_names = self.make_initial_tree(0)
+        self.run_and_check(tree, old_pack_names, old_pack_names)
+
+    def test_one_pack(self):
+        tree, old_pack_names = self.make_initial_tree(1)
+        self.run_and_check(tree, old_pack_names, old_pack_names)
+
+    def test_three_packs(self):
+        tree, old_pack_names = self.make_initial_tree(3)
+        self.run_and_check(tree, old_pack_names, old_pack_names)
+
+    def test_with_real_pack_names_clobbered(self):
+        """With the real pack-names clobbered, should succeed normally"""
+        tree, old_pack_names = self.make_initial_tree(1)
+        f = file('tree/.bzr/repository/pack-names', 'w')
+        f.write("bogus\n")
+        f.close()
+        self.run_and_check(tree, "bogus\n", old_pack_names)
+
+    def test_with_an_index_deleted(self):
+        """Hard-core repo corruption should raise an appropriate exception"""
+        tree, old_pack_names = self.make_initial_tree(1)
+        indices = listdir('tree/.bzr/repository/indices')
+        for index in indices:
+            if index.endswith('.tix'):      # .tix chosen one at random
+                remove('tree/.bzr/repository/indices/' + index)
+
+        self.assertRaises(errors.NoSuchFile,
+            tree.branch.repository.regenerate_pack_names)
+
+        new_pack_names = self.read_file('tree/.bzr/repository/pack-names')
+        self.assertEqualDiff(old_pack_names, new_pack_names)
+
+
 def load_tests(basic_tests, module, loader):
     # these give the bzrdir canned format name, and the repository on-disk
     # format string

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-02-14 12:03:05 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-02-15 02:33:59 +0000
@@ -36,6 +36,10 @@
   the dirstate file to be rebuilt, rather than using a ``bzr checkout``
   workaround. (John Arbash Meinel)
 
+* A new hidden command ``bzr regenerate-pack-names``. This builds a new
+  pack-names file (useful if the old one was lost or damaged).
+  (Eric Siegerman)
+
 * Branching, merging and pulling a branch now copies revisions named in
   tags, not just the tag metadata.  (Andrew Bennetts, #309682)