hook: allow parallel hook execution

Hooks always run in sequential order due to the hardcoded jobs == 1
passed to run_process_parallel(). Remove that hardcoding to allow
users to run hooks in parallel (opt-in).

Users need to decide which hooks to run in parallel, by specifying
"parallel = true" in the config, because git cannot know if their
specific hooks are safe to run or not in parallel (for e.g. two hooks
might write to the same file or call the same program).

Some hooks are unsafe to run in parallel by design: these will marked
in the next commit using RUN_HOOKS_OPT_INIT_FORCE_SERIAL.

The hook.jobs config specifies the default number of jobs applied to all
hooks which have parallelism enabled.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

Documentation/config/hook.adoc

@@ -23,6 +23,19 @@ hook.<name>.enabled::
 	in a system or global config file and needs to be disabled for a
 	specific repository. See linkgit:git-hook[1].
 
+hook.<name>.parallel::
+	Whether the hook `hook.<name>` may run in parallel with other hooks
+	for the same event. Defaults to `false`. Set to `true` only when the
+	hook script is safe to run concurrently with other hooks for the same
+	event. If any hook for an event does not have this set to `true`,
+	all hooks for that event run sequentially regardless of `hook.jobs`.
+	Only configured (named) hooks need to declare this. Traditional hooks
+	found in the hooks directory do not need to, and run in parallel when
+	the effective job count is greater than 1. See linkgit:git-hook[1].
+
 hook.jobs::
 	Specifies how many hooks can be run simultaneously during parallelized
 	hook execution. If unspecified, defaults to 1 (serial execution).
++
+This setting has no effect unless all configured hooks for the event have
+`hook.<name>.parallel` set to `true`.

hook.c

@@ -120,11 +120,11 @@ static void unsorted_string_list_remove(struct string_list *list,
 
 /*
  * Cache entry stored as the .util pointer of string_list items inside the
- * hook config cache. For now carries only the command for the hook. Next
- * commits will add more data.
+ * hook config cache. Carries both the resolved command and the parallel flag.
  */
 struct hook_config_cache_entry {
 	char *command;
+	unsigned int parallel:1;
 };
 
 /*
@@ -132,12 +132,14 @@ struct hook_config_cache_entry {
  * commands: friendly-name to command map.
  * event_hooks: event-name to list of friendly-names map.
  * disabled_hooks: set of friendly-names with hook.name.enabled = false.
+ * parallel_hooks: friendly-name to parallel flag.
  * jobs: value of the global hook.jobs key. Defaults to 0 if unset.
  */
 struct hook_all_config_cb {
 	struct strmap commands;
 	struct strmap event_hooks;
 	struct string_list disabled_hooks;
+	struct strmap parallel_hooks;
 	unsigned int jobs;
 };
 
@@ -216,6 +218,10 @@ static int hook_config_lookup_all(const char *key, const char *value,
 		default:
 			break; /* ignore unrecognised values */
 		}
+	} else if (!strcmp(subkey, "parallel")) {
+		int v = git_parse_maybe_bool(value);
+		if (v >= 0)
+			strmap_put(&data->parallel_hooks, hook_name, (void *)(uintptr_t)v);
 	}
 
 	free(hook_name);
