Re: [PATCH] diff: stop output garbled message in dry run mode

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 17, 2025, 16:17 UTC
Message-ID: <xmqqh5vx1p0q.fsf@gitster.g>
In-Reply-To: <pull.2071.git.git.1760671049113.gitgitgadget@gmail.com>

"Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 9 quoted lines

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

Show 11 quoted lines

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

Show 28 quoted lines

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

Thanks.

[Reference]

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