{"thread":{"id":"60061","subject":"git bug report","startedAt":"2023-08-04T16:47:40Z","lastAt":"2023-08-22T01:30:25Z","messageCount":36,"participants":["Paul Watson","rsbecker@nexbridge.com","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"480155","messageId":"MW2PR12MB25558D1886C4AA2A95A5FC618709A@MW2PR12MB2555.namprd12.prod.outlook.com","threadId":"60061","inReplyTo":null,"subject":"git bug report","fromName":"Paul Watson","fromEmail":"pwatson2@wellmed.net","sentAt":"2023-08-04T16:46:23Z","receivedAt":"2023-08-04T16:47:40Z","isPatch":false,"sender":{"key":"pwatson2@wellmed.net","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\n9:43:55.45 2023-08-04 C:\\src\\t\\scripts>TYPE .\\t1.txt\nthis is t1.txt\n\n9:43:57.92 2023-08-04 C:\\src\\t\\scripts>TYPE .\\t2.txt\nthis is t2.txt\n\n9: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\n1 file changed, 1 insertion(+), 1 deletion(-)\n\n9:43:58.14 2023-08-04 C:\\src\\t\\scripts>ECHO %ERRORLEVEL%\n0\n\nWhat did you expect to happen? (Expected behavior)\n\nI expected that the exit code from `git diff` would be 1, or something non-zero because the files are different.\n\nWhat happened instead? (Actual behavior)\n\nThe exit code was zero (0).\n\nWhat's different between what you expected and what actually happened?\n\nZero (0) is not one (1). :-)\n\nAnything else you want to add:\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.39.2.windows.1\ncpu: x86_64\nbuilt from commit: a82fa99b36ddfd643e61ed45e52abe314687df67\nsizeof-long: 4\nsizeof-size_t: 8\nshell-path: /bin/sh\nfeature: fsmonitor--daemon\nuname: Windows 10.0 19044 \ncompiler info: gnuc: 12.2\nlibc info: no libc information available\n$SHELL (typically, interactive shell): \n PowerShell 7.3.6 Console on Windows 10\n\n[Enabled Hooks]\nThis e-mail, including attachments, may include confidential and/or\nproprietary information, and may be used only by the person or entity\nto which it is addressed. If the reader of this e-mail is not the intended\nrecipient or intended recipient’s authorized agent, the reader is hereby\nnotified that any dissemination, distribution or copying of this e-mail is\nprohibited. If you have received this e-mail in error, please notify the\nsender by replying to this message and delete this e-mail immediately.\n"},{"id":"480158","messageId":"015101d9c6f9$218ceb20$64a6c160$@nexbridge.com","threadId":"60061","inReplyTo":"MW2PR12MB25558D1886C4AA2A95A5FC618709A@MW2PR12MB2555.namprd12.prod.outlook.com","subject":"RE: git bug report","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2023-08-04T17:28:43Z","receivedAt":"2023-08-04T17:28:56Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Friday, August 4, 2023 12:46 PM, Paul Watson wrote:\n>Thank you for filling out a Git bug report!\n>Please answer the following questions to help us understand your issue.\n>\n>What did you do before the bug happened? (Steps to reproduce your issue)\n>\n>9:43:55.45 2023-08-04 C:\\src\\t\\scripts>TYPE .\\t1.txt this is t1.txt\n>\n>9:43:57.92 2023-08-04 C:\\src\\t\\scripts>TYPE .\\t2.txt this is t2.txt\n>\n>9:43:58.04 2023-08-04 C:\\src\\t\\scripts>\"C:\\Program Files\\Git\\cmd\\git.exe\" diff --exit-\n>code --no-index --ignore-all-space --shortstat .\\t1.txt .\\t2.txt\n>1 file changed, 1 insertion(+), 1 deletion(-)\n\nCan you retry this without --shortstat so we can see what git is reporting as a difference.\n\nThanks,\nRandall\n\n"},{"id":"480159","messageId":"MW2PR12MB2555BB4CEE6BA6B38F4381428709A@MW2PR12MB2555.namprd12.prod.outlook.com","threadId":"60061","inReplyTo":"015101d9c6f9$218ceb20$64a6c160$@nexbridge.com","subject":"RE: [EXTERNAL] RE: git bug report","fromName":"Paul Watson","fromEmail":"pwatson2@wellmed.net","sentAt":"2023-08-04T17:48:57Z","receivedAt":"2023-08-04T17:50:09Z","isPatch":false,"sender":{"key":"pwatson2@wellmed.net","avatar":null},"body":"-----Original Message-----\nFrom: rsbecker@nexbridge.com \nSent: Friday, August 4, 2023 12:29\n\n>> Can you retry this without --shortstat so we can see what git is\n>> reporting as a difference.\n\nThe problem appears to be the use of --shortstat. When --shortstat is not used, the exit code for different files is one (1), as expected.\n\n12:46:56.88 2023-08-04 C:\\src\\t\\scripts>TYPE t1.txt\nthis is t1.txt\nthis is t1.txt\nthis is t1.txt\n\n12:47:01.88 2023-08-04 C:\\src\\t\\scripts>type t2.txt\nthis is t2.txt\nthis is t2.txt\nthis is t2.txt\n\n12: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\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\n12:47:18.22 2023-08-04 C:\\src\\t\\scripts>ECHO %ERRORLEVEL%\n0\n\n12: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\ndiff --git a/./t1.txt b/./t2.txt\nindex 6dfb884..66812c5 100644\n--- a/./t1.txt\n+++ b/./t2.txt\n@@ -1,3 +1,3 @@\n-this is t1.txt\n-this is t1.txt\n-this is t1.txt\n+this is t2.txt\n+this is t2.txt\n+this is t2.txt\n\n12:47:27.71 2023-08-04 C:\\src\\t\\scripts>ECHO %ERRORLEVEL%\n1\n\n12: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\n\n12:47:36.58 2023-08-04 C:\\src\\t\\scripts>ECHO %ERRORLEVEL%\n1\nThis e-mail, including attachments, may include confidential and/or\nproprietary information, and may be used only by the person or entity\nto which it is addressed. If the reader of this e-mail is not the intended\nrecipient or intended recipient’s authorized agent, the reader is hereby\nnotified that any dissemination, distribution or copying of this e-mail is\nprohibited. If you have received this e-mail in error, please notify the\nsender by replying to this message and delete this e-mail immediately.\n"},{"id":"480161","messageId":"015901d9c6ff$43606050$ca2120f0$@nexbridge.com","threadId":"60061","inReplyTo":"MW2PR12MB2555BB4CEE6BA6B38F4381428709A@MW2PR12MB2555.namprd12.prod.outlook.com","subject":"RE: [EXTERNAL] RE: git bug report","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2023-08-04T18:12:37Z","receivedAt":"2023-08-04T18:13:17Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Friday, August 4, 2023 1:49 PM, Paul Watson wrote:\n>From: rsbecker@nexbridge.com \n>Sent: Friday, August 4, 2023 12:29\n>\n>>> Can you retry this without --shortstat so we can see what git is\n>>> reporting as a difference.\n>\n>The problem appears to be the use of --shortstat. When --shortstat is not used, the\n>exit code for different files is one (1), as expected.\n\nI 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.\n\n"},{"id":"480214","messageId":"MW2PR12MB25557AAF161A92E862AE5EE2870CA@MW2PR12MB2555.namprd12.prod.outlook.com","threadId":"60061","inReplyTo":"015901d9c6ff$43606050$ca2120f0$@nexbridge.com","subject":"RE: [EXTERNAL] RE: git bug report","fromName":"Paul Watson","fromEmail":"pwatson2@wellmed.net","sentAt":"2023-08-07T15:46:27Z","receivedAt":"2023-08-07T15:47:17Z","isPatch":false,"sender":{"key":"pwatson2@wellmed.net","avatar":null},"body":"From: rsbecker@nexbridge.com \n\n> I am not convinced this is a defect. The result of --shortstat is to\n> report that there are differences, so it is possible that 0 is correct\n> here. Others might have a different opinion. I can recreate without\n> including any other options.\n\nThe code has already done the work to identify if there are differences. Why would it not return a meaningful exit code?\nThis e-mail, including attachments, may include confidential and/or\nproprietary information, and may be used only by the person or entity\nto which it is addressed. If the reader of this e-mail is not the intended\nrecipient or intended recipient’s authorized agent, the reader is hereby\nnotified that any dissemination, distribution or copying of this e-mail is\nprohibited. If you have received this e-mail in error, please notify the\nsender by replying to this message and delete this e-mail immediately.\n"},{"id":"480284","messageId":"MW2PR12MB2555240C4E2A9EB6332A8CAD870DA@MW2PR12MB2555.namprd12.prod.outlook.com","threadId":"60061","inReplyTo":"MW2PR12MB25557AAF161A92E862AE5EE2870CA@MW2PR12MB2555.namprd12.prod.outlook.com","subject":"RE: [EXTERNAL] RE: git bug report","fromName":"Paul Watson","fromEmail":"pwatson2@wellmed.net","sentAt":"2023-08-08T13:07:32Z","receivedAt":"2023-08-08T16:35:51Z","isPatch":false,"sender":{"key":"pwatson2@wellmed.net","avatar":null},"body":">From: rsbecker@nexbridge.com \n>\n>> I am not convinced this is a defect. The result of --shortstat is to \n>> report that there are differences, so it is possible that 0 is correct \n>> here. Others might have a different opinion. I can recreate without \n>> including any other options.\n>\n> The code has already done the work to identify if there are differences. > Why would it not return a meaningful exit code?\n\nAlso, since the --exit-code switch has been specified, it should set the exit code accordingly.\nThis e-mail, including attachments, may include confidential and/or\nproprietary information, and may be used only by the person or entity\nto which it is addressed. If the reader of this e-mail is not the intended\nrecipient or intended recipient’s authorized agent, the reader is hereby\nnotified that any dissemination, distribution or copying of this e-mail is\nprohibited. If you have received this e-mail in error, please notify the\nsender by replying to this message and delete this e-mail immediately.\n"},{"id":"480304","messageId":"xmqqbkfh8nu3.fsf@gitster.g","threadId":"60061","inReplyTo":"MW2PR12MB25558D1886C4AA2A95A5FC618709A@MW2PR12MB2555.namprd12.prod.outlook.com","subject":"Re: git bug report","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-08T17:07:32Z","receivedAt":"2023-08-08T18:53:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Watson writes:\n\n> 9:43:55.45 2023-08-04 C:\\src\\t\\scripts>TYPE .\\t1.txt\n> this is t1.txt\n\n> 9:43:57.92 2023-08-04 C:\\src\\t\\scripts>TYPE .\\t2.txt\n> this is t2.txt\n>\n> 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\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> 9:43:58.14 2023-08-04 C:\\src\\t\\scripts>ECHO %ERRORLEVEL%\n> 0\n\nThis is not specific to Windows port and it can be reproduced on a\nrandom Linux box.\n\n $ echo one >1\n $ echo two >2\n $ git diff --no-index --shortstat 1 2; echo \"<<$?>>\"\n 1 file changed, 1 insertion(+), 1 deletion(-)\n <<1>>\n $ git diff --no-index --shortstat -w 1 2; echo \"<<$?>>\"\n 1 file changed, 1 insertion(+), 1 deletion(-)\n <<0>>\n\nNote that I omitted \"--exit-code\" in the above reproduction, as it\nis always used in the no-index mode. There seems to be interaction\nwith \"-w\" and not using \"-p\", as this is not limited to \"--shortstat\".\n\n $ git diff --no-index -w --stat 1 2; echo \"<<$?>>\"\n 1 => 2 | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n <<0>>\n\n $ git diff --no-index -w --numstat 1 2; echo \"<<$?>>\"\n 1\t1\t1 => 2\n <<0>>\n\nAll of the above that exits with 0 status will exit with 1 when -p\nis added to the command line.\n\nAlso, this is not limited to the no-index mode.\n\n $ echo one >1\n $ git add 1\n $ echo two >1\n $ git diff --exit-code --numstat 1; echo \"<<$?>>\"\n 1\t1\t1\n <<1>>\n $ git diff --exit-code --numstat -w 1; echo \"<<$?>>\"\n 1\t1\t1\n <<0>>\n\nSo the minimum reproduction seems to be\n\n * the diff machinery is asked to do --exit-code (no-index\n implicitly does it)\n * -w is used\n * -p is *not* used\n * to compare two different files.\n\nThanks for a bug report.\n\nPatches welcome ;-)\n"},{"id":"480309","messageId":"064501d9c9fc$31e45750$95ad05f0$@nexbridge.com","threadId":"60061","inReplyTo":"MW2PR12MB2555240C4E2A9EB6332A8CAD870DA@MW2PR12MB2555.namprd12.prod.outlook.com","subject":"RE: [EXTERNAL] RE: git bug report","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2023-08-08T13:28:13Z","receivedAt":"2023-08-08T19:20:46Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Tuesday, August 8, 2023 9:08 AM, Paul Watson wrote:\n>>From: rsbecker@nexbridge.com \n>>\n>>> I am not convinced this is a defect. The result of --shortstat is to\n>>> report that there are differences, so it is possible that 0 is\n>>> correct here. Others might have a different opinion. I can recreate\n>>> without including any other options.\n>>\n>> The code has already done the work to identify if there are differences. > Why\n>would it not return a meaningful exit code?\n>\n>Also, since the --exit-code switch has been specified, it should set the exit code\n>accordingly.\n\nI think this is a decision for the maintainers, whether I agree or disagree. I have to defer to others.\n\n--Randall\n\n"},{"id":"480729","messageId":"xmqqv8ded018.fsf@gitster.g","threadId":"60061","inReplyTo":"xmqqbkfh8nu3.fsf@gitster.g","subject":"[PATCH] diff: tighten interaction between -w and --exit-code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-16T23:45:23Z","receivedAt":"2023-08-16T23:46:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The \"diff\" family of commands have \"--exit-code\" option to make them\nreport if they \"saw\" changes with their exit code, but it did not work\nwhen options in the \"ignore space\" family is used *and* the output mode\ndid not involve the \"--patch\" format output, unless the output is\ntotally suppressed with \"-s\".\n\nInternally, the diff machinery grabs sets of paths from the two\nsides being compared that record different blob objects, optionally\nmatches these paths up for rename processing, and compares the blob\nobjects pair-wise. When options like \"-w\" are *not* involved, the\nmachinery can say that it see a difference immediately after\nnoticing that the object names of the blobs at paths being compared\nare different. In essence, this means that a non-empty diff_queue[]\nmeans the command should exit with 1 under \"--exit-code\" mode.\n\nBut when options that may make two different blobs compare as\nequivalents, like \"-w\" that ignores whitespace differences, are in\neffect, the blobs need to be compared for their contents, as if we\nwere generating a patch, even when the user is only interested in\n\"--exit-code\" and not an actual differences.\n\nThere are two special case tweaks in the existing code. One is that\nthe codepath to compute \"--patch\" output sets .found_changes bit\nonly when it sees a \"real\" change (whose definition is loosened when\n\"-w\" is in effect to ignore whitespace-only changes), and uses that\nbit, instead of whether diff_queue[] has any paths, to decide the\nexit status. The other is that when \"-s\" (no output) is combined\nwith \"-w\", the machinery calls the same \"--patch\" codepath but\nredirecting the output to void, only for the .found_changes bit.\n\nThat is fine, until somebody comes and tries to combine options like\n\"--stat\", \"--name-only\", \"--name-status\", etc. with \"-w\". Because\nthe second special case above to run a fallback \"--patch\" computation\ndoes not kick in for these other output modes, .found_changes bit is\nnot updated correctly.\n\nTo fix this, do two things:\n\n * The codepath to generate \"--stat\" output already calls the\n underlying xdiff machinery with appropriate options like \"-w\",\n but it did not update .found_changes bit. Fixing \"--stat -w\"\n combined with \"--exit-code\" thus becomes just the matter of\n adding these missing .found_changes assignment.\n\n * For generating \"--name-only\", \"--name-status\", etc., the code\n does not look into the contents of the blob objects at all. For\n now, extend the special case used for \"-s -w --exit-code\" to run\n a silent \"--patch\" computation to set the .found_changes bit\n correctly.\n\nNot that the latter may still not be correct in that a path whose\ncontents have no differences other than whitespace changes would\nstill show up in the \"diff -w --name-only --exit-code\" output, even\nthough the exit status may say there is no differences. Arguably\nthis is better than status quo, even though it still is wrong.\n\nReported-by: Paul Watson \nSigned-off-by: Junio C Hamano \n---\nJunio C Hamano writes:\n\n> Also, this is not limited to the no-index mode.\n>\n> $ echo one >1\n> $ git add 1\n> $ echo two >1\n> $ git diff --exit-code --numstat 1; echo \"<<$?>>\"\n> 1\t1\t1\n> <<1>>\n> $ git diff --exit-code --numstat -w 1; echo \"<<$?>>\"\n> 1\t1\t1\n> <<0>>\n>\n> So the minimum reproduction seems to be\n>\n> * the diff machinery is asked to do --exit-code (no-index\n> implicitly does it)\n> * -w is used\n> * -p is *not* used\n> * to compare two different files.\n>\n> Thanks for a bug report.\n>\n> Patches welcome ;-)\n\nSo I ended up looking into this myself X-<.\n\n diff.c | 22 +++++++++-\n t/t4015-diff-whitespace.sh | 100 ++++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 119 insertions(+), 3 deletions(-)\n\ndiff --git c/diff.c w/diff.c\nindex ee3eb629e3..38b57b589f 100644\n--- c/diff.c\n+++ w/diff.c\n@@ -3795,6 +3795,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\t} else {\n \t\t\tdata->added = diff_filespec_size(o->repo, two);\n \t\t\tdata->deleted = diff_filespec_size(o->repo, one);\n+\t\t\to->found_changes = 1;\n \t\t}\n \t}\n \n@@ -3803,6 +3804,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\tdiff_populate_filespec(o->repo, two, NULL);\n \t\tdata->deleted = count_lines(one->data, one->size);\n \t\tdata->added = count_lines(two->data, two->size);\n+\t\to->found_changes = 1;\n \t}\n \n \telse if (may_differ) {\n@@ -3828,6 +3830,11 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\t\t\t diffstat_consume, diffstat, &xpp, &xecfg))\n \t\t\tdie(\"unable to generate diffstat for %s\", one->path);\n \n+\t\t/* Do this before cancelling the no-op diffstat below */\n+\t\tif (diffstat->files[diffstat->nr - 1]->added ||\n+\t\t diffstat->files[diffstat->nr - 1]->deleted)\n+\t\t\to->found_changes = 1;\n+\n \t\tif (DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two)) {\n \t\t\tstruct diffstat_file *file =\n \t\t\t\tdiffstat->files[diffstat->nr - 1];\n@@ -4832,6 +4839,11 @@ void diff_setup_done(struct diff_options *options)\n \telse\n \t\toptions->prefix_length = 0;\n \n+\t/*\n+\t * This is how \"diff --name-status -p --stat --raw\" becomes\n+\t * equivalent to \"diff --name-status\", which may be\n+\t * unintuitive.\n+\t */\n \tif (options->output_format & (DIFF_FORMAT_NAME |\n \t\t\t\t DIFF_FORMAT_NAME_STATUS |\n \t\t\t\t DIFF_FORMAT_CHECKDIFF |\n@@ -6684,13 +6696,19 @@ void diff_flush(struct diff_options *options)\n \t\tseparator++;\n \t}\n \n-\tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n+\tif (((output_format & DIFF_FORMAT_NO_OUTPUT) ||\n+\t /* these compute .found_changes properly */\n+\t !(output_format & (DIFF_FORMAT_DIFFSTAT|\n+\t\t\t\tDIFF_FORMAT_SHORTSTAT|\n+\t\t\t\tDIFF_FORMAT_NUMSTAT|\n+\t\t\t\tDIFF_FORMAT_DIRSTAT|\n+\t\t\t\tDIFF_FORMAT_PATCH))) &&\n \t options->flags.exit_with_status &&\n \t options->flags.diff_from_contents) {\n \t\t/*\n \t\t * run diff_flush_patch for the exit status. setting\n \t\t * options->file to /dev/null should be safe, because we\n-\t\t * aren't supposed to produce any output anyway.\n+\t\t * aren't supposed to produce any output from here.\n \t\t */\n \t\tdiff_free_file(options);\n \t\toptions->file = xfopen(\"/dev/null\", \"w\");\ndiff --git c/t/t4015-diff-whitespace.sh w/t/t4015-diff-whitespace.sh\nindex b298f220e0..1b944d77e4 100755\n--- c/t/t4015-diff-whitespace.sh\n+++ w/t/t4015-diff-whitespace.sh\n@@ -1,7 +1,7 @@\n #!/bin/sh\n #\n # Copyright (c) 2006 Johannes E. Schindelin\n-#\n+# Copyright (c) 2023 Google LLC\n \n test_description='Test special whitespace in diff engine.\n \n@@ -11,6 +11,104 @@ TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-diff.sh\n \n+test_expect_success 'exit status with -w --name-only (different)' '\n+\techo foo >x &&\n+\tgit add x &&\n+\techo bar >x &&\n+\ttest_expect_code 1 git diff -w --name-only --exit-code x\n+'\n+\n+test_expect_success 'exit status with -w --name-only (different but equivalent)' '\n+\techo foo >x &&\n+\tgit add x &&\n+\techo \" foo\" >x &&\n+\tgit diff -w --name-only --exit-code x\n+'\n+\n+test_expect_success 'exit status with -w --raw (different)' '\n+\techo foo >x &&\n+\tgit add x &&\n+\techo bar >x &&\n+\ttest_expect_code 1 git diff -w --raw --exit-code x\n+'\n+\n+test_expect_success 'exit status with -w --raw (different but equivalent)' '\n+\techo foo >x &&\n+\tgit add x &&\n+\techo \" foo\" >x &&\n+\tgit diff -w --raw --exit-code x\n+'\n+\n+test_expect_success 'exit status with -w --stat (different)' '\n+\techo foo >x &&\n+\tgit add x &&\n+\techo bar >x &&\n+\ttest_expect_code 1 git diff -w --stat --exit-code x\n+'\n+\n+test_expect_success 'exit status with -w --stat (different but equivalent)' '\n+\techo foo >x &&\n+\tgit add x &&\n+\techo \" foo\" >x &&\n+\tgit diff -w --stat --exit-code x\n+'\n+\n+test_expect_success 'exit status with -w --shortstat (different)' '\n+\techo foo >x &&\n+\tgit add x &&\n+\techo bar >x &&\n+\ttest_expect_code 1 git diff -w --shortstat --exit-code x\n+'\n+\n+test_expect_success 'exit status with -w --shortstat (different but equivalent)' '\n+\techo foo >x &&\n+\tgit add x &&\n+\techo \" foo\" >x &&\n+\tgit diff -w --shortstat --exit-code x\n+'\n+\n+test_expect_success 'exit status with -w --quiet (different)' '\n+\techo foo >x &&\n+\tgit add x &&\n+\techo bar >x &&\n+\ttest_expect_code 1 git diff -w --quiet --exit-code x\n+'\n+\n+test_expect_success 'exit status with -w --quiet (different but equivalent)' '\n+\techo foo >x &&\n+\tgit add x &&\n+\techo \" foo\" >x &&\n+\tgit diff -w --quiet --exit-code x\n+'\n+\n+test_expect_success 'exit status with -w --summary (different)' '\n+\techo foo >x &&\n+\tgit add x &&\n+\techo bar >x &&\n+\ttest_expect_code 1 git diff -w --summary --exit-code x\n+'\n+\n+test_expect_success 'exit status with -w --summary (different but equivalent)' '\n+\techo foo >x &&\n+\tgit add x &&\n+\techo \" foo\" >x &&\n+\tgit diff -w --summary --exit-code x\n+'\n+\n+test_expect_success 'exit status with -w -s (different)' '\n+\techo foo >x &&\n+\tgit add x &&\n+\techo bar >x &&\n+\ttest_expect_code 1 git diff -w -s --exit-code x\n+'\n+\n+test_expect_success 'exit status with -w -s (different but equivalent)' '\n+\techo foo >x &&\n+\tgit add x &&\n+\techo \" foo\" >x &&\n+\tgit diff -w -s --exit-code x\n+'\n+\n test_expect_success \"Ray Lehtiniemi's example\" '\n \tcat <<-\\EOF >x &&\n \tdo {\n\n"},{"id":"480738","messageId":"20230817051059.GA3006160@coredump.intra.peff.net","threadId":"60061","inReplyTo":"xmqqv8ded018.fsf@gitster.g","subject":"Re: [PATCH] diff: tighten interaction between -w and --exit-code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-17T05:10:59Z","receivedAt":"2023-08-17T05:12:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 16, 2023 at 04:45:23PM -0700, Junio C Hamano wrote:\n\n> To fix this, do two things:\n> \n> * The codepath to generate \"--stat\" output already calls the\n> underlying xdiff machinery with appropriate options like \"-w\",\n> but it did not update .found_changes bit. Fixing \"--stat -w\"\n> combined with \"--exit-code\" thus becomes just the matter of\n> adding these missing .found_changes assignment.\n> \n> * For generating \"--name-only\", \"--name-status\", etc., the code\n> does not look into the contents of the blob objects at all. For\n> now, extend the special case used for \"-s -w --exit-code\" to run\n> a silent \"--patch\" computation to set the .found_changes bit\n> correctly.\n\nNicely explained overall, but one hunk of the patch left me wondering...\n\n> @@ -3828,6 +3830,11 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n> \t\t\t\t diffstat_consume, diffstat, &xpp, &xecfg))\n> \t\t\tdie(\"unable to generate diffstat for %s\", one->path);\n> \n> +\t\t/* Do this before cancelling the no-op diffstat below */\n> +\t\tif (diffstat->files[diffstat->nr - 1]->added ||\n> +\t\t diffstat->files[diffstat->nr - 1]->deleted)\n> +\t\t\to->found_changes = 1;\n> +\n\nSo this is checking whether any lines were added/deleted to see if the\nstat was a noop. But what about non-content bits, like mode changes?\nE.g., the tests below all fail (but would pass without \"-w\"):\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 1b944d77e4..54e56ad911 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -25,6 +25,14 @@ test_expect_success 'exit status with -w --name-only (different but equivalent)'\n \tgit diff -w --name-only --exit-code x\n '\n \n+test_expect_success 'exit status with -w --name-only (mode change)' '\n+\ttest_when_finished \"git reset\" &&\n+\techo foo >x &&\n+\tgit add x &&\n+\tgit update-index --chmod=+x x &&\n+\ttest_expect_code 1 git diff -w --name-only --exit-code\n+'\n+\n test_expect_success 'exit status with -w --raw (different)' '\n \techo foo >x &&\n \tgit add x &&\n@@ -39,6 +47,14 @@ test_expect_success 'exit status with -w --raw (different but equivalent)' '\n \tgit diff -w --raw --exit-code x\n '\n \n+test_expect_success 'exit status with -w --raw (mode change)' '\n+\ttest_when_finished \"git reset\" &&\n+\techo foo >x &&\n+\tgit add x &&\n+\tgit update-index --chmod=+x x &&\n+\ttest_expect_code 1 git diff -w --raw --exit-code\n+'\n+\n test_expect_success 'exit status with -w --stat (different)' '\n \techo foo >x &&\n \tgit add x &&\n@@ -53,6 +69,14 @@ test_expect_success 'exit status with -w --stat (different but equivalent)' '\n \tgit diff -w --stat --exit-code x\n '\n \n+test_expect_success 'exit status with -w --stat (mode change)' '\n+\ttest_when_finished \"git reset\" &&\n+\techo foo >x &&\n+\tgit add x &&\n+\tgit update-index --chmod=+x x &&\n+\ttest_expect_code 1 git diff -w --stat --exit-code\n+'\n+\n test_expect_success 'exit status with -w --shortstat (different)' '\n \techo foo >x &&\n \tgit add x &&\n@@ -67,6 +91,14 @@ test_expect_success 'exit status with -w --shortstat (different but equivalent)'\n \tgit diff -w --shortstat --exit-code x\n '\n \n+test_expect_success 'exit status with -w --shortstat (mode change)' '\n+\ttest_when_finished \"git reset\" &&\n+\techo foo >x &&\n+\tgit add x &&\n+\tgit update-index --chmod=+x x &&\n+\ttest_expect_code 1 git diff -w --shortstat --exit-code\n+'\n+\n test_expect_success 'exit status with -w --quiet (different)' '\n \techo foo >x &&\n \tgit add x &&\n@@ -81,6 +113,14 @@ test_expect_success 'exit status with -w --quiet (different but equivalent)' '\n \tgit diff -w --quiet --exit-code x\n '\n \n+test_expect_success 'exit status with -w --quiet (mode change)' '\n+\ttest_when_finished \"git reset\" &&\n+\techo foo >x &&\n+\tgit add x &&\n+\tgit update-index --chmod=+x x &&\n+\ttest_expect_code 1 git diff -w --quiet --exit-code\n+'\n+\n test_expect_success 'exit status with -w --summary (different)' '\n \techo foo >x &&\n \tgit add x &&\n@@ -95,6 +135,14 @@ test_expect_success 'exit status with -w --summary (different but equivalent)' '\n \tgit diff -w --summary --exit-code x\n '\n \n+test_expect_success 'exit status with -w --summary (mode change)' '\n+\ttest_when_finished \"git reset\" &&\n+\techo foo >x &&\n+\tgit add x &&\n+\tgit update-index --chmod=+x x &&\n+\ttest_expect_code 1 git diff -w --summary --exit-code\n+'\n+\n test_expect_success 'exit status with -w -s (different)' '\n \techo foo >x &&\n \tgit add x &&\n@@ -109,6 +157,14 @@ test_expect_success 'exit status with -w -s (different but equivalent)' '\n \tgit diff -w -s --exit-code x\n '\n \n+test_expect_success 'exit status with -w -s (mode change)' '\n+\ttest_when_finished \"git reset\" &&\n+\techo foo >x &&\n+\tgit add x &&\n+\tgit update-index --chmod=+x x &&\n+\ttest_expect_code 1 git diff -w -s --exit-code x\n+'\n+\n test_expect_success \"Ray Lehtiniemi's example\" '\n \tcat <<-\\EOF >x &&\n \tdo {\n\nFor the diffstat case, I think we could check the mode here, but there\nare other cases (e.g., adding or deleting an empty file). The code right\nbelow the hunk I quoted seems to try to deal with that (the \"cancelling\nthe no-op\" your comment mentions). I'm not sure if we want something\nlike this:\n\ndiff --git a/diff.c b/diff.c\nindex 38b57b589f..1dbfdaeff0 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3853,6 +3853,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\t\t\t&& one->mode == two->mode) {\n \t\t\t\tfree_diffstat_file(file);\n \t\t\t\tdiffstat->nr--;\n+\t\t\t} else {\n+\t\t\t\to->found_changes = 1;\n \t\t\t}\n \t\t}\n \t}\n\nbut I haven't dug too far (and of course all of the other options also\nneed something similar to catch this case).\n\n-Peff\n"},{"id":"480750","messageId":"xmqqy1i9bqcm.fsf@gitster.g","threadId":"60061","inReplyTo":"20230817051059.GA3006160@coredump.intra.peff.net","subject":"Re: [PATCH] diff: tighten interaction between -w and --exit-code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-17T16:12:09Z","receivedAt":"2023-08-17T16:13:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> For the diffstat case, I think we could check the mode here, but there\n> are other cases (e.g., adding or deleting an empty file). The code right\n> below the hunk I quoted seems to try to deal with that (the \"cancelling\n> the no-op\" your comment mentions). I'm not sure if we want something\n> like this:\n>\n> diff --git a/diff.c b/diff.c\n> index 38b57b589f..1dbfdaeff0 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3853,6 +3853,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n> \t\t\t\t&& one->mode == two->mode) {\n> \t\t\t\tfree_diffstat_file(file);\n> \t\t\t\tdiffstat->nr--;\n> +\t\t\t} else {\n> +\t\t\t\to->found_changes = 1;\n> \t\t\t}\n> \t\t}\n> \t}\n\nThat is much better. In all cases where the above diffstat->nr-- is\nnot reached and diffstat is kept is where we found changes, so an\neven simpler solution that fundamentally cannot go wrong would be to\nsee \"diffstat->nr\" at the end (i.e. \"are we going to show diffstat\nfor *any* filepair?\"). If it is non-zero, we did find a difference.\n\nThen we do not have to wonder if that else clause is in the right\nplace, or we have to do something similar to the above for cases\nwhere DIFF_FILE_VALID() is not true for both sides (i.e. creation or\ndeletion).\n\nThanks.\n\n\n\n"},{"id":"480759","messageId":"20230817194912.GB3032779@coredump.intra.peff.net","threadId":"60061","inReplyTo":"xmqqy1i9bqcm.fsf@gitster.g","subject":"Re: [PATCH] diff: tighten interaction between -w and --exit-code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-17T19:49:12Z","receivedAt":"2023-08-17T19:49:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 17, 2023 at 09:12:09AM -0700, Junio C Hamano wrote:\n\n> > diff --git a/diff.c b/diff.c\n> > index 38b57b589f..1dbfdaeff0 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -3853,6 +3853,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n> > \t\t\t\t&& one->mode == two->mode) {\n> > \t\t\t\tfree_diffstat_file(file);\n> > \t\t\t\tdiffstat->nr--;\n> > +\t\t\t} else {\n> > +\t\t\t\to->found_changes = 1;\n> > \t\t\t}\n> > \t\t}\n> > \t}\n> \n> That is much better. In all cases where the above diffstat->nr-- is\n> not reached and diffstat is kept is where we found changes, so an\n> even simpler solution that fundamentally cannot go wrong would be to\n> see \"diffstat->nr\" at the end (i.e. \"are we going to show diffstat\n> for *any* filepair?\"). If it is non-zero, we did find a difference.\n\nYeah, without having really dug into the problem too far, that does\nsound a lot better. I also wonder to what degree you could apply the\nsame strategy to other formats (I guess it depends on them removing\nwhitespace-only changes from a structure). From the test I posted\nearlier, it does look like many of them have the same blind spots for\nmode-only changes (and I suspect addition/removal of empty files is\nanother corner case to check).\n\n-Peff\n"},{"id":"480763","messageId":"xmqqttsxbfyw.fsf@gitster.g","threadId":"60061","inReplyTo":"20230817194912.GB3032779@coredump.intra.peff.net","subject":"Re: [PATCH] diff: tighten interaction between -w and --exit-code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-17T19:56:23Z","receivedAt":"2023-08-17T19:57:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, without having really dug into the problem too far, that does\n> sound a lot better. I also wonder to what degree you could apply the\n> same strategy to other formats (I guess it depends on them removing\n> whitespace-only changes from a structure). From the test I posted\n> earlier, it does look like many of them have the same blind spots for\n> mode-only changes (and I suspect addition/removal of empty files is\n> another corner case to check).\n\nI have something cooking. Stay tuned, without getting excited too\nmuch ;-)\n\nThanks.\n"},{"id":"480771","messageId":"20230817222949.3835424-1-gitster@pobox.com","threadId":"60061","inReplyTo":"xmqqttsxbfyw.fsf@gitster.g","subject":"[PATCH v2 0/5] fix interactions with \"-w\" and \"--exit-code\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-17T22:29:44Z","receivedAt":"2023-08-17T22:30:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"So here is the reworked series. The previous attempt mostly\ncorrespond to [4/5] of this series, but the approach has been quite\nupdated, so here is marked as \"v2\" but I omitted the interdiff as it\nwould be more or less useless.\n\nThe first patch is not part of the main theme, but because I'll be\nadding the first use of \"--dirstat\" to t4015 that has been leak\nsanitizer clean, and the \"--dirstat\" codepath has known leaks, I am\nplugging the leaks as a preliminary step to avoid having to mark\nt4015 as leak sanitizer unclean again.\n\nThe second patch flips around order of processing without changing\nthe meaning. After this change, blocks that compute output that\nalso take contents into account are grouped together, before the\nfallback code for output formats that do not look at the contents to\ncompute their result. It is purely done as a clean-up.\n\nThe earlier series claimed that \"--exit-code -w\" is reliable as long\nas \"--patch\" output is used, but it turns out that there are corner\ncase holes in the \"--patch\" codepath. The third patch fixes them.\n\nThe fourth patch teachs \"--stat\" codepath to help \"--exit-code -w\";\nas it looks at contents to produce its output, we note the fact that\nwe found (or did not find) differences, and use that for the exit\ncode computation.\n\nThe fifth patch deals with other output modes that do not look at\ncontents for their output to reuse the fallback code.\n\nJunio C Hamano (5):\n diff: --dirstat leakfix\n diff: move the fallback \"--exit-code\" code down\n diff: mode-only change should be noticed by \"--patch -w --exit-code\"\n diff: teach \"--stat -w --exit-code\" to notice differences\n diff: teach \"--name-status\" and friends to honor \"--exit-code -w\"\n\n diff.c | 71 +++++++++++++++++++++++++++-----------\n t/t4015-diff-whitespace.sh | 36 ++++++++++++++++++-\n t/t4047-diff-dirstat.sh | 2 ++\n 3 files changed, 87 insertions(+), 22 deletions(-)\n\n-- \n2.42.0-rc2\n\n"},{"id":"480772","messageId":"20230817222949.3835424-2-gitster@pobox.com","threadId":"60061","inReplyTo":"20230817222949.3835424-1-gitster@pobox.com","subject":"[PATCH v2 1/5] diff: --dirstat leakfix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-17T22:29:45Z","receivedAt":"2023-08-17T22:30:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The dirstat_dir structure holds a list of files that had \"damages\"\nand is used to summarize the change by directory. It was allocated,\nused, and then left behind, leaking.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 14 ++++++++++++--\n t/t4047-diff-dirstat.sh | 2 ++\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 648f6717a5..03d0cfc700 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2977,6 +2977,7 @@ static void show_dirstat(struct diff_options *options)\n \tunsigned long changed;\n \tstruct dirstat_dir dir;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n+\tstruct dirstat_file *to_free;\n \n \tdir.files = NULL;\n \tdir.alloc = 0;\n@@ -3060,13 +3061,17 @@ static void show_dirstat(struct diff_options *options)\n \t\tdir.nr++;\n \t}\n \n+\tto_free = dir.files;\n+\n \t/* This can happen even with many files, if everything was renames */\n \tif (!changed)\n-\t\treturn;\n+\t\tgoto free_return;\n \n \t/* Show all directories with more than x% of the changes */\n \tQSORT(dir.files, dir.nr, dirstat_compare);\n \tgather_dirstat(options, &dir, changed, \"\", 0);\n+free_return:\n+\tfree(to_free);\n }\n \n static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *options)\n@@ -3074,6 +3079,7 @@ static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *o\n \tint i;\n \tunsigned long changed;\n \tstruct dirstat_dir dir;\n+\tstruct dirstat_file *to_free;\n \n \tif (data->nr == 0)\n \t\treturn;\n@@ -3104,13 +3110,17 @@ static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *o\n \t\tdir.nr++;\n \t}\n \n+\tto_free = dir.files;\n+\n \t/* This can happen even with many files, if everything was renames */\n \tif (!changed)\n-\t\treturn;\n+\t\tgoto free_return;\n \n \t/* Show all directories with more than x% of the changes */\n \tQSORT(dir.files, dir.nr, dirstat_compare);\n \tgather_dirstat(options, &dir, changed, \"\", 0);\n+free_return:\n+\tfree(to_free);\n }\n \n static void free_diffstat_file(struct diffstat_file *f)\ndiff --git a/t/t4047-diff-dirstat.sh b/t/t4047-diff-dirstat.sh\nindex 7fec2cb9cd..70224c3da1 100755\n--- a/t/t4047-diff-dirstat.sh\n+++ b/t/t4047-diff-dirstat.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='diff --dirstat tests'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # set up two commits where the second commit has these files\n-- \n2.42.0-rc2\n\n"},{"id":"480773","messageId":"20230817222949.3835424-3-gitster@pobox.com","threadId":"60061","inReplyTo":"20230817222949.3835424-1-gitster@pobox.com","subject":"[PATCH v2 2/5] diff: move the fallback \"--exit-code\" code down","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-17T22:29:46Z","receivedAt":"2023-08-17T22:30:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When \"--exit-code\" is asked and the code cannot just answer by\ncomparing the object names on both sides but need to inspect and\ncompare the contents, there are two ways that the result is found\nout.\n\nSome output modes, like \"--stat\" and \"--patch\", inherently have to\ninspect the contents in order to _show_ the differences in the way\nthey do. The codepaths for these modes set the .found_changes bit\nas they compute what to show.\n\nHowever, other output modes do not need to inspect the contents to\nshow the differences in the way they do. The most notable example\nis \"--quiet\", which does not need to compute any output. When they\nare asked to report \"--exit-code\", they run the codepaths for the\n\"--patch\" output with their output redirected to \"/dev/null\", only\nto set the .found_changes bit.\n\nCurrently, this fallback invocation of \"--patch\" output is done\nafter the \"--stat\" output format and its friends and before the\n\"--patch\" and internal callback logic. Move it to the end of\nthe sequence to clarify the fallback status of this code block.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 03d0cfc700..ce9c8272c7 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6555,6 +6555,21 @@ void diff_flush(struct diff_options *options)\n \t\tseparator++;\n \t}\n \n+\tif (output_format & DIFF_FORMAT_PATCH) {\n+\t\tif (separator) {\n+\t\t\temit_diff_symbol(options, DIFF_SYMBOL_SEPARATOR, NULL, 0, 0);\n+\t\t\tif (options->stat_sep)\n+\t\t\t\t/* attach patch instead of inline */\n+\t\t\t\temit_diff_symbol(options, DIFF_SYMBOL_STAT_SEP,\n+\t\t\t\t\t\t NULL, 0, 0);\n+\t\t}\n+\n+\t\tdiff_flush_patch_all_file_pairs(options);\n+\t}\n+\n+\tif (output_format & DIFF_FORMAT_CALLBACK)\n+\t\toptions->format_callback(q, options, options->format_callback_data);\n+\n \tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n \t options->flags.exit_with_status &&\n \t options->flags.diff_from_contents) {\n@@ -6576,21 +6591,6 @@ void diff_flush(struct diff_options *options)\n \t\t}\n \t}\n \n-\tif (output_format & DIFF_FORMAT_PATCH) {\n-\t\tif (separator) {\n-\t\t\temit_diff_symbol(options, DIFF_SYMBOL_SEPARATOR, NULL, 0, 0);\n-\t\t\tif (options->stat_sep)\n-\t\t\t\t/* attach patch instead of inline */\n-\t\t\t\temit_diff_symbol(options, DIFF_SYMBOL_STAT_SEP,\n-\t\t\t\t\t\t NULL, 0, 0);\n-\t\t}\n-\n-\t\tdiff_flush_patch_all_file_pairs(options);\n-\t}\n-\n-\tif (output_format & DIFF_FORMAT_CALLBACK)\n-\t\toptions->format_callback(q, options, options->format_callback_data);\n-\n \tfor (i = 0; i < q->nr; i++)\n \t\tdiff_free_filepair(q->queue[i]);\n free_queue:\n-- \n2.42.0-rc2\n\n"},{"id":"480774","messageId":"20230817222949.3835424-4-gitster@pobox.com","threadId":"60061","inReplyTo":"20230817222949.3835424-1-gitster@pobox.com","subject":"[PATCH v2 3/5] diff: mode-only change should be noticed by \"--patch -w --exit-code\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-17T22:29:47Z","receivedAt":"2023-08-17T22:30:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The codepath to notice the content-level changes, taking certaion\nno-op changes like \"ignore whitespace\" into account, forgot that\na mode-only change is still a change. This resulted in\n\n $ git diff --patch --exit-code -w\n\nto exit with status 0 even when there is such a mode-only change,\nbreaking both \"--patch\" and \"--quiet\" output formats.\n\nTeach the builtin_diff() codepath that creation and deletion as well\nas mode changes are all interesting changes.\n\nNote that the test specifically checks removal of an empty file,\nbecause if there is anything in the preimage (i.e. the removed file\nis not empty), the removal would still trigger textual patch output\nand the codepath for that does update .found_changes bit to report\nthat it found an interesting change. We need to make sure that the\n.found_changes bit is set even without triggering textual patch\noutput.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 3 +++\n t/t4015-diff-whitespace.sh | 35 ++++++++++++++++++++++++++++++++++-\n 2 files changed, 37 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex ce9c8272c7..de18364902 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3501,18 +3501,21 @@ static void builtin_diff(const char *name_a,\n \t\tstrbuf_addf(&header, \"%s%snew file mode %06o%s\\n\", line_prefix, meta, two->mode, reset);\n \t\tif (xfrm_msg)\n \t\t\tstrbuf_addstr(&header, xfrm_msg);\n+\t\to->found_changes = 1;\n \t\tmust_show_header = 1;\n \t}\n \telse if (lbl[1][0] == '/') {\n \t\tstrbuf_addf(&header, \"%s%sdeleted file mode %06o%s\\n\", line_prefix, meta, one->mode, reset);\n \t\tif (xfrm_msg)\n \t\t\tstrbuf_addstr(&header, xfrm_msg);\n+\t\to->found_changes = 1;\n \t\tmust_show_header = 1;\n \t}\n \telse {\n \t\tif (one->mode != two->mode) {\n \t\t\tstrbuf_addf(&header, \"%s%sold mode %06o%s\\n\", line_prefix, meta, one->mode, reset);\n \t\t\tstrbuf_addf(&header, \"%s%snew mode %06o%s\\n\", line_prefix, meta, two->mode, reset);\n+\t\t\to->found_changes = 1;\n \t\t\tmust_show_header = 1;\n \t\t}\n \t\tif (xfrm_msg)\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex f3e20dd5bb..943ad252d4 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1,7 +1,7 @@\n #!/bin/sh\n #\n # Copyright (c) 2006 Johannes E. Schindelin\n-#\n+# Copyright (c) 2023 Google LLC\n \n test_description='Test special whitespace in diff engine.\n \n@@ -11,6 +11,39 @@ TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-diff.sh\n \n+for opts in --patch --quiet -s\n+do\n+\n+\ttest_expect_success \"status with $opts (different)\" '\n+\t\techo foo >x &&\n+\t\tgit add x &&\n+\t\techo bar >x &&\n+\t\ttest_expect_code 1 git diff -w $opts --exit-code x\n+\t'\n+\n+\ttest_expect_success \"status with $opts (mode differs)\" '\n+\t\ttest_when_finished \"git update-index --chmod=-x x\" &&\n+\t\techo foo >x &&\n+\t\tgit add x &&\n+\t\tgit update-index --chmod=+x x &&\n+\t\ttest_expect_code 1 git diff -w $opts --exit-code x\n+\t'\n+\n+\ttest_expect_success \"status with $opts (removing an empty file)\" '\n+\t\t: >x &&\n+\t\tgit add x &&\n+\t\trm x &&\n+\t\ttest_expect_code 1 git diff -w $opts --exit-code -- x\n+\t'\n+\n+\ttest_expect_success \"status with $opts (different but equivalent)\" '\n+\t\techo foo >x &&\n+\t\tgit add x &&\n+\t\techo \" foo\" >x &&\n+\t\tgit diff -w $opts --exit-code x\n+\t'\n+done\n+\n test_expect_success \"Ray Lehtiniemi's example\" '\n \tcat <<-\\EOF >x &&\n \tdo {\n-- \n2.42.0-rc2\n\n"},{"id":"480775","messageId":"20230817222949.3835424-5-gitster@pobox.com","threadId":"60061","inReplyTo":"20230817222949.3835424-1-gitster@pobox.com","subject":"[PATCH v2 4/5] diff: teach \"--stat -w --exit-code\" to notice differences","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-17T22:29:48Z","receivedAt":"2023-08-17T22:30:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When options like \"-w\" is used while \"--exit-code\" option is in\neffect, instead of the usual \"do we have any filepair whose preimage\nand postimage have different ?\" check, we need to compare\nthe contents of the blobs, taking into account that certain changes\nare considered no-op.\n\nWith the previous step, we taught \"--patch\" codepath to set the\n.found_changes bit correctly, even for a change that only affects\nthe mode and not object. The \"--stat\" codepath, however, did not\nset the .found_changes bit at all. This lead to\n\n $ git diff --stat -w --exit-code\n\nfor a change that does have an outout to exit with status 0.\n\nSet the bit by inspecting the list of paths the diffstat output is\ngiven for (a mode-only change will still appear as a \"0-line added\n0-line deleted\" change) to fix it.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 1 +\n t/t4015-diff-whitespace.sh | 2 +-\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex de18364902..7213237675 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6905,6 +6905,7 @@ void compute_diffstat(struct diff_options *options,\n \t\tif (check_pair_status(p))\n \t\t\tdiff_flush_stat(p, options, diffstat);\n \t}\n+\toptions->found_changes = !!diffstat->nr;\n }\n \n void diff_addremove(struct diff_options *options,\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 943ad252d4..355d96aa14 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -11,7 +11,7 @@ TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-diff.sh\n \n-for opts in --patch --quiet -s\n+for opts in --patch --quiet -s --stat --shortstat --dirstat=lines\n do\n \n \ttest_expect_success \"status with $opts (different)\" '\n-- \n2.42.0-rc2\n\n"},{"id":"480776","messageId":"20230817222949.3835424-6-gitster@pobox.com","threadId":"60061","inReplyTo":"20230817222949.3835424-1-gitster@pobox.com","subject":"[PATCH v2 5/5] diff: teach \"--name-status\" and friends to honor \"--exit-code -w\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-17T22:29:49Z","receivedAt":"2023-08-17T22:30:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We have fallback code that is used when \"-s -w --exit-code\" options\nare used together to compute the exit code, because the exit code\nmust take the whitespace-ignoring comparison into account over the\ncontents, but with \"-s\", the normal codepath does not even have to\nlook at the contents at all. The fallback code simply runs a \"git\ndiff --patch\" while sending the output to \"/dev/null\".\n\nThe codepaths for some other output modes, like \"--name-status\" and\n\"--raw\", share the same trait as \"-s\" in that they do not look at\nthe contents for their output generation. Extend the fallback code\nto cover these output modes as well.\n\nNote that they may still not be correct in that a path whose\ncontents have no differences other than whitespace changes would\nstill show up in the \"diff -w --name-only --exit-code\" output, even\nthough the exit status may say there is no differences. Arguably\nthis is better than status quo, even though it still may be wrong.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 23 +++++++++++++++++++----\n t/t4015-diff-whitespace.sh | 3 ++-\n 2 files changed, 21 insertions(+), 5 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 7213237675..3bb9d11bfe 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6573,13 +6573,28 @@ void diff_flush(struct diff_options *options)\n \tif (output_format & DIFF_FORMAT_CALLBACK)\n \t\toptions->format_callback(q, options, options->format_callback_data);\n \n-\tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n+\tif (((output_format & DIFF_FORMAT_NO_OUTPUT) ||\n+\t /* these compute .found_changes properly */\n+\t !(output_format & (DIFF_FORMAT_DIFFSTAT|\n+\t\t\t\tDIFF_FORMAT_SHORTSTAT|\n+\t\t\t\tDIFF_FORMAT_NUMSTAT|\n+\t\t\t\tDIFF_FORMAT_DIRSTAT|\n+\t\t\t\tDIFF_FORMAT_PATCH))) &&\n \t options->flags.exit_with_status &&\n \t options->flags.diff_from_contents) {\n \t\t/*\n-\t\t * run diff_flush_patch for the exit status. setting\n-\t\t * options->file to /dev/null should be safe, because we\n-\t\t * aren't supposed to produce any output anyway.\n+\t\t * We need to inspect the contents, not just object\n+\t\t * names, to determine the exit status, but the usual\n+\t\t * processing for the output format specified does not\n+\t\t * have to work with and does not look at the\n+\t\t * contents. Run an extra and silent \"diff --patch\"\n+\t\t * but discard the output to /dev/null, so that we\n+\t\t * would set the .found_changes bit correctly.\n+\t\t *\n+\t\t * We can safely close and discard the original output\n+\t\t * file here, since all that is left to do from this\n+\t\t * point is to return (we don't do the FORMAT_PATCH\n+\t\t * thing below).\n \t\t */\n \t\tdiff_free_file(options);\n \t\toptions->file = xfopen(\"/dev/null\", \"w\");\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 355d96aa14..412d20181c 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -11,7 +11,8 @@ TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-diff.sh\n \n-for opts in --patch --quiet -s --stat --shortstat --dirstat=lines\n+for opts in --patch --quiet -s --stat --shortstat --dirstat=lines \\\n+\t --name-only --raw --name-status --summary\n do\n \n \ttest_expect_success \"status with $opts (different)\" '\n-- \n2.42.0-rc2\n\n"},{"id":"480777","messageId":"xmqqpm3lz467.fsf@gitster.g","threadId":"60061","inReplyTo":"20230817222949.3835424-1-gitster@pobox.com","subject":"Re: [PATCH v2 0/5] fix interactions with \"-w\" and \"--exit-code\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-17T22:37:20Z","receivedAt":"2023-08-17T22:38:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The first patch is not part of the main theme, but because I'll be\n> adding the first use of \"--dirstat\" to t4015 that has been leak\n> sanitizer clean, and the \"--dirstat\" codepath has known leaks, I am\n> plugging the leaks as a preliminary step to avoid having to mark\n> t4015 as leak sanitizer unclean again.\n\nI completely forgot that I already have done 1e1dcb2a (Merge branch\n'jc/dirstat-plug-leaks', 2023-05-15) in Git 2.41 timeframe. I was\nbasing my work on Git 2.38 maintenance track and needed [1/5] but in\nnewer codebase it is not needed.\n"},{"id":"480789","messageId":"xmqq4jkwuk6j.fsf@gitster.g","threadId":"60061","inReplyTo":"20230817222949.3835424-4-gitster@pobox.com","subject":"Re: [PATCH v2 3/5] diff: mode-only change should be noticed by \"--patch -w --exit-code\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-18T21:15:00Z","receivedAt":"2023-08-18T21:15:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n> index f3e20dd5bb..943ad252d4 100755\n> --- a/t/t4015-diff-whitespace.sh\n> +++ b/t/t4015-diff-whitespace.sh\n> @@ -11,6 +11,39 @@ TEST_PASSES_SANITIZE_LEAK=true\n> + ...\n> +\ttest_expect_success \"status with $opts (mode differs)\" '\n> +\t\ttest_when_finished \"git update-index --chmod=-x x\" &&\n> +\t\techo foo >x &&\n> +\t\tgit add x &&\n> +\t\tgit update-index --chmod=+x x &&\n> +\t\ttest_expect_code 1 git diff -w $opts --exit-code x\n> +\t'\n\nApparently this one needs to skipped on a filesystem without\nsupport for the executable bit.\n\ncf. https://github.com/git/git/actions/runs/5897310248/job/15996914969#step:5:218\n\nI'll give POSIXPERM prerequisite to this test piece.\n"},{"id":"480798","messageId":"20230818235932.3253552-3-gitster@pobox.com","threadId":"60061","inReplyTo":"20230818235932.3253552-1-gitster@pobox.com","subject":"[PATCH v3 2/5] diff: mode-only change should be noticed by \"--patch -w --exit-code\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-18T23:59:29Z","receivedAt":"2023-08-19T00:00:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The codepath to notice the content-level changes, taking certain\nno-op changes like \"ignore whitespace\" into account, forgot that\na mode-only change is still a change. This resulted in\n\n $ git diff --patch --exit-code -w\n\nto exit with status 0 even when there is such a mode-only change,\nbreaking both \"--patch\" and \"--quiet\" output formats.\n\nTeach the builtin_diff() codepath that creation and deletion as well\nas mode changes are all interesting changes.\n\nNote that the test specifically checks removal of an empty file,\nbecause if there is anything in the preimage (i.e. the removed file\nis not empty), the removal would still trigger textual patch output\nand the codepath for that does update .found_changes bit to report\nthat it found an interesting change. We need to make sure that the\n.found_changes bit is set even without triggering textual patch\noutput.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 3 +++\n t/t4015-diff-whitespace.sh | 35 ++++++++++++++++++++++++++++++++++-\n 2 files changed, 37 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 0ce678fc06..998d7ae20c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3497,18 +3497,21 @@ static void builtin_diff(const char *name_a,\n \t\tstrbuf_addf(&header, \"%s%snew file mode %06o%s\\n\", line_prefix, meta, two->mode, reset);\n \t\tif (xfrm_msg)\n \t\t\tstrbuf_addstr(&header, xfrm_msg);\n+\t\to->found_changes = 1;\n \t\tmust_show_header = 1;\n \t}\n \telse if (lbl[1][0] == '/') {\n \t\tstrbuf_addf(&header, \"%s%sdeleted file mode %06o%s\\n\", line_prefix, meta, one->mode, reset);\n \t\tif (xfrm_msg)\n \t\t\tstrbuf_addstr(&header, xfrm_msg);\n+\t\to->found_changes = 1;\n \t\tmust_show_header = 1;\n \t}\n \telse {\n \t\tif (one->mode != two->mode) {\n \t\t\tstrbuf_addf(&header, \"%s%sold mode %06o%s\\n\", line_prefix, meta, one->mode, reset);\n \t\t\tstrbuf_addf(&header, \"%s%snew mode %06o%s\\n\", line_prefix, meta, two->mode, reset);\n+\t\t\to->found_changes = 1;\n \t\t\tmust_show_header = 1;\n \t\t}\n \t\tif (xfrm_msg)\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex f3e20dd5bb..02731dccb9 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1,7 +1,7 @@\n #!/bin/sh\n #\n # Copyright (c) 2006 Johannes E. Schindelin\n-#\n+# Copyright (c) 2023 Google LLC\n \n test_description='Test special whitespace in diff engine.\n \n@@ -11,6 +11,39 @@ TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-diff.sh\n \n+for opts in --patch --quiet -s\n+do\n+\n+\ttest_expect_success \"status with $opts (different)\" '\n+\t\techo foo >x &&\n+\t\tgit add x &&\n+\t\techo bar >x &&\n+\t\ttest_expect_code 1 git diff -w $opts --exit-code x\n+\t'\n+\n+\ttest_expect_success POSIXPERM \"status with $opts (mode differs)\" '\n+\t\ttest_when_finished \"git update-index --chmod=-x x\" &&\n+\t\techo foo >x &&\n+\t\tgit add x &&\n+\t\tgit update-index --chmod=+x x &&\n+\t\ttest_expect_code 1 git diff -w $opts --exit-code x\n+\t'\n+\n+\ttest_expect_success \"status with $opts (removing an empty file)\" '\n+\t\t: >x &&\n+\t\tgit add x &&\n+\t\trm x &&\n+\t\ttest_expect_code 1 git diff -w $opts --exit-code -- x\n+\t'\n+\n+\ttest_expect_success \"status with $opts (different but equivalent)\" '\n+\t\techo foo >x &&\n+\t\tgit add x &&\n+\t\techo \" foo\" >x &&\n+\t\tgit diff -w $opts --exit-code x\n+\t'\n+done\n+\n test_expect_success \"Ray Lehtiniemi's example\" '\n \tcat <<-\\EOF >x &&\n \tdo {\n-- \n2.42.0-rc2-7-gf9972720e9\n\n"},{"id":"480799","messageId":"20230818235932.3253552-1-gitster@pobox.com","threadId":"60061","inReplyTo":"20230817222949.3835424-1-gitster@pobox.com","subject":"[PATCH v3 0/5] fix interactions with \"-w\" and \"--exit-code\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-18T23:59:27Z","receivedAt":"2023-08-19T00:00:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Watson reported that \"diff --no-index --exit-code\n--ignore-all-space\" does not work when used with \"--shortstat\". It\nturns out that this is not limited to \"--no-index\" mode, and it is\nnot limited to \"--shortstat\". Anything that does not use the \"--patch\"\nmachinery to discover the content level differences ignored --exit-code\nwhen used with options like \"-w\" and always exited with 0. In fact,\neven the \"--patch\" machinery was slightly faulty in corner cases.\n\nAnd here is another round to fix it.\nPrevious one is at https://lore.kernel.org/git/20230817222949.3835424-1-gitster@pobox.com/\n\nThe interdiff is not all that interesting. One patch dropped, two\npatches stay the same, one patch has one-line change, and the final\none patch has been completely reworked. Here is the summary:\n\n * The first patch about dirstat leak-fix is now gone. The series\n has instead been rebased on top of the official \"dirstat leak-fix\"\n series that was merged in Git 2.41.\n\n * The next patch (preliminary code clean-up) stays the same. It is\n now [1/5] instead of being the second step.\n\n * The next patch is to fix the corner case bug of \"--patch\"\n machinery. The code stays the same, but the tests were asking\n the filesystem to do something impossible when they do not have\n executable bit support, so a prerequisite has been added to work\n around them.\n\n * The next patch is to fix \"--stat -w --exit-code\", which stays\n the same.\n\n * The last step is completely new. v2 took an approach to reuse\n the \"--patch\" machinery even for \"--raw\" and other modes, but it\n would mean that \"diff --raw --exit-code\" may exit with 0 even\n when it has different paths to report, which is confusing. v3\n changes the approach to align the exit status with the presence\n of reported paths that are different better. \"--raw\" has ignored\n \"-w\" when producing its output. It should ignore \"-w\" when\n reporting differences with its exit code, instead of always\n exiting with 0.\n\nJunio C Hamano (5):\n diff: move the fallback \"--exit-code\" code down\n diff: mode-only change should be noticed by \"--patch -w --exit-code\"\n diff: teach \"--stat -w --exit-code\" to notice differences\n t4040: remove test that succeeded for a wrong reason\n diff: the -w option breaks --exit-code for --raw and other output\n modes\n\n diff.c | 40 ++++++++++++++++++++++--------------\n t/t4015-diff-whitespace.sh | 39 ++++++++++++++++++++++++++++++++++-\n t/t4040-whitespace-status.sh | 3 +--\n 3 files changed, 64 insertions(+), 18 deletions(-)\n\nRange-diff against v2:\n1: 65ff4655a2 < -: ---------- diff: --dirstat leakfix\n2: 533f974c9b ! 1: df869ac550 diff: move the fallback \"--exit-code\" code down\n @@ Commit message\n diff: move the fallback \"--exit-code\" code down\n \n When \"--exit-code\" is asked and the code cannot just answer by\n - comparing the object names on both sides but need to inspect and\n + comparing the object names on both sides but needs to inspect and\n compare the contents, there are two ways that the result is found\n out.\n \n Some output modes, like \"--stat\" and \"--patch\", inherently have to\n - inspect the contents in order to _show_ the differences in the way\n + inspect the contents in order to show the differences in the way\n they do. The codepaths for these modes set the .found_changes bit\n as they compute what to show.\n \n However, other output modes do not need to inspect the contents to\n show the differences in the way they do. The most notable example\n - is \"--quiet\", which does not need to compute any output. When they\n - are asked to report \"--exit-code\", they run the codepaths for the\n - \"--patch\" output with their output redirected to \"/dev/null\", only\n - to set the .found_changes bit.\n + is \"--quiet\", which does not need to compute any output to show.\n + When they are asked to report \"--exit-code\", they run the codepaths\n + for the \"--patch\" output with their output redirected to \"/dev/null\",\n + only to set the .found_changes bit.\n \n Currently, this fallback invocation of \"--patch\" output is done\n after the \"--stat\" output format and its friends and before the\n3: 89338b9302 ! 2: b349ad5278 diff: mode-only change should be noticed by \"--patch -w --exit-code\"\n @@ Metadata\n ## Commit message ##\n diff: mode-only change should be noticed by \"--patch -w --exit-code\"\n \n - The codepath to notice the content-level changes, taking certaion\n + The codepath to notice the content-level changes, taking certain\n no-op changes like \"ignore whitespace\" into account, forgot that\n a mode-only change is still a change. This resulted in\n \n @@ t/t4015-diff-whitespace.sh: TEST_PASSES_SANITIZE_LEAK=true\n +\t\ttest_expect_code 1 git diff -w $opts --exit-code x\n +\t'\n +\n -+\ttest_expect_success \"status with $opts (mode differs)\" '\n ++\ttest_expect_success POSIXPERM \"status with $opts (mode differs)\" '\n +\t\ttest_when_finished \"git update-index --chmod=-x x\" &&\n +\t\techo foo >x &&\n +\t\tgit add x &&\n4: 8fc63f4a04 ! 3: be50b023a8 diff: teach \"--stat -w --exit-code\" to notice differences\n @@ Commit message\n \n $ git diff --stat -w --exit-code\n \n - for a change that does have an outout to exit with status 0.\n + for a change that does have an output to exit with status 0.\n \n Set the bit by inspecting the list of paths the diffstat output is\n given for (a mode-only change will still appear as a \"0-line added\n5: 200593e9e0 < -: ---------- diff: teach \"--name-status\" and friends to honor \"--exit-code -w\"\n-: ---------- > 4: d1a6fa7d17 t4040: remove test that succeeded for a wrong reason\n-: ---------- > 5: 573f810cce diff: the -w option breaks --exit-code for --raw and other output modes\n\nbase-commit: 83973981eb475ce90f829f8a5bd6ea99cd3bbd8e\n-- \n2.42.0-rc2-7-gf9972720e9\n\n"},{"id":"480800","messageId":"20230818235932.3253552-4-gitster@pobox.com","threadId":"60061","inReplyTo":"20230818235932.3253552-1-gitster@pobox.com","subject":"[PATCH v3 3/5] diff: teach \"--stat -w --exit-code\" to notice differences","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-18T23:59:30Z","receivedAt":"2023-08-19T00:00:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When options like \"-w\" is used while \"--exit-code\" option is in\neffect, instead of the usual \"do we have any filepair whose preimage\nand postimage have different ?\" check, we need to compare\nthe contents of the blobs, taking into account that certain changes\nare considered no-op.\n\nWith the previous step, we taught \"--patch\" codepath to set the\n.found_changes bit correctly, even for a change that only affects\nthe mode and not object. The \"--stat\" codepath, however, did not\nset the .found_changes bit at all. This lead to\n\n $ git diff --stat -w --exit-code\n\nfor a change that does have an output to exit with status 0.\n\nSet the bit by inspecting the list of paths the diffstat output is\ngiven for (a mode-only change will still appear as a \"0-line added\n0-line deleted\" change) to fix it.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 1 +\n t/t4015-diff-whitespace.sh | 2 +-\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 998d7ae20c..da965ff688 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6901,6 +6901,7 @@ void compute_diffstat(struct diff_options *options,\n \t\tif (check_pair_status(p))\n \t\t\tdiff_flush_stat(p, options, diffstat);\n \t}\n+\toptions->found_changes = !!diffstat->nr;\n }\n \n void diff_addremove(struct diff_options *options,\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 02731dccb9..230a89b951 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -11,7 +11,7 @@ TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-diff.sh\n \n-for opts in --patch --quiet -s\n+for opts in --patch --quiet -s --stat --shortstat --dirstat=lines\n do\n \n \ttest_expect_success \"status with $opts (different)\" '\n-- \n2.42.0-rc2-7-gf9972720e9\n\n"},{"id":"480801","messageId":"20230818235932.3253552-5-gitster@pobox.com","threadId":"60061","inReplyTo":"20230818235932.3253552-1-gitster@pobox.com","subject":"[PATCH v3 4/5] t4040: remove test that succeeded for a wrong reason","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-18T23:59:31Z","receivedAt":"2023-08-19T00:00:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"diff-tree -b --exit-code\" without \"--patch\" exits with 0 status,\nnot because it finds that the two input files are equivalent while\nignoring whitespaces, but because the implied \"--raw\" mode always\nexits with 0 when whitespace tweaking options like \"-b\" and \"-w\"\nare given, which is a long-standing bug.\n\nWe are about to fix it so that \"--raw\" and friends report the\ndifferences with the exit status (even though they ignore the\nwhitespace tweaking options when producing their output), which\nwill make this test, which succeeded for a wrong reason, start\nfailing. Remove it.\n\nSigned-off-by: Junio C Hamano \n---\n t/t4040-whitespace-status.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/t4040-whitespace-status.sh b/t/t4040-whitespace-status.sh\nindex e70e020ae9..eec3d73dc2 100755\n--- a/t/t4040-whitespace-status.sh\n+++ b/t/t4040-whitespace-status.sh\n@@ -28,8 +28,7 @@ test_expect_success 'diff-tree --exit-code' '\n \n test_expect_success 'diff-tree -b --exit-code' '\n \tgit diff -b --exit-code HEAD^ HEAD &&\n-\tgit diff-tree -b -p --exit-code HEAD^ HEAD &&\n-\tgit diff-tree -b --exit-code HEAD^ HEAD\n+\tgit diff-tree -b -p --exit-code HEAD^ HEAD\n '\n \n test_expect_success 'diff-index --cached --exit-code' '\n-- \n2.42.0-rc2-7-gf9972720e9\n\n"},{"id":"480802","messageId":"20230818235932.3253552-2-gitster@pobox.com","threadId":"60061","inReplyTo":"20230818235932.3253552-1-gitster@pobox.com","subject":"[PATCH v3 1/5] diff: move the fallback \"--exit-code\" code down","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-18T23:59:28Z","receivedAt":"2023-08-19T00:00:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When \"--exit-code\" is asked and the code cannot just answer by\ncomparing the object names on both sides but needs to inspect and\ncompare the contents, there are two ways that the result is found\nout.\n\nSome output modes, like \"--stat\" and \"--patch\", inherently have to\ninspect the contents in order to show the differences in the way\nthey do. The codepaths for these modes set the .found_changes bit\nas they compute what to show.\n\nHowever, other output modes do not need to inspect the contents to\nshow the differences in the way they do. The most notable example\nis \"--quiet\", which does not need to compute any output to show.\nWhen they are asked to report \"--exit-code\", they run the codepaths\nfor the \"--patch\" output with their output redirected to \"/dev/null\",\nonly to set the .found_changes bit.\n\nCurrently, this fallback invocation of \"--patch\" output is done\nafter the \"--stat\" output format and its friends and before the\n\"--patch\" and internal callback logic. Move it to the end of\nthe sequence to clarify the fallback status of this code block.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex d52db685f7..0ce678fc06 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6551,6 +6551,21 @@ void diff_flush(struct diff_options *options)\n \t\tseparator++;\n \t}\n \n+\tif (output_format & DIFF_FORMAT_PATCH) {\n+\t\tif (separator) {\n+\t\t\temit_diff_symbol(options, DIFF_SYMBOL_SEPARATOR, NULL, 0, 0);\n+\t\t\tif (options->stat_sep)\n+\t\t\t\t/* attach patch instead of inline */\n+\t\t\t\temit_diff_symbol(options, DIFF_SYMBOL_STAT_SEP,\n+\t\t\t\t\t\t NULL, 0, 0);\n+\t\t}\n+\n+\t\tdiff_flush_patch_all_file_pairs(options);\n+\t}\n+\n+\tif (output_format & DIFF_FORMAT_CALLBACK)\n+\t\toptions->format_callback(q, options, options->format_callback_data);\n+\n \tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n \t options->flags.exit_with_status &&\n \t options->flags.diff_from_contents) {\n@@ -6572,21 +6587,6 @@ void diff_flush(struct diff_options *options)\n \t\t}\n \t}\n \n-\tif (output_format & DIFF_FORMAT_PATCH) {\n-\t\tif (separator) {\n-\t\t\temit_diff_symbol(options, DIFF_SYMBOL_SEPARATOR, NULL, 0, 0);\n-\t\t\tif (options->stat_sep)\n-\t\t\t\t/* attach patch instead of inline */\n-\t\t\t\temit_diff_symbol(options, DIFF_SYMBOL_STAT_SEP,\n-\t\t\t\t\t\t NULL, 0, 0);\n-\t\t}\n-\n-\t\tdiff_flush_patch_all_file_pairs(options);\n-\t}\n-\n-\tif (output_format & DIFF_FORMAT_CALLBACK)\n-\t\toptions->format_callback(q, options, options->format_callback_data);\n-\n \tfor (i = 0; i < q->nr; i++)\n \t\tdiff_free_filepair(q->queue[i]);\n free_queue:\n-- \n2.42.0-rc2-7-gf9972720e9\n\n"},{"id":"480803","messageId":"20230818235932.3253552-6-gitster@pobox.com","threadId":"60061","inReplyTo":"20230818235932.3253552-1-gitster@pobox.com","subject":"[PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-18T23:59:32Z","receivedAt":"2023-08-19T00:00:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The output from \"--raw\", \"--name-status\", and \"--name-only\" modes in\n\"git diff\" does depend on and does not reflect how certain different\ncontents are considered equal, unlike \"--patch\" and \"--stat\" output\nmodes do, when used with options like \"-w\" (another way of thinking\nabout it is that it is not like we recompute the hash of the blob\nafter removing all whitespaces to show \"git diff --raw -w\" output).\n\nBut that is not an excuse for \"git diff --exit-code --raw\" to fail\nto report differences with its exit status, when used with options\nlike \"-w\". Make sure the command exits with 1 when these options\nreport paths that are different.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 6 ++++++\n t/t4015-diff-whitespace.sh | 8 ++++++--\n 2 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex da965ff688..78f4e7518f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4744,6 +4744,10 @@ void diff_setup_done(struct diff_options *options)\n \telse\n \t\toptions->prefix_length = 0;\n \n+\t/*\n+\t * --name-only, --name-status, --checkdiff, and -s\n+\t * turn other output format off.\n+\t */\n \tif (options->output_format & (DIFF_FORMAT_NAME |\n \t\t\t\t DIFF_FORMAT_NAME_STATUS |\n \t\t\t\t DIFF_FORMAT_CHECKDIFF |\n@@ -6072,6 +6076,8 @@ static void flush_one_pair(struct diff_filepair *p, struct diff_options *opt)\n \t\tfprintf(opt->file, \"%s\", diff_line_prefix(opt));\n \t\twrite_name_quoted(name_a, opt->file, opt->line_termination);\n \t}\n+\n+\topt->found_changes = 1;\n }\n \n static void show_file_mode_name(struct diff_options *opt, const char *newdelete, struct diff_filespec *fs)\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 230a89b951..7fcffa4b11 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -11,8 +11,12 @@ TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-diff.sh\n \n-for opts in --patch --quiet -s --stat --shortstat --dirstat=lines\n+for opt_res in --patch --quiet -s --stat --shortstat --dirstat=lines \\\n+\t --raw! --name-only! --name-status!\n do\n+\topts=${opt_res%!} expect_failure=\n+\ttest \"$opts\" = \"$opt_res\" ||\n+\t\texpect_failure=\"test_expect_code 1\"\n \n \ttest_expect_success \"status with $opts (different)\" '\n \t\techo foo >x &&\n@@ -40,7 +44,7 @@ do\n \t\techo foo >x &&\n \t\tgit add x &&\n \t\techo \" foo\" >x &&\n-\t\tgit diff -w $opts --exit-code x\n+\t\t$expect_failure git diff -w $opts --exit-code x\n \t'\n done\n \n-- \n2.42.0-rc2-7-gf9972720e9\n\n"},{"id":"480875","messageId":"20230821204106.GB1800481@coredump.intra.peff.net","threadId":"60061","inReplyTo":"20230818235932.3253552-2-gitster@pobox.com","subject":"Re: [PATCH v3 1/5] diff: move the fallback \"--exit-code\" code down","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T20:41:06Z","receivedAt":"2023-08-21T20:42:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 18, 2023 at 04:59:28PM -0700, Junio C Hamano wrote:\n\n> diff --git a/diff.c b/diff.c\n> index d52db685f7..0ce678fc06 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6551,6 +6551,21 @@ void diff_flush(struct diff_options *options)\n> \t\tseparator++;\n> \t}\n> \n> +\tif (output_format & DIFF_FORMAT_PATCH) {\n> +\t\tif (separator) {\n\nThis step makes sense, but I did a double-take when looking at the\npatch, because it is moving code _up_ rather than _down_. But the\nproblem is that the block you moved was larger than the intervening bit,\nso the diff chose to flip-flop the context and changed bits.\n\nObviously orthogonal to your series, but I wonder if there's a way to\nconvince Git to show what actually happened. I don't think this is\nreally a heuristic or algorithm problem. Seeing the pre- and\npost-images, it can't know whether it was \"move A up\" or \"move B down\",\nand the \"real\" diff is simply much larger in this case.\n\n-Peff\n"},{"id":"480876","messageId":"20230821204552.GC1800481@coredump.intra.peff.net","threadId":"60061","inReplyTo":"20230818235932.3253552-4-gitster@pobox.com","subject":"Re: [PATCH v3 3/5] diff: teach \"--stat -w --exit-code\" to notice differences","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T20:45:52Z","receivedAt":"2023-08-21T20:46:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 18, 2023 at 04:59:30PM -0700, Junio C Hamano wrote:\n\n> Set the bit by inspecting the list of paths the diffstat output is\n> given for (a mode-only change will still appear as a \"0-line added\n> 0-line deleted\" change) to fix it.\n> [...]\n> diff --git a/diff.c b/diff.c\n> index 998d7ae20c..da965ff688 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6901,6 +6901,7 @@ void compute_diffstat(struct diff_options *options,\n> \t\tif (check_pair_status(p))\n> \t\t\tdiff_flush_stat(p, options, diffstat);\n> \t}\n> +\toptions->found_changes = !!diffstat->nr;\n> }\n\nMakes sense, and this is delightfully simple. I wondered if we needed to\nremove any code setting found_changes via builtin_diffstat(), but it\ndoes not seem to exist in the first place. ;)\n\n> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n> index 02731dccb9..230a89b951 100755\n> --- a/t/t4015-diff-whitespace.sh\n> +++ b/t/t4015-diff-whitespace.sh\n> @@ -11,7 +11,7 @@ TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n> . \"$TEST_DIRECTORY\"/lib-diff.sh\n> \n> -for opts in --patch --quiet -s\n> +for opts in --patch --quiet -s --stat --shortstat --dirstat=lines\n> do\n\nI guess this would fix --numstat, too, though there may be diminishing\nreturns in checking every format if we know it is just hitting the same\ncode paths (though perhaps one could argue that --shortstat is already\nuninteresting for the same reason).\n\n-Peff\n"},{"id":"480878","messageId":"20230821210058.GD1800481@coredump.intra.peff.net","threadId":"60061","inReplyTo":"20230818235932.3253552-6-gitster@pobox.com","subject":"Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T21:00:58Z","receivedAt":"2023-08-21T21:01:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 18, 2023 at 04:59:32PM -0700, Junio C Hamano wrote:\n\n> The output from \"--raw\", \"--name-status\", and \"--name-only\" modes in\n> \"git diff\" does depend on and does not reflect how certain different\n> contents are considered equal, unlike \"--patch\" and \"--stat\" output\n> modes do, when used with options like \"-w\" (another way of thinking\n> about it is that it is not like we recompute the hash of the blob\n> after removing all whitespaces to show \"git diff --raw -w\" output).\n> \n> But that is not an excuse for \"git diff --exit-code --raw\" to fail\n> to report differences with its exit status, when used with options\n> like \"-w\". Make sure the command exits with 1 when these options\n> report paths that are different.\n\nI think s/with options like/without options like/ in the final\nparagraph?\n\nI like this approach much better than the earlier round. It brings the\noutput and exit code of \"--raw\", etc, into harmony. I am open to the\nargument that if you ask for \"--raw -w\", we should start doing\ncontent-level comparisons (since otherwise the \"-w\" is somewhat\nuseless). But even if we went that route, it should affect both the\noutput and the exit code. So this patch would be a prerequisite anyway.\n\nAnd I say \"I am open to\" and not \"I think it is a good idea\" above,\nbecause I suspect there are many lurking corner cases. The hash output\nyou mentioned is one. But also, what \"--raw --patch -w\" does right now\nis sensible (show the raw output, but also show the whitespace-ignoring\npatch), and would be impossible if \"-w\" affected \"--raw\".\n\n> diff --git a/diff.c b/diff.c\n> index da965ff688..78f4e7518f 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4744,6 +4744,10 @@ void diff_setup_done(struct diff_options *options)\n> \telse\n> \t\toptions->prefix_length = 0;\n> \n> +\t/*\n> +\t * --name-only, --name-status, --checkdiff, and -s\n> +\t * turn other output format off.\n> +\t */\n> \tif (options->output_format & (DIFF_FORMAT_NAME |\n> \t\t\t\t DIFF_FORMAT_NAME_STATUS |\n> \t\t\t\t DIFF_FORMAT_CHECKDIFF |\n\nOK, this hunk is just documenting the current state, not changing\nanything.\n\n> @@ -6072,6 +6076,8 @@ static void flush_one_pair(struct diff_filepair *p, struct diff_options *opt)\n> \t\tfprintf(opt->file, \"%s\", diff_line_prefix(opt));\n> \t\twrite_name_quoted(name_a, opt->file, opt->line_termination);\n> \t}\n> +\n> +\topt->found_changes = 1;\n> }\n\nThis seems deceptively simple. :) The key here is that flush_one_pair()\nis called only for the formats that are otherwise broken (because they\nare not looking at the content). I think this is a good spot to put the\nfix, as any similar formats would hopefully get added to the conditional\nin diff_flush() that puts us here.\n\nAlternatively, we could put it in the caller, like so:\n\ndiff --git a/diff.c b/diff.c\nindex 78f4e7518f..e7281e75eb 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6528,6 +6528,7 @@ void diff_flush(struct diff_options *options)\n \t\t\tif (check_pair_status(p))\n \t\t\t\tflush_one_pair(p, options);\n \t\t}\n+\t\toptions->found_changes = !!q->nr;\n \t\tseparator++;\n \t}\n \n\nwhich matches the diffstat technique (I almost thought we could share\nthe code, but for the diffstat we are counting what ends up in the\ndiffstat struct; it does not clean out the original diff_queue when it\nsees a noop pair).\n\nI don't see a real reason to prefer one over the other.\n\n> -for opts in --patch --quiet -s --stat --shortstat --dirstat=lines\n> +for opt_res in --patch --quiet -s --stat --shortstat --dirstat=lines \\\n> +\t --raw! --name-only! --name-status!\n> do\n> +\topts=${opt_res%!} expect_failure=\n> +\ttest \"$opts\" = \"$opt_res\" ||\n> +\t\texpect_failure=\"test_expect_code 1\"\n\nCute \"!\" convention. :)\n\n-Peff\n"},{"id":"480879","messageId":"20230821210250.GE1800481@coredump.intra.peff.net","threadId":"60061","inReplyTo":"20230818235932.3253552-1-gitster@pobox.com","subject":"Re: [PATCH v3 0/5] fix interactions with \"-w\" and \"--exit-code\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T21:02:50Z","receivedAt":"2023-08-21T21:02:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 18, 2023 at 04:59:27PM -0700, Junio C Hamano wrote:\n\n> Paul Watson reported that \"diff --no-index --exit-code\n> --ignore-all-space\" does not work when used with \"--shortstat\". It\n> turns out that this is not limited to \"--no-index\" mode, and it is\n> not limited to \"--shortstat\". Anything that does not use the \"--patch\"\n> machinery to discover the content level differences ignored --exit-code\n> when used with options like \"-w\" and always exited with 0. In fact,\n> even the \"--patch\" machinery was slightly faulty in corner cases.\n> \n> And here is another round to fix it.\n> Previous one is at https://lore.kernel.org/git/20230817222949.3835424-1-gitster@pobox.com/\n\nThis looks good to me. I left some musings, but I don't think there is\nanything that merits a re-roll (you might want to fix-up a commit\nmessage typo locally).\n\nThanks for running with it. It's much nicer than v1. :)\n\n-Peff\n"},{"id":"480880","messageId":"20230821210804.GA1809621@coredump.intra.peff.net","threadId":"60061","inReplyTo":"20230821210058.GD1800481@coredump.intra.peff.net","subject":"Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T21:08:04Z","receivedAt":"2023-08-21T21:08:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 21, 2023 at 05:00:58PM -0400, Jeff King wrote:\n\n> Alternatively, we could put it in the caller, like so:\n> \n> diff --git a/diff.c b/diff.c\n> index 78f4e7518f..e7281e75eb 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6528,6 +6528,7 @@ void diff_flush(struct diff_options *options)\n> \t\t\tif (check_pair_status(p))\n> \t\t\t\tflush_one_pair(p, options);\n> \t\t}\n> +\t\toptions->found_changes = !!q->nr;\n> \t\tseparator++;\n> \t}\n> \n> \n> which matches the diffstat technique (I almost thought we could share\n> the code, but for the diffstat we are counting what ends up in the\n> diffstat struct; it does not clean out the original diff_queue when it\n> sees a noop pair).\n> \n> I don't see a real reason to prefer one over the other.\n\nActually, on second look these are not quite the same. Yours only\ntriggers if check_pair_status() is true. So something like\n--diff-filter should affect both output and exit code. Yours gets that\nright, and mine does not. Sorry for the noise. :)\n\n-Peff\n"},{"id":"480906","messageId":"xmqqttssnir4.fsf@gitster.g","threadId":"60061","inReplyTo":"20230821210058.GD1800481@coredump.intra.peff.net","subject":"Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-21T22:16:47Z","receivedAt":"2023-08-21T22:16:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Alternatively, we could put it in the caller, like so:\n>\n> diff --git a/diff.c b/diff.c\n> index 78f4e7518f..e7281e75eb 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6528,6 +6528,7 @@ void diff_flush(struct diff_options *options)\n> \t\t\tif (check_pair_status(p))\n> \t\t\t\tflush_one_pair(p, options);\n> \t\t}\n> +\t\toptions->found_changes = !!q->nr;\n> \t\tseparator++;\n> \t}\n\nYup, I suspect they amount to the same thing in practice, but I\ncouldn't come up with a good explanation to give casual readers of\nthe conditional call to flush_one_pair() a few lines above why\nthis is correct.\n\n"},{"id":"480907","messageId":"20230821222359.GA1813553@coredump.intra.peff.net","threadId":"60061","inReplyTo":"20230821210804.GA1809621@coredump.intra.peff.net","subject":"Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T22:23:59Z","receivedAt":"2023-08-21T22:24:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 21, 2023 at 05:08:05PM -0400, Jeff King wrote:\n\n> On Mon, Aug 21, 2023 at 05:00:58PM -0400, Jeff King wrote:\n> \n> > Alternatively, we could put it in the caller, like so:\n> > \n> > diff --git a/diff.c b/diff.c\n> > index 78f4e7518f..e7281e75eb 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -6528,6 +6528,7 @@ void diff_flush(struct diff_options *options)\n> > \t\t\tif (check_pair_status(p))\n> > \t\t\t\tflush_one_pair(p, options);\n> > \t\t}\n> > +\t\toptions->found_changes = !!q->nr;\n> > \t\tseparator++;\n> > \t}\n> > \n> > \n> > which matches the diffstat technique (I almost thought we could share\n> > the code, but for the diffstat we are counting what ends up in the\n> > diffstat struct; it does not clean out the original diff_queue when it\n> > sees a noop pair).\n> > \n> > I don't see a real reason to prefer one over the other.\n> \n> Actually, on second look these are not quite the same. Yours only\n> triggers if check_pair_status() is true. So something like\n> --diff-filter should affect both output and exit code. Yours gets that\n> right, and mine does not. Sorry for the noise. :)\n\nSorry, I'm dumb again. That is not where diff-filter is handled (it is\nhandled by diffcore, duh). check_pair_status() is only checking for\nDIFF_STATUS_UNKNOWN. I'm not sure when that would ever be set, but it\nseems like we should be matching the \"if it is output\" logic, which is\nwhat you get by calling flush_one_pair(). So yours is definitely\npreferable, even if I don't understand the possible differences. ;)\n\n-Peff\n"},{"id":"480908","messageId":"xmqqcyzgniaq.fsf@gitster.g","threadId":"60061","inReplyTo":"20230821210058.GD1800481@coredump.intra.peff.net","subject":"Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-21T22:26:37Z","receivedAt":"2023-08-21T22:26:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> ...\n>> about it is that it is not like we recompute the hash of the blob\n>> after removing all whitespaces to show \"git diff --raw -w\" output).\n>> \n>> But that is not an excuse for \"git diff --exit-code --raw\" to fail\n>> to report differences with its exit status, when used with options\n>> like \"-w\". Make sure the command exits with 1 when these options\n>> report paths that are different.\n>\n> I think s/with options like/without options like/ in the final\n> paragraph?\n\nSorry, I am confused. \"diff --raw --exit-code\" without \"-w\" reports\nwith its exit status that it found differences just fine. When \"-w\"\nis given, without this patch, it always reports 0.\n\nWhat I wanted to convey was ...\n\n \"--raw\" and friends deliberately ignore \"-w\" and other \"look at\n contents\" options.\n\n The fact they do ignore the \"look at contents\" options is not a\n good excuse for \"diff --raw -w --exit-code\" to also ignore the\n request to report the differences with its exit status. \"diff\n --raw --exit-code\" does report the differences as requested, and\n we should do the same when given \"-w\".\n\nI guess the two have no logical connection so the latter sentence is\nnot making sense, with its \"with\" kept as-is or changed to \"without\".\n\n"},{"id":"480911","messageId":"20230822013016.GA1815572@coredump.intra.peff.net","threadId":"60061","inReplyTo":"xmqqcyzgniaq.fsf@gitster.g","subject":"Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-22T01:30:16Z","receivedAt":"2023-08-22T01:30:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 21, 2023 at 03:26:37PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> ...\n> >> about it is that it is not like we recompute the hash of the blob\n> >> after removing all whitespaces to show \"git diff --raw -w\" output).\n> >> \n> >> But that is not an excuse for \"git diff --exit-code --raw\" to fail\n> >> to report differences with its exit status, when used with options\n> >> like \"-w\". Make sure the command exits with 1 when these options\n> >> report paths that are different.\n> >\n> > I think s/with options like/without options like/ in the final\n> > paragraph?\n> \n> Sorry, I am confused. \"diff --raw --exit-code\" without \"-w\" reports\n> with its exit status that it found differences just fine. When \"-w\"\n> is given, without this patch, it always reports 0.\n\nI think I just got confused by the multiple negatives \"not an excuse to\nfail to...\".\n\n> What I wanted to convey was ...\n> \n> \"--raw\" and friends deliberately ignore \"-w\" and other \"look at\n> contents\" options.\n> \n> The fact they do ignore the \"look at contents\" options is not a\n> good excuse for \"diff --raw -w --exit-code\" to also ignore the\n> request to report the differences with its exit status. \"diff\n> --raw --exit-code\" does report the differences as requested, and\n> we should do the same when given \"-w\".\n\nYes, that conveys it more clearly (to me, anyway).\n\n-Peff\n"}]}