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

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

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

> Hi,
>
> Thanks Tomi and David for the feedback!
>
> On Fri, Aug 26, 2016 at 02:32:19PM +0300, Tomi Ollila wrote:
>
>> ... but I can think of one problem there (if my memory server correctly)
>
> Yeah, I didn't think of that. I have been thinking about how to make the
> generated configuration show only the options that differ from the
> default, and have the default options commented out, but it got a bit too
> involved for me.
>
> However, I think I have another solution, and I included a updated patch
> for this. There is only one (*) difference from the first patch:
>
> When reading the configuration file and "hooks.path" is either unset or
> set to an empty string, we set config->hooks_path to be the expanded
> path "database.path/.notmuch/hooks", but we set the value of hooks.path in
> config->key_file to be an empty string. That way, when calling
> "notmuch_config_get_hooks_path", the expanded path gets returned, but when
> saving the config file, either from "notmuch setup" or "notmuch config set",
> the value will still be an empty string, given that it wasn't changed.
>
> I believe this is the easiest fix, and if this sounds good, I will start
> working on the tests.

I kinda like how this would work...

The code looked pretty good -- when did I git am to the email content
I got all from the beginning of this email to the commit message --
so before next patches use git-format-patch and git-am... Check

https://notmuchmail.org/contributing/ for more information...

2 things that came up after quick view

1) there is one indentation mismatch ;/

2) I wonder whether calling notmuch_config_get_hooks_path() could
be "lazier".... ARGH no :( -- it would make notmuch_config_get_hooks_path()
have different code than others and ... (**)

3) I don't see any calls to notmuch_config_set_hooks_path()

Tomi


(**) pain tolerance exceeded in a hurry (already 20 minutes late)...

>
> (*) Except for some small changes suggested by David Bremner.
>
> Best,
> Erik
>
> From 9ddb73f8c373d3de8d54a50378653b44edf60ddc Mon Sep 17 00:00:00 2001
> From: Erik Rybakken <erik.rybakken@math.ntnu.no>
> Date: Sat, 27 Aug 2016 11:34:00 +0200
> Subject: [PATCH] Add hooks.path option
>
> ---
>  NEWS                        |  7 +++++++
>  doc/man1/notmuch-config.rst |  8 ++++++++
>  doc/man5/notmuch-hooks.rst  |  7 ++++---
>  hooks.c                     |  5 ++---
>  notmuch-client.h            |  9 ++++++++-
>  notmuch-config.c            | 42 ++++++++++++++++++++++++++++++++++++++++++
>  notmuch-insert.c            |  4 +++-
>  notmuch-new.c               |  6 ++++--
>  8 files changed, 78 insertions(+), 10 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 3a9c8d3..8ddb379 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,6 +1,13 @@
>  Notmuch 0.23 (UNRELEASED)
>  =========================
>  
> +Command Line Interface
> +-------
> +
> +Add option `hooks.path` for setting the directory for hooks. If
> +unset, or set to an empty string, 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..21652d3 100644
> --- a/doc/man1/notmuch-config.rst
> +++ b/doc/man1/notmuch-config.rst
> @@ -51,6 +51,14 @@ The available configuration items are described below.
>  
>          Default: ``$MAILDIR`` variable if set, otherwise ``$HOME/mail``.
>  
> +    **hooks.path**
> +        The directory where your hooks exist. If this value is empty or
> +        not set, it will be interpreted as the sub-directory
> +        ``.notmuch/hooks`` of the path configured in the ``database.path``
> +        option.
> +
> +        Default: empty string.
> +
>      **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..6b6833d 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -38,6 +38,14 @@ 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 exist. If this value is empty, it will be interpreted\n"
> +    " as the sub-directory \".notmuch/hooks\" of the path configured in the\n"
> +    " \"database.path\" option.\n";
> +
>  static const char new_config_comment[] =
>      " Configuration for \"notmuch new\"\n"
>      "\n"
> @@ -115,6 +123,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 +237,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 +260,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 +288,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 +346,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 +364,15 @@ notmuch_config_open (void *ctx,
>  	talloc_free (path);
>      }
>  
> +    const char *hooks_path = notmuch_config_get_hooks_path (config);
> +    if (hooks_path == NULL || strcmp(hooks_path, "") == 0) {
> +	char *path = talloc_asprintf (config, "%s/.notmuch/hooks",
> +				    notmuch_config_get_database_path (config));
> +	config->hooks_path = talloc_strdup (config, path);
> +	talloc_free (path);
> +  g_key_file_set_string (config->key_file, "hooks", "path", "");
> +    }
> +
>      if (notmuch_config_get_user_name (config) == NULL) {
>  	char *name = getenv ("NAME");
>  	if (name)
> @@ -432,6 +455,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 +643,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 +819,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.3