From: "René Scharfe" <l.s.r@web.de>
To: "Junio C Hamano" <gitster@pobox.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
git@vger.kernel.org
Cc: "Phillip Wood" <phillip.wood123@gmail.com>,
"Matthias Aßhauer" <mha1993@live.de>
Subject: Re: [PATCH] 2.36 gitk/diff-tree --stdin regression fix
Date: Wed, 27 Apr 2022 18:42:04 +0200 [thread overview]
Message-ID: <561a73aa-7e94-5a09-0c9c-08e8b6ce7188@web.de> (raw)
In-Reply-To: <xmqq7d7bsu2n.fsf@gitster.g>
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
next prev parent reply other threads:[~2022-04-27 16:43 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-23 5:25 gitk regression in version 2.36.0 Matthias Aßhauer
2022-04-23 5:54 ` Junio C Hamano
2022-04-23 6:05 ` Junio C Hamano
2022-04-23 10:13 ` René Scharfe
2022-04-23 16:00 ` Junio C Hamano
2022-04-25 17:45 ` [PATCH] 2.36 gitk/diff-tree --stdin regression fix Junio C Hamano
2022-04-25 22:37 ` [PATCH] t4013: diff-tree --stdin with pathspec Junio C Hamano
2022-04-26 10:09 ` [PATCH] 2.36 gitk/diff-tree --stdin regression fix Phillip Wood
2022-04-26 13:45 ` Phillip Wood
2022-04-26 15:16 ` Junio C Hamano
2022-04-26 15:26 ` Junio C Hamano
2022-04-26 16:11 ` Junio C Hamano
2022-04-27 16:42 ` René Scharfe [this message]
2022-04-27 18:06 ` René Scharfe
2022-04-27 20:03 ` Junio C Hamano
2022-04-23 9:27 ` gitk regression in version 2.36.0 René Scharfe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=561a73aa-7e94-5a09-0c9c-08e8b6ce7188@web.de \
--to=l.s.r@web.de \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=mha1993@live.de \
--cc=phillip.wood123@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).