launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/upgrade-badly-stacked into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1039558 in Launchpad itself: "upgrade_all_branches script fails on cross-format stacking"
  https://bugs.launchpad.net/launchpad/+bug/1039558

For more details, see:
https://code.launchpad.net/~abentley/launchpad/upgrade-badly-stacked/+merge/120854

= Summary =
Fix bug #1039558: upgrade_all_branches script fails on cross-format stacking

== Proposed fix ==
Implement and use ignore_fallbacks parameter to safe_open.

== Pre-implementation notes ==
None

== LOC Rationale ==
I have a LOC credit of 1860.  Also, once we run this script, we can delete all the associated code.

== Implementation details ==
bzr's upgrade functionality was designed to support upgrading a branch that was stacked on different-format branch, since that is inevitable.  (Upgrade either a stacked or stacked-on branch, and you leave the other branch in that situation.)  The ignore_fallbacks flag on Branch.open is designed to support such upgrades, but we weren't using it.  It wasn't exposed on BzrBranch.getBzrBranch, but since that's a short method, it was simpler to just use safe_open directly.

As a driveby, I fixed old stacking-related tests to forgo specifying a format.  Evidently, these tests were written before the default format supported stacking (i.e. before 2a became the default).  Now, it's just confusing to specify a format.

I extracted force_stacked_on from testSelfStackedBranch, so that we have a standardized, resuable way of forcing stacking.

== Tests ==
bin/test -m 'test_upgrade|test_safe_open'

== Demo and Q/A ==
None

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/codehosting/tests/helpers.py
  lib/lp/codehosting/tests/test_upgrade.py
  lib/lp/codehosting/tests/test_safe_open.py
  lib/lp/codehosting/safe_open.py
  lib/lp/codehosting/upgrade.py
-- 
https://code.launchpad.net/~abentley/launchpad/upgrade-badly-stacked/+merge/120854
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/upgrade-badly-stacked into lp:launchpad.

=== modified file 'lib/lp/codehosting/safe_open.py'
--- lib/lp/codehosting/safe_open.py	2012-06-29 08:40:05 +0000
+++ lib/lp/codehosting/safe_open.py	2012-08-22 19:10:27 +0000
@@ -326,7 +326,7 @@
             redirected)
         return format.open(transport)
 
-    def open(self, url):
+    def open(self, url, ignore_fallbacks=False):
         """Open the Bazaar branch at url, first checking for safety.
 
         What safety means is defined by a subclasses `followReference` and
@@ -334,20 +334,21 @@
         """
         url = self.checkAndFollowBranchReference(url)
 
-        def open_branch(url):
+        def open_branch(url, ignore_fallbacks):
             dir = self._open_dir(url)
-            return dir.open_branch()
+            return dir.open_branch(ignore_fallbacks=ignore_fallbacks)
         return self.runWithTransformFallbackLocationHookInstalled(
-            open_branch, url)
-
-
-def safe_open(allowed_scheme, url):
+            open_branch, url, ignore_fallbacks)
+
+
+def safe_open(allowed_scheme, url, ignore_fallbacks=False):
     """Open the branch at `url`, only accessing URLs on `allowed_scheme`.
 
     :raises BadUrl: An attempt was made to open a URL that was not on
         `allowed_scheme`.
     """
-    return SafeBranchOpener(SingleSchemePolicy(allowed_scheme)).open(url)
+    return SafeBranchOpener(SingleSchemePolicy(allowed_scheme)).open(url,
+                            ignore_fallbacks=ignore_fallbacks)
 
 
 SafeBranchOpener.install_hook()

=== modified file 'lib/lp/codehosting/tests/helpers.py'
--- lib/lp/codehosting/tests/helpers.py	2012-01-01 02:58:52 +0000
+++ lib/lp/codehosting/tests/helpers.py	2012-08-22 19:10:27 +0000
@@ -10,6 +10,7 @@
     'CodeHostingTestProviderAdapter',
     'create_branch_with_one_revision',
     'deferToThread',
+    'force_stacked_on_url',
     'LoomTestMixin',
     'make_bazaar_branch_and_tree',
     'TestResultWrapper',
@@ -187,6 +188,16 @@
     return tree
 
 
+def force_stacked_on_url(branch, url):
+    """Set the stacked_on url of a branch without standard error-checking.
+
+    Bazaar 1.17 and up make it harder to create branches with invalid
+    stacking.  It's still worth testing that we don't blow up in the face of
+    them, so this function lets us create them anyway.
+    """
+    branch.get_config().set_user_option('stacked_on_location', url)
+
+
 class TestResultWrapper:
     """A wrapper for `TestResult` that knows about bzrlib's `TestSkipped`."""
 

=== modified file 'lib/lp/codehosting/tests/test_safe_open.py'
--- lib/lp/codehosting/tests/test_safe_open.py	2011-12-15 10:09:59 +0000
+++ lib/lp/codehosting/tests/test_safe_open.py	2012-08-22 19:10:27 +0000
@@ -32,6 +32,9 @@
     SafeBranchOpener,
     WhitelistPolicy,
     )
