Git development
 help / color / mirror / Atom feed
From: "Thomas Bachem via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Adrian Ratiu <adrian.ratiu@collabora.com>,
	Patrick Steinhardt <ps@pks.im>,
	Emily Shaffer <nasamuffin@google.com>,
	Junio C Hamano <gitster@pobox.com>,
	Thomas Bachem <mail@thomasbachem.com>,
	Thomas Bachem <mail@thomasbachem.com>
Subject: [PATCH 1/2] hook: honor hook.<event>.enabled for the hookdir hook
Date: Tue, 20 Oct 2026 18:19:12 +0000	[thread overview]
Message-ID: <1a7d060fae0decbe7dda63ec4ab57e32e0205df0.1792520353.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.2272.git.1792520353.gitgitgadget@gmail.com>

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


  reply	other threads:[~2026-10-20 18:19 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2026-10-20 18:46   ` [PATCH 1/2] hook: honor hook.<event>.enabled for the hookdir hook Junio C Hamano
2026-10-20 18:19 ` [PATCH 2/2] hook: accept .enabled and .parallel given without a value Thomas Bachem via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1a7d060fae0decbe7dda63ec4ab57e32e0205df0.1792520353.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=adrian.ratiu@collabora.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=mail@thomasbachem.com \
    --cc=nasamuffin@google.com \
    --cc=ps@pks.im \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox