[MERGE] pre_commit hook (was: [RFC] pre_commit hook)

[Aaron Bentley]

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

bb:resubmit

Nam Nguyen wrote:
> On 8/15/07, John Arbash Meinel <john at arbash-meinel.com> wrote:
>> John Arbash Meinel has voted resubmit.
> === modified file 'bzrlib/branch.py'
> --- bzrlib/branch.py	2007-07-27 13:02:00 +0000
> +++ bzrlib/branch.py	2007-08-07 08:23:28 +0000
> @@ -1026,6 +1026,17 @@
>          # is read locked and the target branches write locked. The local
>          # branch is the low-latency branch.
>          self['post_pull'] = []
> +        # invoked before a commit operation takes place.
> +        # the api signature is
> +        # (local, master, old_revno, old_revid, future_revno, future_revid,
> +        #  affected_ids, future_revision_tree).

The documentation should make clear whether a pre-commit hook may make
changes to the tree.

> +        # old_revid is NULL_REVISION for the first commit to a branch
> +        # affected_ids is a dictionary of (change_type, ids) pairs where ids
> +        # is a list of inventory entry ids and change_type is a string
> +        # describing the change (added, deleted, renamed, etc.)

^^^ Do we really need this?  We've already got a bunch of different
change reporting interfaces.  Can't you hook into CommitReporter?  Or
use the TreeDelta format?  Or use the _ChangeReporter format?  You could
even use _iter_changes format.

> @@ -494,19 +511,25 @@
>              old_revid = self.parents[0]
>          else:
>              old_revid = bzrlib.revision.NULL_REVISION
> -        for hook in Branch.hooks['post_commit']:
> +            
> +        for hook in Branch.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 foo at example.com').
> -            self.pb_stage_name = "Running post commit hooks [%s]" % \
> -                Branch.hooks.get_hook_name(hook)
> +            self.pb_stage_name = "Running %s hooks [%s]" % \
> +                (hook_name.replace('_', ' '), Branch.hooks.get_hook_name(hook))

I'm not so sure it's a good idea to change the hook name.  Calling it
"pre_commit" and "post_commit" makes it easier for people to identify.
But if we do change it, I think we should change it to "pre-commit" /
"post-commit", rather than "pre commit" / "post commit".

> +            if hook_name == "post_commit":
> +                hook(hook_local, hook_master, old_revno, old_revid, new_revno,
> +                     self.rev_id)
> +            elif hook_name == "pre_commit":
> +                hook(hook_local, hook_master,
> +                     old_revno, old_revid, new_revno, self.rev_id,
> +                     self.affected_ids, self.builder.revision_tree())

^^^ I think it could be a significant performance issue to call
self.builder.revision_tree repeatedly.


> +        def hook_func(_1, _2, _3, _4, _5, new_revid, _7, _8):
> +            raise PreCommitException(new_revid)

^^^ We don't use _ in dummy parameters, and in fact we don't use dummy
parameters.  The names of the parameters are part of the signature, just
like the position of the parameters is.

> +        # this commit will raise exception
                              "raise an exception"

> +        # so the commit is rollbacked and revno unchanged
                              "rolled back"

> +        err = self.assertRaises(PreCommitException, tree.commit, 'message')
> +        # we have to record the revid to use in assertEqual later

^^^ Well, you could just specify the revid.  Either way is fine, though.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGylpJ0F+nu1YWqI0RAgPnAJ4mfovTdbbyVWardwWLnEvC7dVmYgCfVNfB
/RtE0Y2iL2xSFcb6K86zoaE=
=fuJK
-----END PGP SIGNATURE-----