[RFC] pre_commit hook

[John Arbash Meinel]

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

Nam Nguyen wrote:
> Hi list
> 
> Please comment.
> 
> Thanks
> Nam

Your new hook requests:

+        # invoked before a commit operation takes place.
+        # the api signature is
+        # (local, master, old_revno, old_revid, future_revno, future_revid,
+        #  deleted_paths, added_paths, future_revision_tree).
+        # old_revid is NULL_REVISION for the first commit to a branch
+        # future_revision_tree is an in-memory tree obtained from
+        # CommitBuilder.revision_tree()
+        # renamed paths appear in both deleted_paths and added_paths.
+        self['pre_commit'] = []

I would tend to:

1) Not pass old_revno and future_revno. If people want them, you can pass one,
and infer the other. (future_revno = old_revno+1).
But since you are passing 'local' and 'master' branches, they can just ask the
branch what revno is. (I guess it can't tell you for the future revno because
it hasn't been added yet, but it is just Branch.revno()+1).

2) You pass added_paths, deleted_paths, but just as important are 'modified'
paths. I would actually recommend passing them as a single argument. Possibly
as a "bzrlib.delta.TreeDelta" for some small consistency.
Though you really don't want to build something like that unless you are going
to use it.

I would probably only include a list of paths which have been effected,
ignoring whether they were added/deleted/renamed/modified/etc. But I'm not sure
what sort of pre-commit people are trying to do.

3) In _process_hooks() you convert the sets to a sorted list for *every* hook.
Rather than doing it just one time. I probably wouldn't even do that (if they
need sorted lists, the plugins can do paths = sorted(paths)) But see my comment
about (2) that you may do it different anyway.

4) You pass hook_master, but it hasn't had any data uploaded yet. I guess most
plugins use master to determine a public location. So I'm okay with having it,
but I'm wondering if it is actually useful.

5) Overall, I would try not to pass too many things.



Side note, WT3.put_file_bytes_non_atomic() says to see MutableTree.put_file...
but MutableTree doesn't actually have that function. It only exists on actual
implementations. We should fix that.


Most of your tests look good. I would comment on:

+    def test_pre_commit_fails(self):
+        tree = self.make_branch_and_memory_tree('branch')
+        tree.lock_write()
+        tree.add('')
+        class PreCommitException(Exception): pass
+        def hook_func(_1, _2, _3, _4, _5, new_revid, _7, _8, _9):
+            raise PreCommitException(new_revid)
+        Branch.hooks.install_hook("pre_commit", self.capture_pre_commit_hook)
+        Branch.hooks.install_hook("pre_commit", hook_func)
+        revids = [None, None, None]
+        try:
+            tree.commit('message')
+        except PreCommitException, e:
+            revids[0] = e.message

^- Here you should just do:

err = self.assertRaises(PreCommitException, tree.commit, 'message)
revids[0] = e.message

I comment indicating that you remove the failing hook, so the next commits
should pass would be helpful. But honestly, I think that code should be broken
out into a separate test unless you feel there is a specific reason to put it here.


So I think it is pretty good. I think it could use some tweaking, and we will
want to make sure we know what we want to pass to the pre-commit hooks. If we
make the contract too verbose, it could adversely effect performance.

As we refactor commit to do less work, we don't want to have to put back extra
work because the pre-commit hook api (which may not even be used) requires it.

I might even go so far as to switch from modified paths to just 'affected'
file-ids. Hooks can figure out what they need from there, and I think it
reduces the burden on commit.

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

iD8DBQFGsg1uJdeBCYSNAAMRAh6MAKDFEeYsu3x9denP6xGnOcMog6dQ1QCfWNBZ
sLWw+T1kuG7wKnVEFYXhTMM=
=zuLJ
-----END PGP SIGNATURE-----