From: Phillip Wood Date: Mon, 31 Mar 2025 15:37:41 GMT Subject: Re: [PATCH 1/3] rebase -r: do create merge commit after empty resolution Message-ID: In-Reply-To: <6c8f77cb71c7e0c820704b1725331f4601d8876e.1743181401.git.gitgitgadget@gmail.com> Hi Philippe On 28/03/2025 17:03, Philippe Blain via GitGitGadget wrote: > From: Philippe Blain > > When a user runs 'git rebase --continue' to conclude a conflicted merge > during a 'git rebase -r' invocation, we do not create a merge commit if > the resolution was empty (i.e. if the index and HEAD are identical). We > simply continue the rebase as if no 'merge' instruction had been given. > This is confusing since all commits from the side branch are absent from > the rebased history. What's more, if that 'merge' is the last > instruction in the todo list, we fail to remove the merge state, such > that running 'git status' shows we are still merging after the rebase > has concluded. > > This happens because in 'sequencer.c::commit_staged_changes', we exit > early before calling 'run_git_commit' if 'is_clean' is true, i.e. if > nothing is staged. Fix this by also checking for the presence of > MERGE_HEAD before exiting early, such that we do call 'run_git_commit' > when MERGE_HEAD is present. This also ensures that we unlink > git_path_merge_head later in 'commit_staged_changes' to clear the merge > state. > > Make sure to also remove MERGE_HEAD when a merge command fails to start. > We already remove MERGE_MSG since e032abd5a0 (rebase: fix rewritten list > for failed pick, 2023-09-06). Removing MERGE_HEAD ensures that in this > situation, upon 'git rebase --continue' we still exit early in > 'commit_staged_changes', without calling 'run_git_commit'. This is > already covered by t5407.11, which fails without this change because we > enter 'run_git_commit' and then fail to find 'rebase_path_message'. Thanks for fixing this. > Signed-off-by: Philippe Blain > --- > sequencer.c | 3 ++- > t/t3418-rebase-continue.sh | 24 ++++++++++++++++++++++++ > 2 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/sequencer.c b/sequencer.c > index ad0ab75c8d4..2baaf716a3c 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -4349,6 +4349,7 @@ static int do_merge(struct repository *r, > error(_("could not even attempt to merge '%.*s'"), > merge_arg_len, arg); > unlink(git_path_merge_msg(r)); > + unlink(git_path_merge_head(r)); I think we want to clean up git_path_merge_mode() as well. Perhaps we should call remove_merge_branch_state() instead of deleting the individual files ourselves here. > +test_expect_success '--continue creates merge commit after empty resolution' ' > + git reset --hard main && > + git checkout -b rebase_i_merge && > + test_commit unrelated && > + git checkout -b rebase_i_merge_side && > + test_commit side2 main.txt && > + git checkout rebase_i_merge && > + test_commit side1 main.txt && > + PICK=$(git rev-parse --short rebase_i_merge) && > + test_must_fail git merge rebase_i_merge_side && > + echo side1 >main.txt && > + git add main.txt && > + test_tick && > + git commit --no-edit && > + FAKE_LINES="1 2 3 5 6 7 8 9 10 11" && > + export FAKE_LINES && > + test_must_fail git rebase -ir main && > + echo side1 >main.txt && > + git add main.txt && > + git rebase --continue && > + git log --merges >out && > + test_grep "Merge branch '\''rebase_i_merge_side'\''" out > +' I wonder if t3430 would be a better home for this as it already has the setup necessary to create a failing merge. It would be good to add a test to check that "git rebase --skip" does not create an empty merge as well. Thanks Phillip > test_expect_success '--skip after failed fixup cleans commit message' ' > test_when_finished "test_might_fail git rebase --abort" && > git checkout -b with-conflicting-fixup &&