Code review comment for lp:~jelmer/bzr-pqm/2.5-compat

You beat me to it ;)

8 +

Spurious.

16 + get = _get_user_option
17 +

Cunning !

42 + if bzrlib_version < (2, 5, 0, 'dev', 5):

(2,5,0,'beta',5) < (2, 5, 0, 'dev', 5)
True

Oops !

I think we should go with just (2,5) instead, people encountering issues
with 2.5 compatibility are likely to 1) use at least 2.5b5 or trunk 2) upgrade to at least 2.5b5 anyway.

51 + sections = []
52 + if branch:
53 + bstore = branch._get_config_store()
54 + sections.append(_mod_config.NameMatcher(bstore, None).get_sections)
55 if public_location:
56 - config.add_source(_mod_config.LocationConfig(public_location))
57 - config.add_source(_mod_config.GlobalConfig())
58 + lstore = _mod_config.LocationStore()
59 + sections.append(_mod_config.LocationMatcher(lstore,
60 + public_location).get_sections)
61 + gstore = _mod_config.GlobalStore()
62 + sections.append(_mod_config.NameMatcher(gstore, 'DEFAULT').get_sections)
63 + config = _mod_config.Stack(sections)

This doesn't look correct (locations.conf should override branch,conf when
'branch' is not None and -O also needs to supported), I think we want:

=== modified file 'pqm_submit.py'
--- pqm_submit.py 2012-01-20 00:58:05 +0000
+++ pqm_submit.py 2012-01-20 08:23:21 +0000
@@ -258,17 +258,12 @@
                 config.add_source(_mod_config.LocationConfig(public_location))
             config.add_source(_mod_config.GlobalConfig())
     else:
- sections = []
         if branch:
- bstore = branch._get_config_store()
- sections.append(_mod_config.NameMatcher(bstore, None).get_sections)
- if public_location:
- lstore = _mod_config.LocationStore()
- sections.append(_mod_config.LocationMatcher(lstore,
- public_location).get_sections)
- gstore = _mod_config.GlobalStore()
- sections.append(_mod_config.NameMatcher(gstore, 'DEFAULT').get_sections)
- config = _mod_config.Stack(sections)
+ config = branch.get_config_stack()
+ elif public_location:
+ config = _mod_config.LocationStack(public_location)
+ else:
+ config = _mod_config.GlobalStack()

Which still pass the plugin tests.

I've tested these additional changes and pushed them on lp:~bzr-pqm-devel/bzr-pqm/2.5-compat/

If you agree with them, get them, if you don't tweak them and land the result.