patchdiff: stop output garbled message in dry run mode

> From: Lidong Yan <yldhome2d2@gmail.com>
> 
> 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 <yldhome2d2@gmail.com>
>
> [...]
>
> diff --git a/diff.c b/diff.c
> index 87fa16b730..4baf9b535e 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1351,6 +1351,9 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
>  	int len = eds->len;
>  	unsigned flags = eds->flags;
>  
> +	if (o->dry_run)
> +		return;
> +

>  	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;
>  
>  	/*
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index 55a06eadb3..25fa452656 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -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 () {
> +		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" &&
> +	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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> 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.

> From: Lidong Yan <yldhome2d2@gmail.com>
>
> 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 <yldhome2d2@gmail.com>
> ---
>     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.

> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index 55a06eadb3..25fa452656 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -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 () {
> +		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" &&
> +	test_diff_no_content_changes "--name-only" &&
> +	test_diff_no_content_changes "--name-status"
> +'

> 
> "Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Lidong Yan <yldhome2d2@gmail.com>
>> 
>> 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.
> 
>> Signed-off-by: Lidong Yan <yldhome2d2@gmail.com>
>> ---
>>    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.

> 
>> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
>> index 55a06eadb3..25fa452656 100755
>> --- a/t/t4013-diff-various.sh
>> +++ b/t/t4013-diff-various.sh
>> @@ -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 () {
>> + 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" &&
>> + 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.

> Junio C Hamano <gitster@pobox.com> writes:
>> ...
>> 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
>
>> * https://lore.kernel.org/git/CACJRbWjwOQwJB13CwTfvhV3p+Hbn4KrNM9AtBanGtUS4V_1MbQ@mail.gmail.com/

> > 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

> 
> On Sat, Oct 18, 2025 at 09:11:34AM +0800, Lidong Yan wrote:
> 
>>> 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

> Jeff King <peff@peff.net> writes:
> > 
> > On Sat, Oct 18, 2025 at 09:11:34AM +0800, Lidong Yan wrote:
> > 
> >>> 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
> 
> Sorry I sent my patch before I noticed this message.

> On Sat, Oct 18, 2025 at 09:11:34AM +0800, Lidong Yan wrote:
>
>> > 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.

> 
> 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.

> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> 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.
>
> 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

> +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 &&
> +	100644 $A_hash 1	file3
> +	100644 $B_hash 2	file3
> +	EOF
> +
> +	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_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 <yldhome2d2@gmail.com> writes:
> ...
>> +	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.
>
>> +		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?
>
>> +	} &&
>> +	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(-)
> 
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index d35695f5b0..c0a558da55 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -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" &&
> -- 
> 2.51.1-638-ge1c807bd82
> 

> @@ -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;
>  
>  	/*