Merge lp:~ryorke/bzr/202374-pull-update-showbase into lp:bzr

[Rory Yorke]

Enable use of 3-way conflict markers in pull and update by adding --show-base option

Fixes bug 202374, --show-base for pull and update.

Code more-or-less copied from builtins.cmd_merge. In both cases show_base gets passed down to merge.merge_inner in the appropriate WorkingTree method.

Disallow --show-base for pull if not invoked on a working tree.

Tests added in tests/blackbox/test_pull.py and test_update.py.

Default unchange (i.e., by default don't show-base). The bug reporter asked for --show-base to be the default, which may be sensible, but that is a design decision that I won't take.

I've run './bzr --no-plugins selftest --parallel=fork' on my system which selftest reports as

  bzr-2.3.0dev1 python-2.6.4 Linux-2.6.31-22-generic-i686-with-Ubuntu-9.10-karmic

I get some odd results, including maximum recursion depth exceeded, and a few errors in test_test_server. I hope these are unrelated to my changes.

Running

'./bzr --no-plugins selftest --parallel=fork blackbox'

gives

OK (known_failures=2)
22 tests skipped

[Martin Packman (gz)]

Thanks for working on this Rory, the change looks pretty good to me.

You'll want to add a NEWS entry for this, see the 'Documenting Changes' section of doc/developers/HACKING.txt for details.

> I get some odd results, including maximum recursion depth exceeded, and a few errors in > test_test_server. I hope these are unrelated to my changes.

Almost certainly unrelated, but it might be worth checking if there are open bugs on any of these failures and filing if not.

Some nitpicks on the diff:

+ def run(self, dir='.', revision=None,show_base=None):

PEP 8, you want a space before show_base there.

+ open(pathjoin('a', 'hello'),'wt').write('fee')

The test lines like this are relying on refcounting to do the right thing, which is fine, and as you care about the content for these tests I see why you didn't use the normal make_branch_and_tree options, but a different spelling would be preferable.

+ #tree.update() gives no such revision, so ...
+ self.run_bzr(['update','-r1'])

As your comment notes, this isn't ideal, we only really want to be using run_bzr for commands we're actually testing. Perhaps another reviewer can suggest a better option.

Tests pass here, just need that news added and another reviewer's opinion.

review: Needs Fixing

[Martin Packman (gz)]

Great, thanks.

review: Approve

[Martin Pool (mbp)]

It would be nice to put that into per-process-context configuration,
which would avoid passing so many parameters around, but the
infrastructure for that isn't here yet.

(more review later)

--
Martin

[Martin Packman (gz)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-09-27 19:31:45 +0000
+++ NEWS	2010-09-27 20:26:42 +0000
@@ -240,6 +240,10 @@
 * New development format ``development8-subtree`` which is similar to the 
   ``2a`` format and adds subtree support. (Jelmer Vernooij)
 
+* The ``pull`` and ``update`` commands now take a ``-show-base``
+  option that, in the case of conflicts, shows the base revision text.
+  (Rory Yorke,  #202374)
+
 Bug Fixes
 *********
 
@@ -442,6 +446,11 @@
   constructor has been deprecated, use the ``file_name`` parameter instead.
   (Vincent Ladeuil)
 
+* ``WorkingTree`` methods ``pull``, ``update``, and ``_update_tree``
+  now have an optional argument, ``show_base``, which is by default
+  False.  This is flag is ultimately passed to ``merge.merge_inner``
+  in each case. (Rory Yorke, #202374)
+
 Internals
 *********
 

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-09-24 12:53:00 +0000
+++ bzrlib/builtins.py	2010-09-27 20:26:42 +0000
@@ -923,13 +923,16 @@
                  "branch.  Local pulls are not applied to "
                  "the master branch."
             ),
+        Option('show-base',
+            help="Show base revision text in conflicts.")
         ]
     takes_args = ['location?']
     encoding_type = 'replace'
 
     def run(self, location=None, remember=False, overwrite=False,
             revision=None, verbose=False,
-            directory=None, local=False):
+            directory=None, local=False,
+            show_base=False):
         # FIXME: too much stuff is in the command class
         revision_id = None
         mergeable = None
@@ -944,6 +947,9 @@
             branch_to = Branch.open_containing(directory)[0]
             self.add_cleanup(branch_to.lock_write().unlock)
 
+        if tree_to is None and show_base:
+            raise errors.BzrCommandError("Need working tree for --show-base.")
+
         if local and not branch_to.get_bound_location():
             raise errors.LocalRequiresBoundBranch()
 
@@ -994,7 +1000,8 @@
                 view_info=view_info)
             result = tree_to.pull(
                 branch_from, overwrite, revision_id, change_reporter,
-                possible_transports=possible_transports, local=local)
+                possible_transports=possible_transports, local=local,
+                show_base=show_base)
         else:
             result = branch_to.pull(
                 branch_from, overwrite, revision_id, local=local)
