Re: [PATCH] log: --remerge-diff needs to keep around commit parents

[Elijah Newren <newren@gmail.com>]

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.