git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
May 14, 2019, 15:12 UTC
Message-ID
<nycvar.QRO.7.76.6.1905141653130.44@tvgsbejvaqbjf.bet>
In-Reply-To
<20190514002332.121089-2-sandals@crustytoothpaste.net>
Hi brian,
On Tue, 14 May 2019, brian m. carlson wrote:
Show 10 quoted lines
> 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?

Show 31 quoted lines
>  		/*
>  		 * 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.

Show 14 quoted lines
> +			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.

Show 32 quoted lines
> +}
> +
>  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 ;-)
Show 147 quoted lines
> +	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?
Show 11 quoted lines
> + * 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

Show 213 quoted lines
>  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
>
Previous: brian m. carlsonNext: brian m. carlson
Message 6 of 34 in “Multiple hook support”
  1. 0/7 Multiple hook supportbrian m. carlson, May 14, 2019
  2. 1/7 run-command: add preliminary support for multiple hooksbrian m. carlson, May 14, 2019
  3. Duy NguyenMay 14, 2019
  4. brian m. carlsonMay 15, 2019
  5. brian m. carlsonMay 29, 2019
  6. Johannes SchindelinMay 14, 2019
  7. brian m. carlsonMay 15, 2019
  8. Johannes SixtMay 16, 2019
  9. Johannes SchindelinMay 17, 2019
  10. 2/7 builtin/receive-pack: add support for multiple hooksbrian m. carlson, May 14, 2019
  11. 3/7 rebase: add support for multiple hooksbrian m. carlson, May 14, 2019
  12. Duy NguyenMay 14, 2019
  13. Johannes SixtMay 14, 2019
  14. brian m. carlsonMay 15, 2019
  15. Duy NguyenMay 16, 2019
  16. 3/7 sequencer: add support for multiple hooksbrian m. carlson, May 14, 2019
  17. 4/7 builtin/worktree: add support for multiple post-checkout hooksbrian m. carlson, May 14, 2019
  18. 5/7 transport: add support for multiple pre-push hooksbrian m. carlson, May 14, 2019
  19. 6/7 config: allow configuration of multiple hook error behaviorbrian m. carlson, May 14, 2019
  20. Duy NguyenMay 14, 2019
  21. brian m. carlsonMay 15, 2019
  22. Jeff KingMay 16, 2019
  23. Jeff KingMay 16, 2019
  24. brian m. carlsonMay 16, 2019
  25. Jeff KingMay 16, 2019
  26. 7/7 docs: document multiple hooksbrian m. carlson, May 14, 2019
  27. Duy NguyenMay 14, 2019
  28. Jonathan NiederMay 14, 2019
  29. brian m. carlsonMay 14, 2019
  30. Jonathan NiederMay 14, 2019
  31. brian m. carlsonMay 16, 2019
  32. Jonathan NiederMay 16, 2019
  33. Jeff KingMay 16, 2019
  34. Duy NguyenMay 14, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.