Merge lp:~vila/bzr/tree-has-changes into lp:~bzr/bzr/trunk-old

[Vincent Ladeuil]

[Vincent Ladeuil (vila)]

More an RFC than a merge request, this patch implements a new tree.has_changes()
method as discussed during bug #284038 reviews.

The way I have to filter the root seems ugly but I couldn't find
an alternative, so feedback welcome.

I was also surprised to be able to use _has_changes() in so many different places,
that may have an impact on performances (and for once may be more in real life than
in benchmarks).

John also asked for some kind of has_changes_strictly() to share between send and push --strict.
I also like feedback on that as it seems that there are more use-cases than we discuss
and/or latent bugs (see XXX marks).

[John A Meinel (jameinel)]

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

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/tree-has-changes into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> More an RFC than a merge request, this patch implements a new tree.has_changes()
> method as discussed during bug #284038 reviews.
>
> The way I have to filter the root seems ugly but I couldn't find
> an alternative, so feedback welcome.
>
> I was also surprised to be able to use _has_changes() in so many different places,
> that may have an impact on performances (and for once may be more in real life than
> in benchmarks).
>
> John also asked for some kind of has_changes_strictly() to share between send and push --strict.
> I also like feedback on that as it seems that there are more use-cases than we discuss
> and/or latent bugs (see XXX marks).
>

Do we want to do this as:

  wt.has_changes(other_tree)

or do we want it as simply

  wt.has_changes()

And internally it looks at "self.basis_tree()".

For example, RevisionTrees know explicitly that they can't have changes,
arguably calling it is undefined. I would consider making the function
part of the MutableTree api, and not the base Tree api.

Because of lock lifetimes, etc, I'd rather handle it internally.

I'm not sure what is going on in "merge", but I'm pretty sure it is
doing the same thing.

I think it makes the api cleaner as then we don't have to worry about
arbitrary targets for "has_changes()".

John
=:->

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

iEYEARECAAYFAkpM54MACgkQJdeBCYSNAAN12gCfYVFCK1VFnGvJoR6s1rV4s/4s
8s0An3XWw0FDMuKK4gb2NUtklV12HqfA
=lGw2
-----END PGP SIGNATURE-----

[Robert Collins (lifeless)]

On Thu, 2009-07-02 at 17:00 +0000, John A Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1

> Do we want to do this as:
>
> wt.has_changes(other_tree)
>
> or do we want it as simply
>
> wt.has_changes()

I don't have a strong opinion; if I were deciding I would look at where
it is used: if it is used in operations that take -r parameters, then
the former is probably correct. If its only ever desirable to know if
there are changes without examining them then doing it just on mutable
tree seems ok to me.

-Rob

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2009-07-02 11:37:38 +0000
+++ bzrlib/builtins.py	2009-07-08 12:36:36 +0000
@@ -449,18 +449,18 @@
         except errors.NoWorkingTree:
             raise errors.BzrCommandError("No working tree to remove")
         except errors.NotLocalUrl:
-            raise errors.BzrCommandError("You cannot remove the working tree of a "
-                                         "remote path")
+            raise errors.BzrCommandError("You cannot remove the working tree"
+                                         " of a remote path")
         if not force:
-            changes = working.changes_from(working.basis_tree())
-            if changes.has_changed():
+            # XXX: What about pending merges ? -- vila 20090629
+            if working.has_changes(working.basis_tree()):
                 raise errors.UncommittedChanges(working)
 
         working_path = working.bzrdir.root_transport.base
         branch_path = working.branch.bzrdir.root_transport.base
         if working_path != branch_path:
-            raise errors.BzrCommandError("You cannot remove the working tree from "
-                                         "a lightweight checkout")
+            raise errors.BzrCommandError("You cannot remove the working tree"
+                                         " from a lightweight checkout")
 
         d.destroy_workingtree()
 
@@ -1114,8 +1114,8 @@
             revision_id = None
         if (tree is not None and revision_id is None
             and (strict is None or strict)): # Default to True:
-            changes = tree.changes_from(tree.basis_tree())
-            if changes.has_changed() or len(tree.get_parent_ids()) > 1:
+            if (tree.has_changes(tree.basis_tree())
+                 or len(tree.get_parent_ids()) > 1):
                 raise errors.UncommittedChanges(
                     tree, more='Use --no-strict to force the push.')
             if tree.last_revision() != tree.branch.last_revision():
@@ -3641,8 +3641,7 @@
         except errors.NoSuchRevision:
             basis_tree = tree.basis_tree()
         if not force:
-            changes = tree.changes_from(basis_tree)
-            if changes.has_changed():
+            if tree.has_changes(basis_tree):
                 raise errors.UncommittedChanges(tree)
 
         view_info = _get_view_info_for_change_reporter(tree)

=== modified file 'bzrlib/merge.py'
--- bzrlib/merge.py	2009-04-14 15:02:46 +0000
+++ bzrlib/merge.py	2009-07-08 12:36:36 +0000
@@ -243,8 +243,7 @@
 
         if self.other_rev_id is None:
             other_basis_tree = self.revision_tree(self.other_basis)
