Merge lp:~jelmer/bzr/pre_post_command_hooks into lp:bzr

[Jelmer Vernooij]

Add pre_command and post_command hooks.

This branch introduces two new command hooks, 'pre_command' and 'post_command', that are triggered prior and after a command is run.

Both hooks are handed the command object that is being run.

This is split out from a branch by Brian, which also added support for suppression exceptions.

[Martin Pool (mbp)]

That looks good, thanks.

[fix] This should be mentioned in news and whatsnew.

[idea] Perhaps the whole "call hooks, log failures" should be split
out to a separate function, in hooks.py, since it's called twice here
and perhaps also in other places.

[query] This will mean that if the hooks fail, they will fail totally
silently. I can see how we would want to perhaps not make the command
fail entirely, especially as the hooks may come from out-of-date
plugins, but total silence seems too far the other way. Perhaps you
can give the user at least a single-line warning "pre_command hook %r
failed: %r"?

[Jelmer Vernooij (jelmer)]

Thanks for working on this. I'll echo Martin's comments, but have two additional ones as well:

I'd rather see the hooks take care of handling exceptions, and logging exceptions quietly if they want to.

You might want to use cmd.outf.write() rather than "print", since IIRC print writes to the actual stdout during selftest.

[Brian de Alwis (slyguy)]

Added mention to the news.
Moved hook execution to new method in HookPoint.run(), that logs hook failure with trace.warning().
Added tests for HookPoint.run()

And to answer Jelmer's points:

> I'd rather see the hooks take care of handling exceptions, and
> logging exceptions quietly if they want to.

My concern, along with all hooks, is that errors in the hooks shouldn't interfere with the actual operations.

> You might want to use cmd.outf.write() rather than "print", since IIRC
> print writes to the actual stdout during selftest.

Oops — those prints weren't supposed to be in there at all.

[Jelmer Vernooij (jelmer)]

On 07/11/11 14:41, Brian de Alwis wrote:
> Added mention to the news.
> Moved hook execution to new method in HookPoint.run(), that logs hook
failure with trace.warning().
> Added tests for HookPoint.run()
>
> And to answer Jelmer's points:
>
>> I'd rather see the hooks take care of handling exceptions, and
>> logging exceptions quietly if they want to.
>
> My concern, along with all hooks, is that errors in the hooks shouldn't
interfere with the actual operations.
In some situations it may be required that hooks are run, since not
running them would break other things. E.g. what if I wanted to add a
hook to "bzr commit" that raises BzrCommandError when there were certain
files in my working tree that should prevent committing? If there was
some bug in my hook I would rather have bzr dump a stracktrace rather
than silently discarding the error and letting me commit.

I'd rather leave the decision of whether failure is fatal or not to the
hook itself.

Cheers,

Jelmer

[Brian de Alwis (slyguy)]

That's a good point Jelmer. Perhaps the hook-definer could specifically enable a set of allowed exceptions?

[Brian de Alwis (slyguy)]

Oh, I forgot to add: my preference is to catch and "suppress" exceptions from hooks: there's nothing worse than having a bzr command throw an exception and look like it's failed, to only find that it's actually a harmless error from a hook. This has happened to me a few times when branching, pushing, and pulling.

[Vincent Ladeuil (vila)]

@Brian: I agree with jelmer, in the general case (all over the bzr code base) we don't catch exceptions preventively. Catching an exception should always be the "exception", not the norm.

If a hook *can* harmlessly fail then it's up to this hook to catch the exception(s). Having a bzr command involve failing hooks and still succeed is likely to mask bugs which can be serious. If such exceptions are expected, they should be handled specifically, it's likely that many "harmless" exceptions will require a specific handling too.

Registering "allowed" exceptions... I've used that for our test servers but that was a for a very specific case: exceptions related to sockets being shut down and inherently required because they could happen in the client or the server under circumstances outside of our control.

But even with this case, I'm not convinced it's worth generalising the practice. I seem to recall that C++ went this route at one time and a lot of people complained (it has a lot of scalability issues).

In a related spirit, I don't quite get the point about passing an exception to the post hook, this smells like a recipe for bad juju:
1) If the command fails and raises an exception, something bad has happened and the command probably encountered an error, the hook should not even be called in this case,
2) I don't want to make exceptions that a command can raise part of the command API (and passing them to the hook makes them part of the API). We claim to maintain API compatibility and I don't even want to start thinking about maintaining compatibility for exceptions and even less about how we would deprecate them ;)

These hooks are very valuable, let's keep them easy to use.

Also, _setup_run use a hack because we couldn't use 'with' at the time it was introduced, I'd rather use it now that duplicating the hack.

I had to re-read _hook_run twice before realising you were not calling run() there. I then understood why:

8 + self._hook_run()
9 self._setup_run()

was also confusing (that's because hacks are confusing ;).

In a nutshell: hooks, so far, enforce almost no API nor behaviour. As a consequence, we've been able to explore very different use cases for them (have a look at Merger.hooks['merge_file_content'] in merge.py in _compute_transform and _do_merge_contents for a use case which will probably *never* use HookPoint.run()).

I'm very hesitant about restricting the hook usages without more obvious (and uncontroversial) benefits.

I hope I'm not sounding to negative here, I really think we need these hooks.

review: Needs Fixing

[Vincent Ladeuil (vila)]

Hmm, thinking more about it.

If HookPoint.run() was named run_ignoring_exceptions() and we have enough use cases to triangulate (at least 3 then, excluding tests ;), I may change my mind.

More than five already existing use cases and I may even agree ;0)

