Merge lp:~gz/bzr/unversioned_paths_unprintable_898408 into lp:bzr

[Martin Packman]

Ensure PathsNotVersionedError is only ever given unicode paths

Make notifying the user that they're trying to operate on unversioned files work without 'Unprintable exception' on utf-8 terminals again.

The problem stems from the fact bzrlib.workingtree_4 in a couple of methods encodes the paths it's passed to utf-8 in place as a hack to align with dirstate internals. All paths that bubble back up to the ui level need to be unicode,

There are two reasonable places to re-decode the paths, either in the methods that raise PathsNotVersionedError, or in the exception constructor. As bzrlib.tree has a method that raises the exception without squelching the paths first, doing it in the dirstate-interfacing workingtree_4 methods seems most reasonable.

Reporting on non-utf-8 terminals still mangles paths, but that's a long standing issue and this fix moves in the correct direction on that front as well.

[Vincent Ladeuil (vila)]

Very good explanation in the cover letter really made reviewing this proposal easy. Thanks.

I agree with your analysis and it seems better indeed to leave the exception always deal with unicode paths.

review: Approve

[Martin Packman (gz)]

sent to pqm by email

=== modified file 'bzrlib/tests/blackbox/test_commit.py'
--- bzrlib/tests/blackbox/test_commit.py	2011-11-29 17:23:13 +0000
+++ bzrlib/tests/blackbox/test_commit.py	2011-12-06 11:20:29 +0000
@@ -167,6 +167,26 @@
         finally:
             osutils.get_terminal_encoding = default_get_terminal_enc
 
+    def test_non_ascii_file_unversioned_utf8(self):
+        self.requireFeature(features.UnicodeFilenameFeature)
+        tree = self.make_branch_and_tree(".")
+        self.build_tree(["f"])
+        tree.add(["f"])
+        out, err = self.run_bzr(["commit", "-m", "Wrong filename", u"\xa7"],
+            encoding="utf-8", retcode=3)
+        self.assertContainsRe(err, "(?m)not versioned: \"\xc2\xa7\"$")
+
+    def test_non_ascii_file_unversioned_iso_8859_5(self):
+        self.requireFeature(features.UnicodeFilenameFeature)
+        tree = self.make_branch_and_tree(".")
+        self.build_tree(["f"])
+        tree.add(["f"])
+        out, err = self.run_bzr(["commit", "-m", "Wrong filename", u"\xa7"],
+            encoding="iso-8859-5", retcode=3)
+        self.expectFailure("Error messages are always written as UTF-8",
+            self.assertNotContainsString, err, "\xc2\xa7")
+        self.assertContainsRe(err, "(?m)not versioned: \"\xfd\"$")
+
     def test_warn_about_forgotten_commit_message(self):
         """Test that the lack of -m parameter is caught"""
         wt = self.make_branch_and_tree('.')

=== modified file 'bzrlib/tests/per_workingtree/test_paths2ids.py'
--- bzrlib/tests/per_workingtree/test_paths2ids.py	2009-07-10 07:14:02 +0000
+++ bzrlib/tests/per_workingtree/test_paths2ids.py	2011-12-06 11:20:29 +0000
@@ -24,6 +24,7 @@
 from operator import attrgetter
 
 from bzrlib import errors
+from bzrlib.tests import features
 from bzrlib.tests.per_workingtree import TestCaseWithWorkingTree
 
 
@@ -188,3 +189,13 @@
             ['unversioned'], [tree])
         basis.unlock()
         tree.unlock()
+
+    def test_unversioned_non_ascii_one_tree(self):
+        self.requireFeature(features.UnicodeFilenameFeature)
+        tree = self.make_branch_and_tree('.')
+        self.build_tree([u"\xa7"])
+        self.assertExpectedIds([], tree, [u"\xa7"], require_versioned=False)
+        self.addCleanup(tree.lock_read().unlock)
+        e = self.assertRaises(errors.PathsNotVersionedError,
+            tree.paths2ids, [u"\xa7"])
+        self.assertEqual([u"\xa7"], e.paths)

