Re: [PATCH v3 2/2] read-cache: plug a few leaks

From: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Date: Jun 8, 2013, 17:22 UTC
Message-ID: <51B36849.3030608@lsrfire.ath.cx>
In-Reply-To: <CAMP44s3UYCX+DzgnErB=0GdD3w5k2GkNKjv46ZA_NVHm1Z0YLQ@mail.gmail.com>

Am 08.06.2013 18:53, schrieb Felipe Contreras:

Show 29 quoted lines

> On Sat, Jun 8, 2013 at 10:56 AM, René Scharfe
> <rene.scharfe@lsrfire.ath.cx> wrote:
>> Am 08.06.2013 16:04, schrieb Felipe Contreras:
>>
>>> On Sat, Jun 8, 2013 at 8:22 AM, René Scharfe
>>> <rene.scharfe@lsrfire.ath.cx> wrote:
>>>>
>>>> Am 08.06.2013 14:15, schrieb Felipe Contreras:
>>>
>>>
>>>>> Why leave it out? If somebody makes the mistake of doing the above
>>>>> sequence, would you prefer that we leak?
>>>>
>>>>
>>>> Leaking is better than silently cleaning up after a buggy caller because
>>>> it
>>>> still allows the underlying bug to be found.
>>>
>>>
>>> No, it doesn't. The pointer is replaced and forever lost. How is
>>> leaking memory helping anyone to find the bug?
>>
>> Valgrind can tell you where leaked memory was allocated, but not if you free
>> it in the "wrong" place.
>
> It is the correct place to free it. And nobody will ever find it with
> valgrind, just like nobody has ever tracked down the hundreds of leaks
> in Git that happen reliably 100% of the time, much less the ones that
> happen rarely.

We could argue about freeing it here or adding a discard_index call somewhere else (which seems more natural to me) once we had a call sequence that actually causes such a leak. The test suite contains none, so I'd say we need more tests first.

Lots of the existing leaks were not worth plugging because the process would end right after freeing the memory. Leaving clean-up to the OS was a conscious design choice, simplifying the code.

When such code is reused in a library or run multiple times while it was originally meant to be run only a single time (like with cherry-pick --stdin) the original assumption doesn't hold any more and we have a problem.

Let's find and fix those leaks by freeing memory in the right places. Freeing memory just in case in places where we can show that no leak is triggered by our test suite doesn't help.

René