From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 20 Feb 2026 18:40:16 GMT
Subject: Re: [PATCH v2 5/8] hook: allow disabling config hooks
Message-ID: <aZiqkB61YXCAKEWE@pks.im>
In-Reply-To: <87qzqfwjym.fsf@collabora.com>

On Fri, Feb 20, 2026 at 04:47:13PM +0200, Adrian Ratiu wrote:
> On Fri, 20 Feb 2026, Patrick Steinhardt <ps@pks.im> wrote:
> > On Thu, Feb 19, 2026 at 12:23:49AM +0200, Adrian Ratiu wrote:
> >> diff --git a/hook.c b/hook.c
> >> index 8a9b405f76..35c24bf33d 100644
> >> --- a/hook.c
> >> +++ b/hook.c
> >> @@ -164,6 +164,21 @@ static int hook_config_lookup_all(const char *key, const char *value,
> >>  		char *old = strmap_put(&data->commands, hook_name,
> >>  				       xstrdup(value));
> >>  		free(old);
> >> +	} else if (!strcmp(subkey, "enabled")) {
> >> +		switch (git_parse_maybe_bool(value)) {
> >> +		case 0: /* disabled */
> >> +			if (!unsorted_string_list_lookup(&data->disabled_hooks,
> >> +							 hook_name))
> >> +				string_list_append(&data->disabled_hooks,
> >> +						   hook_name);
> >> +			break;
> >> +		case 1: /* enabled: undo a prior disabled entry */
> >> +			unsorted_string_list_remove(&data->disabled_hooks,
> >> +						    hook_name);
> >> +			break;
> >> +		default:
> >> +			break; /* ignore unrecognised values */
> >> +		}
> >>  	}
> >
> > Somewhat similar to my preceding questions: why don't we store the
> > enabled state in the `struct hook` structure itself? Like that we can
> > for example even list disabled hooks in `git hooks list --disabled`, if
> > we ever wanted to do something like that.
> 
> The answer to the preceeding question is unfortunately no (I tried it),
> however the answer to this question is YES.
> 
> I took the design decision to "filter" disabled hooks at config parsing
> time, however I had no idea when doing it that we might want to run
> something like "git hooks list --disabled".
> 
> I'll change this in v3 and cache the disabled config state instead of
> filtering out disabled hooks completely.
> 
> This way we could still use their information as you suggested and we
> could even enable/disable config hooks via git commands, however that is
> yet-another-new-feature and this series is big enough and should be just
> focused on adding the "base" functionality.
> 
> So in v3 I will cache the disabled state add the `git hooks list
> --disabled` feature as well, but stop there. :)

Note that the "--disabled" flag was just a way to explain why I think
it's useful to track the disabled state explicitly. I didn't mean to say
that it needs to be added :)

Patrick