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

[Aaron Bentley]

[Aaron Bentley (abentley)]

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

 review approve

review: Approve

=== modified file 'NEWS'
--- NEWS	2009-05-11 06:44:30 +0000
+++ NEWS	2009-05-11 20:35:15 +0000
@@ -150,6 +150,9 @@
   broken. This is mainly for testing that lock/unlock are
   balanced in tests. (Vincent Ladeuil)
 
+* New MergeDirective hook 'merge_request_body' allows hooks to supply or
+  alter a body for the message produced by ``bzr send``.
+
 * New smart server verb ``BzrDir.initialize_ex`` which implements a
   refactoring to the core of clone allowing less round trips on new
   branches. (Robert Collins)
 
=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2009-05-07 05:08:46 +0000
+++ bzrlib/builtins.py	2009-05-11 20:35:15 +0000
@@ -4866,13 +4866,7 @@
     def _run(self, submit_branch, revision, public_branch, remember, format,
              no_bundle, no_patch, output, from_, mail_to, message, body):
         from bzrlib.revision import NULL_REVISION
-        branch = Branch.open_containing(from_)[0]
-        if output is None:
-            outfile = cStringIO.StringIO()
-        elif output == '-':
-            outfile = self.outf
-        else:
-            outfile = open(output, 'wb')
+        tree, branch = bzrdir.BzrDir.open_containing_tree_or_branch(from_)[:2]
         # we may need to write data into branch's repository to calculate
         # the data to send.
         branch.lock_write()
@@ -4959,21 +4953,20 @@
                     public_branch=public_branch, patch_type=patch_type,
                     message=message)
 
-            outfile.writelines(directive.to_lines())
             if output is None:
-                subject = '[MERGE] '
-                if message is not None:
-                    subject += message
+                directive.compose_merge_request(mail_client, mail_to, body,
+                                                branch, tree)
+            else:
+                if output == '-':
+                    outfile = self.outf
                 else:
-                    revision = branch.repository.get_revision(revision_id)
-                    subject += revision.get_summary()
-                basename = directive.get_disk_name(branch)
-                mail_client.compose_merge_request(mail_to, subject,
-                                                  outfile.getvalue(),
-                                                  basename, body)
+                    outfile = open(output, 'wb')
+                try:
+                    outfile.writelines(directive.to_lines())
+                finally:
+                    if outfile is not self.outf:
+                        outfile.close()
         finally:
-            if output != '-':
-                outfile.close()
             branch.unlock()
 
 
 
=== modified file 'bzrlib/hooks.py'
--- bzrlib/hooks.py	2009-04-29 15:01:40 +0000
+++ bzrlib/hooks.py	2009-05-11 20:35:15 +0000
@@ -56,6 +56,9 @@
 known_hooks.register_lazy(
     ('bzrlib.version_info_formats.format_rio', 'RioVersionInfoBuilder.hooks'),
     'bzrlib.version_info_formats.format_rio', 'RioVersionInfoBuilderHooks')
+known_hooks.register_lazy(
+    ('bzrlib.merge_directive', '_BaseMergeDirective.hooks'),
+    'bzrlib.merge_directive', 'MergeDirectiveHooks')
 
 
 def known_hooks_key_to_object((module_name, member_name)):
@@ -178,8 +181,8 @@
     :ivar introduced: A version tuple specifying what version the hook was
         introduced in. None indicates an unknown version.
     :ivar deprecated: A version tuple specifying what version the hook was