+from lp.codehosting.tests.helpers import (
+    force_stacked_on_url,
+    )
 from lp.testing import TestCase
 
 
@@ -195,8 +198,8 @@
     def testAllowedURL(self):
         # checkSource does not raise an exception for branches stacked on
         # branches with allowed URLs.
-        stacked_on_branch = self.make_branch('base-branch', format='1.6')
-        stacked_branch = self.make_branch('stacked-branch', format='1.6')
+        stacked_on_branch = self.make_branch('base-branch')
+        stacked_branch = self.make_branch('stacked-branch')
         stacked_branch.set_stacked_on_url(stacked_on_branch.base)
         opener = self.makeBranchOpener(
             [stacked_branch.base, stacked_on_branch.base])
@@ -215,8 +218,8 @@
     def testAllowedRelativeURL(self):
         # checkSource passes on absolute urls to checkOneURL, even if the
         # value of stacked_on_location in the config is set to a relative URL.
-        stacked_on_branch = self.make_branch('base-branch', format='1.6')
-        stacked_branch = self.make_branch('stacked-branch', format='1.6')
+        stacked_on_branch = self.make_branch('base-branch')
+        stacked_branch = self.make_branch('stacked-branch')
         stacked_branch.set_stacked_on_url('../base-branch')
         opener = self.makeBranchOpener(
             [stacked_branch.base, stacked_on_branch.base])
@@ -229,10 +232,10 @@
     def testAllowedRelativeNested(self):
         # Relative URLs are resolved relative to the stacked branch.
         self.get_transport().mkdir('subdir')
-        a = self.make_branch('subdir/a', format='1.6')
-        b = self.make_branch('b', format='1.6')
+        a = self.make_branch('subdir/a')
+        b = self.make_branch('b')
         b.set_stacked_on_url('../subdir/a')
-        c = self.make_branch('subdir/c', format='1.6')
+        c = self.make_branch('subdir/c')
         c.set_stacked_on_url('../../b')
         opener = self.makeBranchOpener([c.base, b.base, a.base])
         # This doesn't raise an exception.
@@ -241,8 +244,8 @@
     def testForbiddenURL(self):
         # checkSource raises a BadUrl exception if a branch is stacked on a
         # branch with a forbidden URL.
-        stacked_on_branch = self.make_branch('base-branch', format='1.6')
-        stacked_branch = self.make_branch('stacked-branch', format='1.6')
+        stacked_on_branch = self.make_branch('base-branch')
+        stacked_branch = self.make_branch('stacked-branch')
         stacked_branch.set_stacked_on_url(stacked_on_branch.base)
         opener = self.makeBranchOpener([stacked_branch.base])
         self.assertRaises(BadUrl, opener.open, stacked_branch.base)
@@ -250,10 +253,10 @@
     def testForbiddenURLNested(self):
         # checkSource raises a BadUrl exception if a branch is stacked on a
         # branch that is in turn stacked on a branch with a forbidden URL.
-        a = self.make_branch('a', format='1.6')
-        b = self.make_branch('b', format='1.6')
+        a = self.make_branch('a')
+        b = self.make_branch('b')
         b.set_stacked_on_url(a.base)
-        c = self.make_branch('c', format='1.6')
+        c = self.make_branch('c')
         c.set_stacked_on_url(b.base)
         opener = self.makeBranchOpener([c.base, b.base])
         self.assertRaises(BadUrl, opener.open, c.base)
@@ -261,11 +264,8 @@
     def testSelfStackedBranch(self):
         # checkSource raises StackingLoopError if a branch is stacked on
         # itself. This avoids infinite recursion errors.
-        a = self.make_branch('a', format='1.6')
-        # Bazaar 1.17 and up make it harder to create branches like this.
-        # It's still worth testing that we don't blow up in the face of them,
-        # so we grovel around a bit to create one anyway.
-        a.get_config().set_user_option('stacked_on_location', a.base)
+        a = self.make_branch('a')
+        force_stacked_on_url(a, a.base)
         opener = self.makeBranchOpener([a.base])
         self.assertRaises(BranchLoopError, opener.open, a.base)
 
@@ -273,8 +273,8 @@
         # checkSource raises StackingLoopError if a branch is stacked in such
         # a way so that it is ultimately stacked on itself. e.g. a stacked on
         # b stacked on a.
-        a = self.make_branch('a', format='1.6')
-        b = self.make_branch('b', format='1.6')
+        a = self.make_branch('a')
+        b = self.make_branch('b')
         a.set_stacked_on_url(b.base)
         b.set_stacked_on_url(a.base)
         opener = self.makeBranchOpener([a.base, b.base])
@@ -283,8 +283,8 @@
 
     def testCustomOpener(self):
         # A custom function for opening a control dir can be specified.
-        a = self.make_branch('a', format='2a')
-        b = self.make_branch('b', format='2a')
+        a = self.make_branch('a')
+        b = self.make_branch('b')
         b.set_stacked_on_url(a.base)
 
         TrackingProber.seen_urls = []
@@ -296,7 +296,7 @@
 
     def testCustomOpenerWithBranchReference(self):
         # A custom function for opening a control dir can be specified.
