Re: [PATCH 1/3] rebase -r: do create merge commit after empty resolution
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Mar 31, 2025, 15:37 UTC
- Message-ID
- <b0263bdb-002a-4a88-b277-fd2afe59cfe6@gmail.com>
- In-Reply-To
- <6c8f77cb71c7e0c820704b1725331f4601d8876e.1743181401.git.gitgitgadget@gmail.com>
Hi Philippe
On 28/03/2025 17:03, Philippe Blain via GitGitGadget wrote:
Show 27 quoted lines
> From: Philippe Blain <levraiphilippeblain@gmail.com> > > 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.
Show 15 quoted lines
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> 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.
Show 23 quoted lines
> +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 &&