Discussion:
[MERGE][RFC] Enhanced hooks
Ian Clatworthy
2008-02-04 13:57:13 UTC
Permalink
This is part 3 of the enhanced hooks changes - the interesting bit. To
be fully functional, it require the previous 2 parts though this can be
reviewed separately and all tests should pass. (The obvious link points
to the 2 routines provided by the other patches are marked with comments.)

Here are the major points:

1. Scripts to run when hooks are triggered can now be defined in
configuration files. Multiple scripts can be given for each
hook and each hook is named.

2. By reusing our (nice) configuration architecture, we provide
a lot of flexibility. Some hooks may only be triggered for
a given branch, or for all users of a branch, or for all
branches. (Right now, that flexibility only applies to
shell hooks, though supporting triggering of Python hooks
is almost complete as well - see below.)

3. Arguments passed to a Python hook can be mapped to the most
appropriate "form" for a command line script - parameter,
stdin or an env variable. Likewise, outputs from the
command line script can be mapped back to Python results
as required. The way that mapping is done defines the
"shell script signature" for a given hook type.

As that signature is a "public interface", I'm marking this
patch as RFC. For the branch hooks, I've gone with what Jelmer did in
his shell-hook plugin (as I said I would). If anyone thinks we should do
something different, now's the time to speak up. :-)

In summary, this feature brings us up to (and arguably beyond) the hook
functionality delivered by Subversion, Git and Hg. Once merged, it will
make it much easier for non Python developers to integrate Bazaar into
their environments.

Ian C.

PS: Is there a nice method for turning a string like
'bzrlib.plugins.mystuff.myhook' into a (loaded) function object?
Bundle Buggy
2008-02-04 13:59:14 UTC
Permalink
Bundle Buggy has detected this merge request.

For details, see:
http://bundlebuggy.aaronbentley.com/request/%3C47A719B9.3090001%40internode.on.net%3E
Jelmer Vernooij
2008-02-04 16:23:54 UTC
Permalink
Hi Ian,

Thanks for this, I think this is really something that belongs in the
bzr core :-)
Post by Ian Clatworthy
This is part 3 of the enhanced hooks changes - the interesting bit. To
be fully functional, it require the previous 2 parts though this can be
reviewed separately and all tests should pass. (The obvious link points
to the 2 routines provided by the other patches are marked with comments.)
1. Scripts to run when hooks are triggered can now be defined in
configuration files. Multiple scripts can be given for each
hook and each hook is named.
2. By reusing our (nice) configuration architecture, we provide
a lot of flexibility. Some hooks may only be triggered for
a given branch, or for all users of a branch, or for all
branches. (Right now, that flexibility only applies to
shell hooks, though supporting triggering of Python hooks
is almost complete as well - see below.)
3. Arguments passed to a Python hook can be mapped to the most
appropriate "form" for a command line script - parameter,
stdin or an env variable. Likewise, outputs from the
command line script can be mapped back to Python results
as required. The way that mapping is done defines the
"shell script signature" for a given hook type.
As that signature is a "public interface", I'm marking this
patch as RFC. For the branch hooks, I've gone with what Jelmer did in
his shell-hook plugin (as I said I would). If anyone thinks we should do
something different, now's the time to speak up. :-)
In summary, this feature brings us up to (and arguably beyond) the hook
functionality delivered by Subversion, Git and Hg. Once merged, it will
make it much easier for non Python developers to integrate Bazaar into
their environments.
I think the syntax in the configuration file is a bit complex. Git and
mercurial simply allow::

[hooks]
pre-commit = foo.sh
post-commit = bar.sh

Using a separate section per hook seems overkill to me.

Also, there are now two word separator characters used, '_' and ' '.
It would be nice if the character used could be consistent, e.g.::

[pre-commit-hooks]
foo=foo.sh

Cheers,

Jelmer
Ian Clatworthy
2008-02-05 01:02:07 UTC
Permalink
Post by Jelmer Vernooij
I think the syntax in the configuration file is a bit complex. Git and
[hooks]
pre-commit = foo.sh
post-commit = bar.sh
Using a separate section per hook seems overkill to me.
Also, there are now two word separator characters used, '_' and ' '.
[pre-commit-hooks]
foo=foo.sh
I'm fine with [pre-commit-hooks] instead of [pre_commit hooks].

It's true that

[pre-commit-hooks]
foo = foo.sh
[post-commit-hooks]
bar = bar.sh

is slightly more complex that the Hg approach. But I prefer it because
its good practice to label hooks anyway and it scales nicer for multiple
hooks.

