Merge lp:~abentley/bzr/is_executable into lp:bzr

[Aaron Bentley]

Hi all,

This branch fixes RevisionTree.is_executable to treat directories and symlinks
the same way as other Tree implementations-- to return False rather than None.
The revised behaviour is also consistent with RevisionTree._comparison_data.

This problem filters up and affects Launchpad's incremental diffs for merge
proposals (which are disabled until this issue can be resolved), causing them
to erroneously report property changes on directories and symlinks
that have not changed.

Three tests have been affected, but FWICT, they were testing actual behaviour,
not desirable behaviour. Specifically, the ROOT_ENTRY should only be included
in iter_changes-style output when it has changed, when it has a new child, or
when include_unchanged is specified.

[Andrew Bennetts (spiv)]

The fix itself seems good.

(I already said this on IRC, but for the benefit of people looking at this page) The change to behaviour seems in principle reasonable, but I'd be nervous about landing it in the stable branch. I'm not sure how likely this is, but it's at least plausible that a plugin might be depending on the root entry being emitted in that case. So my preference is to land this just on lp:bzr.

One extremely minor nit is that the preferred way to do locking and addCleanup is now as a one-liner:

    self.addCleanup(tree.lock_read().unlock)

So if you wish, you could make that tweak, but it's not a big deal at all.

I'm going to vote "Abstain" because the key decision about whether to land this on 2.2 is up to the RM rather than a reviewer, I think.

review: Abstain

[Aaron Bentley (abentley)]

This warning about conflicts is true, but hilarious. Merging the prerequisite branch into lp:bzr produces conflicts, and merging lp:~abentley/bzr/is_executable produces identical conflicts. So they don't show up in the diff, but they do show up in the warning.

[John A Meinel (jameinel)]

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

On 11/29/2010 5:50 PM, Aaron Bentley wrote:
> Aaron Bentley has proposed merging lp:~abentley/bzr/is_executable into lp:bzr/2.2.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> Hi all,
>
> This branch fixes RevisionTree.is_executable to treat directories and symlinks
> the same way as other Tree implementations-- to return False rather than None.
> The revised behaviour is also consistent with RevisionTree._comparison_data.
>
> This problem filters up and affects Launchpad's incremental diffs for merge
> proposals (which are disabled until this issue can be resolved), causing them
> to erroneously report property changes on directories and symlinks
> that have not changed.
>
> Three tests have been affected, but FWICT, they were testing actual behaviour,
> not desirable behaviour. Specifically, the ROOT_ENTRY should only be included
> in iter_changes-style output when it has changed, when it has a new child, or
> when include_unchanged is specified.

I'm a little concerned about doing this in a stable series. I think the
patch is definitely worthy for trunk.

I'm guessing you did it this way because you want it in the launchpad
codebase?

 review: needs_information

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

iEYEARECAAYFAkz1EVsACgkQJdeBCYSNAAN72ACgx2ecqEd8iGOT/Nb/rGTb0KtJ
4jMAn2bc2xDxFQ08Efy6ppdJSgksKdXM
=RuBa
-----END PGP SIGNATURE-----

review: Needs Information

[John A Meinel (jameinel)]

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

On 11/29/2010 9:59 PM, Aaron Bentley wrote:
> Aaron Bentley has proposed merging lp:~abentley/bzr/is_executable into lp:bzr with lp:bzr/2.2 as a prerequisite.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> Hi all,
>
> This branch fixes RevisionTree.is_executable to treat directories and symlinks
> the same way as other Tree implementations-- to return False rather than None.
> The revised behaviour is also consistent with RevisionTree._comparison_data.
>
> This problem filters up and affects Launchpad's incremental diffs for merge
> proposals (which are disabled until this issue can be resolved), causing them
> to erroneously report property changes on directories and symlinks
> that have not changed.
>
> Three tests have been affected, but FWICT, they were testing actual behaviour,
> not desirable behaviour. Specifically, the ROOT_ENTRY should only be included
> in iter_changes-style output when it has changed, when it has a new child, or
> when include_unchanged is specified.

I think this is perfectly fine to land in trunk. Just need some
discussion about landing it in the 2.2 series.

 merge: approve

John
=:->

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

iEYEARECAAYFAkz1EXMACgkQJdeBCYSNAAMqmgCfRgyrNe8o9fbMI8dT7d7g88ZC
cYgAn21lp1gOXUpMJmeAuY+vCo9dR5e7
=Yd01
-----END PGP SIGNATURE-----

review: Approve

[Aaron Bentley (abentley)]

I proposed for 2.2 because I want it for Launchpad, and because it seemed like a small change. I didn't imagine that clients would actually care about the executability of symlinks or directories, but I can think of at least one client (difftacular, used by Launchpad) that cares that the output of RevisionTree.iter_changes(PreviewTree) is sane.

So on balance, it still seems like a reasonable change to make in a stable series. That said, I can propose a more conservative patch for 2.2 that will merely correct the output of RevisionTree.iter_changes(PreviewTree). Or I guess I could fix in in difftactular, though I'd rather not.

[Martin Pool (mbp)]

Keeping 2.2 stable is very important. It's also desirable to keep Launchpad on the stable branch and to have them actually running that branch, not something diverged from it. This also seems at least plausibly like a bug that could be justified to fix in 2.2, assuming the fix is safe.

With regard to assessing safety, it would help to say in the cover letter and NEWS what the behaviour was before. If it crashes, then it seems unlikely that anyone was counting on that behaviour. But I see from the diff it's obviously None.

Consistency with other Trees is a pretty good reason to change it.

=== modified file 'bzrlib/tests/test_transform.py'
--- bzrlib/tests/test_transform.py 2010-10-06 21:29:58 +0000
+++ bzrlib/tests/test_transform.py 2010-11-29 23:49:57 +0000
@@ -2344,7 +2344,7 @@
                   ('TREE_ROOT', 'TREE_ROOT'), ('a', 'a'), ('file', 'file'),
                   (False, False))
 ROOT_ENTRY = ('TREE_ROOT', ('', ''), False, (True, True), (None, None),
- ('', ''), ('directory', 'directory'), (False, None))
+ ('', ''), ('directory', 'directory'), (False, False))

 class TestTransformPreview(tests.TestCaseWithTransport):
@@ -2437,13 +2437,13 @@
         revision_tree, preview_tree = self.get_tree_and_preview_tree()
         changes = preview_tree.iter_changes(revision_tree,
                                             specific_files=[''])
- self.assertEqual([ROOT_ENTRY, A_ENTRY], list(changes))
+ self.assertEqual([A_ENTRY], list(changes))

     def test_want_unversioned(self):
         revision_tree, preview_tree = self.get_tree_and_preview_tree()
         changes = preview_tree.iter_changes(revision_tree,
                                             want_unversioned=True)
- self.assertEqual([ROOT_ENTRY, A_ENTRY], list(changes))
+ self.assertEqual([A_ENTRY], list(changes))

     def test_ignore_extra_trees_no_specific_files(self):
         # extra_trees is harmless without specific_files, so we'll silently

So that's a consequence of the value now being the same in all trees, therefore it's not detected as changing?

It seems like if we do break any API clients, it might be through side effects of things like this, rather than because they directly care about the is_executable value.

We could try testing the 2.2-related branches or releases with this patch.

[Martin Pool (mbp)]

Aaron told me on irc that he checked this against some core plugins, and found no new failures, so I think we should merge this to 2.2.

review: Approve

=== modified file 'bzrlib/revisiontree.py'
--- bzrlib/revisiontree.py	2010-05-06 23:41:35 +0000
+++ bzrlib/revisiontree.py	2010-11-30 03:59:17 +0000
@@ -110,7 +110,7 @@
     def is_executable(self, file_id, path=None):
         ie = self._inventory[file_id]
         if ie.kind != "file":
