Rev 4051: Fix race condition with branch hooks during cloning when the new branch is stacked. in http://people.ubuntu.com/~robertc/baz2.0/clone.branch-takes-strategy

[Robert Collins]

At http://people.ubuntu.com/~robertc/baz2.0/clone.branch-takes-strategy

------------------------------------------------------------
revno: 4051
revision-id: robertc at robertcollins.net-20090225221725-0795sbskjouaxbm5
parent: pqm at pqm.ubuntu.com-20090225171156-l63eiz2bz51ialsg
committer: Robert Collins <robertc at robertcollins.net>
branch nick: clone.branch-takes-strategy
timestamp: Thu 2009-02-26 09:17:25 +1100
message:
  Fix race condition with branch hooks during cloning when the new branch is stacked.
=== modified file 'NEWS'
--- a/NEWS	2009-02-25 14:36:59 +0000
+++ b/NEWS	2009-02-25 22:17:25 +0000
@@ -88,6 +88,13 @@
       via RPC calls rather than VFS calls, reducing round trips for
       pushing new branches substantially. (Robert Collins)
 
+    * ``Branch.clone`` now takes the ``repository_policy`` formerly used
+      inside ``BzrDir.clone_on_transport``, allowing stacking to be
+      configured before the branch tags and revision tip are set. This
+      fixes a race condition cloning stacked branches that would cause
+      plugins to have hooks called on non-stacked instances.
+      (Robert Collins, #334187)
+
     * ``BzrDirFormat.__str__`` now uses the human readable description
       rather than the sometimes-absent disk label. (Robert Collins)
 

=== modified file 'bzrlib/__init__.py'
--- a/bzrlib/__init__.py	2009-02-13 21:21:25 +0000
+++ b/bzrlib/__init__.py	2009-02-25 22:17:25 +0000
@@ -53,8 +53,8 @@
 version_info = (1, 13, 0, 'dev', 0)
 
 
-# API compatibility version: bzrlib is currently API compatible with 1.11.
-api_minimum_version = (1, 11, 0)
+# API compatibility version: bzrlib is currently API compatible with 1.13.
+api_minimum_version = (1, 13, 0)
 
 
 def _format_version_tuple(version_info):

=== modified file 'bzrlib/branch.py'
--- a/bzrlib/branch.py	2009-02-25 15:36:48 +0000
+++ b/bzrlib/branch.py	2009-02-25 22:17:25 +0000
@@ -913,7 +913,7 @@
             raise errors.InvalidRevisionNumber(revno)
 
     @needs_read_lock
-    def clone(self, to_bzrdir, revision_id=None):
+    def clone(self, to_bzrdir, revision_id=None, repository_policy=None):
         """Clone this branch into to_bzrdir preserving all semantic values.
 
         Most API users will want 'create_clone_on_transport', which creates a
@@ -923,6 +923,8 @@
                      be truncated to end with revision_id.
         """
         result = to_bzrdir.create_branch()
+        if repository_policy is not None:
+            repository_policy.configure_branch(result)
         self.copy_content_into(result, revision_id=revision_id)
         return  result
 

=== modified file 'bzrlib/bzrdir.py'
--- a/bzrlib/bzrdir.py	2009-02-25 05:36:18 +0000
+++ b/bzrlib/bzrdir.py	2009-02-25 22:17:25 +0000
@@ -229,9 +229,8 @@
         #   make sure its content is available in the target repository
         #   clone it.
         if local_branch is not None:
-            result_branch = local_branch.clone(result, revision_id=revision_id)
-            if repository_policy is not None:
-                repository_policy.configure_branch(result_branch)
+            result_branch = local_branch.clone(result, revision_id=revision_id,
+                repository_policy=repository_policy)
         try:
             # Cheaper to check if the target is not local, than to try making
             # the tree and fail.

=== modified file 'bzrlib/tests/branch_implementations/test_create_clone.py'
--- a/bzrlib/tests/branch_implementations/test_create_clone.py	2009-02-25 03:22:12 +0000
+++ b/bzrlib/tests/branch_implementations/test_create_clone.py	2009-02-25 22:17:25 +0000
@@ -16,7 +16,9 @@
 
 """Tests for branch.create_clone behaviour."""
 
+from bzrlib.branch import Branch
 from bzrlib.tests.branch_implementations.test_branch import TestCaseWithBranch
+from bzrlib import remote
 
 
 class TestCreateClone(TestCaseWithBranch):
@@ -54,3 +56,30 @@
             stacked_on=trunk.base)
         self.assertEqual(revid, result.last_revision())
         self.assertEqual(trunk.base, result.get_stacked_on_url())
+
+    def assertBranchHookBranchIsStacked(self, pre_change_params):
+        # Just calling will either succeed or fail.
+        pre_change_params.branch.get_stacked_on_url()
+        self.hook_calls.append(pre_change_params)
+
+    def test_create_clone_on_transport_stacked_hooks_get_stacked_branch(self):
+        tree = self.make_branch_and_tree('source')
+        tree.commit('a commit')
+        trunk = tree.branch.create_clone_on_transport(
+            self.get_transport('trunk'))
+        revid = tree.commit('a second commit')
+        source = tree.branch
+        target_transport = self.get_transport('target')
+        self.hook_calls = []
+        Branch.hooks.install_named_hook("pre_change_branch_tip",
+            self.assertBranchHookBranchIsStacked, None)
+        result = tree.branch.create_clone_on_transport(target_transport,
+            stacked_on=trunk.base)
+        self.assertEqual(revid, result.last_revision())
+        self.assertEqual(trunk.base, result.get_stacked_on_url())
+        # Smart servers invoke hooks on both sides
+        if isinstance(result, remote.RemoteBranch):
+            expected_calls = 2
+        else:
+            expected_calls = 1
+        self.assertEqual(expected_calls, len(self.hook_calls))