diff mbox series

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

Message ID pull.1825.v2.git.1731350009491.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series [v2] log: --remerge-diff needs to keep around commit parents | expand

Commit Message

Johannes Schindelin Nov. 11, 2024, 6:33 p.m. UTC 
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/).
    
    Changes since v1:
    
     * Amended the code comment of the affected conditional code block in
       harmony with the newly-added condition.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1825%2Fdscho%2Flog-remerge-diff-needs-commit-parents-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1825/dscho/log-remerge-diff-needs-commit-parents-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1825

Range-diff vs v1:

 1:  68238315c14 ! 1:  afff27f8222 log: --remerge-diff needs to keep around commit parents
     @@ builtin/log.c: static int cmd_log_walk_no_free(struct rev_info *rev)
      +		if (!rev->reflog_info && !rev->remerge_diff) {
       			/*
       			 * We may show a given commit multiple times when
     - 			 * walking the reflogs.
     +-			 * walking the reflogs.
     ++			 * walking the reflogs. Therefore we still need it.
     ++			 *
     ++			 * Likewise, we potentially still need the parents
     ++			 * of * already shown commits to determine merge
     ++			 * bases when showing remerge diffs.
     + 			 */
     + 			free_commit_buffer(the_repository->parsed_objects,
     + 					   commit);
      
       ## t/t4069-remerge-diff.sh ##
      @@ t/t4069-remerge-diff.sh: test_expect_success 'remerge-diff turns off history simplification' '


 builtin/log.c           | 8 ++++++--
 t/t4069-remerge-diff.sh | 7 +++++++
 2 files changed, 13 insertions(+), 2 deletions(-)


base-commit: bea9ecd24b0c3bf06cab4a851694fe09e7e51408
diff mbox series

Patch

diff --git a/builtin/log.c b/builtin/log.c
index c0a8bb95e98..2f88a3e607d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -522,10 +522,14 @@  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.
+			 * walking the reflogs. Therefore we still need it.
+			 *
+			 * Likewise, we potentially still need the parents
+			 * of * already shown commits to determine merge
+			 * bases when showing remerge diffs.
 			 */
 			free_commit_buffer(the_repository->parsed_objects,
 					   commit);
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