Launchpad itself

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

branch-upgrade-out-of-place
Merge into devel

Revision history for this message

Paul Hummer (rockstar) wrote on 2009-10-22:

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

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

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

review: Approve

« Back to merge proposal