[RFC/PATCH] commit: make the error message on unmerged entries user-friendly.

[RFC/PATCH] commit: make the error message on unmerged entries user-friendly.

[Matthieu Moy]

The error message used to look like this:

$ git commit
foo.txt: needs merge
foo.txt: unmerged (c34a92682e0394bc0d6f4d4a67a8e2d32395c169)
foo.txt: unmerged (3afcd75de8de0bb5076942fcb17446be50451030)
foo.txt: unmerged (c9785d77b76dfe4fb038bf927ee518f6ae45ede4)
error: Error building trees

The "need merge" line is given by refresh_cache. We add the IN_PORCELAIN
option to make the output more consistant with the other porcelain
commands, and catch the error in return, to stop with a clean error
message. The next lines were displayed by a call to cache_tree_update(),
which is not reached anymore if we noticed the conflict.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
As usual, I try to have error messages point to the solution, not just
the origin of the problem.

Two questions:

* Did anyone actually use the 3 "file: unmerged (sha1)" lines?

* Do you like my new message?

Thanks,

 builtin-commit.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 3dfcd77..d491a01 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -235,6 +235,18 @@ static void create_base_index(void)
 		exit(128); /* We've already reported the error, finish dying */
 }
 
+static void refresh_cache_or_die(int refresh_flags)
+{
+	/*
+	 * refresh_flags contains REFRESH_QUIET, so the only errors
+	 * are for unmerged entries.
+	*/
+	if(refresh_cache(refresh_flags | REFRESH_IN_PORCELAIN))
+		die("Commit is not possible because you have unmerged files.\n"
+		    "Please, resolve the conflicts listed above, and then mark them\n"
+		    "as resolved with 'git add <filename>', or use 'git commit -a'.");
+}
+
 static char *prepare_index(int argc, const char **argv, const char *prefix, int is_status)
 {
 	int fd;
@@ -274,7 +286,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 	if (all || (also && pathspec && *pathspec)) {
 		int fd = hold_locked_index(&index_lock, 1);
 		add_files_to_cache(also ? prefix : NULL, pathspec, 0);
-		refresh_cache(refresh_flags);
+		refresh_cache_or_die(refresh_flags);
 		if (write_cache(fd, active_cache, active_nr) ||
 		    close_lock_file(&index_lock))
 			die("unable to write new_index file");
@@ -293,7 +305,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 	 */
 	if (!pathspec || !*pathspec) {
 		fd = hold_locked_index(&index_lock, 1);
-		refresh_cache(refresh_flags);
+		refresh_cache_or_die(refresh_flags);
 		if (write_cache(fd, active_cache, active_nr) ||
 		    commit_locked_index(&index_lock))
 			die("unable to write new_index file");
-- 
1.6.6.76.gd6b23.dirty

Re: [RFC/PATCH] commit: make the error message on unmerged entries user-friendly.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> The error message used to look like this:
>
> $ git commit
> foo.txt: needs merge
> foo.txt: unmerged (c34a92682e0394bc0d6f4d4a67a8e2d32395c169)
> foo.txt: unmerged (3afcd75de8de0bb5076942fcb17446be50451030)
> foo.txt: unmerged (c9785d77b76dfe4fb038bf927ee518f6ae45ede4)
> error: Error building trees
>
> The "need merge" line is given by refresh_cache. We add the IN_PORCELAIN
> option to make the output more consistant with the other porcelain
> commands, and catch the error in return, to stop with a clean error
> message. The next lines were displayed by a call to cache_tree_update(),
> which is not reached anymore if we noticed the conflict.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> As usual, I try to have error messages point to the solution, not just
> the origin of the problem.

Seems to be a sensible approach to me.  

> Two questions:
>
> * Did anyone actually use the 3 "file: unmerged (sha1)" lines?

I don't think anybody does these days, and even if the question were about
the historical practice, I doubt anybody used "git merge-file" using these
blob object names to come up with a resolution.  Besides, if they really
want to, they can ask "ls-files -u" for that information themselves. 

> * Do you like my new message?

That would have been much easier to answer if you wrote "Here is an error
message produced with this patch" in the proposed commit log message.  I
haven't applied nor read your patch carefully, but I imagine you would say
something like this?

  With this patch, the error looks like this:

  $ git commit
  U foo.txt
  fatal: Commit is not possible because you have unmerged files.
  Please, resolve the conflicts listed above, and then mark them
  as resolved with 'git add <filename>', or use 'git commit -a'

Do I like it?  Hmmm.

 - "Please, ", especially with the comma, looks superfluous;

 - Some people consider "please resolve the conflicts" to mean the whole
   process of "fix them up in the work tree, and mark them resolved in the
   index", and others consider it to mean only the first step to "fix them
   up in the work tree".  I happen to be in the former camp, and to me
   ",and then mark them as..., add <filename>'," look superfluous because
   of that.

   I however think it is more helpful to new people who benefit from this
   advice message if we spell out these two steps.  Unfortunately, for
   that purpose, the description of the latter "mark them resolved in the
   index" step you have looks inadequate; e.g. sometimes it needs to be
   done with "git rm <filename>".

The need to give this advice on how to resolve conflicts is shared among
commands like 'git merge', 'git cherry-pick', and 'git status', to name a
few.  I think we should consolidate them all.

 - Decide if we go one-step (i.e. just say "resolve conflicts" and nothing
   else) or two-step (i.e. say "Fix them up in the work tree" and "mark
   them resolved in the index") approach; I am leaning toward the latter.

 - Decide the exact language used.  I think "Fix them up in the work tree"
   is passably okay, but "Mark them resolved in the index" needs to be
   made more concrete, and "git add <filename>" is not quite it, I am
   afraid.

 - Omit issuing the advisory message when advice.resolveConflict is set to
   false.

 - Perhaps have a common helper function to do this from the commands that
   need to give the advice.

Thanks.

Re: [RFC/PATCH] commit: make the error message on unmerged entries user-friendly.

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

>> Two questions:
>>
>> * Did anyone actually use the 3 "file: unmerged (sha1)" lines?
>
> I don't think anybody does these days, and even if the question were about
> the historical practice, I doubt anybody used "git merge-file" using these
> blob object names to come up with a resolution.  Besides, if they really
> want to, they can ask "ls-files -u" for that information themselves. 

OK, that's what I thought, but 'wanted to be sure.

>I imagine you would say
> something like this?
>
>   With this patch, the error looks like this:
>
>   $ git commit
>   U foo.txt
>   fatal: Commit is not possible because you have unmerged files.
>   Please, resolve the conflicts listed above, and then mark them
>   as resolved with 'git add <filename>', or use 'git commit -a'

Yes, sorry for not making it more explicit.

> Do I like it?  Hmmm.
>
>  - "Please, ", especially with the comma, looks superfluous;

I wouldn't call it mandatory, but it doesn't harm, and

$ grep -i -n -e '"Please' *.c
builtin-commit.c:246:		    "Please, resolve the conflicts listed above, and then mark them\n"
builtin-commit.c:698:			"Please supply the message using either -m or -F option.\n");
builtin-help.c:209:			"Please consider using 'man.<tool>.cmd' instead.",
builtin-help.c:221:			"Please consider using 'man.<tool>.path' instead.",
builtin-remote.c:1124:					"Please choose one explicitly with:");
builtin-tag.c:321:			"Please supply the message using either -m or -F option.\n");
merge-recursive.c:1191:			"Please, commit your changes or stash them before you can merge.";
setup.c:241:		warning("Please upgrade Git");

>  - Some people consider "please resolve the conflicts" to mean the whole
>    process of "fix them up in the work tree, and mark them resolved in the
>    index", and others consider it to mean only the first step to "fix them
>    up in the work tree".  I happen to be in the former camp, and to me
>    ",and then mark them as..., add <filename>'," look superfluous because
>    of that.
>
>    I however think it is more helpful to new people who benefit from this
>    advice message if we spell out these two steps.

Yes. Someone having "resolved" (i.e. removed markers), and trying to
commit should get a hint on the commands he can use to achieve this
goal.

>    Unfortunately, for that purpose, the description of the latter
>    "mark them resolved in the index" step you have looks inadequate;
>    e.g. sometimes it needs to be done with "git rm <filename>".

Maybe "sometimes", but to me, that's sufficiently rare to be omited
here (I don't think I ever used 'git rm' to resolve a conflict). The
user manual says this:

,----
| Each time you resolve the conflicts in a file and update the index:
| 
| $ git add file.txt
| 
| the different stages of that file will be "collapsed", after which git
| diff will (by default) no longer show diffs for that file.
`----

and I don't think it makes sense to try to be more exhaustive here
than in the user-manual.

> The need to give this advice on how to resolve conflicts is shared among
> commands like 'git merge', 'git cherry-pick', and 'git status', to name a
> few.

Not sure 'status' needs anything more. It already says

# Unmerged paths:
#   (use "git add/rm <file>..." as appropriate to mark resolution)
#
#       both modified:      foo.txt

and the big difference between 'git status' and the others is that
it's legitimate to run it while resolving conflicts, while calling
'git merge' or 'git commit' can be done only by mistake.

It's not serious to eat 3 or 4 lines of the screen to display a
message to tell the user he shouldn't have done this, but it'd be
disturbing to eat more than 1 line for a common case.

> I think we should consolidate them all.

Right, although "commit" is definitely the most important (dumb users
don't need "git merge").

>  - Decide if we go one-step (i.e. just say "resolve conflicts" and nothing
>    else) or two-step (i.e. say "Fix them up in the work tree" and "mark
>    them resolved in the index") approach; I am leaning toward the latter.

Meetoo.

>  - Decide the exact language used.  I think "Fix them up in the work tree"
>    is passably okay, but "Mark them resolved in the index" needs to be
>    made more concrete, and "git add <filename>" is not quite it, I am
>    afraid.

See above. To me, pointing to "git commit -a" and "git add" is
sufficient.

Pointing to "git commit -a" is also important to me, because Git
newbies may have been told to always use "git commit" with "-a"
(common use-case: "I have to use Git, I know SVN and I don't want to
learn anything new").

>  - Omit issuing the advisory message when advice.resolveConflict is set to
>    false.

Sensible, yes.

>  - Perhaps have a common helper function to do this from the commands that
>    need to give the advice.

Probably not, because the advice will be different:

git merge => please resolve and commit before you can merge
git commit => please resolve before you can commit
...

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH] commit: make the error message on unmerged entries user-friendly.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Maybe "sometimes", but to me, that's sufficiently rare to be omited
> here (I don't think I ever used 'git rm' to resolve a conflict). The
> user manual says this:
>
> ,----
> | Each time you resolve the conflicts in a file and update the index:
> | 
> | $ git add file.txt
> | 
> | the different stages of that file will be "collapsed", after which git
> | diff will (by default) no longer show diffs for that file.
> `----
>
> and I don't think it makes sense to try to be more exhaustive here
> than in the user-manual.

I tend to disagree.  The user-manual, as it currently stands, is an
introductory document that covers the concepts and the surface of git.
It's purpose is not to replace the main manual, but to give the necessary
foundation _before_ people read the manual.

>> The need to give this advice on how to resolve conflicts is shared among
>> commands like 'git merge', 'git cherry-pick', and 'git status', to name a
>> few.
>
> Not sure 'status' needs anything more. It already says
>
> # Unmerged paths:
> #   (use "git add/rm <file>..." as appropriate to mark resolution)
> #
> #       both modified:      foo.txt

This message was exactly what I was getting at.  The message in the
parentheses is what I called "advice".  As I was suggesting to share the
advice messages used by commands that deal with mergy situations (like the
one you added in your patch), looking at existing examples to find the
best one and using that in other places would be the most effective
approach.  That one line we see above is concise and does mention "rm" as
well.  Why not use it?

>> I think we should consolidate them all.
>
> Right, although "commit" is definitely the most important (dumb users
> don't need "git merge").

Your "dumb users" don't get the unmerged error from commit, either, if
they don't need "git merge".

Re: [RFC/PATCH] commit: make the error message on unmerged entries user-friendly.

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

>> # Unmerged paths:
>> #   (use "git add/rm <file>..." as appropriate to mark resolution)
[...]
>  That one line we see above is concise and does mention "rm" as
> well.  Why not use it?

Fine, you convinced me. I was thinking of a longer sentence, but "/rm"
doesn't harm.

>>> I think we should consolidate them all.
>>
>> Right, although "commit" is definitely the most important (dumb users
>> don't need "git merge").
>
> Your "dumb users" don't get the unmerged error from commit, either, if
> they don't need "git merge".

They'd use "pull", not merge. Anyway, I did it for commit, merge,
pull, revert, cherry-pick. I guess we covered the common cases.
The patch seems to have a lot of redundancies, but I think trying to
factor this into helper functions would be much more effort than the
few cut-and-paste that I had to do, since each instance is a slight
variant of each other ...

Patch follows. Let me know in case you prefer to split it.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH] commit: make the error message on unmerged entries user-friendly.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> They'd use "pull", not merge. Anyway, I did it for commit, merge,
> pull, revert, cherry-pick. I guess we covered the common cases.
> The patch seems to have a lot of redundancies, but I think trying to
> factor this into helper functions would be much more effort than the
> few cut-and-paste that I had to do, since each instance is a slight
> variant of each other ...

I'd be more worried about longer term maintainability than one time
expediency of producing your single patch to add these messages.  If the
messages are cast in stone, we can just verify they are consistent _now_
and forget about them, but I suspect not even you are perfect to predict
that we won't come up with different/better ways to resolve and mark them
resolved in the future and write a set of messages that won't have to
change.

Re: [RFC/PATCH] commit: make the error message on unmerged entries user-friendly.

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> They'd use "pull", not merge. Anyway, I did it for commit, merge,
>> pull, revert, cherry-pick. I guess we covered the common cases.
>> The patch seems to have a lot of redundancies, but I think trying to
>> factor this into helper functions would be much more effort than the
>> few cut-and-paste that I had to do, since each instance is a slight
>> variant of each other ...
>
> I'd be more worried about longer term maintainability than one time
> expediency of producing your single patch to add these messages.  If the
> messages are cast in stone, we can just verify they are consistent _now_
> and forget about them, but I suspect not even you are perfect to predict
> that we won't come up with different/better ways to resolve and mark them
> resolved in the future and write a set of messages that won't have to
> change.

Sorry, I sent the message a bit too early. Re-reading the patch, there
were actually pieces to factor. There's still duplication between C
and shell, and sentences which are actually reworded from a place to
another.

OTOH, maybe this reveals some differences that should be eliminated.
For example, 'git cherry-pick' has no problem with
$GIT_DIR/MERGE_HEAD, while 'git merge' will immediately if it exists.
Maybe we should write a helper like
ensure_everything_is_ok_for_a_merge_or_die() called by both
cherry-pick and merge?

This time, patch actually follows ;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH v2] Be more user-friendly when refusing to do something because of conflict.

[Matthieu Moy]

Various commands refuse to run in the presence of conflicts (commit,
merge, pull, cherry-pick/revert). They all used to provide rough, and
inconsistant error messages.

A new variable advice.resolveconflict is introduced, and allows more
verbose messages, pointing the user to the appropriate solution.

For commit, the error message used to look like this:

$ git commit
foo.txt: needs merge
foo.txt: unmerged (c34a92682e0394bc0d6f4d4a67a8e2d32395c169)
foo.txt: unmerged (3afcd75de8de0bb5076942fcb17446be50451030)
foo.txt: unmerged (c9785d77b76dfe4fb038bf927ee518f6ae45ede4)
error: Error building trees

The "need merge" line is given by refresh_cache. We add the IN_PORCELAIN
option to make the output more consistant with the other porcelain
commands, and catch the error in return, to stop with a clean error
message. The next lines were displayed by a call to cache_tree_update(),
which is not reached anymore if we noticed the conflict.

Pull is slightly modified to abort immediately if $GIT_DIR/MERGE_HEAD
exists instead of waiting for merge to complain.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/config.txt |    4 ++++
 advice.c                 |   12 ++++++++++++
 advice.h                 |    3 +++
 builtin-commit.c         |   15 +++++++++++++--
 builtin-merge.c          |   15 ++++++++++-----
 builtin-revert.c         |   15 ++++++++++++++-
 git-pull.sh              |   25 +++++++++++++++++++++++--
 7 files changed, 79 insertions(+), 10 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 23a965e..5078d26 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -130,6 +130,10 @@ advice.*::
 		Advice shown when linkgit:git-merge[1] refuses to
 		merge to avoid overwritting local changes.
 		Default: true.
+	resolveConflict::
+		Advices shown by various commands when conflicts
+		prevent the operation from being performed. 
+		Default: true.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index cb666ac..ec2bd82 100644
--- a/advice.c
+++ b/advice.c
@@ -3,6 +3,7 @@
 int advice_push_nonfastforward = 1;
 int advice_status_hints = 1;
 int advice_commit_before_merge = 1;
+int advice_resolve_conflict = 1;
 
 static struct {
 	const char *name;
@@ -11,6 +12,7 @@ static struct {
 	{ "pushnonfastforward", &advice_push_nonfastforward },
 	{ "statushints", &advice_status_hints },
 	{ "commitbeforemerge", &advice_commit_before_merge },
+	{ "resolveconflict", &advice_resolve_conflict },
 };
 
 int git_default_advice_config(const char *var, const char *value)
@@ -27,3 +29,13 @@ int git_default_advice_config(const char *var, const char *value)
 
 	return 0;
 }
+
+void NORETURN die_resolve_conflict(const char *me)
+{
+	if (advice_resolve_conflict)
+		die("'%s' is not possible because you have unmerged files.\n"
+		    "Please, fix them up in the work tree, and then use 'git add/rm <file>'\n"
+		    "as appropriate to mark resolution, or use 'git commit -a'.", me);
+	else
+		die("'%s' is not possible because you have unmerged files.", me);
+}
diff --git a/advice.h b/advice.h
index 3de5000..a6b4422 100644
--- a/advice.h
+++ b/advice.h
@@ -4,7 +4,10 @@
 extern int advice_push_nonfastforward;
 extern int advice_status_hints;
 extern int advice_commit_before_merge;
+extern int advice_resolve_conflict;
 
 int git_default_advice_config(const char *var, const char *value);
 
+extern void NORETURN die_resolve_conflict(const char *me);
+
 #endif /* ADVICE_H */
diff --git a/builtin-commit.c b/builtin-commit.c
index 3dfcd77..a4977ac 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -235,6 +235,17 @@ static void create_base_index(void)
 		exit(128); /* We've already reported the error, finish dying */
 }
 
+static void refresh_cache_or_die(int refresh_flags)
+{
+	/*
+	 * refresh_flags contains REFRESH_QUIET, so the only errors
+	 * are for unmerged entries.
+	*/
+	if(refresh_cache(refresh_flags | REFRESH_IN_PORCELAIN)) {
+		die_resolve_conflict("commit");
+	}
+}
+
 static char *prepare_index(int argc, const char **argv, const char *prefix, int is_status)
 {
 	int fd;
@@ -274,7 +285,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 	if (all || (also && pathspec && *pathspec)) {
 		int fd = hold_locked_index(&index_lock, 1);
 		add_files_to_cache(also ? prefix : NULL, pathspec, 0);
-		refresh_cache(refresh_flags);
+		refresh_cache_or_die(refresh_flags);
 		if (write_cache(fd, active_cache, active_nr) ||
 		    close_lock_file(&index_lock))
 			die("unable to write new_index file");
@@ -293,7 +304,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 	 */
 	if (!pathspec || !*pathspec) {
 		fd = hold_locked_index(&index_lock, 1);
-		refresh_cache(refresh_flags);
+		refresh_cache_or_die(refresh_flags);
 		if (write_cache(fd, active_cache, active_nr) ||
 		    commit_locked_index(&index_lock))
 			die("unable to write new_index file");
diff --git a/builtin-merge.c b/builtin-merge.c
index f1c84d7..abe6c03 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -847,11 +847,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	const char *best_strategy = NULL, *wt_strategy = NULL;
 	struct commit_list **remotes = &remoteheads;
 
-	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."
-				" (index unmerged)");
+	if (read_cache_unmerged()) {
+		die_resolve_conflict("merge");
+	}
+	if (file_exists(git_path("MERGE_HEAD"))) {
+		if (advice_resolve_conflict)
+			die("You have not concluded your merge (MERGE_HEAD exists).\n"
+			    "Please, commit your changes before you can merge.");
+		else
+			die("You have not concluded your merge (MERGE_HEAD exists).");
+	}
 
 	/*
 	 * Check if we are _not_ on a detached HEAD, i.e. if there is a
diff --git a/builtin-revert.c b/builtin-revert.c
index 151aa6a..d14dde3 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -233,6 +233,19 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
+static NORETURN void die_dirty_index(const char *me)
+{
+	if (read_cache_unmerged()) {
+		die_resolve_conflict(me);
+	} else {
+		if (advice_commit_before_merge)
+			die("Your local changes would be overwritten by %s.\n"
+			    "Please, commit your changes or stash them to proceed.", me);
+		else
+			die("Your local changes would be overwritten by %s.\n", me);
+	}
+}
+
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
 	unsigned char head[20];
@@ -269,7 +282,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		if (get_sha1("HEAD", head))
 			die ("You do not have a valid HEAD");
 		if (index_differs_from("HEAD", 0))
-			die ("Dirty index: cannot %s", me);
+			die_dirty_index(me);
 	}
 	discard_cache();
 
diff --git a/git-pull.sh b/git-pull.sh
index 9e69ada..54ce0af 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -13,8 +13,29 @@ set_reflog_action "pull $*"
 require_work_tree
 cd_to_toplevel
 
-test -z "$(git ls-files -u)" ||
-	die "You are in the middle of a conflicted merge."
+
+die_conflict () {
+    git diff-index --cached --name-status -r --ignore-submodules HEAD --
+    if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
+	die "Pull is not possible because you have unmerged files.
+Please, fix them up in the work tree, and then use 'git add/rm <file>'
+as appropriate to mark resolution, or use 'git commit -a'."
+    else
+	die "Pull is not possible because you have unmerged files."
+    fi
+}
+
+die_merge () {
+    if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
+	die "You have not concluded your merge (MERGE_HEAD exists).
+Please, commit your changes before you can merge."
+    else
+	die "You have not concluded your merge (MERGE_HEAD exists)."
+    fi
+}
+
+test -z "$(git ls-files -u)" || die_conflict
+test -f "$GIT_DIR/MERGE_HEAD" && die_merge
 
 strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
 log_arg= verbosity=
-- 
1.6.6.81.g35ec3e.dirty

[PATCH v3] Be more user-friendly when refusing to do something because of conflict.

[Matthieu Moy]

Various commands refuse to run in the presence of conflicts (commit,
merge, pull, cherry-pick/revert). They all used to provide rough, and
inconsistant error messages.

A new variable advice.resolveconflict is introduced, and allows more
verbose messages, pointing the user to the appropriate solution.

For commit, the error message used to look like this:

$ git commit
foo.txt: needs merge
foo.txt: unmerged (c34a92682e0394bc0d6f4d4a67a8e2d32395c169)
foo.txt: unmerged (3afcd75de8de0bb5076942fcb17446be50451030)
foo.txt: unmerged (c9785d77b76dfe4fb038bf927ee518f6ae45ede4)
error: Error building trees

The "need merge" line is given by refresh_cache. We add the IN_PORCELAIN
option to make the output more consistant with the other porcelain
commands, and catch the error in return, to stop with a clean error
message. The next lines were displayed by a call to cache_tree_update(),
which is not reached anymore if we noticed the conflict.

Pull is slightly modified to abort immediately if $GIT_DIR/MERGE_HEAD
exists instead of waiting for merge to complain.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Sorry, v2 missed a #include "git-compat-utils.h", it's not even compilable.

 Documentation/config.txt |    4 ++++
 advice.c                 |   12 ++++++++++++
 advice.h                 |    5 +++++
 builtin-commit.c         |   15 +++++++++++++--
 builtin-merge.c          |   15 ++++++++++-----
 builtin-revert.c         |   15 ++++++++++++++-
 git-pull.sh              |   25 +++++++++++++++++++++++--
 7 files changed, 81 insertions(+), 10 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 23a965e..5078d26 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -130,6 +130,10 @@ advice.*::
 		Advice shown when linkgit:git-merge[1] refuses to
 		merge to avoid overwritting local changes.
 		Default: true.
+	resolveConflict::
+		Advices shown by various commands when conflicts
+		prevent the operation from being performed. 
+		Default: true.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index cb666ac..ec2bd82 100644
--- a/advice.c
+++ b/advice.c
@@ -3,6 +3,7 @@
 int advice_push_nonfastforward = 1;
 int advice_status_hints = 1;
 int advice_commit_before_merge = 1;
+int advice_resolve_conflict = 1;
 
 static struct {
 	const char *name;
@@ -11,6 +12,7 @@ static struct {
 	{ "pushnonfastforward", &advice_push_nonfastforward },
 	{ "statushints", &advice_status_hints },
 	{ "commitbeforemerge", &advice_commit_before_merge },
+	{ "resolveconflict", &advice_resolve_conflict },
 };
 
 int git_default_advice_config(const char *var, const char *value)
@@ -27,3 +29,13 @@ int git_default_advice_config(const char *var, const char *value)
 
 	return 0;
 }
+
+void NORETURN die_resolve_conflict(const char *me)
+{
+	if (advice_resolve_conflict)
+		die("'%s' is not possible because you have unmerged files.\n"
+		    "Please, fix them up in the work tree, and then use 'git add/rm <file>'\n"
+		    "as appropriate to mark resolution, or use 'git commit -a'.", me);
+	else
+		die("'%s' is not possible because you have unmerged files.", me);
+}
diff --git a/advice.h b/advice.h
index 3de5000..acd5fdd 100644
--- a/advice.h
+++ b/advice.h
@@ -1,10 +1,15 @@
 #ifndef ADVICE_H
 #define ADVICE_H
 
+#include "git-compat-util.h"
+
 extern int advice_push_nonfastforward;
 extern int advice_status_hints;
 extern int advice_commit_before_merge;
+extern int advice_resolve_conflict;
 
 int git_default_advice_config(const char *var, const char *value);
 
+extern void NORETURN die_resolve_conflict(const char *me);
+
 #endif /* ADVICE_H */
diff --git a/builtin-commit.c b/builtin-commit.c
index 3dfcd77..a4977ac 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -235,6 +235,17 @@ static void create_base_index(void)
 		exit(128); /* We've already reported the error, finish dying */
 }
 
+static void refresh_cache_or_die(int refresh_flags)
+{
+	/*
+	 * refresh_flags contains REFRESH_QUIET, so the only errors
+	 * are for unmerged entries.
+	*/
+	if(refresh_cache(refresh_flags | REFRESH_IN_PORCELAIN)) {
+		die_resolve_conflict("commit");
+	}
+}
+
 static char *prepare_index(int argc, const char **argv, const char *prefix, int is_status)
 {
 	int fd;
@@ -274,7 +285,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 	if (all || (also && pathspec && *pathspec)) {
 		int fd = hold_locked_index(&index_lock, 1);
 		add_files_to_cache(also ? prefix : NULL, pathspec, 0);
-		refresh_cache(refresh_flags);
+		refresh_cache_or_die(refresh_flags);
 		if (write_cache(fd, active_cache, active_nr) ||
 		    close_lock_file(&index_lock))
 			die("unable to write new_index file");
@@ -293,7 +304,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 	 */
 	if (!pathspec || !*pathspec) {
 		fd = hold_locked_index(&index_lock, 1);
-		refresh_cache(refresh_flags);
+		refresh_cache_or_die(refresh_flags);
 		if (write_cache(fd, active_cache, active_nr) ||
 		    commit_locked_index(&index_lock))
 			die("unable to write new_index file");
diff --git a/builtin-merge.c b/builtin-merge.c
index f1c84d7..abe6c03 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -847,11 +847,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	const char *best_strategy = NULL, *wt_strategy = NULL;
 	struct commit_list **remotes = &remoteheads;
 
-	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."
-				" (index unmerged)");
+	if (read_cache_unmerged()) {
+		die_resolve_conflict("merge");
+	}
+	if (file_exists(git_path("MERGE_HEAD"))) {
+		if (advice_resolve_conflict)
+			die("You have not concluded your merge (MERGE_HEAD exists).\n"
+			    "Please, commit your changes before you can merge.");
+		else
+			die("You have not concluded your merge (MERGE_HEAD exists).");
+	}
 
 	/*
 	 * Check if we are _not_ on a detached HEAD, i.e. if there is a
diff --git a/builtin-revert.c b/builtin-revert.c
index 151aa6a..d14dde3 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -233,6 +233,19 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
+static NORETURN void die_dirty_index(const char *me)
+{
+	if (read_cache_unmerged()) {
+		die_resolve_conflict(me);
+	} else {
+		if (advice_commit_before_merge)
+			die("Your local changes would be overwritten by %s.\n"
+			    "Please, commit your changes or stash them to proceed.", me);
+		else
+			die("Your local changes would be overwritten by %s.\n", me);
+	}
+}
+
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
 	unsigned char head[20];
@@ -269,7 +282,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		if (get_sha1("HEAD", head))
 			die ("You do not have a valid HEAD");
 		if (index_differs_from("HEAD", 0))
-			die ("Dirty index: cannot %s", me);
+			die_dirty_index(me);
 	}
 	discard_cache();
 
diff --git a/git-pull.sh b/git-pull.sh
index 9e69ada..54ce0af 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -13,8 +13,29 @@ set_reflog_action "pull $*"
 require_work_tree
 cd_to_toplevel
 
-test -z "$(git ls-files -u)" ||
-	die "You are in the middle of a conflicted merge."
+
+die_conflict () {
+    git diff-index --cached --name-status -r --ignore-submodules HEAD --
+    if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
+	die "Pull is not possible because you have unmerged files.
+Please, fix them up in the work tree, and then use 'git add/rm <file>'
+as appropriate to mark resolution, or use 'git commit -a'."
+    else
+	die "Pull is not possible because you have unmerged files."
+    fi
+}
+
+die_merge () {
+    if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
+	die "You have not concluded your merge (MERGE_HEAD exists).
+Please, commit your changes before you can merge."
+    else
+	die "You have not concluded your merge (MERGE_HEAD exists)."
+    fi
+}
+
+test -z "$(git ls-files -u)" || die_conflict
+test -f "$GIT_DIR/MERGE_HEAD" && die_merge
 
 strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
 log_arg= verbosity=
-- 
1.6.6.81.g2955.dirty

Re: [PATCH v2] Be more user-friendly when refusing to do something because of conflict.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> Various commands refuse to run in the presence of conflicts (commit,
> merge, pull, cherry-pick/revert). They all used to provide rough, and
> inconsistant error messages.
>
> A new variable advice.resolveconflict is introduced, and allows more
> verbose messages, pointing the user to the appropriate solution.
>
> For commit, the error message used to look like this:
>
> $ git commit
> foo.txt: needs merge
> foo.txt: unmerged (c34a92682e0394bc0d6f4d4a67a8e2d32395c169)
> foo.txt: unmerged (3afcd75de8de0bb5076942fcb17446be50451030)
> foo.txt: unmerged (c9785d77b76dfe4fb038bf927ee518f6ae45ede4)
> error: Error building trees
>
> The "need merge" line is given by refresh_cache. We add the IN_PORCELAIN
> option to make the output more consistant with the other porcelain
> commands, and catch the error in return, to stop with a clean error
> message. The next lines were displayed by a call to cache_tree_update(),
> which is not reached anymore if we noticed the conflict.

"The new output looks like this, which is much better..." is missing
here.

> @@ -27,3 +29,13 @@ int git_default_advice_config(const char *var, const char *value)
>  
>  	return 0;
>  }
> +
> +void NORETURN die_resolve_conflict(const char *me)
> +{
> +	if (advice_resolve_conflict)
> +		die("'%s' is not possible because you have unmerged files.\n"
> +		    "Please, fix them up in the work tree, and then use 'git add/rm <file>'\n"
> +		    "as appropriate to mark resolution, or use 'git commit -a'.", me);
> +	else
> +		die("'%s' is not possible because you have unmerged files.", me);
> +}

Nice, but the advice contrasts between squirrels and oranges.

One advice, "use add/rm as appropriate to mark resolution" goes only as
far as making the index in order, without recording it in a commit.  The
other, "commit -a", will make a commit, I suspect that "commit -a" needs
to be matched with "commit" if the user chooses to take the first advice
of resolving paths incrementally.  IOW

    use 'git add/rm <file>' as appropriate to mark resolution and make a
    commit, or use 'commit -a', before running me again.

might be more appropriate.

> diff --git a/builtin-commit.c b/builtin-commit.c
> index 3dfcd77..a4977ac 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -235,6 +235,17 @@ static void create_base_index(void)
>  		exit(128); /* We've already reported the error, finish dying */
>  }
>  
> +static void refresh_cache_or_die(int refresh_flags)
> +{
> +	/*
> +	 * refresh_flags contains REFRESH_QUIET, so the only errors
> +	 * are for unmerged entries.
> +	*/

Mixed indentation.

> +	if(refresh_cache(refresh_flags | REFRESH_IN_PORCELAIN)) {

SP after "if".

What should we see upon "commit --dry-run" and what does the code after
the patch produce?  Are we sure refresh_flags always lack REFRESH_UNMERGED
that allows unmerged entries and produces the unmerged error messages when
needed?

> diff --git a/builtin-merge.c b/builtin-merge.c
> index f1c84d7..abe6c03 100644
> --- a/builtin-merge.c
> +++ b/builtin-merge.c
> @@ -847,11 +847,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  	const char *best_strategy = NULL, *wt_strategy = NULL;
>  	struct commit_list **remotes = &remoteheads;
>  
> -	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."
> -				" (index unmerged)");
> +	if (read_cache_unmerged()) {
> +		die_resolve_conflict("merge");
> +	}
> +	if (file_exists(git_path("MERGE_HEAD"))) {
> +		if (advice_resolve_conflict)
> +			die("You have not concluded your merge (MERGE_HEAD exists).\n"
> +			    "Please, commit your changes before you can merge.");
> +		else
> +			die("You have not concluded your merge (MERGE_HEAD exists).");
> +	}

It is not a very big deal, but why are these checked in different order
after the patch?  Unless the new order is justifiably better, I'd rather
see the order kept as before.  Otherwise please justify it in the proposed
commit log message.

Note that the user might have already run "add -u" to mark everything
resolved, in which case MERGE_HEAD will still exist even though the index
is free of ummerged entries.

> diff --git a/builtin-revert.c b/builtin-revert.c

Nice.

> diff --git a/git-pull.sh b/git-pull.sh
> index 9e69ada..54ce0af 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -13,8 +13,29 @@ set_reflog_action "pull $*"
>  require_work_tree
>  cd_to_toplevel
>  
> -test -z "$(git ls-files -u)" ||
> -	die "You are in the middle of a conflicted merge."
> +
> +die_conflict () {
> +    git diff-index --cached --name-status -r --ignore-submodules HEAD --
> +    if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
> +	die "Pull is not possible because you have unmerged files.
> +Please, fix them up in the work tree, and then use 'git add/rm <file>'
> +as appropriate to mark resolution, or use 'git commit -a'."
> +    else
> +	die "Pull is not possible because you have unmerged files."
> +    fi
> +}
> +
> +die_merge () {
> +    if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
> +	die "You have not concluded your merge (MERGE_HEAD exists).
> +Please, commit your changes before you can merge."
> +    else
> +	die "You have not concluded your merge (MERGE_HEAD exists)."
> +    fi
> +}
> +
> +test -z "$(git ls-files -u)" || die_conflict
> +test -f "$GIT_DIR/MERGE_HEAD" && die_merge

Nice.  Maybe we want to handle other cases like:

	$ git rebase master
        ... conflicted
        ... called away for a 30-minutes meeting
        ... forgot the user was in the middle of the rebase
        $ git pull

and the "pull" refused to run because the earlier "rebase" hasn't been
concluded (I suspect an earlier "am" failure would be the same issue).

Thanks.

Re: [PATCH v2] Be more user-friendly when refusing to do something because of conflict.

[Matthieu Moy]

Thanks for your detailed feedback.

Junio C Hamano <gitster@pobox.com> writes:

> "The new output looks like this, which is much better..." is missing
> here.

Added.

> One advice, "use add/rm as appropriate to mark resolution" goes only as
> far as making the index in order, without recording it in a commit.  The
> other, "commit -a", will make a commit, I suspect that "commit -a" needs
> to be matched with "commit" if the user chooses to take the first advice
> of resolving paths incrementally.  IOW
>
>     use 'git add/rm <file>' as appropriate to mark resolution and make a
>     commit, or use 'commit -a', before running me again.
>
> might be more appropriate.

Applied.

The same sentence has a slightly different meaning for commit and
merge. For commit "make a commit" means "redo what you tried to do",
and for merge, it means "make a commit before you redo what you tried
to do".

>> +static void refresh_cache_or_die(int refresh_flags)
>> +{
>> +	/*
>> +	 * refresh_flags contains REFRESH_QUIET, so the only errors
>> +	 * are for unmerged entries.
>> +	*/
>
> Mixed indentation.

fixed.

>> +	if(refresh_cache(refresh_flags | REFRESH_IN_PORCELAIN)) {
>
> SP after "if".

fixed (together with the useless brace which you had missed ;-) ).

> What should we see upon "commit --dry-run" and what does the code after
> the patch produce?

Good remark. Just tried, it works without option, with -a, -o, -i.
They all give the usual status output, except -o which complains:

$ git commit --dry-run -o foo.txt
fatal: cannot do a partial commit during a merge.

(as before)

> Are we sure refresh_flags always lack REFRESH_UNMERGED that allows
> unmerged entries and produces the unmerged error messages when
> needed?

Argument from intimidation:
If they hadn't, we would have noticed the output (foo.txt: needs
merge) before the patch.

Rational argument:
prepare_index starts with this:

	if (is_status)
		refresh_flags |= REFRESH_UNMERGED;

and each call to refresh_cache_or_die is within prepare_index, with
these flags, untouched.

>> -	if (file_exists(git_path("MERGE_HEAD"))) [...]
>> -	if (read_cache_unmerged()) [...]
>> +	if (read_cache_unmerged()) {
>> +		die_resolve_conflict("merge");
>> +	}
>> +	if (file_exists(git_path("MERGE_HEAD"))) {
>> +		if (advice_resolve_conflict)
>> +			die("You have not concluded your merge (MERGE_HEAD exists).\n"
>> +			    "Please, commit your changes before you can merge.");
>> +		else
>> +			die("You have not concluded your merge (MERGE_HEAD exists).");
>> +	}
>
> It is not a very big deal, but why are these checked in different order
> after the patch?
[...]
> Note that the user might have already run "add -u" to mark everything
> resolved, in which case MERGE_HEAD will still exist even though the index
> is free of ummerged entries.

That's precisely the reason. You hardly have conflicts without a
MERGE_HEAD, but it's sensible to have a MERGE_HEAD without unmerged
entries in the index, which means you're one step closer to the
commit.

I've added a comment (not a commit message) to make it clear.

> Nice.  Maybe we want to handle other cases like:
>
> 	$ git rebase master
>         ... conflicted

(if it's conflicted, it's OK, same user-friendly message. If you
already git add-ed your conflicts, git pull will run normally :-( ).

>         ... called away for a 30-minutes meeting
>         ... forgot the user was in the middle of the rebase
>         $ git pull
>
> and the "pull" refused to run because the earlier "rebase" hasn't been
> concluded (I suspect an earlier "am" failure would be the same issue).

Yes, but I think that's a separate topic (and I'm getting short in
time budget). This will also apply to many commands (pull,
merge, ...), including shell and C. It would probably be good to have
a C function like can_I_start_merge() exposed as a plumbing command to
be used by git-pull.sh.

And I'm wondering whether the ability to run pull in the middle of a
rebase is a bug or a feature. Never did that, but I can imagine
someone doing a 'rebase -i' with an 'edit' command to let rebase stop,
do a merge, and then 'rebase --continue', to insert a merge commit in
the middle of a patch serie.

Patch follows. Fyi the difference with v3 are:

diff --git a/advice.c b/advice.c
index ec2bd82..3309521 100644
--- a/advice.c
+++ b/advice.c
@@ -33,9 +33,13 @@ int git_default_advice_config(const char *var, const char *value)
 void NORETURN die_resolve_conflict(const char *me)
 {
        if (advice_resolve_conflict)
+               /*
+                * Message used both when 'git commit' fails and when
+                * other commands doing a merge do.
+                */
                die("'%s' is not possible because you have unmerged files.\n"
-                   "Please, fix them up in the work tree, and then use 'git add/rm <file>'\n"
-                   "as appropriate to mark resolution, or use 'git commit -a'.", me);
+                   "Please, fix them up in the work tree, and then use 'git add/rm <file>' as\n"
+                   "appropriate to mark resolution and make a commit, or use 'git commit -a'.", me);
        else
                die("'%s' is not possible because you have unmerged files.", me);
 }
diff --git a/builtin-commit.c b/builtin-commit.c
index a4977ac..b3b37c2 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -240,8 +240,8 @@ static void refresh_cache_or_die(int refresh_flags)
        /*
         * refresh_flags contains REFRESH_QUIET, so the only errors
         * are for unmerged entries.
-       */
-       if(refresh_cache(refresh_flags | REFRESH_IN_PORCELAIN)) {
+        */
+       if (refresh_cache(refresh_flags | REFRESH_IN_PORCELAIN)) {
                die_resolve_conflict("commit");
        }
 }
diff --git a/builtin-merge.c b/builtin-merge.c
index abe6c03..79a35c3 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -851,6 +851,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
                die_resolve_conflict("merge");
        }
        if (file_exists(git_path("MERGE_HEAD"))) {
+               /*
+                * There is no unmerged entry, don't advise 'git
+                * add/rm <file>', just 'git commit'.
+                */
                if (advice_resolve_conflict)
                        die("You have not concluded your merge (MERGE_HEAD exists).\n"
                            "Please, commit your changes before you can merge.");

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH v4] Be more user-friendly when refusing to do something because of conflict.

[Matthieu Moy]

Various commands refuse to run in the presence of conflicts (commit,
merge, pull, cherry-pick/revert). They all used to provide rough, and
inconsistant error messages.

A new variable advice.resolveconflict is introduced, and allows more
verbose messages, pointing the user to the appropriate solution.

For commit, the error message used to look like this:

$ git commit
foo.txt: needs merge
foo.txt: unmerged (c34a92682e0394bc0d6f4d4a67a8e2d32395c169)
foo.txt: unmerged (3afcd75de8de0bb5076942fcb17446be50451030)
foo.txt: unmerged (c9785d77b76dfe4fb038bf927ee518f6ae45ede4)
error: Error building trees

The "need merge" line is given by refresh_cache. We add the IN_PORCELAIN
option to make the output more consistant with the other porcelain
commands, and catch the error in return, to stop with a clean error
message. The next lines were displayed by a call to cache_tree_update(),
which is not reached anymore if we noticed the conflict.

The new output looks like:

U       foo.txt
fatal: 'commit' is not possible because you have unmerged files.
Please, fix them up in the work tree, and then use 'git add/rm <file>' as
appropriate to mark resolution and make a commit, or use 'git commit -a'.

Pull is slightly modified to abort immediately if $GIT_DIR/MERGE_HEAD
exists instead of waiting for merge to complain.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/config.txt |    4 ++++
 advice.c                 |   16 ++++++++++++++++
 advice.h                 |    5 +++++
 builtin-commit.c         |   14 ++++++++++++--
 builtin-merge.c          |   19 ++++++++++++++-----
 builtin-revert.c         |   15 ++++++++++++++-
 git-pull.sh              |   25 +++++++++++++++++++++++--
 7 files changed, 88 insertions(+), 10 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 23a965e..5078d26 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -130,6 +130,10 @@ advice.*::
 		Advice shown when linkgit:git-merge[1] refuses to
 		merge to avoid overwritting local changes.
 		Default: true.
+	resolveConflict::
+		Advices shown by various commands when conflicts
+		prevent the operation from being performed. 
+		Default: true.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index cb666ac..3309521 100644
--- a/advice.c
+++ b/advice.c
@@ -3,6 +3,7 @@
 int advice_push_nonfastforward = 1;
 int advice_status_hints = 1;
 int advice_commit_before_merge = 1;
+int advice_resolve_conflict = 1;
 
 static struct {
 	const char *name;
@@ -11,6 +12,7 @@ static struct {
 	{ "pushnonfastforward", &advice_push_nonfastforward },
 	{ "statushints", &advice_status_hints },
 	{ "commitbeforemerge", &advice_commit_before_merge },
+	{ "resolveconflict", &advice_resolve_conflict },
 };
 
 int git_default_advice_config(const char *var, const char *value)
@@ -27,3 +29,17 @@ int git_default_advice_config(const char *var, const char *value)
 
 	return 0;
 }
+
+void NORETURN die_resolve_conflict(const char *me)
+{
+	if (advice_resolve_conflict)
+		/*
+		 * Message used both when 'git commit' fails and when
+		 * other commands doing a merge do.
+		 */
+		die("'%s' is not possible because you have unmerged files.\n"
+		    "Please, fix them up in the work tree, and then use 'git add/rm <file>' as\n"
+		    "appropriate to mark resolution and make a commit, or use 'git commit -a'.", me);
+	else
+		die("'%s' is not possible because you have unmerged files.", me);
+}
diff --git a/advice.h b/advice.h
index 3de5000..acd5fdd 100644
--- a/advice.h
+++ b/advice.h
@@ -1,10 +1,15 @@
 #ifndef ADVICE_H
 #define ADVICE_H
 
+#include "git-compat-util.h"
+
 extern int advice_push_nonfastforward;
 extern int advice_status_hints;
 extern int advice_commit_before_merge;
+extern int advice_resolve_conflict;
 
 int git_default_advice_config(const char *var, const char *value);
 
+extern void NORETURN die_resolve_conflict(const char *me);
+
 #endif /* ADVICE_H */
diff --git a/builtin-commit.c b/builtin-commit.c
index 3dfcd77..6075a59 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -235,6 +235,16 @@ static void create_base_index(void)
 		exit(128); /* We've already reported the error, finish dying */
 }
 
+static void refresh_cache_or_die(int refresh_flags)
+{
+	/*
+	 * refresh_flags contains REFRESH_QUIET, so the only errors
+	 * are for unmerged entries.
+	 */
+	if (refresh_cache(refresh_flags | REFRESH_IN_PORCELAIN))
+		die_resolve_conflict("commit");
+}
+
 static char *prepare_index(int argc, const char **argv, const char *prefix, int is_status)
 {
 	int fd;
@@ -274,7 +284,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 	if (all || (also && pathspec && *pathspec)) {
 		int fd = hold_locked_index(&index_lock, 1);
 		add_files_to_cache(also ? prefix : NULL, pathspec, 0);
-		refresh_cache(refresh_flags);
+		refresh_cache_or_die(refresh_flags);
 		if (write_cache(fd, active_cache, active_nr) ||
 		    close_lock_file(&index_lock))
 			die("unable to write new_index file");
@@ -293,7 +303,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 	 */
 	if (!pathspec || !*pathspec) {
 		fd = hold_locked_index(&index_lock, 1);
-		refresh_cache(refresh_flags);
+		refresh_cache_or_die(refresh_flags);
 		if (write_cache(fd, active_cache, active_nr) ||
 		    commit_locked_index(&index_lock))
 			die("unable to write new_index file");
diff --git a/builtin-merge.c b/builtin-merge.c
index f1c84d7..79a35c3 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -847,11 +847,20 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	const char *best_strategy = NULL, *wt_strategy = NULL;
 	struct commit_list **remotes = &remoteheads;
 
-	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."
-				" (index unmerged)");
+	if (read_cache_unmerged()) {
+		die_resolve_conflict("merge");
+	}
+	if (file_exists(git_path("MERGE_HEAD"))) {
+		/*
+		 * There is no unmerged entry, don't advise 'git
+		 * add/rm <file>', just 'git commit'.
+		 */
+		if (advice_resolve_conflict)
+			die("You have not concluded your merge (MERGE_HEAD exists).\n"
+			    "Please, commit your changes before you can merge.");
+		else
+			die("You have not concluded your merge (MERGE_HEAD exists).");
+	}
 
 	/*
 	 * Check if we are _not_ on a detached HEAD, i.e. if there is a
diff --git a/builtin-revert.c b/builtin-revert.c
index 151aa6a..d14dde3 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -233,6 +233,19 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
+static NORETURN void die_dirty_index(const char *me)
+{
+	if (read_cache_unmerged()) {
+		die_resolve_conflict(me);
+	} else {
+		if (advice_commit_before_merge)
+			die("Your local changes would be overwritten by %s.\n"
+			    "Please, commit your changes or stash them to proceed.", me);
+		else
+			die("Your local changes would be overwritten by %s.\n", me);
+	}
+}
+
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
 	unsigned char head[20];
@@ -269,7 +282,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		if (get_sha1("HEAD", head))
 			die ("You do not have a valid HEAD");
 		if (index_differs_from("HEAD", 0))
-			die ("Dirty index: cannot %s", me);
+			die_dirty_index(me);
 	}
 	discard_cache();
 
diff --git a/git-pull.sh b/git-pull.sh
index 9e69ada..54ce0af 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -13,8 +13,29 @@ set_reflog_action "pull $*"
 require_work_tree
 cd_to_toplevel
 
-test -z "$(git ls-files -u)" ||
-	die "You are in the middle of a conflicted merge."
+
+die_conflict () {
+    git diff-index --cached --name-status -r --ignore-submodules HEAD --
+    if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
+	die "Pull is not possible because you have unmerged files.
+Please, fix them up in the work tree, and then use 'git add/rm <file>'
+as appropriate to mark resolution, or use 'git commit -a'."
+    else
+	die "Pull is not possible because you have unmerged files."
+    fi
+}
+
+die_merge () {
+    if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
+	die "You have not concluded your merge (MERGE_HEAD exists).
+Please, commit your changes before you can merge."
+    else
+	die "You have not concluded your merge (MERGE_HEAD exists)."
+    fi
+}
+
+test -z "$(git ls-files -u)" || die_conflict
+test -f "$GIT_DIR/MERGE_HEAD" && die_merge
 
 strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
 log_arg= verbosity=
-- 
1.6.6.81.g872e.dirty

[PATCH v5] Be more user-friendly when refusing to do something because of conflict.

[Matthieu Moy]

Various commands refuse to run in the presence of conflicts (commit,
merge, pull, cherry-pick/revert). They all used to provide rough, and
inconsistant error messages.

A new variable advice.resolveconflict is introduced, and allows more
verbose messages, pointing the user to the appropriate solution.

For commit, the error message used to look like this:

$ git commit
foo.txt: needs merge
foo.txt: unmerged (c34a92682e0394bc0d6f4d4a67a8e2d32395c169)
foo.txt: unmerged (3afcd75de8de0bb5076942fcb17446be50451030)
foo.txt: unmerged (c9785d77b76dfe4fb038bf927ee518f6ae45ede4)
error: Error building trees

The "need merge" line is given by refresh_cache. We add the IN_PORCELAIN
option to make the output more consistant with the other porcelain
commands, and catch the error in return, to stop with a clean error
message. The next lines were displayed by a call to cache_tree_update(),
which is not reached anymore if we noticed the conflict.

The new output looks like:

U       foo.txt
fatal: 'commit' is not possible because you have unmerged files.
Please, fix them up in the work tree, and then use 'git add/rm <file>' as
appropriate to mark resolution and make a commit, or use 'git commit -a'.

Pull is slightly modified to abort immediately if $GIT_DIR/MERGE_HEAD
exists instead of waiting for merge to complain.

The behavior of merge and the test-case are slightly modified to reflect
the usual flow: start with conflicts, fix them, and afterwards get rid of
MERGE_HEAD, with different error messages at each stage.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Ahem. *This* one passes the test-suite :-\. Sorry for not having ran it earlier.

Since the test for git-merge is changed a bit, I added the last
paragraph of the commit message.

 Documentation/config.txt      |    4 ++++
 advice.c                      |   16 ++++++++++++++++
 advice.h                      |    5 +++++
 builtin-commit.c              |   14 ++++++++++++--
 builtin-merge.c               |   19 ++++++++++++++-----
 builtin-revert.c              |   15 ++++++++++++++-
 git-pull.sh                   |   25 +++++++++++++++++++++++--
 t/t3030-merge-recursive.sh    |    6 ++++--
 t/t3501-revert-cherry-pick.sh |    2 +-
 9 files changed, 93 insertions(+), 13 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 304eabb..8761411 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -130,6 +130,10 @@ advice.*::
 		Advice shown when linkgit:git-merge[1] refuses to
 		merge to avoid overwritting local changes.
 		Default: true.
+	resolveConflict::
+		Advices shown by various commands when conflicts
+		prevent the operation from being performed. 
+		Default: true.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index cb666ac..3309521 100644
--- a/advice.c
+++ b/advice.c
@@ -3,6 +3,7 @@
 int advice_push_nonfastforward = 1;
 int advice_status_hints = 1;
 int advice_commit_before_merge = 1;
+int advice_resolve_conflict = 1;
 
 static struct {
 	const char *name;
@@ -11,6 +12,7 @@ static struct {
 	{ "pushnonfastforward", &advice_push_nonfastforward },
 	{ "statushints", &advice_status_hints },
 	{ "commitbeforemerge", &advice_commit_before_merge },
+	{ "resolveconflict", &advice_resolve_conflict },
 };
 
 int git_default_advice_config(const char *var, const char *value)
@@ -27,3 +29,17 @@ int git_default_advice_config(const char *var, const char *value)
 
 	return 0;
 }
+
+void NORETURN die_resolve_conflict(const char *me)
+{
+	if (advice_resolve_conflict)
+		/*
+		 * Message used both when 'git commit' fails and when
+		 * other commands doing a merge do.
+		 */
+		die("'%s' is not possible because you have unmerged files.\n"
+		    "Please, fix them up in the work tree, and then use 'git add/rm <file>' as\n"
+		    "appropriate to mark resolution and make a commit, or use 'git commit -a'.", me);
+	else
+		die("'%s' is not possible because you have unmerged files.", me);
+}
diff --git a/advice.h b/advice.h
index 3de5000..acd5fdd 100644
--- a/advice.h
+++ b/advice.h
@@ -1,10 +1,15 @@
 #ifndef ADVICE_H
 #define ADVICE_H
 
+#include "git-compat-util.h"
+
 extern int advice_push_nonfastforward;
 extern int advice_status_hints;
 extern int advice_commit_before_merge;
+extern int advice_resolve_conflict;
 
 int git_default_advice_config(const char *var, const char *value);
 
+extern void NORETURN die_resolve_conflict(const char *me);
+
 #endif /* ADVICE_H */
diff --git a/builtin-commit.c b/builtin-commit.c
index 592b103..c56dca0 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -244,6 +244,16 @@ static void create_base_index(void)
 		exit(128); /* We've already reported the error, finish dying */
 }
 
+static void refresh_cache_or_die(int refresh_flags)
+{
+	/*
+	 * refresh_flags contains REFRESH_QUIET, so the only errors
+	 * are for unmerged entries.
+	 */
+	if (refresh_cache(refresh_flags | REFRESH_IN_PORCELAIN))
+		die_resolve_conflict("commit");
+}
+
 static char *prepare_index(int argc, const char **argv, const char *prefix, int is_status)
 {
 	int fd;
@@ -283,7 +293,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 	if (all || (also && pathspec && *pathspec)) {
 		int fd = hold_locked_index(&index_lock, 1);
 		add_files_to_cache(also ? prefix : NULL, pathspec, 0);
-		refresh_cache(refresh_flags);
+		refresh_cache_or_die(refresh_flags);
 		if (write_cache(fd, active_cache, active_nr) ||
 		    close_lock_file(&index_lock))
 			die("unable to write new_index file");
@@ -302,7 +312,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 	 */
 	if (!pathspec || !*pathspec) {
 		fd = hold_locked_index(&index_lock, 1);
-		refresh_cache(refresh_flags);
+		refresh_cache_or_die(refresh_flags);
 		if (write_cache(fd, active_cache, active_nr) ||
 		    commit_locked_index(&index_lock))
 			die("unable to write new_index file");
diff --git a/builtin-merge.c b/builtin-merge.c
index f1c84d7..79a35c3 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -847,11 +847,20 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	const char *best_strategy = NULL, *wt_strategy = NULL;
 	struct commit_list **remotes = &remoteheads;
 
-	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."
-				" (index unmerged)");
+	if (read_cache_unmerged()) {
+		die_resolve_conflict("merge");
+	}
+	if (file_exists(git_path("MERGE_HEAD"))) {
+		/*
+		 * There is no unmerged entry, don't advise 'git
+		 * add/rm <file>', just 'git commit'.
+		 */
+		if (advice_resolve_conflict)
+			die("You have not concluded your merge (MERGE_HEAD exists).\n"
+			    "Please, commit your changes before you can merge.");
+		else
+			die("You have not concluded your merge (MERGE_HEAD exists).");
+	}
 
 	/*
 	 * Check if we are _not_ on a detached HEAD, i.e. if there is a
diff --git a/builtin-revert.c b/builtin-revert.c
index 151aa6a..d14dde3 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -233,6 +233,19 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
+static NORETURN void die_dirty_index(const char *me)
+{
+	if (read_cache_unmerged()) {
+		die_resolve_conflict(me);
+	} else {
+		if (advice_commit_before_merge)
+			die("Your local changes would be overwritten by %s.\n"
+			    "Please, commit your changes or stash them to proceed.", me);
+		else
+			die("Your local changes would be overwritten by %s.\n", me);
+	}
+}
+
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
 	unsigned char head[20];
@@ -269,7 +282,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		if (get_sha1("HEAD", head))
 			die ("You do not have a valid HEAD");
 		if (index_differs_from("HEAD", 0))
-			die ("Dirty index: cannot %s", me);
+			die_dirty_index(me);
 	}
 	discard_cache();
 
diff --git a/git-pull.sh b/git-pull.sh
index 9e69ada..54ce0af 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -13,8 +13,29 @@ set_reflog_action "pull $*"
 require_work_tree
 cd_to_toplevel
 
-test -z "$(git ls-files -u)" ||
-	die "You are in the middle of a conflicted merge."
+
+die_conflict () {
+    git diff-index --cached --name-status -r --ignore-submodules HEAD --
+    if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
+	die "Pull is not possible because you have unmerged files.
+Please, fix them up in the work tree, and then use 'git add/rm <file>'
+as appropriate to mark resolution, or use 'git commit -a'."
+    else
+	die "Pull is not possible because you have unmerged files."
+    fi
+}
+
+die_merge () {
+    if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
+	die "You have not concluded your merge (MERGE_HEAD exists).
+Please, commit your changes before you can merge."
+    else
+	die "You have not concluded your merge (MERGE_HEAD exists)."
+    fi
+}
+
+test -z "$(git ls-files -u)" || die_conflict
+test -f "$GIT_DIR/MERGE_HEAD" && die_merge
 
 strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
 log_arg= verbosity=
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 9b3fa2b..9929f82 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -276,11 +276,13 @@ 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 "not possible because you have unmerged files" out &&
+	git add -u &&
+	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
-
+	grep "Your local changes to .* would be overwritten by merge." out
 '
 
 test_expect_success 'merge-recursive remove conflict' '
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index bb4cf00..7f85815 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -66,7 +66,7 @@ test_expect_success 'revert forbidden on dirty working tree' '
 	echo content >extra_file &&
 	git add extra_file &&
 	test_must_fail git revert HEAD 2>errors &&
-	grep "Dirty index" errors
+	grep "Your local changes would be overwritten by " errors
 
 '
 
-- 
1.6.6.198.g3c5474

Re: [PATCH v5] Be more user-friendly when refusing to do something because of conflict.

[Junio C Hamano]

Thanks; will queue.