Merge lp:~vila/bzr/440631-has-changes-no-param into lp:bzr

[Vincent Ladeuil]

[Vincent Ladeuil (vila)]

This patch changes MutableTree.has_changes() to stop requiring a tree parameter and default
to the basis tree instead. It also add a check for pending merges.

I've made the '_from_tree' parameter to keep the existing tests or I would have need to rewrite
them all (~20, none of them trivial).

I've updated all the call sites and fixed the test failures.
The later led to a discussion with John and Aaron that prompted the deprecations
of check_basis() and friends since they are not used anymore (and sometimes not tested).

I've also used has_changes() for reconfigure where the pending merges weren't checked,
and added the corresponding test.

[John A Meinel (jameinel)]

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

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/440631-has-changes-no-param into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #440631 mutable_tree.has_changes() should not take a parameter but always check against basis
> https://bugs.launchpad.net/bugs/440631
>
>
> This patch changes MutableTree.has_changes() to stop requiring a tree parameter and default
> to the basis tree instead. It also add a check for pending merges.
>
> I've made the '_from_tree' parameter to keep the existing tests or I would have need to rewrite
> them all (~20, none of them trivial).
>
> I've updated all the call sites and fixed the test failures.
> The later led to a discussion with John and Aaron that prompted the deprecations
> of check_basis() and friends since they are not used anymore (and sometimes not tested).
>
> I've also used has_changes() for reconfigure where the pending merges weren't checked,
> and added the corresponding test.
>

Everything looks good, except I don't see any new tests that *don't*
pass the parameter to "has_changes()". It is certainly tested indirectly
 via the blackbox tests. So certainly don't change all 20, but maybe 1
or 2 to get a simple unit test of it? Up to you.

 review: approve
 merge: approve

John
=:->

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

iEYEARECAAYFAkrLYYMACgkQJdeBCYSNAAMDXgCglJ/A/WjOtwkdyb/AG4TipX5h
+O8AnipiyUVF4S0v86hxRJJoHtye0y95
=WI34
-----END PGP SIGNATURE-----

review: Approve

=== modified file 'NEWS'
--- NEWS	2009-10-06 02:38:44 +0000
+++ NEWS	2009-10-06 16:40:25 +0000
@@ -160,9 +160,6 @@
 API Changes
 ***********
 
-* ``ProgressTask.note`` is deprecated.
-  (Martin Pool)
-
 * ``bzrlib.user_encoding`` has been removed; use
   ``bzrlib.osutils.get_user_encoding`` instead.  (Martin Pool)
 
@@ -172,6 +169,17 @@
 * ``bzrlib.trace.log_error``, ``error`` and ``info`` have been deprecated.
   (Martin Pool)
 
+* ``MutableTree.has_changes()`` does not require a tree parameter anymore. It
+  now defaults to comparing to the basis tree. It now checks for pending
+  merges too.  ``Merger.check_basis`` has been deprecated and replaced by the
+  corresponding has_changes() calls. ``Merge.compare_basis``,
+  ``Merger.file_revisions`` and ``Merger.ensure_revision_trees`` have also
+  been deprecated.
+  (Vincent Ladeuil, #440631)
+
+* ``ProgressTask.note`` is deprecated.
+  (Martin Pool)
+
 Internals
 *********
 

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2009-09-24 19:51:37 +0000
+++ bzrlib/builtins.py	2009-10-06 16:40:25 +0000
@@ -461,8 +461,7 @@
             raise errors.BzrCommandError("You cannot remove the working tree"
                                          " of a remote path")
         if not force:
-            if (working.has_changes(working.basis_tree())
-                or len(working.get_parent_ids()) > 1):
+            if (working.has_changes()):
                 raise errors.UncommittedChanges(working)
 
         working_path = working.bzrdir.root_transport.base
@@ -1109,8 +1108,7 @@
         else:
             revision_id = None
         if strict and tree is not None and revision_id is None:
-            if (tree.has_changes(tree.basis_tree())
-                or len(tree.get_parent_ids()) > 1):
+            if (tree.has_changes()):
                 raise errors.UncommittedChanges(
                     tree, more='Use --no-strict to force the push.')
             if tree.last_revision() != tree.branch.last_revision():
@@ -3663,7 +3661,7 @@
 
         # die as quickly as possible if there are uncommitted changes
         if not force:
-            if tree.has_changes(basis_tree) or len(tree.get_parent_ids()) > 1:
+            if tree.has_changes():
                 raise errors.UncommittedChanges(tree)
 
         view_info = _get_view_info_for_change_reporter(tree)
@@ -3720,7 +3718,10 @@
                                        merger.other_rev_id)
                     result.report(self.outf)
                     return 0
