> > === modified file 'lib/lp/code/model/branchjob.py' > > --- lib/lp/code/model/branchjob.py 2009-10-20 06:03:46 +0000 > > +++ lib/lp/code/model/branchjob.py 2009-10-22 00:05:29 +0000 > > @@ -11,12 +11,15 @@ > > import os > > import shutil > > from StringIO import StringIO > > +import tempfile > > > > +from bzrlib.branch import Branch as BzrBranch > > from bzrlib.bzrdir import BzrDirMetaFormat1 > > from bzrlib.log import log_formatter, show_log > > from bzrlib.diff import show_diff_trees > > from bzrlib.revision import NULL_REVISION > > from bzrlib.revisionspec import RevisionInfo, RevisionSpec > > +from bzrlib.transport import get_transport > > from bzrlib.upgrade import upgrade > > > > from lazr.enum import DBEnumeratedType, DBItem > > @@ -252,16 +255,30 @@ > > > > def run(self): > > """See `IBranchUpgradeJob`.""" > > - self._prepare_upgrade() > > - self._upgrade() > > - > > - def _prepare_upgrade(self): > > - """Prepares the branch for upgrade.""" > > - self._upgrade_branch = self.branch.getBzrBranch() > > - > > - def _upgrade(self): > > - """Performs the upgrade of the branch.""" > > - upgrade(self._upgrade_branch.base, self.upgrade_format) > > + # Set up the new branch structure > > + upgrade_branch_path = tempfile.mkdtemp() > > Everything in this method after this point should be in a try:finally: > that blows away this directory. Done. > > > + upgrade_transport = get_transport(upgrade_branch_path) > > + source_branch = self.branch.getBzrBranch() > > There's a problem here, which is that getBzrBranch opens the branch in > the mirrored area, not the hosted area. > Yea, fixed this. > > + source_branch_transport = source_branch.bzrdir.root_transport > > I think > > source_branch_transport = get_transport(self.branch.getPullURL()) > > is what you want; it gets a transport to the hosted area of the > branch, and is less effort than opening the branch just to get the > transport out of it. Once the copy in the hosted area is upgraded, > the puller will take care of propagating the new format into the > mirrored area. > > (The names of the methods/properties are completely ridiculous, no > argument about that). > Well, I still need the source_branch to pull from later, but I can get that with Branch.open_from_transport > > + source_branch_transport.copy_tree_to_transport(upgrade_transport) > > + upgrade_branch = BzrBranch.open(upgrade_transport.base) > > + > > + # Perform the upgrade. > > + upgrade(upgrade_branch.base, self.upgrade_format) > > + > > + # Re-open the branch, since its format has changed. > > + upgrade_branch = BzrBranch.open(upgrade_transport.base) > > + upgrade_branch.pull(source_branch) > > + upgrade_branch.fetch(source_branch) > > + > > + # Move the branch in the old format to backup.bzr > > I think you should try to write lock the branch before doing this move > to prevent confusion if someone is pushing to the branch right at this > moment. > > > + source_branch_transport.rename('.bzr', 'backup.bzr') > > b = Branch.open_from_transport(source_branch_transport) > b.lock_write() > > I don't know what will happen when bzr notices that the underlying > branch has gone away from this lock though -- something to experiment > with I guess! > So it looks like we lock for the pull and fetch, than unlock right before we move. > > + upgrade_transport.delete_tree('backup.bzr') > > + upgrade_transport.copy_tree_to_transport(source_branch_transport) > > + > > + # Clean up the temporary directory. > > + upgrade_transport.delete_tree('.bzr') > > + os.rmdir(upgrade_branch_path) > > I think a shutil.rmtree(upgrade_branch_path) rather than the > two delete_tree()s then the rmdir makes more sense. > > I can't think of a sane way to test any of these changes I'm proposing > :( > Me neither. However, I take solace in the fact that we're actually testing the functionality we want. > > @property > > def upgrade_format(self): > > > === modified file 'lib/lp/code/model/tests/test_branchjob.py' > > --- lib/lp/code/model/tests/test_branchjob.py 2009-10-05 14:18:33 +0000 > > +++ lib/lp/code/model/tests/test_branchjob.py 2009-10-22 00:05:29 +0000 > > @@ -183,6 +183,25 @@ > > > > layer = LaunchpadZopelessLayer > > > > + def make_format(self, branch_format=None, repo_format=None): > > + # Return a Bzr MetaDir format with the provided branch and > repository > > + # formats. > > + if branch_format is None: > > + branch_format = BzrBranchFormat7 > > + if repo_format is None: > > + repo_format = RepositoryFormatKnitPack6 > > + format = BzrDirMetaFormat1() > > + format.set_branch_format(branch_format()) > > + format._set_repository_format(repo_format()) > > + return format > > + > > + def make_upgrade_branchjob(self): > > + db_branch, tree = self.create_branch_and_tree(format='knit') > > + db_branch.branch_format = BranchFormat.BZR_BRANCH_5 > > + db_branch.repository_format = RepositoryFormat.BZR_KNIT_1 > > + job = BranchUpgradeJob.create(db_branch) > > + return db_branch, job > > It doesn't seem that this helper is actually used. > Ah, that's from the old code where there were more tests. Here's the incremental diff: === modified file 'lib/lp/code/model/branchjob.py' --- lib/lp/code/model/branchjob.py 2009-10-21 23:34:08 +0000 +++ lib/lp/code/model/branchjob.py 2009-10-22 04:22:18 +0000 @@ -257,28 +257,32 @@ """See `IBranchUpgradeJob`.""" # Set up the new branch structure upgrade_branch_path = tempfile.mkdtemp() - upgrade_transport = get_transport(upgrade_branch_path) - source_branch = self.branch.getBzrBranch() - source_branch_transport = source_branch.bzrdir.root_transport - source_branch_transport.copy_tree_to_transport(upgrade_transport) - upgrade_branch = BzrBranch.open(upgrade_transport.base) - - # Perform the upgrade. - upgrade(upgrade_branch.base, self.upgrade_format) - - # Re-open the branch, since its format has changed. - upgrade_branch = BzrBranch.open(upgrade_transport.base) - upgrade_branch.pull(source_branch) - upgrade_branch.fetch(source_branch) - - # Move the branch in the old format to backup.bzr - source_branch_transport.rename('.bzr', 'backup.bzr') - upgrade_transport.delete_tree('backup.bzr') - upgrade_transport.copy_tree_to_transport(source_branch_transport) - - # Clean up the temporary directory. - upgrade_transport.delete_tree('.bzr') - os.rmdir(upgrade_branch_path) + try: + upgrade_transport = get_transport(upgrade_branch_path) + source_branch_transport = get_transport(self.branch.getPullURL()) + source_branch_transport.copy_tree_to_transport(upgrade_transport) + upgrade_branch = BzrBranch.open(upgrade_transport.base) + + # Perform the upgrade. + upgrade(upgrade_branch.base, self.upgrade_format) + + # Re-open the branch, since its format has changed. + upgrade_branch = BzrBranch.open_from_transport( + upgrade_transport) + source_branch = BzrBranch.open_from_transport( + source_branch_transport) + + source_branch.lock_write() + upgrade_branch.pull(source_branch) + upgrade_branch.fetch(source_branch) + source_branch.unlock() + + # Move the branch in the old format to backup.bzr + source_branch_transport.rename('.bzr', 'backup.bzr') + upgrade_transport.delete_tree('backup.bzr') + upgrade_transport.copy_tree_to_transport(source_branch_transport) + finally: + shutil.rmtree(upgrade_branch_path) @property def upgrade_format(self): === modified file 'lib/lp/code/model/tests/test_branchjob.py' --- lib/lp/code/model/tests/test_branchjob.py 2009-10-21 22:01:23 +0000 +++ lib/lp/code/model/tests/test_branchjob.py 2009-10-22 04:31:06 +0000 @@ -195,13 +195,6 @@ format._set_repository_format(repo_format()) return format - def make_upgrade_branchjob(self): - db_branch, tree = self.create_branch_and_tree(format='knit') - db_branch.branch_format = BranchFormat.BZR_BRANCH_5 - db_branch.repository_format = RepositoryFormat.BZR_KNIT_1 - job = BranchUpgradeJob.create(db_branch) - return db_branch, job - def test_providesInterface(self): """Ensure that BranchUpgradeJob implements IBranchUpgradeJob.""" branch = self.factory.makeAnyBranch() @@ -211,7 +204,8 @@ def test_upgrades_branch(self): """Ensure that a branch with an outdated format is upgraded.""" self.useBzrBranches() - db_branch, tree = self.create_branch_and_tree(format='knit') + db_branch, tree = self.create_branch_and_tree( + hosted=True, format='knit') db_branch.branch_format = BranchFormat.BZR_BRANCH_5 db_branch.repository_format = RepositoryFormat.BZR_KNIT_1 self.assertEqual(