git bug report

git bug report

[Paul Watson]

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)

9:43:55.45 2023-08-04  C:\src\t\scripts>TYPE .\t1.txt
this is t1.txt

9:43:57.92 2023-08-04  C:\src\t\scripts>TYPE .\t2.txt
this is t2.txt

9:43:58.04 2023-08-04  C:\src\t\scripts>"C:\Program Files\Git\cmd\git.exe" diff --exit-code --no-index  --ignore-all-space  --shortstat  .\t1.txt .\t2.txt
1 file changed, 1 insertion(+), 1 deletion(-)

9:43:58.14 2023-08-04  C:\src\t\scripts>ECHO %ERRORLEVEL%
0

What did you expect to happen? (Expected behavior)

I expected that the exit code from `git diff` would be 1, or something non-zero because the files are different.

What happened instead? (Actual behavior)

The exit code was zero (0).

What's different between what you expected and what actually happened?

Zero (0) is not one (1). :-)

Anything else you want to add:

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.39.2.windows.1
cpu: x86_64
built from commit: a82fa99b36ddfd643e61ed45e52abe314687df67
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Windows 10.0 19044 
compiler info: gnuc: 12.2
libc info: no libc information available
$SHELL (typically, interactive shell): <unset>
    PowerShell 7.3.6 Console on Windows 10

[Enabled Hooks]
This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or intended recipient’s authorized agent, the reader is hereby
notified that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.

RE: git bug report

[rsbecker]

On Friday, August 4, 2023 12:46 PM, Paul Watson wrote:
>Thank you for filling out a Git bug report!
>Please answer the following questions to help us understand your issue.
>
>What did you do before the bug happened? (Steps to reproduce your issue)
>
>9:43:55.45 2023-08-04  C:\src\t\scripts>TYPE .\t1.txt this is t1.txt
>
>9:43:57.92 2023-08-04  C:\src\t\scripts>TYPE .\t2.txt this is t2.txt
>
>9:43:58.04 2023-08-04  C:\src\t\scripts>"C:\Program Files\Git\cmd\git.exe" diff --exit-
>code --no-index  --ignore-all-space  --shortstat  .\t1.txt .\t2.txt
>1 file changed, 1 insertion(+), 1 deletion(-)

Can you retry this without --shortstat so we can see what git is reporting as a difference.

Thanks,
Randall

RE: [EXTERNAL] RE: git bug report

[Paul Watson]

-----Original Message-----
From: rsbecker@nexbridge.com <rsbecker@nexbridge.com> 
Sent: Friday, August 4, 2023 12:29

>> Can you retry this without --shortstat so we can see what git is
>> reporting as a difference.

The problem appears to be the use of --shortstat. When --shortstat is not used, the exit code for different files is one (1), as expected.

12:46:56.88 2023-08-04  C:\src\t\scripts>TYPE t1.txt
this is t1.txt
this is t1.txt
this is t1.txt

12:47:01.88 2023-08-04  C:\src\t\scripts>type t2.txt
this is t2.txt
this is t2.txt
this is t2.txt

12:47:06.41 2023-08-04  C:\src\t\scripts>"C:\Program Files\Git\cmd\git.exe" diff --exit-code --no-index  --ignore-all-space --shortstat .\t1.txt .\t2.txt
 1 file changed, 3 insertions(+), 3 deletions(-)

12:47:18.22 2023-08-04  C:\src\t\scripts>ECHO %ERRORLEVEL%
0

12:47:20.61 2023-08-04  C:\src\t\scripts>"C:\Program Files\Git\cmd\git.exe" diff --exit-code --no-index --ignore-all-space .\t1.txt .\t2.txt
diff --git a/./t1.txt b/./t2.txt
index 6dfb884..66812c5 100644
--- a/./t1.txt
+++ b/./t2.txt
@@ -1,3 +1,3 @@
-this is t1.txt
-this is t1.txt
-this is t1.txt
+this is t2.txt
+this is t2.txt
+this is t2.txt

12:47:27.71 2023-08-04  C:\src\t\scripts>ECHO %ERRORLEVEL%
1

12:47:30.32 2023-08-04  C:\src\t\scripts>"C:\Program Files\Git\cmd\git.exe" diff --quiet --exit-code --no-index --ignore-all-space .\t1.txt .\t2.txt

12:47:36.58 2023-08-04  C:\src\t\scripts>ECHO %ERRORLEVEL%
1
This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or intended recipient’s authorized agent, the reader is hereby
notified that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.

RE: [EXTERNAL] RE: git bug report

[rsbecker]

On Friday, August 4, 2023 1:49 PM, Paul Watson wrote:
>From: rsbecker@nexbridge.com <rsbecker@nexbridge.com>
>Sent: Friday, August 4, 2023 12:29
>
>>> Can you retry this without --shortstat so we can see what git is
>>> reporting as a difference.
>
>The problem appears to be the use of --shortstat. When --shortstat is not used, the
>exit code for different files is one (1), as expected.

I am not convinced this is a defect. The result of --shortstat is to report that there are differences, so it is possible that 0 is correct here. Others might have a different opinion. I can recreate without including any other options.

RE: [EXTERNAL] RE: git bug report

[Paul Watson]

From: rsbecker@nexbridge.com <rsbecker@nexbridge.com> 

> I am not convinced this is a defect. The result of --shortstat is to
> report that there are differences, so it is possible that 0 is correct
> here. Others might have a different opinion. I can recreate without
> including any other options.

The code has already done the work to identify if there are differences. Why would it not return a meaningful exit code?
This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or intended recipient’s authorized agent, the reader is hereby
notified that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.

RE: [EXTERNAL] RE: git bug report

[Paul Watson]

>From: rsbecker@nexbridge.com <rsbecker@nexbridge.com>
>
>> I am not convinced this is a defect. The result of --shortstat is to 
>> report that there are differences, so it is possible that 0 is correct 
>> here. Others might have a different opinion. I can recreate without 
>> including any other options.
>
> The code has already done the work to identify if there are differences. > Why would it not return a meaningful exit code?

Also, since the --exit-code switch has been specified, it should set the exit code accordingly.
This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or intended recipient’s authorized agent, the reader is hereby
notified that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.

RE: [EXTERNAL] RE: git bug report

[rsbecker]

On Tuesday, August 8, 2023 9:08 AM, Paul Watson wrote:
>>From: rsbecker@nexbridge.com <rsbecker@nexbridge.com>
>>
>>> I am not convinced this is a defect. The result of --shortstat is to
>>> report that there are differences, so it is possible that 0 is
>>> correct here. Others might have a different opinion. I can recreate
>>> without including any other options.
>>
>> The code has already done the work to identify if there are differences. > Why
>would it not return a meaningful exit code?
>
>Also, since the --exit-code switch has been specified, it should set the exit code
>accordingly.

I think this is a decision for the maintainers, whether I agree or disagree. I have to defer to others.

--Randall

Re: git bug report

[Junio C Hamano]

Paul Watson <pwatson2@wellmed.net> writes:

> 9:43:55.45 2023-08-04  C:\src\t\scripts>TYPE .\t1.txt
> this is t1.txt

> 9:43:57.92 2023-08-04  C:\src\t\scripts>TYPE .\t2.txt
> this is t2.txt
>
> 9:43:58.04 2023-08-04  C:\src\t\scripts>"C:\Program Files\Git\cmd\git.exe" diff --exit-code --no-index  --ignore-all-space  --shortstat  .\t1.txt .\t2.txt
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 9:43:58.14 2023-08-04  C:\src\t\scripts>ECHO %ERRORLEVEL%
> 0

This is not specific to Windows port and it can be reproduced on a
random Linux box.

    $ echo one >1
    $ echo two >2
    $ git diff --no-index --shortstat 1 2; echo "<<$?>>"
     1 file changed, 1 insertion(+), 1 deletion(-)
    <<1>>
    $ git diff --no-index --shortstat -w 1 2; echo "<<$?>>"
     1 file changed, 1 insertion(+), 1 deletion(-)
    <<0>>

Note that I omitted "--exit-code" in the above reproduction, as it
is always used in the no-index mode.  There seems to be interaction
with "-w" and not using "-p", as this is not limited to "--shortstat".

    $ git diff --no-index -w --stat 1 2; echo "<<$?>>"
     1 => 2 | 2 +-
     1 file changed, 1 insertion(+), 1 deletion(-)
    <<0>>

    $ git diff --no-index -w --numstat 1 2; echo "<<$?>>"
    1	1	1 => 2
    <<0>>

All of the above that exits with 0 status will exit with 1 when -p
is added to the command line.

Also, this is not limited to the no-index mode.

    $ echo one >1
    $ git add 1
    $ echo two >1
    $ git diff --exit-code --numstat 1; echo "<<$?>>"
    1	1	1
    <<1>>
    $ git diff --exit-code --numstat -w 1; echo "<<$?>>"
    1	1	1
    <<0>>

So the minimum reproduction seems to be

  * the diff machinery is asked to do --exit-code (no-index
    implicitly does it)
  * -w is used
  * -p is *not* used
  * to compare two different files.

Thanks for a bug report.

Patches welcome ;-)

[PATCH] diff: tighten interaction between -w and --exit-code

[Junio C Hamano]

