From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Fri, 17 Oct 2025 11:44:13 GMT
Subject: Re: Regression in `git diff --quiet HEAD` when a new file is staged
Message-ID: <06a127d0-9c4b-6ee3-4e37-1ff768e5f39a@gmx.de>
In-Reply-To: <20251017075153.GA4078773@coredump.intra.peff.net>

Hi Jeff,

On Fri, 17 Oct 2025, Jeff King wrote:

> On Thu, Oct 16, 2025 at 05:09:07PM -0700, Jake Zimmerman wrote:
> 
> > In git v2.51.1, `git diff --quiet HEAD` will actually print something
> > if the diff output includes a new, staged file.
> > [...]
> > I ran a git bisect and isolated this commit:
> > b55e6d36ebce69136559add8fffd1a65df231518
> 
> Yikes, that is a pretty bad regression. I'm rather surprised that this
> wasn't covered in the test suite. t4035 does set this situation up, but
> it checks with git-diff-tree, not git-diff. I initially thought that was
> because diff defaults to "--patch" output and diff-tree does not, but
> even "diff-tree --patch" does not show the bug. Weird. Maybe it has to
> do with running diffcore bits?
> 
> I see that the author of b55e6d36eb (diff: ensure consistent diff
> behavior with ignore options, 2025-08-08) posted this patch earlier
> today:
> 
>   https://lore.kernel.org/git/pull.2071.git.git.1760671049113.gitgitgadget@gmail.com/
> 
> which seems to fix it, but there's no mention there of this thread.

The fix predates the thread, that's why.

The reason why it "seems to fix it" is this: The `git diff --quiet HEAD`
call enters this code block
(https://github.com/git-for-windows/git/blob/rebase-to-v2.51.1/diff.c#L6876-L6886):

```c
	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
	    options->flags.exit_with_status &&
	    options->flags.diff_from_contents) {
		for (i = 0; i < q->nr; i++) {
			struct diff_filepair *p = q->queue[i];
			if (check_pair_status(p))
				diff_flush_patch_quietly(p, options);
			if (options->found_changes)
				break;
		}
	}
```

Specifically, the `diff_flush_patch_quietly()` function is called, which sets the `dry_run` flag. Later on, the `emit_diff_symbol_from_struct()` function is entered. Here is the call stack:

```
#0  emit_diff_symbol_from_struct (o=0x5ff480, eds=0x5fe910) at diff.c:1355
#1  0x00007ff7c3b275fe in emit_diff_symbol (o=0x5ff480, s=DIFF_SYMBOL_HEADER,
    line=0x3561a010380 "\033[1mdiff --git a/file b/file\033[m\n\033[1mnew file mode 100644\033[m\n\033[1mindex 0000000..e69de29\033[m\n", len=90, flags=0) at diff.c:1597
#2  0x00007ff7c3b2d602 in builtin_diff (name_a=0x3561a0702a0 "file", name_b=0x3561a0702a0 "file", one=0x3561a070240,
    two=0x3561a0702b0, xfrm_msg=0x3561a1a0500 "\033[1mindex 0000000..e69de29\033[m\n", must_show_header=1, o=0x5ff480,
    complete_rewrite=0) at diff.c:3723
#3  0x00007ff7c3b2fdc6 in run_diff_cmd (pgm=0x0, name=0x3561a0702a0 "file", other=0x0, attr_path=0x3561a0702a0 "file",
    one=0x3561a070240, two=0x3561a0702b0, msg=0x5febf0, o=0x5ff480, p=0x3561a0220c0) at diff.c:4617
#4  0x00007ff7c3b302af in run_diff (p=0x3561a0220c0, o=0x5ff480) at diff.c:4711
#5  0x00007ff7c3b353b0 in diff_flush_patch (p=0x3561a0220c0, o=0x5ff480) at diff.c:6172
#6  0x00007ff7c3b35413 in diff_flush_patch_quietly (p=0x3561a0220c0, o=0x5ff480) at diff.c:6184
#7  0x00007ff7c3b372ec in diff_flush (options=0x5ff480) at diff.c:6882
#8  0x00007ff7c3b2134f in run_diff_index (revs=0x5feed0, option=0) at diff-lib.c:643
#9  0x00007ff7c39d8427 in builtin_diff_index (revs=0x5feed0, argc=1, argv=0x3561a0202a0) at builtin/diff.c:170
#10 0x00007ff7c39d9487 in cmd_diff (argc=1, argv=0x3561a0202a0, prefix=0x0, repo=0x0) at builtin/diff.c:633
#11 0x00007ff7c39932f0 in run_builtin (p=0x7ff7c3d46368 <commands+840>, argc=3, argv=0x3561a0202a0,
    repo=0x7ff7c3e742c0 <the_repo>) at git.c:506
#12 0x00007ff7c3993849 in handle_builtin (args=0x5ffd70) at git.c:778
#13 0x00007ff7c3993b04 in run_argv (args=0x5ffd70) at git.c:861
#14 0x00007ff7c3993f56 in cmd_main (argc=3, argv=0x3561a0300e0) at git.c:983
#15 0x00007ff7c3ab0a7e in main (argc=7, argv=0x3561a0300c0) at common-main.c:9
```

The `if (o->dry_run) return;` guard introduced in the fix from
https://lore.kernel.org/git/pull.2071.git.git.1760671049113.gitgitgadget@gmail.com/
will then suppress the output, as desired.

> Looking at that patch, my biggest concern is: are we missing other spots
> that need to special-case the dry_run setting?

That's an excellent concern to have, seeing as bugs love like company.

A comparatively deeper analysis shows that the `o->file` attribute is used
in these functions that are not guarded by the early return introduced in
the proposed fix:

- show_numstat()
- gather_dirstat()
- checkdiff_consume()
- builtin_checkdiff()
- run_diff_cmd() (unmerged paths)
- diff_flush_raw()
- flush_one_pair() (DIFF_FORMAT_NAME)

Of these, I think the only concerning one is the one in `run_diff_cmd()`.

Ciao,
Johannes