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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

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/