[PATCH v2 0/7] Multiple hook support

[PATCH v2 0/7] Multiple hook support

[brian m. carlson]

This series introduces multiple hook support.

I've thought a lot about the discussion over whether this series should
use the configuration as the source for multiple hooks. Ultimately, I've
come to the decision that it's not a good idea. Even adopting the empty
entry as a reset marker, the fact that inheritance in the configuration
is in-order and can't be easily modified means that it's not likely to
be very useful, but it is likely to be quite surprising for the average
user. I think a solution that sticks with the existing model and builds
off a design used by other systems people are familiar with, like cron
and run-parts, is going to be a better choice. Moreover, this is the
design that people have already built with outside tooling, which is a
further argument in favor of it.

I have adopted one configuration-based option, which is the per-hook
errorBehavior option that Peff suggested. I think this reduces concerns
over what the best error handling strategy is and is a good thing to
have as part of a minimum viable product. I picked the names that Peff
chose, but if people like different names better, they can be changed.

Just as a preview of what's coming down the line, I plan to build on
this series to notify hooks when --quiet and --dry-run options have been
specified to commands so that they may honor them if they choose.

Changes from v1:
* Adopted several improvements from Duy's series, including an improved
  find_hooks prototype and a helper function.
* Switched to existence checks instead of executability checks for
  determining whether to invoke multiple hooks.
* Adjusted the commit message for patch 3.
* Added error behavior control using the names Peff provided in his
  comment.
* Added documentation.

brian m. carlson (7):
  run-command: add preliminary support for multiple hooks
  builtin/receive-pack: add support for multiple hooks
  rebase: add support for multiple hooks
  builtin/worktree: add support for multiple post-checkout hooks
  transport: add support for multiple pre-push hooks
  config: allow configuration of multiple hook error behavior
  docs: document multiple hooks

 Documentation/config.txt           |   2 +
 Documentation/config/hook.txt      |  19 ++
 Documentation/githooks.txt         |   9 +
 builtin/am.c                       |  20 +--
 builtin/commit.c                   |   2 +-
 builtin/receive-pack.c             |  78 ++++----
 builtin/worktree.c                 |  44 +++--
 config.c                           |  27 +++
 run-command.c                      | 175 +++++++++++++++---
 run-command.h                      |  22 ++-
 sequencer.c                        |  59 ++++---
 sequencer.h                        |   2 +
 t/lib-hooks.sh                     | 274 +++++++++++++++++++++++++++++
 t/t5403-post-checkout-hook.sh      |   8 +
 t/t5407-post-rewrite-hook.sh       |  15 ++
 t/t5516-fetch-push.sh              |  30 ++++
 t/t5571-pre-push-hook.sh           |  19 ++
 t/t7503-pre-commit-hook.sh         |  15 ++
 t/t7505-prepare-commit-msg-hook.sh |   9 +
 transport.c                        |  29 +--
 20 files changed, 730 insertions(+), 128 deletions(-)
 create mode 100644 Documentation/config/hook.txt
 create mode 100644 t/lib-hooks.sh

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

[brian m. carlson]

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

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

[Duy Nguyen]

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;
-- 
Duy

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2954 bytes --]

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.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

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.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Johannes Schindelin]

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
>

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 5540 bytes --]

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?
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Johannes Sixt]

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

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

[Johannes Schindelin]

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

[PATCH v2 2/7] builtin/receive-pack: add support for multiple hooks

[brian m. carlson]

Add support for multiple hooks for the pre-receive, post-receive,
update, post-update, and push-to-checkout hooks. Add tests for these
hooks using the multiple hook test framework.

Because the invocations of test_multiple_hooks contain multiple test
assertions, they (and the cd commands that surround them) must occur
outside of a subshell, or a failing test will not be noticed.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/receive-pack.c | 78 ++++++++++++++++++++++++++----------------
 t/t5516-fetch-push.sh  | 30 ++++++++++++++++
 2 files changed, 78 insertions(+), 30 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 29f165d8bd..5940f6969a 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -669,6 +669,8 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 	}
 }
 
+typedef int (*feed_fn)(void *, const char **, size_t *);
+
 struct receive_hook_feed_state {
 	struct command *cmd;
 	int skip_broken;
@@ -676,34 +678,36 @@ struct receive_hook_feed_state {
 	const struct string_list *push_options;
 };
 
-typedef int (*feed_fn)(void *, const char **, size_t *);
-static int run_and_feed_hook(const char *hook_name, feed_fn feed,
-			     struct receive_hook_feed_state *feed_state)
+struct receive_hook_data {
+	feed_fn fn;
+	struct receive_hook_feed_state *state;
+};
+
+static int do_run_and_feed_hook(const char *name, const char *path, void *cbp)
 {
-	struct child_process proc = CHILD_PROCESS_INIT;
+	struct receive_hook_data *data = cbp;
+	struct child_process proc;
 	struct async muxer;
 	const char *argv[2];
-	int code;
-
-	argv[0] = find_hook(hook_name);
-	if (!argv[0])
-		return 0;
+	int code = 0;
 
+	argv[0] = path;
 	argv[1] = NULL;
 
+	child_process_init(&proc);
 	proc.argv = argv;
 	proc.in = -1;
 	proc.stdout_to_stderr = 1;
-	proc.trace2_hook_name = hook_name;
+	proc.trace2_hook_name = name;
 
-	if (feed_state->push_options) {
+	if (data->state->push_options) {
 		int i;
-		for (i = 0; i < feed_state->push_options->nr; i++)
+		for (i = 0; i < data->state->push_options->nr; i++)
 			argv_array_pushf(&proc.env_array,
 				"GIT_PUSH_OPTION_%d=%s", i,
-				feed_state->push_options->items[i].string);
+				data->state->push_options->items[i].string);
 		argv_array_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT=%d",
-				 feed_state->push_options->nr);
+				 data->state->push_options->nr);
 	} else
 		argv_array_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT");
 
@@ -734,7 +738,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
 	while (1) {
 		const char *buf;
 		size_t n;
-		if (feed(feed_state, &buf, &n))
+		if (data->fn(data->state, &buf, &n))
 			break;
 		if (write_in_full(proc.in, buf, n) < 0)
 			break;
@@ -748,6 +752,13 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
 	return finish_command(&proc);
 }
 
+static int run_and_feed_hook(const char *hook_name, feed_fn feed,
+			     struct receive_hook_feed_state *feed_state)
+{
+	struct receive_hook_data data = { feed, feed_state };
+	return for_each_hook(hook_name, do_run_and_feed_hook, &data);
+}
+
 static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)
 {
 	struct receive_hook_feed_state *state = state_;
@@ -790,16 +801,14 @@ static int run_receive_hook(struct command *commands,
 	return status;
 }
 
-static int run_update_hook(struct command *cmd)
+static int do_run_update_hook(const char *name, const char *path, void *data)
 {
-	const char *argv[5];
+	struct command *cmd = data;
 	struct child_process proc = CHILD_PROCESS_INIT;
+	const char *argv[5];
 	int code;
 
-	argv[0] = find_hook("update");
-	if (!argv[0])
-		return 0;
-
+	argv[0] = path;
 	argv[1] = cmd->ref_name;
 	argv[2] = oid_to_hex(&cmd->old_oid);
 	argv[3] = oid_to_hex(&cmd->new_oid);
@@ -819,6 +828,11 @@ static int run_update_hook(struct command *cmd)
 	return finish_command(&proc);
 }
 
+static int run_update_hook(struct command *cmd)
+{
+	return for_each_hook("update", do_run_update_hook, cmd);
+}
+
 static int is_ref_checked_out(const char *ref)
 {
 	if (is_bare_repository())
@@ -1011,7 +1025,7 @@ static const char *update_worktree(unsigned char *sha1)
 
 	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
 
-	if (!find_hook(push_to_checkout_hook))
+	if (!find_hooks(push_to_checkout_hook, NULL))
 		retval = push_to_deploy(sha1, &env, work_tree);
 	else
 		retval = push_to_checkout(sha1, &env, work_tree);
@@ -1170,25 +1184,23 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 	}
 }
 
-static void run_update_post_hook(struct command *commands)
+static int do_run_update_post_hook(const char *name, const char *path, void *data)
 {
+	struct command *commands = data;
 	struct command *cmd;
 	struct child_process proc = CHILD_PROCESS_INIT;
-	const char *hook;
 
-	hook = find_hook("post-update");
-	if (!hook)
-		return;
+	child_process_init(&proc);
 
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (cmd->error_string || cmd->did_not_exist)
 			continue;
 		if (!proc.args.argc)
-			argv_array_push(&proc.args, hook);
+			argv_array_push(&proc.args, path);
 		argv_array_push(&proc.args, cmd->ref_name);
 	}
 	if (!proc.args.argc)
-		return;
+		return 0;
 
 	proc.no_stdin = 1;
 	proc.stdout_to_stderr = 1;
@@ -1198,8 +1210,14 @@ static void run_update_post_hook(struct command *commands)
 	if (!start_command(&proc)) {
 		if (use_sideband)
 			copy_to_sideband(proc.err, -1, NULL);
-		finish_command(&proc);
+		return finish_command(&proc);
 	}
+	return -1;
+}
+
+static void run_update_post_hook(struct command *commands)
+{
+	for_each_hook("post-update", do_run_update_post_hook, commands);
 }
 
 static void check_aliased_update_internal(struct command *cmd,
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index c81ca360ac..697c3ab074 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -15,6 +15,7 @@ This test checks the following functionality:
 '
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY/lib-hooks.sh"
 
 D=$(pwd)
 
@@ -1712,4 +1713,33 @@ test_expect_success 'updateInstead with push-to-checkout hook' '
 	)
 '
 
+test_expect_success 'setup' '
+	mk_test_with_hooks hooktest heads/master
+'
+
+cmd_receive () {
+	git reset --hard &&
+	echo "$1" >>../file &&
+	git -C .. add file &&
+	git -C .. commit -m "$1" &&
+	git -C .. push hooktest refs/heads/master:refs/heads/master
+}
+
+cd hooktest
+test_multiple_hooks pre-receive cmd_receive
+test_multiple_hooks --ignore-exit-status post-receive cmd_receive
+test_multiple_hooks update cmd_receive
+test_multiple_hooks --ignore-exit-status post-update cmd_receive
+cd ..
+
+test_expect_success 'setup' '
+	rm -fr hooktest &&
+	git init hooktest &&
+	git -C hooktest config receive.denyCurrentBranch updateInstead
+'
+
+cd hooktest
+test_multiple_hooks push-to-checkout cmd_receive
+cd ..
+
 test_done

[PATCH v2 3/7] rebase: add support for multiple hooks

[brian m. carlson]

Add support for multiple post-rewrite hooks, both for "git commit
--amend" and "git rebase". Unify the hook handling between the am-based
and sequencer-based code paths.

Additionally add support for multiple prepare-commit-msg hooks.

