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

From: René Scharfe <l.s.r@web.de>
Date: Apr 27, 2022, 16:42 UTC
Message-ID: <561a73aa-7e94-5a09-0c9c-08e8b6ce7188@web.de>
In-Reply-To: <xmqq7d7bsu2n.fsf@gitster.g>

Am 26.04.22 um 18:11 schrieb Junio C Hamano:

Show 40 quoted lines

> 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