The "diff" family of commands have "--exit-code" option to make them
report if they "saw" changes with their exit code, but it did not work
when options in the "ignore space" family is used *and* the output mode
did not involve the "--patch" format output, unless the output is
totally suppressed with "-s".

Internally, the diff machinery grabs sets of paths from the two
sides being compared that record different blob objects, optionally
matches these paths up for rename processing, and compares the blob
objects pair-wise.  When options like "-w" are *not* involved, the
machinery can say that it see a difference immediately after
noticing that the object names of the blobs at paths being compared
are different.  In essence, this means that a non-empty diff_queue[]
means the command should exit with 1 under "--exit-code" mode.

But when options that may make two different blobs compare as
equivalents, like "-w" that ignores whitespace differences, are in
effect, the blobs need to be compared for their contents, as if we
were generating a patch, even when the user is only interested in
"--exit-code" and not an actual differences.

There are two special case tweaks in the existing code.  One is that
the codepath to compute "--patch" output sets .found_changes bit
only when it sees a "real" change (whose definition is loosened when
"-w" is in effect to ignore whitespace-only changes), and uses that
bit, instead of whether diff_queue[] has any paths, to decide the
exit status.  The other is that when "-s" (no output) is combined
with "-w", the machinery calls the same "--patch" codepath but
redirecting the output to void, only for the .found_changes bit.

That is fine, until somebody comes and tries to combine options like
"--stat", "--name-only", "--name-status", etc. with "-w".  Because
the second special case above to run a fallback "--patch" computation
does not kick in for these other output modes, .found_changes bit is
not updated correctly.

To fix this, do two things:

 * The codepath to generate "--stat" output already calls the
   underlying xdiff machinery with appropriate options like "-w",
   but it did not update .found_changes bit.  Fixing "--stat -w"
   combined with "--exit-code" thus becomes just the matter of
   adding these missing .found_changes assignment.

 * For generating "--name-only", "--name-status", etc., the code
   does not look into the contents of the blob objects at all.  For
   now, extend the special case used for "-s -w --exit-code" to run
   a silent "--patch" computation to set the .found_changes bit
   correctly.

Not that the latter may still not be correct in that a path whose
contents have no differences other than whitespace changes would
still show up in the "diff -w --name-only --exit-code" output, even
though the exit status may say there is no differences.  Arguably
this is better than status quo, even though it still is wrong.

Reported-by: Paul Watson <pwatson2@wellmed.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Junio C Hamano <gitster@pobox.com> writes:

> Also, this is not limited to the no-index mode.
>
>     $ echo one >1
>     $ git add 1
>     $ echo two >1
>     $ git diff --exit-code --numstat 1; echo "<<$?>>"
>     1	1	1
>     <<1>>
>     $ git diff --exit-code --numstat -w 1; echo "<<$?>>"
>     1	1	1
>     <<0>>
>
> So the minimum reproduction seems to be
>
>   * the diff machinery is asked to do --exit-code (no-index
>     implicitly does it)
>   * -w is used
>   * -p is *not* used
>   * to compare two different files.
>
> Thanks for a bug report.
>
> Patches welcome ;-)

So I ended up looking into this myself X-<.

 diff.c                     |  22 +++++++++-
 t/t4015-diff-whitespace.sh | 100 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 119 insertions(+), 3 deletions(-)

diff --git c/diff.c w/diff.c
index ee3eb629e3..38b57b589f 100644
--- c/diff.c
+++ w/diff.c
@@ -3795,6 +3795,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 		} else {
 			data->added = diff_filespec_size(o->repo, two);
 			data->deleted = diff_filespec_size(o->repo, one);
+			o->found_changes = 1;
 		}
 	}
 
@@ -3803,6 +3804,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 		diff_populate_filespec(o->repo, two, NULL);
 		data->deleted = count_lines(one->data, one->size);
 		data->added = count_lines(two->data, two->size);
+		o->found_changes = 1;
 	}
 
 	else if (may_differ) {
@@ -3828,6 +3830,11 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 				  diffstat_consume, diffstat, &xpp, &xecfg))
 			die("unable to generate diffstat for %s", one->path);
 
