[MERGE] Show commit hook names during commit.

[John Arbash Meinel]

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

Robert Collins wrote:
> On Thu, 2007-06-28 at 14:03 +1000, Martin Pool wrote:
>> +0
>>
>> (bb gets an internal error when i vote.)
>>
>> @@ -494,6 +493,9 @@
>>          else:
>>              old_revid = bzrlib.revision.NULL_REVISION
>>          for hook in Branch.hooks['post_commit']:
>> +            self.pb_stage_name = "Running post commit hooks [%s]" % \
>> +                Branch.hooks.get_hook_name(hook)
>> +            self._emit_progress()
>>              hook(hook_local, hook_master, old_revno, old_revid, new_revno,
>>                  self.rev_id)
>>
>> Would it maybe be better to show these as regular messages (notes)
>> rather than through the progress bar, so that a view of what was done
>> remains visible on the user's screen?
> 
> I don't think so. Theres a difference between 'I called the email hook'
> and 'an email was sent'. So having it flip through them quickly is all
> thats appropriate here IMNSHO.
> 
>> +    def get_hook_name(self, a_callable):
>> +        """Get the name for a_callable for UI display.
>> +
>> +        If no name has been registered, the string 'No hook name' is returned.
>> +        """
>> +        return self._callable_names.get(a_callable, "No hook name")
>>
>> Would it be better to show the callable repr or class name if no name is set?
> 
> I don't think so - they are typically quite ugly or not human meaning
> full. I am not -1 on the concept, but I tried it and rather hated it. I
> guess for debugging it would be better, but perhaps -Dhooks could always
> show the repr instead ? (I'd need a pointer at doing -D foodoo to do
> this trivially).

Why not just use "__name__" and maybe "__module__". Specifically you get:
>>> bzrlib.plugins.email.branch_commit_hook.__name__
'branch_commit_hook'
>>> bzrlib.plugins.email.branch_commit_hook.__module__
'bzrlib.plugins.email'

So if you did:

>>> x = bzrlib.plugins.email.branch_commit_hook
>>> x.__module__ + '.' + x.__name__
'bzrlib.plugins.email.branch_commit_hook'

You could even do it as '<%s>' % (x.__name__,)

Which would generally be short, and indicate it wasn't explicitly named.

Regardless, I would rather see:

 Running post commit hooks [unknown]

Than

 Running post commit hooks [No hook name]

But I think

 Running post commit hooks [branch_commit_hook]

is ok. I can see that different plugins might name the actual hook
function the same thing, though.

> 
>> How about making the name an attribute of the hook object, rather than
>> separately registered?  I suppose the problem is that the hooks are
>> typically just callables not objects and so it is ugly to set
>> attributes on them?
> 
> Thats exactly it. They are callables and its a little ugly to do
> def foo(....):
>     """"""
> foo.name_for_bzr = "blah".
> bzrlib.branch.Branch.hooks.install_hook('post_commit', foo)
> 
> Its the same amount of spelling though to do what I've done :).
> 
> -Rob

I'm fine with a register for an explicit name, but having a decent
default is probably reasonable, too.

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

iD8DBQFGg8GeJdeBCYSNAAMRAhSwAJ9kTOPRaIkOQCeKsMpHTsAtzbHKygCcCD+u
Tkd35dp5gT9/cvdxyNIeL5M=
=gYHO
-----END PGP SIGNATURE-----