From: René Scharfe Date: Sun, 09 Jun 2013 17:38:39 GMT Subject: Re: [PATCH v3 2/2] read-cache: plug a few leaks Message-ID: <51B4BD9F.6070107@lsrfire.ath.cx> In-Reply-To: Am 09.06.2013 04:25, schrieb Felipe Contreras: > On Sat, Jun 8, 2013 at 9:11 PM, René Scharfe > wrote: >> Am 08.06.2013 19:27, schrieb Felipe Contreras: >> >>> On Sat, Jun 8, 2013 at 12:22 PM, René Scharfe >>> 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é