Bazaar PQM Plugin

Merge lp:~jelmer/bzr-pqm/2.5-compat into lp:bzr-pqm

  1. 2.5-compat
  2. Merge into devel
Proposed by Jelmer Vernooij
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: 83
Merged at revision: 83
Proposed branch: lp:~jelmer/bzr-pqm/2.5-compat
Merge into: lp:bzr-pqm
Diff against target: 85 lines (+26/-10)
1 file modified
pqm_submit.py (+26/-10)
To merge this branch: bzr merge lp:~jelmer/bzr-pqm/2.5-compat
Reviewer Review Type Date Requested Status
Vincent Ladeuil Needs Fixing
Review via email: mp+89367@code.launchpad.net

Description of the change

Fix compatibility with bzr 2.5, which has a different configuration API.

This will simplify the code quite a bit once we can drop support for older bzr versions (< 2.3).

To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote :

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.

review: Needs Fixing
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

On 01/20/2012 09:42 AM, Vincent Ladeuil wrote:
> Review: Needs Fixing
>
> 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.
 Fair enough.
>
>
> 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:
 It's consistent with the behaviour in the pre-2.5 version of bzr-pqm. I
don't think we should change that.

Cheers,

Jelmer

Revision history for this message
Vincent Ladeuil (vila) wrote :

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

> It's consistent with the behaviour in the pre-2.5 version of bzr-pqm. I
> don't think we should change that.

Yeah, your proposal is bug-compatible but do we need to file a bug first ?

It's a common misconception that branch.conf overrides locations.conf (or that locations.conf provides default values for branch.conf), I'm pretty sure whoever coded that was misled.

Revision history for this message
Vincent Ladeuil (vila) wrote :

Meh, I've re-read the original code:

   config = StackedConfig()
    if branch:
        config.add_source(branch.get_config())
    else:
        if public_location:
            config.add_source(_mod_config.LocationConfig(public_location))
        config.add_source(_mod_config.GlobalConfig())

So that's indeed what my fix does, either there is a branch and the usual location/branch/global is in effect or there is a location and it's location/global or there is only global.

Revision history for this message
Jelmer Vernooij (jelmer) wrote :

On 01/20/2012 03:17 PM, Vincent Ladeuil wrote:
> Meh, I've re-read the original code:
>
> config = StackedConfig()
> if branch:
> config.add_source(branch.get_config())
> else:
> if public_location:
> config.add_source(_mod_config.LocationConfig(public_location))
> config.add_source(_mod_config.GlobalConfig())
>
> So that's indeed what my fix does, either there is a branch and the usual location/branch/global is in effect or there is a location and it's location/global or there is only global.
 Ah, I see. I'll fix my code and land.

Cheers,

Jelmer

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'pqm_submit.py'
2--- pqm_submit.py 2011-12-16 20:21:06 +0000
3+++ pqm_submit.py 2012-01-20 01:00:32 +0000
4@@ -211,6 +211,7 @@
5 def add_source(self, source):
6 self._sources.append(source)
7
8+
9 def _get_user_option(self, option_name):
10 """See Config._get_user_option."""
11 for source in self._sources:
12@@ -219,6 +220,8 @@
13 return value
14 return None
15
16+ get = _get_user_option
17+
18 def _get_user_id(self):
19 for source in self._sources:
20 value = source._get_user_id()
21@@ -233,11 +236,11 @@
22 :param local_config: Config object for local branch
23 :param submit_location: Location of submit branch
24 """
25- mail_to = local_config.get_user_option('pqm_email')
26+ mail_to = local_config.get('pqm_email')
27 if not mail_to:
28 submit_branch = Branch.open(submit_location)
29 submit_branch_config = submit_branch.get_config()
30- mail_to = submit_branch_config.get_user_option('child_pqm_email')
31+ mail_to = submit_branch_config.get('child_pqm_email')
32 if mail_to is None:
33 return None
34 return mail_to.encode('utf8') # same here
35@@ -246,22 +249,35 @@
36 def submit(branch, message, dry_run=False, public_location=None,
37 submit_location=None, tree=None, ignore_local=False):
38 """Submit the given branch to the pqm."""
39- config = StackedConfig()
40- if branch:
41- config.add_source(branch.get_config())
42+ if bzrlib_version < (2, 5, 0, 'dev', 5):
43+ config = StackedConfig()
44+ if branch:
45+ config.add_source(branch.get_config())
46+ else:
47+ if public_location:
48+ config.add_source(_mod_config.LocationConfig(public_location))
49+ config.add_source(_mod_config.GlobalConfig())
50 else:
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)
64
65 submission = PQMSubmission(
66 source_branch=branch, public_location=public_location, message=message,
67 submit_location=submit_location,
68 tree=tree)
69
70- mail_from = config.get_user_option('pqm_user_email')
71+ mail_from = config.get('pqm_user_email')
72 if not mail_from:
73- mail_from = config.username()
74+ mail_from = config.get('email')
75 mail_from = mail_from.encode('utf8') # Make sure this isn't unicode
76 mail_to = pqm_email(config, submit_location)
77 if not mail_to:
78@@ -273,7 +289,7 @@
79
80 message = submission.to_email(mail_from, mail_to)
81
82- mail_bcc = config.get_user_option('pqm_bcc')
83+ mail_bcc = config.get('pqm_bcc')
84 if mail_bcc is not None:
85 message["Bcc"] = mail_bcc
86

Subscribers

People subscribed via source and target branches

to all changes:
to status/vote changes: