From: Johannes Schindelin <Johannes.Schindelin@gmx•de>
To: Jeff King <peff@peff•net>
Cc: Jake Zimmerman <jake@zimmerman•io>,
Lidong Yan <yldhome2d2@gmail•com>,
Junio C Hamano <gitster@pobox•com>,
git@vger•kernel.org
Subject: Re: Regression in `git diff --quiet HEAD` when a new file is staged
Date: Fri, 17 Oct 2025 13:44:13 +0200 (CEST) [thread overview]
Message-ID: <06a127d0-9c4b-6ee3-4e37-1ff768e5f39a@gmx.de> (raw)
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
next prev parent reply other threads:[~2025-10-17 11:44 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 ` Johannes Schindelin [this message]
2025-10-17 17:45 ` Regression in `git diff --quiet HEAD` when a new file is staged Junio C Hamano
2025-10-18 1:04 ` Lidong Yan
2025-10-18 9:42 ` Jeff King
2025-10-18 9:40 ` Jeff King
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=06a127d0-9c4b-6ee3-4e37-1ff768e5f39a@gmx.de \
--to=johannes.schindelin@gmx$(echo .)de \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=jake@zimmerman$(echo .)io \
--cc=peff@peff$(echo .)net \
--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