{"thread":{"id":"48445","subject":"[PATCH] repository: fix free problem with repo_clear(the_repository)","startedAt":"2018-05-09T17:04:44Z","lastAt":"2018-05-10T16:34:18Z","messageCount":11,"participants":["Nguyễn Thái Ngọc Duy","Brandon Williams","Elijah Newren","Stefan Beller","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"347103","messageId":"20180509170409.13666-1-pclouds@gmail.com","threadId":"48445","inReplyTo":null,"subject":"[PATCH] repository: fix free problem with repo_clear(the_repository)","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-09T17:04:09Z","receivedAt":"2018-05-09T17:04:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"the_repository is special. One of the special things about it is that\nit does not allocate a new index_state object like submodules but\npoints to the global the_index variable instead. As a global variable,\nthe_index cannot be free()'d.\n\nAdd an exception for this in repo_clear(). In the future perhaps we\nwould be able to allocate the_repository's index on heap too. Then we\ncan remove revert this.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n I was trying to test the new parsed_object_pool_clear() and found this.\n\n repository.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/repository.c b/repository.c\nindex a4848c1bd0..f44733524a 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -238,7 +238,9 @@ void repo_clear(struct repository *repo)\n \n \tif (repo->index) {\n \t\tdiscard_index(repo->index);\n-\t\tFREE_AND_NULL(repo->index);\n+\t\tif (repo->index != &the_index)\n+\t\t\tfree(repo->index);\n+\t\trepo->index = NULL;\n \t}\n }\n \n-- \n2.17.0.705.g3525833791\n\n"},{"id":"347106","messageId":"20180509171714.GA63423@google.com","threadId":"48445","inReplyTo":"20180509170409.13666-1-pclouds@gmail.com","subject":"Re: [PATCH] repository: fix free problem with repo_clear(the_repository)","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-05-09T17:17:14Z","receivedAt":"2018-05-09T17:17:20Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/09, Nguyễn Thái Ngọc Duy wrote:\n> the_repository is special. One of the special things about it is that\n> it does not allocate a new index_state object like submodules but\n> points to the global the_index variable instead. As a global variable,\n> the_index cannot be free()'d.\n> \n> Add an exception for this in repo_clear(). In the future perhaps we\n> would be able to allocate the_repository's index on heap too. Then we\n> can remove revert this.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> I was trying to test the new parsed_object_pool_clear() and found this.\n\nThis looks good and I do hope we can get to a state soon where we can\nnot have to special case the_repository.\n\n> \n> repository.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/repository.c b/repository.c\n> index a4848c1bd0..f44733524a 100644\n> --- a/repository.c\n> +++ b/repository.c\n> @@ -238,7 +238,9 @@ void repo_clear(struct repository *repo)\n> \n> \tif (repo->index) {\n> \t\tdiscard_index(repo->index);\n> -\t\tFREE_AND_NULL(repo->index);\n> +\t\tif (repo->index != &the_index)\n> +\t\t\tfree(repo->index);\n> +\t\trepo->index = NULL;\n> \t}\n> }\n> \n> -- \n> 2.17.0.705.g3525833791\n> \n\n-- \nBrandon Williams\n"},{"id":"347110","messageId":"CABPp-BHGLkEnYaR2rQqLRSraMgxW7E=fmS1=bGTVeSi2VrPKcw@mail.gmail.com","threadId":"48445","inReplyTo":"20180509170409.13666-1-pclouds@gmail.com","subject":"Re: [PATCH] repository: fix free problem with repo_clear(the_repository)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-05-09T17:42:36Z","receivedAt":"2018-05-09T17:42:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, May 9, 2018 at 10:04 AM, Nguyễn Thái Ngọc Duy wrote:\n> the_repository is special. One of the special things about it is that\n> it does not allocate a new index_state object like submodules but\n> points to the global the_index variable instead. As a global variable,\n> the_index cannot be free()'d.\n>\n> Add an exception for this in repo_clear(). In the future perhaps we\n> would be able to allocate the_repository's index on heap too. Then we\n> can remove revert this.\n\n\"remove revert\"?\n"},{"id":"347113","messageId":"CAGZ79kbFiULj1NJARm6ObYrqv_Fu+U2sb8h_sNJwdWur+JqrvQ@mail.gmail.com","threadId":"48445","inReplyTo":"20180509170409.13666-1-pclouds@gmail.com","subject":"Re: [PATCH] repository: fix free problem with repo_clear(the_repository)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-09T17:50:00Z","receivedAt":"2018-05-09T17:50:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 9, 2018 at 10:04 AM, Nguyễn Thái Ngọc Duy wrote:\n> the_repository is special. One of the special things about it is that\n> it does not allocate a new index_state object like submodules but\n> points to the global the_index variable instead. As a global variable,\n> the_index cannot be free()'d.\n\nok. That is the situation we're in.\n\n>\n> Add an exception for this in repo_clear(). In the future perhaps we\n> would be able to allocate the_repository's index on heap too. Then we\n> can remove revert this.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> I was trying to test the new parsed_object_pool_clear() and found this.\n\nSo this would go with the latest sb/object-store-alloc ?\n\nMy impression was that we never call repo_clear() on\nthe_repository, which would allow us to special case\nthe_repository further just as I did in v2 of that series[1] by\nhaving static allocations for certain objects in case of \\\nthe_repository.\n\n[1] https://public-inbox.org/git/20180501213403.14643-14-sbeller@google.com/\n\nWe could just deal with all the exceptions, but that makes repo_clear\nugly IMHO.\n\nI would rather special case the_repository even more instead\nof having it allocate all its things on the heap. (However we rather\nwant to profile it and argue with data....)\n\n> repository.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/repository.c b/repository.c\n> index a4848c1bd0..f44733524a 100644\n> --- a/repository.c\n> +++ b/repository.c\n> @@ -238,7 +238,9 @@ void repo_clear(struct repository *repo)\n>\n> if (repo->index) {\n> discard_index(repo->index);\n> - FREE_AND_NULL(repo->index);\n> + if (repo->index != &the_index)\n> + free(repo->index);\n> + repo->index = NULL;\n\nSo after this we have a \"dangling\" the_index.\nIt is not really dangling, but it is not part of the_repository any more\nand many places still use the_index, it might make up for interesting\nbugs?\n\nWhat is your use case of repo_clear(the_repository)?\n\nThanks,\nStefan\n"},{"id":"347116","messageId":"CACsJy8DFnWreLW_9xTsxJejvgXcT9EGsZ91cx7h39QpP4_p4Bw@mail.gmail.com","threadId":"48445","inReplyTo":"CABPp-BHGLkEnYaR2rQqLRSraMgxW7E=fmS1=bGTVeSi2VrPKcw@mail.gmail.com","subject":"Re: [PATCH] repository: fix free problem with repo_clear(the_repository)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-09T17:54:20Z","receivedAt":"2018-05-09T17:54:56Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 9, 2018 at 7:42 PM, Elijah Newren wrote:\n> On Wed, May 9, 2018 at 10:04 AM, Nguyễn Thái Ngọc Duy wrote:\n>> the_repository is special. One of the special things about it is that\n>> it does not allocate a new index_state object like submodules but\n>> points to the global the_index variable instead. As a global variable,\n>> the_index cannot be free()'d.\n>>\n>> Add an exception for this in repo_clear(). In the future perhaps we\n>> would be able to allocate the_repository's index on heap too. Then we\n>> can remove revert this.\n>\n> \"remove revert\"?\n\nIt's obvious that double negatives are below me. I'm going to the next\nlevel with double positives! \"remove\" should be removed.\n-- \nDuy\n"},{"id":"347118","messageId":"CACsJy8AdJcQpiGrR3p6xfuqU0=qoP3=StgbWRNCkdfka6di+5w@mail.gmail.com","threadId":"48445","inReplyTo":"CAGZ79kbFiULj1NJARm6ObYrqv_Fu+U2sb8h_sNJwdWur+JqrvQ@mail.gmail.com","subject":"Re: [PATCH] repository: fix free problem with repo_clear(the_repository)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-09T18:00:39Z","receivedAt":"2018-05-09T18:01:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 9, 2018 at 7:50 PM, Stefan Beller wrote:\n>> I was trying to test the new parsed_object_pool_clear() and found this.\n>\n> So this would go with the latest sb/object-store-alloc ?\n\nNo this should be separate because sb/object-store-alloc did not even\ntouch this code. I mistakenly thought you wrote this and sent to you.\nI should have checked and sent to Brandon instead.\n\n> My impression was that we never call repo_clear() on\n> the_repository, which would allow us to special case\n> the_repository further just as I did in v2 of that series[1] by\n> having static allocations for certain objects in case of \\\n> the_repository.\n>\n> [1] https://public-inbox.org/git/20180501213403.14643-14-sbeller@google.com/\n>\n> We could just deal with all the exceptions, but that makes repo_clear\n> ugly IMHO.\n>\n> I would rather special case the_repository even more instead\n> of having it allocate all its things on the heap. (However we rather\n> want to profile it and argue with data....)\n\nI'm actually going the opposite direction and trying hard to make\nthe_repository normal like everybody else :)\n\n>> repository.c | 4 +++-\n>> 1 file changed, 3 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/repository.c b/repository.c\n>> index a4848c1bd0..f44733524a 100644\n>> --- a/repository.c\n>> +++ b/repository.c\n>> @@ -238,7 +238,9 @@ void repo_clear(struct repository *repo)\n>>\n>> if (repo->index) {\n>> discard_index(repo->index);\n>> - FREE_AND_NULL(repo->index);\n>> + if (repo->index != &the_index)\n>> + free(repo->index);\n>> + repo->index = NULL;\n>\n> So after this we have a \"dangling\" the_index.\n> It is not really dangling, but it is not part of the_repository any more\n> and many places still use the_index, it might make up for interesting\n> bugs?\n\nHmm.. yeah, as a \"clearing\" operation, I probaly should not clear\nrepo->index. This way the_repository will be back in initial state and\ncould be reused again. Something like this perhaps?\n\ndiscard_index(repo->index);\nif (repo->index != &the_index)\n FREE_AND_NULL(repo->index);\n\n> What is your use case of repo_clear(the_repository)?\n\nNo actual use case right now. See [1] for the code that triggered\nthis. I do want to free some memory in pack-objects and repo_clear()\n_might_ be the one. I'm not sure yet.\n\n[1] https://gist.github.com/pclouds/86a2df6c28043f1b6fa3d4e72e7a1276\n-- \nDuy\n"},{"id":"347120","messageId":"CAGZ79kZvG4+=Mx24-HWdsaxsiKS_3YynKDnTB1b-a3kLvM16Xw@mail.gmail.com","threadId":"48445","inReplyTo":"CACsJy8AdJcQpiGrR3p6xfuqU0=qoP3=StgbWRNCkdfka6di+5w@mail.gmail.com","subject":"Re: [PATCH] repository: fix free problem with repo_clear(the_repository)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-09T18:09:23Z","receivedAt":"2018-05-09T18:09:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 9, 2018 at 11:00 AM, Duy Nguyen wrote:\n> On Wed, May 9, 2018 at 7:50 PM, Stefan Beller wrote:\n>>> I was trying to test the new parsed_object_pool_clear() and found this.\n>>\n>> So this would go with the latest sb/object-store-alloc ?\n>\n> No this should be separate because sb/object-store-alloc did not even\n> touch this code. I mistakenly thought you wrote this and sent to you.\n> I should have checked and sent to Brandon instead.\n\nYes, they do not produce merge conflicts and do not semantically\nrely on each other. Except as noted below\nthe object store series complicated things in a non-latest version\nof it, such that we'd have to add more special casing. Now everything\nis fine.\n\n\n>> I would rather special case the_repository even more instead\n>> of having it allocate all its things on the heap. (However we rather\n>> want to profile it and argue with data....)\n>\n> I'm actually going the opposite direction and trying hard to make\n> the_repository normal like everybody else :)\n\nok, both Brandon and you want to do this, so I guess we'll just\ngo this route for now.\n\n>\n> discard_index(repo->index);\n> if (repo->index != &the_index)\n> FREE_AND_NULL(repo->index);\n>\n>> What is your use case of repo_clear(the_repository)?\n>\n> No actual use case right now. See [1] for the code that triggered\n> this. I do want to free some memory in pack-objects and repo_clear()\n> _might_ be the one. I'm not sure yet.\n\nThis diff seems good to me. as any repo is cleared and wil have the minimal\namount of memory. the_repository clears its the_index which is also fine\nas it has the minimal amount of memory then, too\n\nThanks,\nStefan\n"},{"id":"347178","messageId":"20180510061310.28265-1-pclouds@gmail.com","threadId":"48445","inReplyTo":"20180509170409.13666-1-pclouds@gmail.com","subject":"[PATCH v2] repository: fix free problem with repo_clear(the_repository)","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-10T06:13:10Z","receivedAt":"2018-05-10T06:13:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"the_repository is special. One of the special things about it is that\nit does not allocate a new index_state object like submodules but\npoints to the global the_index variable instead. As a global variable,\nthe_index cannot be free()'d.\n\nAdd an exception for this in repo_clear(). In the future perhaps we\nwould be able to allocate the_repository's index on heap too. Then we\ncan revert this.\n\nthe_repository->index remains pointed to a clean the_index even after\nrepo_clear() so that it could still be used next time (e.g. in a crazy\nuse case where a dev switches repo in the same process).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n v2 keeps the_repository->index pointed to the_index even after cleared\n\n repository.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/repository.c b/repository.c\nindex a4848c1bd0..4143073f89 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -238,7 +238,8 @@ void repo_clear(struct repository *repo)\n \n \tif (repo->index) {\n \t\tdiscard_index(repo->index);\n-\t\tFREE_AND_NULL(repo->index);\n+\t\tif (repo->index != &the_index)\n+\t\t\tFREE_AND_NULL(repo->index);\n \t}\n }\n \n-- \n2.17.0.705.g3525833791\n\n"},{"id":"347179","messageId":"CACsJy8CosHdk3DA46aNM3v5d4wnmLumv+6MdJMY=WfVtKqZ8LA@mail.gmail.com","threadId":"48445","inReplyTo":"CACsJy8AdJcQpiGrR3p6xfuqU0=qoP3=StgbWRNCkdfka6di+5w@mail.gmail.com","subject":"Re: [PATCH] repository: fix free problem with repo_clear(the_repository)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-10T06:18:44Z","receivedAt":"2018-05-10T06:19:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 9, 2018 at 8:00 PM, Duy Nguyen wrote:\n> discard_index(repo->index);\n> if (repo->index != &the_index)\n> FREE_AND_NULL(repo->index);\n>\n>> What is your use case of repo_clear(the_repository)?\n>\n> No actual use case right now. See [1] for the code that triggered\n> this. I do want to free some memory in pack-objects and repo_clear()\n> _might_ be the one. I'm not sure yet.\n\nAnother use case for repo_clear(the_repository) is \"git worktree\nmove\". Part of the reason I did not support moving the main repository\nwith that command is because I wanted to shut down every access to\n$MAIN_WORK_TREE/.git before moving $MAIN_WORK_TREE. It's probably not\na problem for linux/mac platforms to move files (on the same file\nsystem [1]) with file descriptors still open, but I'm pretty sure it\nwon't work on Windows. If repo_clear() does its job well, I should be\nable to safely move $GIT_WORK_TREE after that.\n\n[1] if we move across file systems then another set of problems arise:\nif file descriptors remain open, writing to those will not affect the\nnew copies in the target. We do not support moving across filesystems\nyet, but we should not shut that door now.\n-- \nDuy\n"},{"id":"347202","messageId":"xmqqbmdnkgh4.fsf@gitster-ct.c.googlers.com","threadId":"48445","inReplyTo":"CAGZ79kbFiULj1NJARm6ObYrqv_Fu+U2sb8h_sNJwdWur+JqrvQ@mail.gmail.com","subject":"Re: [PATCH] repository: fix free problem with repo_clear(the_repository)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-10T09:27:51Z","receivedAt":"2018-05-10T09:27:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> So this would go with the latest sb/object-store-alloc ?\n>\n> My impression was that we never call repo_clear() on\n> the_repository, which would allow us to special case\n> the_repository further just as I did in v2 of that series[1] by\n> having static allocations for certain objects in case of \\\n> the_repository.\n>\n> [1] https://public-inbox.org/git/20180501213403.14643-14-sbeller@google.com/\n>\n> We could just deal with all the exceptions, but that makes repo_clear\n> ugly IMHO.\n\nSo perhaps\n\n void repo_clear(...)\n {\n + if (repo == the_repository)\n + BUG(\"repo_clear() called on the repository\");\n ...\n\nor something?\n"},{"id":"347257","messageId":"CAGZ79kaSu6a+yUqmpQ+EpWjQSp+8fahk8DgpfgwRPwybnRH=eA@mail.gmail.com","threadId":"48445","inReplyTo":"xmqqbmdnkgh4.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] repository: fix free problem with repo_clear(the_repository)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-10T16:34:12Z","receivedAt":"2018-05-10T16:34:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi Junio,\n\nOn Thu, May 10, 2018 at 2:27 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> So this would go with the latest sb/object-store-alloc ?\n>>\n>> My impression was that we never call repo_clear() on\n>> the_repository, which would allow us to special case\n>> the_repository further just as I did in v2 of that series[1] by\n>> having static allocations for certain objects in case of \\\n>> the_repository.\n>>\n>> [1] https://public-inbox.org/git/20180501213403.14643-14-sbeller@google.com/\n>>\n>> We could just deal with all the exceptions, but that makes repo_clear\n>> ugly IMHO.\n>\n> So perhaps\n>\n> void repo_clear(...)\n> {\n> + if (repo == the_repository)\n> + BUG(\"repo_clear() called on the repository\");\n> ...\n>\n> or something?\n\nThis would work, but Duy convinced me to have repo_clear working\non the_repository is a good idea by giving a minimal test helper[1],\nwhich helped me to find leaks[2][3], so I'd rather go with the solution\nby Duy in [4] from a developers perspective.\n\nStefan\n\n[1] https://public-inbox.org/git/CACsJy8C7N2W821H8YR8VaKdCSOSCDtQi_YT7z8hHNDO-VxJmEA@mail.gmail.com/\n https://gist.github.com/pclouds/86a2df6c28043f1b6fa3d4e72e7a1276\n[2] https://public-inbox.org/git/20180510001211.163692-1-sbeller@google.com/\n[3] https://public-inbox.org/git/20180509234059.52156-1-sbeller@google.com/\n[4] https://public-inbox.org/git/CACsJy8AdJcQpiGrR3p6xfuqU0=qoP3=StgbWRNCkdfka6di+5w@mail.gmail.com/\n"}]}