Merge lp:~lifeless/bzr/loomsupport into lp:bzr

[Robert Collins]

InterBranch improvements needed for bzr-loom support.

Essentially, move Branch.copy_content_into to a InterBranch method.

Opportunistically:
 - cleanup a crazy InterBranch structural issue that had me confused while
   doing this.
 - add tests to ensure that that issue can't resurrect itself

[Vincent Ladeuil (vila)]

Tests failing:
ERROR: bzrlib.tests.blackbox.test_push.TestPush.test_push_smart_tags_streaming_acceptance
ERROR: bzrlib.tests.blackbox.test_switch.TestSwitch.test_switch_lightweight_directory
ERROR: bzrlib.tests.blackbox.test_tags.TestTagging.test_branch_push_pull_merge_copies_tags
ERROR: bzrlib.tests.blackbox.test_tags.TestTagging.test_list_tags

I haven't reviewed yet.

review: Needs Fixing

[Robert Collins (lifeless)]

They work for me. Perhaps you have a non-core plugin interfering?

[Vincent Ladeuil (vila)]

Sounds fine, please land.

review: Approve

[Robert Collins (lifeless)]

sent to pqm by email

[lp:~lifeless/bzr/loomsupport]

5290. By Canonical.com Patch Queue Manager <email address hidden> on 2010-06-14

Document branch.get_config

5291. By Canonical.com Patch Queue Manager <email address hidden> on 2010-06-14

Fetch progress bar update now happens every 7 records instead of 71.
 Progress bar is better on slower connections.

5292. By Canonical.com Patch Queue Manager <email address hidden> on 2010-06-14

Improved ``bzrlib.urlutils`` to handle lp:foo/bar URLs.

[Robert Collins (lifeless)]

sent to pqm by email

[lp:~lifeless/bzr/loomsupport]

5293. By Canonical.com Patch Queue Manager <email address hidden> on 2010-06-14

(lifeless) InterBranch improvements needed for bzr-loom support. (Robert
 Collins)

=== modified file 'NEWS'
--- NEWS	2010-06-14 17:58:24 +0000
+++ NEWS	2010-06-14 21:46:28 +0000
@@ -34,29 +34,35 @@
   (Martin [gz], Parth Malwankar, #501307)
 
 * ``bzr log --exclude-common-ancestry`` is now taken into account for
-  linear ancetries.
-  (Vincent Ladeuil, #575631)
+  linear ancetries. (Vincent Ladeuil, #575631)
 
 * Ensure that wrong path specifications in ``BZR_PLUGINS_AT`` display
-  proper error messages.  
-  (Vincent Ladeuil, #591215)
+  proper error messages. (Vincent Ladeuil, #591215)
+
+* Explicitly removing ``--profile-imports`` option from parsed command-line
+  arguments on Windows, because bzr script does the same.
+  (Alexander Belchenko, #588277)
+
+* Fetching was slightly confused about the best code to use and was
+  using a new code path for all branches, resulting in more lookups than
+  necessary on old branches. (Robert Collins, #593515)
 
 * Final fix for 'no help for command' issue. We now show a clean message
   when a command has no help, document how to set help more clearly, and
   test that all commands available to the test suite have help.
   (Robert Collins, #177500)
 
-* Explicitly removing ``--profile-imports`` option from parsed command-line
-  arguments on Windows, because bzr script does the same.
-  (Alexander Belchenko, #588277)
-
 * Relative imports in plugins are now handled correctly when using
-  BZR_PLUGINS_AT.
-  (Vincent Ladeuil, #588959)
+  BZR_PLUGINS_AT. (Vincent Ladeuil, #588959)
 
 Improvements
 ************
 
+* ``Branch.copy_content_into`` is now a convenience method dispatching to
+  a ``InterBranch`` multi-method. This permits ``bzr-loom`` and other
+  plugins to intercept this even when a ``RemoteBranch`` proxy is in use.
+  (Robert Collins, #201613)
+
 * Use lazy imports in ``bzrlib/merge.py`` so that plugins like ``news_merge``
   do not cause modules to be loaded unnecessarily just because the plugin
   registers a merge hook.  This improves ``bzr rocks`` time by about 25%

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2010-06-07 20:25:05 +0000
+++ bzrlib/branch.py	2010-06-14 21:46:28 +0000
@@ -29,6 +29,7 @@
         errors,
         lockdir,
         lockable_files,
+        remote,
         repository,
         revision as _mod_revision,
         rio,
@@ -968,7 +969,6 @@
                 raise errors.NoSuchRevision(self, stop_revision)
         return other_history[self_len:stop_revision]
 
-    @needs_write_lock
     def update_revisions(self, other, stop_revision=None, overwrite=False,
                          graph=None):
         """Pull in new perfect-fit revisions.
@@ -1272,24 +1272,14 @@
                 revno = 1
         destination.set_last_revision_info(revno, revision_id)
 
-    @needs_read_lock
     def copy_content_into(self, destination, revision_id=None):
         """Copy the content of self into destination.
 
         revision_id: if not None, the revision history in the new branch will
                      be truncated to end with revision_id.
         """
-        self.update_references(destination)
-        self._synchronize_history(destination, revision_id)
-        try:
-            parent = self.get_parent()
-        except errors.InaccessibleParent, e:
-            mutter('parent was not accessible to copy: %s', e)
-        else:
-            if parent:
-                destination.set_parent(parent)
-        if self._push_should_merge_tags():
-            self.tags.merge_to(destination.tags)
+        return InterBranch.get(self, destination).copy_content_into(
+            revision_id=revision_id)
 
     def update_references(self, target):
         if not getattr(self._format, 'supports_reference_locations', False):
@@ -3233,110 +3223,112 @@
 
 
 class GenericInterBranch(InterBranch):
-    """InterBranch implementation that uses public Branch functions.
-    """
+    """InterBranch implementation that uses public Branch functions."""
+
+    @classmethod
+    def is_compatible(klass, source, target):
+        # GenericBranch uses the public API, so always compatible
+        return True
 
     @staticmethod
     def _get_branch_formats_to_test():
         return BranchFormat._default_format, BranchFormat._default_format
 
+    @classmethod
+    def unwrap_format(klass, format):
+        if isinstance(format, remote.RemoteBranchFormat):
+            format._ensure_real()
+            return format._custom_format
+        return format                                                                                                  
+
+    @needs_write_lock
+    def copy_content_into(self, revision_id=None):
+        """Copy the content of source into target
+
+        revision_id: if not None, the revision history in the new branch will
+                     be truncated to end with revision_id.
+        """
+        self.source.update_references(self.target)
+        self.source._synchronize_history(self.target, revision_id)
+        try:
+            parent = self.source.get_parent()
+        except errors.InaccessibleParent, e:
+            mutter('parent was not accessible to copy: %s', e)
+        else:
+            if parent:
+                self.target.set_parent(parent)
+        if self.source._push_should_merge_tags():
+            self.source.tags.merge_to(self.target.tags)
+
+    @needs_write_lock
     def update_revisions(self, stop_revision=None, overwrite=False,
         graph=None):
         """See InterBranch.update_revisions()."""
-        self.source.lock_read()
-        try:
-            other_revno, other_last_revision = self.source.last_revision_info()
-            stop_revno = None # unknown
-            if stop_revision is None:
-                stop_revision = other_last_revision
-                if _mod_revision.is_null(stop_revision):
-                    # if there are no commits, we're done.
-                    return
-                stop_revno = other_revno
+        other_revno, other_last_revision = self.source.last_revision_info()
+        stop_revno = None # unknown
+        if stop_revision is None:
+            stop_revision = other_last_revision
+            if _mod_revision.is_null(stop_revision):
+                # if there are no commits, we're done.
+                return
+            stop_revno = other_revno
 
-            # what's the current last revision, before we fetch [and change it
-            # possibly]
-            last_rev = _mod_revision.ensure_null(self.target.last_revision())
-            # we fetch here so that we don't process data twice in the common
-            # case of having something to pull, and so that the check for
-            # already merged can operate on the just fetched graph, which will
-            # be cached in memory.
-            self.target.fetch(self.source, stop_revision)
-            # Check to see if one is an ancestor of the other
-            if not overwrite:
-                if graph is None:
-                    graph = self.target.repository.get_graph()
-                if self.target._check_if_descendant_or_diverged(
-                        stop_revision, last_rev, graph, self.source):
-                    # stop_revision is a descendant of last_rev, but we aren't
-                    # overwriting, so we're done.
-                    return
-            if stop_revno is None:
-                if graph is None:
-                    graph = self.target.repository.get_graph()
-                this_revno, this_last_revision = \
-                        self.target.last_revision_info()
-                stop_revno = graph.find_distance_to_null(stop_revision,
-                                [(other_last_revision, other_revno),
-                                 (this_last_revision, this_revno)])
-            self.target.set_last_revision_info(stop_revno, stop_revision)
-        finally:
-            self.source.unlock()
+        # what's the current last revision, before we fetch [and change it
+        # possibly]
+        last_rev = _mod_revision.ensure_null(self.target.last_revision())
+        # we fetch here so that we don't process data twice in the common
+        # case of having something to pull, and so that the check for
+        # already merged can operate on the just fetched graph, which will
+        # be cached in memory.
+        self.target.fetch(self.source, stop_revision)
+        # Check to see if one is an ancestor of the other
+        if not overwrite:
+            if graph is None:
+                graph = self.target.repository.get_graph()
+            if self.target._check_if_descendant_or_diverged(
+                    stop_revision, last_rev, graph, self.source):
+                # stop_revision is a descendant of last_rev, but we aren't
+                # overwriting, so we're done.
+                return
+        if stop_revno is None:
+            if graph is None:
+                graph = self.target.repository.get_graph()
+            this_revno, this_last_revision = \
+                    self.target.last_revision_info()
+            stop_revno = graph.find_distance_to_null(stop_revision,
+                            [(other_last_revision, other_revno),
+                             (this_last_revision, this_revno)])
+        self.target.set_last_revision_info(stop_revno, stop_revision)
 
     def pull(self, overwrite=False, stop_revision=None,
-             possible_transports=None, _hook_master=None, run_hooks=True,
+             possible_transports=None, run_hooks=True,
              _override_hook_target=None, local=False):
-        """See Branch.pull.
+        """Pull from source into self, updating my master if any.
 
-        :param _hook_master: Private parameter - set the branch to
-            be supplied as the master to pull hooks.
         :param run_hooks: Private parameter - if false, this branch
             is being called because it's the master of the primary branch,
             so it should not run its hooks.
-        :param _override_hook_target: Private parameter - set the branch to be
-            supplied as the target_branch to pull hooks.
-        :param local: Only update the local branch, and not the bound branch.
         """
-        # This type of branch can't be bound.
-        if local:
+        bound_location = self.target.get_bound_location()
+        if local and not bound_location:
             raise errors.LocalRequiresBoundBranch()
-        result = PullResult()
-        result.source_branch = self.source
-        if _override_hook_target is None:
-            result.target_branch = self.target
-        else:
-            result.target_branch = _override_hook_target
-        self.source.lock_read()
+        master_branch = None
+        if not local and bound_location and self.source.user_url != bound_location:
+            # not pulling from master, so we need to update master.
+            master_branch = self.target.get_master_branch(possible_transports)
+            master_branch.lock_write()
         try:
-            # We assume that during 'pull' the target repository is closer than
-            # the source one.
-            self.source.update_references(self.target)
-            graph = self.target.repository.get_graph(self.source.repository)
-            # TODO: Branch formats should have a flag that indicates 
-            # that revno's are expensive, and pull() should honor that flag.
-            # -- JRV20090506
-            result.old_revno, result.old_revid = \
-                self.target.last_revision_info()
-            self.target.update_revisions(self.source, stop_revision,
-                overwrite=overwrite, graph=graph)
-            # TODO: The old revid should be specified when merging tags, 
-            # so a tags implementation that versions tags can only 
-            # pull in the most recent changes. -- JRV20090506
-            result.tag_conflicts = self.source.tags.merge_to(self.target.tags,
-                overwrite)
-            result.new_revno, result.new_revid = self.target.last_revision_info()
-            if _hook_master:
-                result.master_branch = _hook_master
-                result.local_branch = result.target_branch
-            else:
-                result.master_branch = result.target_branch
-                result.local_branch = None
-            if run_hooks:
-                for hook in Branch.hooks['post_pull']:
-                    hook(result)
+            if master_branch:
+                # pull from source into master.
+                master_branch.pull(self.source, overwrite, stop_revision,
+                    run_hooks=False)
+            return self._pull(overwrite,
+                stop_revision, _hook_master=master_branch,
+                run_hooks=run_hooks,
+                _override_hook_target=_override_hook_target)
         finally:
-            self.source.unlock()
-        return result
+            if master_branch:
+                master_branch.unlock()
 
     def push(self, overwrite=False, stop_revision=None,
              _override_hook_source_branch=None):
@@ -3404,48 +3396,63 @@
             _run_hooks()
             return result
 
-    @classmethod
-    def is_compatible(self, source, target):
-        # GenericBranch uses the public API, so always compatible
-        return True
-
-
-class InterToBranch5(GenericInterBranch):
-
-    @staticmethod
-    def _get_branch_formats_to_test():
-        return BranchFormat._default_format, BzrBranchFormat5()
-
-    def pull(self, overwrite=False, stop_revision=None,
-             possible_transports=None, run_hooks=True,
+    def _pull(self, overwrite=False, stop_revision=None,
+             possible_transports=None, _hook_master=None, run_hooks=True,
              _override_hook_target=None, local=False):
-        """Pull from source into self, updating my master if any.
-
+        """See Branch.pull.
+
+        This function is the core worker, used by GenericInterBranch.pull to
+        avoid duplication when pulling source->master and source->local.
+
+        :param _hook_master: Private parameter - set the branch to
+            be supplied as the master to pull hooks.
         :param run_hooks: Private parameter - if false, this branch
             is being called because it's the master of the primary branch,
             so it should not run its hooks.
+        :param _override_hook_target: Private parameter - set the branch to be
+            supplied as the target_branch to pull hooks.
+        :param local: Only update the local branch, and not the bound branch.
         """
-        bound_location = self.target.get_bound_location()
-        if local and not bound_location:
+        # This type of branch can't be bound.
+        if local:
             raise errors.LocalRequiresBoundBranch()
-        master_branch = None
-        if not local and bound_location and self.source.user_url != bound_location:
-            # not pulling from master, so we need to update master.
-            master_branch = self.target.get_master_branch(possible_transports)
-            master_branch.lock_write()
+        result = PullResult()
+        result.source_branch = self.source
+        if _override_hook_target is None:
+            result.target_branch = self.target
+        else:
+            result.target_branch = _override_hook_target
+        self.source.lock_read()
         try:
-            if master_branch:
-                # pull from source into master.
-                master_branch.pull(self.source, overwrite, stop_revision,
-                    run_hooks=False)
-            return super(InterToBranch5, self).pull(overwrite,
-                stop_revision, _hook_master=master_branch,
-                run_hooks=run_hooks,
-                _override_hook_target=_override_hook_target)
+            # We assume that during 'pull' the target repository is closer than
+            # the source one.
+            self.source.update_references(self.target)
+            graph = self.target.repository.get_graph(self.source.repository)
+            # TODO: Branch formats should have a flag that indicates 
+            # that revno's are expensive, and pull() should honor that flag.
+            # -- JRV20090506
+            result.old_revno, result.old_revid = \
+                self.target.last_revision_info()
+            self.target.update_revisions(self.source, stop_revision,
+                overwrite=overwrite, graph=graph)
+            # TODO: The old revid should be specified when merging tags, 
+            # so a tags implementation that versions tags can only 
+            # pull in the most recent changes. -- JRV20090506
+            result.tag_conflicts = self.source.tags.merge_to(self.target.tags,
+                overwrite)
+            result.new_revno, result.new_revid = self.target.last_revision_info()
+            if _hook_master:
+                result.master_branch = _hook_master
+                result.local_branch = result.target_branch
+            else:
+                result.master_branch = result.target_branch
+                result.local_branch = None
+            if run_hooks:
+                for hook in Branch.hooks['post_pull']:
+                    hook(result)
         finally:
-            if master_branch:
-                master_branch.unlock()
+            self.source.unlock()
+        return result
 
 
 InterBranch.register_optimiser(GenericInterBranch)
-InterBranch.register_optimiser(InterToBranch5)

=== modified file 'bzrlib/tests/per_branch/test_pull.py'
--- bzrlib/tests/per_branch/test_pull.py	2010-02-10 17:52:08 +0000
+++ bzrlib/tests/per_branch/test_pull.py	2010-06-14 21:46:28 +0000
@@ -98,17 +98,6 @@
                           master_tree.branch.pull, other.branch, local = True)
         self.assertEqual([rev1], master_tree.branch.revision_history())
 
-    def test_pull_raises_specific_error_on_master_connection_error(self):
-        master_tree = self.make_branch_and_tree('master')
-        checkout = master_tree.branch.create_checkout('checkout')
-        other = master_tree.branch.bzrdir.sprout('other').open_workingtree()
-        # move the branch out of the way on disk to cause a connection
-        # error.
-        os.rename('master', 'master_gone')
-        # try to pull, which should raise a BoundBranchConnectionFailure.
-        self.assertRaises(errors.BoundBranchConnectionFailure,
-                checkout.branch.pull, other.branch)
-
     def test_pull_returns_result(self):
         parent = self.make_branch_and_tree('parent')
         parent.commit('1st post', rev_id='P1')

=== modified file 'bzrlib/tests/per_interbranch/__init__.py'
--- bzrlib/tests/per_interbranch/__init__.py	2009-05-07 05:08:46 +0000
+++ bzrlib/tests/per_interbranch/__init__.py	2010-06-14 21:46:28 +0000
@@ -144,8 +144,38 @@
         return newbranch.bzrdir
 
 
+class StubWithFormat(object):
+    """A stub object used to check that convenience methods call Inter's."""
+
+    _format = object()
+
+
+class StubMatchingInter(object):
+    """An inter for tests.
+
+    This is not a subclass of InterBranch so that missing methods are caught
+    and added rather than actually trying to do something.
+    """
+
+    _uses = []
+
+    def __init__(self, source, target):
+        self.source = source
+        self.target = target
+
+    @classmethod
+    def is_compatible(klass, source, target):
+        return StubWithFormat._format in (source._format, target._format)
+
+    def copy_content_into(self, *args, **kwargs):
+        self.__class__._uses.append(
+            (self, 'copy_content_into', args, kwargs))
+
+
 def load_tests(standard_tests, module, loader):
     submod_tests = loader.loadTestsFromModuleNames([
+        'bzrlib.tests.per_interbranch.test_get',
+        'bzrlib.tests.per_interbranch.test_copy_content_into',
         'bzrlib.tests.per_interbranch.test_pull',
         'bzrlib.tests.per_interbranch.test_push',
         'bzrlib.tests.per_interbranch.test_update_revisions',

=== added file 'bzrlib/tests/per_interbranch/test_copy_content_into.py'
--- bzrlib/tests/per_interbranch/test_copy_content_into.py	1970-01-01 00:00:00 +0000
+++ bzrlib/tests/per_interbranch/test_copy_content_into.py	2010-06-14 21:46:28 +0000
@@ -0,0 +1,47 @@
+# Copyright (C) 2010 Canonical Ltd
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+"""Tests for bzrlib.branch.InterBranch.copy_content_into."""
+
+from bzrlib import branch
+from bzrlib.tests.per_interbranch import (
+    StubMatchingInter,
+    StubWithFormat,
+    TestCaseWithInterBranch,
+    )
+
+
+class TestCopyContentInto(TestCaseWithInterBranch):
+
+    def test_contract_convenience_method(self):
+        self.tree1 = self.make_branch_and_tree('tree1')
+        rev1 = self.tree1.commit('one')
+        branch2 = self.make_to_branch('tree2')
+        branch2.repository.fetch(self.tree1.branch.repository)
+        self.tree1.branch.copy_content_into(branch2, revision_id=rev1)
+
+    def test_inter_is_used(self):
+        self.tree1 = self.make_branch_and_tree('tree1')
+        self.addCleanup(branch.InterBranch.unregister_optimiser,
+            StubMatchingInter)
+        branch.InterBranch.register_optimiser(StubMatchingInter)
+        del StubMatchingInter._uses[:]
+        self.tree1.branch.copy_content_into(StubWithFormat(), revision_id='54')
+        self.assertLength(1, StubMatchingInter._uses)
+        use = StubMatchingInter._uses[0]
+        self.assertEqual('copy_content_into', use[1])
+        self.assertEqual('54', use[3]['revision_id'])
+        del StubMatchingInter._uses[:]

=== added file 'bzrlib/tests/per_interbranch/test_get.py'
--- bzrlib/tests/per_interbranch/test_get.py	1970-01-01 00:00:00 +0000
+++ bzrlib/tests/per_interbranch/test_get.py	2010-06-14 21:46:28 +0000
@@ -0,0 +1,34 @@
+# Copyright (C) 2010 Canonical Ltd
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+"""Tests for bzrlib.branch.InterBranch.get."""
+
+from bzrlib import (
+    branch,
+    )
+from bzrlib.tests.per_interbranch import (
+    TestCaseWithInterBranch,
+    )
+
+
+class TestCopyContentInto(TestCaseWithInterBranch):
+
+    def test_gets_right_inter(self):
+        self.tree1 = self.make_branch_and_tree('tree1')
+        branch2 = self.make_to_branch('tree2')
+        self.assertIs(branch.InterBranch.get(
+            self.tree1.branch, branch2).__class__,
+            self.interbranch_class)

=== modified file 'bzrlib/tests/per_interbranch/test_pull.py'
--- bzrlib/tests/per_interbranch/test_pull.py	2009-07-10 05:49:34 +0000
+++ bzrlib/tests/per_interbranch/test_pull.py	2010-06-14 21:46:28 +0000
@@ -76,13 +76,13 @@
     def test_pull_raises_specific_error_on_master_connection_error(self):
         master_tree = self.make_from_branch_and_tree('master')
         checkout = master_tree.branch.create_checkout('checkout')
-        other = self.sprout_to(master_tree.branch.bzrdir, 'other').open_workingtree()
+        other = self.sprout_to(master_tree.branch.bzrdir, 'other').open_branch()
         # move the branch out of the way on disk to cause a connection
         # error.
         os.rename('master', 'master_gone')
         # try to pull, which should raise a BoundBranchConnectionFailure.
         self.assertRaises(errors.BoundBranchConnectionFailure,
-                checkout.branch.pull, other.branch)
+                checkout.branch.pull, other)
 
     def test_pull_returns_result(self):
         parent = self.make_from_branch_and_tree('parent')