Am 26.04.22 um 18:11 schrieb Junio C Hamano:
> This only surfaced as a regression after 2.36 release, but the
> breakage was already there with us for at least a year.
>
> The diff_free() call is to be used after we completely finished with
> a diffopt structure. After "git diff A B" finishes producing
> output, calling it before process exit is fine. But there are
> commands that prepares diff_options struct once, compares two sets
> of paths, releases resources that were used to do the comparison,
> then reuses the same diff_option struct to go on to compare the next
> two sets of paths, like "git log -p".
>
> After "git log -p" finishes showing a single commit, calling it
> before it goes on to the next commit is NOT fine. There is a
> mechanism, the .no_free member in diff_options struct, to help "git
> log" to avoid calling diff_free() after showing each commit and
> instead call it just one. When the mechanism was introduced in
> e900d494 (diff: add an API for deferred freeing, 2021-02-11),
> however, we forgot to do the same to "diff-tree --stdin", which *is*
> a moral equivalent to "git log".
>
> During 2.36 release cycle, we started clearing the pathspec in
> diff_free(), so programs like gitk that runs
>
> git diff-tree --stdin -- <pathspec>
>
> downstream of a pipe, processing one commit after another, started
> showing irrelevant comparison outside the given <pathspec> from the
> second commit. The same commit, by forgetting to teach the .no_free
> mechanism, broke "diff-tree --stdin -I<regexp>" and nobody noticed
> it for over a year, presumably because it is so seldom used an
> option.
>
> But <pathspec> is a different story. The breakage was very
> prominently visible and was reported immediately after 2.36 was
> released.
>
> Fix this breakage by mimicking how "git log" utilizes the .no_free
> member so that "diff-tree --stdin" behaves more similarly to "log".
>
> Protect the fix with a few new tests.
We could check where reused diffopts caused a pathspec loss at runtime,
like in the patch below. Then we "just" need to get the relevant test
coverage to 100% and we'll find them all.
With your patch on top of main, "make test" passes for me. With the
patch below added as well I get failures in three test scripts:
t3427-rebase-subtree.sh (Wstat: 256 Tests: 3 Failed: 2)
Failed tests: 2-3
Non-zero exit status: 1
t4014-format-patch.sh (Wstat: 256 Tests: 190 Failed: 1)
Failed test: 73
Non-zero exit status: 1
t9350-fast-export.sh (Wstat: 256 Tests: 50 Failed: 3)
Failed tests: 30, 32, 43
Non-zero exit status: 1
The format-patch is a bit surprising to me because it already sets
no_free conditionally. t4014 is successful if no_free is set in all
cases, so the condition seems to be too narrow -- but I don't understand
it. Didn't look at the other cases.
---
diff.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/diff.c b/diff.c
index ef7159968b..b7c837aca8 100644
--- a/diff.c
+++ b/diff.c
@@ -6455,9 +6455,16 @@ static void diff_free_ignore_regex(struct diff_options *options)
void diff_free(struct diff_options *options)
{
+ static struct diff_options *prev_options_with_pathspec;
+
if (options->no_free)
return;
+ if (prev_options_with_pathspec == options && !options->pathspec.nr)
+ BUG("reused struct diff_options, potentially lost pathspec");
+ if (options->pathspec.nr)
+ prev_options_with_pathspec = options;
+
diff_free_file(options);
diff_free_ignore_regex(options);
clear_pathspec(&options->pathspec);
--
2.35.3
