Re: [PATCH] 2.36 gitk/diff-tree --stdin regression fix

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

Phillip Wood <phillip.wood123@gmail.com> writes:

> On further inspection we have tests for "log -p -I<regex>" in t4013
> and e900d494dc ("diff: add an API for deferred freeing", 2021-02-11) 
> modified builtin/log.c to set the new no_free flag so "log" should be
> OK. However "diff-tree --stdin -p -I<regex>" is not as 
> builtin/diff-tree.c is unchanged by e900d494dc so the no_free flag is
> not set which I think is the cause of the problems reported here.

... reported here, meaning some reproduction exists?  It would be
good to have it in the test, next to the ones I added yesterday, I
think.

In any case, I think that is a much older breakage that can be left
after this "oops, where is my pathspec?" regression is dealt with.

> I think the close_file changes in e900d494dc should be safe as far as
> "diff-tree" is concerned as it never sets that flag.
>
> In retrospect the no_free flag is pretty ugly and fragile.

Yes.

> If we
> really cannot do it another way at least requiring callers to set a
> flag when they want things freeing would avoid nasty surprises like
> this at the expense of leaking when the caller forgets to set
> it. Perhaps once 2.36.1 is out we should step back and think about
> exactly what we're trying to achieve by removing these bounded leaks
> rather than annotating them with UNLEAK().

Doubly yes.

There is small per-task resources allocated that is not proportional
to the size of the task, i.e. "git log -p" may need more resource at
"peak" in a project with 100k files than in a project with 1k files,
and we do not want to leak these resources we use to compare two sets
of 100k (or 1k) files between "commit^" and "commit".  It may allocate
and deallocate more times in a project with 100k commits than in a
project with 1k commits, and we do not want to leak 100 times more
resources in the former project than the latter.  Aiming to reclaim
these resources needed proportinally to the size of the task is
absolutely a good thing to do.

But the final clean-up for the very top-level allocations that is
not proportional to the size of the task, like pathspec, regex, and
other result from parsing command line options and configuration
variables?  Using UNLEAK() to squelch the leak checker and letting
process termination to reclaim them is absolutely a no-cost solution
that is much better.