{"thread":{"id":"64556","subject":"[PATCH v2] diff-index: don't queue unchanged filepairs with diff_change()","startedAt":"2025-11-30T11:52:50Z","lastAt":"2025-12-03T15:06:48Z","messageCount":5,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"531462","messageId":"aa28974b-ec73-4562-bfc8-4745ad58b55a@web.de","threadId":"64556","inReplyTo":null,"subject":"[PATCH v2] diff-index: don't queue unchanged filepairs with diff_change()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-11-30T11:47:17Z","receivedAt":"2025-11-30T11:52:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"diff_cache() queues unchanged filepairs if the flag find_copies_harder\nis set, and uses diff_change() for that.  This function allocates a\nfilespec for each side, does a few other things that are unnecessary for\nunchanged filepairs and always sets the diff_flag has_changes, which is\nsimply 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 a single filespec\nfor each filepair and uses it twice with reference counting.  This has a\nmeasurable effect if there are a lot of them, like in 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---\nChanges since v1:\n- Clearer description of memory usage in the commit message.\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 915317025f..63d33251cd 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -7348,6 +7348,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"},{"id":"531471","messageId":"xmqq5xarcsb8.fsf@gitster.g","threadId":"64556","inReplyTo":"aa28974b-ec73-4562-bfc8-4745ad58b55a@web.de","subject":"Re: [PATCH v2] diff-index: don't queue unchanged filepairs with diff_change()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-30T18:02:19Z","receivedAt":"2025-11-30T18:02:22Z","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> 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 a single filespec\n> for each filepair and uses it twice with reference counting.  This has a\n> measurable effect if there are a lot of them, like in the Linux repo:\n>\n> Benchmark 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>\n> Benchmark 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>\n> Summary\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\nNice.  Is this technique only applicable to diff-index among the\nthree diff plumbing siblings?  I suspect diff-files is an oddball\nin that on the working tree side we do not necessarily have the\nblob object names, but it would apply to diff-tree, wouldn't it?\n\nWill queue.  Thanks.\n"},{"id":"531586","messageId":"f2e187bb-c765-4cc3-a0a0-1fbaec9a14e2@web.de","threadId":"64556","inReplyTo":"xmqq5xarcsb8.fsf@gitster.g","subject":"Re: [PATCH v2] diff-index: don't queue unchanged filepairs with diff_change()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-12-02T21:16:27Z","receivedAt":"2025-12-02T21:16:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 11/30/25 7:02 PM, Junio C Hamano wrote:\n> René Scharfe <l.s.r@web.de> writes:\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 a single filespec\n>> for each filepair and uses it twice with reference counting.  This has a\n>> measurable effect if there are a lot of them, like in the Linux repo:\n>>\n>> Benchmark 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>>\n>> Benchmark 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>>\n>> Summary\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> \n> Nice.  Is this technique only applicable to diff-index among the\n> three diff plumbing siblings?  I suspect diff-files is an oddball\n> in that on the working tree side we do not necessarily have the\n> blob object names\nIndeed:\n- git diff-files compares index and working tree,\n- a copy is a new file with contents from an old file,\n- git ignores new files in the working tree.\n\nSo in theory git diff-files can only detect copies in the other\ndirection.  Or is there a way I'm missing?  In practice, however, it\ndoesn't do that reliably because it simply skips up-to-date index\nentries.  Oops.\n\n--- >8 ---\nSubject: [PATCH v2 2/1] diff-files: fix copy detection\n\nCopy 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---\nPatch formatted with -U9 for easier review of the second hunk.\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@@ -220,20 +220,24 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t * from the desired stage.\n \t\t\t */\n \t\t\tpair = diff_unmerge(&revs->diffopt, ce->name);\n \t\t\tif (wt_mode)\n \t\t\t\tpair->two->mode = wt_mode;\n \t\t\tif (ce_stage(ce) != diff_unmerged_stage)\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 \t\t * or via adding paths while core.ignorestat is set to true),\n \t\t * the user has promised that the working tree file for that\n \t\t * path will not be modified.  When CE_FSMONITOR_VALID is true,\n \t\t * the fsmonitor knows that the path hasn't been modified since\n \t\t * we refreshed the cached stat information.  In either case,\n \t\t * we do not have to stat to see if the path has been removed\n@@ -266,20 +270,22 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n \n \t\t\tchanged = match_stat_with_submodule(&revs->diffopt, ce, &st,\n \t\t\t\t\t\t\t    ce_option, &dirty_submodule);\n \t\t\tnewmode = ce_mode_from_stat(ce, st.st_mode);\n \t\t}\n \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;\n \t\tnew_oid = changed ? null_oid(the_hash_algo) : &ce->oid;\n \t\tdiff_change(&revs->diffopt, oldmode, newmode,\n \t\t\t    old_oid, new_oid,\n \t\t\t    !is_null_oid(old_oid),\n \t\t\t    !is_null_oid(new_oid),\n \t\t\t    ce->name, 0, dirty_submodule);\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@@ -61,19 +61,40 @@ cat >expected <<EOF\n :000000 100644 $ZERO_OID $blob A\tpath1/COPYING\n EOF\n \n test_expect_success 'copy, limited to a subtree' '\n \tgit diff-index -C --find-copies-harder $tree path1 >current &&\n \tcompare_diff_raw current expected\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 # not have COPYING anymore and path1 has COPYING which is a copy of\n # path0/COPYING.  Showing the full tree with cache should tell us about\n # the rename.\n \n cat >expected <<EOF\n :100644 100644 $blob $blob R100\tpath0/COPYING\tpath1/COPYING\n-- \n2.52.0\n\n"},{"id":"531588","messageId":"9a253514-376f-49fd-99fe-f076ecb180b6@web.de","threadId":"64556","inReplyTo":"xmqq5xarcsb8.fsf@gitster.g","subject":"Re: [PATCH v2] diff-index: don't queue unchanged filepairs with diff_change()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-12-02T22:07:17Z","receivedAt":"2025-12-02T22:12:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 11/30/25 7:02 PM, Junio C Hamano wrote:\n> René Scharfe <l.s.r@web.de> writes:\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 a single filespec\n>> for each filepair and uses it twice with reference counting.  This has a\n>> measurable effect if there are a lot of them, like in the Linux repo:\n>>\n>> Benchmark 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>>\n>> Benchmark 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>>\n>> Summary\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> \n> Nice.  Is this technique only applicable to diff-index among the\n> three diff plumbing siblings?\n\n> [...] it would apply to diff-tree, wouldn't it?\nYes, but its diff_change() call is behind two layers of callbacks, which\ncomplicates things.\n\nAnd I don't know how to avoid adding an object ID comparison.  Do we\nperhaps have that bit somewhere in tree-diff.c already and can pass it\nalong the pathchange call?\n\nAnd I wonder now if diff_same() is the right name.  Shouldn't it be\ndiff_keep() or a similar verb to match the siblings diff_change(),\ndiff_addremove() and diff_unmerge()?\n\nPerformance looks mixed.  E.g. memory usage is reduced slightly here:\n\n$ for git in ./git_v2.52.0 ./git\n  do\n    for i in $(seq 5)\n    do\n      /usr/bin/time -l $git diff-tree --find-copies-harder -r v2.51.0 v2.51.1 2>&1 >/dev/null\n    done\n  done | grep peak\n            36111744  peak memory footprint\n            36291968  peak memory footprint\n            36177280  peak memory footprint\n            36177280  peak memory footprint\n            36291968  peak memory footprint\n            35456384  peak memory footprint\n            35489152  peak memory footprint\n            35505536  peak memory footprint\n            35636608  peak memory footprint\n            35505536  peak memory footprint\n\nBut diff-tree needs 1% more time with the patch:\n\nBenchmark 1: ./git_v2.52.0 diff-tree --find-copies-harder -r v2.51.0 v2.51.1\n  Time (mean ± σ):      78.3 ms ±   0.2 ms    [User: 57.4 ms, System: 19.8 ms]\n  Range (min … max):    77.9 ms …  78.7 ms    36 runs\n\nBenchmark 2: ./git diff-tree --find-copies-harder -r v2.51.0 v2.51.1\n  Time (mean ± σ):      78.8 ms ±   0.2 ms    [User: 57.9 ms, System: 19.8 ms]\n  Range (min … max):    78.4 ms …  79.2 ms    36 runs\n\nSummary\n  ./git_v2.52.0 diff-tree --find-copies-harder -r v2.51.0 v2.51.1 ran\n    1.01 ± 0.00 times faster than ./git diff-tree --find-copies-harder -r v2.51.0 v2.51.1\n\nOther examples look better:\n\nBenchmark 1: ./git_v2.52.0 -C ../linux diff-tree --find-copies-harder -r v6.8 v6.9\n  Time (mean ± σ):     110.7 ms ±   0.2 ms    [User: 83.1 ms, System: 26.8 ms]\n  Range (min … max):   110.2 ms … 111.4 ms    26 runs\n\nBenchmark 2: ./git -C ../linux diff-tree --find-copies-harder -r v6.8 v6.9\n  Time (mean ± σ):     105.3 ms ±   0.4 ms    [User: 78.9 ms, System: 24.0 ms]\n  Range (min … max):   104.7 ms … 106.0 ms    27 runs\n\nSummary\n  ./git -C ../linux diff-tree --find-copies-harder -r v6.8 v6.9 ran\n    1.05 ± 0.00 times faster than ./git_v2.52.0 -C ../linux diff-tree --find-copies-harder -r v6.8 v6.9\n\nBut overall I'm not impressed. :-|\n\nRené\n\n\n---\n builtin/reset.c | 1 +\n diff.c          | 1 +\n diff.h          | 7 ++++++-\n diffcore.h      | 4 ++--\n tree-diff.c     | 8 ++++++--\n 5 files changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex ed35802af1..ec674694dd 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -210,6 +210,7 @@ static int read_from_tree(const struct pathspec *pathspec,\n \topt.repo = the_repository;\n \topt.change = diff_change;\n \topt.add_remove = diff_addremove;\n+\topt.keep = diff_same;\n \n \tif (pathspec->nr && pathspec_needs_expanded_index(the_repository->index, pathspec))\n \t\tensure_full_index(the_repository->index);\ndiff --git a/diff.c b/diff.c\nindex 436da250eb..9671524d2b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4847,6 +4847,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)\n \t/* pathchange left =NULL by default */\n \toptions->change = diff_change;\n \toptions->add_remove = diff_addremove;\n+\toptions->keep = diff_same;\n \toptions->use_color = diff_use_color_default;\n \toptions->detect_rename = diff_detect_rename_default;\n \toptions->xdl_opts |= diff_algorithm;\ndiff --git a/diff.h b/diff.h\nindex 7eb84aadf4..6dfec55039 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -43,7 +43,8 @@ struct oidset;\n  * set_default in diff_options can be used to tweak this more.\n  *\n  * - As you find different pairs of files, call `diff_change()` to feed\n- * modified files, `diff_addremove()` to feed created or deleted files, or\n+ * modified files, `diff_addremove()` to feed created or deleted files,\n+ * `diff_same()` to feed unmodified files if needed for copy detection, or\n  * `diff_unmerge()` to feed a file whose state is 'unmerged' to the API.\n  * These are thin wrappers to a lower-level `diff_queue()` function that is\n  * flexible enough to record any of these kinds of changes.\n@@ -92,6 +93,9 @@ typedef void (*add_remove_fn_t)(struct diff_options *options,\n \t\t    int oid_valid,\n \t\t    const char *fullpath, unsigned dirty_submodule);\n \n+typedef void (*keep_fn_t)(struct diff_options *options, unsigned mode,\n+\t\t\t  const struct object_id *oid, const char *fullpath);\n+\n typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,\n \t\tstruct diff_options *options, void *data);\n \n@@ -384,6 +388,7 @@ struct diff_options {\n \tpathchange_fn_t pathchange;\n \tchange_fn_t change;\n \tadd_remove_fn_t add_remove;\n+\tkeep_fn_t keep;\n \tvoid *change_fn_data;\n \tdiff_format_fn_t format_callback;\n \tvoid *format_callback_data;\ndiff --git a/diffcore.h b/diffcore.h\nindex 9c0a0e7aaf..64b419b33f 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -35,8 +35,8 @@ struct userdiff_driver;\n /**\n  * the internal representation for a single file (blob).  It records the blob\n  * object name (if known -- for a work tree file it typically is a NUL SHA-1),\n- * filemode and pathname.  This is what the `diff_addremove()`, `diff_change()`\n- * and `diff_unmerge()` synthesize and feed `diff_queue()` function with.\n+ * filemode and pathname.  This is what `diff_addremove()`, `diff_change()`,\n+ * `diff_same()` and `diff_unmerge()` synthesize and feed `diff_queue()`.\n  */\n struct diff_filespec {\n \tstruct object_id oid;\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 5988148b60..c5e2cf6a69 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -167,8 +167,12 @@ static int emit_diff_first_parent_only(struct diff_options *opt, struct combine_\n {\n \tstruct combine_diff_parent *p0 = &p->parent[0];\n \tif (p->mode && p0->mode) {\n-\t\topt->change(opt, p0->mode, p->mode, &p0->oid, &p->oid,\n-\t\t\t1, 1, p->path, 0, 0);\n+\t\tif (opt->flags.find_copies_harder && opt->keep &&\n+\t\t    p0->mode == p->mode && oideq(&p0->oid, &p->oid))\n+\t\t\topt->keep(opt, p->mode, &p->oid, p->path);\n+\t\telse\n+\t\t\topt->change(opt, p0->mode, p->mode, &p0->oid, &p->oid,\n+\t\t\t\t    1, 1, p->path, 0, 0);\n \t}\n \telse {\n \t\tconst struct object_id *oid;\n-- \n2.52.0\n\n"},{"id":"531625","messageId":"5581a8d8-afe3-4097-8a27-7e52c7e915ce@web.de","threadId":"64556","inReplyTo":"9a253514-376f-49fd-99fe-f076ecb180b6@web.de","subject":"Re: [PATCH v2] diff-index: don't queue unchanged filepairs with diff_change()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-12-03T15:06:46Z","receivedAt":"2025-12-03T15:06:48Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 12/2/25 11:07 PM, René Scharfe wrote:\n> On 11/30/25 7:02 PM, Junio C Hamano wrote:\n>> René Scharfe <l.s.r@web.de> writes:\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 a single filespec\n>>> for each filepair and uses it twice with reference counting.  This has a\n>>> measurable effect if there are a lot of them, like in the Linux repo:\n>>>\n>>> Benchmark 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>>>\n>>> Benchmark 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>>>\n>>> Summary\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>>\n>> Nice.  Is this technique only applicable to diff-index among the\n>> three diff plumbing siblings?\n> \n>> [...] it would apply to diff-tree, wouldn't it?\n> Yes, but its diff_change() call is behind two layers of callbacks, which\n> complicates things.\n> \n> And I don't know how to avoid adding an object ID comparison.  Do we\n> perhaps have that bit somewhere in tree-diff.c already and can pass it\n> along the pathchange call?\n\nYes, new patch below.  Not sure if it's better, though.\n\n> Benchmark 1: ./git_v2.52.0 diff-tree --find-copies-harder -r v2.51.0 v2.51.1\n>   Time (mean ± σ):      78.3 ms ±   0.2 ms    [User: 57.4 ms, System: 19.8 ms]\n>   Range (min … max):    77.9 ms …  78.7 ms    36 runs\n> \n> Benchmark 2: ./git diff-tree --find-copies-harder -r v2.51.0 v2.51.1\n>   Time (mean ± σ):      78.8 ms ±   0.2 ms    [User: 57.9 ms, System: 19.8 ms]\n>   Range (min … max):    78.4 ms …  79.2 ms    36 runs\n> \n> Summary\n>   ./git_v2.52.0 diff-tree --find-copies-harder -r v2.51.0 v2.51.1 ran\n>     1.01 ± 0.00 times faster than ./git diff-tree --find-copies-harder -r v2.51.0 v2.51.1\nBenchmark 1: ./git_v2.52.0 diff-tree --find-copies-harder -r v2.51.0 v2.51.1\n  Time (mean ± σ):      75.6 ms ±   0.6 ms    [User: 57.4 ms, System: 17.2 ms]\n  Range (min … max):    75.0 ms …  78.0 ms    37 runs\n\nBenchmark 2: ./git diff-tree --find-copies-harder -r v2.51.0 v2.51.1\n  Time (mean ± σ):      76.0 ms ±   0.2 ms    [User: 57.9 ms, System: 17.1 ms]\n  Range (min … max):    75.6 ms …  76.4 ms    37 runs\n\nSummary\n  ./git_v2.52.0 diff-tree --find-copies-harder -r v2.51.0 v2.51.1 ran\n    1.00 ± 0.01 times faster than ./git diff-tree --find-copies-harder -r v2.51.0 v2.51.1\n\nHmm, I probably ran some background task when I measured yesterday and got\nworse results for the git_v2.52.0 baseline than today.\n\nRené\n\n---\n builtin/reset.c |  1 +\n diff.c          |  1 +\n diff.h          |  9 +++++++--\n diffcore.h      |  4 ++--\n tree-diff.c     | 48 +++++++++++++++++++++++++++---------------------\n 5 files changed, 38 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex ed35802af1..ec674694dd 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -210,6 +210,7 @@ static int read_from_tree(const struct pathspec *pathspec,\n \topt.repo = the_repository;\n \topt.change = diff_change;\n \topt.add_remove = diff_addremove;\n+\topt.keep = diff_same;\n \n \tif (pathspec->nr && pathspec_needs_expanded_index(the_repository->index, pathspec))\n \t\tensure_full_index(the_repository->index);\ndiff --git a/diff.c b/diff.c\nindex 436da250eb..9671524d2b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4847,6 +4847,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)\n \t/* pathchange left =NULL by default */\n \toptions->change = diff_change;\n \toptions->add_remove = diff_addremove;\n+\toptions->keep = diff_same;\n \toptions->use_color = diff_use_color_default;\n \toptions->detect_rename = diff_detect_rename_default;\n \toptions->xdl_opts |= diff_algorithm;\ndiff --git a/diff.h b/diff.h\nindex 7eb84aadf4..2e3a5ac04a 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -43,7 +43,8 @@ struct oidset;\n  * set_default in diff_options can be used to tweak this more.\n  *\n  * - As you find different pairs of files, call `diff_change()` to feed\n- * modified files, `diff_addremove()` to feed created or deleted files, or\n+ * modified files, `diff_addremove()` to feed created or deleted files,\n+ * `diff_same()` to feed unmodified files if needed for copy detection, or\n  * `diff_unmerge()` to feed a file whose state is 'unmerged' to the API.\n  * These are thin wrappers to a lower-level `diff_queue()` function that is\n  * flexible enough to record any of these kinds of changes.\n@@ -76,7 +77,7 @@ struct rev_info;\n struct userdiff_driver;\n \n typedef int (*pathchange_fn_t)(struct diff_options *options,\n-\t\t struct combine_diff_path *path);\n+\t\t struct combine_diff_path *path, bool is_change);\n \n typedef void (*change_fn_t)(struct diff_options *options,\n \t\t unsigned old_mode, unsigned new_mode,\n@@ -92,6 +93,9 @@ typedef void (*add_remove_fn_t)(struct diff_options *options,\n \t\t    int oid_valid,\n \t\t    const char *fullpath, unsigned dirty_submodule);\n \n+typedef void (*keep_fn_t)(struct diff_options *options, unsigned mode,\n+\t\t\t  const struct object_id *oid, const char *fullpath);\n+\n typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,\n \t\tstruct diff_options *options, void *data);\n \n@@ -384,6 +388,7 @@ struct diff_options {\n \tpathchange_fn_t pathchange;\n \tchange_fn_t change;\n \tadd_remove_fn_t add_remove;\n+\tkeep_fn_t keep;\n \tvoid *change_fn_data;\n \tdiff_format_fn_t format_callback;\n \tvoid *format_callback_data;\ndiff --git a/diffcore.h b/diffcore.h\nindex 9c0a0e7aaf..64b419b33f 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -35,8 +35,8 @@ struct userdiff_driver;\n /**\n  * the internal representation for a single file (blob).  It records the blob\n  * object name (if known -- for a work tree file it typically is a NUL SHA-1),\n- * filemode and pathname.  This is what the `diff_addremove()`, `diff_change()`\n- * and `diff_unmerge()` synthesize and feed `diff_queue()` function with.\n+ * filemode and pathname.  This is what `diff_addremove()`, `diff_change()`,\n+ * `diff_same()` and `diff_unmerge()` synthesize and feed `diff_queue()`.\n  */\n struct diff_filespec {\n \tstruct object_id oid;\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 5988148b60..e711456766 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -163,12 +163,17 @@ static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)\n  * emits diff to first parent only, and tells diff tree-walker that we are done\n  * with p and it can be freed.\n  */\n-static int emit_diff_first_parent_only(struct diff_options *opt, struct combine_diff_path *p)\n+static int emit_diff_first_parent_only(struct diff_options *opt,\n+\t\t\t\t       struct combine_diff_path *p,\n+\t\t\t\t       bool is_change)\n {\n \tstruct combine_diff_parent *p0 = &p->parent[0];\n \tif (p->mode && p0->mode) {\n-\t\topt->change(opt, p0->mode, p->mode, &p0->oid, &p->oid,\n-\t\t\t1, 1, p->path, 0, 0);\n+\t\tif (is_change)\n+\t\t\topt->change(opt, p0->mode, p->mode, &p0->oid, &p->oid,\n+\t\t\t\t    1, 1, p->path, 0, 0);\n+\t\telse if (opt->keep)\n+\t\t\topt->keep(opt, p->mode, &p->oid, p->path);\n \t}\n \telse {\n \t\tconst struct object_id *oid;\n@@ -205,7 +210,7 @@ static int emit_diff_first_parent_only(struct diff_options *opt, struct combine_\n static void emit_path(struct combine_diff_path ***tail,\n \t\t      struct strbuf *base, struct diff_options *opt,\n \t\t      int nparent, struct tree_desc *t, struct tree_desc *tp,\n-\t\t      int imin, int depth)\n+\t\t      int imin, int depth, bool is_change)\n {\n \tunsigned short mode;\n \tconst char *path;\n@@ -288,7 +293,7 @@ static void emit_path(struct combine_diff_path ***tail,\n \n \t\tkeep = 1;\n \t\tif (opt->pathchange)\n-\t\t\tkeep = opt->pathchange(opt, p);\n+\t\t\tkeep = opt->pathchange(opt, p, is_change);\n \n \t\tif (keep) {\n \t\t\t**tail = p;\n@@ -518,26 +523,27 @@ static void ll_diff_tree_paths(\n \t\t/* t = p[imin] */\n \t\tif (cmp == 0) {\n \t\t\t/* are either pi > p[imin] or diff(t,pi) != ø ? */\n-\t\t\tif (!opt->flags.find_copies_harder) {\n-\t\t\t\tfor (i = 0; i < nparent; ++i) {\n-\t\t\t\t\t/* p[i] > p[imin] */\n-\t\t\t\t\tif (tp[i].entry.mode & S_IFXMIN_NEQ)\n-\t\t\t\t\t\tcontinue;\n+\t\t\tbool is_change = true;\n \n-\t\t\t\t\t/* diff(t,pi) != ø */\n-\t\t\t\t\tif (!oideq(&t.entry.oid, &tp[i].entry.oid) ||\n-\t\t\t\t\t    (t.entry.mode != tp[i].entry.mode))\n-\t\t\t\t\t\tcontinue;\n+\t\t\tfor (i = 0; i < nparent; ++i) {\n+\t\t\t\t/* p[i] > p[imin] */\n+\t\t\t\tif (tp[i].entry.mode & S_IFXMIN_NEQ)\n+\t\t\t\t\tcontinue;\n \n-\t\t\t\t\tgoto skip_emit_t_tp;\n-\t\t\t\t}\n+\t\t\t\t/* diff(t,pi) != ø */\n+\t\t\t\tif (!oideq(&t.entry.oid, &tp[i].entry.oid) ||\n+\t\t\t\t    (t.entry.mode != tp[i].entry.mode))\n+\t\t\t\t\tcontinue;\n+\n+\t\t\t\tis_change = false;\n+\t\t\t\tbreak;\n \t\t\t}\n \n \t\t\t/* D += {δ(t,pi) if pi=p[imin];  \"+a\" if pi > p[imin]} */\n-\t\t\temit_path(tail, base, opt, nparent,\n-\t\t\t\t  &t, tp, imin, depth);\n+\t\t\tif (is_change || opt->flags.find_copies_harder)\n+\t\t\t\temit_path(tail, base, opt, nparent,\n+\t\t\t\t\t  &t, tp, imin, depth, is_change);\n \n-\t\tskip_emit_t_tp:\n \t\t\t/* t↓,  ∀ pi=p[imin]  pi↓ */\n \t\t\tupdate_tree_entry(&t);\n \t\t\tupdate_tp_entries(tp, nparent);\n@@ -547,7 +553,7 @@ static void ll_diff_tree_paths(\n \t\telse if (cmp < 0) {\n \t\t\t/* D += \"+t\" */\n \t\t\temit_path(tail, base, opt, nparent,\n-\t\t\t\t  &t, /*tp=*/NULL, -1, depth);\n+\t\t\t\t  &t, /*tp=*/NULL, -1, depth, true);\n \n \t\t\t/* t↓ */\n \t\t\tupdate_tree_entry(&t);\n@@ -563,7 +569,7 @@ static void ll_diff_tree_paths(\n \t\t\t}\n \n \t\t\temit_path(tail, base, opt, nparent,\n-\t\t\t\t  /*t=*/NULL, tp, imin, depth);\n+\t\t\t\t  /*t=*/NULL, tp, imin, depth, true);\n \n \t\tskip_emit_tp:\n \t\t\t/* ∀ pi=p[imin]  pi↓ */\n-- \n2.52.0\n\n"}]}