Re: [PATCH v2 1/2] cli: introduce the concept of user defined hooks

[Jani Nikula <jani@nikula.org>]

On Sat, 3 Dec 2011 22:42:10 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> I like it.  See below for some nits.

Thanks for the review!

> Quoth Jani Nikula on Dec 04 at  1:16 am:
> > Add mechanism for running user defined hooks. Hooks are executables or
> > symlinks to executables stored under the new notmuch hooks directory,
> > <database-path>/.notmuch/hooks.
> > 
> > No hooks are introduced here, but adding support for a hook is now a simple
> > matter of calling the new notmuch_run_hook() function at an appropriate
> > location with the hook name.
> > 
> > Signed-off-by: Jani Nikula <jani@nikula.org>
> > 
> > ---
> > 
> > v2: Switch to git style hooks in a hook directory as suggested by Austin
> > Clements in IRC. Update manpage and add polish.
> > ---
> >  Makefile.local   |    1 +
> >  notmuch-client.h |    3 ++
> >  notmuch-hook.c   |   63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 67 insertions(+), 0 deletions(-)
> >  create mode 100644 notmuch-hook.c
> > 
> > diff --git a/Makefile.local b/Makefile.local
> > index c94402b..a1665e1 100644
> > --- a/Makefile.local
> > +++ b/Makefile.local
> > @@ -302,6 +302,7 @@ notmuch_client_srcs =		\
> >  	notmuch-config.c	\
> >  	notmuch-count.c		\
> >  	notmuch-dump.c		\
> > +	notmuch-hook.c		\
> >  	notmuch-new.c		\
> >  	notmuch-reply.c		\
> >  	notmuch-restore.c	\
> > diff --git a/notmuch-client.h b/notmuch-client.h
> > index b50cb38..a91ad6c 100644
> > --- a/notmuch-client.h
> > +++ b/notmuch-client.h
> > @@ -235,6 +235,9 @@ void
> >  notmuch_config_set_maildir_synchronize_flags (notmuch_config_t *config,
> >  					      notmuch_bool_t synchronize_flags);
> >  
> > +int
> > +notmuch_run_hook (const char *db_path, const char *hook);
> > +
> >  notmuch_bool_t
> >  debugger_is_active (void);
> >  
> > diff --git a/notmuch-hook.c b/notmuch-hook.c
> > new file mode 100644
> > index 0000000..fc32044
> > --- /dev/null
> > +++ b/notmuch-hook.c
> > @@ -0,0 +1,63 @@
> > +/* notmuch - Not much of an email program, (just index and search)
> > + *
> > + * This file is part of notmuch.
> > + *
> > + * Copyright © 2011 Jani Nikula
> > + *
> > + * This program is free software: you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation, either version 3 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see http://www.gnu.org/licenses/ .
> > + *
> > + * Author: Jani Nikula <jani@nikula.org>
> > + */
> > +
> > +#include "notmuch-client.h"
> > +
> > +int
> > +notmuch_run_hook (const char *db_path, const char *hook)
> > +{
> > +    char *hook_path;
> > +    int status = 0;
> 
> You use status as both a notmuch_status_t and for generic C library
> results.  This seems a little weird.  You may or may not want to do
> anything about it.

True, it's not consistent. I'll want to do something about it. I wonder
if it's worth returning anything other than ok/fail from this function
anyway.

There seems to be some confusion in notmuch_status_t usage across
notmuch cli. Should notmuch cli return a notmuch_status_t as exit
status? It currently does at least in some cases, but it also returns
plain 1 too which is (unintentionally) NOTMUCH_STATUS_OUT_OF_MEMORY.

Does it make sense for the cli to use the lib statuses internally
anyway; you wouldn't want to add new status codes to the lib just to be
able to use them in cli.

> > +
> > +    if (asprintf (&hook_path, "%s/%s/%s/%s", db_path, ".notmuch", "hooks",
> > +		  hook) == -1)
> 
> asprintf isn't very portable.  Perhaps talloc_asprintf would be
> better?  (And more idiomatic.)

Agreed.

> > +	return NOTMUCH_STATUS_OUT_OF_MEMORY;
> > +
> > +    if (access (hook_path, X_OK) == -1) {
> > +	/* Ignore ENOENT. It's okay not to have a hook, hook dir, or even
> > +	 * notmuch dir. Dangling symbolic links also result in ENOENT, but
> > +	 * we'll ignore that too for simplicity. */
> > +	if (errno != ENOENT) {
> > +	    fprintf (stderr, "Error: %s hook access failed: %s\n", hook,
> > +		     strerror (errno));
> > +	    status = NOTMUCH_STATUS_FILE_ERROR;
> > +	}
> > +	goto DONE;
> > +    }
> > +
> > +    status = system (hook_path);
> 
> It would probably be better to fork/execl.  system is sensitive to all
> sorts of weird things because it invokes the shell (e.g., spaces or
> funny characters in db_path) and it plays funny games with signals.

Agreed. I initially chose system() because it is simple to use and
allowed shell expressions (pipes, redirects, etc.) within the config
file in v1 of the patches. But that's not applicable in v2 anyway.

> Really proper error handling with fork/exec is a pain, but I think you
> can get away with something simpler and even get rid of the access
> call in the process.  Something like
> 
> ret = fork();
> if (ret < 0) ...
> else if (ret == 0) {
>   ret = execl(hook_path, hook_path, NULL);
>   if (ret != ENOENT && ret != EACCESS)
>     print a real error message
>   exit(0);
> } else {
>   waitpid(ret, &status, 0);
>   if (status) .. checks you do now ..
> }
> 
> I guess this wastes a fork if there is no hook script, so maybe the
> access call is worth doing anyway.

I'll look into this.

> > +    if (status) {
> > +	if (WIFSIGNALED(status))
> > +	    fprintf(stderr, "Error: %s hook terminated with signal %d\n", hook,
> > +		    WTERMSIG(status));
> > +	else
> > +	    fprintf(stderr, "Error: %s hook failed with status %d\n", hook,
> > +		    WEXITSTATUS(status));
> > +
> > +	status = NOTMUCH_STATUS_FILE_ERROR; /* Close enough */
> > +    }
> > +
> > +  DONE:
> > +    free (hook_path);
> > +
> > +    return status;
> > +}