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

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

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.

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

Among the callers of run_diff_cmd(), only the caller that wants to
report "this path is unmerged" passes NULL diff_filespec pointers in
parameters one and two, in which case run_diff_cmd() would give that
message.  So if you have an unmerged filepair in queued_diff, this
callchain

	diff_flush()
	  loop over diff_queued_diff
          -> diff_flush_patch_quietly()
	     fiddle with dry_run bit
	     -> diff_flush_patch()
		-> run_diff()
		   -> run_diff_cmd() with one&two set to NULL

may hit the fprintf into o->file.

So you are right to worry about that fprintf().  If I make a
whitespace-only change to one file, and then make another path
unmerged, here is what I would see:

    $ rungit v2.48.0 diff --raw
    :100644 100644 b82c4963e7 0000000000 M  cache-tree.h
    :000000 100644 0000000000 0000000000 U  t/lib-gpg.sh

This is version before that dry-run thing.  It operated under the
old rule to show "--raw" to report object differences, hence
ignoring "-w".

    $ rungit v2.48.0 diff --raw -w
    :100644 100644 b82c4963e7 0000000000 M  cache-tree.h
    :000000 100644 0000000000 0000000000 U  t/lib-gpg.sh

With a version with the dry_run thing, here is what we see:

    $ git diff --raw -w
    * Unmerged path t/lib-gpg.sh
    :000000 100644 0000000000 0000000000 U  t/lib-gpg.sh

As dry_run thing intended, the entry on the whitespace-only path is
gone from the output, but the fprintf(o->file) you noticed comes out,
which is not what we want to see.  Of course, if we omit -w to avoid
triggering the dry-run thing, we won't see it.

    $ git diff --raw
    :100644 100644 b82c4963e7 0000000000 M  cache-tree.h
    :000000 100644 0000000000 0000000000 U  t/lib-gpg.sh

As a regression-fix change, I'd feel safer with Peff's version.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

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

Ah, sorry for replying before noticing and reading your announce on
2.51.1 that was made hours ago.  It seems that you had a separate
reason to make a release with the CVE fix material quickly, so
please ignore the above.

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

Thanks for explaining how to describe a problem in commit message. Will rewrite
soon.

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

I am using Github pull request for convenience. I think the bot repeat my
commit messages twice.

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

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

Thanks,
Lidong

Lidong Yan <yldhome2d2@gmail.com> writes:

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

Also I think the fprintf() in run_diff_cmd() Dscho noticed is a real
problem.  cf. <xmqqa51pz3ih.fsf@gitster.g>

Thanks for working on this.  

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

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. When files are deleted,
file modes change, or there are unmerged entries in the index,
dry-run mode still produces output because we overlooked these
conditions, and as a result, dry-run mode was not quiet.

Since dry-run mode is only set in diff_flush_patch_quietly(),
setting the output file to "/dev/null" within diff_flush_patch_quietly()
ensures no output is emitted in dry-run mode. To improve performance
of dry-run mode, add a check before outputting to determine if we
should exit early to avoid unnecessary output processing.