-        deprecated or superceded in. None indicates that the hook is not
-        superceded or deprecated. If the hook is superceded then the doc
+        deprecated or superseded in. None indicates that the hook is not
+        superseded or deprecated. If the hook is superseded then the doc
         should describe the recommended replacement hook to register for.
     :ivar doc: The docs for using the hook.
     """
 
=== modified file 'bzrlib/merge_directive.py'
--- bzrlib/merge_directive.py	2009-05-06 05:36:28 +0000
+++ bzrlib/merge_directive.py	2009-05-11 20:35:15 +0000
@@ -23,11 +23,13 @@
     diff,
     errors,
     gpg,
+    hooks,
     registry,
     revision as _mod_revision,
     rio,
     testament,
     timestamp,
+    trace,
     )
 from bzrlib.bundle import (
     serializer as bundle_serializer,
@@ -35,8 +37,37 @@
 from bzrlib.email_message import EmailMessage
 
 
+class MergeRequestBodyParams(object):
+    """Parameter object for the merge_request_body hook."""
+
+    def __init__(self, body, orig_body, directive, to, basename, subject,
+                 branch, tree=None):
+        self.body = body
+        self.orig_body = orig_body
+        self.directive = directive
+        self.branch = branch
+        self.tree = tree
+        self.to = to
+        self.basename = basename
+        self.subject = subject
+
+
+class MergeDirectiveHooks(hooks.Hooks):
+    """Hooks for MergeDirective classes."""
+
+    def __init__(self):
+        hooks.Hooks.__init__(self)
+        self.create_hook(hooks.HookPoint('merge_request_body',
+            "Called with a MergeRequestBodyParams when a body is needed for"
+            " a merge request.  Callbacks must return a body.  If more"
+            " than one callback is registered, the output of one callback is"
+            " provided to the next.", (1, 15, 0), False))
+
+
 class _BaseMergeDirective(object):
 
+    hooks = MergeDirectiveHooks()
+
     def __init__(self, revision_id, testament_sha1, time, timezone,
                  target_branch, patch=None, source_branch=None, message=None,
                  bundle=None):
@@ -240,6 +271,37 @@
                 target_repo.fetch(source_branch.repository, self.revision_id)
         return self.revision_id
 
+    def compose_merge_request(self, mail_client, to, body, branch, tree=None):
+        """Compose a request to merge this directive.
+
+        :param mail_client: The mail client to use for composing this request.
+        :param to: The address to compose the request to.
+        :param branch: The Branch that was used to produce this directive.
+        :param tree: The Tree (if any) for the Branch used to produce this
+            directive.
+        """
+        basename = self.get_disk_name(branch)
+        subject = '[MERGE] '
+        if self.message is not None:
+            subject += self.message
+        else:
+            revision = branch.repository.get_revision(self.revision_id)
+            subject += revision.get_summary()
+        if getattr(mail_client, 'supports_body', False):
+            orig_body = body
+            for hook in self.hooks['merge_request_body']:
+                params = MergeRequestBodyParams(body, orig_body, self,
+                                                to, basename, subject, branch,
+                                                tree)
+                body = hook(params)
+        elif len(self.hooks['merge_request_body']) > 0:
+            trace.warning('Cannot run merge_request_body hooks because mail'
+                          ' client %s does not support message bodies.',
+                        mail_client.__class__.__name__)
+        mail_client.compose_merge_request(to, subject,
+                                          ''.join(self.to_lines()),
+                                          basename, body)
+
 
 class MergeDirective(_BaseMergeDirective):
 
 
=== modified file 'bzrlib/tests/test_merge_directive.py'
--- bzrlib/tests/test_merge_directive.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/test_merge_directive.py	2009-05-11 20:35:15 +0000
@@ -19,8 +19,10 @@
 from bzrlib import (
     errors,
     gpg,
+    mail_client,
     merge_directive,
     tests,
+    trace,
     )
 
 
@@ -697,3 +699,89 @@
         self.assertEqual('booga', md.patch)
         self.assertEqual('diff', md.patch_type)
         self.assertEqual('Hi mom!', md.message)
+
+
+class TestHook(object):
+    """Hook callback for test purposes."""
+
+    def __init__(self, result=None):
+        self.calls = []
+        self.result = result
+
+    def __call__(self, params):
+        self.calls.append(params)
+        return self.result
+
+
+class HookMailClient(mail_client.MailClient):
+    """Mail client for testing hooks."""
+
+    def __init__(self, config):
+        self.body = None
+        self.config = config
+
+    def compose(self, prompt, to, subject, attachment, mime_subtype,
+                extension, basename=None, body=None):
+        self.body = body
+
+
+class TestBodyHook(tests.TestCaseWithTransport):
+
+    def compose_with_hooks(self, test_hooks, supports_body=True):
+        client = HookMailClient({})
+        client.supports_body = supports_body
+        for test_hook in test_hooks:
+            merge_directive.MergeDirective.hooks.install_named_hook(
+                'merge_request_body', test_hook, 'test')
+        tree = self.make_branch_and_tree('foo')
+        tree.commit('foo')
+        directive = merge_directive.MergeDirective2(
+            tree.branch.last_revision(), 'sha', 0, 0, 'sha',
+            source_branch=tree.branch.base,
+            base_revision_id=tree.branch.last_revision(),
+            message='This code rox')
+        directive.compose_merge_request(client, '[e-mail]',
+            None, tree.branch)
+        return client, directive
+
+    def test_no_supports_body(self):
+        test_hook = TestHook('foo')
+        old_warn = trace.warning
+        warnings = []
+        def warn(*args):
+            warnings.append(args)
+        trace.warning = warn
+        try:
+            client, directive = self.compose_with_hooks([test_hook],
+                supports_body=False)
+        finally:
+            trace.warning = old_warn
+        self.assertEqual(0, len(test_hook.calls))
+        self.assertEqual(('Cannot run merge_request_body hooks because mail'
+                          ' client %s does not support message bodies.',
+                          'HookMailClient'), warnings[0])
+
+    def test_body_hook(self):
+        test_hook = TestHook('foo')
+        client, directive = self.compose_with_hooks([test_hook])
+        self.assertEqual(1, len(test_hook.calls))
+        self.assertEqual('foo', client.body)
+        params = test_hook.calls[0]
+        self.assertIsInstance(params,
+                              merge_directive.MergeRequestBodyParams)
+        self.assertIs(None, params.body)
+        self.assertIs(None, params.orig_body)
+        self.assertEqual('[e-mail]', params.to)
+        self.assertEqual('[MERGE] This code rox', params.subject)
+        self.assertEqual(directive, params.directive)
+        self.assertEqual('foo-1', params.basename)
+
+    def test_body_hook_chaining(self):
+        test_hook1 = TestHook('foo')
+        test_hook2 = TestHook('bar')
+        client = self.compose_with_hooks([test_hook1, test_hook2])[0]
+        self.assertEqual(None, test_hook1.calls[0].body)
+        self.assertEqual(None, test_hook1.calls[0].orig_body)
+        self.assertEqual('foo', test_hook2.calls[0].body)
+        self.assertEqual(None, test_hook2.calls[0].orig_body)
+        self.assertEqual('bar', client.body)