I still think post_hook shouldn't be called if the command raises an exception though (one more reason being that the hooks will have no guarantee about what is available to them).

Ping me on IRC tomorrow (UTC time) if you want to discuss this proposal more interactively.

[Martin Pool (mbp)]

> Perhaps the hook-definer could specifically enable a set of allowed exceptions?

Well, there is already a mechanism for them to do that, which is to just catch the exceptions they think are non-fatal. Though perhaps we want to centralize this to make it more consistent across hook functions than it would be if they all did their own thing.

One way to handle this is to have a hook-calling function that checks a (global?) config variable as to what to do with exceptions. By default I think it should let them propagate up, but you could give options to just print them or to squash them entirely. I wouldn't encourage people to set that permanently because the exception may indicate a serious problem, but at least this would give them an escape if the warning is known to be just noise.

[Jelmer Vernooij (jelmer)]

On 07/11/11 17:02, Brian de Alwis wrote:
> That's a good point Jelmer. Perhaps the hook-definer could specifically enable a set of allowed exceptions?
Something like that would make sense. Perhaps this should be a different
merge proposal though?

The pre/post command hooks seem pretty uncontroversial and the code and
tests look good, so let's land that and then see if there are ways in
which exceptions from hooks can be handled.

Cheers,

Jelmer

[Martin Pool (mbp)]

> The pre/post command hooks seem pretty uncontroversial and the code and
> tests look good, so let's land that and then see if there are ways in
> which exceptions from hooks can be handled.

+1

[Martin Pool (mbp)]

Hi,

I like the way you separated out the generic run methods.

[tweak] I would suggest renaming .run(), which is a bit generic especially right next to Command.run() into I don't know perhaps Hook.run_all() or Hook.fire().

[query] I see you're still proceeding to run the post-command hooks when one of the pre-command hooks fails, and Hook.run() is running all of the hooks even if one fails. I think we really need to get to grips with this question.

A- I agree it is annoying when a plugin causes a mysterious failure.

B- On the other hand just blindly continuing on after an exception can have bad consequences: ^C is ignored, or the knock-on error masks the real problem (eg 'TooManyConcurrentRequests'), or in the worst case the program ploughs into the ground because of retrying operations that can't safely be retried. I think generally speaking our bias is now to just stop when something goes wrong, but I do recognize A.

I can think of a few options:

- add a configuration option to continue after errors (though, by putting the work on the user this is not really a great solution)
- whitelist some particular exceptions - for me the most obvious would be AttributeError which almost always just means some code is out of date and it can be fairly safely ignored

What do you think?

[Jelmer Vernooij (jelmer)]

Hi Brian,

Are you still working on this?

[Brian de Alwis (slyguy)]

Sorry Jelmer, but I haven't had much time to work on it further. I'll have some time after Dec 20.

I think it sounds best to:

  * rename Hooks.run() to Hooks.fire() that returns a boolean. If True, then all hooks passed with no exceptions raised. If False, then one or more hooks raised an exception.

  * add a boolean "suppress_exceptions" to Hooks that defaults to false and, if true, provides the behaviour I've currently implemented

  * add a list "passed_exceptions" to contain a list of exceptions that should be passed-through when suppress_exceptions = True (e.g, BzrCommandError)

suppress_exceptions and passed_exceptions are configured at hook definition time; the creator of the hook knows best what to allow.

If you really think AttributeError should be ignorable, we could also add one more field:

  * add a list "ignored_exceptions" to contain a list of exceptions that not affect the return result of Hooks.fire(). The exception will still be logged.

[Jelmer Vernooij (jelmer)]

On 12/22/2011 11:49 PM, Brian de Alwis wrote:
> Brian de Alwis has proposed merging
lp:~slyguy/bzr/pre_post_command_hooks into lp:bzr.
>
> Requested reviews:
> Vincent Ladeuil (vila)
>
> For more details, see:
> https://code.launchpad.net/~slyguy/bzr/pre_post_command_hooks/+merge/86763
>
> This branch introduces two new command hooks, 'pre_command' and
'post_command', that are triggered prior and after a command is run.
>
> pre_command is provided the command object. Any pre_command hook can
abort the command by raising BzrCommandError.
>
> post_command is provided the command object and any exception raised
from the execution of the command. post_command hooks cannot affect the
raising of the exception.
>
> HookPoint can be configured to suppress exceptions when the hooks are
fired, and can also mark certain exception classes as to be passed
through. Any exceptions raised when triggering the hooks are quietly logged.
Hi Brian,

This seems to add the ability to suppress or not suppress exceptions per
hookpoint. Per our prior discussion, I really think this should be
per-hook, as some hooks are more important than others. I.e. wh

Cheers,

Jelmer

[Brian de Alwis (slyguy)]

Hi Jelmer.

