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 <calvinwan@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Calvin Wan committed
Aug 4, 2022 at 19:51 UTC
4057523a4061092e9181220d54dca9eadcb75bdc
3 files changed
+231
-17
merge-ort.c
+152
-9
@@ -387,8 +387,24 @@ struct merge_options_internal {
387
388
/* call_depth: recursion level counter for merging merge bases */
389
int call_depth;
390
+
391
+ /* field that holds submodule conflict information */
392
+ struct string_list conflicted_submodules;
393
+};
394
+
395
+struct conflicted_submodule_item {
396
+ char *abbrev;
397
+ int flag;
398
};
399
400
+static void conflicted_submodule_item_free(void *util, const char *str)
401
+{
402
+ struct conflicted_submodule_item *item = util;
403
+
404
+ free(item->abbrev);
405
+ free(item);
406
+}
407
+
408
struct version_info {
409
struct object_id oid;
410
unsigned short mode;
@@ -517,6 +533,7 @@ enum conflict_and_info_types {
533
CONFLICT_SUBMODULE_NOT_INITIALIZED,
534
CONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE,
535
CONFLICT_SUBMODULE_MAY_HAVE_REWINDS,
536
+ CONFLICT_SUBMODULE_NULL_MERGE_BASE,
537
538
/* Keep this entry _last_ in the list */
539
NB_CONFLICT_TYPES,
@@ -570,6 +587,8 @@ static const char *type_short_descriptions[] = {
587
"CONFLICT (submodule history not available)",
588
[CONFLICT_SUBMODULE_MAY_HAVE_REWINDS] =
589
"CONFLICT (submodule may have rewinds)",
590
+ [CONFLICT_SUBMODULE_NULL_MERGE_BASE] =
591
+ "CONFLICT (submodule lacks merge base)"
592
};
593
594
struct logical_conflict_info {
@@ -686,6 +705,9 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
705
706
mem_pool_discard(&opti->pool, 0);
707
708
+ string_list_clear_func(&opti->conflicted_submodules,
709
+ conflicted_submodule_item_free);
710
+
711
/* Clean out callback_data as well. */
712
FREE_AND_NULL(renames->callback_data);
713
renames->callback_data_nr = renames->callback_data_alloc = 0;
@@ -1744,24 +1766,32 @@ static int merge_submodule(struct merge_options *opt,
1766
1767
int i;
1768
int search = !opt->priv->call_depth;
1769
+ int sub_not_initialized = 1;
1770
+ int sub_flag = -1;
1771
1772
/* store fallback answer in result in case we fail */
1773
oidcpy(result, opt->priv->call_depth ? o : a);
1774
1775
/* we can not handle deletion conflicts */
1752
- if (is_null_oid(o))
1753
- return 0;
1754
- if (is_null_oid(a))
1755
- return 0;
1756
- if (is_null_oid(b))
1757
- return 0;
1776
+ if (is_null_oid(a) || is_null_oid(b))
1777
+ BUG("submodule deleted on one side; this should be handled outside of merge_submodule()");
1778
1759
- if (repo_submodule_init(&subrepo, opt->repo, path, null_oid())) {
1779
+ if ((sub_not_initialized = repo_submodule_init(&subrepo,
1780
+ opt->repo, path, null_oid()))) {
1781
path_msg(opt, CONFLICT_SUBMODULE_NOT_INITIALIZED, 0,
1782
path, NULL, NULL, NULL,
1783
_("Failed to merge submodule %s (not checked out)"),
1784
path);
1764
- return 0;
1785
+ sub_flag = CONFLICT_SUBMODULE_NOT_INITIALIZED;
1786
+ goto cleanup;
1787
+ }
1788
+
1789
+ if (is_null_oid(o)) {
1790
+ path_msg(opt, CONFLICT_SUBMODULE_NULL_MERGE_BASE, 0,
1791
+ path, NULL, NULL, NULL,
1792
+ _("Failed to merge submodule %s (no merge base)"),
1793
+ path);
1794
+ goto cleanup;
1795
}
1796
1797
if (!(commit_o = lookup_commit_reference(&subrepo, o)) ||
@@ -1771,6 +1801,7 @@ static int merge_submodule(struct merge_options *opt,
1801
path, NULL, NULL, NULL,
1802
_("Failed to merge submodule %s (commits not present)"),
1803
path);
1804
+ sub_flag = CONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE;
1805
goto cleanup;
1806
}
1807
@@ -1849,7 +1880,23 @@ static int merge_submodule(struct merge_options *opt,
1880
1881
object_array_clear(&merges);
1882
cleanup:
1852
- repo_clear(&subrepo);
1883
+ if (!opt->priv->call_depth && !ret) {
1884
+ struct string_list *csub = &opt->priv->conflicted_submodules;
1885
+ struct conflicted_submodule_item *util;
1886
+ const char *abbrev;
1887
+
1888
+ util = xmalloc(sizeof(*util));
1889
+ util->flag = sub_flag;
1890
+ util->abbrev = NULL;
1891
+ if (!sub_not_initialized) {
1892
+ abbrev = repo_find_unique_abbrev(&subrepo, b, DEFAULT_ABBREV);
1893
+ util->abbrev = xstrdup(abbrev);
1894
+ }
1895
+ string_list_append(csub, path)->util = util;
1896
+ }
1897
+
1898
+ if (!sub_not_initialized)
1899
+ repo_clear(&subrepo);
1900
return ret;
1901
}
1902
@@ -4434,6 +4481,99 @@ static int record_conflicted_index_entries(struct merge_options *opt)
4481
return errs;
4482
}
4483
4484
+static void format_submodule_conflict_suggestion(struct strbuf *msg) {
4485
+ struct strbuf tmp = STRBUF_INIT;
4486
+ struct string_list msg_list = STRING_LIST_INIT_DUP;
4487
+ int i;
4488
+
4489
+ string_list_split(&msg_list, msg->buf, '\n', -1);
4490
+ for (i = 0; i < msg_list.nr; i++) {
4491
+ if (!i)
4492
+ /*
4493
+ * TRANSLATORS: This is line item of submodule conflict message
4494
+ * from print_submodule_conflict_suggestion() below. For RTL
4495
+ * languages, the following swap is suggested:
4496
+ * " - %s\n" -> "%s - \n"
4497
+ */
4498
+ strbuf_addf(&tmp, _(" - %s\n"), msg_list.items[i].string);
4499
+ else
4500
+ /*
4501
+ * TRANSLATORS: This is line item of submodule conflict message
4502
+ * from print_submodule_conflict_suggestion() below. For RTL
4503
+ * languages, the following swap is suggested:
4504
+ * " %s\n" -> "%s \n"
4505
+ */
4506
+ strbuf_addf(&tmp, _(" %s\n"), msg_list.items[i].string);
4507
+ }
4508
+ strbuf_reset(msg);
4509
+ strbuf_add(msg, tmp.buf, tmp.len);
4510
+}
4511
+
4512
+static void print_submodule_conflict_suggestion(struct string_list *csub) {
4513
+ struct string_list_item *item;
4514
+ struct strbuf msg = STRBUF_INIT;
4515
+ struct strbuf tmp = STRBUF_INIT;
4516
+ struct strbuf subs = STRBUF_INIT;
4517
+
4518
+ if (!csub->nr)
4519
+ return;
4520
+
4521
+ /*
4522
+ * NEEDSWORK: The steps to resolve these errors deserve a more
4523
+ * detailed explanation than what is currently printed below.
4524
+ */
4525
+ for_each_string_list_item(item, csub) {
4526
+ struct conflicted_submodule_item *util = item->util;
4527
+
4528
+ if (util->flag == CONFLICT_SUBMODULE_NOT_INITIALIZED ||
4529
+ util->flag == CONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE)
4530
+ return;
4531
+ }
4532
+
4533
+ printf(_("Recursive merging with submodules currently only supports "
4534
+ "trivial cases.\nPlease manually handle the merging of each "
4535
+ "conflicted submodule.\nThis can be accomplished with the following "
4536
+ "steps:"));
4537
+ putchar('\n');
4538
+
4539
+ for_each_string_list_item(item, csub) {
4540
+ struct conflicted_submodule_item *util = item->util;
4541
+ /*
4542
+ * TRANSLATORS: This is a line of advice to resolve a merge conflict
4543
+ * in a submodule. The second argument is the abbreviated id of the
4544
+ * commit that needs to be merged.
4545
+ * E.g. - go to submodule (sub), and either merge commit abc1234"
4546
+ */
4547
+ strbuf_addf(&tmp, _("go to submodule (%s), and either merge commit %s\n"
4548
+ "or update to an existing commit which has merged those changes"),
4549
+ item->string, util->abbrev);
4550
+ format_submodule_conflict_suggestion(&tmp);
4551
+ strbuf_add(&msg, tmp.buf, tmp.len);
4552
+ strbuf_reset(&tmp);
4553
+ }
4554
+ strbuf_add_separated_string_list(&subs, " ", csub);
4555
+ strbuf_addstr(&tmp, _("come back to superproject and run:"));
4556
+ strbuf_addf(&tmp, "\n\ngit add %s\n\n", subs.buf);
4557
+ strbuf_addstr(&tmp, _("to record the above merge or update"));
4558
+ format_submodule_conflict_suggestion(&tmp);
4559
+ strbuf_add(&msg, tmp.buf, tmp.len);
4560
+ strbuf_reset(&tmp);
4561
+
4562
+ strbuf_addstr(&tmp, _("resolve any other conflicts in the superproject"));
4563
+ format_submodule_conflict_suggestion(&tmp);
4564
+ strbuf_add(&msg, tmp.buf, tmp.len);
4565
+ strbuf_reset(&tmp);
4566
+
4567
+ strbuf_addstr(&tmp, _("commit the resulting index in the superproject"));
4568
+ format_submodule_conflict_suggestion(&tmp);
4569
+ strbuf_add(&msg, tmp.buf, tmp.len);
4570
+
4571
+ printf("%s", msg.buf);
4572
+ strbuf_release(&subs);
4573
+ strbuf_release(&tmp);
4574
+ strbuf_release(&msg);
4575
+}
4576
+
4577
void merge_display_update_messages(struct merge_options *opt,
4578
int detailed,
4579
struct merge_result *result)
@@ -4483,6 +4623,8 @@ void merge_display_update_messages(struct merge_options *opt,
4623
}
4624
string_list_clear(&olist, 0);
4625
4626
+ print_submodule_conflict_suggestion(&opti->conflicted_submodules);
4627
+
4628
/* Also include needed rename limit adjustment now */
4629
diff_warn_rename_limit("merge.renamelimit",
4630
opti->renames.needed_limit, 0);
@@ -4679,6 +4821,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
4821
trace2_region_enter("merge", "allocate/init", opt->repo);
4822
if (opt->priv) {
4823
clear_or_reinit_internal_opts(opt->priv, 1);
4824
+ string_list_init_nodup(&opt->priv->conflicted_submodules);
4825
trace2_region_leave("merge", "allocate/init", opt->repo);
4826
return;
4827
}
t/t6437-submodule-merge.sh
+72
-6
@@ -103,8 +103,25 @@ test_expect_success 'setup for merge search' '
103
echo "file-c" > file-c &&
104
git add file-c &&
105
git commit -m "sub-c") &&
106
- git commit -a -m "c" &&
106
+ git commit -a -m "c")
107
+'
108
109
+test_expect_success 'merging should conflict for non fast-forward' '
110
+ test_when_finished "git -C merge-search reset --hard" &&
111
+ (cd merge-search &&
112
+ git checkout -b test-nonforward-a b &&
113
+ if test "$GIT_TEST_MERGE_ALGORITHM" = ort
114
+ then
115
+ test_must_fail git merge c >actual &&
116
+ sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" &&
117
+ grep "$sub_expect" actual
118
+ else
119
+ test_must_fail git merge c 2> actual
120
+ fi)
121
+'
122
+
123
+test_expect_success 'finish setup for merge-search' '
124
+ (cd merge-search &&
125
git checkout -b d a &&
126
(cd sub &&
127
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' '
146
test_cmp expect actual)
147
'
148
132
-test_expect_success 'merging should conflict for non fast-forward' '
149
+test_expect_success 'merging should conflict for non fast-forward (resolution exists)' '
150
(cd merge-search &&
134
- git checkout -b test-nonforward b &&
151
+ git checkout -b test-nonforward-b b &&
152
(cd sub &&
153
git rev-parse --short sub-d > ../expect) &&
154
if test "$GIT_TEST_MERGE_ALGORITHM" = ort
155
then
139
- test_must_fail git merge c >actual
156
+ test_must_fail git merge c >actual &&
157
+ sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" &&
158
+ grep "$sub_expect" actual
159
else
160
test_must_fail git merge c 2> actual
161
fi &&
@@ -161,7 +180,9 @@ test_expect_success 'merging should fail for ambiguous common parent' '
180
) &&
181
if test "$GIT_TEST_MERGE_ALGORITHM" = ort
182
then
164
- test_must_fail git merge c >actual
183
+ test_must_fail git merge c >actual &&
184
+ sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" &&
185
+ grep "$sub_expect" actual
186
else
187
test_must_fail git merge c 2> actual
188
fi &&
@@ -205,7 +226,12 @@ test_expect_success 'merging should fail for changes that are backwards' '
226
git commit -a -m "f" &&
227
228
git checkout -b test-backward e &&
208
- test_must_fail git merge f)
229
+ test_must_fail git merge f >actual &&
230
+ if test "$GIT_TEST_MERGE_ALGORITHM" = ort
231
+ then
232
+ sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-d)" &&
233
+ grep "$sub_expect" actual
234
+ fi)
235
'
236
237
@@ -476,4 +502,44 @@ test_expect_failure 'directory/submodule conflict; merge --abort works afterward
502
)
503
'
504
505
+# Setup:
506
+# - Submodule has 2 commits: a and b
507
+# - Superproject branch 'a' adds and commits submodule pointing to 'commit a'
508
+# - Superproject branch 'b' adds and commits submodule pointing to 'commit b'
509
+# If these two branches are now merged, there is no merge base
510
+test_expect_success 'setup for null merge base' '
511
+ mkdir no-merge-base &&
512
+ (cd no-merge-base &&
513
+ git init &&
514
+ mkdir sub &&
515
+ (cd sub &&
516
+ git init &&
517
+ echo "file-a" > file-a &&
518
+ git add file-a &&
519
+ git commit -m "commit a") &&
520
+ git commit --allow-empty -m init &&
521
+ git branch init &&
522
+ git checkout -b a init &&
523
+ git add sub &&
524
+ git commit -m "a" &&
525
+ git switch main &&
526
+ (cd sub &&
527
+ echo "file-b" > file-b &&
528
+ git add file-b &&
529
+ git commit -m "commit b"))
530
+'
531
+
532
+test_expect_success 'merging should fail with no merge base' '
533
+ (cd no-merge-base &&
534
+ git checkout -b b init &&
535
+ git add sub &&
536
+ git commit -m "b" &&
537
+ test_must_fail git merge a >actual &&
538
+ if test "$GIT_TEST_MERGE_ALGORITHM" = ort
539
+ then
540
+ sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short HEAD^1)" &&
541
+ grep "$sub_expect" actual
542
+ fi)
543
+'
544
+
545
test_done
t/t7402-submodule-rebase.sh
+7
-2
@@ -104,7 +104,7 @@ test_expect_success 'rebasing submodule that should conflict' '
104
test_tick &&
105
git commit -m fourth &&
106
107
- test_must_fail git rebase --onto HEAD^^ HEAD^ HEAD^0 &&
107
+ test_must_fail git rebase --onto HEAD^^ HEAD^ HEAD^0 >actual_output &&
108
git ls-files -s submodule >actual &&
109
(
110
cd submodule &&
@@ -112,7 +112,12 @@ test_expect_success 'rebasing submodule that should conflict' '
112
echo "160000 $(git rev-parse HEAD^^) 2 submodule" &&
113
echo "160000 $(git rev-parse HEAD) 3 submodule"
114
) >expect &&
115
- test_cmp expect actual
115
+ test_cmp expect actual &&
116
+ if test "$GIT_TEST_MERGE_ALGORITHM" = ort
117
+ then
118
+ sub_expect="go to submodule (submodule), and either merge commit $(git -C submodule rev-parse --short HEAD^0)" &&
119
+ grep "$sub_expect" actual_output
120
+ fi
121
'
122
123
test_done