{"thread":{"id":"62469","subject":"[PATCH] log: --remerge-diff needs to keep around commit parents","startedAt":"2024-11-08T13:43:58Z","lastAt":"2024-12-13T15:03:02Z","messageCount":9,"participants":["Johannes Schindelin via GitGitGadget","Elijah Newren","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"506861","messageId":"pull.1825.git.1731073435641.gitgitgadget@gmail.com","threadId":"62469","inReplyTo":null,"subject":"[PATCH] log: --remerge-diff needs to keep around commit parents","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-08T13:43:55Z","receivedAt":"2024-11-08T13:43:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nTo show a remerge diff, the merge needs to be recreated. For that to\nwork, the merge base(s) need to be found, which means that the commits'\nparents have to be traversed until common ancestors are found (if any).\n\nHowever, one optimization that hails all the way back to\ncb115748ec0d (Some more memory leak avoidance, 2006-06-17) is to release\nthe commit's list of parents immediately after showing it. This can break\nthe merge base computation.\n\nNote that it matters more clearly when traversing the commits in\nreverse: In that instance, if a parent of a merge commit has been shown\nas part of the `git log` command, by the time the merge commit's diff\nneeds to be computed, that parent commit's list of parent commits will\nhave been set to `NULL` and as a result no merge base will be found.\n\nLet's fix this by special-casing the `remerge_diff` mode, similar to\nwhat we did with reflogs in f35650dff6a4 (log: do not free parents when\nwalking reflog, 2017-07-07).\n\nSigned-off-by: Johannes Schindelin \n---\n log: --remerge-diff needs to keep around commit parents\n \n This fixes a bug that is pretty much as old as the remerge-diff\n machinery itself. I noticed it while writing my reply to Hannes Sixt's\n concerns about my range-diff --diff-merges= patch\n (https://lore.kernel.org/git/af576487-5de2-fba3-b341-3c082322c9ec@gmx.de/).\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1825%2Fdscho%2Flog-remerge-diff-needs-commit-parents-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1825/dscho/log-remerge-diff-needs-commit-parents-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1825\n\n builtin/log.c | 2 +-\n t/t4069-remerge-diff.sh | 7 +++++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex c0a8bb95e98..a297c6caf59 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -522,7 +522,7 @@ static int cmd_log_walk_no_free(struct rev_info *rev)\n \t\t\t * but we didn't actually show the commit.\n \t\t\t */\n \t\t\trev->max_count++;\n-\t\tif (!rev->reflog_info) {\n+\t\tif (!rev->reflog_info && !rev->remerge_diff) {\n \t\t\t/*\n \t\t\t * We may show a given commit multiple times when\n \t\t\t * walking the reflogs.\ndiff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\nindex 07323ebafe0..a68c6bfa036 100755\n--- a/t/t4069-remerge-diff.sh\n+++ b/t/t4069-remerge-diff.sh\n@@ -317,4 +317,11 @@ test_expect_success 'remerge-diff turns off history simplification' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'remerge-diff with --reverse' '\n+\tgit log -1 --remerge-diff --oneline ab_resolution^ >expect &&\n+\tgit log -1 --remerge-diff --oneline ab_resolution >>expect &&\n+\tgit log -2 --remerge-diff --oneline ab_resolution --reverse >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\nbase-commit: bea9ecd24b0c3bf06cab4a851694fe09e7e51408\n-- \ngitgitgadget\n"},{"id":"506866","messageId":"CABPp-BHauuserYU8t5ybn+ghqRb-c85qwwjLWVe-e5EC8sVVxQ@mail.gmail.com","threadId":"62469","inReplyTo":"pull.1825.git.1731073435641.gitgitgadget@gmail.com","subject":"Re: [PATCH] log: --remerge-diff needs to keep around commit parents","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-11-08T15:47:04Z","receivedAt":"2024-11-08T15:47:16Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Nov 8, 2024 at 5:43 AM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> From: Johannes Schindelin \n>\n> To show a remerge diff, the merge needs to be recreated. For that to\n> work, the merge base(s) need to be found, which means that the commits'\n> parents have to be traversed until common ancestors are found (if any).\n>\n> However, one optimization that hails all the way back to\n> cb115748ec0d (Some more memory leak avoidance, 2006-06-17) is to release\n> the commit's list of parents immediately after showing it. This can break\n> the merge base computation.\n>\n> Note that it matters more clearly when traversing the commits in\n> reverse: In that instance, if a parent of a merge commit has been shown\n> as part of the `git log` command, by the time the merge commit's diff\n> needs to be computed, that parent commit's list of parent commits will\n> have been set to `NULL` and as a result no merge base will be found.\n>\n> Let's fix this by special-casing the `remerge_diff` mode, similar to\n> what we did with reflogs in f35650dff6a4 (log: do not free parents when\n> walking reflog, 2017-07-07).\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> log: --remerge-diff needs to keep around commit parents\n>\n> This fixes a bug that is pretty much as old as the remerge-diff\n> machinery itself. I noticed it while writing my reply to Hannes Sixt's\n> concerns about my range-diff --diff-merges= patch\n> (https://lore.kernel.org/git/af576487-5de2-fba3-b341-3c082322c9ec@gmx.de/).\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1825%2Fdscho%2Flog-remerge-diff-needs-commit-parents-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1825/dscho/log-remerge-diff-needs-commit-parents-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1825\n>\n> builtin/log.c | 2 +-\n> t/t4069-remerge-diff.sh | 7 +++++++\n> 2 files changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index c0a8bb95e98..a297c6caf59 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -522,7 +522,7 @@ static int cmd_log_walk_no_free(struct rev_info *rev)\n> * but we didn't actually show the commit.\n> */\n> rev->max_count++;\n> - if (!rev->reflog_info) {\n> + if (!rev->reflog_info && !rev->remerge_diff) {\n> /*\n> * We may show a given commit multiple times when\n> * walking the reflogs.\n\nShould this comment be updated to reflect the expanded rationale for\nthis block's purpose?\n\n> diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\n> index 07323ebafe0..a68c6bfa036 100755\n> --- a/t/t4069-remerge-diff.sh\n> +++ b/t/t4069-remerge-diff.sh\n> @@ -317,4 +317,11 @@ test_expect_success 'remerge-diff turns off history simplification' '\n> test_cmp expect actual\n> '\n>\n> +test_expect_success 'remerge-diff with --reverse' '\n> + git log -1 --remerge-diff --oneline ab_resolution^ >expect &&\n> + git log -1 --remerge-diff --oneline ab_resolution >>expect &&\n> + git log -2 --remerge-diff --oneline ab_resolution --reverse >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n>\n> base-commit: bea9ecd24b0c3bf06cab4a851694fe09e7e51408\n> --\n> gitgitgadget\n\nWow, nice find, and I appreciate the nice simple testcase\ndemonstrating what had been the problem.\n"},{"id":"506956","messageId":"xmqq7c9ak0e9.fsf@gitster.g","threadId":"62469","inReplyTo":"pull.1825.git.1731073435641.gitgitgadget@gmail.com","subject":"Re: [PATCH] log: --remerge-diff needs to keep around commit parents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-11T01:46:38Z","receivedAt":"2024-11-11T01:46:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> To show a remerge diff, the merge needs to be recreated. For that to\n> work, the merge base(s) need to be found, which means that the commits'\n> parents have to be traversed until common ancestors are found (if any).\n>\n> However, one optimization that hails all the way back to\n> cb115748ec0d (Some more memory leak avoidance, 2006-06-17) is to release\n> the commit's list of parents immediately after showing it. This can break\n> the merge base computation.\n\n> Note that it matters more clearly when traversing the commits in\n> reverse: In that instance, if a parent of a merge commit has been shown\n> as part of the `git log` command, by the time the merge commit's diff\n> needs to be computed, that parent commit's list of parent commits will\n> have been set to `NULL` and as a result no merge base will be found.\n\nOuch.\n\nI am curious about \"more clearly\" in the above, though. Do we also\nlose parents before a base can be computed during a forward\ntraversal? Unlike the reflog walk, we won't show the same commit\ntwice, so unless we are traversing some of the history of our\nparents *and* showing these ancestors found there _before_ we need\nto show the merge in question, I do not think the issue would\nsurface during a forward traversal. So the problematic case is when\nwe are not doing topological display, and traversing down to a\ncommit with skewed timestamp causes it to be shown (and lose its\nparents due to memory optimization) because it appears much newer\nthan our merge, and there is an ancestry chain leading to it that\npasses commits other than our merge, or something?\n\n> Let's fix this by special-casing the `remerge_diff` mode, similar to\n> what we did with reflogs in f35650dff6a4 (log: do not free parents when\n> walking reflog, 2017-07-07).\n\nOK.\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index c0a8bb95e98..a297c6caf59 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -522,7 +522,7 @@ static int cmd_log_walk_no_free(struct rev_info *rev)\n> \t\t\t * but we didn't actually show the commit.\n> \t\t\t */\n> \t\t\trev->max_count++;\n> -\t\tif (!rev->reflog_info) {\n> +\t\tif (!rev->reflog_info && !rev->remerge_diff) {\n> \t\t\t/*\n> \t\t\t * We may show a given commit multiple times when\n> \t\t\t * walking the reflogs.\n\nOK, that's trivial ;-)\n\n> diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\n> index 07323ebafe0..a68c6bfa036 100755\n> --- a/t/t4069-remerge-diff.sh\n> +++ b/t/t4069-remerge-diff.sh\n> @@ -317,4 +317,11 @@ test_expect_success 'remerge-diff turns off history simplification' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'remerge-diff with --reverse' '\n> +\tgit log -1 --remerge-diff --oneline ab_resolution^ >expect &&\n> +\tgit log -1 --remerge-diff --oneline ab_resolution >>expect &&\n> +\tgit log -2 --remerge-diff --oneline ab_resolution --reverse >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nThis is the trivially reproducible \"show in reverse\" case. Nice\nfinding.\n\nWill queue.\n"},{"id":"506957","messageId":"xmqq1pzik046.fsf@gitster.g","threadId":"62469","inReplyTo":"CABPp-BHauuserYU8t5ybn+ghqRb-c85qwwjLWVe-e5EC8sVVxQ@mail.gmail.com","subject":"Re: [PATCH] log: --remerge-diff needs to keep around commit parents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-11T01:52:41Z","receivedAt":"2024-11-11T01:52:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> - if (!rev->reflog_info) {\n>> + if (!rev->reflog_info && !rev->remerge_diff) {\n>> /*\n>> * We may show a given commit multiple times when\n>> * walking the reflogs.\n>\n> Should this comment be updated to reflect the expanded rationale for\n> this block's purpose?\n\nAh, we probably should. Especially if the newly discovered failure\nmode is not due to having to show the same commit multiple times.\n\n During reflog walk, we may show the same commit more than once,\n so do not discard the parents. There may be a merge commit that\n is descendent from the current commit, and in order to show it with\n remerge-diff enabled, we need its ancestry chain, including our\n parents, to compute the merge base.\n\nor something? It is still not clear to me in what scenario a merge\ncommit, in a forward traversal, is shown _after_ one of its ancestor\ncommit is shown (and due to the memory optimization, loses its\nparents).\n\nThanks, both.\n\n"},{"id":"507068","messageId":"b7488a36-0ba6-0847-47cb-6a5f3edec689@gmx.de","threadId":"62469","inReplyTo":"xmqq7c9ak0e9.fsf@gitster.g","subject":"Re: [PATCH] log: --remerge-diff needs to keep around commit parents","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-11-11T18:32:14Z","receivedAt":"2024-11-11T18:32:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 11 Nov 2024, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > To show a remerge diff, the merge needs to be recreated. For that to\n> > work, the merge base(s) need to be found, which means that the commits'\n> > parents have to be traversed until common ancestors are found (if any).\n> >\n> > However, one optimization that hails all the way back to\n> > cb115748ec0d (Some more memory leak avoidance, 2006-06-17) is to release\n> > the commit's list of parents immediately after showing it. This can break\n> > the merge base computation.\n>\n> > Note that it matters more clearly when traversing the commits in\n> > reverse: In that instance, if a parent of a merge commit has been shown\n> > as part of the `git log` command, by the time the merge commit's diff\n> > needs to be computed, that parent commit's list of parent commits will\n> > have been set to `NULL` and as a result no merge base will be found.\n>\n> Ouch.\n>\n> I am curious about \"more clearly\" in the above, though.\n\nI consider these examples less clear, but they are still affected:\n\n\tgit show --remerge-diff v2.45.2^0\n\tvs\n\tgit show --remerge-diff v2.44.2^0 v2.45.2^0\n\n\tgit show --remerge-diff v2.45.1~1\n\tvs\n\tgit log --topo-order --first-parent --remerge-diff v2.44.2 v2.45.1\n\nConcretely, these diffs should be empty, but are not:\n\n\tgit diff --no-index \\\n\t\t<(git show --remerge-diff v2.45.2^0 | sed 1d) \\\n\t\t<(git show --remerge-diff v2.44.2^0 v2.45.2^0 | sed 1,/^commit/d)\n\n\tand\n\n\tgit diff --no-index \\\n\t\t<(git show --remerge-diff v2.45.1~1 | grep -v ^commit) \\\n\t\t<(git log --topo-order --first-parent -6 --remerge-diff v2.44.2 v2.45.1 |\n\t\t sed '1,/^commit 1c00f92eb5ee4a48ab615eefa41f2dd6024d43bc/d;/^commit/,$d')\n\nNo `--reverse` required, not even clock skew.\n\nCiao,\nJohannes\n"},{"id":"507069","messageId":"pull.1825.v2.git.1731350009491.gitgitgadget@gmail.com","threadId":"62469","inReplyTo":"pull.1825.git.1731073435641.gitgitgadget@gmail.com","subject":"[PATCH v2] log: --remerge-diff needs to keep around commit parents","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-11T18:33:29Z","receivedAt":"2024-11-11T18:33:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nTo show a remerge diff, the merge needs to be recreated. For that to\nwork, the merge base(s) need to be found, which means that the commits'\nparents have to be traversed until common ancestors are found (if any).\n\nHowever, one optimization that hails all the way back to\ncb115748ec0d (Some more memory leak avoidance, 2006-06-17) is to release\nthe commit's list of parents immediately after showing it. This can break\nthe merge base computation.\n\nNote that it matters more clearly when traversing the commits in\nreverse: In that instance, if a parent of a merge commit has been shown\nas part of the `git log` command, by the time the merge commit's diff\nneeds to be computed, that parent commit's list of parent commits will\nhave been set to `NULL` and as a result no merge base will be found.\n\nLet's fix this by special-casing the `remerge_diff` mode, similar to\nwhat we did with reflogs in f35650dff6a4 (log: do not free parents when\nwalking reflog, 2017-07-07).\n\nSigned-off-by: Johannes Schindelin \n---\n log: --remerge-diff needs to keep around commit parents\n \n This fixes a bug that is pretty much as old as the remerge-diff\n machinery itself. I noticed it while writing my reply to Hannes Sixt's\n concerns about my range-diff --diff-merges= patch\n (https://lore.kernel.org/git/af576487-5de2-fba3-b341-3c082322c9ec@gmx.de/).\n \n Changes since v1:\n \n * Amended the code comment of the affected conditional code block in\n harmony with the newly-added condition.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1825%2Fdscho%2Flog-remerge-diff-needs-commit-parents-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1825/dscho/log-remerge-diff-needs-commit-parents-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1825\n\nRange-diff vs v1:\n\n 1: 68238315c14 ! 1: afff27f8222 log: --remerge-diff needs to keep around commit parents\n @@ builtin/log.c: static int cmd_log_walk_no_free(struct rev_info *rev)\n +\t\tif (!rev->reflog_info && !rev->remerge_diff) {\n \t\t\t/*\n \t\t\t * We may show a given commit multiple times when\n - \t\t\t * walking the reflogs.\n +-\t\t\t * walking the reflogs.\n ++\t\t\t * walking the reflogs. Therefore we still need it.\n ++\t\t\t *\n ++\t\t\t * Likewise, we potentially still need the parents\n ++\t\t\t * of * already shown commits to determine merge\n ++\t\t\t * bases when showing remerge diffs.\n + \t\t\t */\n + \t\t\tfree_commit_buffer(the_repository->parsed_objects,\n + \t\t\t\t\t commit);\n \n ## t/t4069-remerge-diff.sh ##\n @@ t/t4069-remerge-diff.sh: test_expect_success 'remerge-diff turns off history simplification' '\n\n\n builtin/log.c | 8 ++++++--\n t/t4069-remerge-diff.sh | 7 +++++++\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex c0a8bb95e98..2f88a3e607d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -522,10 +522,14 @@ static int cmd_log_walk_no_free(struct rev_info *rev)\n \t\t\t * but we didn't actually show the commit.\n \t\t\t */\n \t\t\trev->max_count++;\n-\t\tif (!rev->reflog_info) {\n+\t\tif (!rev->reflog_info && !rev->remerge_diff) {\n \t\t\t/*\n \t\t\t * We may show a given commit multiple times when\n-\t\t\t * walking the reflogs.\n+\t\t\t * walking the reflogs. Therefore we still need it.\n+\t\t\t *\n+\t\t\t * Likewise, we potentially still need the parents\n+\t\t\t * of * already shown commits to determine merge\n+\t\t\t * bases when showing remerge diffs.\n \t\t\t */\n \t\t\tfree_commit_buffer(the_repository->parsed_objects,\n \t\t\t\t\t commit);\ndiff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\nindex 07323ebafe0..a68c6bfa036 100755\n--- a/t/t4069-remerge-diff.sh\n+++ b/t/t4069-remerge-diff.sh\n@@ -317,4 +317,11 @@ test_expect_success 'remerge-diff turns off history simplification' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'remerge-diff with --reverse' '\n+\tgit log -1 --remerge-diff --oneline ab_resolution^ >expect &&\n+\tgit log -1 --remerge-diff --oneline ab_resolution >>expect &&\n+\tgit log -2 --remerge-diff --oneline ab_resolution --reverse >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\nbase-commit: bea9ecd24b0c3bf06cab4a851694fe09e7e51408\n-- \ngitgitgadget\n"},{"id":"507081","messageId":"xmqqa5e5berx.fsf@gitster.g","threadId":"62469","inReplyTo":"b7488a36-0ba6-0847-47cb-6a5f3edec689@gmx.de","subject":"Re: [PATCH] log: --remerge-diff needs to keep around commit parents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-11T22:13:06Z","receivedAt":"2024-11-11T22:13:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I consider these examples less clear, but they are still affected:\n>\n> \tgit show --remerge-diff v2.45.2^0\n> \tvs\n> \tgit show --remerge-diff v2.44.2^0 v2.45.2^0\n>\n> \tgit show --remerge-diff v2.45.1~1\n> \tvs\n> \tgit log --topo-order --first-parent --remerge-diff v2.44.2 v2.45.1\n>\n> Concretely, these diffs should be empty, but are not:\n>\n> \tgit diff --no-index \\\n> \t\t<(git show --remerge-diff v2.45.2^0 | sed 1d) \\\n> \t\t<(git show --remerge-diff v2.44.2^0 v2.45.2^0 | sed 1,/^commit/d)\n>\n> \tand\n>\n> \tgit diff --no-index \\\n> \t\t<(git show --remerge-diff v2.45.1~1 | grep -v ^commit) \\\n> \t\t<(git log --topo-order --first-parent -6 --remerge-diff v2.44.2 v2.45.1 |\n> \t\t sed '1,/^commit 1c00f92eb5ee4a48ab615eefa41f2dd6024d43bc/d;/^commit/,$d')\n>\n> No `--reverse` required, not even clock skew.\n\nAs we often tell contributors, questions in reviewer comments should\nbe also answered in an updated patch, so that future readers of \"git\nlog\", who cannot ask direct questions to the author of the patch, do\nnot have to ask the same questions. Can we add an explanation how\nthis affects forward traversal in a three-line paragraph?\n\nThanks.\n\n"},{"id":"509022","messageId":"pull.1825.v3.git.1733999352289.gitgitgadget@gmail.com","threadId":"62469","inReplyTo":"pull.1825.v2.git.1731350009491.gitgitgadget@gmail.com","subject":"[PATCH v3] log: --remerge-diff needs to keep around commit parents","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-12-12T10:29:12Z","receivedAt":"2024-12-12T10:29:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nTo show a remerge diff, the merge needs to be recreated. For that to\nwork, the merge base(s) need to be found, which means that the commits'\nparents have to be traversed until common ancestors are found (if any).\n\nHowever, one optimization that hails all the way back to cb115748ec0d\n(Some more memory leak avoidance, 2006-06-17) is to release the commit's\nlist of parents immediately after showing it _and to set that parent\nlist to `NULL`_. This can break the merge base computation.\n\nThis problem is most obvious when traversing the commits in reverse: In\nthat instance, if a parent of a merge commit has been shown as part of\nthe `git log` command, by the time the merge commit's diff needs to be\ncomputed, that parent commit's list of parent commits will have been set\nto `NULL` and as a result no merge base will be found (even if one\nshould be found).\n\nTraversing commits in reverse is far from the only circumstance in which\nthis problem occurs, though. There are many avenues to traversing at\nleast one commit in the revision walk that will later be part of a merge\nbase computation, for example when not even walking any revisions in\n`git show ` where `` is part of the commit\ngraph between the parents of ``.\n\nAnother way to force a scenario where a commit is traversed before it\nhas to be traversed again as part of a merge base computation is to\nstart with two revisions (where the first one is reachable from the\nsecond but not in a first-parent ancestry) and show the commit log with\n`--topo-order` and `--first-parent`.\n\nLet's fix this by special-casing the `remerge_diff` mode, similar to\nwhat we did with reflogs in f35650dff6a4 (log: do not free parents when\nwalking reflog, 2017-07-07).\n\nSigned-off-by: Johannes Schindelin \n---\n log: --remerge-diff needs to keep around commit parents\n \n This fixes a bug that is pretty much as old as the remerge-diff\n machinery itself. I noticed it while writing my reply to Hannes Sixt's\n concerns about my range-diff --diff-merges= patch\n (https://lore.kernel.org/git/af576487-5de2-fba3-b341-3c082322c9ec@gmx.de/).\n \n Changes since v2:\n \n * Amended the commit message to illustrate why reverse walks are not\n the only instances where this fix is relevant.\n \n Changes since v1:\n \n * Amended the code comment of the affected conditional code block in\n harmony with the newly-added condition.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1825%2Fdscho%2Flog-remerge-diff-needs-commit-parents-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1825/dscho/log-remerge-diff-needs-commit-parents-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1825\n\nRange-diff vs v2:\n\n 1: afff27f8222 ! 1: ab75a565b4c log: --remerge-diff needs to keep around commit parents\n @@ Commit message\n work, the merge base(s) need to be found, which means that the commits'\n parents have to be traversed until common ancestors are found (if any).\n \n - However, one optimization that hails all the way back to\n - cb115748ec0d (Some more memory leak avoidance, 2006-06-17) is to release\n - the commit's list of parents immediately after showing it. This can break\n - the merge base computation.\n -\n - Note that it matters more clearly when traversing the commits in\n - reverse: In that instance, if a parent of a merge commit has been shown\n - as part of the `git log` command, by the time the merge commit's diff\n - needs to be computed, that parent commit's list of parent commits will\n - have been set to `NULL` and as a result no merge base will be found.\n + However, one optimization that hails all the way back to cb115748ec0d\n + (Some more memory leak avoidance, 2006-06-17) is to release the commit's\n + list of parents immediately after showing it _and to set that parent\n + list to `NULL`_. This can break the merge base computation.\n +\n + This problem is most obvious when traversing the commits in reverse: In\n + that instance, if a parent of a merge commit has been shown as part of\n + the `git log` command, by the time the merge commit's diff needs to be\n + computed, that parent commit's list of parent commits will have been set\n + to `NULL` and as a result no merge base will be found (even if one\n + should be found).\n +\n + Traversing commits in reverse is far from the only circumstance in which\n + this problem occurs, though. There are many avenues to traversing at\n + least one commit in the revision walk that will later be part of a merge\n + base computation, for example when not even walking any revisions in\n + `git show ` where `` is part of the commit\n + graph between the parents of ``.\n +\n + Another way to force a scenario where a commit is traversed before it\n + has to be traversed again as part of a merge base computation is to\n + start with two revisions (where the first one is reachable from the\n + second but not in a first-parent ancestry) and show the commit log with\n + `--topo-order` and `--first-parent`.\n \n Let's fix this by special-casing the `remerge_diff` mode, similar to\n what we did with reflogs in f35650dff6a4 (log: do not free parents when\n\n\n builtin/log.c | 8 ++++++--\n t/t4069-remerge-diff.sh | 7 +++++++\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex c0a8bb95e98..2f88a3e607d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -522,10 +522,14 @@ static int cmd_log_walk_no_free(struct rev_info *rev)\n \t\t\t * but we didn't actually show the commit.\n \t\t\t */\n \t\t\trev->max_count++;\n-\t\tif (!rev->reflog_info) {\n+\t\tif (!rev->reflog_info && !rev->remerge_diff) {\n \t\t\t/*\n \t\t\t * We may show a given commit multiple times when\n-\t\t\t * walking the reflogs.\n+\t\t\t * walking the reflogs. Therefore we still need it.\n+\t\t\t *\n+\t\t\t * Likewise, we potentially still need the parents\n+\t\t\t * of * already shown commits to determine merge\n+\t\t\t * bases when showing remerge diffs.\n \t\t\t */\n \t\t\tfree_commit_buffer(the_repository->parsed_objects,\n \t\t\t\t\t commit);\ndiff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\nindex 07323ebafe0..a68c6bfa036 100755\n--- a/t/t4069-remerge-diff.sh\n+++ b/t/t4069-remerge-diff.sh\n@@ -317,4 +317,11 @@ test_expect_success 'remerge-diff turns off history simplification' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'remerge-diff with --reverse' '\n+\tgit log -1 --remerge-diff --oneline ab_resolution^ >expect &&\n+\tgit log -1 --remerge-diff --oneline ab_resolution >>expect &&\n+\tgit log -2 --remerge-diff --oneline ab_resolution --reverse >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\nbase-commit: bea9ecd24b0c3bf06cab4a851694fe09e7e51408\n-- \ngitgitgadget\n"},{"id":"509107","messageId":"xmqq5xnnbp7g.fsf@gitster.g","threadId":"62469","inReplyTo":"pull.1825.v3.git.1733999352289.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] log: --remerge-diff needs to keep around commit parents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-13T15:02:59Z","receivedAt":"2024-12-13T15:03:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> This problem is most obvious when traversing the commits in reverse: In\n> that instance, if a parent of a merge commit has been shown as part of\n> the `git log` command, by the time the merge commit's diff needs to be\n> computed, that parent commit's list of parent commits will have been set\n> to `NULL` and as a result no merge base will be found (even if one\n> should be found).\n>\n> Traversing commits in reverse is far from the only circumstance in which\n> this problem occurs, though. There are many avenues to traversing at\n> least one commit in the revision walk that will later be part of a merge\n> base computation, for example when not even walking any revisions in\n> `git show ` where `` is part of the commit\n> graph between the parents of ``.\n>\n> Another way to force a scenario where a commit is traversed before it\n> has to be traversed again as part of a merge base computation is to\n> start with two revisions (where the first one is reachable from the\n> second but not in a first-parent ancestry) and show the commit log with\n> `--topo-order` and `--first-parent`.\n>\n> Let's fix this by special-casing the `remerge_diff` mode, similar to\n> what we did with reflogs in f35650dff6a4 (log: do not free parents when\n> walking reflog, 2017-07-07).\n\nNicely described.\n\n> This fixes a bug that is pretty much as old as the remerge-diff\n> machinery itself. I noticed it while writing my reply to Hannes Sixt's\n> concerns about my range-diff --diff-merges= patch\n\n> diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\n> index 07323ebafe0..a68c6bfa036 100755\n> --- a/t/t4069-remerge-diff.sh\n> +++ b/t/t4069-remerge-diff.sh\n> @@ -317,4 +317,11 @@ test_expect_success 'remerge-diff turns off history simplification' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'remerge-diff with --reverse' '\n> +\tgit log -1 --remerge-diff --oneline ab_resolution^ >expect &&\n> +\tgit log -1 --remerge-diff --oneline ab_resolution >>expect &&\n> +\tgit log -2 --remerge-diff --oneline ab_resolution --reverse >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n\nWill queue. Let me mark it for 'next'.\n\nThanks.\n\n"}]}