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

["brian m. carlson" <sandals@crustytoothpaste.net>]

A variety of types of software take advantage of Git's hooks. However,
if a user would like to integrate multiple pieces of software which use
a particular hook, they currently must manage those hooks themselves,
which can be burdensome. Sometimes various pieces of software try to
overwrite each other's hooks, leading to problems.

To solve this problem, introduce a framework for running multiple hooks
using a ".d" directory named similarly to the hook, running each hook in
order sorted by name. Wire this framework up for those functions using
run_hook_le or run_hook_ve. To preserve backwards compatibility, ensure
that multiple hooks run only if there is no hook using the current hook
style.

If we are running multiple hooks and one of them exits nonzero, don't
execute the remaining hooks and return that exit code immediately. This
allows hooks to fail fast and it avoids having to deal with what happens
if multiple hooks fail with different exit statuses.

Create a test framework for testing multiple hooks with different
commands. This is necessary because not all hooks use run_hook_ve or
run_hook_le and we'll want to ensure all the various hooks work without
needing to write lots of duplicative test code.

Test the pre-commit hook to verify that the run_hook_ve implementation
works correctly.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/commit.c           |   2 +-
 run-command.c              | 173 +++++++++++++++++++++++++++++--------
 run-command.h              |  15 ++++
 t/lib-hooks.sh             | 172 ++++++++++++++++++++++++++++++++++++
 t/t7503-pre-commit-hook.sh |  15 ++++
 5 files changed, 342 insertions(+), 35 deletions(-)
 create mode 100644 t/lib-hooks.sh

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