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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 6, 2010, 17:13 UTC
Message-ID: <7veim3yx5o.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1262783414-23101-1-git-send-email-Matthieu.Moy@imag.fr>

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

Show 19 quoted lines

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