Merge lp:~vila/bzr/799212-non-ascii-confs into lp:bzr

[Vincent Ladeuil]

Clearer exceptions for bad-content config files

This complements the previous submission by making sure all
config files errors related to a non-utf8 or not parseable
content properly mention the appropriate path/url.

The least obvious part is that TransportConfig() objects have
been involved in some bugs and since it uses ConfigObj directly,
it escaped the exception cathcing that was happening in
IniBasedConfig.

[Jelmer Vernooij (jelmer)]

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

[Martin Packman (gz)]

This set of changes is what I had in mind.

As I understand it, bug 502060 is about TransportConfig lacking
try/except, then bug 688677 and bug 797246 are the same non-utf8 issue
in copied code. However I don't understand where bug 799212 comes in
or even what it is really from the report.

One change I'd suggest is making ConfigContentError take the
UnicodeError instance as a second param so the UI has the option of
giving more details like where in the file to look for the bad bytes.

[Vincent Ladeuil (vila)]

> This set of changes is what I had in mind.

Good.

>
> As I understand it, bug 502060 is about TransportConfig lacking
> try/except,

Yes.

> then bug 688677 and bug 797246 are the same non-utf8 issue
> in copied code.

Not exactly but close enough.

> However I don't understand where bug 799212 comes in
> or even what it is really from the report.

Bah, that's just lp being helpful because I reused the same branch (see lp:bzr, a previous fix has already landed).

>
> One change I'd suggest is making ConfigContentError take the
> UnicodeError instance as a second param so the UI has the option of
> giving more details like where in the file to look for the bad bytes.

I thought about that even tried to point to the right line number.

But whatever trick you may want to do at this point you have to inspect the content of the file again. So you may as well split its content and try to decode each line (which I thought was a bit too much) so adding the UnicodeError doesn't add much (and I didn't want to dive into the various possible errors).

We may revisit that if we get new bug reports from people not getting the clue about the encoding.

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2011-06-20 09:31:17 +0000
+++ bzrlib/config.py	2011-06-20 14:07:30 +0000
@@ -692,6 +692,8 @@
             self._parser = ConfigObj(co_input, encoding='utf-8')
         except configobj.ConfigObjError, e:
             raise errors.ParseConfigError(e.errors, e.config.filename)
+        except UnicodeDecodeError:
+            raise errors.ConfigContentError(self.file_name)
         # Make sure self.reload() will use the right file name
         self._parser.filename = self.file_name
         for hook in OldConfigHooks['load']:
@@ -1697,6 +1699,8 @@
             self._config = ConfigObj(self._input, encoding='utf-8')
         except configobj.ConfigObjError, e:
             raise errors.ParseConfigError(e.errors, e.config.filename)
+        except UnicodeError:
+            raise errors.ConfigContentError(self._filename)
         return self._config
 
     def _save(self):
@@ -2178,12 +2182,21 @@
         except errors.NoSuchFile:
             return StringIO()
 
+    def _external_url(self):
+        return urlutils.join(self._transport.external_url(), self._filename)
+
     def _get_configobj(self):
         f = self._get_config_file()
         try:
-            return ConfigObj(f, encoding='utf-8')
+            try:
+                conf = ConfigObj(f, encoding='utf-8')
+            except configobj.ConfigObjError, e:
+                raise errors.ParseConfigError(e.errors, self._external_url())
+            except UnicodeDecodeError:
+                raise errors.ConfigContentError(self._external_url())
         finally:
             f.close()
+        return conf
 
     def _set_configobj(self, configobj):
         out_file = StringIO()
@@ -2285,14 +2298,10 @@
         """Loads the Store from persistent storage."""
         raise NotImplementedError(self.load)
 
-    def _load_from_string(self, str_or_unicode):
+    def _load_from_string(self, bytes):
         """Create a store from a string in configobj syntax.
 
-        :param str_or_unicode: A string representing the file content. This will
-            be encoded to suit store needs internally.
-
-        This is for tests and should not be used in production unless a
-        convincing use case can be demonstrated :)
+        :param bytes: A string representing the file content.
         """
         raise NotImplementedError(self._load_from_string)
 
