Code review comment for lp:~abentley/bzr/is_executable

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.