+		/* Do this before cancelling the no-op diffstat below */
+		if (diffstat->files[diffstat->nr - 1]->added ||
+		    diffstat->files[diffstat->nr - 1]->deleted)
+			o->found_changes = 1;
+
 		if (DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two)) {
 			struct diffstat_file *file =
 				diffstat->files[diffstat->nr - 1];
@@ -4832,6 +4839,11 @@ void diff_setup_done(struct diff_options *options)
 	else
 		options->prefix_length = 0;
 
+	/*
+	 * This is how "diff --name-status -p --stat --raw" becomes
+	 * equivalent to "diff --name-status", which may be
+	 * unintuitive.
+	 */
 	if (options->output_format & (DIFF_FORMAT_NAME |
 				      DIFF_FORMAT_NAME_STATUS |
 				      DIFF_FORMAT_CHECKDIFF |
@@ -6684,13 +6696,19 @@ void diff_flush(struct diff_options *options)
 		separator++;
 	}
 
-	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
+	if (((output_format & DIFF_FORMAT_NO_OUTPUT) ||
+	     /* these compute .found_changes properly */
+	     !(output_format & (DIFF_FORMAT_DIFFSTAT|
+				DIFF_FORMAT_SHORTSTAT|
+				DIFF_FORMAT_NUMSTAT|
+				DIFF_FORMAT_DIRSTAT|
+				DIFF_FORMAT_PATCH))) &&
 	    options->flags.exit_with_status &&
 	    options->flags.diff_from_contents) {
 		/*
 		 * run diff_flush_patch for the exit status. setting
 		 * options->file to /dev/null should be safe, because we
-		 * aren't supposed to produce any output anyway.
+		 * aren't supposed to produce any output from here.
 		 */
 		diff_free_file(options);
 		options->file = xfopen("/dev/null", "w");
diff --git c/t/t4015-diff-whitespace.sh w/t/t4015-diff-whitespace.sh
index b298f220e0..1b944d77e4 100755
--- c/t/t4015-diff-whitespace.sh
+++ w/t/t4015-diff-whitespace.sh
@@ -1,7 +1,7 @@
 #!/bin/sh
 #
 # Copyright (c) 2006 Johannes E. Schindelin
-#
+# Copyright (c) 2023 Google LLC
 
 test_description='Test special whitespace in diff engine.
 
@@ -11,6 +11,104 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff.sh
 
+test_expect_success 'exit status with -w --name-only (different)' '
+	echo foo >x &&
+	git add x &&
+	echo bar >x &&
+	test_expect_code 1 git diff -w --name-only --exit-code x
+'
+
+test_expect_success 'exit status with -w --name-only (different but equivalent)' '
+	echo foo >x &&
+	git add x &&
+	echo " foo" >x &&
+	git diff -w --name-only --exit-code x
+'
+
+test_expect_success 'exit status with -w --raw (different)' '
+	echo foo >x &&
+	git add x &&
+	echo bar >x &&
+	test_expect_code 1 git diff -w --raw --exit-code x
+'
+
+test_expect_success 'exit status with -w --raw (different but equivalent)' '
+	echo foo >x &&
+	git add x &&
+	echo " foo" >x &&
+	git diff -w --raw --exit-code x
+'
+
+test_expect_success 'exit status with -w --stat (different)' '
+	echo foo >x &&
+	git add x &&
+	echo bar >x &&
+	test_expect_code 1 git diff -w --stat --exit-code x
+'
+
+test_expect_success 'exit status with -w --stat (different but equivalent)' '
+	echo foo >x &&
+	git add x &&
+	echo " foo" >x &&
+	git diff -w --stat --exit-code x
+'
+
+test_expect_success 'exit status with -w --shortstat (different)' '
+	echo foo >x &&
+	git add x &&
+	echo bar >x &&
+	test_expect_code 1 git diff -w --shortstat --exit-code x
+'
+
+test_expect_success 'exit status with -w --shortstat (different but equivalent)' '
+	echo foo >x &&
+	git add x &&
+	echo " foo" >x &&
+	git diff -w --shortstat --exit-code x
+'
+
+test_expect_success 'exit status with -w --quiet (different)' '
+	echo foo >x &&
+	git add x &&
+	echo bar >x &&
+	test_expect_code 1 git diff -w --quiet --exit-code x
+'
+
+test_expect_success 'exit status with -w --quiet (different but equivalent)' '
+	echo foo >x &&
+	git add x &&
+	echo " foo" >x &&
+	git diff -w --quiet --exit-code x
+'
+
+test_expect_success 'exit status with -w --summary (different)' '
+	echo foo >x &&
+	git add x &&
+	echo bar >x &&
+	test_expect_code 1 git diff -w --summary --exit-code x
+'
+
+test_expect_success 'exit status with -w --summary (different but equivalent)' '
+	echo foo >x &&
+	git add x &&
+	echo " foo" >x &&
+	git diff -w --summary --exit-code x
+'
+
+test_expect_success 'exit status with -w -s (different)' '
+	echo foo >x &&
+	git add x &&
+	echo bar >x &&
+	test_expect_code 1 git diff -w -s --exit-code x
+'
+
+test_expect_success 'exit status with -w -s (different but equivalent)' '
+	echo foo >x &&
+	git add x &&
+	echo " foo" >x &&
+	git diff -w -s --exit-code x
+'
+
 test_expect_success "Ray Lehtiniemi's example" '
 	cat <<-\EOF >x &&
 	do {

Re: [PATCH] diff: tighten interaction between -w and --exit-code

[Jeff King]

On Wed, Aug 16, 2023 at 04:45:23PM -0700, Junio C Hamano wrote:

> To fix this, do two things:
> 
>  * The codepath to generate "--stat" output already calls the
>    underlying xdiff machinery with appropriate options like "-w",
>    but it did not update .found_changes bit.  Fixing "--stat -w"
>    combined with "--exit-code" thus becomes just the matter of
>    adding these missing .found_changes assignment.
> 
>  * For generating "--name-only", "--name-status", etc., the code
>    does not look into the contents of the blob objects at all.  For
>    now, extend the special case used for "-s -w --exit-code" to run
>    a silent "--patch" computation to set the .found_changes bit
>    correctly.

Nicely explained overall, but one hunk of the patch left me wondering...

> @@ -3828,6 +3830,11 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
>  				  diffstat_consume, diffstat, &xpp, &xecfg))
>  			die("unable to generate diffstat for %s", one->path);
>  
> +		/* Do this before cancelling the no-op diffstat below */
> +		if (diffstat->files[diffstat->nr - 1]->added ||
> +		    diffstat->files[diffstat->nr - 1]->deleted)
> +			o->found_changes = 1;
> +

So this is checking whether any lines were added/deleted to see if the
stat was a noop. But what about non-content bits, like mode changes?
E.g., the tests below all fail (but would pass without "-w"):

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 1b944d77e4..54e56ad911 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -25,6 +25,14 @@ test_expect_success 'exit status with -w --name-only (different but equivalent)'
 	git diff -w --name-only --exit-code x
 '
 
+test_expect_success 'exit status with -w --name-only (mode change)' '
+	test_when_finished "git reset" &&
+	echo foo >x &&
+	git add x &&
+	git update-index --chmod=+x x &&
+	test_expect_code 1 git diff -w --name-only --exit-code
+'
+
 test_expect_success 'exit status with -w --raw (different)' '
 	echo foo >x &&
 	git add x &&
@@ -39,6 +47,14 @@ test_expect_success 'exit status with -w --raw (different but equivalent)' '
 	git diff -w --raw --exit-code x
 '
 
+test_expect_success 'exit status with -w --raw (mode change)' '
+	test_when_finished "git reset" &&
+	echo foo >x &&
+	git add x &&
+	git update-index --chmod=+x x &&
+	test_expect_code 1 git diff -w --raw --exit-code
+'
+
 test_expect_success 'exit status with -w --stat (different)' '
 	echo foo >x &&
 	git add x &&
@@ -53,6 +69,14 @@ test_expect_success 'exit status with -w --stat (different but equivalent)' '
 	git diff -w --stat --exit-code x
 '
 
+test_expect_success 'exit status with -w --stat (mode change)' '
+	test_when_finished "git reset" &&
+	echo foo >x &&
+	git add x &&
+	git update-index --chmod=+x x &&
+	test_expect_code 1 git diff -w --stat --exit-code
+'
+
 test_expect_success 'exit status with -w --shortstat (different)' '
 	echo foo >x &&
 	git add x &&
@@ -67,6 +91,14 @@ test_expect_success 'exit status with -w --shortstat (different but equivalent)'
 	git diff -w --shortstat --exit-code x
 '
 
+test_expect_success 'exit status with -w --shortstat (mode change)' '
+	test_when_finished "git reset" &&
+	echo foo >x &&
+	git add x &&
+	git update-index --chmod=+x x &&
+	test_expect_code 1 git diff -w --shortstat --exit-code
+'
+
 test_expect_success 'exit status with -w --quiet (different)' '
 	echo foo >x &&
 	git add x &&
@@ -81,6 +113,14 @@ test_expect_success 'exit status with -w --quiet (different but equivalent)' '
 	git diff -w --quiet --exit-code x
 '
 
+test_expect_success 'exit status with -w --quiet (mode change)' '
+	test_when_finished "git reset" &&
+	echo foo >x &&
+	git add x &&
+	git update-index --chmod=+x x &&
+	test_expect_code 1 git diff -w --quiet --exit-code
+'
+
 test_expect_success 'exit status with -w --summary (different)' '
 	echo foo >x &&
 	git add x &&
@@ -95,6 +135,14 @@ test_expect_success 'exit status with -w --summary (different but equivalent)' '
 	git diff -w --summary --exit-code x
 '
 
+test_expect_success 'exit status with -w --summary (mode change)' '
+	test_when_finished "git reset" &&
+	echo foo >x &&
+	git add x &&
+	git update-index --chmod=+x x &&
+	test_expect_code 1 git diff -w --summary --exit-code
+'
+
 test_expect_success 'exit status with -w -s (different)' '
 	echo foo >x &&
 	git add x &&
@@ -109,6 +157,14 @@ test_expect_success 'exit status with -w -s (different but equivalent)' '
 	git diff -w -s --exit-code x
 '
 
+test_expect_success 'exit status with -w -s (mode change)' '
+	test_when_finished "git reset" &&
+	echo foo >x &&
+	git add x &&
+	git update-index --chmod=+x x &&
+	test_expect_code 1 git diff -w -s --exit-code x
+'
+
 test_expect_success "Ray Lehtiniemi's example" '
 	cat <<-\EOF >x &&
 	do {

For the diffstat case, I think we could check the mode here, but there
are other cases (e.g., adding or deleting an empty file). The code right
below the hunk I quoted seems to try to deal with that (the "cancelling
the no-op" your comment mentions). I'm not sure if we want something
like this:

diff --git a/diff.c b/diff.c
index 38b57b589f..1dbfdaeff0 100644
--- a/diff.c
+++ b/diff.c
@@ -3853,6 +3853,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 				&& one->mode == two->mode) {
 				free_diffstat_file(file);
 				diffstat->nr--;
+			} else {
+				o->found_changes = 1;
 			}
 		}
 	}

but I haven't dug too far (and of course all of the other options also
need something similar to catch this case).

-Peff

Re: [PATCH] diff: tighten interaction between -w and --exit-code

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> For the diffstat case, I think we could check the mode here, but there
> are other cases (e.g., adding or deleting an empty file). The code right
> below the hunk I quoted seems to try to deal with that (the "cancelling
> the no-op" your comment mentions). I'm not sure if we want something
> like this:
>
> diff --git a/diff.c b/diff.c
> index 38b57b589f..1dbfdaeff0 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3853,6 +3853,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
>  				&& one->mode == two->mode) {
>  				free_diffstat_file(file);
>  				diffstat->nr--;
> +			} else {
> +				o->found_changes = 1;
>  			}
>  		}
>  	}

That is much better.  In all cases where the above diffstat->nr-- is
not reached and diffstat is kept is where we found changes, so an
even simpler solution that fundamentally cannot go wrong would be to
see "diffstat->nr" at the end (i.e. "are we going to show diffstat
for *any* filepair?").  If it is non-zero, we did find a difference.

Then we do not have to wonder if that else clause is in the right
place, or we have to do something similar to the above for cases
where DIFF_FILE_VALID() is not true for both sides (i.e. creation or
deletion).

Thanks.

Re: [PATCH] diff: tighten interaction between -w and --exit-code

[Jeff King]

On Thu, Aug 17, 2023 at 09:12:09AM -0700, Junio C Hamano wrote:

> > diff --git a/diff.c b/diff.c
> > index 38b57b589f..1dbfdaeff0 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -3853,6 +3853,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
> >  				&& one->mode == two->mode) {
> >  				free_diffstat_file(file);
> >  				diffstat->nr--;
> > +			} else {
> > +				o->found_changes = 1;
> >  			}
> >  		}
> >  	}
> 
> That is much better.  In all cases where the above diffstat->nr-- is
> not reached and diffstat is kept is where we found changes, so an
> even simpler solution that fundamentally cannot go wrong would be to
> see "diffstat->nr" at the end (i.e. "are we going to show diffstat
> for *any* filepair?").  If it is non-zero, we did find a difference.

Yeah, without having really dug into the problem too far, that does
sound a lot better. I also wonder to what degree you could apply the
same strategy to other formats (I guess it depends on them removing
whitespace-only changes from a structure). From the test I posted
earlier, it does look like many of them have the same blind spots for
mode-only changes (and I suspect addition/removal of empty files is
another corner case to check).

-Peff

Re: [PATCH] diff: tighten interaction between -w and --exit-code

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, without having really dug into the problem too far, that does
> sound a lot better. I also wonder to what degree you could apply the
> same strategy to other formats (I guess it depends on them removing
> whitespace-only changes from a structure). From the test I posted
> earlier, it does look like many of them have the same blind spots for
> mode-only changes (and I suspect addition/removal of empty files is
> another corner case to check).

I have something cooking.  Stay tuned, without getting excited too
much ;-)

Thanks.

[PATCH v2 0/5] fix interactions with "-w" and "--exit-code"

[Junio C Hamano]

So here is the reworked series.  The previous attempt mostly
correspond to [4/5] of this series, but the approach has been quite
updated, so here is marked as "v2" but I omitted the interdiff as it
would be more or less useless.

