On Mon, Aug 26, 2019 at 11:06:29AM -0700, Junio C Hamano wrote: > Mike Hommey <mh@xxxxxxxxxxxx> writes: > > > Signed-off-by: Mike Hommey <mh@xxxxxxxxxxxx> > > --- > > 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