-            merger.check_basis(False)
+            if merger.this_basis is None:
+                raise errors.BzrCommandError(
+                    "This branch has no commits."
+                    " (perhaps you would prefer 'bzr pull')")
             if preview:
                 return self._do_preview(merger, cleanups)
             elif interactive:

=== modified file 'bzrlib/bundle/apply_bundle.py'
--- bzrlib/bundle/apply_bundle.py	2009-03-23 14:59:43 +0000
+++ bzrlib/bundle/apply_bundle.py	2009-10-06 16:40:25 +0000
@@ -56,7 +56,8 @@
                         change_reporter=change_reporter)
         merger.pp = pp
         merger.pp.next_phase()
-        merger.check_basis(check_clean, require_commits=False)
+        if check_clean and tree.has_changes():
+            raise errors.UncommittedChanges(self)
         merger.other_rev_id = reader.target
         merger.other_tree = merger.revision_tree(reader.target)
         merger.other_basis = reader.target

=== modified file 'bzrlib/foreign.py'
--- bzrlib/foreign.py	2009-10-02 09:11:43 +0000
+++ bzrlib/foreign.py	2009-10-06 16:40:24 +0000
@@ -305,8 +305,7 @@
                 ).get_user_option_as_bool('dpush_strict')
         if strict is None: strict = True # default value
         if strict and source_wt is not None:
-            if (source_wt.has_changes(source_wt.basis_tree())
-                or len(source_wt.get_parent_ids()) > 1):
+            if (source_wt.has_changes()):
                 raise errors.UncommittedChanges(
                     source_wt, more='Use --no-strict to force the push.')
             if source_wt.last_revision() != source_wt.branch.last_revision():

=== modified file 'bzrlib/merge.py'
--- bzrlib/merge.py	2009-10-06 12:25:59 +0000
+++ bzrlib/merge.py	2009-10-06 16:40:25 +0000
@@ -35,6 +35,10 @@
     ui,
     versionedfile
     )
+from bzrlib.symbol_versioning import (
+    deprecated_in,
+    deprecated_method,
+    )
 # TODO: Report back as changes are merged in
 
 
@@ -226,6 +230,7 @@
         revision_id = _mod_revision.ensure_null(revision_id)
         return branch, self.revision_tree(revision_id, branch)
 
+    @deprecated_method(deprecated_in((2, 1, 0)))
     def ensure_revision_trees(self):
         if self.this_revision_tree is None:
             self.this_basis_tree = self.revision_tree(self.this_basis)
@@ -239,6 +244,7 @@
             other_rev_id = self.other_basis
             self.other_tree = other_basis_tree
 
+    @deprecated_method(deprecated_in((2, 1, 0)))
     def file_revisions(self, file_id):
         self.ensure_revision_trees()
         def get_id(tree, file_id):
@@ -252,6 +258,7 @@
         trees = (self.this_basis_tree, self.other_tree)
         return [get_id(tree, file_id) for tree in trees]
 
