All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Fix two hook configuration bugs
@ 2026-10-20 18:19 Thomas Bachem via GitGitGadget
  2026-10-20 18:19 ` [PATCH 1/2] hook: honor hook.<event>.enabled for the hookdir hook Thomas Bachem via GitGitGadget
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Thomas Bachem via GitGitGadget @ 2026-10-20 18:19 UTC (permalink / raw)
  To: git
  Cc: Adrian Ratiu, Patrick Steinhardt, Emily Shaffer, Junio C Hamano,
	Thomas Bachem

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] hook: honor hook.<event>.enabled for the hookdir hook
  2026-10-20 18:19 [PATCH 0/2] Fix two hook configuration bugs Thomas Bachem via GitGitGadget
@ 2026-10-20 18:19 ` Thomas Bachem via GitGitGadget
  2026-10-20 18:46   ` Junio C Hamano
  2026-10-20 18:19 ` [PATCH 2/2] hook: accept .enabled and .parallel given without a value Thomas Bachem via GitGitGadget
  2026-10-23 13:19 ` [PATCH v2 0/2] Two hook configuration changes Thomas Bachem via GitGitGadget
  2 siblings, 1 reply; 11+ messages in thread
From: Thomas Bachem via GitGitGadget @ 2026-10-20 18:19 UTC (permalink / raw)
  To: git
  Cc: Adrian Ratiu, Patrick Steinhardt, Emily Shaffer, Junio C Hamano,
	Thomas Bachem, Thomas Bachem

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] hook: accept .enabled and .parallel given without a value
  2026-10-20 18:19 [PATCH 0/2] Fix two hook configuration bugs Thomas Bachem via GitGitGadget
  2026-10-20 18:19 ` [PATCH 1/2] hook: honor hook.<event>.enabled for the hookdir hook Thomas Bachem via GitGitGadget
@ 2026-10-20 18:19 ` Thomas Bachem via GitGitGadget
  2026-10-23 13:19 ` [PATCH v2 0/2] Two hook configuration changes Thomas Bachem via GitGitGadget
  2 siblings, 0 replies; 11+ messages in thread
From: Thomas Bachem via GitGitGadget @ 2026-10-20 18:19 UTC (permalink / raw)
  To: git
  Cc: Adrian Ratiu, Patrick Steinhardt, Emily Shaffer, Junio C Hamano,
	Thomas Bachem, Thomas Bachem

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] hook: honor hook.<event>.enabled for the hookdir hook
  2026-10-20 18:19 ` [PATCH 1/2] hook: honor hook.<event>.enabled for the hookdir hook Thomas Bachem via GitGitGadget
@ 2026-10-20 18:46   ` Junio C Hamano
  2026-10-20 20:59     ` Adrian Ratiu
  0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2026-10-20 18:46 UTC (permalink / raw)
  To: Thomas Bachem via GitGitGadget
  Cc: git, Adrian Ratiu, Patrick Steinhardt, Emily Shaffer,
	Thomas Bachem

"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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] hook: honor hook.<event>.enabled for the hookdir hook
  2026-10-20 18:46   ` Junio C Hamano
@ 2026-10-20 20:59     ` Adrian Ratiu
  2026-10-21 17:42       ` Junio C Hamano
  0 siblings, 1 reply; 11+ messages in thread