Note that the prepare-commit-msg hook is not passed a set of hooks
directly because these are discovered later when the code calls
run_hooks_le.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/am.c                       | 20 +---------
 sequencer.c                        | 59 ++++++++++++++++++------------
 sequencer.h                        |  2 +
 t/t5407-post-rewrite-hook.sh       | 15 ++++++++
 t/t7505-prepare-commit-msg-hook.sh |  9 +++++
 5 files changed, 63 insertions(+), 42 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 912d9821b1..340eacbd44 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -441,24 +441,8 @@ static int run_applypatch_msg_hook(struct am_state *state)
  */
 static int run_post_rewrite_hook(const struct am_state *state)
 {
-	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *hook = find_hook("post-rewrite");
-	int ret;
-
-	if (!hook)
-		return 0;
-
-	argv_array_push(&cp.args, hook);
-	argv_array_push(&cp.args, "rebase");
-
-	cp.in = xopen(am_path(state, "rewritten"), O_RDONLY);
-	cp.stdout_to_stderr = 1;
-	cp.trace2_hook_name = "post-rewrite";
-
-	ret = run_command(&cp);
-
-	close(cp.in);
-	return ret;
+	return for_each_hook("post-rewrite", post_rewrite_rebase_hook,
+			     (void *)am_path(state, "rewritten"));
 }
 
 /**
diff --git a/sequencer.c b/sequencer.c
index 546f281898..b899209f76 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1084,30 +1084,32 @@ int update_head_with_reflog(const struct commit *old_head,
 	return ret;
 }
 
-static int run_rewrite_hook(const struct object_id *oldoid,
-			    const struct object_id *newoid)
+struct rewrite_hook_data {
+	const struct object_id *oldoid;
+	const struct object_id *newoid;
+};
+
+static int do_run_rewrite_hook(const char *name, const char *path, void *p)
 {
+	struct rewrite_hook_data *data = p;
 	struct child_process proc = CHILD_PROCESS_INIT;
+	struct strbuf sb = STRBUF_INIT;
 	const char *argv[3];
 	int code;
-	struct strbuf sb = STRBUF_INIT;
-
-	argv[0] = find_hook("post-rewrite");
-	if (!argv[0])
-		return 0;
 
+	argv[0] = path;
 	argv[1] = "amend";
 	argv[2] = NULL;
 
 	proc.argv = argv;
 	proc.in = -1;
 	proc.stdout_to_stderr = 1;
-	proc.trace2_hook_name = "post-rewrite";
+	proc.trace2_hook_name = name;
 
 	code = start_command(&proc);
 	if (code)
 		return code;
-	strbuf_addf(&sb, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid));
+	strbuf_addf(&sb, "%s %s\n", oid_to_hex(data->oldoid), oid_to_hex(data->newoid));
 	sigchain_push(SIGPIPE, SIG_IGN);
 	write_in_full(proc.in, sb.buf, sb.len);
 	close(proc.in);
@@ -1116,6 +1118,25 @@ static int run_rewrite_hook(const struct object_id *oldoid,
 	return finish_command(&proc);
 }
 
+static int run_rewrite_hook(const struct object_id *oldoid,
+			    const struct object_id *newoid)
+{
+	struct rewrite_hook_data data = { oldoid, newoid };
+	return for_each_hook("post-rewrite", do_run_rewrite_hook, &data);
+}
+
+int post_rewrite_rebase_hook(const char *name, const char *path, void *input)
+{
+	struct child_process child = CHILD_PROCESS_INIT;
+
+	child.in = open(input, O_RDONLY);
+	child.stdout_to_stderr = 1;
+	child.trace2_hook_name = "post-rewrite";
+	argv_array_push(&child.args, path);
+	argv_array_push(&child.args, "rebase");
+	return run_command(&child);
+}
+
 void commit_post_rewrite(struct repository *r,
 			 const struct commit *old_head,
 			 const struct object_id *new_head)
@@ -1368,7 +1389,7 @@ static int try_to_commit(struct repository *r,
 		goto out;
 	}
 
-	if (find_hook("prepare-commit-msg")) {
+	if (find_hooks("prepare-commit-msg", NULL)) {
 		res = run_prepare_commit_msg_hook(r, msg, hook_commit);
 		if (res)
 			goto out;
@@ -3763,8 +3784,6 @@ static int pick_commits(struct repository *r,
 		if (!stat(rebase_path_rewritten_list(), &st) &&
 				st.st_size > 0) {
 			struct child_process child = CHILD_PROCESS_INIT;
-			const char *post_rewrite_hook =
-				find_hook("post-rewrite");
 
 			child.in = open(rebase_path_rewritten_list(), O_RDONLY);
 			child.git_cmd = 1;
@@ -3774,18 +3793,10 @@ static int pick_commits(struct repository *r,
 			/* we don't care if this copying failed */
 			run_command(&child);
 
-			if (post_rewrite_hook) {
-				struct child_process hook = CHILD_PROCESS_INIT;
-
-				hook.in = open(rebase_path_rewritten_list(),
-					O_RDONLY);
-				hook.stdout_to_stderr = 1;
-				hook.trace2_hook_name = "post-rewrite";
-				argv_array_push(&hook.args, post_rewrite_hook);
-				argv_array_push(&hook.args, "rebase");
-				/* we don't care if this hook failed */
-				run_command(&hook);
-			}
+			/* we don't care if this hook failed */
+			for_each_hook("post-rewrite",
+				      post_rewrite_rebase_hook,
+				      (void *)rebase_path_rewritten_list());
 		}
 		apply_autostash(opts);
 
diff --git a/sequencer.h b/sequencer.h
index a515ee4457..710ef5c18c 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -154,6 +154,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 		    unsigned autosquash, struct todo_list *todo_list);
 int todo_list_rearrange_squash(struct todo_list *todo_list);
 