Ian C.
Robert Collins
2008-02-05 00:15:33 UTC
Permalink
Post by Ian Clatworthy
PS: Is there a nice method for turning a string like
'bzrlib.plugins.mystuff.myhook' into a (loaded) function object?
bb:reject

This needs discussion, urgently I think. We /had/ hooks like this and
decided it was a pretty crappy UI and stopped. Why are we bringing it
back ?

-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.
Ian Clatworthy
2008-02-05 01:01:24 UTC
Permalink
Post by Robert Collins
This needs discussion, urgently I think. We /had/ hooks like this and
decided it was a pretty crappy UI and stopped. Why are we bringing it
back ?
Our current way of configuring hooks leaves a lot to be desired: it
requires 'writing a plugin'. Other systems are easier and potential
adopters have rejected Bazaar on this point alone. See
https://lists.ubuntu.com/archives/bazaar/2007q4/032657.html.

Following that feedback, I kicked off a discussion thread on this topic
asking people for what they wanted:
https://lists.ubuntu.com/archives/bazaar/2007q4/035833.html. Shell hooks
was repeated time and time again.

Jelmer recently announced his shell-hook plugin. He and I both believe
that the functionality needs to be in the core. The patches submitted
yesterday add that.

The question will then arise, "If shell hooks can be configured in a
flexible way, why can't Python hooks?".

Ian C.
Robert Collins
2008-02-05 01:36:05 UTC
Permalink
Post by Ian Clatworthy
Post by Robert Collins
This needs discussion, urgently I think. We /had/ hooks like this and
decided it was a pretty crappy UI and stopped. Why are we bringing it
back ?
Our current way of configuring hooks leaves a lot to be desired: it
requires 'writing a plugin'. Other systems are easier and potential
adopters have rejected Bazaar on this point alone. See
https://lists.ubuntu.com/archives/bazaar/2007q4/032657.html.

Post by Ian Clatworthy
Following that feedback, I kicked off a discussion thread on this topic
https://lists.ubuntu.com/archives/bazaar/2007q4/035833.html. Shell hooks
was repeated time and time again.
I remember those thread, but I didn't see 'our current system leaves a
lot to be desired' turn up once. I remember our documentation being a
problem, and that for those users they wanted some specific things that
*we didn't even have hooks for* (pre-commit-starting test scripts, and
centralised mail). Its important to take users feedback, but its also
important to design something that works well consistently. The point
about trivial things like 'run a test script before doing anything
related to commit' are totally reasonable, and I support having a shell
hook plugin to answer those needs.

In detail I saw that:
- some folk want shell scripts (but noone there actually asked for it
for themselves)
- feature requests for some new hooks
- smart server to run hooks
- discussion about the evolution of the API (not all hooks use a Result
object, for hyysterical raisins).

And really, writing a plugin to hook something is as simple as creating
'myhook.py' and dropping it in the search path. Its tiny:
mycommit.py:
------
from bzrlib.branch import Branch

def WeRunThis(local, master, old_revno, old_revid, new_revno,
new_revid):
print "Revision %d committed to branch %s" % (new_revno, master)

Branch.hooks.install_hook('post_commit', WeRunThis)
# optional but nice follows
Branch.hooks.name_hook(branch_commit_hook, "WeRunThis")
------

'Easier' is relative obviously I find marshalling arbitrary urls with
problems like user credential hand-offs MUCH HARDER. Lets talk about
specific defects we can look at fixing.

Frankly I find the current way of configuring hooks extremely easy to
use as both a developer and a user. I set a setting in a config file to
configure the hook. I drop the hook script into my plugins dir. Here's
another:
sometimes.py:
------
from bzrlib.branch import Branch

def MightRunThis(local, master, old_revno, old_revid, new_revno,
new_revid):
config = master.get_branch_config()
if config.get_user_option('WeRunThis'):
print "Revision %d committed to branch %s" % (new_revno, master)

Branch.hooks.install_hook('post_commit', MightRunThis)
# optional but nice follows
Branch.hooks.name_hook(branch_commit_hook, "MightRunThis")
-----

John suggested pattern matching shell scripts, a better approach is to
user the idiom Debian and a growing number of upstream tools have for
many configuration related things - FOO.d where all of the items in the
directory foo.d are executed; and this is very friendly for shell users;
its conceptually similar to the way bzr plugins are loaded (and why they
are so easy to work with). We can definitely do this for shell scripts
if we like (why should there be only one shell script post-commit
called, for instance) - but I /really/ don't think shell script support
belongs in core. It will just be too freaking hard to make it ROCK like
in-process stuff does.
Post by Ian Clatworthy
Jelmer recently announced his shell-hook plugin. He and I both believe
that the functionality needs to be in the core. The patches submitted
yesterday add that.
The question will then arise, "If shell hooks can be configured in a
flexible way, why can't Python hooks?".
Python hooks already are much more flexible than shell hooks are likely
to ever be (modulo the serious black mark of not working in a
centralised fashion on the smart server).

