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

Re: [PATCH v7] submodule merge: update conflict error message

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 2, 2022, 00:50 UTC
Message-ID
<xmqqpmhjjwo9.fsf@gitster.g>
In-Reply-To
<20220728211221.2913928-1-calvinwan@google.com>
Calvin Wan <calvinwan@google.com> writes:
Show 31 quoted lines
>  merge-ort.c                 | 112 ++++++++++++++++++++++++++++++++++--
>  t/t6437-submodule-merge.sh  |  78 +++++++++++++++++++++++--
>  t/t7402-submodule-rebase.sh |   9 ++-
>  3 files changed, 185 insertions(+), 14 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 01f150ef3b..4302e900ee 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 {
> @@ -517,6 +520,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 +574,8 @@ static const char *type_short_descriptions[] = {
>  		"CONFLICT (submodule history not available)",
>  	[CONFLICT_SUBMODULE_MAY_HAVE_REWINDS] =
>  		"CONFLICT (submodule may have rewinds)",
The other ones are semi sentences ...
> +	[CONFLICT_SUBMODULE_NULL_MERGE_BASE] =
> +		"CONFLICT (submodule no merge base)"

... and this wants to become one, too, e.g. "submodule lacks merge base", perhaps?

>  };
Show 27 quoted lines
> @@ -686,6 +692,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;
> @@ -1744,26 +1752,40 @@ static int merge_submodule(struct merge_options *opt,
>  
>  	int i;
>  	int search = !opt->priv->call_depth;
> +	int sub_initialized = 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;
> +		BUG("submodule deleted on one side; this should be handled outside of merge_submodule()"); 
>  	if (is_null_oid(b))
> -		return 0;
> +		BUG("submodule deleted on one side; this should be handled outside of merge_submodule()");

OK. It is interesting that the first condition (i.e. 'o' is missing) we are removing is not about "we cannot handle deletion", so this change corrects the code to match the comment. As the BUG() messages are the same on these sides,

	if (is_null_oid(a) || is_null_oid(b))
		BUG(...);
may be easier to read, perhaps?
> -	if (repo_submodule_init(&subrepo, opt->repo, path, null_oid())) {
> +	if ((sub_initialized = repo_submodule_init(&subrepo,
> +									opt->repo, path, null_oid()))) {

I wonder where this overly deep indentation come from? Can the .editorconfig file we ship with the project help?

Show 10 quoted lines
>  		path_msg(opt, CONFLICT_SUBMODULE_NOT_INITIALIZED, 0,
>  			 path, NULL, NULL, NULL,
>  			 _("Failed to merge submodule %s (not checked out)"),
>  			 path);
> +		/*
> +		 * NEEDSWORK: Since the steps to resolve this error are
> +		 * more involved than what is currently in 
> +		 * print_submodule_conflict_suggestion(), we return
> +		 * immediately rather than generating an error message
> +		 */
OK.
Show 8 quoted lines
>  		return 0;
>  	}
>  
> +	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);
OK.
> +		goto cleanup;
> +	}
Show 10 quoted lines
> @@ -1849,7 +1871,15 @@ 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;
> +
> +		string_list_append(csub, path)->util =
> +				xstrdup(repo_find_unique_abbrev(&subrepo, b, DEFAULT_ABBREV));
This line could also lose one level of indent.

We record 'b' in its abbreviated form here, because the assumption is that when merging the superproject, 'a' (which is the assumed fallback answer wet up at the beginning of the furnction) is the commit recorded in our side of the superproject, and it is the commit checked out in the submodule, so the task of making the necessary merge in the submodule is to go to the submodule and then merge 'b' into HEAD. Makes sense.

Show 32 quoted lines
> @@ -4412,6 +4442,73 @@ static int record_conflicted_index_entries(struct merge_options *opt)
>  	return errs;
>  }
>  
> +static void print_submodule_conflict_suggestion(struct string_list *csub) {
> +	if (csub->nr > 0) {
> +		struct string_list_item *item;
> +		struct strbuf msg = STRBUF_INIT;
> +		struct strbuf tmp = STRBUF_INIT;
> +
> +		strbuf_addf(&tmp, _("Recursive merging with submodules currently only supports trivial cases."));
> +		strbuf_addf(&msg, "%s\n", tmp.buf);
> +		strbuf_release(&tmp);
> +
> +		strbuf_addf(&tmp, _("Please manually handle the merging of each conflicted submodule."));
> +		strbuf_addf(&msg, "%s\n", tmp.buf);
> +		strbuf_release(&tmp);
> +
> +		strbuf_addf(&tmp, _("This can be accomplished with the following steps:"));
> +		strbuf_addf(&msg, "%s\n", tmp.buf);
> +		strbuf_release(&tmp);
> +
> +		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"
> +			 */
> +			strbuf_addf(&tmp, _("go to submodule (%s), and either merge commit %s"),
> +													item->string, abbrev);

Is this also an instance of overly deep indentation (it is so deep that I cannot easily tell)?

When we say "either merge commit %s" (where %s is 'b'---what they have as we saw earlier), do we need to mention what the value of 'a' is to the user, or is it redundant because we are absolutely sure that 'a' is what is checked out in the submodule?

> +			strbuf_addf(&msg, _(" - %s"), tmp.buf);
> +			strbuf_addf(&msg, "\n");
> +			strbuf_release(&tmp);
> +			strbuf_addf(&tmp, _("or update to an existing commit which has merged those changes"));

"those changes" means "a..b"? Again, I just want to make sure that the user in this situation knows "the other end" of the range when they are told only about 'b'.

Show 34 quoted lines
> +			strbuf_addf(&msg, _("   %s"), tmp.buf);
> +			strbuf_addf(&msg, "\n");
> +			strbuf_release(&tmp);
> +		}
> +		strbuf_addf(&tmp, _("come back to superproject and run:"));
> +		strbuf_addf(&msg, _(" - %s"), tmp.buf);
> +		strbuf_addf(&msg, "\n\n");
> +		strbuf_release(&tmp);
> +
> +		strbuf_addf(&tmp, "git add ");
> +		strbuf_add_separated_string_list(&tmp, " ", csub);
> +		strbuf_addf(&msg, _("       %s"), tmp.buf);
> +		strbuf_addf(&msg, "\n\n");
> +		strbuf_release(&tmp);
> +
> +		strbuf_addf(&tmp, _("to record the above merge or update"));
> +		strbuf_addf(&msg, _("   %s"), tmp.buf);
> +		strbuf_addf(&msg, "\n");
> +		strbuf_release(&tmp);
> +
> +		strbuf_addf(&tmp, _("resolve any other conflicts in the superproject"));
> +		strbuf_addf(&msg, _(" - %s"), tmp.buf);
> +		strbuf_addf(&msg, "\n");
> +		strbuf_release(&tmp);
> +
> +		strbuf_addf(&tmp, _("commit the resulting index in the superproject"));
> +		strbuf_addf(&msg, _(" - %s"), tmp.buf);
> +		strbuf_addf(&msg, "\n");
> +		strbuf_release(&tmp);
> +
> +		printf("%s", msg.buf);
> +		strbuf_release(&msg);
> +	}
> +}
OK.  Makes sense.
Thanks.
Previous: Ævar Arnfjörð BjarmasonNext: Calvin Wan
Message 51 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.