Merge lp:~torkvemada/bzr/commit_hooks into lp:bzr

[Vsevolod Velichko]

New class of hooks: commit ones; and the corresponding Commit.start_commit hook to enable commit metadata modification.

Rationale: there's a set of the metadata properties that are stored along with the commit itself, but at the moment there is no way to fetch or set any of them.
I suppose that since the interaction with all the metadata strictly belongs to commit itself, it's reasonable to create the hook, allowing plugin to modify the metadata for the Commit class.

As a proof of concept I have implemented the 'cimage' plugin[1], which utilizes the start_hook invented in this diff to add the user webcam photo to commit metadata. Later the photo can be fetched from metadata by appropriate command. Practically, all the cimage branch commits are equipped with the stored photos so one can easily install plugin, checkout that branch and view them.

[1] http://bazaar.launchpad.net/~torkvemada/+junk/bzr-cimage/view/head:/__init__.py

[lp:~torkvemada/bzr/commit_hooks]

6540. By Vsevolod Velichko on 2012-10-02

Test for commit.start_commit hook.

6541. By Vsevolod Velichko on 2012-10-02

Merge with trunk

6542. By Vsevolod Velichko on 2012-10-02

Remove unnecessary import in hook test

[Martin Packman (gz)]

Thanks for making these changes and explaining the merge proposal a little more. I've asked Jelmer to have a look.

review: Abstain

[Jelmer Vernooij (jelmer)]

This doesn't look unreasonable.

I'm wary about passing an instance of the Commit object to a hook; Commit at the moment is fairly private, exposing it this way makes it harder to change things there in the future.

[Vsevolod Velichko (torkvemada)]

Jelmer, thanks for your look.
I thought about the idea passing the properties dict itself to the hook, but finally I supposed that one may wish to access commit timestamp or something like that.
Obviously, if you are sure, that one should never wish things like that, I can modify the hook and pass the properties dict :)
Or may be there should be some special struct having only "safe" fields?

[Martin Packman (gz)]

In the past, hooks have been given custom objects with just a selection of fields, see for instance bzrlib.status.StatusHookParams - doing the same thing here would make sense, though there are a lot of parameters passed to Commit.commit and we probably don't want to expose them all. Perhaps just revprops, message (though that's complicated, there are already two commit message hooks fighting over it), committer, and maybe the two time ones.

I'd be careful about what the hook is allowed to modify, particularly a test that sets an invalid revision property (see bzrlib.revision.Revision._check_properties) should raise a ValueError rather than breaking the commit in another manner.

[lp:~torkvemada/bzr/commit_hooks]

6543. By Vsevolod Velichko on 2012-10-12

Do not pass all the commit data to the hook, but only some restricted set of properties.

6544. By Vsevolod Velichko on 2012-10-12

Some test cases on which Commit.start_commit hook should fail.

[Vsevolod Velichko (torkvemada)]

Martin, the idea of class with custom fields is cool (before your comment I was unsure about it, since I hadn't seen other hooks with such idea in bzr).
I've updated the hook, so it passes only limited set of params (I've eliminated even the message, I suppose that set_commit_message hook should be enough) and added some tests for invalid data.

Unmerged revisions

6544. By Vsevolod Velichko on 2012-10-12

Some test cases on which Commit.start_commit hook should fail.

6543. By Vsevolod Velichko on 2012-10-12

Do not pass all the commit data to the hook, but only some restricted set of properties.

6542. By Vsevolod Velichko on 2012-10-02

Remove unnecessary import in hook test

6541. By Vsevolod Velichko on 2012-10-02

Merge with trunk

6540. By Vsevolod Velichko on 2012-10-02

Test for commit.start_commit hook.

6539. By Vsevolod Velichko on 2012-07-17

Add commit.Commit hooks class and start_commit hook

=== modified file 'bzrlib/commit.py'
--- bzrlib/commit.py	2012-02-17 16:48:41 +0000
+++ bzrlib/commit.py	2012-10-12 16:19:21 +0000
@@ -56,6 +56,7 @@
     trace,
     tree,
     ui,
+    hooks,
     )
 from bzrlib.branch import Branch
 from bzrlib.cleanup import OperationWithCleanups
@@ -74,6 +75,7 @@
 from bzrlib import symbol_versioning
 from bzrlib.urlutils import unescape_for_display
 from bzrlib.i18n import gettext
+from bzrlib.osutils import contains_whitespace
 
 class NullCommitReporter(object):
     """I report on progress of a commit."""
@@ -402,6 +404,9 @@
         if len(self.parents) > 1 and self.exclude:
             raise errors.CannotCommitSelectedFileMerge(self.exclude)
 
+        # process start_commit hooks
+        self._process_commit_hooks('start_commit')
+
         # Collect the changes
         self._set_progress_stage("Collecting changes", counter=True)
         self._lossy = lossy
@@ -618,6 +623,50 @@
         # process new style post commit hooks
         self._process_hooks("post_commit", old_revno, new_revno)
 
