Re: Regression in `git diff --quiet HEAD` when a new file is staged

From: Jeff King <peff@peff.net>
Date: Oct 17, 2025, 07:51 UTC
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:

Show 5 quoted lines

> 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