-            return None
+            return False
         return ie.executable
 
     def has_filename(self, filename):

=== modified file 'bzrlib/tests/per_tree/__init__.py'
--- bzrlib/tests/per_tree/__init__.py	2010-08-21 16:06:24 +0000
+++ bzrlib/tests/per_tree/__init__.py	2010-11-30 03:59:17 +0000
@@ -381,6 +381,7 @@
         'get_symlink_target',
         'inv',
         'iter_search_rules',
+        'is_executable',
         'list_files',
         'locking',
         'path_content_summary',

=== added file 'bzrlib/tests/per_tree/test_is_executable.py'
--- bzrlib/tests/per_tree/test_is_executable.py	1970-01-01 00:00:00 +0000
+++ bzrlib/tests/per_tree/test_is_executable.py	2010-11-30 03:59:17 +0000
@@ -0,0 +1,37 @@
+# Copyright (C) 2010 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
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+from bzrlib.tests import (
+    per_tree,
+    SymlinkFeature,
+    )
+
+
+class TestIsExecutable(per_tree.TestCaseWithTree):
+
+    def test_is_executable_dir(self):
+        tree = self.get_tree_with_subdirs_and_all_supported_content_types(
+            False)
+        tree.lock_read()
+        self.addCleanup(tree.unlock)
+        self.assertEqual(False, tree.is_executable('1top-dir'))
+
+    def test_is_executable_symlink(self):
+        self.requireFeature(SymlinkFeature)
+        tree = self.get_tree_with_subdirs_and_all_content_types()
+        tree.lock_read()
+        self.addCleanup(tree.unlock)
+        self.assertEqual(False, tree.is_executable('symlink'))

=== modified file 'bzrlib/tests/test_transform.py'
--- bzrlib/tests/test_transform.py	2010-11-19 08:34:39 +0000
+++ bzrlib/tests/test_transform.py	2010-11-30 03:59:17 +0000
@@ -2381,7 +2381,7 @@
                   ('TREE_ROOT', 'TREE_ROOT'), ('a', 'a'), ('file', 'file'),
                   (False, False))
 ROOT_ENTRY = ('TREE_ROOT', ('', ''), False, (True, True), (None, None),
-              ('', ''), ('directory', 'directory'), (False, None))
+              ('', ''), ('directory', 'directory'), (False, False))
 
 
 class TestTransformPreview(tests.TestCaseWithTransport):
@@ -2474,13 +2474,13 @@
         revision_tree, preview_tree = self.get_tree_and_preview_tree()
         changes = preview_tree.iter_changes(revision_tree,
                                             specific_files=[''])
-        self.assertEqual([ROOT_ENTRY, A_ENTRY], list(changes))
+        self.assertEqual([A_ENTRY], list(changes))
 
     def test_want_unversioned(self):
         revision_tree, preview_tree = self.get_tree_and_preview_tree()
         changes = preview_tree.iter_changes(revision_tree,
                                             want_unversioned=True)
-        self.assertEqual([ROOT_ENTRY, A_ENTRY], list(changes))
+        self.assertEqual([A_ENTRY], list(changes))
 
     def test_ignore_extra_trees_no_specific_files(self):
         # extra_trees is harmless without specific_files, so we'll silently

=== modified file 'bzrlib/workingtree_4.py'
--- bzrlib/workingtree_4.py	2010-08-03 13:31:06 +0000
+++ bzrlib/workingtree_4.py	2010-11-30 03:59:17 +0000
@@ -1867,7 +1867,7 @@
     def is_executable(self, file_id, path=None):
         ie = self.inventory[file_id]
         if ie.kind != "file":
-            return None
+            return False
         return ie.executable
 
     def is_locked(self):

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2010-11-30 03:59:15 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2010-11-30 03:59:17 +0000
@@ -22,6 +22,9 @@
 Bug Fixes
 *********
 
+* RevisionTree.is_executable handles directories and symlinks like other types
+  and functions.  (Aaron Bentley, #681885)
+
 Improvements
 ************