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

["René Scharfe" <rene.scharfe@lsrfire.ath.cx>]

Am 09.06.2013 04:25, schrieb Felipe Contreras:
> On Sat, Jun 8, 2013 at 9:11 PM, René Scharfe
> <rene.scharfe@lsrfire.ath.cx> wrote:
>> Am 08.06.2013 19:27, schrieb Felipe Contreras:
>>
>>> On Sat, Jun 8, 2013 at 12:22 PM, René Scharfe
>>> <rene.scharfe@lsrfire.ath.cx> wrote:
>>>
>>>> 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.
>>>
>>>
>>> It helps; it prevents leaks. The real culprit is the bogus API, but I
>>> don't see that changing anytime soon, so there are two options when
>>> somebody makes a mistake the API allows; leak or don't leak. And you
>>> seem to prefer the leak, even though it provides absolutely no
>>> advantage.
>>
>> It covers up bugs,
>
> It doesn't. I thought you already silently agreed that nobody would
> ever find that leak, as they haven't found the hundreds of leaks that
> plague Git's code.

Nah, I explained non-silently that leakage was a design decision for 
short-running commands that allocate memory, use it and exit.  Reusing 
such code without freeing allocated memory between runs explicitly turns 
a "good" leak into a "bad" one, as we saw with cherry-pick --stdin.

>> What would be a better API?  Making discard_index free the array is a good
>> first step; what else is bogus?
>
> 'initialized' for starters; it should be renamed to 'loaded' or
> removed, but removing it would require many more changes to make sure
> we don't load twice. Also, when loading cache entries, it might make
> sense to check if there's already entries that have not been
> previously discarded properly.

Adding diagnostics that help find leaks is a good idea.

So, from reading the code, this sequence is OK:

	discard_cache()		// defined starting point
	read_cache()		// reads the cache
	read_cache()		// does nothing

And I guess this one is not OK:

	discard_cache()		// defined starting point
	add_index_entry()	// add single entry
	read_cache()		// currently leaks, should warn/die

Any more sequences that we need to guard against, or counterexamples?

> In the meantime, just in case, the only sane thing to do is free the
> entries rather than leak.

I consider not plugging a leak which we don't know how to trigger with 
existing code even more sane.  Yay, circles! ;-)

> That being said I'm not interested in this patch any more. The patch
> is good yet after three tries and countless arguments it's still not
> applied, nor is there any sign of getting there.

Let's take it step by step: Once the known leak is plugged we can worry 
about the unknown ones.  I'll send small patches.

René