Re: gitk regression in version 2.36.0

[Junio C Hamano <gitster@pobox.com>]

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

>> 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.