-            changes = other_basis_tree.changes_from(self.other_tree)
-            if changes.has_changed():
+            if other_basis_tree.has_changes(self.other_tree):
                 raise WorkingTreeNotRevision(self.this_tree)
             other_rev_id = self.other_basis
             self.other_tree = other_basis_tree
@@ -276,8 +275,7 @@
             basis_tree = self.revision_tree(self.this_tree.last_revision())
         except errors.NoSuchRevision:
             basis_tree = self.this_tree.basis_tree()
-        changes = self.this_tree.changes_from(basis_tree)
-        if not changes.has_changed():
+        if not self.this_tree.has_changes(basis_tree):
             self.this_rev_id = self.this_basis
 
     def set_interesting_files(self, file_list):

=== modified file 'bzrlib/reconfigure.py'
--- bzrlib/reconfigure.py	2009-07-06 06:46:30 +0000
+++ bzrlib/reconfigure.py	2009-07-08 12:36:36 +0000
@@ -218,8 +218,8 @@
     def _check(self):
         """Raise if reconfiguration would destroy local changes"""
         if self._destroy_tree:
-            changes = self.tree.changes_from(self.tree.basis_tree())
-            if changes.has_changed():
+            # XXX: What about pending merges ? -- vila 20090629
+            if self.tree.has_changes(self.tree.basis_tree()):
                 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-01 15:17:33 +0000
+++ bzrlib/send.py	2009-07-08 12:36:36 +0000
@@ -119,8 +119,8 @@
                     except KeyError:
                         strict = None
             if tree is not None and (strict is None or strict):
-                changes = tree.changes_from(tree.basis_tree())
-                if changes.has_changed() or len(tree.get_parent_ids()) > 1:
+                if (tree.has_changes(tree.basis_tree())
+                    or len(tree.get_parent_ids()) > 1):
                     raise errors.UncommittedChanges(
                         tree, more='Use --no-strict to force the send.')
                 if tree.last_revision() != tree.branch.last_revision():

=== modified file 'bzrlib/tests/intertree_implementations/test_compare.py'
--- bzrlib/tests/intertree_implementations/test_compare.py	2009-04-09 20:23:07 +0000
+++ bzrlib/tests/intertree_implementations/test_compare.py	2009-07-08 12:36:36 +0000
@@ -367,6 +367,15 @@
             tree1.unlock()
             tree2.unlock()
 
