[MERGE] Branch hooks implementation with a set_rh hook.

[John Arbash Meinel]

Robert Collins wrote:
> This patch adds Branch.hooks, a dictionary of hooks, containing an
> initial set_rh hook, which is triggered when the revision history is
> altered. This allows plugins to trigger on commit, push, pull, and
> uncommit robustly.
> 
> -Rob
> 
> 

I like the functionality, but it seems a little incomplete.

Shouldn't it pass the old revision history along with the new one?
Otherwise we don't really know what, if anything, has changed.

Should it be dealing in "revision-history" rather than "last-revision"?
I thought we were trying to get rid of "revision-history", since it has
really bad performance and disk space issues. (The mozilla conversion
has 170k revisions in the mainline, and .bzr/branch/revision-history is
around 9.3MB)

It might actually make even more sense to have it pass in the ancestry
graph, rather than a single 'last-revision' marker. But (old_revision,
new_revision) would let you ask the branch for the rest, and be stable
even if we get rid of an explicit revision-history cache.


...

>      @staticmethod
> +    def DefaultHooks():
> +        """Return a dict of the default branch hook settings."""
> +        return {
> +            'set_rh':[], # invoked whenever the revision history has been set
> +                         # with set_revision_history. The api signature is
> +                         # (branch, revision_history), and the branch will
> +                         # be write-locked.
> +            }
> +

^- Why is DefaultHooks capitalized? Was it originally a class?

Would it make sense that rather than expecting plugins to use
  bzrlib.branch.Branch.hooks[hook_name].append()
or even
  bzrlib.branch.Branch.hooks.setdefault(hook_name,[]).append()

We would have a real api:

  bzrlib.branch.Branch.add_hook(hook_name, func)

Or maybe:

  bzrlib.branch.Branch.hooks.add(hook_name, func)

I probably prefer this last one the most. It gives us a better api than
getting a KeyError if the hook name isn't supported, and it breaks the
functionality out of Branch and into a Hooks() class.


Also, the loop for hooks seems like it should be:

  for func in Branch.hooks.get('set_rh', []):
    func(self, ...)

Just in case something or someone sets the hooks dictionary to something
incomplete.

It also seems like you really should do:

  for func in self.hooks.get('set_rh', []):
    func(self, ...)

This lets:

1) Subclasses of Branch define a different set of default hooks.

2) Instances of Branch have a different set of hooks.

I realize you can handle the filtering when the function gets called.
Something like:

 def callback(branch, history):
   if not regex.match(branch.base):
     return # Do nothing, this branch doesn't concern us


Anyway, the #1 thing is that we need to pass the old state along with
the new state. Because otherwise something like bzr-email can't really
use this. Unless it assumes every time the revision history changes it
is because of committing the last entry. (Which is wrong in the case of
uncommit, and is incomplete for the case of push/pull).

John
=:->