Re: gitk regression in version 2.36.0

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 23, 2022, 05:54 UTC
Message-ID: <xmqqpml82vkd.fsf@gitster.g>
In-Reply-To: <AM0PR04MB6019ECF053F1FB7B29D75AB7A5F69@AM0PR04MB6019.eurprd04.prod.outlook.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.