Re: [PATCH v2 05/10] hook: replace hook_list_clear() -> string_list_clear_func()

From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Mar 24, 2026, 22:33 UTC
Message-ID: <87bjgcdfim.fsf@collabora.com>
In-Reply-To: <acJNYPgOSO86hZYq@pks.im>

On Tue, 24 Mar 2026, Patrick Steinhardt <ps@pks.im> wrote:

Show 23 quoted lines

> On Fri, Mar 20, 2026 at 01:52:06PM +0200, Adrian Ratiu wrote:
>> diff --git a/hook.c b/hook.c
>> index 6dfaa7e9b1..f6bb1999ae 100644
>> --- a/hook.c
>> +++ b/hook.c
>> @@ -52,8 +52,14 @@ const char *find_hook(struct repository *r, const char *name)
>>  	return path.buf;
>>  }
>>  
>> -static void hook_clear(struct hook *h, hook_data_free_fn cb_data_free)
>> +/*
>> + * Frees a struct hook stored as the util pointer of a string_list_item.
>> + * Suitable for use as a string_list_clear_func_t callback.
>> + */
>
> This comment should probably live in the header. I also wonder whether
> this wrapper isn't a bit too specific to freeing hooks with a string
> list. Maybe it would be preferable to expose a "proper" `hook_free()`
> function that only takes a hook, and then provide a small wrapper
> function for freeing in the string list?
>
> If so it feels like we're going a bit full circle though. Maybe the
> original code wasn't all that bad in the first place?

I prefer this new design (suggested by you), because:

1. We use the generic string_list_clear_func() API.

2. Each struct hook owns its data_free callback, which means that callers
don't need to keep track of internal state (e.g. when to pass NULL to
skip cleanup).

3. The hook API itself is cleaner for hook.[ch] users, because it's
always string_list_clear_func(head, hook_free); regardless of context.

So if it's ok with you, let's use your new design. :)

I'll move the comment to the header (it's already there, I just forgot to remove the duplicated comment in hook.c above the function definition).