On 2011-12-22, at 6:13 PM, Jelmer Vernooij <email address hidden> wrote:
> This seems to add the ability to suppress or not suppress exceptions per hookpoint. Per our prior discussion, I really think this should be
> per-hook, as some hooks are more important than others. I.e. wh

Your message was cut off — I'd really like to understand your use case.

But I should first point out that this mechanism is opt-in and does not change any existing behaviour. Hook providers are free to continue passing through all exceptions. The change simplifies life for hook points that want more control.

If a hook point wants to be able to raise other exceptions, that changes the definition of the hook as the hook firing points may require adaptation.

Or put another way, hook points define an API where the definer undertakes to notify hooks at certain principled points during the execution. For a hook to unilaterally expand the exception pass-through is a change of API that the definer was programmed against.

If a hook needs different handling, perhaps it indicates a need for several hook pints, each with different semantics.

Brian.

[Jelmer Vernooij (jelmer)]

Hi Brian,

On 12/23/2011 01:22 PM, Brian de Alwis wrote:
> On 2011-12-22, at 6:13 PM, Jelmer Vernooij <email address hidden> wrote:
>> This seems to add the ability to suppress or not suppress exceptions
per hookpoint. Per our prior discussion, I really think this should be
>> per-hook, as some hooks are more important than others. I.e. wh
>
> Your message was cut off — I'd really like to understand your use case.
>
> But I should first point out that this mechanism is opt-in and does not
change any existing behaviour. Hook providers are free to continue
passing through all exceptions. The change simplifies life for hook
points that want more control.
Whether we do or don't want to catch exceptions depends on the hook I
think, not on the hook point. I.e. it's fine to ignore exceptions in
hooks that are purely informational, like pre commit hooks that submit
to cia.vc. On the other hand, you could also have a pre commit hook that
ran the testsuite and prevented commits unless the test suite passed.
Unknown exceptions in the first hook are fine. In the second hook,
they're a problem - and they should result in a traceback.

Likewise, I can imagine people wanting to have some mandatory
pre_command_hooks which aren't simply ignored if they have problems.
> If a hook point wants to be able to raise other exceptions, that
changes the definition of the hook as the hook firing points may require
adaptation.
If it wants to raise exceptions that are caught in particular way, then
that does indeed change the definition the hook a bit. Exceptions that
aren't caught are perfectly valid too, though - to indicate serious
issues in the plugin. It might be fine for those exceptions to be
ignored in some situations, but if that is the case depends on the hook.

In other words, I would imagine something like this:

Branch.hooks['post_branch_tip_change'].install_hook(report_tip_change,
"tip change", suppress_exceptions=ue)
>
>
> Or put another way, hook points define an API where the definer
undertakes to notify hooks at certain principled points during the
execution. For a hook to unilaterally expand the exception pass-through
is a change of API that the definer was programmed against.
>
> If a hook needs different handling, perhaps it indicates a need for
several hook pints, each with different semantics.
I think this is potentially true for almost all hook points. Duplicating
the number of hook points we have doesn't seem useful.

Cheers,

Jelmer

[Brian de Alwis (slyguy)]

> Whether we do or don't want to catch exceptions depends on the hook I
> think, not on the hook point. I.e. it's fine to ignore exceptions in
> hooks that are purely informational, like pre commit hooks that submit
> to cia.vc. On the other hand, you could also have a pre commit hook that
> ran the testsuite and prevented commits unless the test suite passed.
> Unknown exceptions in the first hook are fine. In the second hook,
> they're a problem - and they should result in a traceback.

I disagree — I believe it should be the defined of the hook point that
decides whether the hooks are informational vs advisory. So in your
second case, I think that outcome should be communicated by a known
exception, like BzrAbortCommitException.

As a committer you have the final say, of course, and I've run out of
time to work on this further anyways. Feel free to modify the patch
as you see fit!

Brian.

[Jelmer Vernooij (jelmer)]

On 12/24/2011 04:55 AM, Brian de Alwis wrote:
>> Whether we do or don't want to catch exceptions depends on the hook I
>> think, not on the hook point. I.e. it's fine to ignore exceptions in
>> hooks that are purely informational, like pre commit hooks that submit
>> to cia.vc. On the other hand, you could also have a pre commit hook that
>> ran the testsuite and prevented commits unless the test suite passed.
>> Unknown exceptions in the first hook are fine. In the second hook,
>> they're a problem - and they should result in a traceback.
>
> I disagree — I believe it should be the defined of the hook point that
> decides whether the hooks are informational vs advisory. So in your
> second case, I think that outcome should be communicated by a known
> exception, like BzrAbortCommitException.
I agree that in normal case it should raise some well defined exception.

What I mean is, if there's a bug in the commit hook that makes it raise
an unexpected exception (e.g. SyntaxError), then that exception should
be raised all the way through and not be caught and ignored. If such an
exception is ignored, then that can mean a policy that is enforced by
the plugin hook is ignored.

> As a committer you have the final say, of course, and I've run out of
> time to work on this further anyways. Feel free to modify the patch
> as you see fit!
Sorry, I didn't mean to be discouraging. :-/ I'll see if we can at least
land the pre_command / post_command bit of your branch.

Cheers,

