Re: [PATCH v2 4/8] hook: include hooks from the config

From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Feb 20, 2026, 12:27 UTC
Message-ID: <87o6ljy4z5.fsf@gentoo.mail-host-address-is-not-set>
In-Reply-To: <xmqqy0ko1iri.fsf@gitster.g>

On Thu, 19 Feb 2026, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

> Adrian Ratiu <adrian.ratiu@collabora.com> writes:
>
>> Examples:
>>
>>   $ git config --get-regexp "^hook\."
>>   hook.bar.command=~/bar.sh
>>   hook.bar.event=pre-commit
>
> This is all good when you know where you defined your pre-commit
> hook, but you would want to know in which scope the configuration is
> made, wouldn't you, especially when you are trying to diagnose why
> some command that you do not necessarily recognise when you run a
> Git command?

Yes, I assumed the user knows where his hooks are defined. :) Obviously this is not the case.

This also applies to the "git hook list" command added later in this series, we might want to tell users where the hooks come from there as well.

Show 9 quoted lines

>> @@ -10,17 +11,22 @@ struct repository;
>>   * Represents a hook command to be run.
>>   * Hooks can be:
>>   * 1. "traditional" (found in the hooks directory)
>> - * 2. "configured" (defined in Git's configuration, not yet implemented).
>> + * 2. "configured" (defined in Git's configuration via hook.<name>.event).
>
> Wouldn't it be easier to understand if we do "<name>" -> "<friendly-name>"
> to match the member name used in the struct below?

Yes, name here referes to "friendly-name" :)

I'll make this consestent in v3 across the patch series, to use friendly-name.

Show 18 quoted lines

>>   * The 'kind' field determines which part of the union 'u' is valid.
>>   */
>>  struct hook {
>>  	enum {
>>  		HOOK_TRADITIONAL,
>> +		HOOK_CONFIGURED,
>>  	} kind;
>>  	union {
>>  		struct {
>>  			const char *path;
>>  		} traditional;
>> +		struct {
>> +			const char *friendly_name;
>> +			const char *command;
>
> If we wanted to report which config scope defined a particular hook
> we need to record where the configured hook came from in this
> struct, right?

Yes, this is the place.

We can do it when parsing the configs (in v2, at Patrick's suggestion we just parse once then cache & reuse if possible).

Show 13 quoted lines

>
>> +		} configured;
>>  	} u;
>
>> +	if (repo->hook_config_cache) {
>> +		hook_cache_clear(repo->hook_config_cache);
>> +		FREE_AND_NULL(repo->hook_config_cache);
>> +	}
>
> It is a minor point, but after applying the hole series, there are
> only two calls to hook_cachje_clear(X) and they both are followed by
> free(X) or FREE_AND_NULL(X).  I wonder if we simply want
> hook_cache_free() instead of _clear()?

Yes, we can do this.

I think I have another place where I can apply this pattern in addition to this, to avoid calling both clear() + free() afterwards.

Will make all these uses consistent on a single _free() in v3.

Thanks, Adrian