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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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
>