Merge lp:~mbp/bzr/45719-update-r into lp:bzr

[Martin Pool]

[Martin Pool (mbp)]

This adds a 'bzr update -r' option. If the branch is bound to another, it always updates from the master branch first. This updated form of the patch addresses John's review comments in <https://bugs.edge.launchpad.net/bzr/+bug/45719/comments/20>.

This is an updated version of what was one of the longest-standing merges, and it's for a five-digit bug too <https://bugs.edge.launchpad.net/bzr/+bug/45719>.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/45719-update-r into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #45719 update command cannot take a revision number
> https://bugs.launchpad.net/bugs/45719
>
>
> This adds a 'bzr update -r' option. If the branch is bound to another, it always updates from the master branch first. This updated form of the patch addresses John's review comments in <https://bugs.edge.launchpad.net/bzr/+bug/45719/comments/20>.
>
> This is an updated version of what was one of the longest-standing merges, and it's for a five-digit bug too <https://bugs.edge.launchpad.net/bzr/+bug/45719>.
>

v- does 'change_reporter' need to be inside the try/except ? I think it
just got refactored into there.

+ try:
+ change_reporter = delta._ChangeReporter(
+ unversioned_filter=tree.is_ignored,
+ view_info=view_info)
+ conflicts = tree.update(
+ change_reporter,
+ possible_transports=possible_transports,
+ revision=rev,
+ old_tip=old_tip)
+ except errors.NoSuchRevision, e:
+ raise errors.BzrCommandError(
+ "branch has no revision %s\n"
+ "bzr update --revision only works"
+ " for a revision in the branch
history"
+ % (e.revision))

+ if revision is not None:
+ rev = revision[0].as_revision_id(branch)
+ else:
+ rev = branch.last_revision()

^- It might be clearer as 'rev_id' or 'revision_id = '. We have some
ambiguous naming (is it a Revision, revision_id, or RevSpec...) However,
this is really minor.

 review: approve
 merge: approve

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAksw26oACgkQJdeBCYSNAAO00gCeOtH7vUpSUdkD8xTtoyOgod/g
sdIAn3ABG2q0RkqjrPIhgsm9lv5V5Ofs
=IrMf
-----END PGP SIGNATURE-----

review: Approve

=== modified file 'NEWS'
--- NEWS	2009-12-23 05:04:12 +0000
+++ NEWS	2009-12-23 06:33:19 +0000
@@ -23,11 +23,10 @@
   ``locations.conf`` or ``branch.conf``.
   (Ted Gould, Matthew Fuller, Vincent Ladeuil)
 
