From: Adrian Ratiu Date: Wed, 25 Mar 2026 18:43:58 GMT Subject: Re: [PATCH v4 9/9] hook: add hook..enabled switch Message-ID: <87mrzvda1d.fsf@gentoo.mail-host-address-is-not-set> In-Reply-To: On Tue, 24 Mar 2026, Patrick Steinhardt wrote: > On Fri, Mar 20, 2026 at 03:53:11PM +0200, Adrian Ratiu wrote: >> Add a hook..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..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) >> 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..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..enabled:: >> + Switch to enable or disable all hooks for the `` hook event. >> + When set to `false`, no hooks fire for that event, regardless of any >> + per-hook `hook..enabled` settings. Defaults to `true`. >> + See linkgit:git-hook[1]. >> ++ >> +Note on naming: `` 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..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..event = , 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