+int post_rewrite_rebase_hook(const char *name, const char *path, void *input);
+
 /*
  * Append a signoff to the commit message in "msgbuf". The ignore_footer
  * parameter specifies the number of bytes at the end of msgbuf that should
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index 7344253bfb..f8ce32fe3b 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -5,6 +5,7 @@
 
 test_description='Test the post-rewrite hook.'
 . ./test-lib.sh
+. "$TEST_DIRECTORY/lib-hooks.sh"
 
 test_expect_success 'setup' '
 	test_commit A foo A &&
@@ -263,4 +264,18 @@ test_expect_success 'git rebase -i (exec)' '
 	verify_hook_input
 '
 
+cmd_rebase () {
+	git reset --hard D &&
+	FAKE_LINES="1 fixup 2" git rebase -i B
+}
+
+cmd_amend () {
+	git reset --hard D &&
+	echo "D new message" > newmsg &&
+	git commit -Fnewmsg --amend
+}
+
+test_multiple_hooks --ignore-exit-status post-rewrite cmd_rebase
+test_multiple_hooks --ignore-exit-status post-rewrite cmd_amend
+
 test_done
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index ba8bd1b514..287e13e29d 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -3,6 +3,7 @@
 test_description='prepare-commit-msg hook'
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY/lib-hooks.sh"
 
 test_expect_success 'set up commits for rebasing' '
 	test_commit root &&
@@ -317,4 +318,12 @@ test_expect_success C_LOCALE_OUTPUT 'with failing hook (cherry-pick)' '
 	test $(grep -c prepare-commit-msg actual) = 1
 '
 
+cherry_pick_command () {
+	git checkout -f master &&
+	git checkout -B other b &&
+	git cherry-pick rebase-1
+}
+
+test_multiple_hooks prepare-commit-msg cherry_pick_command
+
 test_done

Re: [PATCH v2 3/7] rebase: add support for multiple hooks

[Duy Nguyen]

On Tue, May 14, 2019 at 7:24 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> diff --git a/builtin/am.c b/builtin/am.c
> index 912d9821b1..340eacbd44 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -441,24 +441,8 @@ static int run_applypatch_msg_hook(struct am_state *state)
>   */
>  static int run_post_rewrite_hook(const struct am_state *state)
>  {
> -       struct child_process cp = CHILD_PROCESS_INIT;
> -       const char *hook = find_hook("post-rewrite");
> -       int ret;
> -
> -       if (!hook)
> -               return 0;
> -
> -       argv_array_push(&cp.args, hook);
> -       argv_array_push(&cp.args, "rebase");
> -
> -       cp.in = xopen(am_path(state, "rewritten"), O_RDONLY);
> -       cp.stdout_to_stderr = 1;
> -       cp.trace2_hook_name = "post-rewrite";
> -
> -       ret = run_command(&cp);
> -
> -       close(cp.in);

In the old code, we close cp.in...

> +int post_rewrite_rebase_hook(const char *name, const char *path, void *input)
> +{
> +       struct child_process child = CHILD_PROCESS_INIT;
> +
> +       child.in = open(input, O_RDONLY);
> +       child.stdout_to_stderr = 1;
> +       child.trace2_hook_name = "post-rewrite";

maybe use "name" and avoid hard coding "post-rewrite".

> +       argv_array_push(&child.args, path);
> +       argv_array_push(&child.args, "rebase");
> +       return run_command(&child);

... but in the new one we don't. Smells fd leaking to me.
-- 
Duy

Re: [PATCH v2 3/7] rebase: add support for multiple hooks

[Johannes Sixt]

Am 14.05.19 um 14:56 schrieb Duy Nguyen:
> On Tue, May 14, 2019 at 7:24 AM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
>> diff --git a/builtin/am.c b/builtin/am.c
>> index 912d9821b1..340eacbd44 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -441,24 +441,8 @@ static int run_applypatch_msg_hook(struct am_state *state)
>>   */
>>  static int run_post_rewrite_hook(const struct am_state *state)
>>  {
>> -       struct child_process cp = CHILD_PROCESS_INIT;
>> -       const char *hook = find_hook("post-rewrite");
>> -       int ret;
>> -
>> -       if (!hook)
>> -               return 0;
>> -
>> -       argv_array_push(&cp.args, hook);
>> -       argv_array_push(&cp.args, "rebase");
>> -
>> -       cp.in = xopen(am_path(state, "rewritten"), O_RDONLY);
>> -       cp.stdout_to_stderr = 1;
>> -       cp.trace2_hook_name = "post-rewrite";
>> -
>> -       ret = run_command(&cp);
>> -
>> -       close(cp.in);
> 
> In the old code, we close cp.in...
> 
>> +int post_rewrite_rebase_hook(const char *name, const char *path, void *input)
>> +{
>> +       struct child_process child = CHILD_PROCESS_INIT;
>> +
>> +       child.in = open(input, O_RDONLY);
>> +       child.stdout_to_stderr = 1;
>> +       child.trace2_hook_name = "post-rewrite";
> 
>> +       argv_array_push(&child.args, path);
>> +       argv_array_push(&child.args, "rebase");
>> +       return run_command(&child);
> 
> ... but in the new one we don't. Smells fd leaking to me.

IIRC, run_command always closes the fds that it receives (even on error
paths). Therefore, the old code is incorrect and should not call
close(cp.in).

-- Hannes

Re: [PATCH v2 3/7] rebase: add support for multiple hooks

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 913 bytes --]

On Tue, May 14, 2019 at 07:56:49PM +0700, Duy Nguyen wrote:
> On Tue, May 14, 2019 at 7:24 AM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > -       close(cp.in);
> 
> In the old code, we close cp.in...
> 
> > +int post_rewrite_rebase_hook(const char *name, const char *path, void *input)
> > +{
> > +       struct child_process child = CHILD_PROCESS_INIT;
> > +
> > +       child.in = open(input, O_RDONLY);
> > +       child.stdout_to_stderr = 1;
> > +       child.trace2_hook_name = "post-rewrite";
> 
> maybe use "name" and avoid hard coding "post-rewrite".
> 
> > +       argv_array_push(&child.args, path);
> > +       argv_array_push(&child.args, "rebase");
> > +       return run_command(&child);
> 
> ... but in the new one we don't. Smells fd leaking to me.

Ah, good point.  Will fix.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH v2 3/7] rebase: add support for multiple hooks

[Duy Nguyen]

On Thu, May 16, 2019 at 5:55 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On Tue, May 14, 2019 at 07:56:49PM +0700, Duy Nguyen wrote:
> > On Tue, May 14, 2019 at 7:24 AM brian m. carlson
> > <sandals@crustytoothpaste.net> wrote:
> > > -       close(cp.in);
> >
> > In the old code, we close cp.in...
> >
> > > +int post_rewrite_rebase_hook(const char *name, const char *path, void *input)
> > > +{
> > > +       struct child_process child = CHILD_PROCESS_INIT;
> > > +
> > > +       child.in = open(input, O_RDONLY);
> > > +       child.stdout_to_stderr = 1;
> > > +       child.trace2_hook_name = "post-rewrite";
> >
> > maybe use "name" and avoid hard coding "post-rewrite".
> >
> > > +       argv_array_push(&child.args, path);
> > > +       argv_array_push(&child.args, "rebase");
> > > +       return run_command(&child);
> >
> > ... but in the new one we don't. Smells fd leaking to me.
>
> Ah, good point.  Will fix.

Actually I think Johannes is right (in the other mail, same thread)
that the old code is buggy.

The only good thing is, I don't think the old code could actually
result in double close() problem. It would just return EBADF, which we
don't care. So no hurry fixing the old code until you replace it with
a better one.

-- 
Duy

[PATCH v2 3/7] sequencer: add support for multiple hooks

[brian m. carlson]

Add support for multiple post-rewrite hooks, both for "git commit
--amend" and "git rebase".

Additionally add support for multiple prepare-commit-msg hooks.

Note that the prepare-commit-msg hook is not passed a set of hooks
directly because these are discovered later when the code calls
run_hooks_le.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/am.c                       | 20 +++++-----
 sequencer.c                        | 59 ++++++++++++++++++------------
 t/t5407-post-rewrite-hook.sh       | 15 ++++++++
 t/t7505-prepare-commit-msg-hook.sh |  9 +++++
 4 files changed, 70 insertions(+), 33 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 912d9821b1..55a5b74da3 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -436,19 +436,13 @@ static int run_applypatch_msg_hook(struct am_state *state)
 	return ret;
 }
 
-/**
- * Runs post-rewrite hook. Returns it exit code.
- */
-static int run_post_rewrite_hook(const struct am_state *state)
+static int do_run_post_rewrite_hook(const char *name, const char *path, void *p)
 {
+	const struct am_state *state = p;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *hook = find_hook("post-rewrite");
 	int ret;
 
-	if (!hook)
-		return 0;
-
-	argv_array_push(&cp.args, hook);
+	argv_array_push(&cp.args, path);
 	argv_array_push(&cp.args, "rebase");
 
 	cp.in = xopen(am_path(state, "rewritten"), O_RDONLY);
@@ -461,6 +455,14 @@ static int run_post_rewrite_hook(const struct am_state *state)
 	return ret;
 }
 
+/**
+ * Runs post-rewrite hook. Returns it exit code.
+ */
+static int run_post_rewrite_hook(const struct am_state *state)
+{
+	return for_each_hook("post-rewrite", do_run_post_rewrite_hook, (void *)state);
+}
+
 /**
  * Reads the state directory's "rewritten" file, and copies notes from the old
  * commits listed in the file to their rewritten commits.
diff --git a/sequencer.c b/sequencer.c
index 546f281898..f7703bc7a5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1084,30 +1084,32 @@ int update_head_with_reflog(const struct commit *old_head,
 	return ret;
 }
 
-static int run_rewrite_hook(const struct object_id *oldoid,
-			    const struct object_id *newoid)
+struct rewrite_hook_data {
+	const struct object_id *oldoid;
+	const struct object_id *newoid;
+};
+
+static int do_run_rewrite_hook(const char *name, const char *path, void *p)
 {
+	struct rewrite_hook_data *data = p;
 	struct child_process proc = CHILD_PROCESS_INIT;
+	struct strbuf sb = STRBUF_INIT;
 	const char *argv[3];
 	int code;
-	struct strbuf sb = STRBUF_INIT;
-
-	argv[0] = find_hook("post-rewrite");
-	if (!argv[0])
-		return 0;
 
+	argv[0] = path;
 	argv[1] = "amend";
 	argv[2] = NULL;
 
 	proc.argv = argv;
 	proc.in = -1;
 	proc.stdout_to_stderr = 1;
-	proc.trace2_hook_name = "post-rewrite";
+	proc.trace2_hook_name = name;
 
 	code = start_command(&proc);
 	if (code)
 		return code;
-	strbuf_addf(&sb, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid));
+	strbuf_addf(&sb, "%s %s\n", oid_to_hex(data->oldoid), oid_to_hex(data->newoid));
 	sigchain_push(SIGPIPE, SIG_IGN);
 	write_in_full(proc.in, sb.buf, sb.len);
 	close(proc.in);
@@ -1116,6 +1118,26 @@ static int run_rewrite_hook(const struct object_id *oldoid,
 	return finish_command(&proc);
 }
 
+static int run_rewrite_hook(const struct object_id *oldoid,
+			    const struct object_id *newoid)
+{
+	struct rewrite_hook_data data = { oldoid, newoid };
+	return for_each_hook("post-rewrite", do_run_rewrite_hook, &data);
+}
+
+static int interactive_rewrite_hook_handler(const char *name, const char *path, void *data)
+{
+	struct child_process child = CHILD_PROCESS_INIT;
+
+	child.in = open(rebase_path_rewritten_list(),
+		O_RDONLY);
+	child.stdout_to_stderr = 1;
+	child.trace2_hook_name = "post-rewrite";
+	argv_array_push(&child.args, path);
+	argv_array_push(&child.args, "rebase");
+	return run_command(&child);
+}
+
 void commit_post_rewrite(struct repository *r,
 			 const struct commit *old_head,
 			 const struct object_id *new_head)
@@ -1368,7 +1390,7 @@ static int try_to_commit(struct repository *r,
 		goto out;
 	}
 
-	if (find_hook("prepare-commit-msg")) {
+	if (find_hooks("prepare-commit-msg", NULL)) {
 		res = run_prepare_commit_msg_hook(r, msg, hook_commit);
 		if (res)
 			goto out;
@@ -3763,8 +3785,6 @@ static int pick_commits(struct repository *r,
 		if (!stat(rebase_path_rewritten_list(), &st) &&
 				st.st_size > 0) {
 			struct child_process child = CHILD_PROCESS_INIT;
-			const char *post_rewrite_hook =
-				find_hook("post-rewrite");
 
 			child.in = open(rebase_path_rewritten_list(), O_RDONLY);
 			child.git_cmd = 1;
@@ -3774,18 +3794,9 @@ static int pick_commits(struct repository *r,
 			/* we don't care if this copying failed */
 			run_command(&child);
 
-			if (post_rewrite_hook) {
-				struct child_process hook = CHILD_PROCESS_INIT;
-
-				hook.in = open(rebase_path_rewritten_list(),
-					O_RDONLY);
-				hook.stdout_to_stderr = 1;
-				hook.trace2_hook_name = "post-rewrite";
-				argv_array_push(&hook.args, post_rewrite_hook);
-				argv_array_push(&hook.args, "rebase");
-				/* we don't care if this hook failed */
-				run_command(&hook);
-			}
+			/* we don't care if this hook failed */
+			for_each_hook("post-rewrite",
+				      interactive_rewrite_hook_handler, NULL);
 		}
 		apply_autostash(opts);
 
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index 7344253bfb..f8ce32fe3b 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -5,6 +5,7 @@
 
 test_description='Test the post-rewrite hook.'
 . ./test-lib.sh
+. "$TEST_DIRECTORY/lib-hooks.sh"
 
 test_expect_success 'setup' '
 	test_commit A foo A &&
@@ -263,4 +264,18 @@ test_expect_success 'git rebase -i (exec)' '
 	verify_hook_input
 '
 
+cmd_rebase () {
+	git reset --hard D &&
+	FAKE_LINES="1 fixup 2" git rebase -i B
+}
+
+cmd_amend () {
+	git reset --hard D &&
+	echo "D new message" > newmsg &&
+	git commit -Fnewmsg --amend
+}
+
+test_multiple_hooks --ignore-exit-status post-rewrite cmd_rebase
+test_multiple_hooks --ignore-exit-status post-rewrite cmd_amend
+
 test_done
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index ba8bd1b514..287e13e29d 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -3,6 +3,7 @@
 test_description='prepare-commit-msg hook'
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY/lib-hooks.sh"
 
 test_expect_success 'set up commits for rebasing' '
 	test_commit root &&
@@ -317,4 +318,12 @@ test_expect_success C_LOCALE_OUTPUT 'with failing hook (cherry-pick)' '
 	test $(grep -c prepare-commit-msg actual) = 1
 '
 
+cherry_pick_command () {
+	git checkout -f master &&
+	git checkout -B other b &&
+	git cherry-pick rebase-1
+}
+
+test_multiple_hooks prepare-commit-msg cherry_pick_command
+
 test_done

[PATCH v2 4/7] builtin/worktree: add support for multiple post-checkout hooks

[brian m. carlson]

Add support for multiple post-checkout hooks. We test only one possible
path in the multiple hook case because the same code path is used for
all checkouts and we know the hooks work from earlier assertions.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/worktree.c            | 44 ++++++++++++++++++++++-------------
 t/t5403-post-checkout-hook.sh |  8 +++++++
 2 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index d2a7e2f3f1..1edcde8c84 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -262,6 +262,32 @@ static void validate_worktree_add(const char *path, const struct add_opts *opts)
 	free_worktrees(worktrees);
 }
 
+struct hook_data {
+	struct commit *commit;
+	const char *dir;
+};
+
+static int run_post_checkout_hook(const char *name, const char *path, void *p)
+{
+	struct hook_data *data = p;
+	struct commit *commit = data->commit;
+	const char *env[] = { "GIT_DIR", "GIT_WORK_TREE", NULL };
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	cp.git_cmd = 0;
+	cp.no_stdin = 1;
+	cp.stdout_to_stderr = 1;
+	cp.dir = data->dir;
+	cp.env = env;
+	cp.argv = NULL;
+	cp.trace2_hook_name = "post-checkout";
+	argv_array_pushl(&cp.args, absolute_path(path),
+			 oid_to_hex(&null_oid),
+			 oid_to_hex(&commit->object.oid),
+			 "1", NULL);
+	return run_command(&cp);
+}
+
 static int add_worktree(const char *path, const char *refname,
 			const struct add_opts *opts)
 {
@@ -395,22 +421,8 @@ static int add_worktree(const char *path, const char *refname,
 	 * is_junk is cleared, but do return appropriate code when hook fails.
 	 */
 	if (!ret && opts->checkout) {
-		const char *hook = find_hook("post-checkout");
-		if (hook) {
-			const char *env[] = { "GIT_DIR", "GIT_WORK_TREE", NULL };
-			cp.git_cmd = 0;
-			cp.no_stdin = 1;
-			cp.stdout_to_stderr = 1;
-			cp.dir = path;
-			cp.env = env;
-			cp.argv = NULL;
-			cp.trace2_hook_name = "post-checkout";
-			argv_array_pushl(&cp.args, absolute_path(hook),
-					 oid_to_hex(&null_oid),
-					 oid_to_hex(&commit->object.oid),
-					 "1", NULL);
-			ret = run_command(&cp);
-		}
+		struct hook_data data = { commit, path };
+		ret = for_each_hook("post-checkout", run_post_checkout_hook, &data);
 	}
 
 	argv_array_clear(&child_env);
diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
index a39b3b5c78..aa265ce610 100755
--- a/t/t5403-post-checkout-hook.sh
+++ b/t/t5403-post-checkout-hook.sh
@@ -5,6 +5,7 @@
 
 test_description='Test the post-checkout hook.'
 . ./test-lib.sh
+. "$TEST_DIRECTORY/lib-hooks.sh"
 
 test_expect_success setup '
 	mkdir -p .git/hooks &&
@@ -73,4 +74,11 @@ test_expect_success 'post-checkout hook is triggered by clone' '
 	test -f clone3/.git/post-checkout.args
 '
 
+cmd_rebase () {
+	git checkout -B hook-test rebase-on-me^ &&
+	git rebase rebase-on-me
+}
+
+test_multiple_hooks post-checkout cmd_rebase
+
 test_done

[PATCH v2 5/7] transport: add support for multiple pre-push hooks

[brian m. carlson]

Add support for multiple pre-push hooks.

Remove find_hook since there are no longer any callers of it.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 run-command.c            | 12 ------------
 run-command.h            |  6 ------
 t/t5571-pre-push-hook.sh | 19 +++++++++++++++++++
 transport.c              | 29 +++++++++++++++++++----------
 4 files changed, 38 insertions(+), 28 deletions(-)

diff --git a/run-command.c b/run-command.c
index eb075ac86b..191d6f6f7e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1346,18 +1346,6 @@ static int has_hook(struct strbuf *path, int strip, int check)
 	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 (has_hook(&path, 1, X_OK)) {
-		return path.buf;
-	}
-	return NULL;
-}
-
 int find_hooks(const char *name, struct string_list *list)
 {
 	struct strbuf path = STRBUF_INIT;
diff --git a/run-command.h b/run-command.h
index 1b3677fcac..15974e26d4 100644
--- a/run-command.h
+++ b/run-command.h
@@ -63,12 +63,6 @@ int finish_command(struct child_process *);
 int finish_command_in_signal(struct child_process *);
 int run_command(struct child_process *);
 
-/*
- * Returns the path to the hook file, or NULL if the hook is missing
- * or disabled. Note that this points to static storage that will be
- * 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.
diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh
index ac53d63869..754ad8eb93 100755
--- a/t/t5571-pre-push-hook.sh
+++ b/t/t5571-pre-push-hook.sh
@@ -2,6 +2,7 @@
 
 test_description='check pre-push hooks'
 . ./test-lib.sh
+. "$TEST_DIRECTORY/lib-hooks.sh"
 
 # Setup hook that always succeeds
 HOOKDIR="$(git rev-parse --git-dir)/hooks"
@@ -125,4 +126,22 @@ test_expect_success 'sigpipe does not cause pre-push hook failure' '
 	git push parent1 "refs/heads/b/*:refs/heads/b/*"
 '
 
+push_command () {
+	test_commit "$1" &&
+	git push hooks refs/heads/master:refs/heads/master
+}
+
+push_no_verify_command () {
+	test_commit "$1" &&
+	git push --no-verify hooks refs/heads/master:refs/heads/master
+}
+
+test_expect_success 'setup' '
+	git checkout master &&
+	git init --bare hooktest &&
+	git remote add hooks hooktest
+'
+
+test_multiple_hooks pre-push push_command push_no_verify_command
+
 test_done
diff --git a/transport.c b/transport.c
index 365ea574c7..7672f4fb57 100644
--- a/transport.c
+++ b/transport.c
@@ -1042,20 +1042,23 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing)
 	die(_("Aborting."));
 }
 
-static int run_pre_push_hook(struct transport *transport,
-			     struct ref *remote_refs)
+struct pre_push_hook_data {
+	struct transport *transport;
+	struct ref *remote_refs;
+};
+
+static int do_run_pre_push_hook(const char *name, const char *path, void *p)
 {
+	struct pre_push_hook_data *data = p;
+	struct child_process proc = CHILD_PROCESS_INIT;
 	int ret = 0, x;
 	struct ref *r;
-	struct child_process proc = CHILD_PROCESS_INIT;
 	struct strbuf buf;
 	const char *argv[4];
 
-	if (!(argv[0] = find_hook("pre-push")))
-		return 0;
-
-	argv[1] = transport->remote->name;
-	argv[2] = transport->url;
+	argv[0] = path;
+	argv[1] = data->transport->remote->name;
+	argv[2] = data->transport->url;
 	argv[3] = NULL;
 
 	proc.argv = argv;
@@ -1071,7 +1074,7 @@ static int run_pre_push_hook(struct transport *transport,
 
 	strbuf_init(&buf, 256);
 
-	for (r = remote_refs; r; r = r->next) {
+	for (r = data->remote_refs; r; r = r->next) {
 		if (!r->peer_ref) continue;
 		if (r->status == REF_STATUS_REJECT_NONFASTFORWARD) continue;
 		if (r->status == REF_STATUS_REJECT_STALE) continue;
@@ -1101,10 +1104,16 @@ static int run_pre_push_hook(struct transport *transport,
 	x = finish_command(&proc);
 	if (!ret)
 		ret = x;
-
 	return ret;
 }
 
+static int run_pre_push_hook(struct transport *transport,
+			     struct ref *remote_refs)
+{
+	struct pre_push_hook_data data = { transport, remote_refs };
+	return for_each_hook("pre-push", do_run_pre_push_hook, &data);
+}
+
 int transport_push(struct repository *r,
 		   struct transport *transport,
 		   struct refspec *rs, int flags,

[PATCH v2 6/7] config: allow configuration of multiple hook error behavior

[brian m. carlson]

There are a variety of situations in which a user may want an error
behavior for multiple hooks other than the default. Add a config option,
hook.<name>.errorBehavior to allow users to customize this behavior on a
per-hook basis. Provide options for the default behavior (exiting
early), executing all hooks and succeeding if all hooks succeed, or
executing all hooks and succeeding if any hook succeeds.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 config.c       |  27 +++++++++++++
 run-command.c  |  42 +++++++++++++++++---
 run-command.h  |   5 +++
 t/lib-hooks.sh | 106 ++++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 173 insertions(+), 7 deletions(-)

diff --git a/config.c b/config.c
index c2846df3f1..9cba4061a9 100644
--- a/config.c
+++ b/config.c
@@ -19,6 +19,7 @@
 #include "utf8.h"
 #include "dir.h"
 #include "color.h"
+#include "run-command.h"
 
 struct config_source {
 	struct config_source *prev;
@@ -1093,6 +1094,29 @@ int git_config_color(char *dest, const char *var, const char *value)
 	return 0;
 }
 
+static int git_default_hook_config(const char *key, const char *value)
+{
+	const char *hook;
+	size_t key_len;
+	uintptr_t behavior;
+
+	key += strlen("hook.");
+	if (strip_suffix(key, ".errorbehavior", &key_len)) {
+		hook = xmemdupz(key, key_len);
+		if (!strcmp(value, "stop-on-first"))
+			behavior = HOOK_ERROR_STOP_ON_FIRST;
+		else if (!strcmp(value, "report-any-error"))
+			behavior = HOOK_ERROR_REPORT_ANY_ERROR;
+		else if (!strcmp(value, "report-any-success"))
+			behavior = HOOK_ERROR_REPORT_ANY_SUCCESS;
+		else
+			die(_("invalid mode for hook %s error behavior: %s"), hook, value);
+		string_list_insert(&hook_error_behavior, hook)->util = (void *)behavior;
+		return 0;
+	}
+	return 0;
+}
+
 static int git_default_core_config(const char *var, const char *value, void *cb)
 {
 	/* This needs a better name */
@@ -1450,6 +1474,9 @@ int git_default_config(const char *var, const char *value, void *cb)
 	    starts_with(var, "committer."))
 		return git_ident_config(var, value, cb);
 
+	if (starts_with(var, "hook."))
+		return git_default_hook_config(var, value);
+
 	if (starts_with(var, "i18n."))
 		return git_default_i18n_config(var, value);
 
diff --git a/run-command.c b/run-command.c
index 191d6f6f7e..70fb19a55b 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1308,6 +1308,8 @@ int async_with_fork(void)
 #endif
 }
 
+struct string_list hook_error_behavior = STRING_LIST_INIT_NODUP;
+
 /*
  * 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,
@@ -1401,18 +1403,48 @@ int for_each_hook(const char *name,
 		  void *data)
 {
 	struct string_list paths = STRING_LIST_INIT_DUP;
-	int i, ret = 0;
+	int i, hret = 0;
+	uintptr_t behavior = HOOK_ERROR_STOP_ON_FIRST;
+	struct string_list_item *item;
+	/* Use -2 as sentinel because failure to exec is -1. */
+	int ret = -2;
+
+	item = string_list_lookup(&hook_error_behavior, name);
+	if (item)
+		behavior = (uintptr_t)item->util;
 
 	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;
+		hret = handler(name, p, data);
+		switch (behavior) {
+			case HOOK_ERROR_STOP_ON_FIRST:
+				if (hret) {
+					ret = hret;
+					goto out;
+				}
+				break;
+			case HOOK_ERROR_REPORT_ANY_SUCCESS:
+				if (ret == -2)
+					ret = 1;
+				if (!hret)
+					ret = 0;
+				break;
+			case HOOK_ERROR_REPORT_ANY_ERROR:
+				if (ret == -2)
+					ret = 0;
+				if (hret)
+					ret = hret;
+				break;
+			default:
+				BUG("unknown hook error behavior");
+		}
 	}
