Merge lp:~bialix/bzr/2.2-configobj into lp:bzr/2.2

[Alexander Belchenko]

Ensure config options containing triple quotes won't corrupt config files.

Fixed bug in the bundled copy of ConfigObj with quoting of triple quotes
in the value string. Fix suggested by ConfigObj's author Michael Foord.

This patch provides the fix to 2.2 series, should I provide the similar patches for 2.3 and bzr.dev, or it will be merged to other series by some integration process?

[Martin Pool (mbp)]

It looks like there's some concern in that bug, but to me this seems reasonable and uncontroversial. Am I missing something? Otherwise, approved. Thanks.

review: Approve

[Alexander Belchenko (bialix)]

Martin Pool пишет:
> Review: Approve
> It looks like there's some concern in that bug, but to me this seems reasonable and uncontroversial. Am I missing something? Otherwise, approved. Thanks.

The test clearly shows that there is indeed bug in configobj.
Bug report much more verbose about what we should do inside QBzr to
avoid such sort of the problems, but it does not mean this bug should
not be fixed.

--
All the dude wanted was his rug back

[Vincent Ladeuil (vila)]

The concern, to me, will be that you seem to work around a second bug in the test.

Otherwise, this proposal fixes the bug it says it fixes, so it should be landed (especially if it's also fixed upstream, but in turn, this deserves a comment if only to inform the poor soul doing the next merge of configobj upstream that he should check that the fix didn't regress).

I'll sync with bialix to land this asap.

[Vincent Ladeuil (vila)]

Crystal clear now !
I'll land.

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

[John A Meinel (jameinel)]

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

I believe you provide this against bzr 2.3 which Martin already
approved. Is that correct? If so, I think it is fine to apply it to
2.2, and merge-forward from there.

I believe the official policy is that we will only focus on
maintaining 1 stable branch, which is now bzr-2.3. However, if you
want it against 2.2, we can certainly do so.

Did we get a patch for this sent to upstream configobj?

 merge: approve

John
=:->

