{"thread":{"id":"64088","subject":"[PATCH] combine-diff: don't override recursive flag","startedAt":"2025-09-04T11:54:04Z","lastAt":"2025-09-04T16:54:12Z","messageCount":2,"participants":["Toon Claes","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"525492","messageId":"20250904-toon-fix-last-modified-v1-1-91bf87ddf62b@iotcl.com","threadId":"64088","inReplyTo":null,"subject":"[PATCH] combine-diff: don't override recursive flag","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2025-09-04T11:53:40Z","receivedAt":"2025-09-04T11:54:04Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Function diff_tree_combined() copies the 'struct diff_options' from the\ninput 'struct rev_info' to override some flags. One flag was\n'recursive', which is set to 1. This has been the case since the\ninception of this function in af3feefa1d (diff-tree -c: show a merge\ncommit a bit more sensibly., 2006-01-24). From that commit there's no\nclear indication why recursive is overridden. But it seems to be\nincorrect.\n\nLooking at a test case in t4013. The expected test results are in\nt/t4013/. And take 'diff.noellipses-diff-tree_-c_--abbrev_master' as an\nexample:\n\n   $ git diff-tree -c --abbrev master\n   59d314ad6f356dd08601a4cd5e530381da3e3c64\n   ::100644 100644 100644 cead32e 7289e35 992913c MM\tdir/sub\n   ::100644 100644 100644 b414108 f4615da 10a8a9f MM\tfile0\n   $\n\nHere we see the expected results include 'dir/sub', while argument\n'-r' (for recursive) was not given. So we should expect 'dir' instead.\n\nStop overriding the 'recursive' flag in diff_tree_combined(). This\nchanges the expected results for the example above to be:\n\n    $ git diff-tree -c --abbrev master\n    59d314ad6f356dd08601a4cd5e530381da3e3c64\n    ::040000 040000 040000 65f5c9d f977ed4 0564e02 MM\tdir\n    ::100644 100644 100644 b414108 f4615da 10a8a9f MM\tfile0\n    $\n\nThis also fixes the recently added subcommand git-last-modified(1),\nwhich in some cases (when merges are involved) exited with the error:\n\n    BUG: paths remaining beyond boundary in last-modified\n\nThe last-modified machinery uses a hashmap for all the files it wants to\nget the last-modified commit for. Through log_tree_commit() the callback\nmark_path() is called. Here the incoming path is looked up in the\nhashmap, but because the diff-tree machinery internally switched to\nrecursive (even if the last-modified machinery wasn't) the entry for\n'dir' was never expelled from the hashmap, and the BUG() statement was\nhit.\n\nBecause internally diff-tree no longer runs recursive, this results in a\nnice speedup when running `git last-modified` on git.git:\n\n    Benchmark 1: next\n      Time (mean ± σ):      6.068 s ±  0.038 s    [User: 5.268 s, System: 0.187 s]\n      Range (min … max):    6.007 s …  6.123 s    10 runs\n\n    Benchmark 2: HEAD\n      Time (mean ± σ):      3.057 s ±  0.009 s    [User: 2.996 s, System: 0.055 s]\n      Range (min … max):    3.047 s …  3.070 s    10 runs\n\n    Summary\n      HEAD ran\n        1.98 ± 0.01 times faster than next\n\nBesides that, set the flag 'recursive' in submodule.c to 1 because it\ncalls diff_tree_combined_merge() which no longer enables the flag\nitself.\n\nSigned-off-by: Toon Claes <toon@iotcl.com>\n---\ngit-last-modified(1) was recently merged into 'next', but upon use\nI've (well actually Patrick) noticed it has a bug:\n\n    $ git last-modified\n    3f3e11118993fe0500b3957ab798e39caaa3952f\t.gitignore\n    3f3e11118993fe0500b3957ab798e39caaa3952f\tMakefile\n    3f3e11118993fe0500b3957ab798e39caaa3952f\tbuiltin.h\n    3f3e11118993fe0500b3957ab798e39caaa3952f\tcommand-list.txt\n    3f3e11118993fe0500b3957ab798e39caaa3952f\tcommit-graph.c\n    3f3e11118993fe0500b3957ab798e39caaa3952f\tgit.c\n    3f3e11118993fe0500b3957ab798e39caaa3952f\tmeson.build\n    56072ff0384da5d874fc378d36e089a18f28f1e3\tfetch-pack.c\n    457534d0417d047b943f76a849f256b739894ce9\tprogress.c\n    0d8f4ccfe3b13bb5eb95f030dc5fe76efb255397\tfor-each-ref.h\n    109c3df14ccf372c2438a470bdfb566265399f0a\tcombine-diff.c\n    109c3df14ccf372c2438a470bdfb566265399f0a\tdiff.c\n    109c3df14ccf372c2438a470bdfb566265399f0a\tdiff.h\n    109c3df14ccf372c2438a470bdfb566265399f0a\tdir.c\n    [snip]\n    9e5878fbede57c0499133adf73844261849cd7b2\tgit-web--browse.sh\n    b2fb3911eab730a08168c7f85a7935ad5a330b53\tconfig.mak.in\n    36268b762c4aa6a0d4831f69852b20ab545aff4d\tLGPL-2.1\n    1e58dba142c673c59fbb9d10aeecf62217d4fc9c\taclocal.m4\n    9517e6b84357252e1882091343661c34d978771e\tlevenshtein.h\n    703601d6780c32d33dadf19b2b367f2f79e1e34c\tCOPYING\n    BUG: ../builtin/last-modified.c:236: paths remaining beyond boundary in last-modified\n    zsh: IOT instruction (core dumped)  git last-modified\n\nI didn't see this before but the BUG() statement was only moved up in\nthe last version of the patch series, and it only comes into trouble\nwith merge commits.\n\nThis series fixes that issue.\n\nThis patch is based on 'next' at 1ba7204a04 (Merge branch\n'kh/doc-markup-fixes' into next, 2025-09-03).\n---\n combine-diff.c                                       |  1 -\n submodule.c                                          |  1 +\n t/t4013/diff.diff-tree_-c_--abbrev_master            |  2 +-\n t/t4013/diff.diff-tree_-c_-r_master                  |  5 +++++\n t/t4013/diff.diff-tree_-c_master                     |  2 +-\n t/t4013/diff.noellipses-diff-tree_-c_--abbrev_master |  2 +-\n t/t8020-last-modified.sh                             | 16 ++++++++++++++++\n 7 files changed, 25 insertions(+), 4 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 3878faabe7bb2f7c80cffbf3add6123f17960627..305414efdf436d53fee8d79aa4219f6a4dd3445e 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1515,7 +1515,6 @@ void diff_tree_combined(const struct object_id *oid,\n \n \tdiffopts = *opt;\n \tcopy_pathspec(&diffopts.pathspec, &opt->pathspec);\n-\tdiffopts.flags.recursive = 1;\n \tdiffopts.flags.allow_external = 0;\n \n \t/* find set of paths that everybody touches\ndiff --git a/submodule.c b/submodule.c\nindex fff3c755703163da423d5978b8bdf0d36c6f8ea9..1c273ec87fa865b635ffb9343cfd28744524c737 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -916,6 +916,7 @@ static void collect_changed_submodules(struct repository *r,\n \t\tdiff_rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n \t\tdiff_rev.diffopt.format_callback = collect_changed_submodules_cb;\n \t\tdiff_rev.diffopt.format_callback_data = &data;\n+\t\tdiff_rev.diffopt.flags.recursive = 1;\n \t\tdiff_rev.dense_combined_merges = 1;\n \t\tdiff_tree_combined_merge(commit, &diff_rev);\n \t\trelease_revisions(&diff_rev);\ndiff --git a/t/t4013/diff.diff-tree_-c_--abbrev_master b/t/t4013/diff.diff-tree_-c_--abbrev_master\nindex b8e4aa2530717abc7d7b8bea62f12d615dfcd759..1f00877ca5ee513aebb185cc1a2827783a6af618 100644\n--- a/t/t4013/diff.diff-tree_-c_--abbrev_master\n+++ b/t/t4013/diff.diff-tree_-c_--abbrev_master\n@@ -1,5 +1,5 @@\n $ git diff-tree -c --abbrev master\n 59d314ad6f356dd08601a4cd5e530381da3e3c64\n-::100644 100644 100644 cead32e... 7289e35... 992913c... MM\tdir/sub\n+::040000 040000 040000 65f5c9d... f977ed4... 0564e02... MM\tdir\n ::100644 100644 100644 b414108... f4615da... 10a8a9f... MM\tfile0\n $\ndiff --git a/t/t4013/diff.diff-tree_-c_-r_master b/t/t4013/diff.diff-tree_-c_-r_master\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..8980be8744079119a7b789947d3442d8f8e6d404\n--- /dev/null\n+++ b/t/t4013/diff.diff-tree_-c_-r_master\n@@ -0,0 +1,5 @@\n+$ git diff-tree -c -r master\n+59d314ad6f356dd08601a4cd5e530381da3e3c64\n+::100644 100644 100644 cead32e925b1420c84c14cbf7cf755e7e45af8ad 7289e35bff32727c08dda207511bec138fdb9ea5 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 MM\tdir/sub\n+::100644 100644 100644 b414108e81e5091fe0974a1858b4d0d22b107f70 f4615da674c09df322d6ba8d6b21ecfb1b1ba510 10a8a9f3657f91a156b9f0184ed79a20adef9f7f MM\tfile0\n+$\ndiff --git a/t/t4013/diff.diff-tree_-c_master b/t/t4013/diff.diff-tree_-c_master\nindex e2d2bb26114ac886fbf2467dc7a33ec8cfab5daf..1ecc2cfc6c50e923a671fba29a6a395f4240e0ff 100644\n--- a/t/t4013/diff.diff-tree_-c_master\n+++ b/t/t4013/diff.diff-tree_-c_master\n@@ -1,5 +1,5 @@\n $ git diff-tree -c master\n 59d314ad6f356dd08601a4cd5e530381da3e3c64\n-::100644 100644 100644 cead32e925b1420c84c14cbf7cf755e7e45af8ad 7289e35bff32727c08dda207511bec138fdb9ea5 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 MM\tdir/sub\n+::040000 040000 040000 65f5c9dd60ce3b2b3324b618ac7accf8d912c113 f977ed46ae6873c1c30ab878e15a4accedc3618b 0564e026437809817a64fff393079714b6dd4628 MM\tdir\n ::100644 100644 100644 b414108e81e5091fe0974a1858b4d0d22b107f70 f4615da674c09df322d6ba8d6b21ecfb1b1ba510 10a8a9f3657f91a156b9f0184ed79a20adef9f7f MM\tfile0\n $\ndiff --git a/t/t4013/diff.noellipses-diff-tree_-c_--abbrev_master b/t/t4013/diff.noellipses-diff-tree_-c_--abbrev_master\nindex bb80f013b37da0f11a9e7d454222487bd2f61d5a..62d655dac240a8bb6334c7247913f24bb4641cfc 100644\n--- a/t/t4013/diff.noellipses-diff-tree_-c_--abbrev_master\n+++ b/t/t4013/diff.noellipses-diff-tree_-c_--abbrev_master\n@@ -1,5 +1,5 @@\n $ git diff-tree -c --abbrev master\n 59d314ad6f356dd08601a4cd5e530381da3e3c64\n-::100644 100644 100644 cead32e 7289e35 992913c MM\tdir/sub\n+::040000 040000 040000 65f5c9d f977ed4 0564e02 MM\tdir\n ::100644 100644 100644 b414108 f4615da 10a8a9f MM\tfile0\n $\ndiff --git a/t/t8020-last-modified.sh b/t/t8020-last-modified.sh\nindex 5eb4cef0359212eeddc68578697ca432760e0be3..ff837a40f40b8c4f92c4975e29615a507a5d3b2d 100755\n--- a/t/t8020-last-modified.sh\n+++ b/t/t8020-last-modified.sh\n@@ -128,6 +128,22 @@ test_expect_success 'only last-modified files in the current tree' '\n \tEOF\n '\n \n+test_expect_success 'last-modified with files in a subdir merged' '\n+\tgit checkout -b branch-k1 1 &&\n+\tmkdir a k &&\n+\ttest_commit k1 a/file2 &&\n+\tgit checkout -b branch-k2 &&\n+\ttest_commit k2 k/file2 &&\n+\tgit checkout branch-k1 &&\n+\ttest_merge km2 branch-k2 &&\n+\ttest_merge km3 3 &&\n+\tcheck_last_modified <<-\\EOF\n+\tkm3 a\n+\tk2 k\n+\t1 file\n+\tEOF\n+'\n+\n test_expect_success 'cross merge boundaries in blaming' '\n \tgit checkout HEAD^0 &&\n \tgit rm -rf . &&\n\n---\nbase-commit: 1ba7204a041bf9fa3af3ad21a018399fff66f7b9\nchange-id: 20250902-toon-fix-last-modified-865172060265\n\nBest regards,\n-- \nToon Claes <toon@iotcl.com>\n\n"},{"id":"525550","messageId":"xmqqecsm9n0u.fsf@gitster.g","threadId":"64088","inReplyTo":"20250904-toon-fix-last-modified-v1-1-91bf87ddf62b@iotcl.com","subject":"Re: [PATCH] combine-diff: don't override recursive flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-04T16:54:09Z","receivedAt":"2025-09-04T16:54:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Toon Claes <toon@iotcl.com> writes:\n\n> Because internally diff-tree no longer runs recursive, this results in a\n> nice speedup when running `git last-modified` on git.git:\n\nAnd users of \"git diff-tree -c\" now needs to pass \"-r\" themselves to\nobtain the result they expect to see?\n\nDo we need to retian backward compatibility there?  As things like this ...\n\n> diff --git a/t/t4013/diff.diff-tree_-c_master b/t/t4013/diff.diff-tree_-c_master\n> index e2d2bb26114ac886fbf2467dc7a33ec8cfab5daf..1ecc2cfc6c50e923a671fba29a6a395f4240e0ff 100644\n> --- a/t/t4013/diff.diff-tree_-c_master\n> +++ b/t/t4013/diff.diff-tree_-c_master\n> @@ -1,5 +1,5 @@\n>  $ git diff-tree -c master\n>  59d314ad6f356dd08601a4cd5e530381da3e3c64\n> -::100644 100644 100644 cead32e925b1420c84c14cbf7cf755e7e45af8ad 7289e35bff32727c08dda207511bec138fdb9ea5 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 MM\tdir/sub\n> +::040000 040000 040000 65f5c9dd60ce3b2b3324b618ac7accf8d912c113 f977ed46ae6873c1c30ab878e15a4accedc3618b 0564e026437809817a64fff393079714b6dd4628 MM\tdir\n>  ::100644 100644 100644 b414108e81e5091fe0974a1858b4d0d22b107f70 f4615da674c09df322d6ba8d6b21ecfb1b1ba510 10a8a9f3657f91a156b9f0184ed79a20adef9f7f MM\tfile0\n>  $\n\n... is a clear regression of age old expectation.\n\nI do not mind too much if you did want to make a breaking change and\nbe honest about it, but then I would expect you would be adding \"-r\"\non the command line without changing the expected output in a test\nlike the above one, i.e. I would expect that the above should look\nmore like this:\n\n> -$ git diff-tree -c master\n> +$ git diff-tree -r -c master\n>  59d314ad6f356dd08601a4cd5e530381da3e3c64\n>  ::100644 100644 100644 cead32e925b1420c84c14cbf7cf755e7e45af8ad 7289e35bff32727c08dda207511bec138fdb9ea5 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 MM\tdir/sub\n>  ::100644 100644 100644 b414108e81e5091fe0974a1858b4d0d22b107f70 f4615da674c09df322d6ba8d6b21ecfb1b1ba510 10a8a9f3657f91a156b9f0184ed79a20adef9f7f MM\tfile0\n\n"}]}