The first patch is not part of the main theme, but because I'll be
adding the first use of "--dirstat" to t4015 that has been leak
sanitizer clean, and the "--dirstat" codepath has known leaks, I am
plugging the leaks as a preliminary step to avoid having to mark
t4015 as leak sanitizer unclean again.

The second patch flips around order of processing without changing
the meaning.  After this change, blocks that compute output that
also take contents into account are grouped together, before the
fallback code for output formats that do not look at the contents to
compute their result.  It is purely done as a clean-up.

The earlier series claimed that "--exit-code -w" is reliable as long
as "--patch" output is used, but it turns out that there are corner
case holes in the "--patch" codepath.  The third patch fixes them.

The fourth patch teachs "--stat" codepath to help "--exit-code -w";
as it looks at contents to produce its output, we note the fact that
we found (or did not find) differences, and use that for the exit
code computation.

The fifth patch deals with other output modes that do not look at
contents for their output to reuse the fallback code.

Junio C Hamano (5):
  diff: --dirstat leakfix
  diff: move the fallback "--exit-code" code down
  diff: mode-only change should be noticed by "--patch -w --exit-code"
  diff: teach "--stat -w --exit-code" to notice differences
  diff: teach "--name-status" and friends to honor "--exit-code -w"

 diff.c                     | 71 +++++++++++++++++++++++++++-----------
 t/t4015-diff-whitespace.sh | 36 ++++++++++++++++++-
 t/t4047-diff-dirstat.sh    |  2 ++
 3 files changed, 87 insertions(+), 22 deletions(-)

-- 
2.42.0-rc2

[PATCH v2 1/5] diff: --dirstat leakfix

[Junio C Hamano]

The dirstat_dir structure holds a list of files that had "damages"
and is used to summarize the change by directory.  It was allocated,
used, and then left behind, leaking.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c                  | 14 ++++++++++++--
 t/t4047-diff-dirstat.sh |  2 ++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 648f6717a5..03d0cfc700 100644
--- a/diff.c
+++ b/diff.c
@@ -2977,6 +2977,7 @@ static void show_dirstat(struct diff_options *options)
 	unsigned long changed;
 	struct dirstat_dir dir;
 	struct diff_queue_struct *q = &diff_queued_diff;
+	struct dirstat_file *to_free;
 
 	dir.files = NULL;
 	dir.alloc = 0;
@@ -3060,13 +3061,17 @@ static void show_dirstat(struct diff_options *options)
 		dir.nr++;
 	}
 
+	to_free = dir.files;
+
 	/* This can happen even with many files, if everything was renames */
 	if (!changed)
-		return;
+		goto free_return;
 
 	/* Show all directories with more than x% of the changes */
 	QSORT(dir.files, dir.nr, dirstat_compare);
 	gather_dirstat(options, &dir, changed, "", 0);
+free_return:
+	free(to_free);
 }
 
 static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *options)
@@ -3074,6 +3079,7 @@ static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *o
 	int i;
 	unsigned long changed;
 	struct dirstat_dir dir;
+	struct dirstat_file *to_free;
 
 	if (data->nr == 0)
 		return;
@@ -3104,13 +3110,17 @@ static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *o
 		dir.nr++;
 	}
 
+	to_free = dir.files;
+
 	/* This can happen even with many files, if everything was renames */
 	if (!changed)
-		return;
+		goto free_return;
 
 	/* Show all directories with more than x% of the changes */
 	QSORT(dir.files, dir.nr, dirstat_compare);
 	gather_dirstat(options, &dir, changed, "", 0);
+free_return:
+	free(to_free);
 }
 
 static void free_diffstat_file(struct diffstat_file *f)
diff --git a/t/t4047-diff-dirstat.sh b/t/t4047-diff-dirstat.sh
index 7fec2cb9cd..70224c3da1 100755
--- a/t/t4047-diff-dirstat.sh
+++ b/t/t4047-diff-dirstat.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='diff --dirstat tests'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # set up two commits where the second commit has these files
-- 
2.42.0-rc2

[PATCH v2 2/5] diff: move the fallback "--exit-code" code down

[Junio C Hamano]

When "--exit-code" is asked and the code cannot just answer by
comparing the object names on both sides but need to inspect and
compare the contents, there are two ways that the result is found
out.

Some output modes, like "--stat" and "--patch", inherently have to
inspect the contents in order to _show_ the differences in the way
they do.  The codepaths for these modes set the .found_changes bit
as they compute what to show.

However, other output modes do not need to inspect the contents to
show the differences in the way they do.  The most notable example
is "--quiet", which does not need to compute any output.  When they
are asked to report "--exit-code", they run the codepaths for the
"--patch" output with their output redirected to "/dev/null", only
to set the .found_changes bit.

Currently, this fallback invocation of "--patch" output is done
after the "--stat" output format and its friends and before the
"--patch" and internal callback logic.  Move it to the end of
the sequence to clarify the fallback status of this code block.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/diff.c b/diff.c
index 03d0cfc700..ce9c8272c7 100644
--- a/diff.c
+++ b/diff.c
@@ -6555,6 +6555,21 @@ void diff_flush(struct diff_options *options)
 		separator++;
 	}
 
+	if (output_format & DIFF_FORMAT_PATCH) {
+		if (separator) {
+			emit_diff_symbol(options, DIFF_SYMBOL_SEPARATOR, NULL, 0, 0);
+			if (options->stat_sep)
+				/* attach patch instead of inline */
+				emit_diff_symbol(options, DIFF_SYMBOL_STAT_SEP,
+						 NULL, 0, 0);
+		}
+
+		diff_flush_patch_all_file_pairs(options);
+	}
+
+	if (output_format & DIFF_FORMAT_CALLBACK)
+		options->format_callback(q, options, options->format_callback_data);
+
 	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
 	    options->flags.exit_with_status &&
 	    options->flags.diff_from_contents) {
@@ -6576,21 +6591,6 @@ void diff_flush(struct diff_options *options)
 		}
 	}
 
-	if (output_format & DIFF_FORMAT_PATCH) {
-		if (separator) {
-			emit_diff_symbol(options, DIFF_SYMBOL_SEPARATOR, NULL, 0, 0);
-			if (options->stat_sep)
-				/* attach patch instead of inline */
-				emit_diff_symbol(options, DIFF_SYMBOL_STAT_SEP,
-						 NULL, 0, 0);
-		}
-
-		diff_flush_patch_all_file_pairs(options);
-	}
-
-	if (output_format & DIFF_FORMAT_CALLBACK)
-		options->format_callback(q, options, options->format_callback_data);
-
 	for (i = 0; i < q->nr; i++)
 		diff_free_filepair(q->queue[i]);
 free_queue:
-- 
2.42.0-rc2

[PATCH v2 3/5] diff: mode-only change should be noticed by "--patch -w --exit-code"

[Junio C Hamano]

The codepath to notice the content-level changes, taking certaion
no-op changes like "ignore whitespace" into account, forgot that
a mode-only change is still a change.  This resulted in

    $ git diff --patch --exit-code -w

to exit with status 0 even when there is such a mode-only change,
breaking both "--patch" and "--quiet" output formats.

Teach the builtin_diff() codepath that creation and deletion as well
as mode changes are all interesting changes.

Note that the test specifically checks removal of an empty file,
because if there is anything in the preimage (i.e. the removed file
is not empty), the removal would still trigger textual patch output
and the codepath for that does update .found_changes bit to report
that it found an interesting change.  We need to make sure that the
.found_changes bit is set even without triggering textual patch
output.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c                     |  3 +++
 t/t4015-diff-whitespace.sh | 35 ++++++++++++++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index ce9c8272c7..de18364902 100644
