[MERGE] Add tests for start_commit hook.

[Jelmer Vernooij]

Hi John,

On Sat, Apr 05, 2008 at 03:55:36PM +0300, John Arbash Meinel wrote:
> Jelmer Vernooij wrote:
> | This patch adds a start_commit hook for MutableTrees (bug 186422). This
> | hook is executed before a commit is started and can actually modify the
> | state of the working tree, for example by adding files. This patch is
> | required to be able to support bzr in etckeeper
> | (http://kitenet.net/~joey/code/etckeeper/)
> This should be a workingtree_implementations test so that we are sure to
> maintain the hook in any future implementation.

> You also need to define the api of the start_commit hook. It seems like you are
> just passing in the mutable tree so just saying:

> # the hook has the signature None = hook(MutableTree)

Thanks, fixed.

> Aka, the return value is ignored, and the Tree being committed is being passed in.

> However....

> Don't you need to pass in the selected_files? Possibly in a way that it can add
> more?
Yeah, that's a good point. In my specific case it wasn't necessary but
I'll have a look at adding that.

> What about the commit message, or any of the other parameters being passed in? I
> know there is some amount of just getting enough for what you need, and we are
> having an edit_message hook, etc. But we also want to make sure that the hook is
> functional for more than just 1 use case.
I didn't really want to go there yet. This hook is only being used when
committing from a MutableTree to make additional changes to the tree, whereas the 
edit_message hook would also be executed when talking to the commit builder directly.

> There is also some small PEP8 stuff in test_mutiabletree.py
Can you be more specific? I can't find anything, but perhaps my PEP8
is getting rusty...

> I'm fine having those tests there, but we need to have a
> workingtree_implementation test for hooks, and we should make sure that all of
> them fire 'start_commit' passing in the working tree so that people can depend
> on it.
There's already a test to make sure start_commit is being raised. 

> In some ways I'd like it to be an end-to-end blackbox test. Mostly because of
> the problems we ran into when we changed 'set_revision_history()' to no longer
> be called, and suddenly the 'set_rh' hook was never fired again. It is okay to
> do that, but we need to *know* that we are doing so.
I'll see if I can add something like that. Is there something like
this for set_rh yet that I could get inspriation from?

Cheers,

Jelmer