Am 27.04.22 um 18:42 schrieb René Scharfe:
> 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;
This can report a false positive if a diffopt is reused with different
pathspecs, and one of them is empty (match all). Which could be countered
by using a fresh diffopt every time (e.g. pushing it into a loop).
> +
> diff_free_file(options);
> diff_free_ignore_regex(options);
> clear_pathspec(&options->pathspec);
