Re: gitk regression in version 2.36.0

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

Matthias Aßhauer <mha1993@live.de> writes:

> Git 2.36.0 (or more precisely 244c27242f (diff.[ch]: have diff_free()
> call clear_pathspec(opts.pathspec), 2022-02-16)) introduced some
> change in behaviour that causes gitks highlight feature not to work
> correctly anymore.

Nicely found.

I think what happens is that gitk REPEATEDLY calls log_tree_commit()
by opening a pipe to "git diff-tree -r -s --stdin $gdtargs" and feed
revisions from its standard input.  

    builtin/diff-tree.c::diff_tree_stdin()
    -> builtin/diff-tree.c::stdin_diff_commit()
       -> log-tree.c::log_tree_commit()

Now, log-tree.c::log_tree_commit() is responsible for formating the
difference between the commit and its parent, using the
opt->diffopt.  After computing the pairwise diff for one tree pair
(commit and its parent), of course it calls diff_free() to flush the
queued diff and release other resources we allocated while showing
that single diff.  Before the broken commit 244c27242f, diff_free()
released ONLY the newly allocated resources that we needed to
compute one pair of trees.  Most importantly, the settings used to
compute diffs that are reused to compute the next pair of trees need
to be kept, e.g. pathspec.

Remember, we are talking about log-tree, the upstream of the pipe
going from the tip of the history and traversing the parent chain to
ancestors, so once we are done with showing the diff between our
current commit and its parent, we will move to the parent and
compute the same diff with its parent (i.e. our grand-parent).

But with the regression, we mistakenly release the pathspec, so the
first commit may use the specified pathspec, but the second and
subsequent ones will lose the pathspec the user gave us, making the
comparison tree-wide one, not limited with any pathspec.

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

Thanks for a report.