=== modified file 'bzrlib/tests/test_workingtree_4.py'
--- bzrlib/tests/test_workingtree_4.py	2011-05-26 21:02:47 +0000
+++ bzrlib/tests/test_workingtree_4.py	2011-12-06 11:20:29 +0000
@@ -29,7 +29,7 @@
     workingtree_4,
     )
 from bzrlib.lockdir import LockDir
-from bzrlib.tests import TestCaseWithTransport, TestSkipped
+from bzrlib.tests import TestCaseWithTransport, TestSkipped, features
 from bzrlib.tree import InterTree
 
 
@@ -673,6 +673,21 @@
                               tree_iter_changes, ['bar', 'foo'])
         self.assertEqual(e.paths, ['foo'])
 
+    def test_iter_changes_unversioned_non_ascii(self):
+        """Unversioned non-ascii paths should be reported as unicode"""
+        self.requireFeature(features.UnicodeFilenameFeature)
+        tree = self.make_branch_and_tree('.')
+        self.build_tree_contents([('f', '')])
+        tree.add(['f'], ['f-id'])
+        def tree_iter_changes(tree, files):
+            return list(tree.iter_changes(tree.basis_tree(),
+                specific_files=files, require_versioned=True))
+        tree.lock_read()
+        self.addCleanup(tree.unlock)
+        e = self.assertRaises(errors.PathsNotVersionedError,
+            tree_iter_changes, tree, [u'\xa7', u'\u03c0'])
+        self.assertEqual(e.paths, [u'\xa7', u'\u03c0'])
+
     def get_tree_with_cachable_file_foo(self):
         tree = self.make_branch_and_tree('.')
         tree.lock_write()

=== modified file 'bzrlib/workingtree_4.py'
--- bzrlib/workingtree_4.py	2011-10-11 12:01:51 +0000
+++ bzrlib/workingtree_4.py	2011-12-06 11:20:29 +0000
@@ -969,7 +969,8 @@
                     all_versioned = False
                     break
             if not all_versioned:
-                raise errors.PathsNotVersionedError(paths)
+                raise errors.PathsNotVersionedError(
+                    [p.decode('utf-8') for p in paths])
         # -- remove redundancy in supplied paths to prevent over-scanning --
         search_paths = osutils.minimum_path_selection(paths)
         # sketch:
@@ -1024,7 +1025,8 @@
             found_dir_names = set(dir_name_id[:2] for dir_name_id in found)
             for dir_name in split_paths:
                 if dir_name not in found_dir_names:
-                    raise errors.PathsNotVersionedError(paths)
+                    raise errors.PathsNotVersionedError(
+                        [p.decode('utf-8') for p in paths])
 
         for dir_name_id, trees_info in found.iteritems():
             for index in search_indexes:
@@ -2174,7 +2176,7 @@
                 path_entries = state._entries_for_path(path)
                 if not path_entries:
                     # this specified path is not present at all: error
-                    not_versioned.append(path)
+                    not_versioned.append(path.decode('utf-8'))
                     continue
                 found_versioned = False
                 # for each id at this path
@@ -2188,7 +2190,7 @@
                 if not found_versioned:
                     # none of the indexes was not 'absent' at all ids for this
                     # path.
-                    not_versioned.append(path)
+                    not_versioned.append(path.decode('utf-8'))
             if len(not_versioned) > 0:
                 raise errors.PathsNotVersionedError(not_versioned)
         # -- remove redundancy in supplied specific_files to prevent over-scanning --

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-12-03 03:30:10 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-12-06 11:20:29 +0000
@@ -96,6 +96,10 @@
   for _CompatabilityThunkFeature based test features.
   (Vincent Ladeuil, #897718)
 
+* Make reporting of mistakes involving unversioned files with non-ascii
+  filenames work again without 'Unprintable exception' being shown.
+  (Martin Packman, #898408)
+
 * Provide names for lazily registered hooks. 
   (Neil Martinsen-Burrell, #894609)