commit code requires plugins to be loaded

[Jelmer Vernooij]

Current bzr.dev will bail out if it's being used from an external application that doesn't load bzrs plugins. The commit code will throw a confusing exception in the post_commit code if this happens.

We should skip plugins if none are loaded or make sure there's at least always an empty list there.

[John A Meinel (jameinel)]

Jelmer Vernooij wrote:
> Public bug reported:
>
> Current bzr.dev will bail out if it's being used from an external
> application that doesn't load bzrs plugins. The commit code will throw a
> confusing exception in the post_commit code if this happens.
>
> We should skip plugins if none are loaded or make sure there's at least
> always an empty list there.

Post_commit only runs whatever you have configured to run. (in
~/.bazaar/bazaar.conf).
If you tell it to run a post-commit action (this doesn't have to be from
a plugin), then it should fail with a reasonable error message saying
that it couldn't perform the action you requested it to do.

It is arguable that we need a way to disable post-commit hooks. But the
real problem is you have configured bzr to do something after a commit,
but your front end doesn't set things up such that you are able to.

John
=:->

[Jelmer Vernooij (jelmer)]

On Mon, 2006-06-19 at 13:01 +0000, John A Meinel wrote:
> Jelmer Vernooij wrote:
> > Public bug reported:
> >
> > Current bzr.dev will bail out if it's being used from an external
> > application that doesn't load bzrs plugins. The commit code will throw a
> > confusing exception in the post_commit code if this happens.
> >
> > We should skip plugins if none are loaded or make sure there's at least
> > always an empty list there.
> Post_commit only runs whatever you have configured to run. (in
> ~/.bazaar/bazaar.conf).
> If you tell it to run a post-commit action (this doesn't have to be from
> a plugin), then it should fail with a reasonable error message saying
> that it couldn't perform the action you requested it to do.
Ah, right. Sorry. So I guess this bug is actually the fact that we don't
throw an exception that makes sense if the post_commit hook can't be
found. Related to that is the fact that plugins can't register
post_commit hooks at the moment, but you have to specify them yourself.

Cheers,

Jelmer
--
Jelmer Vernooij <email address hidden> - http://samba.org/~jelmer/

[Jelmer Vernooij (jelmer)]

Or, alternatively, post_commit hooks should be registerable, just like commands, branch formats, etc. There's not much point in forcing the user to explicitly specify the hook.

If the hook has to be disabled in specific situations or for specific branches, IMHO that's the task of the plugin. For example, the email_sender plugin would only send emails in case a target address was set.

[John A Meinel (jameinel)]

At the least, we should be giving better errors when the hook doesn't exist. The rest is debatable, but that should be done.

[Jelmer Vernooij (jelmer)]

Well, if hooks are registered by the plugins, there will no longer be any hooks specified by the user, so there won't be an error at all.

[John A Meinel (jameinel)]

Thats a much bigger change (since it involves updating the plugins, changing how they configure themselves, etc). Not to mention the changes to the core itself.

A fix which just gives a better error is a <2hr fix. One that does what you are asking is about 1day. If you're willing to put in the time, fix it however you would like.

We still need a spot in the code where post-commit hooks are run. But yeah, I have no problem if that is more of a factory than a single line config.

(Actually, I prefer it, but don't feel it is worth the effort at this point)

[Jelmer Vernooij (jelmer)]

On Mon, 2006-07-24 at 00:18 +0000, John A Meinel wrote:
> Thats a much bigger change (since it involves updating the plugins,
> changing how they configure themselves, etc). Not to mention the changes
> to the core itself.
>
> A fix which just gives a better error is a <2hr fix. One that does what
> you are asking is about 1day. If you're willing to put in the time, fix
> it however you would like.
>
> We still need a spot in the code where post-commit hooks are run. But
> yeah, I have no problem if that is more of a factory than a single line
> config.
>
> (Actually, I prefer it, but don't feel it is worth the effort at this
> point)
Yeah, I understand your point.

I'll try to put the effort in as I'm getting tired of specifying both
post_commit hooks and the settings for those commit hooks everywhere.
(-: Not likely before 0.9 though (if it's going to be released this
week).

Cheers,

Jelmer
--
Jelmer Vernooij <email address hidden> - http://samba.org/~jelmer/

[Robert Collins (lifeless)]

On Mon, 2006-07-24 at 00:06 +0000, Jelmer Vernooij wrote:
> Well, if hooks are registered by the plugins, there will no longer be
> any hooks specified by the user, so there won't be an error at all.

That forces every plugin to have an 'enable option'. Thats also
undesirable.

This needs much more discussion I think. I agree with john about the
error needs to be made nicer.

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

[Jelmer Vernooij (jelmer)]

On Mon, 2006-07-24 at 00:38 +0000, Robert Collins wrote:
> On Mon, 2006-07-24 at 00:06 +0000, Jelmer Vernooij wrote:
> > Well, if hooks are registered by the plugins, there will no longer be
> > any hooks specified by the user, so there won't be an error at all.
> That forces every plugin to have an 'enable option'. Thats also
> undesirable.
In some cases, yes. However, in most cases there are other ways for the
plugin to find out whether it should work on a specific commit. For
example, the email plugin can only be used when a target email address
is set, the cia plugin only when a project is set, etc.

Either way, in those few cases where necessary 'enable_foo = true' is
much more user friendly than 'post_commit =
bzrlib.plugins.foo.post_commit'.

Anyhow, I'll take this to the mailinglist before having a look at
implementing it.

> This needs much more discussion I think. I agree with john about the
> error needs to be made nicer.
Agreed.

Cheers,

Jelmer

--
Jelmer Vernooij <email address hidden> - http://samba.org/~jelmer/

[Robert Collins (lifeless)]

This was fixed by the new branch hooks which dont have users specifying the python code to run, and thus theres no reference to plugins in config files needed anymore.