[PATCH 0/5] Multiple hook support

[PATCH 0/5] Multiple hook support

[brian m. carlson]

Oftentimes, people want to use multiple of the same kind of hook. This
may be because software or a project they use requires a given hook, but
they would also like to have a custom hook, or because they're using
multiple pieces of software that both require involvement with the same
hook.

This series introduces support for multiple hooks by using a ".d"
directory. For example, instead of using a single
".git/hooks/post-checkout" file, you'd use multiple files within
".git/hooks/post-checkout.d". This is the standard Unix paradigm for
multiple files and should be familiar to most people. Hooks are executed
in sorted order.

To preserve backwards compatibility, we don't run the hooks in the ".d"
directory if the single file is a valid hook (i.e. it exists and is
executable). This is because some people already have multiple hook
scripts configured, and if we ran them both, we'd run the hooks twice.
This would be bad for e.g. the prepare-commit-msg hook. This is also the
least surprising behavior.

We check each hook for its exit status, even if the hook normally
ignores exit status, and if it fails, we abort executing further hooks.
This provides an easy way to reason about what the exit status is when a
hook fails; we need not consider how to handle multiple failing hooks.
It's also consistent among all hooks, whether they care about exit
status or not.

This series uses a test library to verify that we run the hooks for each
command instead of writing the same tests multiple times. If there are
other cases you'd like to see, please let me know.

I've talked with some people about this approach, and they've indicated
they would prefer a configuration-based approach. I've tried to make the
series such that it can be replaced with such an approach if that's the
decision we make. It should be easy enough to simply replace find_hooks
with an appropriate implementation and update the test framework.

brian m. carlson (5):
  run-command: add preliminary support for multiple hooks
  builtin/receive-pack: add support for multiple hooks
  sequencer: add support for multiple hooks
  builtin/worktree: add support for multiple post-checkout hooks
  transport: add support for multiple pre-push hooks

 builtin/am.c                       |  28 ++--
 builtin/commit.c                   |   5 +-
 builtin/receive-pack.c             | 212 +++++++++++++++++------------
 builtin/worktree.c                 |  40 ++++--
 run-command.c                      | 117 ++++++++++++----
 run-command.h                      |   9 +-
 sequencer.c                        |  96 ++++++++-----
 t/lib-hooks.sh                     | 156 +++++++++++++++++++++
 t/t5403-post-checkout-hook.sh      |   8 ++
 t/t5407-post-rewrite-hook.sh       |  15 ++
 t/t5516-fetch-push.sh              |  29 ++++
 t/t5571-pre-push-hook.sh           |  19 +++
 t/t7503-pre-commit-hook.sh         |  15 ++
 t/t7505-prepare-commit-msg-hook.sh |   9 ++
 transport.c                        |  98 +++++++------
 15 files changed, 636 insertions(+), 220 deletions(-)
 create mode 100644 t/lib-hooks.sh

[PATCH 1/5] 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: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/commit.c           |   5 +-
 run-command.c              | 141 +++++++++++++++++++++++++--------
 run-command.h              |   7 ++
 t/lib-hooks.sh             | 156 +++++++++++++++++++++++++++++++++++++
 t/t7503-pre-commit-hook.sh |  15 ++++
 5 files changed, 292 insertions(+), 32 deletions(-)
 create mode 100644 t/lib-hooks.sh

diff --git a/builtin/commit.c b/builtin/commit.c
index f17537474a..e7cf6b16ba 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -666,6 +666,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	struct strbuf sb = STRBUF_INIT;
 	const char *hook_arg1 = NULL;
 	const char *hook_arg2 = NULL;
+	struct string_list *hooks;
 	int clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);
 	int old_display_comment_prefix;
 
@@ -943,13 +944,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	if (!no_verify && find_hook("pre-commit")) {
+	hooks = find_hooks("pre-commit");
+	if (!no_verify && hooks) {
 		/*
 		 * 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
 		 * the editor and after we invoke run_status above.
 		 */
 		discard_cache();
+		free_hooks(hooks);
 	}
 	read_cache_from(index_file);
 
diff --git a/run-command.c b/run-command.c
index 3449db319b..669af5ebc7 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1308,58 +1308,137 @@ int async_with_fork(void)
 #endif
 }
 
+static int has_hook(struct strbuf *path, int strip)
+{
+	if (access(path->buf, X_OK) < 0) {
+		int err = errno;
+
+		if (strip) {
+#ifdef STRIP_EXTENSION
+			strbuf_addstr(path, STRIP_EXTENSION);
+			if (access(path->buf, X_OK) >= 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;
+	if (has_hook(&path, 1)) {
+		return path.buf;
+	}
+	return NULL;
+}
 
-#ifdef STRIP_EXTENSION
-		strbuf_addstr(&path, STRIP_EXTENSION);
-		if (access(path.buf, X_OK) >= 0)
-			return path.buf;
-		if (errno == EACCES)
-			err = errno;
-#endif
+struct string_list *find_hooks(const char *name)
+{
+	struct string_list *list = xmalloc(sizeof(*list));
+	struct strbuf path = STRBUF_INIT;
+	DIR *d;
+	struct dirent *de;
 
-		if (err == EACCES && advice_ignored_hook) {
-			static struct string_list advise_given = STRING_LIST_INIT_DUP;
+	string_list_init(list, 1);
 
-			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);
-			}
-		}
+	/*
+	 * 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)) {
+		string_list_append(list, path.buf);
+		return list;
+	}
+
+	strbuf_reset(&path);
+	strbuf_git_path(&path, "hooks/%s.d", name);
+	d = opendir(path.buf);
+	if (!d) {
+		string_list_clear(list, 0);
 		return NULL;
 	}
-	return path.buf;
+	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))
+			string_list_append(list, path.buf);
+	}
+	closedir(d);
+	strbuf_reset(&path);
+	if (!list->nr) {
+		free_hooks(list);
+		return NULL;
+	}
+
+	string_list_sort(list);
+	return list;
+}
+
+void free_hooks(struct string_list *hooks)
+{
+	string_list_clear(hooks, 0);
+	free(hooks);
 }
 
 int run_hook_ve(const char *const *env, const char *name, va_list args)
 {
-	struct child_process hook = CHILD_PROCESS_INIT;
+	struct string_list *hooks;
+	struct string_list arglist = STRING_LIST_INIT_NODUP;
 	const char *p;
+	struct string_list_item *q;
+	int ret = 0;
 
-	p = find_hook(name);
-	if (!p)
+	hooks = find_hooks(name);
+	if (!hooks)
 		return 0;
 
-	argv_array_push(&hook.args, p);
 	while ((p = va_arg(args, const char *)))
-		argv_array_push(&hook.args, p);
-	hook.env = env;
-	hook.no_stdin = 1;
-	hook.stdout_to_stderr = 1;
-	hook.trace2_hook_name = name;
+		string_list_append(&arglist, p);
 
-	return run_command(&hook);
+	for_each_string_list_item(q, hooks) {
+		struct child_process hook = CHILD_PROCESS_INIT;
+		struct string_list_item *arg;
+
+		argv_array_push(&hook.args, q->string);
+		for_each_string_list_item(arg, &arglist) {
+			argv_array_push(&hook.args, arg->string);
+		}
+
+		hook.env = env;
+		hook.no_stdin = 1;
+		hook.stdout_to_stderr = 1;
+		hook.trace2_hook_name = name;
+
+		ret = run_command(&hook);
+		if (ret)
+			break;
+	}
+	string_list_clear(&arglist, 0);
+	free_hooks(hooks);
+	return ret;
 }
 
 int run_hook_le(const char *const *env, const char *name, ...)
diff --git a/run-command.h b/run-command.h
index a6950691c0..7266dc2969 100644
--- a/run-command.h
+++ b/run-command.h
@@ -68,6 +68,13 @@ 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.
+ */
+struct string_list *find_hooks(const char *name);
+/* Frees the result of find_hooks. */
+void free_hooks(struct string_list *hooks);
 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..91b3acaba2
--- /dev/null
+++ b/t/lib-hooks.sh
@@ -0,0 +1,156 @@
+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 '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

[PATCH 2/5] 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 | 212 ++++++++++++++++++++++++-----------------
 t/t5516-fetch-push.sh  |  29 ++++++
 2 files changed, 152 insertions(+), 89 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 29f165d8bd..7454834d2a 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -680,72 +680,82 @@ 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 child_process proc = CHILD_PROCESS_INIT;
+	struct child_process proc;
+	struct string_list *hooks;
+	struct string_list_item *p;
 	struct async muxer;
 	const char *argv[2];
-	int code;
+	int code = 0;
 
-	argv[0] = find_hook(hook_name);
-	if (!argv[0])
+	hooks = find_hooks(hook_name);
+	if (!hooks)
 		return 0;
 
-	argv[1] = NULL;
+	for_each_string_list_item(p, hooks) {
+		argv[0] = p->string;
+		argv[1] = NULL;
 
-	proc.argv = argv;
-	proc.in = -1;
-	proc.stdout_to_stderr = 1;
-	proc.trace2_hook_name = hook_name;
+		child_process_init(&proc);
+		proc.argv = argv;
+		proc.in = -1;
+		proc.stdout_to_stderr = 1;
+		proc.trace2_hook_name = hook_name;
 
-	if (feed_state->push_options) {
-		int i;
-		for (i = 0; i < feed_state->push_options->nr; i++)
-			argv_array_pushf(&proc.env_array,
-				"GIT_PUSH_OPTION_%d=%s", i,
-				feed_state->push_options->items[i].string);
-		argv_array_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT=%d",
-				 feed_state->push_options->nr);
-	} else
-		argv_array_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT");
+		if (feed_state->push_options) {
+			int i;
+			for (i = 0; i < feed_state->push_options->nr; i++)
+				argv_array_pushf(&proc.env_array,
+					"GIT_PUSH_OPTION_%d=%s", i,
+					feed_state->push_options->items[i].string);
+			argv_array_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT=%d",
+					 feed_state->push_options->nr);
+		} else
+			argv_array_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT");
 
-	if (tmp_objdir)
-		argv_array_pushv(&proc.env_array, tmp_objdir_env(tmp_objdir));
+		if (tmp_objdir)
+			argv_array_pushv(&proc.env_array, tmp_objdir_env(tmp_objdir));
 
-	if (use_sideband) {
-		memset(&muxer, 0, sizeof(muxer));
-		muxer.proc = copy_to_sideband;
-		muxer.in = -1;
-		code = start_async(&muxer);
-		if (code)
-			return code;
-		proc.err = muxer.in;
-	}
+		if (use_sideband) {
+			memset(&muxer, 0, sizeof(muxer));
+			muxer.proc = copy_to_sideband;
+			muxer.in = -1;
+			code = start_async(&muxer);
+			if (code)
+				break;
+			proc.err = muxer.in;
+		}
 
-	prepare_push_cert_sha1(&proc);
+		prepare_push_cert_sha1(&proc);
 
-	code = start_command(&proc);
-	if (code) {
+		code = start_command(&proc);
+		if (code) {
+			if (use_sideband)
+				finish_async(&muxer);
+			break;
+		}
+
+		sigchain_push(SIGPIPE, SIG_IGN);
+
+		while (1) {
+			const char *buf;
+			size_t n;
+			if (feed(feed_state, &buf, &n))
+				break;
+			if (write_in_full(proc.in, buf, n) < 0)
+				break;
+		}
+		close(proc.in);
 		if (use_sideband)
 			finish_async(&muxer);
-		return code;
-	}
 
-	sigchain_push(SIGPIPE, SIG_IGN);
+		sigchain_pop(SIGPIPE);
 
-	while (1) {
-		const char *buf;
-		size_t n;
-		if (feed(feed_state, &buf, &n))
-			break;
-		if (write_in_full(proc.in, buf, n) < 0)
+		code = finish_command(&proc);
+		if (code)
 			break;
 	}
-	close(proc.in);
-	if (use_sideband)
-		finish_async(&muxer);
-
-	sigchain_pop(SIGPIPE);
-
-	return finish_command(&proc);
+        free_hooks(hooks);
+	return code;
 }
 
 static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)
@@ -793,30 +803,41 @@ static int run_receive_hook(struct command *commands,
 static int run_update_hook(struct command *cmd)
 {
 	const char *argv[5];
-	struct child_process proc = CHILD_PROCESS_INIT;
+	struct child_process proc;
+	struct string_list *hooks;
+	struct string_list_item *p;
 	int code;
 
-	argv[0] = find_hook("update");
-	if (!argv[0])
+	hooks = find_hooks("update");
+	if (!hooks)
 		return 0;
 
-	argv[1] = cmd->ref_name;
-	argv[2] = oid_to_hex(&cmd->old_oid);
-	argv[3] = oid_to_hex(&cmd->new_oid);
-	argv[4] = NULL;
+	for_each_string_list_item(p, hooks) {
+		child_process_init(&proc);
 
-	proc.no_stdin = 1;
-	proc.stdout_to_stderr = 1;
-	proc.err = use_sideband ? -1 : 0;
-	proc.argv = argv;
-	proc.trace2_hook_name = "update";
+		argv[0] = p->string;
+		argv[1] = cmd->ref_name;
+		argv[2] = oid_to_hex(&cmd->old_oid);
+		argv[3] = oid_to_hex(&cmd->new_oid);
+		argv[4] = NULL;
 
-	code = start_command(&proc);
-	if (code)
-		return code;
-	if (use_sideband)
-		copy_to_sideband(proc.err, -1, NULL);
-	return finish_command(&proc);
+		proc.no_stdin = 1;
+		proc.stdout_to_stderr = 1;
+		proc.err = use_sideband ? -1 : 0;
+		proc.argv = argv;
+		proc.trace2_hook_name = "update";
+
+		code = start_command(&proc);
+		if (code)
+			return code;
+		if (use_sideband)
+			copy_to_sideband(proc.err, -1, NULL);
+		code = finish_command(&proc);
+		if (code)
+			break;
+	}
+        free_hooks(hooks);
+	return code;
 }
 
 static int is_ref_checked_out(const char *ref)
@@ -1005,16 +1026,20 @@ static const char *update_worktree(unsigned char *sha1)
 	const char *retval;
 	const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : "..";
 	struct argv_array env = ARGV_ARRAY_INIT;
+	struct string_list *hooks;
 
 	if (is_bare_repository())
 		return "denyCurrentBranch = updateInstead needs a worktree";
 
 	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
 
-	if (!find_hook(push_to_checkout_hook))
+	hooks = find_hooks(push_to_checkout_hook);
+	if (!hooks)
 		retval = push_to_deploy(sha1, &env, work_tree);
-	else
+	else {
+		free_hooks(hooks);
 		retval = push_to_checkout(sha1, &env, work_tree);
+	}
 
 	argv_array_clear(&env);
 	return retval;
@@ -1173,33 +1198,42 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 static void run_update_post_hook(struct command *commands)
 {
 	struct command *cmd;
-	struct child_process proc = CHILD_PROCESS_INIT;
-	const char *hook;
+	struct child_process proc;
+	struct string_list *hooks;
+	struct string_list_item *p;
 
-	hook = find_hook("post-update");
-	if (!hook)
+	hooks = find_hooks("post-update");
+	if (!hooks)
 		return;
 
-	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (cmd->error_string || cmd->did_not_exist)
-			continue;
+	for_each_string_list_item(p, hooks) {
+		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, p->string);
+			argv_array_push(&proc.args, cmd->ref_name);
+		}
 		if (!proc.args.argc)
-			argv_array_push(&proc.args, hook);
-		argv_array_push(&proc.args, cmd->ref_name);
-	}
-	if (!proc.args.argc)
-		return;
+			return;
 
-	proc.no_stdin = 1;
-	proc.stdout_to_stderr = 1;
-	proc.err = use_sideband ? -1 : 0;
-	proc.trace2_hook_name = "post-update";
+		proc.no_stdin = 1;
+		proc.stdout_to_stderr = 1;
+		proc.err = use_sideband ? -1 : 0;
+		proc.trace2_hook_name = "post-update";
 
-	if (!start_command(&proc)) {
-		if (use_sideband)
-			copy_to_sideband(proc.err, -1, NULL);
-		finish_command(&proc);
+		if (!start_command(&proc)) {
+			int ret;
+			if (use_sideband)
+				copy_to_sideband(proc.err, -1, NULL);
+			ret = finish_command(&proc);
+			if (ret)
+				break;
+		}
 	}
+        free_hooks(hooks);
 }
 
 static void check_aliased_update_internal(struct command *cmd,
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 49bf4280e8..3143422344 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)
 
@@ -1700,4 +1701,32 @@ test_expect_success 'updateInstead with push-to-checkout hook' '
 	)
 '
 
+test_expect_success 'setup' '
+	mk_test_with_hooks hooktest heads/master
+'
+
+cmd_receive () {
+	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 3/5] 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.

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

diff --git a/builtin/am.c b/builtin/am.c
index 4fb107a9d1..303abc98c2 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -442,22 +442,32 @@ 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");
+	struct child_process proc;
+	struct string_list *hooks;
+	struct string_list_item *p;
 	int ret;
 
-	if (!hook)
+	hooks = find_hooks("post-rewrite");
+	if (!hooks)
 		return 0;
 
-	argv_array_push(&cp.args, hook);
-	argv_array_push(&cp.args, "rebase");
+	for_each_string_list_item(p, hooks) {
+		child_process_init(&proc);
 
-	cp.in = xopen(am_path(state, "rewritten"), O_RDONLY);
-	cp.stdout_to_stderr = 1;
-	cp.trace2_hook_name = "post-rewrite";
+		argv_array_push(&cp.args, p->string);
+		argv_array_push(&cp.args, "rebase");
 
-	ret = run_command(&cp);
+		cp.in = xopen(am_path(state, "rewritten"), O_RDONLY);
+		cp.stdout_to_stderr = 1;
+		cp.trace2_hook_name = "post-rewrite";
 
-	close(cp.in);
+		ret = run_command(&cp);
+
+		close(cp.in);
+		if (ret)
+			break;
+	}
+	free_hooks(hooks);
 	return ret;
 }
 
diff --git a/sequencer.c b/sequencer.c
index 79a046d748..3a616ceba6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1088,33 +1088,69 @@ int update_head_with_reflog(const struct commit *old_head,
 static int run_rewrite_hook(const struct object_id *oldoid,
 			    const struct object_id *newoid)
 {
-	struct child_process proc = CHILD_PROCESS_INIT;
+	struct child_process proc;
+	struct string_list *hooks;
+	struct string_list_item *p;
 	const char *argv[3];
 	int code;
 	struct strbuf sb = STRBUF_INIT;
 
-	argv[0] = find_hook("post-rewrite");
-	if (!argv[0])
+	hooks = find_hooks("post-rewrite");
+	if (!hooks)
 		return 0;
 
-	argv[1] = "amend";
-	argv[2] = NULL;
+	for_each_string_list_item(p, hooks) {
+		child_process_init(&proc);
 
-	proc.argv = argv;
-	proc.in = -1;
-	proc.stdout_to_stderr = 1;
-	proc.trace2_hook_name = "post-rewrite";
+		argv[0] = p->string;
+		argv[1] = "amend";
+		argv[2] = NULL;
 
-	code = start_command(&proc);
-	if (code)
-		return code;
-	strbuf_addf(&sb, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid));
-	sigchain_push(SIGPIPE, SIG_IGN);
-	write_in_full(proc.in, sb.buf, sb.len);
-	close(proc.in);
-	strbuf_release(&sb);
-	sigchain_pop(SIGPIPE);
-	return finish_command(&proc);
+		proc.argv = argv;
+		proc.in = -1;
+		proc.stdout_to_stderr = 1;
+		proc.trace2_hook_name = "post-rewrite";
+
+		code = start_command(&proc);
+		if (code)
+			return code;
+		strbuf_addf(&sb, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid));
+		sigchain_push(SIGPIPE, SIG_IGN);
+		write_in_full(proc.in, sb.buf, sb.len);
+		close(proc.in);
+		strbuf_release(&sb);
+		sigchain_pop(SIGPIPE);
+		code = finish_command(&proc);
+		if (code)
+			break;
+	}
+	free_hooks(hooks);
+	return code;
+}
+
+static void run_interactive_rewrite_hook(void)
+{
+	struct string_list *hooks;
+	struct string_list_item *p;
+	struct child_process child;
+
+	hooks = find_hooks("post-rewrite");
+	if (!hooks)
+		return;
+
+	for_each_string_list_item(p, hooks) {
+		child_process_init(&child);
+
+		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, p->string);
+		argv_array_push(&child.args, "rebase");
+		if (run_command(&child))
+			break;
+	}
+	free_hooks(hooks);
 }
 
 void commit_post_rewrite(struct repository *r,
@@ -1326,6 +1362,7 @@ static int try_to_commit(struct repository *r,
 	char *amend_author = NULL;
 	const char *hook_commit = NULL;
 	enum commit_msg_cleanup_mode cleanup;
+	struct string_list *hooks;
 	int res = 0;
 
 	if (parse_head(r, &current_head))
@@ -1369,7 +1406,10 @@ static int try_to_commit(struct repository *r,
 		goto out;
 	}
 
-	if (find_hook("prepare-commit-msg")) {
+	hooks = find_hooks("prepare-commit-msg");
+	if (hooks) {
+		free_hooks(hooks);
+
 		res = run_prepare_commit_msg_hook(r, msg, hook_commit);
 		if (res)
 			goto out;
@@ -3771,8 +3811,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;
@@ -3782,18 +3820,8 @@ 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 */
+			run_interactive_rewrite_hook();
 		}
 		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..5b83f037b5 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
 '
 
+commit_command () {
+	echo "$1" >>file &&
+	git add file &&
+	git commit -m "$1"
+}
+
+test_multiple_hooks prepare-commit-msg commit_command
+
 test_done

[PATCH 4/5] 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            | 40 ++++++++++++++++++++++-------------
 t/t5403-post-checkout-hook.sh |  8 +++++++
 2 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index d2a7e2f3f1..3126fa3a82 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -395,21 +395,31 @@ 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 string_list *hooks;
+		hooks = find_hooks("post-checkout");
+		if (hooks) {
+			struct string_list_item *p;
+			struct child_process cp;
+			for_each_string_list_item(p, hooks) {
+				const char *env[] = { "GIT_DIR", "GIT_WORK_TREE", NULL };
+				child_process_init(&cp);
+
+				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(p->string),
+						 oid_to_hex(&null_oid),
+						 oid_to_hex(&commit->object.oid),
+						 "1", NULL);
+				ret = run_command(&cp);
+				if (ret)
+					break;
+			}
+			free_hooks(hooks);
 		}
 	}
 
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 5/5] 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              | 98 ++++++++++++++++++++++------------------
 4 files changed, 74 insertions(+), 61 deletions(-)