+    def _merge_hook_properties(self, hookData):
+        """
+        Verify that all commit properties were not corrupted by hook
+        and merge them into self.
+        :type hookData: CommitHookParams
+        :param hookData: properties which were modified by hook
+        """
+        for name, value in hookData.revprops.iteritems():
+            if not isinstance(name, basestring) or contains_whitespace(name):
+                raise ValueError("invalid property name %r" % name)
+            if not isinstance(value, basestring):
+                raise ValueError("invalid property value %r for %r" % (value, name))
+        if hookData.timestamp is not None and not isinstance(hookData.timestamp, (int, float)):
+            raise ValueError("Invalid timestamp type: %r" % hookData.timestamp.__class__)
+        if hookData.timezone is not None and not isinstance(hookData.timezone, (int, float)):
+            raise ValueError("Invalid timezone type: %r" % hookData.timezone.__class__)
+        if hookData.committer is not None and not isinstance(hookData.committer, basestring):
+            raise ValueError("Invalid committer type: %r" % hookData.committer.__class__)
+        self.revprops = hookData.revprops
+        self.timestamp = hookData.timestamp
+        self.timezone = hookData.timezone
+        self.committer = hookData.committer
+
+    def _process_commit_hooks(self, hook_name):
+        """Process any registered commit hooks."""
+        self._set_progress_stage("Running %s hooks" % hook_name)
+        if not Commit.hooks[hook_name]:
+            return
+        for hook in Commit.hooks[hook_name]:
+            # show the running hook in the progress bar. As hooks may
+            # end up doing nothing (e.g. because they are not configured by
+            # the user) this is still showing progress, not showing overall
+            # actions - its up to each plugin to show a UI if it want's to
+            # (such as 'Emailing diff to [e-mail]').
+            self.pb_stage_name = "Running %s hooks [%s]" % \
+                (hook_name, Commit.hooks.get_hook_name(hook))
+            self._emit_progress()
+            if 'hooks' in debug.debug_flags:
+                mutter("Invoking commit hook: %r", hook)
+            hookData = CommitHookParams(self.revprops, self.timestamp, self.timezone, self.committer)
+            hook(hookData)
+            self._merge_hook_properties(hookData)
+
+
     def _process_hooks(self, hook_name, old_revno, new_revno):
         if not Branch.hooks[hook_name]:
             return
@@ -1017,3 +1066,48 @@
                     self.specific_files, [self.basis_tree, self.work_tree])
             else:
                 self.specific_file_ids = None
+
+class CommitHookParams(object):
+    """
+    Object holding params passed to Commit hooks.
+
+    :ivar revprops: commit metadata properties, dict of basicstring => basicstring
+    :ivar timestamp: commit timestamp
+    :ivar timezone: commit timezone
+    :ivar committer: committer name
+    """
+    def __init__(self, revprops, timestamp, timezone, committer):
+        self.revprops = revprops
+        self.timestamp = timestamp
+        self.timezone = timezone
+        self.committer = committer
+
+    def __eq__(self, other):
+        if self is other:
+            return True
+        return (self.revprops == other.revprops
+            and self.timestamp == other.timestamp
+            and self.timezone == other.timezone
+            and self.committer == other.committer)
+
+    def __repr__(self):
+        return "<%s(%s, %s, %s, %s)>" % (self.__class__.__name__,
+            self.revprops, self.timestamp, self.timezone, self.committer)
+
+class CommitHooks(hooks.Hooks):
+    """A dictionary mapping a hook name to a list of callables for commit hooks.
+    """
+
+    def __init__(self):
+        """Create the default hooks.
+
+        """
+        hooks.Hooks.__init__(self, "bzrlib.commit", "Commit.hooks")
+        self.add_hook('start_commit',
+            'Called before a commit is performed on a tree. The start commit '
+            'hook is able to change the commit properties before the commit '
+            'takes place. start_commit is called with the bzrlib.commit.Commit '
+            'being performed. The bzrlib.commit.CommitHookParams instance is '
+            'passed as as hook argument.', (2, 6))
+
+Commit.hooks = CommitHooks()

=== modified file 'bzrlib/tests/per_workingtree/test_commit.py'
--- bzrlib/tests/per_workingtree/test_commit.py	2012-09-19 07:58:27 +0000
+++ bzrlib/tests/per_workingtree/test_commit.py	2012-10-12 16:19:21 +0000
@@ -494,12 +494,13 @@
         tree.commit('second post', specific_files=['b'])
         # 5 steps, the first of which is reported 2 times, once per dir
         self.assertEqual(
-            [('update', 1, 5, 'Collecting changes [0] - Stage'),
-             ('update', 1, 5, 'Collecting changes [1] - Stage'),
-             ('update', 2, 5, 'Saving data locally - Stage'),
-             ('update', 3, 5, 'Running pre_commit hooks - Stage'),
-             ('update', 4, 5, 'Updating the working tree - Stage'),
-             ('update', 5, 5, 'Running post_commit hooks - Stage')],
+            [('update', 1, 5, 'Running start_commit hooks - Stage'),
+             ('update', 2, 5, 'Collecting changes [0] - Stage'),
+             ('update', 2, 5, 'Collecting changes [1] - Stage'),
+             ('update', 3, 5, 'Saving data locally - Stage'),
+             ('update', 4, 5, 'Running pre_commit hooks - Stage'),
+             ('update', 5, 5, 'Updating the working tree - Stage'),
+             ('update', 6, 5, 'Running post_commit hooks - Stage')],
             factory._calls
            )
 