--- a/diff.c
+++ b/diff.c
@@ -3501,18 +3501,21 @@ static void builtin_diff(const char *name_a,
 		strbuf_addf(&header, "%s%snew file mode %06o%s\n", line_prefix, meta, two->mode, reset);
 		if (xfrm_msg)
 			strbuf_addstr(&header, xfrm_msg);
+		o->found_changes = 1;
 		must_show_header = 1;
 	}
 	else if (lbl[1][0] == '/') {
 		strbuf_addf(&header, "%s%sdeleted file mode %06o%s\n", line_prefix, meta, one->mode, reset);
 		if (xfrm_msg)
 			strbuf_addstr(&header, xfrm_msg);
+		o->found_changes = 1;
 		must_show_header = 1;
 	}
 	else {
 		if (one->mode != two->mode) {
 			strbuf_addf(&header, "%s%sold mode %06o%s\n", line_prefix, meta, one->mode, reset);
 			strbuf_addf(&header, "%s%snew mode %06o%s\n", line_prefix, meta, two->mode, reset);
+			o->found_changes = 1;
 			must_show_header = 1;
 		}
 		if (xfrm_msg)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index f3e20dd5bb..943ad252d4 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1,7 +1,7 @@
 #!/bin/sh
 #
 # Copyright (c) 2006 Johannes E. Schindelin
-#
+# Copyright (c) 2023 Google LLC
 
 test_description='Test special whitespace in diff engine.
 
@@ -11,6 +11,39 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff.sh
 
+for opts in --patch --quiet -s
+do
+
+	test_expect_success "status with $opts (different)" '
+		echo foo >x &&
+		git add x &&
+		echo bar >x &&
+		test_expect_code 1 git diff -w $opts --exit-code x
+	'
+
+	test_expect_success "status with $opts (mode differs)" '
+		test_when_finished "git update-index --chmod=-x x" &&
+		echo foo >x &&
+		git add x &&
+		git update-index --chmod=+x x &&
+		test_expect_code 1 git diff -w $opts --exit-code x
+	'
+
+	test_expect_success "status with $opts (removing an empty file)" '
+		: >x &&
+		git add x &&
+		rm x &&
+		test_expect_code 1 git diff -w $opts --exit-code -- x
+	'
+
+	test_expect_success "status with $opts (different but equivalent)" '
+		echo foo >x &&
+		git add x &&
+		echo " foo" >x &&
+		git diff -w $opts --exit-code x
+	'
+done
+
 test_expect_success "Ray Lehtiniemi's example" '
 	cat <<-\EOF >x &&
 	do {
-- 
2.42.0-rc2

Re: [PATCH v2 3/5] diff: mode-only change should be noticed by "--patch -w --exit-code"

[Junio C Hamano]

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

> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
> index f3e20dd5bb..943ad252d4 100755
> --- a/t/t4015-diff-whitespace.sh
> +++ b/t/t4015-diff-whitespace.sh
> @@ -11,6 +11,39 @@ TEST_PASSES_SANITIZE_LEAK=true
> + ...
> +	test_expect_success "status with $opts (mode differs)" '
> +		test_when_finished "git update-index --chmod=-x x" &&
> +		echo foo >x &&
> +		git add x &&
> +		git update-index --chmod=+x x &&
> +		test_expect_code 1 git diff -w $opts --exit-code x
> +	'

Apparently this one needs to skipped on a filesystem without
support for the executable bit.

cf. https://github.com/git/git/actions/runs/5897310248/job/15996914969#step:5:218

I'll give POSIXPERM prerequisite to this test piece.

[PATCH v2 4/5] diff: teach "--stat -w --exit-code" to notice differences

[Junio C Hamano]

When options like "-w" is used while "--exit-code" option is in
effect, instead of the usual "do we have any filepair whose preimage
and postimage have different <mode,object>?" check, we need to compare
the contents of the blobs, taking into account that certain changes
are considered no-op.

With the previous step, we taught "--patch" codepath to set the
.found_changes bit correctly, even for a change that only affects
the mode and not object.  The "--stat" codepath, however, did not
set the .found_changes bit at all.  This lead to

    $ git diff --stat -w --exit-code

for a change that does have an outout to exit with status 0.

Set the bit by inspecting the list of paths the diffstat output is
given for (a mode-only change will still appear as a "0-line added
0-line deleted" change) to fix it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c                     | 1 +
 t/t4015-diff-whitespace.sh | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index de18364902..7213237675 100644
--- a/diff.c
+++ b/diff.c
@@ -6905,6 +6905,7 @@ void compute_diffstat(struct diff_options *options,
 		if (check_pair_status(p))
 			diff_flush_stat(p, options, diffstat);
 	}
+	options->found_changes = !!diffstat->nr;
 }
 
 void diff_addremove(struct diff_options *options,
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 943ad252d4..355d96aa14 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -11,7 +11,7 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff.sh
 
-for opts in --patch --quiet -s
+for opts in --patch --quiet -s --stat --shortstat --dirstat=lines
 do
 
 	test_expect_success "status with $opts (different)" '
-- 
2.42.0-rc2

[PATCH v2 5/5] diff: teach "--name-status" and friends to honor "--exit-code -w"

[Junio C Hamano]

We have fallback code that is used when "-s -w --exit-code" options
are used together to compute the exit code, because the exit code
must take the whitespace-ignoring comparison into account over the
contents, but with "-s", the normal codepath does not even have to
look at the contents at all.  The fallback code simply runs a "git
diff --patch" while sending the output to "/dev/null".

The codepaths for some other output modes, like "--name-status" and
"--raw", share the same trait as "-s" in that they do not look at
the contents for their output generation.  Extend the fallback code
to cover these output modes as well.

Note that they may still not be correct in that a path whose
contents have no differences other than whitespace changes would
still show up in the "diff -w --name-only --exit-code" output, even
though the exit status may say there is no differences.  Arguably
this is better than status quo, even though it still may be wrong.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c                     | 23 +++++++++++++++++++----
 t/t4015-diff-whitespace.sh |  3 ++-
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/diff.c b/diff.c
index 7213237675..3bb9d11bfe 100644
--- a/diff.c
+++ b/diff.c
@@ -6573,13 +6573,28 @@ void diff_flush(struct diff_options *options)
 	if (output_format & DIFF_FORMAT_CALLBACK)
 		options->format_callback(q, options, options->format_callback_data);
 
-	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
+	if (((output_format & DIFF_FORMAT_NO_OUTPUT) ||
+	     /* these compute .found_changes properly */
+	     !(output_format & (DIFF_FORMAT_DIFFSTAT|
+				DIFF_FORMAT_SHORTSTAT|
+				DIFF_FORMAT_NUMSTAT|
+				DIFF_FORMAT_DIRSTAT|
+				DIFF_FORMAT_PATCH))) &&
 	    options->flags.exit_with_status &&
 	    options->flags.diff_from_contents) {
 		/*
-		 * run diff_flush_patch for the exit status. setting
-		 * options->file to /dev/null should be safe, because we
-		 * aren't supposed to produce any output anyway.
+		 * We need to inspect the contents, not just object
+		 * names, to determine the exit status, but the usual
+		 * processing for the output format specified does not
+		 * have to work with and does not look at the
+		 * contents.  Run an extra and silent "diff --patch"
+		 * but discard the output to /dev/null, so that we
+		 * would set the .found_changes bit correctly.
+		 *
+		 * We can safely close and discard the original output
+		 * file here, since all that is left to do from this
+		 * point is to return (we don't do the FORMAT_PATCH
+		 * thing below).
 		 */
 		diff_free_file(options);
 		options->file = xfopen("/dev/null", "w");
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 355d96aa14..412d20181c 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -11,7 +11,8 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff.sh
 
-for opts in --patch --quiet -s --stat --shortstat --dirstat=lines
+for opts in --patch --quiet -s --stat --shortstat --dirstat=lines \
+	    --name-only --raw --name-status --summary
 do
 
 	test_expect_success "status with $opts (different)" '
-- 
2.42.0-rc2

Re: [PATCH v2 0/5] fix interactions with "-w" and "--exit-code"

[Junio C Hamano]

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

> The first patch is not part of the main theme, but because I'll be
> adding the first use of "--dirstat" to t4015 that has been leak
> sanitizer clean, and the "--dirstat" codepath has known leaks, I am
> plugging the leaks as a preliminary step to avoid having to mark
> t4015 as leak sanitizer unclean again.

I completely forgot that I already have done 1e1dcb2a (Merge branch
'jc/dirstat-plug-leaks', 2023-05-15) in Git 2.41 timeframe.  I was
basing my work on Git 2.38 maintenance track and needed [1/5] but in
newer codebase it is not needed.

[PATCH v3 0/5] fix interactions with "-w" and "--exit-code"

[Junio C Hamano]

Paul Watson reported that "diff --no-index --exit-code
--ignore-all-space" does not work when used with "--shortstat".  It
turns out that this is not limited to "--no-index" mode, and it is
not limited to "--shortstat".  Anything that does not use the "--patch"
machinery to discover the content level differences ignored --exit-code
when used with options like "-w" and always exited with 0.  In fact,
even the "--patch" machinery was slightly faulty in corner cases.

And here is another round to fix it.
Previous one is at https://lore.kernel.org/git/20230817222949.3835424-1-gitster@pobox.com/

The interdiff is not all that interesting.  One patch dropped, two
patches stay the same, one patch has one-line change, and the final
one patch has been completely reworked.  Here is the summary:

 * The first patch about dirstat leak-fix is now gone.  The series
   has instead been rebased on top of the official "dirstat leak-fix"
   series that was merged in Git 2.41.

 * The next patch (preliminary code clean-up) stays the same.  It is
   now [1/5] instead of being the second step.

 * The next patch is to fix the corner case bug of "--patch"
   machinery.  The code stays the same, but the tests were asking
   the filesystem to do something impossible when they do not have
   executable bit support, so a prerequisite has been added to work
   around them.

 * The next patch is to fix "--stat -w --exit-code", which stays
   the same.

 * The last step is completely new.  v2 took an approach to reuse
   the "--patch" machinery even for "--raw" and other modes, but it
   would mean that "diff --raw --exit-code" may exit with 0 even
   when it has different paths to report, which is confusing.  v3
   changes the approach to align the exit status with the presence
   of reported paths that are different better.  "--raw" has ignored
   "-w" when producing its output.  It should ignore "-w" when
   reporting differences with its exit code, instead of always
   exiting with 0.

Junio C Hamano (5):
  diff: move the fallback "--exit-code" code down
  diff: mode-only change should be noticed by "--patch -w --exit-code"
  diff: teach "--stat -w --exit-code" to notice differences
  t4040: remove test that succeeded for a wrong reason
  diff: the -w option breaks --exit-code for --raw and other output
    modes

 diff.c                       | 40 ++++++++++++++++++++++--------------
 t/t4015-diff-whitespace.sh   | 39 ++++++++++++++++++++++++++++++++++-
 t/t4040-whitespace-status.sh |  3 +--
 3 files changed, 64 insertions(+), 18 deletions(-)

Range-diff against v2:
1:  65ff4655a2 < -:  ---------- diff: --dirstat leakfix
2:  533f974c9b ! 1:  df869ac550 diff: move the fallback "--exit-code" code down
    @@ Commit message
         diff: move the fallback "--exit-code" code down
     
         When "--exit-code" is asked and the code cannot just answer by
    -    comparing the object names on both sides but need to inspect and
    +    comparing the object names on both sides but needs to inspect and
         compare the contents, there are two ways that the result is found
         out.
     
         Some output modes, like "--stat" and "--patch", inherently have to
    -    inspect the contents in order to _show_ the differences in the way
    +    inspect the contents in order to show the differences in the way
         they do.  The codepaths for these modes set the .found_changes bit
         as they compute what to show.
     
         However, other output modes do not need to inspect the contents to
         show the differences in the way they do.  The most notable example
    -    is "--quiet", which does not need to compute any output.  When they
    -    are asked to report "--exit-code", they run the codepaths for the
    -    "--patch" output with their output redirected to "/dev/null", only
    -    to set the .found_changes bit.
    +    is "--quiet", which does not need to compute any output to show.
    +    When they are asked to report "--exit-code", they run the codepaths
    +    for the "--patch" output with their output redirected to "/dev/null",
    +    only to set the .found_changes bit.
     
         Currently, this fallback invocation of "--patch" output is done
         after the "--stat" output format and its friends and before the
3:  89338b9302 ! 2:  b349ad5278 diff: mode-only change should be noticed by "--patch -w --exit-code"
    @@ Metadata
      ## Commit message ##
         diff: mode-only change should be noticed by "--patch -w --exit-code"
     
    -    The codepath to notice the content-level changes, taking certaion
    +    The codepath to notice the content-level changes, taking certain
         no-op changes like "ignore whitespace" into account, forgot that
         a mode-only change is still a change.  This resulted in
     
    @@ t/t4015-diff-whitespace.sh: TEST_PASSES_SANITIZE_LEAK=true
     +		test_expect_code 1 git diff -w $opts --exit-code x
     +	'
     +
    -+	test_expect_success "status with $opts (mode differs)" '
    ++	test_expect_success POSIXPERM "status with $opts (mode differs)" '
     +		test_when_finished "git update-index --chmod=-x x" &&
     +		echo foo >x &&
     +		git add x &&
4:  8fc63f4a04 ! 3:  be50b023a8 diff: teach "--stat -w --exit-code" to notice differences
    @@ Commit message
     
             $ git diff --stat -w --exit-code
     
    -    for a change that does have an outout to exit with status 0.
    +    for a change that does have an output to exit with status 0.
     
         Set the bit by inspecting the list of paths the diffstat output is
         given for (a mode-only change will still appear as a "0-line added
5:  200593e9e0 < -:  ---------- diff: teach "--name-status" and friends to honor "--exit-code -w"
-:  ---------- > 4:  d1a6fa7d17 t4040: remove test that succeeded for a wrong reason
-:  ---------- > 5:  573f810cce diff: the -w option breaks --exit-code for --raw and other output modes

base-commit: 83973981eb475ce90f829f8a5bd6ea99cd3bbd8e
-- 
2.42.0-rc2-7-gf9972720e9

[PATCH v3 1/5] diff: move the fallback "--exit-code" code down

[Junio C Hamano]

When "--exit-code" is asked and the code cannot just answer by
comparing the object names on both sides but needs to inspect and
compare the contents, there are two ways that the result is found
out.

Some output modes, like "--stat" and "--patch", inherently have to
inspect the contents in order to show the differences in the way
they do.  The codepaths for these modes set the .found_changes bit
as they compute what to show.

However, other output modes do not need to inspect the contents to
show the differences in the way they do.  The most notable example
is "--quiet", which does not need to compute any output to show.
When they are asked to report "--exit-code", they run the codepaths
for the "--patch" output with their output redirected to "/dev/null",
only to set the .found_changes bit.

Currently, this fallback invocation of "--patch" output is done
after the "--stat" output format and its friends and before the
"--patch" and internal callback logic.  Move it to the end of
the sequence to clarify the fallback status of this code block.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/diff.c b/diff.c
index d52db685f7..0ce678fc06 100644
--- a/diff.c
+++ b/diff.c
@@ -6551,6 +6551,21 @@ void diff_flush(struct diff_options *options)
 		separator++;
 	}
 
+	if (output_format & DIFF_FORMAT_PATCH) {
+		if (separator) {
+			emit_diff_symbol(options, DIFF_SYMBOL_SEPARATOR, NULL, 0, 0);
+			if (options->stat_sep)
+				/* attach patch instead of inline */
+				emit_diff_symbol(options, DIFF_SYMBOL_STAT_SEP,
+						 NULL, 0, 0);
+		}
+
+		diff_flush_patch_all_file_pairs(options);
+	}
+
+	if (output_format & DIFF_FORMAT_CALLBACK)
+		options->format_callback(q, options, options->format_callback_data);
+
 	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
 	    options->flags.exit_with_status &&
 	    options->flags.diff_from_contents) {
@@ -6572,21 +6587,6 @@ void diff_flush(struct diff_options *options)
 		}
 	}
 
-	if (output_format & DIFF_FORMAT_PATCH) {
-		if (separator) {
-			emit_diff_symbol(options, DIFF_SYMBOL_SEPARATOR, NULL, 0, 0);
-			if (options->stat_sep)
-				/* attach patch instead of inline */
-				emit_diff_symbol(options, DIFF_SYMBOL_STAT_SEP,
-						 NULL, 0, 0);
-		}
-
-		diff_flush_patch_all_file_pairs(options);
-	}
-
-	if (output_format & DIFF_FORMAT_CALLBACK)
-		options->format_callback(q, options, options->format_callback_data);
-
 	for (i = 0; i < q->nr; i++)
 		diff_free_filepair(q->queue[i]);
 free_queue:
-- 
2.42.0-rc2-7-gf9972720e9

Re: [PATCH v3 1/5] diff: move the fallback "--exit-code" code down

[Jeff King]

On Fri, Aug 18, 2023 at 04:59:28PM -0700, Junio C Hamano wrote:

> diff --git a/diff.c b/diff.c
> index d52db685f7..0ce678fc06 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6551,6 +6551,21 @@ void diff_flush(struct diff_options *options)
>  		separator++;
>  	}
>  
> +	if (output_format & DIFF_FORMAT_PATCH) {
> +		if (separator) {

This step makes sense, but I did a double-take when looking at the
patch, because it is moving code _up_ rather than _down_. But the
problem is that the block you moved was larger than the intervening bit,
so the diff chose to flip-flop the context and changed bits.

Obviously orthogonal to your series, but I wonder if there's a way to
convince Git to show what actually happened. I don't think this is
really a heuristic or algorithm problem. Seeing the pre- and
post-images, it can't know whether it was "move A up" or "move B down",
and the "real" diff is simply much larger in this case.

-Peff

[PATCH v3 2/5] diff: mode-only change should be noticed by "--patch -w --exit-code"

[Junio C Hamano]

The codepath to notice the content-level changes, taking certain
no-op changes like "ignore whitespace" into account, forgot that
a mode-only change is still a change.  This resulted in

    $ git diff --patch --exit-code -w

to exit with status 0 even when there is such a mode-only change,
breaking both "--patch" and "--quiet" output formats.

Teach the builtin_diff() codepath that creation and deletion as well
as mode changes are all interesting changes.

Note that the test specifically checks removal of an empty file,
because if there is anything in the preimage (i.e. the removed file
is not empty), the removal would still trigger textual patch output
and the codepath for that does update .found_changes bit to report
that it found an interesting change.  We need to make sure that the
.found_changes bit is set even without triggering textual patch
output.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c                     |  3 +++
 t/t4015-diff-whitespace.sh | 35 ++++++++++++++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 0ce678fc06..998d7ae20c 100644
--- a/diff.c
+++ b/diff.c
@@ -3497,18 +3497,21 @@ static void builtin_diff(const char *name_a,
 		strbuf_addf(&header, "%s%snew file mode %06o%s\n", line_prefix, meta, two->mode, reset);
 		if (xfrm_msg)
 			strbuf_addstr(&header, xfrm_msg);
+		o->found_changes = 1;
 		must_show_header = 1;
 	}
 	else if (lbl[1][0] == '/') {
 		strbuf_addf(&header, "%s%sdeleted file mode %06o%s\n", line_prefix, meta, one->mode, reset);
 		if (xfrm_msg)
 			strbuf_addstr(&header, xfrm_msg);
+		o->found_changes = 1;
 		must_show_header = 1;
 	}
 	else {
 		if (one->mode != two->mode) {
 			strbuf_addf(&header, "%s%sold mode %06o%s\n", line_prefix, meta, one->mode, reset);
 			strbuf_addf(&header, "%s%snew mode %06o%s\n", line_prefix, meta, two->mode, reset);
+			o->found_changes = 1;
 			must_show_header = 1;
 		}
 		if (xfrm_msg)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index f3e20dd5bb..02731dccb9 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1,7 +1,7 @@
 #!/bin/sh
 #
 # Copyright (c) 2006 Johannes E. Schindelin
-#
+# Copyright (c) 2023 Google LLC
 
 test_description='Test special whitespace in diff engine.
 
@@ -11,6 +11,39 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff.sh
 
+for opts in --patch --quiet -s
+do
+
+	test_expect_success "status with $opts (different)" '
+		echo foo >x &&
+		git add x &&
+		echo bar >x &&
+		test_expect_code 1 git diff -w $opts --exit-code x
+	'
+
+	test_expect_success POSIXPERM "status with $opts (mode differs)" '
+		test_when_finished "git update-index --chmod=-x x" &&
+		echo foo >x &&
+		git add x &&
+		git update-index --chmod=+x x &&
+		test_expect_code 1 git diff -w $opts --exit-code x
+	'
+
+	test_expect_success "status with $opts (removing an empty file)" '
+		: >x &&
+		git add x &&
+		rm x &&
+		test_expect_code 1 git diff -w $opts --exit-code -- x
+	'
+
+	test_expect_success "status with $opts (different but equivalent)" '
+		echo foo >x &&
+		git add x &&
+		echo " foo" >x &&
+		git diff -w $opts --exit-code x
+	'
+done
+
 test_expect_success "Ray Lehtiniemi's example" '
 	cat <<-\EOF >x &&
 	do {
-- 
2.42.0-rc2-7-gf9972720e9

[PATCH v3 3/5] diff: teach "--stat -w --exit-code" to notice differences

[Junio C Hamano]

When options like "-w" is used while "--exit-code" option is in
effect, instead of the usual "do we have any filepair whose preimage
and postimage have different <mode,object>?" check, we need to compare
the contents of the blobs, taking into account that certain changes
are considered no-op.

With the previous step, we taught "--patch" codepath to set the
.found_changes bit correctly, even for a change that only affects
the mode and not object.  The "--stat" codepath, however, did not
set the .found_changes bit at all.  This lead to

    $ git diff --stat -w --exit-code

for a change that does have an output to exit with status 0.

Set the bit by inspecting the list of paths the diffstat output is
given for (a mode-only change will still appear as a "0-line added
0-line deleted" change) to fix it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c                     | 1 +
 t/t4015-diff-whitespace.sh | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 998d7ae20c..da965ff688 100644
--- a/diff.c
+++ b/diff.c
@@ -6901,6 +6901,7 @@ void compute_diffstat(struct diff_options *options,
 		if (check_pair_status(p))
 			diff_flush_stat(p, options, diffstat);
 	}
+	options->found_changes = !!diffstat->nr;
 }
 
 void diff_addremove(struct diff_options *options,
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 02731dccb9..230a89b951 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -11,7 +11,7 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff.sh
 
-for opts in --patch --quiet -s
+for opts in --patch --quiet -s --stat --shortstat --dirstat=lines
 do
 
 	test_expect_success "status with $opts (different)" '
-- 
2.42.0-rc2-7-gf9972720e9

Re: [PATCH v3 3/5] diff: teach "--stat -w --exit-code" to notice differences

[Jeff King]

On Fri, Aug 18, 2023 at 04:59:30PM -0700, Junio C Hamano wrote:

> Set the bit by inspecting the list of paths the diffstat output is
> given for (a mode-only change will still appear as a "0-line added
> 0-line deleted" change) to fix it.
> [...]
> diff --git a/diff.c b/diff.c
> index 998d7ae20c..da965ff688 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6901,6 +6901,7 @@ void compute_diffstat(struct diff_options *options,
>  		if (check_pair_status(p))
>  			diff_flush_stat(p, options, diffstat);
>  	}
> +	options->found_changes = !!diffstat->nr;
>  }

Makes sense, and this is delightfully simple. I wondered if we needed to
remove any code setting found_changes via builtin_diffstat(), but it
does not seem to exist in the first place. ;)

> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
> index 02731dccb9..230a89b951 100755
> --- a/t/t4015-diff-whitespace.sh
> +++ b/t/t4015-diff-whitespace.sh
> @@ -11,7 +11,7 @@ TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  . "$TEST_DIRECTORY"/lib-diff.sh
>  
> -for opts in --patch --quiet -s
> +for opts in --patch --quiet -s --stat --shortstat --dirstat=lines
>  do

I guess this would fix --numstat, too, though there may be diminishing
returns in checking every format if we know it is just hitting the same
code paths (though perhaps one could argue that --shortstat is already
uninteresting for the same reason).

-Peff

[PATCH v3 4/5] t4040: remove test that succeeded for a wrong reason

[Junio C Hamano]

"diff-tree -b --exit-code" without "--patch" exits with 0 status,
not because it finds that the two input files are equivalent while
ignoring whitespaces, but because the implied "--raw" mode always
exits with 0 when whitespace tweaking options like "-b" and "-w"
are given, which is a long-standing bug.

We are about to fix it so that "--raw" and friends report the
differences with the exit status (even though they ignore the
whitespace tweaking options when producing their output), which
will make this test, which succeeded for a wrong reason, start
failing.  Remove it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4040-whitespace-status.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t4040-whitespace-status.sh b/t/t4040-whitespace-status.sh
index e70e020ae9..eec3d73dc2 100755
--- a/t/t4040-whitespace-status.sh
+++ b/t/t4040-whitespace-status.sh
@@ -28,8 +28,7 @@ test_expect_success 'diff-tree --exit-code' '
 
 test_expect_success 'diff-tree -b --exit-code' '
 	git diff -b --exit-code HEAD^ HEAD &&
-	git diff-tree -b -p --exit-code HEAD^ HEAD &&
-	git diff-tree -b --exit-code HEAD^ HEAD
+	git diff-tree -b -p --exit-code HEAD^ HEAD
 '
 
 test_expect_success 'diff-index --cached --exit-code' '
-- 
2.42.0-rc2-7-gf9972720e9

[PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes

[Junio C Hamano]

The output from "--raw", "--name-status", and "--name-only" modes in
"git diff" does depend on and does not reflect how certain different
contents are considered equal, unlike "--patch" and "--stat" output
modes do, when used with options like "-w" (another way of thinking
about it is that it is not like we recompute the hash of the blob
after removing all whitespaces to show "git diff --raw -w" output).

But that is not an excuse for "git diff --exit-code --raw" to fail
to report differences with its exit status, when used with options
like "-w".  Make sure the command exits with 1 when these options
report paths that are different.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c                     | 6 ++++++
 t/t4015-diff-whitespace.sh | 8 ++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index da965ff688..78f4e7518f 100644
--- a/diff.c
+++ b/diff.c
@@ -4744,6 +4744,10 @@ void diff_setup_done(struct diff_options *options)
 	else
 		options->prefix_length = 0;
 
+	/*
+	 * --name-only, --name-status, --checkdiff, and -s
+	 * turn other output format off.
+	 */
 	if (options->output_format & (DIFF_FORMAT_NAME |
 				      DIFF_FORMAT_NAME_STATUS |
 				      DIFF_FORMAT_CHECKDIFF |
@@ -6072,6 +6076,8 @@ static void flush_one_pair(struct diff_filepair *p, struct diff_options *opt)
 		fprintf(opt->file, "%s", diff_line_prefix(opt));
 		write_name_quoted(name_a, opt->file, opt->line_termination);
 	}
+
+	opt->found_changes = 1;
 }
 
 static void show_file_mode_name(struct diff_options *opt, const char *newdelete, struct diff_filespec *fs)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 230a89b951..7fcffa4b11 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -11,8 +11,12 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff.sh
 
-for opts in --patch --quiet -s --stat --shortstat --dirstat=lines
+for opt_res in --patch --quiet -s --stat --shortstat --dirstat=lines \
+	       --raw! --name-only! --name-status!
 do
+	opts=${opt_res%!} expect_failure=
+	test "$opts" = "$opt_res" ||
+		expect_failure="test_expect_code 1"
 
 	test_expect_success "status with $opts (different)" '
 		echo foo >x &&
@@ -40,7 +44,7 @@ do
 		echo foo >x &&
 		git add x &&
 		echo " foo" >x &&
-		git diff -w $opts --exit-code x
+		$expect_failure git diff -w $opts --exit-code x
 	'
 done
 
-- 
2.42.0-rc2-7-gf9972720e9

Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes

[Jeff King]

On Fri, Aug 18, 2023 at 04:59:32PM -0700, Junio C Hamano wrote:

> The output from "--raw", "--name-status", and "--name-only" modes in
> "git diff" does depend on and does not reflect how certain different
> contents are considered equal, unlike "--patch" and "--stat" output
> modes do, when used with options like "-w" (another way of thinking
> about it is that it is not like we recompute the hash of the blob
> after removing all whitespaces to show "git diff --raw -w" output).
> 
> But that is not an excuse for "git diff --exit-code --raw" to fail
> to report differences with its exit status, when used with options
> like "-w".  Make sure the command exits with 1 when these options
> report paths that are different.

I think s/with options like/without options like/ in the final
paragraph?

I like this approach much better than the earlier round. It brings the
output and exit code of "--raw", etc, into harmony. I am open to the
argument that if you ask for "--raw -w", we should start doing
content-level comparisons (since otherwise the "-w" is somewhat
useless). But even if we went that route, it should affect both the
output and the exit code. So this patch would be a prerequisite anyway.

And I say "I am open to" and not "I think it is a good idea" above,
because I suspect there are many lurking corner cases. The hash output
you mentioned is one. But also, what "--raw --patch -w" does right now
is sensible (show the raw output, but also show the whitespace-ignoring
patch), and would be impossible if "-w" affected "--raw".

> diff --git a/diff.c b/diff.c
> index da965ff688..78f4e7518f 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4744,6 +4744,10 @@ void diff_setup_done(struct diff_options *options)
>  	else
>  		options->prefix_length = 0;
>  
> +	/*
> +	 * --name-only, --name-status, --checkdiff, and -s
> +	 * turn other output format off.
> +	 */
>  	if (options->output_format & (DIFF_FORMAT_NAME |
>  				      DIFF_FORMAT_NAME_STATUS |
>  				      DIFF_FORMAT_CHECKDIFF |

OK, this hunk is just documenting the current state, not changing
anything.

> @@ -6072,6 +6076,8 @@ static void flush_one_pair(struct diff_filepair *p, struct diff_options *opt)
>  		fprintf(opt->file, "%s", diff_line_prefix(opt));
>  		write_name_quoted(name_a, opt->file, opt->line_termination);
>  	}
> +
> +	opt->found_changes = 1;
>  }

This seems deceptively simple. :) The key here is that flush_one_pair()
is called only for the formats that are otherwise broken (because they
are not looking at the content). I think this is a good spot to put the
fix, as any similar formats would hopefully get added to the conditional
in diff_flush() that puts us here.

Alternatively, we could put it in the caller, like so:

diff --git a/diff.c b/diff.c
index 78f4e7518f..e7281e75eb 100644
--- a/diff.c
+++ b/diff.c
@@ -6528,6 +6528,7 @@ void diff_flush(struct diff_options *options)
 			if (check_pair_status(p))
 				flush_one_pair(p, options);
 		}
+		options->found_changes = !!q->nr;
 		separator++;
 	}
 

which matches the diffstat technique (I almost thought we could share
the code, but for the diffstat we are counting what ends up in the
diffstat struct; it does not clean out the original diff_queue when it
sees a noop pair).

I don't see a real reason to prefer one over the other.

> -for opts in --patch --quiet -s --stat --shortstat --dirstat=lines
> +for opt_res in --patch --quiet -s --stat --shortstat --dirstat=lines \
> +	       --raw! --name-only! --name-status!
>  do
> +	opts=${opt_res%!} expect_failure=
> +	test "$opts" = "$opt_res" ||
> +		expect_failure="test_expect_code 1"

Cute "!" convention. :)

-Peff

Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes

[Jeff King]

On Mon, Aug 21, 2023 at 05:00:58PM -0400, Jeff King wrote:

> Alternatively, we could put it in the caller, like so:
> 
> diff --git a/diff.c b/diff.c
> index 78f4e7518f..e7281e75eb 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6528,6 +6528,7 @@ void diff_flush(struct diff_options *options)
>  			if (check_pair_status(p))
>  				flush_one_pair(p, options);
>  		}
> +		options->found_changes = !!q->nr;
>  		separator++;
>  	}
>  
> 
> which matches the diffstat technique (I almost thought we could share
> the code, but for the diffstat we are counting what ends up in the
> diffstat struct; it does not clean out the original diff_queue when it
> sees a noop pair).
> 
> I don't see a real reason to prefer one over the other.

