Merge lp:~jr/bzr/707274-commit-message-hook into lp:bzr

[Jonathan Riddell]

New hook set_commit_message in bzrlib.msgeditor to set a commit message and revision properties. (Jonathan Riddell, #274578)

Proposed new hook set_commit_message() to set a commit message

For bug 707274

called as part of the get_message() callback which is too late to set properties directory on the Commit object, although it can be done through editing commit.builder._revprops

No option to confirm commit message yet
Needs bzr-builddeb updated to use it
Needs qbzr updated to use it

[Jonathan Riddell (jr)]

Submitted bug https://bugs.launchpad.net/qbzr/+bug/788216 on qbzr to use the commit message hooks

[Jonathan Riddell (jr)]

It needs to be called as part of get_message(), any earlier and there is no Commit object to get interesting data out of (such as the changes to debian/changelog that bzr-buiddeb needs)

[Vincent Ladeuil (vila)]

@spiv: I seem to remember you had a deep discussion in this area but I didn't participate nor remember the details. I have a vague feeling that this proposal is not doing the Right Thing but I can't put my finger on it. Care to review it ?

@Jonathan: I really hate spreading FUD on proposals, so wait for spiv input before worrying about me ;)

review: Needs Information

[Martin Packman (gz)]

You're probably thinking of these two threads from Gary and Alexander trying to rationalise the way qcommit works:

<https://lists.ubuntu.com/archives/bazaar/2009q3/062169.html>
<https://lists.ubuntu.com/archives/bazaar/2010q2/068315.html>

[John A Meinel (jameinel)]

There is a small issue if 2 plugins both provide "set_commit_message". Since neither one can see that another plugin has set it, and the ordering of hooks is intentionally undefined.

So I think we at least want the hook to take 2 parameters. The Commit object, and the current commit message.

We might want to future proof this by having the took take a Params object. But ATM I feel that it YAGNI.

Otherwise this looks fine, so "tweak".

review: Needs Fixing

[Jonathan Riddell (jr)]

updated

[Martin Packman (gz)]

Would it be helpful to get Alexander or Gary to look at this and see if it helps the qbzr use case?

[Jelmer Vernooij (jelmer)]

If the intend of this is to allow e.g. plugins to modify the revision properties, I think the _revprops attribute on CommitBuilder should explicitly be made part of the interface.

E.g. bzr-svn, bzr-git and bzr-hg which have custom CommitBuilders currently get the revprops passed in from Repository.get_commit_builder() and do not necessarily set them as a private CommitBuilder._revprops variable and/or assume nobody else touches them.

[Jelmer Vernooij (jelmer)]

I'm surprised by how small this is, well done.

A minor nitpick: You need an extra empty line after set_commit_message.

Approving, though it would indeed be useful if one of the qbzr folks could also have a look.

review: Approve (code)

[John A Meinel (jameinel)]

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

On 6/4/2011 4:34 PM, Jelmer Vernooij wrote:
> Review: Approve code
> I'm surprised by how small this is, well done.
>
> A minor nitpick: You need an extra empty line after set_commit_message.
>
> Approving, though it would indeed be useful if one of the qbzr folks could also have a look.

In talking with Jonathan about this (voice-chat no less), it doesn't fit
the qbzr use case, but they really need something custom anyway.
Specifically, they already select files before committing (so they run
'status' and then let the user use radio boxes, etc.)

They want to get the templated commit message before actually running
commit (which the current api, nor the new api support). And then they
can set the commit message with just the equivalent of '-m' or '-F'. So
they don't need a hook like this (bzr-builddeb does). What they need is
a hook that lets template messages be generated without actually having
processed a commit at all.

Note that there are lots of fuzzy bits to that. Because if you haven't
actually processed a commit, what if the code actually wants to insert
the names of the new files, or a per-file comment, or ... We can evolve
to that. Eg as a first pass, having a templated commit message that does
*not* take the list of files, or having it take the list, but qbzr needs
to re-query whenever a user changes the selection list.

All that to say, this handles what bzr-builddeb needs, and qbzr needs to
design something that is going to work for them, because it is very
different from the existing stuff. So we shouldn't block on this.

My only concern is if we would rather just have a flag on the existing
hook, that says "if the user uses this template message without changing
it, just commit the text, don't prompt them if they really want it.".
(Extend the existing hook, rather than creating a new one.)

However, I don't think the current hook is easy to extend, so I'm good
with this.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3s4wUACgkQJdeBCYSNAANd7QCg1Zjob46KFud1AJRxKDqmparL
ONUAniPGsNswxVwtUsAt0l6xAPCnk+jt
=Rx7Z
-----END PGP SIGNATURE-----

[John A Meinel (jameinel)]

Looking at it with fresh eyes, the only thing I see missing is a test that "bzr commit" will use the set_commit_message hook. You test that msgeditor.... works, but not that the Commit code uses it.

So small tweak and I think it is ready to land.

[Jonathan Riddell (jr)]

sent to pqm by email

[Jonathan Riddell (jr)]

