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

> 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;