Actually, on second look these are not quite the same. Yours only
triggers if check_pair_status() is true. So something like
--diff-filter should affect both output and exit code. Yours gets that
right, and mine does not. Sorry for the noise. :)

-Peff

Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes

[Jeff King]

On Mon, Aug 21, 2023 at 05:08:05PM -0400, Jeff King wrote:

> On Mon, Aug 21, 2023 at 05:00:58PM -0400, Jeff King wrote:
> 
> > Alternatively, we could put it in the caller, like so:
> > 
> > diff --git a/diff.c b/diff.c
> > index 78f4e7518f..e7281e75eb 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -6528,6 +6528,7 @@ void diff_flush(struct diff_options *options)
> >  			if (check_pair_status(p))
> >  				flush_one_pair(p, options);
> >  		}
> > +		options->found_changes = !!q->nr;
> >  		separator++;
> >  	}
> >  
> > 
> > which matches the diffstat technique (I almost thought we could share
> > the code, but for the diffstat we are counting what ends up in the
> > diffstat struct; it does not clean out the original diff_queue when it
> > sees a noop pair).
> > 
> > I don't see a real reason to prefer one over the other.
> 
> Actually, on second look these are not quite the same. Yours only
> triggers if check_pair_status() is true. So something like
> --diff-filter should affect both output and exit code. Yours gets that
> right, and mine does not. Sorry for the noise. :)

Sorry, I'm dumb again. That is not where diff-filter is handled (it is
handled by diffcore, duh). check_pair_status() is only checking for
DIFF_STATUS_UNKNOWN. I'm not sure when that would ever be set, but it
seems like we should be matching the "if it is output" logic, which is
what you get by calling flush_one_pair(). So yours is definitely
preferable, even if I don't understand the possible differences. ;)

-Peff

Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Alternatively, we could put it in the caller, like so:
>
> diff --git a/diff.c b/diff.c
> index 78f4e7518f..e7281e75eb 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6528,6 +6528,7 @@ void diff_flush(struct diff_options *options)
>  			if (check_pair_status(p))
>  				flush_one_pair(p, options);
>  		}
> +		options->found_changes = !!q->nr;
>  		separator++;
>  	}

Yup, I suspect they amount to the same thing in practice, but I
couldn't come up with a good explanation to give casual readers of
the conditional call to flush_one_pair() a few lines above why
this is correct.

Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> ...
>> about it is that it is not like we recompute the hash of the blob
>> after removing all whitespaces to show "git diff --raw -w" output).
>> 
>> But that is not an excuse for "git diff --exit-code --raw" to fail
>> to report differences with its exit status, when used with options
>> like "-w".  Make sure the command exits with 1 when these options
>> report paths that are different.
>
> I think s/with options like/without options like/ in the final
> paragraph?

Sorry, I am confused.  "diff --raw --exit-code" without "-w" reports
with its exit status that it found differences just fine.  When "-w"
is given, without this patch, it always reports 0.

What I wanted to convey was ...

    "--raw" and friends deliberately ignore "-w" and other "look at
    contents" options.

    The fact they do ignore the "look at contents" options is not a
    good excuse for "diff --raw -w --exit-code" to also ignore the
    request to report the differences with its exit status.  "diff
    --raw --exit-code" does report the differences as requested, and
    we should do the same when given "-w".

I guess the two have no logical connection so the latter sentence is
not making sense, with its "with" kept as-is or changed to "without".

Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes

[Jeff King]

On Mon, Aug 21, 2023 at 03:26:37PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> ...
> >> about it is that it is not like we recompute the hash of the blob
> >> after removing all whitespaces to show "git diff --raw -w" output).
> >> 
> >> But that is not an excuse for "git diff --exit-code --raw" to fail
> >> to report differences with its exit status, when used with options
> >> like "-w".  Make sure the command exits with 1 when these options
> >> report paths that are different.
> >
> > I think s/with options like/without options like/ in the final
> > paragraph?
> 
> Sorry, I am confused.  "diff --raw --exit-code" without "-w" reports
> with its exit status that it found differences just fine.  When "-w"
> is given, without this patch, it always reports 0.

I think I just got confused by the multiple negatives "not an excuse to
fail to...".

> What I wanted to convey was ...
> 
>     "--raw" and friends deliberately ignore "-w" and other "look at
>     contents" options.
> 
>     The fact they do ignore the "look at contents" options is not a
>     good excuse for "diff --raw -w --exit-code" to also ignore the
>     request to report the differences with its exit status.  "diff
>     --raw --exit-code" does report the differences as requested, and
>     we should do the same when given "-w".

Yes, that conveys it more clearly (to me, anyway).

-Peff

Re: [PATCH v3 0/5] fix interactions with "-w" and "--exit-code"

[Jeff King]

On Fri, Aug 18, 2023 at 04:59:27PM -0700, Junio C Hamano wrote:

> Paul Watson reported that "diff --no-index --exit-code
> --ignore-all-space" does not work when used with "--shortstat".  It
> turns out that this is not limited to "--no-index" mode, and it is
> not limited to "--shortstat".  Anything that does not use the "--patch"
> machinery to discover the content level differences ignored --exit-code
> when used with options like "-w" and always exited with 0.  In fact,
> even the "--patch" machinery was slightly faulty in corner cases.
> 
> And here is another round to fix it.
> Previous one is at https://lore.kernel.org/git/20230817222949.3835424-1-gitster@pobox.com/

This looks good to me. I left some musings, but I don't think there is
anything that merits a re-roll (you might want to fix-up a commit
message typo locally).

Thanks for running with it. It's much nicer than v1. :)

-Peff