sent to pqm by email

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2011-05-31 18:30:32 +0000
+++ bzrlib/builtins.py	2011-06-09 12:02:30 +0000
@@ -3193,7 +3193,8 @@
         from bzrlib.msgeditor import (
             edit_commit_message_encoded,
             generate_commit_message_template,
-            make_commit_message_template_encoded
+            make_commit_message_template_encoded,
+            set_commit_message,
         )
 
         commit_stamp = offset = None
@@ -3265,9 +3266,11 @@
                 # make_commit_message_template_encoded returns user encoding.
                 # We probably want to be using edit_commit_message instead to
                 # avoid this.
-                start_message = generate_commit_message_template(commit_obj)
-                my_message = edit_commit_message_encoded(text,
-                    start_message=start_message)
+                my_message = set_commit_message(commit_obj)
+                if my_message is None:
+                    start_message = generate_commit_message_template(commit_obj)
+                    my_message = edit_commit_message_encoded(text,
+                        start_message=start_message)
                 if my_message is None:
                     raise errors.BzrCommandError("please specify a commit"
                         " message with either --message or --file")
 
=== modified file 'bzrlib/msgeditor.py'
--- bzrlib/msgeditor.py	2011-05-31 21:08:52 +0000
+++ bzrlib/msgeditor.py	2011-06-09 12:02:30 +0000
@@ -303,6 +303,13 @@
         These are all empty initially.
         """
         Hooks.__init__(self, "bzrlib.msgeditor", "hooks")
+        self.add_hook('set_commit_message',
+            "Set a fixed commit message. "
+            "set_commit_message is called with the "
+            "bzrlib.commit.Commit object (so you can also change e.g. revision "
+            "properties by editing commit.builder._revprops) and the message "
+            "so far. set_commit_message must return the message to use or None"
+            " if it should use the message editor as normal.", (2, 4))
         self.add_hook('commit_message_template',
             "Called when a commit message is being generated. "
             "commit_message_template is called with the bzrlib.commit.Commit "
@@ -317,6 +324,17 @@
 hooks = MessageEditorHooks()
 
 
+def set_commit_message(commit, start_message=None):
+    """Sets the commit message.
+    :param commit: Commit object for the active commit.
+    :return: The commit message or None to continue using the message editor
+    """
+    start_message = None
+    for hook in hooks['set_commit_message']:
+        start_message = hook(commit, start_message)
+    return start_message
+
+
 def generate_commit_message_template(commit, start_message=None):
     """Generate a commit message template.
 
 
=== modified file 'bzrlib/tests/blackbox/test_commit.py'
--- bzrlib/tests/blackbox/test_commit.py	2011-05-17 14:27:30 +0000
+++ bzrlib/tests/blackbox/test_commit.py	2011-06-09 12:02:30 +0000
@@ -754,6 +754,16 @@
             "commit tree/hello.txt", stdin="n\n")
         self.assertEqual(expected, tree.last_revision())
 
+    def test_set_commit_message(self):
+        msgeditor.hooks.install_named_hook("set_commit_message",
+                lambda commit_obj, msg: "save me some typing\n", None)
+        tree = self.make_branch_and_tree('tree')
+        self.build_tree(['tree/hello.txt'])
+        tree.add('hello.txt')
+        out, err = self.run_bzr("commit tree/hello.txt")
+        last_rev = tree.branch.repository.get_revision(tree.last_revision())
+        self.assertEqual('save me some typing\n', last_rev.message)
+
     def test_commit_without_username(self):
         """Ensure commit error if username is not set.
         """
@@ -782,4 +792,3 @@
         self.assertEqual(out, '')
         self.assertContainsRe(err,
             'Branch.*test_checkout.*appears to be bound to itself')
-
 
=== modified file 'bzrlib/tests/test_msgeditor.py'
--- bzrlib/tests/test_msgeditor.py	2011-05-11 11:35:28 +0000
+++ bzrlib/tests/test_msgeditor.py	2011-06-09 12:02:30 +0000
@@ -344,6 +344,18 @@
         self.assertRaises(errors.BadCommitMessageEncoding,
                           msgeditor.edit_commit_message, '')
 
+    def test_set_commit_message_no_hooks(self):
+        commit_obj = commit.Commit()
+        self.assertIs(None,
+            msgeditor.set_commit_message(commit_obj))
+
+    def test_set_commit_message_hook(self):
+        msgeditor.hooks.install_named_hook("set_commit_message",
+                lambda commit_obj, existing_message: "save me some typing\n", None)
+        commit_obj = commit.Commit()
+        self.assertEquals("save me some typing\n",
+            msgeditor.set_commit_message(commit_obj))
+
     def test_generate_commit_message_template_no_hooks(self):
         commit_obj = commit.Commit()
         self.assertIs(None,
 
=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-06-08 13:51:30 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-06-09 12:02:30 +0000
@@ -24,6 +24,10 @@
   exception caused while running bzr serve.  (Jonathan Riddell,
   #274578)
 
+* New hook set_commit_message in bzrlib.msgeditor to set
+  a commit message and revision properties.  (Jonathan Riddell,
+  #274578)
+
 * Support ``-S`` as an alias for ``--short`` for the ``log`` and
   ``missing`` commands. (Martin von Gagern, #38655)