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

[Austin Clements]

Quoth Jani Nikula on Dec 09 at  1:55 pm:
> On Thu, 8 Dec 2011 18:34:29 -0500, Austin Clements <amdragon at 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.

Consider me convinced.