From: Adrian Ratiu @ 2026-10-20 20:59 UTC (permalink / raw)
  To: Junio C Hamano, Thomas Bachem via GitGitGadget
  Cc: git, Patrick Steinhardt, Emily Shaffer, Thomas Bachem


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. :)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] hook: honor hook.<event>.enabled for the hookdir hook
  2026-10-20 20:59     ` Adrian Ratiu
@ 2026-10-21 17:42       ` Junio C Hamano
  2026-10-22 13:40         ` Adrian Ratiu
  0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2026-10-21 17:42 UTC (permalink / raw)
  To: Adrian Ratiu
  Cc: Thomas Bachem via GitGitGadget, git, Patrick Steinhardt,
	Emily Shaffer, Thomas Bachem

Adrian Ratiu <adrian.ratiu@collabora.com> writes:

> 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.

Thanks for confirming my suspicion.  After re-reading the document,
I am not sure if it was overly loose (see below).

> 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. :)

I am undecided if we should treat it as a backward incompatible
change or if we should just label this change as a "bugfix".

The additional documentation in question said

    hook.<event>.enabled::
       Switch to enable or disable all hooks for the `<event>` hook event.
       When set to `false`, no hooks fire for that event, regardless of any
       per-hook `hook.<friendly-name>.enabled` settings. Defaults to `true`.
       See linkgit:git-hook[1].

which talks about "hook.<friendly-name>.enabled" setting, hinting
that this is also about the hooks that can be controlled by that
other setting, which was added by 1ecce722cd (hook: allow disabling
config hooks, 2026-02-19), which added "diable" specifically for
"config hooks", and the documentation from that addition says:

    hook.<friendly-name>.enabled::
	Whether the hook `hook.<name>` is enabled. Defaults to `true`.
	Set to `false` to disable the hook without removing its
	configuration. This is particularly useful when a hook is defined
	in a system or global config file and needs to be disabled for a
	specific repository. See linkgit:git-hook[1].

mentioning "removing its configuration", and "defined in ... config
file", to me, it is very clear that this configuration variable is
only about the hooks defined in the configuration system.

Which tempts me to say that the change being discussed is a backward
incompatible breakage, rather than a bugfix.

But casual end-users would of course do not know how the feature
evolved, and may not read the documentation of the feature they use
carefully enough.  So I dunno.  Perhaps changing the behaviour at
the same time updating the documentation to make it clear these
cover all kinds of hooks, not just the configuration-defined ones,
may be sufficient.

Thanks.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] hook: honor hook.<event>.enabled for the hookdir hook
  2026-10-21 17:42       ` Junio C Hamano
@ 2026-10-22 13:40         ` Adrian Ratiu
  0 siblings, 0 replies; 11+ messages in thread
From: Adrian Ratiu @ 2026-10-22 13:40 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Bachem via GitGitGadget, git, Patrick Steinhardt,
	Emily Shaffer, Thomas Bachem

On Wed, 21 Oct 2026, Junio C Hamano <gitster@pobox.com> wrote:
> Adrian Ratiu <adrian.ratiu@collabora.com> writes:
>> 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. :)
>
> I am undecided if we should treat it as a backward incompatible
> change or if we should just label this change as a "bugfix".
>
> The additional documentation in question said
>
>     hook.<event>.enabled::
>        Switch to enable or disable all hooks for the `<event>` hook event.
>        When set to `false`, no hooks fire for that event, regardless of any
>        per-hook `hook.<friendly-name>.enabled` settings. Defaults to `true`.
>        See linkgit:git-hook[1].
>
> which talks about "hook.<friendly-name>.enabled" setting, hinting
> that this is also about the hooks that can be controlled by that
> other setting, which was added by 1ecce722cd (hook: allow disabling
> config hooks, 2026-02-19), which added "diable" specifically for
> "config hooks", and the documentation from that addition says:
>
>     hook.<friendly-name>.enabled::
> 	Whether the hook `hook.<name>` is enabled. Defaults to `true`.
> 	Set to `false` to disable the hook without removing its
> 	configuration. This is particularly useful when a hook is defined
> 	in a system or global config file and needs to be disabled for a
> 	specific repository. See linkgit:git-hook[1].
>
> mentioning "removing its configuration", and "defined in ... config
> file", to me, it is very clear that this configuration variable is
> only about the hooks defined in the configuration system.
>
> Which tempts me to say that the change being discussed is a backward
> incompatible breakage, rather than a bugfix.
>
> But casual end-users would of course do not know how the feature
> evolved, and may not read the documentation of the feature they use
> carefully enough.  So I dunno.  Perhaps changing the behaviour at
> the same time updating the documentation to make it clear these
> cover all kinds of hooks, not just the configuration-defined ones,
> may be sufficient.

Agreed, let's keep it simple.

@Thomas, can you please send a v2 to update the documentation to make it
clear that disabling the hook event disables both the config-defined
hooks and the hookdir hooks for that event?

Just a short mention like you specified in hook.h, to avoid any
ambiguity.

Many thanks,
Adrian

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 0/2] Two hook configuration changes
  2026-10-20 18:19 [PATCH 0/2] Fix two hook configuration bugs Thomas Bachem via GitGitGadget
  2026-10-20 18:19 ` [PATCH 1/2] hook: honor hook.<event>.enabled for the hookdir hook Thomas Bachem via GitGitGadget
  2026-10-20 18:19 ` [PATCH 2/2] hook: accept .enabled and .parallel given without a value Thomas Bachem via GitGitGadget
@ 2026-10-23 13:19 ` Thomas Bachem via GitGitGadget
  2026-10-23 13:19   ` [PATCH v2 1/2] hook: make hook.<event>.enabled cover the hookdir hook Thomas Bachem via GitGitGadget
                     ` (2 more replies)
  2 siblings, 3 replies; 11+ messages in thread
