From: Phillip Wood Date: Mon, 10 Nov 2025 09:42:53 GMT Subject: Re: [PATCH v2] diff: disable rename detection with --quiet Message-ID: <61e6b077-26ef-49ed-83cf-fa5b7888429c@gmail.com> In-Reply-To: Hi Ben On 09/11/2025 17:34, D. Ben Knoble wrote: > On Sun, Nov 9, 2025 at 11:43 AM René Scharfe wrote: >> > λ hyperfine -NiP v 1 2 ~/code/git/'buildv{v}/git diff --cached --quiet > --no-ext-diff --find-copies-harder' > Benchmark 1: /home/benknoble/code/git/buildv1/git diff --cached > --quiet --no-ext-diff --find-copies-harder > Time (mean ± σ): 72.0 ms ± 3.3 ms [User: 45.2 ms, System: 26.2 ms] > Range (min … max): 67.6 ms … 79.6 ms 42 runs > > Warning: Ignoring non-zero exit code. > > Benchmark 2: /home/benknoble/code/git/buildv2/git diff --cached > --quiet --no-ext-diff --find-copies-harder > Time (mean ± σ): 19.9 ms ± 1.5 ms [User: 8.9 ms, System: 10.6 ms] > Range (min … max): 16.1 ms … 24.0 ms 151 runs > > Warning: Ignoring non-zero exit code. > > Summary > /home/benknoble/code/git/buildv2/git diff --cached --quiet > --no-ext-diff --find-copies-harder ran > 3.61 ± 0.31 times faster than /home/benknoble/code/git/buildv1/git > diff --cached --quiet --no-ext-diff --find-copies-harder That's a nice speedup. Thanks for sharing that - I knew in an abstract way that "--find-copies-harder" slowed things down but seeing some concrete numbers really brings it home. Best Wishes Phillip >> This actually fixes the error code when using the options --cached, >> --find-copies-harder, --no-ext-diff and --quiet together: >> run_diff_index() indirectly calls diff-lib.c::show_modified(), which >> queues even non-modified entries using diff_change() because we need >> them for copy detection. diff_change() sets flags.has_changes, though, >> which causes diff_can_quit_early() to declare we're done after seeing >> only the very first entry -- way too soon. > > This does describe the behavior I saw, but it seems to me that, if we > have changes, then we ought to be able to quit early for --quiet, no? > > So there's some other knock-on effect that causes quitting early to be > wrong here, and I'm not exactly sure what it is (other than the diff > queues being different sizes when we hit relevant parts of > diffcore_std, though it's the working case that has the larger queue). > So I'm having a hard time tying this paragraph to the actual issue > (mostly due to my complete unfamiliarity with the diffing subsystem). > >> Using --cached, --find-copies-harder and --quiet together without >> --no-ext-diff was not affected even before, as it causes the flag >> flags.diff_from_contents to be set, which disables the optimization >> in a different way. >> >> Reported-by: D. Ben Knoble >> Suggested-by: Phillip Wood >> Signed-off-by: René Scharfe >> --- >> diff.c | 2 ++ >> t/t4007-rename-3.sh | 10 ++++++++++ >> 2 files changed, 12 insertions(+) >> >> diff --git a/diff.c b/diff.c >> index a1961526c0..efa8d9773c 100644 >> --- a/diff.c >> +++ b/diff.c >> @@ -4987,6 +4987,8 @@ void diff_setup_done(struct diff_options *options) >> if (options->flags.quick) { >> options->output_format = DIFF_FORMAT_NO_OUTPUT; >> options->flags.exit_with_status = 1; >> + options->detect_rename = 0; >> + options->flags.find_copies_harder = 0; >> } >> >> /* >> diff --git a/t/t4007-rename-3.sh b/t/t4007-rename-3.sh >> index e8faf0dd2e..3fc81bcd76 100755 >> --- a/t/t4007-rename-3.sh >> +++ b/t/t4007-rename-3.sh >> @@ -41,6 +41,16 @@ test_expect_success 'copy detection, cached' ' >> compare_diff_raw current expected >> ' >> >> +test_expect_success 'exit code of quiet copy detection' ' >> + test_expect_code 1 \ >> + git diff --quiet --cached --find-copies-harder $tree >> +' >> + >> +test_expect_success 'exit code of quiet copy detection with --no-ext-diff' ' >> + test_expect_code 1 \ >> + git diff --quiet --cached --find-copies-harder --no-ext-diff $tree >> +' >> + >> # In the tree, there is only path0/COPYING. In the cache, path0 and >> # path1 both have COPYING and the latter is a copy of path0/COPYING. >> # However when we say we care only about path1, we should just see >> -- >> 2.51.2 > > Covering both seems like the right move to me, thanks! > > -- > D. Ben Knoble >