[RFC] pre_commit hook

[Nam Nguyen]

On 8/3/07, Robert Collins <robertc at robertcollins.net> wrote:
> On Thu, 2007-08-02 at 11:59 -0500, John Arbash Meinel wrote:
> > -----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).
>
> Its not that cheap to ask, particularly with bound branches. And its not
> cached at the moment. This is why the post commit hook passes them in
> even though they are derivable - because they have already been derived.
> So I think it should be carefully considered before passing in just the
> minimum.

In order to keep the two hooks (pre and post) similar, I intentionally
leave their signatures as close as possible.

>
> > 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.

They are now passed in as a dictionary of (change, ids) pairs.

>
> We should check if there are any hooks to run first, that would allow
> conditional creation of such a thing.

Done.

>
> > 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.

I, for one, would be interested in new additions and modifications to
the old tree.

>
> > 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.
>
> I'd say it is useful, but perhaps data should be uploaded (but not
> inserted to the branch tip) before the hook fires?
>
> > 5) Overall, I would try not to pass too many things.
>
> I think passing in a single object with many attributes is possibly
> better. It allows incremental API changes more easily for sure, and
> theres less of a 'omg the whole kitchen sink is in the hook method'
> feeling.

The changes are passed to the hook via a dictionary now. There are
spaces for changes later (when we have enumeration of
InventoryChanges) and I find the dictionary more compact too.

I put some more comments in test_pre_commit_fails. This test is to
test that a commit is rollbacked when the pre hook raises an
Exception. The first commit fails, the second commit should have the
same revno as the first, but the third commit should have different
revno than the second.

Here's the commit message. I hope it is descriptive enough.

``Branch.hooks`` now supports ``pre_commit`` hook.

The hook's signature is

  ::

    hook(local, master, old_revno, old_revid, new_revno, new_revid,
         affected_ids, future_revision_tree)

``affected_ids`` is a dictionary of (change_type, ids). change_type is
a string describing the change (e.g. 'added', 'deleted', 'modified'), and
ids is a list of inventory entry ids.

``future_revision_tree`` is obtained from ``CommitBuilder.revision_tree``
to save hooks from getting it from the branch again.

For example, hooks can get an added file:

  ::

    for id in affected_ids.get('added', []):
        if future_revision_tree.kind(id) == 'file':
            file = future_revision_tree.get_file(id)
            ...

or export a tree and do ``make check`` or similar

  ::

    import bzrlib.export
    bzrlib.export.export(future_revision_tree, 'tmp_space')

If the commit is to be rejected, hooks should raise an ``Exception``.


Cheers
Nam
-------------- next part --------------
A non-text attachment was scrubbed...
Name: precommit2.patch
Type: application/octet-stream
Size: 24056 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070807/4593693e/attachment-0001.obj