Re: [PATCH v4 1/3] cli: introduce the concept of user defined hooks

[Jani Nikula <jani@nikula.org>]

On Thu, 8 Dec 2011 18:34:29 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Jani Nikula on Dec 09 at 12:48 am:
> > +    /* Check access before fork() for speed and simplicity of error handling. */
> > +    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 = 1;
> > +	}
> 
> Is it the intent that a present but non-executable hook (errno ==
> EACCES) will print the above error message and return with a failure?
> I'm pretty sure this differs from the behavior of git hooks.

It differs from git, and it is intentional. Git bails out with success
status, without even a warning, for *all* access() failures. That may be
fine for git (which generally expects the user to know what he's doing)
but I'd argue notmuch should let the user know something is wrong.

Also for EACCES, IMHO failing is more useful to the user than silently
ignoring. If the hook exists, but isn't executable, I think it's way
more likely that the user forgot to chmod +x than intentionally dropped
x so the hook would not be run. (And I think we agreed on IRC that in
the future, sample hooks would be named hook.sample and have executable
bit set.)

Anyway, this is my opinion; it's not a big deal to change if there are
compelling reasons to ignore EACCES that I didn't think of.


BR,
Jani.