On Mon, Aug 05, 2024 at 10:24:10AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
> > However, I am fuzzy on the existing uses in the backend
> > implementation. For example:
> >
> > static void files_ref_store_release(struct ref_store *ref_store)
> > {
> > struct files_ref_store *refs = files_downcast(ref_store, 0, "release");
> > free_ref_cache(refs->loose);
> > free(refs->gitcommondir);
> > ref_store_release(refs->packed_ref_store);
> > }
> >
> > The packed-ref-store is "released" here, as part of "releasing" the
> > files-ref-store that uses it as a fallback backend. The caller of
> > files_ref_store_release() is refs.c:ref_store_release()
> >
> > void ref_store_release(struct ref_store *ref_store)
> > {
> > ref_store->be->release(ref_store);
> > free(ref_store->gitdir);
> > }
> >
> > So if you have a files based ref store, when you are done you'd be
> > calling ref_store_release() on it, releasing the resources held by
> > the files_ref_store structure, but I do not know who frees the
> > packed_ref_store allocated by files_ref_store_init(). Perhaps it is
> > already leaking? If that is the case then an API update like I
> > suggested above would make even more sense to make it less likely
> > for such a leak to be added to the system in the future, I suspect.
>
> Ahh, that was the leak that you plugged in a separate patch.
>
> So it does point us in the other direction to redefine _release with
> a different behaviour that releases the resource held by the
> structure, and frees the structure itself.
>
> Something along the following line (caution: totally untested) that
> allows your two patches to become empty, and also allows a few
> callers to lose their existing explicit free()s immediately after
> they call _release(), perhaps?
I don't really know whether it's worth the churn, but if somebody wants
to pull through with this I'm game :) But: if we are going to do this,
we should rename the function to be called `ref_store_free()` instead of
`ref_store_release()` according to our recent coding style update :)
> If this were to become a real patch, I think debug backend should
> learn to use the same _downcast() to become more like the real ones
> before it happens in a preliminary clean-up patch.
That certainly wouldn't hurt, yeah.
Patrick
Attachment:
signature.asc
Description: PGP signature
