Merge lp:~vila/bzr/376582-auth-prompt into lp:~bzr/bzr/trunk-old

[Vincent Ladeuil]

[Vincent Ladeuil (vila)]

There is no good reason to use stdout for prompting the user. Bug #376582 raised the issue that doing that forbid the redirection of command outputs.

This patch use stderr instead and should address the issue.

[Ian Clatworthy (ian-clatworthy)]

I suspect this change ought to be mentioned as a Compatibility break, not just a bug fix, in NEWS. When landing, make sure you tweak NEWS to put this in 1.16 (not 1.15) as well. :-)

review: Approve

=== modified file 'NEWS'
--- NEWS	2009-06-02 01:37:30 +0000
+++ NEWS	2009-06-02 14:35:20 +0000
@@ -12,6 +12,11 @@
 Compatibility Breaks
 ********************
 
+* Display prompt on stderr (instead of stdout) when querying users so
+  that the output of commands can be safely redirected.
+  (Vincent Ladeuil, #376582)
+
+
 New Features
 ************
 
 
=== modified file 'bzrlib/plugins/launchpad/test_register.py'
--- bzrlib/plugins/launchpad/test_register.py	2009-03-23 14:59:43 +0000
+++ bzrlib/plugins/launchpad/test_register.py	2009-06-02 14:35:20 +0000
@@ -335,12 +335,14 @@
         g_conf = config.GlobalConfig()
         g_conf.set_user_option('email', 'Test User <[e-mail]>')
         stdout = tests.StringIOWrapper()
+        stderr = tests.StringIOWrapper()
         ui.ui_factory = tests.TestUIFactory(stdin='userpass\n',
-                                            stdout=stdout)
+                                            stdout=stdout, stderr=stderr)
         self.assertIs(None, service.registrant_password)
         service.gather_user_credentials()
         self.assertEqual('[e-mail]', service.registrant_email)
         self.assertEqual('userpass', service.registrant_password)
-        self.assertContainsRe(stdout.getvalue(),
+        self.assertEquals('', stdout.getvalue())
+        self.assertContainsRe(stderr.getvalue(),
                              'launchpad.net password for test@user\\.com')
 
 
=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2009-04-27 16:10:10 +0000
+++ bzrlib/tests/test_config.py	2009-06-02 14:35:20 +0000
@@ -1461,7 +1461,8 @@
     """Test AuthenticationConfig behaviour"""
 
     def _check_default_password_prompt(self, expected_prompt_format, scheme,
-                              host=None, port=None, realm=None, path=None):
+                                       host=None, port=None, realm=None,
+                                       path=None):
         if host is None:
             host = 'bar.org'
         user, password = 'jim', 'precious'
@@ -1470,17 +1471,20 @@
             'user': user, 'realm': realm}
 
         stdout = tests.StringIOWrapper()
+        stderr = tests.StringIOWrapper()
         ui.ui_factory = tests.TestUIFactory(stdin=password + '\n',
-                                            stdout=stdout)
+                                            stdout=stdout, stderr=stderr)
         # We use an empty conf so that the user is always prompted
         conf = config.AuthenticationConfig()
         self.assertEquals(password,
                           conf.get_password(scheme, host, user, port=port,
                                             realm=realm, path=path))
-        self.assertEquals(stdout.getvalue(), expected_prompt)
+        self.assertEquals(expected_prompt, stderr.getvalue())
+        self.assertEquals('', stdout.getvalue())
 
     def _check_default_username_prompt(self, expected_prompt_format, scheme,
-                              host=None, port=None, realm=None, path=None):
+                                       host=None, port=None, realm=None,
+                                       path=None):
         if host is None:
             host = 'bar.org'
         username = 'jim'
@@ -1488,13 +1492,15 @@
             'scheme': scheme, 'host': host, 'port': port,
             'realm': realm}
         stdout = tests.StringIOWrapper()
+        stderr = tests.StringIOWrapper()
         ui.ui_factory = tests.TestUIFactory(stdin=username+ '\n',
-                                            stdout=stdout)
+                                            stdout=stdout, stderr=stderr)
         # We use an empty conf so that the user is always prompted
         conf = config.AuthenticationConfig()
         self.assertEquals(username, conf.get_user(scheme, host, port=port,
                           realm=realm, path=path, ask=True))
-        self.assertEquals(stdout.getvalue(), expected_prompt)
+        self.assertEquals(expected_prompt, stderr.getvalue())
+        self.assertEquals('', stdout.getvalue())
 
     def test_username_defaults_prompts(self):
         # HTTP prompts can't be tested here, see test_http.py
 