@@ -515,13 +516,14 @@
                                                'hook name')
         tree.commit('first post')
         self.assertEqual(
-            [('update', 1, 5, 'Collecting changes [0] - Stage'),
-             ('update', 1, 5, 'Collecting changes [1] - Stage'),
-             ('update', 2, 5, 'Saving data locally - Stage'),
-             ('update', 3, 5, 'Running pre_commit hooks - Stage'),
-             ('update', 4, 5, 'Updating the working tree - Stage'),
-             ('update', 5, 5, 'Running post_commit hooks - Stage'),
-             ('update', 5, 5, 'Running post_commit hooks [hook name] - Stage'),
+            [('update', 1, 5, 'Running start_commit hooks - Stage'),
+             ('update', 2, 5, 'Collecting changes [0] - Stage'),
+             ('update', 2, 5, 'Collecting changes [1] - Stage'),
+             ('update', 3, 5, 'Saving data locally - Stage'),
+             ('update', 4, 5, 'Running pre_commit hooks - Stage'),
+             ('update', 5, 5, 'Updating the working tree - Stage'),
+             ('update', 6, 5, 'Running post_commit hooks - Stage'),
+             ('update', 6, 5, 'Running post_commit hooks [hook name] - Stage'),
              ],
             factory._calls
            )
@@ -538,13 +540,14 @@
                                                'hook name')
         tree.commit('first post')
         self.assertEqual(
-            [('update', 1, 5, 'Collecting changes [0] - Stage'),
-             ('update', 1, 5, 'Collecting changes [1] - Stage'),
-             ('update', 2, 5, 'Saving data locally - Stage'),
-             ('update', 3, 5, 'Running pre_commit hooks - Stage'),
-             ('update', 3, 5, 'Running pre_commit hooks [hook name] - Stage'),
-             ('update', 4, 5, 'Updating the working tree - Stage'),
-             ('update', 5, 5, 'Running post_commit hooks - Stage'),
+            [('update', 1, 5, 'Running start_commit hooks - Stage'),
+             ('update', 2, 5, 'Collecting changes [0] - Stage'),
+             ('update', 2, 5, 'Collecting changes [1] - Stage'),
+             ('update', 3, 5, 'Saving data locally - Stage'),
+             ('update', 4, 5, 'Running pre_commit hooks - Stage'),
+             ('update', 4, 5, 'Running pre_commit hooks [hook name] - Stage'),
+             ('update', 5, 5, 'Updating the working tree - Stage'),
+             ('update', 6, 5, 'Running post_commit hooks - Stage'),
              ],
             factory._calls
            )

=== modified file 'bzrlib/tests/test_commit.py'
--- bzrlib/tests/test_commit.py	2012-02-23 23:26:35 +0000
+++ bzrlib/tests/test_commit.py	2012-10-12 16:19:21 +0000
@@ -484,6 +484,42 @@
         finally:
             del bzrlib.ahook
 
+    def test_commit_start_commit_hook(self):
+        import bzrlib.commit as cmt
+        wt = self.make_branch_and_tree('.')
+        branch = wt.branch
+        def test_start_commit_hook(commit):
+            commit.revprops['test_prop'] = '42'
+        def broken_start_commit_hook_revprops(commit):
+            commit.revprops[17] = 22
+        def broken_start_commit_hook_committer(commit):
+            commit.committer = 42
+        def broken_start_commit_hook_timestamp(commit):
+            commit.timestamp = 'vasya'
+        def broken_start_commit_hook_timezone(commit):
+            commit.timezone = 'vasya'
+        broken_hooks = [broken_start_commit_hook_revprops,
+                        broken_start_commit_hook_timezone,
+                        broken_start_commit_hook_timestamp,
+                        broken_start_commit_hook_committer]
+        try:
+            cmt.Commit.hooks.install_named_hook('start_commit', test_start_commit_hook, 'test_prop_setter')
+            cmt.Commit().commit(
+                message="base", allow_pointless=True, rev_id='A',
+                working_tree=wt)
+        finally:
+            cmt.Commit.hooks.uninstall_named_hook('start_commit', 'test_prop_setter')
+        for hook in broken_hooks:
+            try:
+                cmt.Commit.hooks.install_named_hook('start_commit', hook, 'test_broken_prop_setter')
+                self.assertRaises(ValueError, cmt.Commit().commit,
+                    message='base', allow_pointless=True, rev_id='A',
+                    working_tree=wt)
+            finally:
+                cmt.Commit.hooks.uninstall_named_hook('start_commit', 'test_broken_prop_setter')
+
+        self.assertEquals(branch.repository.get_revision('A').properties['test_prop'], '42')
+
     def test_commit_object_doesnt_set_nick(self):
         # using the Commit object directly does not set the branch nick.
         wt = self.make_branch_and_tree('.')