[PATCH] refuse to merge during a merge

[PATCH] refuse to merge during a merge

[Clemens Buchacher]

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.

Reported-by: Dave Olszewski <cxreg@pobox.com>
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

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

diff --git a/builtin-merge.c b/builtin-merge.c
index 0b58e5e..74a8c8f 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -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

Re: [PATCH] refuse to merge during a merge

[Constantine Plotnikov]

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

On Thu, May 28, 2009 at 1:04 AM, Clemens Buchacher <drizzd@aon.at> wrote:
> 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.
>
> Reported-by: Dave Olszewski <cxreg@pobox.com>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---
>
>  builtin-merge.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/builtin-merge.c b/builtin-merge.c
> index 0b58e5e..74a8c8f 100644
> --- a/builtin-merge.c
> +++ b/builtin-merge.c
> @@ -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
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] refuse to merge during a merge

[John Tapsell]

> +       if (read_cache_unmerged() || file_exists(git_path("MERGE_HEAD")))
>                die("You are in the middle of a conflicted merge.");

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

Re: [PATCH] refuse to merge during a merge

[Clemens Buchacher]

On Thu, May 28, 2009 at 05:12:40PM +0100, John Tapsell wrote:
> > +       if (read_cache_unmerged() || file_exists(git_path("MERGE_HEAD")))
> >                die("You are in the middle of a conflicted merge.");
> 
> 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.

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

Re: [PATCH] refuse to merge during a merge

[Jakub Narebski]

Clemens Buchacher <drizzd@aon.at> writes:

> On Thu, May 28, 2009 at 05:12:40PM +0100, John Tapsell wrote:
> > > +       if (read_cache_unmerged() || file_exists(git_path("MERGE_HEAD")))
> > >                die("You are in the middle of a conflicted merge.");
> > 
> > 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.
> 
> 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.

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

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] refuse to merge during a merge

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

Jakub Narebski wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> > 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.
> 
> Why not advertise new "git reset --merge HEAD" then?

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] refuse to merge during a merge

[Clemens Buchacher]

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

Reported-by: Dave Olszewski <cxreg@pobox.com>
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

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(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index 0b58e5e..8169ded 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -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

Re: [PATCH] refuse to merge during a merge

[John Tapsell]

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

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

Re: [PATCH] refuse to merge during a merge

[Clemens Buchacher]

On Sun, May 31, 2009 at 12:05:31PM +0100, John Tapsell wrote:
> > "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).
> 
> Sorry could you just clarify...  if the user does "git reset HEAD"
> will that sometimes always or never fail?

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

Re: [PATCH] refuse to merge during a merge

[Junio C Hamano]

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.

[PATCH v3] refuse to merge during a merge

[Clemens Buchacher]

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.

Reported-by: Dave Olszewski <cxreg@pobox.com>
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Sun, May 31, 2009 at 12:36:37PM -0700, Junio C Hamano wrote:
> 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.

Indeed. I wasn't thinking.

Clemens

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

diff --git a/builtin-merge.c b/builtin-merge.c
index 0b58e5e..9e9bd52 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -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
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 0de613d..9b3fa2b 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -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