From: Jeff King <peff@peff.net>
Date: Sat, 18 Oct 2025 09:40:37 GMT
Subject: Re: Regression in `git diff --quiet HEAD` when a new file is staged
Message-ID: <20251018094037.GA1060824@coredump.intra.peff.net>
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