Merge lp:~jml/bzr/default-stacking-bug-385132 into lp:~bzr/bzr/trunk-old

[Jonathan Lange]

[Jonathan Lange (jml)]

Bazaar 1.15 added a verb for initialize_ex which broke certain default stacking policies, notably the one that Launchpad uses. This patch attempts to fix this regression by returning relative URLs when the stacking policy itself has a relative URL.

There are two interesting cases. One is where the policy looks like:
  default_stack_on=/foo/bar/baz

and the other where it looks like:
  default_stack_on=trunk

Current versions of Bazaar send these as ('/foo/bar/baz', 'chroot-123124:///') and ('trunk', 'chroot-15245:///). This patch changes them to be sent as ('/foo/bar/baz', '.') and ('trunk', '..').

Note that the fix touches both client and server side. That means that 1.15 clients will forever be broken when pushing new branches to Launchpad. This sucks, can we do something about it?

I think the rest of the patch is fairly simple: it adds some tests, trying to be as direct as possible; it changes the client to handle relative final_stack_pwd; it changes the server to generate relative final_stack & final_stack_pwd; it cleans up some lint & spelling mistakes found along the way.

This bug is blocking the 1.16rc1 release. I would greatly appreciate a swift review.

Thanks,
jml

[Andrew Bennetts (spiv)]

This feedback has already been given over IRC, but for the record:

38 + full_path = self._root_client_path + final_stack[1:]
39 + final_stack = urlutils.relative_url(
40 + target_transport.base, full_path)

didn't make sense to me.

What would make sense to me would be s/full_path/client_path/, and changing the first arg of relative_url to _root_client_path.

+ self.reset_smart_call_log()

Omit self.reset_smart_call_log() calls if you aren't going to inspect the call log in the test.

Everything else seems ok, I think.

review: Approve

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2009-06-10 03:56:49 +0000
+++ bzrlib/bzrdir.py	2009-06-11 11:35:12 +0000
@@ -3249,6 +3249,9 @@
                 repo_bzr = bzrdir
             final_stack = response[8] or None
             final_stack_pwd = response[9] or None
+            if final_stack_pwd:
+                final_stack_pwd = urlutils.join(
+                    transport.base, final_stack_pwd)
             remote_repo = remote.RemoteRepository(repo_bzr, repo_format)
             if len(response) > 10:
                 # Updated server verb that locks remotely.

=== modified file 'bzrlib/smart/bzrdir.py'
--- bzrlib/smart/bzrdir.py	2009-04-28 03:55:56 +0000
+++ bzrlib/smart/bzrdir.py	2009-06-11 11:35:12 +0000
@@ -17,7 +17,7 @@
 """Server-side bzrdir related request implmentations."""
 
 
-from bzrlib import branch, errors, repository
+from bzrlib import branch, errors, repository, urlutils
 from bzrlib.bzrdir import (
     BzrDir,
     BzrDirFormat,
@@ -407,6 +407,19 @@
             repo.unlock()
         final_stack = final_stack or ''
         final_stack_pwd = final_stack_pwd or ''
+
+        # We want this to be relative to the bzrdir.
+        if final_stack_pwd:
+            final_stack_pwd = urlutils.relative_url(
+                target_transport.base, final_stack_pwd)
+
+        # Can't meaningfully return a root path.
+        if final_stack.startswith('/'):
+            client_path = self._root_client_path + final_stack[1:]
+            final_stack = urlutils.relative_url(
+                self._root_client_path, client_path)
+            final_stack_pwd = '.'
+
         return SuccessfulSmartServerResponse((repo_path, rich_root, tree_ref,
             external_lookup, repo_name, repo_bzrdir_name,
             bzrdir._format.network_name(),

=== modified file 'bzrlib/tests/blackbox/test_push.py'
--- bzrlib/tests/blackbox/test_push.py	2009-06-10 03:56:49 +0000
+++ bzrlib/tests/blackbox/test_push.py	2009-06-11 11:35:12 +0000
@@ -215,6 +215,31 @@
         remote = branch.Branch.open('public')
         self.assertEndsWith(remote.get_stacked_on_url(), '/parent')
 
+    def test_push_smart_with_default_stacking_url_path_segment(self):
+        # If the default stacked-on location is a path element then branches
+        # we push there over the smart server are stacked and their
+        # stacked_on_url is that exact path segment. Added to nail bug 385132.
+        self.setup_smart_server_with_call_log()
+        self.make_branch('stack-on', format='1.9')
+        self.make_bzrdir('.').get_config().set_default_stack_on(
+            '/stack-on')
+        self.make_branch('from', format='1.9')
+        out, err = self.run_bzr(['push', '-d', 'from', self.get_url('to')])
+        b = branch.Branch.open(self.get_url('to'))
+        self.assertEqual('/extra/stack-on', b.get_stacked_on_url())
+
+    def test_push_smart_with_default_stacking_relative_path(self):
+        # If the default stacked-on location is a relative path then branches
+        # we push there over the smart server are stacked and their
+        # stacked_on_url is a relative path. Added to nail bug 385132.
+        self.setup_smart_server_with_call_log()
+        self.make_branch('stack-on', format='1.9')
+        self.make_bzrdir('.').get_config().set_default_stack_on('stack-on')
+        self.make_branch('from', format='1.9')
+        out, err = self.run_bzr(['push', '-d', 'from', self.get_url('to')])
+        b = branch.Branch.open(self.get_url('to'))
+        self.assertEqual('../stack-on', b.get_stacked_on_url())
+
     def create_simple_tree(self):
         tree = self.make_branch_and_tree('tree')
         self.build_tree(['tree/a'])

=== modified file 'bzrlib/tests/bzrdir_implementations/test_bzrdir.py'
--- bzrlib/tests/bzrdir_implementations/test_bzrdir.py	2009-04-28 03:55:56 +0000
+++ bzrlib/tests/bzrdir_implementations/test_bzrdir.py	2009-06-11 11:35:12 +0000
@@ -1246,6 +1246,36 @@
         self.assertTrue(repo.is_write_locked())
         repo.unlock()
 
+    def test_format_initialize_on_transport_ex_default_stack_on(self):
+        # When initialize_on_transport_ex uses a stacked-on branch because of
+        # a stacking policy on the target, the location of the fallback
+        # repository is the same as the external location of the stacked-on
+        # branch.
+        balloon = self.make_bzrdir('balloon')
+        if isinstance(balloon, bzrdir.BzrDirMetaFormat1):
+            stack_on = self.make_branch('stack-on', format='1.9')
+        else:
+            stack_on = self.make_branch('stack-on')
+        config = self.make_bzrdir('.').get_config()
+        try:
+            config.set_default_stack_on('stack-on')
+        except errors.BzrError:
+            raise TestNotApplicable('Only relevant for stackable formats.')
+        # Initialize a bzrdir subject to the policy.
+        t = self.get_transport('stacked')
+        repo_fmt = bzrdir.format_registry.make_bzrdir('1.9')
+        repo_name = repo_fmt.repository_format.network_name()
+        repo, control = self.assertInitializeEx(
+            t, need_meta=True, repo_format_name=repo_name, stacked_on=None)
+        if control is None:
+            # uninitialisable format
+            return
+        # There's one fallback repo, with a public location.
+        self.assertLength(1, repo._fallback_repositories)
+        fallback_repo = repo._fallback_repositories[0]
+        self.assertEqual(
+            stack_on.base, fallback_repo.bzrdir.root_transport.base)
+
     def test_format_initialize_on_transport_ex_repo_fmt_name_None(self):
         t = self.get_transport('dir')
         repo, control = self.assertInitializeEx(t)

=== modified file 'bzrlib/tests/test_bzrdir.py'
--- bzrlib/tests/test_bzrdir.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/test_bzrdir.py	2009-06-11 11:35:12 +0000
@@ -20,8 +20,6 @@
 """
 
 import os
-import os.path
-from StringIO import StringIO
 import subprocess
 import sys
 
@@ -31,7 +29,6 @@
     help_topics,
     repository,
     osutils,
-    symbol_versioning,
     remote,
     urlutils,
     win32utils,
@@ -47,7 +44,6 @@
     TestCaseWithMemoryTransport,
     TestCaseWithTransport,
     TestSkipped,
-    test_sftp_transport
     )
 from bzrlib.tests import(
     http_server,

=== modified file 'bzrlib/tests/test_smart.py'
--- bzrlib/tests/test_smart.py	2009-06-10 03:56:49 +0000
+++ bzrlib/tests/test_smart.py	2009-06-11 11:35:12 +0000
@@ -381,7 +381,7 @@
             'subdir/dir', 'False', 'False', 'False', '', '', '', '', 'False')
 
     def test_initialized_dir(self):
-        """Initializing an extant dirctory should fail like the bzrdir api."""
+        """Initializing an extant directory should fail like the bzrdir api."""
         backing = self.get_transport()
         name = self.make_bzrdir('reference')._format.network_name()
         request = smart.bzrdir.SmartServerRequestBzrDirInitializeEx(backing)