@@ -1363,10 +1370,13 @@
 
     _see_also = ['pull', 'working-trees', 'status-flags']
     takes_args = ['dir?']
-    takes_options = ['revision']
+    takes_options = ['revision',
+                     Option('show-base',
+                            help="Show base revision text in conflicts."),
+                     ]
     aliases = ['up']
 
-    def run(self, dir='.', revision=None):
+    def run(self, dir='.', revision=None, show_base=None):
         if revision is not None and len(revision) != 1:
             raise errors.BzrCommandError(
                         "bzr update --revision takes exactly one revision")
@@ -1412,7 +1422,8 @@
                 change_reporter,
                 possible_transports=possible_transports,
                 revision=revision_id,
-                old_tip=old_tip)
+                old_tip=old_tip,
+                show_base=show_base)
         except errors.NoSuchRevision, e:
             raise errors.BzrCommandError(
                                   "branch has no revision %s\n"

=== modified file 'bzrlib/tests/blackbox/test_pull.py'
--- bzrlib/tests/blackbox/test_pull.py	2010-06-11 07:32:12 +0000
+++ bzrlib/tests/blackbox/test_pull.py	2010-09-27 20:26:42 +0000
@@ -453,3 +453,46 @@
         out, err = self.run_bzr(['pull', '-d', 'to', 'from'])
         self.assertContainsRe(err,
             "(?m)Fetching into experimental format")
+
+    def test_pull_show_base(self):
+        """bzr pull supports --show-base
+
+        see https://bugs.launchpad.net/bzr/+bug/202374"""
+        # create two trees with conflicts, setup conflict, check that
+        # conflicted file looks correct
+        a_tree = self.example_branch('a')
+        b_tree = a_tree.bzrdir.sprout('b').open_workingtree()
+
+        f = open(pathjoin('a', 'hello'),'wt')
+        f.write('fee')
+        f.close()
+        a_tree.commit('fee')
+
+        f = open(pathjoin('b', 'hello'),'wt')
+        f.write('fie')
+        f.close()
+
+        out,err=self.run_bzr(['pull','-d','b','a','--show-base'])
+
+        # check for message here
+        self.assertEqual(err,
+                         ' M  hello\nText conflict in hello\n1 conflicts encountered.\n')
+
+        self.assertEqualDiff('<<<<<<< TREE\n'
+                             'fie||||||| BASE-REVISION\n'
+                             'foo=======\n'
+                             'fee>>>>>>> MERGE-SOURCE\n',
+                             open(pathjoin('b', 'hello')).read())
+
+    def test_pull_show_base_working_tree_only(self):
+        """--show-base only allowed if there's a working tree
+
+        see https://bugs.launchpad.net/bzr/+bug/202374"""
+        # create a branch, see that --show-base fails
+        self.make_branch('from')
+        self.make_branch('to')
+        out=self.run_bzr(['pull','-d','to','from','--show-base'],retcode=3)
+        self.assertEqual(out,
+                         ('','bzr: ERROR: Need working tree for --show-base.\n'))
+
+

=== modified file 'bzrlib/tests/blackbox/test_update.py'
--- bzrlib/tests/blackbox/test_update.py	2010-04-14 04:48:00 +0000
+++ bzrlib/tests/blackbox/test_update.py	2010-09-27 20:26:42 +0000
@@ -359,6 +359,44 @@
 2>Updated to revision 2 of branch .../master
 ''')
 
+    def test_update_show_base(self):
+        """bzr update support --show-base
+
+        see https://bugs.launchpad.net/bzr/+bug/202374"""
+
+        tree=self.make_branch_and_tree('.')
+
+        f = open('hello','wt')
+        f.write('foo')
+        f.close()
+        tree.add('hello')
+        tree.commit('fie')
+
+        f = open('hello','wt')
+        f.write('fee')
+        f.close()
+        tree.commit('fee')
+
+        #tree.update() gives no such revision, so ...
+        self.run_bzr(['update','-r1'])
+
+        #create conflict
+        f = open('hello','wt')
+        f.write('fie')
+        f.close()
+
+        out, err = self.run_bzr(['update','--show-base'],retcode=1)
+
+        # check for conflict notification
+        self.assertContainsString(err,
+                                  ' M  hello\nText conflict in hello\n1 conflicts encountered.\n')
+        
+        self.assertEqualDiff('<<<<<<< TREE\n'
+                             'fie||||||| BASE-REVISION\n'
+                             'foo=======\n'
+                             'fee>>>>>>> MERGE-SOURCE\n',
+                             open('hello').read())
+
     def test_update_checkout_prevent_double_merge(self):
         """"Launchpad bug 113809 in bzr "update performs two merges"
         https://launchpad.net/bugs/113809"""

=== modified file 'bzrlib/workingtree.py'
--- bzrlib/workingtree.py	2010-08-20 19:07:17 +0000
+++ bzrlib/workingtree.py	2010-09-27 20:26:42 +0000
@@ -1663,7 +1663,8 @@
 
     @needs_write_lock
     def pull(self, source, overwrite=False, stop_revision=None,
-             change_reporter=None, possible_transports=None, local=False):
+             change_reporter=None, possible_transports=None, local=False,
+             show_base=False):
         source.lock_read()
         try:
             old_revision_info = self.branch.last_revision_info()
@@ -1683,7 +1684,8 @@
                                 basis_tree,
                                 this_tree=self,
                                 pb=None,
-                                change_reporter=change_reporter)
+                                change_reporter=change_reporter,
+                                show_base=show_base)
                     basis_root_id = basis_tree.get_root_id()
                     new_root_id = new_basis_tree.get_root_id()
                     if basis_root_id != new_root_id:
@@ -2261,7 +2263,7 @@
     _marker = object()
 
     def update(self, change_reporter=None, possible_transports=None,
-               revision=None, old_tip=_marker):
+               revision=None, old_tip=_marker, show_base=False):
         """Update a working tree along its branch.
 
         This will update the branch if its bound too, which means we have
@@ -2304,12 +2306,13 @@
             else:
                 if old_tip is self._marker:
                     old_tip = None
-            return self._update_tree(old_tip, change_reporter, revision)
+            return self._update_tree(old_tip, change_reporter, revision, show_base)
         finally:
             self.unlock()
 
     @needs_tree_write_lock
-    def _update_tree(self, old_tip=None, change_reporter=None, revision=None):
+    def _update_tree(self, old_tip=None, change_reporter=None, revision=None,
+                     show_base=False):
         """Update a tree to the master branch.
 
         :param old_tip: if supplied, the previous tip revision the branch,
@@ -2342,7 +2345,8 @@
             other_tree = self.branch.repository.revision_tree(old_tip)
             nb_conflicts = merge.merge_inner(self.branch, other_tree,
                                              base_tree, this_tree=self,
-                                             change_reporter=change_reporter)
+                                             change_reporter=change_reporter,
+                                             show_base=show_base)
             if nb_conflicts:
                 self.add_parent_tree((old_tip, other_tree))
                 trace.note('Rerun update after fixing the conflicts.')
@@ -2372,7 +2376,8 @@
 
             nb_conflicts = merge.merge_inner(self.branch, to_tree, base_tree,
                                              this_tree=self,
-                                             change_reporter=change_reporter)
+                                             change_reporter=change_reporter,
+                                             show_base=show_base)
             self.set_last_revision(revision)
             # TODO - dedup parents list with things merged by pull ?
             # reuse the tree we've updated to to set the basis: