[PATCH 0/2] Fix two hook configuration bugs

[PATCH 0/2] Fix two hook configuration bugs

[Thomas Bachem via GitGitGadget]

Two small fixes for hook configuration. With the first,
hook.pre-commit.enabled=false also stops the pre-commit hook in the hooks
directory, as the documentation says. With the second, hook.*.enabled and
hook.*.parallel can be given without a value, which means true, as for any
other boolean.

Thomas Bachem (2):
  hook: honor hook.<event>.enabled for the hookdir hook
  hook: accept .enabled and .parallel given without a value

 builtin/hook.c  |  6 ++++--
 hook.c          | 17 +++++++++++------
 hook.h          |  8 +++++++-
 t/t1800-hook.sh | 51 +++++++++++++++++++++++++++++++++++++++++++++++--
 4 files changed, 71 insertions(+), 11 deletions(-)


base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2272%2Fthomasbachem%2Fhook-event-enabled-hookdir-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2272/thomasbachem/hook-event-enabled-hookdir-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2272
-- 
gitgitgadget

[PATCH 1/2] hook: honor hook.<event>.enabled for the hookdir hook

[Thomas Bachem via GitGitGadget]

From: Thomas Bachem <mail@thomasbachem.com>

The documentation added by dcfb5af67e (hook: add hook.<event>.enabled
switch, 2026-04-10) says that with hook.<event>.enabled set to false
"no hooks fire for that event", but only hooks defined in the
configuration are skipped. With hook.pre-commit.enabled=false,
"git commit" still runs .git/hooks/pre-commit, and "git hook list"
shows that hook without the "event-disabled" mark.

Move the event_disabled flag out of u.configured in struct hook, set
it for the hookdir hook too, and check it for both kinds of hooks.

The test for hook.<event>.enabled=false looks only at the standard
output, but "git hook run" sends hook output to the standard error,
so the test passes even when a disabled hook runs. Add a hookdir hook
to it and check the standard error as well.

Assisted-by: Claude Fable 5.1
Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
---
 builtin/hook.c  |  6 ++++--
 hook.c          | 14 +++++++++-----
 hook.h          |  8 +++++++-
 t/t1800-hook.sh | 29 +++++++++++++++++++++++++++--
 4 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/builtin/hook.c b/builtin/hook.c
index cceeb3586e..1ea698ecc2 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -82,7 +82,9 @@ static int list(int argc, const char **argv, const char *prefix,
 
 		switch (h->kind) {
 		case HOOK_TRADITIONAL:
-			printf("%s%c", _("hook from hookdir"), line_terminator);
+			printf("%s%s%c",
+			       h->event_disabled ? "event-disabled\t" : "",
+			       _("hook from hookdir"), line_terminator);
 			break;
 		case HOOK_CONFIGURED: {
 			const char *name = h->u.configured.friendly_name;
@@ -95,7 +97,7 @@ static int list(int argc, const char **argv, const char *prefix,
 			 * "disabled" surfaces once the event is re-enabled.
 			 */
 			const char *disability =
-				h->u.configured.event_disabled ? "event-disabled\t" :
+				h->event_disabled              ? "event-disabled\t" :
 				h->u.configured.disabled       ? "disabled\t"       :
 								 "";
 			if (scope)
diff --git a/hook.c b/hook.c
index d10eef4763..5e756cb353 100644
--- a/hook.c
+++ b/hook.c
@@ -115,6 +115,9 @@ static void list_hooks_add_default(struct repository *r, const char *hookname,
 
 	h->kind = HOOK_TRADITIONAL;
 	h->u.traditional.path = xstrdup(hook_path);
+	/* list_hooks_add_configured() has loaded r->disabled_events */
+	h->event_disabled = !!unsorted_string_list_lookup(&r->disabled_events,
+							  hookname);
 
 	string_list_append(hook_list, hook_path)->util = h;
 }
@@ -514,7 +517,7 @@ static void list_hooks_add_configured(struct repository *r,
 			entry->command ? xstrdup(entry->command) : NULL;
 		hook->u.configured.scope = entry->scope;
 		hook->u.configured.disabled = entry->disabled;
-		hook->u.configured.event_disabled = event_is_disabled;
+		hook->event_disabled = event_is_disabled;
 		hook->parallel = entry->parallel;
 
 		string_list_append(list, friendly_name)->util = hook;
@@ -559,8 +562,9 @@ int hook_exists(struct repository *r, const char *name)
 
 	for (size_t i = 0; i < hooks->nr; i++) {
 		struct hook *h = hooks->items[i].util;
-		if (h->kind == HOOK_TRADITIONAL ||
-		    (!h->u.configured.disabled && !h->u.configured.event_disabled)) {
+		if (!h->event_disabled &&
+		    (h->kind == HOOK_TRADITIONAL ||
+		     !h->u.configured.disabled)) {
 			exists = 1;
 			break;
 		}
@@ -583,8 +587,8 @@ static int pick_next_hook(struct child_process *cp,
 		if (hook_cb->hook_to_run_index >= hook_list->nr)
 			return 0;
 		h = hook_list->items[hook_cb->hook_to_run_index++].util;
-	} while (h->kind == HOOK_CONFIGURED &&
-		 (h->u.configured.disabled || h->u.configured.event_disabled));
+	} while (h->event_disabled ||
+		 (h->kind == HOOK_CONFIGURED && h->u.configured.disabled));
 
 	cp->no_stdin = 1;
 	strvec_pushv(&cp->env, hook_cb->options->env.v);
diff --git a/hook.h b/hook.h
index 27bb1aeb2e..6af24cb331 100644
--- a/hook.h
+++ b/hook.h
@@ -32,10 +32,16 @@ struct hook {
 			const char *command;
 			enum config_scope scope;
 			bool disabled;
-			bool event_disabled;
 		} configured;
 	} u;
 
+	/**
+	 * Whether this hook's event is disabled with
+	 * `hook.<event>.enabled = false`. This applies to configured hooks
+	 * and the hookdir hook alike.
+	 */
+	bool event_disabled;
+
 	/**
 	 * Whether this hook may run in parallel with other hooks for the same
 	 * event. Only useful for configured (named) hooks. Traditional hooks
diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
index 7d4b2982b4..27cb0eb01c 100755
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -496,6 +496,15 @@ test_expect_success 'disabled configured hook is not reported as existing by hoo
 	test_grep ! "pre-commit" git-bugreport-hook-exists-test.txt
 '
 
+test_expect_success 'event-disabled hookdir hook is not reported as existing by hook_exists' '
+	test_when_finished "rm -f git-bugreport-hook-exists-test.txt" &&
+	setup_hookdir &&
+	test_config hook.pre-commit.enabled false &&
+
+	git bugreport -s hook-exists-test &&
+	test_grep ! "pre-commit" git-bugreport-hook-exists-test.txt
+'
+
 test_expect_success 'globally disabled hook can be re-enabled locally' '
 	test_config_global hook.global-hook.event "test-hook" &&
 	test_config_global hook.global-hook.command "echo \"global-hook ran\"" &&
@@ -1110,9 +1119,13 @@ test_expect_success 'hook.<event>.jobs rejects values less than -1' '
 test_expect_success 'hook.<event>.enabled=false skips all hooks for event' '
 	test_config hook.hook-1.event test-hook &&
 	test_config hook.hook-1.command "echo ran" &&
+	test_hook test-hook <<-\EOF &&
+	echo ran
+	EOF
 	test_config hook.test-hook.enabled false &&
 	git hook run --allow-unknown-hook-name test-hook >out 2>err &&
-	test_must_be_empty out
+	test_must_be_empty out &&
+	test_must_be_empty err
 '
 
 test_expect_success 'hook.<event>.enabled=true does not suppress hooks' '
@@ -1147,10 +1160,14 @@ test_expect_success 'git hook list shows event-disabled hooks as event-disabled'
 	test_config hook.hook-1.command "echo ran" &&
 	test_config hook.hook-2.event test-hook &&
 	test_config hook.hook-2.command "echo ran" &&
+	test_hook test-hook <<-\EOF &&
+	echo ran
+	EOF
 	test_config hook.test-hook.enabled false &&
 	git hook list --allow-unknown-hook-name test-hook >actual &&
 	test_grep "^event-disabled	hook-1$" actual &&
-	test_grep "^event-disabled	hook-2$" actual
+	test_grep "^event-disabled	hook-2$" actual &&
+	test_grep "^event-disabled	hook from hookdir$" actual
 '
 
 test_expect_success 'git hook list shows scope with event-disabled' '
@@ -1169,6 +1186,14 @@ test_expect_success 'git hook list still shows hooks when event is disabled' '
 	test_grep "event-disabled" actual
 '
 
+test_expect_success 'commit skips the hookdir hook with hook.pre-commit.enabled=false' '
+	test_hook pre-commit <<-\EOF &&
+	exit 1
+	EOF
+	test_must_fail git commit --allow-empty -m blocked &&
+	git -c hook.pre-commit.enabled=false commit --allow-empty -m allowed
+'
+
 test_expect_success 'friendly-name matching known event name is rejected' '
 	test_config hook.pre-commit.event pre-commit &&
 	test_config hook.pre-commit.command "echo oops" &&
-- 
gitgitgadget

Re: [PATCH 1/2] hook: honor hook.<event>.enabled for the hookdir hook

[Junio C Hamano]

"Thomas Bachem via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Thomas Bachem <mail@thomasbachem.com>
>
> The documentation added by dcfb5af67e (hook: add hook.<event>.enabled
> switch, 2026-04-10) says that with hook.<event>.enabled set to false
> "no hooks fire for that event", but only hooks defined in the
> configuration are skipped.

It is good to see that the original author and those who helped
were CC'ed.

I have a suspicion that the current behavior may be very much
deliberate, and the documentation was written too loosely.  In
other words, the author may have been so excited about their shiny
new toy, the configuration-defined hook mechanism, that they wrote
as if that were the only kind of hook that existed in the world,
even though that is not true.

Let's see what others want to happen.

Thanks.

Re: [PATCH 1/2] hook: honor hook.<event>.enabled for the hookdir hook

[Adrian Ratiu]

On October 20, 2026 6:46:43 PM UTC, Junio C Hamano <gitster@pobox.com> wrote:
>"Thomas Bachem via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Thomas Bachem <mail@thomasbachem.com>
>>
>> The documentation added by dcfb5af67e (hook: add hook.<event>.enabled
>> switch, 2026-04-10) says that with hook.<event>.enabled set to false
>> "no hooks fire for that event", but only hooks defined in the
>> configuration are skipped.
>
>It is good to see that the original author and those who helped
>were CC'ed.
>
>I have a suspicion that the current behavior may be very much
>deliberate, and the documentation was written too loosely.  In
>other words, the author may have been so excited about their shiny
>new toy, the configuration-defined hook mechanism, that they wrote
>as if that were the only kind of hook that existed in the world,
>even though that is not true.

The suspicion is correct: I did not intend to disable the hookdir hooks
via configs when I wrote that initially.

My reasoning was something like "configs affect just the config-defined
hooks, the traditional hookdir hooks behave the same as before". 

So currently the doc is indeed just too loose.

However, having said that, I don't have a strong opinion and am ok
either way, so if others think it's a good idea, we could accept this
patch instead of fixing the doc. :)

[PATCH 2/2] hook: accept .enabled and .parallel given without a value

[Thomas Bachem via GitGitGadget]

From: Thomas Bachem <mail@thomasbachem.com>

hook.<name>.enabled and hook.<name>.parallel are booleans, and
git-config(1) documents a boolean without a value as true. But
hook_config_lookup_all() rejects every hook.<name>.<key> that has no
value, so "git -c hook.lint.parallel hook run pre-commit" fails with
"missing value for 'hook.lint.parallel'", and so does a bare
"parallel" under [hook "lint"]. The same happens with .enabled, for a
hook as well as for an event.

Keep rejecting a missing value for the other keys. For .enabled and
.parallel, pass it to git_parse_maybe_bool(), which takes it as true.

Assisted-by: Claude Fable 5.1
Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
---
 hook.c          |  3 ++-
 t/t1800-hook.sh | 22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/hook.c b/hook.c
index 5e756cb353..c500fec33e 100644
--- a/hook.c
+++ b/hook.c
@@ -184,7 +184,8 @@ static int hook_config_lookup_all(const char *key, const char *value,
 		return 0;
 	}
 
-	if (!value)
+	/* .enabled and .parallel are bools, where a missing value means true */
+	if (!value && strcmp(subkey, "enabled") && strcmp(subkey, "parallel"))
 		return config_error_nonbool(key);
 
 	/* Extract name, ensuring it is null-terminated. */
diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
index 27cb0eb01c..223c399ef0 100755
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -516,6 +516,17 @@ test_expect_success 'globally disabled hook can be re-enabled locally' '
 	test_cmp expected actual
 '
 
+test_expect_success 'hook.<friendly-name>.enabled without a value means true' '
+	test_config_global hook.global-hook.event "test-hook" &&
+	test_config_global hook.global-hook.command "echo \"global-hook ran\"" &&
+	test_config_global hook.global-hook.enabled false &&
+
+	echo "global-hook ran" >expected &&
+	git -c hook.global-hook.enabled \
+		hook run --allow-unknown-hook-name test-hook 2>actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'configured hooks run before hookdir hook' '
 	setup_hookdir &&
 	test_config hook.first.event "pre-commit" &&
@@ -1080,6 +1091,17 @@ test_expect_success 'hook.jobs=-1 resolves to online_cpus()' '
 	test_grep "\"region_enter\".*\"hook\".*\"test-hook\".*\"max:$cpus\"" trace.txt
 '
 
+test_expect_success 'hook.<friendly-name>.parallel without a value means true' '
+	test_config hook.hook-1.event test-hook &&
+	test_config hook.hook-1.command "true" &&
+	test_config hook.jobs 2 &&
+
+	GIT_TRACE2_EVENT="$(pwd)/trace-parallel.txt" \
+		git -c hook.hook-1.parallel \
+		hook run --allow-unknown-hook-name test-hook >out 2>err &&
+	test_grep "\"region_enter\".*\"hook\".*\"test-hook\".*\"max:2\"" trace-parallel.txt
+'
+
 test_expect_success 'hook.<event>.jobs=-1 resolves to online_cpus()' '
 	test_config hook.hook-1.event test-hook &&
 	test_config hook.hook-1.command "true" &&
-- 
gitgitgadget