=== modified file 'bzrlib/tests/test_http.py'
--- bzrlib/tests/test_http.py	2009-05-27 13:59:47 +0000
+++ bzrlib/tests/test_http.py	2009-06-02 14:35:20 +0000
@@ -1569,15 +1569,18 @@
         self.server.add_user('joe', 'foo')
         t = self.get_user_transport(None, None)
         stdout = tests.StringIOWrapper()
-        ui.ui_factory = tests.TestUIFactory(stdin='joe\nfoo\n', stdout=stdout)
+        stderr = tests.StringIOWrapper()
+        ui.ui_factory = tests.TestUIFactory(stdin='joe\nfoo\n',
+                                            stdout=stdout, stderr=stderr)
         self.assertEqual('contents of a\n',t.get('a').read())
         # stdin should be empty
         self.assertEqual('', ui.ui_factory.stdin.readline())
-        stdout.seek(0)
+        stderr.seek(0)
         expected_prompt = self._expected_username_prompt(t._unqualified_scheme)
-        self.assertEquals(expected_prompt, stdout.read(len(expected_prompt)))
+        self.assertEquals(expected_prompt, stderr.read(len(expected_prompt)))
+        self.assertEquals('', stdout.getvalue())
         self._check_password_prompt(t._unqualified_scheme, 'joe',
-                                    stdout.readline())
+                                    stderr.readline())
 
     def test_prompt_for_password(self):
         if self._testing_pycurl():
@@ -1588,12 +1591,15 @@
         self.server.add_user('joe', 'foo')
         t = self.get_user_transport('joe', None)
         stdout = tests.StringIOWrapper()
-        ui.ui_factory = tests.TestUIFactory(stdin='foo\n', stdout=stdout)
-        self.assertEqual('contents of a\n',t.get('a').read())
+        stderr = tests.StringIOWrapper()
+        ui.ui_factory = tests.TestUIFactory(stdin='foo\n',
+                                            stdout=stdout, stderr=stderr)
+        self.assertEqual('contents of a\n', t.get('a').read())
         # stdin should be empty
         self.assertEqual('', ui.ui_factory.stdin.readline())
         self._check_password_prompt(t._unqualified_scheme, 'joe',
-                                    stdout.getvalue())
+                                    stderr.getvalue())
+        self.assertEquals('', stdout.getvalue())
         # And we shouldn't prompt again for a different request
         # against the same transport.
         self.assertEqual('contents of b\n',t.get('b').read())
 
=== modified file 'bzrlib/tests/test_ui.py'
--- bzrlib/tests/test_ui.py	2009-04-24 13:30:48 +0000
+++ bzrlib/tests/test_ui.py	2009-06-02 14:35:20 +0000
@@ -67,15 +67,17 @@
         self.assertEqual('', stdout.getvalue())
 
     def test_text_factory_ascii_password(self):
-        ui = TestUIFactory(stdin='secret\n', stdout=StringIOWrapper())
+        ui = TestUIFactory(stdin='secret\n', stdout=StringIOWrapper(),
+                           stderr=StringIOWrapper())
         pb = ui.nested_progress_bar()
         try:
             self.assertEqual('secret',
                              self.apply_redirected(ui.stdin, ui.stdout,
-                                                   ui.stdout,
+                                                   ui.stderr,
                                                    ui.get_password))
             # ': ' is appended to prompt
-            self.assertEqual(': ', ui.stdout.getvalue())
+            self.assertEqual(': ', ui.stderr.getvalue())
+            self.assertEqual('', ui.stdout.readline())
             # stdin should be empty
             self.assertEqual('', ui.stdin.readline())
         finally:
@@ -88,21 +90,22 @@
         it to utf8 to test that we transport the password correctly.
         """
         ui = TestUIFactory(stdin=u'baz\u1234'.encode('utf8'),
-                           stdout=StringIOWrapper())
-        ui.stdin.encoding = 'utf8'
-        ui.stdout.encoding = ui.stdin.encoding
+                           stdout=StringIOWrapper(),
+                           stderr=StringIOWrapper())
+        ui.stderr.encoding = ui.stdout.encoding = ui.stdin.encoding = 'utf8'
         pb = ui.nested_progress_bar()
         try:
-            password = self.apply_redirected(ui.stdin, ui.stdout, ui.stdout,
+            password = self.apply_redirected(ui.stdin, ui.stdout, ui.stderr,
                                              ui.get_password,
                                              u'Hello \u1234 %(user)s',
                                              user=u'some\u1234')
             # We use StringIO objects, we need to decode them
             self.assertEqual(u'baz\u1234', password.decode('utf8'))
             self.assertEqual(u'Hello \u1234 some\u1234: ',
-                             ui.stdout.getvalue().decode('utf8'))
-            # stdin should be empty
+                             ui.stderr.getvalue().decode('utf8'))
+            # stdin and stdout should be empty
             self.assertEqual('', ui.stdin.readline())
+            self.assertEqual('', ui.stdout.readline())
         finally:
             pb.finished()
 
@@ -193,6 +196,7 @@
                                  "yes\nn\nnot an answer\n"
                                  "no\nfoo\n")
         factory.stdout = StringIO()
+        factory.stderr = StringIO()
         # there is no output from the base factory
         self.assertEqual(True, factory.get_boolean(""))
         self.assertEqual(True, factory.get_boolean(""))
@@ -223,8 +227,10 @@
 
     def test_text_factory_prompt(self):
         # see <https://launchpad.net/bugs/365891>
-        factory = TextUIFactory(None, StringIO(), StringIO())
+        factory = TextUIFactory(None, StringIO(), StringIO(), StringIO())
         factory.prompt('foo %2e')
+        self.assertEqual('', factory.stdout.getvalue())
+        self.assertEqual('foo %2e', factory.stderr.getvalue())
 
     def test_text_factory_prompts_and_clears(self):
         # a get_boolean call should clear the pb before prompting
@@ -263,37 +269,41 @@
         factory = SilentUIFactory()
         factory.stdin = StringIO("someuser\n\n")
         factory.stdout = StringIO()
-        self.assertEquals(None, 
+        factory.stderr = StringIO()
+        self.assertEquals(None,
             factory.get_username(u'Hello\u1234 %(host)s', host=u'some\u1234'))
         self.assertEquals("", factory.stdout.getvalue())
+        self.assertEquals("", factory.stderr.getvalue())
         self.assertEquals("someuser\n\n", factory.stdin.getvalue())
 
     def test_text_ui_getusername(self):
         factory = TextUIFactory(None, None, None)
         factory.stdin = StringIO("someuser\n\n")
         factory.stdout = StringIO()
+        factory.stderr = StringIO()
         factory.stdout.encoding = "utf8"
         # there is no output from the base factory
-        self.assertEqual("someuser", 
-            factory.get_username('Hello %(host)s', host='some'))
-        self.assertEquals("Hello some: ", factory.stdout.getvalue())
+        self.assertEqual("someuser",
+                         factory.get_username('Hello %(host)s', host='some'))
+        self.assertEquals("Hello some: ", factory.stderr.getvalue())
+        self.assertEquals('', factory.stdout.getvalue())
         self.assertEqual("", factory.get_username("Gebruiker"))
         # stdin should be empty
         self.assertEqual('', factory.stdin.readline())
 
     def test_text_ui_getusername_utf8(self):
         ui = TestUIFactory(stdin=u'someuser\u1234'.encode('utf8'),
-                           stdout=StringIOWrapper())
-        ui.stdin.encoding = "utf8"
-        ui.stdout.encoding = ui.stdin.encoding
+                           stdout=StringIOWrapper(), stderr=StringIOWrapper())
+        ui.stderr.encoding = ui.stdout.encoding = ui.stdin.encoding = "utf8"
         pb = ui.nested_progress_bar()
         try:
             # there is no output from the base factory
-            username = self.apply_redirected(ui.stdin, ui.stdout, ui.stdout,
+            username = self.apply_redirected(ui.stdin, ui.stdout, ui.stderr,
                 ui.get_username, u'Hello\u1234 %(host)s', host=u'some\u1234')
             self.assertEquals(u"someuser\u1234", username.decode('utf8'))
-            self.assertEquals(u"Hello\u1234 some\u1234: ", 
-                ui.stdout.getvalue().decode("utf8"))
+            self.assertEquals(u"Hello\u1234 some\u1234: ",
+                              ui.stderr.getvalue().decode("utf8"))
+            self.assertEquals('', ui.stdout.getvalue())
         finally:
             pb.finished()
 
 
=== modified file 'bzrlib/ui/__init__.py'
--- bzrlib/ui/__init__.py	2009-04-24 15:49:33 +0000
+++ bzrlib/ui/__init__.py	2009-06-02 14:35:20 +0000
@@ -227,7 +227,7 @@
             prompt = prompt % kwargs
         prompt = prompt.encode(osutils.get_terminal_encoding(), 'replace')
         self.clear_term()
-        self.stdout.write(prompt)
+        self.stderr.write(prompt)
 
     def note(self, msg):
         """Write an already-formatted message."""