Merge into bzr.dev : regenerate-pack-names : Code : Bazaar

Status:	Work in progress
Proposed branch:	lp://staging/~eric97/bzr/regenerate-pack-names
Merge into:	lp://staging/bzr
Diff against target:	455 lines (+325/-8) 8 files modified bzrlib/builtins.py (+32/-0) bzrlib/repofmt/pack_repo.py (+53/-4) bzrlib/repository.py (+12/-1) bzrlib/tests/__init__.py (+1/-1) bzrlib/tests/blackbox/__init__.py (+1/-0) bzrlib/tests/blackbox/test_regenerate_pack_names.py (+131/-0) bzrlib/tests/per_pack_repository.py (+91/-2) doc/en/release-notes/bzr-2.4.txt (+4/-0)
To merge this branch:	bzr merge lp://staging/~eric97/bzr/regenerate-pack-names
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Martin Pool		2011-02-15	Needs Fixing on 2011-02-15
Review via email: mp+49756@code.staging.launchpad.net

Description of the change

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.

Revision history for this message

Martin Pool (mbp) wrote on 2011-02-15:

#

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

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

Reply

Revision history for this message

Eric Siegerman (eric97) wrote on 2011-02-18:

#

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

Reply

Revision history for this message

Martin Pool (mbp) wrote on 2011-02-21:

#

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.

Reply

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.

Bazaar

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

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers