| Message ID | 20220606235449.2890858-1-calvinwan@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | submodule merge: update conflict error message | expand |
Calvin Wan <calvinwan@google.com> writes: > When attempting to do a non-fast-forward merge in a project with > conflicts in the submodules, the merge fails and git prints the > following error: > > Failed to merge submodule <submodules> > CONFLICT (submodule):Merge conflict in <submodule> > Automatic merge failed; fix conflicts and then commit the result. > > Git is left in a conflicted state, which requires the user to: > 1. abort the merge > 2. merge submodules > 3. merge superproject > These steps are non-obvious for newer submodule users to figure out Hmph. Is 1. necessary? IOW, based on the information we already have (we may not be surfacing, which can be corrected), wouldn't it be easier to instead (A) go to submodule and make a merge and then (B) come back to the superproject, "git add <submodule" to record the result of submodule merge, and say "git commit" to conclude? The thing I am worried most about is that you may be throwing away information that would help the user by aborting the superproject merge. Before doing so, you have stage #2 and stage #3 of the submodule commit, so which commits in the submodule you need to merge in (A) above should be fairly clear. If you abort the merge first, how does the user know which commits in the submodule the user needs to merge? > The error message is based off of what would happen when `merge > --recurse-submodules` is eventually supported OK. > Failed to merge submodule <submodule> > CONFLICT (submodule): Merge conflict in <submodule> > Automatic merge failed; recursive merging with submodules is currently > not supported. To manually merge, the following steps are recommended: > - abort the current merge > - merge submodules individually > - merge superproject Again, I am not sure about the recommendation. The message saying "currently not supported" I think is a good idea. > I considered automatically aborting the merge if git detects the merge > failed because of a submodule conflict, however, doing so causes a > significant amount of tests in `t7610-mergetool.sh` (and some other test > scripts as well) to fail, suggesting users have come to expect this > state and have their workarounds with `git mergetool` With or without test failures, my gut feeling sais that it cannot be a good idea to automatically abort the merge, without first grabbing some information out of the conflicted state. Thanks.
> Hmph. Is 1. necessary? I just tested it and it is not, so I do agree recommending to abort the merge is unnecessary/bad advice. How does this sound? Failed to merge submodule <submodule> CONFLICT (submodule): Merge conflict in <submodule> Automatic merge failed; recursive merging with submodules is currently not supported. To manually merge, merge conflicted submodules first before merging the superproject. On Mon, Jun 6, 2022 at 5:48 PM Junio C Hamano <gitster@pobox.com> wrote: > > Calvin Wan <calvinwan@google.com> writes: > > > When attempting to do a non-fast-forward merge in a project with > > conflicts in the submodules, the merge fails and git prints the > > following error: > > > > Failed to merge submodule <submodules> > > CONFLICT (submodule):Merge conflict in <submodule> > > Automatic merge failed; fix conflicts and then commit the result. > > > > Git is left in a conflicted state, which requires the user to: > > 1. abort the merge > > 2. merge submodules > > 3. merge superproject > > These steps are non-obvious for newer submodule users to figure out > > Hmph. Is 1. necessary? > > IOW, based on the information we already have (we may not be > surfacing, which can be corrected), wouldn't it be easier to instead > (A) go to submodule and make a merge and then (B) come back to the > superproject, "git add <submodule" to record the result of submodule > merge, and say "git commit" to conclude? > > The thing I am worried most about is that you may be throwing away > information that would help the user by aborting the superproject > merge. Before doing so, you have stage #2 and stage #3 of the > submodule commit, so which commits in the submodule you need to > merge in (A) above should be fairly clear. If you abort the merge > first, how does the user know which commits in the submodule the > user needs to merge? > > > The error message is based off of what would happen when `merge > > --recurse-submodules` is eventually supported > > OK. > > > Failed to merge submodule <submodule> > > CONFLICT (submodule): Merge conflict in <submodule> > > Automatic merge failed; recursive merging with submodules is currently > > not supported. To manually merge, the following steps are recommended: > > - abort the current merge > > - merge submodules individually > > - merge superproject > > Again, I am not sure about the recommendation. The message saying > "currently not supported" I think is a good idea. > > > I considered automatically aborting the merge if git detects the merge > > failed because of a submodule conflict, however, doing so causes a > > significant amount of tests in `t7610-mergetool.sh` (and some other test > > scripts as well) to fail, suggesting users have come to expect this > > state and have their workarounds with `git mergetool` > > With or without test failures, my gut feeling sais that it cannot be > a good idea to automatically abort the merge, without first grabbing > some information out of the conflicted state. > > Thanks.
Calvin Wan <calvinwan@google.com> writes: >> Hmph. Is 1. necessary? > > I just tested it and it is not, so I do agree recommending to abort the > merge is unnecessary/bad advice. How does this sound? > > Failed to merge submodule <submodule> > CONFLICT (submodule): Merge conflict in <submodule> > Automatic merge failed; recursive merging with submodules is currently > not supported. To manually merge, merge conflicted submodules first > before merging the superproject. This message sounds ok to me, since this is probably what the user wants 90% of the time. Since we don't abort the merge, this just a 'regular' merge conflict resolution (albeit with submodules). The user probably wants to merge the submodules, but they can choose however they want to resolve the merge conflict, e.g. maybe they'd prefer to just pick one side (or even more exotically, a different commit altogether.) An improvement for that other 10% would be to print this help message with the advice() API so that users can turn it off if they don't find it helpful. Or maybe it's confusing to some new users who use a different merging workflow and so an admin might turn off this advice for them.
> The user probably > wants to merge the submodules, but they can choose however they want to > resolve the merge conflict It sounds like I should reword "merge conflicted submodules" to "resolve conflicted submodules". That should cover those 10% cases. I would prefer to find a generic, but still helpful message that doesn't require going into the advice() API or require some config change On Wed, Jun 8, 2022 at 10:35 AM Glen Choo <chooglen@google.com> wrote: > > Calvin Wan <calvinwan@google.com> writes: > > >> Hmph. Is 1. necessary? > > > > I just tested it and it is not, so I do agree recommending to abort the > > merge is unnecessary/bad advice. How does this sound? > > > > Failed to merge submodule <submodule> > > CONFLICT (submodule): Merge conflict in <submodule> > > Automatic merge failed; recursive merging with submodules is currently > > not supported. To manually merge, merge conflicted submodules first > > before merging the superproject. > > This message sounds ok to me, since this is probably what the user wants > 90% of the time. Since we don't abort the merge, this just a 'regular' > merge conflict resolution (albeit with submodules). The user probably > wants to merge the submodules, but they can choose however they want to > resolve the merge conflict, e.g. maybe they'd prefer to just pick one > side (or even more exotically, a different commit altogether.) > > An improvement for that other 10% would be to print this help message > with the advice() API so that users can turn it off if they don't find > it helpful. Or maybe it's confusing to some new users who use a > different merging workflow and so an admin might turn off this advice > for them.
Calvin Wan <calvinwan@google.com> writes: >> The user probably >> wants to merge the submodules, but they can choose however they want to >> resolve the merge conflict > > It sounds like I should reword "merge conflicted submodules" to > "resolve conflicted submodules". That should cover those 10% cases. > > I would prefer to find a generic, but still helpful message that doesn't > require going into the advice() API or require some config change We want the users not to blow away the half-merged state in the working tree. We are guiding them to first go into submodules and merge (in which case, we should tell them merge what with what---I think the first parent should be what they have checked out there, but the other parent, which is what is recorded in the tree of the superproject commit being merged as gitlink, may not be at the tip of any branch you have in the submodule). And then they come back to the superproject and resolve the conflict in the working tree and the index. > > Failed to merge submodule <submodule> > > CONFLICT (submodule): Merge conflict in <submodule> > > Automatic merge failed; recursive merging with submodules is currently > > not supported. To manually merge, merge conflicted submodules first > > before merging the superproject. So, to manually complete the merge: - go to submodule A, and merge commit a24c4e37d0 - go to submodule B, and merge commit a6f14c960b - come back to superproject, and "git add A B" to record the above merge - in superproject, resolve the other conflicts - commit the resulting index in the superproject or something along that line, perhaps?
diff --git a/builtin/merge.c b/builtin/merge.c index f178f5a3ee..39f5ee66d6 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -88,6 +88,7 @@ static const char *sign_commit; static int autostash; static int no_verify; static char *into_name; +static int submodule_conflict = 0; static struct strategy all_strategy[] = { { "recursive", NO_TRIVIAL }, @@ -757,6 +758,8 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, else clean = merge_recursive(&o, head, remoteheads->item, reversed, &result); + if (o.submodule_conflict) + submodule_conflict = 1; if (clean < 0) exit(128); if (write_locked_index(&the_index, &lock, @@ -973,7 +976,14 @@ static int suggest_conflicts(void) strbuf_release(&msgbuf); fclose(fp); repo_rerere(the_repository, allow_rerere_auto); - printf(_("Automatic merge failed; " + if (submodule_conflict) + printf(_("Automatic merge failed; recursive merging with submodules is currently\n" + "not supported. To manually merge, the following steps are recommended:\n" + " - abort the current merge\n" + " - merge submodules individually\n" + " - merge superproject\n")); + else + printf(_("Automatic merge failed; " "fix conflicts and then commit the result.\n")); return 1; } diff --git a/merge-ort.c b/merge-ort.c index 0d3f42592f..205d6658bc 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -3866,8 +3866,10 @@ static void process_entry(struct merge_options *opt, const char *reason = _("content"); if (ci->filemask == 6) reason = _("add/add"); - if (S_ISGITLINK(merged_file.mode)) + if (S_ISGITLINK(merged_file.mode)) { reason = _("submodule"); + opt->submodule_conflict = 1; + } path_msg(opt, path, 0, _("CONFLICT (%s): Merge conflict in %s"), reason, path); diff --git a/merge-recursive.c b/merge-recursive.c index fd1bbde061..535b8cc758 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3149,8 +3149,10 @@ static int handle_content_merge(struct merge_file_info *mfi, } if (!mfi->clean) { - if (S_ISGITLINK(mfi->blob.mode)) + if (S_ISGITLINK(mfi->blob.mode)) { reason = _("submodule"); + opt->submodule_conflict = 1; + } output(opt, 1, _("CONFLICT (%s): Merge conflict in %s"), reason, path); if (ci && !df_conflict_remains) diff --git a/merge-recursive.h b/merge-recursive.h index b88000e3c2..6fd31644ad 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -51,6 +51,7 @@ struct merge_options { /* internal fields used by the implementation */ struct merge_options_internal *priv; + unsigned submodule_conflict : 1; }; void init_merge_options(struct merge_options *opt, struct repository *repo); diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh index 178413c22f..bfcc81cd06 100755 --- a/t/t6437-submodule-merge.sh +++ b/t/t6437-submodule-merge.sh @@ -141,6 +141,7 @@ test_expect_success 'merging should conflict for non fast-forward' ' test_must_fail git merge c 2> actual fi && grep $(cat expect) actual > /dev/null && + test_i18ngrep "recursive merging with submodules is currently" actual && git reset --hard) ' @@ -167,6 +168,7 @@ test_expect_success 'merging should fail for ambiguous common parent' ' fi && grep $(cat expect1) actual > /dev/null && grep $(cat expect2) actual > /dev/null && + test_i18ngrep "recursive merging with submodules is currently" actual && git reset --hard) ' @@ -205,7 +207,8 @@ test_expect_success 'merging should fail for changes that are backwards' ' git commit -a -m "f" && git checkout -b test-backward e && - test_must_fail git merge f) + test_must_fail git merge f >actual && + test_i18ngrep "recursive merging with submodules is currently" actual) '
When attempting to do a non-fast-forward merge in a project with conflicts in the submodules, the merge fails and git prints the following error: Failed to merge submodule <submodules> CONFLICT (submodule):Merge conflict in <submodule> Automatic merge failed; fix conflicts and then commit the result. Git is left in a conflicted state, which requires the user to: 1. abort the merge 2. merge submodules 3. merge superproject These steps are non-obvious for newer submodule users to figure out based on the error message and neither `git submodule status` nor `git status` provide any useful pointers. Update error message to the following when attempting to do a non-fast-forward merge in a project with conflicts in the submodules. The error message is based off of what would happen when `merge --recurse-submodules` is eventually supported Failed to merge submodule <submodule> CONFLICT (submodule): Merge conflict in <submodule> Automatic merge failed; recursive merging with submodules is currently not supported. To manually merge, the following steps are recommended: - abort the current merge - merge submodules individually - merge superproject I considered automatically aborting the merge if git detects the merge failed because of a submodule conflict, however, doing so causes a significant amount of tests in `t7610-mergetool.sh` (and some other test scripts as well) to fail, suggesting users have come to expect this state and have their workarounds with `git mergetool` Signed-off-by: Calvin Wan <calvinwan@google.com> --- builtin/merge.c | 12 +++++++++++- merge-ort.c | 4 +++- merge-recursive.c | 4 +++- merge-recursive.h | 1 + t/t6437-submodule-merge.sh | 5 ++++- 5 files changed, 22 insertions(+), 4 deletions(-) base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085