| Message ID | 20220629224059.1016645-1-calvinwan@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] submodule merge: update conflict error message | expand |
Hi, On Wed, Jun 29, 2022 at 3:41 PM Calvin Wan <calvinwan@google.com> wrote: > > When attempting to merge in a superproject with conflicting submodule > pointers that cannot be fast-forwarded, the merge fails and git prints > the following error: > > Failed to merge submodule <submodule> > 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. merge submodules or update submodules to an already existing > commit that reflects the merge > 2. add submodules changes to the superproject > 3. finish merging 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 match the steps above. If git does not detect a > merge resolution, the following is printed: > > -------- > > Failed to merge submodule <submodule> > CONFLICT (submodule): Merge conflict in <submodule> > Automatic merge failed; recursive merging with submodules is currently > not supported. To manually complete the merge: > - go to submodule (<submodule>), and merge commit <commit> I'm still a little unsure on this. The merge_submodule() logic we have may not have detected a merge commit that merges the two branches together, but that doesn't automatically imply a new merge must be created in the submodule to resolve this conflict. There might be various reasons that other existing commits in the submodule could be used as the "correct" update. Perhaps that is uncommon enough to not be worth mentioning; I'm just a little hesitant to treat the merge_submodule() logic as robust and finding the only choices users might want to use. If we do keep the wording as-is, it might be nice to at least discuss in the commit message why we chose that and ignored the or-update-submodule option in this case. > - come back to superproject, and `git add <submodule>` to record the above merge > - resolve any other conflicts in the superproject > - commit the resulting index in the superproject > Automatic merge failed; fix conflicts and then commit the result. > > -------- > > If git detects a possible merge resolution, the following is printed: > > -------- > > Failed to merge submodule sub, but a possible merge resolution exists: > <commit> Merge branch '<branch1>' into <branch2> > > > If this is correct simply add it to the index for example > by using: > > git update-index --cacheinfo 160000 <commit> "<submodule>" > > which will accept this suggestion. The last few lines above will no longer be part of the output once en/merge-tree is merged; see commit a4040cfa35 (merge-ort: remove command-line-centric submodule message from merge-ort, 2022-06-18). > CONFLICT (submodule): Merge conflict in <submodule> > Recursive merging with submodules is currently not supported. > To manually complete the merge: > - go to submodule (<submodule>), and either update the submodule to a possible commit above or merge commit <commit> Again, I'm hesitant to treat the suggestions from merge_submodule() as the only possibilities. For example, perhaps the user will want to pick a commit that contains one of the ones found by merge_submodule() in its history -- perhaps because the newer commit they want to select contains an important bugfix for an issue not discovered at the time of the merge in the submodule. Also, I'm worried your sentence may be easy to misunderstand, due to it being ambiguous whether "merge" is a verb or an adjective. More precisely, your sentence could be read as "update the submodule to a possible commit above, or update the submodule to merge commit <commit>" and then users say, "But <commit> isn't a merge commit; why does this message claim it is? Do I just update the submodule to that commit anyway?" Or perhaps users just update it to <commit> without even checking to find out that it's not a merge commit, with the deleterious consequences of missing all the important changes currently found in the submodule. > - come back to superproject, and `git add <submodule>` to record the above merge "...to record the above merge or update"? > - resolve any other conflicts in the superproject > - commit the resulting index in the superproject > Automatic merge failed; fix conflicts and then commit the result. > > -------- > > If git detects multiple possible merge resolutions, the following is printed: > > -------- > > Failed to merge submodule sub, but multiple possible merges exist: > <commit> Merge branch '<branch1>' into <branch2> > <commit> Merge branch '<branch1>' into <branch3> > > CONFLICT (submodule): Merge conflict in <submodule> > Recursive merging with submodules is currently not supported. > To manually complete the merge: > - go to submodule (<submodule>), and either update the submodule to a possible commit above or merge commit <commit> Same issues as I mentioned above for the single-merge-resolution-found case. > - come back to superproject, and `git add <submodule>` to record the above merge "merge or update", right? > - resolve any other conflicts in the superproject > - commit the resulting index in the superproject > Automatic merge failed; fix conflicts and then commit the result. > > ------- > > Changes since v2: > There are three major changes in this patch thanks to all the helpful > feedback! I have moved the print function from builtin/merge.c to > merge-ort.c and added it to merge_finalize() in merge-recursive.c and > merge_switch_to_result() in merge-ort.c. This allows other merge > machinery commands besides merge to print submodule conflict advice. > > I have moved the check for submodule conflicts from process_entry() to > merge_submodule(). This also required removing the early returns and > instead going to my submodule conflict check allowing us to pass back > information on whether git detected a possible merge resolution or not. > > I have also updated the error message to better reflect the merge > status. Specifically, if git detects a possible merge resolution, the > error message now also suggest updating to one of those resolutions. > > Other small changes: > - Moved fields that hold submodule conflict information to new object > - Shortened printed commit id to DEFAULT_ABBREV > - Added a test to t6437-submodule-merge.sh for merges with no > resolution existence > - Modified a test in t7402-submodule-rebase to show error message > prints in other parts of the merge machinery > > Changes since v1: > - Removed advice to abort merge > - Error message updated to contain more commit/submodule information > > Signed-off-by: Calvin Wan <calvinwan@google.com> > --- > merge-ort.c | 53 ++++++++++++++++++++++++++++++++++--- > merge-recursive.c | 26 +++++++++++++++--- > merge-recursive.h | 18 +++++++++++++ > t/t6437-submodule-merge.sh | 36 ++++++++++++++++++++++--- > t/t7402-submodule-rebase.sh | 6 +++-- > 5 files changed, 125 insertions(+), 14 deletions(-) > > diff --git a/merge-ort.c b/merge-ort.c > index 0d3f42592f..2d9f03ea8c 100644 > --- a/merge-ort.c > +++ b/merge-ort.c > @@ -1610,6 +1610,9 @@ static int merge_submodule(struct merge_options *opt, > struct commit *commit_o, *commit_a, *commit_b; > int parent_count; > struct object_array merges; > + struct conflicted_submodule_list *csub = opt->conflicted_submodules; > + struct conflicted_submodule_item csub_item; > + int resolution_exists = 0; > > int i; > int search = !opt->priv->call_depth; > @@ -1619,17 +1622,17 @@ static int merge_submodule(struct merge_options *opt, > > /* we can not handle deletion conflicts */ > if (is_null_oid(o)) > - return 0; > + goto ret; > if (is_null_oid(a)) > - return 0; > + goto ret; > if (is_null_oid(b)) > - return 0; > + goto ret; > > if (repo_submodule_init(&subrepo, opt->repo, path, null_oid())) { > path_msg(opt, path, 0, > _("Failed to merge submodule %s (not checked out)"), > path); > - return 0; > + goto ret; > } > > if (!(commit_o = lookup_commit_reference(&subrepo, o)) || > @@ -1703,6 +1706,7 @@ static int merge_submodule(struct merge_options *opt, > "which will accept this suggestion.\n"), > oid_to_hex(&merges.objects[0].item->oid), path); > strbuf_release(&sb); > + resolution_exists = 1; > break; > default: > for (i = 0; i < merges.nr; i++) > @@ -1712,11 +1716,24 @@ static int merge_submodule(struct merge_options *opt, > _("Failed to merge submodule %s, but multiple " > "possible merges exist:\n%s"), path, sb.buf); > strbuf_release(&sb); > + resolution_exists = 1; > } > > object_array_clear(&merges); > cleanup: > repo_clear(&subrepo); > +ret: > + if (!ret) { > + if (!csub) { > + CALLOC_ARRAY(csub, 1); > + } > + csub_item.oid = *result; Shouldn't this be "*b" rather than "*result"? Also, are we risking telling the user to "merge commit 0000000000000000000000000000000000000000" from the submodule, given some of the early exits that you redirected to come through here? > + csub_item.path = xstrdup(path); > + csub_item.resolution_exists = resolution_exists; > + ALLOC_GROW(csub->items, csub->nr + 1, csub->alloc); > + csub->items[csub->nr++] = csub_item; > + opt->conflicted_submodules = csub; > + } > return ret; > } > > @@ -4256,6 +4273,32 @@ static int record_conflicted_index_entries(struct merge_options *opt) > return errs; > } > > +void print_submodule_conflict_suggestion(struct conflicted_submodule_list *csub) { Maybe just make this function be static? > + if (csub && csub->nr > 0) { > + int i; > + printf(_("Recursive merging with submodules is currently not supported.\n" > + "To manually complete the merge:\n")); > + for (i = 0; i < csub->nr; i++) { > + const struct object_id id = csub->items[i].oid; > + if (csub->items[i].resolution_exists) > + printf(_(" - go to submodule (%s), and either update the submodule " > + "to a possible commit above or merge commit %s\n"), > + csub->items[i].path, > + find_unique_abbrev(&id, DEFAULT_ABBREV)); Shouldn't this be repo_find_unique_abbrev(), and in particular with the submodule repository being passed to it? Or perhaps using the format_commit() function, since merge_submodule() is already using it for the earlier submodule-related messages? > + else > + printf(_(" - go to submodule (%s), and merge commit %s\n"), > + csub->items[i].path, > + find_unique_abbrev(&id, DEFAULT_ABBREV)); Likewise? > + } > + printf(_(" - come back to superproject, and `git add")); > + for (i = 0; i < csub->nr; i++) > + printf(_(" %s"), csub->items[i].path); > + printf(_("` to record the above merge \n" > + " - resolve any other conflicts in the superproject\n" > + " - commit the resulting index in the superproject\n")); > + } > +} > + > void merge_switch_to_result(struct merge_options *opt, > struct tree *head, > struct merge_result *result, > @@ -4324,6 +4367,8 @@ void merge_switch_to_result(struct merge_options *opt, > } > string_list_clear(&olist, 0); > > + print_submodule_conflict_suggestion(opt->conflicted_submodules); > + > /* Also include needed rename limit adjustment now */ > diff_warn_rename_limit("merge.renamelimit", > opti->renames.needed_limit, 0); > diff --git a/merge-recursive.c b/merge-recursive.c > index fd1bbde061..311cc37886 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c Is it worth updating merge-recursive.c here? I'd rather only give it security fix updates until we delete it. > @@ -1190,6 +1190,9 @@ static int merge_submodule(struct merge_options *opt, > struct commit *commit_base, *commit_a, *commit_b; > int parent_count; > struct object_array merges; > + struct conflicted_submodule_list *csub = opt->conflicted_submodules; > + struct conflicted_submodule_item csub_item; > + int resolution_exists = 0; > > int i; > int search = !opt->priv->call_depth; > @@ -1204,15 +1207,15 @@ static int merge_submodule(struct merge_options *opt, > > /* we can not handle deletion conflicts */ > if (is_null_oid(base)) > - return 0; > + goto ret; > if (is_null_oid(a)) > - return 0; > + goto ret; > if (is_null_oid(b)) > - return 0; > + goto ret; > > if (repo_submodule_init(&subrepo, opt->repo, path, null_oid())) { > output(opt, 1, _("Failed to merge submodule %s (not checked out)"), path); > - return 0; > + goto ret; > } > > if (!(commit_base = lookup_commit_reference(&subrepo, base)) || > @@ -1287,17 +1290,31 @@ static int merge_submodule(struct merge_options *opt, > " git update-index --cacheinfo 160000 %s \"%s\"\n\n" > "which will accept this suggestion.\n"), > oid_to_hex(&merges.objects[0].item->oid), path); > + resolution_exists = 1; > break; > > default: > output(opt, 1, _("Failed to merge submodule %s (multiple merges found)"), path); > for (i = 0; i < merges.nr; i++) > print_commit(&subrepo, (struct commit *) merges.objects[i].item); > + resolution_exists = 1; > } > > object_array_clear(&merges); > cleanup: > repo_clear(&subrepo); > +ret: > + if (!ret) { > + if (!csub) { > + CALLOC_ARRAY(csub, 1); > + } > + csub_item.oid = *result; > + csub_item.path = xstrdup(path); > + csub_item.resolution_exists = resolution_exists; > + ALLOC_GROW(csub->items, csub->nr + 1, csub->alloc); > + csub->items[csub->nr++] = csub_item; > + opt->conflicted_submodules = csub; > + } > return ret; > } > > @@ -3736,6 +3753,7 @@ static void merge_finalize(struct merge_options *opt) > flush_output(opt); > if (!opt->priv->call_depth && opt->buffer_output < 2) > strbuf_release(&opt->obuf); > + print_submodule_conflict_suggestion(opt->conflicted_submodules); > if (show(opt, 2)) > diff_warn_rename_limit("merge.renamelimit", > opt->priv->needed_rename_limit, 0); > diff --git a/merge-recursive.h b/merge-recursive.h > index b88000e3c2..b615955fb7 100644 > --- a/merge-recursive.h > +++ b/merge-recursive.h > @@ -9,6 +9,7 @@ struct object_id; > struct repository; > struct tree; > > +struct conflicted_submodule_list; > struct merge_options_internal; > struct merge_options { > struct repository *repo; > @@ -51,6 +52,21 @@ struct merge_options { > > /* internal fields used by the implementation */ > struct merge_options_internal *priv; > + > + /* field that holds submodule conflict information */ > + struct conflicted_submodule_list *conflicted_submodules; I think this should be added to merge_options_internal rather than to merge_options. There's no need for an API caller to make use of these. Also, if a clear need arises later for API callers to make use of this information, then it should be part of merge_result for merge-ort.c, not part of merge_options. > +}; > + > +struct conflicted_submodule_item { > + struct object_id oid; > + char *path; > + int resolution_exists; > +}; > + > +struct conflicted_submodule_list { > + struct conflicted_submodule_item *items; > + size_t nr; > + size_t alloc; > }; Similarly, I think these should be defined in merge-ort.c (and maybe also merge-recursive.c) near struct merge_options_internal. > void init_merge_options(struct merge_options *opt, struct repository *repo); > @@ -122,4 +138,6 @@ int merge_recursive_generic(struct merge_options *opt, > const struct object_id **merge_bases, > struct commit **result); > > +void print_submodule_conflict_suggestion(struct conflicted_submodule_list *csub); and I think there's no reason to put this in the header file; it should just be a static function in merge-ort.c. (And, if we really want to update merge-recursive.c, then copy the function over there. *Nothing* in merge-ort.c should call a function in merge-recursive.c and similarly nothing in merge-recursive.c should call any function in merge-ort.c. Yes, that implies some duplication. There is a fair amount already and we've discussed it, and chosen against creating a shared module as well. I want merge-recursive to not be broken by merge-ort updates; it should remain stable until the day we finally get to nuke it. I'm personally of the opinion that merge-recursive should only get security fixes at this point, though I haven't pinged to see if other folks agree with that point of view yet or not. I'm not wasting my time fixing/improving it, though...) > + > #endif > diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh > index 178413c22f..f0a31c3a61 100755 > --- a/t/t6437-submodule-merge.sh > +++ b/t/t6437-submodule-merge.sh > @@ -103,8 +103,28 @@ test_expect_success 'setup for merge search' ' > echo "file-c" > file-c && > git add file-c && > git commit -m "sub-c") && > - git commit -a -m "c" && > + git commit -a -m "c") > +' > + > +test_expect_success 'merging should conflict for non fast-forward' ' > + (cd merge-search && > + git checkout -b test-nonforward-a b && > + (cd sub && > + git rev-parse --short sub-b > ../expect) && > + if test "$GIT_TEST_MERGE_ALGORITHM" = ort > + then > + test_must_fail git merge c >actual > + else > + test_must_fail git merge c 2> actual > + fi && > + grep $(cat expect) actual > /dev/null && > + sub_expect="go to submodule (sub), and merge commit $(git -C sub rev-parse --short sub-b)" && > + grep "$sub_expect" actual && > + git reset --hard) > +' > > +test_expect_success 'finish setup for merge-search' ' > + (cd merge-search && > git checkout -b d a && > (cd sub && > git checkout -b sub-d sub-b && > @@ -129,9 +149,9 @@ test_expect_success 'merge with one side as a fast-forward of the other' ' > test_cmp expect actual) > ' > > -test_expect_success 'merging should conflict for non fast-forward' ' > +test_expect_success 'merging should conflict for non fast-forward (resolution exists)' ' > (cd merge-search && > - git checkout -b test-nonforward b && > + git checkout -b test-nonforward-b b && > (cd sub && > git rev-parse sub-d > ../expect) && > if test "$GIT_TEST_MERGE_ALGORITHM" = ort > @@ -141,6 +161,9 @@ 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 && > + sub_expect="go to submodule (sub), and either update the submodule to a \ > +possible commit above or merge commit $(git -C sub rev-parse --short sub-b)" && > + grep "$sub_expect" actual && > git reset --hard) > ' > > @@ -167,6 +190,9 @@ test_expect_success 'merging should fail for ambiguous common parent' ' > fi && > grep $(cat expect1) actual > /dev/null && > grep $(cat expect2) actual > /dev/null && > + sub_expect="go to submodule (sub), and either update the submodule to a \ > +possible commit above or merge commit $(git -C sub rev-parse --short sub-b)" && > + grep "$sub_expect" actual && > git reset --hard) > ' > > @@ -205,7 +231,9 @@ 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 && > + sub_expect="go to submodule (sub), and merge commit $(git -C sub rev-parse --short sub-a)" && > + grep "$sub_expect" actual) > ' > > > diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh > index 8e32f19007..21c8b4e97c 100755 > --- a/t/t7402-submodule-rebase.sh > +++ b/t/t7402-submodule-rebase.sh > @@ -104,7 +104,7 @@ test_expect_success 'rebasing submodule that should conflict' ' > test_tick && > git commit -m fourth && > > - test_must_fail git rebase --onto HEAD^^ HEAD^ HEAD^0 && > + test_must_fail git rebase --onto HEAD^^ HEAD^ HEAD^0 >actual_output && > git ls-files -s submodule >actual && > ( > cd submodule && > @@ -112,7 +112,9 @@ test_expect_success 'rebasing submodule that should conflict' ' > echo "160000 $(git rev-parse HEAD^^) 2 submodule" && > echo "160000 $(git rev-parse HEAD) 3 submodule" > ) >expect && > - test_cmp expect actual > + test_cmp expect actual && > + sub_expect="go to submodule (submodule), and merge commit $(git -C submodule rev-parse --short HEAD^^)" && > + grep "$sub_expect" actual_output > ' > > test_done > > base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085 > -- > 2.37.0.rc0.161.g10f37bed90-goog Despite my many nitpicks and whatnot, it looks like your change will make things nicer for the user, and I think your patch is coming along nicely. There are some things to fix up, but the overall direction seems good.
> > Failed to merge submodule <submodule> > > CONFLICT (submodule): Merge conflict in <submodule> > > Automatic merge failed; recursive merging with submodules is currently > > not supported. To manually complete the merge: > > - go to submodule (<submodule>), and merge commit <commit> > > I'm still a little unsure on this. The merge_submodule() logic we > have may not have detected a merge commit that merges the two branches > together, but that doesn't automatically imply a new merge must be > created in the submodule to resolve this conflict. There might be > various reasons that other existing commits in the submodule could be > used as the "correct" update. > > Perhaps that is uncommon enough to not be worth mentioning; I'm just a > little hesitant to treat the merge_submodule() logic as robust and > finding the only choices users might want to use. If we do keep the > wording as-is, it might be nice to at least discuss in the commit > message why we chose that and ignored the or-update-submodule option > in this case. You make a good point about merge_submodule() possibly not finding all of the right choices -- I'll add back the or-update-submodule option back. > > If this is correct simply add it to the index for example > > by using: > > > > git update-index --cacheinfo 160000 <commit> "<submodule>" > > > > which will accept this suggestion. > > The last few lines above will no longer be part of the output once > en/merge-tree is merged; see commit a4040cfa35 (merge-ort: remove > command-line-centric submodule message from merge-ort, 2022-06-18). ack > > CONFLICT (submodule): Merge conflict in <submodule> > > Recursive merging with submodules is currently not supported. > > To manually complete the merge: > > - go to submodule (<submodule>), and either update the submodule to a possible commit above or merge commit <commit> > > Again, I'm hesitant to treat the suggestions from merge_submodule() as > the only possibilities. For example, perhaps the user will want to > pick a commit that contains one of the ones found by merge_submodule() > in its history -- perhaps because the newer commit they want to select > contains an important bugfix for an issue not discovered at the time > of the merge in the submodule. > > Also, I'm worried your sentence may be easy to misunderstand, due to > it being ambiguous whether "merge" is a verb or an adjective. More > precisely, your sentence could be read as "update the submodule to a > possible commit above, or update the submodule to merge commit > <commit>" and then users say, "But <commit> isn't a merge commit; why > does this message claim it is? Do I just update the submodule to that > commit anyway?" Or perhaps users just update it to <commit> without > even checking to find out that it's not a merge commit, with the > deleterious consequences of missing all the important changes > currently found in the submodule. I agree this sentence can be misinterpreted. I also think the main reason my current message is unclear is because there is not enough context for the user to understand why the merge failed. In a standard merge, the reason in "CONFLICT (<reason>)" provides context, whereas in this case, "CONFLICT (submodule)" only tells the user that the submodule is conflicted in some way. The user is unaware that we attempted to fast-forward the submodule, failed, and now require the user to either update the commit or merge the commit. How does this rewording sound? Automatic merge failed; recursive merging with submodules currently only supports fast-forward merges. For each conflicted submodule, if the current commit does not reflect the desired state, either update or merge the commit. This can be accomplished with the following steps: - go to submodule (<submodule>), and either update the commit or merge commit <commit> > > - come back to superproject, and `git add <submodule>` to record the above merge > > "...to record the above merge or update"? will... "update" haha > > If git detects multiple possible merge resolutions, the following is printed: > > > > -------- > > > > Failed to merge submodule sub, but multiple possible merges exist: > > <commit> Merge branch '<branch1>' into <branch2> > > <commit> Merge branch '<branch1>' into <branch3> > > > > CONFLICT (submodule): Merge conflict in <submodule> > > Recursive merging with submodules is currently not supported. > > To manually complete the merge: > > - go to submodule (<submodule>), and either update the submodule to a possible commit above or merge commit <commit> > > Same issues as I mentioned above for the single-merge-resolution-found case. > ditto > > - come back to superproject, and `git add <submodule>` to record the above merge > > "merge or update", right? ditto > > +ret: > > + if (!ret) { > > + if (!csub) { > > + CALLOC_ARRAY(csub, 1); > > + } > > + csub_item.oid = *result; > > Shouldn't this be "*b" rather than "*result"? Yes it should > > Also, are we risking telling the user to "merge commit > 0000000000000000000000000000000000000000" from the submodule, given > some of the early exits that you redirected to come through here? You are correct, but I'm not quite sure what should happen in this case. What does it mean for either o, a, or b to be null? Did a submodule get deleted on one side? > > +void print_submodule_conflict_suggestion(struct conflicted_submodule_list *csub) { > > Maybe just make this function be static? It should be static now that this won't be called in merge-recursive > > + find_unique_abbrev(&id, DEFAULT_ABBREV)); > > Shouldn't this be repo_find_unique_abbrev(), and in particular with > the submodule repository being passed to it? Or perhaps using the > format_commit() function, since merge_submodule() is already using it > for the earlier submodule-related messages? It should and I will go with the format_commit() option so I don't have to pass subrepo information into the print function. > > void merge_switch_to_result(struct merge_options *opt, > > struct tree *head, > > struct merge_result *result, > > @@ -4324,6 +4367,8 @@ void merge_switch_to_result(struct merge_options *opt, > > } > > string_list_clear(&olist, 0); > > > > + print_submodule_conflict_suggestion(opt->conflicted_submodules); > > + > > /* Also include needed rename limit adjustment now */ > > diff_warn_rename_limit("merge.renamelimit", > > opti->renames.needed_limit, 0); > > diff --git a/merge-recursive.c b/merge-recursive.c > > index fd1bbde061..311cc37886 100644 > > --- a/merge-recursive.c > > +++ b/merge-recursive.c > > Is it worth updating merge-recursive.c here? I'd rather only give it > security fix updates until we delete it. Ah wasn't aware that was the status of merge-recursive. Will delete. > > diff --git a/merge-recursive.h b/merge-recursive.h > > index b88000e3c2..b615955fb7 100644 > > --- a/merge-recursive.h > > +++ b/merge-recursive.h > > @@ -9,6 +9,7 @@ struct object_id; > > struct repository; > > struct tree; > > > > +struct conflicted_submodule_list; > > struct merge_options_internal; > > struct merge_options { > > struct repository *repo; > > @@ -51,6 +52,21 @@ struct merge_options { > > > > /* internal fields used by the implementation */ > > struct merge_options_internal *priv; > > + > > + /* field that holds submodule conflict information */ > > + struct conflicted_submodule_list *conflicted_submodules; > > I think this should be added to merge_options_internal rather than to > merge_options. There's no need for an API caller to make use of > these. > > Also, if a clear need arises later for API callers to make use of this > information, then it should be part of merge_result for merge-ort.c, > not part of merge_options. ack > > +}; > > + > > +struct conflicted_submodule_item { > > + struct object_id oid; > > + char *path; > > + int resolution_exists; > > +}; > > + > > +struct conflicted_submodule_list { > > + struct conflicted_submodule_item *items; > > + size_t nr; > > + size_t alloc; > > }; > > Similarly, I think these should be defined in merge-ort.c (and maybe > also merge-recursive.c) near struct merge_options_internal. ack > > void init_merge_options(struct merge_options *opt, struct repository *repo); > > @@ -122,4 +138,6 @@ int merge_recursive_generic(struct merge_options *opt, > > const struct object_id **merge_bases, > > struct commit **result); > > > > +void print_submodule_conflict_suggestion(struct conflicted_submodule_list *csub); > > and I think there's no reason to put this in the header file; it > should just be a static function in merge-ort.c. (And, if we really > want to update merge-recursive.c, then copy the function over there. > *Nothing* in merge-ort.c should call a function in merge-recursive.c > and similarly nothing in merge-recursive.c should call any function in > merge-ort.c. Yes, that implies some duplication. There is a fair > amount already and we've discussed it, and chosen against creating a > shared module as well. I want merge-recursive to not be broken by > merge-ort updates; it should remain stable until the day we finally > get to nuke it. I'm personally of the opinion that merge-recursive > should only get security fixes at this point, though I haven't pinged > to see if other folks agree with that point of view yet or not. I'm > not wasting my time fixing/improving it, though...) Good to know going forward I should only update merge-ort (unless for security fixes). I'll keep this in mind refactoring my patch. > Despite my many nitpicks and whatnot, it looks like your change will > make things nicer for the user, and I think your patch is coming along > nicely. There are some things to fix up, but the overall direction > seems good. Thank you for all of the helpful feedback!
Hi! I have a suggestion for the output text; I haven't looked closely at the code changes. Calvin Wan <calvinwan@google.com> writes: > Changes since v2: > [...] > Changes since v1: > [...] I notice that this is all above the "---", i.e. this becomes part of the commit message when "git am"-ed. Intentional? > If git detects a possible merge resolution, the following is printed: > > -------- > > Failed to merge submodule sub, but a possible merge resolution exists: > <commit> Merge branch '<branch1>' into <branch2> > > > If this is correct simply add it to the index for example > by using: > > git update-index --cacheinfo 160000 <commit> "<submodule>" > > which will accept this suggestion. > > CONFLICT (submodule): Merge conflict in <submodule> > Recursive merging with submodules is currently not supported. > To manually complete the merge: > - go to submodule (<submodule>), and either update the submodule to a possible commit above or merge commit <commit> > - come back to superproject, and `git add <submodule>` to record the above merge > - resolve any other conflicts in the superproject > - commit the resulting index in the superproject > Automatic merge failed; fix conflicts and then commit the result. > > -------- I'm hesitant to recommend a plumbing command like "git update-index" to the user, especially if the user is one who needs help resolving a submodule merge conflict. I also believe this would be the first time we recommend "git update-index". To do this using only porcelain commands, maybe: git -C <submodule> checkout <commit> && git add <submodule> (Though this might need to be broken up into two commands because I'm not sure if we ever include "&&" in a help message. I'm guessing we don't for portability reasons?) > If git detects multiple possible merge resolutions, the following is printed: > > -------- > > Failed to merge submodule sub, but multiple possible merges exist: > <commit> Merge branch '<branch1>' into <branch2> > <commit> Merge branch '<branch1>' into <branch3> > > CONFLICT (submodule): Merge conflict in <submodule> > Recursive merging with submodules is currently not supported. > To manually complete the merge: > - go to submodule (<submodule>), and either update the submodule to a possible commit above or merge commit <commit> > - come back to superproject, and `git add <submodule>` to record the above merge > - resolve any other conflicts in the superproject > - commit the resulting index in the superproject > Automatic merge failed; fix conflicts and then commit the result. > > ------- For consistency, perhaps include the "here's how to use the suggestion" instructions here as well?
Glen Choo <chooglen@google.com> writes: > Hi! I have a suggestion for the output text; I haven't looked closely at > the code changes. > >> If git detects a possible merge resolution, the following is printed: >> >> -------- >> >> Failed to merge submodule sub, but a possible merge resolution exists: >> <commit> Merge branch '<branch1>' into <branch2> >> >> >> If this is correct simply add it to the index for example >> by using: >> >> git update-index --cacheinfo 160000 <commit> "<submodule>" >> >> which will accept this suggestion. >> >> CONFLICT (submodule): Merge conflict in <submodule> >> Recursive merging with submodules is currently not supported. >> To manually complete the merge: >> - go to submodule (<submodule>), and either update the submodule to a possible commit above or merge commit <commit> >> - come back to superproject, and `git add <submodule>` to record the above merge >> - resolve any other conflicts in the superproject >> - commit the resulting index in the superproject >> Automatic merge failed; fix conflicts and then commit the result. >> >> -------- > > I'm hesitant to recommend a plumbing command like "git update-index" to > the user, especially if the user is one who needs help resolving a > submodule merge conflict. I also believe this would be the first time we > recommend "git update-index". > > To do this using only porcelain commands, maybe: > > git -C <submodule> checkout <commit> && > git add <submodule> > > (Though this might need to be broken up into two commands because I'm > not sure if we ever include "&&" in a help message. I'm guessing we > don't for portability reasons?) Gah, ignore everything I said here. I should have read the thread closer: - The update-index suggestion didn't come from you; it had already existed prior to this series. - Both Philppe and Elijah have already suggested the exact same thing.
> I notice that this is all above the "---", i.e. this becomes part of the > commit message when "git am"-ed. Intentional? Good catch. Not intentional at all. On Thu, Jun 30, 2022 at 1:35 PM Glen Choo <chooglen@google.com> wrote: > > Hi! I have a suggestion for the output text; I haven't looked closely at > the code changes. > > Calvin Wan <calvinwan@google.com> writes: > > > Changes since v2: > > [...] > > Changes since v1: > > [...] > > I notice that this is all above the "---", i.e. this becomes part of the > commit message when "git am"-ed. Intentional? > > > If git detects a possible merge resolution, the following is printed: > > > > -------- > > > > Failed to merge submodule sub, but a possible merge resolution exists: > > <commit> Merge branch '<branch1>' into <branch2> > > > > > > If this is correct simply add it to the index for example > > by using: > > > > git update-index --cacheinfo 160000 <commit> "<submodule>" > > > > which will accept this suggestion. > > > > CONFLICT (submodule): Merge conflict in <submodule> > > Recursive merging with submodules is currently not supported. > > To manually complete the merge: > > - go to submodule (<submodule>), and either update the submodule to a possible commit above or merge commit <commit> > > - come back to superproject, and `git add <submodule>` to record the above merge > > - resolve any other conflicts in the superproject > > - commit the resulting index in the superproject > > Automatic merge failed; fix conflicts and then commit the result. > > > > -------- > > I'm hesitant to recommend a plumbing command like "git update-index" to > the user, especially if the user is one who needs help resolving a > submodule merge conflict. I also believe this would be the first time we > recommend "git update-index". > > To do this using only porcelain commands, maybe: > > git -C <submodule> checkout <commit> && > git add <submodule> > > (Though this might need to be broken up into two commands because I'm > not sure if we ever include "&&" in a help message. I'm guessing we > don't for portability reasons?) > > > If git detects multiple possible merge resolutions, the following is printed: > > > > -------- > > > > Failed to merge submodule sub, but multiple possible merges exist: > > <commit> Merge branch '<branch1>' into <branch2> > > <commit> Merge branch '<branch1>' into <branch3> > > > > CONFLICT (submodule): Merge conflict in <submodule> > > Recursive merging with submodules is currently not supported. > > To manually complete the merge: > > - go to submodule (<submodule>), and either update the submodule to a possible commit above or merge commit <commit> > > - come back to superproject, and `git add <submodule>` to record the above merge > > - resolve any other conflicts in the superproject > > - commit the resulting index in the superproject > > Automatic merge failed; fix conflicts and then commit the result. > > > > ------- > > For consistency, perhaps include the "here's how to use the suggestion" > instructions here as well?
On Thu, Jun 30, 2022 at 12:48 PM Calvin Wan <calvinwan@google.com> wrote: > > > > Failed to merge submodule <submodule> > > > CONFLICT (submodule): Merge conflict in <submodule> > > > Automatic merge failed; recursive merging with submodules is currently > > > not supported. To manually complete the merge: > > > - go to submodule (<submodule>), and merge commit <commit> > > > > I'm still a little unsure on this. The merge_submodule() logic we > > have may not have detected a merge commit that merges the two branches > > together, but that doesn't automatically imply a new merge must be > > created in the submodule to resolve this conflict. There might be > > various reasons that other existing commits in the submodule could be > > used as the "correct" update. > > > > Perhaps that is uncommon enough to not be worth mentioning; I'm just a > > little hesitant to treat the merge_submodule() logic as robust and > > finding the only choices users might want to use. If we do keep the > > wording as-is, it might be nice to at least discuss in the commit > > message why we chose that and ignored the or-update-submodule option > > in this case. > > You make a good point about merge_submodule() possibly not finding all > of the right choices -- I'll add back the or-update-submodule option > back. > > > > If this is correct simply add it to the index for example > > > by using: > > > > > > git update-index --cacheinfo 160000 <commit> "<submodule>" > > > > > > which will accept this suggestion. > > > > The last few lines above will no longer be part of the output once > > en/merge-tree is merged; see commit a4040cfa35 (merge-ort: remove > > command-line-centric submodule message from merge-ort, 2022-06-18). > > ack > > > > CONFLICT (submodule): Merge conflict in <submodule> > > > Recursive merging with submodules is currently not supported. > > > To manually complete the merge: > > > - go to submodule (<submodule>), and either update the submodule to a possible commit above or merge commit <commit> > > > > Again, I'm hesitant to treat the suggestions from merge_submodule() as > > the only possibilities. For example, perhaps the user will want to > > pick a commit that contains one of the ones found by merge_submodule() > > in its history -- perhaps because the newer commit they want to select > > contains an important bugfix for an issue not discovered at the time > > of the merge in the submodule. > > > > Also, I'm worried your sentence may be easy to misunderstand, due to > > it being ambiguous whether "merge" is a verb or an adjective. More > > precisely, your sentence could be read as "update the submodule to a > > possible commit above, or update the submodule to merge commit > > <commit>" and then users say, "But <commit> isn't a merge commit; why > > does this message claim it is? Do I just update the submodule to that > > commit anyway?" Or perhaps users just update it to <commit> without > > even checking to find out that it's not a merge commit, with the > > deleterious consequences of missing all the important changes > > currently found in the submodule. I probably should have mentioned that listing "merge" first and "update" second in your instructions at least avoids the ambiguity because it makes it clear that "merge" is a verb that way: - go to submodule (<submodule>), and either merge commit <commit> or update the submodule to a possible commit above > I agree this sentence can be misinterpreted. I also think the main > reason my current message is unclear is because there is not enough > context for the user to understand why the merge failed. In a standard > merge, the reason in "CONFLICT (<reason>)" provides context, whereas in > this case, "CONFLICT (submodule)" only tells the user that the submodule > is conflicted in some way. The user is unaware that we attempted to > fast-forward the submodule, failed, and now require the user to either > update the commit or merge the commit. How does this rewording sound? Do they need to know what we attempted? I'm not sure why that matters, beyond maybe telling them that actual automatic merging of submodules is currently only done by Git's merging machinery in very trivial cases. All that should really matter at this point is that there was a submodule modified on both sides of history, and we need them to handle the merge of the submodule. > Automatic merge failed; recursive merging with submodules currently > only supports fast-forward merges. For each conflicted submodule, if I'd rather substitute "trivial cases" instead of "fast-forward merges". For example, we handle deletions on both sides, it's just that that's done before it ever gets to merge_submodule(). And if we add more types of special cases beyond fast-forwards at some point, it'd be nice to not need to update this text. > the current commit does not reflect the desired state, either update or > merge the commit. This can be accomplished with the following steps: Maybe replace the last two sentences with "Please manually handle the merging of each conflicted submodule; this can be accomplished with the following steps:" > - go to submodule (<submodule>), and either update the commit or merge commit <commit> What would you think of switching this to - go to submodule <submodule>, and either merge commit <commit> or update to an existing commit which has merged those changes such as one of the ones listed above > > > - come back to superproject, and `git add <submodule>` to record the above merge > > > > "...to record the above merge or update"? > > will... "update" haha :-) > > > If git detects multiple possible merge resolutions, the following is printed: > > > > > > -------- > > > > > > Failed to merge submodule sub, but multiple possible merges exist: > > > <commit> Merge branch '<branch1>' into <branch2> > > > <commit> Merge branch '<branch1>' into <branch3> > > > > > > CONFLICT (submodule): Merge conflict in <submodule> > > > Recursive merging with submodules is currently not supported. > > > To manually complete the merge: > > > - go to submodule (<submodule>), and either update the submodule to a possible commit above or merge commit <commit> > > > > Same issues as I mentioned above for the single-merge-resolution-found case. > > > > ditto > > > > - come back to superproject, and `git add <submodule>` to record the above merge > > > > "merge or update", right? > > ditto > > > > +ret: > > > + if (!ret) { > > > + if (!csub) { > > > + CALLOC_ARRAY(csub, 1); > > > + } > > > + csub_item.oid = *result; > > > > Shouldn't this be "*b" rather than "*result"? > > Yes it should > > > > > Also, are we risking telling the user to "merge commit > > 0000000000000000000000000000000000000000" from the submodule, given > > some of the early exits that you redirected to come through here? > > You are correct, but I'm not quite sure what should happen in this case. What does it mean for either o, a, or b to be null? Did a submodule get deleted on one side? o is the version in the merge-base, a is the version from the first parent (thus the submodule version stored within HEAD), and b is the version from the second parent (thus the submodule version stored within the main module's MERGE_HEAD commit). We can't have all three be null, because that would just mean there was never a submodule at this path. We can't have two of the three be null, because that would either be deleted on both sides, or added on one side, and those cases are trivially resolvable elsewhere in the merge machinery and there's no need to call merge_submodule(). If o is null, the submodule didn't exist in the merge base. So it must be added on both sides (and the two sides have to have different submodule commits, or the merge of the submodule would be trivially handle-able elsewhere). If a is null, it didn't exist in HEAD, meaning it was deleted on our side of history. (And b has to be different than o, or else this would be trivially resolvable as a deletion.) If b is null, then it's similar to the above case, but the submodule was deleted on the other side of history that we are trying to merge into HEAD rather than on HEAD's side of history. However, now that I've said this, I took another look through the code. I think this actually isn't relevant right now. merge_submodule() is only called from handle_content_merge(), which in turn is only called from two places: (1) process_renames(), and (2) the filemask >= 6 section of process_entry(). The process_renames() cases, since we can't detect renames involving submodules, can't involve a case with a null oid for a submodule. And the filemask >= 6 implies that only o could have a null hash. That means the checks for a and b being null oids within merge_submodule will never trigger. And we don't actually run the risk of telling users to merge the all null commit. Any time we have a modify/delete issue with submodules, we'll just end up at the codepath that reports "CONFLICT (modify/delete): ...", and which doesn't mention anything about the path being a submodule, but it really doesn't need to; the text is accurate regardless. > > > +void print_submodule_conflict_suggestion(struct conflicted_submodule_list *csub) { > > > > Maybe just make this function be static? > > It should be static now that this won't be called in merge-recursive > > > > + find_unique_abbrev(&id, DEFAULT_ABBREV)); > > > > Shouldn't this be repo_find_unique_abbrev(), and in particular with > > the submodule repository being passed to it? Or perhaps using the > > format_commit() function, since merge_submodule() is already using it > > for the earlier submodule-related messages? > > It should and I will go with the format_commit() option so I don't have to pass subrepo information into the print function. > > > > void merge_switch_to_result(struct merge_options *opt, > > > struct tree *head, > > > struct merge_result *result, > > > @@ -4324,6 +4367,8 @@ void merge_switch_to_result(struct merge_options *opt, > > > } > > > string_list_clear(&olist, 0); > > > > > > + print_submodule_conflict_suggestion(opt->conflicted_submodules); > > > + > > > /* Also include needed rename limit adjustment now */ > > > diff_warn_rename_limit("merge.renamelimit", > > > opti->renames.needed_limit, 0); > > > diff --git a/merge-recursive.c b/merge-recursive.c > > > index fd1bbde061..311cc37886 100644 > > > --- a/merge-recursive.c > > > +++ b/merge-recursive.c > > > > Is it worth updating merge-recursive.c here? I'd rather only give it > > security fix updates until we delete it. > > Ah wasn't aware that was the status of merge-recursive. Will delete. > > > > diff --git a/merge-recursive.h b/merge-recursive.h > > > index b88000e3c2..b615955fb7 100644 > > > --- a/merge-recursive.h > > > +++ b/merge-recursive.h > > > @@ -9,6 +9,7 @@ struct object_id; > > > struct repository; > > > struct tree; > > > > > > +struct conflicted_submodule_list; > > > struct merge_options_internal; > > > struct merge_options { > > > struct repository *repo; > > > @@ -51,6 +52,21 @@ struct merge_options { > > > > > > /* internal fields used by the implementation */ > > > struct merge_options_internal *priv; > > > + > > > + /* field that holds submodule conflict information */ > > > + struct conflicted_submodule_list *conflicted_submodules; > > > > I think this should be added to merge_options_internal rather than to > > merge_options. There's no need for an API caller to make use of > > these. > > > > Also, if a clear need arises later for API callers to make use of this > > information, then it should be part of merge_result for merge-ort.c, > > not part of merge_options. > > ack > > > > +}; > > > + > > > +struct conflicted_submodule_item { > > > + struct object_id oid; > > > + char *path; > > > + int resolution_exists; > > > +}; > > > + > > > +struct conflicted_submodule_list { > > > + struct conflicted_submodule_item *items; > > > + size_t nr; > > > + size_t alloc; > > > }; > > > > Similarly, I think these should be defined in merge-ort.c (and maybe > > also merge-recursive.c) near struct merge_options_internal. > > ack > > > > void init_merge_options(struct merge_options *opt, struct repository *repo); > > > @@ -122,4 +138,6 @@ int merge_recursive_generic(struct merge_options *opt, > > > const struct object_id **merge_bases, > > > struct commit **result); > > > > > > +void print_submodule_conflict_suggestion(struct conflicted_submodule_list *csub); > > > > and I think there's no reason to put this in the header file; it > > should just be a static function in merge-ort.c. (And, if we really > > want to update merge-recursive.c, then copy the function over there. > > *Nothing* in merge-ort.c should call a function in merge-recursive.c > > and similarly nothing in merge-recursive.c should call any function in > > merge-ort.c. Yes, that implies some duplication. There is a fair > > amount already and we've discussed it, and chosen against creating a > > shared module as well. I want merge-recursive to not be broken by > > merge-ort updates; it should remain stable until the day we finally > > get to nuke it. I'm personally of the opinion that merge-recursive > > should only get security fixes at this point, though I haven't pinged > > to see if other folks agree with that point of view yet or not. I'm > > not wasting my time fixing/improving it, though...) > > Good to know going forward I should only update merge-ort (unless for security fixes). I'll keep this in mind refactoring my patch. > > > Despite my many nitpicks and whatnot, it looks like your change will > > make things nicer for the user, and I think your patch is coming along > > nicely. There are some things to fix up, but the overall direction > > seems good. > > Thank you for all of the helpful feedback!
diff --git a/merge-ort.c b/merge-ort.c index 0d3f42592f..2d9f03ea8c 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -1610,6 +1610,9 @@ static int merge_submodule(struct merge_options *opt, struct commit *commit_o, *commit_a, *commit_b; int parent_count; struct object_array merges; + struct conflicted_submodule_list *csub = opt->conflicted_submodules; + struct conflicted_submodule_item csub_item; + int resolution_exists = 0; int i; int search = !opt->priv->call_depth; @@ -1619,17 +1622,17 @@ static int merge_submodule(struct merge_options *opt, /* we can not handle deletion conflicts */ if (is_null_oid(o)) - return 0; + goto ret; if (is_null_oid(a)) - return 0; + goto ret; if (is_null_oid(b)) - return 0; + goto ret; if (repo_submodule_init(&subrepo, opt->repo, path, null_oid())) { path_msg(opt, path, 0, _("Failed to merge submodule %s (not checked out)"), path); - return 0; + goto ret; } if (!(commit_o = lookup_commit_reference(&subrepo, o)) || @@ -1703,6 +1706,7 @@ static int merge_submodule(struct merge_options *opt, "which will accept this suggestion.\n"), oid_to_hex(&merges.objects[0].item->oid), path); strbuf_release(&sb); + resolution_exists = 1; break; default: for (i = 0; i < merges.nr; i++) @@ -1712,11 +1716,24 @@ static int merge_submodule(struct merge_options *opt, _("Failed to merge submodule %s, but multiple " "possible merges exist:\n%s"), path, sb.buf); strbuf_release(&sb); + resolution_exists = 1; } object_array_clear(&merges); cleanup: repo_clear(&subrepo); +ret: + if (!ret) { + if (!csub) { + CALLOC_ARRAY(csub, 1); + } + csub_item.oid = *result; + csub_item.path = xstrdup(path); + csub_item.resolution_exists = resolution_exists; + ALLOC_GROW(csub->items, csub->nr + 1, csub->alloc); + csub->items[csub->nr++] = csub_item; + opt->conflicted_submodules = csub; + } return ret; } @@ -4256,6 +4273,32 @@ static int record_conflicted_index_entries(struct merge_options *opt) return errs; } +void print_submodule_conflict_suggestion(struct conflicted_submodule_list *csub) { + if (csub && csub->nr > 0) { + int i; + printf(_("Recursive merging with submodules is currently not supported.\n" + "To manually complete the merge:\n")); + for (i = 0; i < csub->nr; i++) { + const struct object_id id = csub->items[i].oid; + if (csub->items[i].resolution_exists) + printf(_(" - go to submodule (%s), and either update the submodule " + "to a possible commit above or merge commit %s\n"), + csub->items[i].path, + find_unique_abbrev(&id, DEFAULT_ABBREV)); + else + printf(_(" - go to submodule (%s), and merge commit %s\n"), + csub->items[i].path, + find_unique_abbrev(&id, DEFAULT_ABBREV)); + } + printf(_(" - come back to superproject, and `git add")); + for (i = 0; i < csub->nr; i++) + printf(_(" %s"), csub->items[i].path); + printf(_("` to record the above merge \n" + " - resolve any other conflicts in the superproject\n" + " - commit the resulting index in the superproject\n")); + } +} + void merge_switch_to_result(struct merge_options *opt, struct tree *head, struct merge_result *result, @@ -4324,6 +4367,8 @@ void merge_switch_to_result(struct merge_options *opt, } string_list_clear(&olist, 0); + print_submodule_conflict_suggestion(opt->conflicted_submodules); + /* Also include needed rename limit adjustment now */ diff_warn_rename_limit("merge.renamelimit", opti->renames.needed_limit, 0); diff --git a/merge-recursive.c b/merge-recursive.c index fd1bbde061..311cc37886 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1190,6 +1190,9 @@ static int merge_submodule(struct merge_options *opt, struct commit *commit_base, *commit_a, *commit_b; int parent_count; struct object_array merges; + struct conflicted_submodule_list *csub = opt->conflicted_submodules; + struct conflicted_submodule_item csub_item; + int resolution_exists = 0; int i; int search = !opt->priv->call_depth; @@ -1204,15 +1207,15 @@ static int merge_submodule(struct merge_options *opt, /* we can not handle deletion conflicts */ if (is_null_oid(base)) - return 0; + goto ret; if (is_null_oid(a)) - return 0; + goto ret; if (is_null_oid(b)) - return 0; + goto ret; if (repo_submodule_init(&subrepo, opt->repo, path, null_oid())) { output(opt, 1, _("Failed to merge submodule %s (not checked out)"), path); - return 0; + goto ret; } if (!(commit_base = lookup_commit_reference(&subrepo, base)) || @@ -1287,17 +1290,31 @@ static int merge_submodule(struct merge_options *opt, " git update-index --cacheinfo 160000 %s \"%s\"\n\n" "which will accept this suggestion.\n"), oid_to_hex(&merges.objects[0].item->oid), path); + resolution_exists = 1; break; default: output(opt, 1, _("Failed to merge submodule %s (multiple merges found)"), path); for (i = 0; i < merges.nr; i++) print_commit(&subrepo, (struct commit *) merges.objects[i].item); + resolution_exists = 1; } object_array_clear(&merges); cleanup: repo_clear(&subrepo); +ret: + if (!ret) { + if (!csub) { + CALLOC_ARRAY(csub, 1); + } + csub_item.oid = *result; + csub_item.path = xstrdup(path); + csub_item.resolution_exists = resolution_exists; + ALLOC_GROW(csub->items, csub->nr + 1, csub->alloc); + csub->items[csub->nr++] = csub_item; + opt->conflicted_submodules = csub; + } return ret; } @@ -3736,6 +3753,7 @@ static void merge_finalize(struct merge_options *opt) flush_output(opt); if (!opt->priv->call_depth && opt->buffer_output < 2) strbuf_release(&opt->obuf); + print_submodule_conflict_suggestion(opt->conflicted_submodules); if (show(opt, 2)) diff_warn_rename_limit("merge.renamelimit", opt->priv->needed_rename_limit, 0); diff --git a/merge-recursive.h b/merge-recursive.h index b88000e3c2..b615955fb7 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -9,6 +9,7 @@ struct object_id; struct repository; struct tree; +struct conflicted_submodule_list; struct merge_options_internal; struct merge_options { struct repository *repo; @@ -51,6 +52,21 @@ struct merge_options { /* internal fields used by the implementation */ struct merge_options_internal *priv; + + /* field that holds submodule conflict information */ + struct conflicted_submodule_list *conflicted_submodules; +}; + +struct conflicted_submodule_item { + struct object_id oid; + char *path; + int resolution_exists; +}; + +struct conflicted_submodule_list { + struct conflicted_submodule_item *items; + size_t nr; + size_t alloc; }; void init_merge_options(struct merge_options *opt, struct repository *repo); @@ -122,4 +138,6 @@ int merge_recursive_generic(struct merge_options *opt, const struct object_id **merge_bases, struct commit **result); +void print_submodule_conflict_suggestion(struct conflicted_submodule_list *csub); + #endif diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh index 178413c22f..f0a31c3a61 100755 --- a/t/t6437-submodule-merge.sh +++ b/t/t6437-submodule-merge.sh @@ -103,8 +103,28 @@ test_expect_success 'setup for merge search' ' echo "file-c" > file-c && git add file-c && git commit -m "sub-c") && - git commit -a -m "c" && + git commit -a -m "c") +' + +test_expect_success 'merging should conflict for non fast-forward' ' + (cd merge-search && + git checkout -b test-nonforward-a b && + (cd sub && + git rev-parse --short sub-b > ../expect) && + if test "$GIT_TEST_MERGE_ALGORITHM" = ort + then + test_must_fail git merge c >actual + else + test_must_fail git merge c 2> actual + fi && + grep $(cat expect) actual > /dev/null && + sub_expect="go to submodule (sub), and merge commit $(git -C sub rev-parse --short sub-b)" && + grep "$sub_expect" actual && + git reset --hard) +' +test_expect_success 'finish setup for merge-search' ' + (cd merge-search && git checkout -b d a && (cd sub && git checkout -b sub-d sub-b && @@ -129,9 +149,9 @@ test_expect_success 'merge with one side as a fast-forward of the other' ' test_cmp expect actual) ' -test_expect_success 'merging should conflict for non fast-forward' ' +test_expect_success 'merging should conflict for non fast-forward (resolution exists)' ' (cd merge-search && - git checkout -b test-nonforward b && + git checkout -b test-nonforward-b b && (cd sub && git rev-parse sub-d > ../expect) && if test "$GIT_TEST_MERGE_ALGORITHM" = ort @@ -141,6 +161,9 @@ 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 && + sub_expect="go to submodule (sub), and either update the submodule to a \ +possible commit above or merge commit $(git -C sub rev-parse --short sub-b)" && + grep "$sub_expect" actual && git reset --hard) ' @@ -167,6 +190,9 @@ test_expect_success 'merging should fail for ambiguous common parent' ' fi && grep $(cat expect1) actual > /dev/null && grep $(cat expect2) actual > /dev/null && + sub_expect="go to submodule (sub), and either update the submodule to a \ +possible commit above or merge commit $(git -C sub rev-parse --short sub-b)" && + grep "$sub_expect" actual && git reset --hard) ' @@ -205,7 +231,9 @@ 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 && + sub_expect="go to submodule (sub), and merge commit $(git -C sub rev-parse --short sub-a)" && + grep "$sub_expect" actual) ' diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh index 8e32f19007..21c8b4e97c 100755 --- a/t/t7402-submodule-rebase.sh +++ b/t/t7402-submodule-rebase.sh @@ -104,7 +104,7 @@ test_expect_success 'rebasing submodule that should conflict' ' test_tick && git commit -m fourth && - test_must_fail git rebase --onto HEAD^^ HEAD^ HEAD^0 && + test_must_fail git rebase --onto HEAD^^ HEAD^ HEAD^0 >actual_output && git ls-files -s submodule >actual && ( cd submodule && @@ -112,7 +112,9 @@ test_expect_success 'rebasing submodule that should conflict' ' echo "160000 $(git rev-parse HEAD^^) 2 submodule" && echo "160000 $(git rev-parse HEAD) 3 submodule" ) >expect && - test_cmp expect actual + test_cmp expect actual && + sub_expect="go to submodule (submodule), and merge commit $(git -C submodule rev-parse --short HEAD^^)" && + grep "$sub_expect" actual_output ' test_done
When attempting to merge in a superproject with conflicting submodule pointers that cannot be fast-forwarded, the merge fails and git prints the following error: Failed to merge submodule <submodule> 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. merge submodules or update submodules to an already existing commit that reflects the merge 2. add submodules changes to the superproject 3. finish merging 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 match the steps above. If git does not detect a merge resolution, the following is printed: -------- Failed to merge submodule <submodule> CONFLICT (submodule): Merge conflict in <submodule> Automatic merge failed; recursive merging with submodules is currently not supported. To manually complete the merge: - go to submodule (<submodule>), and merge commit <commit> - come back to superproject, and `git add <submodule>` to record the above merge - resolve any other conflicts in the superproject - commit the resulting index in the superproject Automatic merge failed; fix conflicts and then commit the result. -------- If git detects a possible merge resolution, the following is printed: -------- Failed to merge submodule sub, but a possible merge resolution exists: <commit> Merge branch '<branch1>' into <branch2> If this is correct simply add it to the index for example by using: git update-index --cacheinfo 160000 <commit> "<submodule>" which will accept this suggestion. CONFLICT (submodule): Merge conflict in <submodule> Recursive merging with submodules is currently not supported. To manually complete the merge: - go to submodule (<submodule>), and either update the submodule to a possible commit above or merge commit <commit> - come back to superproject, and `git add <submodule>` to record the above merge - resolve any other conflicts in the superproject - commit the resulting index in the superproject Automatic merge failed; fix conflicts and then commit the result. -------- If git detects multiple possible merge resolutions, the following is printed: -------- Failed to merge submodule sub, but multiple possible merges exist: <commit> Merge branch '<branch1>' into <branch2> <commit> Merge branch '<branch1>' into <branch3> CONFLICT (submodule): Merge conflict in <submodule> Recursive merging with submodules is currently not supported. To manually complete the merge: - go to submodule (<submodule>), and either update the submodule to a possible commit above or merge commit <commit> - come back to superproject, and `git add <submodule>` to record the above merge - resolve any other conflicts in the superproject - commit the resulting index in the superproject Automatic merge failed; fix conflicts and then commit the result. ------- Changes since v2: There are three major changes in this patch thanks to all the helpful feedback! I have moved the print function from builtin/merge.c to merge-ort.c and added it to merge_finalize() in merge-recursive.c and merge_switch_to_result() in merge-ort.c. This allows other merge machinery commands besides merge to print submodule conflict advice. I have moved the check for submodule conflicts from process_entry() to merge_submodule(). This also required removing the early returns and instead going to my submodule conflict check allowing us to pass back information on whether git detected a possible merge resolution or not. I have also updated the error message to better reflect the merge status. Specifically, if git detects a possible merge resolution, the error message now also suggest updating to one of those resolutions. Other small changes: - Moved fields that hold submodule conflict information to new object - Shortened printed commit id to DEFAULT_ABBREV - Added a test to t6437-submodule-merge.sh for merges with no resolution existence - Modified a test in t7402-submodule-rebase to show error message prints in other parts of the merge machinery Changes since v1: - Removed advice to abort merge - Error message updated to contain more commit/submodule information Signed-off-by: Calvin Wan <calvinwan@google.com> --- merge-ort.c | 53 ++++++++++++++++++++++++++++++++++--- merge-recursive.c | 26 +++++++++++++++--- merge-recursive.h | 18 +++++++++++++ t/t6437-submodule-merge.sh | 36 ++++++++++++++++++++++--- t/t7402-submodule-rebase.sh | 6 +++-- 5 files changed, 125 insertions(+), 14 deletions(-) base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085