From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Fri, 17 Oct 2025 12:07:50 GMT
Subject: Re: [PATCH] diff: stop output garbled message in dry run mode
Message-ID: <4ff55fc5-7880-b8bf-257f-3186552e9c36@gmx.de>
In-Reply-To: <pull.2071.git.git.1760671049113.gitgitgadget@gmail.com>

Hi,

On Fri, 17 Oct 2025, Lidong Yan via GitGitGadget wrote:

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

Very good. This is a minimal change that covers all of the `emit_*()`
calls (except for `checkdiff_consume()`, but if the `--check` code path
is entered under `o->dry_run`, it is debatable whether or not it should
output something, therefore we could claim that this is "by design").

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. Granted, this is quite tedious to reason
about and requires considerable cognitive load to analyze, but judging
from past attempts to land patches that simplify logic e.g. in
https://lore.kernel.org/git/pull.1888.git.1743079429.gitgitgadget@gmail.com/
I have concluded that core reviewers on this mailing list delight too much
in such analyses to be interested in making Git's code easier to reason
about.

>  	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
> +	} &&

Nice! While this function obviously is not strictly scoped to this test
case (it will still be defined when the next test case is executed), it is
wonderful to see the structure that helps readers along.

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

Ciao,
Johannes