Re: gitk regression in version 2.36.0

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 23, 2022, 16:00 UTC
Message-ID: <xmqqh76j3i3r.fsf@gitster.g>
In-Reply-To: <737bd2f8-dfd7-6a51-b7f5-33eefa33e975@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 9 quoted lines

>> I am reasonably sure that reverting that commit will be the right
>> thing to do.  It is somewhat unfortunate that it would reintroduce
>> resource leaks that having clear_pathspec() in a wrong place (i.e.
>> diff_free()) was covering up.  We should instead need to find the
>> place where a diff_opt struct goes out of scope (after being used
>> for zero or more times, calling diff_free() after each iteration to
>> release resources consumed per-iteration) and call clear_pathspec().
>
> Right; a small memory leak is better than wrong output.

Yeah, it is an absolute requirement to avoid producing wrong output, and when producing output for 100 or 1000 commits, we should not leak resources in proportion to the number of these commits processed, so forgetting to call diff_free() that releases resources that are required per diff-invocation is also a must. Compared to that, cleaning up the resource allocated just once and repeatedly used while we handle these 100 or 1000 commits, while it is nice to do so, is of much lower priority, certainly much lower than computing the right result.

> Finding those places is a bit complicated by diff_options often being
> embedded in struct rev_info, though.

Perhaps, but we should have a resource-releasing helper for rev_info, so that may be a good place to do so, hopefully.

Thanks

> PS: And I need to learn to download new posts before hitting Send
> (or become faster); sorry for my near-duplicate reply.

Actually this time I very much appreciated an independent validation of my findings ;-) Thanks.