Skip to content

Two hook configuration changes #2272

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
thomasbachem wants to merge 2 commits into
base: master
Choose a base branch
from
+78 −13
Open

Two hook configuration changes #2272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cfcbe80
hook: make hook.<event>.enabled cover the hookdir hook
thomasbachem Oct 12, 2026
53a5a17
hook: accept .enabled and .parallel given without a value
thomasbachem Oct 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Documentation/config/hook.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down
6 changes: 4 additions & 2 deletions builtin/hook.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,9 @@ static int list(int argc, const char **argv, const char *prefix,

@gitgitgadget gitgitgadget Bot Oct 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

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

@gitgitgadget gitgitgadget Bot Oct 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adrian Ratiu wrote on the Git mailing list (how to reply to this email):

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

@gitgitgadget gitgitgadget Bot Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

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.

@gitgitgadget gitgitgadget Bot Oct 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adrian Ratiu wrote on the Git mailing list (how to reply to this email):

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

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;
Expand All @@ -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)
Expand Down
17 changes: 11 additions & 6 deletions hook.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -181,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. */
Expand Down Expand Up @@ -514,7 +518,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;
Expand Down Expand Up @@ -559,8 +563,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;
}
Expand All @@ -583,8 +588,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);
Expand Down
8 changes: 7 additions & 1 deletion hook.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
57 changes: 54 additions & 3 deletions t/t1800-hook.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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\"" &&
Expand All @@ -507,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" &&
Expand Down Expand Up @@ -1071,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" &&
Expand Down Expand Up @@ -1110,9 +1141,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' '
Expand All @@ -1126,9 +1161,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' '
Expand All @@ -1147,10 +1186,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' '
Expand All @@ -1169,6 +1212,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" &&
Expand Down
Loading