+    @deprecated_method(deprecated_in((2, 1, 0)))
     def check_basis(self, check_clean, require_commits=True):
         if self.this_basis is None and require_commits is True:
             raise errors.BzrCommandError(
@@ -262,6 +269,7 @@
             if self.this_basis != self.this_rev_id:
                 raise errors.UncommittedChanges(self.this_tree)
 
+    @deprecated_method(deprecated_in((2, 1, 0)))
     def compare_basis(self):
         try:
             basis_tree = self.revision_tree(self.this_tree.last_revision())
@@ -274,7 +282,8 @@
         self.interesting_files = file_list
 
     def set_pending(self):
-        if not self.base_is_ancestor or not self.base_is_other_ancestor or self.other_rev_id is None:
+        if (not self.base_is_ancestor or not self.base_is_other_ancestor
+            or self.other_rev_id is None):
             return
         self._add_parent()
 

=== modified file 'bzrlib/mutabletree.py'
--- bzrlib/mutabletree.py	2009-10-06 12:25:59 +0000
+++ bzrlib/mutabletree.py	2009-10-06 16:40:24 +0000
@@ -233,12 +233,20 @@
         raise NotImplementedError(self._gather_kinds)
 
     @needs_read_lock
-    def has_changes(self, from_tree):
-        """Quickly check that the tree contains at least one change.
+    def has_changes(self, _from_tree=None):
+        """Quickly check that the tree contains at least one commitable change.
+
+        :param _from_tree: tree to compare against to find changes (default to
+            the basis tree and is intended to be used by tests).
 
         :return: True if a change is found. False otherwise
         """
-        changes = self.iter_changes(from_tree)
+        # Check pending merges
+        if len(self.get_parent_ids()) > 1:
+            return True
+        if _from_tree is None:
+            _from_tree = self.basis_tree()
+        changes = self.iter_changes(_from_tree)
         try:
             change = changes.next()
             # Exclude root (talk about black magic... --vila 20090629)

=== modified file 'bzrlib/reconfigure.py'
--- bzrlib/reconfigure.py	2009-07-24 03:15:56 +0000
+++ bzrlib/reconfigure.py	2009-10-06 16:40:25 +0000
@@ -265,9 +265,7 @@
 
     def _check(self):
         """Raise if reconfiguration would destroy local changes"""
-        if self._destroy_tree:
-            # XXX: What about pending merges ? -- vila 20090629
-            if self.tree.has_changes(self.tree.basis_tree()):
+        if self._destroy_tree and self.tree.has_changes():
                 raise errors.UncommittedChanges(self.tree)
         if self._create_reference and self.local_branch is not None:
             reference_branch = branch.Branch.open(self._select_bind_location())

=== modified file 'bzrlib/send.py'
--- bzrlib/send.py	2009-07-17 14:41:02 +0000
+++ bzrlib/send.py	2009-10-06 16:40:24 +0000
@@ -115,14 +115,13 @@
                     ).get_user_option_as_bool('send_strict')
             if strict is None: strict = True # default value
             if strict and tree is not None:
-                if (tree.has_changes(tree.basis_tree())
-                    or len(tree.get_parent_ids()) > 1):
+                if (tree.has_changes()):
                     raise errors.UncommittedChanges(
                         tree, more='Use --no-strict to force the send.')
                 if tree.last_revision() != tree.branch.last_revision():
                     # The tree has lost sync with its branch, there is little
                     # chance that the user is aware of it but he can still force
-                    # the push with --no-strict
+                    # the send with --no-strict
                     raise errors.OutOfDateTree(
                         tree, more='Use --no-strict to force the send.')
             revision_id = branch.last_revision()

=== modified file 'bzrlib/tests/blackbox/test_merge.py'
--- bzrlib/tests/blackbox/test_merge.py	2009-09-09 15:43:52 +0000
+++ bzrlib/tests/blackbox/test_merge.py	2009-10-06 16:40:24 +0000
@@ -605,7 +605,7 @@
 
     def test_merge_force(self):
         self.tree_a.commit('empty change to allow merge to run')
-        # Second merge on top if the uncommitted one
+        # Second merge on top of the uncommitted one
         self.run_bzr(['merge', '../a', '--force'], working_dir='b')
 
 

=== modified file 'bzrlib/tests/blackbox/test_uncommit.py'
--- bzrlib/tests/blackbox/test_uncommit.py	2009-04-04 02:50:01 +0000
+++ bzrlib/tests/blackbox/test_uncommit.py	2009-10-06 16:40:24 +0000
@@ -233,14 +233,14 @@
         tree3.commit('unchanged', rev_id='c3')
 
         wt.merge_from_branch(tree2.branch)
-        wt.merge_from_branch(tree3.branch)
+        wt.merge_from_branch(tree3.branch, force=True)
         wt.commit('merge b3, c3', rev_id='a3')
 
         tree2.commit('unchanged', rev_id='b4')
         tree3.commit('unchanged', rev_id='c4')
 
         wt.merge_from_branch(tree3.branch)
-        wt.merge_from_branch(tree2.branch)
+        wt.merge_from_branch(tree2.branch, force=True)
         wt.commit('merge b4, c4', rev_id='a4')
 
         self.assertEqual(['a4'], wt.get_parent_ids())

=== modified file 'bzrlib/tests/test_msgeditor.py'
--- bzrlib/tests/test_msgeditor.py	2009-08-20 04:09:58 +0000
+++ bzrlib/tests/test_msgeditor.py	2009-10-06 16:40:24 +0000
@@ -93,7 +93,7 @@
         tree3.commit('Feature Y, based on initial X work.',
                      timestamp=1233285960, timezone=0)
         tree.merge_from_branch(tree2.branch)
-        tree.merge_from_branch(tree3.branch)
+        tree.merge_from_branch(tree3.branch, force=True)
         return tree
 
     def test_commit_template_pending_merges(self):

=== modified file 'bzrlib/tests/test_mutabletree.py'
--- bzrlib/tests/test_mutabletree.py	2009-09-07 23:14:05 +0000
+++ bzrlib/tests/test_mutabletree.py	2009-10-06 16:40:24 +0000
@@ -19,15 +19,18 @@
 Most functionality of MutableTree is tested as part of WorkingTree.
 """
 
-from bzrlib.tests import TestCase
-from bzrlib.mutabletree import MutableTree, MutableTreeHooks
-
-class TestHooks(TestCase):
+from bzrlib import (
+    mutabletree,
+    tests,
+    )
+
+
+class TestHooks(tests.TestCase):
 
     def test_constructor(self):
         """Check that creating a MutableTreeHooks instance has the right
         defaults."""
-        hooks = MutableTreeHooks()
+        hooks = mutabletree.MutableTreeHooks()
         self.assertTrue("start_commit" in hooks,
                         "start_commit not in %s" % hooks)
         self.assertTrue("post_commit" in hooks,
@@ -36,7 +39,25 @@
     def test_installed_hooks_are_MutableTreeHooks(self):
         """The installed hooks object should be a MutableTreeHooks."""
         # the installed hooks are saved in self._preserved_hooks.
-        self.assertIsInstance(self._preserved_hooks[MutableTree][1],
-                              MutableTreeHooks)
-
-
+        self.assertIsInstance(self._preserved_hooks[mutabletree.MutableTree][1],
+                              mutabletree.MutableTreeHooks)
+
+
+class TestHasChanges(tests.TestCaseWithTransport):
+
+    def setUp(self):
+        super(TestHasChanges, self).setUp()
+        self.tree = self.make_branch_and_tree('tree')
+
+    def test_with_uncommitted_changes(self):
+        self.build_tree(['tree/file'])
+        self.tree.add('file')
+        self.assertTrue(self.tree.has_changes())
+
+    def test_with_pending_merges(self):
+        other_tree = self.tree.bzrdir.sprout('other').open_workingtree()
+        self.build_tree(['other/file'])
+        other_tree.add('file')
+        other_tree.commit('added file')
+        self.tree.merge_from_branch(other_tree.branch)
+        self.assertTrue(self.tree.has_changes())

=== modified file 'bzrlib/tests/test_reconfigure.py'
--- bzrlib/tests/test_reconfigure.py	2009-04-28 20:12:44 +0000
+++ bzrlib/tests/test_reconfigure.py	2009-10-06 16:40:25 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2007 Canonical Ltd
+# Copyright (C) 2007, 2008, 2009 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
@@ -44,6 +44,19 @@
         self.assertRaises(errors.NoWorkingTree, workingtree.WorkingTree.open,
                           'tree')
 
+    def test_tree_with_pending_merge_to_branch(self):
+        tree = self.make_branch_and_tree('tree')
+        other_tree = tree.bzrdir.sprout('other').open_workingtree()
+        self.build_tree(['other/file'])
+        other_tree.add('file')
+        other_tree.commit('file added')
+        tree.merge_from_branch(other_tree.branch)
+        reconfiguration = reconfigure.Reconfigure.to_branch(tree.bzrdir)
+        self.assertRaises(errors.UncommittedChanges, reconfiguration.apply)
+        reconfiguration.apply(force=True)
+        self.assertRaises(errors.NoWorkingTree, workingtree.WorkingTree.open,
+                          'tree')
+
     def test_branch_to_branch(self):
         branch = self.make_branch('branch')
         self.assertRaises(errors.AlreadyBranch,

=== modified file 'bzrlib/tests/test_status.py'
--- bzrlib/tests/test_status.py	2009-08-20 04:09:58 +0000
+++ bzrlib/tests/test_status.py	2009-10-06 16:40:24 +0000
@@ -53,7 +53,7 @@
         tree2.commit('commit 3b', timestamp=1196796819, timezone=0)
         tree3.commit('commit 3c', timestamp=1196796819, timezone=0)
         tree.merge_from_branch(tree2.branch)
-        tree.merge_from_branch(tree3.branch)
+        tree.merge_from_branch(tree3.branch, force=True)
         return tree
 
     def test_multiple_pending(self):

=== modified file 'bzrlib/workingtree.py'
--- bzrlib/workingtree.py	2009-08-26 05:38:16 +0000
+++ bzrlib/workingtree.py	2009-10-06 16:40:25 +0000
@@ -896,7 +896,7 @@
 
     @needs_write_lock # because merge pulls data into the branch.
     def merge_from_branch(self, branch, to_revision=None, from_revision=None,
-        merge_type=None):
+                          merge_type=None, force=False):
         """Merge from a branch into this working tree.
 
         :param branch: The branch to merge from.
@@ -911,9 +911,9 @@
             merger = Merger(self.branch, this_tree=self, pb=pb)
             merger.pp = ProgressPhase("Merge phase", 5, pb)
             merger.pp.next_phase()
-            # check that there are no
-            # local alterations
-            merger.check_basis(check_clean=True, require_commits=False)
+            # check that there are no local alterations
+            if not force and self.has_changes():
+                raise errors.UncommittedChanges(self)
             if to_revision is None:
                 to_revision = _mod_revision.ensure_null(branch.last_revision())
             merger.other_rev_id = to_revision