Merge lp:~sinzui/bzr-gtk/gpush-do-push into lp:bzr-gtk

[Curtis Hovey]

Update gpush.do_push to use fast, non-deprecated calls.

do_push() call branch.revision_history() and and PullResult.__init__
which are deprecated. Working with revision_history slows down the push.

--------------------------------------------------------------------

RULES

    * Add tests for the current do_push().
    * Base a new do_push on bzrlib.push which uses create_clone_on_transport
      or push_branch to do the task and it is much faster.
      * Do not call deprecated methods.
    * Remove the old do_push() with the new do_push() when the tests pass for
      both.
    * Provide a better method to show messages about the push to the user.

QA

    BZR_PLUGINS_AT=gtk@./ bzr gpush

IMPLEMENTATION

The diff is a little surprising since I had the new and old do_push
method running in parallel for a time. I struggled writing tests that
setup the three main condition of the old do_push(). The methods
push_branch() and create_clone_on_transport() handle some of the
try-except and if-else rules of the old do_push method. I had to add a
method the create a PushResult to ensure consistent data was available
to make a message. The message function makes a message for the UI based
on the old do_push message and the messages that were created for the
text UI when using `bzr push`.

[lp:~sinzui/bzr-gtk/gpush-do-push]

799. By Curtis Hovey on 2012-03-11

Merged trunk.

[Jelmer Vernooij (jelmer)]

review: Approve

=== modified file 'push.py'
--- push.py	2012-03-05 14:01:51 +0000
+++ push.py	2012-03-11 21:44:18 +0000
@@ -24,7 +24,8 @@
     errors,
     ui,
     )