@@ -2370,21 +2379,22 @@
         for hook in ConfigHooks['load']:
             hook(self)
 
-    def _load_from_string(self, str_or_unicode):
+    def _load_from_string(self, bytes):
         """Create a config store from a string.
 
-        :param str_or_unicode: A string representing the file content. This will
-            be utf-8 encoded internally.
+        :param bytes: A string representing the file content.
         """
         if self.is_loaded():
             raise AssertionError('Already loaded: %r' % (self._config_obj,))
-        co_input = StringIO(str_or_unicode)
+        co_input = StringIO(bytes)
         try:
             # The config files are always stored utf8-encoded
             self._config_obj = ConfigObj(co_input, encoding='utf-8')
         except configobj.ConfigObjError, e:
             self._config_obj = None
             raise errors.ParseConfigError(e.errors, self.external_url())
+        except UnicodeDecodeError:
+            raise errors.ConfigContentError(self.external_url())
 
     def save(self):
         if not self.is_loaded():
 
=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2011-06-02 16:57:09 +0000
+++ bzrlib/errors.py	2011-06-20 14:07:30 +0000
@@ -1770,6 +1770,15 @@
     _fmt = "Working tree has conflicts."
 
 
+class ConfigContentError(BzrError):
+
+    _fmt = "Config file %(filename)s is not UTF-8 encoded\n"
+
+    def __init__(self, filename):
+        BzrError.__init__(self)
+        self.filename = filename
+
+
 class ParseConfigError(BzrError):
 
     _fmt = "Error(s) parsing config file %(filename)s:\n%(errors)s"
 
=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2011-06-20 09:31:17 +0000
+++ bzrlib/tests/test_config.py	2011-06-20 14:07:30 +0000
@@ -1871,9 +1871,34 @@
 
 class TestTransportConfig(tests.TestCaseWithTransport):
 
+    def test_load_utf8(self):
+        """Ensure we can load an utf8-encoded file."""
+        t = self.get_transport()
+        unicode_user = u'b\N{Euro Sign}ar'
+        unicode_content = u'user=%s' % (unicode_user,)
+        utf8_content = unicode_content.encode('utf8')
+        # Store the raw content in the config file
+        t.put_bytes('foo.conf', utf8_content)
+        conf = config.TransportConfig(t, 'foo.conf')
+        self.assertEquals(unicode_user, conf.get_option('user'))
+
+    def test_load_non_ascii(self):
+        """Ensure we display a proper error on non-ascii, non utf-8 content."""
+        t = self.get_transport()
+        t.put_bytes('foo.conf', 'user=foo\n#\xff\n')
+        conf = config.TransportConfig(t, 'foo.conf')
+        self.assertRaises(errors.ConfigContentError, conf._get_configobj)
+
+    def test_load_erroneous_content(self):
+        """Ensure we display a proper error on content that can't be parsed."""
+        t = self.get_transport()
+        t.put_bytes('foo.conf', '[open_section\n')
+        conf = config.TransportConfig(t, 'foo.conf')
+        self.assertRaises(errors.ParseConfigError, conf._get_configobj)
+
     def test_get_value(self):
         """Test that retreiving a value from a section is possible"""
