Re: Regression in `git diff --quiet HEAD` when a new file is staged
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 23, 2025, 13:35 UTC
- Message-ID
- <xmqqms5hwxkm.fsf@gitster.g>
- In-Reply-To
- <20251023120101.GA1123594@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 10 quoted lines
> 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.
Yeah, my bad. Lidong noticed the same thing.
Show 52 quoted lines
> 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.That would be bigger change than a regression fix warrants, so let's leave it out, but let me use the above to replace my botched attempt.
Thanks, both of you.