Merge lp:~eric97/bzr/regenerate-pack-names into lp:bzr
Status: | Work in progress |
---|---|
Proposed branch: | lp:~eric97/bzr/regenerate-pack-names |
Merge into: | lp: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:~eric97/bzr/regenerate-pack-names |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Martin Pool | Needs Fixing | ||
Review via email: mp+49756@code.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-
command.
It does *not* actually overwrite .bzr/repository
rather writes its new version to "pack-names.
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-
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.
Unmerged revisions
- 5671. By Eric Siegerman <email address hidden>
-
Blackbox tests now verify that the old pack-names wasn't touched.
- 5670. By Eric Siegerman <email address hidden>
-
Doc and release-note fixes.
- 5669. By Eric Siegerman <email address hidden>
-
Add a test case, and rename another one.
- 5668. By Eric Siegerman <email address hidden>
-
Implement pack_repo.
RepositoryPackC ollection. regenerate_ pack_names( ). - 5667. By Eric Siegerman <email address hidden>
-
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>
-
A couple more doc fixes: simple typos.
- 5665. By Eric Siegerman <email address hidden>
-
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>
-
Drive-by doc fix.
- 5663. By Eric Siegerman <email address hidden>
-
Skeleton.
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 assertFileExist s(self, path): (os.path. exists( path), "%s exists" % path) ists(self, path): e(os.path. exists( path), "%s does not exist" % path)
+ self.assertTrue
+
+ def assertNotFileEx
+ self.assertFals
+
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