+    def do_has_changes(self, tree1, tree2, **extra_args):
+        tree1.lock_read()
+        tree2.lock_read()
+        try:
+            return tree1.has_changes(tree2)
+        finally:
+            tree1.unlock()
+            tree2.unlock()
+
     def mutable_trees_to_locked_test_trees(self, tree1, tree2):
         """Convert the working trees into test trees.
 
@@ -436,6 +445,7 @@
         tree2 = self.get_tree_no_parents_no_content(tree2)
         tree1, tree2 = self.mutable_trees_to_test_trees(self, tree1, tree2)
         self.assertEqual([], self.do_iter_changes(tree1, tree2))
+        self.assertEquals(False, self.do_has_changes(tree1, tree2))
 
     def added(self, tree, file_id):
         path, entry = self.get_path_entry(tree, file_id)
@@ -519,6 +529,7 @@
             self.added(tree2, 'c-id'),
             self.deleted(tree1, 'empty-root-id')])
         self.assertEqual(expected_results, self.do_iter_changes(tree1, tree2))
+        self.assertEquals(True, self.do_has_changes(tree1, tree2))
 
     def test_empty_specific_files(self):
         tree1 = self.make_branch_and_tree('1')
@@ -542,6 +553,7 @@
             self.added(tree2, 'c-id'),
             self.deleted(tree1, 'empty-root-id')])
         self.assertEqual(expected_results, self.do_iter_changes(tree1, tree2))
+        self.assertEquals(True, self.do_has_changes(tree1, tree2))
 
     def test_empty_to_abc_content_a_only(self):
         tree1 = self.make_branch_and_tree('1')
@@ -593,6 +605,7 @@
         self.assertEqual(
             expected_results,
             self.do_iter_changes(tree1, tree2))
+        self.assertEquals(True, self.do_has_changes(tree1, tree2))
 
     def test_content_modification(self):
         tree1 = self.make_branch_and_tree('1')
@@ -605,6 +618,7 @@
                            (root_id, root_id), ('a', 'a'),
                            ('file', 'file'), (False, False))],
                          self.do_iter_changes(tree1, tree2))
+        self.assertEquals(True, self.do_has_changes(tree1, tree2))
 
     def test_meta_modification(self):
         tree1 = self.make_branch_and_tree('1')
@@ -834,6 +848,7 @@
             self.content_changed(tree2, 'c-id'),
             ])
         self.assertEqual(expected, self.do_iter_changes(tree1, tree2))
+        self.assertEquals(True, self.do_has_changes(tree1, tree2))
 
     def test_unversioned_paths_in_tree(self):
         tree1 = self.make_branch_and_tree('tree1')
@@ -1051,6 +1066,7 @@
         self.assertEqual(expected,
             self.do_iter_changes(tree1, tree2, include_unchanged=True,
                 want_unversioned=True))
+        self.assertEquals(True, self.do_has_changes(tree1, tree2))
 
     def test_versioned_symlinks_specific_files(self):
         self.requireFeature(tests.SymlinkFeature)
@@ -1072,17 +1088,20 @@
         self.assertEqual(expected, self.do_iter_changes(tree1, tree2,
             specific_files=['added', 'changed', 'fromdir', 'fromfile',
             'removed', 'unchanged', 'todir', 'tofile']))
+        self.assertEquals(True, self.do_has_changes(tree1, tree2))
 
     def test_tree_with_special_names(self):
         tree1, tree2, paths, path_ids = self.make_tree_with_special_names()
         expected = sorted(self.added(tree2, f_id) for f_id in path_ids)
         self.assertEqual(expected, self.do_iter_changes(tree1, tree2))
+        self.assertEquals(True, self.do_has_changes(tree1, tree2))
 
     def test_trees_with_special_names(self):
         tree1, tree2, paths, path_ids = self.make_trees_with_special_names()
         expected = sorted(self.content_changed(tree2, f_id) for f_id in path_ids
                           if f_id.endswith('_f-id'))
         self.assertEqual(expected, self.do_iter_changes(tree1, tree2))
+        self.assertEquals(True, self.do_has_changes(tree1, tree2))
 
     def test_trees_with_deleted_dir(self):
         tree1 = self.make_branch_and_tree('tree1')
@@ -1106,6 +1125,7 @@
             self.deleted(tree1, 'e-id'),
             ])
         self.assertEqual(expected, self.do_iter_changes(tree1, tree2))
+        self.assertEquals(True, self.do_has_changes(tree1, tree2))
 
     def test_added_unicode(self):
         tree1 = self.make_branch_and_tree('tree1')
@@ -1134,6 +1154,7 @@
         self.assertEqual([self.added(tree2, added_id)],
                          self.do_iter_changes(tree1, tree2,
                                               specific_files=[u'\u03b1']))
+        self.assertEquals(True, self.do_has_changes(tree1, tree2))
 
     def test_deleted_unicode(self):
         tree1 = self.make_branch_and_tree('tree1')
@@ -1162,6 +1183,7 @@
         self.assertEqual([self.deleted(tree1, deleted_id)],
                          self.do_iter_changes(tree1, tree2,
                                               specific_files=[u'\u03b1']))
+        self.assertEquals(True, self.do_has_changes(tree1, tree2))
 
     def test_modified_unicode(self):
         tree1 = self.make_branch_and_tree('tree1')
@@ -1191,6 +1213,7 @@
         self.assertEqual([self.content_changed(tree1, mod_id)],
                          self.do_iter_changes(tree1, tree2,
                                               specific_files=[u'\u03b1']))
+        self.assertEquals(True, self.do_has_changes(tree1, tree2))
 
     def test_renamed_unicode(self):
         tree1 = self.make_branch_and_tree('tree1')
@@ -1221,6 +1244,7 @@
         self.assertEqual([self.renamed(tree1, tree2, rename_id, False)],
                          self.do_iter_changes(tree1, tree2,
                                               specific_files=[u'\u03b1']))
+        self.assertEquals(True, self.do_has_changes(tree1, tree2))
 
     def test_unchanged_unicode(self):
         tree1 = self.make_branch_and_tree('tree1')
@@ -1309,6 +1333,7 @@
                                               want_unversioned=True))
         self.assertEqual([], # Without want_unversioned we should get nothing
                          self.do_iter_changes(tree1, tree2))
+        self.assertEquals(False, self.do_has_changes(tree1, tree2))
 
         # We should also be able to select just a subset
         expected = sorted([
@@ -1389,6 +1414,7 @@
             ])
         self.assertEqual(expected,
                          self.do_iter_changes(tree1, tree2))
+        self.assertEquals(True, self.do_has_changes(tree1, tree2))
 
     def test_deleted_and_unknown(self):
         """Test a file marked removed, but still present on disk."""

=== modified file 'bzrlib/tree.py'
--- bzrlib/tree.py	2009-07-01 10:56:49 +0000
+++ bzrlib/tree.py	2009-07-08 12:36:36 +0000
@@ -102,6 +102,24 @@
         return intertree.iter_changes(include_unchanged, specific_files, pb,
             extra_trees, require_versioned, want_unversioned=want_unversioned)
 
+
+    @needs_read_lock
+    def has_changes(self, from_tree):
+        """Quickly check that the tree contains at least one change.
+
+        :return: True if a change is found. False otherwise
+        """
+        changes = self.iter_changes(from_tree)
+        try:
+            change = changes.next()
+            # Exclude root (talk about black magic... --vila 20090629)
+            if change[4] == (None, None):
+                change = changes.next()
+            return True
+        except StopIteration:
+            # No changes
+            return False
+
     def conflicts(self):
         """Get a list of the conflicts in the tree.