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

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