Patrick Steinhardt <ps@xxxxxx> 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@xxxxxx>
> ---
> 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.
Thanks again for the cleanups -- I'm tempted to take a stab at some of
the other memory leaks you mentioned during our biweekly hackathon. All
of the other patches look reasonable to me.