-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.
Ian Clatworthy
2008-02-05 09:04:43 UTC
Permalink
Post by Robert Collins
John suggested pattern matching shell scripts, a better approach is to
user the idiom Debian and a growing number of upstream tools have for
many configuration related things - FOO.d where all of the items in the
directory foo.d are executed; and this is very friendly for shell users;
its conceptually similar to the way bzr plugins are loaded (and why they
are so easy to work with). We can definitely do this for shell scripts
if we like (why should there be only one shell script post-commit
called, for instance) - but I /really/ don't think shell script support
belongs in core. It will just be too freaking hard to make it ROCK like
in-process stuff does.
I've tried to capture some of this debate on
http://bazaar-vcs.org/BzrHooks under some new sections added at the end
today.

I can see that using directories has advantages. I can't say that I find
it clearly superior to using config files.

It's good to have this debate and I'm happy to let the patch float until
we get some consensus on the numerous issues:

* how should scripts be configured?
* should scripts be supported for all hooks or just some?
* what should the script signature look like for each?
* what is core and what is not?

I'd like to also state that the purpose of shell hooks IMO is *not* to
do magic things, so we shouldn't get bogged down on the basis that they
are inferior to our current approach (in terms of locking and
performance) for things like pre-commit. In my experience (and yours may
differ), people asking for shell hooks want to simply do the following
95% of the time:

* make target
* ant target
* send a message to another system

More often than not, Perl will be used for the last case as that's the
language the majority of sysadmins use for glue like this. Passing some
basic stuff in via string parameters is therefore good enough.

Beyond those boundaries, we should continue to champion our current
approach, not shell hooks.

Ian C.
Vincent Ladeuil
2008-02-05 22:01:37 UTC
Permalink
Post by Robert Collins
John suggested pattern matching shell scripts, a better approach is to
user the idiom Debian and a growing number of upstream tools have for
many configuration related things - FOO.d where all of the items in the
directory foo.d are executed; and this is very friendly for shell users;
its conceptually similar to the way bzr plugins are loaded (and why they
are so easy to work with).
I prefer organizing hook scripts into directories too.

Hook signatures seems the most blocking problem. I can't imagine
passing more than a list of strings to a shell script and even
there we may hit some shell limits.

That means post_pull and post_push are already excluded, their
signature is a [Pull|Push]Result object...

The server_started signature may be addressed by reverting the
arguments:

def hook(public_url, backing_urls)

instead of

def hook(backing_urls, public_url)

IMHO, this limitation restrict shell hooks use to cases where:
- the script gives back nothing,
- we may even ignore errors (or may be not for pre_commit ?),
- we should define them where we really have no better solution
to address user needs.
Post by Robert Collins
We can definitely do this for shell scripts if we like
(why should there be only one shell script post-commit
called, for instance) - but I /really/ don't think shell
script support belongs in core.
I agree with that, bzr commands should be atomic and as such
wrapping them in scripts should address most needs.

The only cases where most users may want a hook is pre_commit or
post_commit ones and mostly because their workflow may be unclear
when migrating from centralized to decentralized.

So may be some patterns should be extracted from existing
plugins like bzr-email and bzr-pqm to make it easier to address
yet-unknown needs.
Post by Robert Collins
It will just be too freaking hard to make it ROCK like
in-process stuff does.
I share that feeling too.

Making different languages communicate efficiently is a damn hard
problem and is generally done to gain either better performances
or more capabilities. Shell can't give us either.

Vincent
Robert Collins
2008-02-05 22:59:41 UTC
Permalink
Post by Vincent Ladeuil
Hook signatures seems the most blocking problem. I can't imagine
passing more than a list of strings to a shell script and even
there we may hit some shell limits.
Amen. This is why I like the idea of doing small mini-hook-like-things
like the example pre-commit-check thunk I posted. We could include such
things in core if we want (though making them very accessible as
plugins)++.

-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.
Aaron Bentley
2008-02-06 04:10:00 UTC
Permalink
Post by Vincent Ladeuil
Hook signatures seems the most blocking problem. I can't imagine
passing more than a list of strings to a shell script and even
there we may hit some shell limits.
You can also pass in environment variables, and this makes it simpler to
add new ones and deal with variadic results.