@@ -259,6 +265,7 @@ static void build_hook_config_map(struct repository *r,
 	strmap_init(&cb_data.commands);
 	strmap_init(&cb_data.event_hooks);
 	string_list_init_dup(&cb_data.disabled_hooks);
+	strmap_init(&cb_data.parallel_hooks);
 
 	/* Parse all configs in one run, capturing hook.* including hook.jobs. */
 	repo_config(r, hook_config_lookup_all, &cb_data);
@@ -273,6 +280,7 @@ static void build_hook_config_map(struct repository *r,
 		for (size_t i = 0; i < hook_names->nr; i++) {
 			const char *hname = hook_names->items[i].string;
 			struct hook_config_cache_entry *entry;
+			void *par = strmap_get(&cb_data.parallel_hooks, hname);
 			char *command;
 
 			/* filter out disabled hooks */
@@ -289,6 +297,7 @@ static void build_hook_config_map(struct repository *r,
 			/* util stores a cache entry; owned by the cache. */
 			CALLOC_ARRAY(entry, 1);
 			entry->command = xstrdup(command);
+			entry->parallel = par ? (int)(uintptr_t)par : 0;
 			string_list_append(hooks, hname)->util = entry;
 		}
 
@@ -298,6 +307,7 @@ static void build_hook_config_map(struct repository *r,
 	cache->jobs = cb_data.jobs;
 
 	strmap_clear(&cb_data.commands, 1);
+	strmap_clear(&cb_data.parallel_hooks, 0); /* values are uintptr_t, not heap ptrs */
 	string_list_clear(&cb_data.disabled_hooks, 0);
 	strmap_for_each_entry(&cb_data.event_hooks, &iter, e) {
 		string_list_clear(e->value, 0);
@@ -364,6 +374,7 @@ static void list_hooks_add_configured(struct repository *r,
 		hook->kind = HOOK_CONFIGURED;
 		hook->u.configured.friendly_name = xstrdup(friendly_name);
 		hook->u.configured.command = xstrdup(entry->command);
+		hook->parallel = entry->parallel;
 
 		string_list_append(list, friendly_name)->util = hook;
 	}
@@ -499,21 +510,67 @@ static void run_hooks_opt_clear(struct run_hooks_opt *options)
 	strvec_clear(&options->args);
 }
 
+/* Determine how many jobs to use for hook execution. */
+static unsigned int get_hook_jobs(struct repository *r,
+				  struct run_hooks_opt *options,
+				  struct string_list *hook_list)
+{
+	unsigned int jobs;
+
+	/*
+	 * Hooks needing separate output streams must run sequentially. Next
+	 * commits will add an extension to allow parallelizing these as well.
+	 */
+	if (!options->stdout_to_stderr)
+		return 1;
+
+	/* An explicit job count (FORCE_SERIAL jobs=1, or -j from CLI). */
+	if (options->jobs)
+		return options->jobs;
+
+	/*
+	 * Use hook.jobs from the already-parsed config cache (in-repo), or
+	 * fall back to a direct config lookup (out-of-repo).  Default to 1.
+	 */
+	if (r && r->gitdir && r->hook_config_cache)
+		/* Use the already-parsed cache (in-repo) */
+		jobs = r->hook_config_cache->jobs ? r->hook_config_cache->jobs : 1;
+	else
+		/* No cache present (out-of-repo call), use direct cfg lookup */
+		jobs = repo_config_get_uint(r, "hook.jobs", &jobs) ? 1 : jobs;
+
+	/*
+	 * Cap to serial any configured hook not marked as parallel = true.
+	 * This enforces the parallel = false default, even for "traditional"
+	 * hooks from the hookdir which cannot be marked parallel = true.
+	 */
+	for (size_t i = 0; jobs > 1 && i < hook_list->nr; i++) {
+		struct hook *h = hook_list->items[i].util;
+		if (h->kind == HOOK_CONFIGURED && !h->parallel)
+			jobs = 1;
+	}
+
+	return jobs;
+}
+
 int run_hooks_opt(struct repository *r, const char *hook_name,
 		  struct run_hooks_opt *options)
 {
+	struct string_list *hook_list = list_hooks(r, hook_name, options);
 	struct hook_cb_data cb_data = {
 		.rc = 0,
 		.hook_name = hook_name,
+		.hook_command_list = hook_list,
 		.options = options,
 	};
 	int ret = 0;
+	unsigned int jobs = get_hook_jobs(r, options, hook_list);
 	const struct run_process_parallel_opts opts = {
 		.tr2_category = "hook",
 		.tr2_label = hook_name,
 
-		.processes = options->jobs,
-		.ungroup = options->jobs == 1,
+		.processes = jobs,
+		.ungroup = jobs == 1,
 
 		.get_next_task = pick_next_hook,
 		.start_failure = notify_start_failure,
@@ -529,9 +586,6 @@ int run_hooks_opt(struct repository *r, const char *hook_name,
 	if (options->path_to_stdin && options->feed_pipe)
 		BUG("options path_to_stdin and feed_pipe are mutually exclusive");
 
-	if (!options->jobs)
-		BUG("run_hooks_opt must be called with options.jobs >= 1");
-
 	/*
 	 * Ensure cb_data copy and free functions are either provided together,
 	 * or neither one is provided.
@@ -543,7 +597,6 @@ int run_hooks_opt(struct repository *r, const char *hook_name,
 	if (options->invoked_hook)
 		*options->invoked_hook = 0;
 
-	cb_data.hook_command_list = list_hooks(r, hook_name, options);
 	if (!cb_data.hook_command_list->nr) {
 		if (options->error_if_missing)
 			ret = error("cannot find a hook named %s", hook_name);

hook.h

@@ -29,6 +29,13 @@ struct hook {
 		} configured;
 	} u;
 
+	/**
+	 * Whether this hook may run in parallel with other hooks for the same
+	 * event. Only useful for configured (named) hooks. Traditional hooks
+	 * always default to 0 (serial). Set via `hook.<name>.parallel = true`.
+	 */
+	unsigned int parallel:1;
+
 	/**
 	 * Opaque data pointer used to keep internal state across callback calls.
 	 *
@@ -62,6 +69,8 @@ struct run_hooks_opt
 	 *
 	 * If > 1, output will be buffered and de-interleaved (ungroup=0).
 	 * If == 1, output will be real-time (ungroup=1).
+	 * If == 0, the 'hook.jobs' config is used or, if the config is unset,
+	 * defaults to 1 (serial execution).
 	 */
 	unsigned int jobs;
 
@@ -142,7 +151,23 @@ struct run_hooks_opt
 	cb_data_free_fn feed_pipe_cb_data_free;
 };
 
+/**
+ * Default initializer for hooks. Parallelism is opt-in: .jobs = 0 defers to
+ * the 'hook.jobs' config, falling back to serial (1) if unset.
+ */
 #define RUN_HOOKS_OPT_INIT { \
+	.env = STRVEC_INIT, \
+	.args = STRVEC_INIT, \
+	.stdout_to_stderr = 1, \
+	.jobs = 0, \
+}
+
+/**
+ * Initializer for hooks that must always run sequentially regardless of
+ * 'hook.jobs'. Use this when git knows the hook cannot safely be parallelized
+ * .jobs = 1 is non-overridable.
+ */
+#define RUN_HOOKS_OPT_INIT_FORCE_SERIAL { \
 	.env = STRVEC_INIT, \
 	.args = STRVEC_INIT, \
 	.stdout_to_stderr = 1, \

t/t1800-hook.sh

@@ -21,6 +21,57 @@ setup_hookdir () {
 	test_when_finished rm -rf .git/hooks
 }
 
+# write_sentinel_hook <path> [sentinel]
+#
+# Writes a hook that marks itself as started, sleeps for a few seconds, then
+# marks itself done. The sleep must be long enough that sentinel_detector can
+# observe <sentinel>.started before <sentinel>.done appears when both hooks
+# run concurrently in parallel mode.
+write_sentinel_hook () {
+	sentinel="${2:-sentinel}"
+	write_script "$1" <<-EOF
+	touch ${sentinel}.started &&
+	sleep 2 &&
+	touch ${sentinel}.done
+	EOF
+}
+
+# sentinel_detector <sentinel> <output>
+#
+# Returns a shell command string suitable for use as hook.<name>.command.
+# The detector must be registered after the sentinel:
+# 1. In serial mode, the sentinel has completed (and <sentinel>.done exists)
+#    before the detector starts.
+# 2. In parallel mode, both run concurrently so <sentinel>.done has not appeared
+#    yet and the detector just sees <sentinel>.started.
+#
+# At start, poll until <sentinel>.started exists to absorb startup jitter, then
+# write to <output>:
+# 1. 'serial'   if <sentinel>.done exists (sentinel finished before we started),
+# 2. 'parallel' if only <sentinel>.started exists (sentinel still running),
+# 3. 'timeout'  if <sentinel>.started never appeared.
+#
+# The command ends with ':' so when git appends "$@" for hooks that receive
+# positional arguments (e.g. pre-push), the result ': "$@"' is valid shell
+# rather than a syntax error 'fi "$@"'.
+sentinel_detector () {
+	cat <<-EOF
+	i=0
+	while ! test -f ${1}.started && test \$i -lt 10; do
+	    sleep 1
+	    i=\$((i+1))
+	done
+	if test -f ${1}.done; then
+	    echo serial >${2}
+	elif test -f ${1}.started; then
+	    echo parallel >${2}
+	else
+	    echo timeout >${2}
+	fi
+	:
+	EOF
+}
+
 test_expect_success 'git hook usage' '
 	test_expect_code 129 git hook &&
 	test_expect_code 129 git hook run &&
@@ -553,4 +604,95 @@ test_expect_success 'server push-to-checkout hook expects stdout redirected to s
 	check_stdout_merged_to_stderr push-to-checkout
 '
 
+test_expect_success 'hook.jobs=1 config runs hooks in series' '
+	test_when_finished "rm -f sentinel.started sentinel.done hook.order" &&
+
+	# Use two configured hooks so the execution order is deterministic:
+	# hook-1 (sentinel) is listed before hook-2 (detector), so hook-1
+	# always runs first even in serial mode.
+	test_config hook.hook-1.event test-hook &&
+	test_config hook.hook-1.command \
+	    "touch sentinel.started; sleep 2; touch sentinel.done" &&
+	test_config hook.hook-2.event test-hook &&
+	test_config hook.hook-2.command \
+	    "$(sentinel_detector sentinel hook.order)" &&
+
+	test_config hook.jobs 1 &&
+
+	git hook run test-hook >out 2>err &&
+	echo serial >expect &&
+	test_cmp expect hook.order
+'
+
+test_expect_success 'hook.jobs=2 config runs hooks in parallel' '
+	test_when_finished "rm -f sentinel.started sentinel.done hook.order" &&
+	test_when_finished "rm -rf .git/hooks" &&
+
+	mkdir -p .git/hooks &&
+	write_sentinel_hook .git/hooks/test-hook &&
+
+	test_config hook.hook-2.event test-hook &&
+	test_config hook.hook-2.command \
+	    "$(sentinel_detector sentinel hook.order)" &&
+	test_config hook.hook-2.parallel true &&
+
+	test_config hook.jobs 2 &&
+
+	git hook run test-hook >out 2>err &&
+	echo parallel >expect &&
+	test_cmp expect hook.order
+'
+
+test_expect_success 'hook.<name>.parallel=true enables parallel execution' '
+	test_when_finished "rm -f sentinel.started sentinel.done hook.order" &&
+	test_config hook.hook-1.event test-hook &&
+	test_config hook.hook-1.command \
+	    "touch sentinel.started; sleep 2; touch sentinel.done" &&
+	test_config hook.hook-1.parallel true &&
+	test_config hook.hook-2.event test-hook &&
+	test_config hook.hook-2.command \
+	    "$(sentinel_detector sentinel hook.order)" &&
+	test_config hook.hook-2.parallel true &&
+
+	test_config hook.jobs 2 &&
+
+	git hook run test-hook >out 2>err &&
+	echo parallel >expect &&
+	test_cmp expect hook.order
+'
+
+test_expect_success 'hook.<name>.parallel=false (default) forces serial execution' '
+	test_when_finished "rm -f sentinel.started sentinel.done hook.order" &&
+	test_config hook.hook-1.event test-hook &&
+	test_config hook.hook-1.command \
+	    "touch sentinel.started; sleep 2; touch sentinel.done" &&
+	test_config hook.hook-2.event test-hook &&
+	test_config hook.hook-2.command \
+	    "$(sentinel_detector sentinel hook.order)" &&
+
+	test_config hook.jobs 2 &&
+
+	git hook run test-hook >out 2>err &&
+	echo serial >expect &&
+	test_cmp expect hook.order
+'
+
+test_expect_success 'one non-parallel hook forces the whole event to run serially' '
+	test_when_finished "rm -f sentinel.started sentinel.done hook.order" &&
+	test_config hook.hook-1.event test-hook &&
+	test_config hook.hook-1.command \
+	    "touch sentinel.started; sleep 2; touch sentinel.done" &&
+	test_config hook.hook-1.parallel true &&
+	test_config hook.hook-2.event test-hook &&
+	test_config hook.hook-2.command \
+	    "$(sentinel_detector sentinel hook.order)" &&
+	# hook-2 has no parallel=true: should force serial for all
+
+	test_config hook.jobs 2 &&
+
+	git hook run test-hook >out 2>err &&
+	echo serial >expect &&
+	test_cmp expect hook.order
+'
+
 test_done