-
+out:
 	string_list_clear(&paths, 0);
+	if (ret == -2)
+		return 0;
 	return ret;
 }
 
diff --git a/run-command.h b/run-command.h
index 15974e26d4..879ebb768f 100644
--- a/run-command.h
+++ b/run-command.h
@@ -63,6 +63,11 @@ int finish_command(struct child_process *);
 int finish_command_in_signal(struct child_process *);
 int run_command(struct child_process *);
 
+#define HOOK_ERROR_STOP_ON_FIRST 1
+#define HOOK_ERROR_REPORT_ANY_ERROR 2
+#define HOOK_ERROR_REPORT_ANY_SUCCESS 3
+extern struct string_list hook_error_behavior;
+
 /*
  * Returns the paths to all hook files, or NULL if all hooks are missing or
  * disabled.
diff --git a/t/lib-hooks.sh b/t/lib-hooks.sh
index 721250aea0..c1d7688313 100644
--- a/t/lib-hooks.sh
+++ b/t/lib-hooks.sh
@@ -121,7 +121,7 @@ test_multiple_hooks () {
 		! test -f "$OUTPUTDIR/c"
 	'
 
-	test_expect_success "$hook: multiple hooks, all successful" '
+	test_expect_success "$hook: multiple hooks, all successful by default" '
 		test_when_finished "rm -fr \"$OUTPUTDIR\"" &&
 		rm -f "$HOOK" &&
 		create_multihooks 0 0 0 &&
@@ -131,7 +131,40 @@ test_multiple_hooks () {
 		test -f "$OUTPUTDIR/c"
 	'
 
-	test_expect_success "$hook: hooks after first failure not executed" '
+	test_expect_success "$hook: multiple hooks, all successful with stop-on-first" '
+		test_when_finished "rm -fr \"$OUTPUTDIR\"" &&
+		test_config "hook.$hook.errorbehavior" stop-on-first &&
+		rm -f "$HOOK" &&
+		create_multihooks 0 0 0 &&
+		$cmd content-stop-on-first &&
+		test -f "$OUTPUTDIR/a" &&
+		test -f "$OUTPUTDIR/b" &&
+		test -f "$OUTPUTDIR/c"
+	'
+
+	test_expect_success "$hook: multiple hooks, all successful with report-any-error" '
+		test_when_finished "rm -fr \"$OUTPUTDIR\"" &&
+		test_config "hook.$hook.errorbehavior" report-any-error &&
+		rm -f "$HOOK" &&
+		create_multihooks 0 0 0 &&
+		$cmd content-report-any-error &&
+		test -f "$OUTPUTDIR/a" &&
+		test -f "$OUTPUTDIR/b" &&
+		test -f "$OUTPUTDIR/c"
+	'
+
+	test_expect_success "$hook: multiple hooks, all successful with report-any-success" '
+		test_when_finished "rm -fr \"$OUTPUTDIR\"" &&
+		test_config "hook.$hook.errorbehavior" report-any-success &&
+		rm -f "$HOOK" &&
+		create_multihooks 0 0 0 &&
+		$cmd content-report-any-success &&
+		test -f "$OUTPUTDIR/a" &&
+		test -f "$OUTPUTDIR/b" &&
+		test -f "$OUTPUTDIR/c"
+	'
+
+	test_expect_success "$hook: hooks after first failure not executed by default" '
 		test_when_finished "rm -fr \"$OUTPUTDIR\"" &&
 		create_multihooks 0 1 0 &&
 		$must_fail $cmd more-content &&
@@ -140,6 +173,75 @@ test_multiple_hooks () {
 		! test -f "$OUTPUTDIR/c"
 	'
 
+	test_expect_success "$hook: hooks after first failure not executed with stop-on-first" '
+		test_when_finished "rm -fr \"$OUTPUTDIR\"" &&
+		test_config "hook.$hook.errorbehavior" stop-on-first &&
+		create_multihooks 0 1 0 &&
+		$must_fail $cmd more-content-stop-on-first &&
+		test -f "$OUTPUTDIR/a" &&
+		test -f "$OUTPUTDIR/b" &&
+		! test -f "$OUTPUTDIR/c"
+	'
+
+	test_expect_success "$hook: hooks after first failure executed with report-any-error" '
+		test_when_finished "rm -fr \"$OUTPUTDIR\"" &&
+		test_config "hook.$hook.errorbehavior" report-any-error &&
+		create_multihooks 0 1 0 &&
+		$must_fail $cmd more-content-report-any-error &&
+		test -f "$OUTPUTDIR/a" &&
+		test -f "$OUTPUTDIR/b" &&
+		test -f "$OUTPUTDIR/c"
+	'
+
+	test_expect_success "$hook: hooks after first failure executed with report-any-success" '
+		test_when_finished "rm -fr \"$OUTPUTDIR\"" &&
+		test_config "hook.$hook.errorbehavior" report-any-success &&
+		create_multihooks 0 1 0 &&
+		$cmd more-content-report-any-success &&
+		test -f "$OUTPUTDIR/a" &&
+		test -f "$OUTPUTDIR/b" &&
+		test -f "$OUTPUTDIR/c"
+	'
+
+	test_expect_success "$hook: failing hooks by default" '
+		test_when_finished "rm -fr \"$OUTPUTDIR\"" &&
+		create_multihooks 1 1 1 &&
+		$must_fail $cmd most-content &&
+		test -f "$OUTPUTDIR/a" &&
+		! test -f "$OUTPUTDIR/b" &&
+		! test -f "$OUTPUTDIR/c"
+	'
+
+	test_expect_success "$hook: failing hooks with stop-on-first" '
+		test_when_finished "rm -fr \"$OUTPUTDIR\"" &&
+		test_config "hook.$hook.errorbehavior" stop-on-first &&
+		create_multihooks 1 1 1 &&
+		$must_fail $cmd most-content-stop-on-first &&
+		test -f "$OUTPUTDIR/a" &&
+		! test -f "$OUTPUTDIR/b" &&
+		! test -f "$OUTPUTDIR/c"
+	'
+
+	test_expect_success "$hook: failing hooks with report-any-error" '
+		test_when_finished "rm -fr \"$OUTPUTDIR\"" &&
+		test_config "hook.$hook.errorbehavior" report-any-error &&
+		create_multihooks 1 1 1 &&
+		$must_fail $cmd most-content-report-any-error &&
+		test -f "$OUTPUTDIR/a" &&
+		test -f "$OUTPUTDIR/b" &&
+		test -f "$OUTPUTDIR/c"
+	'
+
+	test_expect_success "$hook: failing hooks with report-any-success" '
+		test_when_finished "rm -fr \"$OUTPUTDIR\"" &&
+		test_config "hook.$hook.errorbehavior" report-any-success &&
+		create_multihooks 1 1 1 &&
+		$must_fail $cmd most-content-report-any-success &&
+		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 &&

Re: [PATCH v2 6/7] config: allow configuration of multiple hook error behavior

[Duy Nguyen]

On Tue, May 14, 2019 at 7:24 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> There are a variety of situations in which a user may want an error
> behavior for multiple hooks other than the default. Add a config option,
> hook.<name>.errorBehavior to allow users to customize this behavior on a

An alternative name is onError, probably more often used for event
callbacks. But I don't know, maybe errorBehavior is actually better.

> per-hook basis. Provide options for the default behavior (exiting

should we fall back to hook.errorBehavior? That allows people to set
global policy, then customize just a small set of weird hooks.

> early), executing all hooks and succeeding if all hooks succeed, or
> executing all hooks and succeeding if any hook succeeds.
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  config.c       |  27 +++++++++++++
>  run-command.c  |  42 +++++++++++++++++---
>  run-command.h  |   5 +++
>  t/lib-hooks.sh | 106 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 173 insertions(+), 7 deletions(-)
>
> diff --git a/config.c b/config.c
> index c2846df3f1..9cba4061a9 100644
> --- a/config.c
> +++ b/config.c
> @@ -19,6 +19,7 @@
>  #include "utf8.h"
>  #include "dir.h"
>  #include "color.h"
> +#include "run-command.h"
>
>  struct config_source {
>         struct config_source *prev;
> @@ -1093,6 +1094,29 @@ int git_config_color(char *dest, const char *var, const char *value)
>         return 0;
>  }
>
> +static int git_default_hook_config(const char *key, const char *value)
> +{
> +       const char *hook;
> +       size_t key_len;
> +       uintptr_t behavior;
> +
> +       key += strlen("hook.");
> +       if (strip_suffix(key, ".errorbehavior", &key_len)) {
> +               hook = xmemdupz(key, key_len);
> +               if (!strcmp(value, "stop-on-first"))

maybe stop-on-first-error (or if you go with the "onError" name, I
think "stop" is enough). I know "stop on/after first hook" does not
really make any sense when you think about it. Maybe stop-on-first is
sufficient.

I was going to suggest strcasecmp. But core.whitespace (also has
multiple-word-values) already sets a precedent on strcmp. I think
we're good. Or mostly good, I don't know, we still accept False, false
and FALSE.

> +                       behavior = HOOK_ERROR_STOP_ON_FIRST;

This is basically the logical "and" behavior in a C expression. Which
makes me think if anybody's crazy enough to need the "or" counterpart
(i.e. run hooks, expect failure, keep going until the first success).

I guess it's a crazy mode. We should not care about until a real use
case shows up.

> +               else if (!strcmp(value, "report-any-error"))

I couldn't guess based on this name alone, whether we continue or stop
after the reporting part. The 7/7 document makes it clear though. So
all good.

> diff --git a/run-command.c b/run-command.c
> index 191d6f6f7e..70fb19a55b 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1308,6 +1308,8 @@ int async_with_fork(void)
>  #endif
>  }
>
> +struct string_list hook_error_behavior = STRING_LIST_INIT_NODUP;

Maybe stick this in 'struct repository'. I know most config variables
are still global. But I think we have to move/reorganize them at some
point. Most may end up in 'struct repository'.

> @@ -1401,18 +1403,48 @@ int for_each_hook(const char *name,
>                   void *data)
>  {
>         struct string_list paths = STRING_LIST_INIT_DUP;
> -       int i, ret = 0;
> +       int i, hret = 0;
> +       uintptr_t behavior = HOOK_ERROR_STOP_ON_FIRST;
> +       struct string_list_item *item;
> +       /* Use -2 as sentinel because failure to exec is -1. */
> +       int ret = -2;
> +
> +       item = string_list_lookup(&hook_error_behavior, name);
> +       if (item)
> +               behavior = (uintptr_t)item->util;
>
>         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;
> +               hret = handler(name, p, data);
> +               switch (behavior) {
> +                       case HOOK_ERROR_STOP_ON_FIRST:
> +                               if (hret) {
> +                                       ret = hret;
> +                                       goto out;
> +                               }
> +                               break;
> +                       case HOOK_ERROR_REPORT_ANY_SUCCESS:
> +                               if (ret == -2)
> +                                       ret = 1;
> +                               if (!hret)
> +                                       ret = 0;
> +                               break;
> +                       case HOOK_ERROR_REPORT_ANY_ERROR:
> +                               if (ret == -2)
> +                                       ret = 0;
> +                               if (hret)
> +                                       ret = hret;
> +                               break;
> +                       default:
> +                               BUG("unknown hook error behavior");

maybe BUG(".. behavior %d", behavior);

> +               }
>         }
> -
> +out:
>         string_list_clear(&paths, 0);
> +       if (ret == -2)
> +               return 0;
>         return ret;
>  }
>
-- 
Duy

