[RFC 0/3] Make git more user-friendly during a merge conflict

Users may not be aware that they need to use "git merge --abort" or "git reset
--merge" to properly abort a merge conflict. They are likely to just use "git
reset", because that "usually" cleans up the repo. But in the case where the
user had local changes, "git reset" would leave the repo in a messy state where
merged changes and local changes are mixed together.

The first two patches are just about rewording a message, and adding messages
to tell users to use "git merge --abort" to abort a merge.

We could stop here and hope that the users would read the messages, but I think
git could be a bit more user-friendly. The last patch might be a bit more
controversial. It changes the default behavior of "git reset" to default to
"git reset --merge" during a merge conflict. I imagine that's what the user
would want most of the time, and not "git reset --mixed". I haven't updated the
"git reset" docs yet, I'll update it if we decide to use this new behavior.

Comments?

Andrew Wong (3):
  wt-status: Make conflict hint message more consistent with other hints
  merge: Add hints to tell users about "git merge --abort"
  reset: Change the default behavior to use "--merge" during a merge

 builtin/merge.c | 3 ++-
 builtin/reset.c | 7 ++++++-
 wt-status.c     | 5 ++++-
 3 files changed, 12 insertions(+), 3 deletions(-)

-- 
1.9.0.6.g16e5f9a

Signed-off-by: Andrew Wong <redacted>
---
 builtin/merge.c | 3 ++-
 wt-status.c     | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index e576a7f..07af427 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -909,7 +909,8 @@ static int suggest_conflicts(int renormalizing)
 	fclose(fp);
 	rerere(allow_rerere_auto);
 	printf(_("Automatic merge failed; "
-			"fix conflicts and then commit the result.\n"));
+			"fix conflicts and then commit the result.\n"
+			"To abort the merge, use \"git merge --abort\".\n"));
 	return 1;
 }
 

diff --git a/wt-status.c b/wt-status.c
index 6e1ad7d..54c2203 100644
--- a/wt-status.c
+++ b/wt-status.c

@@ -907,6 +907,9 @@ static void show_merge_in_progress(struct wt_status *s,
 			status_printf_ln(s, color,
 				_("  (use \"git commit\" to conclude merge)"));
 	}
+	if (s->hints)
+		status_printf_ln(s, color,
+			_("  (use \"git merge --abort\" to abort the merge)\n"));
 	wt_status_print_trailer(s);
 }
 

-- 
1.9.0.6.g16e5f9a

Signed-off-by: Andrew Wong <redacted>
---
 wt-status.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index 4e55810..6e1ad7d 100644
--- a/wt-status.c
+++ b/wt-status.c

@@ -899,7 +899,7 @@ static void show_merge_in_progress(struct wt_status *s,
 		status_printf_ln(s, color, _("You have unmerged paths."));
 		if (s->hints)
 			status_printf_ln(s, color,
-				_("  (fix conflicts and run \"git commit\")"));
+				_("  (fix conflicts, and use \"git commit\" to conclude the merge)"));
 	} else {
 		status_printf_ln(s, color,
 			_("All conflicts fixed but you are still merging."));

-- 
1.9.0.6.g16e5f9a

If the user wants to do "git reset" during a merge, the user most likely
wants to do a "git reset --merge". This is especially true during a
merge conflict and the user had local changes, because "git reset" would
leave the merged changes mixed in with the local changes. This makes
"git reset" a little more user-friendly during a merge.

Signed-off-by: Andrew Wong <redacted>
---
 builtin/reset.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 6004803..740263d 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -318,7 +318,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 					_(reset_type_names[reset_type]));
 	}
 	if (reset_type == NONE)
-		reset_type = MIXED; /* by default */
+	{
+		if(is_merge())
+			reset_type = MERGE;
+		else
+			reset_type = MIXED;
+	}
 
 	if (reset_type != SOFT && reset_type != MIXED)
 		setup_work_tree();

-- 
1.9.0.6.g16e5f9a

Hi,

Andrew Wong wrote:

The first two patches are just about rewording a message, and adding messages
to tell users to use "git merge --abort" to abort a merge.

Sounds like a good idea.  I look forward to reading the patches.

We could stop here and hope that the users would read the messages, but I think
git could be a bit more user-friendly. The last patch might be a bit more
controversial. It changes the default behavior of "git reset" to default to
"git reset --merge" during a merge conflict. I imagine that's what the user
would want most of the time, and not "git reset --mixed".

I don't think that's a good idea.  I'm not sure what new users would
expect; in any case, making the command context-dependent just makes
the learning process harder imho.  And for experienced users, this
would be a bad regression.

An 'advice' message pointing the user to 'git merge --abort' might
make sense, though.

What do you think?

Thanks,
Jonathan

Hi,

Andrew Wong wrote:

[Subject: wt-status: Make conflict hint message more consistent with other hints]

Thanks for working on this.

Could you include a little more detail?  What other hints is this
making the message more consistent with?

Ideally the commit message would include a quick sample interaction,
so the reviewer could see the user going "Wha?" and then look at the
patch to see how it resolves the confusion.

[...]

Quick thoughts:

 - The comma just moves the message closer to the right margin.  I think
   it makes the message less readable.

 - What else would "git commit" do other than concluding the merge?
   What confusion is this meant to prevent?

 - Would introducing a new "git merge --continue" command help?

   Advantages: (1) the name of the command makes it obvious what
   it does; (2) the command could check that there is actually
   a merge in progress, helping the user when the state is not
   what they think; (3) consistency with "git cherry-pick --abort" /
   "git cherry-pick --continue".

   Disadvantage: redundancy (but see (2) above).

Hope that helps,
Jonathan

Andrew Wong wrote:

Seems reasonable, but I worry about the command growing too noisy.

Could this be guarded by an advice.<something> setting?  (See advice.*
in git-config(1) for what I mean.)

[...]

Perhaps:

	...
		_("  (or use \"git merge --abort\" to abort the merge)"));

to clarify that this is an alternative to the advice immediately above.

`status_printf_ln` already prints a newline, so the translated message
shouldn't include an extra one.

Thanks,
Jonathan

On Wed, Feb 26, 2014 at 3:38 PM, Jonathan Nieder [off-list ref] wrote:

Seems reasonable, but I worry about the command growing too noisy.

Could this be guarded by an advice.<something> setting?  (See advice.*
in git-config(1) for what I mean.)

Ah, good idea. This seems to belong to advice.resolveConflict. I'll
add it there.

`status_printf_ln` already prints a newline, so the translated message
shouldn't include an extra one.

Good catch. Thanks!

Andrew

On Wed, Feb 26, 2014 at 3:38 PM, Jonathan Nieder [off-list ref] wrote:

Andrew Wong wrote:

quoted

--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -909,7 +909,8 @@ static int suggest_conflicts(int renormalizing)
      fclose(fp);
      rerere(allow_rerere_auto);
      printf(_("Automatic merge failed; "
-                     "fix conflicts and then commit the result.\n"));
+                     "fix conflicts and then commit the result.\n"
+                     "To abort the merge, use \"git merge --abort\".\n"));

Seems reasonable, but I worry about the command growing too noisy.

Could this be guarded by an advice.<something> setting?  (See advice.*
in git-config(1) for what I mean.)

I was planning to use advice.resolveConflict, but as I went through
merge.c, I noticed there could be a few other situations where we
could print out the same message:
1. when prepare_to_commit() fails, due to hook error, editor error, or
empty commit message
2. "git commit --no-commit"

This means contexts are no longer only about "resolving conflict", so
I was thinking of renaming advice.resolveConflict to something like
advice.mergeHints.

Any thoughts?