diff --git a/run-command.c b/run-command.c
index 669af5ebc7..2c387a7aef 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1340,18 +1340,6 @@ static int has_hook(struct strbuf *path, int strip)
 	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)) {
-		return path.buf;
-	}
-	return NULL;
-}
-
 struct string_list *find_hooks(const char *name)
 {
 	struct string_list *list = xmalloc(sizeof(*list));
diff --git a/run-command.h b/run-command.h
index 7266dc2969..a657c09a6c 100644
--- a/run-command.h
+++ b/run-command.h
@@ -62,12 +62,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 d0608df5c9..a2a9437c5b 100644
--- a/transport.c
+++ b/transport.c
@@ -1049,61 +1049,73 @@ static int run_pre_push_hook(struct transport *transport,
 {
 	int ret = 0, x;
 	struct ref *r;
-	struct child_process proc = CHILD_PROCESS_INIT;
+	struct child_process proc;
+	struct string_list *hooks;
+	struct string_list_item *p;
 	struct strbuf buf;
 	const char *argv[4];
 
-	if (!(argv[0] = find_hook("pre-push")))
+	hooks = find_hooks("pre-push");
+	if (!hooks)
 		return 0;
 
-	argv[1] = transport->remote->name;
-	argv[2] = transport->url;
-	argv[3] = NULL;
 
-	proc.argv = argv;
-	proc.in = -1;
-	proc.trace2_hook_name = "pre-push";
+	for_each_string_list_item(p, hooks) {
+		child_process_init(&proc);
 
-	if (start_command(&proc)) {
-		finish_command(&proc);
-		return -1;
-	}
+		argv[0] = p->string;
+		argv[1] = transport->remote->name;
+		argv[2] = transport->url;
+		argv[3] = NULL;
 
-	sigchain_push(SIGPIPE, SIG_IGN);
+		proc.argv = argv;
+		proc.in = -1;
+		proc.trace2_hook_name = "pre-push";
 
-	strbuf_init(&buf, 256);
-
-	for (r = 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;
-		if (r->status == REF_STATUS_UPTODATE) continue;
-
-		strbuf_reset(&buf);
-		strbuf_addf( &buf, "%s %s %s %s\n",
-			 r->peer_ref->name, oid_to_hex(&r->new_oid),
-			 r->name, oid_to_hex(&r->old_oid));
-
-		if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
-			/* We do not mind if a hook does not read all refs. */
-			if (errno != EPIPE)
-				ret = -1;
+		if (start_command(&proc)) {
+			finish_command(&proc);
+			ret = -1;
 			break;
 		}
+
+		sigchain_push(SIGPIPE, SIG_IGN);
+
+		strbuf_init(&buf, 256);
+
+		for (r = 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;
+			if (r->status == REF_STATUS_UPTODATE) continue;
+
+			strbuf_reset(&buf);
+			strbuf_addf( &buf, "%s %s %s %s\n",
+				 r->peer_ref->name, oid_to_hex(&r->new_oid),
+				 r->name, oid_to_hex(&r->old_oid));
+
+			if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
+				/* We do not mind if a hook does not read all refs. */
+				if (errno != EPIPE)
+					ret = -1;
+				break;
+			}
+		}
+
+		strbuf_release(&buf);
+
+		x = close(proc.in);
+		if (!ret)
+			ret = x;
+
+		sigchain_pop(SIGPIPE);
+
+		x = finish_command(&proc);
+		if (!ret)
+			ret = x;
+		if (ret)
+			break;
 	}
-
-	strbuf_release(&buf);
-
-	x = close(proc.in);
-	if (!ret)
-		ret = x;
-
-	sigchain_pop(SIGPIPE);
-
-	x = finish_command(&proc);
-	if (!ret)
-		ret = x;
-
+	free_hooks(hooks);
 	return ret;
 }
 

Re: [PATCH 0/5] Multiple hook support

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> To preserve backwards compatibility, we don't run the hooks in the ".d"
> directory if the single file is a valid hook (i.e. it exists and is
> executable). This is because some people already have multiple hook
> scripts configured, and if we ran them both, we'd run the hooks twice.
> This would be bad for e.g. the prepare-commit-msg hook. This is also the
> least surprising behavior.

OK.  An obvious alternative may be to see if the expected hooks path
is a directory and use the contents.  If ".git/hooks/pre-commit" is
a single file, we know it is the single hook as before, and if it is
a directory, we know that is not a custom made (i.e. from the world
before this series supported in the core-git) multi-hook setup.

> We check each hook for its exit status, even if the hook normally
> ignores exit status, and if it fails, we abort executing further hooks.

This part may become the most controversial in the whole topic, but
a design discussion is helped by having a concrete proposal that
makes its own design decision, and this is the simplest design of
the failure case that is the easiest to understand.

Thanks.  Let's see how the review discussion goes ;-)

> brian m. carlson (5):
>   run-command: add preliminary support for multiple hooks
>   builtin/receive-pack: add support for multiple hooks
>   sequencer: add support for multiple hooks
>   builtin/worktree: add support for multiple post-checkout hooks
>   transport: add support for multiple pre-push hooks
>
>  builtin/am.c                       |  28 ++--
>  builtin/commit.c                   |   5 +-
>  builtin/receive-pack.c             | 212 +++++++++++++++++------------
>  builtin/worktree.c                 |  40 ++++--
>  run-command.c                      | 117 ++++++++++++----
>  run-command.h                      |   9 +-
>  sequencer.c                        |  96 ++++++++-----
>  t/lib-hooks.sh                     | 156 +++++++++++++++++++++
>  t/t5403-post-checkout-hook.sh      |   8 ++
>  t/t5407-post-rewrite-hook.sh       |  15 ++
>  t/t5516-fetch-push.sh              |  29 ++++
>  t/t5571-pre-push-hook.sh           |  19 +++
>  t/t7503-pre-commit-hook.sh         |  15 ++
>  t/t7505-prepare-commit-msg-hook.sh |   9 ++
>  transport.c                        |  98 +++++++------
>  15 files changed, 636 insertions(+), 220 deletions(-)
>  create mode 100644 t/lib-hooks.sh

Re: [PATCH 0/5] Multiple hook support

[brian m. carlson]

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

On Wed, Apr 24, 2019 at 11:09:10AM +0900, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > To preserve backwards compatibility, we don't run the hooks in the ".d"
> > directory if the single file is a valid hook (i.e. it exists and is
> > executable). This is because some people already have multiple hook
> > scripts configured, and if we ran them both, we'd run the hooks twice.
> > This would be bad for e.g. the prepare-commit-msg hook. This is also the
> > least surprising behavior.
> 
> OK.  An obvious alternative may be to see if the expected hooks path
> is a directory and use the contents.  If ".git/hooks/pre-commit" is
> a single file, we know it is the single hook as before, and if it is
> a directory, we know that is not a custom made (i.e. from the world
> before this series supported in the core-git) multi-hook setup.

That's an idea I hadn't considered. I'm interested to hear other folks'
ideas on it, but that certainly avoids a lot of the problems in my
approach.

> > We check each hook for its exit status, even if the hook normally
> > ignores exit status, and if it fails, we abort executing further hooks.
> 
> This part may become the most controversial in the whole topic, but
> a design discussion is helped by having a concrete proposal that
> makes its own design decision, and this is the simplest design of
> the failure case that is the easiest to understand.

I expected it might be. I'm not strongly wedded to it, so if people make
a good argument for something else, I'm okay with changing it.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

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

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

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> diff --git a/builtin/commit.c b/builtin/commit.c
> index f17537474a..e7cf6b16ba 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -666,6 +666,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  	struct strbuf sb = STRBUF_INIT;
>  	const char *hook_arg1 = NULL;
>  	const char *hook_arg2 = NULL;
> +	struct string_list *hooks;
>  	int clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);
>  	int old_display_comment_prefix;
>  
> @@ -943,13 +944,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		return 0;
>  	}
>  
> -	if (!no_verify && find_hook("pre-commit")) {
> +	hooks = find_hooks("pre-commit");
> +	if (!no_verify && hooks) {
>  		/*
>  		 * 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
>  		 * the editor and after we invoke run_status above.
>  		 */
>  		discard_cache();
> +		free_hooks(hooks);
>  	}
>  	read_cache_from(index_file);

OK, so find_hook() that used to return a single hook now can return
a list of hook scripts.  Running the single one becomes a simple
special case of "run each of them in turn, and stop at the first
failure".

> diff --git a/run-command.c b/run-command.c
> index 3449db319b..669af5ebc7 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1308,58 +1308,137 @@ int async_with_fork(void)
>  #endif
>  }
>  
> +static int has_hook(struct strbuf *path, int strip)
> +{
> +	if (access(path->buf, X_OK) < 0) {

Does ".git/post-commit" that is not an executable exist?

It was perfectly fine for find_hook() to say "there is no hook for
post-commit" in the old world in such a case, because the
unexecutable file it found is not going to be run anyway.

But it is not clear if has_hook(), that affects "there is no single
hook file for post-commit, so let's look at post-commit.d" decision
made by find_hooks(), should behave that way.  It somehow feels more
intuitive if a post-commit file that is not executable, by merely
existing, stops post-commit.d directory from being scanned, at least
to me.

>  int run_hook_ve(const char *const *env, const char *name, va_list args)
>  {
> -	struct child_process hook = CHILD_PROCESS_INIT;
> +	struct string_list *hooks;
> +	struct string_list arglist = STRING_LIST_INIT_NODUP;
>  	const char *p;
> +	struct string_list_item *q;
> +	int ret = 0;
>   ...
> +		hook.env = env;
> +		hook.no_stdin = 1;
> +		hook.stdout_to_stderr = 1;
> +		hook.trace2_hook_name = name;
> +
> +		ret = run_command(&hook);
> +		if (ret)
> +			break;
> +	}
> +	string_list_clear(&arglist, 0);
> +	free_hooks(hooks);
> +	return ret;
>  }

These "run with command line arguments as its sole input, with the
exit status as its sole output" style hooks are easily handled and
the above looks like reasonable enhancement to the existing
abstraction (e.g. run 'prepare-commit-msg' hook with these
arguments).

I however wonder how the hooks in the other style should/can be
handled, that are fed data from their standard input stream, and
returns more than one bit via their standard output stream.  In
any case, they are not in the scope of this step.

Re: [PATCH 0/5] Multiple hook support

[Jonathan Nieder]

Hi,

brian m. carlson wrote:

> I've talked with some people about this approach, and they've indicated
> they would prefer a configuration-based approach.

I would, too, mostly because that reduces the problem of securing
hooks to securing configuration.  See
https://public-inbox.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/
for more on this subject.

More precisely, a few problems with the current hooks system:

 1. There's not a standard way to have multiple hooks for a single event.
    That's what this series is about.

    (The recommended workaround has been to use a trampoline script as
    your hook, and to make that trampoline script implement whatever
    policy for the order of invocation and accumulation of results is
    appropriate, but that's a bit of a cop-out.)

 2. Because they are stored in the Git repository, they do not have a
    way to be automatically updated.

    (The recommended workaround is to use a trampoline script as your
    hook and put the actual hook somewhere standard like $PATH where
    it can be upgraded system-wide.  But that's a bit of a cop-out.)

 3. Because they are part of the Git repository, it is very easy to
    compromise a user's account by tricking them into running an
    attacker-authored hook.  Attacks include "hey admin, can you tell
    me why 'git commit' is failing in this repo?" and "here's a zip file
    containing a Git repository with our fancy software.  Feel free
    to look around, run 'git pull', etc".

    Similar attacks, probably even worse, apply to shell prompt scripts
    using commands from attacker-controlled .git/config.

    (The recommended workaround is to inspect .git/config and
    .git/hooks whenever you're looking at an untrusted repository, and
    to write your shell prompt script defensively.)

Solving (1) without (2) feels like a bit of a missed opportunity to
me.  Ideally, what I would like is

   i. A central registry of trustworthy Git hooks that can be upgraded
      using the system package manager to address (2).  Perhaps just
      git-hook-* commands on the $PATH.

  ii. Instead of putting hooks in .git/hooks, put a list of hooks to
      run for each event in .git/config.

 iii. For backward compatibility, perform a multi-stage migration.
      In the stage I am most interested in:

      When encountering a hook in .git/hooks, don't run it, but print
      a message about how to migrate it to the modern scheme.

      To make migration to the modern scheme painless, stick a
      standard trampoline script in .git/hooks in all converted and
      all newly "git init"ed repositories to allow old versions of Git
      to respect the configuration from (i) and (ii).

That doesn't handle core.pager et al, but those we can handle
separately (for example by, at least optionally, not respecting values
for them in per-repo config at all).

Thanks for tackling this.  What do you think?

Thanks,
Jonathan

Re: [PATCH 0/5] Multiple hook support

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On Wed, Apr 24, 2019 at 11:09:10AM +0900, Junio C Hamano wrote:
>> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>> 
>> > To preserve backwards compatibility, we don't run the hooks in the ".d"
>> > directory if the single file is a valid hook (i.e. it exists and is
>> > executable). This is because some people already have multiple hook
>> > scripts configured, and if we ran them both, we'd run the hooks twice.
>> > This would be bad for e.g. the prepare-commit-msg hook. This is also the
>> > least surprising behavior.
>> 
>> OK.  An obvious alternative may be to see if the expected hooks path
>> is a directory and use the contents.  If ".git/hooks/pre-commit" is
>> a single file, we know it is the single hook as before, and if it is
>> a directory, we know that is not a custom made (i.e. from the world
>> before this series supported in the core-git) multi-hook setup.
>
> That's an idea I hadn't considered. I'm interested to hear other folks'
> ideas on it, but that certainly avoids a lot of the problems in my
> approach.

One downside is that the transition from the old to the new world
order becomes a bigger deal.  You could have been using a precursor
of this series implemented entirely as a shell script

	$ cat >.git/pre-commit <<\EOF
	#!/bin/sh
	for x in .git/pre-commit.d/*
	do
		"$x" "$@" || exit 1
	done
	exit 0
	EOF
	$ chmod +x .git/pre-commit

and with your approach, the user can keep using that set-up and then
simply remove that single file once all the Git executables the user
uses are natively aware of the multi-hook setup.  With "either a
file or directory" approach, the transition would be removing the
file *and* renaming pre-commit.d directory to pre-commit.

Re: [PATCH 0/5] Multiple hook support

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 24 2019, Jonathan Nieder wrote:

> brian m. carlson wrote:

brian: I'm very interested in this. I barked up this tree before almost
exactly 3 years ago:

    https://public-inbox.org/git/CACBZZX6j6q2DUN_Z-Pnent1u714dVNPFBrL_PiEQyLmCzLUVxg@mail.gmail.com/
    https://public-inbox.org/git/1461367997-28745-1-git-send-email-avarab@gmail.com/

If you haven't seen those threads you might find them interesting. In
particular there's a previous discussion about the "exit on first fail"
v.s. "run them all" semantics. I'll elaborate elsewhere in this thread.

The only bit that landed from that was 867ad08a26 ("hooks: allow
customizing where the hook directory is", 2016-05-04), which, in reply
to JN below...:

>> I've talked with some people about this approach, and they've indicated
>> they would prefer a configuration-based approach.
>
> I would, too, mostly because that reduces the problem of securing
> hooks to securing configuration.  See
> https://public-inbox.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/
> for more on this subject.
>
> More precisely, a few problems with the current hooks system:
>
>  1. There's not a standard way to have multiple hooks for a single event.
>     That's what this series is about.
>
>     (The recommended workaround has been to use a trampoline script as
>     your hook, and to make that trampoline script implement whatever
>     policy for the order of invocation and accumulation of results is
>     appropriate, but that's a bit of a cop-out.)
>
>  2. Because they are stored in the Git repository, they do not have a
>     way to be automatically updated.
>
>     (The recommended workaround is to use a trampoline script as your
>     hook and put the actual hook somewhere standard like $PATH where
>     it can be upgraded system-wide.  But that's a bit of a cop-out.)

You can accomplish this with core.hooksPath, and presumably a
combination of core.hooksPath and brian's patches here. That was my
two-step plan in 2016, but I obviously never got to step #2.

So in /etc/gitconfig on your server you set core.hooksPath=/etc/githooks
and then your pre-receive hook will be /etc/githooks/pre-receive, or
/etc/githooks/pre-receive.d/*.

>  3. Because they are part of the Git repository, it is very easy to
>     compromise a user's account by tricking them into running an
>     attacker-authored hook.  Attacks include "hey admin, can you tell
>     me why 'git commit' is failing in this repo?" and "here's a zip file
>     containing a Git repository with our fancy software.  Feel free
>     to look around, run 'git pull', etc".
>
>     Similar attacks, probably even worse, apply to shell prompt scripts
>     using commands from attacker-controlled .git/config.
>
>     (The recommended workaround is to inspect .git/config and
>     .git/hooks whenever you're looking at an untrusted repository, and
>     to write your shell prompt script defensively.)
>
> Solving (1) without (2) feels like a bit of a missed opportunity to
> me.  Ideally, what I would like is
>
>    i. A central registry of trustworthy Git hooks that can be upgraded
>       using the system package manager to address (2).  Perhaps just
>       git-hook-* commands on the $PATH.
>
>   ii. Instead of putting hooks in .git/hooks, put a list of hooks to
>       run for each event in .git/config.
>
>  iii. For backward compatibility, perform a multi-stage migration.
>       In the stage I am most interested in:
>
>       When encountering a hook in .git/hooks, don't run it, but print
>       a message about how to migrate it to the modern scheme.
>
>       To make migration to the modern scheme painless, stick a
>       standard trampoline script in .git/hooks in all converted and
>       all newly "git init"ed repositories to allow old versions of Git
>       to respect the configuration from (i) and (ii).
>
> That doesn't handle core.pager et al, but those we can handle
> separately (for example by, at least optionally, not respecting values
> for them in per-repo config at all).
>
> Thanks for tackling this.  What do you think?
>
> Thanks,
> Jonathan

Re: [PATCH 0/5] Multiple hook support

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 24 2019, brian m. carlson wrote:

> Oftentimes, people want to use multiple of the same kind of hook. This
> may be because software or a project they use requires a given hook, but
> they would also like to have a custom hook, or because they're using
> multiple pieces of software that both require involvement with the same
> hook.
>
> This series introduces support for multiple hooks by using a ".d"
> directory. For example, instead of using a single
> ".git/hooks/post-checkout" file, you'd use multiple files within
> ".git/hooks/post-checkout.d". This is the standard Unix paradigm for
> multiple files and should be familiar to most people. Hooks are executed
> in sorted order.

I think it's interesting if people can chime in with current in-the-wild
implementations of this.

I know GitLab's the best, not because I was in any way involved in it,
I've just dealt with writing hooks for it:
https://docs.gitlab.com/ee/administration/custom_hooks.html#chained-hooks-support

There:

 1. The instance has a 'hooks' dir in the .git repo that's a symlink to
    /my/global/hooks. They could also use a /etc/gitconfig
    core.hooksPath for this part, but whatever.

 2. That /my/global/hooks has e.g. a /my/global/hooks/pre-receive that
    git itself runs, which is a trampoline script that runs all over the
    place and executes global/per-project hooks (which live in
    .git/custom_hooks/).

 3. "The hooks of the same type are executed in order and execution
     stops on the first script exiting with a non-zero value."

I wonder if the eventual goal of this facility would be to get such
users on board with using git's native feature for this. This series is
most of the way there for GitLab's case, but not quite.

> To preserve backwards compatibility, we don't run the hooks in the ".d"
> directory if the single file is a valid hook (i.e. it exists and is
> executable). This is because some people already have multiple hook
> scripts configured, and if we ran them both, we'd run the hooks twice.
> This would be bad for e.g. the prepare-commit-msg hook. This is also the
> least surprising behavior.
>
> We check each hook for its exit status, even if the hook normally
> ignores exit status, and if it fails, we abort executing further hooks.
> This provides an easy way to reason about what the exit status is when a
> hook fails; we need not consider how to handle multiple failing hooks.
> It's also consistent among all hooks, whether they care about exit
> status or not.

Others have replied to this already. I linked to the 2016 discussion of
my RFC for this in
https://public-inbox.org/git/87wojjsv9p.fsf@evledraar.gmail.com where I
made the same choice. Some points on that:

 * There was the mention of "but what if someone wants to run them all",
   e.g. for logging to N logging systems where one fails, that's been
   brought up again this time around by others.

 * The case I find more interesting is the ability to run the hooks in
   parallel. Saying "glob order and exit on first fail" categorically
   closes the door to that.

> I've talked with some people about this approach, and they've indicated
> they would prefer a configuration-based approach. I've tried to make the
> series such that it can be replaced with such an approach if that's the
> decision we make. It should be easy enough to simply replace find_hooks
> with an appropriate implementation and update the test framework.

I think whatever opinions we all have on the current implementation it's
OK to get this in in *some* form and just made it configurable or
whatever later.

Most of the work is the ability to run N hooks, how exactly that happens
can be tweaked later, and if this series lands and someone isn't 100%
happy with the semantics they're no worse off than they are now.

I.e. we could get something like this in its current form, and later have:

    core.hooksOrder = glob | random
    core.hooksHaltOnError = true | never

Where we'd say that what this series does is core.hooksOrder=glob and
core.hooksHaltOnError=true, but that e.g. parallel "run them all" could
be done in the future with core.hooksOrder=random &
core.hooksHaltOnError=never

Re: [PATCH 0/5] Multiple hook support

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 24 2019, brian m. carlson wrote:

> On Wed, Apr 24, 2019 at 11:09:10AM +0900, Junio C Hamano wrote:
>> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>>
>> > To preserve backwards compatibility, we don't run the hooks in the ".d"
>> > directory if the single file is a valid hook (i.e. it exists and is
>> > executable). This is because some people already have multiple hook
>> > scripts configured, and if we ran them both, we'd run the hooks twice.
>> > This would be bad for e.g. the prepare-commit-msg hook. This is also the
>> > least surprising behavior.
>>
>> OK.  An obvious alternative may be to see if the expected hooks path
>> is a directory and use the contents.  If ".git/hooks/pre-commit" is
>> a single file, we know it is the single hook as before, and if it is
>> a directory, we know that is not a custom made (i.e. from the world
>> before this series supported in the core-git) multi-hook setup.
>
> That's an idea I hadn't considered. I'm interested to hear other folks'
> ideas on it, but that certainly avoids a lot of the problems in my
> approach.

Two things on that:

 1. As noted upthread we have some in-the-wild users who use the *.d
    semantics already. Would be nice to be able to migrate them, also
    *.d is a commonly-used pattern in other software.

 2. It's more of a pain in some configuration management systems like
    e.g. puppet to change a managed file to a directory than removing an
    existing file and adding a directory.

Re: [PATCH 0/5] Multiple hook support

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 24 2019, Jonathan Nieder wrote:

> Hi,
>
> brian m. carlson wrote:
>
>> I've talked with some people about this approach, and they've indicated
>> they would prefer a configuration-based approach.
>
> I would, too, mostly because that reduces the problem of securing
> hooks to securing configuration.  See
> https://public-inbox.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/
> for more on this subject.

I hadn't noticed this E-Mail when I later wrote similar musings on the
subject:

https://public-inbox.org/git/87zi6eakkt.fsf@evledraar.gmail.com/
https://public-inbox.org/git/874lkq11ug.fsf@evledraar.gmail.com/

I.e. what I wanted out of it was an in-repository .gitconfig instead of
inspecting some some random repo with a .git/config.

But the problem to be solved is the same: The ability to carefully poke
a config file with a stick at a distance, and carefully whitelist which
parts of it (if any) you want to trust.

Re: [PATCH 0/5] Multiple hook support

[Duy Nguyen]

On Wed, Apr 24, 2019 at 7:54 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> Oftentimes, people want to use multiple of the same kind of hook. This
> may be because software or a project they use requires a given hook, but
> they would also like to have a custom hook, or because they're using
> multiple pieces of software that both require involvement with the same
> hook.

Heh you beat me to it. My config-hooks branch [1] has not been updated
for half a year. I only skimmed through quickly so no useful comments,
but I went with a slightly different design, introducing
for_each_hook() instead (see run-command.[ch] in the last patch).

Back then I wasn't sure what would be the right policy for running
multiple hooks. By keeping the hook loop in one place, we could change
that policy much easier later (e.g. continue anyway in case of
failure, or stop as soon as you see a problem, or even run some
special hooks at the end even when there is an error earlier).

I'm not saying that should be the way to go. Just pointing it out. You
decide. Personally I think open coding the loop is just as well.

[1] https://gitlab.com/pclouds/git/commits/config-hooks
-- 
Duy

Re: [PATCH 3/5] sequencer: add support for multiple hooks

[Phillip Wood]

On 24/04/2019 01:49, brian m. carlson wrote:
> Add support for multiple post-rewrite hooks, both for "git commit
> --amend" and "git rebase".
> 
> Additionally add support for multiple prepare-commit-msg hooks.
> 
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  builtin/am.c                       | 28 ++++++---

Having read the patch subject I was surprised to see this touching
bulitin/am.c

>  sequencer.c                        | 96 +++++++++++++++++++-----------
>  t/t5407-post-rewrite-hook.sh       | 15 +++++
>  t/t7505-prepare-commit-msg-hook.sh |  9 +++
>  4 files changed, 105 insertions(+), 43 deletions(-)
> 
> diff --git a/builtin/am.c b/builtin/am.c
> index 4fb107a9d1..303abc98c2 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -442,22 +442,32 @@ 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");
> +	struct child_process proc;
> +	struct string_list *hooks;
> +	struct string_list_item *p;
>  	int ret;
>  
> -	if (!hook)
> +	hooks = find_hooks("post-rewrite");
> +	if (!hooks)
>  		return 0;
>  
> -	argv_array_push(&cp.args, hook);
> -	argv_array_push(&cp.args, "rebase");
> +	for_each_string_list_item(p, hooks) {
> +		child_process_init(&proc);
>  
> -	cp.in = xopen(am_path(state, "rewritten"), O_RDONLY);
> -	cp.stdout_to_stderr = 1;
> -	cp.trace2_hook_name = "post-rewrite";
> +		argv_array_push(&cp.args, p->string);
> +		argv_array_push(&cp.args, "rebase");
>  
> -	ret = run_command(&cp);
> +		cp.in = xopen(am_path(state, "rewritten"), O_RDONLY);
> +		cp.stdout_to_stderr = 1;
> +		cp.trace2_hook_name = "post-rewrite";
>  
> -	close(cp.in);
> +		ret = run_command(&cp);
> +
> +		close(cp.in);
> +		if (ret)
> +			break;
> +	}
> +	free_hooks(hooks);
>  	return ret;
>  }
>  
> diff --git a/sequencer.c b/sequencer.c
> index 79a046d748..3a616ceba6 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1088,33 +1088,69 @@ int update_head_with_reflog(const struct commit *old_head,
>  static int run_rewrite_hook(const struct object_id *oldoid,
>  			    const struct object_id *newoid)
>  {
> -	struct child_process proc = CHILD_PROCESS_INIT;
> +	struct child_process proc;
> +	struct string_list *hooks;
> +	struct string_list_item *p;
>  	const char *argv[3];
>  	int code;
>  	struct strbuf sb = STRBUF_INIT;
>  
> -	argv[0] = find_hook("post-rewrite");
> -	if (!argv[0])
> +	hooks = find_hooks("post-rewrite");
> +	if (!hooks)
>  		return 0;
>  
> -	argv[1] = "amend";
> -	argv[2] = NULL;
> +	for_each_string_list_item(p, hooks) {
> +		child_process_init(&proc);
>  
> -	proc.argv = argv;
> -	proc.in = -1;
> -	proc.stdout_to_stderr = 1;
> -	proc.trace2_hook_name = "post-rewrite";
> +		argv[0] = p->string;
> +		argv[1] = "amend";
> +		argv[2] = NULL;
>  
> -	code = start_command(&proc);
> -	if (code)
> -		return code;
> -	strbuf_addf(&sb, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid));
> -	sigchain_push(SIGPIPE, SIG_IGN);
> -	write_in_full(proc.in, sb.buf, sb.len);
> -	close(proc.in);
> -	strbuf_release(&sb);
> -	sigchain_pop(SIGPIPE);
> -	return finish_command(&proc);
> +		proc.argv = argv;
> +		proc.in = -1;
> +		proc.stdout_to_stderr = 1;
> +		proc.trace2_hook_name = "post-rewrite";
> +
> +		code = start_command(&proc);
> +		if (code)
> +			return code;
> +		strbuf_addf(&sb, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid));
> +		sigchain_push(SIGPIPE, SIG_IGN);
> +		write_in_full(proc.in, sb.buf, sb.len);
> +		close(proc.in);
> +		strbuf_release(&sb);
> +		sigchain_pop(SIGPIPE);
> +		code = finish_command(&proc);
> +		if (code)
> +			break;
> +	}
> +	free_hooks(hooks);
> +	return code;
> +}
> +
> +static void run_interactive_rewrite_hook(void)
> +{
> +	struct string_list *hooks;
> +	struct string_list_item *p;
> +	struct child_process child;
> +
> +	hooks = find_hooks("post-rewrite");
> +	if (!hooks)
> +		return;
> +
> +	for_each_string_list_item(p, hooks) {
> +		child_process_init(&child);
> +
> +		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, p->string);
> +		argv_array_push(&child.args, "rebase");
> +		if (run_command(&child))
> +			break;
> +	}
> +	free_hooks(hooks);
>  }

If you're adding a function to do this it would be nice to use it from
am.c as well rather than duplicating essentially the same code. Is there
any way to use a helper to run all the hooks, rather than introducing a
similar loop everywhere where we call a hook?

>  void commit_post_rewrite(struct repository *r,
> @@ -1326,6 +1362,7 @@ static int try_to_commit(struct repository *r,
>  	char *amend_author = NULL;
>  	const char *hook_commit = NULL;
>  	enum commit_msg_cleanup_mode cleanup;
> +	struct string_list *hooks;
>  	int res = 0;
>  
>  	if (parse_head(r, &current_head))
> @@ -1369,7 +1406,10 @@ static int try_to_commit(struct repository *r,
>  		goto out;
>  	}
>  
> -	if (find_hook("prepare-commit-msg")) {
> +	hooks = find_hooks("prepare-commit-msg");
> +	if (hooks) {
> +		free_hooks(hooks);

I think you forgot to update run_prepare_commit_msg_hook(), it should
probably be passed this list now. It might be outside the scope of this
series but unifying this with builtin/commit.c

> +
>  		res = run_prepare_commit_msg_hook(r, msg, hook_commit);
>  		if (res)
>  			goto out;
> @@ -3771,8 +3811,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;
> @@ -3782,18 +3820,8 @@ 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 */
> +			run_interactive_rewrite_hook();
>  		}
>  		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..5b83f037b5 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
>  '
>  
> +commit_command () {
> +	echo "$1" >>file &&
> +	git add file &&
> +	git commit -m "$1"
> +}
> +
> +test_multiple_hooks prepare-commit-msg commit_command

It's not clear to me that this is testing the sequencer

Best Wishes

Phillip

> +
>  test_done
> 

Re: [PATCH 0/5] Multiple hook support

[Phillip Wood]

On 24/04/2019 09:10, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Apr 24 2019, brian m. carlson wrote:
> 
>> Oftentimes, people want to use multiple of the same kind of hook. This
>> may be because software or a project they use requires a given hook, but
>> they would also like to have a custom hook, or because they're using
>> multiple pieces of software that both require involvement with the same
>> hook.
>>
>> This series introduces support for multiple hooks by using a ".d"
>> directory. For example, instead of using a single
>> ".git/hooks/post-checkout" file, you'd use multiple files within
>> ".git/hooks/post-checkout.d". This is the standard Unix paradigm for
>> multiple files and should be familiar to most people. Hooks are executed
>> in sorted order.
> 
> I think it's interesting if people can chime in with current in-the-wild
> implementations of this.
> 
> I know GitLab's the best, not because I was in any way involved in it,
> I've just dealt with writing hooks for it:
> https://docs.gitlab.com/ee/administration/custom_hooks.html#chained-hooks-support
> 
> There:
> 
>  1. The instance has a 'hooks' dir in the .git repo that's a symlink to
>     /my/global/hooks. They could also use a /etc/gitconfig
>     core.hooksPath for this part, but whatever.
> 
>  2. That /my/global/hooks has e.g. a /my/global/hooks/pre-receive that
>     git itself runs, which is a trampoline script that runs all over the
>     place and executes global/per-project hooks (which live in
>     .git/custom_hooks/).

Having global and local hooks could be useful. I've got some tooling
that rewrites multiple branches and has hooks to prevent committing and
rebasing in worktrees where the branch is being rewritten by a rewrite
in another worktree. I'm using core.hooksPath at the moment, but if I
ever work with a project that uses its own hooks I'll have to add some
sort of trampoline like gitlab.

Best Wishes

Phillip

> 
>  3. "The hooks of the same type are executed in order and execution
>      stops on the first script exiting with a non-zero value."
> 
> I wonder if the eventual goal of this facility would be to get such
> users on board with using git's native feature for this. This series is
> most of the way there for GitLab's case, but not quite.
> 
>> To preserve backwards compatibility, we don't run the hooks in the ".d"
>> directory if the single file is a valid hook (i.e. it exists and is
>> executable). This is because some people already have multiple hook
>> scripts configured, and if we ran them both, we'd run the hooks twice.
>> This would be bad for e.g. the prepare-commit-msg hook. This is also the
>> least surprising behavior.
>>
>> We check each hook for its exit status, even if the hook normally
>> ignores exit status, and if it fails, we abort executing further hooks.
>> This provides an easy way to reason about what the exit status is when a
>> hook fails; we need not consider how to handle multiple failing hooks.
>> It's also consistent among all hooks, whether they care about exit
>> status or not.
> 
> Others have replied to this already. I linked to the 2016 discussion of
> my RFC for this in
> https://public-inbox.org/git/87wojjsv9p.fsf@evledraar.gmail.com where I
> made the same choice. Some points on that:
> 
>  * There was the mention of "but what if someone wants to run them all",
>    e.g. for logging to N logging systems where one fails, that's been
>    brought up again this time around by others.
> 
>  * The case I find more interesting is the ability to run the hooks in
>    parallel. Saying "glob order and exit on first fail" categorically
>    closes the door to that.
> 
>> I've talked with some people about this approach, and they've indicated
>> they would prefer a configuration-based approach. I've tried to make the
>> series such that it can be replaced with such an approach if that's the
>> decision we make. It should be easy enough to simply replace find_hooks
>> with an appropriate implementation and update the test framework.
> 
> I think whatever opinions we all have on the current implementation it's
> OK to get this in in *some* form and just made it configurable or
> whatever later.
> 
> Most of the work is the ability to run N hooks, how exactly that happens
> can be tweaked later, and if this series lands and someone isn't 100%
> happy with the semantics they're no worse off than they are now.
> 
> I.e. we could get something like this in its current form, and later have:
> 
>     core.hooksOrder = glob | random
>     core.hooksHaltOnError = true | never
> 
> Where we'd say that what this series does is core.hooksOrder=glob and
> core.hooksHaltOnError=true, but that e.g. parallel "run them all" could
> be done in the future with core.hooksOrder=random &
> core.hooksHaltOnError=never
> 

Re: [PATCH 0/5] Multiple hook support

[Bryan Turner]

On Wed, Apr 24, 2019 at 1:10 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Wed, Apr 24 2019, brian m. carlson wrote:
>
> > Oftentimes, people want to use multiple of the same kind of hook. This
> > may be because software or a project they use requires a given hook, but
> > they would also like to have a custom hook, or because they're using
> > multiple pieces of software that both require involvement with the same
> > hook.
> >
> > This series introduces support for multiple hooks by using a ".d"
> > directory. For example, instead of using a single
> > ".git/hooks/post-checkout" file, you'd use multiple files within
> > ".git/hooks/post-checkout.d". This is the standard Unix paradigm for
> > multiple files and should be familiar to most people. Hooks are executed
> > in sorted order.
>
> I think it's interesting if people can chime in with current in-the-wild
> implementations of this.
>

Bitbucket Server also uses trampoline scripts for pre-receive and
post-receive that loop over scripts in matching *.d directories and
run them.

* It passes --template to "git init" (new repositories) and "git
clone" (forks) to have Git copy hook structure into place as part of
creating the repository.
* To avoid looping over thousands or tens of thousands of repositories
if those scripts need to change, the scripts put in place (both at the
pre-receive/post-receive level and for *.d/20_bitbucket_callback) are
essentially placeholders that use environment variables exported when
running "git http-backend" and "git receive-pack" to delegate to
helper scripts that actually do the work.
* The helper scripts only exist in one place, so they can easily be
updated if they need to change.
* When looping over *.d scripts, the helper script stops after the
first non-zero return.

On Tue, Apr 23, 2019 at 7:41 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> One downside is that the transition from the old to the new world
> order becomes a bigger deal.  You could have been using a precursor
> of this series implemented entirely as a shell script
>
>         $ cat >.git/pre-commit <<\EOF
>         #!/bin/sh
>         for x in .git/pre-commit.d/*
>         do
>                 "$x" "$@" || exit 1
>         done
>         exit 0
>         EOF
>         $ chmod +x .git/pre-commit
>
> and with your approach, the user can keep using that set-up and then
> simply remove that single file once all the Git executables the user
> uses are natively aware of the multi-hook setup.  With "either a
> file or directory" approach, the transition would be removing the
> file *and* renaming pre-commit.d directory to pre-commit.
>

This would be true for Bitbucket Server, since its trampoline scripts
are really simple: It could just delete them and let Git loop over the
existing *.d directories itself, so the current approach offers a
really straightforward upgrade path.

Of course, since Bitbucket Server supports a range of Git versions on
the server, and for compatibility reasons the minimum is only raised
in major version releases, it would likely be quite some time before
that could happen; the minimum supported Git version would need this
series to have landed in or before it. That's a big part of why
Bitbucket Server doesn't use core.hooksPath; it's too new. (Bitbucket
Server 6.0 raised the minimum Git version on the server to 2.11.0, so
it's the first major series that could use core.hooksPath; Bitbucket
Server 5.x supported back to Git 2.2.0, which doesn't have it.)

Best regards,
Bryan Turner

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

[Johannes Sixt]

Am 24.04.19 um 04:27 schrieb Junio C Hamano:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>> +static int has_hook(struct strbuf *path, int strip)
>> +{
>> +	if (access(path->buf, X_OK) < 0) {
> 
> Does ".git/post-commit" that is not an executable exist?
> 
> It was perfectly fine for find_hook() to say "there is no hook for
> post-commit" in the old world in such a case, because the
> unexecutable file it found is not going to be run anyway.
> 
> But it is not clear if has_hook(), that affects "there is no single
> hook file for post-commit, so let's look at post-commit.d" decision
> made by find_hooks(), should behave that way.  It somehow feels more
> intuitive if a post-commit file that is not executable, by merely
> existing, stops post-commit.d directory from being scanned, at least
> to me.

Furthermore, basing a decision on whether a file is executable won't
work on Windows as intended. So, it is better to aim for an existence check.

-- Hannes

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

[brian m. carlson]

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

On Wed, Apr 24, 2019 at 11:27:59AM +0900, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> > diff --git a/run-command.c b/run-command.c
> > index 3449db319b..669af5ebc7 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -1308,58 +1308,137 @@ int async_with_fork(void)
> >  #endif
> >  }
> >  
> > +static int has_hook(struct strbuf *path, int strip)
> > +{
> > +	if (access(path->buf, X_OK) < 0) {
> 
> Does ".git/post-commit" that is not an executable exist?
> 
> It was perfectly fine for find_hook() to say "there is no hook for
> post-commit" in the old world in such a case, because the
> unexecutable file it found is not going to be run anyway.
> 
> But it is not clear if has_hook(), that affects "there is no single
> hook file for post-commit, so let's look at post-commit.d" decision
> made by find_hooks(), should behave that way.  It somehow feels more
> intuitive if a post-commit file that is not executable, by merely
> existing, stops post-commit.d directory from being scanned, at least
> to me.

Unfortunately, we used to have Git versions that wrote an non-executable
file in place instead of a ".sample" file when initializing the
repository. We have a warning for that, but I have many repositories
that have lived long enough that they're still affected (and I've turned
off the warning for that reason). I feel like we'll be creating
surprising behavior for long-term users.

Also, if someone is using an old manager script that uses the ".d"
directory or some other workaround, it's easy enough for them to simply
clear the executable bit; they need not delete it, and can restore it at
any time simply by toggling the bit.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

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

Re: [PATCH 3/5] sequencer: add support for multiple hooks

[brian m. carlson]

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

On Wed, Apr 24, 2019 at 10:51:56AM +0100, Phillip Wood wrote:
> On 24/04/2019 01:49, brian m. carlson wrote:
> > Add support for multiple post-rewrite hooks, both for "git commit
> > --amend" and "git rebase".
> > 
> > Additionally add support for multiple prepare-commit-msg hooks.
> > 
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > ---
> >  builtin/am.c                       | 28 ++++++---
> 
> Having read the patch subject I was surprised to see this touching
> bulitin/am.c

I can rewrite the subject. Unfortunately, the same hook for rebase goes
through two wildly different modules, so in order to completely convert
the post-rewrite hook, I have to update both at the same time.

> > +static void run_interactive_rewrite_hook(void)
> > +{
> > +	struct string_list *hooks;
> > +	struct string_list_item *p;
> > +	struct child_process child;
> > +
> > +	hooks = find_hooks("post-rewrite");
> > +	if (!hooks)
> > +		return;
> > +
> > +	for_each_string_list_item(p, hooks) {
> > +		child_process_init(&child);
> > +
> > +		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, p->string);
> > +		argv_array_push(&child.args, "rebase");
> > +		if (run_command(&child))
> > +			break;
> > +	}
> > +	free_hooks(hooks);
> >  }
> 
> If you're adding a function to do this it would be nice to use it from
> am.c as well rather than duplicating essentially the same code. Is there
> any way to use a helper to run all the hooks, rather than introducing a
> similar loop everywhere where we call a hook?

It's becoming clear to me that a helper is probably going to be cleaner,
so I'll add one in for v2.

> >  void commit_post_rewrite(struct repository *r,
> > @@ -1326,6 +1362,7 @@ static int try_to_commit(struct repository *r,
> >  	char *amend_author = NULL;
> >  	const char *hook_commit = NULL;
> >  	enum commit_msg_cleanup_mode cleanup;
> > +	struct string_list *hooks;
> >  	int res = 0;
> >  
> >  	if (parse_head(r, &current_head))
> > @@ -1369,7 +1406,10 @@ static int try_to_commit(struct repository *r,
> >  		goto out;
> >  	}
> >  
> > -	if (find_hook("prepare-commit-msg")) {
> > +	hooks = find_hooks("prepare-commit-msg");
> > +	if (hooks) {
> > +		free_hooks(hooks);
> 
> I think you forgot to update run_prepare_commit_msg_hook(), it should
> probably be passed this list now. It might be outside the scope of this
> series but unifying this with builtin/commit.c

run_prepare_commit_msg_hook calls run_hook_le, which looks up that value
itself. It's unfortunate that we have to do it twice, but we need to
know whether we need to re-read the commit msg or not. I can explain
this further in the commit message.

> > diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
> > index ba8bd1b514..5b83f037b5 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
> >  '
> >  
> > +commit_command () {
> > +	echo "$1" >>file &&
> > +	git add file &&
> > +	git commit -m "$1"
> > +}
> > +
> > +test_multiple_hooks prepare-commit-msg commit_command
> 
> It's not clear to me that this is testing the sequencer

You're right. I need to adopt a different approach here.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

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

Re: [PATCH 0/5] Multiple hook support

[brian m. carlson]

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

On Wed, Apr 24, 2019 at 04:49:54PM +0700, Duy Nguyen wrote:
> Heh you beat me to it. My config-hooks branch [1] has not been updated
> for half a year. I only skimmed through quickly so no useful comments,
> but I went with a slightly different design, introducing
> for_each_hook() instead (see run-command.[ch] in the last patch).

I figured this was a common enough request that people wanted that it
was worth doing. I'll take a look at your series and I may steal some
code.

> Back then I wasn't sure what would be the right policy for running
> multiple hooks. By keeping the hook loop in one place, we could change
> that policy much easier later (e.g. continue anyway in case of
> failure, or stop as soon as you see a problem, or even run some
> special hooks at the end even when there is an error earlier).

Yeah, I think from feedback elsewhere, I'm going to roll it into a
single function. The nature of callbacks in C makes it potentially
messy, but I'll try to see if there's a tidy way to do it.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

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

Re: [PATCH 0/5] Multiple hook support

[brian m. carlson]

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

On Tue, Apr 23, 2019 at 07:34:38PM -0700, Jonathan Nieder wrote:
> Hi,
> 
> brian m. carlson wrote:
> 
> > I've talked with some people about this approach, and they've indicated
> > they would prefer a configuration-based approach.
> 
> I would, too, mostly because that reduces the problem of securing
> hooks to securing configuration.  See
> https://public-inbox.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/
> for more on this subject.

I know this is a common issue, but fixing it is a non-goal for this
series. Anything we do here is going to have to be backwards compatible,
so we can't make any changes to the security model.

> Solving (1) without (2) feels like a bit of a missed opportunity to
> me.  Ideally, what I would like is
> 
>    i. A central registry of trustworthy Git hooks that can be upgraded
>       using the system package manager to address (2).  Perhaps just
>       git-hook-* commands on the $PATH.
> 
>   ii. Instead of putting hooks in .git/hooks, put a list of hooks to
>       run for each event in .git/config.

The problem I had with this when discussing it was that our
configuration system lacks a good way to control inheritance from outer
files. I recently was working with a system-wide gitconfig file that
referred to files I didn't have, and my Git installation was subtly
broken in a variety of ways.

If I have a system-wide hook to run for company code, but I have a
checkout for my personal dotfiles on my machine where I don't want to
run that hook, our configuration lacks a way for me to disable that
system-wide configuration. However, using our current system, I can
override core.hooksPath in this case and everything works fine.

I mentioned this for completeness, and because I hope that some of those
people will get some time to chime in here, but I think without that
feature, we end up with a worse experience than we have now.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

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

Re: [PATCH 0/5] Multiple hook support

[Jonathan Nieder]

Hi,

brian m. carlson wrote:
> On Tue, Apr 23, 2019 at 07:34:38PM -0700, Jonathan Nieder wrote:
>> brian m. carlson wrote:

>>> I've talked with some people about this approach, and they've indicated
>>> they would prefer a configuration-based approach.
>>
>> I would, too, mostly because that reduces the problem of securing
>> hooks to securing configuration.  See
>> https://public-inbox.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/
>> for more on this subject.
>
> I know this is a common issue, but fixing it is a non-goal for this
> series. Anything we do here is going to have to be backwards compatible,
> so we can't make any changes to the security model.

I think it's worth bringing up because we should have some idea of where
we want to head.

I think the backward compatibility part is actually one of the easier
aspects of this one.  We don't have to change the security model right
away because there are similar places to exploit like core.pager.  To
address that, we need a notion of configuration that individual repos
and worktrees can't override, and using such a configuration item, we
can provide a way to opt in to the new security model.  That provides
a smooth path forward.

[...]
>> Solving (1) without (2) feels like a bit of a missed opportunity to
>> me.  Ideally, what I would like is
>>
>>    i. A central registry of trustworthy Git hooks that can be upgraded
>>       using the system package manager to address (2).  Perhaps just
>>       git-hook-* commands on the $PATH.
>>
>>   ii. Instead of putting hooks in .git/hooks, put a list of hooks to
>>       run for each event in .git/config.
>
> The problem I had with this when discussing it was that our
> configuration system lacks a good way to control inheritance from outer
> files.

The standard approach in lists defined in Git configuration is for
assigning an empty item to clear / restart the list.  See
http.extraHeader for an example.

Thanks and hope that helps,
Jonathan

Re: [PATCH 0/5] Multiple hook support

[brian m. carlson]

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

On Wed, Apr 24, 2019 at 04:49:54PM +0700, Duy Nguyen wrote:
> Heh you beat me to it. My config-hooks branch [1] has not been updated
> for half a year. I only skimmed through quickly so no useful comments,
> but I went with a slightly different design, introducing
> for_each_hook() instead (see run-command.[ch] in the last patch).

If I grab code from this series, may I apply your sign-off to it?
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

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

Re: [PATCH 0/5] Multiple hook support

[Duy Nguyen]

On Thu, Apr 25, 2019 at 6:41 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On Wed, Apr 24, 2019 at 04:49:54PM +0700, Duy Nguyen wrote:
> > Heh you beat me to it. My config-hooks branch [1] has not been updated
> > for half a year. I only skimmed through quickly so no useful comments,
> > but I went with a slightly different design, introducing
> > for_each_hook() instead (see run-command.[ch] in the last patch).
>
> If I grab code from this series, may I apply your sign-off to it?

Yes
-- 
Duy

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Furthermore, basing a decision on whether a file is executable won't
> work on Windows as intended. So, it is better to aim for an existence check.

That is a good point.

So it may be OK for "do we have a single hook script for this hook
name?" to say "no" when the path exists but not executable on
POSIXPERM systems, but it is better to say "yes" for consistency
across platforms (I think that is one of the reasons why we use
.sample suffix these days).

And for the same reason, for the purpose of deciding "because we do
not have a single hook script, let's peek into .d directory
ourselves", mere presence of the file with that name, regardless of
the executable bit, should signal that we should not handle the .d
directory.

IOW, you think access(X_OK) should be more like access(F_OK)?

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

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 25 2019, Junio C Hamano wrote:

> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Furthermore, basing a decision on whether a file is executable won't
>> work on Windows as intended. So, it is better to aim for an existence check.
>
> That is a good point.
>
> So it may be OK for "do we have a single hook script for this hook
> name?" to say "no" when the path exists but not executable on
> POSIXPERM systems, but it is better to say "yes" for consistency
> across platforms (I think that is one of the reasons why we use
> .sample suffix these days).
>
> And for the same reason, for the purpose of deciding "because we do
> not have a single hook script, let's peek into .d directory
> ourselves", mere presence of the file with that name, regardless of
> the executable bit, should signal that we should not handle the .d
> directory.
>
> IOW, you think access(X_OK) should be more like access(F_OK)?

To me this is another point in favor of bypassing this problem entirely
and adopting the semantics GitLab (and it seems others) use. I.e. in
order execute:

    .git/hooks/pre-receive .git/hooks/pre-receive.d/*

Instead of going further down this avenue of:

    if exists_or_executable_or_whatever .git/hooks/pre-receive
    then
        .git/hooks/pre-receive
    else
        for hook in .git/hooks/pre-receive.d/*
        do
            $hook
        done
    fi

It also:

 1) Makes it easier for users to experiment with more granular hooks if
    they have one big pre-receive hook by adding pre-receive.d/* hooks
    without having to move their existing pre-receive to
    pre-receive.d/000-existing hook (which will be incompatible across
    git versions!).

 2) Is compatible with any existing trampoline scripts you might want to
    migrate from that *don't* use pre-receive.d/*, e.g. one script in
    our infrastructure (that I didn't write) does:

        my ($hook_phase, $dir) = fileparse($0);
        my $exit = 0;
        my @hooks = glob("${dir}${hook_phase}-*");
        for my $hook (@hooks) {
            next unless -x $hook;
            $exit |= system $hook;
        }
        exit ($exit >> 8);

   I.e. you have a ".git/hooks/pre-receive" trampoline and it runs
   ".git/hooks/pre-receive-*" scripts.

It occurs to me that we might want to make things configurable for the
#2 case. I.e. have a core.hooksDSuffix=".d/" by default, but you could
also set it to "-". So we'd then construct a glob of either
"pre-receive.d/*" or "pre-receive-*" from that.

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> To me this is another point in favor of bypassing this problem entirely
> and adopting the semantics GitLab (and it seems others) use. I.e. in
> order execute:
>
>     .git/hooks/pre-receive .git/hooks/pre-receive.d/*

But isn't that exactly what Brian wanted to avoid?  Consider those
who run the current version of Git (which considers pre-receive.d/
is just a floating cruft Git does not care), with their own
implementation of pre-receive hook that goes over the contents of
pre-receive.d/ directory and executes each in order.  If a new
version of Git you release runs pre-receive followed by each of the
files in pre-receive.d/ directory, especially because you designed
to use *.d convention to match existing practice, doesn't your new
version of Git end up running the scripts in *.d directory twice?

How to undo previously set configuration? (again)

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 25 2019, brian m. carlson wrote:

> On Tue, Apr 23, 2019 at 07:34:38PM -0700, Jonathan Nieder wrote:
>> Hi,
>>
>> brian m. carlson wrote:
>>
>> > I've talked with some people about this approach, and they've indicated
>> > they would prefer a configuration-based approach.
>>
>> I would, too, mostly because that reduces the problem of securing
>> hooks to securing configuration.  See
>> https://public-inbox.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/
>> for more on this subject.
>
> I know this is a common issue, but fixing it is a non-goal for this
> series. Anything we do here is going to have to be backwards compatible,
> so we can't make any changes to the security model.

Agreed on the problem, and on the non-goal for solving it as part of
this series.

>> Solving (1) without (2) feels like a bit of a missed opportunity to
>> me.  Ideally, what I would like is
>>
>>    i. A central registry of trustworthy Git hooks that can be upgraded
>>       using the system package manager to address (2).  Perhaps just
>>       git-hook-* commands on the $PATH.
>>
>>   ii. Instead of putting hooks in .git/hooks, put a list of hooks to
>>       run for each event in .git/config.
>
> The problem I had with this when discussing it was that our
> configuration system lacks a good way to control inheritance from outer
> files. I recently was working with a system-wide gitconfig file that
> referred to files I didn't have, and my Git installation was subtly
> broken in a variety of ways.
>
> If I have a system-wide hook to run for company code, but I have a
> checkout for my personal dotfiles on my machine where I don't want to
> run that hook, our configuration lacks a way for me to disable that
> system-wide configuration. However, using our current system, I can
> override core.hooksPath in this case and everything works fine.
>
> I mentioned this for completeness, and because I hope that some of those
> people will get some time to chime in here, but I think without that
> feature, we end up with a worse experience than we have now.

I sent a proposal for this last year "How to undo previously set
configuration?":
https://public-inbox.org/git/874lkq11ug.fsf@evledraar.gmail.com/

Obviously the main bottleneck is someone like me working on patching it,
but in this case it would be very useful if those who are interested in
this could look that proposal over and bikeshed it / point out issues I
may have missed, i.e. "no, this categorically won't work with this
proposed syntax due to XYZ you haven't thought of...".

Because we don't have some general config facility for this it keeps
coming up, and various existing/proposed options have their own little
custom hacks for doing it, e.g. this for Barret Rhoden's proposed "blame
skip commits" feature
https://public-inbox.org/git/878swhfzxb.fsf@evledraar.gmail.com/
(b.t.w. I *meant* /dev/null in that E-Mail, but due to PEBCAK wrote
/dev/zero).

Re: How to undo previously set configuration? (again)

[Duy Nguyen]

On Thu, Apr 25, 2019 at 5:08 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> >> Solving (1) without (2) feels like a bit of a missed opportunity to
> >> me.  Ideally, what I would like is
> >>
> >>    i. A central registry of trustworthy Git hooks that can be upgraded
> >>       using the system package manager to address (2).  Perhaps just
> >>       git-hook-* commands on the $PATH.
> >>
> >>   ii. Instead of putting hooks in .git/hooks, put a list of hooks to
> >>       run for each event in .git/config.
> >
> > The problem I had with this when discussing it was that our
> > configuration system lacks a good way to control inheritance from outer
> > files. I recently was working with a system-wide gitconfig file that
> > referred to files I didn't have, and my Git installation was subtly
> > broken in a variety of ways.
> >
> > If I have a system-wide hook to run for company code, but I have a
> > checkout for my personal dotfiles on my machine where I don't want to
> > run that hook, our configuration lacks a way for me to disable that
> > system-wide configuration. However, using our current system, I can
> > override core.hooksPath in this case and everything works fine.
> >
> > I mentioned this for completeness, and because I hope that some of those
> > people will get some time to chime in here, but I think without that
> > feature, we end up with a worse experience than we have now.
>
> I sent a proposal for this last year "How to undo previously set
> configuration?":
> https://public-inbox.org/git/874lkq11ug.fsf@evledraar.gmail.com/

While reading that mail, it occurs to me that perhaps we can reuse the
.gitignore idea.

Instead of having a list of untracked files, we have a list of config
keys. Instead of having .gitignore files associated to different
directories to apply the rules to those dirs only, we have ignore
rules that should apply on certain config files (probably based on
path).

A few differences from your reject/accept/priority example:

- we don't redefine priority, inheritance rules apply the same way
- reject/accept is handled the same way as positive/negative ignore
rules. If we're lucky, we could even reuse the exclude code.
- instead of special section names like

    [config "section"]

we have something more like

    [config "/this/path"] # (or pattern)

this lets us handle even other config files included by [include] or [includeIf]

So, some examples

[exclude]            # exclude from all inherited files
    key = core.*     # exclude core.*
    key = !core.bar  # but keep core.bar

[excludeIf "path:/etc/config"] # rules apply for only this file
   key = ...

[excludeIf "glob:/home/*"]     # rules apply for these config paths
   key = ...

[excludeIf "system"]           # special names for convenience maybe
   key = ...

> Obviously the main bottleneck is someone like me working on patching it,

Yes, manpower is always the problem.

> but in this case it would be very useful if those who are interested in
> this could look that proposal over and bikeshed it / point out issues I
> may have missed, i.e. "no, this categorically won't work with this
> proposed syntax due to XYZ you haven't thought of...".
-- 
Duy

Re: How to undo previously set configuration? (again)

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 25 2019, Duy Nguyen wrote:

> On Thu, Apr 25, 2019 at 5:08 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> >> Solving (1) without (2) feels like a bit of a missed opportunity to
>> >> me.  Ideally, what I would like is
>> >>
>> >>    i. A central registry of trustworthy Git hooks that can be upgraded
>> >>       using the system package manager to address (2).  Perhaps just
>> >>       git-hook-* commands on the $PATH.
>> >>
>> >>   ii. Instead of putting hooks in .git/hooks, put a list of hooks to
>> >>       run for each event in .git/config.
>> >
>> > The problem I had with this when discussing it was that our
>> > configuration system lacks a good way to control inheritance from outer
>> > files. I recently was working with a system-wide gitconfig file that
>> > referred to files I didn't have, and my Git installation was subtly
>> > broken in a variety of ways.
>> >
>> > If I have a system-wide hook to run for company code, but I have a
>> > checkout for my personal dotfiles on my machine where I don't want to
>> > run that hook, our configuration lacks a way for me to disable that
>> > system-wide configuration. However, using our current system, I can
>> > override core.hooksPath in this case and everything works fine.
>> >
>> > I mentioned this for completeness, and because I hope that some of those
>> > people will get some time to chime in here, but I think without that
>> > feature, we end up with a worse experience than we have now.
>>
>> I sent a proposal for this last year "How to undo previously set
>> configuration?":
>> https://public-inbox.org/git/874lkq11ug.fsf@evledraar.gmail.com/
>
> While reading that mail, it occurs to me that perhaps we can reuse the
> .gitignore idea.
>
> Instead of having a list of untracked files, we have a list of config
> keys. Instead of having .gitignore files associated to different
> directories to apply the rules to those dirs only, we have ignore
> rules that should apply on certain config files (probably based on
> path).
>
> A few differences from your reject/accept/priority example:
>
> - we don't redefine priority, inheritance rules apply the same way
> - reject/accept is handled the same way as positive/negative ignore
> rules. If we're lucky, we could even reuse the exclude code.
> - instead of special section names like
>
>     [config "section"]
>
> we have something more like
>
>     [config "/this/path"] # (or pattern)
>
> this lets us handle even other config files included by [include] or [includeIf]
>
> So, some examples
>
> [exclude]            # exclude from all inherited files
>     key = core.*     # exclude core.*
>     key = !core.bar  # but keep core.bar
>
> [excludeIf "path:/etc/config"] # rules apply for only this file
>    key = ...
>
> [excludeIf "glob:/home/*"]     # rules apply for these config paths
>    key = ...
>
> [excludeIf "system"]           # special names for convenience maybe
>    key = ...
>
>> Obviously the main bottleneck is someone like me working on patching it,
>
> Yes, manpower is always the problem.
>
>> but in this case it would be very useful if those who are interested in
>> this could look that proposal over and bikeshed it / point out issues I
>> may have missed, i.e. "no, this categorically won't work with this
>> proposed syntax due to XYZ you haven't thought of...".

Thanks, I like this syntax/proposal much better than my initial one,
especially re-using the syntax we have in .gitignore. Also in that it's
more similar to the existing include syntax, which in retrospect with an
example is the obviously better choice both in terms of UI consistency
and flexibility.

I.e. I didn't want config files by globs, because depending on compile
options the /etc/gitconfig may be in /opt/etc/gitconfig, but as your
'[excludeIf "system"]' and '[excludeIf "path:/etc/config"]' examples
show we can have our cake and eat it too, and as you demonstrate there's
other reasons to do path globs that excluding the "git config
--{system,global,local,worktree}" file doesn't cover.

Re priorities: My "I don't really have a use-case for that" in 2018 is
still 95% true, just a couple of things:

 1. Having it would be a really nice smoke test for this working
    properly, i.e. now all the config parsing is "streamed" to its
    consumers via callbacks, having priorities would require the ability
    to pre-buffer and re-arrange it, the "pre-buffer" you'd need for any
    exclude mechanism anyway.

    Once we have that "priorities" should just be a quick sort of what
    order we loop over the files in.

 2. There is the use-case of "I don't want to exclude core.* from config
    file <A>, I just want file <B> to override it". I can imagine
    especially if/when we have in-repo .gitconfig that you'd want to
    trust say core.* from there, but have you ~/.gitconfig override it
    if you've bothered to set any of them yourself.

    But I think most of that use-case doesn't need priorities. It could
    just be another "exclude" syntax for common cases, e.g.:

        # ...Having done something else previously to opt-in to scary
        # in-repo .gitconfig...
        [excludeIf "repo"]
        key = core.* # exclude core.*
        [excludeIf "repo"]
        existingKey = true # exclude any existing key

    So e.g. you'd keep that .gitconfig's gc.bigPackThreshold or
    whatever, unless your ~/.gitconfig (parsed before, lower priority)
    had already set it.

Re: How to undo previously set configuration? (again)

[Jonathan Nieder]

Hi,

Ævar Arnfjörð Bjarmason wrote:

> Because we don't have some general config facility for this it keeps
> coming up, and various existing/proposed options have their own little
> custom hacks for doing it, e.g. this for Barret Rhoden's proposed "blame
> skip commits" feature
> https://public-inbox.org/git/878swhfzxb.fsf@evledraar.gmail.com/
> (b.t.w. I *meant* /dev/null in that E-Mail, but due to PEBCAK wrote
> /dev/zero).

I'm confused.  Isn't that bog-standard Git usage, not a custom hack?
That is, I thought the intended behavior is always

 1. For single-valued options, last value wins.
 2. For multi-valued options, empty clears the list.
 3. When there is a special behavior triggered by not supplying the
    option at all, offer an explicit value like "default" that triggers
    the same behavior, too.

and that any instance of a command that isn't following that is a bug.

Which of course leaves room for improvement in documentation and how
we organize the implementation (as Peff discussed in [1]), but isn't
it nice to already have something in place that doesn't require
inventing a new syntax?

Thanks,
Jonahtan

[1] https://public-inbox.org/git/20180406175044.GA32228@sigill.intra.peff.net/

Re: How to undo previously set configuration? (again)

[Barret Rhoden]

Hi -

On 4/25/19 10:36 AM, Jonathan Nieder wrote:
> Hi,
> 
> Ævar Arnfjörð Bjarmason wrote:
> 
>> Because we don't have some general config facility for this it keeps
>> coming up, and various existing/proposed options have their own little
>> custom hacks for doing it, e.g. this for Barret Rhoden's proposed "blame
>> skip commits" feature
>> https://public-inbox.org/git/878swhfzxb.fsf@evledraar.gmail.com/
>> (b.t.w. I*meant*  /dev/null in that E-Mail, but due to PEBCAK wrote
>> /dev/zero).
> I'm confused.  Isn't that bog-standard Git usage, not a custom hack?
> That is, I thought the intended behavior is always
> 
>   1. For single-valued options, last value wins.
>   2. For multi-valued options, empty clears the list.
>   3. When there is a special behavior triggered by not supplying the
>      option at all, offer an explicit value like "default" that triggers
>      the same behavior, too.
> 
> and that any instance of a command that isn't following that is a bug.

Not sure if it's meant to be the standard, but I just went with the 
style used by credential.helper.  This was suggested to me by Johannes 
in [1]:

On 2019-01-18 at 10:47 Johannes Schindelin <Johannes.Schindelin@gmx.de>
wrote:
> A better idea IMHO would be to use an OPT_STRING_LIST() for
> `--ignore-revs-file`, too, and to allow for multiple
> `blame.ignoreRevsFile` config entries (with our usual trick of handling an
> empty setting by resetting the list of paths that were accumulated so
> far, see e.g. how `credential.helper` is handled).

[1] 
https://public-inbox.org/git/nycvar.QRO.7.76.6.1901181038540.41@tvgsbejvaqbjf.bet/

Barret

Re: [PATCH 3/5] sequencer: add support for multiple hooks

[Phillip Wood]

On 24/04/2019 23:46, brian m. carlson wrote:
> On Wed, Apr 24, 2019 at 10:51:56AM +0100, Phillip Wood wrote:
>> On 24/04/2019 01:49, brian m. carlson wrote:
>>> Add support for multiple post-rewrite hooks, both for "git commit
>>> --amend" and "git rebase".
>>>
>>> Additionally add support for multiple prepare-commit-msg hooks.
>>>
>>> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
>>> ---
>>>   builtin/am.c                       | 28 ++++++---
>>
>> Having read the patch subject I was surprised to see this touching
>> bulitin/am.c
> 
> I can rewrite the subject. Unfortunately, the same hook for rebase goes
> through two wildly different modules, so in order to completely convert
> the post-rewrite hook, I have to update both at the same time.

It makes sense to convert both in the same commit, it's just that when I 
see "sequencer" I think of the subset of rebase (-k/-i/-m/-r/-x) that 
uses sequencer.c

>>> +static void run_interactive_rewrite_hook(void)
>>> +{
>>> +	struct string_list *hooks;
>>> +	struct string_list_item *p;
>>> +	struct child_process child;
>>> +
>>> +	hooks = find_hooks("post-rewrite");
>>> +	if (!hooks)
>>> +		return;
>>> +
>>> +	for_each_string_list_item(p, hooks) {
>>> +		child_process_init(&child);
>>> +
>>> +		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, p->string);
>>> +		argv_array_push(&child.args, "rebase");
>>> +		if (run_command(&child))
>>> +			break;
>>> +	}
>>> +	free_hooks(hooks);
>>>   }
>>
>> If you're adding a function to do this it would be nice to use it from
>> am.c as well rather than duplicating essentially the same code. Is there
>> any way to use a helper to run all the hooks, rather than introducing a
>> similar loop everywhere where we call a hook?
> 
> It's becoming clear to me that a helper is probably going to be cleaner,
> so I'll add one in for v2.
> 
>>>   void commit_post_rewrite(struct repository *r,
>>> @@ -1326,6 +1362,7 @@ static int try_to_commit(struct repository *r,
>>>   	char *amend_author = NULL;
>>>   	const char *hook_commit = NULL;
>>>   	enum commit_msg_cleanup_mode cleanup;
>>> +	struct string_list *hooks;
>>>   	int res = 0;
>>>   
>>>   	if (parse_head(r, &current_head))
>>> @@ -1369,7 +1406,10 @@ static int try_to_commit(struct repository *r,
>>>   		goto out;
>>>   	}
>>>   
>>> -	if (find_hook("prepare-commit-msg")) {
>>> +	hooks = find_hooks("prepare-commit-msg");
>>> +	if (hooks) {
>>> +		free_hooks(hooks);
>>
>> I think you forgot to update run_prepare_commit_msg_hook(), it should
>> probably be passed this list now. It might be outside the scope of this
>> series but unifying this with builtin/commit.c
> 
> run_prepare_commit_msg_hook calls run_hook_le, which looks up that value
> itself. It's unfortunate that we have to do it twice, but we need to
> know whether we need to re-read the commit msg or not. I can explain
> this further in the commit message.

Thanks for explaining that, it would be nice to have that in the commit 
message (I probably should have read the previous two patches to see 
what run_hook_le() was doing).

Best Wishes

Phillip


>>> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
>>> index ba8bd1b514..5b83f037b5 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
>>>   '
>>>   
>>> +commit_command () {
>>> +	echo "$1" >>file &&
>>> +	git add file &&
>>> +	git commit -m "$1"
>>> +}
>>> +
>>> +test_multiple_hooks prepare-commit-msg commit_command
>>
>> It's not clear to me that this is testing the sequencer
> 
> You're right. I need to adopt a different approach here.
> 

Re: How to undo previously set configuration? (again)

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 25 2019, Jonathan Nieder wrote:

> Ævar Arnfjörð Bjarmason wrote:
>
>> Because we don't have some general config facility for this it keeps
>> coming up, and various existing/proposed options have their own little
>> custom hacks for doing it, e.g. this for Barret Rhoden's proposed "blame
>> skip commits" feature
>> https://public-inbox.org/git/878swhfzxb.fsf@evledraar.gmail.com/
>> (b.t.w. I *meant* /dev/null in that E-Mail, but due to PEBCAK wrote
>> /dev/zero).
>
> I'm confused.  Isn't that bog-standard Git usage, not a custom hack?
> That is, I thought the intended behavior is always
>
>  1. For single-valued options, last value wins.
>  2. For multi-valued options, empty clears the list.
>  3. When there is a special behavior triggered by not supplying the
>     option at all, offer an explicit value like "default" that triggers
>     the same behavior, too.
>
> and that any instance of a command that isn't following that is a bug.
>
> Which of course leaves room for improvement in documentation and how
> we organize the implementation (as Peff discussed in [1]), but isn't
> it nice to already have something in place that doesn't require
> inventing a new syntax?

We can quibble over the wording, but I'd say that's a "hack". Yeah for
*some* options setting it to an empty value clears whether it's a
single-value or multi-value.

But it's entirely dependent on the specific callback implemented for
that option, sometimes there's no way to "reset" something, and in the
best case you'd need to exhaustively enumerate everything (e.g. for the
sendemail.* stuff) and hope nobody adds a new option tomorrow, you can't
say "clear this wildard" or "no config from system-wide".

So I think it makes sense to think about how we could promote this to
the config syntax itself so that what somewhat-mostly-but-not-quite
works now by convention would be guaranteed to work, and things that you
can't do at all today (e.g. ignore system config, or selectively ignore
something) would be supported.

Re: How to undo previously set configuration? (again)

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 25 2019, Barret Rhoden wrote:

> Hi -
>
> On 4/25/19 10:36 AM, Jonathan Nieder wrote:
>> Hi,
>>
>> Ævar Arnfjörð Bjarmason wrote:
>>
>>> Because we don't have some general config facility for this it keeps
>>> coming up, and various existing/proposed options have their own little
>>> custom hacks for doing it, e.g. this for Barret Rhoden's proposed "blame
>>> skip commits" feature
>>> https://public-inbox.org/git/878swhfzxb.fsf@evledraar.gmail.com/
>>> (b.t.w. I*meant*  /dev/null in that E-Mail, but due to PEBCAK wrote
>>> /dev/zero).
>> I'm confused.  Isn't that bog-standard Git usage, not a custom hack?
>> That is, I thought the intended behavior is always
>>
>>   1. For single-valued options, last value wins.
>>   2. For multi-valued options, empty clears the list.
>>   3. When there is a special behavior triggered by not supplying the
>>      option at all, offer an explicit value like "default" that triggers
>>      the same behavior, too.
>>
>> and that any instance of a command that isn't following that is a bug.
>
> Not sure if it's meant to be the standard, but I just went with the
> style used by credential.helper.  This was suggested to me by Johannes
> in [1]:

Just to be clear I'm not picking on your patch, I think doing it that
way makes perfect sense in the current system.

Just as noted upthread using it as an example (that was fresh in my
mind...) of a case where we want this, but it's a bespoke thing for
every use-case, and not consistent (i.e. just supported for your new
thing, but not for the existing fsck code that's mostly the same).

> On 2019-01-18 at 10:47 Johannes Schindelin <Johannes.Schindelin@gmx.de>
> wrote:
>> A better idea IMHO would be to use an OPT_STRING_LIST() for
>> `--ignore-revs-file`, too, and to allow for multiple
>> `blame.ignoreRevsFile` config entries (with our usual trick of handling an
>> empty setting by resetting the list of paths that were accumulated so
>> far, see e.g. how `credential.helper` is handled).
>
> [1]
> https://public-inbox.org/git/nycvar.QRO.7.76.6.1901181038540.41@tvgsbejvaqbjf.bet/
>
> Barret

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

[Johannes Sixt]

Am 25.04.19 um 02:55 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> Furthermore, basing a decision on whether a file is executable won't
>> work on Windows as intended. So, it is better to aim for an existence check.
> 
> That is a good point.
> 
> So it may be OK for "do we have a single hook script for this hook
> name?" to say "no" when the path exists but not executable on
> POSIXPERM systems, but it is better to say "yes" for consistency
> across platforms (I think that is one of the reasons why we use
> .sample suffix these days).

All correct.

> And for the same reason, for the purpose of deciding "because we do
> not have a single hook script, let's peek into .d directory
> ourselves", mere presence of the file with that name, regardless of
> the executable bit, should signal that we should not handle the .d
> directory.
> 
> IOW, you think access(X_OK) should be more like access(F_OK)?

Yes, that's my conclusion.

-- Hannes

Re: How to undo previously set configuration? (again)

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I'm confused.  Isn't that bog-standard Git usage, not a custom hack?

Depends on the definition of "hack".

The only native provision at the config API level that lets the
calling programs realize the "desired behaviour" you listed below
without doing anything special is "for a single-valued one, the last
one wins", for which the newer style config_get_value() will give
you the "last one wins" semantics.  With the original style parsing
using the git_config() with callback still needs to do the "last one
wins" by the caller of the config API.

The "empty clears the list" for multi-valued option is totally up to
the application.  Even the newer configset__get_value_multi() API
function does not reset a list to empty when it sees an empty
string.  The caller gets a list of value strings that happens to
have an empty string in the middle.

So, I think the "bog-standard" Git convention must be maintained by
each application codepath to implement it with a custom hack.

Perhaps you did not mean with "hack" a "hacky implementation", but a
"hacky design".  If that is the case, yeah, I agree with you that
the items #1 and #2 below are what we try to make sure our programs
follow; I am not sure about #3 myself, as I do not think offhand a
good example of it, though.

> That is, I thought the intended behavior is always
>
>  1. For single-valued options, last value wins.
>  2. For multi-valued options, empty clears the list.
>  3. When there is a special behavior triggered by not supplying the
>     option at all, offer an explicit value like "default" that triggers
>     the same behavior, too.
>
> and that any instance of a command that isn't following that is a bug.

If we make that declaration, we could enforce the "empty clears the
list" at the API level when the configset API is in use.

Re: How to undo previously set configuration? (again)

[Duy Nguyen]

On Thu, Apr 25, 2019 at 9:36 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Hi,
>
> Ævar Arnfjörð Bjarmason wrote:
>
> > Because we don't have some general config facility for this it keeps
> > coming up, and various existing/proposed options have their own little
> > custom hacks for doing it, e.g. this for Barret Rhoden's proposed "blame
> > skip commits" feature
> > https://public-inbox.org/git/878swhfzxb.fsf@evledraar.gmail.com/
> > (b.t.w. I *meant* /dev/null in that E-Mail, but due to PEBCAK wrote
> > /dev/zero).
>
> I'm confused.  Isn't that bog-standard Git usage, not a custom hack?
> That is, I thought the intended behavior is always
>
>  1. For single-valued options, last value wins.
>  2. For multi-valued options, empty clears the list.

I didn't know this! Should it be documented? At least a quick skimming
through config.txt does not mention anything about empty value
clearing multi-valued options.

I also wanted to see if it's true. However, the first var I checked,
branch.*.merge does not follow this rule. I got disappointed and
stopped.

>  3. When there is a special behavior triggered by not supplying the
>     option at all, offer an explicit value like "default" that triggers
>     the same behavior, too.
>
> and that any instance of a command that isn't following that is a bug.

Not sure how many we need to fix. The behaviour does make sense to me though.

> Which of course leaves room for improvement in documentation and how
> we organize the implementation (as Peff discussed in [1]), but isn't
> it nice to already have something in place that doesn't require
> inventing a new syntax?

This cannot undefine a variable though, especially those single-valued
ones. But I think for most cases, the user just needs to find out what
is the default value and set to that one.

I don't know if there is any case where the lack of a variable behaves
differently (or the default value is too dynamic to be set manually)
though.

> Thanks,
> Jonahtan
>
> [1] https://public-inbox.org/git/20180406175044.GA32228@sigill.intra.peff.net/



-- 
Duy

Re: How to undo previously set configuration? (again)

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 25 2019, Ævar Arnfjörð Bjarmason wrote:

> On Thu, Apr 25 2019, Duy Nguyen wrote:
>
>> On Thu, Apr 25, 2019 at 5:08 PM Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>>> >> Solving (1) without (2) feels like a bit of a missed opportunity to
>>> >> me.  Ideally, what I would like is
>>> >>
>>> >>    i. A central registry of trustworthy Git hooks that can be upgraded
>>> >>       using the system package manager to address (2).  Perhaps just
>>> >>       git-hook-* commands on the $PATH.
>>> >>
>>> >>   ii. Instead of putting hooks in .git/hooks, put a list of hooks to
>>> >>       run for each event in .git/config.
>>> >
>>> > The problem I had with this when discussing it was that our
>>> > configuration system lacks a good way to control inheritance from outer
>>> > files. I recently was working with a system-wide gitconfig file that
>>> > referred to files I didn't have, and my Git installation was subtly
>>> > broken in a variety of ways.
>>> >
>>> > If I have a system-wide hook to run for company code, but I have a
>>> > checkout for my personal dotfiles on my machine where I don't want to
>>> > run that hook, our configuration lacks a way for me to disable that
>>> > system-wide configuration. However, using our current system, I can
>>> > override core.hooksPath in this case and everything works fine.
>>> >
>>> > I mentioned this for completeness, and because I hope that some of those
>>> > people will get some time to chime in here, but I think without that
>>> > feature, we end up with a worse experience than we have now.
>>>
>>> I sent a proposal for this last year "How to undo previously set
>>> configuration?":
>>> https://public-inbox.org/git/874lkq11ug.fsf@evledraar.gmail.com/
>>
>> While reading that mail, it occurs to me that perhaps we can reuse the
>> .gitignore idea.
>>
>> Instead of having a list of untracked files, we have a list of config
>> keys. Instead of having .gitignore files associated to different
>> directories to apply the rules to those dirs only, we have ignore
>> rules that should apply on certain config files (probably based on
>> path).
>>
>> A few differences from your reject/accept/priority example:
>>
>> - we don't redefine priority, inheritance rules apply the same way
>> - reject/accept is handled the same way as positive/negative ignore
>> rules. If we're lucky, we could even reuse the exclude code.
>> - instead of special section names like
>>
>>     [config "section"]
>>
>> we have something more like
>>
>>     [config "/this/path"] # (or pattern)
>>
>> this lets us handle even other config files included by [include] or [includeIf]
>>
>> So, some examples
>>
>> [exclude]            # exclude from all inherited files
>>     key = core.*     # exclude core.*
>>     key = !core.bar  # but keep core.bar
>>
>> [excludeIf "path:/etc/config"] # rules apply for only this file
>>    key = ...
>>
>> [excludeIf "glob:/home/*"]     # rules apply for these config paths
>>    key = ...
>>
>> [excludeIf "system"]           # special names for convenience maybe
>>    key = ...
>>
>>> Obviously the main bottleneck is someone like me working on patching it,
>>
>> Yes, manpower is always the problem.
>>
>>> but in this case it would be very useful if those who are interested in
>>> this could look that proposal over and bikeshed it / point out issues I
>>> may have missed, i.e. "no, this categorically won't work with this
>>> proposed syntax due to XYZ you haven't thought of...".
>
> Thanks, I like this syntax/proposal much better than my initial one,
> especially re-using the syntax we have in .gitignore. Also in that it's
> more similar to the existing include syntax, which in retrospect with an
> example is the obviously better choice both in terms of UI consistency
> and flexibility.
>
> I.e. I didn't want config files by globs, because depending on compile
> options the /etc/gitconfig may be in /opt/etc/gitconfig, but as your
> '[excludeIf "system"]' and '[excludeIf "path:/etc/config"]' examples
> show we can have our cake and eat it too, and as you demonstrate there's
> other reasons to do path globs that excluding the "git config
> --{system,global,local,worktree}" file doesn't cover.
>
> Re priorities: My "I don't really have a use-case for that" in 2018 is
> still 95% true, just a couple of things:
>
>  1. Having it would be a really nice smoke test for this working
>     properly, i.e. now all the config parsing is "streamed" to its
>     consumers via callbacks, having priorities would require the ability
>     to pre-buffer and re-arrange it, the "pre-buffer" you'd need for any
>     exclude mechanism anyway.
>
>     Once we have that "priorities" should just be a quick sort of what
>     order we loop over the files in.
>
>  2. There is the use-case of "I don't want to exclude core.* from config
>     file <A>, I just want file <B> to override it". I can imagine
>     especially if/when we have in-repo .gitconfig that you'd want to
>     trust say core.* from there, but have you ~/.gitconfig override it
>     if you've bothered to set any of them yourself.
>
>     But I think most of that use-case doesn't need priorities. It could
>     just be another "exclude" syntax for common cases, e.g.:
>
>         # ...Having done something else previously to opt-in to scary
>         # in-repo .gitconfig...
>         [excludeIf "repo"]
>         key = core.* # exclude core.*
>         [excludeIf "repo"]
>         existingKey = true # exclude any existing key
>
>     So e.g. you'd keep that .gitconfig's gc.bigPackThreshold or
>     whatever, unless your ~/.gitconfig (parsed before, lower priority)
>     had already set it.

A #3 I just encountered[1] where this settable "config priority" might
be handy, but perhaps it's still stupid and exclusions are enough:

 * Vendor's git server wants to run 'git -c gc.writeCommitGraph gc' to
   get commit graphs. I might want to override it.

 * The vendor can't just add that to /etc/gitconfig because they don't
   want to screw with the OS, or might not know which "git" they'll use
   (their own or OS, so system "gitconfig" in different
   places/inconsistent)

So something where they can just do that and I can in what *I* know to
be the system "gitconfig" do:

    [configPriority "cli-at-cwd:/var/lib/vendor/git-storage/*"]
    value = 5

If I know they'll be be running those commands on that path, and I'd
like to s/100/5/ the priority for "-c" there so it goes last (see the
suggested priority numbers in [2]).

Or maybe alternatively, we'd have something like "-c" (unfortunately
"-C" is taken) to add a new config file to the mix, without making it an
all-or-nothing like GIT_CONFIG_NOSYSTEM=1 and GIT_CONFIG=<path>). So
they:

    git --add-this-config-last-please=/var/lib/vendor/etc/gitaly/gitconfig gc

And then I do in /etc/gitconfig:

    [excludeIf "glob:/var/lib/vendor/etc/gitaly/gitconfig"]
    key = gc.*

But priorities might still be sensible. This use-case could be
alternatively done without them with a more sensible version of
"excludeIf.existingKey" discussed in the last mail. I.e. having
"existingKey" be a glob, not "true":

    [excludeIf "glob:/var/lib/vendor/etc/gitaly/gitconfig"]
    existingKey = gc.*

Ditto for "-c" values:

    [excludeIf "cli-at-cwd:/var/lib/vendor/git-storage/*"]
    existingKey = gc.*

So maybe I've managed to talk myself out this whole notion of
priorities.

I.e. maybe we can always process config in a pre-determined order and
just allow people to reach forward/backward with [excludeIf path/glob/-c
at cwd] & [exclude], respectively.

There's still the *theoretical* use-case of a user saying "I know the
sysadmin here knows better, I want their /etc/gitconfig to go after my
~/.gitconfig", but does anyone need/want it in practice? I don't know...

1. https://gitlab.com/gitlab-org/gitaly/issues/1643
2. https://public-inbox.org/git/874lkq11ug.fsf@evledraar.gmail.com/

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

[brian m. carlson]

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

On Thu, Apr 25, 2019 at 09:40:34PM +0200, Johannes Sixt wrote:
> Am 25.04.19 um 02:55 schrieb Junio C Hamano:
> > Johannes Sixt <j6t@kdbg.org> writes:
> > 
> >> Furthermore, basing a decision on whether a file is executable won't
> >> work on Windows as intended. So, it is better to aim for an existence check.
> > 
> > That is a good point.
> > 
> > So it may be OK for "do we have a single hook script for this hook
> > name?" to say "no" when the path exists but not executable on
> > POSIXPERM systems, but it is better to say "yes" for consistency
> > across platforms (I think that is one of the reasons why we use
> > .sample suffix these days).
> 
> All correct.

I would like to point out that we still have to perform an executability
check before we run the hook or we'll get errors printed to the user. As
I mentioned, there are many people with repositories that have the
non-.sample files. For me, any repository older than about five years
will likely have those files.

> > And for the same reason, for the purpose of deciding "because we do
> > not have a single hook script, let's peek into .d directory
> > ourselves", mere presence of the file with that name, regardless of
> > the executable bit, should signal that we should not handle the .d
> > directory.
> > 
> > IOW, you think access(X_OK) should be more like access(F_OK)?
> 
> Yes, that's my conclusion.

Right now, we have a standard way to handle the way we handle hooks: if
they are not executable, we warn and pretend there's no hook. With this
new paradigm, we have to check whether the main hook is executable, and
if it's not, we then have to check whether it's present, and if so, we
skip the multiple hooks.

I understand the executable bit is not useful on Windows, but on Unix,
we should be consistent with how we treat the hooks.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

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

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

[Johannes Sixt]

Am 26.04.19 um 22:58 schrieb brian m. carlson:
> On Thu, Apr 25, 2019 at 09:40:34PM +0200, Johannes Sixt wrote:
> I would like to point out that we still have to perform an executability
> check before we run the hook or we'll get errors printed to the user.

That's fine. On Windows, when a hook is present, it is also executable.

> Right now, we have a standard way to handle the way we handle hooks: if
> they are not executable, we warn and pretend there's no hook. With this
> new paradigm, we have to check whether the main hook is executable, and
> if it's not, we then have to check whether it's present, and if so, we
> skip the multiple hooks.
> 
> I understand the executable bit is not useful on Windows, but on Unix,
> we should be consistent with how we treat the hooks.

We want to check for two vastly different conditions:

- Do we have to inspect the multi-hook directory? That decision should
be based on existence.

- Do we have to issue a warning? That can be based on the executable
flag. (As I understand, this is just a convenience warning because we do
not want the user to see a cryptic "cannot execute this thing" error or
something.)

I can see that you sense an inconsistency when you treat "not
executable" as "does not exist". But that is just too subtle in my book,
hard to explain, and not the practice that we are exercising these days.

I'm more concerned about the platform differences that we would have to
note in the documentation:

  "To have multple hooks, do X and Y and make sure the standard hook
   file is not executable. Oh, and by the way, if you are on Windows,
   you have to remove the file to make it not executable."

Let's not go there.

-- Hannes

Re: How to undo previously set configuration? (again)

[Jeff King]

On Fri, Apr 26, 2019 at 04:36:40PM +0700, Duy Nguyen wrote:

> > I'm confused.  Isn't that bog-standard Git usage, not a custom hack?
> > That is, I thought the intended behavior is always
> >
> >  1. For single-valued options, last value wins.
> >  2. For multi-valued options, empty clears the list.
> 
> I didn't know this! Should it be documented? At least a quick skimming
> through config.txt does not mention anything about empty value
> clearing multi-valued options.
> 
> I also wanted to see if it's true. However, the first var I checked,
> branch.*.merge does not follow this rule. I got disappointed and
> stopped.

It's definitely not implemented universally; each consumer of the config
option must decide on it (and it will probably always be that way to
some degree, since we don't know the semantics of each options; recall
that we may be holding config keys for other non-core programs, too).
And we just haven't retro-fitted a lot of those older options because
nobody has been bothered by it.

That said, I am a proponent of having some kind of clearing mechanism
(and I was the one who added credential.helper's mechanism, which has
been mentioned in this thread).  I think it makes things a lot less
difficult if we don't have to change the syntax of the config files to
do it. With that constraint, that pretty much leaves:

  1. Some sentinel value like the empty string. That one _probably_
     works in most cases, but there may be lists which want to represent
     the empty value. There could be other sentinel values (e.g.,
     "CLEAR") which are simply unlikely to be used as real values.

  2. The boolean syntax (i.e., "[foo]bar" with no equals) is almost
     always bogus for a list. So that can work as a sentinel that is
     OK syntactically.

> > Which of course leaves room for improvement in documentation and how
> > we organize the implementation (as Peff discussed in [1]), but isn't
> > it nice to already have something in place that doesn't require
> > inventing a new syntax?
> 
> This cannot undefine a variable though, especially those single-valued
> ones. But I think for most cases, the user just needs to find out what
> is the default value and set to that one.

Having "default" is a little more convenient, but it would need to be
implemented on a per-value basis anyway. In many cases the caller of the
config code has implemented last-one-wins by overwriting an old value,
and only it knows how to restore "default".

I guess if we moved completely to a configset world, then asking for
git_config_get_string() could resolve "default" completely. But we are a
ways from that.

-Peff

Re: [PATCH 0/5] Multiple hook support

[Jeff King]

On Wed, Apr 24, 2019 at 12:49:43AM +0000, brian m. carlson wrote:

> I've talked with some people about this approach, and they've indicated
> they would prefer a configuration-based approach.

I think I'm some people. :)

I agree with the thoughts that Jonathan pointed out in [1], but I wanted
to raise a few points that are more directly related to hook features:

  1. Config is resolved at run-time, making it much easier to have
     system or user-level hooks (as opposed to our current system of
     on-disk files, which require copying or symlinking hooks ahead of
     time into each repository you want to impact).

  2. Config values let you easily run hooks from multiple sources (e.g.,
     a hook specified in /etc/gitconfig, one in ~/.gitconfig, and then a
     repo-level hook in .git/config). Even with a "hook.d" feature like
     this, you are back to doing lots of symlinks within the ".d"
     directory to get this behavior.

     I specifically worry that adding ".d" directories is a step in the
     wrong direction because our solution will probably make this point
     _worse_ than whatever custom trampolines people are already using.

  3. A well-designed config schema can leave room for more
     configuration. E.g., one of the big questions with multi-hooks is
     the error semantics. But what if we had:

       [hook "pre-receive"]
       command = my-hook-cmd
       command = another-hook-cmd
       # stop running and return failure at first non-zero exit
       errorBehavior = stop-on-first
       # ...or run all and return error if _any_ failed
       errorBehavior = report-any-error
       # ...or run and report if any _accepted_
       errorBehavior = report-any-success

      Those are just off the top of my head. But my point is that by
      staking out a config section for each hook, it gives us a place to
      naturally add new config options. And we can do it on a per-hook
      basis, which I think will be important since each hook has its own
      semantics.

      Now that's not _strictly_ necessary. We could still have
      "hook.pre-receive.errorBehavior" and just assume
      "hook.pre-receive.command" is "$GIT_DIR/hooks/pre-receive". But I
      think doing the whole thing from config makes the behavior simple
      and consistent (and the backwards compatibility is easy -- if they
      aren't using the new command config, we really do behave "as if"
      they had set it to the file in the hooks directory).

So I agree with your general sentiment that the multi-hook support
is conceptually orthogonal to switching to a config-based system. But I
think it's worth considering whether we want to do something
config-based first:

  - if we introduce it later, it saves us from having _three_ ways to do
    the same thing

  - I think it provides a more natural way to express the options that
    will inevitably grow once we have multi-hook support

-Peff

[1] https://public-inbox.org/git/20190424023438.GE98980@google.com/

Re: How to undo previously set configuration? (again)

[Duy Nguyen]

On Wed, May 1, 2019 at 4:14 AM Jeff King <peff@peff.net> wrote:
> It's definitely not implemented universally; each consumer of the config
> option must decide on it (and it will probably always be that way to
> some degree, since we don't know the semantics of each options; recall
> that we may be holding config keys for other non-core programs, too).
> And we just haven't retro-fitted a lot of those older options because
> nobody has been bothered by it.
>
> That said, I am a proponent of having some kind of clearing mechanism
> (and I was the one who added credential.helper's mechanism, which has
> been mentioned in this thread).  I think it makes things a lot less
> difficult if we don't have to change the syntax of the config files to
> do it. With that constraint, that pretty much leaves:
>
>   1. Some sentinel value like the empty string. That one _probably_
>      works in most cases, but there may be lists which want to represent
>      the empty value. There could be other sentinel values (e.g.,
>      "CLEAR") which are simply unlikely to be used as real values.
>
>   2. The boolean syntax (i.e., "[foo]bar" with no equals) is almost
>      always bogus for a list. So that can work as a sentinel that is
>      OK syntactically.

Another question about the universal clearing mechanism, how does "git
config" fit into this? It would be great if the user can actually see
the same thing a git command sees to troubleshoot. Option 1 leaves the
interpretation/guessing to the user, "git config" simply gives the raw
input list before "clear" is processed. Option 2, "git config"
probably can be taught to optionally clear when it sees the boolean
syntax.
-- 
Duy

Re: How to undo previously set configuration? (again)

[Ævar Arnfjörð Bjarmason]

On Wed, May 01 2019, Duy Nguyen wrote:

> On Wed, May 1, 2019 at 4:14 AM Jeff King <peff@peff.net> wrote:
>> It's definitely not implemented universally; each consumer of the config
>> option must decide on it (and it will probably always be that way to
>> some degree, since we don't know the semantics of each options; recall
>> that we may be holding config keys for other non-core programs, too).
>> And we just haven't retro-fitted a lot of those older options because
>> nobody has been bothered by it.
>>
>> That said, I am a proponent of having some kind of clearing mechanism
>> (and I was the one who added credential.helper's mechanism, which has
>> been mentioned in this thread).  I think it makes things a lot less
>> difficult if we don't have to change the syntax of the config files to
>> do it. With that constraint, that pretty much leaves:
>>
>>   1. Some sentinel value like the empty string. That one _probably_
>>      works in most cases, but there may be lists which want to represent
>>      the empty value. There could be other sentinel values (e.g.,
>>      "CLEAR") which are simply unlikely to be used as real values.
>>
>>   2. The boolean syntax (i.e., "[foo]bar" with no equals) is almost
>>      always bogus for a list. So that can work as a sentinel that is
>>      OK syntactically.
>
> Another question about the universal clearing mechanism, how does "git
> config" fit into this? It would be great if the user can actually see
> the same thing a git command sees to troubleshoot. Option 1 leaves the
> interpretation/guessing to the user, "git config" simply gives the raw
> input list before "clear" is processed. Option 2, "git config"
> probably can be taught to optionally clear when it sees the boolean
> syntax.

We can make it fancier, but we already deal with this, e.g. if you do
"git config -l" we'll show "include{,if}" directives at the same "level"
as other "normal" keys.

We also provide no way in "git config" to properly interpret a
value. E.g. does a "user.email" showing up twice for me mean I have two
E-Mails at the same time, or does the last one win? We both know the
answer, but git-config itself doesn't, and that information lives in
docs/code outside of it.

Similarly we'd just print a sequence of:

    user.name=foo
    user.email=bar
    exclude.key=user.*
    user.name=baz

And it would be up to some "smarter" reader of the config data to
realize that the end result is one where we have no "user.email" set,
and "user.name=baz".

But yeah, optionally having some new --list-normalized or
--list-after-excludes or whatever would be great, and presumably not
hard if we had some central "excludes" mechanism...

Re: How to undo previously set configuration? (again)

[Duy Nguyen]

On Wed, May 1, 2019 at 7:18 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>
> On Wed, May 01 2019, Duy Nguyen wrote:
>
> > On Wed, May 1, 2019 at 4:14 AM Jeff King <peff@peff.net> wrote:
> >> It's definitely not implemented universally; each consumer of the config
> >> option must decide on it (and it will probably always be that way to
> >> some degree, since we don't know the semantics of each options; recall
> >> that we may be holding config keys for other non-core programs, too).
> >> And we just haven't retro-fitted a lot of those older options because
> >> nobody has been bothered by it.
> >>
> >> That said, I am a proponent of having some kind of clearing mechanism
> >> (and I was the one who added credential.helper's mechanism, which has
> >> been mentioned in this thread).  I think it makes things a lot less
> >> difficult if we don't have to change the syntax of the config files to
> >> do it. With that constraint, that pretty much leaves:
> >>
> >>   1. Some sentinel value like the empty string. That one _probably_
> >>      works in most cases, but there may be lists which want to represent
> >>      the empty value. There could be other sentinel values (e.g.,
> >>      "CLEAR") which are simply unlikely to be used as real values.
> >>
> >>   2. The boolean syntax (i.e., "[foo]bar" with no equals) is almost
> >>      always bogus for a list. So that can work as a sentinel that is
> >>      OK syntactically.
> >
> > Another question about the universal clearing mechanism, how does "git
> > config" fit into this? It would be great if the user can actually see
> > the same thing a git command sees to troubleshoot. Option 1 leaves the
> > interpretation/guessing to the user, "git config" simply gives the raw
> > input list before "clear" is processed. Option 2, "git config"
> > probably can be taught to optionally clear when it sees the boolean
> > syntax.
>
> We can make it fancier, but we already deal with this, e.g. if you do
> "git config -l" we'll show "include{,if}" directives at the same "level"
> as other "normal" keys.
>
> We also provide no way in "git config" to properly interpret a
> value. E.g. does a "user.email" showing up twice for me mean I have two
> E-Mails at the same time, or does the last one win?

Actually --get knows this. Single-valued options can be handled
correctly quite easily. It's --get-all (or rather, the future
--get-multi because we can't change --get-all's behavior) which can't
interpret values because there's no standardized way of doing it.

> We both know the
> answer, but git-config itself doesn't, and that information lives in
> docs/code outside of it.
>
> Similarly we'd just print a sequence of:
>
>     user.name=foo
>     user.email=bar
>     exclude.key=user.*
>     user.name=baz
>
> And it would be up to some "smarter" reader of the config data to
> realize that the end result is one where we have no "user.email" set,
> and "user.name=baz".
>
> But yeah, optionally having some new --list-normalized or
> --list-after-excludes or whatever would be great, and presumably not
> hard if we had some central "excludes" mechanism...
-- 
Duy

Re: How to undo previously set configuration? (again)

[Jeff King]

On Wed, May 01, 2019 at 07:32:20PM +0700, Duy Nguyen wrote:

> > We also provide no way in "git config" to properly interpret a
> > value. E.g. does a "user.email" showing up twice for me mean I have two
> > E-Mails at the same time, or does the last one win?
> 
> Actually --get knows this. Single-valued options can be handled
> correctly quite easily. It's --get-all (or rather, the future
> --get-multi because we can't change --get-all's behavior) which can't
> interpret values because there's no standardized way of doing it.

Right. We need a hint from the caller about how they expect us to
interpret the values. And I agree we should probably introduce a new
verb instead of modifying --get-all, which some callers might be
expecting to do their own list processing.

Likewise in the C code. We probably want to leave existing callers of
git_config_get_value_multi() alone, and give them a new
git_config_get_string_list() or something.

-Peff

Re: How to undo previously set configuration? (again)

[Jeff King]

On Wed, May 01, 2019 at 02:18:34PM +0200, Ævar Arnfjörð Bjarmason wrote:

> We can make it fancier, but we already deal with this, e.g. if you do
> "git config -l" we'll show "include{,if}" directives at the same "level"
> as other "normal" keys.

We show them, but we _do_ interpret them if the caller asks for it with
--includes (which defaults to on when doing the usual "look in all
files").

I think we'd have something similar here, where the caller can ask to
apply excludes or not.

> We also provide no way in "git config" to properly interpret a
> value. E.g. does a "user.email" showing up twice for me mean I have two
> E-Mails at the same time, or does the last one win? We both know the
> answer, but git-config itself doesn't, and that information lives in
> docs/code outside of it.
> 
> Similarly we'd just print a sequence of:
> 
>     user.name=foo
>     user.email=bar
>     exclude.key=user.*
>     user.name=baz
> 
> And it would be up to some "smarter" reader of the config data to
> realize that the end result is one where we have no "user.email" set,
> and "user.name=baz".
> 
> But yeah, optionally having some new --list-normalized or
> --list-after-excludes or whatever would be great, and presumably not
> hard if we had some central "excludes" mechanism...

I think that is all because "--list" really is just about dumping all
values, not about interpreting. If we had an exclude mechanism, then I'd
expect:

  git config user.name

to apply it just like git_config_get_string() would.

Because of its lack of interpretation, I don't think --list is actually
good for much besides debugging. Some scripts do use it to avoid making
a bunch of individual git-config calls, but they'd be much better served
by a --stdin mode which let you feed in a sequence of operations ("I want
x.y.z, as a single last-one-wins value, and interpreted as a bool") and
get a sequence of outputs.

-Peff