Re: Regression in `git diff --quiet HEAD` when a new file is staged
- From
Lidong Yan <yldhome2d2@gmail.com>
- Date
- Oct 18, 2025, 01:04 UTC
- Message-ID
- <918E56B8-7009-4E8E-A98E-AC5B9CE4DD7C@gmail.com>
- In-Reply-To
- <xmqq7bwt1kyf.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
Show 43 quoted lines
>
> Jeff King <peff@peff.net> writes:
>
>> 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.
>
> Ahh. I like this "stupid but cannot be incorrect" version even
> better than the original one that introduced the "dry run" mode.
>
> But once we go in that direction, do we still need the dry-run
> machinery with diff_flush_patch_quietly() helper function?I believe we can move Peff’s code from diff_flush() to diff_flush_patch_quiet(). However, I'm unsure whether we should remove the dry-run logic. In dry-run mode, we would halt as early as possible in xdl_diff by using quick_consume().