| Message ID | 20190514002332.121089-2-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Multiple hook support | expand |
On Tue, May 14, 2019 at 7:24 AM brian m. carlson <sandals@crustytoothpaste.net> wrote: > -int run_hook_ve(const char *const *env, const char *name, va_list args) > +int find_hooks(const char *name, struct string_list *list) > { > - struct child_process hook = CHILD_PROCESS_INIT; > - const char *p; > + struct strbuf path = STRBUF_INIT; > + DIR *d; > + struct dirent *de; > > - p = find_hook(name); > - if (!p) > + /* > + * We look for a single hook. If present, return it, and skip the > + * individual directories. > + */ > + strbuf_git_path(&path, "hooks/%s", name); > + if (has_hook(&path, 1, X_OK)) { > + if (list) > + string_list_append(list, path.buf); > + return 1; > + } > + > + if (has_hook(&path, 1, F_OK)) > return 0; > > - argv_array_push(&hook.args, p); > - while ((p = va_arg(args, const char *))) > - argv_array_push(&hook.args, p); > - hook.env = env; > + strbuf_reset(&path); > + strbuf_git_path(&path, "hooks/%s.d", name); > + d = opendir(path.buf); > + if (!d) { > + if (list) > + string_list_clear(list, 0); > + return 0; > + } > + while ((de = readdir(d))) { > + if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, "..")) > + continue; > + strbuf_reset(&path); > + strbuf_git_path(&path, "hooks/%s.d/%s", name, de->d_name); > + if (has_hook(&path, 0, X_OK)) { Do we want to support hooks in subdirectories as well (if so, using dir-iterator.h might be more appropriate) If not, what happens when "path" is a directory. X_OK could be set (and often are) on them too. > + if (list) > + string_list_append(list, path.buf); > + else > + return 1; > + } > + } > + closedir(d); > + strbuf_reset(&path); > + if (!list->nr) { > + return 0; > + } > + > + string_list_sort(list); This is going to be interesting on case-insensitive filesystems because we do strcmp by default, not the friendlier fspathcmp. And the ".exe" suffix might affect sort order too. But I suppose we just need to be clear here (in documentation). They can always prefix with a number to keep hook files in expected order. > + return 1; > +} > + > diff --git a/run-command.h b/run-command.h > index a6950691c0..1b3677fcac 100644 > --- a/run-command.h > +++ b/run-command.h > @@ -4,6 +4,7 @@ > #include "thread-utils.h" > > #include "argv-array.h" > +#include "string-list.h" 'struct string_list;' should be enough (and a bit lighter) although I don't suppose it really matters. > > struct child_process { > const char **argv;
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. Speaking of which... Shouldn't that `run_commit_hook()` call be adjusted at the same time, or else we have an inconsistent situation? > /* > * Re-read the index as pre-commit hook could have updated it, > * and write it out as a tree. We must do this before we invoke > 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 ;-) > + > + 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. > + 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. > +} > + > 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 ;-) > + if (has_hook(&path, 1, X_OK)) { > + return path.buf; > } > - return path.buf; > + return NULL; > } > > -int run_hook_ve(const char *const *env, const char *name, va_list args) > +int find_hooks(const char *name, struct string_list *list) > { > - struct child_process hook = CHILD_PROCESS_INIT; > - const char *p; > + struct strbuf path = STRBUF_INIT; > + DIR *d; > + struct dirent *de; > > - p = find_hook(name); > - if (!p) > + /* > + * We look for a single hook. If present, return it, and skip the > + * individual directories. > + */ > + strbuf_git_path(&path, "hooks/%s", name); > + if (has_hook(&path, 1, X_OK)) { > + if (list) > + string_list_append(list, path.buf); > + return 1; > + } > + > + if (has_hook(&path, 1, F_OK)) > return 0; > > - argv_array_push(&hook.args, p); > - while ((p = va_arg(args, const char *))) > - argv_array_push(&hook.args, p); > - hook.env = env; > + strbuf_reset(&path); > + strbuf_git_path(&path, "hooks/%s.d", name); > + d = opendir(path.buf); > + if (!d) { > + if (list) > + string_list_clear(list, 0); > + return 0; > + } > + while ((de = readdir(d))) { > + if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, "..")) > + continue; > + strbuf_reset(&path); > + strbuf_git_path(&path, "hooks/%s.d/%s", name, de->d_name); > + if (has_hook(&path, 0, X_OK)) { > + if (list) > + string_list_append(list, path.buf); > + else > + return 1; > + } > + } > + closedir(d); > + strbuf_reset(&path); > + if (!list->nr) { > + return 0; > + } > + > + string_list_sort(list); > + return 1; > +} > + > +int for_each_hook(const char *name, > + int (*handler)(const char *name, const char *path, void *), > + void *data) > +{ > + struct string_list paths = STRING_LIST_INIT_DUP; > + int i, ret = 0; > + > + find_hooks(name, &paths); > + for (i = 0; i < paths.nr; i++) { > + const char *p = paths.items[i].string; > + > + ret = handler(name, p, data); > + if (ret) > + break; > + } > + > + string_list_clear(&paths, 0); > + return ret; > +} > + > +struct hook_data { > + const char *const *env; > + struct string_list *args; > +}; > + > +static int do_run_hook_ve(const char *name, const char *path, void *cb) > +{ > + struct hook_data *data = cb; > + struct child_process hook = CHILD_PROCESS_INIT; > + struct string_list_item *arg; > + > + argv_array_push(&hook.args, path); > + for_each_string_list_item(arg, data->args) { > + argv_array_push(&hook.args, arg->string); > + } > + > + hook.env = data->env; > hook.no_stdin = 1; > hook.stdout_to_stderr = 1; > hook.trace2_hook_name = name; > @@ -1362,6 +1452,21 @@ int run_hook_ve(const char *const *env, const char *name, va_list args) > return run_command(&hook); > } > > +int run_hook_ve(const char *const *env, const char *name, va_list args) > +{ > + struct string_list arglist = STRING_LIST_INIT_DUP; > + struct hook_data data = {env, &arglist}; > + const char *p; > + int ret = 0; > + > + while ((p = va_arg(args, const char *))) > + string_list_append(&arglist, p); > + > + ret = for_each_hook(name, do_run_hook_ve, &data); > + string_list_clear(&arglist, 0); > + return ret; > +} > + > int run_hook_le(const char *const *env, const char *name, ...) > { > va_list args; > diff --git a/run-command.h b/run-command.h > index a6950691c0..1b3677fcac 100644 > --- a/run-command.h > +++ b/run-command.h > @@ -4,6 +4,7 @@ > #include "thread-utils.h" > > #include "argv-array.h" > +#include "string-list.h" > > struct child_process { > const char **argv; > @@ -68,6 +69,20 @@ int run_command(struct child_process *); > * overwritten by further calls to find_hook and run_hook_*. > */ > extern const char *find_hook(const char *name); > +/* > + * Returns the paths to all hook files, or NULL if all hooks are missing or > + * disabled. Left-over comment? > + * 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") I can't follow up with code, though, as I'm off for the day! Ciao, Dscho > LAST_ARG_MUST_BE_NULL > extern int run_hook_le(const char *const *env, const char *name, ...); > extern int run_hook_ve(const char *const *env, const char *name, va_list args); > diff --git a/t/lib-hooks.sh b/t/lib-hooks.sh > new file mode 100644 > index 0000000000..721250aea0 > --- /dev/null > +++ b/t/lib-hooks.sh > @@ -0,0 +1,172 @@ > +create_multihooks () { > + mkdir -p "$MULTIHOOK_DIR" > + for i in "a $1" "b $2" "c $3" > + do > + echo "$i" | (while read script ex > + do > + mkdir -p "$MULTIHOOK_DIR" > + write_script "$MULTIHOOK_DIR/$script" <<-EOF > + mkdir -p "$OUTPUTDIR" > + touch "$OUTPUTDIR/$script" > + exit $ex > + EOF > + done) > + done > +} > + > +# Run the multiple hook tests. > +# Usage: test_multiple_hooks [--ignore-exit-status] HOOK COMMAND [SKIP-COMMAND] > +# HOOK: the name of the hook to test > +# COMMAND: command to test the hook for; takes a single argument indicating test > +# name. > +# SKIP-COMMAND: like $1, except the hook should be skipped. > +# --ignore-exit-status: the command does not fail if the exit status from the > +# hook is nonzero. > +test_multiple_hooks () { > + local must_fail cmd skip_cmd hook > + if test "$1" = "--ignore-exit-status" > + then > + shift > + else > + must_fail="test_must_fail" > + fi > + hook="$1" > + cmd="$2" > + skip_cmd="$3" > + > + HOOKDIR="$(git rev-parse --absolute-git-dir)/hooks" > + OUTPUTDIR="$(git rev-parse --absolute-git-dir)/../hook-output" > + HOOK="$HOOKDIR/$hook" > + MULTIHOOK_DIR="$HOOKDIR/$hook.d" > + rm -f "$HOOK" "$MULTIHOOK_DIR" "$OUTPUTDIR" > + > + test_expect_success "$hook: with no hook" ' > + $cmd foo > + ' > + > + if test -n "$skip_cmd" > + then > + test_expect_success "$hook: skipped hook with no hook" ' > + $skip_cmd bar > + ' > + fi > + > + test_expect_success 'setup' ' > + mkdir -p "$HOOKDIR" && > + write_script "$HOOK" <<-EOF > + mkdir -p "$OUTPUTDIR" > + touch "$OUTPUTDIR/simple" > + exit 0 > + EOF > + ' > + > + test_expect_success "$hook: with succeeding hook" ' > + test_when_finished "rm -fr \"$OUTPUTDIR\"" && > + $cmd more && > + test -f "$OUTPUTDIR/simple" > + ' > + > + if test -n "$skip_cmd" > + then > + test_expect_success "$hook: skipped but succeeding hook" ' > + test_when_finished "rm -fr \"$OUTPUTDIR\"" && > + $skip_cmd even-more && > + ! test -f "$OUTPUTDIR/simple" > + ' > + fi > + > + test_expect_success "$hook: with both simple and multiple hooks, simple success" ' > + test_when_finished "rm -fr \"$OUTPUTDIR\"" && > + create_multihooks 0 1 0 && > + $cmd yet-more && > + test -f "$OUTPUTDIR/simple" && > + ! test -f "$OUTPUTDIR/a" && > + ! test -f "$OUTPUTDIR/b" && > + ! test -f "$OUTPUTDIR/c" > + ' > + > + test_expect_success 'setup' ' > + rm -fr "$MULTIHOOK_DIR" && > + > + # now a hook that fails > + write_script "$HOOK" <<-EOF > + mkdir -p "$OUTPUTDIR" > + touch "$OUTPUTDIR/simple" > + exit 1 > + EOF > + ' > + > + test_expect_success "$hook: with failing hook" ' > + test_when_finished "rm -fr \"$OUTPUTDIR\"" && > + $must_fail $cmd another && > + test -f "$OUTPUTDIR/simple" > + ' > + > + if test -n "$skip_cmd" > + then > + test_expect_success "$hook: skipped but failing hook" ' > + test_when_finished "rm -fr \"$OUTPUTDIR\"" && > + $skip_cmd stuff && > + ! test -f "$OUTPUTDIR/simple" > + ' > + fi > + > + test_expect_success "$hook: with both simple and multiple hooks, simple failure" ' > + test_when_finished "rm -fr \"$OUTPUTDIR\"" && > + create_multihooks 0 1 0 && > + $must_fail $cmd more-stuff && > + test -f "$OUTPUTDIR/simple" && > + ! test -f "$OUTPUTDIR/a" && > + ! test -f "$OUTPUTDIR/b" && > + ! test -f "$OUTPUTDIR/c" > + ' > + > + test_expect_success "$hook: multiple hooks, all successful" ' > + test_when_finished "rm -fr \"$OUTPUTDIR\"" && > + rm -f "$HOOK" && > + create_multihooks 0 0 0 && > + $cmd content && > + test -f "$OUTPUTDIR/a" && > + test -f "$OUTPUTDIR/b" && > + test -f "$OUTPUTDIR/c" > + ' > + > + test_expect_success "$hook: hooks after first failure not executed" ' > + test_when_finished "rm -fr \"$OUTPUTDIR\"" && > + create_multihooks 0 1 0 && > + $must_fail $cmd more-content && > + test -f "$OUTPUTDIR/a" && > + test -f "$OUTPUTDIR/b" && > + ! test -f "$OUTPUTDIR/c" > + ' > + > + test_expect_success POSIXPERM "$hook: non-executable hook not executed" ' > + test_when_finished "rm -fr \"$OUTPUTDIR\"" && > + create_multihooks 0 1 0 && > + chmod -x "$MULTIHOOK_DIR/b" && > + $cmd things && > + test -f "$OUTPUTDIR/a" && > + ! test -f "$OUTPUTDIR/b" && > + test -f "$OUTPUTDIR/c" > + ' > + > + test_expect_success POSIXPERM "$hook: multiple hooks not executed if simple hook present" ' > + test_when_finished "rm -fr \"$OUTPUTDIR\" && rm -f \"$HOOK\"" && > + write_script "$HOOK" <<-EOF && > + mkdir -p "$OUTPUTDIR" > + touch "$OUTPUTDIR/simple" > + exit 0 > + EOF > + create_multihooks 0 1 0 && > + chmod -x "$HOOK" && > + $cmd other-things && > + ! test -f "$OUTPUTDIR/simple" && > + ! test -f "$OUTPUTDIR/a" && > + ! test -f "$OUTPUTDIR/b" && > + ! test -f "$OUTPUTDIR/c" > + ' > + > + test_expect_success 'cleanup' ' > + rm -fr "$MULTIHOOK_DIR" > + ' > +} > diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh > index 984889b39d..d63d059e04 100755 > --- a/t/t7503-pre-commit-hook.sh > +++ b/t/t7503-pre-commit-hook.sh > @@ -3,6 +3,7 @@ > test_description='pre-commit hook' > > . ./test-lib.sh > +. "$TEST_DIRECTORY/lib-hooks.sh" > > test_expect_success 'with no hook' ' > > @@ -136,4 +137,18 @@ test_expect_success 'check the author in hook' ' > git show -s > ' > > +commit_command () { > + echo "$1" >>file && > + git add file && > + git commit -m "$1" > +} > + > +commit_no_verify_command () { > + echo "$1" >>file && > + git add file && > + git commit --no-verify -m "$1" > +} > + > +test_multiple_hooks pre-commit commit_command commit_no_verify_command > + > test_done >
On Tue, May 14, 2019 at 07:46:17PM +0700, Duy Nguyen wrote: > On Tue, May 14, 2019 at 7:24 AM brian m. carlson > <sandals@crustytoothpaste.net> wrote: > > - argv_array_push(&hook.args, p); > > - while ((p = va_arg(args, const char *))) > > - argv_array_push(&hook.args, p); > > - hook.env = env; > > + strbuf_reset(&path); > > + strbuf_git_path(&path, "hooks/%s.d", name); > > + d = opendir(path.buf); > > + if (!d) { > > + if (list) > > + string_list_clear(list, 0); > > + return 0; > > + } > > + while ((de = readdir(d))) { > > + if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, "..")) > > + continue; > > + strbuf_reset(&path); > > + strbuf_git_path(&path, "hooks/%s.d/%s", name, de->d_name); > > + if (has_hook(&path, 0, X_OK)) { > > Do we want to support hooks in subdirectories as well (if so, using > dir-iterator.h might be more appropriate) No, I don't think so. That's not what most other software does, and I don't really think recursive behavior provides a lot of value. > If not, what happens when "path" is a directory. X_OK could be set > (and often are) on them too. We get an exec failure when trying to run it, just like if you create a directory in place of the existing hook now. I think that's a fine behavior. > > + if (list) > > + string_list_append(list, path.buf); > > + else > > + return 1; > > + } > > + } > > + closedir(d); > > + strbuf_reset(&path); > > + if (!list->nr) { > > + return 0; > > + } > > + > > + string_list_sort(list); > > This is going to be interesting on case-insensitive filesystems > because we do strcmp by default, not the friendlier fspathcmp. And the > ".exe" suffix might affect sort order too. > > But I suppose we just need to be clear here (in documentation). They > can always prefix with a number to keep hook files in expected order. Numbering is definitely the way I'd go on a case-insensitive file system. Also, I don't think the ".exe" will be a problem in practice, since generally you'd have to have a mix of binary and non-binary hooks, which is unlikely to be super common. > > diff --git a/run-command.h b/run-command.h > > index a6950691c0..1b3677fcac 100644 > > --- a/run-command.h > > +++ b/run-command.h > > @@ -4,6 +4,7 @@ > > #include "thread-utils.h" > > > > #include "argv-array.h" > > +#include "string-list.h" > > 'struct string_list;' should be enough (and a bit lighter) although I > don't suppose it really matters. I can make that change.
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?
Am 16.05.19 um 00:44 schrieb brian m. carlson: > On Tue, May 14, 2019 at 05:12:39PM +0200, Johannes Schindelin wrote: >> On Tue, 14 May 2019, brian m. carlson wrote: >>> +/* >>> + * 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. But then we'll have an unused argument in some build configurations. -- Hannes
Hi Hannes, On Thu, 16 May 2019, Johannes Sixt wrote: > Am 16.05.19 um 00:44 schrieb brian m. carlson: > > On Tue, May 14, 2019 at 05:12:39PM +0200, Johannes Schindelin wrote: > >> On Tue, 14 May 2019, brian m. carlson wrote: > >>> +/* > >>> + * 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. > > But then we'll have an unused argument in some build configurations. That's a valid point. Thanks, Dscho
On 2019-05-15 at 22:27:56, brian m. carlson wrote: > On Tue, May 14, 2019 at 07:46:17PM +0700, Duy Nguyen wrote: > > 'struct string_list;' should be enough (and a bit lighter) although I > > don't suppose it really matters. > > I can make that change. One thing I noticed when making this change is that we're going to need the definition of the struct string_list in a later patch in the series (originally to define a variable, but now to define a struct). So knowing that, I think it makes sense for us to just include the header up front, since we're going to be using it a few patches later.
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)) { /* * Re-read the index as pre-commit hook could have updated it, * and write it out as a tree. We must do this before we invoke 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 + } + + if (err == EACCES && advice_ignored_hook) { + 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; +} + 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; + if (has_hook(&path, 1, X_OK)) { + return path.buf; } - return path.buf; + return NULL; } -int run_hook_ve(const char *const *env, const char *name, va_list args) +int find_hooks(const char *name, struct string_list *list) { - struct child_process hook = CHILD_PROCESS_INIT; - const char *p; + struct strbuf path = STRBUF_INIT; + DIR *d; + struct dirent *de; - p = find_hook(name); - if (!p) + /* + * We look for a single hook. If present, return it, and skip the + * individual directories. + */ + strbuf_git_path(&path, "hooks/%s", name); + if (has_hook(&path, 1, X_OK)) { + if (list) + string_list_append(list, path.buf); + return 1; + } + + if (has_hook(&path, 1, F_OK)) return 0; - argv_array_push(&hook.args, p); - while ((p = va_arg(args, const char *))) - argv_array_push(&hook.args, p); - hook.env = env; + strbuf_reset(&path); + strbuf_git_path(&path, "hooks/%s.d", name); + d = opendir(path.buf); + if (!d) { + if (list) + string_list_clear(list, 0); + return 0; + } + while ((de = readdir(d))) { + if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, "..")) + continue; + strbuf_reset(&path); + strbuf_git_path(&path, "hooks/%s.d/%s", name, de->d_name); + if (has_hook(&path, 0, X_OK)) { + if (list) + string_list_append(list, path.buf); + else + return 1; + } + } + closedir(d); + strbuf_reset(&path); + if (!list->nr) { + return 0; + } + + string_list_sort(list); + return 1; +} + +int for_each_hook(const char *name, + int (*handler)(const char *name, const char *path, void *), + void *data) +{ + struct string_list paths = STRING_LIST_INIT_DUP; + int i, ret = 0; + + find_hooks(name, &paths); + for (i = 0; i < paths.nr; i++) { + const char *p = paths.items[i].string; + + ret = handler(name, p, data); + if (ret) + break; + } + + string_list_clear(&paths, 0); + return ret; +} + +struct hook_data { + const char *const *env; + struct string_list *args; +}; + +static int do_run_hook_ve(const char *name, const char *path, void *cb) +{ + struct hook_data *data = cb; + struct child_process hook = CHILD_PROCESS_INIT; + struct string_list_item *arg; + + argv_array_push(&hook.args, path); + for_each_string_list_item(arg, data->args) { + argv_array_push(&hook.args, arg->string); + } + + hook.env = data->env; hook.no_stdin = 1; hook.stdout_to_stderr = 1; hook.trace2_hook_name = name; @@ -1362,6 +1452,21 @@ int run_hook_ve(const char *const *env, const char *name, va_list args) return run_command(&hook); } +int run_hook_ve(const char *const *env, const char *name, va_list args) +{ + struct string_list arglist = STRING_LIST_INIT_DUP; + struct hook_data data = {env, &arglist}; + const char *p; + int ret = 0; + + while ((p = va_arg(args, const char *))) + string_list_append(&arglist, p); + + ret = for_each_hook(name, do_run_hook_ve, &data); + string_list_clear(&arglist, 0); + return ret; +} + int run_hook_le(const char *const *env, const char *name, ...) { va_list args; diff --git a/run-command.h b/run-command.h index a6950691c0..1b3677fcac 100644 --- a/run-command.h +++ b/run-command.h @@ -4,6 +4,7 @@ #include "thread-utils.h" #include "argv-array.h" +#include "string-list.h" struct child_process { const char **argv; @@ -68,6 +69,20 @@ int run_command(struct child_process *); * overwritten by further calls to find_hook and run_hook_*. */ extern const char *find_hook(const char *name); +/* + * Returns the paths to all hook files, or NULL if all hooks are missing or + * disabled. + * 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); LAST_ARG_MUST_BE_NULL extern int run_hook_le(const char *const *env, const char *name, ...); extern int run_hook_ve(const char *const *env, const char *name, va_list args); diff --git a/t/lib-hooks.sh b/t/lib-hooks.sh new file mode 100644 index 0000000000..721250aea0 --- /dev/null +++ b/t/lib-hooks.sh @@ -0,0 +1,172 @@ +create_multihooks () { + mkdir -p "$MULTIHOOK_DIR" + for i in "a $1" "b $2" "c $3" + do + echo "$i" | (while read script ex + do + mkdir -p "$MULTIHOOK_DIR" + write_script "$MULTIHOOK_DIR/$script" <<-EOF + mkdir -p "$OUTPUTDIR" + touch "$OUTPUTDIR/$script" + exit $ex + EOF + done) + done +} + +# Run the multiple hook tests. +# Usage: test_multiple_hooks [--ignore-exit-status] HOOK COMMAND [SKIP-COMMAND] +# HOOK: the name of the hook to test +# COMMAND: command to test the hook for; takes a single argument indicating test +# name. +# SKIP-COMMAND: like $1, except the hook should be skipped. +# --ignore-exit-status: the command does not fail if the exit status from the +# hook is nonzero. +test_multiple_hooks () { + local must_fail cmd skip_cmd hook + if test "$1" = "--ignore-exit-status" + then + shift + else + must_fail="test_must_fail" + fi + hook="$1" + cmd="$2" + skip_cmd="$3" + + HOOKDIR="$(git rev-parse --absolute-git-dir)/hooks" + OUTPUTDIR="$(git rev-parse --absolute-git-dir)/../hook-output" + HOOK="$HOOKDIR/$hook" + MULTIHOOK_DIR="$HOOKDIR/$hook.d" + rm -f "$HOOK" "$MULTIHOOK_DIR" "$OUTPUTDIR" + + test_expect_success "$hook: with no hook" ' + $cmd foo + ' + + if test -n "$skip_cmd" + then + test_expect_success "$hook: skipped hook with no hook" ' + $skip_cmd bar + ' + fi + + test_expect_success 'setup' ' + mkdir -p "$HOOKDIR" && + write_script "$HOOK" <<-EOF + mkdir -p "$OUTPUTDIR" + touch "$OUTPUTDIR/simple" + exit 0 + EOF + ' + + test_expect_success "$hook: with succeeding hook" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + $cmd more && + test -f "$OUTPUTDIR/simple" + ' + + if test -n "$skip_cmd" + then + test_expect_success "$hook: skipped but succeeding hook" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + $skip_cmd even-more && + ! test -f "$OUTPUTDIR/simple" + ' + fi + + test_expect_success "$hook: with both simple and multiple hooks, simple success" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + create_multihooks 0 1 0 && + $cmd yet-more && + test -f "$OUTPUTDIR/simple" && + ! test -f "$OUTPUTDIR/a" && + ! test -f "$OUTPUTDIR/b" && + ! test -f "$OUTPUTDIR/c" + ' + + test_expect_success 'setup' ' + rm -fr "$MULTIHOOK_DIR" && + + # now a hook that fails + write_script "$HOOK" <<-EOF + mkdir -p "$OUTPUTDIR" + touch "$OUTPUTDIR/simple" + exit 1 + EOF + ' + + test_expect_success "$hook: with failing hook" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + $must_fail $cmd another && + test -f "$OUTPUTDIR/simple" + ' + + if test -n "$skip_cmd" + then + test_expect_success "$hook: skipped but failing hook" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + $skip_cmd stuff && + ! test -f "$OUTPUTDIR/simple" + ' + fi + + test_expect_success "$hook: with both simple and multiple hooks, simple failure" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + create_multihooks 0 1 0 && + $must_fail $cmd more-stuff && + test -f "$OUTPUTDIR/simple" && + ! test -f "$OUTPUTDIR/a" && + ! test -f "$OUTPUTDIR/b" && + ! test -f "$OUTPUTDIR/c" + ' + + test_expect_success "$hook: multiple hooks, all successful" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + rm -f "$HOOK" && + create_multihooks 0 0 0 && + $cmd content && + test -f "$OUTPUTDIR/a" && + test -f "$OUTPUTDIR/b" && + test -f "$OUTPUTDIR/c" + ' + + test_expect_success "$hook: hooks after first failure not executed" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + create_multihooks 0 1 0 && + $must_fail $cmd more-content && + test -f "$OUTPUTDIR/a" && + test -f "$OUTPUTDIR/b" && + ! test -f "$OUTPUTDIR/c" + ' + + test_expect_success POSIXPERM "$hook: non-executable hook not executed" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + create_multihooks 0 1 0 && + chmod -x "$MULTIHOOK_DIR/b" && + $cmd things && + test -f "$OUTPUTDIR/a" && + ! test -f "$OUTPUTDIR/b" && + test -f "$OUTPUTDIR/c" + ' + + test_expect_success POSIXPERM "$hook: multiple hooks not executed if simple hook present" ' + test_when_finished "rm -fr \"$OUTPUTDIR\" && rm -f \"$HOOK\"" && + write_script "$HOOK" <<-EOF && + mkdir -p "$OUTPUTDIR" + touch "$OUTPUTDIR/simple" + exit 0 + EOF + create_multihooks 0 1 0 && + chmod -x "$HOOK" && + $cmd other-things && + ! test -f "$OUTPUTDIR/simple" && + ! test -f "$OUTPUTDIR/a" && + ! test -f "$OUTPUTDIR/b" && + ! test -f "$OUTPUTDIR/c" + ' + + test_expect_success 'cleanup' ' + rm -fr "$MULTIHOOK_DIR" + ' +} diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh index 984889b39d..d63d059e04 100755 --- a/t/t7503-pre-commit-hook.sh +++ b/t/t7503-pre-commit-hook.sh @@ -3,6 +3,7 @@ test_description='pre-commit hook' . ./test-lib.sh +. "$TEST_DIRECTORY/lib-hooks.sh" test_expect_success 'with no hook' ' @@ -136,4 +137,18 @@ test_expect_success 'check the author in hook' ' git show -s ' +commit_command () { + echo "$1" >>file && + git add file && + git commit -m "$1" +} + +commit_no_verify_command () { + echo "$1" >>file && + git add file && + git commit --no-verify -m "$1" +} + +test_multiple_hooks pre-commit commit_command commit_no_verify_command + test_done