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

[Martin Packman]

Fix unicode handling in bzrlib.conflicts ui code

Conflict objects contain paths, which are unicode, but the conflicts ui calls str() on them which only works for ascii paths.

This branch adds tests that cover the Conflict stringifying code, and changes str() calls. Note, despite changing the __str__ method to __unicode__ this won't protect other callers from doing the wrong thing. Anyone currently using str() will continue to work in the ascii case and continue to fail in the non-ascii case due to the joys of Python string logic.

A particularly fun gotcha was the per-conflict tests actually passed regardless, because of this:

>>> class U(object):
... def __str__(self):
... return u"\xa7"
...
>>> unicode(U())
u'\xa7'
>>> str(U())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeEncodeError: 'ascii' codec can't encode character u'\xa7' in position 0: ordinal not in range(128)

[Martin Packman (gz)]

Forgot to mention: TestUnicodePathsOnAsciiTerminal is currently skipped because the ui code doesn't decide what to do in this case yet apparently. That's really another issue so this fix shouldn't depend on that being resolved.

[John A Meinel (jameinel)]

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

On 5/20/2011 3:26 PM, Martin [gz] wrote:
> Martin [gz] has proposed merging lp:~gz/bzr/conflicts_non_ascii_ui_686161 into lp:bzr with lp:~gz/bzr/trivial_refactor_conflict_stanza_tests as a prerequisite.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> Bug #686161 in Bazaar: "conflicts involving unicode paths can't be displayed"
> https://bugs.launchpad.net/bzr/+bug/686161
>
> For more details, see:
> https://code.launchpad.net/~gz/bzr/conflicts_non_ascii_ui_686161/+merge/61763
>
> Conflict objects contain paths, which are unicode, but the conflicts ui calls str() on them which only works for ascii paths.
>
> This branch adds tests that cover the Conflict stringifying code, and changes str() calls. Note, despite changing the __str__ method to __unicode__ this won't protect other callers from doing the wrong thing. Anyone currently using str() will continue to work in the ascii case and continue to fail in the non-ascii case due to the joys of Python string logic.
>
> A particularly fun gotcha was the per-conflict tests actually passed regardless, because of this:
>
>>>> class U(object):
> ... def __str__(self):
> ... return u"\xa7"
> ...
>>>> unicode(U())
> u'\xa7'
>>>> str(U())
> Traceback (most recent call last):
> File "<stdin>", line 1, in <module>
> UnicodeEncodeError: 'ascii' codec can't encode character u'\xa7' in position 0: ordinal not in range(128)
>

Looks fine to me.

 merge: approve

John
=:->

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

iEYEARECAAYFAk3WcnsACgkQJdeBCYSNAAOJ0wCguczgoaf5SdOLfFbLRNFVucBU
RR8AnRQy0plKnZBN0vnex9aune5M+HWH
=BhG6
-----END PGP SIGNATURE-----

review: Approve

[Martin Packman (gz)]

sent to pqm by email

[Martin Packman (gz)]

sent to pqm by email

[Martin Packman (gz)]

My understanding of Python string method logic was wrong, with no __str__ method the __repr__ is used as a fallback instead. This is a good thing, it means callers can't accidentally hit UnicodeError if they do the wrong thing.

However, it broke a bunch of tests in other commands that print conflicts, which I missed grepping the source. This is because trace.warning and trace.note implicitly str() their first parameter, and it needs unicode() instead. The existing coverage looks pretty good here and as the tests broke with the repr output, I don't think I need to hunt around trying to inject non-ascii text into these various commands that print conflicts.

[Martin Packman (gz)]

sent to pqm by email

[Martin Packman (gz)]

Trying to finish this quickly to land on the last day was a bad idea. :)

I've gone back a few revisions and cleaned up some dumb mistakes and will try on PQM again, and repropose if it still runs into problems.

[Martin Packman (gz)]

sent to pqm by email

=== modified file 'bzrlib/conflicts.py'
--- bzrlib/conflicts.py	2011-02-08 13:56:49 +0000
+++ bzrlib/conflicts.py	2011-05-21 16:48:38 +0000
@@ -72,7 +72,7 @@
                     continue
                 self.outf.write(conflict.path + '\n')
             else:
-                self.outf.write(str(conflict) + '\n')
+                self.outf.write(unicode(conflict) + '\n')
 
 
 resolve_action_registry = registry.Registry()
@@ -148,7 +148,7 @@
                     trace.note('%d conflict(s) auto-resolved.', len(resolved))
                     trace.note('Remaining conflicts:')
                     for conflict in un_resolved:
-                        trace.note(conflict)
+                        trace.note(unicode(conflict))
                     return 1
                 else:
                     trace.note('All conflicts resolved.')
@@ -291,7 +291,7 @@
     def to_strings(self):
         """Generate strings for the provided conflicts"""
         for conflict in self:
-            yield str(conflict)
+            yield unicode(conflict)
 
     def remove_files(self, tree):
         """Remove the THIS, BASE and OTHER files for listed conflicts"""
@@ -390,7 +390,7 @@
     def __ne__(self, other):
         return not self.__eq__(other)
 
-    def __str__(self):
+    def __unicode__(self):
         return self.format % self.__dict__
 
     def __repr__(self):

=== modified file 'bzrlib/merge.py'
--- bzrlib/merge.py	2011-05-18 16:42:48 +0000
+++ bzrlib/merge.py	2011-05-21 16:48:38 +0000
@@ -888,7 +888,7 @@
                 self.tt.iter_changes(), self.change_reporter)
         self.cook_conflicts(fs_conflicts)
         for conflict in self.cooked_conflicts:
-            trace.warning(conflict)
+            trace.warning(unicode(conflict))
 
     def _entries3(self):
         """Gather data about files modified between three trees.

=== modified file 'bzrlib/status.py'
--- bzrlib/status.py	2011-03-30 11:50:40 +0000
+++ bzrlib/status.py	2011-05-21 16:48:38 +0000
@@ -194,7 +194,7 @@
                     prefix = 'C  '
                 else:
                     prefix = ' '
-                to_file.write("%s %s\n" % (prefix, conflict))
+                to_file.write("%s %s\n" % (prefix, unicode(conflict)))
             # Show files that were requested but don't exist (and are
             # not versioned).  We don't involve delta in this; these
             # paths are really the province of just the status

=== modified file 'bzrlib/tests/blackbox/test_conflicts.py'
--- bzrlib/tests/blackbox/test_conflicts.py	2010-11-07 16:32:51 +0000
+++ bzrlib/tests/blackbox/test_conflicts.py	2011-05-21 16:48:38 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006, 2007, 2009, 2010 Canonical Ltd
+# Copyright (C) 2006, 2007, 2009, 2010, 2011 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
@@ -21,29 +21,31 @@
     )
 from bzrlib.tests import script
 
-def make_tree_with_conflicts(test, this_path='this', other_path='other'):
+
+def make_tree_with_conflicts(test, this_path='this', other_path='other',
+        prefix='my'):
     this_tree = test.make_branch_and_tree(this_path)
     test.build_tree_contents([
-        ('%s/myfile' % (this_path,), 'this content\n'),
-        ('%s/my_other_file' % (this_path,), 'this content\n'),
-        ('%s/mydir/' % (this_path,),),
+        ('%s/%sfile' % (this_path, prefix), 'this content\n'),
+        ('%s/%s_other_file' % (this_path, prefix), 'this content\n'),
+        ('%s/%sdir/' % (this_path, prefix),),
         ])
-    this_tree.add('myfile')
-    this_tree.add('my_other_file')
-    this_tree.add('mydir')
+    this_tree.add(prefix+'file')
+    this_tree.add(prefix+'_other_file')
+    this_tree.add(prefix+'dir')
     this_tree.commit(message="new")
     other_tree = this_tree.bzrdir.sprout(other_path).open_workingtree()
     test.build_tree_contents([
-        ('%s/myfile' % (other_path,), 'contentsb\n'),
-        ('%s/my_other_file' % (other_path,), 'contentsb\n'),
+        ('%s/%sfile' % (other_path, prefix), 'contentsb\n'),
+        ('%s/%s_other_file' % (other_path, prefix), 'contentsb\n'),
         ])
-    other_tree.rename_one('mydir', 'mydir2')
+    other_tree.rename_one(prefix+'dir', prefix+'dir2')
     other_tree.commit(message="change")
     test.build_tree_contents([
-        ('%s/myfile' % (this_path,), 'contentsa2\n'),
-        ('%s/my_other_file' % (this_path,), 'contentsa2\n'),
+        ('%s/%sfile' % (this_path, prefix), 'contentsa2\n'),
+        ('%s/%s_other_file' % (this_path, prefix), 'contentsa2\n'),
         ])
-    this_tree.rename_one('mydir', 'mydir3')
+    this_tree.rename_one(prefix+'dir', prefix+'dir3')
     this_tree.commit(message='change')
     this_tree.merge_from_branch(other_tree.branch)
     return this_tree, other_tree
@@ -79,3 +81,45 @@
 Path conflict: mydir3 / mydir2
 Text conflict in myfile
 """)
+
+
+class TestUnicodePaths(tests.TestCaseWithTransport):
+    """Unicode characters in conflicts should be displayed properly"""
+
+    encoding = "UTF-8"
+
+    def _as_output(self, text):
+        return text
+
+    def test_messages(self):
+        """Conflict messages involving non-ascii paths are displayed okay"""
+        make_tree_with_conflicts(self, "branch", prefix=u"\xA7")
+        out, err = self.run_bzr(["conflicts", "-d", "branch"],
+            encoding=self.encoding)
+        self.assertEqual(out.decode(self.encoding),
+            u"Text conflict in \xA7_other_file\n"
+            u"Path conflict: \xA7dir3 / \xA7dir2\n"
+            u"Text conflict in \xA7file\n")
+        self.assertEqual(err, "")
+
+    def test_text_conflict_paths(self):
+        """Text conflicts on non-ascii paths are displayed okay"""
+        make_tree_with_conflicts(self, "branch", prefix=u"\xA7")
+        out, err = self.run_bzr(["conflicts", "-d", "branch", "--text"],
+            encoding=self.encoding)
+        self.assertEqual(out.decode(self.encoding),
+            u"\xA7_other_file\n"
+            u"\xA7file\n")
+        self.assertEqual(err, "")
+
+
+class TestUnicodePathsOnAsciiTerminal(TestUnicodePaths):
+    """Undisplayable unicode characters in conflicts should be escaped"""
+
+    encoding = "ascii"
+
+    def setUp(self):
+        self.skip("Need to decide if replacing is the desired behaviour")
+
+    def _as_output(self, text):
+        return text.encode(self.encoding, "replace")

=== modified file 'bzrlib/tests/test_conflicts.py'
--- bzrlib/tests/test_conflicts.py	2011-05-21 16:48:37 +0000
+++ bzrlib/tests/test_conflicts.py	2011-05-21 16:48:38 +0000
@@ -125,10 +125,17 @@
         self.assertEqual(conflicts.ConflictList([]), tree.conflicts())
 
 
-class TestConflictStanzas(tests.TestCase):
+class TestPerConflict(tests.TestCase):
 
     scenarios = scenarios.multiply_scenarios(vary_by_conflicts())
 
+    def test_stringification(self):
+        text = unicode(self.conflict)
+        self.assertContainsString(text, self.conflict.path)
+        self.assertContainsString(text.lower(), "conflict")
+        self.assertContainsString(repr(self.conflict),
+            self.conflict.__class__.__name__)
+
     def test_stanza_roundtrip(self):
         p = self.conflict
         o = conflicts.Conflict.factory(**p.as_stanza().as_dict())
@@ -159,13 +166,17 @@
             self.assertStartsWith(stanza['conflict_file_id'], u'\xeed')
 
 
-class TestConflictListStanzas(tests.TestCase):
+class TestConflictList(tests.TestCase):
 
     def test_stanzas_roundtrip(self):
         stanzas_iter = example_conflicts.to_stanzas()
         processed = conflicts.ConflictList.from_stanzas(stanzas_iter)
         self.assertEqual(example_conflicts, processed)
 
+    def test_stringification(self):
+        for text, o in zip(example_conflicts.to_strings(), example_conflicts):
+            self.assertEqual(text, unicode(o))
+
 
 # FIXME: The shell-like tests should be converted to real whitebox tests... or
 # moved to a blackbox module -- vila 20100205

=== modified file 'bzrlib/tests/test_transform.py'
--- bzrlib/tests/test_transform.py	2011-05-13 12:51:05 +0000
+++ bzrlib/tests/test_transform.py	2011-05-21 16:48:38 +0000
@@ -811,7 +811,7 @@
         raw_conflicts = resolve_conflicts(tt)
         cooked_conflicts = cook_conflicts(raw_conflicts, tt)
         tt.finalize()
-        conflicts_s = [str(c) for c in cooked_conflicts]
+        conflicts_s = [unicode(c) for c in cooked_conflicts]
         self.assertEqual(len(cooked_conflicts), len(conflicts_s))
         self.assertEqual(conflicts_s[0], 'Conflict adding file dorothy.  '
                                          'Moved existing file to '

=== modified file 'bzrlib/transform.py'
--- bzrlib/transform.py	2011-05-13 14:02:14 +0000
+++ bzrlib/transform.py	2011-05-21 16:48:38 +0000
@@ -2578,7 +2578,7 @@
             precomputed_delta = None
         conflicts = cook_conflicts(raw_conflicts, tt)
         for conflict in conflicts:
-            trace.warning(conflict)
+            trace.warning(unicode(conflict))
         try:
             wt.add_conflicts(conflicts)
         except errors.UnsupportedOperation:
@@ -2820,7 +2820,7 @@
                 unversioned_filter=working_tree.is_ignored)
             delta.report_changes(tt.iter_changes(), change_reporter)
         for conflict in conflicts:
-            trace.warning(conflict)
+            trace.warning(unicode(conflict))
         pp.next_phase()
         tt.apply()
         working_tree.set_merge_modified(merge_modified)

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-05-20 13:28:35 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-05-21 16:48:38 +0000
@@ -87,6 +87,9 @@
   submit_location public_location`` never sets ``submit_branch``
   nor ``public_branch``.  (Vincent Ladeuil)
 
+* Conflicts involving non-ascii filenames are now properly reported rather
+  than failing with a UnicodeEncodeError. (Martin [GZ], #686161)
+
 * Correct parent is now set when using 'switch -b' with bound branches.
   (A. S. Budden, #513709)