Merge lp:~jelmer/bzr/uninstall-hook into lp:bzr

[Jelmer Vernooij]

Add Hooks.uninstall_named_hook().

Add a Hooks.uninstall_named_hook(), which allows uninstalling named hooks (traditional and lazy).

[Andrew Bennetts (spiv)]

What happens if two calls to install_named_hook use the same label?

[Jelmer Vernooij (jelmer)]

On Wed, 2011-03-30 at 12:59 +0000, Andrew Bennetts wrote:
> What happens if two calls to install_named_hook use the same label?
Bad things (basically, the first one with the label will be removed).

I guess the alternative is to allow uninstalling by callable, but that
will require loading all lazy hooks. Would that be an issue?

Cheers,

Jelmer

[Jonathan Lange (jml)]

Thanks! This looks pretty sensible to me.

review: Approve

[John A Meinel (jameinel)]

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

On 3/30/2011 2:41 PM, Jelmer Vernooij wrote:
> Jelmer Vernooij has proposed merging lp:~jelmer/bzr/uninstall-hook into lp:bzr with lp:~jelmer/bzr/more-lazy-hooks as a prerequisite.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> Bug #301472 in Bazaar: "need a way to uninstall a hook"
> https://bugs.launchpad.net/bzr/+bug/301472
>
> For more details, see:
> https://code.launchpad.net/~jelmer/bzr/uninstall-hook/+merge/55527
>
> Add a Hooks.uninstall_named_hook(), which allows uninstalling named hooks (traditional and lazy).

We pretty much never use getattr/except instead do:

uninstall = getattr(hook, "uninstall", None)
if uninstall is None:
  raise errors.Unsupported...
else:
  uninstall()

The rest looks good to me.

 merge: approve

(tweak)

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

iEYEARECAAYFAk2TO44ACgkQJdeBCYSNAAMVOACfY6Otldq1ltDBZZnCPJgJEHjH
fzYAoKy8gukMam7H88wNbbye7cl/1Fim
=b+Z8
-----END PGP SIGNATURE-----

review: Approve

[Andrew Bennetts (spiv)]

Jelmer Vernooij wrote:
> On Wed, 2011-03-30 at 12:59 +0000, Andrew Bennetts wrote:
> > What happens if two calls to install_named_hook use the same label?
> Bad things (basically, the first one with the label will be removed).
>
> I guess the alternative is to allow uninstalling by callable, but that
> will require loading all lazy hooks. Would that be an issue?

Another alternative would be to return a unique handle from
install_named_hook. Possibly an object with a .uninstall() method for
extra convenience.

I suspect it'd be fiddly to extend the registry objects to support that,
though.

[Jelmer Vernooij (jelmer)]

> Jelmer Vernooij wrote:
> > On Wed, 2011-03-30 at 12:59 +0000, Andrew Bennetts wrote:
> > > What happens if two calls to install_named_hook use the same label?
> > Bad things (basically, the first one with the label will be removed).
> >
> > I guess the alternative is to allow uninstalling by callable, but that
> > will require loading all lazy hooks. Would that be an issue?
>
> Another alternative would be to return a unique handle from
> install_named_hook. Possibly an object with a .uninstall() method for
> extra convenience.
>
> I suspect it'd be fiddly to extend the registry objects to support that,
> though.
Yeah, that seems a bit like overkill. I think we should either:

- Prevent registration of multiple callbacks with the same name - this might break some existing plugins (?)

- Allow unregistering callables by the method object. this means it's necessary to load all callables when unregistering one, but on the other hand unregistering hooks will mostly be used for testsuites, etc.

[Jelmer Vernooij (jelmer)]

... or we could just remove all hooks with the specified label. I think it's reasonable to require people to specify a unique label if they want to be able to uninstall their hooks.

[Jelmer Vernooij (jelmer)]

updated to uninstall all hooks with a specified label, not just the first one.

[Vincent Ladeuil (vila)]

Yeah, I agree that labels should be unique.

On the other hand... this should then be checked at registration time ;)

review: Approve

[Jelmer Vernooij (jelmer)]

sent to pqm by email

[Jelmer Vernooij (jelmer)]

sent to pqm by email

=== modified file 'bzrlib/hooks.py'
--- bzrlib/hooks.py	2011-03-30 13:03:08 +0000
+++ bzrlib/hooks.py	2011-04-02 00:14:32 +0000
@@ -243,6 +243,24 @@
         if name is not None:
             self.name_hook(a_callable, name)
 
+    def uninstall_named_hook(self, hook_name, label):
+        """Uninstall named hooks.
+
+        :param hook_name: Hook point name
+        :param label: Label of the callable to uninstall
+        """
+        try:
+            hook = self[hook_name]
+        except KeyError:
+            raise errors.UnknownHook(self.__class__.__name__, hook_name)
+        try:
+            uninstall = getattr(hook, "uninstall")
+        except AttributeError:
+            raise errors.UnsupportedOperation(self.install_named_hook_lazy,
+                self)
+        else:
+            uninstall(label)
+
     def name_hook(self, a_callable, name):
         """Associate name with a_callable to show users what is running."""
         self._callable_names[a_callable] = name
@@ -328,6 +346,21 @@
         obj_getter = registry._ObjectGetter(callback)
         self._callbacks.append((obj_getter, callback_label))
 
+    def uninstall(self, label):
+        """Uninstall the callback with the specified label.
+
+        :param label: Label of the entry to uninstall
+        """
+        entries_to_remove = []
+        for entry in self._callbacks:
+            (entry_callback, entry_label) = entry
+            if entry_label == label:
+                entries_to_remove.append(entry)
+        if entries_to_remove == []:
+            raise KeyError("No entry with label %r" % label)
+        for entry in entries_to_remove:
+            self._callbacks.remove(entry)
+
     def __iter__(self):
         return (callback.get_obj() for callback, name in self._callbacks)
 

=== modified file 'bzrlib/tests/test_hooks.py'
--- bzrlib/tests/test_hooks.py	2011-03-30 13:03:08 +0000
+++ bzrlib/tests/test_hooks.py	2011-04-02 00:14:32 +0000
@@ -114,6 +114,43 @@
         hooks.install_named_hook('set_rh', None, "demo")
         self.assertEqual("demo", hooks.get_hook_name(None))
 
+    def test_uninstall_named_hook(self):
+        hooks = Hooks("bzrlib.tests.test_hooks", "some_hooks")
+        hooks.add_hook('set_rh', "Set revision history", (2, 0))
+        hooks.install_named_hook('set_rh', None, "demo")
+        self.assertEqual(1, len(hooks["set_rh"]))
+        hooks.uninstall_named_hook("set_rh", "demo")
+        self.assertEqual(0, len(hooks["set_rh"]))
+
+    def test_uninstall_multiple_named_hooks(self):
+        # Multiple callbacks with the same label all get removed
+        hooks = Hooks("bzrlib.tests.test_hooks", "some_hooks")
+        hooks.add_hook('set_rh', "Set revision history", (2, 0))
+        hooks.install_named_hook('set_rh', 1, "demo")
+        hooks.install_named_hook('set_rh', 2, "demo")
+        hooks.install_named_hook('set_rh', 3, "othername")
+        self.assertEqual(3, len(hooks["set_rh"]))
+        hooks.uninstall_named_hook("set_rh", "demo")
+        self.assertEqual(1, len(hooks["set_rh"]))
+
+    def test_uninstall_named_hook_unknown_callable(self):
+        hooks = Hooks("bzrlib.tests.test_hooks", "some_hooks")
+        hooks.add_hook('set_rh', "Set revision hsitory", (2, 0))
+        self.assertRaises(KeyError, hooks.uninstall_named_hook, "set_rh",
+            "demo")
+
+    def test_uninstall_named_hook_raises_unknown_hook(self):
+        hooks = Hooks("bzrlib.tests.test_hooks", "some_hooks")
+        self.assertRaises(errors.UnknownHook, hooks.uninstall_named_hook,
+            'silly', "")
+
+    def test_uninstall_named_hook_old_style(self):
+        hooks = Hooks("bzrlib.tests.test_hooks", "some_hooks")
+        hooks["set_rh"] = []
+        hooks.install_named_hook('set_rh', None, "demo")
+        self.assertRaises(errors.UnsupportedOperation,
+            hooks.uninstall_named_hook, "set_rh", "demo")
+
     hooks = Hooks("bzrlib.tests.test_hooks", "TestHooks.hooks")
 
     def test_install_lazy_named_hook(self):
@@ -202,6 +239,19 @@
             "my callback")
         self.assertEqual([TestHook.lazy_callback], list(hook))
 
+    def test_uninstall(self):
+        hook = HookPoint("foo", "no docs", None, None)
+        hook.hook_lazy(
+            "bzrlib.tests.test_hooks", "TestHook.lazy_callback",
+            "my callback")
+        self.assertEqual([TestHook.lazy_callback], list(hook))
+        hook.uninstall("my callback")
+        self.assertEqual([], list(hook))
+
+    def test_uninstall_unknown(self):
+        hook = HookPoint("foo", "no docs", None, None)
+        self.assertRaises(KeyError, hook.uninstall, "my callback")
+
     def test___repr(self):
         # The repr should list all the callbacks, with names.
         hook = HookPoint("foo", "no docs", None, None)

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-04-01 01:46:42 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-04-02 00:14:32 +0000
@@ -65,6 +65,8 @@
   mysteriously silent.)
   (Martin Pool)
 
+* New method ``Hooks.uninstall_named_hook``. (Jelmer Vernooij, #301472)
+
 Internals
 *********