That was the way Arch did it, and frankly, I think it had better hook
capabilities than Bazaar. Not more powerful, just more useful.
Post by Vincent Ladeuil
That means post_pull and post_push are already excluded, their
signature is a [Pull|Push]Result object...
I think that's an overly mechanical approach. There's plenty of string
data in a Result.
Post by Vincent Ladeuil
- the script gives back nothing,
Sorry, I don't understand why.
Post by Vincent Ladeuil
- we may even ignore errors (or may be not for pre_commit ?),
No, I would expect at least some hooks to respect status codes.
Post by Vincent Ladeuil
I agree with that, bzr commands should be atomic and as such
wrapping them in scripts should address most needs.
I disagree. I want to hook operations, not commands. If I want to hook
the fetch operation, then I want to hook push, pull, branch, and
anything else that uses it.
Post by Vincent Ladeuil
The only cases where most users may want a hook is pre_commit or
post_commit ones and mostly because their workflow may be unclear
when migrating from centralized to decentralized.
So may be some patterns should be extracted from existing
plugins like bzr-email and bzr-pqm to make it easier to address
yet-unknown needs.
These are good, but IMO they aren't an adequate reaction to the
question, "Do you support shell hooks." The only adequate reaction is
"yes."
Post by Vincent Ladeuil
Post by Robert Collins
It will just be too freaking hard to make it ROCK like
in-process stuff does.
For the average user, the in-process stuff is a swiss-army chainsaw.
People don't want to learn Python and the Bazaar API and write a plugin.
They just want to write a shell script that runs "make lint" before
committing.

I agree entirely that it won't ROCK like the in-process stuff does. I
don't think it needs to. I'm absolutely fine with requiring Python for
advanced hooks.

This all smacks of absolutism, but I want Bazaar to be a joy to use for
everyone who uses it. I think there's a large group of people who don't
want to use Python for simple hook scripts. And frankly, that group
includes me.
Post by Vincent Ladeuil
Making different languages communicate efficiently is a damn hard
problem and is generally done to gain either better performances
or more capabilities. Shell can't give us either.
I don't agree, and I would point out that these scripts can be in any
language, not just Bash. An all-Ruby project might find it very
convenient to be able to call into their library from a hook. They
might even unit-test it.

Aaron
Ian Clatworthy
2008-02-06 04:57:25 UTC
Permalink
Post by Aaron Bentley
This all smacks of absolutism, but I want Bazaar to be a joy to use for
everyone who uses it. I think there's a large group of people who don't
want to use Python for simple hook scripts. And frankly, that group
includes me.
I think that captures my feelings on the subject. Bazaar needs to be
friendly to all development teams, not just Python-centric ones. Perl
teams will write their hooks in Perl, Java teams will write their hooks
in Java, Ruby teams will write their hooks in Ruby, etc. In teams with
central repositories where the IT admin team looks after the development
environment, shell or Windows batch files will be the technologies of
choice. Telling users that they only get hooks (or most hooks) if they
use Python will undoubtedly rule Bazaar out of many communities and
organisations. That's sad.

Good usability is about making the common cases easy and the hard cases
possible. As I said earlier, most hooks for most teams (in my
experience) are either "make xxx", "ant xxx" or "send a message to
system yyy". The first two don't require any info passed usually - the
mere existence of the event is enough! The 3rd case only requires simple
strings most of the time. Sure, there are exceptions - we have our
existing hook architecture for those. It's the simple cases where Bazaar
is well below where *I* think it needs to be.

I had a go overnight in implementing Robert's preference for putting
shell hook support in a plugin. Given the lack of consensus, that's the
right place for this feature at this point in time. Longer term, I feel
we need to make external hooks a core feature so users can rely on their
signatures (which would be in the main documentation) and count on them
being there.

I'd /really/ like a small change to the core to make the plugin simpler
and more efficient, namely changing the lookup (not registration) of
hooks to be via the object, not the Branch.hooks or SmartTCPServer.hooks
singletons. So

for hook in Branch.hooks['post_uncommit']:

would become

for hook in branch.iter_hooks('post_uncommit'):

for example. If that sounds ok, I'll put up a small patch for that
change. If it doesn't, then shell hooks beyond Jelmer's plugin will need
to wait a while - other priorities have taken over for me.

Ian C.
Robert Collins
2008-02-06 05:21:55 UTC
Permalink
Post by Ian Clatworthy
I'd /really/ like a small change to the core to make the plugin simpler
and more efficient, namely changing the lookup (not registration) of
hooks to be via the object, not the Branch.hooks or
SmartTCPServer.hooks
singletons. So
would become
for example. If that sounds ok, I'll put up a small patch for that
change. If it doesn't, then shell hooks beyond Jelmer's plugin will need
to wait a while - other priorities have taken over for me.
I have two concerns about this.

One is that it leads to two places hooks can be for any given gook and
makes things less clear for debugging. I think you can do your shell
hook stuff really very cleanly without this change. (On IRC I suggested
one way; in a later mail I suggested two ways, and the latter is
probably cleaner in all regards - its commensurate with regular hooks).

Not to mention me providing an implementation of pre-commit checking
thunk for shell thats clean, will work well, and is ~ 14 lines long.

-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.
Vincent Ladeuil
2008-02-06 08:33:16 UTC
Permalink
Post by Vincent Ladeuil
Hook signatures seems the most blocking problem. I can't imagine
passing more than a list of strings to a shell script and even
there we may hit some shell limits.
aaron> You can also pass in environment variables, and this makes it simpler to
aaron> add new ones and deal with variadic results.

I didn't say there were no ways to allow a powerful API.

My point is that doing more than strings will be a nightmare.

Just look at the smart server. Python both sides, communication
via a socket, a frontier quite similar than crossing languages:
we need to serialize and deserialize.

Or look at the plugins that try to use bzr (the command) instead
of bzrlib, xml-output, tortoisebzr, bzr-eclipse, etc.

The problem is not the hooks by themselves, I'm all *for* hooks
(I'm an emacs user after all). Hooks can *add* their own problems
if they are allowed to introduce conditional behavior though.

<snip/>

aaron> I disagree. I want to hook operations, not commands.
aaron> If I want to hook the fetch operation, then I want to
aaron> hook push, pull, branch, and anything else that uses
aaron> it.

Does that mean bzr don't address your needs ?

I mean, as an emacs user, I know and love hookable softwares, but
hooks are there to address needs that the software can't
realistically address. Before using hooks, you can change command
behavior through options or environment variables, hooks are
really the last resort because your need is just so exotic it
will never be taken into account.
Post by Vincent Ladeuil
The only cases where most users may want a hook is pre_commit or
post_commit ones and mostly because their workflow may be unclear
when migrating from centralized to decentralized.
So may be some patterns should be extracted from existing
plugins like bzr-email and bzr-pqm to make it easier to address
yet-unknown needs.
aaron> These are good, but IMO they aren't an adequate
aaron> reaction to the question, "Do you support shell
aaron> hooks." The only adequate reaction is "yes."

Amen.

In carefully reviewed places. But not blindly, not everywhere and
with a clearly defined API (not forgetting forward and backward
compatibility issues).

<snip/>
Post by Vincent Ladeuil
Making different languages communicate efficiently is a
damn hard problem and is generally done to gain either
better performances or more capabilities. Shell can't give
us either.
aaron> I don't agree, and I would point out that these
aaron> scripts can be in any language, not just Bash.

That doesn't matter. What matters is how they communicate. Do you
want to allow the shell scripts to (for example) throw exceptions ?
How ?

What means could be used to communicate between bzr and shell
(lowest component, some of the problems will be easier to solve
for more evolved languages), in increasing order of difficulty:

- procedures with no parameters
- functions with no parameters returning an integer
- parameters:
* string (mostly trivial, but watch for protecting embedded
separator (space?))
* integers (use strings),
* None (hmm, magic string ?),
* flat lists (watch for the separator (different one)),
* hierarchical lists (watch for the separatorS),
* hashes (separators issues again),
* objects (ha ha, just kidding),
- return value
* integer ok,
* string (hmmm),
* flat list (errr),

And this is just the basic stuff, more sophistication will
require handling of exceptions, locking, reusing already built
objects, etc.

aaron> An all-Ruby project might find it very convenient to
aaron> be able to call into their library from a hook. They
aaron> might even unit-test it.

Yeah ! Ruby ftw ! :)

I'm sure they will prefer being able to call Ruby from Python
though...( no ! no ! I didn't mean to open another can of
worms !!! Argh, to late ;)

Well, I don't want to sound like I'm *against* hooks (I'm
certainly not), I just wanted to point that it will be hard and
that there will be restrictions compared to python hooks.

Vincent
Voelker, Bernhard
2008-02-06 06:52:51 UTC
Permalink
Post by Ian Clatworthy
More often than not, Perl will be used for the last case as that's the
language the majority of sysadmins use for glue like this. Passing some
basic stuff in via string parameters is therefore good enough.
e.g. ClearCase (centralized+comm. VCS) doesn't use argv style argument
passing
to hooks (there: trigger), but simple environment variables.
So the script/program doesn't have to bother about command line parsing.

There are environment variables for the current action (dep. on the
action) for:
* the file name,
* the branch name and version number,
* maybe the predecessing version of the file,
* ...

For each action, a different set of variables is provided and denoted in
the man page.

