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

From: Stefan Beller <sbeller@google.com>
Date: May 9, 2018, 18:09 UTC
Message-ID: <CAGZ79kZvG4+=Mx24-HWdsaxsiKS_3YynKDnTB1b-a3kLvM16Xw@mail.gmail.com>
In-Reply-To: <CACsJy8AdJcQpiGrR3p6xfuqU0=qoP3=StgbWRNCkdfka6di+5w@mail.gmail.com>

On Wed, May 9, 2018 at 11:00 AM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 8 quoted lines

> 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.

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.

Show 6 quoted lines

>> 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.

Show 10 quoted lines

>
> 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