diff --cached --no-ext-diff --find-copies-harder --quiet exits with wrong status code

diff --cached --no-ext-diff --find-copies-harder --quiet exits with wrong status code

[D. Ben Knoble]

AFAICT, you need all of the mentioned options to trigger the bug.
Allowing ext-diff works fine, I don't think it's triggered in
non-cached diffs, and I've never seen it without --find-copies-harder.
Notably, s/quiet/exit-code works just fine.

Here's a repro from git.git:

    cp git{,1}.c
    git add git1.c
    git diff --cached --no-ext-diff --quiet --find-copies-harder &&
echo 'this should exit 1!'

(And of course, ^quiet^exit-code if your shell supports it yields a
different outcome)

Context: my distro applies a patch that allows
diff.renames=copies-harder. In a repo with that turned on,
git-prompt.sh stopped showing some staged changes.  Turns out it runs
git diff with all these flags (less --find-copies-harder, which is
enabled by the config option). I _have_ confirmed this bug exists in
unpatched Git, however.

Some rough debugging notes: when entering diffcore_std (or
diffcore_rename_extended's cleanup loop):
- for exit-code, diff_queued_diff.nr matches "git ls-files :/ | wc -l"
- for quiet, it's just 1 (the first file listed by git ls-files :/, AFAICT)
The only other obvious difference I spotted is that the "quick" flag
is turned on for quiet, which makes sense.

I tried to figure out who builds the queue, and it looks like it's
diff_cache -> unpack_trees -> ..., where at some point for exit-code
we _keep_ queuing files, but for quiet we don't.

I also saw diff_cache tweaks opts.diff_index_cached based on
--find-copies-harder, but I haven't looked further to see how that
affects things or what the interaction with ext-diff is.

Help welcome, thanks.

-- 
D. Ben Knoble

Re: diff --cached --no-ext-diff --find-copies-harder --quiet exits with wrong status code

[D. Ben Knoble]

On Sat, Nov 8, 2025 at 2:05 PM D. Ben Knoble <ben.knoble@gmail.com> wrote:
>
> AFAICT, you need all of the mentioned options to trigger the bug.
> Allowing ext-diff works fine, I don't think it's triggered in
> non-cached diffs, and I've never seen it without --find-copies-harder.
> Notably, s/quiet/exit-code works just fine.
>
> Here's a repro from git.git:
>
>     cp git{,1}.c
>     git add git1.c
>     git diff --cached --no-ext-diff --quiet --find-copies-harder &&
> echo 'this should exit 1!'
>
> (And of course, ^quiet^exit-code if your shell supports it yields a
> different outcome)
>
> Context: my distro applies a patch that allows
> diff.renames=copies-harder. In a repo with that turned on,
> git-prompt.sh stopped showing some staged changes.  Turns out it runs
> git diff with all these flags (less --find-copies-harder, which is
> enabled by the config option). I _have_ confirmed this bug exists in
> unpatched Git, however.
>
> Some rough debugging notes: when entering diffcore_std (or
> diffcore_rename_extended's cleanup loop):
> - for exit-code, diff_queued_diff.nr matches "git ls-files :/ | wc -l"
> - for quiet, it's just 1 (the first file listed by git ls-files :/, AFAICT)
> The only other obvious difference I spotted is that the "quick" flag
> is turned on for quiet, which makes sense.

Ah, woops. The reason this matters is that, after diff_rename, in the
correct version the queue is non-empty and has_changes gets set, which
plays into diff_result_code. In the broken version, the resulting
queue ends up empty, so has_changes is _reset_ to 0 (despite
previously being 1?)

I think I also spotted a difference in diff_from_contents, but not
sure if that's relevant.


-- 
D. Ben Knoble

Re: diff --cached --no-ext-diff --find-copies-harder --quiet exits with wrong status code

[D. Ben Knoble]

On Sat, Nov 8, 2025 at 2:08 PM D. Ben Knoble <ben.knoble@gmail.com> wrote:
>
> On Sat, Nov 8, 2025 at 2:05 PM D. Ben Knoble <ben.knoble@gmail.com> wrote:
> >
> > AFAICT, you need all of the mentioned options to trigger the bug.
> > Allowing ext-diff works fine, I don't think it's triggered in
> > non-cached diffs, and I've never seen it without --find-copies-harder.
> > Notably, s/quiet/exit-code works just fine.
>
>
> I think I also spotted a difference in diff_from_contents, but not
> sure if that's relevant.

Yeesh. You know how writing for others clarifies thoughts? Well...

I just noticed that diff_setup_done tweaks diff_from_contents based on
whether external diffs are allowed. Possibly relevant? I haven't been
able to easily identify a place where all 3 relevant options come
together, but this would be 2 of them (quiet and ext-diff).

-- 
D. Ben Knoble

[PATCH] diff: disabled quick optimization with --find-copies-harder

[René Scharfe]

If --find-copies-harder is given, diff-lib.c::show_modified() queues
even non-modified entries using diff_change() because we need them for
copy detection.  diff_change() sets flags.has_changes, though.  If
--quiet is also given this causes diff_can_quit_early() to declare we're
done after seeing only the very first entry, which is way too soon.
Disable this optimization in that case.

This issue is hidden without --no-ext-diff because then we set
flags.diff_from_contents, which disables the optimization in a
different way.

Reported-by: D. Ben Knoble <ben.knoble@gmail.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 diff.c              |  1 +
 t/t4007-rename-3.sh | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/diff.c b/diff.c
index a1961526c0..84ac148c37 100644
--- a/diff.c
+++ b/diff.c
@@ -7188,6 +7188,7 @@ int diff_can_quit_early(struct diff_options *opt)
 {
 	return (opt->flags.quick &&
 		!opt->filter &&
+		!opt->flags.find_copies_harder &&
 		opt->flags.has_changes);
 }
 
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

Re: [PATCH] diff: disabled quick optimization with --find-copies-harder

[Phillip Wood]

On 09/11/2025 12:11, René Scharfe wrote:
> If --find-copies-harder is given, diff-lib.c::show_modified() queues
> even non-modified entries using diff_change() because we need them for
> copy detection.  diff_change() sets flags.has_changes, though.  If
> --quiet is also given this causes diff_can_quit_early() to declare we're
> done after seeing only the very first entry, which is way too soon.
> Disable this optimization in that case.

Stepping back a bit I'm confused as to why we don't disable rename and 
copy detection when "--quiet" is given. I can't see why detecting copies 
or renames would change the exit code but maybe I'm missing something.

Thanks

Phillip

> This issue is hidden without --no-ext-diff because then we set
> flags.diff_from_contents, which disables the optimization in a
> different way.
> 
> Reported-by: D. Ben Knoble <ben.knoble@gmail.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>   diff.c              |  1 +
>   t/t4007-rename-3.sh | 10 ++++++++++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/diff.c b/diff.c
> index a1961526c0..84ac148c37 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -7188,6 +7188,7 @@ int diff_can_quit_early(struct diff_options *opt)
>   {
>   	return (opt->flags.quick &&
>   		!opt->filter &&
> +		!opt->flags.find_copies_harder &&
>   		opt->flags.has_changes);
>   }
>   
> 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

Re: [PATCH] diff: disabled quick optimization with --find-copies-harder

[René Scharfe]

On 11/9/25 3:18 PM, Phillip Wood wrote:
> 
> Stepping back a bit I'm confused as to why we don't disable rename
> and copy detection when "--quiet" is given. I can't see why
> detecting copies or renames would change the exit code but maybe I'm
> missing something.

Excellent question!  I also can't think of a reason.

René

[PATCH v2] diff: disable rename detection with --quiet

[René Scharfe]

Detecting renames and copies improves diff's output.  This effort is
wasted if we don't show any.  Disable detection in that case.

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.

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 <ben.knoble@gmail.com>
Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 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

Re: [PATCH v2] diff: disable rename detection with --quiet

[D. Ben Knoble]

On Sun, Nov 9, 2025 at 11:43 AM René Scharfe <l.s.r@web.de> wrote:
>
> Detecting renames and copies improves diff's output.  This effort is
> wasted if we don't show any.  Disable detection in that case.

Indeed. I've confirmed this and v1 both fix the issue, although v2 is
significantly faster (which is great for the intended use in
git-prompt.sh!):

λ 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

> 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 <ben.knoble@gmail.com>
> Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  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

Re: [PATCH v2] diff: disable rename detection with --quiet

[René Scharfe]

On 11/9/25 6:34 PM, D. Ben Knoble wrote:
> On Sun, Nov 9, 2025 at 11:43 AM René Scharfe <l.s.r@web.de> wrote:
>>
>> 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).

run_diff_index() calls diff-lib.c::diff_cache() to queue up index
entries.  As mentioned above it only queues up the very first one, no
matter if it's a change or not.  In Git's repo this would be
.cirrus.yml.  That's not the end of it, yet, though.  It then calls
diffcore_std(), which calls diffcore_rename() to remove non-changes
from the queue and overwrites flags.has_changes based on whether the
queue is empty now.

René

Re: [PATCH v2] diff: disable rename detection with --quiet

[Phillip Wood]

Hi Ben

On 09/11/2025 17:34, D. Ben Knoble wrote:
> On Sun, Nov 9, 2025 at 11:43 AM René Scharfe <l.s.r@web.de> 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 <ben.knoble@gmail.com>
>> Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>   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
> 

Re: [PATCH v2] diff: disable rename detection with --quiet

[Jeff King]

On Sun, Nov 09, 2025 at 05:43:36PM +0100, René Scharfe wrote:

> Detecting renames and copies improves diff's output.  This effort is
> wasted if we don't show any.  Disable detection in that case.
> 
> 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.
> 
> 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.

This makes sense to me, and I can't think of a reason why you would want
rename detection on if we're not going to show the results (and likewise
I can't think of a way that a rename result would affect has_changes).

I wonder if we should _also_ take the hunk from v1 that teaches
can_quit_early() to avoid triggering when copy detection is on. It's
probably redundant now, but it feels to me like that's the place where
the correctness check should kick in. And the patch here is just
optimizing out the unnecessary work, but also happens to align things
for correctness downstream.

But I dunno. Maybe a check for a condition that we think can never be
triggered becomes too confusing for later maintenance.


You don't say in the commit message when this bug started. I briefly
wondered if it was caused by the recent diff_from_contents stuff we've
been discussing. But it's the opposite here (the bug happens when we
_don't_ set diff_from_contents). And I think it goes all the way back to
b4194828dc (diff-index --quiet: learn the "stop feeding the backend
early" logic, 2011-05-31).

-Peff

Re: [PATCH v2] diff: disable rename detection with --quiet

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This makes sense to me, and I can't think of a reason why you would want
> rename detection on if we're not going to show the results (and likewise
> I can't think of a way that a rename result would affect has_changes).
>
> I wonder if we should _also_ take the hunk from v1 that teaches
> can_quit_early() to avoid triggering when copy detection is on. It's
> probably redundant now, but it feels to me like that's the place where
> the correctness check should kick in. And the patch here is just
> optimizing out the unnecessary work, but also happens to align things
> for correctness downstream.

Concurred on both counts.

> You don't say in the commit message when this bug started. I briefly
> wondered if it was caused by the recent diff_from_contents stuff we've
> been discussing. But it's the opposite here (the bug happens when we
> _don't_ set diff_from_contents). And I think it goes all the way back to
> b4194828dc (diff-index --quiet: learn the "stop feeding the backend
> early" logic, 2011-05-31).

Yup, I think so.  Back then I think our assumptions are that the
user knows better than giving complex diffcore requests like
find-copies-harder only to discard the results with --quiet, and the
patch started this thread helps other users, which is good ;-).

Re: [PATCH v2] diff: disable rename detection with --quiet

[D. Ben Knoble]

On Sun, Nov 9, 2025 at 1:35 PM René Scharfe <l.s.r@web.de> wrote:
>
> On 11/9/25 6:34 PM, D. Ben Knoble wrote:
> > On Sun, Nov 9, 2025 at 11:43 AM René Scharfe <l.s.r@web.de> wrote:
> >>
> >> 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).
>
> run_diff_index() calls diff-lib.c::diff_cache() to queue up index
> entries.  As mentioned above it only queues up the very first one, no
> matter if it's a change or not.  In Git's repo this would be
> .cirrus.yml.  That's not the end of it, yet, though.  It then calls
> diffcore_std(), which calls diffcore_rename() to remove non-changes
> from the queue and overwrites flags.has_changes based on whether the
> queue is empty now.
>
> René
>

Thanks, btw. Still try to absorb this part of the code, but this helps :)

-- 
D. Ben Knoble

Re: [PATCH v2] diff: disable rename detection with --quiet

[René Scharfe]

On 11/10/25 6:54 PM, Jeff King wrote:
> 
> I wonder if we should _also_ take the hunk from v1 that teaches
> can_quit_early() to avoid triggering when copy detection is on. It's
> probably redundant now, but it feels to me like that's the place where
> the correctness check should kick in. And the patch here is just
> optimizing out the unnecessary work, but also happens to align things
> for correctness downstream.
> 
> But I dunno. Maybe a check for a condition that we think can never be
> triggered becomes too confusing for later maintenance.
That check was only necessary because we queue unchanged filepairs for
rename detection as if they were changes.  With --quiet forcing rename
detection off we won't run into this anymore, but it still feels like a
trip hazard.  Adding a check would help, but we could also stop doing
that in the first place.  Patch below.

René


--- >8 ---
Subject: [PATCH] diff-index: don't queue unchanged filepairs with diff_change()

diff_cache() queues unchanged filepairs if the flag find_copies_harder
is set, and uses diff_change() for that.  This function does a few
things that are unnecessary for unchanged filepairs and always sets the
diff_flag has_changes, which is simply misleading in this case.

Add a new streamlined function for queuing unchanged filepairs and
use it in show_modified(), which is called by diff_cache() via
oneway_diff() and do_oneway_diff().  It allocates only one half of each
filepair, which has a measurable effect if there are a lot of them, like
in the Linux repo:

Benchmark 1: ./git_v2.52.0 -C ../linux diff --cached --find-copies-harder
  Time (mean ± σ):      31.8 ms ±   0.2 ms    [User: 24.2 ms, System: 6.3 ms]
  Range (min … max):    31.5 ms …  32.3 ms    85 runs

Benchmark 2: ./git -C ../linux diff --cached --find-copies-harder
  Time (mean ± σ):      23.9 ms ±   0.2 ms    [User: 18.1 ms, System: 4.6 ms]
  Range (min … max):    23.5 ms …  24.4 ms    111 runs

Summary
  ./git -C ../linux diff --cached --find-copies-harder ran
    1.33 ± 0.01 times faster than ./git_v2.52.0 -C ../linux diff --cached --find-copies-harder

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 diff-lib.c | 13 ++++++-------
 diff.c     | 20 ++++++++++++++++++++
 diff.h     |  5 +++++
 3 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index b8f8f3bc31..8e624f38c6 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -418,13 +418,12 @@ static int show_modified(struct rev_info *revs,
 	}
 
 	oldmode = old_entry->ce_mode;
-	if (mode == oldmode && oideq(oid, &old_entry->oid) && !dirty_submodule &&
-	    !revs->diffopt.flags.find_copies_harder)
-		return 0;
-
-	diff_change(&revs->diffopt, oldmode, mode,
-		    &old_entry->oid, oid, 1, !is_null_oid(oid),
-		    old_entry->name, 0, dirty_submodule);
+	if (mode != oldmode || !oideq(oid, &old_entry->oid) || dirty_submodule)
+		diff_change(&revs->diffopt, oldmode, mode,
+			    &old_entry->oid, oid, 1, !is_null_oid(oid),
+			    old_entry->name, 0, dirty_submodule);
+	else if (revs->diffopt.flags.find_copies_harder)
+		diff_same(&revs->diffopt, mode, oid, old_entry->name);
 	return 0;
 }
 