Pre-action triggers can prevent the action by simply returning
TRUE/FALSE.
This can e.g. be used for HOC or other syntax checks.

Not a bad idea ...

Have a nice day,
Berny
Robert Collins
2008-02-05 00:57:23 UTC
Permalink
bb:reject
(as before, but I'm going to review the code this time)


-
- These are all empty initially, because by default nothing
should get
- notified.


This change, and the static initialisation of the hooks is bad style
IMO. The fact that we are supplying some stuff that thunks across to
shell is irrelevant - we should initialise that separately. This makes
tests cleaner and provides the ability to have a completely empty *Hook
as desired.

The adapters need docstrings at a minimum, or better yet to be methods
on an adapter objects as they seem to share a lot.

The api change to Hook seems unneeded, you should explain why (or better
yet, as it seems unneeded, don't change it :)).

The python style changes are really very ugly and definitely a step
backwards. We have automatically registered hooks today and they work
great. This is the fundamental thing that is driving the bb:reject - the
rest we can work with.

I don't think the hooks need to be typed either.

Basically I think what you have done is turned a self-registered system
into a manunally registered system. And this is why it is cumbersome to
work with.

I suggest you approach the problem differently and ask yourself 'how can
I get a hook written in shell to auto-register a python callable for
e.g. branch'.

-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.
Ian Clatworthy
2008-02-05 01:43:04 UTC
Permalink
Post by Robert Collins
Basically I think what you have done is turned a self-registered system
into a manunally registered system. And this is why it is cumbersome to
work with.
I suggest you approach the problem differently and ask yourself 'how can
I get a hook written in shell to auto-register a python callable for
e.g. branch'.
I haven't removed any of the (implicitly global) self-registration -
I've added selective registration so the active hooks can differ (for
example) per branch.

Let's me know on IRC what API you want given the current branch is now
needed to find the list of hooks.

Ian C.
Robert Collins
2008-02-05 02:05:38 UTC
Permalink
Post by Ian Clatworthy
I haven't removed any of the (implicitly global) self-registration -
I've added selective registration so the active hooks can differ (for
example) per branch.
Let's me know on IRC what API you want given the current branch is now
needed to find the list of hooks.
Its not needed to find the list of hooks. Here are two ways of doing it
by branch.

* Add a hook instance at the end of branch.py for each hook, to a static
method on Branch.
e.g. Branch.set_rh_shell_hook

This hook does:

config = master.get_branch_config()
# look for shell hook
...
if not found:
return


* Add a dynamically created hook when the branch config is first parsed,
and free it when unlock() drops the refcount all the way to zero.
it will say something like:
if master is not self:
return

-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.
Aaron Bentley
2008-02-06 07:22:23 UTC
Permalink
Post by Ian Clatworthy
1. Scripts to run when hooks are triggered can now be defined in
configuration files. Multiple scripts can be given for each
hook and each hook is named.
Until I read your wiki page, I did not realize you were contemplating
hooks in branch.conf. Since this file may be controlled by another
user, this opens the door to malicious activity. It's something I tried
very hard to guard against happening, which is why gpg_signing_command
cannot be specified in branch.conf.

Should anyone pursue the idea of configuration files, I strongly suggest
that branch.conf be considered untrusted by default.

Aaron
Ian Clatworthy
2008-02-06 07:57:02 UTC
Permalink
Post by Aaron Bentley
Until I read your wiki page, I did not realize you were contemplating
hooks in branch.conf. Since this file may be controlled by another
user, this opens the door to malicious activity. It's something I tried
very hard to guard against happening, which is why gpg_signing_command
cannot be specified in branch.conf.
Should anyone pursue the idea of configuration files, I strongly suggest
that branch.conf be considered untrusted by default.
Jelmer,

Finding the hooks to run from that location (and that location only) is
a feature of shell-hook plugin. It sounds like we need some large
warnings put in the README in the short term, yes?

Aaron,

Do you have ideas on how we ought to support "run this hook for all
users accessing this branch"?

W.r.t. security, do you see using a hooks/ directory within .bzr for a
branch safer than branch.conf or not? It seems cleaner in that the hook
code is part of the branch, not just the hook command, but both ways are
equally unsecure, yes?

One of the reasons why I like using config files over directories is
because putting hook commands inside locations.conf - leveraging the
pattern matching we provide when processing that file - has a lot of
potential to lower cost of ownership. For our existing Python hooks, one
can always put config setting there and call back into the config
infrastructure to check whether to run or not. It's doable, though ugly
in the eyes of some (like me). External scripts are different: we don't
want the cost of executing them unnecessarily and we don't want the
complexity of calling back from them into our API as a rule. Any
suggestions on how to provide this flexibility if we go the "find hooks
in a directory" route?

