{"thread":{"id":"64623","subject":"[PATCH RESEND] diff-files: fix copy detection","startedAt":"2025-12-14T15:57:09Z","lastAt":"2025-12-16T01:21:33Z","messageCount":3,"participants":["René Scharfe","SZEDER Gábor","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"532147","messageId":"4b06a448-0935-4f2a-9061-238c7cc800c3@web.de","threadId":"64623","inReplyTo":null,"subject":"[PATCH RESEND] diff-files: fix copy detection","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-12-14T15:57:06Z","receivedAt":"2025-12-14T15:57:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Copy detection cannot work when comparing the index to the working tree\nbecause Git ignores files that it is not explicitly told to track.  It\nshould work in the other direction, though, i.e. for a reverse diff of\nthe deletion of a copy from the index.\n\nd1f2d7e8ca (Make run_diff_index() use unpack_trees(), not read_tree(),\n2008-01-19) broke it with a seemingly stray change to run_diff_files().\n\nWe didn't notice because there's no test for that.  But even if we had\none, it might have gone unnoticed because the breakage only happens\nwith index preloading, which requires at least 1000 entries (more than\nmost test repos have) and is racy because it runs in parallel with the\nactual command.\n\nFix copy detection by queuing up-to-date and skip-worktree entries using\ndiff_same().\n\nWhile at it, use diff_same() also for queuing unchanged files not\nflagged as up-to-date, i.e. clean submodules and entries where\npreloading was not done at all or not quickly enough.  It uses less\nmemory than diff_change() and doesn't unnecessarily set the diff flag\nhas_changes.\n\nAdd two tests to cover running both without and with preloading.  The\nfirst one passes reliably with the original code.  The second one\nenables preloading and thus is racy.  It has a good chance to pass even\nwithout the fix, but fails within seconds when running the test script\nwith --stress.  With the fix it runs fine for several minutes, until\nmy patience runs out.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nOriginal submission:\nhttps://lore.kernel.org/git/f2e187bb-c765-4cc3-a0a0-1fbaec9a14e2@web.de/\n\n diff-lib.c          | 12 +++++++++---\n t/t4007-rename-3.sh | 23 ++++++++++++++++++++++-\n 2 files changed, 31 insertions(+), 4 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 8e624f38c6..5307390ff3 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -226,8 +226,12 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\tcontinue;\n \t\t}\n \n-\t\tif (ce_uptodate(ce) || ce_skip_worktree(ce))\n+\t\tif (ce_uptodate(ce) || ce_skip_worktree(ce)) {\n+\t\t\tif (revs->diffopt.flags.find_copies_harder)\n+\t\t\t\tdiff_same(&revs->diffopt, ce->ce_mode,\n+\t\t\t\t\t  &ce->oid, ce->name);\n \t\t\tcontinue;\n+\t\t}\n \n \t\t/*\n \t\t * When CE_VALID is set (via \"update-index --assume-unchanged\"\n@@ -272,8 +276,10 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tif (!changed && !dirty_submodule) {\n \t\t\tce_mark_uptodate(ce);\n \t\t\tmark_fsmonitor_valid(istate, ce);\n-\t\t\tif (!revs->diffopt.flags.find_copies_harder)\n-\t\t\t\tcontinue;\n+\t\t\tif (revs->diffopt.flags.find_copies_harder)\n+\t\t\t\tdiff_same(&revs->diffopt, newmode,\n+\t\t\t\t\t  &ce->oid, ce->name);\n+\t\t\tcontinue;\n \t\t}\n \t\toldmode = ce->ce_mode;\n \t\told_oid = &ce->oid;\ndiff --git a/t/t4007-rename-3.sh b/t/t4007-rename-3.sh\nindex 3fc81bcd76..1012a370dd 100755\n--- a/t/t4007-rename-3.sh\n+++ b/t/t4007-rename-3.sh\n@@ -67,7 +67,28 @@ test_expect_success 'copy, limited to a subtree' '\n '\n \n test_expect_success 'tweak work tree' '\n-\trm -f path0/COPYING &&\n+\trm -f path0/COPYING\n+'\n+\n+cat >expected <<EOF\n+:100644 100644 $blob $blob C100\tpath1/COPYING\tpath0/COPYING\n+EOF\n+\n+# The cache has path0/COPYING and path1/COPYING, the working tree only\n+# path1/COPYING.  This is a deletion -- we don't treat deduplication\n+# specially.  In reverse it should be detected as a copy, though.\n+test_expect_success 'copy detection, files to index' '\n+\tgit diff-files -C --find-copies-harder -R >current &&\n+\tcompare_diff_raw current expected\n+'\n+\n+test_expect_success 'copy detection, files to preloaded index' '\n+\tGIT_TEST_PRELOAD_INDEX=1 \\\n+\tgit diff-files -C --find-copies-harder -R >current &&\n+\tcompare_diff_raw current expected\n+'\n+\n+test_expect_success 'tweak index' '\n \tgit update-index --remove path0/COPYING\n '\n # In the tree, there is only path0/COPYING.  In the cache, path0 does\n-- \n2.52.0\n"},{"id":"532218","messageId":"aUCTdUMKslSo3XR9@szeder.dev","threadId":"64623","inReplyTo":"4b06a448-0935-4f2a-9061-238c7cc800c3@web.de","subject":"Re: [PATCH RESEND] diff-files: fix copy detection","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2025-12-15T23:02:13Z","receivedAt":"2025-12-15T23:02:16Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sun, Dec 14, 2025 at 04:57:06PM +0100, René Scharfe wrote:\n> Fix copy detection by queuing up-to-date and skip-worktree entries using\n> diff_same().\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 8e624f38c6..5307390ff3 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n\n> @@ -272,8 +276,10 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n>  \t\tif (!changed && !dirty_submodule) {\n>  \t\t\tce_mark_uptodate(ce);\n>  \t\t\tmark_fsmonitor_valid(istate, ce);\n> -\t\t\tif (!revs->diffopt.flags.find_copies_harder)\n> -\t\t\t\tcontinue;\n> +\t\t\tif (revs->diffopt.flags.find_copies_harder)\n> +\t\t\t\tdiff_same(&revs->diffopt, newmode,\n> +\t\t\t\t\t  &ce->oid, ce->name);\n\nJunio, this patch should be queued on top of 38f88051da\n(diff-index: don't queue unchanged filepairs with diff_change(),\n2025-11-30), because diff_same() was introduced in that commit.\n\n  ~/src/git ((7077c385f9...) %)$ git log --oneline -1\n  7077c385f9 (HEAD) diff-files: fix copy detection\n  ~/src/git ((7077c385f9...) %)$ make diff-lib.o\n      CC diff-lib.o\n  diff-lib.c: In function ‘run_diff_files’:\n  diff-lib.c:231:33: error: implicit declaration of function ‘diff_same’; did you mean ‘diff_free’? [-Werror=implicit-function-declaration]\n    231 |                                 diff_same(&revs->diffopt, ce->ce_mode,\n        |                                 ^~~~~~~~~\n        |                                 diff_free\n  cc1: all warnings being treated as errors\n  make: *** [Makefile:2862: diff-lib.o] Error 1\n\n"},{"id":"532230","messageId":"xmqqecov5ihw.fsf@gitster.g","threadId":"64623","inReplyTo":"aUCTdUMKslSo3XR9@szeder.dev","subject":"Re: [PATCH RESEND] diff-files: fix copy detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-12-16T01:21:31Z","receivedAt":"2025-12-16T01:21:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> On Sun, Dec 14, 2025 at 04:57:06PM +0100, René Scharfe wrote:\n>> Fix copy detection by queuing up-to-date and skip-worktree entries using\n>> diff_same().\n>\n>> diff --git a/diff-lib.c b/diff-lib.c\n>> index 8e624f38c6..5307390ff3 100644\n>> --- a/diff-lib.c\n>> +++ b/diff-lib.c\n>\n>> @@ -272,8 +276,10 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n>>  \t\tif (!changed && !dirty_submodule) {\n>>  \t\t\tce_mark_uptodate(ce);\n>>  \t\t\tmark_fsmonitor_valid(istate, ce);\n>> -\t\t\tif (!revs->diffopt.flags.find_copies_harder)\n>> -\t\t\t\tcontinue;\n>> +\t\t\tif (revs->diffopt.flags.find_copies_harder)\n>> +\t\t\t\tdiff_same(&revs->diffopt, newmode,\n>> +\t\t\t\t\t  &ce->oid, ce->name);\n>\n> Junio, this patch should be queued on top of 38f88051da\n> (diff-index: don't queue unchanged filepairs with diff_change(),\n> 2025-11-30), because diff_same() was introduced in that commit.\n\nVery true.  Thanks.\n\n>\n>   ~/src/git ((7077c385f9...) %)$ git log --oneline -1\n>   7077c385f9 (HEAD) diff-files: fix copy detection\n>   ~/src/git ((7077c385f9...) %)$ make diff-lib.o\n>       CC diff-lib.o\n>   diff-lib.c: In function ‘run_diff_files’:\n>   diff-lib.c:231:33: error: implicit declaration of function ‘diff_same’; did you mean ‘diff_free’? [-Werror=implicit-function-declaration]\n>     231 |                                 diff_same(&revs->diffopt, ce->ce_mode,\n>         |                                 ^~~~~~~~~\n>         |                                 diff_free\n>   cc1: all warnings being treated as errors\n>   make: *** [Makefile:2862: diff-lib.o] Error 1\n"}]}