From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 25 Mar 2026 05:26:02 GMT
Subject: Re: [PATCH v2 05/10] hook: replace hook_list_clear() -> string_list_clear_func()
Message-ID: <acNx6pDjOMQaMsBr@pks.im>
In-Reply-To: <87bjgcdfim.fsf@collabora.com>

On Wed, Mar 25, 2026 at 12:33:21AM +0200, Adrian Ratiu wrote:
> On Tue, 24 Mar 2026, Patrick Steinhardt <ps@pks.im> wrote:
> > 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).

Fine with me, thanks!

Patrick