refuse to merge during a merge

The following is an easy mistake to make for users coming from version control systems with an "update and commit"-style workflow.

1. git merge
2. resolve conflicts
3. git pull, instead of commit

This overrides MERGE_HEAD, starting a new merge with dirty index. IOW, probably not what the user intented. Instead, refuse to merge again if a merge is in progress.

---

 builtin-merge.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 0b58e5e..74a8c8f 100644
@@ -836,7 +836,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	struct commit_list **remotes = &remoteheads;
 
 	setup_work_tree();
-	if (read_cache_unmerged())
+	if (read_cache_unmerged() || file_exists(git_path("MERGE_HEAD")))
 		die("You are in the middle of a conflicted merge.");
 
 	/*

1.6.3.1.147.g637c3

Could the error message also give possible solutions? "Commit the
current merge first with 'git commit', or discard the current merge attempt with 'git reset --hard'" or something. Or at least a pointer to where to read for more info.

John

How about this.

fatal: You are in the middle of a [conflicted] merge. To complete the merge [resolve conflicts and] commit the changes. To abort, use "git reset HEAD".

The part about resolving changes is only displayed if there are unmerged entries. I intentionally left out --hard, because it potentially removes changes unrelated to the merge (if the work tree was dirty prior to the merge). The user will find out how to reset the work tree by reading the docs.

Clemens

Why not advertise new "git reset --merge HEAD" then?

Jakub Narebski
Poland
ShadeHawk on #git

That doesn't deal with conflicts at all. It fills the rather different case where you did a clean merge with some uncommitted changes in the worktree, but then want to discard the merge again without losing the uncommitted changes. In absence of the changes, you would just use --hard, but here you want to move the branch tip while merging them over, similar to what 'git checkout -m' does for moving HEAD.

Thomas Rast
trast@{inf,student}.ethz.ch

The following is an easy mistake to make for users coming from version control systems with an "update and commit"-style workflow.

1. git pull
2. resolve conflicts
3. git pull

Step 3 overrides MERGE_HEAD, starting a new merge with dirty index. IOW, probably not what the user intented. Instead, refuse to merge again if a merge is in progress and present the user with his options.

"git reset --hard" is not suggested, because it potentially removes changes unrelated to the merge (if the work tree was dirty prior to the merge).

---

Ok, since I'm not seeing any more objections. Here's the code.

Clemens

 builtin-merge.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

index 0b58e5e..8169ded 100644
@@ -834,10 +834,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	struct commit_list *common = NULL;
 	const char *best_strategy = NULL, *wt_strategy = NULL;
 	struct commit_list **remotes = &remoteheads;
+	int unmerged;
 
 	setup_work_tree();
-	if (read_cache_unmerged())
-		die("You are in the middle of a conflicted merge.");
+	unmerged = read_cache_unmerged();
+	if (unmerged || file_exists(git_path("MERGE_HEAD")))
+		die("You are in the middle of a %smerge. To complete "
+			"the merge %scommit the changes. To abort, "
+			"use \"git reset HEAD\".",
+			unmerged ? "conflicted " : "",
+			unmerged ? "resolve conflicts and " : "");
 
 	/*
 	 * Check if we are _not_ on a detached HEAD, i.e. if there is a

1.6.3.1.147.g637c3

Sorry could you just clarify... if the user does "git reset HEAD" will that sometimes always or never fail?

If it sometimes or always fails, then doesn't it seem kinda confusing if the user is told to run that command, but then when they do they get an error?

John

It will never fail. It aborts the merge as suggested. But "git reset --hard HEAD" would also reset the work tree, so that "any changes to tracked files in the working tree since <commit> are lost." This is generally desireable, since an incomplete merge also leaves the auto-merged files in the work tree.

But if the user does not already know that, it's better to leave the user wondering how to clean a dirty work tree, than to suggest a potentially harmful operation.

Clemens

I think the new condition that you added to stop the merge is more in line with the original intent of the check. We never wanted to check "do we still have unmerged entries?" but wanted to see "is another merge in progress?"; not checking MERGE_HEAD was a simple omission.

But I do not necessarily agree with the combined check nor with the new message. I think it would be more sensible to split the codepath like this:

	if (we see MERGE_HEAD)
		die("You have not concluded your merge (MERGE_HEAD exists).");
	if (the index is unmerged)
		die("You are in the middle of a conflicted merge (index unmerged).");

Then in a _later_ patch, you could try to be more helpful by paying more attention to the context. E.g.

	if (we see MERGE_HEAD) {
        	figure out what was attempted by looking at
                MERGE_MSG and other cues;
		die("You have not concluded your merge with %s.\n"
		    "Perhaps you would want 'git reset' to recover?"
                    that);
	}
	if (the index is unmerged)
		die("You are in the middle of a conflicted merge");

The point is that combining the checks makes it harder to later give more appropriate diagnosis and suggestion to the end user.

For example, "git merge" may learn "git merge --abort" like other commands that have "attempt, stop, let the user fix up to conclude" modes of operations (i.e. rebase and am), and we may suggest to use that to recover in the message, instead of 'git reset'. But that can only be used if we stopped because we saw MERGE_HEAD; you definitely do not want to suggest "git merge --abort" if the index is unmerged due to a conflicted rebase in progress.

Note that I am not suggesting you to blow this up to one large patch by adding fancier "what were we doing" logic; I am perfectly OK with the minimum "detect MERGE_HEAD and refuse". I am only saying that I am unhappy with the way two different error conditions are conflated.

Personally, I'd suggest not to give "you can do this to recover" message.

The following is an easy mistake to make for users coming from version control systems with an "update and commit"-style workflow.

1. git pull
2. resolve conflicts
3. git pull

Step 3 overrides MERGE_HEAD, starting a new merge with dirty index. IOW, probably not what the user intended. Instead, refuse to merge again if a merge is in progress.

---

Indeed. I wasn't thinking.

Clemens

 builtin-merge.c            |    5 ++++-
 t/t3030-merge-recursive.sh |    3 +++
 2 files changed, 7 insertions(+), 1 deletions(-)

index 0b58e5e..9e9bd52 100644
@@ -836,8 +836,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	struct commit_list **remotes = &remoteheads;
 
 	setup_work_tree();
+	if (file_exists(git_path("MERGE_HEAD")))
+		die("You have not concluded your merge. (MERGE_HEAD exists)");
 	if (read_cache_unmerged())
-		die("You are in the middle of a conflicted merge.");
+		die("You are in the middle of a conflicted merge."
+				" (index unmerged)");
 
 	/*
 	 * Check if we are _not_ on a detached HEAD, i.e. if there is a

index 0de613d..9b3fa2b 100755
@@ -276,6 +276,9 @@ test_expect_success 'fail if the index has unresolved entries' '
 
 	test_must_fail git merge "$c5" &&
 	test_must_fail git merge "$c5" 2> out &&
+	grep "You have not concluded your merge" out &&
+	rm -f .git/MERGE_HEAD &&
+	test_must_fail git merge "$c5" 2> out &&
 	grep "You are in the middle of a conflicted merge" out
 
 '

1.6.3.1.147.g637c3

MERGE_HEAD file could also happen in case of --no-commit option. In that case there might be no conflict and the message would look confusing to the user. I suggest to change a message to "You are in the middle of a uncommitted or conflicted merge." or something like it.

Regards,
Constantine