Re: [PATCH v2] Be more user-friendly when refusing to do something because of conflict.
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:47:58
Matthieu Moy [off-list ref] 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.
quoted hunk
@@ -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.
quoted hunk
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?
quoted hunk
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.
quoted hunk
diff --git a/builtin-revert.c b/builtin-revert.c
Nice.
quoted hunk
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.