From: Thomas Bachem via GitGitGadget @ 2026-10-23 13:19 UTC (permalink / raw)
  To: git
  Cc: Adrian Ratiu, Patrick Steinhardt, Emily Shaffer, Junio C Hamano,
	Thomas Bachem

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

Changes since v1:

 * The documentation of hook.<event>.enabled now says that it covers
   configured hooks and the hook in the hooks directory alike. The subject
   and message of patch 1 present this as a change in behavior rather than a
   fix (Adrian, Junio).

 * The test that hook.<event>.enabled=false leaves the hooks of other events
   alone now has a hookdir hook too.

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

 Documentation/config/hook.adoc |  3 +-
 builtin/hook.c                 |  6 ++--
 hook.c                         | 17 ++++++----
 hook.h                         |  8 ++++-
 t/t1800-hook.sh                | 57 ++++++++++++++++++++++++++++++++--
 5 files changed, 78 insertions(+), 13 deletions(-)


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

Range-diff vs v1:

 1:  1a7d060fae ! 1:  cfcbe80ba5 hook: honor hook.<event>.enabled for the hookdir hook
     @@ Metadata
      Author: Thomas Bachem <mail@thomasbachem.com>
      
       ## Commit message ##
     -    hook: honor hook.<event>.enabled for the hookdir hook
     +    hook: make hook.<event>.enabled cover the hookdir hook
      
     -    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.
     +    hook.<event>.enabled, added by dcfb5af67e (hook: add
     +    hook.<event>.enabled switch, 2026-04-10), skips only the hooks
     +    defined in the configuration when set to false. 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.
     +    That was the intent, but the documentation says "no hooks fire for
     +    that event", which reads as covering the hookdir hook too. No other
     +    setting turns off the hookdir hook of a single event either:
     +    core.hooksPath=/dev/null turns off the hookdir hooks of all events.
     +    Make the switch cover the hookdir hook as well, and say so in the
     +    documentation. This changes behavior for anyone who set the switch
     +    and relied on the hookdir hook still running.
      
          The test for hook.<event>.enabled=false looks only at the standard
          output, but "git hook run" sends hook output to the standard error,
     @@ Commit message
          Assisted-by: Claude Fable 5.1
          Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
      
     + ## Documentation/config/hook.adoc ##
     +@@ Documentation/config/hook.adoc: hook.<friendly-name>.parallel::
     + 	the effective job count is greater than 1. {see-git-hook}
     + 
     + hook.<event>.enabled::
     +-	Switch to enable or disable all hooks for the `<event>` hook event.
     ++	Switch to enable or disable all hooks for the `<event>` hook event,
     ++	configured hooks and the traditional hook in the hooks directory alike.
     + 	When set to `false`, no hooks fire for that event, regardless of any
     + 	per-hook `hook.<friendly-name>.enabled` settings. Defaults to `true`.
     + 	{see-git-hook}
     +
       ## builtin/hook.c ##
      @@ builtin/hook.c: static int list(int argc, const char **argv, const char *prefix,
       
     @@ t/t1800-hook.sh: test_expect_success 'hook.<event>.jobs rejects values less than
       '
       
       test_expect_success 'hook.<event>.enabled=true does not suppress hooks' '
     +@@ t/t1800-hook.sh: test_expect_success 'hook.<event>.enabled=true does not suppress hooks' '
     + test_expect_success 'hook.<event>.enabled=false does not affect other events' '
     + 	test_config hook.hook-1.event test-hook &&
     + 	test_config hook.hook-1.command "echo ran" &&
     ++	test_hook test-hook <<-\EOF &&
     ++	echo hookdir ran
     ++	EOF
     + 	test_config hook.other-event.enabled false &&
     + 	git hook run --allow-unknown-hook-name test-hook >out 2>err &&
     +-	test_grep "ran" err
     ++	test_grep "^ran$" err &&
     ++	test_grep "^hookdir ran$" err
     + '
     + 
     + test_expect_success 'hook.<friendly-name>.enabled=false still disables that hook' '
      @@ t/t1800-hook.sh: 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 &&
 2:  3db167c9d1 = 2:  53a5a1787a hook: accept .enabled and .parallel given without a value

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 1/2] hook: make hook.<event>.enabled cover the hookdir hook
  2026-10-23 13:19 ` [PATCH v2 0/2] Two hook configuration changes Thomas Bachem via GitGitGadget
@ 2026-10-23 13:19   ` Thomas Bachem via GitGitGadget
  2026-10-23 13:19   ` [PATCH v2 2/2] hook: accept .enabled and .parallel given without a value Thomas Bachem via GitGitGadget
  2026-10-23 17:53   ` [PATCH v2 0/2] Two hook configuration changes Junio C Hamano
  2 siblings, 0 replies; 11+ messages in thread
