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

Proposed by Vsevolod Velichko on 2012-07-17
Status: Needs review
Proposed branch: lp:~torkvemada/bzr/commit_hooks
Merge into: lp:bzr
Diff against target: 244 lines (+153/-20)
3 files modified
bzrlib/commit.py (+94/-0)
bzrlib/tests/per_workingtree/test_commit.py (+23/-20)
bzrlib/tests/test_commit.py (+36/-0)
To merge this branch: bzr merge lp:~torkvemada/bzr/commit_hooks
Reviewer Review Type Date Requested Status
Martin Packman (community) 2012-07-17 Abstain on 2012-10-10
Review via email: mp+115348@code.launchpad.net

Description of the Change

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

To post a comment you must log in.
lp:~torkvemada/bzr/commit_hooks updated on 2012-10-02
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) wrote :

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

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

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

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 updated on 2012-10-12
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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/commit.py'
2--- bzrlib/commit.py 2012-02-17 16:48:41 +0000
3+++ bzrlib/commit.py 2012-10-12 16:19:21 +0000
4@@ -56,6 +56,7 @@
5 trace,
6 tree,
7 ui,
8+ hooks,
9 )
10 from bzrlib.branch import Branch
11 from bzrlib.cleanup import OperationWithCleanups
12@@ -74,6 +75,7 @@
13 from bzrlib import symbol_versioning
14 from bzrlib.urlutils import unescape_for_display
15 from bzrlib.i18n import gettext
16+from bzrlib.osutils import contains_whitespace
17
18 class NullCommitReporter(object):
19 """I report on progress of a commit."""
20@@ -402,6 +404,9 @@
21 if len(self.parents) > 1 and self.exclude:
22 raise errors.CannotCommitSelectedFileMerge(self.exclude)
23
24+ # process start_commit hooks
25+ self._process_commit_hooks('start_commit')
26+
27 # Collect the changes
28 self._set_progress_stage("Collecting changes", counter=True)
29 self._lossy = lossy
30@@ -618,6 +623,50 @@
31 # process new style post commit hooks
32 self._process_hooks("post_commit", old_revno, new_revno)
33
34+ def _merge_hook_properties(self, hookData):
35+ """
36+ Verify that all commit properties were not corrupted by hook
37+ and merge them into self.
38+ :type hookData: CommitHookParams
39+ :param hookData: properties which were modified by hook
40+ """
41+ for name, value in hookData.revprops.iteritems():
42+ if not isinstance(name, basestring) or contains_whitespace(name):
43+ raise ValueError("invalid property name %r" % name)
44+ if not isinstance(value, basestring):
45+ raise ValueError("invalid property value %r for %r" % (value, name))
46+ if hookData.timestamp is not None and not isinstance(hookData.timestamp, (int, float)):
47+ raise ValueError("Invalid timestamp type: %r" % hookData.timestamp.__class__)
48+ if hookData.timezone is not None and not isinstance(hookData.timezone, (int, float)):
49+ raise ValueError("Invalid timezone type: %r" % hookData.timezone.__class__)
50+ if hookData.committer is not None and not isinstance(hookData.committer, basestring):
51+ raise ValueError("Invalid committer type: %r" % hookData.committer.__class__)
52+ self.revprops = hookData.revprops
53+ self.timestamp = hookData.timestamp
54+ self.timezone = hookData.timezone
55+ self.committer = hookData.committer
56+
57+ def _process_commit_hooks(self, hook_name):
58+ """Process any registered commit hooks."""
59+ self._set_progress_stage("Running %s hooks" % hook_name)
60+ if not Commit.hooks[hook_name]:
61+ return
62+ for hook in Commit.hooks[hook_name]:
63+ # show the running hook in the progress bar. As hooks may
64+ # end up doing nothing (e.g. because they are not configured by
65+ # the user) this is still showing progress, not showing overall
66+ # actions - its up to each plugin to show a UI if it want's to
67+ # (such as 'Emailing diff to foo@example.com').
68+ self.pb_stage_name = "Running %s hooks [%s]" % \
69+ (hook_name, Commit.hooks.get_hook_name(hook))
70+ self._emit_progress()
71+ if 'hooks' in debug.debug_flags:
72+ mutter("Invoking commit hook: %r", hook)
73+ hookData = CommitHookParams(self.revprops, self.timestamp, self.timezone, self.committer)
74+ hook(hookData)
75+ self._merge_hook_properties(hookData)
76+
77+
78 def _process_hooks(self, hook_name, old_revno, new_revno):
79 if not Branch.hooks[hook_name]:
80 return
81@@ -1017,3 +1066,48 @@
82 self.specific_files, [self.basis_tree, self.work_tree])
83 else:
84 self.specific_file_ids = None
85+
86+class CommitHookParams(object):
87+ """
88+ Object holding params passed to Commit hooks.
89+
90+ :ivar revprops: commit metadata properties, dict of basicstring => basicstring
91+ :ivar timestamp: commit timestamp
92+ :ivar timezone: commit timezone
93+ :ivar committer: committer name
94+ """
95+ def __init__(self, revprops, timestamp, timezone, committer):
96+ self.revprops = revprops
97+ self.timestamp = timestamp
98+ self.timezone = timezone
99+ self.committer = committer
100+
101+ def __eq__(self, other):
102+ if self is other:
103+ return True
104+ return (self.revprops == other.revprops
105+ and self.timestamp == other.timestamp
106+ and self.timezone == other.timezone
107+ and self.committer == other.committer)
108+
109+ def __repr__(self):
110+ return "<%s(%s, %s, %s, %s)>" % (self.__class__.__name__,
111+ self.revprops, self.timestamp, self.timezone, self.committer)
112+
113+class CommitHooks(hooks.Hooks):
114+ """A dictionary mapping a hook name to a list of callables for commit hooks.
115+ """
116+
117+ def __init__(self):
118+ """Create the default hooks.
119+
120+ """
121+ hooks.Hooks.__init__(self, "bzrlib.commit", "Commit.hooks")
122+ self.add_hook('start_commit',
123+ 'Called before a commit is performed on a tree. The start commit '
124+ 'hook is able to change the commit properties before the commit '
125+ 'takes place. start_commit is called with the bzrlib.commit.Commit '
126+ 'being performed. The bzrlib.commit.CommitHookParams instance is '
127+ 'passed as as hook argument.', (2, 6))
128+
129+Commit.hooks = CommitHooks()
130
131=== modified file 'bzrlib/tests/per_workingtree/test_commit.py'
132--- bzrlib/tests/per_workingtree/test_commit.py 2012-09-19 07:58:27 +0000
133+++ bzrlib/tests/per_workingtree/test_commit.py 2012-10-12 16:19:21 +0000
134@@ -494,12 +494,13 @@
135 tree.commit('second post', specific_files=['b'])
136 # 5 steps, the first of which is reported 2 times, once per dir
137 self.assertEqual(
138- [('update', 1, 5, 'Collecting changes [0] - Stage'),
139- ('update', 1, 5, 'Collecting changes [1] - Stage'),
140- ('update', 2, 5, 'Saving data locally - Stage'),
141- ('update', 3, 5, 'Running pre_commit hooks - Stage'),
142- ('update', 4, 5, 'Updating the working tree - Stage'),
143- ('update', 5, 5, 'Running post_commit hooks - Stage')],
144+ [('update', 1, 5, 'Running start_commit hooks - Stage'),
145+ ('update', 2, 5, 'Collecting changes [0] - Stage'),
146+ ('update', 2, 5, 'Collecting changes [1] - Stage'),
147+ ('update', 3, 5, 'Saving data locally - Stage'),
148+ ('update', 4, 5, 'Running pre_commit hooks - Stage'),
149+ ('update', 5, 5, 'Updating the working tree - Stage'),
150+ ('update', 6, 5, 'Running post_commit hooks - Stage')],
151 factory._calls
152 )
153
154@@ -515,13 +516,14 @@
155 'hook name')
156 tree.commit('first post')
157 self.assertEqual(
158- [('update', 1, 5, 'Collecting changes [0] - Stage'),
159- ('update', 1, 5, 'Collecting changes [1] - Stage'),
160- ('update', 2, 5, 'Saving data locally - Stage'),
161- ('update', 3, 5, 'Running pre_commit hooks - Stage'),
162- ('update', 4, 5, 'Updating the working tree - Stage'),
163- ('update', 5, 5, 'Running post_commit hooks - Stage'),
164- ('update', 5, 5, 'Running post_commit hooks [hook name] - Stage'),
165+ [('update', 1, 5, 'Running start_commit hooks - Stage'),
166+ ('update', 2, 5, 'Collecting changes [0] - Stage'),
167+ ('update', 2, 5, 'Collecting changes [1] - Stage'),
168+ ('update', 3, 5, 'Saving data locally - Stage'),
169+ ('update', 4, 5, 'Running pre_commit hooks - Stage'),
170+ ('update', 5, 5, 'Updating the working tree - Stage'),
171+ ('update', 6, 5, 'Running post_commit hooks - Stage'),
172+ ('update', 6, 5, 'Running post_commit hooks [hook name] - Stage'),
173 ],
174 factory._calls
175 )
176@@ -538,13 +540,14 @@
177 'hook name')
178 tree.commit('first post')
179 self.assertEqual(
180- [('update', 1, 5, 'Collecting changes [0] - Stage'),
181- ('update', 1, 5, 'Collecting changes [1] - Stage'),
182- ('update', 2, 5, 'Saving data locally - Stage'),
183- ('update', 3, 5, 'Running pre_commit hooks - Stage'),
184- ('update', 3, 5, 'Running pre_commit hooks [hook name] - Stage'),
185- ('update', 4, 5, 'Updating the working tree - Stage'),
186- ('update', 5, 5, 'Running post_commit hooks - Stage'),
187+ [('update', 1, 5, 'Running start_commit hooks - Stage'),
188+ ('update', 2, 5, 'Collecting changes [0] - Stage'),
189+ ('update', 2, 5, 'Collecting changes [1] - Stage'),
190+ ('update', 3, 5, 'Saving data locally - Stage'),
191+ ('update', 4, 5, 'Running pre_commit hooks - Stage'),
192+ ('update', 4, 5, 'Running pre_commit hooks [hook name] - Stage'),
193+ ('update', 5, 5, 'Updating the working tree - Stage'),
194+ ('update', 6, 5, 'Running post_commit hooks - Stage'),
195 ],
196 factory._calls
197 )
198
199=== modified file 'bzrlib/tests/test_commit.py'
200--- bzrlib/tests/test_commit.py 2012-02-23 23:26:35 +0000
201+++ bzrlib/tests/test_commit.py 2012-10-12 16:19:21 +0000
202@@ -484,6 +484,42 @@
203 finally:
204 del bzrlib.ahook
205
206+ def test_commit_start_commit_hook(self):
207+ import bzrlib.commit as cmt
208+ wt = self.make_branch_and_tree('.')
209+ branch = wt.branch
210+ def test_start_commit_hook(commit):
211+ commit.revprops['test_prop'] = '42'
212+ def broken_start_commit_hook_revprops(commit):
213+ commit.revprops[17] = 22
214+ def broken_start_commit_hook_committer(commit):
215+ commit.committer = 42
216+ def broken_start_commit_hook_timestamp(commit):
217+ commit.timestamp = 'vasya'
218+ def broken_start_commit_hook_timezone(commit):
219+ commit.timezone = 'vasya'
220+ broken_hooks = [broken_start_commit_hook_revprops,
221+ broken_start_commit_hook_timezone,
222+ broken_start_commit_hook_timestamp,
223+ broken_start_commit_hook_committer]
224+ try:
225+ cmt.Commit.hooks.install_named_hook('start_commit', test_start_commit_hook, 'test_prop_setter')
226+ cmt.Commit().commit(
227+ message="base", allow_pointless=True, rev_id='A',
228+ working_tree=wt)
229+ finally:
230+ cmt.Commit.hooks.uninstall_named_hook('start_commit', 'test_prop_setter')
231+ for hook in broken_hooks:
232+ try:
233+ cmt.Commit.hooks.install_named_hook('start_commit', hook, 'test_broken_prop_setter')
234+ self.assertRaises(ValueError, cmt.Commit().commit,
235+ message='base', allow_pointless=True, rev_id='A',
236+ working_tree=wt)
237+ finally:
238+ cmt.Commit.hooks.uninstall_named_hook('start_commit', 'test_broken_prop_setter')
239+
240+ self.assertEquals(branch.repository.get_revision('A').properties['test_prop'], '42')
241+
242 def test_commit_object_doesnt_set_nick(self):
243 # using the Commit object directly does not set the branch nick.
244 wt = self.make_branch_and_tree('.')