Re: [PATCH] refuse to merge during a merge

[Junio C Hamano <gitster@pobox.com>]

Clemens Buchacher <drizzd@aon.at> writes:

> 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.

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.