Re: [PATCH v2 6/7] config: allow configuration of multiple hook error behavior

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 3178 bytes --]

On Tue, May 14, 2019 at 08:20:10PM +0700, Duy Nguyen wrote:
> On Tue, May 14, 2019 at 7:24 AM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> >
> > There are a variety of situations in which a user may want an error
> > behavior for multiple hooks other than the default. Add a config option,
> > hook.<name>.errorBehavior to allow users to customize this behavior on a
> 
> An alternative name is onError, probably more often used for event
> callbacks. But I don't know, maybe errorBehavior is actually better.

I'm going to use "errorStrategy", since we already have
submodule.alternateErrorStrategy.

> > per-hook basis. Provide options for the default behavior (exiting
> 
> should we fall back to hook.errorBehavior? That allows people to set
> global policy, then customize just a small set of weird hooks.

Sure, that sounds good.

> maybe stop-on-first-error (or if you go with the "onError" name, I
> think "stop" is enough). I know "stop on/after first hook" does not
> really make any sense when you think about it. Maybe stop-on-first is
> sufficient.
> 
> I was going to suggest strcasecmp. But core.whitespace (also has
> multiple-word-values) already sets a precedent on strcmp. I think
> we're good. Or mostly good, I don't know, we still accept False, false
> and FALSE.

I think with errorStrategy, "stop" is fine. Simpler is better.

I literally picked what Peff had suggested in his email (mostly because
I'm terrible at naming things), and I don't get the impression he spent
a great deal of time analyzing the ins and outs of the names before
sending. I could be wrong, though.

> > +                       behavior = HOOK_ERROR_STOP_ON_FIRST;
> 
> This is basically the logical "and" behavior in a C expression. Which
> makes me think if anybody's crazy enough to need the "or" counterpart
> (i.e. run hooks, expect failure, keep going until the first success).
> 
> I guess it's a crazy mode. We should not care about until a real use
> case shows up.

Yeah, I think that's unlikely to be the case. Nothing prevents us from
adding it later.

> > +               else if (!strcmp(value, "report-any-error"))
> 
> I couldn't guess based on this name alone, whether we continue or stop
> after the reporting part. The 7/7 document makes it clear though. So
> all good.

I'm open to hearing better suggestions if anyone has any.

> > diff --git a/run-command.c b/run-command.c
> > index 191d6f6f7e..70fb19a55b 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -1308,6 +1308,8 @@ int async_with_fork(void)
> >  #endif
> >  }
> >
> > +struct string_list hook_error_behavior = STRING_LIST_INIT_NODUP;
> 
> Maybe stick this in 'struct repository'. I know most config variables
> are still global. But I think we have to move/reorganize them at some
> point. Most may end up in 'struct repository'.

Okay, sounds fine.

> > +                       default:
> > +                               BUG("unknown hook error behavior");
> 
> maybe BUG(".. behavior %d", behavior);

Sure.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH v2 6/7] config: allow configuration of multiple hook error behavior

