Merge lp:~abentley/bzr/send-hookage into lp:~bzr/bzr/trunk-old

Proposed by Aaron Bentley on 2009-05-11
Status: Merged
Merged at revision: not available
Proposed branch: lp:~abentley/bzr/send-hookage
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 289 lines
To merge this branch: bzr merge lp:~abentley/bzr/send-hookage
Reviewer Review Type Date Requested Status
Robert Collins (community) 2009-05-11 Approve on 2009-05-11
Review via email: mp+6436@code.launchpad.net
To post a comment you must log in.
Aaron Bentley (abentley) wrote :

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

Hi all,

This patch adds a new MergeDirective hook to calculate the message body.
 This helps projects with a specific review template, like Launchpad, to
ensure the template is considered for all reviews.

It does this by moving the mail_client-specific functionality of
cmd_send into the new MergeDirective.compose_merge_request method.
There is a similarly-named method on MailClient, but this receives a
MergeDirective that has already been serialized, and for the hook, live
objects are more useful.

As well as the Branch and directive, this method also takes an optional
Tree parameter. This will typically be the WorkingTree that the user
ran 'bzr send' against. This may be useful for hooks that want to
examine the working tree to generate the body. Launchpad, for example,
runs 'lint' against files which have changed.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkoIh8cACgkQ0F+nu1YWqI1KPACeNrcy/bMN+CqdgEjOnA5rNJKl
eGQAn1hIvbNAj7PUUgZRGqMM8NJlMPB0
=biQr
-----END PGP SIGNATURE-----

