{"thread":{"id":"64340","subject":"Regression in `git diff --quiet HEAD` when a new file is staged","startedAt":"2025-10-17T00:09:20Z","lastAt":"2025-10-23T13:42:41Z","messageCount":27,"participants":["Jake Zimmerman","Jeff King","Johannes Schindelin","Junio C Hamano","Lidong Yan"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"529033","messageId":"CACJRbWjwOQwJB13CwTfvhV3p+Hbn4KrNM9AtBanGtUS4V_1MbQ@mail.gmail.com","threadId":"64340","inReplyTo":null,"subject":"Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Jake Zimmerman","fromEmail":"jake@zimmerman.io","sentAt":"2025-10-17T00:09:07Z","receivedAt":"2025-10-17T00:09:20Z","isPatch":false,"sender":{"key":"jake@zimmerman.io","avatar":null},"body":"In git v2.51.1, `git diff --quiet HEAD` will actually print something\nif the diff output includes a new, staged file.\n\n## To reproduce\n\n ❯ mkdir foo\n ❯ cd foo\n ❯ git init .\n Initialized empty Git repository in /Users/jez/foo/.git/\n ❯ gc --allow-empty -m \"Initial empty commit\"\n [master (root-commit) 858966f] Initial empty commit\n ❯ touch foo.txt\n ❯ git add foo.txt\n ❯ git diff --quiet HEAD\n\nOn git v2.51.0, the output of the last command is empty.\nOn git v2.51.1, the output of the last command is this:\n\n diff --git a/foo.txt b/foo.txt\n new file mode 100644\n index 0000000..e69de29\n\n## Expected behavior\n\nThe stated docs for `--quiet`: \"Disable all output of the program,\" so\nI expect there to be no output, like in older versions.\n\n## Likely cause\n\nI ran a git bisect and isolated this commit:\n\nb55e6d36ebce69136559add8fffd1a65df231518\n( https://github.com/git/git/commit/e1d3d61a45bfdc5031d2066c0e4505ebd8145777 )\n\n\"diff: ensure consistent diff behavior with ignore options\"\n"},{"id":"529044","messageId":"20251017075153.GA4078773@coredump.intra.peff.net","threadId":"64340","inReplyTo":"CACJRbWjwOQwJB13CwTfvhV3p+Hbn4KrNM9AtBanGtUS4V_1MbQ@mail.gmail.com","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-17T07:51:53Z","receivedAt":"2025-10-17T07:51:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 16, 2025 at 05:09:07PM -0700, Jake Zimmerman wrote:\n\n> In git v2.51.1, `git diff --quiet HEAD` will actually print something\n> if the diff output includes a new, staged file.\n> [...]\n> I ran a git bisect and isolated this commit:\n> b55e6d36ebce69136559add8fffd1a65df231518\n\nYikes, that is a pretty bad regression. I'm rather surprised that this\nwasn't covered in the test suite. t4035 does set this situation up, but\nit checks with git-diff-tree, not git-diff. I initially thought that was\nbecause diff defaults to \"--patch\" output and diff-tree does not, but\neven \"diff-tree --patch\" does not show the bug. Weird. Maybe it has to\ndo with running diffcore bits?\n\nI see that the author of b55e6d36eb (diff: ensure consistent diff\nbehavior with ignore options, 2025-08-08) posted this patch earlier\ntoday:\n\n https://lore.kernel.org/git/pull.2071.git.git.1760671049113.gitgitgadget@gmail.com/\n\nwhich seems to fix it, but there's no mention there of this thread. And\nthe included test is still using \"-I\", where there is clearly collateral\ndamage even for people who are not using \"-I\" at all. So I'm not sure if\nit's coincidence, or meant to be a fix. ;)\n\nLooking at that patch, my biggest concern is: are we missing other spots\nthat need to special-case the dry_run setting? Because it's a regression\nin a maint release, I'm tempted to say we should do the dumbest possible\nthing that covers all cases and just revert this hunk from the original\npatch, like:\n\ndiff --git a/diff.c b/diff.c\nindex 87fa16b730..687206f353 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)\n \tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n \t options->flags.exit_with_status &&\n \t options->flags.diff_from_contents) {\n+\t\t/*\n+\t\t * run diff_flush_patch for the exit status. setting\n+\t\t * options->file to /dev/null should be safe, because we\n+\t\t * aren't supposed to produce any output anyway.\n+\t\t */\n+\t\tdiff_free_file(options);\n+\t\toptions->file = xfopen(\"/dev/null\", \"w\");\n+\t\toptions->close_file = 1;\n+\t\toptions->color_moved = 0;\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n \t\t\tif (check_pair_status(p))\n\nThat would catch the bug here, as well as any others lurking. And it\nconverts any missing dry_run from correctness problems (we definitely\nwill not produce extra output) into optimization problems (we might emit\ndata we do not need, but we can fix those separately). At least for the\nnormal code paths. I think without those extra fixes the problems that\nb55e6d36eb tried to fix for \"-I\" would still be observable, but at least\nits fixes could not regress the other code paths.\n\n-Peff\n"},{"id":"529047","messageId":"20251017083641.GB4073661@coredump.intra.peff.net","threadId":"64340","inReplyTo":"20251017075153.GA4078773@coredump.intra.peff.net","subject":"[PATCH] diff: restore redirection to /dev/null for diff_from_contents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-17T08:36:41Z","receivedAt":"2025-10-17T08:36:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 17, 2025 at 03:51:53AM -0400, Jeff King wrote:\n\n> On Thu, Oct 16, 2025 at 05:09:07PM -0700, Jake Zimmerman wrote:\n> \n> > In git v2.51.1, `git diff --quiet HEAD` will actually print something\n> > if the diff output includes a new, staged file.\n> > [...]\n> > I ran a git bisect and isolated this commit:\n> > b55e6d36ebce69136559add8fffd1a65df231518\n> \n> Yikes, that is a pretty bad regression. I'm rather surprised that this\n> wasn't covered in the test suite. t4035 does set this situation up, but\n> it checks with git-diff-tree, not git-diff. I initially thought that was\n> because diff defaults to \"--patch\" output and diff-tree does not, but\n> even \"diff-tree --patch\" does not show the bug. Weird. Maybe it has to\n> do with running diffcore bits?\n\nAh, I see. It is because porcelain diff has --ext-diff turned on by\ndefault. And that triggers content-level diffs due to this bit in\ndiff_setup_done():\n\n /*\n * External diffs could declare non-identical contents equal\n * (think diff --ignore-space-change).\n */\n if (options->flags.allow_external && options->flags.exit_with_status)\n options->flags.diff_from_contents = 1;\n\nThe really gross part, of course, is that this triggers even when you do\nnot have any external diff commands defined, because we don't find out\nabout them until flushing individual pairs!\n\nI suspect that things could be improved there. Once we're in\nrun_diff_cmd() and realize that no, we don't have have an external diff\ncommand, I think we still run the actual diff anyway, not realizing that\nwe are only here on a contingency that is not true. So we produce the\ndiff and throw it away, but could return early. But again, that's an\noptimization issue, not a correctness one (and it has been that way for\nmany years, so perhaps nobody cares too much).\n\nI also suspect that textconv should get the same treatment (you could\ndefine a textconv that turns two distinct binary blobs into an identical\ntext, so we should trigger a content diff for that). But again, it has\nbeen that way for years.\n\n> Looking at that patch, my biggest concern is: are we missing other spots\n> that need to special-case the dry_run setting? Because it's a regression\n> in a maint release, I'm tempted to say we should do the dumbest possible\n> thing that covers all cases and just revert this hunk from the original\n> patch, like:\n\nHere it is with a commit message and test, in case that is helpful.\n\n-- >8 --\nSubject: [PATCH] diff: restore redirection to /dev/null for diff_from_contents\n\nIn --quiet mode, since we produce only an exit code for \"something was\nchanged\" and no actual output, we can often get by with just a\ntree-level diff. However, certain options require us to actually look at\nthe file contents (e.g., if we are ignoring whitespace changes). We have\na flag \"diff_from_contents\" for that, and if it is set we call\ndiff_flush() on each path.\n\nTo avoid producing any output (since we were asked to be --quiet), we\ntraditionally just redirected the output to /dev/null. That changed in\nb55e6d36eb (diff: ensure consistent diff behavior with ignore options,\n2025-08-08), which replaced that with a \"dry_run\" flag. In theory, with\ndry_run set, we should produce no output. But it carries a risk of\nregression: if we forget to respect dry_run in any of the output paths,\nwe'll accidentally produce output.\n\nAnd indeed, there is at least one such regression in that commit, as it\ncovered only the case where we actually call into xdiff, and not\ncreation or deletion diffs, where we manually generate the headers. We\neven test this case in t4035, but only with diff-tree, which does not\nshow the bug by default because it does not require diff_from_contents.\nBut git-diff does, because it allows external diff programs by default\n(so we must dig into each diff filepair to decide if it requires running\nan external diff that may declare two distinct blobs to actually be the\nsame).\n\nWe should fix all of those code paths to respect dry_run correctly, but\nin the meantime we can protect ourselves more fully by restoring the\nredirection to /dev/null. This gives us an extra layer of protection\nagainst regressions dues to other code paths we've missed.\n\nThough the original issue was reported with \"git diff\" (and due to its\ndefault of --ext-diff), I've used \"diff-tree -w\" in the new test. It\ntriggers the same issue, but I think the fact that \"-w\" implies\ndiff_from_contents is a bit more obvious, and fits in with the rest of\nt4035.\n\nReported-by: Jake Zimmerman \nSigned-off-by: Jeff King \n---\nI didn't test, but I also wondered if this might be necessary to avoid\nactual external diff programs from spewing to stdout. Looking at\nrun_external_diff(), we do:\n\n int quiet = !(o->output_format & DIFF_FORMAT_PATCH);\n [...]\n cmd.no_stdout = quiet;\n\nso I _think_ it should be OK even without this patch. But again, I like\nthe extra layer of protection here.\n\n diff.c | 9 +++++++++\n t/t4035-diff-quiet.sh | 4 ++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 87fa16b730..687206f353 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)\n \tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n \t options->flags.exit_with_status &&\n \t options->flags.diff_from_contents) {\n+\t\t/*\n+\t\t * run diff_flush_patch for the exit status. setting\n+\t\t * options->file to /dev/null should be safe, because we\n+\t\t * aren't supposed to produce any output anyway.\n+\t\t */\n+\t\tdiff_free_file(options);\n+\t\toptions->file = xfopen(\"/dev/null\", \"w\");\n+\t\toptions->close_file = 1;\n+\t\toptions->color_moved = 0;\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n \t\t\tif (check_pair_status(p))\ndiff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh\nindex 0352bf81a9..35eaf0855f 100755\n--- a/t/t4035-diff-quiet.sh\n+++ b/t/t4035-diff-quiet.sh\n@@ -50,6 +50,10 @@ test_expect_success 'git diff-tree HEAD HEAD' '\n \ttest_expect_code 0 git diff-tree --quiet HEAD HEAD >cnt &&\n \ttest_line_count = 0 cnt\n '\n+test_expect_success 'git diff-tree -w HEAD^ HEAD' '\n+\ttest_expect_code 1 git diff-tree --quiet -w HEAD^ HEAD >cnt &&\n+\ttest_line_count = 0 cnt\n+'\n test_expect_success 'git diff-files' '\n \ttest_expect_code 0 git diff-files --quiet >cnt &&\n \ttest_line_count = 0 cnt\n-- \n2.51.1.685.g6bf3278fbc\n\n"},{"id":"529061","messageId":"06a127d0-9c4b-6ee3-4e37-1ff768e5f39a@gmx.de","threadId":"64340","inReplyTo":"20251017075153.GA4078773@coredump.intra.peff.net","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-10-17T11:44:13Z","receivedAt":"2025-10-17T11:44:28Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jeff,\n\nOn Fri, 17 Oct 2025, Jeff King wrote:\n\n> On Thu, Oct 16, 2025 at 05:09:07PM -0700, Jake Zimmerman wrote:\n> \n> > In git v2.51.1, `git diff --quiet HEAD` will actually print something\n> > if the diff output includes a new, staged file.\n> > [...]\n> > I ran a git bisect and isolated this commit:\n> > b55e6d36ebce69136559add8fffd1a65df231518\n> \n> Yikes, that is a pretty bad regression. I'm rather surprised that this\n> wasn't covered in the test suite. t4035 does set this situation up, but\n> it checks with git-diff-tree, not git-diff. I initially thought that was\n> because diff defaults to \"--patch\" output and diff-tree does not, but\n> even \"diff-tree --patch\" does not show the bug. Weird. Maybe it has to\n> do with running diffcore bits?\n> \n> I see that the author of b55e6d36eb (diff: ensure consistent diff\n> behavior with ignore options, 2025-08-08) posted this patch earlier\n> today:\n> \n> https://lore.kernel.org/git/pull.2071.git.git.1760671049113.gitgitgadget@gmail.com/\n> \n> which seems to fix it, but there's no mention there of this thread.\n\nThe fix predates the thread, that's why.\n\nThe reason why it \"seems to fix it\" is this: The `git diff --quiet HEAD`\ncall enters this code block\n(https://github.com/git-for-windows/git/blob/rebase-to-v2.51.1/diff.c#L6876-L6886):\n\n```c\n\tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n\t options->flags.exit_with_status &&\n\t options->flags.diff_from_contents) {\n\t\tfor (i = 0; i < q->nr; i++) {\n\t\t\tstruct diff_filepair *p = q->queue[i];\n\t\t\tif (check_pair_status(p))\n\t\t\t\tdiff_flush_patch_quietly(p, options);\n\t\t\tif (options->found_changes)\n\t\t\t\tbreak;\n\t\t}\n\t}\n```\n\nSpecifically, 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:\n\n```\n#0 emit_diff_symbol_from_struct (o=0x5ff480, eds=0x5fe910) at diff.c:1355\n#1 0x00007ff7c3b275fe in emit_diff_symbol (o=0x5ff480, s=DIFF_SYMBOL_HEADER,\n 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\n#2 0x00007ff7c3b2d602 in builtin_diff (name_a=0x3561a0702a0 \"file\", name_b=0x3561a0702a0 \"file\", one=0x3561a070240,\n two=0x3561a0702b0, xfrm_msg=0x3561a1a0500 \"\\033[1mindex 0000000..e69de29\\033[m\\n\", must_show_header=1, o=0x5ff480,\n complete_rewrite=0) at diff.c:3723\n#3 0x00007ff7c3b2fdc6 in run_diff_cmd (pgm=0x0, name=0x3561a0702a0 \"file\", other=0x0, attr_path=0x3561a0702a0 \"file\",\n one=0x3561a070240, two=0x3561a0702b0, msg=0x5febf0, o=0x5ff480, p=0x3561a0220c0) at diff.c:4617\n#4 0x00007ff7c3b302af in run_diff (p=0x3561a0220c0, o=0x5ff480) at diff.c:4711\n#5 0x00007ff7c3b353b0 in diff_flush_patch (p=0x3561a0220c0, o=0x5ff480) at diff.c:6172\n#6 0x00007ff7c3b35413 in diff_flush_patch_quietly (p=0x3561a0220c0, o=0x5ff480) at diff.c:6184\n#7 0x00007ff7c3b372ec in diff_flush (options=0x5ff480) at diff.c:6882\n#8 0x00007ff7c3b2134f in run_diff_index (revs=0x5feed0, option=0) at diff-lib.c:643\n#9 0x00007ff7c39d8427 in builtin_diff_index (revs=0x5feed0, argc=1, argv=0x3561a0202a0) at builtin/diff.c:170\n#10 0x00007ff7c39d9487 in cmd_diff (argc=1, argv=0x3561a0202a0, prefix=0x0, repo=0x0) at builtin/diff.c:633\n#11 0x00007ff7c39932f0 in run_builtin (p=0x7ff7c3d46368 , argc=3, argv=0x3561a0202a0,\n repo=0x7ff7c3e742c0 ) at git.c:506\n#12 0x00007ff7c3993849 in handle_builtin (args=0x5ffd70) at git.c:778\n#13 0x00007ff7c3993b04 in run_argv (args=0x5ffd70) at git.c:861\n#14 0x00007ff7c3993f56 in cmd_main (argc=3, argv=0x3561a0300e0) at git.c:983\n#15 0x00007ff7c3ab0a7e in main (argc=7, argv=0x3561a0300c0) at common-main.c:9\n```\n\nThe `if (o->dry_run) return;` guard introduced in the fix from\nhttps://lore.kernel.org/git/pull.2071.git.git.1760671049113.gitgitgadget@gmail.com/\nwill then suppress the output, as desired.\n\n> Looking at that patch, my biggest concern is: are we missing other spots\n> that need to special-case the dry_run setting?\n\nThat's an excellent concern to have, seeing as bugs love like company.\n\nA comparatively deeper analysis shows that the `o->file` attribute is used\nin these functions that are not guarded by the early return introduced in\nthe proposed fix:\n\n- show_numstat()\n- gather_dirstat()\n- checkdiff_consume()\n- builtin_checkdiff()\n- run_diff_cmd() (unmerged paths)\n- diff_flush_raw()\n- flush_one_pair() (DIFF_FORMAT_NAME)\n\nOf these, I think the only concerning one is the one in `run_diff_cmd()`.\n\nCiao,\nJohannes\n"},{"id":"529086","messageId":"xmqq7bwt1kyf.fsf@gitster.g","threadId":"64340","inReplyTo":"20251017075153.GA4078773@coredump.intra.peff.net","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-17T17:45:12Z","receivedAt":"2025-10-17T17:45:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Looking at that patch, my biggest concern is: are we missing other spots\n> that need to special-case the dry_run setting? Because it's a regression\n> in a maint release, I'm tempted to say we should do the dumbest possible\n> thing that covers all cases and just revert this hunk from the original\n> patch, like:\n>\n> diff --git a/diff.c b/diff.c\n> index 87fa16b730..687206f353 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)\n> \tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n> \t options->flags.exit_with_status &&\n> \t options->flags.diff_from_contents) {\n> +\t\t/*\n> +\t\t * run diff_flush_patch for the exit status. setting\n> +\t\t * options->file to /dev/null should be safe, because we\n> +\t\t * aren't supposed to produce any output anyway.\n> +\t\t */\n> +\t\tdiff_free_file(options);\n> +\t\toptions->file = xfopen(\"/dev/null\", \"w\");\n> +\t\toptions->close_file = 1;\n> +\t\toptions->color_moved = 0;\n> \t\tfor (i = 0; i < q->nr; i++) {\n> \t\t\tstruct diff_filepair *p = q->queue[i];\n> \t\t\tif (check_pair_status(p))\n>\n> That would catch the bug here, as well as any others lurking. And it\n> converts any missing dry_run from correctness problems (we definitely\n> will not produce extra output) into optimization problems (we might emit\n> data we do not need, but we can fix those separately). At least for the\n> normal code paths. I think without those extra fixes the problems that\n> b55e6d36eb tried to fix for \"-I\" would still be observable, but at least\n> its fixes could not regress the other code paths.\n\nAhh. I like this \"stupid but cannot be incorrect\" version even\nbetter than the original one that introduced the \"dry run\" mode.\n\nBut once we go in that direction, do we still need the dry-run\nmachinery with diff_flush_patch_quietly() helper function?\n\n"},{"id":"529087","messageId":"xmqqzf9pz8uq.fsf@gitster.g","threadId":"64340","inReplyTo":"20251017083641.GB4073661@coredump.intra.peff.net","subject":"Re: [PATCH] diff: restore redirection to /dev/null for diff_from_contents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-17T18:22:37Z","receivedAt":"2025-10-17T18:22:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Looking at that patch, my biggest concern is: are we missing other spots\n>> that need to special-case the dry_run setting? Because it's a regression\n>> in a maint release, I'm tempted to say we should do the dumbest possible\n>> thing that covers all cases and just revert this hunk from the original\n>> patch, like:\n>\n> Here it is with a commit message and test, in case that is helpful.\n>\n> -- >8 --\n> Subject: [PATCH] diff: restore redirection to /dev/null for diff_from_contents\n> ...\n> I didn't test, but I also wondered if this might be necessary to avoid\n> actual external diff programs from spewing to stdout. Looking at\n> run_external_diff(), we do:\n>\n> int quiet = !(o->output_format & DIFF_FORMAT_PATCH);\n> [...]\n> cmd.no_stdout = quiet;\n>\n> so I _think_ it should be OK even without this patch. But again, I like\n> the extra layer of protection here.\n\nI do like this direction, in addition I really do appreciate your\nthought above on optimizing ext-diff and textconv away when they are\nnot necessary (obviously outside the scope of the regression fix).\n\nI also wonder if we want to get rid of the new code related to the\n\"dry-run\" mode that we no longer have to use. But as a regression\nfix that wants to be minimum, I think this patch stops at the right\nplace.\n\nThanks.\n\n\n> diff.c | 9 +++++++++\n> t/t4035-diff-quiet.sh | 4 ++++\n> 2 files changed, 13 insertions(+)\n>\n> diff --git a/diff.c b/diff.c\n> index 87fa16b730..687206f353 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)\n> \tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n> \t options->flags.exit_with_status &&\n> \t options->flags.diff_from_contents) {\n> +\t\t/*\n> +\t\t * run diff_flush_patch for the exit status. setting\n> +\t\t * options->file to /dev/null should be safe, because we\n> +\t\t * aren't supposed to produce any output anyway.\n> +\t\t */\n> +\t\tdiff_free_file(options);\n> +\t\toptions->file = xfopen(\"/dev/null\", \"w\");\n> +\t\toptions->close_file = 1;\n> +\t\toptions->color_moved = 0;\n> \t\tfor (i = 0; i < q->nr; i++) {\n> \t\t\tstruct diff_filepair *p = q->queue[i];\n> \t\t\tif (check_pair_status(p))\n> diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh\n> index 0352bf81a9..35eaf0855f 100755\n> --- a/t/t4035-diff-quiet.sh\n> +++ b/t/t4035-diff-quiet.sh\n> @@ -50,6 +50,10 @@ test_expect_success 'git diff-tree HEAD HEAD' '\n> \ttest_expect_code 0 git diff-tree --quiet HEAD HEAD >cnt &&\n> \ttest_line_count = 0 cnt\n> '\n> +test_expect_success 'git diff-tree -w HEAD^ HEAD' '\n> +\ttest_expect_code 1 git diff-tree --quiet -w HEAD^ HEAD >cnt &&\n> +\ttest_line_count = 0 cnt\n> +'\n> test_expect_success 'git diff-files' '\n> \ttest_expect_code 0 git diff-files --quiet >cnt &&\n> \ttest_line_count = 0 cnt\n"},{"id":"529110","messageId":"918E56B8-7009-4E8E-A98E-AC5B9CE4DD7C@gmail.com","threadId":"64340","inReplyTo":"xmqq7bwt1kyf.fsf@gitster.g","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-10-18T01:04:40Z","receivedAt":"2025-10-18T01:05:24Z","isPatch":false,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Junio C Hamano writes:\n> \n> Jeff King writes:\n> \n>> Looking at that patch, my biggest concern is: are we missing other spots\n>> that need to special-case the dry_run setting? Because it's a regression\n>> in a maint release, I'm tempted to say we should do the dumbest possible\n>> thing that covers all cases and just revert this hunk from the original\n>> patch, like:\n>> \n>> diff --git a/diff.c b/diff.c\n>> index 87fa16b730..687206f353 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)\n>> if (output_format & DIFF_FORMAT_NO_OUTPUT &&\n>> options->flags.exit_with_status &&\n>> options->flags.diff_from_contents) {\n>> + /*\n>> + * run diff_flush_patch for the exit status. setting\n>> + * options->file to /dev/null should be safe, because we\n>> + * aren't supposed to produce any output anyway.\n>> + */\n>> + diff_free_file(options);\n>> + options->file = xfopen(\"/dev/null\", \"w\");\n>> + options->close_file = 1;\n>> + options->color_moved = 0;\n>> for (i = 0; i < q->nr; i++) {\n>> struct diff_filepair *p = q->queue[i];\n>> if (check_pair_status(p))\n>> \n>> That would catch the bug here, as well as any others lurking. And it\n>> converts any missing dry_run from correctness problems (we definitely\n>> will not produce extra output) into optimization problems (we might emit\n>> data we do not need, but we can fix those separately). At least for the\n>> normal code paths. I think without those extra fixes the problems that\n>> b55e6d36eb tried to fix for \"-I\" would still be observable, but at least\n>> its fixes could not regress the other code paths.\n> \n> Ahh. I like this \"stupid but cannot be incorrect\" version even\n> better than the original one that introduced the \"dry run\" mode.\n> \n> But once we go in that direction, do we still need the dry-run\n> machinery with diff_flush_patch_quietly() helper function?\n\nI believe we can move Peff’s code from diff_flush() to diff_flush_patch_quiet().\nHowever, I'm unsure whether we should remove the dry-run logic. In dry-run\nmode, we would halt as early as possible in xdl_diff by using quick_consume().\n\n"},{"id":"529115","messageId":"20251018094037.GA1060824@coredump.intra.peff.net","threadId":"64340","inReplyTo":"xmqq7bwt1kyf.fsf@gitster.g","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-18T09:40:37Z","receivedAt":"2025-10-18T09:40:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 17, 2025 at 10:45:12AM -0700, Junio C Hamano wrote:\n\n> > diff --git a/diff.c b/diff.c\n> > index 87fa16b730..687206f353 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)\n> > \tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n> > \t options->flags.exit_with_status &&\n> > \t options->flags.diff_from_contents) {\n> > +\t\t/*\n> > +\t\t * run diff_flush_patch for the exit status. setting\n> > +\t\t * options->file to /dev/null should be safe, because we\n> > +\t\t * aren't supposed to produce any output anyway.\n> > +\t\t */\n> > +\t\tdiff_free_file(options);\n> > +\t\toptions->file = xfopen(\"/dev/null\", \"w\");\n> > +\t\toptions->close_file = 1;\n> > +\t\toptions->color_moved = 0;\n> > \t\tfor (i = 0; i < q->nr; i++) {\n> > \t\t\tstruct diff_filepair *p = q->queue[i];\n> > \t\t\tif (check_pair_status(p))\n> >\n> > That would catch the bug here, as well as any others lurking. And it\n> > converts any missing dry_run from correctness problems (we definitely\n> > will not produce extra output) into optimization problems (we might emit\n> > data we do not need, but we can fix those separately). At least for the\n> > normal code paths. I think without those extra fixes the problems that\n> > b55e6d36eb tried to fix for \"-I\" would still be observable, but at least\n> > its fixes could not regress the other code paths.\n> \n> Ahh. I like this \"stupid but cannot be incorrect\" version even\n> better than the original one that introduced the \"dry run\" mode.\n> \n> But once we go in that direction, do we still need the dry-run\n> machinery with diff_flush_patch_quietly() helper function?\n\nI'm not sure which of these you mean:\n\n - Do we still need to call diff_flush_patch_quietly() directly below\n the hunk above, in diff_flush()?\n\n The answer is no, we do not need to (just like we did not before\n b55e6d36eb). But I think it is worth doing so still, because the\n low-level code may be able to use the flag to do things more\n efficiently.\n\n - Do we still need the dry-run code at all?\n\n My impression is yes, because there are other code paths which do\n the dry-run thing and need it for correctness.\n\n If I understand the motivation of b55e6d36eb, it really has multiple\n parts:\n\n 1. Add a dry-run mode to the diff code.\n\n 2. Use that dry-run mode for handling -I with name-status, etc.\n\n 3. Since we now have dry-run mode, convert diff_flush()'s\n\t /dev/null for --quiet mode to use it.\n\n The goal was really part (2). And any bugs in (1) would show up\n there, but they couldn't actually be regressions, but rather just an\n incomplete fix for (2). But by doing part (3), now bugs in (1) are\n regressions for --quiet. Hence my suggestion to undo just that part,\n and then do fixes for (1) separately.\n\n Or did you just mean: can we just go to a world where the _quietly()\n function just redirects /dev/null rather than worrying about dry-run\n at all? That is certainly an option, though I do think there is room\n for more efficiency with dry-run. So I think I prefer the\n belt-and-suspenders of \"redirect to /dev/null just in case we miss a\n spot, but also tell the low-level code nobody is looking at the\n output\".\n\n-Peff\n"},{"id":"529116","messageId":"20251018094245.GB1060824@coredump.intra.peff.net","threadId":"64340","inReplyTo":"918E56B8-7009-4E8E-A98E-AC5B9CE4DD7C@gmail.com","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-18T09:42:45Z","receivedAt":"2025-10-18T09:42:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 18, 2025 at 09:04:40AM +0800, Lidong Yan wrote:\n\n> I believe we can move Peff’s code from diff_flush() to diff_flush_patch_quiet().\n> However, I'm unsure whether we should remove the dry-run logic. In dry-run\n> mode, we would halt as early as possible in xdl_diff by using quick_consume().\n\nYeah, exactly.\n\nI am OK to put the /dev/null code into diff_flush_patch_quiet(). That\nwould give all callers the same belt-and-suspenders protection.\n\nThe patch I posted put it where it was because that's where it was prior\nto b55e6d36eb. It is essentially a revert (because I only wanted one\nhunk I didn't call \"git revert\", but rather did a reversed patch\napplication).\n\nBut after that revert, I think it would be reasonable to move the code\non top (with the justification that it is helping the other caller of\nthe _quiet function).\n\n-Peff\n"},{"id":"529128","messageId":"xmqqh5vww7xa.fsf@gitster.g","threadId":"64340","inReplyTo":"20251018094037.GA1060824@coredump.intra.peff.net","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-18T15:23:13Z","receivedAt":"2025-10-18T15:23:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm not sure which of these you mean:\n>\n> - Do we still need to call diff_flush_patch_quietly() directly below\n> the hunk above, in diff_flush()?\n>\n> - Do we still need the dry-run code at all?\n\nBoth. We do not have to call flush_quietly() and can call the real\nthing with output disabled. The dry-run bit was only added to\nimplement the flush_quietly() variant. If we lose the only caller\nto flush_quietly(), all of the supporting infrastructure can go.\n\nIt concentrates only on the regression-fix aspect of the changes.\nGoing forward, my preference is:\n\n * Apply your patch. This is the base of the fix for 'maint' and\n all branches.\n\n * As Lidong updates dry-run code by adding more \"ah we are in\n dry-run, so we should stop at the first change and se should be\n silent\" fixes, we can queue them on the 'master' front for the\n preparation for a better future. Note that the 'master' front\n would contain your \"In from_contents modes, run flush_quietly()\n with output redirected to /dev/null\".\n\n * Once we regain enough confidence for dry-run with the above\n effort, we mark your \"why not redirect to /dev/null for extra\n protection?\" code with NEEDSWORK comment to be removed after a\n thorough code audit to ensure that dry-run is now sound.\n\nAnd I do not mind if the NEEDSWORK comment stay there for extended\nperiod of time.\n\nThanks.\n"},{"id":"529154","messageId":"d5895f9c-5b3c-7a69-46e0-cf16cda5bf3a@gmx.de","threadId":"64340","inReplyTo":"20251017083641.GB4073661@coredump.intra.peff.net","subject":"Re: [PATCH] diff: restore redirection to /dev/null for diff_from_contents","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-10-19T21:09:28Z","receivedAt":"2025-10-19T21:09:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jeff,\n\nOn Fri, 17 Oct 2025, Jeff King wrote:\n\n> diff --git a/diff.c b/diff.c\n> index 87fa16b730..687206f353 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)\n> \tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n> \t options->flags.exit_with_status &&\n> \t options->flags.diff_from_contents) {\n> +\t\t/*\n> +\t\t * run diff_flush_patch for the exit status. setting\n> +\t\t * options->file to /dev/null should be safe, because we\n> +\t\t * aren't supposed to produce any output anyway.\n> +\t\t */\n> +\t\tdiff_free_file(options);\n> +\t\toptions->file = xfopen(\"/dev/null\", \"w\");\n> +\t\toptions->close_file = 1;\n> +\t\toptions->color_moved = 0;\n\nI do not see any discussion about the `color_moved` line in\nhttps://lore.kernel.org/git/20250808033019.78817-1-yldhome2d2@gmail.com/#r,\nnor here.\n\nSince you re-add it, I consider at least a little bit of reasonsing in\norder, e.g. why this is necessary, and if it is necessary, why isn't\n`options->use_color` forced to 0 also?\n\nTaking a step back to see the 100ft view, I can understand why you want\nthat \"extra level of protection\" here. An even more important thing, that\nis missing, is a plan to avoid the need for this protection.\n\nGiven that you're still on GitHub's payroll if the hallway rumors are\ncorrect, I am quite a bit puzzled that you did not immediately reach for\nCodeQL (which is a GitHub-sponsored technology, after all) to get clarity\non the code paths that would make this exra \"layer of protection\" still\nnecessary, and thereby provide said plan.\n\nI started an AI-assisted brainstorm session and ended up with this query\n(which is neither as concise nor as comprehensible as I would have liked,\nbut at least it does the job of finding the `run_diff_cmd()` code path\nthat I also find, and no other code path, and in v4 of Lidong Yan's patch,\nit finds no remaining code path):\n\n```codeql\n/**\n * @name Potential file write during a dry run\n * @description Traces paths where `diff_options->dry_run` is set to non-zero\n * and the corresponding `diff_options->file` is later used.\n * @kind path-problem\n * @problem.severity warning\n * @id cpp/potential-dry-run-file-write\n * @tags correctness\n */\n\nimport cpp\nimport semmle.code.cpp.dataflow.new.DataFlow\nimport semmle.code.cpp.controlflow.IRGuards as IRGuards\nimport semmle.code.cpp.exprs.LogicalOperation\nimport semmle.code.cpp.exprs.ComparisonOperation\nimport semmle.code.cpp.exprs.Literal\n\n/** Holds when `assign` sets `dry_run` to a non-zero literal. */\npredicate setsDryRunNonZero(AssignExpr assign, FieldAccess dryRunField) {\n dryRunField.getTarget().hasName(\"dry_run\") and\n assign.getLValue() = dryRunField and\n assign.getOperator() = \"=\" and\n isNonZeroLiteralExpr(assign.getRValue())\n}\n\n/** True when `expr` is literally zero (allowing common suffixes). */\npredicate isZeroLiteralExpr(Expr expr) {\n exists(Literal lit |\n expr = lit and\n lit.getValueText().regexpMatch(\"(?i)\\\\s*0[uUlL]*\\\\s*\")\n )\n}\n\n/** True when `expr` is a literal that is definitely non-zero. */\npredicate isNonZeroLiteralExpr(Expr expr) {\n exists(Literal lit |\n expr = lit and\n not lit.getValueText().regexpMatch(\"(?i)\\\\s*0[uUlL]*\\\\s*\")\n )\n}\n\n/** Holds if `access` uses the `file` member of a diff_options instance. */\npredicate usesFileField(FieldAccess access) {\n access.getTarget().hasName(\"file\")\n}\n\n/** True when an `if (options->dry_run)` immediately returns. */\npredicate earlyReturnOnDryRun(FieldAccess fa) {\n fa.getTarget().hasName(\"dry_run\") and\n exists(IfStmt ifStmt, ReturnStmt ret |\n ret = ifStmt.getThen() and\n ifStmt.getCondition() = fa and\n not exists(Stmt elseStmt | elseStmt = ifStmt.getElse())\n )\n}\n\n/** Data-flow configuration tracking diff options pointers while dry-run is enabled. */\nmodule DryRunConfig implements DataFlow::ConfigSig {\n /** Sources: the `diff_options *` pointer whose `dry_run` field is set to non-zero. */\n predicate isSource(DataFlow::Node source) {\n exists(AssignExpr assign, FieldAccess dryRunField |\n setsDryRunNonZero(assign, dryRunField) and\n source.asExpr() = dryRunField.getQualifier()\n )\n }\n\n /** Sinks: any dereference of the `file` field through that diff options pointer. */\n predicate isSink(DataFlow::Node sink) {\n exists(FieldAccess access |\n usesFileField(access) and\n sink.asExpr() = access.getQualifier()\n )\n }\n\n /** Barriers: proofs that `dry_run` is zero or explicit resets back to zero. */\n predicate isBarrier(DataFlow::Node barrier) {\n exists(IRGuards::GuardCondition guard, FieldAccess fa |\n fa.getTarget().hasName(\"dry_run\") and\n guard.getAChild*() = fa and\n barrier.asExpr() = fa.getQualifier() and\n safeDryRunCheck(guard, fa)\n )\n or\n exists(AssignExpr assign, FieldAccess fa |\n fa.getTarget().hasName(\"dry_run\") and\n assign.getLValue() = fa and\n assign.getOperator() = \"=\" and\n barrier.asExpr() = fa.getQualifier() and\n isZeroLiteralExpr(assign.getRValue())\n )\n or\n exists(FieldAccess fa |\n earlyReturnOnDryRun(fa) and\n barrier.asExpr() = fa.getQualifier()\n )\n }\n\n /** Holds if `guard` ensures that `dry_run` evaluates to zero/false. */\n additional predicate safeDryRunCheck(IRGuards::GuardCondition guard, FieldAccess fa) {\n exists(NotExpr notExpr |\n guard.getAChild*() = notExpr and\n notExpr.getOperand() = fa\n )\n or\n exists(EQExpr eqExpr |\n guard.getAChild*() = eqExpr and\n (\n eqExpr.getLeftOperand() = fa and\n isZeroLiteralExpr(eqExpr.getRightOperand())\n or\n eqExpr.getRightOperand() = fa and\n isZeroLiteralExpr(eqExpr.getLeftOperand())\n )\n )\n }\n}\n\n/** Execute the configured global data-flow analysis. */\nmodule DryRunFlow = DataFlow::Global;\n\nfrom DryRunFlow::PathNode source, DryRunFlow::PathNode sink,\n AssignExpr srcAssign, FieldAccess dryRunAccess, FieldAccess fileAccess\nwhere\n DryRunFlow::flowPath(source, sink) and\n setsDryRunNonZero(srcAssign, dryRunAccess) and\n source.getNode().asExpr() = dryRunAccess.getQualifier() and\n usesFileField(fileAccess) and\n sink.getNode().asExpr() = fileAccess.getQualifier()\nselect fileAccess, source, sink,\n \"`diff_options->file` used while `dry_run` forced non-zero at $@ and consumed here at $@.\",\n srcAssign, \"dry_run assignment\",\n fileAccess, \"file field use\"\n\nquery predicate edges(DryRunFlow::PathNode edgeSource, DryRunFlow::PathNode edgeSink,\n string edgeKind, string edgeText) {\n DryRunFlow::PathGraph::edges(edgeSource, edgeSink, edgeKind, edgeText)\n}\n```\n\n> \t\tfor (i = 0; i < q->nr; i++) {\n> \t\t\tstruct diff_filepair *p = q->queue[i];\n> \t\t\tif (check_pair_status(p))\n> diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh\n> index 0352bf81a9..35eaf0855f 100755\n> --- a/t/t4035-diff-quiet.sh\n> +++ b/t/t4035-diff-quiet.sh\n> @@ -50,6 +50,10 @@ test_expect_success 'git diff-tree HEAD HEAD' '\n> \ttest_expect_code 0 git diff-tree --quiet HEAD HEAD >cnt &&\n> \ttest_line_count = 0 cnt\n> '\n> +test_expect_success 'git diff-tree -w HEAD^ HEAD' '\n> +\ttest_expect_code 1 git diff-tree --quiet -w HEAD^ HEAD >cnt &&\n> +\ttest_line_count = 0 cnt\n\nI understand that you imitate the surrounding code, but there is\n`test_must_be_empty` now, which has the huge advantage of documenting\nintention much better than requiring the line count to be zero (and I wish\nthat there was a comprehensive roadmap and planning in general to avoid,\nor at least clean up, the vast amount of style inconsistencies in Git,\npreferably via automation so that no human being is burdened with _that_\ncognitive load).\n\nCiao,\nJohannes\n\n> +'\n> test_expect_success 'git diff-files' '\n> \ttest_expect_code 0 git diff-files --quiet >cnt &&\n> \ttest_line_count = 0 cnt\n> -- \n> 2.51.1.685.g6bf3278fbc\n> \n> \n> \n"},{"id":"529224","messageId":"20251021073640.GB259661@coredump.intra.peff.net","threadId":"64340","inReplyTo":"xmqqh5vww7xa.fsf@gitster.g","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-21T07:36:40Z","receivedAt":"2025-10-21T07:36:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 18, 2025 at 08:23:13AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I'm not sure which of these you mean:\n> >\n> > - Do we still need to call diff_flush_patch_quietly() directly below\n> > the hunk above, in diff_flush()?\n> >\n> > - Do we still need the dry-run code at all?\n> \n> Both. We do not have to call flush_quietly() and can call the real\n> thing with output disabled. The dry-run bit was only added to\n> implement the flush_quietly() variant. If we lose the only caller\n> to flush_quietly(), all of the supporting infrastructure can go.\n\nIt's not the only caller, though. b55e6d36eb added another earlier in\ndiff_flush(), to handle --name-status, etc (which was its original\ngoal). That code possibly remains broken, even with my patch, and\nwould wait either on Lidong's dry-run fixes, or lifting the /dev/null\ninto the flush_quietly() function.\n\n> It concentrates only on the regression-fix aspect of the changes.\n> Going forward, my preference is:\n> \n> * Apply your patch. This is the base of the fix for 'maint' and\n> all branches.\n> \n> * As Lidong updates dry-run code by adding more \"ah we are in\n> dry-run, so we should stop at the first change and se should be\n> silent\" fixes, we can queue them on the 'master' front for the\n> preparation for a better future. Note that the 'master' front\n> would contain your \"In from_contents modes, run flush_quietly()\n> with output redirected to /dev/null\".\n> \n> * Once we regain enough confidence for dry-run with the above\n> effort, we mark your \"why not redirect to /dev/null for extra\n> protection?\" code with NEEDSWORK comment to be removed after a\n> thorough code audit to ensure that dry-run is now sound.\n> \n> And I do not mind if the NEEDSWORK comment stay there for extended\n> period of time.\n\nYeah, that matched my thinking exactly.\n\nBut thinking on it more, I think the regression is slightly bigger than\nI originally counted. My view was that:\n\n - the attempt to fix \"-I\" was incomplete but did not make anything\n worse there\n\n - that attempt also broke \"--quiet\"\n\nSo we should first un-break \"--quiet\" as simply as possible, and then\ntry to make the fix for \"-I\" more complete as a separate step. But I\nthink \"-I\" may actually have regressed, too, since it is subject to\nprinting the extra bogus output when trying to decide if the\ncontent-diff is applicable, which it did not do before.\n\nSo really, the regression fix should probably cover both of them (which\nit would if we move the /dev/null redirection into the flush_quietly()\nvariant).\n\n-Peff\n"},{"id":"529233","messageId":"20251021075226.GC259661@coredump.intra.peff.net","threadId":"64340","inReplyTo":"d5895f9c-5b3c-7a69-46e0-cf16cda5bf3a@gmx.de","subject":"Re: [PATCH] diff: restore redirection to /dev/null for diff_from_contents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-21T07:52:26Z","receivedAt":"2025-10-21T07:52:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 19, 2025 at 11:09:28PM +0200, Johannes Schindelin wrote:\n\n> > @@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)\n> > \tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n> > \t options->flags.exit_with_status &&\n> > \t options->flags.diff_from_contents) {\n> > +\t\t/*\n> > +\t\t * run diff_flush_patch for the exit status. setting\n> > +\t\t * options->file to /dev/null should be safe, because we\n> > +\t\t * aren't supposed to produce any output anyway.\n> > +\t\t */\n> > +\t\tdiff_free_file(options);\n> > +\t\toptions->file = xfopen(\"/dev/null\", \"w\");\n> > +\t\toptions->close_file = 1;\n> > +\t\toptions->color_moved = 0;\n> \n> I do not see any discussion about the `color_moved` line in\n> https://lore.kernel.org/git/20250808033019.78817-1-yldhome2d2@gmail.com/#r,\n> nor here.\n> \n> Since you re-add it, I consider at least a little bit of reasonsing in\n> order, e.g. why this is necessary, and if it is necessary, why isn't\n> `options->use_color` forced to 0 also?\n\nMy patch is a revert of the hunk from b55e6d36eb which caused the\n\"--quiet\" regression, and hence includes that line. Perhaps I could have\nmade that more clear in the commit message.\n\nI don't think use_color is related here. There's no clue in the commit\nmessage which added that color_moved line (it was just the commit which\nadded the color_moved feature in the first place). But knowing the code,\nI'd guess that it is not about trying to avoid producing color (which\nis, after all, just going to go to /dev/null anyway) but rather avoiding\nthe computation to detect moved lines, since nobody will see them.\n\nSo probably (but I did not do any experimenting) the code produces the\ncorrect output with or without color_moved. But it is also probably\nwasting some extra CPU since b55e6d36eb. In a world with a dry_run flag,\nit probably would make sense to skip the color_moved feature when\ndry_run is set.\n\n> Taking a step back to see the 100ft view, I can understand why you want\n> that \"extra level of protection\" here. An even more important thing, that\n> is missing, is a plan to avoid the need for this protection.\n\nSure. The goal of my patch was not to fix the dry-run feature. It was to\ndo the release engineering to undo the \"--quiet\" regression in the\nsimplest and least error-prone way possible. One way to do that is to\njust revert b55e6d36eb entirely, add a new test covering the regression,\nand then try again on top (perhaps on master this time). But I did the\nmore selective revert to reduce the back-and-forth noise of dropping the\ndry_run code and then adding it back, which I thought gave the original\nauthor a better base to work from.\n\nWhether that /dev/null redirection survives once we are confident that\ndry_run is hitting all of the code paths is up for debate.\n\nI take it that you would prefer to try to fix dry_run in place on\n'maint'. I think that can work, too. It's just not how I would do it\n(not because I think this particular case is so hard, but because as a\ngeneral release engineering principle I prefer to fix regressions by\nbacking out changes rather than piling more changes on top). I am OK if\nyou want to go the other way, though.\n\n> Given that you're still on GitHub's payroll if the hallway rumors are\n> correct, I am quite a bit puzzled that you did not immediately reach for\n> CodeQL (which is a GitHub-sponsored technology, after all) to get clarity\n> on the code paths that would make this exra \"layer of protection\" still\n> necessary, and thereby provide said plan.\n\nThere is no need to be puzzled. I have never actually used CodeQL at\nall, beyond analyzing some of the false positives I've seen it report.\nAnd the fact that GitHub sponsors my work on git.git is not really\nrelevant to how I go about that work.\n\n> I started an AI-assisted brainstorm session and ended up with this query\n> (which is neither as concise nor as comprehensible as I would have liked,\n> but at least it does the job of finding the `run_diff_cmd()` code path\n> that I also find, and no other code path, and in v4 of Lidong Yan's patch,\n> it finds no remaining code path):\n\nNeat, though it is very hard for me to quickly assess whether that\nCodeQL block is doing the right thing. Your idea of manually tracing the\npaths that touch opts->file seemed much simpler to me (and I think came\nup with similar results).\n\n-Peff\n"},{"id":"529294","messageId":"xmqqy0p4wcac.fsf@gitster.g","threadId":"64340","inReplyTo":"20251021073640.GB259661@coredump.intra.peff.net","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-21T14:38:03Z","receivedAt":"2025-10-21T14:38:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Both. We do not have to call flush_quietly() and can call the real\n>> thing with output disabled. The dry-run bit was only added to\n>> implement the flush_quietly() variant. If we lose the only caller\n>> to flush_quietly(), all of the supporting infrastructure can go.\n>\n> It's not the only caller, though. b55e6d36eb added another earlier in\n> diff_flush(), to handle --name-status, etc (which was its original\n> goal). That code possibly remains broken, even with my patch, and\n> would wait either on Lidong's dry-run fixes, or lifting the /dev/null\n> into the flush_quietly() function.\n\nAh, OK. That makes sense.\n\n> So really, the regression fix should probably cover both of them (which\n> it would if we move the /dev/null redirection into the flush_quietly()\n> variant).\n\nDo you mean something like this on top of your patch for 'maint',\nand the latest from Lidong to the 'master' front, then?\n\nHaving calls to this helper in two loops in one function looks a bit\nawkward but the conditions to enter these two loops are mutually\nexclusive, so it is not like we can remember the result of the calls\nwe make in the first loop and reuse in the second loop, so this\nprobably is the best we can do.\n\n--- >8 ---\nSubject: diff: fix \"-w -I --quiet\"\n\nAn earlier fix made sure we stay quiet during \"dry run\" patch output\ntaken for the purpose of choosing which filepairs should be shown,\nbut the same helper function needs to be made silent when we iterate\nover the diff-queue to compute the exit status.\n\n diff.c | 20 +++++++++++---------\n 1 file changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git c/diff.c w/diff.c\nindex 9b8d658b9e..1492ae108f 100644\n--- c/diff.c\n+++ w/diff.c\n@@ -6172,6 +6172,8 @@ static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o)\n \trun_diff(p, o);\n }\n \n+static void diff_free_file(struct diff_options *options);\n+\n /* return 1 if any change is found; otherwise, return 0 */\n static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options *o)\n {\n@@ -6179,6 +6181,15 @@ static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options\n \tint saved_found_changes = o->found_changes;\n \tint ret;\n \n+\t/*\n+\t * run diff_flush_patch for the exit status. setting\n+\t * options->file to /dev/null should be safe, because we\n+\t * aren't supposed to produce any output anyway.\n+\t */\n+\tdiff_free_file(o);\n+\to->file = xfopen(\"/dev/null\", \"w\");\n+\to->close_file = 1;\n+\to->color_moved = 0;\n \to->dry_run = 1;\n \to->found_changes = 0;\n \tdiff_flush_patch(p, o);\n@@ -6876,15 +6887,6 @@ void diff_flush(struct diff_options *options)\n \tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n \t options->flags.exit_with_status &&\n \t options->flags.diff_from_contents) {\n-\t\t/*\n-\t\t * run diff_flush_patch for the exit status. setting\n-\t\t * options->file to /dev/null should be safe, because we\n-\t\t * aren't supposed to produce any output anyway.\n-\t\t */\n-\t\tdiff_free_file(options);\n-\t\toptions->file = xfopen(\"/dev/null\", \"w\");\n-\t\toptions->close_file = 1;\n-\t\toptions->color_moved = 0;\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n \t\t\tif (check_pair_status(p))\n"},{"id":"529349","messageId":"E76C71D8-103E-4C37-B05C-86DC180BD519@gmail.com","threadId":"64340","inReplyTo":"xmqqy0p4wcac.fsf@gitster.g","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-10-22T04:46:55Z","receivedAt":"2025-10-22T04:47:06Z","isPatch":false,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Junio C Hamano writes:\n> \n> /* return 1 if any change is found; otherwise, return 0 */\n> static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options *o)\n> {\n> @@ -6179,6 +6181,15 @@ static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options\n> int saved_found_changes = o->found_changes;\n> int ret;\n> \n> + /*\n> + * run diff_flush_patch for the exit status. setting\n> + * options->file to /dev/null should be safe, because we\n> + * aren't supposed to produce any output anyway.\n> + */\n> + diff_free_file(o);\n> + o->file = xfopen(\"/dev/null\", \"w\");\n> + o->close_file = 1;\n> + o->color_moved = 0;\n> o->dry_run = 1;\n> o->found_changes = 0;\n> diff_flush_patch(p, o);\n> \n\nThis would make everything going to \"/dev/null\" after the flush_quietly() call.\nI think we need to restore o->file.\n\nThanks\nLidong"},{"id":"529400","messageId":"20251022091112.GB853931@coredump.intra.peff.net","threadId":"64340","inReplyTo":"xmqqy0p4wcac.fsf@gitster.g","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-22T09:11:12Z","receivedAt":"2025-10-22T09:11:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 21, 2025 at 07:38:03AM -0700, Junio C Hamano wrote:\n\n> > So really, the regression fix should probably cover both of them (which\n> > it would if we move the /dev/null redirection into the flush_quietly()\n> > variant).\n> \n> Do you mean something like this on top of your patch for 'maint',\n> and the latest from Lidong to the 'master' front, then?\n\nYep, exactly (though with the \"o->file\" restoration that Lidong\npointed out).\n\n> Having calls to this helper in two loops in one function looks a bit\n> awkward but the conditions to enter these two loops are mutually\n> exclusive, so it is not like we can remember the result of the calls\n> we make in the first loop and reuse in the second loop, so this\n> probably is the best we can do.\n\nYeah. I suspect there is some formulation along the lines of: if we have\ndiff_from_contents set but are not looking at a content-level diff, then\nup-front in diff_flush() we should quietly flush each to find out what\nis changed and what is not. But the loop for NAME_STATUS, etc, needs to\nknow _which_ pairs still had changes (whereas --quiet only cares about\nwhether there were any changes at all). So we'd have to store that\nsomewhere.\n\nAnd of course the chance of regressing some unconsidered corner case is\nhigh. Definitely not something we should entertain while doing another\nregression fix. ;)\n\n-Peff\n"},{"id":"529401","messageId":"20251022091433.GC853931@coredump.intra.peff.net","threadId":"64340","inReplyTo":"E76C71D8-103E-4C37-B05C-86DC180BD519@gmail.com","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-22T09:14:33Z","receivedAt":"2025-10-22T09:14:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 22, 2025 at 12:46:55PM +0800, Lidong Yan wrote:\n\n> Junio C Hamano writes:\n> > \n> > /* return 1 if any change is found; otherwise, return 0 */\n> > static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options *o)\n> > {\n> > @@ -6179,6 +6181,15 @@ static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options\n> > int saved_found_changes = o->found_changes;\n> > int ret;\n> > \n> > + /*\n> > + * run diff_flush_patch for the exit status. setting\n> > + * options->file to /dev/null should be safe, because we\n> > + * aren't supposed to produce any output anyway.\n> > + */\n> > + diff_free_file(o);\n> > + o->file = xfopen(\"/dev/null\", \"w\");\n> > + o->close_file = 1;\n> > + o->color_moved = 0;\n> > o->dry_run = 1;\n> > o->found_changes = 0;\n> > diff_flush_patch(p, o);\n> > \n> \n> This would make everything going to \"/dev/null\" after the flush_quietly() call.\n> I think we need to restore o->file.\n\nWe probably also need to restore o->color_moved, too.\n\nIn the long run (and this is the kind of cleanup I was hoping you'd work\non for 'master'), we probably could drop that line entirely and just\nskip running the moved-line detection when dry_run is set. Assuming it\neven runs at all. From a quick look at the code, it looks like we only\ndo color-moved handling via diff_flush_patch_all_file_pairs(), so it\nwouldn't trigger at all for the cases that do individual calls to\ndiff_flush_patch_quietly()?\n\n-Peff\n"},{"id":"529419","messageId":"819C2F6E-BE85-4B05-B975-894033E51D96@gmail.com","threadId":"64340","inReplyTo":"20251022091433.GC853931@coredump.intra.peff.net","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-10-22T14:20:59Z","receivedAt":"2025-10-22T14:21:13Z","isPatch":false,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Jeff King writes:\n> \n> We probably also need to restore o->color_moved, too.\n> \n> In the long run (and this is the kind of cleanup I was hoping you'd work\n> on for 'master'), we probably could drop that line entirely and just\n> skip running the moved-line detection when dry_run is set. Assuming it\n> even runs at all. From a quick look at the code, it looks like we only\n> do color-moved handling via diff_flush_patch_all_file_pairs(), so it\n> wouldn't trigger at all for the cases that do individual calls to\n> diff_flush_patch_quietly()?\n> \n\nSounds interesting, I’d like to dig into this ‘color_moved’ option and see\nif we can optimize some code path in dry-run mode.\n\nThanks,\nLidong\n\n\n"},{"id":"529421","messageId":"xmqqa51j0zzj.fsf@gitster.g","threadId":"64340","inReplyTo":"E76C71D8-103E-4C37-B05C-86DC180BD519@gmail.com","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-22T14:31:44Z","receivedAt":"2025-10-22T14:31:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lidong Yan writes:\n\n>> + diff_free_file(o);\n>> + o->file = xfopen(\"/dev/null\", \"w\");\n>> + o->close_file = 1;\n>> + o->color_moved = 0;\n>> o->dry_run = 1;\n>> o->found_changes = 0;\n>> diff_flush_patch(p, o);\n>> \n>\n> This would make everything going to \"/dev/null\" after the flush_quietly() call.\n> I think we need to restore o->file.\n\nAh, true, the original location was only for NO_OUTPUT but the other\ncaller to the diff_flush_patch_quietly() helper does deal with other\ncases as well.\n\nThanks.\n"},{"id":"529428","messageId":"xmqqms5izysb.fsf@gitster.g","threadId":"64340","inReplyTo":"xmqqa51j0zzj.fsf@gitster.g","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-22T16:28:20Z","receivedAt":"2025-10-22T16:28:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Lidong Yan writes:\n>\n>>> + diff_free_file(o);\n>>> + o->file = xfopen(\"/dev/null\", \"w\");\n>>> + o->close_file = 1;\n>>> + o->color_moved = 0;\n>>> o->dry_run = 1;\n>>> o->found_changes = 0;\n>>> diff_flush_patch(p, o);\n>>> \n>>\n>> This would make everything going to \"/dev/null\" after the flush_quietly() call.\n>> I think we need to restore o->file.\n>\n> Ah, true, the original location was only for NO_OUTPUT but the other\n> caller to the diff_flush_patch_quietly() helper does deal with other\n> cases as well.\n\nNow it turns out to be rather ugly, having to go back and forth on a\nfew members of the diff_options structure. I suspect there are\nmembers other than color_moved that would not affect the outcome\n(like --word-diff and --color-words) that cost us without giving any\nbenefit in this context that we may want to disable, but that would\nmake it even uglier.\n\nI am having second thoughts on this approach to move the redirection\nto patch_quietly(), which means for N-path change, we end up /dev/null\nredirection N times. We have two callers, so we may be better off\nhaving the redirection around the loops that contain these callers?\n\nI dunno.\n\n\n diff.c | 23 ++++++++++++++---------\n 1 file changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git c/diff.c w/diff.c\nindex 9b8d658b9e..d28f69e5ce 100644\n--- c/diff.c\n+++ w/diff.c\n@@ -6177,14 +6177,28 @@ static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options\n {\n \tint saved_dry_run = o->dry_run;\n \tint saved_found_changes = o->found_changes;\n+\tint saved_color_moved = o->color_moved;\n+\tFILE *saved_file = o->file;\n \tint ret;\n \n+\t/*\n+\t * Do the dry-run check while sending output to /dev/null and\n+\t * extra computation like color_moved that would not change\n+\t * the final outcome disabled.\n+\t */\n+\to->file = xfopen(\"/dev/null\", \"w\");\n+\to->color_moved = 0;\n \to->dry_run = 1;\n \to->found_changes = 0;\n+\n \tdiff_flush_patch(p, o);\n \tret = o->found_changes;\n+\tfclose((o->file);\n+\n \to->dry_run = saved_dry_run;\n \to->found_changes |= saved_found_changes;\n+\to->color_moved = saved_color_moved;\n+\to->file = saved_file;\n \treturn ret;\n }\n \n@@ -6876,15 +6890,6 @@ void diff_flush(struct diff_options *options)\n \tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n \t options->flags.exit_with_status &&\n \t options->flags.diff_from_contents) {\n-\t\t/*\n-\t\t * run diff_flush_patch for the exit status. setting\n-\t\t * options->file to /dev/null should be safe, because we\n-\t\t * aren't supposed to produce any output anyway.\n-\t\t */\n-\t\tdiff_free_file(options);\n-\t\toptions->file = xfopen(\"/dev/null\", \"w\");\n-\t\toptions->close_file = 1;\n-\t\toptions->color_moved = 0;\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n \t\t\tif (check_pair_status(p))\n"},{"id":"529429","messageId":"xmqqikg6zxui.fsf@gitster.g","threadId":"64340","inReplyTo":"20251022091112.GB853931@coredump.intra.peff.net","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-22T16:48:37Z","receivedAt":"2025-10-22T16:48:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Oct 21, 2025 at 07:38:03AM -0700, Junio C Hamano wrote:\n>\n>> > So really, the regression fix should probably cover both of them (which\n>> > it would if we move the /dev/null redirection into the flush_quietly()\n>> > variant).\n>> \n>> Do you mean something like this on top of your patch for 'maint',\n>> and the latest from Lidong to the 'master' front, then?\n>\n> Yep, exactly (though with the \"o->file\" restoration that Lidong\n> pointed out).\n>\n>> Having calls to this helper in two loops in one function looks a bit\n>> awkward but the conditions to enter these two loops are mutually\n>> exclusive, so it is not like we can remember the result of the calls\n>> we make in the first loop and reuse in the second loop, so this\n>> probably is the best we can do.\n>\n> Yeah. I suspect there is some formulation along the lines of: if we have\n> diff_from_contents set but are not looking at a content-level diff, then\n> up-front in diff_flush() we should quietly flush each to find out what\n> is changed and what is not. But the loop for NAME_STATUS, etc, needs to\n> know _which_ pairs still had changes (whereas --quiet only cares about\n> whether there were any changes at all). So we'd have to store that\n> somewhere.\n>\n> And of course the chance of regressing some unconsidered corner case is\n> high. Definitely not something we should entertain while doing another\n> regression fix. ;)\n\nOf course. The \"redirect inside flush_quietly()\" change by itself\nis turning out to be tricky enough for the other caller of the\nhelper.\n\nHere is what I have on top of your patch right now, after ditching\nthe idea to move the redirect to flush_quietly() because it would\nmean redirecting N times for a N-path patch, but one thing that is\nfrustrating is that I cannot come up with a scenario or test in\nwhich it makes a difference to this other caller if we forget to\nrestore o->file member.\n\ndiff --git c/diff.c i/diff.c\nindex 9b8d658b9e..ceb57d1ef8 100644\n--- c/diff.c\n+++ i/diff.c\n@@ -6814,6 +6814,16 @@ void diff_flush(struct diff_options *options)\n \t\t\t DIFF_FORMAT_NAME |\n \t\t\t DIFF_FORMAT_NAME_STATUS |\n \t\t\t DIFF_FORMAT_CHECKDIFF)) {\n+\t\t/*\n+\t\t * make sure diff_Flush_patch_quietly() to be silent.\n+\t\t */\n+\t\tFILE *saved_file = options->file;\n+\t\tint saved_color_moved = options->color_moved;\n+\n+\t\tif (options->flags.diff_from_contents) {\n+\t\t\toptions->file = xfopen(\"/dev/null\", \"w\");\n+\t\t\toptions->color_moved = 0;\n+\t\t}\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n \n@@ -6826,6 +6836,11 @@ void diff_flush(struct diff_options *options)\n \n \t\t\tflush_one_pair(p, options);\n \t\t}\n+\t\tif (options->flags.diff_from_contents) {\n+\t\t\tfclose(options->file);\n+\t\t\toptions->file = saved_file;\n+\t\t\toptions->color_moved = saved_color_moved;\n+\t\t}\n \t\tseparator++;\n \t}\n \n\n"},{"id":"529430","messageId":"xmqqcy6ezvi7.fsf@gitster.g","threadId":"64340","inReplyTo":"xmqqy0p4wcac.fsf@gitster.g","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-22T17:39:12Z","receivedAt":"2025-10-22T17:39:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> So really, the regression fix should probably cover both of them (which\n>> it would if we move the /dev/null redirection into the flush_quietly()\n>> variant).\n\nSo, here is what I ended up with. Instead of redirect many times in\nthe loop, dealing with the two callers would be simpler and less\nerror prone. If we ever have the third caller, that is where we\nshould consider refactoring this even more into a separate\nabstraction.\n\nThis goes on top of your patch and intend to go to 'maint'.\n\n----- >8 -----\nSubject: [PATCH] diff: make sure the other caller of diff_flush_patch_quietly() is silent\n\nEarlier, we added is a protection for the loop that computes \"git\ndiff --quiet -w\" to ensure calls to the diff_flush_patch_quietly()\nhelper stays quiet. Do the same for another loop that deals with\noptions like \"--name-status\" to make calls to the same helper.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 9b8d658b9e..ceb57d1ef8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6814,6 +6814,16 @@ void diff_flush(struct diff_options *options)\n \t\t\t DIFF_FORMAT_NAME |\n \t\t\t DIFF_FORMAT_NAME_STATUS |\n \t\t\t DIFF_FORMAT_CHECKDIFF)) {\n+\t\t/*\n+\t\t * make sure diff_Flush_patch_quietly() to be silent.\n+\t\t */\n+\t\tFILE *saved_file = options->file;\n+\t\tint saved_color_moved = options->color_moved;\n+\n+\t\tif (options->flags.diff_from_contents) {\n+\t\t\toptions->file = xfopen(\"/dev/null\", \"w\");\n+\t\t\toptions->color_moved = 0;\n+\t\t}\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n \n@@ -6826,6 +6836,11 @@ void diff_flush(struct diff_options *options)\n \n \t\t\tflush_one_pair(p, options);\n \t\t}\n+\t\tif (options->flags.diff_from_contents) {\n+\t\t\tfclose(options->file);\n+\t\t\toptions->file = saved_file;\n+\t\t\toptions->color_moved = saved_color_moved;\n+\t\t}\n \t\tseparator++;\n \t}\n \n-- \n2.51.1-633-gaa2b1236d0\n\n"},{"id":"529462","messageId":"09150C80-0238-49C3-BAA2-42983741C905@gmail.com","threadId":"64340","inReplyTo":"xmqqcy6ezvi7.fsf@gitster.g","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-10-23T00:33:48Z","receivedAt":"2025-10-23T00:34:03Z","isPatch":false,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Junio C Hamano writes:\n> \n> ----- >8 -----\n> Subject: [PATCH] diff: make sure the other caller of diff_flush_patch_quietly() is silent\n> \n> Earlier, we added is a protection for the loop that computes \"git\n> diff --quiet -w\" to ensure calls to the diff_flush_patch_quietly()\n> helper stays quiet. Do the same for another loop that deals with\n> options like \"--name-status\" to make calls to the same helper.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> diff.c | 15 +++++++++++++++\n> 1 file changed, 15 insertions(+)\n> \n> diff --git a/diff.c b/diff.c\n> index 9b8d658b9e..ceb57d1ef8 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6814,6 +6814,16 @@ void diff_flush(struct diff_options *options)\n> DIFF_FORMAT_NAME |\n> DIFF_FORMAT_NAME_STATUS |\n> DIFF_FORMAT_CHECKDIFF)) {\n> + /*\n> + * make sure diff_Flush_patch_quietly() to be silent.\n> + */\n> + FILE *saved_file = options->file;\n> + int saved_color_moved = options->color_moved;\n> +\n> + if (options->flags.diff_from_contents) {\n> + options->file = xfopen(\"/dev/null\", \"w\");\n> + options->color_moved = 0;\n> + }\n> for (i = 0; i < q->nr; i++) {\n> struct diff_filepair *p = q->queue[i];\n> \n> @@ -6826,6 +6836,11 @@ void diff_flush(struct diff_options *options)\n> \n> flush_one_pair(p, options);\n> }\n> + if (options->flags.diff_from_contents) {\n> + fclose(options->file);\n> + options->file = saved_file;\n> + options->color_moved = saved_color_moved;\n> + }\n> separator++;\n> }\n> \n> -- \n> 2.51.1-633-gaa2b1236d0\n> \n\nDo you think we should make a new ‘going to be flushed’ queue\nand flush them out of ‘quiet’ loop would be a good idea? I think we\nshouldn’t discard output of flush_one_pair().\n\nThanks,\nLidong\n\n"},{"id":"529495","messageId":"20251023120101.GA1123594@coredump.intra.peff.net","threadId":"64340","inReplyTo":"xmqqikg6zxui.fsf@gitster.g","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-23T12:01:01Z","receivedAt":"2025-10-23T12:01:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 22, 2025 at 09:48:37AM -0700, Junio C Hamano wrote:\n\n> Here is what I have on top of your patch right now, after ditching\n> the idea to move the redirect to flush_quietly() because it would\n> mean redirecting N times for a N-path patch, but one thing that is\n> frustrating is that I cannot come up with a scenario or test in\n> which it makes a difference to this other caller if we forget to\n> restore o->file member.\n\nIsn't it just running \"git show -w --name-status\" at all? If I take the\npatch you showed below and drop the restoration, like so:\n\n diff --git a/diff.c b/diff.c\n index ceb57d1ef8..d402f960a9 100644\n --- a/diff.c\n +++ b/diff.c\n @@ -6836,11 +6836,6 @@ void diff_flush(struct diff_options *options)\n \n \t\t\tflush_one_pair(p, options);\n \t\t}\n -\t\tif (options->flags.diff_from_contents) {\n -\t\t\tfclose(options->file);\n -\t\t\toptions->file = saved_file;\n -\t\t\toptions->color_moved = saved_color_moved;\n -\t\t}\n \t\tseparator++;\n \t}\n \n\nand then do:\n\n git init\n echo content >file\n git add file\n git commit -m file\n git show -w --name-status\n\nthen we do not show anything. We redirect to /dev/null to run\ndiff_flush_patch_quietly() and find that it does indeed have changes to\nshow (despite -w). But when we try to show the name-status output via\nflush_one_pair(), we are still redirected to /dev/null.\n\nBut wait! That bug is already there in what you have queued in\njc/diff-from-contents-fix, even without my change!\n\nThat is because you are trying to redirect to /dev/null once at the\nbeginning of the loop. But the loop is effectively:\n\n for each pair\n check for content changes with diff_flush_patch_quietly();\n output actual pair data with flush_one_pair();\n\nWe want the redirection to /dev/null for the first part of the loop\nbody, but not the second. So you have to do the redirection inside the\nloop.\n\nI agree that opening /dev/null over and over is silly. But we can reuse\nthe same filehandle for each one. I.e., like:\n\ndiff --git a/diff.c b/diff.c\nindex dac3ea9e01..e903afcf04 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6835,11 +6835,11 @@ void diff_flush(struct diff_options *options)\n \t\t/*\n \t\t * make sure diff_Flush_patch_quietly() to be silent.\n \t\t */\n-\t\tFILE *saved_file = options->file;\n+\t\tFILE *dev_null = NULL;\n \t\tint saved_color_moved = options->color_moved;\n \n \t\tif (options->flags.diff_from_contents) {\n-\t\t\toptions->file = xfopen(\"/dev/null\", \"w\");\n+\t\t\tdev_null = xfopen(\"/dev/null\", \"w\");\n \t\t\toptions->color_moved = 0;\n \t\t}\n \t\tfor (i = 0; i < q->nr; i++) {\n@@ -6848,15 +6848,20 @@ void diff_flush(struct diff_options *options)\n \t\t\tif (!check_pair_status(p))\n \t\t\t\tcontinue;\n \n-\t\t\tif (options->flags.diff_from_contents &&\n-\t\t\t !diff_flush_patch_quietly(p, options))\n-\t\t\t\tcontinue;\n+\t\t\tif (options->flags.diff_from_contents) {\n+\t\t\t\tFILE *saved_file = options->file;\n+\t\t\t\tint r;\n+\t\t\t\toptions->file = dev_null;\n+\t\t\t\tr = diff_flush_patch_quietly(p, options);\n+\t\t\t\toptions->file = saved_file;\n+\t\t\t\tif (!r)\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n \n \t\t\tflush_one_pair(p, options);\n \t\t}\n \t\tif (options->flags.diff_from_contents) {\n-\t\t\tfclose(options->file);\n-\t\t\toptions->file = saved_file;\n+\t\t\tfclose(dev_null);\n \t\t\toptions->color_moved = saved_color_moved;\n \t\t}\n \t\tseparator++;\n\nYou could even imagine diff_flush_patch_quietly() saving the /dev/null\ndescriptor in a static variable and effectively leaking it (or if we\nwant to be more structured, cached inside the diff_options struct). And\nthen the callers do not have to worry about it at all.\n\nAnd of course this all explains your confusion with Lidong's t4013 test\nthat started failing. It should generate three lines, because they are\nthe actual --raw lines. Once the bug in jc/diff-from-contents-fix is\nfixed as above, they come back. And running it with the test fixup you\nhave queued on ly/diff-name-only-with-diff-from-content yields a failure\nwith:\n\n 'actual' is not empty, it contains:\n :100644 000000 e69de29 0000000 D\tfile1\n :100644 000000 e69de29 0000000 D\tfile2\n :000000 100644 0000000 0000000 U\tfile3\n\n-Peff\n"},{"id":"529496","messageId":"20251023121525.GB1123594@coredump.intra.peff.net","threadId":"64340","inReplyTo":"20251023120101.GA1123594@coredump.intra.peff.net","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-23T12:15:25Z","receivedAt":"2025-10-23T12:15:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 23, 2025 at 08:01:01AM -0400, Jeff King wrote:\n\n> You could even imagine diff_flush_patch_quietly() saving the /dev/null\n> descriptor in a static variable and effectively leaking it (or if we\n> want to be more structured, cached inside the diff_options struct). And\n> then the callers do not have to worry about it at all.\n\nSomething like this (on top of jk/diff-from-contents-fix, replacing what\nyou have in jc/diff-from-contents/fix):\n\ndiff --git a/diff.c b/diff.c\nindex 9b8d658b9e..c9d3aaeb0f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6175,14 +6175,34 @@ static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o)\n /* return 1 if any change is found; otherwise, return 0 */\n static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options *o)\n {\n+\tstatic FILE *dev_null;\n+\tFILE *saved_file = o->file;\n+\tint saved_color_moved = o->color_moved;\n \tint saved_dry_run = o->dry_run;\n \tint saved_found_changes = o->found_changes;\n \tint ret;\n \n+\t/*\n+\t * As an extra precaution against code sending output to o->file even\n+\t * when o->dry_run is set, redirect to /dev/null.\n+\t *\n+\t * We cache the /dev/null filehandle forever, effectively leaking it.\n+\t * Gross, but it's O(1) gross-ness. A better solution would perhaps be\n+\t * stuffing it into o->cached_dev_null or something, and freeing it\n+\t * with the rest of the diff options.\n+\t */\n+\tif (!dev_null)\n+\t\tdev_null = xfopen(\"/dev/null\", \"w\");\n+\n+\to->file = dev_null;\n+\t/* TODO check if this is actually doing anything! */\n+\to->color_moved = 0;\n \to->dry_run = 1;\n \to->found_changes = 0;\n \tdiff_flush_patch(p, o);\n \tret = o->found_changes;\n+\to->file = saved_file;\n+\to->color_moved = saved_color_moved;\n \to->dry_run = saved_dry_run;\n \to->found_changes |= saved_found_changes;\n \treturn ret;\n@@ -6876,15 +6896,6 @@ void diff_flush(struct diff_options *options)\n \tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n \t options->flags.exit_with_status &&\n \t options->flags.diff_from_contents) {\n-\t\t/*\n-\t\t * run diff_flush_patch for the exit status. setting\n-\t\t * options->file to /dev/null should be safe, because we\n-\t\t * aren't supposed to produce any output anyway.\n-\t\t */\n-\t\tdiff_free_file(options);\n-\t\toptions->file = xfopen(\"/dev/null\", \"w\");\n-\t\toptions->close_file = 1;\n-\t\toptions->color_moved = 0;\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n \t\t\tif (check_pair_status(p))\n\nAnd you can see the difference with the tests Lidong added in t4013, or\njust with this simple sequence:\n\n git init\n echo content >file\n git add file\n git commit -m foo\n git.compile show -w --name-status\n\nWithout either the /dev/null redirection above (or the actual dry_run\nfixes), you get a bogus \"diff --git\" header in the output.\n\nSo mulling over that for a moment...if we are going to teach all code\npaths that look at o->file to check o->dry_run, why do we need a\n/dev/null redirection at all? Can't we just set o->file to NULL, and\nthat is the clue that we do not want output?\n\nI know that is more intricate, and not what we want to do for the\nimmediate regression fix. But in the long run it makes more sense to me.\nWe get rid of the extra flag, and any code that does the wrong thing (by\ntrying to write to o->file) will blow up horribly with a segfault rather\nthan quietly produce wrong output.\n\n-Peff\n"},{"id":"529505","messageId":"xmqqms5hwxkm.fsf@gitster.g","threadId":"64340","inReplyTo":"20251023120101.GA1123594@coredump.intra.peff.net","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-23T13:35:05Z","receivedAt":"2025-10-23T13:35:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That is because you are trying to redirect to /dev/null once at the\n> beginning of the loop. But the loop is effectively:\n>\n> for each pair\n> check for content changes with diff_flush_patch_quietly();\n> output actual pair data with flush_one_pair();\n>\n> We want the redirection to /dev/null for the first part of the loop\n> body, but not the second. So you have to do the redirection inside the\n> loop.\n\nYeah, my bad. Lidong noticed the same thing.\n\n> I agree that opening /dev/null over and over is silly. But we can reuse\n> the same filehandle for each one. I.e., like:\n>\n> diff --git a/diff.c b/diff.c\n> index dac3ea9e01..e903afcf04 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6835,11 +6835,11 @@ void diff_flush(struct diff_options *options)\n> \t\t/*\n> \t\t * make sure diff_Flush_patch_quietly() to be silent.\n> \t\t */\n> -\t\tFILE *saved_file = options->file;\n> +\t\tFILE *dev_null = NULL;\n> \t\tint saved_color_moved = options->color_moved;\n> \n> \t\tif (options->flags.diff_from_contents) {\n> -\t\t\toptions->file = xfopen(\"/dev/null\", \"w\");\n> +\t\t\tdev_null = xfopen(\"/dev/null\", \"w\");\n> \t\t\toptions->color_moved = 0;\n> \t\t}\n> \t\tfor (i = 0; i < q->nr; i++) {\n> @@ -6848,15 +6848,20 @@ void diff_flush(struct diff_options *options)\n> \t\t\tif (!check_pair_status(p))\n> \t\t\t\tcontinue;\n> \n> -\t\t\tif (options->flags.diff_from_contents &&\n> -\t\t\t !diff_flush_patch_quietly(p, options))\n> -\t\t\t\tcontinue;\n> +\t\t\tif (options->flags.diff_from_contents) {\n> +\t\t\t\tFILE *saved_file = options->file;\n> +\t\t\t\tint r;\n> +\t\t\t\toptions->file = dev_null;\n> +\t\t\t\tr = diff_flush_patch_quietly(p, options);\n> +\t\t\t\toptions->file = saved_file;\n> +\t\t\t\tif (!r)\n> +\t\t\t\t\tcontinue;\n> +\t\t\t}\n> \n> \t\t\tflush_one_pair(p, options);\n> \t\t}\n> \t\tif (options->flags.diff_from_contents) {\n> -\t\t\tfclose(options->file);\n> -\t\t\toptions->file = saved_file;\n> +\t\t\tfclose(dev_null);\n> \t\t\toptions->color_moved = saved_color_moved;\n> \t\t}\n> \t\tseparator++;\n>\n> You could even imagine diff_flush_patch_quietly() saving the /dev/null\n> descriptor in a static variable and effectively leaking it (or if we\n> want to be more structured, cached inside the diff_options struct). And\n> then the callers do not have to worry about it at all.\n\nThat would be bigger change than a regression fix warrants, so let's\nleave it out, but let me use the above to replace my botched\nattempt.\n\nThanks, both of you.\n\n"},{"id":"529506","messageId":"xmqqikg5wx81.fsf@gitster.g","threadId":"64340","inReplyTo":"09150C80-0238-49C3-BAA2-42983741C905@gmail.com","subject":"Re: Regression in `git diff --quiet HEAD` when a new file is staged","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-23T13:42:38Z","receivedAt":"2025-10-23T13:42:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lidong Yan writes:\n\n> Do you think we should make a new ‘going to be flushed’ queue\n> and flush them out of ‘quiet’ loop would be a good idea? I think we\n> shouldn’t discard output of flush_one_pair().\n\nThanks for catching my sillyness. \n\nPeff caught the same thing but in each iteration of this loop we do\nthe \"diff -p >/dev/null\" to decide if we do \"diff\n--(raw|name-only|...)\" for the path, so redirecting the whole thing\nwould break it big time.\n"}]}