{"thread":{"id":"57784","subject":"gitk regression in version 2.36.0","startedAt":"2022-04-23T05:25:28Z","lastAt":"2022-04-27T20:04:08Z","messageCount":16,"participants":["Matthias Aßhauer","Junio C Hamano","René Scharfe","Phillip Wood"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"454324","messageId":"AM0PR04MB6019ECF053F1FB7B29D75AB7A5F69@AM0PR04MB6019.eurprd04.prod.outlook.com","threadId":"57784","inReplyTo":null,"subject":"gitk regression in version 2.36.0","fromName":"Matthias Aßhauer","fromEmail":"mha1993@live.de","sentAt":"2022-04-23T05:25:15Z","receivedAt":"2022-04-23T05:25:28Z","isPatch":false,"sender":{"key":"mha1993@live.de","avatar":"https://avatars.githubusercontent.com/u/6178234?v=4"},"body":"Git 2.36.0 (or more precisely 244c27242f (diff.[ch]: have diff_free() call \nclear_pathspec(opts.pathspec), 2022-02-16)) introduced some change in \nbehaviour that causes gitks highlight feature not to work correctly \nanymore.\n\nHere's a quick reproducer based on git.git:\n\ngit checkout 244c27242f44e6b88e3a381c90bde08d134c274b~1\nmake install\ngit checkout 244c27242f44e6b88e3a381c90bde08d134c274b\nPATH=~/bin:$PATH ~/bin/gitk\n# In commit 4c53a8c20f (Git 2.35.1, 2022-01-28) (2nd from the top)\n# right click GIT-VERSION-GEN and select \"Highlight this only\".\n# You'll see 4c53a8c20f (Git 2.35.1, 2022-01-28) and\n# 89bece5c8c (Git 2.35, 2022-01-24) highlighted, but not the surrounding\n# commits. Exit gitk.\nmake install\nPATH=~/bin:$PATH ~/bin/gitk\n# In commit 4c53a8c20f (Git 2.35.1, 2022-01-28) (2nd from the top)\n# right click GIT-VERSION-GEN and select \"Highlight this only\".\n# Almost every non-merge commmit will be highlighted.\n\nI think this is a change in behaviour in `git diff-tree`, but I'm honestly \nnot sure what arguments gitk passes to `git diff-tree`, so I'm struggling \nto figure out what exactly changed.\n\nThis issue was originally reported as a Git for Windows issue [1], but I \ncan also reproduce it on Linux.\n\n[1] https://github.com/git-for-windows/git/issues/3815\n"},{"id":"454325","messageId":"xmqqpml82vkd.fsf@gitster.g","threadId":"57784","inReplyTo":"AM0PR04MB6019ECF053F1FB7B29D75AB7A5F69@AM0PR04MB6019.eurprd04.prod.outlook.com","subject":"Re: gitk regression in version 2.36.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-23T05:54:58Z","receivedAt":"2022-04-23T05:55:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthias Aßhauer writes:\n\n> Git 2.36.0 (or more precisely 244c27242f (diff.[ch]: have diff_free()\n> call clear_pathspec(opts.pathspec), 2022-02-16)) introduced some\n> change in behaviour that causes gitks highlight feature not to work\n> correctly anymore.\n\nNicely found.\n\nI think what happens is that gitk REPEATEDLY calls log_tree_commit()\nby opening a pipe to \"git diff-tree -r -s --stdin $gdtargs\" and feed\nrevisions from its standard input. \n\n builtin/diff-tree.c::diff_tree_stdin()\n -> builtin/diff-tree.c::stdin_diff_commit()\n -> log-tree.c::log_tree_commit()\n\nNow, log-tree.c::log_tree_commit() is responsible for formating the\ndifference between the commit and its parent, using the\nopt->diffopt. After computing the pairwise diff for one tree pair\n(commit and its parent), of course it calls diff_free() to flush the\nqueued diff and release other resources we allocated while showing\nthat single diff. Before the broken commit 244c27242f, diff_free()\nreleased ONLY the newly allocated resources that we needed to\ncompute one pair of trees. Most importantly, the settings used to\ncompute diffs that are reused to compute the next pair of trees need\nto be kept, e.g. pathspec.\n\nRemember, we are talking about log-tree, the upstream of the pipe\ngoing from the tip of the history and traversing the parent chain to\nancestors, so once we are done with showing the diff between our\ncurrent commit and its parent, we will move to the parent and\ncompute the same diff with its parent (i.e. our grand-parent).\n\nBut with the regression, we mistakenly release the pathspec, so the\nfirst commit may use the specified pathspec, but the second and\nsubsequent ones will lose the pathspec the user gave us, making the\ncomparison tree-wide one, not limited with any pathspec.\n\nI am reasonably sure that reverting that commit will be the right\nthing to do. It is somewhat unfortunate that it would reintroduce\nresource leaks that having clear_pathspec() in a wrong place (i.e.\ndiff_free()) was covering up. We should instead need to find the\nplace where a diff_opt struct goes out of scope (after being used\nfor zero or more times, calling diff_free() after each iteration to\nrelease resources consumed per-iteration) and call clear_pathspec().\n\nThanks for a report.\n"},{"id":"454326","messageId":"xmqqlevw2v2d.fsf@gitster.g","threadId":"57784","inReplyTo":"xmqqpml82vkd.fsf@gitster.g","subject":"Re: gitk regression in version 2.36.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-23T06:05:46Z","receivedAt":"2022-04-23T06:05:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Matthias Aßhauer writes:\n>\n>> Git 2.36.0 (or more precisely 244c27242f (diff.[ch]: have diff_free()\n>> call clear_pathspec(opts.pathspec), 2022-02-16)) introduced some\n>> change in behaviour that causes gitks highlight feature not to work\n>> correctly anymore.\n>\n> Nicely found.\n\nA simple reproduction recipe without gitk is a command line\ninvocation like this:\n\n$ git rev-list -10 --max-parents=1 v2.36.0 -- Documentation | \n git diff-tree --stdin --stat -- Documentation\n\nThe upstream of the pipe lists 10 topmost non-merge commits, going\nback from v2.36.0, that touch Documentation/ directory, and the\ndownstream \"diff-tree\" is told to show for each of these commits to\ncompute equivalent of \"git show --stat -- Documentation\", i.e. only\nthe Documentation directory. But \"diff-tree\" loses pathspec and we\nwill see paths outside Documentation appearing in the output.\n\nIf I substitute \"git diff-tree\" on the downstream of the pipe with\nthe version from v2.35.0, of course the correct thing happens X-<.\n"},{"id":"454330","messageId":"8424860b-e557-55b1-49a0-246a2eb112d4@web.de","threadId":"57784","inReplyTo":"AM0PR04MB6019ECF053F1FB7B29D75AB7A5F69@AM0PR04MB6019.eurprd04.prod.outlook.com","subject":"Re: gitk regression in version 2.36.0","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-04-23T09:27:40Z","receivedAt":"2022-04-23T09:27:48Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 23.04.22 um 07:25 schrieb Matthias Aßhauer:\n> Git 2.36.0 (or more precisely 244c27242f (diff.[ch]: have diff_free()\n> call clear_pathspec(opts.pathspec), 2022-02-16)) introduced some\n> change in behaviour that causes gitks highlight feature not to work\n> correctly anymore.\n>\n> Here's a quick reproducer based on git.git:\n>\n> git checkout 244c27242f44e6b88e3a381c90bde08d134c274b~1\n> make install\n> git checkout 244c27242f44e6b88e3a381c90bde08d134c274b\n> PATH=~/bin:$PATH ~/bin/gitk\n> # In commit 4c53a8c20f (Git 2.35.1, 2022-01-28) (2nd from the top)\n> # right click GIT-VERSION-GEN and select \"Highlight this only\".\n> # You'll see 4c53a8c20f (Git 2.35.1, 2022-01-28) and\n> # 89bece5c8c (Git 2.35, 2022-01-24) highlighted, but not the surrounding\n> # commits. Exit gitk.\n> make install\n> PATH=~/bin:$PATH ~/bin/gitk\n> # In commit 4c53a8c20f (Git 2.35.1, 2022-01-28) (2nd from the top)\n> # right click GIT-VERSION-GEN and select \"Highlight this only\".\n> # Almost every non-merge commmit will be highlighted.\n>\n> I think this is a change in behaviour in `git diff-tree`, but I'm\n> honestly not sure what arguments gitk passes to `git diff-tree`, so\n> I'm struggling to figure out what exactly changed.\n>\n> This issue was originally reported as a Git for Windows issue [1],\n> but I can also reproduce it on Linux.\n>\n> [1] https://github.com/git-for-windows/git/issues/3815\n\ngitk does something like this to find commits that touched that file\n(just with more commits):\n\n # v2.25.3\n $ git rev-parse 4c53a8c20f ff5b7913f0 | git diff-tree -r -s --stdin GIT-VERSION-GEN\n 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a\n\n # 244c27242f (diff.[ch]: have diff_free() call clear_pathspec(opts.pathspec), 2022-02-16))\n $ git rev-parse 4c53a8c20f ff5b7913f0 | git diff-tree -r -s --stdin GIT-VERSION-GEN\n 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a\n ff5b7913f0af62c26682b0376d0aa2d7f5d74b2e\n\nSomewhere in diff-tree a struct diff_options is reused between commits,\nand the caller expects its pathspec to be preserved, but 244c27242f\nclears it. With the path filter gone, the following commits match.\n\nRené\n"},{"id":"454331","messageId":"737bd2f8-dfd7-6a51-b7f5-33eefa33e975@web.de","threadId":"57784","inReplyTo":"xmqqpml82vkd.fsf@gitster.g","subject":"Re: gitk regression in version 2.36.0","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-04-23T10:13:11Z","receivedAt":"2022-04-23T10:13:31Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 23.04.22 um 07:54 schrieb Junio C Hamano:\n> Matthias Aßhauer writes:\n>\n>> Git 2.36.0 (or more precisely 244c27242f (diff.[ch]: have diff_free()\n>> call clear_pathspec(opts.pathspec), 2022-02-16)) introduced some\n>> change in behaviour that causes gitks highlight feature not to work\n>> correctly anymore.\n>\n> I am reasonably sure that reverting that commit will be the right\n> thing to do. It is somewhat unfortunate that it would reintroduce\n> resource leaks that having clear_pathspec() in a wrong place (i.e.\n> diff_free()) was covering up. We should instead need to find the\n> place where a diff_opt struct goes out of scope (after being used\n> for zero or more times, calling diff_free() after each iteration to\n> release resources consumed per-iteration) and call clear_pathspec().\n\nRight; a small memory leak is better than wrong output.\n\nFinding those places is a bit complicated by diff_options often being\nembedded in struct rev_info, though.\n\nRené\n\n\nPS: And I need to learn to download new posts before hitting Send\n(or become faster); sorry for my near-duplicate reply.\n"},{"id":"454340","messageId":"xmqqh76j3i3r.fsf@gitster.g","threadId":"57784","inReplyTo":"737bd2f8-dfd7-6a51-b7f5-33eefa33e975@web.de","subject":"Re: gitk regression in version 2.36.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-23T16:00:24Z","receivedAt":"2022-04-23T16:00:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> I am reasonably sure that reverting that commit will be the right\n>> thing to do. It is somewhat unfortunate that it would reintroduce\n>> resource leaks that having clear_pathspec() in a wrong place (i.e.\n>> diff_free()) was covering up. We should instead need to find the\n>> place where a diff_opt struct goes out of scope (after being used\n>> for zero or more times, calling diff_free() after each iteration to\n>> release resources consumed per-iteration) and call clear_pathspec().\n>\n> Right; a small memory leak is better than wrong output.\n\nYeah, it is an absolute requirement to avoid producing wrong output,\nand when producing output for 100 or 1000 commits, we should not\nleak resources in proportion to the number of these commits\nprocessed, so forgetting to call diff_free() that releases resources\nthat are required per diff-invocation is also a must. Compared to\nthat, cleaning up the resource allocated just once and repeatedly\nused while we handle these 100 or 1000 commits, while it is nice to\ndo so, is of much lower priority, certainly much lower than computing\nthe right result.\n\n> Finding those places is a bit complicated by diff_options often being\n> embedded in struct rev_info, though.\n\nPerhaps, but we should have a resource-releasing helper for rev_info,\nso that may be a good place to do so, hopefully.\n\nThanks\n\n> PS: And I need to learn to download new posts before hitting Send\n> (or become faster); sorry for my near-duplicate reply.\n\nActually this time I very much appreciated an independent validation\nof my findings ;-) Thanks.\n"},{"id":"454390","messageId":"xmqqbkwpvyyc.fsf@gitster.g","threadId":"57784","inReplyTo":"xmqqh76j3i3r.fsf@gitster.g","subject":"[PATCH] 2.36 gitk/diff-tree --stdin regression fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-25T17:45:47Z","receivedAt":"2022-04-25T17:45:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This reverts commit 244c2724 (diff.[ch]: have diff_free() call\nclear_pathspec(opts.pathspec), 2022-02-16).\n\nThe diff_free() call is to be used after a diffopt structure is used\nto compare two sets of paths to release resources that were needed\nonly for that comparison, and keep the data such as pathspec that\nare reused by the diffopt structure to make the next and subsequent\ncomparison (imagine \"git log -p - -- \" where the\noptions and pathspec are kept in the diffopt structure, used to\ncompare HEAD and HEAD~, then used again when HEAD~ and HEAD~2 are\ncompared).\n\nWe by mistake started clearing the pathspec in diff_free(), so\nprograms like gitk that runs\n\n git diff-tree --stdin -- \n\ndownstream of a pipe, processing one commit after another, started\nshowing irrelevant comparison outside the given from the\nsecond commit.\n\nThe buggy commit may have been hiding the places where diff\nmachinery is used only once and called diff_free() to release that\nper-comparison resources, but forgetting to call clear_pathspec() to\nrelease the resource held for the (potentially) repeated comparison,\nand we eventually would want to add clear_pathspec() to clear\nresources to be released after a (potentially repeated) diff session\nis done (if there are similar resources other than pathspec that\nneed to be cleared at the end, we should then know where to clear\nthem), but that is \"per program invocation\" leak that will be\ncleaned up by calling exit(3) and of lower priority than fixing this\nbehavior-breaking regression.\n\nReported-by: Matthias Aßhauer \nHelped-by: René Scharfe \nSigned-off-by: Junio C Hamano \n---\n add-interactive.c | 6 +++---\n blame.c | 3 +++\n\n builtin/reset.c | 1 +\n diff.c | 1 -\n notes-merge.c | 2 ++\n 5 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex e1ab39cce3..6498ae196f 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -797,14 +797,14 @@ static int run_revert(struct add_i_state *s, const struct pathspec *ps,\n \tdiffopt.flags.override_submodule_config = 1;\n \tdiffopt.repo = s->r;\n \n-\tif (do_diff_cache(&oid, &diffopt)) {\n-\t\tdiff_free(&diffopt);\n+\tif (do_diff_cache(&oid, &diffopt))\n \t\tres = -1;\n-\t} else {\n+\telse {\n \t\tdiffcore_std(&diffopt);\n \t\tdiff_flush(&diffopt);\n \t}\n \tfree(paths);\n+\tclear_pathspec(&diffopt.pathspec);\n \n \tif (!res && write_locked_index(s->r->index, &index_lock,\n \t\t\t\t COMMIT_LOCK) < 0)\ndiff --git a/blame.c b/blame.c\nindex 401990726e..206c295660 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -1403,6 +1403,7 @@ static struct blame_origin *find_origin(struct repository *r,\n \t\t}\n \t}\n \tdiff_flush(&diff_opts);\n+\tclear_pathspec(&diff_opts.pathspec);\n \treturn porigin;\n }\n \n@@ -1446,6 +1447,7 @@ static struct blame_origin *find_rename(struct repository *r,\n \t\t}\n \t}\n \tdiff_flush(&diff_opts);\n+\tclear_pathspec(&diff_opts.pathspec);\n \treturn porigin;\n }\n \n@@ -2326,6 +2328,7 @@ static void find_copy_in_parent(struct blame_scoreboard *sb,\n \t} while (unblamed);\n \ttarget->suspects = reverse_blame(leftover, NULL);\n \tdiff_flush(&diff_opts);\n+\tclear_pathspec(&diff_opts.pathspec);\n }\n \n /*\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 24968dd628..b97745ee94 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -274,6 +274,7 @@ static int read_from_tree(const struct pathspec *pathspec,\n \t\treturn 1;\n \tdiffcore_std(&opt);\n \tdiff_flush(&opt);\n+\tclear_pathspec(&opt.pathspec);\n \n \treturn 0;\n }\ndiff --git a/diff.c b/diff.c\nindex 0aef3db6e1..c862771a58 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6345,7 +6345,6 @@ void diff_free(struct diff_options *options)\n \n \tdiff_free_file(options);\n \tdiff_free_ignore_regex(options);\n-\tclear_pathspec(&options->pathspec);\n }\n \n void diff_flush(struct diff_options *options)\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 7ba40cfb08..b4a3a903e8 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -175,6 +175,7 @@ static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o,\n \t\t oid_to_hex(&mp->remote));\n \t}\n \tdiff_flush(&opt);\n+\tclear_pathspec(&opt.pathspec);\n \n \t*num_changes = len;\n \treturn changes;\n@@ -260,6 +261,7 @@ static void diff_tree_local(struct notes_merge_options *o,\n \t\t oid_to_hex(&mp->local));\n \t}\n \tdiff_flush(&opt);\n+\tclear_pathspec(&opt.pathspec);\n }\n \n static void check_notes_merge_worktree(struct notes_merge_options *o)\n-- \n2.36.0-194-g075c326b54\n\n\n"},{"id":"454415","messageId":"xmqqpml4u6w2.fsf_-_@gitster.g","threadId":"57784","inReplyTo":"xmqqbkwpvyyc.fsf@gitster.g","subject":"[PATCH] t4013: diff-tree --stdin with pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-25T22:37:17Z","receivedAt":"2022-04-25T22:37:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Add a test that feeds \"diff-tree --stdin \" with list of\nrevisions, in a way very similar to how \"gitk\" drives it. If we had\nsuch a test, we would have caught the recent regression.\n\nSigned-off-by: Junio C Hamano \n---\n * On top of that \"revert\" change.\n\n t/t4013-diff-various.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 750aee17ea..628b01f355 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -542,6 +542,20 @@ test_expect_success 'diff-tree --stdin with log formatting' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'diff-tree --stdin with pathspec' '\n+\tcat >expect <<-EOF &&\n+\tThird\n+\n+\tdir/sub\n+\tSecond\n+\n+\tdir/sub\n+\tEOF\n+\tgit rev-list master^ |\n+\tgit diff-tree -r --stdin --name-only --format=%s dir >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'diff -I: setup' '\n \tgit checkout master &&\n \ttest_seq 50 >file0 &&\n-- \n2.36.0-200-g3c19986797\n\n"},{"id":"454423","messageId":"83a76d46-5069-d6c7-b8b3-f3a063637abb@gmail.com","threadId":"57784","inReplyTo":"xmqqbkwpvyyc.fsf@gitster.g","subject":"Re: [PATCH] 2.36 gitk/diff-tree --stdin regression fix","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-04-26T10:09:55Z","receivedAt":"2022-04-26T10:29:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 25/04/2022 18:45, Junio C Hamano wrote:\n> This reverts commit 244c2724 (diff.[ch]: have diff_free() call\n> clear_pathspec(opts.pathspec), 2022-02-16).\n> \n> The diff_free() call is to be used after a diffopt structure is used\n> to compare two sets of paths to release resources that were needed\n> only for that comparison, and keep the data such as pathspec that\n> are reused by the diffopt structure to make the next and subsequent\n> comparison (imagine \"git log -p - -- \" where the\n> options and pathspec are kept in the diffopt structure, used to\n> compare HEAD and HEAD~, then used again when HEAD~ and HEAD~2 are\n> compared).\n> \n> We by mistake started clearing the pathspec in diff_free(), so\n> programs like gitk that runs\n> \n> git diff-tree --stdin -- \n> \n> downstream of a pipe, processing one commit after another, started\n> showing irrelevant comparison outside the given from the\n> second commit.\n\nI notice from the patch context that we are still calling \ndiff_free_ignore_regex(options) which was added in c45dc9cf30 (\"diff: \nplug memory leak from regcomp() on {log,diff} -I\", 2021-02-11). I think \nthat will need reverting as well as it freeing data that is needed when \noptions is reused by \"diff-tree --stdin\" or \"log -p\".\n\nBest Wishes\n\nPhillip\n\n> The buggy commit may have been hiding the places where diff\n> machinery is used only once and called diff_free() to release that\n> per-comparison resources, but forgetting to call clear_pathspec() to\n> release the resource held for the (potentially) repeated comparison,\n> and we eventually would want to add clear_pathspec() to clear\n> resources to be released after a (potentially repeated) diff session\n> is done (if there are similar resources other than pathspec that\n> need to be cleared at the end, we should then know where to clear\n> them), but that is \"per program invocation\" leak that will be\n> cleaned up by calling exit(3) and of lower priority than fixing this\n> behavior-breaking regression.\n> \n> Reported-by: Matthias Aßhauer \n> Helped-by: René Scharfe \n> Signed-off-by: Junio C Hamano \n> ---\n> add-interactive.c | 6 +++---\n> blame.c | 3 +++\n> \n> builtin/reset.c | 1 +\n> diff.c | 1 -\n> notes-merge.c | 2 ++\n> 5 files changed, 9 insertions(+), 4 deletions(-)\n> \n> diff --git a/add-interactive.c b/add-interactive.c\n> index e1ab39cce3..6498ae196f 100644\n> --- a/add-interactive.c\n> +++ b/add-interactive.c\n> @@ -797,14 +797,14 @@ static int run_revert(struct add_i_state *s, const struct pathspec *ps,\n> \tdiffopt.flags.override_submodule_config = 1;\n> \tdiffopt.repo = s->r;\n> \n> -\tif (do_diff_cache(&oid, &diffopt)) {\n> -\t\tdiff_free(&diffopt);\n> +\tif (do_diff_cache(&oid, &diffopt))\n> \t\tres = -1;\n> -\t} else {\n> +\telse {\n> \t\tdiffcore_std(&diffopt);\n> \t\tdiff_flush(&diffopt);\n> \t}\n> \tfree(paths);\n> +\tclear_pathspec(&diffopt.pathspec);\n> \n> \tif (!res && write_locked_index(s->r->index, &index_lock,\n> \t\t\t\t COMMIT_LOCK) < 0)\n> diff --git a/blame.c b/blame.c\n> index 401990726e..206c295660 100644\n> --- a/blame.c\n> +++ b/blame.c\n> @@ -1403,6 +1403,7 @@ static struct blame_origin *find_origin(struct repository *r,\n> \t\t}\n> \t}\n> \tdiff_flush(&diff_opts);\n> +\tclear_pathspec(&diff_opts.pathspec);\n> \treturn porigin;\n> }\n> \n> @@ -1446,6 +1447,7 @@ static struct blame_origin *find_rename(struct repository *r,\n> \t\t}\n> \t}\n> \tdiff_flush(&diff_opts);\n> +\tclear_pathspec(&diff_opts.pathspec);\n> \treturn porigin;\n> }\n> \n> @@ -2326,6 +2328,7 @@ static void find_copy_in_parent(struct blame_scoreboard *sb,\n> \t} while (unblamed);\n> \ttarget->suspects = reverse_blame(leftover, NULL);\n> \tdiff_flush(&diff_opts);\n> +\tclear_pathspec(&diff_opts.pathspec);\n> }\n> \n> /*\n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index 24968dd628..b97745ee94 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -274,6 +274,7 @@ static int read_from_tree(const struct pathspec *pathspec,\n> \t\treturn 1;\n> \tdiffcore_std(&opt);\n> \tdiff_flush(&opt);\n> +\tclear_pathspec(&opt.pathspec);\n> \n> \treturn 0;\n> }\n> diff --git a/diff.c b/diff.c\n> index 0aef3db6e1..c862771a58 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6345,7 +6345,6 @@ void diff_free(struct diff_options *options)\n> \n> \tdiff_free_file(options);\n> \tdiff_free_ignore_regex(options);\n> -\tclear_pathspec(&options->pathspec);\n> }\n> \n> void diff_flush(struct diff_options *options)\n> diff --git a/notes-merge.c b/notes-merge.c\n> index 7ba40cfb08..b4a3a903e8 100644\n> --- a/notes-merge.c\n> +++ b/notes-merge.c\n> @@ -175,6 +175,7 @@ static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o,\n> \t\t oid_to_hex(&mp->remote));\n> \t}\n> \tdiff_flush(&opt);\n> +\tclear_pathspec(&opt.pathspec);\n> \n> \t*num_changes = len;\n> \treturn changes;\n> @@ -260,6 +261,7 @@ static void diff_tree_local(struct notes_merge_options *o,\n> \t\t oid_to_hex(&mp->local));\n> \t}\n> \tdiff_flush(&opt);\n> +\tclear_pathspec(&opt.pathspec);\n> }\n> \n> static void check_notes_merge_worktree(struct notes_merge_options *o)\n"},{"id":"454431","messageId":"6ac7e164-c4ba-0ffc-a8a4-4cede1d4e7ae@gmail.com","threadId":"57784","inReplyTo":"83a76d46-5069-d6c7-b8b3-f3a063637abb@gmail.com","subject":"Re: [PATCH] 2.36 gitk/diff-tree --stdin regression fix","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-04-26T13:45:58Z","receivedAt":"2022-04-26T13:46:05Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 26/04/2022 11:09, Phillip Wood wrote:\n> On 25/04/2022 18:45, Junio C Hamano wrote:\n>> This reverts commit 244c2724 (diff.[ch]: have diff_free() call\n>> clear_pathspec(opts.pathspec), 2022-02-16).\n>>\n>> The diff_free() call is to be used after a diffopt structure is used\n>> to compare two sets of paths to release resources that were needed\n>> only for that comparison, and keep the data such as pathspec that\n>> are reused by the diffopt structure to make the next and subsequent\n>> comparison (imagine \"git log -p - -- \" where the\n>> options and pathspec are kept in the diffopt structure, used to\n>> compare HEAD and HEAD~, then used again when HEAD~ and HEAD~2 are\n>> compared).\n>>\n>> We by mistake started clearing the pathspec in diff_free(), so\n>> programs like gitk that runs\n>>\n>>      git diff-tree --stdin -- \n>>\n>> downstream of a pipe, processing one commit after another, started\n>> showing irrelevant comparison outside the given from the\n>> second commit.\n> \n> I notice from the patch context that we are still calling \n> diff_free_ignore_regex(options) which was added in c45dc9cf30 (\"diff: \n> plug memory leak from regcomp() on {log,diff} -I\", 2021-02-11). I think \n> that will need reverting as well as it freeing data that is needed when \n> options is reused by \"diff-tree --stdin\" or \"log -p\".\n\nOn further inspection we have tests for \"log -p -I\" in t4013 and \ne900d494dc (\"diff: add an API for deferred freeing\", 2021-02-11) \nmodified builtin/log.c to set the new no_free flag so \"log\" should be \nOK. However \"diff-tree --stdin -p -I\" is not as \nbuiltin/diff-tree.c is unchanged by e900d494dc so the no_free flag is \nnot set which I think is the cause of the problems reported here.\n\nI think the close_file changes in e900d494dc should be safe as far as \n\"diff-tree\" is concerned as it never sets that flag.\n\nIn retrospect the no_free flag is pretty ugly and fragile. If we really \ncannot do it another way at least requiring callers to set a flag when \nthey want things freeing would avoid nasty surprises like this at the \nexpense of leaking when the caller forgets to set it. Perhaps once \n2.36.1 is out we should step back and think about exactly what we're \ntrying to achieve by removing these bounded leaks rather than annotating \nthem with UNLEAK().\n\nBest Wishes\n\nPhillip\n\n> Best Wishes\n> \n> Phillip\n> \n>> The buggy commit may have been hiding the places where diff\n>> machinery is used only once and called diff_free() to release that\n>> per-comparison resources, but forgetting to call clear_pathspec() to\n>> release the resource held for the (potentially) repeated comparison,\n>> and we eventually would want to add clear_pathspec() to clear\n>> resources to be released after a (potentially repeated) diff session\n>> is done (if there are similar resources other than pathspec that\n>> need to be cleared at the end, we should then know where to clear\n>> them), but that is \"per program invocation\" leak that will be\n>> cleaned up by calling exit(3) and of lower priority than fixing this\n>> behavior-breaking regression.\n>>\n>> Reported-by: Matthias Aßhauer \n>> Helped-by: René Scharfe \n>> Signed-off-by: Junio C Hamano \n>> ---\n>>   add-interactive.c | 6 +++---\n>>   blame.c           | 3 +++\n>>\n>>   builtin/reset.c   | 1 +\n>>   diff.c            | 1 -\n>>   notes-merge.c     | 2 ++\n>>   5 files changed, 9 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/add-interactive.c b/add-interactive.c\n>> index e1ab39cce3..6498ae196f 100644\n>> --- a/add-interactive.c\n>> +++ b/add-interactive.c\n>> @@ -797,14 +797,14 @@ static int run_revert(struct add_i_state *s, \n>> const struct pathspec *ps,\n>>       diffopt.flags.override_submodule_config = 1;\n>>       diffopt.repo = s->r;\n>> -    if (do_diff_cache(&oid, &diffopt)) {\n>> -        diff_free(&diffopt);\n>> +    if (do_diff_cache(&oid, &diffopt))\n>>           res = -1;\n>> -    } else {\n>> +    else {\n>>           diffcore_std(&diffopt);\n>>           diff_flush(&diffopt);\n>>       }\n>>       free(paths);\n>> +    clear_pathspec(&diffopt.pathspec);\n>>       if (!res && write_locked_index(s->r->index, &index_lock,\n>>                          COMMIT_LOCK) < 0)\n>> diff --git a/blame.c b/blame.c\n>> index 401990726e..206c295660 100644\n>> --- a/blame.c\n>> +++ b/blame.c\n>> @@ -1403,6 +1403,7 @@ static struct blame_origin *find_origin(struct \n>> repository *r,\n>>           }\n>>       }\n>>       diff_flush(&diff_opts);\n>> +    clear_pathspec(&diff_opts.pathspec);\n>>       return porigin;\n>>   }\n>> @@ -1446,6 +1447,7 @@ static struct blame_origin *find_rename(struct \n>> repository *r,\n>>           }\n>>       }\n>>       diff_flush(&diff_opts);\n>> +    clear_pathspec(&diff_opts.pathspec);\n>>       return porigin;\n>>   }\n>> @@ -2326,6 +2328,7 @@ static void find_copy_in_parent(struct \n>> blame_scoreboard *sb,\n>>       } while (unblamed);\n>>       target->suspects = reverse_blame(leftover, NULL);\n>>       diff_flush(&diff_opts);\n>> +    clear_pathspec(&diff_opts.pathspec);\n>>   }\n>>   /*\n>> diff --git a/builtin/reset.c b/builtin/reset.c\n>> index 24968dd628..b97745ee94 100644\n>> --- a/builtin/reset.c\n>> +++ b/builtin/reset.c\n>> @@ -274,6 +274,7 @@ static int read_from_tree(const struct pathspec \n>> *pathspec,\n>>           return 1;\n>>       diffcore_std(&opt);\n>>       diff_flush(&opt);\n>> +    clear_pathspec(&opt.pathspec);\n>>       return 0;\n>>   }\n>> diff --git a/diff.c b/diff.c\n>> index 0aef3db6e1..c862771a58 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -6345,7 +6345,6 @@ void diff_free(struct diff_options *options)\n>>       diff_free_file(options);\n>>       diff_free_ignore_regex(options);\n>> -    clear_pathspec(&options->pathspec);\n>>   }\n>>   void diff_flush(struct diff_options *options)\n>> diff --git a/notes-merge.c b/notes-merge.c\n>> index 7ba40cfb08..b4a3a903e8 100644\n>> --- a/notes-merge.c\n>> +++ b/notes-merge.c\n>> @@ -175,6 +175,7 @@ static struct notes_merge_pair \n>> *diff_tree_remote(struct notes_merge_options *o,\n>>                  oid_to_hex(&mp->remote));\n>>       }\n>>       diff_flush(&opt);\n>> +    clear_pathspec(&opt.pathspec);\n>>       *num_changes = len;\n>>       return changes;\n>> @@ -260,6 +261,7 @@ static void diff_tree_local(struct \n>> notes_merge_options *o,\n>>                  oid_to_hex(&mp->local));\n>>       }\n>>       diff_flush(&opt);\n>> +    clear_pathspec(&opt.pathspec);\n>>   }\n>>   static void check_notes_merge_worktree(struct notes_merge_options *o)\n"},{"id":"454434","messageId":"xmqqsfpzswmq.fsf@gitster.g","threadId":"57784","inReplyTo":"6ac7e164-c4ba-0ffc-a8a4-4cede1d4e7ae@gmail.com","subject":"Re: [PATCH] 2.36 gitk/diff-tree --stdin regression fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-26T15:16:29Z","receivedAt":"2022-04-26T15:20:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> On further inspection we have tests for \"log -p -I\" in t4013\n> and e900d494dc (\"diff: add an API for deferred freeing\", 2021-02-11) \n> modified builtin/log.c to set the new no_free flag so \"log\" should be\n> OK. However \"diff-tree --stdin -p -I\" is not as \n> builtin/diff-tree.c is unchanged by e900d494dc so the no_free flag is\n> not set which I think is the cause of the problems reported here.\n\n... reported here, meaning some reproduction exists? It would be\ngood to have it in the test, next to the ones I added yesterday, I\nthink.\n\nIn any case, I think that is a much older breakage that can be left\nafter this \"oops, where is my pathspec?\" regression is dealt with.\n\n> I think the close_file changes in e900d494dc should be safe as far as\n> \"diff-tree\" is concerned as it never sets that flag.\n>\n> In retrospect the no_free flag is pretty ugly and fragile.\n\nYes.\n\n> If we\n> really cannot do it another way at least requiring callers to set a\n> flag when they want things freeing would avoid nasty surprises like\n> this at the expense of leaking when the caller forgets to set\n> it. Perhaps once 2.36.1 is out we should step back and think about\n> exactly what we're trying to achieve by removing these bounded leaks\n> rather than annotating them with UNLEAK().\n\nDoubly yes.\n\nThere is small per-task resources allocated that is not proportional\nto the size of the task, i.e. \"git log -p\" may need more resource at\n\"peak\" in a project with 100k files than in a project with 1k files,\nand we do not want to leak these resources we use to compare two sets\nof 100k (or 1k) files between \"commit^\" and \"commit\". It may allocate\nand deallocate more times in a project with 100k commits than in a\nproject with 1k commits, and we do not want to leak 100 times more\nresources in the former project than the latter. Aiming to reclaim\nthese resources needed proportinally to the size of the task is\nabsolutely a good thing to do.\n\nBut the final clean-up for the very top-level allocations that is\nnot proportional to the size of the task, like pathspec, regex, and\nother result from parsing command line options and configuration\nvariables? Using UNLEAK() to squelch the leak checker and letting\nprocess termination to reclaim them is absolutely a no-cost solution\nthat is much better.\n"},{"id":"454437","messageId":"xmqqo80nsw5h.fsf@gitster.g","threadId":"57784","inReplyTo":"6ac7e164-c4ba-0ffc-a8a4-4cede1d4e7ae@gmail.com","subject":"Re: [PATCH] 2.36 gitk/diff-tree --stdin regression fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-26T15:26:50Z","receivedAt":"2022-04-26T15:27:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I wonder if we would have caught a regression like the one if we\nused FREE_AND_NULL more sparingly. For example, if we prematurely\ncalled clear_pathspec(), the second iteration, because there is\nfree-and-null of pathspec->items and resetting pathspec->nr to 0,\nwould behave very normally as if there is no pathspec. If we just\nfreed things, without NULLing them out or resetting .nr to 0, the\nsecond iteration would try to access garbage and hopefully we will\ncatch a crash before such a code would have escaped the lab.\n\nIn any case, based on what I heard here, it appears that mimicking\n\"git log\" does may probably be a better way to deal with this\nregression? As you said, all the other things diff_free() calls are\nunwanted while \"diff-tree --stdin\" is still working, just like\n\"log\"?\n\nThanks.\n\n"},{"id":"454448","messageId":"xmqq7d7bsu2n.fsf@gitster.g","threadId":"57784","inReplyTo":"xmqqo80nsw5h.fsf@gitster.g","subject":"[PATCH] 2.36 gitk/diff-tree --stdin regression fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-26T16:11:44Z","receivedAt":"2022-04-26T16:11:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This only surfaced as a regression after 2.36 release, but the\nbreakage was already there with us for at least a year.\n\nThe diff_free() call is to be used after we completely finished with\na diffopt structure. After \"git diff A B\" finishes producing\noutput, calling it before process exit is fine. But there are\ncommands that prepares diff_options struct once, compares two sets\nof paths, releases resources that were used to do the comparison,\nthen reuses the same diff_option struct to go on to compare the next\ntwo sets of paths, like \"git log -p\". \n\nAfter \"git log -p\" finishes showing a single commit, calling it\nbefore it goes on to the next commit is NOT fine. There is a\nmechanism, the .no_free member in diff_options struct, to help \"git\nlog\" to avoid calling diff_free() after showing each commit and\ninstead call it just one. When the mechanism was introduced in\ne900d494 (diff: add an API for deferred freeing, 2021-02-11),\nhowever, we forgot to do the same to \"diff-tree --stdin\", which *is*\na moral equivalent to \"git log\".\n\nDuring 2.36 release cycle, we started clearing the pathspec in\ndiff_free(), so programs like gitk that runs\n\n git diff-tree --stdin -- \n\ndownstream of a pipe, processing one commit after another, started\nshowing irrelevant comparison outside the given from the\nsecond commit. The same commit, by forgetting to teach the .no_free\nmechanism, broke \"diff-tree --stdin -I\" and nobody noticed\nit for over a year, presumably because it is so seldom used an\noption.\n\nBut is a different story. The breakage was very\nprominently visible and was reported immediately after 2.36 was\nreleased.\n\nFix this breakage by mimicking how \"git log\" utilizes the .no_free\nmember so that \"diff-tree --stdin\" behaves more similarly to \"log\".\n\nProtect the fix with a few new tests.\n\nReported-by: Matthias Aßhauer \nHelped-by: René Scharfe \nHelped-by: Phillip Wood \nSigned-off-by: Junio C Hamano \n---\n\n * I feel MUCH better with this than the revert, now Phillip helped\n me to get the root cause straight. Addition of clear_pathspec()\n to diff_tree() was *not* a mistake but is quite reasonable thing\n to do. Not using the .no_free hack in a code path that needed it\n was.\n\n\n\n builtin/diff-tree.c | 3 +++\n log-tree.c | 1 +\n t/t4013-diff-various.sh | 14 ++++++++++++++\n 3 files changed, 18 insertions(+)\n\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 0e0ac1f167..116097a404 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -195,6 +195,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\tint saved_dcctc = 0;\n \n \t\topt->diffopt.rotate_to_strict = 0;\n+\t\topt->diffopt.no_free = 1;\n \t\tif (opt->diffopt.detect_rename) {\n \t\t\tif (!the_index.cache)\n \t\t\t\trepo_read_index(the_repository);\n@@ -217,6 +218,8 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\topt->diffopt.degraded_cc_to_c = saved_dcctc;\n \t\topt->diffopt.needed_rename_limit = saved_nrl;\n+\t\topt->diffopt.no_free = 0;\n+\t\tdiff_free(&opt->diffopt);\n \t}\n \n \treturn diff_result_code(&opt->diffopt, 0);\ndiff --git a/log-tree.c b/log-tree.c\nindex 25165e2a91..f8c18fd8b9 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -1098,6 +1098,7 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)\n \topt->loginfo = &log;\n \topt->diffopt.no_free = 1;\n \n+\t/* NEEDSWORK: no restoring of no_free? Why? */\n \tif (opt->line_level_traverse)\n \t\treturn line_log_print(opt, commit);\n \ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 750aee17ea..628b01f355 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -542,6 +542,20 @@ test_expect_success 'diff-tree --stdin with log formatting' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'diff-tree --stdin with pathspec' '\n+\tcat >expect <<-EOF &&\n+\tThird\n+\n+\tdir/sub\n+\tSecond\n+\n+\tdir/sub\n+\tEOF\n+\tgit rev-list master^ |\n+\tgit diff-tree -r --stdin --name-only --format=%s dir >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'diff -I: setup' '\n \tgit checkout master &&\n \ttest_seq 50 >file0 &&\n-- \n2.36.0-202-g319c44b8f9\n\n"},{"id":"454509","messageId":"561a73aa-7e94-5a09-0c9c-08e8b6ce7188@web.de","threadId":"57784","inReplyTo":"xmqq7d7bsu2n.fsf@gitster.g","subject":"Re: [PATCH] 2.36 gitk/diff-tree --stdin regression fix","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-04-27T16:42:04Z","receivedAt":"2022-04-27T16:42:32Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 26.04.22 um 18:11 schrieb Junio C Hamano:\n> This only surfaced as a regression after 2.36 release, but the\n> breakage was already there with us for at least a year.\n>\n> The diff_free() call is to be used after we completely finished with\n> a diffopt structure. After \"git diff A B\" finishes producing\n> output, calling it before process exit is fine. But there are\n> commands that prepares diff_options struct once, compares two sets\n> of paths, releases resources that were used to do the comparison,\n> then reuses the same diff_option struct to go on to compare the next\n> two sets of paths, like \"git log -p\".\n>\n> After \"git log -p\" finishes showing a single commit, calling it\n> before it goes on to the next commit is NOT fine. There is a\n> mechanism, the .no_free member in diff_options struct, to help \"git\n> log\" to avoid calling diff_free() after showing each commit and\n> instead call it just one. When the mechanism was introduced in\n> e900d494 (diff: add an API for deferred freeing, 2021-02-11),\n> however, we forgot to do the same to \"diff-tree --stdin\", which *is*\n> a moral equivalent to \"git log\".\n>\n> During 2.36 release cycle, we started clearing the pathspec in\n> diff_free(), so programs like gitk that runs\n>\n> git diff-tree --stdin -- \n>\n> downstream of a pipe, processing one commit after another, started\n> showing irrelevant comparison outside the given from the\n> second commit. The same commit, by forgetting to teach the .no_free\n> mechanism, broke \"diff-tree --stdin -I\" and nobody noticed\n> it for over a year, presumably because it is so seldom used an\n> option.\n>\n> But is a different story. The breakage was very\n> prominently visible and was reported immediately after 2.36 was\n> released.\n>\n> Fix this breakage by mimicking how \"git log\" utilizes the .no_free\n> member so that \"diff-tree --stdin\" behaves more similarly to \"log\".\n>\n> Protect the fix with a few new tests.\n\nWe could check where reused diffopts caused a pathspec loss at runtime,\nlike in the patch below. Then we \"just\" need to get the relevant test\ncoverage to 100% and we'll find them all.\n\nWith your patch on top of main, \"make test\" passes for me. With the\npatch below added as well I get failures in three test scripts:\n\nt3427-rebase-subtree.sh (Wstat: 256 Tests: 3 Failed: 2)\n Failed tests: 2-3\n Non-zero exit status: 1\nt4014-format-patch.sh (Wstat: 256 Tests: 190 Failed: 1)\n Failed test: 73\n Non-zero exit status: 1\nt9350-fast-export.sh (Wstat: 256 Tests: 50 Failed: 3)\n Failed tests: 30, 32, 43\n Non-zero exit status: 1\n\nThe format-patch is a bit surprising to me because it already sets\nno_free conditionally. t4014 is successful if no_free is set in all\ncases, so the condition seems to be too narrow -- but I don't understand\nit. Didn't look at the other cases.\n\n---\n diff.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex ef7159968b..b7c837aca8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6455,9 +6455,16 @@ static void diff_free_ignore_regex(struct diff_options *options)\n\n void diff_free(struct diff_options *options)\n {\n+\tstatic struct diff_options *prev_options_with_pathspec;\n+\n \tif (options->no_free)\n \t\treturn;\n\n+\tif (prev_options_with_pathspec == options && !options->pathspec.nr)\n+\t\tBUG(\"reused struct diff_options, potentially lost pathspec\");\n+\tif (options->pathspec.nr)\n+\t\tprev_options_with_pathspec = options;\n+\n \tdiff_free_file(options);\n \tdiff_free_ignore_regex(options);\n \tclear_pathspec(&options->pathspec);\n--\n2.35.3\n"},{"id":"454523","messageId":"a1dcac11-98ac-9fcd-8375-4fd1be9ace78@web.de","threadId":"57784","inReplyTo":"561a73aa-7e94-5a09-0c9c-08e8b6ce7188@web.de","subject":"Re: [PATCH] 2.36 gitk/diff-tree --stdin regression fix","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-04-27T18:06:39Z","receivedAt":"2022-04-27T18:07:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.04.22 um 18:42 schrieb René Scharfe:\n> Am 26.04.22 um 18:11 schrieb Junio C Hamano:\n>> This only surfaced as a regression after 2.36 release, but the\n>> breakage was already there with us for at least a year.\n>>\n>> The diff_free() call is to be used after we completely finished with\n>> a diffopt structure. After \"git diff A B\" finishes producing\n>> output, calling it before process exit is fine. But there are\n>> commands that prepares diff_options struct once, compares two sets\n>> of paths, releases resources that were used to do the comparison,\n>> then reuses the same diff_option struct to go on to compare the next\n>> two sets of paths, like \"git log -p\".\n>>\n>> After \"git log -p\" finishes showing a single commit, calling it\n>> before it goes on to the next commit is NOT fine. There is a\n>> mechanism, the .no_free member in diff_options struct, to help \"git\n>> log\" to avoid calling diff_free() after showing each commit and\n>> instead call it just one. When the mechanism was introduced in\n>> e900d494 (diff: add an API for deferred freeing, 2021-02-11),\n>> however, we forgot to do the same to \"diff-tree --stdin\", which *is*\n>> a moral equivalent to \"git log\".\n>>\n>> During 2.36 release cycle, we started clearing the pathspec in\n>> diff_free(), so programs like gitk that runs\n>>\n>> git diff-tree --stdin -- \n>>\n>> downstream of a pipe, processing one commit after another, started\n>> showing irrelevant comparison outside the given from the\n>> second commit. The same commit, by forgetting to teach the .no_free\n>> mechanism, broke \"diff-tree --stdin -I\" and nobody noticed\n>> it for over a year, presumably because it is so seldom used an\n>> option.\n>>\n>> But is a different story. The breakage was very\n>> prominently visible and was reported immediately after 2.36 was\n>> released.\n>>\n>> Fix this breakage by mimicking how \"git log\" utilizes the .no_free\n>> member so that \"diff-tree --stdin\" behaves more similarly to \"log\".\n>>\n>> Protect the fix with a few new tests.\n>\n> We could check where reused diffopts caused a pathspec loss at runtime,\n> like in the patch below. Then we \"just\" need to get the relevant test\n> coverage to 100% and we'll find them all.\n>\n> With your patch on top of main, \"make test\" passes for me. With the\n> patch below added as well I get failures in three test scripts:\n>\n> t3427-rebase-subtree.sh (Wstat: 256 Tests: 3 Failed: 2)\n> Failed tests: 2-3\n> Non-zero exit status: 1\n> t4014-format-patch.sh (Wstat: 256 Tests: 190 Failed: 1)\n> Failed test: 73\n> Non-zero exit status: 1\n> t9350-fast-export.sh (Wstat: 256 Tests: 50 Failed: 3)\n> Failed tests: 30, 32, 43\n> Non-zero exit status: 1\n>\n> The format-patch is a bit surprising to me because it already sets\n> no_free conditionally. t4014 is successful if no_free is set in all\n> cases, so the condition seems to be too narrow -- but I don't understand\n> it. Didn't look at the other cases.\n>\n> ---\n> diff.c | 7 +++++++\n> 1 file changed, 7 insertions(+)\n>\n> diff --git a/diff.c b/diff.c\n> index ef7159968b..b7c837aca8 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6455,9 +6455,16 @@ static void diff_free_ignore_regex(struct diff_options *options)\n>\n> void diff_free(struct diff_options *options)\n> {\n> +\tstatic struct diff_options *prev_options_with_pathspec;\n> +\n> \tif (options->no_free)\n> \t\treturn;\n>\n> +\tif (prev_options_with_pathspec == options && !options->pathspec.nr)\n> +\t\tBUG(\"reused struct diff_options, potentially lost pathspec\");\n> +\tif (options->pathspec.nr)\n> +\t\tprev_options_with_pathspec = options;\n\nThis can report a false positive if a diffopt is reused with different\npathspecs, and one of them is empty (match all). Which could be countered\nby using a fresh diffopt every time (e.g. pushing it into a loop).\n\n> +\n> \tdiff_free_file(options);\n> \tdiff_free_ignore_regex(options);\n> \tclear_pathspec(&options->pathspec);\n"},{"id":"454540","messageId":"xmqqv8uul2ea.fsf@gitster.g","threadId":"57784","inReplyTo":"a1dcac11-98ac-9fcd-8375-4fd1be9ace78@web.de","subject":"Re: [PATCH] 2.36 gitk/diff-tree --stdin regression fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-27T20:03:41Z","receivedAt":"2022-04-27T20:04:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> +\tif (prev_options_with_pathspec == options && !options->pathspec.nr)\n>> +\t\tBUG(\"reused struct diff_options, potentially lost pathspec\");\n>> +\tif (options->pathspec.nr)\n>> +\t\tprev_options_with_pathspec = options;\n>\n> This can report a false positive if a diffopt is reused with different\n> pathspecs, and one of them is empty (match all). Which could be countered\n> by using a fresh diffopt every time (e.g. pushing it into a loop).\n\nThe only use case to reset pathspec of a diffopt during iteration I\ncan think of is the hacky[*] version of \"git log --follow\" where the\npathspec is swapped when a rename of a single path being followed is\ndetected.\n\n Side note: hacky because the way it swaps a single pathspec upon\n seeing one rename means it does not work in a mergy-branchy\n history where one branch renames and there are still commits\n that need to be explored on the other branch that had the path\n under its original name.\n"}]}