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

[Junio C Hamano <gitster@pobox.com>]

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

Thanks.


[Reference]

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