threads / patch / 19541

patchrefuse to merge during a merge

Subject: [PATCH] refuse to merge during a merge

## tl;dr

11 messages between May 27, 2009 and Jun 1, 2009. Diffs are folded; open one to read it.

replies: 10people: 6as markdown or json

Clemens Buchacher· May 27, 2009, 21:04 UTC · lore

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(-)
Show changes to builtin-merge.c +1 −1
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
Constantine Plotnikov· May 28, 2009, 16:00 UTC · re: Clemens Buchacher · lore

Re: [PATCH] refuse to merge during a merge

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:
Show 39 quoted lines
> 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
>
John Tapsell· May 28, 2009, 16:12 UTC · re: Clemens Buchacher · lore

Re: [PATCH] refuse to merge during a merge

> +       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
Clemens Buchacher· May 30, 2009, 08:37 UTC · re: John Tapsell · lore

Re: [PATCH] refuse to merge during a merge

On Thu, May 28, 2009 at 05:12:40PM +0100, John Tapsell wrote:
Show 7 quoted lines
> > +       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
Jakub Narebski· May 30, 2009, 10:38 UTC · re: Clemens Buchacher · lore

Re: [PATCH] refuse to merge during a merge

Clemens Buchacher <drizzd@aon.at> writes:
Show 19 quoted lines
> 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
Thomas Rast· May 30, 2009, 10:57 UTC · re: Jakub Narebski · lore

Re: [PATCH] refuse to merge during a merge

Jakub Narebski wrote:
Show 11 quoted lines
> 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
Clemens Buchacher· May 31, 2009, 10:43 UTC · re: Clemens Buchacher · lore

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(-)
Show changes to builtin-merge.c +8 −2
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
John Tapsell· May 31, 2009, 11:05 UTC · re: Clemens Buchacher · lore

Re: [PATCH] refuse to merge during a merge

> "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
Clemens Buchacher· May 31, 2009, 14:05 UTC · re: John Tapsell · lore

Re: [PATCH] refuse to merge during a merge

On Sun, May 31, 2009 at 12:05:31PM +0100, John Tapsell wrote:
Show 6 quoted lines
> > "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
Junio C Hamano· May 31, 2009, 19:36 UTC · re: Clemens Buchacher · lore

Re: [PATCH] refuse to merge during a merge

Clemens Buchacher <drizzd@aon.at> writes:
Show 8 quoted lines
> 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.
Clemens Buchacher· Jun 1, 2009, 09:20 UTC · re: Junio C Hamano · lore

[PATCH v3] 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 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:
Show 7 quoted lines
> 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(-)
Show changes to 2 files +7 −1

builtin-merge.c, t/t3030-merge-recursive.sh

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

← back to recent threads