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

Proposed by Alexander Belchenko on 2011-02-20
Status: Merged
Approved by: John A Meinel on 2011-02-24
Approved revision: 5130
Merge reported by: Alexander Belchenko
Merged at revision: not available
Proposed branch: lp:~bialix/bzr/2.2-configobj
Merge into: lp:bzr/2.2
Diff against target: 74 lines (+35/-3)
3 files modified
NEWS (+4/-0)
bzrlib/tests/test_config.py (+27/-1)
bzrlib/util/configobj/configobj.py (+4/-2)
To merge this branch: bzr merge lp:~bialix/bzr/2.2-configobj
Reviewer Review Type Date Requested Status
John A Meinel Approve on 2011-02-24
Vincent Ladeuil Approve on 2011-02-22
Martin Pool 2011-02-20 Approve on 2011-02-21
Review via email: mp+50507@code.launchpad.net

Commit message

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

Description of the change

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?

To post a comment you must log in.
Martin Pool (mbp) wrote :

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) wrote :

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) wrote :

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.

lp:~bialix/bzr/2.2-configobj updated on 2011-02-22
5129. By Alexander Belchenko on 2011-02-22

comment about the change in the bundled copy of configobj

5130. By Alexander Belchenko on 2011-02-22

don't use lines in the tests, and better comment about the corresponding bug in configobj; avoid using write() method without outfile parameter.

Vincent Ladeuil (vila) wrote :

Crystal clear now !
I'll land.

review: Approve
Vincent Ladeuil (vila) wrote :

sent to pqm by email

John A Meinel (jameinel) wrote :
Download full text (4.0 KiB)

-----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'])
    +

     ...

Read more...

review: Approve
Alexander Belchenko (bialix) 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?

John A Meinel (jameinel) wrote :

-----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-----

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2011-02-09 04:16:55 +0000
3+++ NEWS 2011-02-22 09:22:07 +0000
4@@ -36,6 +36,10 @@
5 Internals
6 *********
7
8+* Fixed bug in the bundled copy of ConfigObj with quoting of triple quotes
9+ in the value string. Fix suggested by ConfigObj's author Michael Foord.
10+ (Alexander Belchenko, #710410)
11+
12 Testing
13 *******
14
15
16=== modified file 'bzrlib/tests/test_config.py'
17--- bzrlib/tests/test_config.py 2010-07-21 03:07:02 +0000
18+++ bzrlib/tests/test_config.py 2011-02-22 09:22:07 +0000
19@@ -241,11 +241,37 @@
20 """
21 co = config.ConfigObj()
22 co['test'] = 'foo#bar'
23- lines = co.write()
24+ outfile = StringIO()
25+ co.write(outfile=outfile)
26+ lines = outfile.getvalue().splitlines()
27 self.assertEqual(lines, ['test = "foo#bar"'])
28 co2 = config.ConfigObj(lines)
29 self.assertEqual(co2['test'], 'foo#bar')
30
31+ def test_triple_quotes(self):
32+ # Bug #710410: if the value string has triple quotes
33+ # then ConfigObj versions up to 4.7.2 will quote them wrong
34+ # and won't able to read them back
35+ triple_quotes_value = '''spam
36+""" that's my spam """
37+eggs'''
38+ co = config.ConfigObj()
39+ co['test'] = triple_quotes_value
40+ # While writing this test another bug in ConfigObj has been found:
41+ # method co.write() without arguments produces list of lines
42+ # one option per line, and multiline values are not split
43+ # across multiple lines,
44+ # and that breaks the parsing these lines back by ConfigObj.
45+ # This issue only affects test, but it's better to avoid
46+ # `co.write()` construct at all.
47+ # [bialix 20110222] bug report sent to ConfigObj's author
48+ outfile = StringIO()
49+ co.write(outfile=outfile)
50+ output = outfile.getvalue()
51+ # now we're trying to read it back
52+ co2 = config.ConfigObj(StringIO(output))
53+ self.assertEquals(triple_quotes_value, co2['test'])
54+
55
56 erroneous_config = """[section] # line 1
57 good=good # line 2
58
59=== modified file 'bzrlib/util/configobj/configobj.py'
60--- bzrlib/util/configobj/configobj.py 2009-04-17 22:24:54 +0000
61+++ bzrlib/util/configobj/configobj.py 2011-02-22 09:22:07 +0000
62@@ -1794,10 +1794,12 @@
63 def _get_triple_quote(self, value):
64 if (value.find('"""') != -1) and (value.find("'''") != -1):
65 raise ConfigObjError('Value "%s" cannot be safely quoted.' % value)
66+ # upstream version (up to version 4.7.2) has the bug with incorrect quoting;
67+ # fixed in our copy based on the suggestion of ConfigObj's author
68 if value.find('"""') == -1:
69+ quot = tsquot
70+ else:
71 quot = tdquot
72- else:
73- quot = tsquot
74 return quot
75
76

Subscribers

People subscribed via source and target branches