Jelmer

[Vincent Ladeuil (vila)]

Good to land, sorry for the delay.

news^W don't forget details ;)

review: Approve

[Jelmer Vernooij (jelmer)]

sent to pqm by email

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2012-01-04 17:12:42 +0000
+++ bzrlib/branch.py	2012-01-18 19:52:54 +0000
@@ -2038,6 +2038,8 @@
         :param name: Name of colocated branch to create, if any
         :return: a branch in this format
         """
+        if name is None:
+            name = a_bzrdir._get_selected_branch()
         mutter('creating branch %r in %s', self, a_bzrdir.user_url)
         branch_transport = a_bzrdir.get_branch_transport(self, name=name)
         control_files = lockable_files.LockableFiles(branch_transport,
@@ -2060,6 +2062,8 @@
     def open(self, a_bzrdir, name=None, _found=False, ignore_fallbacks=False,
             found_repository=None, possible_transports=None):
         """See BranchFormat.open()."""
+        if name is None:
+            name = a_bzrdir._get_selected_branch()
         if not _found:
             format = BranchFormatMetadir.find_format(a_bzrdir, name=name)
             if format.__class__ != self.__class__:
@@ -2305,12 +2309,13 @@
         mutter('creating branch reference in %s', a_bzrdir.user_url)
         if a_bzrdir._format.fixed_components:
             raise errors.IncompatibleFormat(self, a_bzrdir._format)
+        if name is None:
+            name = a_bzrdir._get_selected_branch()
         branch_transport = a_bzrdir.get_branch_transport(self, name=name)
         branch_transport.put_bytes('location',
             target_branch.user_url)
         branch_transport.put_bytes('format', self.as_string())
-        branch = self.open(
-            a_bzrdir, name, _found=True,
+        branch = self.open(a_bzrdir, name, _found=True,
             possible_transports=[target_branch.bzrdir.root_transport])
         self._run_post_branch_init_hooks(a_bzrdir, name, branch)
         return branch
@@ -2342,6 +2347,8 @@
             a_bzrdir.
         :param possible_transports: An optional reusable transports list.
         """
+        if name is None:
+            name = a_bzrdir._get_selected_branch()
         if not _found:
             format = BranchFormatMetadir.find_format(a_bzrdir, name=name)
             if format.__class__ != self.__class__:
@@ -2351,8 +2358,7 @@
             location = self.get_reference(a_bzrdir, name)
         real_bzrdir = controldir.ControlDir.open(
             location, possible_transports=possible_transports)
-        result = real_bzrdir.open_branch(name=name, 
-            ignore_fallbacks=ignore_fallbacks,
+        result = real_bzrdir.open_branch(ignore_fallbacks=ignore_fallbacks,
             possible_transports=possible_transports)
         # this changes the behaviour of result.clone to create a new reference
         # rather than a copy of the content of the branch.
@@ -2445,10 +2451,11 @@
         """Create new branch object at a particular location."""
         if a_bzrdir is None:
             raise ValueError('a_bzrdir must be supplied')
-        else:
-            self.bzrdir = a_bzrdir
+        if name is None:
+            raise ValueError('name must be supplied')
+        self.bzrdir = a_bzrdir
         self._user_transport = self.bzrdir.transport.clone('..')
-        if name is not None:
+        if name != "":
             self._user_transport.set_segment_parameter(
                 "branch", urlutils.escape(name))
         self._base = self._user_transport.base

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2012-01-16 14:45:48 +0000
+++ bzrlib/builtins.py	2012-01-18 19:52:54 +0000
@@ -1444,13 +1444,13 @@
         else:
             dir = controldir.ControlDir.open_containing(location)[0]
             try:
-                active_branch = dir.open_branch(name=None)
+                active_branch = dir.open_branch(name="")
             except errors.NotBranchError:
                 active_branch = None
             branches = dir.get_branches()
             names = {}
             for name, branch in branches.iteritems():
-                if name is None:
+                if name == "":
                     continue
                 active = (active_branch is not None and
                           active_branch.base == branch.base)

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2011-12-22 19:54:56 +0000
+++ bzrlib/bzrdir.py	2012-01-18 19:52:54 +0000
@@ -829,7 +829,9 @@
 
     def destroy_branch(self, name=None):
         """See BzrDir.create_branch."""
-        if name is not None:
+        if name is None:
+            name = self._get_selected_branch()
+        if name != "":
             raise errors.NoColocatedBranchSupport(self)
         self.transport.delete_tree('branch')
 
@@ -887,7 +889,9 @@
 
     def get_branch_transport(self, branch_format, name=None):
         """See BzrDir.get_branch_transport()."""
-        if name is not None:
+        if name is None:
+            name = self._get_selected_branch()
+        if name != "":
             raise errors.NoColocatedBranchSupport(self)
         # XXX: this shouldn't implicitly create the directory if it's just
         # promising to get a transport -- mbp 20090727
@@ -1021,7 +1025,7 @@
         :param name: Optional branch name to use
         :return: Relative path to branch
         """
-        if name is None:
+        if name == "":
             return 'branch'
         return urlutils.join('branches', name.encode("utf-8"))
 
@@ -1056,7 +1060,7 @@
         if name is None:
             name = self._get_selected_branch()
         path = self._get_branch_path(name)
-        if name is not None:
+        if name != "":
             self.control_files.lock_write()
             try:
                 branches = self._read_branch_list()
@@ -1073,17 +1077,19 @@
         """See ControlDir.get_branches."""
         ret = {}
         try:
-            ret[None] = self.open_branch()
+            ret[""] = self.open_branch(name="")
         except (errors.NotBranchError, errors.NoRepositoryPresent):
             pass
 
         for name in self._read_branch_list():
-            ret[name] = self.open_branch(name.decode('utf-8'))
+            ret[name] = self.open_branch(name=name.decode('utf-8'))
 
         return ret
 
     def get_branch_transport(self, branch_format, name=None):
         """See BzrDir.get_branch_transport()."""
+        if name is None:
+            name = self._get_selected_branch()
         path = self._get_branch_path(name)
         # XXX: this shouldn't implicitly create the directory if it's just
         # promising to get a transport -- mbp 20090727
@@ -1093,7 +1099,7 @@
             branch_format.get_format_string()
         except NotImplementedError:
             raise errors.IncompatibleFormat(branch_format, self._format)
-        if name is not None:
+        if name != "":
             try:
                 self.transport.mkdir('branches', mode=self._get_mkdir_mode())
             except errors.FileExists:

=== modified file 'bzrlib/commands.py'
--- bzrlib/commands.py	2011-12-18 12:46:49 +0000
+++ bzrlib/commands.py	2012-01-18 19:52:54 +0000
@@ -689,11 +689,15 @@
         """
         class_run = self.run
         def run(*args, **kwargs):
+            for hook in Command.hooks['pre_command']:
+                hook(self)
             self._operation = cleanup.OperationWithCleanups(class_run)
             try:
                 return self._operation.run_simple(*args, **kwargs)
             finally:
                 del self._operation
+                for hook in Command.hooks['post_command']:
+                    hook(self)
         self.run = run
 
     def run(self):
@@ -787,6 +791,12 @@
             " is safe to mutate - e.g. to remove a command. "
             "list_commands should return the updated set of command names.",
             (1, 17))