-* ``bzrlib.tests.permute_for_extension`` is a helper that simplifies
-  running all tests in the current module, once against a pure python
-  implementation, and once against an extension (pyrex/C) implementation.
-  It can be used to dramatically simplify the implementation of
-  ``load_tests``.  (John Arbash Meinel)
+* ``bzr update`` now takes a ``--revision`` argument. This lets you
+  change the revision of the working tree to any revision in the
+  ancestry of the current or master branch. (Matthieu Moy, Mark Hammond,
+  Martin Pool, #45719)
 
 Bug Fixes
 *********
@@ -60,6 +59,9 @@
   CamelCase. For the features that were more likely to be used, we added a
   deprecation thunk, but not all. (John Arbash Meinel)
 
+* ``WorkingTree.update`` implementations must now accept a ``revision``
+  parameter.
+
 Internals
 *********
 
@@ -71,6 +73,12 @@
 Testing
 *******
 
+* ``bzrlib.tests.permute_for_extension`` is a helper that simplifies
+  running all tests in the current module, once against a pure python
+  implementation, and once against an extension (pyrex/C) implementation.
+  It can be used to dramatically simplify the implementation of
+  ``load_tests``.  (John Arbash Meinel)
+
 * ``bzrlib.tests.TestCase`` now subclasses ``testtools.testcase.TestCase``.
   This permits features in testtools such as getUniqueInteger and
   getUniqueString to be used. Because of this, testtools version 0.9.2 or
 
=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2009-12-21 17:24:22 +0000
+++ bzrlib/builtins.py	2009-12-23 06:33:19 +0000
@@ -1388,16 +1388,24 @@
 
     If you want to discard your local changes, you can just do a
     'bzr revert' instead of 'bzr commit' after the update.
+
+    If the tree's branch is bound to a master branch, it will also update
+    the branch from the master.
     """
 
     _see_also = ['pull', 'working-trees', 'status-flags']
     takes_args = ['dir?']
+    takes_options = ['revision']
     aliases = ['up']
 
-    def run(self, dir='.'):
+    def run(self, dir='.', revision=None):
+        if revision is not None and len(revision) != 1:
+            raise errors.BzrCommandError(
+                        "bzr update --revision takes exactly one revision")
         tree = WorkingTree.open_containing(dir)[0]
+        branch = tree.branch
         possible_transports = []
-        master = tree.branch.get_master_branch(
+        master = branch.get_master_branch(
             possible_transports=possible_transports)
         if master is not None:
             tree.lock_write()
@@ -1410,20 +1418,38 @@
                                                         self.outf.encoding)
         try:
             existing_pending_merges = tree.get_parent_ids()[1:]
-            last_rev = _mod_revision.ensure_null(tree.last_revision())
-            if last_rev == _mod_revision.ensure_null(
-                tree.branch.last_revision()):
-                # may be up to date, check master too.
-                if master is None or last_rev == _mod_revision.ensure_null(
-                    master.last_revision()):
-                    revno = tree.branch.revision_id_to_revno(last_rev)
-                    note('Tree is up to date at revision %d of branch %s'
-                         % (revno, branch_location))
-                    return 0
+            if master is None:
+                old_tip = None
+            else:
+                # may need to fetch data into a heavyweight checkout
+                # XXX: this may take some time, maybe we should display a
+                # message
+                old_tip = branch.update(possible_transports)
+            if revision is not None:
+                revision_id = revision[0].as_revision_id(branch)
+            else:
+                revision_id = branch.last_revision()
+            if revision_id == _mod_revision.ensure_null(tree.last_revision()):
+                revno = branch.revision_id_to_revno(revision_id)
+                note("Tree is up to date at revision %d of branch %s" %
+                    (revno, branch_location))
+                return 0
             view_info = _get_view_info_for_change_reporter(tree)
-            conflicts = tree.update(
-                delta._ChangeReporter(unversioned_filter=tree.is_ignored,
-                view_info=view_info), possible_transports=possible_transports)
+            change_reporter = delta._ChangeReporter(
+                unversioned_filter=tree.is_ignored,
+                view_info=view_info)
+            try:
+                conflicts = tree.update(
+                    change_reporter,
+                    possible_transports=possible_transports,
+                    revision=revision_id,
+                    old_tip=old_tip)
+            except errors.NoSuchRevision, e:
+                raise errors.BzrCommandError(
+                                      "branch has no revision %s\n"
+                                      "bzr update --revision only works"
+                                      " for a revision in the branch history"
+                                      % (e.revision))
             revno = tree.branch.revision_id_to_revno(
                 _mod_revision.ensure_null(tree.last_revision()))
             note('Updated to revision %d of branch %s' %
 
=== modified file 'bzrlib/tests/blackbox/test_update.py'
--- bzrlib/tests/blackbox/test_update.py	2009-12-14 15:51:36 +0000
+++ bzrlib/tests/blackbox/test_update.py	2009-12-23 06:33:19 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006 Canonical Ltd
+# Copyright (C) 2006, 2009 Canonical Ltd
 # -*- coding: utf-8 -*-
 #
 # This program is free software; you can redistribute it and/or modify
@@ -29,6 +29,7 @@
     urlutils,
     workingtree,
     )
+from bzrlib.tests.script import ScriptRunner
 
 
 class TestUpdate(tests.TestCaseWithTransport):
@@ -67,11 +68,11 @@
     def test_update_up_to_date_checkout(self):
         self.make_branch_and_tree('branch')
         self.run_bzr('checkout branch checkout')
-        out, err = self.run_bzr('update checkout')
-        self.assertEqual('Tree is up to date at revision 0 of branch %s\n'
-                         % osutils.pathjoin(self.test_dir, 'branch'),
-                         err)
-        self.assertEqual('', out)
+        sr = ScriptRunner()
+        sr.run_script(self, '''
+$ bzr update checkout
+2>Tree is up to date at revision 0 of branch .../branch
+''')
 
     def test_update_out_of_date_standalone_tree(self):
         # FIXME the default format has to change for this to pass
@@ -239,3 +240,78 @@
                                                    lightweight=True)
         tree.commit('empty commit')
         self.run_bzr('update checkout')
+
+    def test_update_dash_r(self):
+        # Test that 'bzr update' works correctly when you have
+        # an update in the master tree, and a lightweight checkout
+        # which has merged another branch
+        master = self.make_branch_and_tree('master')
+        os.chdir('master')
+        self.build_tree(['./file1'])
+        master.add(['file1'])
+        master.commit('one', rev_id='m1')
+        self.build_tree(['./file2'])
+        master.add(['file2'])
+        master.commit('two', rev_id='m2')
+
+        sr = ScriptRunner()
+        sr.run_script(self, '''
+$ bzr update -r 1
+2>-D  file2
+2>All changes applied successfully.
+2>Updated to revision 1 of .../master
+''')
+        self.failUnlessExists('./file1')
+        self.failIfExists('./file2')
+        self.assertEquals(['m1'], master.get_parent_ids())
+
+    def test_update_dash_r_outside_history(self):
+        # Test that 'bzr update' works correctly when you have
+        # an update in the master tree, and a lightweight checkout
+        # which has merged another branch
+        master = self.make_branch_and_tree('master')
+        self.build_tree(['master/file1'])
+        master.add(['file1'])
+        master.commit('one', rev_id='m1')
+
+        # Create a second branch, with an extra commit
+        other = master.bzrdir.sprout('other').open_workingtree()
+        self.build_tree(['other/file2'])
+        other.add(['file2'])
+        other.commit('other2', rev_id='o2')
+
+        os.chdir('master')
+        self.run_bzr('merge ../other')
+        master.commit('merge', rev_id='merge')
+
+        out, err = self.run_bzr('update -r revid:o2',
+                                retcode=3)
+        self.assertEqual('', out)
+        self.assertEqual('bzr: ERROR: branch has no revision o2\n'
+                         'bzr update --revision only works'
+                         ' for a revision in the branch history\n',
+                         err)
+
+    def test_update_dash_r_in_master(self):
+        # Test that 'bzr update' works correctly when you have
+        # an update in the master tree,
+        master = self.make_branch_and_tree('master')
+        self.build_tree(['master/file1'])
+        master.add(['file1'])
+        master.commit('one', rev_id='m1')
+
+        self.run_bzr('checkout master checkout')
+
+        # add a revision in the master.
+        self.build_tree(['master/file2'])
+        master.add(['file2'])
+        master.commit('two', rev_id='m2')
+
+        os.chdir('checkout')
+        sr = ScriptRunner()
+        sr.run_script(self, '''
+$ bzr update -r revid:m2
+2>+N  file2
+2>All changes applied successfully.
+2>Updated to revision 2 of branch .../master
+''')
 
=== modified file 'bzrlib/tests/per_workingtree/test_workingtree.py'
--- bzrlib/tests/per_workingtree/test_workingtree.py	2009-11-08 05:28:57 +0000
+++ bzrlib/tests/per_workingtree/test_workingtree.py	2009-12-23 06:33:19 +0000
@@ -590,6 +590,20 @@
         self.assertEqual(master_tree.branch.revision_history(),
             tree.branch.revision_history())
 
+    def test_update_takes_revision_parameter(self):
+        wt = self.make_branch_and_tree('wt')
+        self.build_tree_contents([('wt/a', 'old content')])
+        wt.add(['a'])
+        rev1 = wt.commit('first master commit')
+        self.build_tree_contents([('wt/a', 'new content')])
+        rev2 = wt.commit('second master commit')
+        # https://bugs.edge.launchpad.net/bzr/+bug/45719/comments/20
+        # when adding 'update -r' we should make sure all wt formats support
+        # it
+        conflicts = wt.update(revision=rev1)
+        self.assertFileEqual('old content', 'wt/a')
+        self.assertEqual([rev1], wt.get_parent_ids())
+
     def test_merge_modified_detects_corruption(self):
         # FIXME: This doesn't really test that it works; also this is not
         # implementation-independent. mbp 20070226
 
=== modified file 'bzrlib/workingtree.py'
--- bzrlib/workingtree.py	2009-12-02 18:15:55 +0000
+++ bzrlib/workingtree.py	2009-12-23 06:33:19 +0000
@@ -2191,7 +2191,10 @@
         """
         raise NotImplementedError(self.unlock)
 
-    def update(self, change_reporter=None, possible_transports=None):
+    _marker = object()
+
+    def update(self, change_reporter=None, possible_transports=None,
+               revision=None, old_tip=_marker):
         """Update a working tree along its branch.
 
         This will update the branch if its bound too, which means we have
@@ -2215,10 +2218,16 @@
         - Merge current state -> basis tree of the master w.r.t. the old tree
           basis.
         - Do a 'normal' merge of the old branch basis if it is relevant.
+
+        :param revision: The target revision to update to. Must be in the
+            revision history.
+        :param old_tip: If branch.update() has already been run, the value it
+            returned (old tip of the branch or None). _marker is used
+            otherwise.
         """
         if self.branch.get_bound_location() is not None:
             self.lock_write()
-            update_branch = True
+            update_branch = (old_tip is self._marker)
         else:
             self.lock_tree_write()
             update_branch = False
@@ -2226,13 +2235,14 @@
             if update_branch:
                 old_tip = self.branch.update(possible_transports)
             else:
-                old_tip = None
-            return self._update_tree(old_tip, change_reporter)
+                if old_tip is self._marker:
+                    old_tip = None
+            return self._update_tree(old_tip, change_reporter, revision)
         finally:
             self.unlock()
 
     @needs_tree_write_lock
-    def _update_tree(self, old_tip=None, change_reporter=None):
+    def _update_tree(self, old_tip=None, change_reporter=None, revision=None):
         """Update a tree to the master branch.
 
         :param old_tip: if supplied, the previous tip revision the branch,
@@ -2253,12 +2263,17 @@
             last_rev = self.get_parent_ids()[0]
         except IndexError:
             last_rev = _mod_revision.NULL_REVISION
-        if last_rev != _mod_revision.ensure_null(self.branch.last_revision()):
-            # merge tree state up to new branch tip.
+        if revision is None:
+            revision = self.branch.last_revision()
+        else:
+            if revision not in self.branch.revision_history():
+                raise errors.NoSuchRevision(self.branch, revision)
+        if last_rev != _mod_revision.ensure_null(revision):
+            # merge tree state up to specified revision.
             basis = self.basis_tree()
             basis.lock_read()
             try:
-                to_tree = self.branch.basis_tree()
+                to_tree = self.branch.repository.revision_tree(revision)
                 if basis.inventory.root is None:
                     self.set_root_id(to_tree.get_root_id())
                     self.flush()
@@ -2268,11 +2283,12 @@
                                       basis,
                                       this_tree=self,
                                       change_reporter=change_reporter)
+                self.set_last_revision(revision)
             finally:
                 basis.unlock()
             # TODO - dedup parents list with things merged by pull ?
             # reuse the tree we've updated to to set the basis:
-            parent_trees = [(self.branch.last_revision(), to_tree)]
+            parent_trees = [(revision, to_tree)]
             merges = self.get_parent_ids()[1:]
             # Ideally we ask the tree for the trees here, that way the working
             # tree can decide whether to give us the entire tree or give us a
@@ -2308,8 +2324,7 @@
             #       should be able to remove this extra flush.
             self.flush()
             graph = self.branch.repository.get_graph()
-            base_rev_id = graph.find_unique_lca(self.branch.last_revision(),
-                                                old_tip)
+            base_rev_id = graph.find_unique_lca(revision, old_tip)
             base_tree = self.branch.repository.revision_tree(base_rev_id)
             other_tree = self.branch.repository.revision_tree(old_tip)
             result += merge.merge_inner(