Launchpad itself

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

  1. branch-upgrade-out-of-place
  2. Merge into devel
Proposed by Paul Hummer
Status: Merged
Approved by: Michael Hudson-Doyle
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~rockstar/launchpad/branch-upgrade-out-of-place
Merge into: lp:launchpad
Diff against target: 116 lines
2 files modified
lib/lp/code/model/branchjob.py (+31/-10)
lib/lp/code/model/tests/test_branchjob.py (+14/-13)
To merge this branch: bzr merge lp:~rockstar/launchpad/branch-upgrade-out-of-place
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Paul Hummer (community) Approve
Review via email: mp+13747@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Paul Hummer (rockstar) wrote :

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

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :
Download full text (5.8 KiB)

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

Read more...

review: Approve
Revision history for this message
Michael Hudson-Doyle (mwhudson) :
review: Needs Fixing
Revision history for this message
Paul Hummer (rockstar) wrote :
Download full text (9.1 KiB)

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

Read more...

review: Approve
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/model/branchjob.py'
2--- lib/lp/code/model/branchjob.py 2009-10-20 06:03:46 +0000
3+++ lib/lp/code/model/branchjob.py 2009-10-22 04:39:17 +0000
4@@ -11,12 +11,15 @@
5 import os
6 import shutil
7 from StringIO import StringIO
8+import tempfile
9
10+from bzrlib.branch import Branch as BzrBranch
11 from bzrlib.bzrdir import BzrDirMetaFormat1
12 from bzrlib.log import log_formatter, show_log
13 from bzrlib.diff import show_diff_trees
14 from bzrlib.revision import NULL_REVISION
15 from bzrlib.revisionspec import RevisionInfo, RevisionSpec
16+from bzrlib.transport import get_transport
17 from bzrlib.upgrade import upgrade
18
19 from lazr.enum import DBEnumeratedType, DBItem
20@@ -252,16 +255,34 @@
21
22 def run(self):
23 """See `IBranchUpgradeJob`."""
24- self._prepare_upgrade()
25- self._upgrade()
26-
27- def _prepare_upgrade(self):
28- """Prepares the branch for upgrade."""
29- self._upgrade_branch = self.branch.getBzrBranch()
30-
31- def _upgrade(self):
32- """Performs the upgrade of the branch."""
33- upgrade(self._upgrade_branch.base, self.upgrade_format)
34+ # Set up the new branch structure
35+ upgrade_branch_path = tempfile.mkdtemp()
36+ try:
37+ upgrade_transport = get_transport(upgrade_branch_path)
38+ source_branch_transport = get_transport(self.branch.getPullURL())
39+ source_branch_transport.copy_tree_to_transport(upgrade_transport)
40+ upgrade_branch = BzrBranch.open(upgrade_transport.base)
41+
42+ # Perform the upgrade.
43+ upgrade(upgrade_branch.base, self.upgrade_format)
44+
45+ # Re-open the branch, since its format has changed.
46+ upgrade_branch = BzrBranch.open_from_transport(
47+ upgrade_transport)
48+ source_branch = BzrBranch.open_from_transport(
49+ source_branch_transport)
50+
51+ source_branch.lock_write()
52+ upgrade_branch.pull(source_branch)
53+ upgrade_branch.fetch(source_branch)
54+ source_branch.unlock()
55+
56+ # Move the branch in the old format to backup.bzr
57+ source_branch_transport.rename('.bzr', 'backup.bzr')
58+ upgrade_transport.delete_tree('backup.bzr')
59+ upgrade_transport.copy_tree_to_transport(source_branch_transport)
60+ finally:
61+ shutil.rmtree(upgrade_branch_path)
62
63 @property
64 def upgrade_format(self):
65
66=== modified file 'lib/lp/code/model/tests/test_branchjob.py'
67--- lib/lp/code/model/tests/test_branchjob.py 2009-10-05 14:18:33 +0000
68+++ lib/lp/code/model/tests/test_branchjob.py 2009-10-22 04:39:17 +0000
69@@ -183,6 +183,18 @@
70
71 layer = LaunchpadZopelessLayer
72
73+ def make_format(self, branch_format=None, repo_format=None):
74+ # Return a Bzr MetaDir format with the provided branch and repository
75+ # formats.
76+ if branch_format is None:
77+ branch_format = BzrBranchFormat7
78+ if repo_format is None:
79+ repo_format = RepositoryFormatKnitPack6
80+ format = BzrDirMetaFormat1()
81+ format.set_branch_format(branch_format())
82+ format._set_repository_format(repo_format())
83+ return format
84+
85 def test_providesInterface(self):
86 """Ensure that BranchUpgradeJob implements IBranchUpgradeJob."""
87 branch = self.factory.makeAnyBranch()
88@@ -192,7 +204,8 @@
89 def test_upgrades_branch(self):
90 """Ensure that a branch with an outdated format is upgraded."""
91 self.useBzrBranches()
92- db_branch, tree = self.create_branch_and_tree(format='knit')
93+ db_branch, tree = self.create_branch_and_tree(
94+ hosted=True, format='knit')
95 db_branch.branch_format = BranchFormat.BZR_BRANCH_5
96 db_branch.repository_format = RepositoryFormat.BZR_KNIT_1
97 self.assertEqual(
98@@ -224,18 +237,6 @@
99 REPOSITORY_FORMAT_UPGRADE_PATH.get(
100 RepositoryFormat.BZR_REPOSITORY_4))
101
102- def make_format(self, branch_format=None, repo_format=None):
103- # Return a Bzr MetaDir format with the provided branch and repository
104- # formats.
105- if branch_format is None:
106- branch_format = BzrBranchFormat7
107- if repo_format is None:
108- repo_format = RepositoryFormatKnitPack6
109- format = BzrDirMetaFormat1()
110- format.set_branch_format(branch_format())
111- format._set_repository_format(repo_format())
112- return format
113-
114 def test_upgrade_format_no_branch_upgrade_needed(self):
115 # getUpgradeFormat should not downgrade the branch format when it is
116 # more up to date than the default formats provided.