Merge lp:~rockstar/launchpad/branch-upgrade-out-of-place into lp:launchpad

Hi Michael-

  So, this branch is basically a re-write of what I had yesterday, since I
wasn't using transports. Aaron showed me the way, and so this branch is a lot
cleaner. I also didn't change any tests, just made sure the old ones passed.
I did this because I thought the tests were getting too introspective into the
implementation. Basically, I had the job split up into _pre_upgrade, _upgrade,
and _post_upgrade methods, and were testing that each method did what it was
supposed to. This led to madness, IMHO. If you think I'm wrong, I'd be more
than happy to change it back.

  I also fixed some lint that came up in the lint report. The lint warnings
about not finding lazr are craziness.

  To test: bin/test -vvc lp.code.model.tests.test_branchjob TestBranchUpgradeJob

  I've never really had to dive this far into the bzrlib stuff before, so I
expect there are probably better ways to do what I'm doing. I'm all ears.

 reviewer mwhudson

Cheers,
Paul

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

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

 merge approve

All but fine now...

Paul Hummer wrote:

> === 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')

You don't need this line.

> + upgrade_transport.copy_tree_to_transport(source_branch_transport)
> + finally:
> + shutil.rmtree(upgrade_branch_path)
>
> @property
> def upgrade_format(self):

Cheers,
mwh
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkrf4tAACgkQeTTOPm7A7kjE3gCfcqpSHwpQHkYyUFe6nBycGfSb
gdAAoILC14HcnXFuSv0kbBRrhRDD9StK
=zuVZ
-----END PGP SIGNATURE-----