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
>
