From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 11 Mar 2026 10:24:42 GMT
Subject: Re: [PATCH 10/10] hook: show disabled hooks in "git hook list"
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:
> 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 :/

> 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`?

> @@ -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 :)

> + * 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.

>  			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.

> +			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