Re: [PATCH 1/3] is_promisor_object(): free tree buffer after parsing
- From
Jeff King <peff@peff.net>
- Date
- Apr 14, 2021, 05:18 UTC
- Message-ID
- <YHZ7Iy29FS9SjKjT@coredump.intra.peff.net>
- In-Reply-To
- <xmqqtuoakkgc.fsf@gitster.g>
On Tue, Apr 13, 2021 at 01:17:55PM -0700, Junio C Hamano wrote:
Show 15 quoted lines
> > 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