Re: Regression in `git diff --quiet HEAD` when a new file is staged
- From
Jeff King <peff@peff.net>
- Date
- Oct 23, 2025, 12:01 UTC
- Message-ID
- <20251023120101.GA1123594@coredump.intra.peff.net>
- In-Reply-To
- <xmqqikg6zxui.fsf@gitster.g>
On Wed, Oct 22, 2025 at 09:48:37AM -0700, Junio C Hamano wrote:
Show 6 quoted lines
> 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.
Isn't it just running "git show -w --name-status" at all? If I take the patch you showed below and drop the restoration, like so:
diff --git a/diff.c b/diff.c
index ceb57d1ef8..d402f960a9 100644
--- a/diff.c
+++ b/diff.c
@@ -6836,11 +6836,6 @@ 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++;
}
and then do:
git init echo content >file git add file git commit -m file git show -w --name-status
then we do not show anything. We redirect to /dev/null to run diff_flush_patch_quietly() and find that it does indeed have changes to show (despite -w). But when we try to show the name-status output via flush_one_pair(), we are still redirected to /dev/null.
But wait! That bug is already there in what you have queued in jc/diff-from-contents-fix, even without my change!
That is because you are trying to redirect to /dev/null once at the beginning of the loop. But the loop is effectively:
for each pair
check for content changes with diff_flush_patch_quietly();
output actual pair data with flush_one_pair();We want the redirection to /dev/null for the first part of the loop body, but not the second. So you have to do the redirection inside the loop.
I agree that opening /dev/null over and over is silly. But we can reuse the same filehandle for each one. I.e., like:
diff --git a/diff.c b/diff.c index dac3ea9e01..e903afcf04 100644 --- a/diff.c +++ b/diff.c @@ -6835,11 +6835,11 @@ void diff_flush(struct diff_options *options) /* * make sure diff_Flush_patch_quietly() to be silent. */ - FILE *saved_file = options->file; + FILE *dev_null = NULL; int saved_color_moved = options->color_moved; if (options->flags.diff_from_contents) { - options->file = xfopen("/dev/null", "w"); + dev_null = xfopen("/dev/null", "w"); options->color_moved = 0; } for (i = 0; i < q->nr; i++) { @@ -6848,15 +6848,20 @@ void diff_flush(struct diff_options *options) if (!check_pair_status(p)) continue; - if (options->flags.diff_from_contents && - !diff_flush_patch_quietly(p, options)) - continue; + if (options->flags.diff_from_contents) { + FILE *saved_file = options->file; + int r; + options->file = dev_null; + r = diff_flush_patch_quietly(p, options); + options->file = saved_file; + if (!r) + continue; + } flush_one_pair(p, options); } if (options->flags.diff_from_contents) { - fclose(options->file); - options->file = saved_file; + fclose(dev_null); options->color_moved = saved_color_moved; } separator++; You could even imagine diff_flush_patch_quietly() saving the /dev/null descriptor in a static variable and effectively leaking it (or if we want to be more structured, cached inside the diff_options struct). And then the callers do not have to worry about it at all. And of course this all explains your confusion with Lidong's t4013 test that started failing. It should generate three lines, because they are the actual --raw lines. Once the bug in jc/diff-from-contents-fix is fixed as above, they come back. And running it with the test fixup you have queued on ly/diff-name-only-with-diff-from-content yields a failure with: 'actual' is not empty, it contains: :100644 000000 e69de29 0000000 D file1 :100644 000000 e69de29 0000000 D file2 :000000 100644 0000000 0000000 U file3 -Peff