Merge lp:~jelmer/bzr/private-update-revisions into lp:bzr

[Jelmer Vernooij]

Make InterBranch.update_revisions private.

Remove Branch.update_revisions and make InterBranch.update_revisions private.

InterBranch.update_revisions is more of a helper method for InterBranch.pull
and InterBranch.push, this makes it private and removes the interface tests
for it (so non-GenericInterBranch implementations don't have to provide it).

The individual tests for .push and .pull already had proper test coverage.

[Vincent Ladeuil (vila)]

There is something weird in the way the diff is presented here and reviewing locally also present a different one >-/

Anyway, digging a bit to clear my surprise about:

8 - def fetch(self, from_branch, last_revision=None, fetch_spec=None):
9 + def fetch(self, from_branch, last_revision=None):

This seems to have already landed on trunk anyway.

So: approve

review: Approve

[Jelmer Vernooij (jelmer)]

It merged a branch which has landed since this MP was created, which makes the diff for this branch appear larger than it actually is.

The other branch that was merged (and removes fetch_spec as argument) can be found here: https://code.launchpad.net/~jelmer/bzr/move-interbranch-fetch2/+merge/59204

[Jelmer Vernooij (jelmer)]

Thanks for the reviews \o/

[Jelmer Vernooij (jelmer)]

sent to pqm by email

[Jelmer Vernooij (jelmer)]

sent to pqm by email

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2011-04-27 10:55:49 +0000
+++ bzrlib/branch.py	2011-05-04 20:05:59 +0000
@@ -994,21 +994,6 @@
         else:
             return (0, _mod_revision.NULL_REVISION)
 
-    def update_revisions(self, other, stop_revision=None, overwrite=False,
-            graph=None):
-        """Pull in new perfect-fit revisions.
-
-        :param other: Another Branch to pull from
-        :param stop_revision: Updated until the given revision
-        :param overwrite: Always set the branch pointer, rather than checking
-            to see if it is a proper descendant.
-        :param graph: A Graph object that can be used to query history
-            information. This can be None.
-        :return: None
-        """
-        return InterBranch.get(other, self).update_revisions(stop_revision,
-            overwrite, graph)
-
     @deprecated_method(deprecated_in((2, 4, 0)))
     def import_last_revision_info(self, source_repo, revno, revid):
         """Set the last revision info, importing from another repo if necessary.
@@ -2611,27 +2596,6 @@
                 pass
         return None
 
-    def _basic_push(self, target, overwrite, stop_revision):
-        """Basic implementation of push without bound branches or hooks.
-
-        Must be called with source read locked and target write locked.
-        """
-        result = BranchPushResult()
-        result.source_branch = self
-        result.target_branch = target
-        result.old_revno, result.old_revid = target.last_revision_info()
-        self.update_references(target)
-        if result.old_revid != stop_revision:
-            # We assume that during 'push' this repository is closer than
-            # the target.
-            graph = self.repository.get_graph(target.repository)
-            target.update_revisions(self, stop_revision,
-                overwrite=overwrite, graph=graph)
-        if self._push_should_merge_tags():
-            result.tag_conflicts = self.tags.merge_to(target.tags, overwrite)
-        result.new_revno, result.new_revid = target.last_revision_info()
-        return result
-
     def get_stacked_on_url(self):
         raise errors.UnstackableBranchFormat(self._format, self.user_url)
 
@@ -3287,20 +3251,6 @@
         raise NotImplementedError(self.pull)
 
     @needs_write_lock
-    def update_revisions(self, stop_revision=None, overwrite=False,
-            graph=None):
-        """Pull in new perfect-fit revisions.
-
-        :param stop_revision: Updated until the given revision
-        :param overwrite: Always set the branch pointer, rather than checking
-            to see if it is a proper descendant.
-        :param graph: A Graph object that can be used to query history
-            information. This can be None.
-        :return: None
-        """
-        raise NotImplementedError(self.update_revisions)
-
-    @needs_write_lock
     def push(self, overwrite=False, stop_revision=None,
              _override_hook_source_branch=None):
         """Mirror the source branch into the target branch.
@@ -3384,9 +3334,8 @@
             self.source.unlock()
 
     @needs_write_lock
-    def update_revisions(self, stop_revision=None, overwrite=False,
+    def _update_revisions(self, stop_revision=None, overwrite=False,
             graph=None):
-        """See InterBranch.update_revisions()."""
         other_revno, other_last_revision = self.source.last_revision_info()
         stop_revno = None # unknown
         if stop_revision is None:
@@ -3478,6 +3427,28 @@
         finally:
             self.source.unlock()
 
+    def _basic_push(self, overwrite, stop_revision):
+        """Basic implementation of push without bound branches or hooks.
+
+        Must be called with source read locked and target write locked.
+        """
+        result = BranchPushResult()
+        result.source_branch = self.source
+        result.target_branch = self.target
+        result.old_revno, result.old_revid = self.target.last_revision_info()
+        self.source.update_references(self.target)
+        if result.old_revid != stop_revision:
+            # We assume that during 'push' this repository is closer than
+            # the target.
+            graph = self.source.repository.get_graph(self.target.repository)
+            self._update_revisions(stop_revision, overwrite=overwrite,
+                    graph=graph)
+        if self.source._push_should_merge_tags():
+            result.tag_conflicts = self.source.tags.merge_to(self.target.tags,
+                overwrite)
+        result.new_revno, result.new_revid = self.target.last_revision_info()
+        return result
+
     def _push_with_bound_branches(self, overwrite, stop_revision,
             _override_hook_source_branch=None):
         """Push from source into target, and into target's master if any.
@@ -3498,12 +3469,12 @@
             master_branch.lock_write()
             try:
                 # push into the master from the source branch.
-                self.source._basic_push(master_branch, overwrite, stop_revision)
-                # and push into the target branch from the source. Note that we
-                # push from the source branch again, because it's considered the
-                # highest bandwidth repository.
-                result = self.source._basic_push(self.target, overwrite,
-                    stop_revision)
+                master_inter = InterBranch.get(self.source, master_branch)
+                master_inter._basic_push(overwrite, stop_revision)
+                # and push into the target branch from the source. Note that
+                # we push from the source branch again, because it's considered
+                # the highest bandwidth repository.
+                result = self._basic_push(overwrite, stop_revision)
                 result.master_branch = master_branch
                 result.local_branch = self.target
                 _run_hooks()
@@ -3512,8 +3483,7 @@
                 master_branch.unlock()
         else:
             # no master branch
-            result = self.source._basic_push(self.target, overwrite,
-                stop_revision)
+            result = self._basic_push(overwrite, stop_revision)
             # TODO: Why set master_branch and local_branch if there's no
             # binding?  Maybe cleaner to just leave them unset? -- mbp
             # 20070504
@@ -3562,8 +3532,8 @@
             # -- JRV20090506
             result.old_revno, result.old_revid = \
                 self.target.last_revision_info()
-            self.target.update_revisions(self.source, stop_revision,
-                overwrite=overwrite, graph=graph)
+            self._update_revisions(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

=== modified file 'bzrlib/tests/per_branch/test_update.py'
--- bzrlib/tests/per_branch/test_update.py	2011-01-10 21:31:59 +0000
+++ bzrlib/tests/per_branch/test_update.py	2011-05-04 20:05:59 +0000
@@ -70,54 +70,6 @@
         self.assertEqual('foo', child_tree.branch.update())
         self.assertEqual(['bar'], child_tree.branch.revision_history())
 
-
-class TestUpdateRevisions(per_branch.TestCaseWithBranch):
-
-    def test_accepts_graph(self):
-        # An implementation may not use it, but it should allow a 'graph' to be
-        # supplied
-        tree1 = self.make_branch_and_tree('tree1')
-        rev1 = tree1.commit('one')
-        tree2 = tree1.bzrdir.sprout('tree2').open_workingtree()
-        rev2 = tree2.commit('two')
-
-        tree1.lock_write()
-        self.addCleanup(tree1.unlock)
-        tree2.lock_read()
-        self.addCleanup(tree2.unlock)
-        graph = tree2.branch.repository.get_graph(tree1.branch.repository)
-
-        tree1.branch.update_revisions(tree2.branch, graph=graph)
-        self.assertEqual((2, rev2), tree1.branch.last_revision_info())
-
-    def test_overwrite_ignores_diverged(self):
-        tree1 = self.make_branch_and_tree('tree1')
-        rev1 = tree1.commit('one')
-        tree2 = tree1.bzrdir.sprout('tree2').open_workingtree()
-        rev2 = tree1.commit('two')
-        rev2b = tree2.commit('alt two')
-
-        self.assertRaises(errors.DivergedBranches,
-                          tree1.branch.update_revisions,
-                          tree2.branch, overwrite=False)
-        # However, the revision should be copied into the repository
-        self.assertTrue(tree1.branch.repository.has_revision(rev2b))
-
-        tree1.branch.update_revisions(tree2.branch, overwrite=True)
-        self.assertEqual((2, rev2b), tree1.branch.last_revision_info())
-
-    def test_ignores_older_unless_overwrite(self):
-        tree1 = self.make_branch_and_tree('tree1')
-        rev1 = tree1.commit('one')
-        tree2 = tree1.bzrdir.sprout('tree2').open_workingtree()
-        rev2 = tree1.commit('two')
-
-        tree1.branch.update_revisions(tree2.branch)
-        self.assertEqual((2, rev2), tree1.branch.last_revision_info())
-
-        tree1.branch.update_revisions(tree2.branch, overwrite=True)
-        self.assertEqual((1, rev1), tree1.branch.last_revision_info())
-
     def test_update_in_checkout_of_readonly(self):
         tree1 = self.make_branch_and_tree('tree1')
         rev1 = tree1.commit('one')

=== modified file 'bzrlib/tests/per_interbranch/__init__.py'
--- bzrlib/tests/per_interbranch/__init__.py	2011-03-26 01:53:34 +0000
+++ bzrlib/tests/per_interbranch/__init__.py	2011-05-04 20:05:59 +0000
@@ -171,7 +171,6 @@
         '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',
         ])
     scenarios = make_scenarios(default_test_list())
     return multiply_tests(submod_tests, scenarios, standard_tests)

=== removed file 'bzrlib/tests/per_interbranch/test_update_revisions.py'
--- bzrlib/tests/per_interbranch/test_update_revisions.py	2010-06-17 09:23:19 +0000
+++ bzrlib/tests/per_interbranch/test_update_revisions.py	1970-01-01 00:00:00 +0000
@@ -1,70 +0,0 @@
-# Copyright (C) 2009, 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
-
-from bzrlib import (
-    errors,
-    )
-from bzrlib.tests.per_interbranch import (
-    TestCaseWithInterBranch,
-    )
-
-
-class TestUpdateRevisions(TestCaseWithInterBranch):
-
-    def setUp(self):
-        super(TestUpdateRevisions, self).setUp()
-        self.tree1 = self.make_from_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)
-        self.tree2 = branch2.bzrdir.create_workingtree()
-
-    def test_accepts_graph(self):
-        # An implementation may not use it, but it should allow a 'graph' to be
-        # supplied
-        rev2 = self.tree2.commit('two')
-
-        self.tree1.lock_write()
-        self.addCleanup(self.tree1.unlock)
-        self.tree2.lock_read()
-        self.addCleanup(self.tree2.unlock)
-        graph = self.tree2.branch.repository.get_graph(
-            self.tree1.branch.repository)
-
-        self.tree1.branch.update_revisions(self.tree2.branch, graph=graph)
-        self.assertEqual((2, rev2), self.tree1.branch.last_revision_info())
-
-    def test_overwrite_ignores_diverged(self):
-        rev2 = self.tree1.commit('two')
-        rev2b = self.tree2.commit('alt two')
-
-        self.assertRaises(errors.DivergedBranches,
-                          self.tree1.branch.update_revisions,
-                          self.tree2.branch, overwrite=False)
-        # However, the revision should be copied into the repository
-        self.assertTrue(self.tree1.branch.repository.has_revision(rev2b))
-
-        self.tree1.branch.update_revisions(self.tree2.branch, overwrite=True)
-        self.assertEqual((2, rev2b), self.tree1.branch.last_revision_info())
-
-    def test_ignores_older_unless_overwrite(self):
-        rev2 = self.tree1.commit('two')
-
-        self.tree1.branch.update_revisions(self.tree2.branch)
-        self.assertEqual((2, rev2), self.tree1.branch.last_revision_info())
-
-        self.tree1.branch.update_revisions(self.tree2.branch, overwrite=True)

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-04-28 14:12:08 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-05-04 20:05:59 +0000
@@ -171,6 +171,10 @@
 .. Changes that may require updates in plugins or other code that uses
    bzrlib.
 
+* ``Branch.update_revisions`` has been made private and should no
+  longer be used by external users. Use ``Branch.pull`` or ``Branch.push``
+  instead. (Jelmer Vernooij, #771765)
+
 * Commands now have an `invoked_as` attribute, showing the name under
   which they were called before alias expansion.
   (Martin Pool)
@@ -425,6 +429,9 @@
   ``RepositoryFormat.supports_leaving_lock`` flags have been added.
   (Jelmer Vernooij)
 
+* ``Branch.fetch`` implementations must now accept an optional
+  ``fetch_tags`` keyword argument. (Andrew Bennetts)
+
 * ``Branch.import_last_revision_info`` is deprecated.  Use the
   ``import_last_revision_info_and_tags`` method instead.
   (Andrew Bennetts)