git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v3] submodule merge: update conflict error message

From
Calvin Wan <calvinwan@google.com>
Date
Jun 29, 2022, 22:40 UTC
Message-ID
<20220629224059.1016645-1-calvinwan@google.com>
In-Reply-To
<20220610231152.2594428-1-calvinwan@google.com>

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(-)
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

base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085

-- 
2.37.0.rc0.161.g10f37bed90-goog
Previous: Calvin WanNext: Elijah Newren
Message 14 of 60 in “submodule merge: update conflict error message”
  1. submodule merge: update conflict error messageCalvin Wan, Jun 6, 2022
  2. Junio C HamanoJun 7, 2022
  3. Calvin WanJun 8, 2022
  4. Glen ChooJun 8, 2022
  5. Calvin WanJun 8, 2022
  6. Junio C HamanoJun 8, 2022
  7. submodule merge: update conflict error messageCalvin Wan, Jun 10, 2022
  8. Elijah NewrenJun 11, 2022
  9. Philippe BlainJun 11, 2022
  10. Elijah NewrenJun 12, 2022
  11. Calvin WanJun 13, 2022
  12. Junio C HamanoJun 12, 2022
  13. Calvin WanJun 13, 2022
  14. submodule merge: update conflict error messageCalvin Wan, Jun 29, 2022
  15. Elijah NewrenJun 30, 2022
  16. Calvin WanJun 30, 2022
  17. Elijah NewrenJul 1, 2022
  18. Glen ChooJun 30, 2022
  19. Glen ChooJun 30, 2022
  20. Calvin WanJun 30, 2022
  21. submodule merge: update conflict error messageCalvin Wan, Jul 12, 2022
  22. Junio C HamanoJul 13, 2022
  23. Elijah NewrenJul 17, 2022
  24. Johannes SchindelinJul 15, 2022
  25. Junio C HamanoJul 16, 2022
  26. Elijah NewrenJul 17, 2022
  27. Calvin WanJul 18, 2022
  28. submodule merge: update conflict error messageCalvin Wan, Jul 18, 2022
  29. Junio C HamanoJul 19, 2022
  30. Calvin WanJul 19, 2022
  31. Junio C HamanoJul 19, 2022
  32. Junio C HamanoJul 19, 2022
  33. Calvin WanJul 19, 2022
  34. Junio C HamanoJul 19, 2022
  35. Ævar Arnfjörð BjarmasonJul 25, 2022
  36. Ævar Arnfjörð BjarmasonJul 25, 2022
  37. Calvin WanJul 25, 2022
  38. Ævar Arnfjörð BjarmasonJul 25, 2022
  39. Calvin WanJul 25, 2022
  40. submodule merge: update conflict error messageCalvin Wan, Jul 26, 2022
  41. Elijah NewrenJul 27, 2022
  42. Calvin WanJul 27, 2022
  43. Elijah NewrenJul 28, 2022
  44. Calvin WanJul 28, 2022
  45. Ævar Arnfjörð BjarmasonJul 27, 2022
  46. submodule merge: update conflict error messageCalvin Wan, Jul 28, 2022
  47. Ævar Arnfjörð BjarmasonJul 28, 2022
  48. Elijah NewrenJul 29, 2022
  49. Calvin WanAug 1, 2022
  50. Ævar Arnfjörð BjarmasonAug 1, 2022
  51. Junio C HamanoAug 2, 2022
  52. Calvin WanAug 2, 2022
  53. Junio C HamanoAug 2, 2022
  54. Calvin WanAug 2, 2022
  55. Junio C HamanoAug 2, 2022
  56. submodule merge: update conflict error messageCalvin Wan, Aug 4, 2022
  57. Junio C HamanoAug 16, 2022
  58. Junio C HamanoAug 16, 2022
  59. Calvin WanAug 16, 2022
  60. Junio C HamanoAug 16, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.