Re: [PATCH 10/10] hook: show disabled hooks in "git hook list"
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Mar 11, 2026, 10:24 UTC
- Message-ID
- <abFC6pEEmicjG6a9@pks.im>
- In-Reply-To
- <20260309005416.2760030-11-adrian.ratiu@collabora.com>
On Mon, Mar 09, 2026 at 02:54:16AM +0200, Adrian Ratiu wrote:
Show 32 quoted lines
> 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 :/
Show 12 quoted lines
> 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`?
Show 9 quoted lines
> @@ -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 :)
Show 35 quoted lines
> + * 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.
Show 16 quoted lines
> 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.
Show 19 quoted lines
> + 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.
Patrick