Re: [PATCH v2 1/7] run-command: add preliminary support for multiple hooks

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 5540 bytes --]

On Tue, May 14, 2019 at 05:12:39PM +0200, Johannes Schindelin wrote:
> Hi brian,
> 
> On Tue, 14 May 2019, brian m. carlson wrote:
> 
> > diff --git a/builtin/commit.c b/builtin/commit.c
> > index 833ecb316a..29bf80e0d1 100644
> > --- a/builtin/commit.c
> > +++ b/builtin/commit.c
> > @@ -943,7 +943,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> >  		return 0;
> >  	}
> >
> > -	if (!no_verify && find_hook("pre-commit")) {
> > +	if (!no_verify && find_hooks("pre-commit", NULL)) {
> 
> Hmm. This makes me concerned, as `find_hook()` essentially boiled down to
> a semi-fast `stat()` check, but `find_hooks()` needs to really look,
> right?
> 
> It might make sense to somehow keep the list of discovered and executed
> hooks, as we already have a call to `run_commit_hook()` to execute the
> `pre-commit` hook at the beginning of this function.

With NULL as an argument, we return 1 as soon as we know there's a
single hook, so this is fairly optimized. I've tried to make it as cheap
as possible to check.

> Speaking of which... Shouldn't that `run_commit_hook()` call be adjusted
> at the same time, or else we have an inconsistent situation?

Nope, it calls run_hook_ve, which is updated.

> > diff --git a/run-command.c b/run-command.c
> > index 3449db319b..eb075ac86b 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -1308,53 +1308,143 @@ int async_with_fork(void)
> >  #endif
> >  }
> >
> > +/*
> > + * Return 1 if a hook exists at path (which may be modified) using access(2)
> > + * with check (which should be F_OK or X_OK), 0 otherwise. If strip is true,
> > + * additionally consider the same filename but with STRIP_EXTENSION added.
> > + * If check is X_OK, warn if the hook exists but is not executable.
> > + */
> > +static int has_hook(struct strbuf *path, int strip, int check)
> > +{
> > +	if (access(path->buf, check) < 0) {
> > +		int err = errno;
> > +
> > +		if (strip) {
> > +#ifdef STRIP_EXTENSION
> > +			strbuf_addstr(path, STRIP_EXTENSION);
> > +			if (access(path->buf, check) >= 0)
> > +				return 1;
> > +			if (errno == EACCES)
> > +				err = errno;
> > +#endif
> > +		}
> 
> How about simply guarding the entire `if()`? It is a bit unusual to guard
> *only* the inside block ;-)

I can make that change.

> > +
> > +		if (err == EACCES && advice_ignored_hook) {
> 
> Didn't you want to test for `X_OK` here, too? The code comment above the
> function seems to suggest that.

Yeah, that makes sense. I'll do that.

> > +			static struct string_list advise_given = STRING_LIST_INIT_DUP;
> > +
> > +			if (!string_list_lookup(&advise_given, path->buf)) {
> > +				string_list_insert(&advise_given, path->buf);
> > +				advise(_("The '%s' hook was ignored because "
> > +					 "it's not set as executable.\n"
> > +					 "You can disable this warning with "
> > +					 "`git config advice.ignoredHook false`."),
> > +				       path->buf);
> > +			}
> > +		}
> > +		return 0;
> > +	}
> > +	return 1;
> 
> Wouldn't it make sense to do this very early? Something like
> 
> 	if (!access(path->buf, check))
> 		return 1;
> 
> first thing in the function, that that part is already out of the way and
> we don't have to indent so much.

Sure. That's a nice improvement.

> >  const char *find_hook(const char *name)
> >  {
> >  	static struct strbuf path = STRBUF_INIT;
> >
> >  	strbuf_reset(&path);
> >  	strbuf_git_path(&path, "hooks/%s", name);
> > -	if (access(path.buf, X_OK) < 0) {
> > -		int err = errno;
> > -
> > -#ifdef STRIP_EXTENSION
> > -		strbuf_addstr(&path, STRIP_EXTENSION);
> > -		if (access(path.buf, X_OK) >= 0)
> > -			return path.buf;
> > -		if (errno == EACCES)
> > -			err = errno;
> > -#endif
> > -
> > -		if (err == EACCES && advice_ignored_hook) {
> > -			static struct string_list advise_given = STRING_LIST_INIT_DUP;
> > -
> > -			if (!string_list_lookup(&advise_given, name)) {
> > -				string_list_insert(&advise_given, name);
> > -				advise(_("The '%s' hook was ignored because "
> > -					 "it's not set as executable.\n"
> > -					 "You can disable this warning with "
> > -					 "`git config advice.ignoredHook false`."),
> > -				       path.buf);
> > -			}
> > -		}
> > -		return NULL;
> 
> So that's where this comes from ;-)

Exactly. I didn't make a lot of changes.

> > +/*
> > + * Returns the paths to all hook files, or NULL if all hooks are missing or
> > + * disabled.
> 
> Left-over comment?

Yup, thanks for pointing it out.

> > + * Returns 1 if there are hooks; 0 otherwise. If hooks is not NULL, stores the
> > + * names of the hooks into them in the order they should be executed.
> > + */
> > +int find_hooks(const char *name, struct string_list *hooks);
> > +/*
> > + * Invokes the handler function once for each hook. Returns 0 if all hooks were
> > + * successful, or the exit status of the first failing hook.
> > + */
> > +int for_each_hook(const char *name,
> > +		  int (*handler)(const char *name, const char *path, void *),
> > +		  void *data);
> 
> My gut says that it would make sense for `struct repository *` to sprout a
> hashmap for hook lookup that would be populated by demand, and allowed
> things like
> 
> 	hash_hook(r, "pre-commit")

Knowing that we have an optimized check, do you still think we should do
this, or are you okay leaving it as it is?
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]