Robert Collins (lifeless) wrote :

 review approve

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-05-11 06:44:30 +0000
3+++ NEWS 2009-05-11 20:35:15 +0000
4@@ -150,6 +150,9 @@
5 broken. This is mainly for testing that lock/unlock are
6 balanced in tests. (Vincent Ladeuil)
7
8+* New MergeDirective hook 'merge_request_body' allows hooks to supply or
9+ alter a body for the message produced by ``bzr send``.
10+
11 * New smart server verb ``BzrDir.initialize_ex`` which implements a
12 refactoring to the core of clone allowing less round trips on new
13 branches. (Robert Collins)
14
15=== modified file 'bzrlib/builtins.py'
16--- bzrlib/builtins.py 2009-05-07 05:08:46 +0000
17+++ bzrlib/builtins.py 2009-05-11 20:35:15 +0000
18@@ -4866,13 +4866,7 @@
19 def _run(self, submit_branch, revision, public_branch, remember, format,
20 no_bundle, no_patch, output, from_, mail_to, message, body):
21 from bzrlib.revision import NULL_REVISION
22- branch = Branch.open_containing(from_)[0]
23- if output is None:
24- outfile = cStringIO.StringIO()
25- elif output == '-':
26- outfile = self.outf
27- else:
28- outfile = open(output, 'wb')
29+ tree, branch = bzrdir.BzrDir.open_containing_tree_or_branch(from_)[:2]
30 # we may need to write data into branch's repository to calculate
31 # the data to send.
32 branch.lock_write()
33@@ -4959,21 +4953,20 @@
34 public_branch=public_branch, patch_type=patch_type,
35 message=message)
36
37- outfile.writelines(directive.to_lines())
38 if output is None:
39- subject = '[MERGE] '
40- if message is not None:
41- subject += message
42+ directive.compose_merge_request(mail_client, mail_to, body,
43+ branch, tree)
44+ else:
45+ if output == '-':
46+ outfile = self.outf
47 else:
48- revision = branch.repository.get_revision(revision_id)
49- subject += revision.get_summary()
50- basename = directive.get_disk_name(branch)
51- mail_client.compose_merge_request(mail_to, subject,
52- outfile.getvalue(),
53- basename, body)
54+ outfile = open(output, 'wb')
55+ try:
56+ outfile.writelines(directive.to_lines())
57+ finally:
58+ if outfile is not self.outf:
59+ outfile.close()
60 finally:
61- if output != '-':
62- outfile.close()
63 branch.unlock()
64
65
66
67=== modified file 'bzrlib/hooks.py'
68--- bzrlib/hooks.py 2009-04-29 15:01:40 +0000
69+++ bzrlib/hooks.py 2009-05-11 20:35:15 +0000
70@@ -56,6 +56,9 @@
71 known_hooks.register_lazy(
72 ('bzrlib.version_info_formats.format_rio', 'RioVersionInfoBuilder.hooks'),
73 'bzrlib.version_info_formats.format_rio', 'RioVersionInfoBuilderHooks')
74+known_hooks.register_lazy(
75+ ('bzrlib.merge_directive', '_BaseMergeDirective.hooks'),
76+ 'bzrlib.merge_directive', 'MergeDirectiveHooks')
77
78
79 def known_hooks_key_to_object((module_name, member_name)):
80@@ -178,8 +181,8 @@
81 :ivar introduced: A version tuple specifying what version the hook was
82 introduced in. None indicates an unknown version.
83 :ivar deprecated: A version tuple specifying what version the hook was
84- deprecated or superceded in. None indicates that the hook is not
85- superceded or deprecated. If the hook is superceded then the doc
86+ deprecated or superseded in. None indicates that the hook is not
87+ superseded or deprecated. If the hook is superseded then the doc
88 should describe the recommended replacement hook to register for.
89 :ivar doc: The docs for using the hook.
90 """
91
92=== modified file 'bzrlib/merge_directive.py'
93--- bzrlib/merge_directive.py 2009-05-06 05:36:28 +0000
94+++ bzrlib/merge_directive.py 2009-05-11 20:35:15 +0000
95@@ -23,11 +23,13 @@
96 diff,
97 errors,
98 gpg,
99+ hooks,
100 registry,
101 revision as _mod_revision,
102 rio,
103 testament,
104 timestamp,
105+ trace,
106 )
107 from bzrlib.bundle import (
108 serializer as bundle_serializer,
109@@ -35,8 +37,37 @@
110 from bzrlib.email_message import EmailMessage
111
112
113+class MergeRequestBodyParams(object):
114+ """Parameter object for the merge_request_body hook."""
115+
116+ def __init__(self, body, orig_body, directive, to, basename, subject,
117+ branch, tree=None):
118+ self.body = body
119+ self.orig_body = orig_body
120+ self.directive = directive
121+ self.branch = branch
122+ self.tree = tree
123+ self.to = to
124+ self.basename = basename
125+ self.subject = subject
126+
127+
128+class MergeDirectiveHooks(hooks.Hooks):
129+ """Hooks for MergeDirective classes."""
130+
131+ def __init__(self):
132+ hooks.Hooks.__init__(self)
133+ self.create_hook(hooks.HookPoint('merge_request_body',
134+ "Called with a MergeRequestBodyParams when a body is needed for"
135+ " a merge request. Callbacks must return a body. If more"
136+ " than one callback is registered, the output of one callback is"
137+ " provided to the next.", (1, 15, 0), False))
138+
139+
140 class _BaseMergeDirective(object):
141
142+ hooks = MergeDirectiveHooks()
143+
144 def __init__(self, revision_id, testament_sha1, time, timezone,
145 target_branch, patch=None, source_branch=None, message=None,
146 bundle=None):
147@@ -240,6 +271,37 @@
148 target_repo.fetch(source_branch.repository, self.revision_id)
149 return self.revision_id
150
151+ def compose_merge_request(self, mail_client, to, body, branch, tree=None):
152+ """Compose a request to merge this directive.
153+
154+ :param mail_client: The mail client to use for composing this request.
155+ :param to: The address to compose the request to.
156+ :param branch: The Branch that was used to produce this directive.
157+ :param tree: The Tree (if any) for the Branch used to produce this
158+ directive.
159+ """
160+ basename = self.get_disk_name(branch)
161+ subject = '[MERGE] '
162+ if self.message is not None:
163+ subject += self.message
164+ else:
165+ revision = branch.repository.get_revision(self.revision_id)
166+ subject += revision.get_summary()
167+ if getattr(mail_client, 'supports_body', False):
168+ orig_body = body
169+ for hook in self.hooks['merge_request_body']:
170+ params = MergeRequestBodyParams(body, orig_body, self,
171+ to, basename, subject, branch,
172+ tree)
173+ body = hook(params)
174+ elif len(self.hooks['merge_request_body']) > 0:
175+ trace.warning('Cannot run merge_request_body hooks because mail'
176+ ' client %s does not support message bodies.',
177+ mail_client.__class__.__name__)
178+ mail_client.compose_merge_request(to, subject,
179+ ''.join(self.to_lines()),
180+ basename, body)
181+
182
183 class MergeDirective(_BaseMergeDirective):
184
185
186=== modified file 'bzrlib/tests/test_merge_directive.py'
187--- bzrlib/tests/test_merge_directive.py 2009-03-23 14:59:43 +0000
188+++ bzrlib/tests/test_merge_directive.py 2009-05-11 20:35:15 +0000
189@@ -19,8 +19,10 @@
190 from bzrlib import (
191 errors,
192 gpg,
193+ mail_client,
194 merge_directive,
195 tests,
196+ trace,
197 )
198
199
200@@ -697,3 +699,89 @@
201 self.assertEqual('booga', md.patch)
202 self.assertEqual('diff', md.patch_type)
203 self.assertEqual('Hi mom!', md.message)
204+
205+
206+class TestHook(object):
207+ """Hook callback for test purposes."""
208+
209+ def __init__(self, result=None):
210+ self.calls = []
211+ self.result = result
212+
213+ def __call__(self, params):
214+ self.calls.append(params)
215+ return self.result
216+
217+
218+class HookMailClient(mail_client.MailClient):
219+ """Mail client for testing hooks."""
220+
221+ def __init__(self, config):
222+ self.body = None
223+ self.config = config
224+
225+ def compose(self, prompt, to, subject, attachment, mime_subtype,
226+ extension, basename=None, body=None):
227+ self.body = body
228+
229+
230+class TestBodyHook(tests.TestCaseWithTransport):
231+
232+ def compose_with_hooks(self, test_hooks, supports_body=True):
233+ client = HookMailClient({})
234+ client.supports_body = supports_body
235+ for test_hook in test_hooks:
236+ merge_directive.MergeDirective.hooks.install_named_hook(
237+ 'merge_request_body', test_hook, 'test')
238+ tree = self.make_branch_and_tree('foo')
239+ tree.commit('foo')
240+ directive = merge_directive.MergeDirective2(
241+ tree.branch.last_revision(), 'sha', 0, 0, 'sha',
242+ source_branch=tree.branch.base,
243+ base_revision_id=tree.branch.last_revision(),
244+ message='This code rox')
245+ directive.compose_merge_request(client, 'jrandom@example.com',
246+ None, tree.branch)
247+ return client, directive
248+
249+ def test_no_supports_body(self):
250+ test_hook = TestHook('foo')
251+ old_warn = trace.warning
252+ warnings = []
253+ def warn(*args):
254+ warnings.append(args)
255+ trace.warning = warn
256+ try:
257+ client, directive = self.compose_with_hooks([test_hook],
258+ supports_body=False)
259+ finally:
260+ trace.warning = old_warn
261+ self.assertEqual(0, len(test_hook.calls))
262+ self.assertEqual(('Cannot run merge_request_body hooks because mail'
263+ ' client %s does not support message bodies.',
264+ 'HookMailClient'), warnings[0])
265+
266+ def test_body_hook(self):
267+ test_hook = TestHook('foo')
268+ client, directive = self.compose_with_hooks([test_hook])
269+ self.assertEqual(1, len(test_hook.calls))
270+ self.assertEqual('foo', client.body)
271+ params = test_hook.calls[0]
272+ self.assertIsInstance(params,
273+ merge_directive.MergeRequestBodyParams)
274+ self.assertIs(None, params.body)
275+ self.assertIs(None, params.orig_body)
276+ self.assertEqual('jrandom@example.com', params.to)
277+ self.assertEqual('[MERGE] This code rox', params.subject)
278+ self.assertEqual(directive, params.directive)
279+ self.assertEqual('foo-1', params.basename)
280+
281+ def test_body_hook_chaining(self):
282+ test_hook1 = TestHook('foo')
283+ test_hook2 = TestHook('bar')
284+ client = self.compose_with_hooks([test_hook1, test_hook2])[0]
285+ self.assertEqual(None, test_hook1.calls[0].body)
286+ self.assertEqual(None, test_hook1.calls[0].orig_body)
287+ self.assertEqual('foo', test_hook2.calls[0].body)
288+ self.assertEqual(None, test_hook2.calls[0].orig_body)
289+ self.assertEqual('bar', client.body)