| Message ID | 20220726210020.3397249-1-calvinwan@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v6] submodule merge: update conflict error message | expand |
On Tue, Jul 26, 2022 at 2:00 PM Calvin Wan <calvinwan@google.com> wrote: > > When attempting to merge in a superproject with conflicting submodule > pointers that cannot be fast-forwarded or trivially resolved, the merge > fails and git prints an error message that accurately describes the micro nit: s/git/Git/ (feel free to ignore since you're already at v6) > failure, but does not provide steps for the user to resolve the error. > > 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 provide steps to resolve submodule merge > conflict. Future work could involve adding an advice flag to the > message. Although the message is long, it also has the id of the > commit that needs to be merged, which could be useful information > for the user. Well explained. One very minor suggestion: perhaps change "id of the commit" to "id of the submodule commit" just to make it slightly clearer that this information would take work for the user to discover on their own? (When I first read it, I was thinking, "but they have the commit, it's what they passed to merge", before I realized my error.) > Signed-off-by: Calvin Wan <calvinwan@google.com> > --- > > I'm a little unsure on the code style for the `git add` section of > the error message. It works, but is tricky for a code reviewer > to decipher the formatting. I don't think it should affect > translation since the text is only a git command and folder names. > > Changes since v5: > - Fixed memory leak reported by Ævar > - Fixed error message formatting and addded TRANSLATOR tags > - Removed "resolution exists" > > merge-ort.c | 61 +++++++++++++++++++++++++++++++++++++ > t/t6437-submodule-merge.sh | 38 +++++++++++++++++++---- > t/t7402-submodule-rebase.sh | 9 ++++-- > 3 files changed, 100 insertions(+), 8 deletions(-) > > diff --git a/merge-ort.c b/merge-ort.c > index 01f150ef3b..147be0ce31 100644 > --- a/merge-ort.c > +++ b/merge-ort.c > @@ -387,6 +387,9 @@ struct merge_options_internal { > > /* call_depth: recursion level counter for merging merge bases */ > int call_depth; > + > + /* field that holds submodule conflict information */ > + struct string_list conflicted_submodules; > }; > > struct version_info { > @@ -686,6 +689,8 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti, > > mem_pool_discard(&opti->pool, 0); > > + string_list_clear(&opti->conflicted_submodules, 1); > + > /* Clean out callback_data as well. */ > FREE_AND_NULL(renames->callback_data); > renames->callback_data_nr = renames->callback_data_alloc = 0; > @@ -1849,6 +1854,17 @@ static int merge_submodule(struct merge_options *opt, Sorry for not catching this in an earlier round, but merge_submodule() has four "return 0" cases, for particular types of conflicts. Those should probably be switched to "goto cleanup" or something like that, so that these messages you are adding are also provided if one of those conflict cases are hit. > object_array_clear(&merges); > cleanup: > + if (!ret) { And here's another item I have to apologize for not catching in an earlier round. We should also require !opt->priv->call_depth as well in this if-condition. If merging of merge bases produced a submodule conflict, but the outer merge involves two sides that resolved the inner conflict the exact same way, then there's no conflict at the outer level and nothing for the user to resolve. If users don't have any conflicts to resolve, we don't want to print messages telling them how to resolve their non-existent conflicts. And if there is still a conflict in the submodule for the outer merge as well as in the recursive merge(s), we don't want to list the module twice (or more) when we tell the user to fix conflicts in their submodules (especially since that means we'd be telling them to merge multiple different commits for the single submodule, which could get confusing). > + struct string_list *csub = &opt->priv->conflicted_submodules; > + char *util; > + const char *abbrev; > + > + abbrev = repo_find_unique_abbrev(&subrepo, b, DEFAULT_ABBREV); > + util = xstrdup(abbrev); > + > + string_list_append(csub, path)->util = util; > + } > + > repo_clear(&subrepo); > return ret; > } > @@ -4412,6 +4428,47 @@ static int record_conflicted_index_entries(struct merge_options *opt) > return errs; > } > > +static void print_submodule_conflict_suggestion(struct string_list *csub) { > + int first = 1; > + if (csub->nr > 0) { > + struct string_list_item *item; > + printf(_("Recursive merging with submodules currently only supports trivial cases.\n" > + "Please manually handle the merging of each conflicted submodule.\n" > + "This can be accomplished with the following steps:\n")); > + > + for_each_string_list_item(item, csub) { > + const char *abbrev= item->util; Messed up indent here? > + /* > + * TRANSLATORS: This is a line of advice to resolve a merge conflict > + * in a submodule. The second argument is the abbreviated id of the > + * commit that needs to be merged. > + * E.g. - go to submodule (sub), and either merge commit abc1234" > + */ > + printf(_(" - go to submodule (%s), and either merge commit %s\n" > + " or update to an existing commit which has merged those changes\n"), Indent may be wrong here too, at least if you're trying to get the leftmost quote marks to align. (Maybe the non-alignment was intentional, it was just unclear because of the earlier strange indent.) > + item->string, abbrev); > + } > + printf(_(" - come back to superproject and run:\n\n")); > + for_each_string_list_item(item, csub) > + /* > + * TRANSLATORS: This is a line of a recommended `git add` command > + * with multiple lines of submodule folders. > + * E.g.: git add sub \ > + * sub2 \ > + * sub3 Why does such a message need to be translated? It's literal text the user should type, right? I'm not sure what a translator would do with the message other than regurgitate it. > + */ > + if (first) { > + printf(" git add %s", item->string); But if you did mean for there to be a translation and a TRANSLATORS note, then did you forget to translate it by calling _()? > + first = 0; > + } else { > + printf(" \\\n %s", item->string); > + } Can we put braces around this for_each_string_list_item() block? Or, as an alternative to the whole block, do you want to consider: strub strbuf tmp = STRBUF_INIT; strbuf_add_separated_string_list(&tmp, ' ', csub); printf(_(" git add %s"), tmp.buf); /* or maybe remove the translation; not sure what the point is */ strbuf_release(&tmp); ? It is likely easier to copy & paste, and might be understood by more users (I'm not sure how many are aware that command lines can use backslashes for line continuation), but on the negative side, if you have a lot of submodules it might make it harder to read. Even if you don't like space separated, though, you could still use this strategy by changing the second line to strbuf_add_separated_string_list(&tmp, " \\\n ", csub); > + printf(_("\n\n to record the above merge or update\n" > + " - resolve any other conflicts in the superproject\n" > + " - commit the resulting index in the superproject\n")); > + } > +} > + > void merge_display_update_messages(struct merge_options *opt, > int detailed, > struct merge_result *result) > @@ -4461,6 +4518,9 @@ void merge_display_update_messages(struct merge_options *opt, > } > string_list_clear(&olist, 0); > > + print_submodule_conflict_suggestion(&opti->conflicted_submodules); > + string_list_clear(&opti->conflicted_submodules, 1); > + It would be more consistent to have things allocated in merge_start() continue to be cleared out in clear_or_reinit_internal_opts(). This kind of breaks that pairing, and you're already making sure to clear it there, so I'd rather remove this duplicate string_list_clear() call. > /* Also include needed rename limit adjustment now */ > diff_warn_rename_limit("merge.renamelimit", > opti->renames.needed_limit, 0); > @@ -4657,6 +4717,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result) > trace2_region_enter("merge", "allocate/init", opt->repo); > if (opt->priv) { > clear_or_reinit_internal_opts(opt->priv, 1); > + string_list_init_dup(&opt->priv->conflicted_submodules); This works, but there is a minor optimization available here if you're interested (I understand if you're not since you're already at v6). Assuming you make the important opt->priv->call_depth fix, you can replace string_list_init_dup() with string_list_init_nodup() here. The paths aren't freed until clear_or_reinit_internal_opts() which (under the assumption previously stated) isn't called until merge_finalize(), which comes after the call to merge_display_update_messages(), which is where you use the data. (As repository paths are used all over merge-ort.c, the optimization to store them in one place (opt->priv->paths) and avoid duplicating them is used pretty heavily. It's more important for a lot of the other strmaps since they'll have a lot more paths in them, but it is kind of nice to use this optimization where possible.) > trace2_region_leave("merge", "allocate/init", opt->repo); > return; > } > diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh > index c253bf759a..84913525cf 100755 > --- a/t/t6437-submodule-merge.sh > +++ b/t/t6437-submodule-merge.sh > @@ -103,8 +103,25 @@ 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' ' > + test_when_finished "git -C merge-search reset --hard" && > + (cd merge-search && > + git checkout -b test-nonforward-a b && > + if test "$GIT_TEST_MERGE_ALGORITHM" = ort > + then > + test_must_fail git merge c >actual && > + sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" && > + grep "$sub_expect" actual > + else > + test_must_fail git merge c 2> actual > + fi) > +' > + > +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,14 +146,16 @@ 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 --short sub-d > ../expect) && > if test "$GIT_TEST_MERGE_ALGORITHM" = ort > then > - test_must_fail git merge c >actual > + test_must_fail git merge c >actual && > + sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" && > + grep "$sub_expect" actual > else > test_must_fail git merge c 2> actual > fi && > @@ -161,7 +180,9 @@ test_expect_success 'merging should fail for ambiguous common parent' ' > ) && > if test "$GIT_TEST_MERGE_ALGORITHM" = ort > then > - test_must_fail git merge c >actual > + test_must_fail git merge c >actual && > + sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" && > + grep "$sub_expect" actual > else > test_must_fail git merge c 2> actual > fi && > @@ -205,7 +226,12 @@ 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 && > + if test "$GIT_TEST_MERGE_ALGORITHM" = ort > + then > + sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-d)" && > + grep "$sub_expect" actual > + fi) > ' > > > diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh > index 8e32f19007..ebeca12a71 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,12 @@ 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 && > + if test "$GIT_TEST_MERGE_ALGORITHM" = ort > + then > + sub_expect="go to submodule (submodule), and either merge commit $(git -C submodule rev-parse --short HEAD^0)" && > + grep "$sub_expect" actual_output > + fi > ' > > test_done > > base-commit: 9dd64cb4d310986dd7b8ca7fff92f9b61e0bd21a > -- > 2.37.1.359.gd136c6c3e2-goog
On Tue, Jul 26 2022, Calvin Wan wrote: Aside from what Elijah pointed out already... > + > + /* field that holds submodule conflict information */ > + struct string_list conflicted_submodules; Looks good! > cleanup: > + if (!ret) { > + struct string_list *csub = &opt->priv->conflicted_submodules; > + char *util; > + const char *abbrev; > + > + abbrev = repo_find_unique_abbrev(&subrepo, b, DEFAULT_ABBREV); > + util = xstrdup(abbrev); > + > + string_list_append(csub, path)->util = util; Elijah pointed out that these don't need to be dup'd at all, and you should follow that advice. I'm not really familiar at all with this code (compared to him). FWIW the "util" here could be dropped in any case, in my version it was because we were making a struct, but the idiom for this would just be: string_list_append(....)->util = xstrdup(...); > + printf(_(" - go to submodule (%s), and either merge commit %s\n" > + " or update to an existing commit which has merged those changes\n"), > + item->string, abbrev); FWIW what I mentioned in v5 was to arrange this so that " - " or whatever would be _()'d separately, so translators wouldn't need to worry about the formatting... > + printf(" git add %s", item->string); > + first = 0; > + } else { > + printf(" \\\n %s", item->string); And if we're translating *some* whitespace we should translate all of it. In RTL languages the a string like " foo" needs to be translated as "foo ". I.e. the whitespace from the "right" side of your terminal. That was what I was pointing out in the object-name.c code, usage_with_options_internal() is another example. > + } > + printf(_("\n\n to record the above merge or update\n" We can add \n\n unconditionally, no need to put it in the translation.
> Well explained. One very minor suggestion: perhaps change "id of the > commit" to "id of the submodule commit" just to make it slightly > clearer that this information would take work for the user to discover > on their own? (When I first read it, I was thinking, "but they have > the commit, it's what they passed to merge", before I realized my > error.) ack > Sorry for not catching this in an earlier round, but merge_submodule() > has four "return 0" cases, for particular types of conflicts. Those > should probably be switched to "goto cleanup" or something like that, > so that these messages you are adding are also provided if one of > those conflict cases are hit. I didn't send these four "return 0" cases to cleanup because I thought the error message wouldn't accurately reflect the resolution steps. Is merging or updating the submodule still the correct resolution? The first three cases are for a null o/a/b, and the fourth case is for a missing local submodule. Also in cleanup, the subrepo is cleared but the subrepo hasn't been initialized/failed to initialize in these four cases. > > object_array_clear(&merges); > > cleanup: > > + if (!ret) { > > And here's another item I have to apologize for not catching in an > earlier round. We should also require !opt->priv->call_depth as well > in this if-condition. If merging of merge bases produced a submodule > conflict, but the outer merge involves two sides that resolved the > inner conflict the exact same way, then there's no conflict at the > outer level and nothing for the user to resolve. If users don't have > any conflicts to resolve, we don't want to print messages telling them > how to resolve their non-existent conflicts. And if there is still a > conflict in the submodule for the outer merge as well as in the > recursive merge(s), we don't want to list the module twice (or more) > when we tell the user to fix conflicts in their submodules (especially > since that means we'd be telling them to merge multiple different > commits for the single submodule, which could get confusing). ack. > > + for_each_string_list_item(item, csub) { > > + const char *abbrev= item->util; > > Messed up indent here? Looks like going from my editor to `git format-patch` messed something up here. > > + for_each_string_list_item(item, csub) > > + /* > > + * TRANSLATORS: This is a line of a recommended `git add` command > > + * with multiple lines of submodule folders. > > + * E.g.: git add sub \ > > + * sub2 \ > > + * sub3 > > Why does such a message need to be translated? It's literal text the > user should type, right? I'm not sure what a translator would do with > the message other than regurgitate it. It doesn't. My point was to let the translator know that the only text in this print is for a git command. I should probably add that context to the comment though. > > + */ > > + if (first) { > > + printf(" git add %s", item->string); > > But if you did mean for there to be a translation and a TRANSLATORS > note, then did you forget to translate it by calling _()? Same reasoning as above. > > + first = 0; > > + } else { > > + printf(" \\\n %s", item->string); > > + } > > Can we put braces around this for_each_string_list_item() block? Or, > as an alternative to the whole block, do you want to consider: > > strub strbuf tmp = STRBUF_INIT; > strbuf_add_separated_string_list(&tmp, ' ', csub); > printf(_(" git add %s"), tmp.buf); /* or maybe remove the > translation; not sure what the point is */ > strbuf_release(&tmp); > ? It is likely easier to copy & paste, and might be understood by > more users (I'm not sure how many are aware that command lines can use > backslashes for line continuation), but on the negative side, if you > have a lot of submodules it might make it harder to read. Even if you > don't like space separated, though, you could still use this strategy > by changing the second line to > > strbuf_add_separated_string_list(&tmp, " \\\n ", csub); This is a much cleaner implementation, thanks! If my goal is to make submodule merging easier for newer submodule users, then I think it's a good assumption to remove any additional possible points of confusion, aka with the "command lines can use backslashes for line continuation", so I'll swap over to spaces. > > + print_submodule_conflict_suggestion(&opti->conflicted_submodules); > > + string_list_clear(&opti->conflicted_submodules, 1); > > + > > It would be more consistent to have things allocated in merge_start() > continue to be cleared out in clear_or_reinit_internal_opts(). This > kind of breaks that pairing, and you're already making sure to clear > it there, so I'd rather remove this duplicate string_list_clear() > call. ack. > > /* Also include needed rename limit adjustment now */ > > diff_warn_rename_limit("merge.renamelimit", > > opti->renames.needed_limit, 0); > > @@ -4657,6 +4717,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result) > > trace2_region_enter("merge", "allocate/init", opt->repo); > > if (opt->priv) { > > clear_or_reinit_internal_opts(opt->priv, 1); > > + string_list_init_dup(&opt->priv->conflicted_submodules); > > This works, but there is a minor optimization available here if you're > interested (I understand if you're not since you're already at v6). > Assuming you make the important opt->priv->call_depth fix, you can > replace string_list_init_dup() with string_list_init_nodup() here. > The paths aren't freed until clear_or_reinit_internal_opts() which > (under the assumption previously stated) isn't called until > merge_finalize(), which comes after the call to > merge_display_update_messages(), which is where you use the data. > > (As repository paths are used all over merge-ort.c, the optimization > to store them in one place (opt->priv->paths) and avoid duplicating > them is used pretty heavily. It's more important for a lot of the > other strmaps since they'll have a lot more paths in them, but it is > kind of nice to use this optimization where possible.) Thanks for all the context of this one. I agree it's minor, but any place we can provide a better example to future contributors seems like a more than worthy reason to make this change :)
Hi, On Wed, Jul 27, 2022 at 3:00 PM Calvin Wan <calvinwan@google.com> wrote: [...] > > Sorry for not catching this in an earlier round, but merge_submodule() > > has four "return 0" cases, for particular types of conflicts. Those > > should probably be switched to "goto cleanup" or something like that, > > so that these messages you are adding are also provided if one of > > those conflict cases are hit. > > I didn't send these four "return 0" cases to cleanup because I thought > the error message wouldn't accurately reflect the resolution steps. Is > merging or updating the submodule still the correct resolution? The > first three cases are for a null o/a/b, and the fourth case is for a missing > local submodule. Also in cleanup, the subrepo is cleared but the > subrepo hasn't been initialized/failed to initialize in these four cases. Ah, I remember we partially discussed this earlier in this thread; sorry for forgetting. For the failed to initialize case, yes we also need a merge -- the submodule is conflicted due to the lack of one. The steps the user needs to take are probably even more involved, though (they also need to initialize the submodule), so perhaps that one should be special cased. The 'a' or 'b' being a null oid is actually dead code, as discussed earlier in the thread. Perhaps we should change those two code paths from "return 0" to 'BUG("submodule deleted on one side; this should be handled outside of merge_submodule()")', and possibly with a commit message linking to https://lore.kernel.org/git/CABPp-BE0qGwUy80dmVszkJQ+tcpfLRW0OZyErymzhZ9+HWY1mw@mail.gmail.com/ (and mentioning the "a and b being null oids within merge_submodule will never trigger" portion of that email). The 'o' being a null oid is not dead code. That particular case means that there was no submodule in the merge base, but both sides of the merge introduced the submodule and have it checked out to different oids. (At least, hopefully it's the same submodule.) In that case, yes we do need some kind of merge. So I think your message should probably be included in this case, as-is. Since the cleanup thing you mention is an issue, perhaps you need to refactor the code a bit so that you can make this case somehow get the same message printed for users? All that said, if you want to defer any or all of this to a follow-on series, that's fine...but it would be nice to have it mentioned in the commit message. > > > + for_each_string_list_item(item, csub) > > > + /* > > > + * TRANSLATORS: This is a line of a recommended `git add` command > > > + * with multiple lines of submodule folders. > > > + * E.g.: git add sub \ > > > + * sub2 \ > > > + * sub3 > > > > Why does such a message need to be translated? It's literal text the > > user should type, right? I'm not sure what a translator would do with > > the message other than regurgitate it. > > It doesn't. My point was to let the translator know that the only text > in this print is for a git command. I should probably add that context > to the comment though. Um...if the string doesn't need to be marked for translation, and isn't marked for translation, why would translators go looking for a comment to help explain how to translate something that doesn't appear in their list of strings they need to translate? Using printf(" git add %s", ...) means that the string " git add %s" will not appear in the po/*.po files. If it had been printf(_(" git add %s"), ...) then it would appear in those files with filename(s) and line number(s) stating where the string had come from in case translators needed to look for clues about the context in order to know how to translate the string. So, I think you can just drop the comment. Or am I still not understanding some nuance here?
> For the failed to initialize case, yes we also need a merge -- the > submodule is conflicted due to the lack of one. The steps the user > needs to take are probably even more involved, though (they also need > to initialize the submodule), so perhaps that one should be special > cased. Adding a needswork bit to this one. Don't quite have the bandwidth to figure out what would be a useful message in this case. > The 'a' or 'b' being a null oid is actually dead code, as discussed > earlier in the thread. Perhaps we should change those two code paths > from "return 0" to 'BUG("submodule deleted on one side; this should be > handled outside of merge_submodule()")', and possibly with a commit > message linking to > https://lore.kernel.org/git/CABPp-BE0qGwUy80dmVszkJQ+tcpfLRW0OZyErymzhZ9+HWY1mw@mail.gmail.com/ > (and mentioning the "a and b being null oids within merge_submodule > will never trigger" portion of that email). > > The 'o' being a null oid is not dead code. That particular case means > that there was no submodule in the merge base, but both sides of the > merge introduced the submodule and have it checked out to different > oids. (At least, hopefully it's the same submodule.) In that case, > yes we do need some kind of merge. So I think your message should > probably be included in this case, as-is. Since the cleanup thing you > mention is an issue, perhaps you need to refactor the code a bit so > that you can make this case somehow get the same message printed for > users? > > All that said, if you want to defer any or all of this to a follow-on > series, that's fine...but it would be nice to have it mentioned in the > commit message. BUGging out for 'a' and 'b' sounds reasonable to me. And I also do believe my error message applies to the 'o' case. I'll add a test to confirm. > Um...if the string doesn't need to be marked for translation, and > isn't marked for translation, why would translators go looking for a > comment to help explain how to translate something that doesn't appear > in their list of strings they need to translate? Ahhhhhhhhh I see...facepalm > Using > printf(" git add %s", ...) > means that the string " git add %s" will not appear in the po/*.po > files. If it had been > printf(_(" git add %s"), ...) > then it would appear in those files with filename(s) and line > number(s) stating where the string had come from in case translators > needed to look for clues about the context in order to know how to > translate the string. > > So, I think you can just drop the comment. Or am I still not > understanding some nuance here? You're not missing anything. Wasn't aware of how translators worked with the project.
diff --git a/merge-ort.c b/merge-ort.c index 01f150ef3b..147be0ce31 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -387,6 +387,9 @@ struct merge_options_internal { /* call_depth: recursion level counter for merging merge bases */ int call_depth; + + /* field that holds submodule conflict information */ + struct string_list conflicted_submodules; }; struct version_info { @@ -686,6 +689,8 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti, mem_pool_discard(&opti->pool, 0); + string_list_clear(&opti->conflicted_submodules, 1); + /* Clean out callback_data as well. */ FREE_AND_NULL(renames->callback_data); renames->callback_data_nr = renames->callback_data_alloc = 0; @@ -1849,6 +1854,17 @@ static int merge_submodule(struct merge_options *opt, object_array_clear(&merges); cleanup: + if (!ret) { + struct string_list *csub = &opt->priv->conflicted_submodules; + char *util; + const char *abbrev; + + abbrev = repo_find_unique_abbrev(&subrepo, b, DEFAULT_ABBREV); + util = xstrdup(abbrev); + + string_list_append(csub, path)->util = util; + } + repo_clear(&subrepo); return ret; } @@ -4412,6 +4428,47 @@ static int record_conflicted_index_entries(struct merge_options *opt) return errs; } +static void print_submodule_conflict_suggestion(struct string_list *csub) { + int first = 1; + if (csub->nr > 0) { + struct string_list_item *item; + printf(_("Recursive merging with submodules currently only supports trivial cases.\n" + "Please manually handle the merging of each conflicted submodule.\n" + "This can be accomplished with the following steps:\n")); + + for_each_string_list_item(item, csub) { + const char *abbrev= item->util; + /* + * TRANSLATORS: This is a line of advice to resolve a merge conflict + * in a submodule. The second argument is the abbreviated id of the + * commit that needs to be merged. + * E.g. - go to submodule (sub), and either merge commit abc1234" + */ + printf(_(" - go to submodule (%s), and either merge commit %s\n" + " or update to an existing commit which has merged those changes\n"), + item->string, abbrev); + } + printf(_(" - come back to superproject and run:\n\n")); + for_each_string_list_item(item, csub) + /* + * TRANSLATORS: This is a line of a recommended `git add` command + * with multiple lines of submodule folders. + * E.g.: git add sub \ + * sub2 \ + * sub3 + */ + if (first) { + printf(" git add %s", item->string); + first = 0; + } else { + printf(" \\\n %s", item->string); + } + printf(_("\n\n to record the above merge or update\n" + " - resolve any other conflicts in the superproject\n" + " - commit the resulting index in the superproject\n")); + } +} + void merge_display_update_messages(struct merge_options *opt, int detailed, struct merge_result *result) @@ -4461,6 +4518,9 @@ void merge_display_update_messages(struct merge_options *opt, } string_list_clear(&olist, 0); + print_submodule_conflict_suggestion(&opti->conflicted_submodules); + string_list_clear(&opti->conflicted_submodules, 1); + /* Also include needed rename limit adjustment now */ diff_warn_rename_limit("merge.renamelimit", opti->renames.needed_limit, 0); @@ -4657,6 +4717,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result) trace2_region_enter("merge", "allocate/init", opt->repo); if (opt->priv) { clear_or_reinit_internal_opts(opt->priv, 1); + string_list_init_dup(&opt->priv->conflicted_submodules); trace2_region_leave("merge", "allocate/init", opt->repo); return; } diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh index c253bf759a..84913525cf 100755 --- a/t/t6437-submodule-merge.sh +++ b/t/t6437-submodule-merge.sh @@ -103,8 +103,25 @@ 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' ' + test_when_finished "git -C merge-search reset --hard" && + (cd merge-search && + git checkout -b test-nonforward-a b && + if test "$GIT_TEST_MERGE_ALGORITHM" = ort + then + test_must_fail git merge c >actual && + sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" && + grep "$sub_expect" actual + else + test_must_fail git merge c 2> actual + fi) +' + +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,14 +146,16 @@ 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 --short sub-d > ../expect) && if test "$GIT_TEST_MERGE_ALGORITHM" = ort then - test_must_fail git merge c >actual + test_must_fail git merge c >actual && + sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" && + grep "$sub_expect" actual else test_must_fail git merge c 2> actual fi && @@ -161,7 +180,9 @@ test_expect_success 'merging should fail for ambiguous common parent' ' ) && if test "$GIT_TEST_MERGE_ALGORITHM" = ort then - test_must_fail git merge c >actual + test_must_fail git merge c >actual && + sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" && + grep "$sub_expect" actual else test_must_fail git merge c 2> actual fi && @@ -205,7 +226,12 @@ 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 && + if test "$GIT_TEST_MERGE_ALGORITHM" = ort + then + sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-d)" && + grep "$sub_expect" actual + fi) ' diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh index 8e32f19007..ebeca12a71 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,12 @@ 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 && + if test "$GIT_TEST_MERGE_ALGORITHM" = ort + then + sub_expect="go to submodule (submodule), and either merge commit $(git -C submodule rev-parse --short HEAD^0)" && + grep "$sub_expect" actual_output + fi ' test_done
When attempting to merge in a superproject with conflicting submodule pointers that cannot be fast-forwarded or trivially resolved, the merge fails and git prints an error message that accurately describes the failure, but does not provide steps for the user to resolve the error. 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 provide steps to resolve submodule merge conflict. Future work could involve adding an advice flag to the message. Although the message is long, it also has the id of the commit that needs to be merged, which could be useful information for the user. Signed-off-by: Calvin Wan <calvinwan@google.com> --- I'm a little unsure on the code style for the `git add` section of the error message. It works, but is tricky for a code reviewer to decipher the formatting. I don't think it should affect translation since the text is only a git command and folder names. Changes since v5: - Fixed memory leak reported by Ævar - Fixed error message formatting and addded TRANSLATOR tags - Removed "resolution exists" merge-ort.c | 61 +++++++++++++++++++++++++++++++++++++ t/t6437-submodule-merge.sh | 38 +++++++++++++++++++---- t/t7402-submodule-rebase.sh | 9 ++++-- 3 files changed, 100 insertions(+), 8 deletions(-) base-commit: 9dd64cb4d310986dd7b8ca7fff92f9b61e0bd21a