Re: [PATCH 15/22] negotiator/skipping: fix leaking commit entries

> Patrick Steinhardt <ps@pks.im> writes:
> > When releasing the skipping negotiator we free its priority queue, but
> > not the contained entries. Fix this to plug a memory leak.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  negotiator/skipping.c                | 7 +++++--
> >  t/t5552-skipping-fetch-negotiator.sh | 2 ++
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/negotiator/skipping.c b/negotiator/skipping.c
> > index f65d47858b4..b738fe4faef 100644
> > --- a/negotiator/skipping.c
> > +++ b/negotiator/skipping.c
> > @@ -247,8 +247,11 @@ static int ack(struct fetch_negotiator *n, struct commit *c)
> >  
> >  static void release(struct fetch_negotiator *n)
> >  {
> > -	clear_prio_queue(&((struct data *)n->data)->rev_list);
> > -	FREE_AND_NULL(n->data);
> > +	struct data *data = n->data;
> > +	for (int i = 0; i < data->rev_list.nr; i++)
> > +		free(data->rev_list.array[i].data);
> > +	clear_prio_queue(&data->rev_list);
> > +	FREE_AND_NULL(data);
> >  }
> >  
> 
> It seems unintuitive that clear_prio_queue() doesn't also free the data
> underneath and that a caller would have to know to free that as well to
> avoid leaking memory. Would it make more sense to add this change to
> clear_prio_queue() instead? Patch 14 has that pattern already.