Code review comment for lp:~eric97/bzr/regenerate-pack-names

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