Re: [PATCH v2 2/2] cli: add support for pre and post notmuch new hooks

[Jani Nikula <jani@nikula.org>]

On Sat, 3 Dec 2011 23:00:47 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> 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?

Yes.

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

Oops. I just took it from the --verbose handling above without
checking. I'll fix this one, as I don't think everyone else making the
same mistake is a good reason to repeat it. The rest will be taken care
of in the Great Argument Parsing Overhaul which is in the works...

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

I wasn't sure either, but I ended up thinking that the hooks become part
of 'notmuch new' and claiming success when a hook fails is not quite
right. This might have importance if scripting 'notmuch new'.

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

That makes two of us; I shamelessly admit I'm just following what
everyone else seems to be doing... cargo cult.

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

Better, thanks.

> > +
> > +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 ..."?

Yeah, I was trying to cover also the errors that may happen before the
script is actually run, but perhaps that's not important.

> > +.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 ..."?

Not being a native speaker, I'll take your word for it. :)

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

Ditto.

> 
> > +.RE
> >  .SH ENVIRONMENT
> >  The following environment variables can be used to control the
> >  behavior of notmuch.

Many thanks for your thorough review, as always!


BR,
Jani.