Ian C.
John Arbash Meinel
2008-02-06 13:53:47 UTC
Permalink
Post by Ian Clatworthy
Post by Aaron Bentley
Until I read your wiki page, I did not realize you were contemplating
hooks in branch.conf. Since this file may be controlled by another
user, this opens the door to malicious activity. It's something I tried
very hard to guard against happening, which is why gpg_signing_command
cannot be specified in branch.conf.
Should anyone pursue the idea of configuration files, I strongly suggest
that branch.conf be considered untrusted by default.
Jelmer,
Finding the hooks to run from that location (and that location only) is
a feature of shell-hook plugin. It sounds like we need some large
warnings put in the README in the short term, yes?
Aaron,
Do you have ideas on how we ought to support "run this hook for all
users accessing this branch"?
W.r.t. security, do you see using a hooks/ directory within .bzr for a
branch safer than branch.conf or not? It seems cleaner in that the hook
code is part of the branch, not just the hook command, but both ways are
equally unsecure, yes?
Yes, they are both equally insecure.
Post by Ian Clatworthy
One of the reasons why I like using config files over directories is
because putting hook commands inside locations.conf - leveraging the
pattern matching we provide when processing that file - has a lot of
potential to lower cost of ownership. For our existing Python hooks, one
can always put config setting there and call back into the config
infrastructure to check whether to run or not. It's doable, though ugly
in the eyes of some (like me). External scripts are different: we don't
want the cost of executing them unnecessarily and we don't want the
complexity of calling back from them into our API as a rule. Any
suggestions on how to provide this flexibility if we go the "find hooks
in a directory" route?
Ian C.
Actually, for some, branch.conf represents lower cost. Specifically
thing a large company with a lot of branches wanting to enforce some
company policy.
To go one step further, how about "encourage" company policy. They could
always get around it, but the point is to *help* them do the right thing.

That is actually one of the reasons Monty wanted to put the config files
in .bzr-<plugin>/default.conf or possibly .bzr-plugins/plugin.conf

It would be a way to version them which makes it easier to send them to
the end-users.

Remember that while centralized version control is "evil and wrong" :)
it does allow a single administrator to set things up for a bunch of people.

Hooks often fall into this "everyone should follow X" rules.

My personal preference is to provide enough core support so that a
3rd-party can write a plugin which does the "trust settings in branches"
for places that they want their users to trust. That way it doesn't have
to be trusted everywhere, and hopefully we don't have to get too
involved in hook ACLs and trust mechanisms.

Consider, one solution would be to require hooks to be gpg signed, and
then give a list of keys that you are willing to trust the hooks. It
would be possible, but it really starts adding a lot of complexity. I
think it is far easier to have someone install the "site-configuration
for company Foo" plugin.

John
=:->
Aaron Bentley
2008-02-06 14:19:07 UTC
Permalink
Post by John Arbash Meinel
Remember that while centralized version control is "evil and wrong" :)
it does allow a single administrator to set things up for a bunch of people.
Hooks often fall into this "everyone should follow X" rules.
My personal preference is to provide enough core support so that a
3rd-party can write a plugin which does the "trust settings in branches"
for places that they want their users to trust. That way it doesn't have
to be trusted everywhere, and hopefully we don't have to get too
involved in hook ACLs and trust mechanisms.
I had imagined at some point we could extend the config mechanism so
that people could mark locations as trusted in locations.conf. I'm not
sure whether that seems "too involved" to you.

But supporting an explicit central configuration might work even better
than supporting particular instances of branches.conf.

Aaron
John Arbash Meinel
2008-02-06 14:51:00 UTC
Permalink
Post by Aaron Bentley
Post by John Arbash Meinel
Remember that while centralized version control is "evil and wrong" :)
it does allow a single administrator to set things up for a bunch of people.
Hooks often fall into this "everyone should follow X" rules.
My personal preference is to provide enough core support so that a
3rd-party can write a plugin which does the "trust settings in branches"
for places that they want their users to trust. That way it doesn't have
to be trusted everywhere, and hopefully we don't have to get too
involved in hook ACLs and trust mechanisms.
I had imagined at some point we could extend the config mechanism so
that people could mark locations as trusted in locations.conf. I'm not
sure whether that seems "too involved" to you.
But supporting an explicit central configuration might work even better
than supporting particular instances of branches.conf.
Aaron
Well the general point is to avoid our current situation. Where to get
post-commit emails, every developer needs to install and configure
bzr-email. And to get pre-commit filtering, every developer needs to
install X and configure it.

