Re: [PATCH] packfile: free packed_git memory when closing object store

Mike Hommey [off-list ref] writes:

Signed-off-by: Mike Hommey <redacted>
---
 packfile.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Note, I'm not sure this is the right place to do it.

I do not think this patch is complete, given that o->packed_git
still has a non-NULL pointer.  IIRC, close_pack() was written for
the explicit purpose of releasing resources while allowing us to
reopen with open_packed_git() on it, so with the current
arrangement, after releasing the resources held for this object
store and doing something else, you should be able to come back to
this object store and work in it again---this patch makes it harder
if not impossible to do so.

I _think_ the patch is OK if you assigned NULL to o->packed_git,
after making sure that the intention of all the callers of
close_object_store() is to declare that this object store will not
be accessed any longer during the lifetime of the process, and write
it down as the contract between the callers and this function in a
comment perhaps in packfile.h where the function is declared.

Thanks.

On Mon, Aug 26, 2019 at 11:06:29AM -0700, Junio C Hamano wrote:

Mike Hommey [off-list ref] writes:

quoted

Signed-off-by: Mike Hommey <redacted>
---
 packfile.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Note, I'm not sure this is the right place to do it.

I do not think this patch is complete, given that o->packed_git
still has a non-NULL pointer.  IIRC, close_pack() was written for
the explicit purpose of releasing resources while allowing us to
reopen with open_packed_git() on it, so with the current
arrangement, after releasing the resources held for this object
store and doing something else, you should be able to come back to
this object store and work in it again---this patch makes it harder
if not impossible to do so.

I _think_ the patch is OK if you assigned NULL to o->packed_git,
after making sure that the intention of all the callers of
close_object_store() is to declare that this object store will not
be accessed any longer during the lifetime of the process, and write
it down as the contract between the callers and this function in a
comment perhaps in packfile.h where the function is declared.

Maybe it would make more sense to do the complete cleanup in
raw_object_store_clear, then?

Relatedly, while looking around the other things that close_object_store
does, I saw that multi_pack_index is a sort of linked list... and
close_midx doesn't follow the links. Which raises the question whether
it should, or whether close_object_store should (considering it's
similar to packed_git in that regard, it would seem like
close_object_store should). It also raises the question what should be
free()ing multi_pack_index, because like packed_git, it's not free()d.

Mike