Date: Fri, 9 Dec 2011 10:59:39 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Jani Nikula <jani@nikula.org>
Subject: Re: [PATCH v4 1/3] cli: introduce the concept of user defined hooks
Message-ID: <20111209155939.GF3190@mit.edu>
References: <cover.1323384304.git.jani@nikula.org>
	<1e4ddb5e3a6b980e47418f67e16709a42e63bc47.1323384304.git.jani@nikula.org>
	<20111208233429.GA3190@mit.edu> <87r50dyhmg.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87r50dyhmg.fsf@nikula.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Jani Nikula on Dec 09 at  1:55 pm:
> 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.

Consider me convinced.