Merge lp:~jelmer/bzr/prompt-unicode into lp:bzr

[Jelmer Vernooij]

Require prompts to always be unicode.

Require prompts to always be unicode, as they're going to be re-encoded
using the terminal encoding.

This should prevent problems with prompts that happen to contain non-ascii
characters and are being encoded using the terminal encoding.
(e.g. bug 592083)

We already had a test to make sure that UIFactory.prompt() did the right thing in case
it was passed a unicode prompt with non-ascii characters.

[John A Meinel (jameinel)]

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

On 05/16/2011 03:57 PM, Jelmer Vernooij wrote:
> Jelmer Vernooij has proposed merging lp:~jelmer/bzr/prompt-unicode into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> Bug #592083 in Bazaar: "UnicodeDecodeError in get_password if user name is unicode"
> https://bugs.launchpad.net/bzr/+bug/592083
>
> For more details, see:
> https://code.launchpad.net/~jelmer/bzr/prompt-unicode/+merge/61123
>
> Require prompts to always be unicode, as they're going to be re-encoded
> using the terminal encoding.
>
> This should prevent problems with prompts that happen to contain non-ascii
> characters and are being encoded using the terminal encoding.
> (e.g. bug 592083)
>
> We already had a test to make sure that UIFactory.prompt() did the right thing in case
> it was passed a unicode prompt with non-ascii characters.

This doesn't actually fix the bug, it just moves it. So I'm not really
sure what the benefit is. To *get* the error, we are reading an 8-bit
username and leaving it as an 8-bit string, rather than decoding it to
Unicode.

I don't think requiring u"" format strings actually helps. (Maybe it
gets us closer to python3 compatibility?) As we would have still passed

 u'Are you %(user)?' % ('\xe5\xb5',)

Which would have still failed with a Unicode error.

So I'm happy to fix the bug, but I don't think requiring Unicode objects
actually helps prevent bugs like this.

John
=:->
 review:needsinfo

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

iEYEARECAAYFAk3RL+IACgkQJdeBCYSNAAPrOQCfdgIdz3AVVtbXfWMXtcl9o8BA
gNMAn1qhhCan3T2CAM3JD7OpoNfzBCaK
=Jqc/
-----END PGP SIGNATURE-----

[Jelmer Vernooij (jelmer)]

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 05/16/2011 03:57 PM, Jelmer Vernooij wrote:
> > Jelmer Vernooij has proposed merging lp:~jelmer/bzr/prompt-unicode into
> lp:bzr.
> >
> > Requested reviews:
> > bzr-core (bzr-core)
> > Related bugs:
> > Bug #592083 in Bazaar: "UnicodeDecodeError in get_password if user name is
> unicode"
> > https://bugs.launchpad.net/bzr/+bug/592083
> >
> > For more details, see:
> > https://code.launchpad.net/~jelmer/bzr/prompt-unicode/+merge/61123
> >
> > Require prompts to always be unicode, as they're going to be re-encoded
> > using the terminal encoding.
> >
> > This should prevent problems with prompts that happen to contain non-ascii
> > characters and are being encoded using the terminal encoding.
> > (e.g. bug 592083)
> >
> > We already had a test to make sure that UIFactory.prompt() did the right
> thing in case
> > it was passed a unicode prompt with non-ascii characters.
>
> This doesn't actually fix the bug, it just moves it. So I'm not really
> sure what the benefit is. To *get* the error, we are reading an 8-bit
> username and leaving it as an 8-bit string, rather than decoding it to
> Unicode.
in bzrlib/transport/ssh.py, this now does a decode(osutils._fs_enc) on filename, which is what was causing the problem for the user in the associated bug report.

I guess the unicode checks indeed don't really help if one of the format arguments can still be a regular string. Perhaps we should be checking for that in prompt() ?

Cheers,

Jelmer

[Vincent Ladeuil (vila)]

This looks like it's fixing the bug and is an improvement in any case, let's land it.

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2011-05-03 13:53:46 +0000
+++ bzrlib/builtins.py	2011-05-16 14:05:24 +0000
@@ -4938,7 +4938,7 @@
 
         if not force:
             if not ui.ui_factory.confirm_action(
-                    'Uncommit these revisions',
+                    u'Uncommit these revisions',
                     'bzrlib.builtins.uncommit',
                     {}):
                 self.outf.write('Canceled\n')

=== modified file 'bzrlib/lockdir.py'
--- bzrlib/lockdir.py	2011-03-31 09:01:27 +0000
+++ bzrlib/lockdir.py	2011-05-16 14:05:24 +0000
@@ -355,7 +355,7 @@
             holder_info = self.peek()
         except LockCorrupt, e:
             # The lock info is corrupt.
-            if bzrlib.ui.ui_factory.get_boolean("Break (corrupt %r)" % (self,)):
+            if bzrlib.ui.ui_factory.get_boolean(u"Break (corrupt %r)" % (self,)):
                 self.force_break_corrupt(e.file_data)
             return
         if holder_info is not None:

=== modified file 'bzrlib/msgeditor.py'
--- bzrlib/msgeditor.py	2011-03-30 11:45:54 +0000
+++ bzrlib/msgeditor.py	2011-05-16 14:05:24 +0000
@@ -151,7 +151,7 @@
         edited_content = msg_transport.get_bytes(basename)
         if edited_content == reference_content:
             if not ui.ui_factory.confirm_action(
-                "Commit message was not edited, use anyway",
+                u"Commit message was not edited, use anyway",
                 "bzrlib.msgeditor.unchanged",
                 {}):
                 # Returning "" makes cmd_commit raise 'empty commit message

=== modified file 'bzrlib/tests/per_uifactory/__init__.py'
--- bzrlib/tests/per_uifactory/__init__.py	2011-01-10 22:20:12 +0000
+++ bzrlib/tests/per_uifactory/__init__.py	2011-05-16 14:05:24 +0000
@@ -66,9 +66,8 @@
         # confirm_action should be answered by every ui factory; even
         # noninteractive ones should have a reasonable default
         self._load_responses([True])
-        result = self.factory.confirm_action(
-            'Break a lock?',
-            'bzr.lock.break.confirm',
+        result = self.factory.confirm_action(u'Break a lock?',
+             'bzr.lock.break.confirm',
             {})
         # will be true either because we read it from the input or because
         # that's the default

=== modified file 'bzrlib/tests/test_script.py'
--- bzrlib/tests/test_script.py	2011-05-13 12:51:05 +0000
+++ bzrlib/tests/test_script.py	2011-05-16 14:05:24 +0000
@@ -558,7 +558,7 @@
 
     def run(self):
         if ui.ui_factory.get_boolean(
-            'Really do it',
+            u'Really do it',
             # 'bzrlib.tests.test_script.confirm',
             # {}
             ):

=== modified file 'bzrlib/tests/test_ui.py'
--- bzrlib/tests/test_ui.py	2011-04-05 17:37:53 +0000
+++ bzrlib/tests/test_ui.py	2011-05-16 14:05:24 +0000
@@ -63,7 +63,7 @@
     def test_text_factory_confirm(self):
         # turns into reading a regular boolean
         ui = self.make_test_ui_factory('n\n')
-        self.assertEquals(ui.confirm_action('Should %(thing)s pass?',
+        self.assertEquals(ui.confirm_action(u'Should %(thing)s pass?',
             'bzrlib.tests.test_ui.confirmation',
             {'thing': 'this'},),
             False)
@@ -119,12 +119,12 @@
         stdout = tests.StringIOWrapper()
         stderr = tests.StringIOWrapper()
         factory = _mod_ui_text.TextUIFactory(stdin, stdout, stderr)
-        self.assertEqual(True, factory.get_boolean(""))
-        self.assertEqual(False, factory.get_boolean(""))
-        self.assertEqual(True, factory.get_boolean(""))
-        self.assertEqual(False, factory.get_boolean(""))
-        self.assertEqual(True, factory.get_boolean(""))
-        self.assertEqual(False, factory.get_boolean(""))
+        self.assertEqual(True, factory.get_boolean(u""))
+        self.assertEqual(False, factory.get_boolean(u""))
+        self.assertEqual(True, factory.get_boolean(u""))
+        self.assertEqual(False, factory.get_boolean(u""))
+        self.assertEqual(True, factory.get_boolean(u""))
+        self.assertEqual(False, factory.get_boolean(u""))
         self.assertEqual("foo\n", factory.stdin.read())
         # stdin should be empty
         self.assertEqual('', factory.stdin.readline())
@@ -137,15 +137,15 @@
         stdout = tests.StringIOWrapper()
         stderr = tests.StringIOWrapper()
         factory = _mod_ui_text.TextUIFactory(stdin, stdout, stderr)
-        self.assertEqual(1, factory.get_integer(""))
-        self.assertEqual(-2, factory.get_integer(""))
-        self.assertEqual(42, factory.get_integer(""))
+        self.assertEqual(1, factory.get_integer(u""))
+        self.assertEqual(-2, factory.get_integer(u""))
+        self.assertEqual(42, factory.get_integer(u""))
 
     def test_text_factory_prompt(self):
         # see <https://launchpad.net/bugs/365891>
         StringIO = tests.StringIOWrapper
         factory = _mod_ui_text.TextUIFactory(StringIO(), StringIO(), StringIO())
-        factory.prompt('foo %2e')
+        factory.prompt(u'foo %2e')
         self.assertEqual('', factory.stdout.getvalue())
         self.assertEqual('foo %2e', factory.stderr.getvalue())
 
@@ -166,7 +166,7 @@
                          self.apply_redirected(None, factory.stdout,
                                                factory.stdout,
                                                factory.get_boolean,
-                                               "what do you want"))
+                                               u"what do you want"))
         output = out.getvalue()
         self.assertContainsRe(output,
             "| foo *\r\r  *\r*")
@@ -195,10 +195,10 @@
         factory.stdout.encoding = "utf8"
         # there is no output from the base factory
         self.assertEqual("someuser",
-                         factory.get_username('Hello %(host)s', host='some'))
+                         factory.get_username(u'Hello %(host)s', host='some'))
         self.assertEquals("Hello some: ", factory.stderr.getvalue())
         self.assertEquals('', factory.stdout.getvalue())
-        self.assertEqual("", factory.get_username("Gebruiker"))
+        self.assertEqual("", factory.get_username(u"Gebruiker"))
         # stdin should be empty
         self.assertEqual('', factory.stdin.readline())
 
@@ -352,7 +352,7 @@
         self.assertRaises(
             NotImplementedError,
             self.apply_redirected,
-            None, stdout, stdout, factory.get_boolean, "foo")
+            None, stdout, stdout, factory.get_boolean, u"foo")
 
 
 class TestUIFactoryTests(tests.TestCase):
@@ -371,12 +371,12 @@
 
     def test_canned_input_get_input(self):
         uif = _mod_ui.CannedInputUIFactory([True, 'mbp', 'password', 42])
-        self.assertEqual(True, uif.get_boolean('Extra cheese?'))
-        self.assertEqual('mbp', uif.get_username('Enter your user name'))
+        self.assertEqual(True, uif.get_boolean(u'Extra cheese?'))
+        self.assertEqual('mbp', uif.get_username(u'Enter your user name'))
         self.assertEqual('password',
-                         uif.get_password('Password for %(host)s',
+                         uif.get_password(u'Password for %(host)s',
                                           host='example.com'))
-        self.assertEqual(42, uif.get_integer('And all that jazz ?'))
+        self.assertEqual(42, uif.get_integer(u'And all that jazz ?'))
 
 
 class TestBoolFromString(tests.TestCase):

=== modified file 'bzrlib/transport/ssh.py'
--- bzrlib/transport/ssh.py	2011-01-11 20:20:13 +0000
+++ bzrlib/transport/ssh.py	2011-05-16 14:05:24 +0000
@@ -573,7 +573,8 @@
         return True
     except paramiko.PasswordRequiredException:
         password = ui.ui_factory.get_password(
-            prompt='SSH %(filename)s password', filename=filename)
+            prompt=u'SSH %(filename)s password',
+            filename=filename.decode(osutils._fs_enc))
         try:
             key = pkey_class.from_private_key_file(filename, password)
             paramiko_transport.auth_publickey(username, key)

=== modified file 'bzrlib/ui/__init__.py'
--- bzrlib/ui/__init__.py	2011-04-11 01:23:58 +0000
+++ bzrlib/ui/__init__.py	2011-05-16 14:05:24 +0000
@@ -204,10 +204,10 @@
         """
         return self.get_boolean(prompt % prompt_kwargs)
 
-    def get_password(self, prompt='', **kwargs):
+    def get_password(self, prompt=u'', **kwargs):
         """Prompt the user for a password.
 
-        :param prompt: The prompt to present the user
+        :param prompt: The prompt to present the user (must be unicode)
         :param kwargs: Arguments which will be expanded into the prompt.
                        This lets front ends display different things if
                        they so choose.
@@ -483,7 +483,7 @@
     def get_integer(self, prompt):
         return self.responses.pop(0)
 
-    def get_password(self, prompt='', **kwargs):
+    def get_password(self, prompt=u'', **kwargs):
         return self.responses.pop(0)
 
     def get_username(self, prompt, **kwargs):

=== modified file 'bzrlib/ui/text.py'
--- bzrlib/ui/text.py	2011-04-07 10:36:24 +0000
+++ bzrlib/ui/text.py	2011-05-16 14:05:24 +0000
@@ -114,7 +114,7 @@
                 password = password[:-1]
         return password
 
-    def get_password(self, prompt='', **kwargs):
+    def get_password(self, prompt=u'', **kwargs):
         """Prompt the user for a password.
 
         :param prompt: The prompt to present the user
@@ -198,6 +198,8 @@
         :param kwargs: Dictionary of arguments to insert into the prompt,
             to allow UIs to reformat the prompt.
         """
+        if type(prompt) != unicode:
+            raise ValueError("prompt %r not a unicode string" % prompt)
         if kwargs:
             # See <https://launchpad.net/bugs/365891>
             prompt = prompt % kwargs

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-05-16 11:17:40 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-05-16 14:05:24 +0000
@@ -64,6 +64,10 @@
 * Correct parent is now set when using 'switch -b' with bound branches.
   (A. S. Budden, #513709)
 
+* ``UIFactory.prompt``, ``UIFactory.get_username``,
+  ``UIFactory.get_password`` and ``UIFactory.get_boolean`` now require a
+  unicode prompt to be passed in. (Jelmer Vernooij, #592083)
+
 * ``WT.inventory`` and ``WT.iter_entries_by_dir()`` was not correctly
   reporting subdirectories that were tree references (in formats that
   supported them). (John Arbash Meinel, #764677)