Subject: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved

From: "Tao Klerks via GitGitGadget" <[email-masked]>
Date: Tue, 02 May 2023 06:27:49 +0000

From: Tao Klerks <[email-masked]>

When a "git checkout" branch-switch operation runs during a merge, the
in-progress merge metadata is removed. The refusal to maintain the merge
matadata makes sense if the new commit is different to the original commit,
because a merge operation against a different commit could have turned out
differently / maintaining the merge relationship would be misleading. It is
still a difficult-to-understand behavior for new users, however, as they
would expect a switch (to a new or same-commit branch at least) to allow
committing local changes "faithfully", as in the case of regular non-merge
local changes.

"git switch" introduces a little more safety, by refusing to switch if there
is a merge in progress - or a number of other operations such as rebase,
cherry-pick, or "git am". This is less of a nasty surprise than the merge
metadata/state being silently discarded, but is still not very helpful, when
a user has a complex merge resolved, and wishes to commit it to a new branch
for testing.

Change the behavior of "git switch" and "git checkout" to no longer delete
merge metadata, nor prohibit the switch, if a merge is in progress and the
commit being switched to is the same commit the HEAD was previously set to.

Also add a warning when the merge metadata is deleted (in case of a
"git checkout" to another commit) to let the user know the merge state
was lost, and that "git switch" would prevent this.

Also add a warning when the merge metadata is preserved (same commit),
to let the user know the commit message prepared for the merge may still
refer to the previous branch.

Add tests to verify the exception works correctly, and to verify that
--force always wipes out in-progress merge metadata when it is discarding
worktree changes.

Signed-off-by: Tao Klerks <[email-masked]>
---
    RFC: switch: allow same-commit switch during merge if conflicts resolved
    
    RFC V1: proposing a limited-scope change as per mailing discussion
    https://lore.kernel.org/git/[email-masked]/T/#t

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1527%2FTaoK%2Ftao-checkout-during-merge-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1527/TaoK/tao-checkout-during-merge-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1527

 branch.c           |  7 ++++-
 branch.h           |  6 +++++
 builtin/checkout.c | 64 ++++++++++++++++++++++++++++++++++++++++++----
 t/t2060-switch.sh  | 18 ++++++++++++-
 4 files changed, 88 insertions(+), 7 deletions(-)

diff --git a/branch.c b/branch.c
index 5aaf073dce1..8cc7fabe599 100644
--- a/branch.c
+++ b/branch.c
@@ -812,10 +812,15 @@ void remove_merge_branch_state(struct repository *r)
 }
 
 void remove_branch_state(struct repository *r, int verbose)
+{
+	remove_branch_state_except_merge(r, verbose);
+	remove_merge_branch_state(r);
+}
+
+void remove_branch_state_except_merge(struct repository *r, int verbose)
 {
 	sequencer_post_commit_cleanup(r, verbose);
 	unlink(git_path_squash_msg(r));
-	remove_merge_branch_state(r);
 }
 
 void die_if_checked_out(const char *branch, int ignore_current_worktree)
diff --git a/branch.h b/branch.h
index ef56103c050..c73d98b8766 100644
--- a/branch.h
+++ b/branch.h
@@ -135,6 +135,12 @@ void remove_merge_branch_state(struct repository *r);
  */
 void remove_branch_state(struct repository *r, int verbose);
 