-        a = self.make_branch('a', format='2a')
+        a = self.make_branch('a')
         b_dir = self.make_bzrdir('b')
         b = BranchReferenceFormat().initialize(b_dir, target_branch=a)
         TrackingProber.seen_urls = []
@@ -306,6 +306,20 @@
         self.assertEquals(
             set(TrackingProber.seen_urls), set([b.base, a.base]))
 
+    def test_ignore_fallbacks(self):
+        """"Cross-format stacking doesn't error with ignore_fallbacks."""
+        stacked, stacked_on = make_cross_format_stacked(self)
+        opener = self.makeBranchOpener([stacked.base, stacked_on.base])
+        opener.open(stacked.base, ignore_fallbacks=True)
+
+
+def make_cross_format_stacked(test_case):
+    test_case.get_transport().mkdir('inside')
+    stacked = test_case.make_branch('inside/stacked', format='1.6')
+    stacked_on = test_case.make_branch('inside/stacked-on', format='2a')
+    force_stacked_on_url(stacked, stacked_on.base)
+    return stacked, stacked_on
+
 
 class TestSafeOpen(TestCaseWithTransport):
     """Tests for `safe_open`."""
@@ -361,3 +375,10 @@
             self.get_url('outside/stacked-on'))
         self.assertRaises(
             BadUrl, safe_open, scheme, get_chrooted_url('stacked'))
+
+    def test_ignore_fallbacks(self):
+        """"Cross-format stacking doesn't error with ignore_fallbacks."""
+        scheme, get_chrooted_url = self.get_chrooted_scheme('inside')
+        stacked, stacked_on = make_cross_format_stacked(self)
+        force_stacked_on_url(stacked, get_chrooted_url('stacked-on'))
+        safe_open(scheme, get_chrooted_url('stacked'), ignore_fallbacks=True)

=== modified file 'lib/lp/codehosting/tests/test_upgrade.py'
--- lib/lp/codehosting/tests/test_upgrade.py	2012-06-14 05:18:22 +0000
+++ lib/lp/codehosting/tests/test_upgrade.py	2012-08-22 19:10:27 +0000
@@ -29,6 +29,7 @@
     )
 from lp.codehosting.bzrutils import read_locked
 from lp.codehosting.upgrade import Upgrader
+from lp.codehosting.tests.helpers import force_stacked_on_url
 from lp.services.config import config
 from lp.testing import TestCaseWithFactory
 from lp.testing.layers import ZopelessDatabaseLayer
@@ -55,16 +56,19 @@
             bzr_branch = tree.branch
         return self.getUpgrader(bzr_branch, branch)
 
+    def getTargetDir(self, bzr_branch):
+        return self.useFixture(TempDir(
+            rootdir=dirname(config.codehosting.mirrored_branches_root))).path
+
     def getUpgrader(self, bzr_branch, branch):
         """Return an upgrader for the specified branches.
 
         :param bzr_branch: the bzr branch to use.
         :param branch: The DB branch to use.
         """
-        target_dir = self.useFixture(TempDir(
-            rootdir=dirname(config.codehosting.mirrored_branches_root))).path
         return Upgrader(
-            branch, target_dir, logging.getLogger(), bzr_branch)
+            branch, self.getTargetDir(bzr_branch), logging.getLogger(),
+            bzr_branch)
 
     def addTreeReference(self, tree):
         """Add a tree reference to a tree and commit.
@@ -270,3 +274,12 @@
             upgraded.repository._format.__class__)
         self.assertEqual(
             'foo', upgraded.repository.get_revision('prepare-commit').message)
+
+    def test_invalid_stacking(self):
+        """Upgrade tolerates branches stacked on different-format branches."""
+        self.useBzrBranches(direct_database=True)
+        target, target_tree = self.create_branch_and_tree(format='1.6')
+        trunk, trunk_tree = self.create_branch_and_tree(format='2a')
+        force_stacked_on_url(target_tree.branch, trunk_tree.branch.base)
+        Upgrader(target, self.getTargetDir(target_tree.branch),
+                 logging.getLogger())

=== modified file 'lib/lp/codehosting/upgrade.py'
--- lib/lp/codehosting/upgrade.py	2012-03-22 23:21:24 +0000
+++ lib/lp/codehosting/upgrade.py	2012-08-22 19:10:27 +0000
@@ -36,6 +36,7 @@
     )
 from lp.code.model.branch import Branch
 from lp.codehosting.bzrutils import read_locked
+from lp.codehosting.safe_open import safe_open
 from lp.codehosting.vfs.branchfs import get_real_branch_path
 from lp.services.database.lpstorm import IStore
 
@@ -51,7 +52,9 @@
         self.branch = branch
         self.bzr_branch = bzr_branch
         if self.bzr_branch is None:
-            self.bzr_branch = self.branch.getBzrBranch()
+            self.bzr_branch = safe_open('lp-internal',
+                                        self.branch.getInternalBzrUrl(),
+                                        ignore_fallbacks=True)
         self.target_dir = target_dir
         self.target_subdir = os.path.join(
             self.target_dir, str(self.branch.id))