Code review comment for lp:~rockstar/launchpad/branch-upgrade-out-of-place

Hi Paul,

I hope you're enjoying using the bzrlib apis :-)

A few fixes required I think, nothing fundamental though.

> === 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.

> + 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.

> + 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).

> + 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!

> + 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
:(

> @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.

> def test_providesInterface(self):
> """Ensure that BranchUpgradeJob implements IBranchUpgradeJob."""
> branch = self.factory.makeAnyBranch()
> @@ -224,18 +243,6 @@
> REPOSITORY_FORMAT_UPGRADE_PATH.get(
> RepositoryFormat.BZR_REPOSITORY_4))
>
> - 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 test_upgrade_format_no_branch_upgrade_needed(self):
> # getUpgradeFormat should not downgrade the branch format when it is
> # more up to date than the default formats provided.

Cheers,
mwh