Re: [PATCH] Add option `hooks.path` for setting the directory of hooks.

Subject: Re: [PATCH] Add option `hooks.path` for setting the directory of hooks.

Date: Fri, 26 Aug 2016 07:57:06 -0300

To: Erik Rybakken, Jani Nikula,


From: David Bremner

Erik Rybakken <> writes:

> Hi again,
> I implemented the option for hooks myself. The patch is included. Please
> bear with me, this is my first contribution to notmuch (and my first
> attempt to write C code). I tested the option, and it seems to work.
> Best,
> Erik

Welcome. You may find has some
helpful hints, if you didn't already see it.  In particular, your patch
needs an "meaningful commit message". You seem to have mostly managed to
follow the notmuch coding style, so thanks for that.

> +General
> +-------
> +
> +Add option `hooks.path` for setting the directory for hooks. If
> +unset, it will default to the `.notmuch/hooks` sub-directory.
> +

As a minor point, this should go under "Command Line Interface", since
hooks are strictly a CLI feature.

> +static const char hooks_config_comment[] =
> +    " Hook configuration\n"
> +    "\n"
> +    " The only value supported here is 'path' which should be the directory\n"
> +    " where your hooks exists.\n";
> +
"hooks exist."

No real objections to the code or the feature, but you'll need to update
the test suite. In particular T030-config and T040-setup are currently
broken by your patch (but easy to fix).  You should also add at least
test to to ensure running hooks from a non-standard
location works. Feel free to ask for help with the test suite, if
copying existing tests isn't enough.