-        bzrdir_config = config.TransportConfig(transport.get_transport('.'),
+        bzrdir_config = config.TransportConfig(self.get_transport('.'),
                                                'control.conf')
         bzrdir_config.set_option('value', 'key', 'SECTION')
         bzrdir_config.set_option('value2', 'key2')
@@ -2328,12 +2353,22 @@
         self.assertRaises(AssertionError, store._load_from_string, 'bar=baz')
 
 
-class TestBug799212(TestStore):
-
-   def test_load_utf8(self):
+class TestIniFileStoreContent(tests.TestCaseWithTransport):
+    """Simulate loading a config store without content of various encodings.
+
+    All files produced by bzr are in utf8 content.
+
+    Users may modify them manually and end up with a file that can't be
+    loaded. We need to issue proper error messages in this case.
+    """
+
+    invalid_utf8_char = '\xff'
+
+    def test_load_utf8(self):
+        """Ensure we can load an utf8-encoded file."""
         t = self.get_transport()
         # From http://pad.lv/799212
-        unicode_user = u'Piotr O\u017carowski'
+        unicode_user = u'b\N{Euro Sign}ar'
         unicode_content = u'user=%s' % (unicode_user,)
         utf8_content = unicode_content.encode('utf8')
         # Store the raw content in the config file
@@ -2343,6 +2378,58 @@
         stack = config.Stack([store.get_sections], store)
         self.assertEquals(unicode_user, stack.get('user'))
 
+    def test_load_non_ascii(self):
+        """Ensure we display a proper error on non-ascii, non utf-8 content."""
+        t = self.get_transport()
+        t.put_bytes('foo.conf', 'user=foo\n#%s\n' % (self.invalid_utf8_char,))
+        store = config.IniFileStore(t, 'foo.conf')
+        self.assertRaises(errors.ConfigContentError, store.load)
+
+    def test_load_erroneous_content(self):
+        """Ensure we display a proper error on content that can't be parsed."""
+        t = self.get_transport()
+        t.put_bytes('foo.conf', '[open_section\n')
+        store = config.IniFileStore(t, 'foo.conf')
+        self.assertRaises(errors.ParseConfigError, store.load)
+
+
+class TestIniConfigContent(tests.TestCaseWithTransport):
+    """Simulate loading a IniBasedConfig without content of various encodings.
+
+    All files produced by bzr are in utf8 content.
+
+    Users may modify them manually and end up with a file that can't be
+    loaded. We need to issue proper error messages in this case.
+    """
+
+    invalid_utf8_char = '\xff'
+
+    def test_load_utf8(self):
+        """Ensure we can load an utf8-encoded file."""
+        # From http://pad.lv/799212
+        unicode_user = u'b\N{Euro Sign}ar'
+        unicode_content = u'user=%s' % (unicode_user,)
+        utf8_content = unicode_content.encode('utf8')
+        # Store the raw content in the config file
+        with open('foo.conf', 'wb') as f:
+            f.write(utf8_content)
+        conf = config.IniBasedConfig(file_name='foo.conf')
+        self.assertEquals(unicode_user, conf.get_user_option('user'))
+
+    def test_load_badly_encoded_content(self):
+        """Ensure we display a proper error on non-ascii, non utf-8 content."""
+        with open('foo.conf', 'wb') as f:
+            f.write('user=foo\n#%s\n' % (self.invalid_utf8_char,))
+        conf = config.IniBasedConfig(file_name='foo.conf')
+        self.assertRaises(errors.ConfigContentError, conf._get_parser)
+
+    def test_load_erroneous_content(self):
+        """Ensure we display a proper error on content that can't be parsed."""
+        with open('foo.conf', 'wb') as f:
+            f.write('[open_section\n')
+        conf = config.IniBasedConfig(file_name='foo.conf')
+        self.assertRaises(errors.ParseConfigError, conf._get_parser)
+
 
 class TestMutableStore(TestStore):
 
@@ -3052,6 +3139,11 @@
         self.assertEquals({}, conf._get_config())
         self._got_user_passwd(None, None, conf, 'http', 'foo.net')
 
+    def test_non_utf8_config(self):
+        conf = config.AuthenticationConfig(_file=StringIO(
+                'foo = bar\xff'))
+        self.assertRaises(errors.ConfigContentError, conf._get_config)
+        
     def test_missing_auth_section_header(self):
         conf = config.AuthenticationConfig(_file=StringIO('foo = bar'))
         self.assertRaises(ValueError, conf.get_credentials, 'ftp', 'foo.net')
 
=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-06-20 09:31:17 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-06-20 14:07:30 +0000
@@ -29,6 +29,10 @@
 Bug Fixes
 *********
 
+* Display a proper error message when a config file content cannot be
+  decoded as UTF-8 or when it cannot be parsed.
+  (Vincent Ladeuil, #502060, #688677, #792246)
+
 * Properly load utf8-encoded config files. (Vincent Ladeuil, #799212)
 
 .. Fixes for situations where bzr would previously crash or give incorrect