From: Phillip Wood Date: Tue, 16 Dec 2025 14:19:11 GMT Subject: Re: [PATCH v2] replay: drop commits that become empty Message-ID: <80477d23-eed3-4a99-be97-f692bc36095e@gmail.com> In-Reply-To: On 15/12/2025 23:50, Junio C Hamano wrote: > Phillip Wood writes: > >> From: Phillip Wood >> >> If the changes in a commit being replayed are already in the branch >> that the commits are being replayed onto then "git replay" creates an >> empty commit. This is confusing because the commit message no longer >> matches the contents of the commit. Drop the commit instead. Commits >> that start off empty are not dropped. This matches the behavior of >> "git rebase --reapply-cherry-pick --empty=drop" and "git cherry-pick >> --empty-drop". > > OK. Maybe it is just me but "onto then" -> "onto," would flow the > sentence better? I agree it reads better with a comma here and in the second paragraph, I'll re-roll Thanks Phillip >> If a branch points to a commit that is dropped it will be updated to >> point to the last commit that was not dropped. This can been seen > > If one thinks about it, it is the only natural behaviour to use the > last surviving commit to point the branch at. Thanks for spelling > it out so clearly. > > BTW, "can been seen" -> "can be seen" (will amend locally). > >> in the new test where "topic1" is updated to point to the rebased >> "C" as "F" is dropped because it is already upstream. While this is >> a breaking change "git replay" is marked as experimental to allow >> improvements like this that change the behavior. > > Again maybe it is just me, but I'd prefer to see a comma after "a > breaking change" to flow the sentence better. > >> Helped-by: Elijah Newren >> Signed-off-by: Phillip Wood >> --- >> ... >> diff --git a/Documentation/git-replay.adoc b/Documentation/git-replay.adoc >> index dcb26e8a8e8..96a3a557bf3 100644 >> --- a/Documentation/git-replay.adoc >> +++ b/Documentation/git-replay.adoc >> @@ -59,7 +59,9 @@ The default mode can be configured via the `replay.refAction` configuration vari >> be passed, but in `--advance ` mode, they should have >> a single tip, so that it's clear where should point >> to. See "Specifying Ranges" in linkgit:git-rev-parse[1] and the >> - "Commit Limiting" options below. >> + "Commit Limiting" options below. Any commits in the range whose >> + changes are already present in the branch the commits are being >> + replayed onto will be dropped. > > OK. > >> diff --git a/replay.c b/replay.c >> index 13983dbc566..2864c213993 100644 >> --- a/replay.c >> +++ b/replay.c >> @@ -88,12 +88,12 @@ struct commit *replay_pick_regular_commit(struct repository *repo, >> struct merge_result *result) >> { >> struct commit *base, *replayed_base; >> - struct tree *pickme_tree, *base_tree; >> + struct tree *pickme_tree, *base_tree, *replayed_base_tree; >> >> base = pickme->parents->item; >> replayed_base = mapped_commit(replayed_commits, base, onto); >> >> - result->tree = repo_get_commit_tree(repo, replayed_base); >> + replayed_base_tree = repo_get_commit_tree(repo, replayed_base); >> pickme_tree = repo_get_commit_tree(repo, pickme); >> base_tree = repo_get_commit_tree(repo, base); >> >> @@ -103,13 +103,17 @@ struct commit *replay_pick_regular_commit(struct repository *repo, >> >> merge_incore_nonrecursive(merge_opt, >> base_tree, >> - result->tree, >> + replayed_base_tree, >> pickme_tree, >> result); >> >> free((char*)merge_opt->ancestor); >> merge_opt->ancestor = NULL; >> if (!result->clean) >> return NULL; >> + /* Drop commits that become empty */ >> + if (oideq(&replayed_base_tree->object.oid, &result->tree->object.oid) && >> + !oideq(&pickme_tree->object.oid, &base_tree->object.oid)) >> + return replayed_base; >> return replay_create_commit(repo, result->tree, pickme, replayed_base); >> } > > OK, that is straight-forward. Instead of overriding the > result->tree upfront, we try the same using a temporary > replayed_base_tree, and that allows us to see if the resulting tree > computed by merge_incore matches. Only when it made a non-empty > change, we proceed to create a new commit. > >> diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh >> index cf3aacf3551..9d4b0dd1a77 100755 >> --- a/t/t3650-replay-basics.sh >> +++ b/t/t3650-replay-basics.sh >> @@ -25,6 +25,8 @@ test_expect_success 'setup' ' >> git switch -c topic3 && >> test_commit G && >> test_commit H && >> + git switch -c empty && >> + git commit --allow-empty --only -m empty && > > The use of "--only" here is a bit curious. As there is no change > between the index and the commit our "empty" branch points at, > wouldn't it be unnecessary? The option, together with --allow-empty, > would only matter if you did > > git switch -c empty && > modify blah && > git add blah && > git commit --allow-empty --only -m empty > > because without --only, the changes to blah will be taken.