From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Fri, 20 Feb 2026 12:27:58 GMT
Subject: Re: [PATCH v2 4/8] hook: include hooks from the config
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:
> 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.

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

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

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