+        self.add_hook('pre_command',
+            "Called prior to executing a command. Called with the command "
+            "object.", (2, 5))
+        self.add_hook('post_command',
+            "Called after executing a command. Called with the command "
+            "object.", (2, 5))
 
 Command.hooks = CommandHooks()
 

=== modified file 'bzrlib/controldir.py'
--- bzrlib/controldir.py	2012-01-03 13:47:01 +0000
+++ bzrlib/controldir.py	2012-01-18 19:52:54 +0000
@@ -116,7 +116,7 @@
         :return: Dictionary mapping branch names to instances.
         """
         try:
-           return { None: self.open_branch() }
+           return { "": self.open_branch() }
         except (errors.NotBranchError, errors.NoRepositoryPresent):
            return {}
 
@@ -294,9 +294,9 @@
         :return: Name of the branch selected by the user, or None.
         """
         branch = self.root_transport.get_segment_parameters().get("branch")
-        if branch is not None:
-            branch = urlutils.unescape(branch)
-        return branch
+        if branch is None:
+            branch = ""
+        return urlutils.unescape(branch)
 
     def has_workingtree(self):
         """Tell if this controldir contains a working tree.

=== modified file 'bzrlib/mutabletree.py'
--- bzrlib/mutabletree.py	2012-01-06 14:09:04 +0000
+++ bzrlib/mutabletree.py	2012-01-18 19:52:54 +0000
@@ -520,10 +520,18 @@
             "called with a bzrlib.mutabletree.PostCommitHookParams object. "
             "The mutable tree the commit was performed on is available via "
             "the mutable_tree attribute of that object.", (2, 0))
+        self.add_hook('pre_transform',
+            "Called before a tree transform on this tree. The hook is called "
+            "with the tree that is being transformed and the transform.",
+            (2, 5))
         self.add_hook('post_build_tree',
             "Called after a completely new tree is built. The hook is "
             "called with the tree as its only argument.", (2, 5))
-
+        self.add_hook('post_transform',
+            "Called after a tree transform has been performed on a tree. "
+            "The hook is called with the tree that is being transformed and "
+            "the transform.",
+            (2, 5))
 
 # install the default hooks into the MutableTree class.
 MutableTree.hooks = MutableTreeHooks()

=== modified file 'bzrlib/plugins/weave_fmt/branch.py'
--- bzrlib/plugins/weave_fmt/branch.py	2011-12-19 13:23:58 +0000
+++ bzrlib/plugins/weave_fmt/branch.py	2012-01-18 19:52:54 +0000
@@ -138,7 +138,9 @@
     def open(self, a_bzrdir, name=None, _found=False, ignore_fallbacks=False,
             found_repository=None, possible_transports=None):
         """See BranchFormat.open()."""
-        if name is not None:
+        if name is None:
+            name = a_bzrdir._get_selected_branch()
+        if name != "":
             raise errors.NoColocatedBranchSupport(self)
         if not _found:
             # we are being called directly and must probe.

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2012-01-04 16:02:14 +0000
+++ bzrlib/remote.py	2012-01-18 19:52:54 +0000
@@ -625,6 +625,8 @@
 
     def create_branch(self, name=None, repository=None,
                       append_revisions_only=None):
+        if name is None:
+            name = self._get_selected_branch()
         # as per meta1 formats - just delegate to the format object which may
         # be parameterised.
         real_branch = self._format.get_branch_format().initialize(self,
@@ -724,6 +726,8 @@
 
     def open_branch(self, name=None, unsupported=False,
                     ignore_fallbacks=False, possible_transports=None):
+        if name is None:
+            name = self._get_selected_branch()
         if unsupported:
             raise NotImplementedError('unsupported flag support not implemented yet.')
         if self._next_open_branch_result is not None:
@@ -3102,6 +3106,8 @@
 
     def initialize(self, a_bzrdir, name=None, repository=None,
                    append_revisions_only=None):
+        if name is None:
+            name = a_bzrdir._get_selected_branch()
         # 1) get the network name to use.
         if self._custom_format:
             network_name = self._custom_format.network_name()
@@ -3122,7 +3128,7 @@
         # Creating on a remote bzr dir.
         # 2) try direct creation via RPC
         path = a_bzrdir._path_for_remote_call(a_bzrdir._client)
-        if name is not None:
+        if name != "":
             # XXX JRV20100304: Support creating colocated branches
             raise errors.NoColocatedBranchSupport(self)
         verb = 'BzrDir.create_branch'

=== modified file 'bzrlib/smart/bzrdir.py'
--- bzrlib/smart/bzrdir.py	2011-12-19 13:23:58 +0000
+++ bzrlib/smart/bzrdir.py	2012-01-18 19:52:54 +0000
@@ -268,7 +268,7 @@
             self.transport_from_client_path(path))
         format = branch.network_format_registry.get(network_name)
         bzrdir.branch_format = format
-        result = format.initialize(bzrdir)
+        result = format.initialize(bzrdir, name="")
         rich_root, tree_ref, external_lookup = self._format_to_capabilities(
             result.repository._format)
         branch_format = result._format.network_name()

=== modified file 'bzrlib/tests/per_bzrdir/test_bzrdir.py'
--- bzrlib/tests/per_bzrdir/test_bzrdir.py	2011-12-12 12:09:50 +0000
+++ bzrlib/tests/per_bzrdir/test_bzrdir.py	2012-01-18 19:52:54 +0000
@@ -693,5 +693,5 @@
             raise TestNotApplicable('Format does not support colocation')
         reference = branch.BranchReferenceFormat().initialize(
             repo.bzrdir, target_branch=target_branch)
-        self.assertEqual(set([None, 'foo']),
+        self.assertEqual(set(["", 'foo']),
                          set(repo.bzrdir.get_branches().keys()))

=== modified file 'bzrlib/tests/per_controldir/test_controldir.py'
--- bzrlib/tests/per_controldir/test_controldir.py	2011-12-27 12:18:36 +0000
+++ bzrlib/tests/per_controldir/test_controldir.py	2012-01-18 19:52:54 +0000
@@ -1195,7 +1195,7 @@
     def test_get_branches(self):
         repo = self.make_repository('branch-1')
         target_branch = repo.bzrdir.create_branch()
-        self.assertEqual([None], repo.bzrdir.get_branches().keys())
+        self.assertEqual([""], repo.bzrdir.get_branches().keys())
 
     def test_create_repository(self):
         # a bzrdir can construct a repository for itself.
@@ -1342,7 +1342,7 @@
             raise TestSkipped("Can't initialize %r on transport %r"
                               % (self.bzrdir_format, t))
         dir = bzrdir.BzrDir.open(t.base)
-        self.assertIs(None, dir._get_selected_branch())
+        self.assertEqual(u"", dir._get_selected_branch())
 
     def test_root_transport(self):
         dir = self.make_bzrdir('.')

=== modified file 'bzrlib/tests/per_controldir_colo/test_unsupported.py'
--- bzrlib/tests/per_controldir_colo/test_unsupported.py	2011-12-11 04:07:08 +0000
+++ bzrlib/tests/per_controldir_colo/test_unsupported.py	2012-01-18 19:52:54 +0000
@@ -73,4 +73,4 @@
         made_control = self.make_bzrdir_with_repo()
         made_control.create_branch()
         self.assertEqual(made_control.get_branches().keys(),
-                         [None])
+                         [""])

=== modified file 'bzrlib/tests/test_commands.py'
--- bzrlib/tests/test_commands.py	2011-12-09 14:24:21 +0000
+++ bzrlib/tests/test_commands.py	2012-01-18 19:52:54 +0000
@@ -25,6 +25,7 @@
     errors,
     option,
     tests,
+    trace,
     )
 from bzrlib.commands import display_command
 from bzrlib.tests import TestSkipped
@@ -370,3 +371,81 @@
         cmds = list(commands.all_command_names())
         self.assertEqual(['called'], hook_calls)
         self.assertSubset(['foo', 'bar'], cmds)
+
+class TestPreAndPostCommandHooks(tests.TestCase):
+    class TestError(StandardError):
+        __doc__ = """A test exception."""
+
+    def test_pre_and_post_hooks(self):
+        hook_calls = []
+
+        def pre_command(cmd):
+            self.assertEqual([], hook_calls)
+            hook_calls.append('pre')
+
+        def post_command(cmd):
+            self.assertEqual(['pre', 'run'], hook_calls)
+            hook_calls.append('post')
+
+        def run(cmd):
+            self.assertEqual(['pre'], hook_calls)
+            hook_calls.append('run')
+
+        self.overrideAttr(builtins.cmd_rocks, 'run', run)
+        commands.install_bzr_command_hooks()
+        commands.Command.hooks.install_named_hook(
+            "pre_command", pre_command, None)
+        commands.Command.hooks.install_named_hook(
+            "post_command", post_command, None)
+
+        self.assertEqual([], hook_calls)
+        self.run_bzr(['rocks', '-Oxx=12', '-Oyy=foo'])
+        self.assertEqual(['pre', 'run', 'post'], hook_calls)
+
+    def test_post_hook_provided_exception(self):
+        hook_calls = []
+
+        def post_command(cmd):
+            hook_calls.append('post')
+
+        def run(cmd):
+            hook_calls.append('run')
+            raise self.TestError()
+
+        self.overrideAttr(builtins.cmd_rocks, 'run', run)
+        commands.install_bzr_command_hooks()
+        commands.Command.hooks.install_named_hook(
+            "post_command", post_command, None)
+
+        self.assertEqual([], hook_calls)
+        self.assertRaises(self.TestError, commands.run_bzr, [u'rocks'])
+        self.assertEqual(['run', 'post'], hook_calls)
+
+    def test_pre_command_error(self):
+        """Ensure an BzrCommandError in pre_command aborts the command"""
+
+        hook_calls = []
+
+        def pre_command(cmd):
+            hook_calls.append('pre')
+            # verify that all subclasses of BzrCommandError caught too
+            raise errors.BzrOptionError()
+
+        def post_command(cmd, e):
+            self.fail('post_command should not be called')
+
+        def run(cmd):
+            self.fail('command should not be called')
+
+        self.overrideAttr(builtins.cmd_rocks, 'run', run)
+        commands.install_bzr_command_hooks()
+        commands.Command.hooks.install_named_hook(
+            "pre_command", pre_command, None)
+        commands.Command.hooks.install_named_hook(
+            "post_command", post_command, None)
+
+        self.assertEqual([], hook_calls)
+        self.assertRaises(errors.BzrCommandError,
+                          commands.run_bzr, [u'rocks'])
+        self.assertEqual(['pre'], hook_calls)
+

=== modified file 'bzrlib/tests/test_foreign.py'
--- bzrlib/tests/test_foreign.py	2011-12-19 19:15:58 +0000
+++ bzrlib/tests/test_foreign.py	2012-01-18 19:52:54 +0000
@@ -240,6 +240,8 @@
 
     def open(self, a_bzrdir, name=None, _found=False, ignore_fallbacks=False,
             found_repository=None):
+        if name is None:
+            name = a_bzrdir._get_selected_branch()
         if not _found:
             raise NotImplementedError
         try:
@@ -251,7 +253,8 @@
             return DummyForeignVcsBranch(_format=self,
                               _control_files=control_files,
                               a_bzrdir=a_bzrdir,
-                              _repository=found_repository)
+                              _repository=found_repository,
+                              name=name)
         except errors.NoSuchFile:
             raise errors.NotBranchError(path=transport.base)
 
@@ -317,7 +320,9 @@
 
     def open_branch(self, name=None, unsupported=False, ignore_fallbacks=True,
             possible_transports=None):
-        if name is not None:
+        if name is None:
+            name = self._get_selected_branch()
+        if name != "":
             raise errors.NoColocatedBranchSupport(self)
         return self._format.get_branch_format().open(self, _found=True)
 

=== modified file 'bzrlib/tests/test_transform.py'
--- bzrlib/tests/test_transform.py	2011-11-17 13:45:49 +0000
+++ bzrlib/tests/test_transform.py	2012-01-18 19:52:54 +0000
@@ -60,6 +60,7 @@
     pathjoin,
 )
 from bzrlib.merge import Merge3Merger, Merger
+from bzrlib.mutabletree import MutableTree
 from bzrlib.tests import (
     features,
     TestCaseInTempDir,
@@ -3717,3 +3718,40 @@
                          remaining_conflicts.pop())
         self.assertLength(1, warnings)
         self.assertStartsWith(warnings[0], 'donttouchmypreciouuus')
+
+
+class TestTransformHooks(tests.TestCaseWithTransport):
+
+    def setUp(self):
+        super(TestTransformHooks, self).setUp()
+        self.wt = self.make_branch_and_tree('.')
+        os.chdir('..')
+
+    def get_transform(self):
+        transform = TreeTransform(self.wt)
+        self.addCleanup(transform.finalize)
+        return transform, transform.root
+
+    def test_pre_commit_hooks(self):
+        calls = []
+        def record_pre_transform(tree, tt):
+            calls.append((tree, tt))
+        MutableTree.hooks.install_named_hook('pre_transform',
+            record_pre_transform, "Pre transform")
+        transform, root = self.get_transform()
+        old_root_id = transform.tree_file_id(root)
+        transform.apply()
+        self.assertEqual(old_root_id, self.wt.get_root_id())
+        self.assertEquals([(self.wt, transform)], calls)
+
+    def test_post_commit_hooks(self):
+        calls = []
+        def record_post_transform(tree, tt):
+            calls.append((tree, tt))
+        MutableTree.hooks.install_named_hook('post_transform',
+            record_post_transform, "Post transform")
+        transform, root = self.get_transform()
+        old_root_id = transform.tree_file_id(root)
+        transform.apply()
+        self.assertEqual(old_root_id, self.wt.get_root_id())
+        self.assertEquals([(self.wt, transform)], calls)

=== modified file 'bzrlib/transform.py'
--- bzrlib/transform.py	2011-12-19 17:39:35 +0000
+++ bzrlib/transform.py	2012-01-18 19:52:54 +0000
@@ -50,6 +50,7 @@
                            ExistingLimbo, ImmortalLimbo, NoFinalPath,
                            UnableCreateSymlink)
 from bzrlib.filters import filtered_output_bytes, ContentFilterContext
+from bzrlib.mutabletree import MutableTree
 from bzrlib.osutils import (
     delete_any,
     file_kind,
@@ -57,7 +58,6 @@
     pathjoin,
     sha_file,
     splitpath,
-    supports_executable,
     )
 from bzrlib.progress import ProgressPhase
 from bzrlib.symbol_versioning import (
@@ -156,6 +156,8 @@
         """
         if self._tree is None:
             return
+        for hook in MutableTree.hooks['post_transform']:
+            hook(self._tree, self)
         self._tree.unlock()
         self._tree = None
 
@@ -230,7 +232,7 @@
         irrelevant.
 
         """
-        new_roots = [k for k, v in self._new_parent.iteritems() if v is
+        new_roots = [k for k, v in self._new_parent.iteritems() if v ==
                      ROOT_PARENT]
         if len(new_roots) < 1:
             return
@@ -626,7 +628,7 @@
         for trans_id in self._new_parent:
             seen = set()
             parent_id = trans_id
-            while parent_id is not ROOT_PARENT:
+            while parent_id != ROOT_PARENT:
                 seen.add(parent_id)
                 try:
                     parent_id = self.final_parent(parent_id)
@@ -642,7 +644,7 @@
         """If parent directories are versioned, children must be versioned."""
         conflicts = []
         for parent_id, children in by_parent.iteritems():
-            if parent_id is ROOT_PARENT:
+            if parent_id == ROOT_PARENT:
                 continue
             if self.final_file_id(parent_id) is not None:
                 continue
@@ -741,7 +743,7 @@
         """Children must have a directory parent"""
         conflicts = []
         for parent_id, children in by_parent.iteritems():
-            if parent_id is ROOT_PARENT:
+            if parent_id == ROOT_PARENT:
                 continue
             no_children = True
             for child_id in children:
@@ -1722,6 +1724,8 @@
             calculating one.
         :param _mover: Supply an alternate FileMover, for testing
         """
+        for hook in MutableTree.hooks['pre_transform']:
+            hook(self._tree, self)
         if not no_conflicts:
             self._check_malformed()
         child_pb = ui.ui_factory.nested_progress_bar()

=== modified file 'doc/en/admin-guide/migration.txt'
--- doc/en/admin-guide/migration.txt	2010-08-13 19:08:57 +0000
+++ doc/en/admin-guide/migration.txt	2012-01-18 19:52:54 +0000
@@ -47,7 +47,7 @@
 Bazaar's `svn`_ plugin provides tools for interaction with Subversion
 projects.  In fact, Bazaar can be used transparently with projects stored in
 Subversion, but that is beyond the scope of this document.  (See
-http://doc.bazaar.canonical.com/en/migration/foreign/bzr-on-svn-projects.html for
+http://doc.bazaar.canonical.com/migration/en/foreign/bzr-on-svn-projects.html for
 more on that subject.)  What is relevant here is the ``svn-import`` command
 provided by that plugin.  This can import an entire subversion repository
 including tags and branches, particularly if they are stored in Subversion's

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2012-01-16 17:24:42 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2012-01-18 19:52:54 +0000
@@ -26,12 +26,19 @@
 .. Improvements to existing commands, especially improved performance 
    or memory usage, or better results.
 
+* Two new command hooks, ``pre_command`` and ``post_command``,
+  provide notification before and after a command has been run.
+  (Brian de Alwis, Jelmer Vernooij)
+
 Bug Fixes
 *********
 
 .. Fixes for situations where bzr would previously crash or give incorrect
    or undesirable results.
 
+* Test for equality instead of object identity where ROOT_PARENT is concerned.
+  (Wouter van Heyst, #881142)
+
 Documentation
 *************
 
@@ -49,6 +56,10 @@
 .. Major internal changes, unlikely to be visible to users or plugin 
    developers, but interesting for bzr developers.
 
+* ``MutableTree`` has two new hooks ``pre_transform`` and
+  ``post_transform`` that are called for tree transform operations.
+  (Jelmer Vernooij, #912084)
+
 Testing
 *******