Improving merge failure message

Improving merge failure message

[Nanako Shiraishi]

I often see my students confused after a failed merge and they can't
figure out what to do next. Two typical error messages they get are

[1]% git merge master
Auto-merging cool
CONFLICT (content): Merge conflict in cool
Automatic merge failed; fix conflicts and then commit the result.

[2]% git merge feature
error: Entry 'cool' not uptodate. Cannot merge.
fatal: merging of trees 8ec1d96451ff05451720e4e8968812c46b35e5e4 and aad8d5cef3915ab78b3227abaaac99b62db9eb54 failed

In the former case, the merge command gives a helpful message that
automatic merge failed because it found a conflict and tells enough
instruction to the user.

But in the latter case, the messages look unnecessarily scary, with two
"error" and "fatal" comments, and long sha1 commit names.

Those of us who used git for some time can tell what it wants to say.
The merge checked the files in the working tree before doing anything,
found that the user has uncommitted change to a file that is involved in
the merge, and it stopped. And it didn't change anything. It may be "fatal"
but the user has much less reason to be scared about this failure than
the conflicting case.

It would be nice if the message in the latter case can be toned down.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: Improving merge failure message

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> [2]% git merge feature
> error: Entry 'cool' not uptodate. Cannot merge.
> fatal: merging of trees 8ec1d96451ff05451720e4e8968812c46b35e5e4 and aad8d5cef3915ab78b3227abaaac99b62db9eb54 failed
>
> ... the messages look unnecessarily scary, with two
> "error" and "fatal" comments, and long sha1 commit names.

Just a technical nit.  I think these are tree object names.

> Those of us who used git for some time can tell what it wants to say.
> The merge checked the files in the working tree before doing anything,
> found that the user has uncommitted change to a file that is involved in
> the merge, and it stopped. And it didn't change anything. It may be "fatal"
> but the user has much less reason to be scared about this failure than
> the conflicting case.
>
> It would be nice if the message in the latter case can be toned down.

Yeah, it would be nice.  This actually was something that bothered me as
well while trying to explain the recovery procedure for these two cases.
Give me half an hour or so to cook up something...

Re: Improving merge failure message

[Junio C Hamano]

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

> Nanako Shiraishi <nanako3@lavabit.com> writes:
>
>> [2]% git merge feature
>> error: Entry 'cool' not uptodate. Cannot merge.
>> fatal: merging of trees 8ec1d96451ff05451720e4e8968812c46b35e5e4 and aad8d5cef3915ab78b3227abaaac99b62db9eb54 failed
>>
>> ... the messages look unnecessarily scary, with two
>> "error" and "fatal" comments, and long sha1 commit names.
>
> Just a technical nit.  I think these are tree object names.
>
>> Those of us who used git for some time can tell what it wants to say.
>> The merge checked the files in the working tree before doing anything,
>> found that the user has uncommitted change to a file that is involved in
>> the merge, and it stopped. And it didn't change anything. It may be "fatal"
>> but the user has much less reason to be scared about this failure than
>> the conflicting case.
>>
>> It would be nice if the message in the latter case can be toned down.
>
> Yeah, it would be nice.  This actually was something that bothered me as
> well while trying to explain the recovery procedure for these two cases.
> Give me half an hour or so to cook up something...

It turns out to be a lot simpler than I thought, because 8ccba00
(unpack-trees: allow Porcelain to give different error messages,
2008-05-17) already laid enough groundwork for doing this kind of thing
easily.

Notable points are:

 - End the messages with "Aborting."; they are given when the three-way
   merge stops without harming the work tree;

 - Do not give the extra message after unpack_trees() already errored out.
   This "merging of trees failed" message was primarily for debugging
   merge-recursive itself, and the end user cannot do much with the object
   names given in the message anyway.

   But do give it under higher verbosity level, or when it happens during
   the inner merge (the "recursive" one), as unpack_trees() should not
   fail for the inner merge under normal conditions.

