git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Elijah Newren <newren@gmail.com>
Date
Nov 8, 2024, 15:47 UTC
Message-ID
<CABPp-BHauuserYU8t5ybn+ghqRb-c85qwwjLWVe-e5EC8sVVxQ@mail.gmail.com>
In-Reply-To
<pull.1825.git.1731073435641.gitgitgadget@gmail.com>

On Fri, Nov 8, 2024 at 5:43 AM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 52 quoted lines
>
> 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?

Show 20 quoted lines
> 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.

Previous: Johannes Schindelin via GitGitGadgetNext: Junio C Hamano
Message 2 of 9 in “log: --remerge-diff needs to keep around commit parents”
  1. log: --remerge-diff needs to keep around commit parentsJohannes Schindelin via GitGitGadget, Nov 8, 2024
  2. Elijah NewrenNov 8, 2024
  3. Junio C HamanoNov 11, 2024
  4. Junio C HamanoNov 11, 2024
  5. Johannes SchindelinNov 11, 2024
  6. Junio C HamanoNov 11, 2024
  7. log: --remerge-diff needs to keep around commit parentsJohannes Schindelin via GitGitGadget, Nov 11, 2024
  8. log: --remerge-diff needs to keep around commit parentsJohannes Schindelin via GitGitGadget, Dec 12, 2024
  9. Junio C HamanoDec 13, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.