From: Adrian Ratiu Date: Wed, 11 Mar 2026 12:24:14 GMT Subject: Re: [PATCH 10/10] hook: show disabled hooks in "git hook list" Message-ID: <87eclqa70x.fsf@collabora.com> In-Reply-To: On Wed, 11 Mar 2026, Patrick Steinhardt wrote: > On Mon, Mar 09, 2026 at 02:54:16AM +0200, Adrian Ratiu wrote: >> diff --git a/builtin/hook.c b/builtin/hook.c >> index c806640361..ff446948fa 100644 >> --- a/builtin/hook.c >> +++ b/builtin/hook.c >> @@ -72,16 +72,20 @@ static int list(int argc, const char **argv, const char *prefix, >> case HOOK_TRADITIONAL: >> printf("%s%c", _("hook from hookdir"), line_terminator); >> break; >> - case HOOK_CONFIGURED: >> - if (show_scope) >> - printf("%s (%s)%c", >> - h->u.configured.friendly_name, >> - config_scope_name(h->u.configured.scope), >> + case HOOK_CONFIGURED: { >> + const char *name = h->u.configured.friendly_name; >> + const char *scope = show_scope ? >> + config_scope_name(h->u.configured.scope) : NULL; >> + if (scope) >> + printf("%s (%s%s)%c", name, scope, >> + h->u.configured.disabled ? ", disabled" : "", >> line_terminator); >> + else if (h->u.configured.disabled) >> + printf("%s (disabled)%c", name, line_terminator); >> else >> - printf("%s%c", h->u.configured.friendly_name, >> - line_terminator); >> + printf("%s%c", name, line_terminator); >> break; >> + } >> default: >> BUG("unknown hook kind"); >> } > > Hm. This starts to feel less and less like an interface that can easily > be parsed by a machine, even with "-z". I guess this partly comes from > our insistence to reinvent the wheel in Git instead of just using > something like JSON :/ Yes, I agree, a structured output format like JSON would be ideal in this case. Please see my previous patch suggestion of mirroring the existing git config --show-scope by using tab separated prefixes. Maybe we could do that here as well. Suggestions welcome. >> diff --git a/hook.c b/hook.c >> index 2c03baeaac..4f4f060156 100644 >> --- a/hook.c >> +++ b/hook.c >> @@ -119,6 +119,7 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, >> struct hook_config_cache_entry { >> char *command; >> enum config_scope scope; >> + int disabled; >> }; >> >> /* > > Is there any reason this is an `int` and not a `bool`? No, I'll change it to bool in v2, it was just an oversight on my part. >> @@ -217,8 +218,10 @@ static int hook_config_lookup_all(const char *key, const char *value, >> * every item's string is the hook's friendly-name and its util pointer is >> * a hook_config_cache_entry. All strings are owned by the map. >> * >> - * Disabled hooks and hooks missing a command are already filtered out at >> - * parse time, so callers can iterate the list directly. >> + * Disabled hooks are kept in the cache with entry->disabled set, so that >> + * "git hook list" can display them. Hooks missing a command are filtered >> + * out at build time; if a disabled hook has no command it is silently > > What exactly does "build time" refer to? To me this reads like invoking > make :) This is one of my pet peeves: I (mis)use "build time" a lot. build time in this case == cache construction time. :) The comment is wrong anyway, because in the end I decided to issue a warning (see code immediately below) instead of silently ignoring. I'll reword this in v2. Thanks! >> + * skipped rather than triggering a fatal error. >> */ >> void hook_cache_clear(struct hook_config_cache *cache) >> { >> @@ -268,21 +271,26 @@ static void build_hook_config_map(struct repository *r, >> struct hook_config_cache_entry *entry; >> char *command; >> >> - /* filter out disabled hooks */ >> - if (unsorted_string_list_lookup(&cb_data.disabled_hooks, >> - hname)) >> - continue; >> + int is_disabled = >> + !!unsorted_string_list_lookup( >> + &cb_data.disabled_hooks, hname); >> >> command = strmap_get(&cb_data.commands, hname); >> - if (!command) >> - die(_("'hook.%s.command' must be configured or " >> - "'hook.%s.event' must be removed;" >> - " aborting."), hname, hname); >> + if (!command) { >> + if (is_disabled) >> + warning(_("disabled hook '%s' has no " >> + "command configured"), hname); >> + else >> + die(_("'hook.%s.command' must be configured or " >> + "'hook.%s.event' must be removed;" >> + " aborting."), hname, hname); >> + } >> >> /* util stores a cache entry; owned by the cache. */ >> CALLOC_ARRAY(entry, 1); >> - entry->command = xstrdup(command); >> + entry->command = command ? xstrdup(command) : NULL; > > You can use `xstrdup_or_null()` here. Ack, will do in v2. >> entry->scope = scope; >> + entry->disabled = is_disabled; >> string_list_append(hooks, hname)->util = entry; >> } >> >> @@ -401,7 +411,16 @@ struct string_list *list_hooks(struct repository *r, const char *hookname, >> int hook_exists(struct repository *r, const char *name) >> { >> struct string_list *hooks = list_hooks(r, name, NULL); >> - int exists = hooks->nr > 0; >> + int exists = 0; >> + >> + 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) { > > Is the first condition required? I would expect that `disabled` would > always be false for traditional hooks. Yes, it is required because !h->u.configured.disabled only applies to non-traditional (configured) hooks. The "traditional" member of the union doesn't even have a disabled field, when h->kind == HOOK_TRADITIONAL => always exists = 1; Basically we're checking two different types here, if they exist and short-circuiting in the traditional hook case. Hope that explanation makes sense. I'll see if I can make this condition clearer in v2. >> + exists = 1; >> + break; >> + } >> + } >> string_list_clear_func(hooks, hook_free); >> free(hooks); >> return exists; >> diff --git a/hook.h b/hook.h >> index 0d711ed21a..0432df963f 100644 >> --- a/hook.h >> +++ b/hook.h >> @@ -31,6 +31,7 @@ struct hook { >> const char *friendly_name; >> const char *command; >> enum config_scope scope; >> + int disabled; >> } configured; >> } u; >> > > Same question here regarding the type of the struct member. Yes, it can be a bool. Will do in v2.