[Jeff King]

On Wed, May 15, 2019 at 11:10:17PM +0000, brian m. carlson wrote:

> > An alternative name is onError, probably more often used for event
> > callbacks. But I don't know, maybe errorBehavior is actually better.
> 
> I'm going to use "errorStrategy", since we already have
> submodule.alternateErrorStrategy.

That sounds good (and I don't care too much about the name as long as it
it is in the per-hook subsection like this).

> > should we fall back to hook.errorBehavior? That allows people to set
> > global policy, then customize just a small set of weird hooks.
> 
> Sure, that sounds good.

I like this, too.

> > maybe stop-on-first-error (or if you go with the "onError" name, I
> > think "stop" is enough). I know "stop on/after first hook" does not
> > really make any sense when you think about it. Maybe stop-on-first is
> > sufficient.
> > 
> > I was going to suggest strcasecmp. But core.whitespace (also has
> > multiple-word-values) already sets a precedent on strcmp. I think
> > we're good. Or mostly good, I don't know, we still accept False, false
> > and FALSE.
> 
> I think with errorStrategy, "stop" is fine. Simpler is better.
> 
> I literally picked what Peff had suggested in his email (mostly because
> I'm terrible at naming things), and I don't get the impression he spent
> a great deal of time analyzing the ins and outs of the names before
> sending. I could be wrong, though.

No, I didn't. :) I think "stop" is good. If the others are
report-any-error and report-any-success, then the matching name for this
could be report-first-error.

> > > +               else if (!strcmp(value, "report-any-error"))
> > 
> > I couldn't guess based on this name alone, whether we continue or stop
> > after the reporting part. The 7/7 document makes it clear though. So
> > all good.
> 
> I'm open to hearing better suggestions if anyone has any.

Maybe report-all-errors would indicate that it was going to run all of
the hooks. I dunno. I think the documentation you wrote is plenty clear
with the current name.

-Peff

Re: [PATCH v2 6/7] config: allow configuration of multiple hook error behavior

[Jeff King]

On Tue, May 14, 2019 at 12:23:31AM +0000, brian m. carlson wrote:

> There are a variety of situations in which a user may want an error
> behavior for multiple hooks other than the default. Add a config option,
> hook.<name>.errorBehavior to allow users to customize this behavior on a
> per-hook basis. Provide options for the default behavior (exiting
> early), executing all hooks and succeeding if all hooks succeed, or
> executing all hooks and succeeding if any hook succeeds.

Thanks for using that naming scheme. I think if we do move to allowing
config-based hooks, the config schemes will mesh together well.

> +static int git_default_hook_config(const char *key, const char *value)
> +{
> +	const char *hook;
> +	size_t key_len;
> +	uintptr_t behavior;
> +
> +	key += strlen("hook.");
> +	if (strip_suffix(key, ".errorbehavior", &key_len)) {

There's an undocumented assumption that the caller has confirmed that
the key starts with "hook." here. Can we be a little more defensive and
do:

  if (skip_prefix(key, "hook.", &key))
	return 0;

here (we could even drop the check in git_default_config).

Or we could use parse_key(), which is designed for this:

  if (parse_key(key, "hook", &subsection, &subsection_len, &key) < 0 ||
      !subsection)
	return 0;
  if (!strcmp(key, "errorbehavior"))
	...

> +	/* Use -2 as sentinel because failure to exec is -1. */
> +	int ret = -2;

Maybe this would be simpler to follow by using an enum for the handler
return value?

Aside from these nits, the code looked sensible to me.

-Peff

Re: [PATCH v2 6/7] config: allow configuration of multiple hook error behavior

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1419 bytes --]

On 2019-05-16 at 05:02:00, Jeff King wrote:
> > +static int git_default_hook_config(const char *key, const char *value)
> > +{
> > +	const char *hook;
> > +	size_t key_len;
> > +	uintptr_t behavior;
> > +
> > +	key += strlen("hook.");
> > +	if (strip_suffix(key, ".errorbehavior", &key_len)) {
> 
> There's an undocumented assumption that the caller has confirmed that
> the key starts with "hook." here. Can we be a little more defensive and
> do:
> 
>   if (skip_prefix(key, "hook.", &key))
> 	return 0;

Yeah, the caller checks that, but I think being a little more defensive
is fine.

> here (we could even drop the check in git_default_config).
> 
> Or we could use parse_key(), which is designed for this:
> 
>   if (parse_key(key, "hook", &subsection, &subsection_len, &key) < 0 ||
>       !subsection)
> 	return 0;

Oh, good, I didn't know we had that. That's exactly what I want.

>   if (!strcmp(key, "errorbehavior"))
> 	...
> 
> > +	/* Use -2 as sentinel because failure to exec is -1. */
> > +	int ret = -2;
> 
> Maybe this would be simpler to follow by using an enum for the handler
> return value?

We can't make this variable an enum because we'd have to define 256
entries (well, we can, but it would be a hassle), but I can create an
enum and assign it to the int variable, sure.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH v2 6/7] config: allow configuration of multiple hook error behavior

[Jeff King]

On Thu, May 16, 2019 at 05:19:53PM +0000, brian m. carlson wrote:

> > > +	/* Use -2 as sentinel because failure to exec is -1. */
> > > +	int ret = -2;
> > 
> > Maybe this would be simpler to follow by using an enum for the handler
> > return value?
> 
> We can't make this variable an enum because we'd have to define 256
> entries (well, we can, but it would be a hassle), but I can create an
> enum and assign it to the int variable, sure.

I think you can do:

  enum HOOK_ERR {
	HOOK_ERR_NONE = -2,
	HOOK_ERR_EXEC = -1,
	/* otherwise it should be a system exit code */
	HOOK_ERR_MAX = 255
  };

which ensures that the enum can hold any exit status.

-Peff

[PATCH v2 7/7] docs: document multiple hooks

[brian m. carlson]

Document the semantics and behavior of multiple hooks, including the
configuration options and requirements for them to be run.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 Documentation/config.txt      |  2 ++
 Documentation/config/hook.txt | 19 +++++++++++++++++++
 Documentation/githooks.txt    |  9 +++++++++
 3 files changed, 30 insertions(+)
 create mode 100644 Documentation/config/hook.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d87846faa6..f62b8ce494 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -350,6 +350,8 @@ include::config/guitool.txt[]
 
 include::config/help.txt[]
 
+include::config/hook.txt[]
+
 include::config/http.txt[]
 
 include::config/i18n.txt[]
diff --git a/Documentation/config/hook.txt b/Documentation/config/hook.txt
new file mode 100644
index 0000000000..4585cc7f55
--- /dev/null
+++ b/Documentation/config/hook.txt
@@ -0,0 +1,19 @@
+hook.<name>.errorBehavior::
+  Control the error behavior when using multiple hooks.
++
+--
+* `stop-on-first` - If a hook fails, do not execute further hooks, even if the
+  command normally ignores whether hooks succeed or fail, and return its exit
+  code as the exit code of the hook set. If all hooks succeed, the exit code is 0.
+  This is the default.
+* `report-any-error` - Always execute all hooks, but return the exit code of the
+  first failing hook as the exit code of the hook set. If all hooks succeed, the
+  exit code of the hook set is 0.
+* `report-any-success` - Always execute all hooks, and if any hook succeeds,
+  return 0 as the exit code of the hook set. If all hooks fail, the exit code of
+  the hook set is 1.
+--
++
+If the exit code of the hook set is zero, then the hooks are considered to have
+succeeded; otherwise, they are considered to have failed. Note that the success
+or failure of some hooks is ignored (see linkgit:githooks[5] for more).
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 786e778ab8..c680e575b3 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -31,6 +31,15 @@ Hooks can get their arguments via the environment, command-line
 arguments, and stdin. See the documentation for each hook below for
 details.
 
+It is possible to provide multiple hooks for a single function. If the
+main hook file is absent, hooks are additionally looked for in a
+directory with the name of the main hook file with a `.d` appended.
+(That is, if `post-receive` is missing, `post-receive.d` is inspected
+for any hooks that might be present.) Each of these hooks is executed in order,
+sorted by file name. By default, if a hook fails, additional hooks are not
+executed, but this can be controlled with the `hook.*.errorBehavior` setting
+(see linkgit:git-config[1]).
+
 `git init` may copy hooks to the new repository, depending on its
 configuration. See the "TEMPLATE DIRECTORY" section in
 linkgit:git-init[1] for details. When the rest of this document refers

Re: [PATCH v2 7/7] docs: document multiple hooks

[Duy Nguyen]

On Tue, May 14, 2019 at 7:24 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> +It is possible to provide multiple hooks for a single function. If the
> +main hook file is absent,

If I remember 1/7 correctly, if the hook "file" is a directory, you
ignore it and check for hook.d too.

Which makes me think, can we just check if hooks/<hook> is a directory
and use it instead of hooks/<hook>.d? [1]

The only advantage of <hook>.d that I can see is if we support some
sort of combination of <hook> and <hook>.d.

[1] Of course if you're really strict on backward compatibility then
this is out of question.

> hooks are additionally looked for in a
> +directory with the name of the main hook file with a `.d` appended.
> +(That is, if `post-receive` is missing, `post-receive.d` is inspected
> +for any hooks that might be present.) Each of these hooks is executed in order,
> +sorted by file name. By default, if a hook fails, additional hooks are not
> +executed, but this can be controlled with the `hook.*.errorBehavior` setting
> +(see linkgit:git-config[1]).
> +
>  `git init` may copy hooks to the new repository, depending on its
>  configuration. See the "TEMPLATE DIRECTORY" section in
>  linkgit:git-init[1] for details. When the rest of this document refers



-- 
Duy

Re: [PATCH v2 0/7] Multiple hook support

[Jonathan Nieder]

Hi,

brian m. carlson wrote:

> I've thought a lot about the discussion over whether this series should
> use the configuration as the source for multiple hooks. Ultimately, I've
> come to the decision that it's not a good idea. Even adopting the empty
> entry as a reset marker, the fact that inheritance in the configuration
> is in-order and can't be easily modified means that it's not likely to
> be very useful, but it is likely to be quite surprising for the average
> user.

Can we discuss this some more?  What would it take to make it likely
to be useful in your view?

> I think a solution that sticks with the existing model and builds
> off a design used by other systems people are familiar with, like cron
> and run-parts, is going to be a better choice. Moreover, this is the
> design that people have already built with outside tooling, which is a
> further argument in favor of it.

To be clear, the main advantage I see for config versus the .git/hooks
model is that with the config model, a user doesn't have to search
throughout the filesystem for .git/hooks directories to update when a
hook is broken.

There are other models that similarly fix that --- e.g. putting hooks
in /etc.  But as long as they're in .git/hooks, I think we're digging
ourselves deeper into the same hole.

Thanks,
Jonathan

Re: [PATCH v2 0/7] Multiple hook support

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 4222 bytes --]

On Mon, May 13, 2019 at 05:51:01PM -0700, Jonathan Nieder wrote:
> Hi,
> 
> brian m. carlson wrote:
> 
> > I've thought a lot about the discussion over whether this series should
> > use the configuration as the source for multiple hooks. Ultimately, I've
> > come to the decision that it's not a good idea. Even adopting the empty
> > entry as a reset marker, the fact that inheritance in the configuration
> > is in-order and can't be easily modified means that it's not likely to
> > be very useful, but it is likely to be quite surprising for the average
> > user.
> 
> Can we discuss this some more?  What would it take to make it likely
> to be useful in your view?

There are two aspects here which I think are worth discussing. Let's
discuss the inheritance issue first.

Order with multiple hooks matters. The best hook as an example for this
is prepare-commit-msg. If I have a hook which I want to run on every
repository, such as a hook that inserts some sort of ID (bug tracker,
Gerrit, etc.), that hook, due to inheritance, *has* to be first, before
any other prepare-commit-msg hooks. If I want a hook that runs before
it, perhaps because a particular repository needs a different
configuration, I have to wipe the list and insert both hooks. I'm now
maintaining two separate locations for the command lines instead of just
inserting a symlink to the global hook and dropping a new hook before
it.

I don't think there's a good way to make it easier unless we radically
customize the way configuration is done. I don't doubt that there are a
small number of configurations where the inheritance behavior is fine—I
believe GitHub's backend is one of them—but overall I think it's hard to
reason about and customize.

The second issue here is that it's surprising. Users don't know how to
reset a configuration option because we don't have a consistent way to
do that. Users will not expect for there to be multiple ways to set
hooks. Users will also not expect that their hooks in their
configuration aren't run if there are hooks in .git/hooks. Tooling that
has so far used .git/hooks will compete with users' global configuration
options, which I guarantee you will be a surprise for users using older
versions of tools.

The new behavior, which puts everything in the same directory
(.git/hooks) is much easier to reason about. I think we're obligated to
consider the experience for the average end user, who may not be
intimately familiar with how Git works but still needs to use it to get
work done.

It also provides a convenient place for hooks to live, which a
config-based option doesn't. We'll need to invoke things using /bin/sh,
so will they all have to live in PATH? What about one-offs that don't
really belong in PATH?

> > I think a solution that sticks with the existing model and builds
> > off a design used by other systems people are familiar with, like cron
> > and run-parts, is going to be a better choice. Moreover, this is the
> > design that people have already built with outside tooling, which is a
> > further argument in favor of it.
> 
> To be clear, the main advantage I see for config versus the .git/hooks
> model is that with the config model, a user doesn't have to search
> throughout the filesystem for .git/hooks directories to update when a
> hook is broken.

I agree this is an advantage if they don't hit the ordering issue. I
think a lot of the common use cases where this approach has benefits can
be handled well with core.hooksPath and hooks that can turn themselves
on or off depending on the repository config.

What might be an interesting approach that would address these concerns
is a core.globalHooks[0] option that points to a set (or sets,
depending) of multiple-hook directories. We then enumerate hooks in sort
order, considering both the global and the local directories as one
unit, perhaps with some way of disabling hooks. I'm not planning on
working on this myself, but I wouldn't be opposed to seeing someone else
work on it.

[0] Better name suggestions are, of course, welcome.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH v2 0/7] Multiple hook support

[Jonathan Nieder]

Hi,

brian m. carlson wrote:
> On Mon, May 13, 2019 at 05:51:01PM -0700, Jonathan Nieder wrote:
>> brian m. carlson wrote:

>>>                          the fact that inheritance in the configuration
>>> is in-order and can't be easily modified means that it's not likely to
>>> be very useful, but it is likely to be quite surprising for the average
>>> user.
>>
>> Can we discuss this some more?  What would it take to make it likely
>> to be useful in your view?
>
> There are two aspects here which I think are worth discussing. Let's
> discuss the inheritance issue first.
>
> Order with multiple hooks matters. The best hook as an example for this
> is prepare-commit-msg. If I have a hook which I want to run on every
> repository, such as a hook that inserts some sort of ID (bug tracker,
> Gerrit, etc.), that hook, due to inheritance, *has* to be first, before
> any other prepare-commit-msg hooks. If I want a hook that runs before
> it, perhaps because a particular repository needs a different
> configuration, I have to wipe the list and insert both hooks. I'm now
> maintaining two separate locations for the command lines instead of just
> inserting a symlink to the global hook and dropping a new hook before
> it.
>
> I don't think there's a good way to make it easier unless we radically
> customize the way configuration is done.

Wouldn't a separate config item e.g. to reverse order (or to perform
whatever other customization seems appropriate) cover this?

