Re: [PATCH 08/10] hook: refactor hook_config_cache from strmap to named struct

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 9, 2026, 21:59 UTC
Message-ID: <xmqqtsuo3bru.fsf@gitster.g>
In-Reply-To: <20260309005416.2760030-9-adrian.ratiu@collabora.com>

Adrian Ratiu <adrian.ratiu@collabora.com> writes:

Show 17 quoted lines

> Replace the raw `struct strmap *hook_config_cache` in `struct repository`
> with a `struct hook_config_cache` which wraps the strmap in a named field.
>
> Replace the bare `char *command` util pointer stored in each string_list
> item with a heap-allocated `struct hook_config_cache_entry` that carries
> that command string.
>
> This is just a refactoring with no behavior changes, to give the cache
> struct room to grow so it can carry the additional hook metadata we'll
> be adding in the following commits.
>
> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
>  hook.c       | 61 +++++++++++++++++++++++++++++++++-------------------
>  hook.h       | 10 ++++++++-
>  repository.h |  3 ++-
>  3 files changed, 50 insertions(+), 24 deletions(-)

Hmph, so originally we pointed directly at a "struct strmap" instance from inside a "struct repository", and the strmap mapped a hook's event name to the command to run, which is a raw "char *".

In order to future expand the attribute we keep track of for each hook beyond a simple string, we replace the "char *" with a pointer to "struct hook_config_cache_entry". That part makes quite a lot of sense.

However, I'm less convinced about the value of wrapping the `strmap` itself in a new `struct hook_config_cache`. Since the "growth" in later patches happens entirely within the entry struct, this top-level wrapper feels like unnecessary boilerplate that adds an extra layer of indirection without a clear benefit. It might be better to keep the cache as a bare `strmap` and only introduce the entry struct.