diff --git a/diff.c b/diff.c
index efa8d9773c..e2a2927f8c 100644
--- a/diff.c
+++ b/diff.c
@@ -7349,6 +7349,26 @@ void diff_change(struct diff_options *options,
 			  concatpath, old_dirty_submodule, new_dirty_submodule);
 }
 
+void diff_same(struct diff_options *options,
+	       unsigned mode,
+	       const struct object_id *oid,
+	       const char *concatpath)
+{
+	struct diff_filespec *one;
+
+	if (S_ISGITLINK(mode) && is_submodule_ignored(concatpath, options))
+		return;
+
+	if (options->prefix &&
+	    strncmp(concatpath, options->prefix, options->prefix_length))
+		return;
+
+	one = alloc_filespec(concatpath);
+	fill_filespec(one, oid, 1, mode);
+	one->count++;
+	diff_queue(&diff_queued_diff, one, one);
+}
+
 struct diff_filepair *diff_unmerge(struct diff_options *options, const char *path)
 {
 	struct diff_filepair *pair;
diff --git a/diff.h b/diff.h
index 31eedd5c0c..e80503aebb 100644
--- a/diff.h
+++ b/diff.h
@@ -572,6 +572,11 @@ void diff_change(struct diff_options *,
 		 const char *fullpath,
 		 unsigned dirty_submodule1, unsigned dirty_submodule2);
 
+void diff_same(struct diff_options *,
+	       unsigned mode,
+	       const struct object_id *oid,
+	       const char *fullpath);
+
 struct diff_filepair *diff_unmerge(struct diff_options *, const char *path);
 
 void compute_diffstat(struct diff_options *options, struct diffstat_t *diffstat,
-- 
2.52.0

Re: [PATCH v2] diff: disable rename detection with --quiet

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> --- >8 ---
> Subject: [PATCH] diff-index: don't queue unchanged filepairs with diff_change()
>
> diff_cache() queues unchanged filepairs if the flag find_copies_harder
> is set, and uses diff_change() for that.  This function does a few
> things that are unnecessary for unchanged filepairs and always sets the
> diff_flag has_changes, which is simply misleading in this case.
>
> Add a new streamlined function for queuing unchanged filepairs and
> use it in show_modified(), which is called by diff_cache() via
> oneway_diff() and do_oneway_diff().  It allocates only one half of each
> filepair, ...

It's a misleading thing to say.  It allocates a full filepair, but
because a filespec is reference counted, it can reuse the same
filespec to hold both preimage and postimage, halving the memory
requirement without leading to double freeing.  And having a
separete helper do so would make it almost trivial to avoid setting
the has_changes bit.

Cleverly done.

Thanks.

> ... which has a measurable effect if there are a lot of them, like
> in the Linux repo: