From 4057523a4061092e9181220d54dca9eadcb75bdc Mon Sep 17 00:00:00 2001 From: Calvin Wan Date: Thu, 4 Aug 2022 19:51:05 +0000 Subject: submodule merge: update conflict error message 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 submodule commit that needs to be merged, which could be useful information for the user. Additionally, 5 merge failures that resulted in an early return have been updated to reflect the status of the merge. 1. Null merge base (null o): CONFLICT_SUBMODULE_NULL_MERGE_BASE added as a new conflict type and will print updated error message. 2. Null merge side a (null a): BUG(). See [1] for discussion 3. Null merge side b (null b): BUG(). See [1] for discussion 4. Submodule not checked out: added NEEDSWORK bit 5. Submodule commits not present: added NEEDSWORK bit The errors with a NEEDSWORK bit deserve a more detailed explanation of how to resolve them. See [2] for more context. [1] https://lore.kernel.org/git/CABPp-BE0qGwUy80dmVszkJQ+tcpfLRW0OZyErymzhZ9+HWY1mw@mail.gmail.com/ [2] https://lore.kernel.org/git/xmqqpmhjjwo9.fsf@gitster.g/ Signed-off-by: Calvin Wan Signed-off-by: Junio C Hamano --- merge-ort.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 152 insertions(+), 9 deletions(-) (limited to 'merge-ort.c') diff --git a/merge-ort.c b/merge-ort.c index 8b7de0fbd8..a52faf6e21 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -387,8 +387,24 @@ 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 conflicted_submodule_item { + char *abbrev; + int flag; }; +static void conflicted_submodule_item_free(void *util, const char *str) +{ + struct conflicted_submodule_item *item = util; + + free(item->abbrev); + free(item); +} + struct version_info { struct object_id oid; unsigned short mode; @@ -517,6 +533,7 @@ enum conflict_and_info_types { CONFLICT_SUBMODULE_NOT_INITIALIZED, CONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE, CONFLICT_SUBMODULE_MAY_HAVE_REWINDS, + CONFLICT_SUBMODULE_NULL_MERGE_BASE, /* Keep this entry _last_ in the list */ NB_CONFLICT_TYPES, @@ -570,6 +587,8 @@ static const char *type_short_descriptions[] = { "CONFLICT (submodule history not available)", [CONFLICT_SUBMODULE_MAY_HAVE_REWINDS] = "CONFLICT (submodule may have rewinds)", + [CONFLICT_SUBMODULE_NULL_MERGE_BASE] = + "CONFLICT (submodule lacks merge base)" }; struct logical_conflict_info { @@ -686,6 +705,9 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti, mem_pool_discard(&opti->pool, 0); + string_list_clear_func(&opti->conflicted_submodules, + conflicted_submodule_item_free); + /* Clean out callback_data as well. */ FREE_AND_NULL(renames->callback_data); renames->callback_data_nr = renames->callback_data_alloc = 0; @@ -1744,24 +1766,32 @@ static int merge_submodule(struct merge_options *opt, int i; int search = !opt->priv->call_depth; + int sub_not_initialized = 1; + int sub_flag = -1; /* store fallback answer in result in case we fail */ oidcpy(result, opt->priv->call_depth ? o : a); /* we can not handle deletion conflicts */ - if (is_null_oid(o)) - return 0; - if (is_null_oid(a)) - return 0; - if (is_null_oid(b)) - return 0; + if (is_null_oid(a) || is_null_oid(b)) + BUG("submodule deleted on one side; this should be handled outside of merge_submodule()"); - if (repo_submodule_init(&subrepo, opt->repo, path, null_oid())) { + if ((sub_not_initialized = repo_submodule_init(&subrepo, + opt->repo, path, null_oid()))) { path_msg(opt, CONFLICT_SUBMODULE_NOT_INITIALIZED, 0, path, NULL, NULL, NULL, _("Failed to merge submodule %s (not checked out)"), path); - return 0; + sub_flag = CONFLICT_SUBMODULE_NOT_INITIALIZED; + goto cleanup; + } + + if (is_null_oid(o)) { + path_msg(opt, CONFLICT_SUBMODULE_NULL_MERGE_BASE, 0, + path, NULL, NULL, NULL, + _("Failed to merge submodule %s (no merge base)"), + path); + goto cleanup; } if (!(commit_o = lookup_commit_reference(&subrepo, o)) || @@ -1771,6 +1801,7 @@ static int merge_submodule(struct merge_options *opt, path, NULL, NULL, NULL, _("Failed to merge submodule %s (commits not present)"), path); + sub_flag = CONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE; goto cleanup; } @@ -1849,7 +1880,23 @@ static int merge_submodule(struct merge_options *opt, object_array_clear(&merges); cleanup: - repo_clear(&subrepo); + if (!opt->priv->call_depth && !ret) { + struct string_list *csub = &opt->priv->conflicted_submodules; + struct conflicted_submodule_item *util; + const char *abbrev; + + util = xmalloc(sizeof(*util)); + util->flag = sub_flag; + util->abbrev = NULL; + if (!sub_not_initialized) { + abbrev = repo_find_unique_abbrev(&subrepo, b, DEFAULT_ABBREV); + util->abbrev = xstrdup(abbrev); + } + string_list_append(csub, path)->util = util; + } + + if (!sub_not_initialized) + repo_clear(&subrepo); return ret; } @@ -4434,6 +4481,99 @@ static int record_conflicted_index_entries(struct merge_options *opt) return errs; } +static void format_submodule_conflict_suggestion(struct strbuf *msg) { + struct strbuf tmp = STRBUF_INIT; + struct string_list msg_list = STRING_LIST_INIT_DUP; + int i; + + string_list_split(&msg_list, msg->buf, '\n', -1); + for (i = 0; i < msg_list.nr; i++) { + if (!i) + /* + * TRANSLATORS: This is line item of submodule conflict message + * from print_submodule_conflict_suggestion() below. For RTL + * languages, the following swap is suggested: + * " - %s\n" -> "%s - \n" + */ + strbuf_addf(&tmp, _(" - %s\n"), msg_list.items[i].string); + else + /* + * TRANSLATORS: This is line item of submodule conflict message + * from print_submodule_conflict_suggestion() below. For RTL + * languages, the following swap is suggested: + * " %s\n" -> "%s \n" + */ + strbuf_addf(&tmp, _(" %s\n"), msg_list.items[i].string); + } + strbuf_reset(msg); + strbuf_add(msg, tmp.buf, tmp.len); +} + +static void print_submodule_conflict_suggestion(struct string_list *csub) { + struct string_list_item *item; + struct strbuf msg = STRBUF_INIT; + struct strbuf tmp = STRBUF_INIT; + struct strbuf subs = STRBUF_INIT; + + if (!csub->nr) + return; + + /* + * NEEDSWORK: The steps to resolve these errors deserve a more + * detailed explanation than what is currently printed below. + */ + for_each_string_list_item(item, csub) { + struct conflicted_submodule_item *util = item->util; + + if (util->flag == CONFLICT_SUBMODULE_NOT_INITIALIZED || + util->flag == CONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE) + return; + } + + printf(_("Recursive merging with submodules currently only supports " + "trivial cases.\nPlease manually handle the merging of each " + "conflicted submodule.\nThis can be accomplished with the following " + "steps:")); + putchar('\n'); + + for_each_string_list_item(item, csub) { + struct conflicted_submodule_item *util = 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" + */ + strbuf_addf(&tmp, _("go to submodule (%s), and either merge commit %s\n" + "or update to an existing commit which has merged those changes"), + item->string, util->abbrev); + format_submodule_conflict_suggestion(&tmp); + strbuf_add(&msg, tmp.buf, tmp.len); + strbuf_reset(&tmp); + } + strbuf_add_separated_string_list(&subs, " ", csub); + strbuf_addstr(&tmp, _("come back to superproject and run:")); + strbuf_addf(&tmp, "\n\ngit add %s\n\n", subs.buf); + strbuf_addstr(&tmp, _("to record the above merge or update")); + format_submodule_conflict_suggestion(&tmp); + strbuf_add(&msg, tmp.buf, tmp.len); + strbuf_reset(&tmp); + + strbuf_addstr(&tmp, _("resolve any other conflicts in the superproject")); + format_submodule_conflict_suggestion(&tmp); + strbuf_add(&msg, tmp.buf, tmp.len); + strbuf_reset(&tmp); + + strbuf_addstr(&tmp, _("commit the resulting index in the superproject")); + format_submodule_conflict_suggestion(&tmp); + strbuf_add(&msg, tmp.buf, tmp.len); + + printf("%s", msg.buf); + strbuf_release(&subs); + strbuf_release(&tmp); + strbuf_release(&msg); +} + void merge_display_update_messages(struct merge_options *opt, int detailed, struct merge_result *result) @@ -4483,6 +4623,8 @@ void merge_display_update_messages(struct merge_options *opt, } string_list_clear(&olist, 0); + print_submodule_conflict_suggestion(&opti->conflicted_submodules); + /* Also include needed rename limit adjustment now */ diff_warn_rename_limit("merge.renamelimit", opti->renames.needed_limit, 0); @@ -4679,6 +4821,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_nodup(&opt->priv->conflicted_submodules); trace2_region_leave("merge", "allocate/init", opt->repo); return; } -- cgit v1.2.3 From a5834b775b233d61b71927e1e0b98d2372406492 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 18 Aug 2022 07:15:25 +0000 Subject: merge-ort: remove translator lego in new "submodule conflict suggestion" In commit 4057523a40 ("submodule merge: update conflict error message", 2022-08-04), the new "submodule conflict suggestion" code was translating 6 different pieces of the new message and then used carefully crafted logic to allow stitching it back together with special formatting. Keep the components of the message together as much as possible, so that: * we reduce the number of things translators have to translate * translators have more control over the format of the output * the code is much easier for developers to understand too Also, reformat some comments running beyond the 80th column while at it. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-ort.c | 88 ++++++++++++++++++++----------------------------------------- 1 file changed, 28 insertions(+), 60 deletions(-) (limited to 'merge-ort.c') diff --git a/merge-ort.c b/merge-ort.c index a52faf6e21..67159fc6ef 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -4481,34 +4481,6 @@ static int record_conflicted_index_entries(struct merge_options *opt) return errs; } -static void format_submodule_conflict_suggestion(struct strbuf *msg) { - struct strbuf tmp = STRBUF_INIT; - struct string_list msg_list = STRING_LIST_INIT_DUP; - int i; - - string_list_split(&msg_list, msg->buf, '\n', -1); - for (i = 0; i < msg_list.nr; i++) { - if (!i) - /* - * TRANSLATORS: This is line item of submodule conflict message - * from print_submodule_conflict_suggestion() below. For RTL - * languages, the following swap is suggested: - * " - %s\n" -> "%s - \n" - */ - strbuf_addf(&tmp, _(" - %s\n"), msg_list.items[i].string); - else - /* - * TRANSLATORS: This is line item of submodule conflict message - * from print_submodule_conflict_suggestion() below. For RTL - * languages, the following swap is suggested: - * " %s\n" -> "%s \n" - */ - strbuf_addf(&tmp, _(" %s\n"), msg_list.items[i].string); - } - strbuf_reset(msg); - strbuf_add(msg, tmp.buf, tmp.len); -} - static void print_submodule_conflict_suggestion(struct string_list *csub) { struct string_list_item *item; struct strbuf msg = STRBUF_INIT; @@ -4530,45 +4502,41 @@ static void print_submodule_conflict_suggestion(struct string_list *csub) { return; } - printf(_("Recursive merging with submodules currently only supports " - "trivial cases.\nPlease manually handle the merging of each " - "conflicted submodule.\nThis can be accomplished with the following " - "steps:")); - putchar('\n'); - + strbuf_add_separated_string_list(&subs, " ", csub); for_each_string_list_item(item, csub) { struct conflicted_submodule_item *util = 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" + * TRANSLATORS: This is a line of advice to resolve a merge + * conflict in a submodule. The first argument is the submodule + * name, and the second argument is the abbreviated id of the + * commit that needs to be merged. For example: + * - go to submodule (mysubmodule), and either merge commit abc1234" */ - strbuf_addf(&tmp, _("go to submodule (%s), and either merge commit %s\n" - "or update to an existing commit which has merged those changes"), - item->string, util->abbrev); - format_submodule_conflict_suggestion(&tmp); - strbuf_add(&msg, tmp.buf, tmp.len); - strbuf_reset(&tmp); + strbuf_addf(&tmp, _(" - go to submodule (%s), and either merge commit %s\n" + " or update to an existing commit which has merged those changes\n"), + item->string, util->abbrev); } - strbuf_add_separated_string_list(&subs, " ", csub); - strbuf_addstr(&tmp, _("come back to superproject and run:")); - strbuf_addf(&tmp, "\n\ngit add %s\n\n", subs.buf); - strbuf_addstr(&tmp, _("to record the above merge or update")); - format_submodule_conflict_suggestion(&tmp); - strbuf_add(&msg, tmp.buf, tmp.len); - strbuf_reset(&tmp); - - strbuf_addstr(&tmp, _("resolve any other conflicts in the superproject")); - format_submodule_conflict_suggestion(&tmp); - strbuf_add(&msg, tmp.buf, tmp.len); - strbuf_reset(&tmp); - - strbuf_addstr(&tmp, _("commit the resulting index in the superproject")); - format_submodule_conflict_suggestion(&tmp); - strbuf_add(&msg, tmp.buf, tmp.len); + + /* + * TRANSLATORS: This is a detailed message for resolving submodule + * conflicts. The first argument is string containing one step per + * submodule. The second is a space-separated list of submodule names. + */ + strbuf_addf(&msg, + _("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" + "%s" + " - come back to superproject and run:\n\n" + " git add %s\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"), + tmp.buf, subs.buf); printf("%s", msg.buf); + strbuf_release(&subs); strbuf_release(&tmp); strbuf_release(&msg); -- cgit v1.2.3 From 34ce504a33cca435b3fc689c3e2a0ae2f71d5a85 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 18 Aug 2022 07:15:26 +0000 Subject: merge-ort: avoid surprise with new sub_flag variable Commit 4057523a40 ("submodule merge: update conflict error message", 2022-08-04) added a sub_flag variable that is used to store a value from enum conflict_and_info_types, but initializes it with a value of -1 that does not correspond to any of the conflict_and_info_types. The code may never set it to a valid value and yet still use it, which can be surprising when reading over the code at first. Initialize it instead to the generic CONFLICT_SUBMODULE_FAILED_TO_MERGE value, which is still distinct from the two values we need to special case. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-ort.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'merge-ort.c') diff --git a/merge-ort.c b/merge-ort.c index 67159fc6ef..8f14f1ad0b 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -1767,7 +1767,7 @@ static int merge_submodule(struct merge_options *opt, int i; int search = !opt->priv->call_depth; int sub_not_initialized = 1; - int sub_flag = -1; + int sub_flag = CONFLICT_SUBMODULE_FAILED_TO_MERGE; /* store fallback answer in result in case we fail */ oidcpy(result, opt->priv->call_depth ? o : a); -- cgit v1.2.3 From 565577ed883057d9935cfc56e33be8d4abd9a2fc Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 18 Aug 2022 07:15:27 +0000 Subject: merge-ort: provide helpful submodule update message when possible In commit 4057523a40 ("submodule merge: update conflict error message", 2022-08-04), a more detailed message was provided when submodules conflict, in order to help users know how to resolve those conflicts. There were a couple situations for which a different message would be more appropriate, but that commit left handling those for future work. Unfortunately, that commit would check if any submodules were of the type that it didn't know how to explain, and, if so, would avoid providing the more detailed explanation even for the submodules it did know how to explain. Change this to have the code print the helpful messages for the subset of submodules it knows how to explain. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- merge-ort.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) (limited to 'merge-ort.c') diff --git a/merge-ort.c b/merge-ort.c index 8f14f1ad0b..d94e3f17d7 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -4490,21 +4490,17 @@ static void print_submodule_conflict_suggestion(struct string_list *csub) { if (!csub->nr) return; - /* - * NEEDSWORK: The steps to resolve these errors deserve a more - * detailed explanation than what is currently printed below. - */ + strbuf_add_separated_string_list(&subs, " ", csub); for_each_string_list_item(item, csub) { struct conflicted_submodule_item *util = item->util; + /* + * NEEDSWORK: The steps to resolve these errors deserve a more + * detailed explanation than what is currently printed below. + */ if (util->flag == CONFLICT_SUBMODULE_NOT_INITIALIZED || - util->flag == CONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE) - return; - } - - strbuf_add_separated_string_list(&subs, " ", csub); - for_each_string_list_item(item, csub) { - struct conflicted_submodule_item *util = item->util; + util->flag == CONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE) + continue; /* * TRANSLATORS: This is a line of advice to resolve a merge -- cgit v1.2.3