On Sun, Feb 20, 2011 at 7:59 AM, Alexander Belchenko <<email address hidden>
<mailto:<email address hidden>>> wrote:

    Alexander Belchenko has proposed merging
    lp:~bialix/bzr/2.2-configobj into lp:bzr/2.2.

    Requested reviews:
     bzr-core (bzr-core)

    For more details, see:
    https://code.launchpad.net/~bialix/bzr/2.2-configobj/+merge/50507
    <https://code.launchpad.net/%7Ebialix/bzr/2.2-configobj/+merge/50507>

    Fixed bug in the bundled copy of ConfigObj with quoting of triple
    quotes
    in the value string. Fix suggested by ConfigObj's author Michael
    Foord.

    This patch provides the fix to 2.2 series, should I provide the
    similar patches for 2.3 and bzr.dev, or it will be merged to other
    series by some integration process?
    --
    https://code.launchpad.net/~bialix/bzr/2.2-configobj/+merge/50507
    <https://code.launchpad.net/%7Ebialix/bzr/2.2-configobj/+merge/50507>
    Your team bzr-core is requested to review the proposed merge of
    lp:~bialix/bzr/2.2-configobj into lp:bzr/2.2.

    === modified file 'NEWS'
    --- NEWS 2011-02-09 04:16:55 +0000
    +++ NEWS 2011-02-20 13:59:00 +0000
    @@ -36,6 +36,10 @@
     Internals
     *********

    +* Fixed bug in the bundled copy of ConfigObj with quoting of
    triple quotes
    + in the value string. Fix suggested by ConfigObj's author
    Michael Foord.
    + (Alexander Belchenko, #710410)
    +
     Testing
     *******

    === modified file 'bzrlib/tests/test_config.py'
    --- bzrlib/tests/test_config.py 2010-07-21 03:07:02 +0000
    +++ bzrlib/tests/test_config.py 2011-02-20 13:59:00 +0000
    @@ -246,6 +246,27 @@
            co2 = config.ConfigObj(lines)
            self.assertEqual(co2['test'], 'foo#bar')

    + def test_triple_quotes(self):
    + # Bug #710410: if the value string has triple quotes
    + # then ConfigObj versions up to 4.7.2 will quote them wrong
    + # and won't able to read them back
    + triple_quotes_value = '''spam
    +""" that's my spam """
    +eggs'''
    + co = config.ConfigObj()
    + co['test'] = triple_quotes_value
    + # another bug in ConfigObj:
    + # method co.write() without arguments produces list of lines
    + # one option per line, and multiline values are not split
    + # across multiple lines,
    + # and that breaks the parsing these lines back by ConfigObj
    + outfile = StringIO()
    + co.write(outfile=outfile)
    + lines = outfile.getvalue().splitlines()
    + # now we're trying to read it back
    + co2 = config.ConfigObj(lines)
    + self.assertEquals(triple_quotes_value, co2['test'])
    +

     ...

review: Approve

[Alexander Belchenko (bialix)]

John, this patch has been suggested by the ConfigObj author. I think he should fix the bug in the upstream trunk, shouldn't he?

[John A Meinel (jameinel)]

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

On 2/24/2011 12:21 PM, Alexander Belchenko wrote:
> John, this patch has been suggested by the ConfigObj author. I think he should fix the bug in the upstream trunk, shouldn't he?

Sure. I just wanted to make sure that if we are patching ConfigObj that
we are making sure upstream knows about the bug, etc. Ideally we'd be
able to not have any patches vs upstream. Especially since on
debian/Ubuntu we use the system ConfigObj (we don't bundle our own).

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

iEYEARECAAYFAk1mykEACgkQJdeBCYSNAAMjawCg0LjN/JnHaSJkxWM6GPK+tu+A
lSsAoMVPZOWio0WOai5tzCko7qIYs7FA
=cNQu
-----END PGP SIGNATURE-----

=== modified file 'NEWS'
--- NEWS	2011-02-09 04:16:55 +0000
+++ NEWS	2011-02-22 09:22:07 +0000
@@ -36,6 +36,10 @@
 Internals
 *********
 
+* Fixed bug in the bundled copy of ConfigObj with quoting of triple quotes
+  in the value string. Fix suggested by ConfigObj's author Michael Foord.
+  (Alexander Belchenko, #710410)
+
 Testing
 *******
 

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2010-07-21 03:07:02 +0000
+++ bzrlib/tests/test_config.py	2011-02-22 09:22:07 +0000
@@ -241,11 +241,37 @@
         """
         co = config.ConfigObj()
         co['test'] = 'foo#bar'
-        lines = co.write()
+        outfile = StringIO()
+        co.write(outfile=outfile)
+        lines = outfile.getvalue().splitlines()
         self.assertEqual(lines, ['test = "foo#bar"'])
         co2 = config.ConfigObj(lines)
         self.assertEqual(co2['test'], 'foo#bar')
 
+    def test_triple_quotes(self):
+        # Bug #710410: if the value string has triple quotes
+        # then ConfigObj versions up to 4.7.2 will quote them wrong
+        # and won't able to read them back
+        triple_quotes_value = '''spam
+""" that's my spam """
+eggs'''
+        co = config.ConfigObj()
+        co['test'] = triple_quotes_value
+        # While writing this test another bug in ConfigObj has been found:
+        # method co.write() without arguments produces list of lines
+        # one option per line, and multiline values are not split
+        # across multiple lines,
+        # and that breaks the parsing these lines back by ConfigObj.
+        # This issue only affects test, but it's better to avoid
+        # `co.write()` construct at all.
+        # [bialix 20110222] bug report sent to ConfigObj's author
+        outfile = StringIO()
+        co.write(outfile=outfile)
+        output = outfile.getvalue()
+        # now we're trying to read it back
+        co2 = config.ConfigObj(StringIO(output))
+        self.assertEquals(triple_quotes_value, co2['test'])
+
 
 erroneous_config = """[section] # line 1
 good=good # line 2

=== modified file 'bzrlib/util/configobj/configobj.py'
--- bzrlib/util/configobj/configobj.py	2009-04-17 22:24:54 +0000
+++ bzrlib/util/configobj/configobj.py	2011-02-22 09:22:07 +0000
@@ -1794,10 +1794,12 @@
     def _get_triple_quote(self, value):
         if (value.find('"""') != -1) and (value.find("'''") != -1):
             raise ConfigObjError('Value "%s" cannot be safely quoted.' % value)
+        # upstream version (up to version 4.7.2) has the bug with incorrect quoting;
+        # fixed in our copy based on the suggestion of ConfigObj's author
         if value.find('"""') == -1:
+            quot = tsquot
+        else:
             quot = tdquot
-        else:
-            quot = tsquot
         return quot