Exception mangling in hooks?

[Andrew Bennetts]

Robert Collins wrote:
> Can anyone explain to me why we mangle exceptions in the
> pre_branch_change_hook calling code - it makes it hard to debug why a
> hook actually fails, because the entire call stack is relevant, and we
> don't do it for other hooks.

So that callers of branch.set_last_revision (etc) can distinguish between “a
hook blew up” and “branch.set_last_revision itself blew up”.  (Well, there's
actually a third option, TipChangeRejected, i.e. “a hook explicitly rejects
the tip change”, but that's not relevant here.)

Note that the entire original call stack is still present in the HookFailed
exception instance (and the __str__ of that exception class will show that
traceback to you).

If you're using pdb I guess it might be little bit more awkward, although
you should be able to pdb.post_mortem the .exc_info of the exception... or
insert a pdb.set_trace() in HookFailed.__init__.

> I'd like to apply this diff to remove this:
> 
> === modified file 'bzrlib/branch.py'
> --- bzrlib/branch.py    2009-02-25 15:36:48 +0000
> +++ bzrlib/branch.py    2009-02-25 22:03:59 +0000
> @@ -875,15 +875,7 @@
>          params = ChangeBranchTipParams(
>              self, old_revno, new_revno, old_revid, new_revid)
>          for hook in hooks:
> -            try:
> -                hook(params)
> -            except errors.TipChangeRejected:
> -                raise
> -            except Exception:
> -                exc_info = sys.exc_info()
> -                hook_name = Branch.hooks.get_hook_name(hook)
> -                raise errors.HookFailed(
> -                    'pre_change_branch_tip', hook_name, exc_info)
> +            hook(params)

I'd prefer to keep this, I think it's useful.  It helps identify that it's a
hook that has the bug, not bzr itself, and it also identifies in the message
which hook is to blame.  And because HookFailed is not an internal_error,
the user doesn't get a message telling them to report a bug on bzr just
because they misconfigured a plugin.

-Andrew.