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

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Jul 25, 2022, 12:11 UTC
Message-ID
<220725.86r129pd8m.gmgdl@evledraar.gmail.com>
In-Reply-To
<220725.868rohragt.gmgdl@evledraar.gmail.com>
On Mon, Jul 25 2022, Ævar Arnfjörð Bjarmason wrote:
Show 59 quoted lines
> On Mon, Jul 18 2022, Calvin Wan wrote:
>
>
> One of the CI failures in "seen" is because of my topic to mark (among
> other things) t1500*.sh as passing with SANITIZE=leak, and this change.
>
> Because...
>
>> ..
>>  	object_array_clear(&merges);
>>  cleanup:
>> +	if (!ret) {
>> +		if (!csub) {
>> +			CALLOC_ARRAY(csub, 1);
>> +		}
>> +		csub_item.oid = xstrdup(repo_find_unique_abbrev(&subrepo, b, DEFAULT_ABBREV));
>> +		csub_item.path = xstrdup(path);
>> +		csub_item.resolution_exists = resolution_exists;
>> +		ALLOC_GROW(csub->items, csub->nr + 1, csub->alloc);
>
> ... in "cleanup" we're ALLOC_GROW()-ing? I haven't looked into this yet,
> but this seems susppect. This is line 1879 in the following stacktrace:
> 	
> 	+ git -C super merge branch1
> 	Failed to merge submodule dir/sub
> 	CONFLICT (submodule): Merge conflict in dir/sub
> 	Recursive merging with submodules currently only supports trivial cases.
> 	Please manually handle the merging of each conflicted submodule.
> 	This can be accomplished with the following steps:
> 	 - go to submodule (dir/sub), and either merge commit 7018b5f
> 	or update to an existing commit which has merged those changes
> 	 - come back to superproject, and `git add dir/sub` to record the above merge or update
> 	 - resolve any other conflicts in the superproject
> 	 - commit the resulting index in the superproject
> 	Automatic merge failed; fix conflicts and then commit the result.
> 	
> 	=================================================================
> 	==31261==ERROR: LeakSanitizer: detected memory leaks
> 	
> 	Direct leak of 576 byte(s) in 1 object(s) allocated from:
> 	    #0 0x4565ad in __interceptor_realloc (git+0x4565ad)
> 	    #1 0x76ecfd in xrealloc wrapper.c:136:8
> 	    #2 0x64fcd3 in merge_submodule merge-ort.c:1879:3
> 	    #3 0x64ee9b in handle_content_merge merge-ort.c:2118:11
> 	    #4 0x651c14 in process_entry merge-ort.c:4056:17
> 	    #5 0x648c05 in process_entries merge-ort.c:4267:4
> 	    #6 0x646c03 in merge_ort_nonrecursive_internal merge-ort.c:4893:2
> 	    #7 0x6470f3 in merge_ort_internal merge-ort.c:4982:2
> 	    #8 0x646de0 in merge_incore_recursive merge-ort.c:5033:2
> 	    #9 0x652d1a in merge_ort_recursive merge-ort-wrappers.c:57:2
> 	    #10 0x4ec0f6 in try_merge_strategy builtin/merge.c:764:12
> 	    #11 0x4e9bf2 in cmd_merge builtin/merge.c:1710:9
> 	    #12 0x45a3aa in run_builtin git.c:466:11
> 	    #13 0x458e41 in handle_builtin git.c:720:3
> 	    #14 0x459d85 in run_argv git.c:787:4
> 	    #15 0x458bfa in cmd_main git.c:920:19
> 	    #16 0x56a049 in main common-main.c:56:11
> 	    #17 0x7fe592bca81c in __libc_start_main csu/../csu/libc-start.c:332:16
> 	    #18 0x431139 in _start (git+0x431139)

Looking at this a bit more this fixes it, and also the NIH allocation pattern & what we can do with a "struct string_list" instead of managing our own allocations.

I did wonder why you need to allocate the "oid" at all, i.e. at the time of merge_submodules() can't we just squirrel away the "const struct object_id *" and only when we emit the message later run repo_find_unique_abbrev()?

I think we can do that, but it means re-init-ing the submodule, and I'm not sure if there's any edge cases with the state changing between us trying merge_submodules() and the eventual messages we emit.

But it would mean that we'd just need to allocate a pointer to that oid
+ resolution_exists.

Perhaps we could also continue down that path and do this with just one allocation for that "oid/resolution_exists" side-data. I.e. you'd have a second string_list in the struct, have a non-NULL util mean "resolution exists", and the "string" there would be nodup'd (we could refer to the "string" in the first one).

But I think all of that probably isn't worth it, but the below seems like a good trade-off & good place to stop, and fixes the memory leak this topic introduces.

 merge-ort.c | 69 +++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 40 insertions(+), 29 deletions(-)
diff --git a/merge-ort.c b/merge-ort.c
index 154d33f2f45..5cf87f70b58 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -293,16 +293,17 @@ struct rename_info {
 };
 
 struct conflicted_submodule_item {
-	char *oid;
-	char *path;
-	int resolution_exists;
+	char *abbrev;
+	int resolution_exists:1;
 };
 
-struct conflicted_submodule_list {
-	struct conflicted_submodule_item *items;
-	size_t nr;
-	size_t alloc;
-};
+static void conflicted_submodule_item_free(void *util, const char *str)
+{
+	struct conflicted_submodule_item *item = util;
+
+	free(item->abbrev);
+	free(item);
+}
 
 struct merge_options_internal {
 	/*
@@ -401,7 +402,7 @@ struct merge_options_internal {
 	int call_depth;
 
 	/* field that holds submodule conflict information */
-	struct conflicted_submodule_list conflicted_submodules;
+	struct string_list conflicted_submodules;
 };
 
 struct version_info {
@@ -701,6 +702,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;
@@ -1756,8 +1760,6 @@ 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->priv->conflicted_submodules;
-	struct conflicted_submodule_item csub_item;
 	int resolution_exists = 0;
 
 	int i;
@@ -1870,15 +1872,17 @@ static int merge_submodule(struct merge_options *opt,
 	object_array_clear(&merges);
 cleanup:
 	if (!ret) {
-		if (!csub) {
-			CALLOC_ARRAY(csub, 1);
-		}
-		csub_item.oid = xstrdup(repo_find_unique_abbrev(&subrepo, b, DEFAULT_ABBREV));
-		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->priv->conflicted_submodules = *csub;
+		struct string_list *csub = &opt->priv->conflicted_submodules;
+		struct conflicted_submodule_item *util;
+		const char *abbrev;
+
+		abbrev = repo_find_unique_abbrev(&subrepo, b, DEFAULT_ABBREV);;
+
+		util = xmalloc(sizeof(*util));
+		util->abbrev = xstrdup(abbrev);
+		util->resolution_exists = resolution_exists;
+
+		string_list_append(csub, path)->util = util;
 	}
 	repo_clear(&subrepo);
 	return ret;
@@ -4465,23 +4469,26 @@ static int record_conflicted_index_entries(struct merge_options *opt)
 	return errs;
 }
 
-static void print_submodule_conflict_suggestion(struct conflicted_submodule_list *csub) {
-	if (csub && csub->nr > 0) {
-		int i;
+static void print_submodule_conflict_suggestion(struct string_list *csub) {
+	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 (i = 0; i < csub->nr; i++) {
+
+		for_each_string_list_item(item, csub) {
+			struct conflicted_submodule_item *util = item->util;
+
 			printf(_(" - go to submodule (%s), and either merge commit %s\n"
 				    "or update to an existing commit which has merged those changes\n"),
-					csub->items[i].path,
-					csub->items[i].oid);
-			if (csub->items[i].resolution_exists)
+					item->string, util->abbrev);
+			if (util->resolution_exists)
 				printf(_("such as one listed above\n"));
 		}
 		printf(_(" - come back to superproject, and `git add"));
-		for (i = 0; i < csub->nr; i++)
-			printf(_(" %s"), csub->items[i].path);
+		for_each_string_list_item(item, csub)
+			printf(_(" %s"), item->string);
 		printf(_("` to record the above merge or update \n"
 			" - resolve any other conflicts in the superproject\n"
 			" - commit the resulting index in the superproject\n"));
@@ -4538,6 +4545,8 @@ void merge_display_update_messages(struct merge_options *opt,
 	string_list_clear(&olist, 0);
 
 	print_submodule_conflict_suggestion(&opti->conflicted_submodules);
+	string_list_clear_func(&opti->conflicted_submodules,
+			       conflicted_submodule_item_free);
 
 	/* Also include needed rename limit adjustment now */
 	diff_warn_rename_limit("merge.renamelimit",
@@ -4795,6 +4804,8 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
 	 */
 	strmap_init(&opt->priv->conflicts);
 
+	string_list_init_dup(&opt->priv->conflicted_submodules);
+
 	trace2_region_leave("merge", "allocate/init", opt->repo);
 }
Previous: Ævar Arnfjörð BjarmasonNext: Calvin Wan
Message 36 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.