[MERGE] Add tests for start_commit hook.

[John Arbash Meinel]

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

Jelmer Vernooij wrote:
| 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
|
|

I don't know that we do, but you can look at James's set_last_revision_info tests.

In general, I would do something like add a hook during the test case, make sure
it cleans itself out at the end.
And then do an operation and make sure it gets called. Something like this:

def test_start_commit(self):

~  calls = []
~  def start_hook_call(*args, **kwargs):
~    calls.append((args, kwargs))

~  def remove_hook():
~    WorkingTree.hooks.remove(start_hook_call)

~  WorkingTree.hooks.install_hook('start_commit', start_hook_call)
~  self.addCleanup(remove_hook)

~  wt = self.make_branch_and_tree('foo')
~  wt.commit('one')

~  self.assertEqual([something], calls)


However, you may not want to assert that you have a WT object, so you could have
the calls append information rather than the raw arguments.

I would generally recommend passing a specific Parameter object like Ian did for
the Branch revision info changed hook. As it makes it easy to add more
information into the api without breaking existing hooks.

So something like:

start_commit(MutableTree, StartCommitParameters)

where StartCommitParameters has stuff like "selected_files" or
"selected_file_ids" etc.

If we have a hook for modifying the commit message (which Ian proposed, I don't
know if it was merged) then I wouldn't do it here. I don't really like having a
separate hook for every single thing that you might want to inspect, though I
also don't want a single hook that has to handle everything at once. So we need
a bit of care to have *good* hooks rather than just hooks.

(If you just want hooks you can decorate objects at runtime and get all the
functionality you could possibly want)

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

iD8DBQFH/HnOJdeBCYSNAAMRAi8TAJ4uwT2HspgBM8fEyJoKtSGLaV4E4gCeLJca
kj8sPUtFQF71/tDW4ZHsNo4=
=Y44y
-----END PGP SIGNATURE-----