Re: [PATCH v4 9/9] hook: add hook.<event>.enabled switch

From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Mar 25, 2026, 18:43 UTC
Message-ID: <87mrzvda1d.fsf@gentoo.mail-host-address-is-not-set>
In-Reply-To: <acJUfVjby_QyZvj1@pks.im>

On Tue, 24 Mar 2026, Patrick Steinhardt <ps@pks.im> wrote:

Show 29 quoted lines

> On Fri, Mar 20, 2026 at 03:53:11PM +0200, Adrian Ratiu wrote:
>> Add a hook.<event>.enabled config key that disables all hooks for
>> a given event, when set to false, acting as a high-level switch
>> above the existing per-hook hook.<friendly-name>.enabled.
>> 
>> Event-disabled hooks are shown in "git hook list" with an
>> "event-disabled" tab-separated prefix before the name:
>> 
>> $ git hook list test-hook
>> event-disabled  hook-1
>> event-disabled  hook-2
>> 
>> With --show-scope:
>> 
>> $ git hook list --show-scope test-hook
>> local   event-disabled  hook-1
>> 
>> When a hook is both per-hook disabled and event-disabled, only
>> "event-disabled" is shown: the event-level switch is the more
>> relevant piece of information, and the per-hook "disabled" status
>> will surface once the event is re-enabled.
>> 
>> Reuses is_friendly_name() from the previous commit to distinguish
>> event names from friendly-names when processing .enabled settings.
>
> I think having this makes sense in general. But what about the case
> where I have configured a hook where the friendly name matches the event
> name? Is that now forbidden, or would such a hook silently also disable
> all the other hooks?

In this current patch, if (friendly-name == event-name), then hook.*.enabled = false only disables that one hook and there's no way to disable the entire event... (see below)

Show 25 quoted lines

>> diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc
>> index d4fa29d936..0a9f04b154 100644
>> --- a/Documentation/config/hook.adoc
>> +++ b/Documentation/config/hook.adoc
>> @@ -33,6 +33,18 @@ hook.<friendly-name>.parallel::
>>  	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.<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].
>> ++
>> +Note on naming: `<event>` must be the event name (e.g. `pre-commit`),
>> +not a hook friendly-name. A name that also carries `.command`, `.event`,
>> +or `.parallel` is treated as a friendly-name and its `.enabled` value
>> +applies only to that individual hook. See `hook.<friendly-name>.enabled`
>> +above.
>
> Ah, okay, so you've thought about that already. I wonder whether this
> behaviour is okay in general or whether it is going to be confusing. An
> alternative would be to disallow configuring hooks where the event name
> matches the friendly name, which would fix the ambiguity that we now
> have.

... It is rather confusing, yes.

I think disallowing the collision, as you suggested, is the better approach, so I will do this in v5.

I think it can be done at parse time and rather easy to check because we have hook.<name>.event = <value>, where name == value results in a collision.

Or even simpler: reject if name is in hook_name_list! I already implemented this check for the `--allow-unknown-hook-name` arg you suggested in the other "cleanup" series.

Many thanks as always, your careful feedback is very valuable, Adrian