Bug#718536: gbp.conf: only the last of multiple filter options are used

2014-03-09 Thread Guido Günther
clone 718536 -1
retitle -1 gbp.conf support multiple filter options
severity -1 wishlist
thanks

Hi Christian,
On Sat, Mar 08, 2014 at 09:34:24PM +0100, Christian Kastner wrote:
 tag 718536 + confirmed patch
 found 718536 0.6.10
 thanks
 
 The problem is that by default, ConfigParser uses a dict (OrderDict in
 2.7) as a backing store, so specifying an option more than once per
 section results in the values being overwritten instead of combined as
 gbp.conf(5) says.

I do wonder how this ended up there since I'm sure this was never
supported. I've removed the wrong docs for now.

 The attached patch 0001-* extends the config unit tests with such a
 case; you can observe that the test fails.
 
 Patch 0002-* implements a custom dict that overrides __setitem__ and
 squashes the value of reapeated option occurrences into one value, as if
 it had been specified as a Python list following gbp.conf(5). This has
 to be enabled to be effective, and currently it is only enable for the
 'filter' option.
 
 The simpler alternative of course would be what Diane proposed: keep the
 code as-is, and just fix the man page. I believe this would also be
 cleaner code-wise. However, the ability of multiple filter options can
 make gbp.conf more readable, especially when you need to filter quite a
 few files for DFSG purposes.

Agreed. We're currently about to rework the config parser stuff (Markus
already sent patches for this) so we should revisit that at a later
point so I'll turn this into a wishlist bug to extend the gbp.conf
parser. Thanks a lot for the patch!
Cheers,
  -- Guido


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org



Bug#718536: gbp.conf: only the last of multiple filter options are used

2014-03-08 Thread Christian Kastner
tag 718536 + confirmed patch
found 718536 0.6.10
thanks

The problem is that by default, ConfigParser uses a dict (OrderDict in
2.7) as a backing store, so specifying an option more than once per
section results in the values being overwritten instead of combined as
gbp.conf(5) says.

The attached patch 0001-* extends the config unit tests with such a
case; you can observe that the test fails.

Patch 0002-* implements a custom dict that overrides __setitem__ and
squashes the value of reapeated option occurrences into one value, as if
it had been specified as a Python list following gbp.conf(5). This has
to be enabled to be effective, and currently it is only enable for the
'filter' option.

The simpler alternative of course would be what Diane proposed: keep the
code as-is, and just fix the man page. I believe this would also be
cleaner code-wise. However, the ability of multiple filter options can
make gbp.conf more readable, especially when you need to filter quite a
few files for DFSG purposes.

Christian



From c28d83519a07d29ece9ae4cd668b8790c33beb88 Mon Sep 17 00:00:00 2001
From: Christian Kastner deb...@kvr.at
Date: Fri, 7 Mar 2014 22:43:13 +0100
Subject: [PATCH 1/2] gbp.conf: Extend test_Config with repeated options
 testcase

---
 tests/test_Config.py | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/tests/test_Config.py b/tests/test_Config.py
index 2bae765..2930373 100644
--- a/tests/test_Config.py
+++ b/tests/test_Config.py
@@ -97,6 +97,13 @@ def test_filter():
  parser.config['filter']
 ['asdf']
  f = open(confname, 'w')
+ f.write('[bar]\\nfilter = first filter\\n')
+ f.write('filter = second filter\\n')
+ f.close()
+ parser._parse_config_files()
+ parser.config['filter']
+['first filter', 'second filter']
+ f = open(confname, 'w')
  f.write([bar]\\nfilter = ['this', 'is', 'a', 'list']\\n)
  f.close()
  parser._parse_config_files()
-- 
1.9.0

From 7c2345cc943a970dd7043c099ad3f4db4f893a81 Mon Sep 17 00:00:00 2001
From: Christian Kastner deb...@kvr.at
Date: Sat, 8 Mar 2014 14:32:43 +0100
Subject: [PATCH 2/2] gbp.conf: Use custom dict GbpOptionSquashingDict in
 SafeConfigParser

Repeatedly setting a key will not overwrite the previous value but combine both
old and new values to a Python list (if necessary by appending a value to an
existing list, or by concatenating two existing lists). This is necessary for
the following stanza from gbp.conf(5):

[import-orig]
filter = .svn
filter = .hg

To avoid regressions, the options for which this is enabled have to be
specified explicitly.

Closes: #718536
---
 gbp/config.py | 67 +--
 1 file changed, 65 insertions(+), 2 deletions(-)

diff --git a/gbp/config.py b/gbp/config.py
index fa6e679..b25339e 100644
--- a/gbp/config.py
+++ b/gbp/config.py
@@ -51,6 +51,69 @@ class GbpOption(Option):
 TYPE_CHECKER['path'] = expand_path
 TYPE_CHECKER['tristate'] = check_tristate
 
+class GbpOptionSquashingDict(dict):
+Dictionary squashing repeated options into a single one
+
+This dictionary acts exactly like a normal dictionary with one exception:
+when setting a value, if the key already exists in the dictionary, instead
+of overwriting the old value, the old and new values are combined to a
+Python list of strings.
+
+This is useful in ConfigParsers when an option might appear multiple times
+within the same section. See the import-orig example of gbp.conf(5).
+
+Let d['foo'] = 'bar' and then d['foo'] = 'baz'.
+Let e['xx'] = 'mm' and e['xx'] = ['nn', 'oo'].
+
+With regular dicts, d['foo'] == 'baz' and e['xxx'] = ['nn', 'oo'].
+With this dict, d['foo'] == ['bar', 'baz'] and e['xx'] = ['mm', 'nn', 'oo'].
+
+To protect against possible regressions, the keys for which squashing should
+be enabled have to be added to the _squashkeys list.
+
+def __init__(self, *args):
+# We have to use dict because OrderedDict (the default backing store in
+# 2.7) has its own internal logic that conflicts with this approach
+dict.__init__(self, args)
+self._squashkeys = ['filter']
+
+def __setitem__(self, key, val):
+# NOTE: ConfigParser appears to use this dict in strange way
+# internally. Initially, values are passed as 1-element lists of
+# strings (with one call per option); later on, the plain strings
+# are used, but only with the option value last seen.
+
+if (isinstance(val, str)):
+# In pass 2, we just do the default op
+dict.__setitem__(self, key, val)
+return
+
+# We're in Pass 1: perform hackery
+if key in self and key in self._squashkeys:
+ curr = self[key][0]
+ vall = val[0]
+
+ # First, generate the string that will be appended. We know that it
+ # must result in a Python list, but it may not yet be one
+