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:
signature.asc
Description: PGP signature
