From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 04 Mar 2026 06:57:51 GMT
Subject: Re: [PATCH v2 5/5] sequencer: use oidmap_clear_with_free() for string_entry cleanup
Message-ID: <aafX7_BqIYDfXQtN@pks.im>
In-Reply-To: <xmqqwlztx3f7.fsf@gitster.g>

On Mon, Mar 02, 2026 at 02:38:36PM -0800, Junio C Hamano wrote:
> Seyi Kuforiji <kuforiji98@gmail.com> writes:
> 
> > From: Seyi Kufoiji <kuforiji98@gmail.com>
> >
> > Switch cleanup of the string_entry oidmap to
> > oidmap_clear_with_free() and introduce a free_string_entry()
> > helper to properly free each allocated struct string_entry.
> >
> > This aligns with the ongoing migration to use the callback-based
> > oidmap cleanup API.
> >
> > Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
> > ---
> >  sequencer.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index a3eb39bb25..75ef2ace4f 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -5654,6 +5654,12 @@ struct string_entry {
> >  	char string[FLEX_ARRAY];
> >  };
> >  
> > +static void free_string_entry(void *e)
> > +{
> > +	struct string_entry *entry = container_of(e, struct string_entry, entry);
> > +	free(entry);
> > +}
> 
> Exactly the same comment applies to this step as [PATCH v2 3/5].
> 
> In other words, with the current codebase, these three steps in the
> context of the current code are uninteresting with little value, but
> if we ever add a member to these entries that hold their own
> resources, it would become easier to manage the lifetime rules of
> them.

Personally I'd lean towards keeping the first two patches and drop the
remaining ones though. I think it makes the code harder to understand to
convert all callsites of `oidmap_clear()`, even if it doesn't actually
provide a benefit.

We can still convert callsites to use `oidmap_clear_with_free()` in case
they grow additional allocations per entry that we'll have to care
about.

Thanks!

Patrick