From: Thomas Bachem via GitGitGadget @ 2026-10-23 13:19 UTC (permalink / raw)
  To: git
  Cc: Adrian Ratiu, Patrick Steinhardt, Emily Shaffer, Junio C Hamano,
	Thomas Bachem, Thomas Bachem

From: Thomas Bachem <mail@thomasbachem.com>

hook.<event>.enabled, added by dcfb5af67e (hook: add
hook.<event>.enabled switch, 2026-04-10), skips only the hooks
defined in the configuration when set to false. 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.

That was the intent, but the documentation says "no hooks fire for
that event", which reads as covering the hookdir hook too. No other
setting turns off the hookdir hook of a single event either:
core.hooksPath=/dev/null turns off the hookdir hooks of all events.
Make the switch cover the hookdir hook as well, and say so in the
documentation. This changes behavior for anyone who set the switch
and relied on the hookdir hook still running.

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>
---
 Documentation/config/hook.adoc |  3 ++-
 builtin/hook.c                 |  6 ++++--
 hook.c                         | 14 +++++++++-----
 hook.h                         |  8 +++++++-
 t/t1800-hook.sh                | 35 +++++++++++++++++++++++++++++++---
 5 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc
index 083dc60a13..332657e70f 100644
--- a/Documentation/config/hook.adoc
+++ b/Documentation/config/hook.adoc
@@ -47,7 +47,8 @@ hook.<friendly-name>.parallel::
 	the effective job count is greater than 1. {see-git-hook}
 
 hook.<event>.enabled::
-	Switch to enable or disable all hooks for the `<event>` hook event.
+	Switch to enable or disable all hooks for the `<event>` hook event,
+	configured hooks and the traditional hook in the hooks directory alike.
 	When set to `false`, no hooks fire for that event, regardless of any
 	per-hook `hook.<friendly-name>.enabled` settings. Defaults to `true`.
 	{see-git-hook}
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..45b99a7f04 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' '
@@ -1126,9 +1139,13 @@ test_expect_success 'hook.<event>.enabled=true does not suppress hooks' '
 test_expect_success 'hook.<event>.enabled=false does not affect other events' '
 	test_config hook.hook-1.event test-hook &&
 	test_config hook.hook-1.command "echo ran" &&
+	test_hook test-hook <<-\EOF &&
+	echo hookdir ran
+	EOF
 	test_config hook.other-event.enabled false &&
 	git hook run --allow-unknown-hook-name test-hook >out 2>err &&
-	test_grep "ran" err
+	test_grep "^ran$" err &&
+	test_grep "^hookdir ran$" err
 '
 
 test_expect_success 'hook.<friendly-name>.enabled=false still disables that hook' '
@@ -1147,10 +1164,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 +1190,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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 2/2] hook: accept .enabled and .parallel given without a value
  2026-10-23 13:19 ` [PATCH v2 0/2] Two hook configuration changes Thomas Bachem via GitGitGadget
  2026-10-23 13:19   ` [PATCH v2 1/2] hook: make hook.<event>.enabled cover the hookdir hook Thomas Bachem via GitGitGadget
@ 2026-10-23 13:19   ` Thomas Bachem via GitGitGadget
  2026-10-23 17:53   ` [PATCH v2 0/2] Two hook configuration changes Junio C Hamano
  2 siblings, 0 replies; 11+ messages in thread
From: Thomas Bachem via GitGitGadget @ 2026-10-23 13:19 UTC (permalink / raw)
  To: git
  Cc: Adrian Ratiu, Patrick Steinhardt, Emily Shaffer, Junio C Hamano,
	Thomas Bachem, Thomas Bachem

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 45b99a7f04..fca4cfc712 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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 0/2] Two hook configuration changes
  2026-10-23 13:19 ` [PATCH v2 0/2] Two hook configuration changes Thomas Bachem via GitGitGadget
  2026-10-23 13:19   ` [PATCH v2 1/2] hook: make hook.<event>.enabled cover the hookdir hook Thomas Bachem via GitGitGadget
  2026-10-23 13:19   ` [PATCH v2 2/2] hook: accept .enabled and .parallel given without a value Thomas Bachem via GitGitGadget
@ 2026-10-23 17:53   ` Junio C Hamano
  2 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2026-10-23 17:53 UTC (permalink / raw)
  To: Thomas Bachem via GitGitGadget
  Cc: git, Adrian Ratiu, Patrick Steinhardt, Emily Shaffer,
	Thomas Bachem

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

> Two small changes to hook configuration. With the first,
> hook.pre-commit.enabled=false also stops the pre-commit hook in the hooks
> directory. With the second, hook.*.enabled and hook.*.parallel can be given
> without a value, which means true, as for any other boolean.
>
> Changes since v1:
>
>  * The documentation of hook.<event>.enabled now says that it covers
>    configured hooks and the hook in the hooks directory alike. The subject
>    and message of patch 1 present this as a change in behavior rather than a
>    fix (Adrian, Junio).
>
>  * The test that hook.<event>.enabled=false leaves the hooks of other events
>    alone now has a hookdir hook too.
>
> Thomas Bachem (2):
>   hook: make hook.<event>.enabled cover the hookdir hook
>   hook: accept .enabled and .parallel given without a value

Thanks, the range-diff looks good.  Will queue.


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2026-10-23 17:53 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-20 18:19 [PATCH 0/2] Fix two hook configuration bugs Thomas Bachem via GitGitGadget
2026-10-20 18:19 ` [PATCH 1/2] hook: honor hook.<event>.enabled for the hookdir hook Thomas Bachem via GitGitGadget
2026-10-20 18:46   ` Junio C Hamano
2026-10-20 20:59     ` Adrian Ratiu
2026-10-21 17:42       ` Junio C Hamano
2026-10-22 13:40         ` Adrian Ratiu
2026-10-20 18:19 ` [PATCH 2/2] hook: accept .enabled and .parallel given without a value Thomas Bachem via GitGitGadget
2026-10-23 13:19 ` [PATCH v2 0/2] Two hook configuration changes Thomas Bachem via GitGitGadget
2026-10-23 13:19   ` [PATCH v2 1/2] hook: make hook.<event>.enabled cover the hookdir hook Thomas Bachem via GitGitGadget
2026-10-23 13:19   ` [PATCH v2 2/2] hook: accept .enabled and .parallel given without a value Thomas Bachem via GitGitGadget
2026-10-23 17:53   ` [PATCH v2 0/2] Two hook configuration changes Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.