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

Re: [PATCH 1/3] rebase -r: do create merge commit after empty resolution

From
PWPhillip 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 &&
Previous: Johannes SchindelinNext: Philippe Blain via GitGitGadget
Message 6 of 17 in “rebase -r: a bugfix and two status-related improvements”
  1. 0/3 rebase -r: a bugfix and two status-related improvementsPhilippe Blain via GitGitGadget, Mar 28, 2025
  2. 1/3 rebase -r: do create merge commit after empty resolutionPhilippe Blain via GitGitGadget, Mar 28, 2025
  3. Eric SunshineMar 28, 2025
  4. Eric SunshineMar 28, 2025
  5. Johannes SchindelinApr 1, 2025
  6. Phillip WoodMar 31, 2025
  7. 2/3 wt-status: also abbreviate 'merge' and 'fixup -C' lines during rebasePhilippe Blain via GitGitGadget, Mar 28, 2025
  8. Phillip WoodMar 31, 2025
  9. 3/3 wt-status: suggest 'git rebase --continue' to conclude 'merge' instructionPhilippe Blain via GitGitGadget, Mar 28, 2025
  10. Phillip WoodMar 31, 2025
  11. Johannes SchindelinApr 1, 2025
  12. phillip.wood123@gmail.comApr 2, 2025
  13. Johannes SchindelinApr 3, 2025
  14. phillip.wood123@gmail.comApr 3, 2025
  15. Johannes SchindelinApr 4, 2025
  16. Phillip WoodApr 4, 2025
  17. Phillip WoodMar 31, 2025

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.