[RFC] pre_commit hook

[Robert Collins]

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.

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

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

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

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

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070803/50ac58ff/attachment.pgp