Am 09.06.2013 04:25, schrieb Felipe Contreras:
On Sat, Jun 8, 2013 at 9:11 PM, René Scharfe
<rene.scharfe@xxxxxxxxxxxxxx> wrote:
Am 08.06.2013 19:27, schrieb Felipe Contreras:
On Sat, Jun 8, 2013 at 12:22 PM, René Scharfe
<rene.scharfe@xxxxxxxxxxxxxx> 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é
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html