From: Jeff King <peff@peff•net>
To: Junio C Hamano <gitster@pobox•com>
Cc: Jake Zimmerman <jake@zimmerman•io>,
Lidong Yan <yldhome2d2@gmail•com>,
git@vger•kernel.org
Subject: Re: Regression in `git diff --quiet HEAD` when a new file is staged
Date: Sat, 18 Oct 2025 05:40:37 -0400 [thread overview]
Message-ID: <20251018094037.GA1060824@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqq7bwt1kyf.fsf@gitster.g>
On Fri, Oct 17, 2025 at 10:45:12AM -0700, Junio C Hamano wrote:
> > 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'm not sure which of these you mean:
- Do we still need to call diff_flush_patch_quietly() directly below
the hunk above, in diff_flush()?
The answer is no, we do not need to (just like we did not before
b55e6d36eb). But I think it is worth doing so still, because the
low-level code may be able to use the flag to do things more
efficiently.
- Do we still need the dry-run code at all?
My impression is yes, because there are other code paths which do
the dry-run thing and need it for correctness.
If I understand the motivation of b55e6d36eb, it really has multiple
parts:
1. Add a dry-run mode to the diff code.
2. Use that dry-run mode for handling -I with name-status, etc.
3. Since we now have dry-run mode, convert diff_flush()'s
/dev/null for --quiet mode to use it.
The goal was really part (2). And any bugs in (1) would show up
there, but they couldn't actually be regressions, but rather just an
incomplete fix for (2). But by doing part (3), now bugs in (1) are
regressions for --quiet. Hence my suggestion to undo just that part,
and then do fixes for (1) separately.
Or did you just mean: can we just go to a world where the _quietly()
function just redirects /dev/null rather than worrying about dry-run
at all? That is certainly an option, though I do think there is room
for more efficiency with dry-run. So I think I prefer the
belt-and-suspenders of "redirect to /dev/null just in case we miss a
spot, but also tell the low-level code nobody is looking at the
output".
-Peff
next prev parent reply other threads:[~2025-10-18 9:40 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-17 0:09 Regression in `git diff --quiet HEAD` when a new file is staged Jake Zimmerman
2025-10-17 7:51 ` Jeff King
2025-10-17 8:36 ` [PATCH] diff: restore redirection to /dev/null for diff_from_contents Jeff King
2025-10-17 18:22 ` Junio C Hamano
2025-10-19 21:09 ` Johannes Schindelin
2025-10-21 7:52 ` Jeff King
2025-10-17 11:44 ` Regression in `git diff --quiet HEAD` when a new file is staged Johannes Schindelin
2025-10-17 17:45 ` Junio C Hamano
2025-10-18 1:04 ` Lidong Yan
2025-10-18 9:42 ` Jeff King
2025-10-18 9:40 ` Jeff King [this message]
2025-10-18 15:23 ` Junio C Hamano
2025-10-21 7:36 ` Jeff King
2025-10-21 14:38 ` Junio C Hamano
2025-10-22 4:46 ` Lidong Yan
2025-10-22 9:14 ` Jeff King
2025-10-22 14:20 ` Lidong Yan
2025-10-22 14:31 ` Junio C Hamano
2025-10-22 16:28 ` Junio C Hamano
2025-10-22 9:11 ` Jeff King
2025-10-22 16:48 ` Junio C Hamano
2025-10-23 12:01 ` Jeff King
2025-10-23 12:15 ` Jeff King
2025-10-23 13:35 ` Junio C Hamano
2025-10-22 17:39 ` Junio C Hamano
2025-10-23 0:33 ` Lidong Yan
2025-10-23 13:42 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20251018094037.GA1060824@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=jake@zimmerman$(echo .)io \
--cc=yldhome2d2@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox