From: Jeff King <peff@peff.net>
Date: Tue, 21 Oct 2025 07:36:40 GMT
Subject: Re: Regression in `git diff --quiet HEAD` when a new file is staged
Message-ID: <20251021073640.GB259661@coredump.intra.peff.net>
In-Reply-To: <xmqqh5vww7xa.fsf@gitster.g>

On Sat, Oct 18, 2025 at 08:23:13AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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()?
> >
> >   - Do we still need the dry-run code at all?
> 
> Both.  We do not have to call flush_quietly() and can call the real
> thing with output disabled.  The dry-run bit was only added to
> implement the flush_quietly() variant.  If we lose the only caller
> to flush_quietly(), all of the supporting infrastructure can go.

It's not the only caller, though. b55e6d36eb added another earlier in
diff_flush(), to handle --name-status, etc (which was its original
goal). That code possibly remains broken, even with my patch, and
would wait either on Lidong's dry-run fixes, or lifting the /dev/null
into the flush_quietly() function.

> It concentrates only on the regression-fix aspect of the changes.
> Going forward, my preference is:
> 
>  * Apply your patch.  This is the base of the fix for 'maint' and
>    all branches.
> 
>  * As Lidong updates dry-run code by adding more "ah we are in
>    dry-run, so we should stop at the first change and se should be
>    silent" fixes, we can queue them on the 'master' front for the
>    preparation for a better future.  Note that the 'master' front
>    would contain your "In from_contents modes, run flush_quietly()
>    with output redirected to /dev/null".
> 
>  * Once we regain enough confidence for dry-run with the above
>    effort, we mark your "why not redirect to /dev/null for extra
>    protection?" code with NEEDSWORK comment to be removed after a
>    thorough code audit to ensure that dry-run is now sound.
> 
> And I do not mind if the NEEDSWORK comment stay there for extended
> period of time.

Yeah, that matched my thinking exactly.

But thinking on it more, I think the regression is slightly bigger than
I originally counted. My view was that:

  - the attempt to fix "-I" was incomplete but did not make anything
    worse there

  - that attempt also broke "--quiet"

So we should first un-break "--quiet" as simply as possible, and then
try to make the fix for "-I" more complete as a separate step. But I
think "-I" may actually have regressed, too, since it is subject to
printing the extra bogus output when trying to decide if the
content-diff is applicable, which it did not do before.

So really, the regression fix should probably cover both of them (which
it would if we move the /dev/null redirection into the flush_quietly()
variant).

-Peff