[MERGE] Hook documentation

[Andrew Bennetts]

Michael Ernst wrote:
[...]
> This patch improves the User Guide's documentation of hooks.
> 
> The text no longer intersperses discussion of what a user does with a
> descripton of the code.  Nor does it confusingly use first person to
> describe what the code does.  It puts two lists of hooks together in one
> place.  It fixes bugs in hook examples (per
> https://lists.ubuntu.com/archives/bazaar/2008q3/047498.html).  It clarifies
> where each hook is run (on the client, the server, or both).

Thanks for this!  I have a few comments, but overall I'm extremely happy to see
this patch.

bb:tweak

> # Bazaar merge directive format 2 (Bazaar 0.90)
> # revision_id: mernst at csail.mit.edu-20081020065324-8oz1mxqa5p0zkdss
> # target_branch: file:///DS/home-0/mernst/bin/install/bzr/bzr.dev/
> # testament_sha1: c07c876785dde8584fa150e6885acf919bfd6ef5
> # timestamp: 2008-10-20 08:54:25 +0200
> # base_revision_id: pqm at pqm.ubuntu.com-20081020020359-7f8c4hviijt1m5vq
> # 
> # Begin patch
> === modified file 'bzrlib/help_topics/en/hooks.txt'
> --- bzrlib/help_topics/en/hooks.txt	2008-09-23 23:45:04 +0000
> +++ bzrlib/help_topics/en/hooks.txt	2008-10-20 06:53:24 +0000
> @@ -12,19 +12,35 @@
>  
>  .. _Using hooks: ../user-guide/index.html#using-hooks
>  
> -The class of each hook is given immediately after each hook type below.
> +The class of each hook is given in parentheses immediately after each hook
> +type below.
> +
> +Each description also indicates whether the hook runs on the client (the
> +machine with the child branch) or the server (the machine with the parent
> +branch).  These may be, but are not necessarily, the same machine.

“child” and “parent” aren't quite right as labels for these branches.  Sometimes
the server has a branch that is more accurately called a “mirror” or “public”
branch.

The best alternative suggestion I can think of is “local” and “remote”, which
are fairly self-evident abbreviations of “client-side” and “server-side” if you
assume that the user/reader is looking at things from the client's side, rather
than the server's.

(Another alternative that has to be ruled out “source” and “target”: the
fundamental symmetry in the way bzr works on branches makes these terms
ambiguous here.)

> +Plugins (including hooks) are run on the server if any of these is true:
> +
> +  * the server is the same machine as the client.
> +
> +  * the connection is via the smart server (accessed with a URL starting
> +    with "bzr") and you load the plugins in the wsgi handler script.
> +
> +  * the connection is via ssh.

A connection over SSH (that is, via a bzr+ssh:// URL) *is* via a smart server.
Also, WSGI handler scripts are specific to deployment of an HTTP smart server,
i.e. they don't apply to bzr:// URLs, although your wording implies that.

Also, the caveat about plugins being enabled is really true for both the client
and the server, and in recent bzr versions the wsgi handler script will by
default load plugins (see the new load_plugins=True argument to
bzrlib.transport.wsgi.make_app).  So I don't think WSGI scripts need to be
singled out for special treatment here, although the point that plugins need to
be enabled is worth making.

So I'd rewrite these bullets as:

  * the remote branch is on the same machine as the client, and the client has
    plugins enabled.

  * the connection is via a smart server (accessed with a URL starting with
    “bzr://”, “bzr+ssh://” or “bzr+http://”, or accessed via a “http://” URL
    when a smart server is available via HTTP), and that server has plugins
    enabled.

It's a bit verbose, but precise.  If someone can suggest a more concise phrasing
that keeps the precision, that would be great :)

[...]
> @@ -115,6 +137,7 @@
>  -------------------------------
>  
>  Run before a branch tip has been changed, while the branch is write-locked.
> +Runs on the server.
>  Note that push, pull, commit and uncommit all invoke this hook.
>  
>  The hook signature is (params), where params is an object containing

The pre_change_branch_tip hook runs on both the client and the server, doesn't
it?

> @@ -143,7 +166,9 @@
>  -------------------------------
>  
>  Run after a branch tip has been changed but while the branch is still
> -write-locked. Note that push, pull, commit and uncommit all invoke this hook.
> +write-locked.
> +Runs on the server.
> +Note that push, pull, commit and uncommit all invoke this hook.
>  
>  The hook signature is (params), where params is an object containing
>  the members

The post_change_branch_tip hook runs on both the client and the server, doesn't
it?

> === modified file 'doc/en/user-guide/hooks.txt'
> --- doc/en/user-guide/hooks.txt	2008-05-07 06:49:50 +0000
> +++ doc/en/user-guide/hooks.txt	2008-10-20 06:53:24 +0000
> @@ -7,6 +7,14 @@
>  One way to customize Bazaar's behaviour is with *hooks*.  Hooks allow you to
>  perform actions before or after certain Bazaar operations.  The operations
>  include ``commit``, ``push``, ``pull``, and ``uncommit``.
> +For a complete list of hooks and their parameters, see `Hooks
> +<../user-reference/bzr_man.html#hooks>`_ in the User Reference.
> +
> +Most hooks are run on the client, but a few are run on the server.  (Also
> +see the `bzr-push-and-update`_ plugin that handles one special case of
> +server-side operations.)
> +
> +.. _bzr-push-and-update: https://launchpad.net/bzr-push-and-update/

Good idea to mention push-and-update here.  Thanks!

[...]
> +The plugin code does two things.  First, it defines a function that will be
> +run after ``push`` completes.  (It could instead use an instance method or
> +a callable object.)  All push hooks take a single argument, the
> +``push_result``.
>  
> -For a complete list of hooks and their parameters, see `Hooks
> -<../user-reference/bzr_man.html#hooks>`_ in the User Reference.
> +Second, the plugin installs the hook.  The first parameter ``'post_push'``

I'm not sure that the markup of “``'post_push'``” makes sense — why include
single quotes here, inside the monospaced text?

> +identifies where to install the hook.  The second parameter is the hook
> +itself.  The thirs parameter is a name 'My post_push hook', which can be
> +used in progress messages and error messages.

Typo: “thirs” should be “third”.

>  Debugging hooks
>  ---------------
> 
> === modified file 'doc/en/user-guide/http_smart_server.txt'
> --- doc/en/user-guide/http_smart_server.txt	2008-09-14 21:51:09 +0000
> +++ doc/en/user-guide/http_smart_server.txt	2008-10-20 06:53:24 +0000
> @@ -131,6 +131,9 @@
>      import fcgi
>      from bzrlib.transport.http import wsgi
>  
> +    from bzrlib.plugin import load_plugins
> +    load_plugins()
> +
>      smart_server_app = wsgi.make_app(
>          root='/srv/example.com/www/code',
>          prefix='/code/',
> @@ -157,6 +160,9 @@
>      import modpywsgi
>      from bzrlib.transport.http import wsgi
>  
> +    from bzrlib.plugin import load_plugins
> +    load_plugins()
> +
>      smart_server_app = wsgi.make_app(
>          root='/srv/example.com/www/code',
>          prefix='/code/',

These changes to include load_plugins() calls in the example scripts aren't
necessary, because wsgi.make_app does this by default in current bzr releases.

Again, thanks very much for taking the time to improve our documentation!

-Andrew.