{"thread":{"id":"64343","subject":"[PATCH] diff: stop output garbled message in dry run mode","startedAt":"2025-10-17T03:17:31Z","lastAt":"2025-10-23T12:30:56Z","messageCount":21,"participants":["Lidong Yan via GitGitGadget","Johannes Schindelin","Junio C Hamano","Lidong Yan","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"529037","messageId":"pull.2071.git.git.1760671049113.gitgitgadget@gmail.com","threadId":"64343","inReplyTo":null,"subject":"[PATCH] diff: stop output garbled message in dry run mode","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-10-17T03:17:29Z","receivedAt":"2025-10-17T03:17:31Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan \n\nIn dry run mode, diff_flush_patch() should not produce any output.\nHowever, in commit b55e6d36eb (diff: ensure consistent diff behavior\nwith ignore options, 2025-08-08), only the output during the\ncomparison of two file contents was suppressed. For file deletions\nor mode changes, diff_flush_patch() still produces output. In\nrun_extern_diff(), set quiet to true if in dry run mode. In\nemit_diff_symbol_from_struct(), directly return if in dry run mode.\n\nSigned-off-by: Lidong Yan \n---\n diff: stop output garbled message in dry run mode\n \n In dry run mode, diff_flush_patch() should not produce any output.\n However, in commit b55e6d36eb (diff: ensure consistent diff behavior\n with ignore options, 2025-08-08), only the output during the comparison\n of two file contents was suppressed. For file deletions or mode changes,\n diff_flush_patch() still produces output. In run_extern_diff(), set\n quiet to true if in dry run mode. In emit_diff_symbol_from_struct(),\n directly return if in dry run mode.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2071%2Fbrandb97%2Ffix-diff-dry-run-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2071/brandb97/fix-diff-dry-run-v1\nPull-Request: https://github.com/git/git/pull/2071\n\n diff.c | 5 ++++-\n t/t4013-diff-various.sh | 21 +++++++++++++++++++++\n 2 files changed, 25 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 87fa16b730..4baf9b535e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1351,6 +1351,9 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \tint len = eds->len;\n \tunsigned flags = eds->flags;\n \n+\tif (o->dry_run)\n+\t\treturn;\n+\n \tswitch (s) {\n \tcase DIFF_SYMBOL_NO_LF_EOF:\n \t\tcontext = diff_get_color_opt(o, DIFF_CONTEXT);\n@@ -4420,7 +4423,7 @@ static void run_external_diff(const struct external_diff *pgm,\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n-\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH);\n+\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH) || o->dry_run;\n \tint rc;\n \n \t/*\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 55a06eadb3..25fa452656 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -661,6 +661,27 @@ test_expect_success 'diff -I: ignore matching file' '\n \ttest_grep ! \"file1\" actual\n '\n \n+test_expect_success 'diff -I: ignore all content changes' '\n+\ttest_when_finished \"git rm -f file1 file2\" &&\n+\t: >file1 &&\n+\tgit add file1 &&\n+\t: >file2 &&\n+\tgit add file2 &&\n+\n+\trm -f file1 file2 &&\n+\tmkdir file2 &&\n+\ttest_diff_no_content_changes () {\n+\t\tgit diff $1 --ignore-blank-lines -I\".*\" >actual &&\n+\t\ttest_line_count = 2 actual &&\n+\t\ttest_grep \"file1\" actual &&\n+\t\ttest_grep \"file2\" actual &&\n+\t\ttest_grep ! \"diff --git\" actual\n+\t} &&\n+\ttest_diff_no_content_changes \"--raw\" &&\n+\ttest_diff_no_content_changes \"--name-only\" &&\n+\ttest_diff_no_content_changes \"--name-status\"\n+'\n+\n # check_prefix \n # check only lines with paths to avoid dependency on exact oid/contents\n check_prefix () {\n\nbase-commit: 143f58ef7535f8f8a80d810768a18bdf3807de26\n-- \ngitgitgadget\n"},{"id":"529064","messageId":"4ff55fc5-7880-b8bf-257f-3186552e9c36@gmx.de","threadId":"64343","inReplyTo":"pull.2071.git.git.1760671049113.gitgitgadget@gmail.com","subject":"Re: [PATCH] diff: stop output garbled message in dry run mode","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-10-17T12:07:50Z","receivedAt":"2025-10-17T12:07:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 17 Oct 2025, Lidong Yan via GitGitGadget wrote:\n\n> From: Lidong Yan \n> \n> In dry run mode, diff_flush_patch() should not produce any output.\n> However, in commit b55e6d36eb (diff: ensure consistent diff behavior\n> with ignore options, 2025-08-08), only the output during the\n> comparison of two file contents was suppressed. For file deletions\n> or mode changes, diff_flush_patch() still produces output. In\n> run_extern_diff(), set quiet to true if in dry run mode. In\n> emit_diff_symbol_from_struct(), directly return if in dry run mode.\n> \n> Signed-off-by: Lidong Yan \n>\n> [...]\n>\n> diff --git a/diff.c b/diff.c\n> index 87fa16b730..4baf9b535e 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1351,6 +1351,9 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n> \tint len = eds->len;\n> \tunsigned flags = eds->flags;\n> \n> +\tif (o->dry_run)\n> +\t\treturn;\n> +\n\nVery good. This is a minimal change that covers all of the `emit_*()`\ncalls (except for `checkdiff_consume()`, but if the `--check` code path\nis entered under `o->dry_run`, it is debatable whether or not it should\noutput something, therefore we could claim that this is \"by design\").\n\nI do see a still-unguarded `fprintf(o->file, ...)` call in\n`run_diff_cmd()`, but as far as I can see, this call is not in any code\npath where `dry_run` is set. Granted, this is quite tedious to reason\nabout and requires considerable cognitive load to analyze, but judging\nfrom past attempts to land patches that simplify logic e.g. in\nhttps://lore.kernel.org/git/pull.1888.git.1743079429.gitgitgadget@gmail.com/\nI have concluded that core reviewers on this mailing list delight too much\nin such analyses to be interested in making Git's code easier to reason\nabout.\n\n> \tswitch (s) {\n> \tcase DIFF_SYMBOL_NO_LF_EOF:\n> \t\tcontext = diff_get_color_opt(o, DIFF_CONTEXT);\n> @@ -4420,7 +4423,7 @@ static void run_external_diff(const struct external_diff *pgm,\n> {\n> \tstruct child_process cmd = CHILD_PROCESS_INIT;\n> \tstruct diff_queue_struct *q = &diff_queued_diff;\n> -\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH);\n> +\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH) || o->dry_run;\n> \tint rc;\n> \n> \t/*\n> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> index 55a06eadb3..25fa452656 100755\n> --- a/t/t4013-diff-various.sh\n> +++ b/t/t4013-diff-various.sh\n> @@ -661,6 +661,27 @@ test_expect_success 'diff -I: ignore matching file' '\n> \ttest_grep ! \"file1\" actual\n> '\n> \n> +test_expect_success 'diff -I: ignore all content changes' '\n> +\ttest_when_finished \"git rm -f file1 file2\" &&\n> +\t: >file1 &&\n> +\tgit add file1 &&\n> +\t: >file2 &&\n> +\tgit add file2 &&\n> +\n> +\trm -f file1 file2 &&\n> +\tmkdir file2 &&\n> +\ttest_diff_no_content_changes () {\n> +\t\tgit diff $1 --ignore-blank-lines -I\".*\" >actual &&\n> +\t\ttest_line_count = 2 actual &&\n> +\t\ttest_grep \"file1\" actual &&\n> +\t\ttest_grep \"file2\" actual &&\n> +\t\ttest_grep ! \"diff --git\" actual\n> +\t} &&\n\nNice! While this function obviously is not strictly scoped to this test\ncase (it will still be defined when the next test case is executed), it is\nwonderful to see the structure that helps readers along.\n\n> +\ttest_diff_no_content_changes \"--raw\" &&\n> +\ttest_diff_no_content_changes \"--name-only\" &&\n> +\ttest_diff_no_content_changes \"--name-status\"\n> +'\n> +\n> # check_prefix \n> # check only lines with paths to avoid dependency on exact oid/contents\n> check_prefix () {\n> \n> base-commit: 143f58ef7535f8f8a80d810768a18bdf3807de26\n\nThank you for fixing this so quickly! From my point of view, this is ready\nto go. I will integrate this patch into Git for Windows v2.51.1 (which I\nam sadly forced to release on a Friday).\n\nCiao,\nJohannes\n"},{"id":"529082","messageId":"xmqqh5vx1p0q.fsf@gitster.g","threadId":"64343","inReplyTo":"pull.2071.git.git.1760671049113.gitgitgadget@gmail.com","subject":"Re: [PATCH] diff: stop output garbled message in dry run mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-17T16:17:25Z","receivedAt":"2025-10-17T16:17:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Lidong Yan via GitGitGadget\" writes:\n\n> From: Lidong Yan \n>\n> In dry run mode, diff_flush_patch() should not produce any output.\n> However, in commit b55e6d36eb (diff: ensure consistent diff behavior\n> with ignore options, 2025-08-08), only the output during the\n> comparison of two file contents was suppressed. For file deletions\n> or mode changes, diff_flush_patch() still produces output. In\n> run_extern_diff(), set quiet to true if in dry run mode. In\n> emit_diff_symbol_from_struct(), directly return if in dry run mode.\n\nThe above makes it sound as if the dry-run mode was an inherent part\nof the diff machinery that existed even before b55e6d36 came, and\nb55e6d36 somehow broke it. But that is not what you are telling us,\nI think.\n\nYou may know what the \"dry-run\" mode is, but others don't. You\nshould tell the backstory a bit better to help them. I am guessing\nthat this patch is to fix a breakage introduced when the dry-run\nmode is added in b55e6d36 (diff: ensure consistent diff behavior\nwith ignore options, 2025-08-08)? If so, I would expect an\nexplanation like ...\n\n Earlier, b55e6d36 (diff: ensure consistent diff behavior with\n ignore options, 2025-08-08) introduced \"dry-run\" mode to the\n diff machinery so that content based diff filtering (like\n ignoring space changes or those that match -I) can first\n try to produce a patch without emitting any output to see if\n under the given diff filtering condition we would get any output\n lines, and a new helper function diff_flush_patch_quietly() was\n introduced to use the mode to see an individual filepair needs\n to be shown.\n\n However, the solution was not complete. IN SUCH AND SUCH CASES,\n THIS BAD THING HAPPENED BECAUSE WE OVERLOOKED THIS AND THAT\n CONDITION, AND AS A RESULT, DRY-RUN MODE WAS NOT QUIET.\n\n To fix this, DO THIS AND THAT. THIS WOULD AFFECT ONLY SUCH AND\n SUCH CASES WITHOUT AFFECTING OTHER CODE PATHS LIKE DOING X AND Y.\n\n... is given to help readers understand what we wanted to do in the\nearlier commit, what we failed to do there and why, and what we can\ndo at this point to clean up the mess without making further\ndamange.\n\n> Signed-off-by: Lidong Yan \n> ---\n> diff: stop output garbled message in dry run mode\n> \n> In dry run mode, diff_flush_patch() should not produce any output.\n> However, in commit b55e6d36eb (diff: ensure consistent diff behavior\n> with ignore options, 2025-08-08), only the output during the comparison\n> of two file contents was suppressed. For file deletions or mode changes,\n> diff_flush_patch() still produces output. In run_extern_diff(), set\n> quiet to true if in dry run mode. In emit_diff_symbol_from_struct(),\n> directly return if in dry run mode.\n\nThe \"below three-dash\" space is a place to explain what does not\nhave to be a part of the resulting commit but would help those who\nare reading the mailing list and reviewing. Repeating the same\nthing as the proposed log message does not help readers.\n\n> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> index 55a06eadb3..25fa452656 100755\n> --- a/t/t4013-diff-various.sh\n> +++ b/t/t4013-diff-various.sh\n> @@ -661,6 +661,27 @@ test_expect_success 'diff -I: ignore matching file' '\n> \ttest_grep ! \"file1\" actual\n> '\n> \n> +test_expect_success 'diff -I: ignore all content changes' '\n> +\ttest_when_finished \"git rm -f file1 file2\" &&\n> +\t: >file1 &&\n> +\tgit add file1 &&\n> +\t: >file2 &&\n> +\tgit add file2 &&\n> +\n> +\trm -f file1 file2 &&\n> +\tmkdir file2 &&\n> +\ttest_diff_no_content_changes () {\n> +\t\tgit diff $1 --ignore-blank-lines -I\".*\" >actual &&\n> +\t\ttest_line_count = 2 actual &&\n> +\t\ttest_grep \"file1\" actual &&\n> +\t\ttest_grep \"file2\" actual &&\n> +\t\ttest_grep ! \"diff --git\" actual\n> +\t} &&\n> +\ttest_diff_no_content_changes \"--raw\" &&\n> +\ttest_diff_no_content_changes \"--name-only\" &&\n> +\ttest_diff_no_content_changes \"--name-status\"\n> +'\n\nTest that exercises \"git diff -I\" is in line with what the\noriginal b55e6d36eb wanted to address, but given that we saw a\nrecent regression report like [*], I would have liked to see \"git\ndiff --quiet\" in the test as well.\n\nThanks.\n\n\n[Reference]\n\n * https://lore.kernel.org/git/CACJRbWjwOQwJB13CwTfvhV3p+Hbn4KrNM9AtBanGtUS4V_1MbQ@mail.gmail.com/\n\n"},{"id":"529090","messageId":"xmqqjz0tz6eg.fsf@gitster.g","threadId":"64343","inReplyTo":"4ff55fc5-7880-b8bf-257f-3186552e9c36@gmx.de","subject":"Re: [PATCH] diff: stop output garbled message in dry run mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-17T19:15:35Z","receivedAt":"2025-10-17T19:15:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Thank you for fixing this so quickly! From my point of view, this is ready\n> to go. I will integrate this patch into Git for Windows v2.51.1 (which I\n> am sadly forced to release on a Friday).\n\nYou may not want to. I think I'll have to do 2.51.2 either with\nPeff's fix (or a rerolled version of this one if it comes quickly\nenough) early next week anyway.\n\nThanks.\n"},{"id":"529098","messageId":"xmqqa51pz3ih.fsf@gitster.g","threadId":"64343","inReplyTo":"4ff55fc5-7880-b8bf-257f-3186552e9c36@gmx.de","subject":"Re: [PATCH] diff: stop output garbled message in dry run mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-17T20:17:58Z","receivedAt":"2025-10-17T20:18:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I do see a still-unguarded `fprintf(o->file, ...)` call in\n> `run_diff_cmd()`, but as far as I can see, this call is not in any code\n> path where `dry_run` is set.\n\nAmong the callers of run_diff_cmd(), only the caller that wants to\nreport \"this path is unmerged\" passes NULL diff_filespec pointers in\nparameters one and two, in which case run_diff_cmd() would give that\nmessage. So if you have an unmerged filepair in queued_diff, this\ncallchain\n\n\tdiff_flush()\n\t loop over diff_queued_diff\n -> diff_flush_patch_quietly()\n\t fiddle with dry_run bit\n\t -> diff_flush_patch()\n\t\t-> run_diff()\n\t\t -> run_diff_cmd() with one&two set to NULL\n\nmay hit the fprintf into o->file.\n\nSo you are right to worry about that fprintf(). If I make a\nwhitespace-only change to one file, and then make another path\nunmerged, here is what I would see:\n\n $ rungit v2.48.0 diff --raw\n :100644 100644 b82c4963e7 0000000000 M cache-tree.h\n :000000 100644 0000000000 0000000000 U t/lib-gpg.sh\n\nThis is version before that dry-run thing. It operated under the\nold rule to show \"--raw\" to report object differences, hence\nignoring \"-w\".\n\n $ rungit v2.48.0 diff --raw -w\n :100644 100644 b82c4963e7 0000000000 M cache-tree.h\n :000000 100644 0000000000 0000000000 U t/lib-gpg.sh\n\nWith a version with the dry_run thing, here is what we see:\n\n $ git diff --raw -w\n * Unmerged path t/lib-gpg.sh\n :000000 100644 0000000000 0000000000 U t/lib-gpg.sh\n\nAs dry_run thing intended, the entry on the whitespace-only path is\ngone from the output, but the fprintf(o->file) you noticed comes out,\nwhich is not what we want to see. Of course, if we omit -w to avoid\ntriggering the dry-run thing, we won't see it.\n\n $ git diff --raw\n :100644 100644 b82c4963e7 0000000000 M cache-tree.h\n :000000 100644 0000000000 0000000000 U t/lib-gpg.sh\n\nAs a regression-fix change, I'd feel safer with Peff's version.\n\nThanks.\n"},{"id":"529099","messageId":"xmqq5xcdz3f9.fsf@gitster.g","threadId":"64343","inReplyTo":"xmqqjz0tz6eg.fsf@gitster.g","subject":"Re: [PATCH] diff: stop output garbled message in dry run mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-17T20:19:54Z","receivedAt":"2025-10-17T20:19:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>> Thank you for fixing this so quickly! From my point of view, this is ready\n>> to go. I will integrate this patch into Git for Windows v2.51.1 (which I\n>> am sadly forced to release on a Friday).\n>\n> You may not want to. I think I'll have to do 2.51.2 either with\n> Peff's fix (or a rerolled version of this one if it comes quickly\n> enough) early next week anyway.\n>\n> Thanks.\n\nAh, sorry for replying before noticing and reading your announce on\n2.51.1 that was made hours ago. It seems that you had a separate\nreason to make a release with the CVE fix material quickly, so\nplease ignore the above.\n"},{"id":"529111","messageId":"6C994C9C-0034-46D0-8112-FF88773B5CF5@gmail.com","threadId":"64343","inReplyTo":"xmqqh5vx1p0q.fsf@gitster.g","subject":"Re: [PATCH] diff: stop output garbled message in dry run mode","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-10-18T01:11:34Z","receivedAt":"2025-10-18T01:11:49Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Junio C Hamano writes:\n> \n> \"Lidong Yan via GitGitGadget\" writes:\n> \n>> From: Lidong Yan \n>> \n>> In dry run mode, diff_flush_patch() should not produce any output.\n>> However, in commit b55e6d36eb (diff: ensure consistent diff behavior\n>> with ignore options, 2025-08-08), only the output during the\n>> comparison of two file contents was suppressed. For file deletions\n>> or mode changes, diff_flush_patch() still produces output. In\n>> run_extern_diff(), set quiet to true if in dry run mode. In\n>> emit_diff_symbol_from_struct(), directly return if in dry run mode.\n> \n> The above makes it sound as if the dry-run mode was an inherent part\n> of the diff machinery that existed even before b55e6d36 came, and\n> b55e6d36 somehow broke it. But that is not what you are telling us,\n> I think.\n> \n> You may know what the \"dry-run\" mode is, but others don't. You\n> should tell the backstory a bit better to help them. I am guessing\n> that this patch is to fix a breakage introduced when the dry-run\n> mode is added in b55e6d36 (diff: ensure consistent diff behavior\n> with ignore options, 2025-08-08)? If so, I would expect an\n> explanation like ...\n> \n> Earlier, b55e6d36 (diff: ensure consistent diff behavior with\n> ignore options, 2025-08-08) introduced \"dry-run\" mode to the\n> diff machinery so that content based diff filtering (like\n> ignoring space changes or those that match -I) can first\n> try to produce a patch without emitting any output to see if\n> under the given diff filtering condition we would get any output\n> lines, and a new helper function diff_flush_patch_quietly() was\n> introduced to use the mode to see an individual filepair needs\n> to be shown.\n> \n> However, the solution was not complete. IN SUCH AND SUCH CASES,\n> THIS BAD THING HAPPENED BECAUSE WE OVERLOOKED THIS AND THAT\n> CONDITION, AND AS A RESULT, DRY-RUN MODE WAS NOT QUIET.\n> \n> To fix this, DO THIS AND THAT. THIS WOULD AFFECT ONLY SUCH AND\n> SUCH CASES WITHOUT AFFECTING OTHER CODE PATHS LIKE DOING X AND Y.\n\nThanks for explaining how to describe a problem in commit message. Will rewrite\nsoon.\n\n> \n> ... is given to help readers understand what we wanted to do in the\n> earlier commit, what we failed to do there and why, and what we can\n> do at this point to clean up the mess without making further\n> damange.\n> \n>> Signed-off-by: Lidong Yan \n>> ---\n>> diff: stop output garbled message in dry run mode\n>> \n>> In dry run mode, diff_flush_patch() should not produce any output.\n>> However, in commit b55e6d36eb (diff: ensure consistent diff behavior\n>> with ignore options, 2025-08-08), only the output during the comparison\n>> of two file contents was suppressed. For file deletions or mode changes,\n>> diff_flush_patch() still produces output. In run_extern_diff(), set\n>> quiet to true if in dry run mode. In emit_diff_symbol_from_struct(),\n>> directly return if in dry run mode.\n> \n> The \"below three-dash\" space is a place to explain what does not\n> have to be a part of the resulting commit but would help those who\n> are reading the mailing list and reviewing. Repeating the same\n> thing as the proposed log message does not help readers.\n\nI am using Github pull request for convenience. I think the bot repeat my\ncommit messages twice.\n\n> \n>> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n>> index 55a06eadb3..25fa452656 100755\n>> --- a/t/t4013-diff-various.sh\n>> +++ b/t/t4013-diff-various.sh\n>> @@ -661,6 +661,27 @@ test_expect_success 'diff -I: ignore matching file' '\n>> test_grep ! \"file1\" actual\n>> '\n>> \n>> +test_expect_success 'diff -I: ignore all content changes' '\n>> + test_when_finished \"git rm -f file1 file2\" &&\n>> + : >file1 &&\n>> + git add file1 &&\n>> + : >file2 &&\n>> + git add file2 &&\n>> +\n>> + rm -f file1 file2 &&\n>> + mkdir file2 &&\n>> + test_diff_no_content_changes () {\n>> + git diff $1 --ignore-blank-lines -I\".*\" >actual &&\n>> + test_line_count = 2 actual &&\n>> + test_grep \"file1\" actual &&\n>> + test_grep \"file2\" actual &&\n>> + test_grep ! \"diff --git\" actual\n>> + } &&\n>> + test_diff_no_content_changes \"--raw\" &&\n>> + test_diff_no_content_changes \"--name-only\" &&\n>> + test_diff_no_content_changes \"--name-status\"\n>> +'\n> \n> Test that exercises \"git diff -I\" is in line with what the\n> original b55e6d36eb wanted to address, but given that we saw a\n> recent regression report like [*], I would have liked to see \"git\n> diff --quiet\" in the test as well.\n\nI will read Peff’s test and see if I should also add some similar tests\n\n> * https://lore.kernel.org/git/CACJRbWjwOQwJB13CwTfvhV3p+Hbn4KrNM9AtBanGtUS4V_1MbQ@mail.gmail.com/\n> \n\nThanks,\nLidong"},{"id":"529113","messageId":"xmqqo6q4x0o3.fsf@gitster.g","threadId":"64343","inReplyTo":"6C994C9C-0034-46D0-8112-FF88773B5CF5@gmail.com","subject":"Re: [PATCH] diff: stop output garbled message in dry run mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-18T05:02:20Z","receivedAt":"2025-10-18T05:02:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lidong Yan writes:\n\n> Junio C Hamano writes:\n>> ...\n>> Test that exercises \"git diff -I\" is in line with what the\n>> original b55e6d36eb wanted to address, but given that we saw a\n>> recent regression report like [*], I would have liked to see \"git\n>> diff --quiet\" in the test as well.\n>\n> I will read Peff’s test and see if I should also add some similar tests\n>\n>> * https://lore.kernel.org/git/CACJRbWjwOQwJB13CwTfvhV3p+Hbn4KrNM9AtBanGtUS4V_1MbQ@mail.gmail.com/\n\nAlso I think the fprintf() in run_diff_cmd() Dscho noticed is a real\nproblem. cf. \n\nThanks for working on this. \n"},{"id":"529117","messageId":"20251018094722.GC1060824@coredump.intra.peff.net","threadId":"64343","inReplyTo":"6C994C9C-0034-46D0-8112-FF88773B5CF5@gmail.com","subject":"Re: [PATCH] diff: stop output garbled message in dry run mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-18T09:47:22Z","receivedAt":"2025-10-18T09:47:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 18, 2025 at 09:11:34AM +0800, Lidong Yan wrote:\n\n> > Test that exercises \"git diff -I\" is in line with what the\n> > original b55e6d36eb wanted to address, but given that we saw a\n> > recent regression report like [*], I would have liked to see \"git\n> > diff --quiet\" in the test as well.\n> \n> I will read Peff’s test and see if I should also add some similar tests\n\nWhat I was hoping was that we'd apply my patch, as a matter of release\nengineering (backing out the regression-causing bit of b55e6d36eb). And\nthen you could make more-specific fixes on top (since -I would still\nhave potential problems). And then you don't need to add a test for the\nregression case, since it's already there.\n\n-Peff\n"},{"id":"529118","messageId":"20251018094823.31173-1-yldhome2d2@gmail.com","threadId":"64343","inReplyTo":"pull.2071.git.git.1760671049113.gitgitgadget@gmail.com","subject":"[PATCH v2] diff: stop output garbled message in dry run mode","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-10-18T09:48:23Z","receivedAt":"2025-10-18T09:48:39Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Earlier, b55e6d36 (diff: ensure consistent diff behavior with\nignore options, 2025-08-08) introduced \"dry-run\" mode to the\ndiff machinery so that content-based diff filtering (like\nignoring space changes or those that match -I) can first\ntry to produce a patch without emitting any output to see if\nunder the given diff filtering condition we would get any output\nlines, and a new helper function diff_flush_patch_quietly() was\nintroduced to use the mode to see an individual filepair needs\nto be shown.\n\nHowever, the solution was not complete. When files are deleted,\nfile modes change, or there are unmerged entries in the index,\ndry-run mode still produces output because we overlooked these\nconditions, and as a result, dry-run mode was not quiet.\n\nSince dry-run mode is only set in diff_flush_patch_quietly(),\nsetting the output file to \"/dev/null\" within diff_flush_patch_quietly()\nensures no output is emitted in dry-run mode. To improve performance\nof dry-run mode, add a check before outputting to determine if we\nshould exit early to avoid unnecessary output processing.\n\nSigned-off-by: Lidong Yan \n---\nI copied Peff's code from https://lore.kernel.org/git/20251017083641.GB4073661@coredump.intra.peff.net/\n\n diff.c | 20 ++++++++++++++++++--\n t/t4013-diff-various.sh | 37 +++++++++++++++++++++++++++++++++++++\n t/t4035-diff-quiet.sh | 4 ++++\n 3 files changed, 59 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 87fa16b730..ec05ac565b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1351,6 +1351,9 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \tint len = eds->len;\n \tunsigned flags = eds->flags;\n \n+\tif (o->dry_run)\n+\t\treturn;\n+\n \tswitch (s) {\n \tcase DIFF_SYMBOL_NO_LF_EOF:\n \t\tcontext = diff_get_color_opt(o, DIFF_CONTEXT);\n@@ -4420,7 +4423,7 @@ static void run_external_diff(const struct external_diff *pgm,\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n-\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH);\n+\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH) || o->dry_run;\n \tint rc;\n \n \t/*\n@@ -4615,7 +4618,8 @@ static void run_diff_cmd(const struct external_diff *pgm,\n \t\t p->status == DIFF_STATUS_RENAMED)\n \t\t\to->found_changes = 1;\n \t} else {\n-\t\tfprintf(o->file, \"* Unmerged path %s\\n\", name);\n+\t\tif (!o->dry_run)\n+\t\t\tfprintf(o->file, \"* Unmerged path %s\\n\", name);\n \t\to->found_changes = 1;\n \t}\n }\n@@ -6194,14 +6198,26 @@ 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+\tint saved_close_file = o->close_file;\n+\tFILE *saved_file = o->file;\n \tint ret;\n \n \to->dry_run = 1;\n \to->found_changes = 0;\n+\to->color_moved = 0;\n+\to->close_file = 1;\n+\to->file = xfopen(\"/dev/null\", \"w\");\n \tdiff_flush_patch(p, o);\n \tret = o->found_changes;\n+\tif (o->file)\n+\t\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->close_file = saved_close_file;\n+\to->file = saved_file;\n \treturn ret;\n }\n \ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 55a06eadb3..2f8fe191b8 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -661,6 +661,43 @@ test_expect_success 'diff -I: ignore matching file' '\n \ttest_grep ! \"file1\" actual\n '\n \n+test_expect_success 'diff -I: ignore all content changes' '\n+\ttest_when_finished \"git rm -f file1 file2 file3\" &&\n+\t: >file1 &&\n+\tgit add file1 &&\n+\t: >file2 &&\n+\tgit add file2 &&\n+\t: >file3 &&\n+\tgit add file3 &&\n+\n+\techo \"A\" >file3 &&\n+\tA_hash=$(git hash-object -w file3) &&\n+\techo \"B\" >file3 &&\n+\tB_hash=$(git hash-object -w file3) &&\n+\tcat <<-EOF | git update-index --index-info &&\n+\t100644 $A_hash 1\tfile3\n+\t100644 $B_hash 2\tfile3\n+\tEOF\n+\n+\trm -f file1 file2 &&\n+\tmkdir file2 &&\n+\ttest_diff_no_content_changes () {\n+\t\tgit diff $1 --ignore-blank-lines -I\".*\" >actual &&\n+\t\ttest_line_count = 3 actual &&\n+\t\ttest_grep \"file1\" actual &&\n+\t\ttest_grep \"file2\" actual &&\n+\t\ttest_grep \"file3\" actual &&\n+\t\ttest_grep ! \"diff --git\" actual\n+\t} &&\n+\ttest_diff_no_content_changes \"--raw\" &&\n+\ttest_diff_no_content_changes \"--name-only\" &&\n+\ttest_diff_no_content_changes \"--name-status\" &&\n+\n+\t: >actual &&\n+\ttest_must_fail git diff --quiet -I\".*\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n # check_prefix \n # check only lines with paths to avoid dependency on exact oid/contents\n check_prefix () {\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.50.1 (Apple Git-155)\n\n"},{"id":"529120","messageId":"FE99A260-ECD9-4B39-9E31-B4E842DC7D04@gmail.com","threadId":"64343","inReplyTo":"20251018094722.GC1060824@coredump.intra.peff.net","subject":"Re: [PATCH] diff: stop output garbled message in dry run mode","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-10-18T09:50:56Z","receivedAt":"2025-10-18T09:51:16Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Jeff King writes:\n> \n> On Sat, Oct 18, 2025 at 09:11:34AM +0800, Lidong Yan wrote:\n> \n>>> Test that exercises \"git diff -I\" is in line with what the\n>>> original b55e6d36eb wanted to address, but given that we saw a\n>>> recent regression report like [*], I would have liked to see \"git\n>>> diff --quiet\" in the test as well.\n>> \n>> I will read Peff’s test and see if I should also add some similar tests\n> \n> What I was hoping was that we'd apply my patch, as a matter of release\n> engineering (backing out the regression-causing bit of b55e6d36eb). And\n> then you could make more-specific fixes on top (since -I would still\n> have potential problems). And then you don't need to add a test for the\n> regression case, since it's already there.\n> \n> -Peff\n\nSorry I sent my patch before I noticed this message.\n\nLidong\n\n"},{"id":"529123","messageId":"20251018095650.GG1060824@coredump.intra.peff.net","threadId":"64343","inReplyTo":"FE99A260-ECD9-4B39-9E31-B4E842DC7D04@gmail.com","subject":"Re: [PATCH] diff: stop output garbled message in dry run mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-18T09:56:50Z","receivedAt":"2025-10-18T09:56:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 18, 2025 at 05:50:56PM +0800, Lidong Yan wrote:\n\n> Jeff King writes:\n> > \n> > On Sat, Oct 18, 2025 at 09:11:34AM +0800, Lidong Yan wrote:\n> > \n> >>> Test that exercises \"git diff -I\" is in line with what the\n> >>> original b55e6d36eb wanted to address, but given that we saw a\n> >>> recent regression report like [*], I would have liked to see \"git\n> >>> diff --quiet\" in the test as well.\n> >> \n> >> I will read Peff’s test and see if I should also add some similar tests\n> > \n> > What I was hoping was that we'd apply my patch, as a matter of release\n> > engineering (backing out the regression-causing bit of b55e6d36eb). And\n> > then you could make more-specific fixes on top (since -I would still\n> > have potential problems). And then you don't need to add a test for the\n> > regression case, since it's already there.\n> > \n> > -Peff\n> \n> Sorry I sent my patch before I noticed this message.\n\nNo worries. I just saw it, and it looks reasonable to me. So while what\nI wrote above was my preferred outcome, I am OK with doing it all as one\npatch, too.\n\n-Peff\n"},{"id":"529129","messageId":"xmqqa51ow6xu.fsf@gitster.g","threadId":"64343","inReplyTo":"20251018094722.GC1060824@coredump.intra.peff.net","subject":"Re: [PATCH] diff: stop output garbled message in dry run mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-18T15:44:29Z","receivedAt":"2025-10-18T15:44:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Oct 18, 2025 at 09:11:34AM +0800, Lidong Yan wrote:\n>\n>> > Test that exercises \"git diff -I\" is in line with what the\n>> > original b55e6d36eb wanted to address, but given that we saw a\n>> > recent regression report like [*], I would have liked to see \"git\n>> > diff --quiet\" in the test as well.\n>> \n>> I will read Peff’s test and see if I should also add some similar tests\n>\n> What I was hoping was that we'd apply my patch, as a matter of release\n> engineering (backing out the regression-causing bit of b55e6d36eb). And\n> then you could make more-specific fixes on top (since -I would still\n> have potential problems). And then you don't need to add a test for the\n> regression case, since it's already there.\n\nYup, that matches my expectation more closely, which is\n\n * We'll do the \"send to /dev/null as we used to do before the\n dry-run thing\" on the 'maint' front, which will be merged up to\n 'master' and above.\n\n * We'll queue \"here are fixes to the recently introduced dry-run\n code\" (without the /dev/null thing mixed in), and cook that in\n the usual 'seen' down to 'next' down to 'master' route.\n\nIn a distant future, we may consider removing the /dev/null thing\nonce the dry-run code path proves to be stable and robust.\n\nThanks.\n"},{"id":"529144","messageId":"DEF57576-E0E5-4F09-B7E7-CE1B8753F088@gmail.com","threadId":"64343","inReplyTo":"xmqqa51ow6xu.fsf@gitster.g","subject":"Re: [PATCH] diff: stop output garbled message in dry run mode","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-10-19T14:31:27Z","receivedAt":"2025-10-19T14:31:41Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Junio C Hamano writes:\n> \n> Yup, that matches my expectation more closely, which is\n> \n> * We'll do the \"send to /dev/null as we used to do before the\n> dry-run thing\" on the 'maint' front, which will be merged up to\n> 'master' and above.\n> \n> * We'll queue \"here are fixes to the recently introduced dry-run\n> code\" (without the /dev/null thing mixed in), and cook that in\n> the usual 'seen' down to 'next' down to 'master' route.\n> \n> In a distant future, we may consider removing the /dev/null thing\n> once the dry-run code path proves to be stable and robust.\n> \n> Thanks.\n\nI am not sure what should I do. Should I make a new patch which\nonly contains “fixes to the recently introduced dry-run code” without\nPeff’s code in it? Or Junio would do that for me?\n\nThanks,\nLidong"},{"id":"529145","messageId":"xmqq4iru52k6.fsf@gitster.g","threadId":"64343","inReplyTo":"DEF57576-E0E5-4F09-B7E7-CE1B8753F088@gmail.com","subject":"Re: [PATCH] diff: stop output garbled message in dry run mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-19T15:33:29Z","receivedAt":"2025-10-19T15:33:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lidong Yan writes:\n\n> Junio C Hamano writes:\n>> \n>> Yup, that matches my expectation more closely, which is\n>> \n>> * We'll do the \"send to /dev/null as we used to do before the\n>> dry-run thing\" on the 'maint' front, which will be merged up to\n>> 'master' and above.\n>> \n>> * We'll queue \"here are fixes to the recently introduced dry-run\n>> code\" (without the /dev/null thing mixed in), and cook that in\n>> the usual 'seen' down to 'next' down to 'master' route.\n>> \n>> In a distant future, we may consider removing the /dev/null thing\n>> once the dry-run code path proves to be stable and robust.\n>> \n>> Thanks.\n>\n> I am not sure what should I do. Should I make a new patch which\n> only contains “fixes to the recently introduced dry-run code” without\n> Peff’s code in it\n\nThat would be my preference, rather than I make up a Chimera out of\nyour initial fix, proposed log message and a single fprintf() fix in\nyour second version in this thread.\n\nThanks.\n"},{"id":"529150","messageId":"20251019162053.14950-1-yldhome2d2@gmail.com","threadId":"64343","inReplyTo":"20251018094823.31173-1-yldhome2d2@gmail.com","subject":"[PATCH v3] diff: stop output garbled message in dry run mode","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-10-19T16:20:53Z","receivedAt":"2025-10-19T16:21:05Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Earlier, b55e6d36 (diff: ensure consistent diff behavior with\nignore options, 2025-08-08) introduced \"dry-run\" mode to the\ndiff machinery so that content-based diff filtering (like\nignoring space changes or those that match -I) can first\ntry to produce a patch without emitting any output to see if\nunder the given diff filtering condition we would get any output\nlines, and a new helper function diff_flush_patch_quietly() was\nintroduced to use the mode to see an individual filepair needs\nto be shown.\n\nHowever, the solution was not complete. When files are deleted,\nfile modes change, or there are unmerged entries in the index,\ndry-run mode still produces output because we overlooked these\nconditions, and as a result, dry-run mode was not quiet.\n\nSince dry-run mode is only set in diff_flush_patch_quietly(),\nsetting the output file to \"/dev/null\" within diff_flush_patch_quietly()\nensures no output is emitted in dry-run mode. To improve performance\nof dry-run mode, add a check before outputting to determine if we\nshould exit early to avoid unnecessary output processing.\n\nSigned-off-by: Lidong Yan \n---\n diff.c | 8 ++++++--\n t/t4013-diff-various.sh | 37 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 87fa16b730..3c92f0d806 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1351,6 +1351,9 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \tint len = eds->len;\n \tunsigned flags = eds->flags;\n \n+\tif (o->dry_run)\n+\t\treturn;\n+\n \tswitch (s) {\n \tcase DIFF_SYMBOL_NO_LF_EOF:\n \t\tcontext = diff_get_color_opt(o, DIFF_CONTEXT);\n@@ -4420,7 +4423,7 @@ static void run_external_diff(const struct external_diff *pgm,\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n-\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH);\n+\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH) || o->dry_run;\n \tint rc;\n \n \t/*\n@@ -4615,7 +4618,8 @@ static void run_diff_cmd(const struct external_diff *pgm,\n \t\t p->status == DIFF_STATUS_RENAMED)\n \t\t\to->found_changes = 1;\n \t} else {\n-\t\tfprintf(o->file, \"* Unmerged path %s\\n\", name);\n+\t\tif (!o->dry_run)\n+\t\t\tfprintf(o->file, \"* Unmerged path %s\\n\", name);\n \t\to->found_changes = 1;\n \t}\n }\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 55a06eadb3..d35695f5b0 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -661,6 +661,43 @@ test_expect_success 'diff -I: ignore matching file' '\n \ttest_grep ! \"file1\" actual\n '\n \n+test_expect_success 'diff -I: ignore all content changes' '\n+\ttest_when_finished \"git rm -f file1 file2 file3\" &&\n+\t: >file1 &&\n+\tgit add file1 &&\n+\t: >file2 &&\n+\tgit add file2 &&\n+\t: >file3 &&\n+\tgit add file3 &&\n+\n+\trm -f file1 file2 &&\n+\tmkdir file2 &&\n+\techo \"A\" >file3 &&\n+\tA_hash=$(git hash-object -w file3) &&\n+\techo \"B\" >file3 &&\n+\tB_hash=$(git hash-object -w file3) &&\n+\tcat <<-EOF | git update-index --index-info &&\n+\t100644 $A_hash 1\tfile3\n+\t100644 $B_hash 2\tfile3\n+\tEOF\n+\n+\ttest_diff_no_content_changes () {\n+\t\tgit diff $1 --ignore-blank-lines -I\".*\" >actual &&\n+\t\ttest_line_count = 3 actual &&\n+\t\ttest_grep \"file1\" actual &&\n+\t\ttest_grep \"file2\" actual &&\n+\t\ttest_grep \"file3\" actual &&\n+\t\ttest_grep ! \"diff --git\" actual\n+\t} &&\n+\ttest_diff_no_content_changes \"--raw\" &&\n+\ttest_diff_no_content_changes \"--name-only\" &&\n+\ttest_diff_no_content_changes \"--name-status\" &&\n+\n+\t: >actual &&\n+\ttest_must_fail git diff --quiet -I\".*\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n # check_prefix \n # check only lines with paths to avoid dependency on exact oid/contents\n check_prefix () {\n-- \n2.50.1 (Apple Git-155)\n\n"},{"id":"529151","messageId":"20251019163024.18939-1-yldhome2d2@gmail.com","threadId":"64343","inReplyTo":"20251018094823.31173-1-yldhome2d2@gmail.com","subject":"[PATCH v4] diff: stop output garbled message in dry run mode","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-10-19T16:30:24Z","receivedAt":"2025-10-19T16:30:40Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Earlier, b55e6d36 (diff: ensure consistent diff behavior with\nignore options, 2025-08-08) introduced \"dry-run\" mode to the\ndiff machinery so that content-based diff filtering (like\nignoring space changes or those that match -I) can first\ntry to produce a patch without emitting any output to see if\nunder the given diff filtering condition we would get any output\nlines, and a new helper function diff_flush_patch_quietly() was\nintroduced to use the mode to see an individual filepair needs\nto be shown.\n\nHowever, the solution was not complete. When files are deleted,\nfile modes change, or there are unmerged entries in the index,\ndry-run mode still produces output because we overlooked these\nconditions, and as a result, dry-run mode was not quiet.\n\nTo fix this, return early in emit_diff_symbol_from_struct() if\nwe are in dry-run mode. This function will be called by all the\nemit functions to output the results. Returning early can avoid\ndiff output when files are deleted or file modes are changed.\nStop print message in dry-run mode if we have unmerged entries\nin index. Discard output of external diff tool in dry-run mode.\n\nSigned-off-by: Lidong Yan \n---\n diff.c | 8 ++++++--\n t/t4013-diff-various.sh | 37 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 87fa16b730..3c92f0d806 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1351,6 +1351,9 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \tint len = eds->len;\n \tunsigned flags = eds->flags;\n \n+\tif (o->dry_run)\n+\t\treturn;\n+\n \tswitch (s) {\n \tcase DIFF_SYMBOL_NO_LF_EOF:\n \t\tcontext = diff_get_color_opt(o, DIFF_CONTEXT);\n@@ -4420,7 +4423,7 @@ static void run_external_diff(const struct external_diff *pgm,\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n-\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH);\n+\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH) || o->dry_run;\n \tint rc;\n \n \t/*\n@@ -4615,7 +4618,8 @@ static void run_diff_cmd(const struct external_diff *pgm,\n \t\t p->status == DIFF_STATUS_RENAMED)\n \t\t\to->found_changes = 1;\n \t} else {\n-\t\tfprintf(o->file, \"* Unmerged path %s\\n\", name);\n+\t\tif (!o->dry_run)\n+\t\t\tfprintf(o->file, \"* Unmerged path %s\\n\", name);\n \t\to->found_changes = 1;\n \t}\n }\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 55a06eadb3..d35695f5b0 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -661,6 +661,43 @@ test_expect_success 'diff -I: ignore matching file' '\n \ttest_grep ! \"file1\" actual\n '\n \n+test_expect_success 'diff -I: ignore all content changes' '\n+\ttest_when_finished \"git rm -f file1 file2 file3\" &&\n+\t: >file1 &&\n+\tgit add file1 &&\n+\t: >file2 &&\n+\tgit add file2 &&\n+\t: >file3 &&\n+\tgit add file3 &&\n+\n+\trm -f file1 file2 &&\n+\tmkdir file2 &&\n+\techo \"A\" >file3 &&\n+\tA_hash=$(git hash-object -w file3) &&\n+\techo \"B\" >file3 &&\n+\tB_hash=$(git hash-object -w file3) &&\n+\tcat <<-EOF | git update-index --index-info &&\n+\t100644 $A_hash 1\tfile3\n+\t100644 $B_hash 2\tfile3\n+\tEOF\n+\n+\ttest_diff_no_content_changes () {\n+\t\tgit diff $1 --ignore-blank-lines -I\".*\" >actual &&\n+\t\ttest_line_count = 3 actual &&\n+\t\ttest_grep \"file1\" actual &&\n+\t\ttest_grep \"file2\" actual &&\n+\t\ttest_grep \"file3\" actual &&\n+\t\ttest_grep ! \"diff --git\" actual\n+\t} &&\n+\ttest_diff_no_content_changes \"--raw\" &&\n+\ttest_diff_no_content_changes \"--name-only\" &&\n+\ttest_diff_no_content_changes \"--name-status\" &&\n+\n+\t: >actual &&\n+\ttest_must_fail git diff --quiet -I\".*\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n # check_prefix \n # check only lines with paths to avoid dependency on exact oid/contents\n check_prefix () {\n-- \n2.50.1 (Apple Git-155)\n\n"},{"id":"529442","messageId":"xmqqms5iyap5.fsf@gitster.g","threadId":"64343","inReplyTo":"20251019163024.18939-1-yldhome2d2@gmail.com","subject":"Re: [PATCH v4] diff: stop output garbled message in dry run mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-22T19:53:58Z","receivedAt":"2025-10-22T19:54:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lidong Yan writes:\n\n> +test_expect_success 'diff -I: ignore all content changes' '\n> +\ttest_when_finished \"git rm -f file1 file2 file3\" &&\n> +\t: >file1 &&\n> +\tgit add file1 &&\n> +\t: >file2 &&\n> +\tgit add file2 &&\n> +\t: >file3 &&\n> +\tgit add file3 &&\n> +\n> +\trm -f file1 file2 &&\n> +\tmkdir file2 &&\n> +\techo \"A\" >file3 &&\n> +\tA_hash=$(git hash-object -w file3) &&\n> +\techo \"B\" >file3 &&\n> +\tB_hash=$(git hash-object -w file3) &&\n> +\tcat <<-EOF | git update-index --index-info &&\n> +\t100644 $A_hash 1\tfile3\n> +\t100644 $B_hash 2\tfile3\n> +\tEOF\n> +\n> +\ttest_diff_no_content_changes () {\n> +\t\tgit diff $1 --ignore-blank-lines -I\".*\" >actual &&\n> +\t\ttest_line_count = 3 actual &&\n> +\t\ttest_grep \"file1\" actual &&\n> +\t\ttest_grep \"file2\" actual &&\n> +\t\ttest_grep \"file3\" actual &&\n\nI am puzzled by this part of the new test.\n\n> +\t\ttest_grep ! \"diff --git\" actual\n\nThe \"test_grep !\" is to make sure we do not leak the \"patch\" output\nrun in diff_flush_patch_quietly(), which is understandable, but in\nthe new world order that even raw, name-only, and name-status honor\n\"diff-from-contents\" since b55e6d36 (diff: ensure consistent diff\nbehavior with ignore options, 2025-08-08), shouldn't we expect empty\n\"actual\" that does not say file1/file2/file3 in it?\n\n> +\t} &&\n> +\ttest_diff_no_content_changes \"--raw\" &&\n> +\ttest_diff_no_content_changes \"--name-only\" &&\n> +\ttest_diff_no_content_changes \"--name-status\" &&\n> +\n> +\t: >actual &&\n> +\ttest_must_fail git diff --quiet -I\".*\" >actual &&\n> +\ttest_must_be_empty actual\n> +'\n> +\n> # check_prefix \n> # check only lines with paths to avoid dependency on exact oid/contents\n> check_prefix () {\n"},{"id":"529454","messageId":"xmqqwm4mwrix.fsf@gitster.g","threadId":"64343","inReplyTo":"xmqqms5iyap5.fsf@gitster.g","subject":"Re: [PATCH v4] diff: stop output garbled message in dry run mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-22T21:33:26Z","receivedAt":"2025-10-22T21:33:29Z","isPatch":true,"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>> +\ttest_diff_no_content_changes () {\n>> +\t\tgit diff $1 --ignore-blank-lines -I\".*\" >actual &&\n>> +\t\ttest_line_count = 3 actual &&\n>> +\t\ttest_grep \"file1\" actual &&\n>> +\t\ttest_grep \"file2\" actual &&\n>> +\t\ttest_grep \"file3\" actual &&\n>\n> I am puzzled by this part of the new test.\n>\n>> +\t\ttest_grep ! \"diff --git\" actual\n>\n> The \"test_grep !\" is to make sure we do not leak the \"patch\" output\n> run in diff_flush_patch_quietly(), which is understandable, but in\n> the new world order that even raw, name-only, and name-status honor\n> \"diff-from-contents\" since b55e6d36 (diff: ensure consistent diff\n> behavior with ignore options, 2025-08-08), shouldn't we expect empty\n> \"actual\" that does not say file1/file2/file3 in it?\n>\n>> +\t} &&\n>> +\ttest_diff_no_content_changes \"--raw\" &&\n>> +\ttest_diff_no_content_changes \"--name-only\" &&\n>> +\ttest_diff_no_content_changes \"--name-status\" &&\n\nI think this was due to the lack of /dev/null redirect around the\nother call site of diff_flush_patch_quietly(). I've rearranged\npatches in this order:\n\n * Peff's /dev/null redirect for --quiet (NO_OUTPUT) codepath around\n diff_flush_patch_quietly();\n\n * My /dev/null redirect for --raw/--name-only/--name-status\n codepath around diff_flush_patch_quietly() on top of the above;\n\n * This patch with tests on top of the above.\n\nAnd that was when I noticed the above test that expects 3 output\nlines was fishy.\n\nI have the following patch on top of your patch that started this\nthread to queue it in 'seen' and have tests pass for today's\nintegration result.\n\n\n\n t/t4013-diff-various.sh | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex d35695f5b0..c0a558da55 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -683,11 +683,7 @@ test_expect_success 'diff -I: ignore all content changes' '\n \n \ttest_diff_no_content_changes () {\n \t\tgit diff $1 --ignore-blank-lines -I\".*\" >actual &&\n-\t\ttest_line_count = 3 actual &&\n-\t\ttest_grep \"file1\" actual &&\n-\t\ttest_grep \"file2\" actual &&\n-\t\ttest_grep \"file3\" actual &&\n-\t\ttest_grep ! \"diff --git\" actual\n+\t\ttest_must_be_empty actual\n \t} &&\n \ttest_diff_no_content_changes \"--raw\" &&\n \ttest_diff_no_content_changes \"--name-only\" &&\n-- \n2.51.1-638-ge1c807bd82\n\n\n\n"},{"id":"529461","messageId":"4CB69AD9-2A1B-46FE-88DA-B98CF81C528A@gmail.com","threadId":"64343","inReplyTo":"xmqqwm4mwrix.fsf@gitster.g","subject":"Re: [PATCH v4] diff: stop output garbled message in dry run mode","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-10-23T00:27:49Z","receivedAt":"2025-10-23T00:28:03Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Junio C Hamano writes:\n> \n> t/t4013-diff-various.sh | 6 +-----\n> 1 file changed, 1 insertion(+), 5 deletions(-)\n> \n> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> index d35695f5b0..c0a558da55 100755\n> --- a/t/t4013-diff-various.sh\n> +++ b/t/t4013-diff-various.sh\n> @@ -683,11 +683,7 @@ test_expect_success 'diff -I: ignore all content changes' '\n> \n> test_diff_no_content_changes () {\n> git diff $1 --ignore-blank-lines -I\".*\" >actual &&\n> - test_line_count = 3 actual &&\n> - test_grep \"file1\" actual &&\n> - test_grep \"file2\" actual &&\n> - test_grep \"file3\" actual &&\n> - test_grep ! \"diff --git\" actual\n> + test_must_be_empty actual\n> } &&\n> test_diff_no_content_changes \"--raw\" &&\n> test_diff_no_content_changes \"--name-only\" &&\n> -- \n> 2.51.1-638-ge1c807bd82\n> \n\nfile1 is removed, file2 changes its mode from a regular file\nto a directory and file3 is unmerged -- but the output is empty?\nI am just a little confused why the ‘actual’ file should be empty.\n\nThanks,\nLidong "},{"id":"529497","messageId":"20251023123055.GA1160519@coredump.intra.peff.net","threadId":"64343","inReplyTo":"20251019163024.18939-1-yldhome2d2@gmail.com","subject":"Re: [PATCH v4] diff: stop output garbled message in dry run mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-23T12:30:55Z","receivedAt":"2025-10-23T12:30:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 20, 2025 at 12:30:24AM +0800, Lidong Yan wrote:\n\n> @@ -4420,7 +4423,7 @@ static void run_external_diff(const struct external_diff *pgm,\n> {\n> \tstruct child_process cmd = CHILD_PROCESS_INIT;\n> \tstruct diff_queue_struct *q = &diff_queued_diff;\n> -\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH);\n> +\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH) || o->dry_run;\n> \tint rc;\n> \n> \t/*\n\nBTW, this hunk is interesting because it is the one spot (that we know\nof!) which cannot be found by looking for mentions of o->file. But I\nthink that is a sign that it was already buggy, because it is not\nrespecting o->file in the first place!\n\nIf I make a simple commit like this:\n\n git init\n echo old >file && git add file && git commit -m old\n echo new >file && git add file && git commit -m new\n\nand then run this:\n\n git diff-tree --output=foo.out -p HEAD^ HEAD\n\nI should get the diff in foo.out, and I do. But if I instead do:\n\n GIT_EXTERNAL_DIFF='echo doing diff:' \\\n git diff-tree --output=foo.out -p --ext-diff HEAD^ HEAD\n\nthen the external diff output goes to stdout. Whoops.\n\nAFAICT this has been the case since \"--output\" was added. So we don't\nneed to worry about it in the context of the current regression.\n\nProbably the solution is something like:\n\ndiff --git a/diff.c b/diff.c\nindex dac3ea9e01..15ef06ac9e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4458,6 +4458,8 @@ static void run_external_diff(const struct external_diff *pgm,\n \tdiff_free_filespec_data(two);\n \tcmd.use_shell = 1;\n \tcmd.no_stdout = quiet;\n+\tfflush(o->file);\n+\tcmd.out = fileno(o->file);\n \trc = run_command(&cmd);\n \tif (!pgm->trust_exit_code && rc == 0)\n \t\to->found_changes = 1;\n\nbut I didn't test it beyond seeing that it makes the command above work.\n\n-Peff\n"}]}