{"thread":{"id":"64458","subject":"diff --cached --no-ext-diff --find-copies-harder --quiet exits with wrong status code","startedAt":"2025-11-08T19:05:59Z","lastAt":"2025-11-23T07:09:25Z","messageCount":15,"participants":["D. Ben Knoble","René Scharfe","Phillip Wood","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"530405","messageId":"CALnO6CBsj+aMvHJoUQ+LHAtXhcFhQeH8AuHyrX+rumur6MQQog@mail.gmail.com","threadId":"64458","inReplyTo":null,"subject":"diff --cached --no-ext-diff --find-copies-harder --quiet exits with wrong status code","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-11-08T19:05:47Z","receivedAt":"2025-11-08T19:05:59Z","isPatch":false,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"AFAICT, you need all of the mentioned options to trigger the bug.\nAllowing ext-diff works fine, I don't think it's triggered in\nnon-cached diffs, and I've never seen it without --find-copies-harder.\nNotably, s/quiet/exit-code works just fine.\n\nHere's a repro from git.git:\n\n    cp git{,1}.c\n    git add git1.c\n    git diff --cached --no-ext-diff --quiet --find-copies-harder &&\necho 'this should exit 1!'\n\n(And of course, ^quiet^exit-code if your shell supports it yields a\ndifferent outcome)\n\nContext: my distro applies a patch that allows\ndiff.renames=copies-harder. In a repo with that turned on,\ngit-prompt.sh stopped showing some staged changes.  Turns out it runs\ngit diff with all these flags (less --find-copies-harder, which is\nenabled by the config option). I _have_ confirmed this bug exists in\nunpatched Git, however.\n\nSome rough debugging notes: when entering diffcore_std (or\ndiffcore_rename_extended's cleanup loop):\n- for exit-code, diff_queued_diff.nr matches \"git ls-files :/ | wc -l\"\n- for quiet, it's just 1 (the first file listed by git ls-files :/, AFAICT)\nThe only other obvious difference I spotted is that the \"quick\" flag\nis turned on for quiet, which makes sense.\n\nI tried to figure out who builds the queue, and it looks like it's\ndiff_cache -> unpack_trees -> ..., where at some point for exit-code\nwe _keep_ queuing files, but for quiet we don't.\n\nI also saw diff_cache tweaks opts.diff_index_cached based on\n--find-copies-harder, but I haven't looked further to see how that\naffects things or what the interaction with ext-diff is.\n\nHelp welcome, thanks.\n\n-- \nD. Ben Knoble\n"},{"id":"530406","messageId":"CALnO6CBsXEpeCTS=FpcOvXEOw7sNGT8fdb4Z8PBewoW0iRMnXQ@mail.gmail.com","threadId":"64458","inReplyTo":"CALnO6CBsj+aMvHJoUQ+LHAtXhcFhQeH8AuHyrX+rumur6MQQog@mail.gmail.com","subject":"Re: diff --cached --no-ext-diff --find-copies-harder --quiet exits with wrong status code","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-11-08T19:08:35Z","receivedAt":"2025-11-08T19:08:48Z","isPatch":false,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Sat, Nov 8, 2025 at 2:05 PM D. Ben Knoble <ben.knoble@gmail.com> wrote:\n>\n> AFAICT, you need all of the mentioned options to trigger the bug.\n> Allowing ext-diff works fine, I don't think it's triggered in\n> non-cached diffs, and I've never seen it without --find-copies-harder.\n> Notably, s/quiet/exit-code works just fine.\n>\n> Here's a repro from git.git:\n>\n>     cp git{,1}.c\n>     git add git1.c\n>     git diff --cached --no-ext-diff --quiet --find-copies-harder &&\n> echo 'this should exit 1!'\n>\n> (And of course, ^quiet^exit-code if your shell supports it yields a\n> different outcome)\n>\n> Context: my distro applies a patch that allows\n> diff.renames=copies-harder. In a repo with that turned on,\n> git-prompt.sh stopped showing some staged changes.  Turns out it runs\n> git diff with all these flags (less --find-copies-harder, which is\n> enabled by the config option). I _have_ confirmed this bug exists in\n> unpatched Git, however.\n>\n> Some rough debugging notes: when entering diffcore_std (or\n> diffcore_rename_extended's cleanup loop):\n> - for exit-code, diff_queued_diff.nr matches \"git ls-files :/ | wc -l\"\n> - for quiet, it's just 1 (the first file listed by git ls-files :/, AFAICT)\n> The only other obvious difference I spotted is that the \"quick\" flag\n> is turned on for quiet, which makes sense.\n\nAh, woops. The reason this matters is that, after diff_rename, in the\ncorrect version the queue is non-empty and has_changes gets set, which\nplays into diff_result_code. In the broken version, the resulting\nqueue ends up empty, so has_changes is _reset_ to 0 (despite\npreviously being 1?)\n\nI think I also spotted a difference in diff_from_contents, but not\nsure if that's relevant.\n\n\n-- \nD. Ben Knoble\n"},{"id":"530407","messageId":"CALnO6CA187833M7SFDTrbSaTOpo5vSt3UGUFEiLGpiJnk_ekmg@mail.gmail.com","threadId":"64458","inReplyTo":"CALnO6CBsXEpeCTS=FpcOvXEOw7sNGT8fdb4Z8PBewoW0iRMnXQ@mail.gmail.com","subject":"Re: diff --cached --no-ext-diff --find-copies-harder --quiet exits with wrong status code","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-11-08T19:12:12Z","receivedAt":"2025-11-08T19:12:23Z","isPatch":false,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Sat, Nov 8, 2025 at 2:08 PM D. Ben Knoble <ben.knoble@gmail.com> wrote:\n>\n> On Sat, Nov 8, 2025 at 2:05 PM D. Ben Knoble <ben.knoble@gmail.com> wrote:\n> >\n> > AFAICT, you need all of the mentioned options to trigger the bug.\n> > Allowing ext-diff works fine, I don't think it's triggered in\n> > non-cached diffs, and I've never seen it without --find-copies-harder.\n> > Notably, s/quiet/exit-code works just fine.\n>\n>\n> I think I also spotted a difference in diff_from_contents, but not\n> sure if that's relevant.\n\nYeesh. You know how writing for others clarifies thoughts? Well...\n\nI just noticed that diff_setup_done tweaks diff_from_contents based on\nwhether external diffs are allowed. Possibly relevant? I haven't been\nable to easily identify a place where all 3 relevant options come\ntogether, but this would be 2 of them (quiet and ext-diff).\n\n-- \nD. Ben Knoble\n"},{"id":"530424","messageId":"bbd1a371-b0a4-4412-b329-cb4d654a0ca8@web.de","threadId":"64458","inReplyTo":"CALnO6CBsj+aMvHJoUQ+LHAtXhcFhQeH8AuHyrX+rumur6MQQog@mail.gmail.com","subject":"[PATCH] diff: disabled quick optimization with --find-copies-harder","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-11-09T12:11:39Z","receivedAt":"2025-11-09T12:11:48Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If --find-copies-harder is given, diff-lib.c::show_modified() queues\neven non-modified entries using diff_change() because we need them for\ncopy detection.  diff_change() sets flags.has_changes, though.  If\n--quiet is also given this causes diff_can_quit_early() to declare we're\ndone after seeing only the very first entry, which is way too soon.\nDisable this optimization in that case.\n\nThis issue is hidden without --no-ext-diff because then we set\nflags.diff_from_contents, which disables the optimization in a\ndifferent way.\n\nReported-by: D. Ben Knoble <ben.knoble@gmail.com>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n diff.c              |  1 +\n t/t4007-rename-3.sh | 10 ++++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex a1961526c0..84ac148c37 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -7188,6 +7188,7 @@ int diff_can_quit_early(struct diff_options *opt)\n {\n \treturn (opt->flags.quick &&\n \t\t!opt->filter &&\n+\t\t!opt->flags.find_copies_harder &&\n \t\topt->flags.has_changes);\n }\n \ndiff --git a/t/t4007-rename-3.sh b/t/t4007-rename-3.sh\nindex e8faf0dd2e..3fc81bcd76 100755\n--- a/t/t4007-rename-3.sh\n+++ b/t/t4007-rename-3.sh\n@@ -41,6 +41,16 @@ test_expect_success 'copy detection, cached' '\n \tcompare_diff_raw current expected\n '\n \n+test_expect_success 'exit code of quiet copy detection' '\n+\ttest_expect_code 1 \\\n+\tgit diff --quiet --cached --find-copies-harder $tree\n+'\n+\n+test_expect_success 'exit code of quiet copy detection with --no-ext-diff' '\n+\ttest_expect_code 1 \\\n+\tgit diff --quiet --cached --find-copies-harder --no-ext-diff $tree\n+'\n+\n # In the tree, there is only path0/COPYING.  In the cache, path0 and\n # path1 both have COPYING and the latter is a copy of path0/COPYING.\n # However when we say we care only about path1, we should just see\n-- \n2.51.2\n"},{"id":"530426","messageId":"40a322a6-7fd7-4003-a23f-7672a36b5bf2@gmail.com","threadId":"64458","inReplyTo":"bbd1a371-b0a4-4412-b329-cb4d654a0ca8@web.de","subject":"Re: [PATCH] diff: disabled quick optimization with --find-copies-harder","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-11-09T14:18:39Z","receivedAt":"2025-11-09T14:18:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 09/11/2025 12:11, René Scharfe wrote:\n> If --find-copies-harder is given, diff-lib.c::show_modified() queues\n> even non-modified entries using diff_change() because we need them for\n> copy detection.  diff_change() sets flags.has_changes, though.  If\n> --quiet is also given this causes diff_can_quit_early() to declare we're\n> done after seeing only the very first entry, which is way too soon.\n> Disable this optimization in that case.\n\nStepping back a bit I'm confused as to why we don't disable rename and \ncopy detection when \"--quiet\" is given. I can't see why detecting copies \nor renames would change the exit code but maybe I'm missing something.\n\nThanks\n\nPhillip\n\n> This issue is hidden without --no-ext-diff because then we set\n> flags.diff_from_contents, which disables the optimization in a\n> different way.\n> \n> Reported-by: D. Ben Knoble <ben.knoble@gmail.com>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>   diff.c              |  1 +\n>   t/t4007-rename-3.sh | 10 ++++++++++\n>   2 files changed, 11 insertions(+)\n> \n> diff --git a/diff.c b/diff.c\n> index a1961526c0..84ac148c37 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -7188,6 +7188,7 @@ int diff_can_quit_early(struct diff_options *opt)\n>   {\n>   \treturn (opt->flags.quick &&\n>   \t\t!opt->filter &&\n> +\t\t!opt->flags.find_copies_harder &&\n>   \t\topt->flags.has_changes);\n>   }\n>   \n> diff --git a/t/t4007-rename-3.sh b/t/t4007-rename-3.sh\n> index e8faf0dd2e..3fc81bcd76 100755\n> --- a/t/t4007-rename-3.sh\n> +++ b/t/t4007-rename-3.sh\n> @@ -41,6 +41,16 @@ test_expect_success 'copy detection, cached' '\n>   \tcompare_diff_raw current expected\n>   '\n>   \n> +test_expect_success 'exit code of quiet copy detection' '\n> +\ttest_expect_code 1 \\\n> +\tgit diff --quiet --cached --find-copies-harder $tree\n> +'\n> +\n> +test_expect_success 'exit code of quiet copy detection with --no-ext-diff' '\n> +\ttest_expect_code 1 \\\n> +\tgit diff --quiet --cached --find-copies-harder --no-ext-diff $tree\n> +'\n> +\n>   # In the tree, there is only path0/COPYING.  In the cache, path0 and\n>   # path1 both have COPYING and the latter is a copy of path0/COPYING.\n>   # However when we say we care only about path1, we should just see\n\n"},{"id":"530427","messageId":"2f47defa-1164-437a-b81b-294c7fddabc8@web.de","threadId":"64458","inReplyTo":"40a322a6-7fd7-4003-a23f-7672a36b5bf2@gmail.com","subject":"Re: [PATCH] diff: disabled quick optimization with --find-copies-harder","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-11-09T16:43:22Z","receivedAt":"2025-11-09T16:43:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 11/9/25 3:18 PM, Phillip Wood wrote:\n> \n> Stepping back a bit I'm confused as to why we don't disable rename\n> and copy detection when \"--quiet\" is given. I can't see why\n> detecting copies or renames would change the exit code but maybe I'm\n> missing something.\n\nExcellent question!  I also can't think of a reason.\n\nRené\n\n"},{"id":"530428","messageId":"8796cd59-2335-4674-823d-d682ce7b7f8e@web.de","threadId":"64458","inReplyTo":"CALnO6CBsj+aMvHJoUQ+LHAtXhcFhQeH8AuHyrX+rumur6MQQog@mail.gmail.com","subject":"[PATCH v2] diff: disable rename detection with --quiet","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-11-09T16:43:36Z","receivedAt":"2025-11-09T16:43:51Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Detecting renames and copies improves diff's output.  This effort is\nwasted if we don't show any.  Disable detection in that case.\n\nThis actually fixes the error code when using the options --cached,\n--find-copies-harder, --no-ext-diff and --quiet together:\nrun_diff_index() indirectly calls diff-lib.c::show_modified(), which\nqueues even non-modified entries using diff_change() because we need\nthem for copy detection.  diff_change() sets flags.has_changes, though,\nwhich causes diff_can_quit_early() to declare we're done after seeing\nonly the very first entry -- way too soon.\n\nUsing --cached, --find-copies-harder and --quiet together without\n--no-ext-diff was not affected even before, as it causes the flag\nflags.diff_from_contents to be set, which disables the optimization\nin a different way.\n\nReported-by: D. Ben Knoble <ben.knoble@gmail.com>\nSuggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n diff.c              |  2 ++\n t/t4007-rename-3.sh | 10 ++++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex a1961526c0..efa8d9773c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4987,6 +4987,8 @@ void diff_setup_done(struct diff_options *options)\n \tif (options->flags.quick) {\n \t\toptions->output_format = DIFF_FORMAT_NO_OUTPUT;\n \t\toptions->flags.exit_with_status = 1;\n+\t\toptions->detect_rename = 0;\n+\t\toptions->flags.find_copies_harder = 0;\n \t}\n \n \t/*\ndiff --git a/t/t4007-rename-3.sh b/t/t4007-rename-3.sh\nindex e8faf0dd2e..3fc81bcd76 100755\n--- a/t/t4007-rename-3.sh\n+++ b/t/t4007-rename-3.sh\n@@ -41,6 +41,16 @@ test_expect_success 'copy detection, cached' '\n \tcompare_diff_raw current expected\n '\n \n+test_expect_success 'exit code of quiet copy detection' '\n+\ttest_expect_code 1 \\\n+\tgit diff --quiet --cached --find-copies-harder $tree\n+'\n+\n+test_expect_success 'exit code of quiet copy detection with --no-ext-diff' '\n+\ttest_expect_code 1 \\\n+\tgit diff --quiet --cached --find-copies-harder --no-ext-diff $tree\n+'\n+\n # In the tree, there is only path0/COPYING.  In the cache, path0 and\n # path1 both have COPYING and the latter is a copy of path0/COPYING.\n # However when we say we care only about path1, we should just see\n-- \n2.51.2\n"},{"id":"530429","messageId":"CALnO6CC+ke1L7T+dO13B0FSjLyJqihKHKZaa-B4dh9guxk7z0Q@mail.gmail.com","threadId":"64458","inReplyTo":"8796cd59-2335-4674-823d-d682ce7b7f8e@web.de","subject":"Re: [PATCH v2] diff: disable rename detection with --quiet","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-11-09T17:34:54Z","receivedAt":"2025-11-09T17:35:06Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Sun, Nov 9, 2025 at 11:43 AM René Scharfe <l.s.r@web.de> wrote:\n>\n> Detecting renames and copies improves diff's output.  This effort is\n> wasted if we don't show any.  Disable detection in that case.\n\nIndeed. I've confirmed this and v1 both fix the issue, although v2 is\nsignificantly faster (which is great for the intended use in\ngit-prompt.sh!):\n\nλ hyperfine -NiP v 1 2 ~/code/git/'buildv{v}/git diff --cached --quiet\n--no-ext-diff --find-copies-harder'\nBenchmark 1: /home/benknoble/code/git/buildv1/git diff --cached\n--quiet --no-ext-diff --find-copies-harder\n  Time (mean ± σ):      72.0 ms ±   3.3 ms    [User: 45.2 ms, System: 26.2 ms]\n  Range (min … max):    67.6 ms …  79.6 ms    42 runs\n\n  Warning: Ignoring non-zero exit code.\n\nBenchmark 2: /home/benknoble/code/git/buildv2/git diff --cached\n--quiet --no-ext-diff --find-copies-harder\n  Time (mean ± σ):      19.9 ms ±   1.5 ms    [User: 8.9 ms, System: 10.6 ms]\n  Range (min … max):    16.1 ms …  24.0 ms    151 runs\n\n  Warning: Ignoring non-zero exit code.\n\nSummary\n  /home/benknoble/code/git/buildv2/git diff --cached --quiet\n--no-ext-diff --find-copies-harder ran\n    3.61 ± 0.31 times faster than /home/benknoble/code/git/buildv1/git\ndiff --cached --quiet --no-ext-diff --find-copies-harder\n\n> This actually fixes the error code when using the options --cached,\n> --find-copies-harder, --no-ext-diff and --quiet together:\n> run_diff_index() indirectly calls diff-lib.c::show_modified(), which\n> queues even non-modified entries using diff_change() because we need\n> them for copy detection.  diff_change() sets flags.has_changes, though,\n> which causes diff_can_quit_early() to declare we're done after seeing\n> only the very first entry -- way too soon.\n\nThis does describe the behavior I saw, but it seems to me that, if we\nhave changes, then we ought to be able to quit early for --quiet, no?\n\nSo there's some other knock-on effect that causes quitting early to be\nwrong here, and I'm not exactly sure what it is (other than the diff\nqueues being different sizes when we hit relevant parts of\ndiffcore_std, though it's the working case that has the larger queue).\nSo I'm having a hard time tying this paragraph to the actual issue\n(mostly due to my complete unfamiliarity with the diffing subsystem).\n\n> Using --cached, --find-copies-harder and --quiet together without\n> --no-ext-diff was not affected even before, as it causes the flag\n> flags.diff_from_contents to be set, which disables the optimization\n> in a different way.\n>\n> Reported-by: D. Ben Knoble <ben.knoble@gmail.com>\n> Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  diff.c              |  2 ++\n>  t/t4007-rename-3.sh | 10 ++++++++++\n>  2 files changed, 12 insertions(+)\n>\n> diff --git a/diff.c b/diff.c\n> index a1961526c0..efa8d9773c 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4987,6 +4987,8 @@ void diff_setup_done(struct diff_options *options)\n>         if (options->flags.quick) {\n>                 options->output_format = DIFF_FORMAT_NO_OUTPUT;\n>                 options->flags.exit_with_status = 1;\n> +               options->detect_rename = 0;\n> +               options->flags.find_copies_harder = 0;\n>         }\n>\n>         /*\n> diff --git a/t/t4007-rename-3.sh b/t/t4007-rename-3.sh\n> index e8faf0dd2e..3fc81bcd76 100755\n> --- a/t/t4007-rename-3.sh\n> +++ b/t/t4007-rename-3.sh\n> @@ -41,6 +41,16 @@ test_expect_success 'copy detection, cached' '\n>         compare_diff_raw current expected\n>  '\n>\n> +test_expect_success 'exit code of quiet copy detection' '\n> +       test_expect_code 1 \\\n> +       git diff --quiet --cached --find-copies-harder $tree\n> +'\n> +\n> +test_expect_success 'exit code of quiet copy detection with --no-ext-diff' '\n> +       test_expect_code 1 \\\n> +       git diff --quiet --cached --find-copies-harder --no-ext-diff $tree\n> +'\n> +\n>  # In the tree, there is only path0/COPYING.  In the cache, path0 and\n>  # path1 both have COPYING and the latter is a copy of path0/COPYING.\n>  # However when we say we care only about path1, we should just see\n> --\n> 2.51.2\n\nCovering both seems like the right move to me, thanks!\n\n--\nD. Ben Knoble\n"},{"id":"530430","messageId":"916cf3cc-185f-447d-845d-a65eddee4a36@web.de","threadId":"64458","inReplyTo":"CALnO6CC+ke1L7T+dO13B0FSjLyJqihKHKZaa-B4dh9guxk7z0Q@mail.gmail.com","subject":"Re: [PATCH v2] diff: disable rename detection with --quiet","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-11-09T18:35:12Z","receivedAt":"2025-11-09T18:35:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 11/9/25 6:34 PM, D. Ben Knoble wrote:\n> On Sun, Nov 9, 2025 at 11:43 AM René Scharfe <l.s.r@web.de> wrote:\n>>\n>> This actually fixes the error code when using the options --cached,\n>> --find-copies-harder, --no-ext-diff and --quiet together:\n>> run_diff_index() indirectly calls diff-lib.c::show_modified(), which\n>> queues even non-modified entries using diff_change() because we need\n>> them for copy detection.  diff_change() sets flags.has_changes, though,\n>> which causes diff_can_quit_early() to declare we're done after seeing\n>> only the very first entry -- way too soon.\n> \n> This does describe the behavior I saw, but it seems to me that, if we\n> have changes, then we ought to be able to quit early for --quiet, no?\n> \n> So there's some other knock-on effect that causes quitting early to be\n> wrong here, and I'm not exactly sure what it is (other than the diff\n> queues being different sizes when we hit relevant parts of\n> diffcore_std, though it's the working case that has the larger queue).\n> So I'm having a hard time tying this paragraph to the actual issue\n> (mostly due to my complete unfamiliarity with the diffing subsystem).\n\nrun_diff_index() calls diff-lib.c::diff_cache() to queue up index\nentries.  As mentioned above it only queues up the very first one, no\nmatter if it's a change or not.  In Git's repo this would be\n.cirrus.yml.  That's not the end of it, yet, though.  It then calls\ndiffcore_std(), which calls diffcore_rename() to remove non-changes\nfrom the queue and overwrites flags.has_changes based on whether the\nqueue is empty now.\n\nRené\n\n"},{"id":"530435","messageId":"61e6b077-26ef-49ed-83cf-fa5b7888429c@gmail.com","threadId":"64458","inReplyTo":"CALnO6CC+ke1L7T+dO13B0FSjLyJqihKHKZaa-B4dh9guxk7z0Q@mail.gmail.com","subject":"Re: [PATCH v2] diff: disable rename detection with --quiet","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-11-10T09:42:53Z","receivedAt":"2025-11-10T09:43:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ben\n\nOn 09/11/2025 17:34, D. Ben Knoble wrote:\n> On Sun, Nov 9, 2025 at 11:43 AM René Scharfe <l.s.r@web.de> wrote:\n>>\n> λ hyperfine -NiP v 1 2 ~/code/git/'buildv{v}/git diff --cached --quiet\n> --no-ext-diff --find-copies-harder'\n> Benchmark 1: /home/benknoble/code/git/buildv1/git diff --cached\n> --quiet --no-ext-diff --find-copies-harder\n>    Time (mean ± σ):      72.0 ms ±   3.3 ms    [User: 45.2 ms, System: 26.2 ms]\n>    Range (min … max):    67.6 ms …  79.6 ms    42 runs\n> \n>    Warning: Ignoring non-zero exit code.\n> \n> Benchmark 2: /home/benknoble/code/git/buildv2/git diff --cached\n> --quiet --no-ext-diff --find-copies-harder\n>    Time (mean ± σ):      19.9 ms ±   1.5 ms    [User: 8.9 ms, System: 10.6 ms]\n>    Range (min … max):    16.1 ms …  24.0 ms    151 runs\n> \n>    Warning: Ignoring non-zero exit code.\n> \n> Summary\n>    /home/benknoble/code/git/buildv2/git diff --cached --quiet\n> --no-ext-diff --find-copies-harder ran\n>      3.61 ± 0.31 times faster than /home/benknoble/code/git/buildv1/git\n> diff --cached --quiet --no-ext-diff --find-copies-harder\n\nThat's a nice speedup. Thanks for sharing that - I knew in an abstract \nway that \"--find-copies-harder\" slowed things down but seeing some \nconcrete numbers really brings it home.\n\nBest Wishes\n\nPhillip\n\n>> This actually fixes the error code when using the options --cached,\n>> --find-copies-harder, --no-ext-diff and --quiet together:\n>> run_diff_index() indirectly calls diff-lib.c::show_modified(), which\n>> queues even non-modified entries using diff_change() because we need\n>> them for copy detection.  diff_change() sets flags.has_changes, though,\n>> which causes diff_can_quit_early() to declare we're done after seeing\n>> only the very first entry -- way too soon.\n> \n> This does describe the behavior I saw, but it seems to me that, if we\n> have changes, then we ought to be able to quit early for --quiet, no?\n> \n> So there's some other knock-on effect that causes quitting early to be\n> wrong here, and I'm not exactly sure what it is (other than the diff\n> queues being different sizes when we hit relevant parts of\n> diffcore_std, though it's the working case that has the larger queue).\n> So I'm having a hard time tying this paragraph to the actual issue\n> (mostly due to my complete unfamiliarity with the diffing subsystem).\n> \n>> Using --cached, --find-copies-harder and --quiet together without\n>> --no-ext-diff was not affected even before, as it causes the flag\n>> flags.diff_from_contents to be set, which disables the optimization\n>> in a different way.\n>>\n>> Reported-by: D. Ben Knoble <ben.knoble@gmail.com>\n>> Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>>   diff.c              |  2 ++\n>>   t/t4007-rename-3.sh | 10 ++++++++++\n>>   2 files changed, 12 insertions(+)\n>>\n>> diff --git a/diff.c b/diff.c\n>> index a1961526c0..efa8d9773c 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -4987,6 +4987,8 @@ void diff_setup_done(struct diff_options *options)\n>>          if (options->flags.quick) {\n>>                  options->output_format = DIFF_FORMAT_NO_OUTPUT;\n>>                  options->flags.exit_with_status = 1;\n>> +               options->detect_rename = 0;\n>> +               options->flags.find_copies_harder = 0;\n>>          }\n>>\n>>          /*\n>> diff --git a/t/t4007-rename-3.sh b/t/t4007-rename-3.sh\n>> index e8faf0dd2e..3fc81bcd76 100755\n>> --- a/t/t4007-rename-3.sh\n>> +++ b/t/t4007-rename-3.sh\n>> @@ -41,6 +41,16 @@ test_expect_success 'copy detection, cached' '\n>>          compare_diff_raw current expected\n>>   '\n>>\n>> +test_expect_success 'exit code of quiet copy detection' '\n>> +       test_expect_code 1 \\\n>> +       git diff --quiet --cached --find-copies-harder $tree\n>> +'\n>> +\n>> +test_expect_success 'exit code of quiet copy detection with --no-ext-diff' '\n>> +       test_expect_code 1 \\\n>> +       git diff --quiet --cached --find-copies-harder --no-ext-diff $tree\n>> +'\n>> +\n>>   # In the tree, there is only path0/COPYING.  In the cache, path0 and\n>>   # path1 both have COPYING and the latter is a copy of path0/COPYING.\n>>   # However when we say we care only about path1, we should just see\n>> --\n>> 2.51.2\n> \n> Covering both seems like the right move to me, thanks!\n> \n> --\n> D. Ben Knoble\n> \n\n"},{"id":"530456","messageId":"20251110175408.GB76603@coredump.intra.peff.net","threadId":"64458","inReplyTo":"8796cd59-2335-4674-823d-d682ce7b7f8e@web.de","subject":"Re: [PATCH v2] diff: disable rename detection with --quiet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-10T17:54:08Z","receivedAt":"2025-11-10T17:54:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 09, 2025 at 05:43:36PM +0100, René Scharfe wrote:\n\n> Detecting renames and copies improves diff's output.  This effort is\n> wasted if we don't show any.  Disable detection in that case.\n> \n> This actually fixes the error code when using the options --cached,\n> --find-copies-harder, --no-ext-diff and --quiet together:\n> run_diff_index() indirectly calls diff-lib.c::show_modified(), which\n> queues even non-modified entries using diff_change() because we need\n> them for copy detection.  diff_change() sets flags.has_changes, though,\n> which causes diff_can_quit_early() to declare we're done after seeing\n> only the very first entry -- way too soon.\n> \n> Using --cached, --find-copies-harder and --quiet together without\n> --no-ext-diff was not affected even before, as it causes the flag\n> flags.diff_from_contents to be set, which disables the optimization\n> in a different way.\n\nThis makes sense to me, and I can't think of a reason why you would want\nrename detection on if we're not going to show the results (and likewise\nI can't think of a way that a rename result would affect has_changes).\n\nI wonder if we should _also_ take the hunk from v1 that teaches\ncan_quit_early() to avoid triggering when copy detection is on. It's\nprobably redundant now, but it feels to me like that's the place where\nthe correctness check should kick in. And the patch here is just\noptimizing out the unnecessary work, but also happens to align things\nfor correctness downstream.\n\nBut I dunno. Maybe a check for a condition that we think can never be\ntriggered becomes too confusing for later maintenance.\n\n\nYou don't say in the commit message when this bug started. I briefly\nwondered if it was caused by the recent diff_from_contents stuff we've\nbeen discussing. But it's the opposite here (the bug happens when we\n_don't_ set diff_from_contents). And I think it goes all the way back to\nb4194828dc (diff-index --quiet: learn the \"stop feeding the backend\nearly\" logic, 2011-05-31).\n\n-Peff\n"},{"id":"530466","messageId":"xmqqseelzong.fsf@gitster.g","threadId":"64458","inReplyTo":"20251110175408.GB76603@coredump.intra.peff.net","subject":"Re: [PATCH v2] diff: disable rename detection with --quiet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-10T19:13:55Z","receivedAt":"2025-11-10T19:13:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This makes sense to me, and I can't think of a reason why you would want\n> rename detection on if we're not going to show the results (and likewise\n> I can't think of a way that a rename result would affect has_changes).\n>\n> I wonder if we should _also_ take the hunk from v1 that teaches\n> can_quit_early() to avoid triggering when copy detection is on. It's\n> probably redundant now, but it feels to me like that's the place where\n> the correctness check should kick in. And the patch here is just\n> optimizing out the unnecessary work, but also happens to align things\n> for correctness downstream.\n\nConcurred on both counts.\n\n> You don't say in the commit message when this bug started. I briefly\n> wondered if it was caused by the recent diff_from_contents stuff we've\n> been discussing. But it's the opposite here (the bug happens when we\n> _don't_ set diff_from_contents). And I think it goes all the way back to\n> b4194828dc (diff-index --quiet: learn the \"stop feeding the backend\n> early\" logic, 2011-05-31).\n\nYup, I think so.  Back then I think our assumptions are that the\nuser knows better than giving complex diffcore requests like\nfind-copies-harder only to discard the results with --quiet, and the\npatch started this thread helps other users, which is good ;-).\n"},{"id":"530480","messageId":"CALnO6CDxz3eKFfJgG5dQF5sUutT_bRrH0itpLtmRj6cW_=WPBA@mail.gmail.com","threadId":"64458","inReplyTo":"916cf3cc-185f-447d-845d-a65eddee4a36@web.de","subject":"Re: [PATCH v2] diff: disable rename detection with --quiet","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-11-10T23:58:29Z","receivedAt":"2025-11-10T23:58:41Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Sun, Nov 9, 2025 at 1:35 PM René Scharfe <l.s.r@web.de> wrote:\n>\n> On 11/9/25 6:34 PM, D. Ben Knoble wrote:\n> > On Sun, Nov 9, 2025 at 11:43 AM René Scharfe <l.s.r@web.de> wrote:\n> >>\n> >> This actually fixes the error code when using the options --cached,\n> >> --find-copies-harder, --no-ext-diff and --quiet together:\n> >> run_diff_index() indirectly calls diff-lib.c::show_modified(), which\n> >> queues even non-modified entries using diff_change() because we need\n> >> them for copy detection.  diff_change() sets flags.has_changes, though,\n> >> which causes diff_can_quit_early() to declare we're done after seeing\n> >> only the very first entry -- way too soon.\n> >\n> > This does describe the behavior I saw, but it seems to me that, if we\n> > have changes, then we ought to be able to quit early for --quiet, no?\n> >\n> > So there's some other knock-on effect that causes quitting early to be\n> > wrong here, and I'm not exactly sure what it is (other than the diff\n> > queues being different sizes when we hit relevant parts of\n> > diffcore_std, though it's the working case that has the larger queue).\n> > So I'm having a hard time tying this paragraph to the actual issue\n> > (mostly due to my complete unfamiliarity with the diffing subsystem).\n>\n> run_diff_index() calls diff-lib.c::diff_cache() to queue up index\n> entries.  As mentioned above it only queues up the very first one, no\n> matter if it's a change or not.  In Git's repo this would be\n> .cirrus.yml.  That's not the end of it, yet, though.  It then calls\n> diffcore_std(), which calls diffcore_rename() to remove non-changes\n> from the queue and overwrites flags.has_changes based on whether the\n> queue is empty now.\n>\n> René\n>\n\nThanks, btw. Still try to absorb this part of the code, but this helps :)\n\n-- \nD. Ben Knoble\n"},{"id":"531164","messageId":"8cc12ef2-1d2c-4924-b130-bd740a975ce0@web.de","threadId":"64458","inReplyTo":"20251110175408.GB76603@coredump.intra.peff.net","subject":"Re: [PATCH v2] diff: disable rename detection with --quiet","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-11-22T21:44:59Z","receivedAt":"2025-11-22T21:45:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 11/10/25 6:54 PM, Jeff King wrote:\n> \n> I wonder if we should _also_ take the hunk from v1 that teaches\n> can_quit_early() to avoid triggering when copy detection is on. It's\n> probably redundant now, but it feels to me like that's the place where\n> the correctness check should kick in. And the patch here is just\n> optimizing out the unnecessary work, but also happens to align things\n> for correctness downstream.\n> \n> But I dunno. Maybe a check for a condition that we think can never be\n> triggered becomes too confusing for later maintenance.\nThat check was only necessary because we queue unchanged filepairs for\nrename detection as if they were changes.  With --quiet forcing rename\ndetection off we won't run into this anymore, but it still feels like a\ntrip hazard.  Adding a check would help, but we could also stop doing\nthat in the first place.  Patch below.\n\nRené\n\n\n--- >8 ---\nSubject: [PATCH] diff-index: don't queue unchanged filepairs with diff_change()\n\ndiff_cache() queues unchanged filepairs if the flag find_copies_harder\nis set, and uses diff_change() for that.  This function does a few\nthings that are unnecessary for unchanged filepairs and always sets the\ndiff_flag has_changes, which is simply misleading in this case.\n\nAdd a new streamlined function for queuing unchanged filepairs and\nuse it in show_modified(), which is called by diff_cache() via\noneway_diff() and do_oneway_diff().  It allocates only one half of each\nfilepair, which has a measurable effect if there are a lot of them, like\nin the Linux repo:\n\nBenchmark 1: ./git_v2.52.0 -C ../linux diff --cached --find-copies-harder\n  Time (mean ± σ):      31.8 ms ±   0.2 ms    [User: 24.2 ms, System: 6.3 ms]\n  Range (min … max):    31.5 ms …  32.3 ms    85 runs\n\nBenchmark 2: ./git -C ../linux diff --cached --find-copies-harder\n  Time (mean ± σ):      23.9 ms ±   0.2 ms    [User: 18.1 ms, System: 4.6 ms]\n  Range (min … max):    23.5 ms …  24.4 ms    111 runs\n\nSummary\n  ./git -C ../linux diff --cached --find-copies-harder ran\n    1.33 ± 0.01 times faster than ./git_v2.52.0 -C ../linux diff --cached --find-copies-harder\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n diff-lib.c | 13 ++++++-------\n diff.c     | 20 ++++++++++++++++++++\n diff.h     |  5 +++++\n 3 files changed, 31 insertions(+), 7 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex b8f8f3bc31..8e624f38c6 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -418,13 +418,12 @@ static int show_modified(struct rev_info *revs,\n \t}\n \n \toldmode = old_entry->ce_mode;\n-\tif (mode == oldmode && oideq(oid, &old_entry->oid) && !dirty_submodule &&\n-\t    !revs->diffopt.flags.find_copies_harder)\n-\t\treturn 0;\n-\n-\tdiff_change(&revs->diffopt, oldmode, mode,\n-\t\t    &old_entry->oid, oid, 1, !is_null_oid(oid),\n-\t\t    old_entry->name, 0, dirty_submodule);\n+\tif (mode != oldmode || !oideq(oid, &old_entry->oid) || dirty_submodule)\n+\t\tdiff_change(&revs->diffopt, oldmode, mode,\n+\t\t\t    &old_entry->oid, oid, 1, !is_null_oid(oid),\n+\t\t\t    old_entry->name, 0, dirty_submodule);\n+\telse if (revs->diffopt.flags.find_copies_harder)\n+\t\tdiff_same(&revs->diffopt, mode, oid, old_entry->name);\n \treturn 0;\n }\n \ndiff --git a/diff.c b/diff.c\nindex efa8d9773c..e2a2927f8c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -7349,6 +7349,26 @@ void diff_change(struct diff_options *options,\n \t\t\t  concatpath, old_dirty_submodule, new_dirty_submodule);\n }\n \n+void diff_same(struct diff_options *options,\n+\t       unsigned mode,\n+\t       const struct object_id *oid,\n+\t       const char *concatpath)\n+{\n+\tstruct diff_filespec *one;\n+\n+\tif (S_ISGITLINK(mode) && is_submodule_ignored(concatpath, options))\n+\t\treturn;\n+\n+\tif (options->prefix &&\n+\t    strncmp(concatpath, options->prefix, options->prefix_length))\n+\t\treturn;\n+\n+\tone = alloc_filespec(concatpath);\n+\tfill_filespec(one, oid, 1, mode);\n+\tone->count++;\n+\tdiff_queue(&diff_queued_diff, one, one);\n+}\n+\n struct diff_filepair *diff_unmerge(struct diff_options *options, const char *path)\n {\n \tstruct diff_filepair *pair;\ndiff --git a/diff.h b/diff.h\nindex 31eedd5c0c..e80503aebb 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -572,6 +572,11 @@ void diff_change(struct diff_options *,\n \t\t const char *fullpath,\n \t\t unsigned dirty_submodule1, unsigned dirty_submodule2);\n \n+void diff_same(struct diff_options *,\n+\t       unsigned mode,\n+\t       const struct object_id *oid,\n+\t       const char *fullpath);\n+\n struct diff_filepair *diff_unmerge(struct diff_options *, const char *path);\n \n void compute_diffstat(struct diff_options *options, struct diffstat_t *diffstat,\n-- \n2.52.0\n\n"},{"id":"531174","messageId":"xmqqpl99z0m5.fsf@gitster.g","threadId":"64458","inReplyTo":"8cc12ef2-1d2c-4924-b130-bd740a975ce0@web.de","subject":"Re: [PATCH v2] diff: disable rename detection with --quiet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-23T07:09:22Z","receivedAt":"2025-11-23T07:09:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> --- >8 ---\n> Subject: [PATCH] diff-index: don't queue unchanged filepairs with diff_change()\n>\n> diff_cache() queues unchanged filepairs if the flag find_copies_harder\n> is set, and uses diff_change() for that.  This function does a few\n> things that are unnecessary for unchanged filepairs and always sets the\n> diff_flag has_changes, which is simply misleading in this case.\n>\n> Add a new streamlined function for queuing unchanged filepairs and\n> use it in show_modified(), which is called by diff_cache() via\n> oneway_diff() and do_oneway_diff().  It allocates only one half of each\n> filepair, ...\n\nIt's a misleading thing to say.  It allocates a full filepair, but\nbecause a filespec is reference counted, it can reuse the same\nfilespec to hold both preimage and postimage, halving the memory\nrequirement without leading to double freeing.  And having a\nseparete helper do so would make it almost trivial to avoid setting\nthe has_changes bit.\n\nCleverly done.\n\nThanks.\n\n> ... which has a measurable effect if there are a lot of them, like\n> in the Linux repo:\n"}]}