Re: [PATCH] repository: fix free problem with repo_clear(the_repository)
On 05/09, Nguyễn Thái Ngọc Duy wrote: > the_repository is special. One of the special things about it is that > it does not allocate a new index_state object like submodules but > points to the global the_index variable instead. As a global variable, > the_index cannot be free()'d. > > Add an exception for this in repo_clear(). In the future perhaps we > would be able to allocate the_repository's index on heap too. Then we > can remove revert this. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > I was trying to test the new parsed_object_pool_clear() and found this. This looks good and I do hope we can get to a state soon where we can not have to special case the_repository. > > repository.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/repository.c b/repository.c > index a4848c1bd0..f44733524a 100644 > --- a/repository.c > +++ b/repository.c > @@ -238,7 +238,9 @@ void repo_clear(struct repository *repo) > > if (repo->index) { > discard_index(repo->index); > - FREE_AND_NULL(repo->index); > + if (repo->index != &the_index) > + free(repo->index); > + repo->index = NULL; > } > } > > -- > 2.17.0.705.g3525833791 > -- Brandon Williams
Re: [PATCH] repository: fix free problem with repo_clear(the_repository)
On Wed, May 9, 2018 at 10:04 AM, Nguyễn Thái Ngọc Duy wrote: > the_repository is special. One of the special things about it is that > it does not allocate a new index_state object like submodules but > points to the global the_index variable instead. As a global variable, > the_index cannot be free()'d. > > Add an exception for this in repo_clear(). In the future perhaps we > would be able to allocate the_repository's index on heap too. Then we > can remove revert this. "remove revert"?
Re: [PATCH] repository: fix free problem with repo_clear(the_repository)
On Wed, May 9, 2018 at 10:04 AM, Nguyễn Thái Ngọc Duy wrote: > the_repository is special. One of the special things about it is that > it does not allocate a new index_state object like submodules but > points to the global the_index variable instead. As a global variable, > the_index cannot be free()'d. ok. That is the situation we're in. > > Add an exception for this in repo_clear(). In the future perhaps we > would be able to allocate the_repository's index on heap too. Then we > can remove revert this. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > I was trying to test the new parsed_object_pool_clear() and found this. So this would go with the latest sb/object-store-alloc ? My impression was that we never call repo_clear() on the_repository, which would allow us to special case the_repository further just as I did in v2 of that series[1] by having static allocations for certain objects in case of \ the_repository. [1] https://public-inbox.org/git/20180501213403.14643-14-sbel...@google.com/ We could just deal with all the exceptions, but that makes repo_clear ugly IMHO. I would rather special case the_repository even more instead of having it allocate all its things on the heap. (However we rather want to profile it and argue with data) > repository.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/repository.c b/repository.c > index a4848c1bd0..f44733524a 100644 > --- a/repository.c > +++ b/repository.c > @@ -238,7 +238,9 @@ void repo_clear(struct repository *repo) > > if (repo->index) { > discard_index(repo->index); > - FREE_AND_NULL(repo->index); > + if (repo->index != &the_index) > + free(repo->index); > + repo->index = NULL; So after this we have a "dangling" the_index. It is not really dangling, but it is not part of the_repository any more and many places still use the_index, it might make up for interesting bugs? What is your use case of repo_clear(the_repository)? Thanks, Stefan
Re: [PATCH] repository: fix free problem with repo_clear(the_repository)
On Wed, May 9, 2018 at 7:42 PM, Elijah Newren wrote: > On Wed, May 9, 2018 at 10:04 AM, Nguyễn Thái Ngọc Duy > wrote: >> the_repository is special. One of the special things about it is that >> it does not allocate a new index_state object like submodules but >> points to the global the_index variable instead. As a global variable, >> the_index cannot be free()'d. >> >> Add an exception for this in repo_clear(). In the future perhaps we >> would be able to allocate the_repository's index on heap too. Then we >> can remove revert this. > > "remove revert"? It's obvious that double negatives are below me. I'm going to the next level with double positives! "remove" should be removed. -- Duy
Re: [PATCH] repository: fix free problem with repo_clear(the_repository)
On Wed, May 9, 2018 at 7:50 PM, Stefan Beller wrote: >> I was trying to test the new parsed_object_pool_clear() and found this. > > So this would go with the latest sb/object-store-alloc ? No this should be separate because sb/object-store-alloc did not even touch this code. I mistakenly thought you wrote this and sent to you. I should have checked and sent to Brandon instead. > My impression was that we never call repo_clear() on > the_repository, which would allow us to special case > the_repository further just as I did in v2 of that series[1] by > having static allocations for certain objects in case of \ > the_repository. > > [1] https://public-inbox.org/git/20180501213403.14643-14-sbel...@google.com/ > > We could just deal with all the exceptions, but that makes repo_clear > ugly IMHO. > > I would rather special case the_repository even more instead > of having it allocate all its things on the heap. (However we rather > want to profile it and argue with data) I'm actually going the opposite direction and trying hard to make the_repository normal like everybody else :) >> repository.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/repository.c b/repository.c >> index a4848c1bd0..f44733524a 100644 >> --- a/repository.c >> +++ b/repository.c >> @@ -238,7 +238,9 @@ void repo_clear(struct repository *repo) >> >> if (repo->index) { >> discard_index(repo->index); >> - FREE_AND_NULL(repo->index); >> + if (repo->index != &the_index) >> + free(repo->index); >> + repo->index = NULL; > > So after this we have a "dangling" the_index. > It is not really dangling, but it is not part of the_repository any more > and many places still use the_index, it might make up for interesting > bugs? Hmm.. yeah, as a "clearing" operation, I probaly should not clear repo->index. This way the_repository will be back in initial state and could be reused again. Something like this perhaps? discard_index(repo->index); if (repo->index != &the_index) FREE_AND_NULL(repo->index); > What is your use case of repo_clear(the_repository)? No actual use case right now. See [1] for the code that triggered this. I do want to free some memory in pack-objects and repo_clear() _might_ be the one. I'm not sure yet. [1] https://gist.github.com/pclouds/86a2df6c28043f1b6fa3d4e72e7a1276 -- Duy
Re: [PATCH] repository: fix free problem with repo_clear(the_repository)
On Wed, May 9, 2018 at 11:00 AM, Duy Nguyen wrote: > On Wed, May 9, 2018 at 7:50 PM, Stefan Beller wrote: >>> I was trying to test the new parsed_object_pool_clear() and found this. >> >> So this would go with the latest sb/object-store-alloc ? > > No this should be separate because sb/object-store-alloc did not even > touch this code. I mistakenly thought you wrote this and sent to you. > I should have checked and sent to Brandon instead. Yes, they do not produce merge conflicts and do not semantically rely on each other. Except as noted below the object store series complicated things in a non-latest version of it, such that we'd have to add more special casing. Now everything is fine. >> I would rather special case the_repository even more instead >> of having it allocate all its things on the heap. (However we rather >> want to profile it and argue with data) > > I'm actually going the opposite direction and trying hard to make > the_repository normal like everybody else :) ok, both Brandon and you want to do this, so I guess we'll just go this route for now. > > discard_index(repo->index); > if (repo->index != &the_index) > FREE_AND_NULL(repo->index); > >> What is your use case of repo_clear(the_repository)? > > No actual use case right now. See [1] for the code that triggered > this. I do want to free some memory in pack-objects and repo_clear() > _might_ be the one. I'm not sure yet. This diff seems good to me. as any repo is cleared and wil have the minimal amount of memory. the_repository clears its the_index which is also fine as it has the minimal amount of memory then, too Thanks, Stefan
Re: [PATCH] repository: fix free problem with repo_clear(the_repository)
On Wed, May 9, 2018 at 8:00 PM, Duy Nguyen wrote: > discard_index(repo->index); > if (repo->index != &the_index) > FREE_AND_NULL(repo->index); > >> What is your use case of repo_clear(the_repository)? > > No actual use case right now. See [1] for the code that triggered > this. I do want to free some memory in pack-objects and repo_clear() > _might_ be the one. I'm not sure yet. Another use case for repo_clear(the_repository) is "git worktree move". Part of the reason I did not support moving the main repository with that command is because I wanted to shut down every access to $MAIN_WORK_TREE/.git before moving $MAIN_WORK_TREE. It's probably not a problem for linux/mac platforms to move files (on the same file system [1]) with file descriptors still open, but I'm pretty sure it won't work on Windows. If repo_clear() does its job well, I should be able to safely move $GIT_WORK_TREE after that. [1] if we move across file systems then another set of problems arise: if file descriptors remain open, writing to those will not affect the new copies in the target. We do not support moving across filesystems yet, but we should not shut that door now. -- Duy
Re: [PATCH] repository: fix free problem with repo_clear(the_repository)
Stefan Beller writes: > So this would go with the latest sb/object-store-alloc ? > > My impression was that we never call repo_clear() on > the_repository, which would allow us to special case > the_repository further just as I did in v2 of that series[1] by > having static allocations for certain objects in case of \ > the_repository. > > [1] https://public-inbox.org/git/20180501213403.14643-14-sbel...@google.com/ > > We could just deal with all the exceptions, but that makes repo_clear > ugly IMHO. So perhaps void repo_clear(...) { + if (repo == the_repository) + BUG("repo_clear() called on the repository"); ... or something?
Re: [PATCH] repository: fix free problem with repo_clear(the_repository)
Hi Junio, On Thu, May 10, 2018 at 2:27 AM, Junio C Hamano wrote: > Stefan Beller writes: > >> So this would go with the latest sb/object-store-alloc ? >> >> My impression was that we never call repo_clear() on >> the_repository, which would allow us to special case >> the_repository further just as I did in v2 of that series[1] by >> having static allocations for certain objects in case of \ >> the_repository. >> >> [1] https://public-inbox.org/git/20180501213403.14643-14-sbel...@google.com/ >> >> We could just deal with all the exceptions, but that makes repo_clear >> ugly IMHO. > > So perhaps > > void repo_clear(...) > { > + if (repo == the_repository) > + BUG("repo_clear() called on the repository"); > ... > > or something? This would work, but Duy convinced me to have repo_clear working on the_repository is a good idea by giving a minimal test helper[1], which helped me to find leaks[2][3], so I'd rather go with the solution by Duy in [4] from a developers perspective. Stefan [1] https://public-inbox.org/git/cacsjy8c7n2w821h8yr8vakdcsoscdtqi_yt7z8hhndo-vxj...@mail.gmail.com/ https://gist.github.com/pclouds/86a2df6c28043f1b6fa3d4e72e7a1276 [2] https://public-inbox.org/git/20180510001211.163692-1-sbel...@google.com/ [3] https://public-inbox.org/git/20180509234059.52156-1-sbel...@google.com/ [4] https://public-inbox.org/git/CACsJy8AdJcQpiGrR3p6xfuqU0=qoP3=stgbwrnckdfka6di...@mail.gmail.com/