Date: Sat, 3 Dec 2011 23:00:47 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Jani Nikula <jani@nikula.org>
Subject: Re: [PATCH v2 2/2] cli: add support for pre and post notmuch new
 hooks
Message-ID: <20111204040047.GB16405@mit.edu>
References:
 <6688b09fffa2a66b496af78008102f88ab4e9450.1322953841.git.jani@nikula.org>
	<6ccaa31da55b0dfc9e339780e43e24e1489235e8.1322953841.git.jani@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To:
 <6ccaa31da55b0dfc9e339780e43e24e1489235e8.1322953841.git.jani@nikula.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Jani Nikula on Dec 04 at  1:16 am:
> Run notmuch new pre and post hooks, named "pre-new" and "post-new", if
> present in the notmuch hooks directory. The hooks will be run before and
> after incorporating new messages to the database.
> 
> Typical use cases for pre-new and post-new hooks are fetching or delivering
> new mail to the maildir, and custom tagging of the mail incorporated to the
> database.
> 
> Also add command line option --no-hooks to notmuch new to bypass the hooks.
> 
> Signed-off-by: Jani Nikula <jani@nikula.org>
> ---
>  notmuch-new.c |   12 ++++++++++++
>  notmuch.1     |   50 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 61 insertions(+), 1 deletions(-)
> 
> diff --git a/notmuch-new.c b/notmuch-new.c
> index 81a9350..27dde0c 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -811,6 +811,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
>      _filename_node_t *f;
>      int i;
>      notmuch_bool_t timer_is_active = FALSE;
> +    int run_hooks = 1;

notmuch_bool_t?

>  
>      add_files_state.verbose = 0;
>      add_files_state.output_is_a_tty = isatty (fileno (stdout));
> @@ -820,6 +821,8 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
>      for (i = 0; i < argc && argv[i][0] == '-'; i++) {
>  	if (STRNCMP_LITERAL (argv[i], "--verbose") == 0) {
>  	    add_files_state.verbose = 1;
> +	} else if (STRNCMP_LITERAL (argv[i], "--no-hooks") == 0) {

I see this mistake all over notmuch, so maybe it's better to
perpetuate it here and fix it everywhere in another patch, but this
should be strcmp, not STRNCMP_LITERAL.  STRNCMP_LITERAL is the right
thing for options that take values, but for boolean options like this,
it will accept
  notmuch new --no-hooks-just-kidding

> +	    run_hooks = 0;
>  	} else {
>  	    fprintf (stderr, "Unrecognized option: %s\n", argv[i]);
>  	    return 1;
> @@ -833,6 +836,12 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
>      add_files_state.synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
>      db_path = notmuch_config_get_database_path (config);
>  
> +    if (run_hooks) {
> +	ret = notmuch_run_hook (db_path, "pre-new");
> +	if (ret)
> +	    return ret;
> +    }
> +
>      dot_notmuch_path = talloc_asprintf (ctx, "%s/%s", db_path, ".notmuch");
>  
>      if (stat (dot_notmuch_path, &st)) {
> @@ -981,5 +990,8 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
>  
>      notmuch_database_close (notmuch);
>  
> +    if (run_hooks && !ret && !interrupted)
> +	ret = notmuch_run_hook (db_path, "post-new");

Does it matter at this point if the hook fails?  I'm not sure.

> +
>      return ret || interrupted;
>  }
> diff --git a/notmuch.1 b/notmuch.1
> index 92931d7..66f82e9 100644
> --- a/notmuch.1
> +++ b/notmuch.1

I am willfully ignorant of nroff, so somebody else will have to
comment if any of the nroff code/formatting is wrong.

> @@ -85,7 +85,7 @@ The
>  command is used to incorporate new mail into the notmuch database.
>  .RS 4
>  .TP 4
> -.B new
> +.BR new " [options...]"
>  
>  Find and import any new messages to the database.
>  
> @@ -118,6 +118,22 @@ if
>  has previously been completed, but
>  .B "notmuch new"
>  has not previously been run.
> +
> +The
> +.B new
> +command supports hooks. See the
> +.B "HOOKS"
> +section below for more details on hooks.
> +
> +Supported options for
> +.B new
> +include
> +.RS 4
> +.TP 4
> +.BR \-\-no\-hooks
> +
> +Prevents hooks from being run.
> +.RE
>  .RE
>  
>  Several of the notmuch commands accept search terms with a common
> @@ -705,6 +721,38 @@ specify a date range to return messages from 2009\-10\-01 until the
>  current time:
>  
>  	$(date +%s \-d 2009\-10\-01)..$(date +%s)
> +.SH HOOKS
> +Hooks are scripts (or arbitrary executables or symlinks to such) you can place
> +in the notmuch hooks directory to trigger action at certain points. The hooks
> +directory is .notmuch/hooks within the database directory. The user must have
> +executable permission set on the scripts.

Could be more concise.  Maybe something like "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."

> +
> +The currently available hooks are described below.
> +.RS 4
> +.TP 4
> +.B pre\-new
> +This hook is invoked by the
> +.B new
> +command before scanning or importing new messages into the database. Any errors
> +in running the hook will abort further processing of the

"If this script exits with a non-zero status, notmuch will abort ..."?

> +.B new
> +command.
> +
> +Typical use case for this hook is fetching or delivering new mail to be imported
> +into the database.

Perhaps "Typically this hook is used for ..."?

> +.RE
> +.RS 4
> +.TP 4
> +.B post\-new
> +This hook is invoked by the
> +.B new
> +command after new messages have been imported into the database and initial tags
> +have been applied. The hook will not be run if there have been any errors during
> +the scan or import.
> +
> +Typical use case for this hook is performing additional query based tagging on
> +the imported messages.

Same thing.  "Typically this hook is used to perform ..."?  Also,
"query-based".

> +.RE
>  .SH ENVIRONMENT
>  The following environment variables can be used to control the
>  behavior of notmuch.