Matthieu Moy <Matthieu.Moy@xxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html