{"thread":{"id":"51737","subject":"[PATCH] packfile: free packed_git memory when closing object store","startedAt":"2019-08-26T02:45:13Z","lastAt":"2019-08-27T00:25:39Z","messageCount":3,"participants":["Mike Hommey","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"381165","messageId":"20190826024508.8444-1-mh@glandium.org","threadId":"51737","inReplyTo":null,"subject":"[PATCH] packfile: free packed_git memory when closing object store","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-08-26T02:45:08Z","receivedAt":"2019-08-26T02:45:13Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Signed-off-by: Mike Hommey \n---\n packfile.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\nNote, I'm not sure this is the right place to do it.\n\ndiff --git a/packfile.c b/packfile.c\nindex fc43a6c52c..b0cb84adda 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -339,13 +339,16 @@ void close_pack(struct packed_git *p)\n \n void close_object_store(struct raw_object_store *o)\n {\n-\tstruct packed_git *p;\n+\tstruct packed_git *p = o->packed_git;\n \n-\tfor (p = o->packed_git; p; p = p->next)\n+\twhile (p) {\n+\t\tstruct packed_git *current = p;\n \t\tif (p->do_not_close)\n \t\t\tBUG(\"want to close pack marked 'do-not-close'\");\n-\t\telse\n-\t\t\tclose_pack(p);\n+\t\tclose_pack(p);\n+\t\tp = p->next;\n+\t\tfree(current);\n+\t}\n \n \tif (o->multi_pack_index) {\n \t\tclose_midx(o->multi_pack_index);\n-- \n2.23.0\n\n"},{"id":"381222","messageId":"xmqqftlnhkd6.fsf@gitster-ct.c.googlers.com","threadId":"51737","inReplyTo":"20190826024508.8444-1-mh@glandium.org","subject":"Re: [PATCH] packfile: free packed_git memory when closing object store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-26T18:06:29Z","receivedAt":"2019-08-26T18:06:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> Signed-off-by: Mike Hommey \n> ---\n> packfile.c | 11 +++++++----\n> 1 file changed, 7 insertions(+), 4 deletions(-)\n>\n> Note, I'm not sure this is the right place to do it.\n\nI do not think this patch is complete, given that o->packed_git\nstill has a non-NULL pointer. IIRC, close_pack() was written for\nthe explicit purpose of releasing resources while allowing us to\nreopen with open_packed_git() on it, so with the current\narrangement, after releasing the resources held for this object\nstore and doing something else, you should be able to come back to\nthis object store and work in it again---this patch makes it harder\nif not impossible to do so.\n\nI _think_ the patch is OK if you assigned NULL to o->packed_git,\nafter making sure that the intention of all the callers of\nclose_object_store() is to declare that this object store will not\nbe accessed any longer during the lifetime of the process, and write\nit down as the contract between the callers and this function in a\ncomment perhaps in packfile.h where the function is declared.\n\nThanks.\n\n> diff --git a/packfile.c b/packfile.c\n> index fc43a6c52c..b0cb84adda 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -339,13 +339,16 @@ void close_pack(struct packed_git *p)\n> \n> void close_object_store(struct raw_object_store *o)\n> {\n> -\tstruct packed_git *p;\n> +\tstruct packed_git *p = o->packed_git;\n> \n> -\tfor (p = o->packed_git; p; p = p->next)\n> +\twhile (p) {\n> +\t\tstruct packed_git *current = p;\n> \t\tif (p->do_not_close)\n> \t\t\tBUG(\"want to close pack marked 'do-not-close'\");\n> -\t\telse\n> -\t\t\tclose_pack(p);\n> +\t\tclose_pack(p);\n> +\t\tp = p->next;\n> +\t\tfree(current);\n> +\t}\n> \n> \tif (o->multi_pack_index) {\n> \t\tclose_midx(o->multi_pack_index);\n"},{"id":"381274","messageId":"20190827002532.aqjbik6haoa5axhj@glandium.org","threadId":"51737","inReplyTo":"xmqqftlnhkd6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] packfile: free packed_git memory when closing object store","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-08-27T00:25:32Z","receivedAt":"2019-08-27T00:25:39Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Mon, Aug 26, 2019 at 11:06:29AM -0700, Junio C Hamano wrote:\n> Mike Hommey writes:\n> \n> > Signed-off-by: Mike Hommey \n> > ---\n> > packfile.c | 11 +++++++----\n> > 1 file changed, 7 insertions(+), 4 deletions(-)\n> >\n> > Note, I'm not sure this is the right place to do it.\n> \n> I do not think this patch is complete, given that o->packed_git\n> still has a non-NULL pointer. IIRC, close_pack() was written for\n> the explicit purpose of releasing resources while allowing us to\n> reopen with open_packed_git() on it, so with the current\n> arrangement, after releasing the resources held for this object\n> store and doing something else, you should be able to come back to\n> this object store and work in it again---this patch makes it harder\n> if not impossible to do so.\n> \n> I _think_ the patch is OK if you assigned NULL to o->packed_git,\n> after making sure that the intention of all the callers of\n> close_object_store() is to declare that this object store will not\n> be accessed any longer during the lifetime of the process, and write\n> it down as the contract between the callers and this function in a\n> comment perhaps in packfile.h where the function is declared.\n\nMaybe it would make more sense to do the complete cleanup in\nraw_object_store_clear, then?\n\nRelatedly, while looking around the other things that close_object_store\ndoes, I saw that multi_pack_index is a sort of linked list... and\nclose_midx doesn't follow the links. Which raises the question whether\nit should, or whether close_object_store should (considering it's\nsimilar to packed_git in that regard, it would seem like\nclose_object_store should). It also raises the question what should be\nfree()ing multi_pack_index, because like packed_git, it's not free()d.\n\nMike\n"}]}