diff: stop output garbled message in dry run mode

From: Lidong Yan <[email protected]>

In dry run mode, diff_flush_patch() should not produce any output. However, in commit b55e6d36eb (diff: ensure consistent diff behavior with ignore options, 2025-08-08), only the output during the comparison of two file contents was suppressed. For file deletions or mode changes, diff_flush_patch() still produces output. In run_extern_diff(), set quiet to true if in dry run mode. In emit_diff_symbol_from_struct(), directly return if in dry run mode.

Signed-off-by Lidong Yan <[email protected]>

[...]

diff.c +0 -0

index 87fa16b730..4baf9b535e 100644

diff.c +1 -1

 	switch (s) {
 	case DIFF_SYMBOL_NO_LF_EOF:
 		context = diff_get_color_opt(o, DIFF_CONTEXT);

@@ -4420,7 +4423,7 @@ static void run_external_diff(const struct external_diff *pgm,
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct diff_queue_struct *q = &diff_queued_diff;
-	int quiet = !(o->output_format & DIFF_FORMAT_PATCH);
+	int quiet = !(o->output_format & DIFF_FORMAT_PATCH) || o->dry_run;
 	int rc;
 
 	/*

t/t4013-diff-various.sh +6 -0

index 55a06eadb3..25fa452656 100755
@@ -661,6 +661,27 @@ test_expect_success 'diff -I<regex>: ignore matching file' '
 	test_grep ! "file1" actual
 '
 
+test_expect_success 'diff -I<regex>: ignore all content changes' '
+	test_when_finished "git rm -f file1 file2" &&
+	: >file1 &&
+	git add file1 &&
+	: >file2 &&
+	git add file2 &&

t/t4013-diff-various.sh +5 -0

+	test_diff_no_content_changes "--raw" &&
+	test_diff_no_content_changes "--name-only" &&
+	test_diff_no_content_changes "--name-status"
+'
+
 # check_prefix <patch> <src> <dst>
 # check only lines with paths to avoid dependency on exact oid/contents
 check_prefix () {

base-commit: 143f58ef7535f8f8a80d810768a18bdf3807de26

Thank you for fixing this so quickly! From my point of view, this is ready to go. I will integrate this patch into Git for Windows v2.51.1 (which I am sadly forced to release on a Friday).

Quoted 3 lines · Johannes Schindelin

Thank you for fixing this so quickly! From my point of view, this is ready to go. I will integrate this patch into Git for Windows v2.51.1 (which I am sadly forced to release on a Friday).

You may not want to. I think I'll have to do 2.51.2 either with Peff's fix (or a rerolled version of this one if it comes quickly enough) early next week anyway.

Thanks.

I do see a still-unguarded fprintf(o->file, ...) call in run_diff_cmd(), but as far as I can see, this call is not in any code path where dry_run is set.

From: Lidong Yan <[email protected]>

In dry run mode, diff_flush_patch() should not produce any output. However, in commit b55e6d36eb (diff: ensure consistent diff behavior with ignore options, 2025-08-08), only the output during the comparison of two file contents was suppressed. For file deletions or mode changes, diff_flush_patch() still produces output. In run_extern_diff(), set quiet to true if in dry run mode. In emit_diff_symbol_from_struct(), directly return if in dry run mode.

Signed-off-by Lidong Yan <[email protected]>

---
diff: stop output garbled message in dry run mode

In dry run mode, diff_flush_patch() should not produce any output. However, in commit b55e6d36eb (diff: ensure consistent diff behavior with ignore options, 2025-08-08), only the output during the comparison of two file contents was suppressed. For file deletions or mode changes, diff_flush_patch() still produces output. In run_extern_diff(), set quiet to true if in dry run mode. In emit_diff_symbol_from_struct(), directly return if in dry run mode.

t/t4013-diff-various.sh +10 -0

index 55a06eadb3..25fa452656 100755
@@ -661,6 +661,27 @@ test_expect_success 'diff -I<regex>: ignore matching file' '
 	test_grep ! "file1" actual
 '
 
+test_expect_success 'diff -I<regex>: ignore all content changes' '
+	test_when_finished "git rm -f file1 file2" &&
+	: >file1 &&
+	git add file1 &&
+	: >file2 &&
+	git add file2 &&
+
+	rm -f file1 file2 &&
+	mkdir file2 &&
+	test_diff_no_content_changes () {

Quoted 9 lines · Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>

In dry run mode, diff_flush_patch() should not produce any output. However, in commit b55e6d36eb (diff: ensure consistent diff behavior with ignore options, 2025-08-08), only the output during the comparison of two file contents was suppressed. For file deletions or mode changes, diff_flush_patch() still produces output. In run_extern_diff(), set quiet to true if in dry run mode. In emit_diff_symbol_from_struct(), directly return if in dry run mode.

The above makes it sound as if the dry-run mode was an inherent part of the diff machinery that existed even before b55e6d36 came, and b55e6d36 somehow broke it. But that is not what you are telling us, I think.

You may know what the "dry-run" mode is, but others don't. You should tell the backstory a bit better to help them. I am guessing that this patch is to fix a breakage introduced when the dry-run mode is added in b55e6d36 (diff: ensure consistent diff behavior
with ignore options, 2025-08-08)? If so, I would expect an
explanation like ...

Earlier, b55e6d36 (diff: ensure consistent diff behavior with ignore options, 2025-08-08) introduced "dry-run" mode to the diff machinery so that content based diff filtering (like ignoring space changes or those that match -I<regex>) can first try to produce a patch without emitting any output to see if under the given diff filtering condition we would get any output lines, and a new helper function diff_flush_patch_quietly() was introduced to use the mode to see an individual filepair needs to be shown.

However, the solution was not complete. IN SUCH AND SUCH CASES, THIS BAD THING HAPPENED BECAUSE WE OVERLOOKED THIS AND THAT CONDITION, AND AS A RESULT, DRY-RUN MODE WAS NOT QUIET.

To fix this, DO THIS AND THAT. THIS WOULD AFFECT ONLY SUCH AND SUCH CASES WITHOUT AFFECTING OTHER CODE PATHS LIKE DOING X AND Y.

... is given to help readers understand what we wanted to do in the earlier commit, what we failed to do there and why, and what we can do at this point to clean up the mess without making further damange.

Quoted 11 lines

Signed-off-by Lidong Yan <[email protected]>

---
diff: stop output garbled message in dry run mode

In dry run mode, diff_flush_patch() should not produce any output. However, in commit b55e6d36eb (diff: ensure consistent diff behavior with ignore options, 2025-08-08), only the output during the comparison of two file contents was suppressed. For file deletions or mode changes, diff_flush_patch() still produces output. In run_extern_diff(), set quiet to true if in dry run mode. In emit_diff_symbol_from_struct(), directly return if in dry run mode.

The "below three-dash" space is a place to explain what does not have to be a part of the resulting commit but would help those who are reading the mailing list and reviewing. Repeating the same thing as the proposed log message does not help readers.

Quoted 26 lines

t/t4013-diff-various.sh +0 -0

index 55a06eadb3..25fa452656 100755
@@ -661,6 +661,27 @@ test_expect_success 'diff -I<regex>: ignore matching file' '

test_grep ! "file1" actual
'

t/t4013-diff-various.sh +17 -0

+test_expect_success 'diff -I<regex>: ignore all content changes' '
+ test_when_finished "git rm -f file1 file2" &&
+ : >file1 &&
+ git add file1 &&
+ : >file2 &&
+ git add file2 &&
+
+ rm -f file1 file2 &&
+ mkdir file2 &&
+ test_diff_no_content_changes () {
+ git diff $1 --ignore-blank-lines -I".*" >actual &&
+ test_line_count = 2 actual &&
+ test_grep "file1" actual &&
+ test_grep "file2" actual &&
+ test_grep ! "diff --git" actual
+ } &&
+ test_diff_no_content_changes "--raw" &&

Quoted 3 lines

+ test_diff_no_content_changes "--name-only" &&
+ test_diff_no_content_changes "--name-status"
+'

Test that exercises "git diff -I<regex>" is in line with what the original b55e6d36eb wanted to address, but given that we saw a recent regression report like [*], I would have liked to see "git diff --quiet" in the test as well.

* https://lore.kernel.org/git/CACJRbWjwOQwJB13CwTfvhV3p+Hbn4KrNM9AtBanGtUS4V_1MbQ@mail.gmail.com/

Quoted 5 lines · Junio C Hamano

...
Test that exercises "git diff -I<regex>" is in line with what the original b55e6d36eb wanted to address, but given that we saw a recent regression report like [*], I would have liked to see "git diff --quiet" in the test as well.

I will read Peff’s test and see if I should also add some similar tests

Quoted 1 line

* https://lore.kernel.org/git/CACJRbWjwOQwJB13CwTfvhV3p+Hbn4KrNM9AtBanGtUS4V_1MbQ@mail.gmail.com/

Quoted 4 lines

Test that exercises "git diff -I<regex>" is in line with what the original b55e6d36eb wanted to address, but given that we saw a recent regression report like [*], I would have liked to see "git diff --quiet" in the test as well.

I will read Peff’s test and see if I should also add some similar tests

Quoted 6 lines · Lidong Yan

Quoted 4 lines

Test that exercises "git diff -I<regex>" is in line with what the original b55e6d36eb wanted to address, but given that we saw a recent regression report like [*], I would have liked to see "git diff --quiet" in the test as well.

I will read Peff’s test and see if I should also add some similar tests

What I was hoping was that we'd apply my patch, as a matter of release engineering (backing out the regression-causing bit of b55e6d36eb). And then you could make more-specific fixes on top (since -I would still have potential problems). And then you don't need to add a test for the regression case, since it's already there.

-Peff

Quoted 16 lines · Jeff King

Quoted 6 lines · Lidong Yan

Quoted 4 lines

Test that exercises "git diff -I<regex>" is in line with what the original b55e6d36eb wanted to address, but given that we saw a recent regression report like [*], I would have liked to see "git diff --quiet" in the test as well.

I will read Peff’s test and see if I should also add some similar tests

What I was hoping was that we'd apply my patch, as a matter of release engineering (backing out the regression-causing bit of b55e6d36eb). And then you could make more-specific fixes on top (since -I would still have potential problems). And then you don't need to add a test for the regression case, since it's already there.

Quoted 1 line

-Peff

Sorry I sent my patch before I noticed this message.

Quoted 5 lines · Lidong Yan

Quoted 4 lines

Test that exercises "git diff -I<regex>" is in line with what the original b55e6d36eb wanted to address, but given that we saw a recent regression report like [*], I would have liked to see "git diff --quiet" in the test as well.

Quoted 1 line

I will read Peff’s test and see if I should also add some similar tests

What I was hoping was that we'd apply my patch, as a matter of release engineering (backing out the regression-causing bit of b55e6d36eb). And then you could make more-specific fixes on top (since -I would still have potential problems). And then you don't need to add a test for the regression case, since it's already there.

Yup, that matches my expectation more closely, which is

* We'll do the "send to /dev/null as we used to do before the dry-run thing" on the 'maint' front, which will be merged up to 'master' and above.

* We'll queue "here are fixes to the recently introduced dry-run code" (without the /dev/null thing mixed in), and cook that in the usual 'seen' down to 'next' down to 'master' route.

In a distant future, we may consider removing the /dev/null thing once the dry-run code path proves to be stable and robust.

Thanks.

Quoted 11 lines · Junio C Hamano

Yup, that matches my expectation more closely, which is

* We'll do the "send to /dev/null as we used to do before the dry-run thing" on the 'maint' front, which will be merged up to 'master' and above.

* We'll queue "here are fixes to the recently introduced dry-run code" (without the /dev/null thing mixed in), and cook that in the usual 'seen' down to 'next' down to 'master' route.

Quoted 4 lines

In a distant future, we may consider removing the /dev/null thing once the dry-run code path proves to be stable and robust.

Thanks.

I am not sure what should I do. Should I make a new patch which only contains “fixes to the recently introduced dry-run code” without Peff’s code in it

t/t4013-diff-various.sh +16 -0

+test_expect_success 'diff -I<regex>: ignore all content changes' '
+	test_when_finished "git rm -f file1 file2 file3" &&
+	: >file1 &&
+	git add file1 &&
+	: >file2 &&
+	git add file2 &&
+	: >file3 &&
+	git add file3 &&
+
+	rm -f file1 file2 &&
+	mkdir file2 &&
+	echo "A" >file3 &&
+	A_hash=$(git hash-object -w file3) &&
+	echo "B" >file3 &&
+	B_hash=$(git hash-object -w file3) &&
+	cat <<-EOF | git update-index --index-info &&

t/t4013-diff-various.sh +10 -0

+	} &&
+	test_diff_no_content_changes "--raw" &&
+	test_diff_no_content_changes "--name-only" &&
+	test_diff_no_content_changes "--name-status" &&
+
+	: >actual &&
+	test_must_fail git diff --quiet -I".*" >actual &&
+	test_must_be_empty actual
+'
+
 # check_prefix <patch> <src> <dst>
 # check only lines with paths to avoid dependency on exact oid/contents
 check_prefix () {

Lidong Yan <[email protected]> writes:
...

Review

Quoted 6 lines

(quoted diff) +6 -0

+	test_diff_no_content_changes () {
+		git diff $1 --ignore-blank-lines -I".*" >actual &&
+		test_line_count = 3 actual &&
+		test_grep "file1" actual &&
+		test_grep "file2" actual &&
+		test_grep "file3" actual &&

I am puzzled by this part of the new test.

Review

Quoted 1 line

(quoted diff) +1 -0

+		test_grep ! "diff --git" actual

The "test_grep !" is to make sure we do not leak the "patch" output run in diff_flush_patch_quietly(), which is understandable, but in the new world order that even raw, name-only, and name-status honor "diff-from-contents" since b55e6d36 (diff: ensure consistent diff behavior with ignore options, 2025-08-08), shouldn't we expect empty "actual" that does not say file1/file2/file3 in it?

Review

Quoted 4 lines

(quoted diff) +4 -0

+	} &&
+	test_diff_no_content_changes "--raw" &&
+	test_diff_no_content_changes "--name-only" &&
+	test_diff_no_content_changes "--name-status" &&

t/t4013-diff-various.sh | 6 +-----

1 file changed, 1 insertion(+), 5 deletions(-)

t/t4013-diff-various.sh +0 -0

index d35695f5b0..c0a558da55 100755
@@ -683,11 +683,7 @@ test_expect_success 'diff -I<regex>: ignore all content changes' '
 
 test_diff_no_content_changes () {

git diff $1 --ignore-blank-lines -I".*" >actual &&
- test_line_count = 3 actual &&

- test_grep "file1" actual &&
- test_grep "file2" actual &&
- test_grep "file3" actual &&
- test_grep ! "diff --git" actual
+ test_must_be_empty actual
} &&
test_diff_no_content_changes "--raw" &&
test_diff_no_content_changes "--name-only" &&

Signature · 1 line

2.51.1-638-ge1c807bd82