Signed-off-by: Lidong Yan <yldhome2d2@gmail.com>
---
I copied Peff's code from https://lore.kernel.org/git/20251017083641.GB4073661@coredump.intra.peff.net/

 diff.c                  | 20 ++++++++++++++++++--
 t/t4013-diff-various.sh | 37 +++++++++++++++++++++++++++++++++++++
 t/t4035-diff-quiet.sh   |  4 ++++
 3 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 87fa16b730..ec05ac565b 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;
 
 	/*
@@ -4615,7 +4618,8 @@ static void run_diff_cmd(const struct external_diff *pgm,
 		    p->status == DIFF_STATUS_RENAMED)
 			o->found_changes = 1;
 	} else {
-		fprintf(o->file, "* Unmerged path %s\n", name);
+		if (!o->dry_run)
+			fprintf(o->file, "* Unmerged path %s\n", name);
 		o->found_changes = 1;
 	}
 }
@@ -6194,14 +6198,26 @@ static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options
 {
 	int saved_dry_run = o->dry_run;
 	int saved_found_changes = o->found_changes;
+	int saved_color_moved = o->color_moved;
+	int saved_close_file = o->close_file;
+	FILE *saved_file = o->file;
 	int ret;
 
 	o->dry_run = 1;
 	o->found_changes = 0;
+	o->color_moved = 0;
+	o->close_file = 1;
+	o->file = xfopen("/dev/null", "w");
 	diff_flush_patch(p, o);
 	ret = o->found_changes;
+	if (o->file)
+		fclose(o->file);
+
 	o->dry_run = saved_dry_run;
 	o->found_changes |= saved_found_changes;
+	o->color_moved = saved_color_moved;
+	o->close_file = saved_close_file;
+	o->file = saved_file;
 	return ret;
 }
 
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 55a06eadb3..2f8fe191b8 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -661,6 +661,43 @@ 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 file3" &&
+	: >file1 &&
+	git add file1 &&
+	: >file2 &&
+	git add file2 &&
+	: >file3 &&
+	git add file3 &&
+
+	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
+
+	rm -f file1 file2 &&
+	mkdir file2 &&
+	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_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 () {
diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh
index 0352bf81a9..35eaf0855f 100755
--- a/t/t4035-diff-quiet.sh
+++ b/t/t4035-diff-quiet.sh
@@ -50,6 +50,10 @@ test_expect_success 'git diff-tree HEAD HEAD' '
 	test_expect_code 0 git diff-tree --quiet HEAD HEAD >cnt &&
 	test_line_count = 0 cnt
 '
+test_expect_success 'git diff-tree -w HEAD^ HEAD' '
+	test_expect_code 1 git diff-tree --quiet -w HEAD^ HEAD >cnt &&
+	test_line_count = 0 cnt
+'
 test_expect_success 'git diff-files' '
 	test_expect_code 0 git diff-files --quiet >cnt &&
 	test_line_count = 0 cnt
-- 
2.50.1 (Apple Git-155)

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.

Lidong

On Sat, Oct 18, 2025 at 05:50:56PM +0800, Lidong Yan wrote:

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

No worries. I just saw it, and it looks reasonable to me. So while what
I wrote above was my preferred outcome, I am OK with doing it all as one
patch, too.

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

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? Or Junio would do that for me?

Thanks,
Lidong

Lidong Yan <yldhome2d2@gmail.com> writes:

> 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

That would be my preference, rather than I make up a Chimera out of
your initial fix, proposed log message and a single fprintf() fix in
your second version in this thread.

Thanks.

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. When files are deleted,
file modes change, or there are unmerged entries in the index,
dry-run mode still produces output because we overlooked these
conditions, and as a result, dry-run mode was not quiet.

Since dry-run mode is only set in diff_flush_patch_quietly(),
setting the output file to "/dev/null" within diff_flush_patch_quietly()
ensures no output is emitted in dry-run mode. To improve performance
of dry-run mode, add a check before outputting to determine if we
should exit early to avoid unnecessary output processing.

Signed-off-by: Lidong Yan <yldhome2d2@gmail.com>
---
 diff.c                  |  8 ++++++--
 t/t4013-diff-various.sh | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 87fa16b730..3c92f0d806 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;
 
 	/*
@@ -4615,7 +4618,8 @@ static void run_diff_cmd(const struct external_diff *pgm,
 		    p->status == DIFF_STATUS_RENAMED)
 			o->found_changes = 1;
 	} else {
-		fprintf(o->file, "* Unmerged path %s\n", name);
+		if (!o->dry_run)
+			fprintf(o->file, "* Unmerged path %s\n", name);
 		o->found_changes = 1;
 	}
 }
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 55a06eadb3..d35695f5b0 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -661,6 +661,43 @@ 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 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_grep ! "diff --git" 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 () {
-- 
2.50.1 (Apple Git-155)

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. When files are deleted,
file modes change, or there are unmerged entries in the index,
dry-run mode still produces output because we overlooked these
conditions, and as a result, dry-run mode was not quiet.

To fix this, return early in emit_diff_symbol_from_struct() if
we are in dry-run mode. This function will be called by all the
emit functions to output the results. Returning early can avoid
diff output when files are deleted or file modes are changed.
Stop print message in dry-run mode if we have unmerged entries
in index. Discard output of external diff tool in dry-run mode.

Signed-off-by: Lidong Yan <yldhome2d2@gmail.com>
---
 diff.c                  |  8 ++++++--
 t/t4013-diff-various.sh | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 87fa16b730..3c92f0d806 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;
 
 	/*
@@ -4615,7 +4618,8 @@ static void run_diff_cmd(const struct external_diff *pgm,
 		    p->status == DIFF_STATUS_RENAMED)
 			o->found_changes = 1;
 	} else {
-		fprintf(o->file, "* Unmerged path %s\n", name);
+		if (!o->dry_run)
+			fprintf(o->file, "* Unmerged path %s\n", name);
 		o->found_changes = 1;
 	}
 }
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 55a06eadb3..d35695f5b0 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -661,6 +661,43 @@ 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 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_grep ! "diff --git" 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 () {
-- 
2.50.1 (Apple Git-155)

Lidong Yan <yldhome2d2@gmail.com> writes:

> +test_expect_success 'diff -I<regex>: ignore all content changes' '
> +	test_when_finished "git rm -f file1 file2 file3" &
E800
&
> +	: >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 &&

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" &&
> +
> +	: >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 () {

Junio C Hamano <gitster@pobox.com> writes:

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

I think this was due to the lack of /dev/null redirect around the
other call site of diff_flush_patch_quietly().  I've rearranged
patches in this order:

 * Peff's /dev/null redirect for --quiet (NO_OUTPUT) codepath around
   diff_flush_patch_quietly();

 * My /dev/null redirect for --raw/--name-only/--name-status
   codepath around diff_flush_patch_quietly() on top of the above;

 * This patch with tests on top of the above.

And that was when I noticed the above test that expects 3 output
lines was fishy.

I have the following patch on top of your patch that started this
thread to queue it in 'seen' and have tests pass for today's
integration result.



 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

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

file1 is removed, file2 changes its mode from a regular file
to a directory and file3 is unmerged -- but the output is empty?
I am just a little confused why the ‘actual’ file should be empty.

Thanks,
Lidong