-from bzrlib.bzrdir import BzrDir
+from bzrlib.controldir import ControlDir
+from bzrlib.push import PushResult
 from bzrlib.transport import get_transport
 
 from bzrlib.plugins.gtk.dialog import (
@@ -140,43 +141,62 @@
     :param overwrite: overwrite target location if it diverged
     :return: number of revisions pushed
     """
-    transport = get_transport(location)
-    location_url = transport.base
-
+    revision_id = None
+    to_transport = get_transport(location)
     try:
-        dir_to = BzrDir.open(location_url)
-        br_to = dir_to.open_branch()
+        dir_to = ControlDir.open_from_transport(to_transport)
     except errors.NotBranchError:
-        # create a branch.
-        transport = transport.clone('..')
+        dir_to = None
+
+    if dir_to is None:
         try:
-            relurl = transport.relpath(location_url)
-            transport.mkdir(relurl)
+            br_to = br_from.create_clone_on_transport(
+                to_transport, revision_id=revision_id)
         except errors.NoSuchFile:
             response = question_dialog(
                 _i18n('Non existing parent directory'),
                 _i18n("The parent directory (%s)\ndoesn't exist. Create?") %
-                location)
+                    location)
             if response == Gtk.ResponseType.OK:
-                transport.create_prefix()
+                br_to = br_from.create_clone_on_transport(
+                    to_transport, revision_id=revision_id, create_prefix=True)
             else:
-                return
-        dir_to = br_from.bzrdir.clone(location_url,
-            revision_id=br_from.last_revision())
-        br_to = dir_to.open_branch()
-        count = len(br_to.revision_history())
+                return _i18n("Push aborted.")
+        push_result = create_push_result(br_from, br_to)
     else:
-        br_to.revision_history()
-        try:
-            tree_to = dir_to.open_workingtree()
-        except errors.NotLocalUrl:
-            # FIXME - what to do here? how should we warn the user?
-            count = br_to.pull(br_from, overwrite)
-        except errors.NoWorkingTree:
-            count = br_to.pull(br_from, overwrite)
-        else:
-            count = tree_to.pull(br_from, overwrite)
-
-    # The count var is either an int or a PushResult. PushResult is being
-    # coerced into an int, but the method is deprecated.
-    return _i18n("%d revision(s) pushed.") % int(count)
+        push_result = dir_to.push_branch(br_from, revision_id, overwrite)
+    message = create_push_message(br_from, push_result)
+    return message
+
+
+def create_push_message(br_from, push_result):
+    """Return a mesage explaining what happened during the push."""
+    messages = []
+    rev_count = br_from.revno() - push_result.old_revno
+    messages.append(_i18n("%d revision(s) pushed.") % rev_count)
+    if push_result.stacked_on is not None:
+        messages.append(_i18n("Stacked on %s.") % push_result.stacked_on)
+    if push_result.workingtree_updated is False:
+        messages.append(_i18n(
+            "\nThe working tree was not updated:"
+            "\nSee 'bzr help working-trees' for more information."))
+    return '\n'.join(messages)
+
+
+def create_push_result(br_from, br_to):
+    """Return a PushResult like one created by ControlDir.push_branch()."""
+    push_result = PushResult()
+    push_result.source_branch = br_from
+    push_result.target_branch = br_to
+    push_result.branch_push_result = None
+    push_result.master_branch = None
+    push_result.old_revno = 0
+    push_result.old_revid = br_to.last_revision()
+    push_result.workingtree_updated = None  # Not applicable to this case.
+    try:
+        push_result.stacked_on = br_to.get_stacked_on_url()
+    except (errors.UnstackableBranchFormat,
+            errors.UnstackableRepositoryFormat,
+            errors.NotStacked):
+        push_result.stacked_on = None
+    return push_result

=== modified file 'tests/test_push.py'
--- tests/test_push.py	2012-03-04 01:54:22 +0000
+++ tests/test_push.py	2012-03-11 21:44:18 +0000
@@ -133,3 +133,98 @@
             (branch, 'lp:~user/fnord/test'), push.do_push.args)
         self.assertEqual(
             {'overwrite': True}, push.do_push.kwargs)
+
+
+class DoPushTestCase(tests.TestCaseWithTransport):
+
+    def setup_ui(self):
+        set_ui_factory()
+        progress_panel = ProgressPanel()
+        ui.ui_factory.set_progress_bar_widget(progress_panel)
+        MockMethod.bind(self, progress_panel.pb, 'tick')
+        MockMethod.bind(self, progress_panel.pb, 'update')
+        MockMethod.bind(self, progress_panel.pb, 'finished')
+        return progress_panel
+
+    def make_from_branch(self):
+        from_tree = self.make_branch_and_tree('this')
+        self.build_tree(['this/a', 'this/b'])
+        from_tree.add(['a', 'b'])
+        from_tree.commit("msg")
+        return from_tree.branch
+
+    def test_do_push_without_dir(self):
+        progress_panel = self.setup_ui()
+        from_branch = self.make_from_branch()
+        message = push.do_push(from_branch, 'that', False)
+        self.assertEqual('1 revision(s) pushed.', message)
+        self.assertEqual(True, progress_panel.pb.update.called)
+        self.assertEqual(True, progress_panel.pb.finished.called)
+
+    def test_do_push_without_parent_dir(self):
+        progress_panel = self.setup_ui()
+        from_branch = self.make_from_branch()
+        MockMethod.bind(self, push, 'question_dialog', Gtk.ResponseType.OK)
+        message = push.do_push(from_branch, 'that/there', False)
+        self.assertEqual('1 revision(s) pushed.', message)
+        self.assertEqual(True, push.question_dialog.called)
+        self.assertEqual(True, progress_panel.pb.update.called)
+        self.assertEqual(True, progress_panel.pb.finished.called)
+
+    def test_do_push_without_parent_dir_aborted(self):
+        self.setup_ui()
+        from_branch = self.make_from_branch()
+        MockMethod.bind(self, push, 'question_dialog', Gtk.ResponseType.CANCEL)
+        message = push.do_push(from_branch, 'that/there', False)
+        self.assertEqual('Push aborted.', message)
+
+    def test_do_push_with_dir(self):
+        progress_panel = self.setup_ui()
+        self.make_branch_and_tree('that')
+        from_branch = self.make_from_branch()
+        message = push.do_push(from_branch, 'that', False)
+        self.assertEqual('1 revision(s) pushed.', message)
+        self.assertEqual(True, progress_panel.pb.update.called)
+        self.assertEqual(True, progress_panel.pb.finished.called)
+
+    def test_create_push_result_unstacked(self):
+        from_branch = object()
+        to_branch = self.make_branch_and_tree('that').branch
+        push_result = push.create_push_result(from_branch, to_branch)
+        self.assertIs(from_branch, push_result.source_branch)
+        self.assertIs(to_branch, push_result.target_branch)
+        self.assertIs(None, push_result.branch_push_result)
+        self.assertIs(None, push_result.master_branch)
+        self.assertEqual(0, push_result.old_revno)
+        self.assertEqual(to_branch.last_revision(), push_result.old_revid)
+        self.assertIs(None, push_result.workingtree_updated)
+        self.assertIs(None, push_result.stacked_on)
+
+    def test_create_push_result_stacked(self):
+        from_branch = object()
+        to_branch = self.make_branch_and_tree('that').branch
+        MockMethod.bind(self, to_branch, 'get_stacked_on_url', 'lp:project')
+        push_result = push.create_push_result(from_branch, to_branch)
+        self.assertEqual('lp:project', push_result.stacked_on)
+
+    def test_create_push_message_stacked_on(self):
+        from_branch = object()
+        to_branch = self.make_branch_and_tree('that').branch
+        MockMethod.bind(self, to_branch, 'get_stacked_on_url', 'lp:project')
+        push_result = push.create_push_result(from_branch, to_branch)
+        from_branch = self.make_from_branch()
+        message = push.create_push_message(from_branch, push_result)
+        self.assertEqual(
+            '1 revision(s) pushed.\nStacked on lp:project.', message)
+
+    def test_create_push_message_workingtree_updated_false(self):
+        from_branch = object()
+        to_branch = self.make_branch_and_tree('that').branch
+        push_result = push.create_push_result(from_branch, to_branch)
+        push_result.workingtree_updated = False
+        from_branch = self.make_from_branch()
+        message = push.create_push_message(from_branch, push_result)
+        self.assertEqual(
+            "1 revision(s) pushed.\n\nThe working tree was not updated:\n"
+            "See 'bzr help working-trees' for more information.",
+            message)