We could later add instructions on how to recover (i.e. "stash changes
away or commit on a side branch and retry") instead of the silent
exit(128) I have down there, and then use Peff's advice.* mechanism to
squelch it (e.g. "advice.mergeindirtytree"), but they are separate topics.

 merge-recursive.c |   25 +++++++++++++++++++++----
 1 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 10d7913..a237240 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -170,6 +170,18 @@ static int git_merge_trees(int index_only,
 	int rc;
 	struct tree_desc t[3];
 	struct unpack_trees_options opts;
+	static struct unpack_trees_error_msgs msgs = {
+		/* would_overwrite */
+		"Your local changes to '%s' will be clobbered by merge.  Aborting.",
+		/* not_uptodate_file */
+		"Your local changes to '%s' will be clobbered by merge.  Aborting.",
+		/* not_uptodate_dir */
+		"Updating '%s' would lose untracked files in it.  Aborting.",
+		/* would_lose_untracked */
+		"Untracked working tree file '%s' would be %s by merge.  Aborting",
+		/* bind_overlap -- will not happen here */
+		NULL,
+	};
 
 	memset(&opts, 0, sizeof(opts));
 	if (index_only)
@@ -181,6 +193,7 @@ static int git_merge_trees(int index_only,
 	opts.fn = threeway_merge;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
+	opts.msgs = msgs;
 
 	init_tree_desc_from_tree(t+0, common);
 	init_tree_desc_from_tree(t+1, head);
@@ -1188,10 +1201,14 @@ int merge_trees(struct merge_options *o,
 
 	code = git_merge_trees(o->call_depth, common, head, merge);
 
-	if (code != 0)
-		die("merging of trees %s and %s failed",
-		    sha1_to_hex(head->object.sha1),
-		    sha1_to_hex(merge->object.sha1));
+	if (code != 0) {
+		if (show(o, 4) || o->call_depth)
+			die("merging of trees %s and %s failed",
+			    sha1_to_hex(head->object.sha1),
+			    sha1_to_hex(merge->object.sha1));
+		else
+			exit(128);
+	}
 
 	if (unmerged_cache()) {
 		struct string_list *entries, *re_head, *re_merge;

Re: Improving merge failure message

[Sverre Rabbelier]

Heya,

On Tue, Sep 8, 2009 at 09:15, Junio C Hamano<gitster@pobox.com> wrote:
> +               /* would_overwrite */
> +               "Your local changes to '%s' will be clobbered by merge.  Aborting.",

Still scary, shouldn't that be s/will be/would be/ ?

> +               /* not_uptodate_file */
> +               "Your local changes to '%s' will be clobbered by merge.  Aborting.",

Ditto.

> +               /* not_uptodate_dir */
> +               "Updating '%s' would lose untracked files in it.  Aborting.",

Not sure, but maybe s/in it//

-- 
Cheers,

Sverre Rabbelier

Re: Improving merge failure message

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> On Tue, Sep 8, 2009 at 09:15, Junio C Hamano<gitster@pobox.com> wrote:
>> +               /* would_overwrite */
>> +               "Your local changes to '%s' will be clobbered by merge.  Aborting.",
>
> Still scary, shouldn't that be s/will be/would be/ ?

Thanks, very true indeed.  "It would be clobbered if we were to continue
hence we abort." is how we want to explain our behaviour, so "would" is
definitely better here.

>> +               /* not_uptodate_dir */
>> +               "Updating '%s' would lose untracked files in it.  Aborting.",

This is "merge would resolve to have a file X, but you have a directory X
in your work tree and it is not empty" case.

I'll leave the exact wording up to other people.  My primary focus was to
end all of these messages with "Aborting."

This turns out to be a continuation of an older discussion thread back in
May 2008, and I do not know if anybody took up the challenge back then.  I
wouldn't be surprised if "checkout", which was the topic of the old
thread, has some other scary plumbing message still seeping through to the
UI layer.  Perhaps there are some other commands that needs similar kind
of love.

Re: Improving merge failure message

[Jeff King]

On Tue, Sep 08, 2009 at 12:48:23AM -0700, Junio C Hamano wrote:

> Thanks, very true indeed.  "It would be clobbered if we were to continue
> hence we abort." is how we want to explain our behaviour, so "would" is
> definitely better here.

While we're picking apart your wording, is "clobbered" the word we want
to use? Everywhere else that is user-facing we tend to use the term
"overwritten".

-Peff

Re: Improving merge failure message

[Mike Ralphson]

2009/9/8 Junio C Hamano <gitster@pobox.com>:
> I'll leave the exact wording up to other people.  My primary focus was to
> end all of these messages with "Aborting."
>
> This turns out to be a continuation of an older discussion thread back in
> May 2008, and I do not know if anybody took up the challenge back then.  I
> wouldn't be surprised if "checkout", which was the topic of the old
> thread, has some other scary plumbing message still seeping through to the
> UI layer.  Perhaps there are some other commands that needs similar kind
> of love.

Just a note that Documentation/git-checkout.txt references this
message in an example and should be kept in step with the final
wording change. It would be ideal to be able to regression-test the
examples in the documentation somehow but that might involve abusing
the asciidoc markup somewhat.

2009/9/8 Jeff King <peff@peff.net>:
> While we're picking apart your wording, is "clobbered" the word we want
> to use?

If we're debating 'clobbered', then maybe the non-word 'uptodate' is
fair game too? 8-)

Re: Improving merge failure message

[Johannes Sixt]

Junio C Hamano schrieb:
> Nanako Shiraishi <nanako3@lavabit.com> writes:
> 
>> [2]% git merge feature
>> error: Entry 'cool' not uptodate. Cannot merge.
>> fatal: merging of trees 8ec1d96451ff05451720e4e8968812c46b35e5e4 and aad8d5cef3915ab78b3227abaaac99b62db9eb54 failed
>>
>> ... the messages look unnecessarily scary, with two
>> "error" and "fatal" comments, and long sha1 commit names.
> 
>> It would be nice if the message in the latter case can be toned down.
> 
> Yeah, it would be nice.  This actually was something that bothered me as
> well while trying to explain the recovery procedure for these two cases.
> Give me half an hour or so to cook up something...

I think that this is a symptom of a much more involved issue about error
handling.

Currently, it is customary in the code to report an error at each location
where an exceptional condition is detected:

	if (frotz())
		ret = error("frotz messed up");

If frotz() is a "low-level" routine, like a C library function, then this
is the right thing to do. However, if frotz() is our own function
("high-level") which itself calls low-level functions with the same
pattern, then this is obviously the wrong thing to do, because it results
into two error messages.

We need a guideline how errors are reported. This guideline could be:

(1) Low-level functions do not print error messages, but set an error code
and leave the reporting to the caller.

(2) High-level functions must print an error message (through error() or
die()) when they detect a failure of a low-level function that was called
directly.

(3) High-level functions must not print an error message when they detect
a failure of another high-level function that was called directly.

There remains to classify our functions into high-level or low-level. But
I think we won't have a lot of low-level functions.

BTW, I applied this guideline when I worked on {start,finish,run}_command
recently, and IMHO it worked out quite nicely because exactly one error is
reported after a failure.

-- Hannes

Re: Improving merge failure message

[Alex Riesen]

On Tue, Sep 8, 2009 at 09:15, Junio C Hamano<gitster@pobox.com> wrote:
> @@ -170,6 +170,18 @@ static int git_merge_trees(int index_only,
>        int rc;
>        struct tree_desc t[3];
>        struct unpack_trees_options opts;
> +       static struct unpack_trees_error_msgs msgs = {

You can make these const, the struct is copied anyway:

> +       opts.msgs = msgs;

Here --^

Re: Improving merge failure message

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com>

> Notable points are:
>
>  - End the messages with "Aborting."; they are given when the three-way
>    merge stops without harming the work tree;
>
>  - Do not give the extra message after unpack_trees() already errored out.
>    This "merging of trees failed" message was primarily for debugging
>    merge-recursive itself, and the end user cannot do much with the object
>    names given in the message anyway.
>
>    But do give it under higher verbosity level, or when it happens during
>    the inner merge (the "recursive" one), as unpack_trees() should not
>    fail for the inner merge under normal conditions.
>
> We could later add instructions on how to recover (i.e. "stash changes
> away or commit on a side branch and retry") instead of the silent
> exit(128) I have down there, and then use Peff's advice.* mechanism to
> squelch it (e.g. "advice.mergeindirtytree"), but they are separate topics.

Thank you for a quick response. The patch works fine here, so if you want
please add:

Tested-by: Nanako Shiraishi <nanako3@lavabit.com>

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: Improving merge failure message

[Junio C Hamano]

Mike Ralphson <mike.ralphson@gmail.com> writes:

> 2009/9/8 Junio C Hamano <gitster@pobox.com>:
>> I'll leave the exact wording up to other people.  My primary focus was to
>> end all of these messages with "Aborting."
>>
>> This turns out to be a continuation of an older discussion thread back in
>> May 2008, and I do not know if anybody took up the challenge back then.  I
>> wouldn't be surprised if "checkout", which was the topic of the old
>> thread, has some other scary plumbing message still seeping through to the
>> UI layer.  Perhaps there are some other commands that needs similar kind
>> of love.
>
> Just a note that Documentation/git-checkout.txt references this
> message in an example and should be kept in step with the final
> wording change. It would be ideal to be able to regression-test the
> examples in the documentation somehow but that might involve abusing
> the asciidoc markup somewhat.

Thanks.

> 2009/9/8 Jeff King <peff@peff.net>:
>> While we're picking apart your wording, is "clobbered" the word we want
>> to use?
>
> If we're debating 'clobbered', then maybe the non-word 'uptodate' is
> fair game too? 8-)

I think you added smiley because you already knew the answer, but if that
is not the case, please see

    http://thread.gmane.org/gmane.comp.version-control.git/81100/focus=82358