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

[Tomi Ollila <tomi.ollila@iki.fi>]

On Thu, Aug 25 2016, Erik Rybakken <erik.rybakken@math.ntnu.no> wrote:

> 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.

The implementation looks pretty good...

... but I can think of one problem there (if my memory server correctly)

If this new option is added, and as (IIRC) our configuration system writes
all missing options to the configuration file we may have this
(hypothetical for me, at least (*)) "coupling" problem.


database.path points to some directory

hooks.path points to 'hooks/' subdirectory in database.path

this directory we're talking about is moved elsewhere

now just editing database.path in configuration gives user
a broken system where current hooks cannot be used.

If our configuration syste did not write nonexistent entries we could just
have [hooks]\n#path outcommented in the configuration by default. To
change this is just Simple Matter of Programming (SMOP), or course.

(*) I do not use hooks, nor I plan to move my directory (which means that
both of these are going to change some day in the future ;))


I hope someone(™) can counterargument this email...

Tomi


>
> Best,
> Erik
>
> On Wed, Aug 24, 2016 at 08:45:36PM +0300, Jani Nikula wrote:
>> On Wed, 24 Aug 2016, Erik Rybakken <erik.rybakken@math.ntnu.no> wrote:
>> > I would like to store hooks in a different directory from my mail.
>> > It would be nice to have an option "hooks.path" with default being
>> > "$DATABASEDIR/.notmuch/hooks/". Also, I think a similar option for
>> > the xabian database would make sense.
>> 
>> I think we agree it would be great to be able to configure the location
>> of .notmuch. Unfortunately, the notmuch library currently assumes it can
>> deduce the database location from the mail store location. The last I
>> checked, making it configurable was a bit complicated, and so far nobody
>> has thought it would be worth it. Making the hooks directory
>> configurable would be significantly simpler.
>> 
>> For the time being, you can work around the limitations by symlinking
>> .notmuch or .notmuch/hooks to your preferred location.
>> 
>> BR,
>> Jani.
>
> ---
>  NEWS                        |  6 ++++++
>  doc/man1/notmuch-config.rst |  5 +++++
>  doc/man5/notmuch-hooks.rst  |  7 ++++---
>  hooks.c                     |  5 ++---
>  notmuch-client.h            |  9 ++++++++-
>  notmuch-config.c            | 38 ++++++++++++++++++++++++++++++++++++++
>  notmuch-insert.c            |  4 +++-
>  notmuch-new.c               |  6 ++++--
>  8 files changed, 70 insertions(+), 10 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 3a9c8d3..b200bbf 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,6 +1,12 @@
>  Notmuch 0.23 (UNRELEASED)
>  =========================
>  
> +General
> +-------
> +
> +Add option `hooks.path` for setting the directory for hooks. If
> +unset, it will default to the `.notmuch/hooks` sub-directory.
> +
>  Emacs
>  -----
>  
> diff --git a/doc/man1/notmuch-config.rst b/doc/man1/notmuch-config.rst
> index 5a517eb..b1e266c 100644
> --- a/doc/man1/notmuch-config.rst
> +++ b/doc/man1/notmuch-config.rst
> @@ -51,6 +51,11 @@ The available configuration items are described below.
>  
>          Default: ``$MAILDIR`` variable if set, otherwise ``$HOME/mail``.
>  
> +    **hooks.path**
> +        The directory where your hooks exists.
> +
> +        Default: ``database.path/.notmuch/hooks``.
> +
>      **user.name**
>          Your full name.
>  
> diff --git a/doc/man5/notmuch-hooks.rst b/doc/man5/notmuch-hooks.rst
> index f96a923..fbbebe3 100644
> --- a/doc/man5/notmuch-hooks.rst
> +++ b/doc/man5/notmuch-hooks.rst
> @@ -11,9 +11,10 @@ DESCRIPTION
>  ===========
>  
>  Hooks are scripts (or arbitrary executables or symlinks to such) that
> -notmuch invokes before and after certain actions. These scripts reside
> -in the .notmuch/hooks directory within the database directory and must
> -have executable permissions.
> +notmuch invokes before and after certain actions. By default, these
> +scripts reside in the .notmuch/hooks directory within the database
> +directory, but this can be changed by setting the ``hooks.path``
> +option. The hooks must have executable permissions.
>  
>  The currently available hooks are described below.
>  
> diff --git a/hooks.c b/hooks.c
> index 7348d32..8bc7ca6 100644
> --- a/hooks.c
> +++ b/hooks.c
> @@ -24,14 +24,13 @@
>  #include <sys/wait.h>
>  
>  int
> -notmuch_run_hook (const char *db_path, const char *hook)
> +notmuch_run_hook (const char *hooks_path, const char *hook)
>  {
>      char *hook_path;
>      int status = 0;
>      pid_t pid;
>  
> -    hook_path = talloc_asprintf (NULL, "%s/%s/%s/%s", db_path, ".notmuch",
> -				 "hooks", hook);
> +    hook_path = talloc_asprintf (NULL, "%s/%s", hooks_path, hook);
>      if (hook_path == NULL) {
>  	fprintf (stderr, "Out of memory\n");
>  	return 1;
> diff --git a/notmuch-client.h b/notmuch-client.h
> index ebc092b..2cbfc5b 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -274,6 +274,13 @@ notmuch_config_set_database_path (notmuch_config_t *config,
>  				  const char *database_path);
>  
>  const char *
> +notmuch_config_get_hooks_path (notmuch_config_t *config);
> +
> +void
> +notmuch_config_set_hooks_path (notmuch_config_t *config,
> +				  const char *hooks_path);
> +
> +const char *
>  notmuch_config_get_crypto_gpg_path (notmuch_config_t *config);
>  
>  void
> @@ -336,7 +343,7 @@ notmuch_config_set_search_exclude_tags (notmuch_config_t *config,
>  				      size_t length);
>  
>  int
> -notmuch_run_hook (const char *db_path, const char *hook);
> +notmuch_run_hook (const char *hooks_path, const char *hook);
>  
>  notmuch_bool_t
>  debugger_is_active (void);
> diff --git a/notmuch-config.c b/notmuch-config.c
> index e5d42a0..7b5ac8b 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -38,6 +38,12 @@ static const char database_config_comment[] =
>      " Notmuch will store its database within a sub-directory of the path\n"
>      " configured here named \".notmuch\".\n";
>  
> +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";
> +
>  static const char new_config_comment[] =
>      " Configuration for \"notmuch new\"\n"
>      "\n"
> @@ -115,6 +121,7 @@ struct _notmuch_config {
>      notmuch_bool_t is_new;
>  
>      char *database_path;
> +    char *hooks_path;
>      char *crypto_gpg_path;
>      char *user_name;
>      char *user_primary_email;
> @@ -228,6 +235,8 @@ get_username_from_passwd_file (void *ctx)
>   *
>   *		database_path:		$MAILDIR, otherwise $HOME/mail
>   *
> + *		hooks_path:		database_path/.notmuch/hooks
> + *
>   *		user_name:		$NAME variable if set, otherwise
>   *					read from /etc/passwd
>   *
> @@ -249,6 +258,7 @@ notmuch_config_open (void *ctx,
>      size_t tmp;
>      char *notmuch_config_env = NULL;
>      int file_had_database_group;
> +    int file_had_hooks_group;
>      int file_had_new_group;
>      int file_had_user_group;
>      int file_had_maildir_group;
> @@ -276,6 +286,7 @@ notmuch_config_open (void *ctx,
>  
>      config->is_new = FALSE;
>      config->database_path = NULL;
> +    config->hooks_path = NULL;
>      config->user_name = NULL;
>      config->user_primary_email = NULL;
>      config->user_other_email = NULL;
> @@ -333,6 +344,7 @@ notmuch_config_open (void *ctx,
>       */
>      file_had_database_group = g_key_file_has_group (config->key_file,
>  						    "database");
> +    file_had_hooks_group = g_key_file_has_group (config->key_file, "hooks");
>      file_had_new_group = g_key_file_has_group (config->key_file, "new");
>      file_had_user_group = g_key_file_has_group (config->key_file, "user");
>      file_had_maildir_group = g_key_file_has_group (config->key_file, "maildir");
> @@ -350,6 +362,13 @@ notmuch_config_open (void *ctx,
>  	talloc_free (path);
>      }
>  
> +    if (notmuch_config_get_hooks_path (config) == NULL) {
> +	char *path = talloc_asprintf (config, "%s/.notmuch/hooks",
> +				    notmuch_config_get_database_path (config));
> +	notmuch_config_set_hooks_path (config, path);
> +	talloc_free (path);
> +    }
> +
>      if (notmuch_config_get_user_name (config) == NULL) {
>  	char *name = getenv ("NAME");
>  	if (name)
> @@ -432,6 +451,10 @@ notmuch_config_open (void *ctx,
>  	g_key_file_set_comment (config->key_file, "database", NULL,
>  				database_config_comment, NULL);
>  
> +    if (! file_had_hooks_group)
> +	g_key_file_set_comment (config->key_file, "hooks", NULL,
> +				hooks_config_comment, NULL);
> +
>      if (! file_had_new_group)
>  	g_key_file_set_comment (config->key_file, "new", NULL,
>  				new_config_comment, NULL);
> @@ -616,6 +639,19 @@ notmuch_config_set_database_path (notmuch_config_t *config,
>  }
>  
>  const char *
> +notmuch_config_get_hooks_path (notmuch_config_t *config)
> +{
> +    return _config_get (config, &config->hooks_path, "hooks", "path");
> +}
> +
> +void
> +notmuch_config_set_hooks_path (notmuch_config_t *config,
> +				  const char *hooks_path)
> +{
> +    _config_set (config, &config->hooks_path, "hooks", "path", hooks_path);
> +}
> +
> +const char *
>  notmuch_config_get_user_name (notmuch_config_t *config)
>  {
>      return _config_get (config, &config->user_name, "user", "name");
> @@ -779,6 +815,8 @@ notmuch_config_command_get (notmuch_config_t *config, char *item)
>  {
>      if (strcmp(item, "database.path") == 0) {
>  	printf ("%s\n", notmuch_config_get_database_path (config));
> +    } else if (strcmp(item, "hooks.path") == 0) {
> +	printf ("%s\n", notmuch_config_get_hooks_path (config));
>      } else if (strcmp(item, "user.name") == 0) {
>  	printf ("%s\n", notmuch_config_get_user_name (config));
>      } else if (strcmp(item, "user.primary_email") == 0) {
> diff --git a/notmuch-insert.c b/notmuch-insert.c
> index 131f09e..fe6d038 100644
> --- a/notmuch-insert.c
> +++ b/notmuch-insert.c
> @@ -447,6 +447,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
>      notmuch_database_t *notmuch;
>      struct sigaction action;
>      const char *db_path;
> +    const char *hooks_path;
>      const char **new_tags;
>      size_t new_tags_length;
>      tag_op_list_t *tag_ops;
> @@ -477,6 +478,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
>      notmuch_process_shared_options (argv[0]);
>  
>      db_path = notmuch_config_get_database_path (config);
> +    hooks_path = notmuch_config_get_hooks_path (config);
>      new_tags = notmuch_config_get_new_tags (config, &new_tags_length);
>      synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
>  
> @@ -574,7 +576,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
>  
>      if (! no_hooks && status == NOTMUCH_STATUS_SUCCESS) {
>  	/* Ignore hook failures. */
> -	notmuch_run_hook (db_path, "post-insert");
> +	notmuch_run_hook (hooks_path, "post-insert");
>      }
>  
>      return status ? EXIT_FAILURE : EXIT_SUCCESS;
> diff --git a/notmuch-new.c b/notmuch-new.c
> index 799fec2..c71fd45 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -936,6 +936,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>      int ret = 0;
>      struct stat st;
>      const char *db_path;
> +    const char *hooks_path;
>      char *dot_notmuch_path;
>      struct sigaction action;
>      _filename_node_t *f;
> @@ -971,6 +972,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>      add_files_state.new_ignore = notmuch_config_get_new_ignore (config, &add_files_state.new_ignore_length);
>      add_files_state.synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
>      db_path = notmuch_config_get_database_path (config);
> +    hooks_path = notmuch_config_get_hooks_path (config);
>  
>      for (i = 0; i < add_files_state.new_tags_length; i++) {
>  	const char *error_msg;
> @@ -984,7 +986,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>      }
>  
>      if (!no_hooks) {
> -	ret = notmuch_run_hook (db_path, "pre-new");
> +	ret = notmuch_run_hook (hooks_path, "pre-new");
>  	if (ret)
>  	    return EXIT_FAILURE;
>      }
> @@ -1149,7 +1151,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>      notmuch_database_destroy (notmuch);
>  
>      if (!no_hooks && !ret && !interrupted)
> -	ret = notmuch_run_hook (db_path, "post-new");
> +	ret = notmuch_run_hook (hooks_path, "post-new");
>  
>      return ret || interrupted ? EXIT_FAILURE : EXIT_SUCCESS;
>  }
> -- 
> 2.9.2
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch