Re: [PATCH] repository: fix free problem with repo_clear(the_repository)

[Duy Nguyen <pclouds@gmail.com>]

On Wed, May 9, 2018 at 7:50 PM, Stefan Beller <sbeller@google.com> 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-sbeller@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