In other words, use the standard config convention for the set of
hooks, and treat the order in which they are invoked as a separate
question.  You could even use the hooks.d style alphabetical order
convention.

[...]
> The second issue here is that it's surprising. Users don't know how to
> reset a configuration option because we don't have a consistent way to
> do that.

I agree that it's underdocumented and underimplemented.  But I'm not
aware of any other method that Git provides to reset a configuration
item.  What is it inconsistent with?

>           Users will not expect for there to be multiple ways to set
> hooks. Users will also not expect that their hooks in their
> configuration aren't run if there are hooks in .git/hooks. Tooling that
> has so far used .git/hooks will compete with users' global configuration
> options, which I guarantee you will be a surprise for users using older
> versions of tools.

Indeed, in the long term I think we should remove the .git/hooks/
mechanism entirely.

In the shorter term, I think the kind of inconsistency you're referring
to applies to hooks.d as well.

> The new behavior, which puts everything in the same directory
> (.git/hooks) is much easier to reason about.

That's a good point: a .git/hooks/README sounds like it would be
helpful here.

[...]
> It also provides a convenient place for hooks to live, which a
> config-based option doesn't. We'll need to invoke things using /bin/sh,
> so will they all have to live in PATH? What about one-offs that don't
> really belong in PATH?

This hasn't been a problem for remote helpers, merge drivers, etc in
the past.  Why are hooks different?

To be clear, I think it's a reasonable problem to solve, and I've
actually been surprised that it hasn't been a problem for people.

[...]
> I agree this is an advantage if they don't hit the ordering issue.

Wonderful.  Sounds like if I do some work on the ordering issue, then
we have a path forward.

>                                                                    I
> think a lot of the common use cases where this approach has benefits can
> be handled well with core.hooksPath and hooks that can turn themselves
> on or off depending on the repository config.

I think core.hooksPath attempted to solve this problem, but it has
several deficiencies:

1. It assumes a single, centrally managed hooks directory, and there's
   no standard for where that directory lives.  This means that it
   can't be counted on by tools like "git secrets" --- instead, each
   particular installation has to set up a custom hooks directory for
   themselves.

2. Since it assumes a single, centrally managed hooks directory,
   customizations in a single repository (e.g. to enable or disable a
   single hook) require duplicating the entire directory.

3. It had no migration path defined to becoming the default, so it
   doesn't end up being discoverable.  core.hooksPath is designed as
   a special case, making it hard to debug, instead of being a
   mainstream setting that can be recommended as a future default.

> What might be an interesting approach that would address these concerns
> is a core.globalHooks[0] option that points to a set (or sets,
> depending) of multiple-hook directories. We then enumerate hooks in sort
> order, considering both the global and the local directories as one
> unit, perhaps with some way of disabling hooks. I'm not planning on
> working on this myself, but I wouldn't be opposed to seeing someone else
> work on it.

