Re: [PATCH 1/3] is_promisor_object(): free tree buffer after parsing

[Jeff King <peff@peff.net>]

On Tue, Apr 13, 2021 at 01:17:55PM -0700, Junio C Hamano wrote:

> > diff --git a/packfile.c b/packfile.c
> > index 8668345d93..b79cbc8cd4 100644
> > --- a/packfile.c
> > +++ b/packfile.c
> > @@ -2247,6 +2247,7 @@ static int add_promisor_object(const struct object_id *oid,
> >  			return 0;
> >  		while (tree_entry_gently(&desc, &entry))
> >  			oidset_insert(set, &entry.oid);
> > +		free_tree_buffer(tree);
> >  	} else if (obj->type == OBJ_COMMIT) {
> >  		struct commit *commit = (struct commit *) obj;
> >  		struct commit_list *parents = commit->parents;
> 
> Hmph, does an added free() without removing one later mean we've
> been leaking?

Yes. Though perhaps not technically a leak, in that we are still holding
on to the "struct tree" entries via the obj_hash table. But nobody was
freeing them at all until the end of the program.

I actually think it may be a mistake for "struct tree" to have
buffer/len fields at all. It is a slight convenience to be able to pass
them around with the struct, but it makes the expected lifetime much
more confusing. In practice, all code wants to deal with one tree at a
time, then drop the buffer when it's done (we might hold several when
recursing through subtrees, but we'd never hold more than the distance
from the leaf to the root, and each recursive invocation of something
like process_tree() is holding exactly one tree buffer).

It may not be worth the trouble to try to clean it up at this point,
though.

-Peff