+/*
+ * Remove information about the state of working on the current
+ * branch, *except* merge state.
+ */
+void remove_branch_state_except_merge(struct repository *r, int verbose);
+
 /*
  * Configure local branch "local" as downstream to branch "remote"
  * from remote "origin".  Used by git branch --set-upstream.
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 21a4335abb0..cae54af997b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -985,7 +985,11 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 				delete_reflog(old_branch_info->path);
 		}
 	}
-	remove_branch_state(the_repository, !opts->quiet);
+
+	remove_branch_state_except_merge(the_repository, !opts->quiet);
+	if (opts->force || old_branch_info->commit != new_branch_info->commit) {
+		remove_merge_branch_state(the_repository);
+	}
 	strbuf_release(&msg);
 	if (!opts->quiet &&
 	    (new_branch_info->path || (!opts->force_detach && !strcmp(new_branch_info->name, "HEAD"))))
@@ -1098,6 +1102,36 @@ static void orphaned_commit_warning(struct commit *old_commit, struct commit *ne
 	release_revisions(&revs);
 }
 
+/*
+ * Check whether we're in a merge, and if so warn - about the ongoing merge and surprising merge
+ * message if the merge state will be preserved, and about the destroyed merge state otherwise.
+ */
+static void merging_checkout_warning(const char *name, struct commit *old_commit,
+				      struct commit *new_commit)
+{
+	struct wt_status_state state;
+	memset(&state, 0, sizeof(state));
+	wt_status_get_state(the_repository, &state, 0);
+
+	if (!state.merge_in_progress)
+	{
+		wt_status_state_free_buffers(&state);
+		return;
+	}
+
+	if (old_commit == new_commit)
+		warning(_("switching while merge-in-progress (without changing commit).\n"
+			  "An auto-generated commit message may still refer to the previous\n"
+			  "branch.\n"));
+	else
+		warning(_("switching to a different commit while while merge-in-progress;\n"
+			  "merge metadata was removed. To avoid accidentally losing merge,\n"
+			  "metadata in this way, please use \"git switch\" instead of\n"
+			  "\"git checkout\".\n"));
+
+	wt_status_state_free_buffers(&state);
+}
+
 static int switch_branches(const struct checkout_opts *opts,
 			   struct branch_info *new_branch_info)
 {
@@ -1153,6 +1187,9 @@ static int switch_branches(const struct checkout_opts *opts,
 	if (!opts->quiet && !old_branch_info.path && old_branch_info.commit && new_branch_info->commit != old_branch_info.commit)
 		orphaned_commit_warning(old_branch_info.commit, new_branch_info->commit);
 
+	if (!opts->quiet && !opts->force)
+		merging_checkout_warning(old_branch_info.name, old_branch_info.commit, new_branch_info->commit);
+
 	update_refs_for_switch(opts, &old_branch_info, new_branch_info);
 
 	ret = post_checkout_hook(old_branch_info.commit, new_branch_info->commit, 1);
@@ -1445,15 +1482,31 @@ static void die_expecting_a_branch(const struct branch_info *branch_info)
 	exit(code);
 }
 
-static void die_if_some_operation_in_progress(void)
+static void die_if_some_incompatible_operation_in_progress(struct commit *new_commit)
 {
+	/*
+	 * Note: partially coordinated logic in related function
+	 * merging_checkout_warning(), checking for merge_in_progress
+	 * and old_commit != new_commit to issue warnings. Issuing those
+	 * warnings here would be simpler to implement, but would make the
+	 * language more complex to account for common situations where the
+	 * switch still won't happen (namely working tree merge failure).
+	 */
+
 	struct wt_status_state state;
+	struct branch_info old_branch_info = { 0 };
+	struct object_id rev;
+	int flag;
 
 	memset(&state, 0, sizeof(state));
 	wt_status_get_state(the_repository, &state, 0);
+	memset(&old_branch_info, 0, sizeof(old_branch_info));
+	old_branch_info.path = resolve_refdup("HEAD", 0, &rev, &flag);
+	if (old_branch_info.path)
+		old_branch_info.commit = lookup_commit_reference_gently(the_repository, &rev, 1);
 
-	if (state.merge_in_progress)
-		die(_("cannot switch branch while merging\n"
+	if (state.merge_in_progress && old_branch_info.commit != new_commit)
+		die(_("cannot switch to a different commit while merging\n"
 		      "Consider \"git merge --quit\" "
 		      "or \"git worktree add\"."));
 	if (state.am_in_progress)
@@ -1476,6 +1529,7 @@ static void die_if_some_operation_in_progress(void)
 		warning(_("you are switching branch while bisecting"));
 
 	wt_status_state_free_buffers(&state);
+	branch_info_release(&old_branch_info);
 }
 
 static int checkout_branch(struct checkout_opts *opts,
@@ -1536,7 +1590,7 @@ static int checkout_branch(struct checkout_opts *opts,
 		die_expecting_a_branch(new_branch_info);
 
 	if (!opts->can_switch_when_in_progress)
-		die_if_some_operation_in_progress();
+		die_if_some_incompatible_operation_in_progress(new_branch_info->commit);
 
 	if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
 	    !opts->ignore_other_worktrees) {
diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
index 5a7caf958c3..9c80d469b6b 100755
--- a/t/t2060-switch.sh
+++ b/t/t2060-switch.sh
@@ -111,13 +111,29 @@ test_expect_success 'guess and create branch' '
 	test_cmp expected actual
 '
 
-test_expect_success 'not switching when something is in progress' '
+test_expect_success 'not switching to a different commit when something is in progress' '
 	test_when_finished rm -f .git/MERGE_HEAD &&
 	# fake a merge-in-progress
 	cp .git/HEAD .git/MERGE_HEAD &&
 	test_must_fail git switch -d @^
 '
 
+test_expect_success 'switching to same-commit when merge is in progress succeeds' '
+	test_when_finished rm -f .git/MERGE_HEAD &&
+	# fake a merge-in-progress
+	cp .git/HEAD .git/MERGE_HEAD &&
+	git switch -d @ &&
+	# confirm the merge-in-progress is still there
+	test -e .git/MERGE_HEAD
+'
+test_expect_success 'switching with --force removes merge state' '
+	test_when_finished rm -f .git/MERGE_HEAD &&
+	# fake a merge-in-progress
+	cp .git/HEAD .git/MERGE_HEAD &&
+	git switch --force -d @ &&
+	# confirm the merge-in-progress is removed
+	test ! -e .git/MERGE_HEAD
+'
 test_expect_success 'tracking info copied with autoSetupMerge=inherit' '
 	# default config does not copy tracking info
 	git switch -c foo-no-inherit foo &&

base-commit: 950264636c68591989456e3ba0a5442f93152c1a
-- 
gitgitgadget