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

[Junio C Hamano <gitster@pobox.com>]

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

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