This sounds overflexible to me.  Because of that, I don't think it
would end up as a default, so we wouldn't have a path to improving our
security stature.

If I implement a config based multiple hooks feature with name based
ordering, would that be useful to you?

Thanks,
Jonathan

> [0] Better name suggestions are, of course, welcome.

Re: [PATCH v2 0/7] Multiple hook support

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 5452 bytes --]

On Mon, May 13, 2019 at 07:26:53PM -0700, Jonathan Nieder wrote:
> brian m. carlson wrote:
> In other words, use the standard config convention for the set of
> hooks, and treat the order in which they are invoked as a separate
> question.  You could even use the hooks.d style alphabetical order
> convention.

If we're going to have named items in PATH, we do not want to have those
items numbered.

> [...]
> > The second issue here is that it's surprising. Users don't know how to
> > reset a configuration option because we don't have a consistent way to
> > do that.
> 
> I agree that it's underdocumented and underimplemented.  But I'm not
> aware of any other method that Git provides to reset a configuration
> item.  What is it inconsistent with?

Some options support it, and some don't.

> >           Users will not expect for there to be multiple ways to set
> > hooks. Users will also not expect that their hooks in their
> > configuration aren't run if there are hooks in .git/hooks. Tooling that
> > has so far used .git/hooks will compete with users' global configuration
> > options, which I guarantee you will be a surprise for users using older
> > versions of tools.
> 
> Indeed, in the long term I think we should remove the .git/hooks/
> mechanism entirely.

See, I don't. I like the current mechanism and don't want to get rid of
it.

> In the shorter term, I think the kind of inconsistency you're referring
> to applies to hooks.d as well.

I think it's a lot easier to reason about adding a directory to the
existing mechanism than adopting a dramatically new one. Users can
easily understand that if there's a file, that the directory is ignored.

Also, this is the way that most other programs on Unix do this behavior,
and I think that is a compelling argument for this design in and of
itself. I think Unix has generally made the best decisions in operating
system design, and I aim to emulate it as much as possible.

> > It also provides a convenient place for hooks to live, which a
> > config-based option doesn't. We'll need to invoke things using /bin/sh,
> > so will they all have to live in PATH? What about one-offs that don't
> > really belong in PATH?
> 
> This hasn't been a problem for remote helpers, merge drivers, etc in
> the past.  Why are hooks different?

Because they're often customized or installed via the repository itself.
Some projects provide pre-commit hooks to run tests or such in a
repository-specific way. Some people have one-off scripts that are
appropriate for a repo but they don't want in tab-completion. Also, they
may wrap a system command with helpful information, but that doesn't
belong in PATH.

Git LFS, for example, installs hooks that warn the user if the command
isn't installed, since it's a common misconfiguration and not pushing
the LFS objects (via the pre-push hook) along with the Git objects is a
common source of data loss. Uninstalling the tool (or not installing it
if it's a shared repository) doesn't mean the hook still shouldn't be
run.

> > What might be an interesting approach that would address these concerns
> > is a core.globalHooks[0] option that points to a set (or sets,
> > depending) of multiple-hook directories. We then enumerate hooks in sort
> > order, considering both the global and the local directories as one
> > unit, perhaps with some way of disabling hooks. I'm not planning on
> > working on this myself, but I wouldn't be opposed to seeing someone else
> > work on it.
> 
> This sounds overflexible to me.  Because of that, I don't think it
> would end up as a default, so we wouldn't have a path to improving our
> security stature.
> 
> If I implement a config based multiple hooks feature with name based
> ordering, would that be useful to you?

One of my primary motivations for writing this series is because it's a
requested feature in conjunction with Git LFS. We often get requests for
features that are properly directed to Git itself, but because the issue
comes up in conjunction with the use of Git LFS, folks report the issues
to us. (Also, people don't like reporting issues to mailing lists.) I
think it's a useful feature for others, though, judging by the multiple
times it's come up.

I see the series that I'm proposing as easy to implement and work with
using the existing tools and with other tools that use the existing
hooks. It's a simple change of path. I anticipate there being more work
to implement a config-based multiple hooks solution in various tools,
and I expect it will be less intuitive and less discoverable. I can't
provide definitive evidence for this, but my experience answering a lot
of user questions on a daily basis leads me to believe that it's not the
right way forward.

I'm not opposed to extending the config system to implement multiple
hooks directories or add support for inheriting hooks, because that's a
common thing that people want. I just don't think our config system is
the right tool for specifying what commands to run, for the reasons I've
specified.

I can't prevent you from writing a series that implements a config-based
option, and if it's the solution the list wants, I'll go along with it,
but it's not the solution I want to see personally or as a tooling
implementer.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH v2 0/7] Multiple hook support

[Jonathan Nieder]

brian m. carlson wrote:

> Also, this is the way that most other programs on Unix do this behavior,
> and I think that is a compelling argument for this design in and of
> itself. I think Unix has generally made the best decisions in operating
> system design, and I aim to emulate it as much as possible.

Do a lot of other programs run commands from a specially named
subdirectory of the current directory?

If you were talking about a hooks dir in /etc, I would completely
agree.  But we are talking about .git/hooks/, which has been a
constant source of real compromises.

Anyway, I think we've gone back and forth enough times to discover
we're not going to agree on this.

[...]
>> This hasn't been a problem for remote helpers, merge drivers, etc in
>> the past.  Why are hooks different?
[...]
> Git LFS, for example, installs hooks that warn the user if the command
> isn't installed, since it's a common misconfiguration and not pushing
> the LFS objects (via the pre-push hook) along with the Git objects is a
> common source of data loss. Uninstalling the tool (or not installing it
> if it's a shared repository) doesn't mean the hook still shouldn't be
> run.

I don't understand this example.  If the repository is configured to
use the Git LFS hooks, why wouldn't it print a friendly message?

[...]
> I'm not opposed to extending the config system to implement multiple
> hooks directories or add support for inheriting hooks, because that's a
> common thing that people want. I just don't think our config system is
> the right tool for specifying what commands to run, for the reasons I've
> specified.
>
> I can't prevent you from writing a series that implements a config-based
> option, and if it's the solution the list wants, I'll go along with it,
> but it's not the solution I want to see personally or as a tooling
> implementer.

I think you're answering a different question than I asked.

One thing I've been talking about is having a path to eventually
getting rid of support for .git/hooks/, on a user controlled timeline,
with a smooth migration.  I proposed one possible way to do that, and
I was asking whether it would work okay for your use case, or whether
there are problems with it (which would give me something to work with
on iterating toward something that would work for you).

Your answer is "I can't prevent you", which means you don't like the
proposal, but doesn't tell me what about it would not work.

Thanks,
Jonathan

Re: [PATCH v2 0/7] Multiple hook support

[Jeff King]

On Tue, May 14, 2019 at 01:59:28AM +0000, brian m. carlson wrote:

> There are two aspects here which I think are worth discussing. Let's
> discuss the inheritance issue first.
> 
> Order with multiple hooks matters. The best hook as an example for this
> is prepare-commit-msg. If I have a hook which I want to run on every
> repository, such as a hook that inserts some sort of ID (bug tracker,
> Gerrit, etc.), that hook, due to inheritance, *has* to be first, before
> any other prepare-commit-msg hooks. If I want a hook that runs before
> it, perhaps because a particular repository needs a different
> configuration, I have to wipe the list and insert both hooks. I'm now
> maintaining two separate locations for the command lines instead of just
> inserting a symlink to the global hook and dropping a new hook before
> it.

This part confuses me. In the config based scheme you describe, you have
to mention the original hook again. But isn't that exactly what creating
a symlink in your hooks directory is doing?

And in fact, I think a config based scheme can be a lot more flexible in
the end, simply because the parser _does_ see all of the hooks (whereas
in the hook directory scheme, we only ever look in one directory). So we
can have an option to sort them before running: alphabetically,
system-to-repo order, repo-to-system order, etc.

> The second issue here is that it's surprising. Users don't know how to
> reset a configuration option because we don't have a consistent way to
> do that. Users will not expect for there to be multiple ways to set
> hooks. Users will also not expect that their hooks in their
> configuration aren't run if there are hooks in .git/hooks. Tooling that
> has so far used .git/hooks will compete with users' global configuration
> options, which I guarantee you will be a surprise for users using older
> versions of tools.

I don't agree here. If the rule is "config takes precedence", and "if
config is absent "behave as if .git/hooks/whatever was in the config",
then the transition is easy to explain. And nobody sees any change at
all until they decide to set the config. It would probably also be
prudent to issue a warning if there's config _and_ an on-disk hook file
(or even run them both, though then you open up the question of
ordering).

Which isn't to say it's impossible for a person to get confused (new
tool sets the config, disabling their old hook?). But I think any
transition to multi-hook support (including your directory scheme) is
going to have some possibility that tools automatically setting things
up behind the scenes is going to confuse a user.

> It also provides a convenient place for hooks to live, which a
> config-based option doesn't. We'll need to invoke things using /bin/sh,
> so will they all have to live in PATH? What about one-offs that don't
> really belong in PATH?

Actually, my biggest beef with the current hooks mechanism is that it's
an _inconvenient_ place for them to live.

  - it's not version controlled, and putting a repository inside another
    repository is awkward (though it at least works for the bare case)

  - touching the filesystem is awkward and expensive if you have a large
    number of repositories whose hooks you want to update

  - it requires a manual step to modify the filesystem after a fresh
    clone (as opposed to putting things in ~/.gitconfig). Technically
    one can solve this by modifying .../share/git/templates, but that
    has its own issues.

In my world-view config-based hooks would just be run with SHELL_PATH,
just like our other config options. If you don't want one-offs in your
PATH, then use absolute paths (just like you would have to for
symlinks). If you really don't know where to put one-off hooks, perhaps
we could document a base directory from which the hook script is run,
and you could put it in a directory in .git and use a relative path? You
could even call that directory "hooks", but I suspect that would be too
confusing. :)

-Peff

Re: [PATCH v2 0/7] Multiple hook support

[Duy Nguyen]

On Tue, May 14, 2019 at 7:23 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> This series introduces multiple hook support.
>
> I've thought a lot about the discussion over whether this series should
> use the configuration as the source for multiple hooks. Ultimately, I've
> come to the decision that it's not a good idea. Even adopting the empty
> entry as a reset marker, the fact that inheritance in the configuration
> is in-order and can't be easily modified means that it's not likely to
> be very useful, but it is likely to be quite surprising for the average
> user.

Can we just do like we do with hooks-in-directory? Ignore the config
variable order. Sort them all alphabetically. The user then has to
prefix a number or something to control order. Easier transition from
hooks-in-dir to hooks-in-config too.

> I think a solution that sticks with the existing model and builds
> off a design used by other systems people are familiar with, like cron
> and run-parts, is going to be a better choice. Moreover, this is the
> design that people have already built with outside tooling, which is a
> further argument in favor of it.
-- 
Duy