| Message ID | pull.1825.git.1731073435641.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | log: --remerge-diff needs to keep around commit parents | expand |
On Fri, Nov 8, 2024 at 5:43 AM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > To show a remerge diff, the merge needs to be recreated. For that to > work, the merge base(s) need to be found, which means that the commits' > parents have to be traversed until common ancestors are found (if any). > > However, one optimization that hails all the way back to > cb115748ec0d (Some more memory leak avoidance, 2006-06-17) is to release > the commit's list of parents immediately after showing it. This can break > the merge base computation. > > Note that it matters more clearly when traversing the commits in > reverse: In that instance, if a parent of a merge commit has been shown > as part of the `git log` command, by the time the merge commit's diff > needs to be computed, that parent commit's list of parent commits will > have been set to `NULL` and as a result no merge base will be found. > > Let's fix this by special-casing the `remerge_diff` mode, similar to > what we did with reflogs in f35650dff6a4 (log: do not free parents when > walking reflog, 2017-07-07). > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > log: --remerge-diff needs to keep around commit parents > > This fixes a bug that is pretty much as old as the remerge-diff > machinery itself. I noticed it while writing my reply to Hannes Sixt's > concerns about my range-diff --diff-merges=<mode> patch > (https://lore.kernel.org/git/af576487-5de2-fba3-b341-3c082322c9ec@gmx.de/). > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1825%2Fdscho%2Flog-remerge-diff-needs-commit-parents-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1825/dscho/log-remerge-diff-needs-commit-parents-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1825 > > builtin/log.c | 2 +- > t/t4069-remerge-diff.sh | 7 +++++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/builtin/log.c b/builtin/log.c > index c0a8bb95e98..a297c6caf59 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -522,7 +522,7 @@ static int cmd_log_walk_no_free(struct rev_info *rev) > * but we didn't actually show the commit. > */ > rev->max_count++; > - if (!rev->reflog_info) { > + if (!rev->reflog_info && !rev->remerge_diff) { > /* > * We may show a given commit multiple times when > * walking the reflogs. Should this comment be updated to reflect the expanded rationale for this block's purpose? > diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh > index 07323ebafe0..a68c6bfa036 100755 > --- a/t/t4069-remerge-diff.sh > +++ b/t/t4069-remerge-diff.sh > @@ -317,4 +317,11 @@ test_expect_success 'remerge-diff turns off history simplification' ' > test_cmp expect actual > ' > > +test_expect_success 'remerge-diff with --reverse' ' > + git log -1 --remerge-diff --oneline ab_resolution^ >expect && > + git log -1 --remerge-diff --oneline ab_resolution >>expect && > + git log -2 --remerge-diff --oneline ab_resolution --reverse >actual && > + test_cmp expect actual > +' > + > test_done > > base-commit: bea9ecd24b0c3bf06cab4a851694fe09e7e51408 > -- > gitgitgadget Wow, nice find, and I appreciate the nice simple testcase demonstrating what had been the problem.
diff --git a/builtin/log.c b/builtin/log.c index c0a8bb95e98..a297c6caf59 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -522,7 +522,7 @@ static int cmd_log_walk_no_free(struct rev_info *rev) * but we didn't actually show the commit. */ rev->max_count++; - if (!rev->reflog_info) { + if (!rev->reflog_info && !rev->remerge_diff) { /* * We may show a given commit multiple times when * walking the reflogs. diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh index 07323ebafe0..a68c6bfa036 100755 --- a/t/t4069-remerge-diff.sh +++ b/t/t4069-remerge-diff.sh @@ -317,4 +317,11 @@ test_expect_success 'remerge-diff turns off history simplification' ' test_cmp expect actual ' +test_expect_success 'remerge-diff with --reverse' ' + git log -1 --remerge-diff --oneline ab_resolution^ >expect && + git log -1 --remerge-diff --oneline ab_resolution >>expect && + git log -2 --remerge-diff --oneline ab_resolution --reverse >actual && + test_cmp expect actual +' + test_done