So an admin has to do this N times for N developers, rather than 1 time
in a central location.

Having trusted locations might be sufficient.

My current idea is to have a site-config plugin which can be customized
and get its own updates/install add-ons/etc.

We currently don't have a great story to tell admins who are setting up
large deployments. It hasn't been something we've thought about a lot,
because we've been working on giving power and flexibility to end users
more than assistance for the admins.

I certainly think we can come up with lots of very tasteful solutions,
we just need to spend some time thinking about it.

John
=:->
Monty Taylor
2008-02-06 14:48:05 UTC
Permalink
Post by John Arbash Meinel
Actually, for some, branch.conf represents lower cost. Specifically
thing a large company with a lot of branches wanting to enforce some
company policy.
To go one step further, how about "encourage" company policy. They could
always get around it, but the point is to *help* them do the right thing.
That is actually one of the reasons Monty wanted to put the config files
in .bzr-<plugin>/default.conf or possibly .bzr-plugins/plugin.conf
Yes - but also the reason that my approach is specifically for plugin
config that still requires plugins to be installed _and_ configured as
trusted plugins. I feel pretty strongly that I _don't_ want hooks to
come along with a branch that might execute without me knowing about it.
Post by John Arbash Meinel
Consider, one solution would be to require hooks to be gpg signed, and
then give a list of keys that you are willing to trust the hooks. It
would be possible, but it really starts adding a lot of complexity. I
think it is far easier to have someone install the "site-configuration
for company Foo" plugin.
Yes - and in my company where we are trying to roll out bzr as a
replacement for things, people are not rebelling against that. In fact,
we tell them to just bzr branch the company plugins into their plugin
dir, and occasionally we can tell them to update the company plugin. Of
course, I think it was suggest to write a hook in the company plugin
that goes and checks to make sure the user has the latest company plugin
version and bails out until they install it...

Monty

PS. Haven't gotten a chance to fix the plugin config patch yet... :)
Robert Collins
2008-02-06 21:11:26 UTC
Permalink
Post by John Arbash Meinel
Consider, one solution would be to require hooks to be gpg signed, and
then give a list of keys that you are willing to trust the hooks. It
would be possible, but it really starts adding a lot of complexity. I
think it is far easier to have someone install the
"site-configuration
for company Foo" plugin.
I think the right answer for getting centralised config is having the
hooks installed on a server, and requiring bzr+ssh access be used to
that server.

This avoids all the security issues (the client does not run the hooks),
an deployment and versioning issues.

-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.
John Arbash Meinel
2008-02-06 21:14:16 UTC
Permalink
Robert Collins wrote:
| On Wed, 2008-02-06 at 07:53 -0600, John Arbash Meinel wrote:
|>
|> Consider, one solution would be to require hooks to be gpg signed,
|> and
|> then give a list of keys that you are willing to trust the hooks. It
|> would be possible, but it really starts adding a lot of complexity. I
|> think it is far easier to have someone install the
|> "site-configuration
|> for company Foo" plugin.
|
| I think the right answer for getting centralised config is having the
| hooks installed on a server, and requiring bzr+ssh access be used to
| that server.
|
| This avoids all the security issues (the client does not run the hooks),
| an deployment and versioning issues.
|
| -Rob
|

Well, if you want people to conform to a given commit message template,
even when they are working on a plane....

There are certainly lots of different use cases. I agree that having a
central machine do some of the work is a decent solution. It certainly
shares some ideas with our PQM model.

John
=:->
Robert Collins
2008-02-06 21:53:57 UTC
Permalink
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
|>
|> Consider, one solution would be to require hooks to be gpg signed,
|> and
|> then give a list of keys that you are willing to trust the hooks. It
|> would be possible, but it really starts adding a lot of complexity. I
|> think it is far easier to have someone install the
|> "site-configuration
|> for company Foo" plugin.
|
| I think the right answer for getting centralised config is having the
| hooks installed on a server, and requiring bzr+ssh access be used to
| that server.
|
| This avoids all the security issues (the client does not run the hooks),
| an deployment and versioning issues.
|
| -Rob
|
Well, if you want people to conform to a given commit message template,
even when they are working on a plane....
Sure, but they can just remove the plugin. Or edit the branch.conf to
remove the pointer.

BTW the plugins I was suggesting in .bzr/plugins would not propogate
with fetch operations. I don't know if that was clear - they would be
separate, just a place to put them to make them affect a branch only.
There are certainly lots of different use cases. I agree that having a
central machine do some of the work is a decent solution. It certainly
shares some ideas with our PQM model.
Indeed. PQM has some friction, but its designed to do long tasks; these
plugins probably should not be making commits take hours :)

-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.
Loading...