From: Jeff King <peff@peff.net>
Date: Fri, 17 Oct 2025 07:51:53 GMT
Subject: Re: Regression in `git diff --quiet HEAD` when a new file is staged
Message-ID: <20251017075153.GA4078773@coredump.intra.peff.net>
In-Reply-To: <CACJRbWjwOQwJB13CwTfvhV3p+Hbn4KrNM9AtBanGtUS4V_1MbQ@mail.gmail.com>

On Thu, Oct 16, 2025 at 05:09:07PM -0700, Jake Zimmerman wrote:

> In git v2.51.1, `git diff --quiet HEAD` will actually print something
> if the diff output includes a new, staged file.
> [...]
> I ran a git bisect and isolated this commit:
> b55e6d36ebce69136559add8fffd1a65df231518

Yikes, that is a pretty bad regression. I'm rather surprised that this
wasn't covered in the test suite. t4035 does set this situation up, but
it checks with git-diff-tree, not git-diff. I initially thought that was
because diff defaults to "--patch" output and diff-tree does not, but
even "diff-tree --patch" does not show the bug. Weird. Maybe it has to
do with running diffcore bits?

I see that the author of b55e6d36eb (diff: ensure consistent diff
behavior with ignore options, 2025-08-08) posted this patch earlier
today:

  https://lore.kernel.org/git/pull.2071.git.git.1760671049113.gitgitgadget@gmail.com/

which seems to fix it, but there's no mention there of this thread. And
the included test is still using "-I", where there is clearly collateral
damage even for people who are not using "-I" at all. So I'm not sure if
it's coincidence, or meant to be a fix. ;)

Looking at that patch, my biggest concern is: are we missing other spots
that need to special-case the dry_run setting? Because it's a regression
in a maint release, I'm tempted to say we should do the dumbest possible
thing that covers all cases and just revert this hunk from the original
patch, like:

diff --git a/diff.c b/diff.c
index 87fa16b730..687206f353 100644
--- a/diff.c
+++ b/diff.c
@@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)
 	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
 	    options->flags.exit_with_status &&
 	    options->flags.diff_from_contents) {
+		/*
+		 * run diff_flush_patch for the exit status. setting
+		 * options->file to /dev/null should be safe, because we
+		 * aren't supposed to produce any output anyway.
+		 */
+		diff_free_file(options);
+		options->file = xfopen("/dev/null", "w");
+		options->close_file = 1;
+		options->color_moved = 0;
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 			if (check_pair_status(p))

That would catch the bug here, as well as any others lurking. And it
converts any missing dry_run from correctness problems (we definitely
will not produce extra output) into optimization problems (we might emit
data we do not need, but we can fix those separately). At least for the
normal code paths. I think without those extra fixes the problems that
b55e6d36eb tried to fix for "-I" would still be observable, but at least
its fixes could not regress the other code paths.

-Peff