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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 22, 2025, 16:48 UTC
Message-ID: <xmqqikg6zxui.fsf@gitster.g>
In-Reply-To: <20251022091112.GB853931@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 29 quoted lines

> On Tue, Oct 21, 2025 at 07:38:03AM -0700, Junio C Hamano wrote:
>
>> > So really, the regression fix should probably cover both of them (which
>> > it would if we move the /dev/null redirection into the flush_quietly()
>> > variant).
>> 
>> Do you mean something like this on top of your patch for 'maint',
>> and the latest from Lidong to the 'master' front, then?
>
> Yep, exactly (though with the "o->file" restoration that Lidong
> pointed out).
>
>> Having calls to this helper in two loops in one function looks a bit
>> awkward but the conditions to enter these two loops are mutually
>> exclusive, so it is not like we can remember the result of the calls
>> we make in the first loop and reuse in the second loop, so this
>> probably is the best we can do.
>
> Yeah. I suspect there is some formulation along the lines of: if we have
> diff_from_contents set but are not looking at a content-level diff, then
> up-front in diff_flush() we should quietly flush each to find out what
> is changed and what is not. But the loop for NAME_STATUS, etc, needs to
> know _which_ pairs still had changes (whereas --quiet only cares about
> whether there were any changes at all). So we'd have to store that
> somewhere.
>
> And of course the chance of regressing some unconsidered corner case is
> high. Definitely not something we should entertain while doing another
> regression fix. ;)

Of course. The "redirect inside flush_quietly()" change by itself is turning out to be tricky enough for the other caller of the helper.

Here is what I have on top of your patch right now, after ditching the idea to move the redirect to flush_quietly() because it would mean redirecting N times for a N-path patch, but one thing that is frustrating is that I cannot come up with a scenario or test in which it makes a difference to this other caller if we forget to restore o->file member.

diff --git c/diff.c i/diff.c
index 9b8d658b9e..ceb57d1ef8 100644
--- c/diff.c
+++ i/diff.c
@@ -6814,6 +6814,16 @@ void diff_flush(struct diff_options *options)
 			     DIFF_FORMAT_NAME |
 			     DIFF_FORMAT_NAME_STATUS |
 			     DIFF_FORMAT_CHECKDIFF)) {
+		/*
+		 * make sure diff_Flush_patch_quietly() to be silent.
+		 */
+		FILE *saved_file = options->file;
+		int saved_color_moved = options->color_moved;
+
+		if (options->flags.diff_from_contents) {
+			options->file = xfopen("/dev/null", "w");
+			options->color_moved = 0;
+		}
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 
@@ -6826,6 +6836,11 @@ void diff_flush(struct diff_options *options)
 
 			flush_one_pair(p, options);
 		}
+		if (options->flags.diff_from_contents) {
+			fclose(options->file);
+			options->file = saved_file;
+			options->color_moved = saved_color_moved;
+		}
 		separator++;
 	}