[PATCH v7 11/17] merge-ort: store messages in a list, not in a single strbuf

["Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

To prepare for using the `merge-ort` machinery in server operations, we
cannot simply produce a free-form string that combines a variable-length
list of messages.

Instead, we need to list them one by one. The natural fit for this is a
`string_list`.

We will subsequently add even more information in the `util` attribute
of the string list items.

Based-on-a-patch-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 123 ++++++++++++++++++++++++++++++++++------------------
 merge-ort.h |   2 +-
 2 files changed, 81 insertions(+), 44 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 7e8b9cd6ea7..668aec64f13 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -349,13 +349,15 @@ struct merge_options_internal {
 	struct mem_pool pool;
 
 	/*
-	 * output: special messages and conflict notices for various paths
+	 * conflicts: logical conflicts and messages stored by _primary_ path
 	 *
 	 * This is a map of pathnames (a subset of the keys in "paths" above)
-	 * to strbufs.  It gathers various warning/conflict/notice messages
-	 * for later processing.
+	 * to struct string_list, with each item's `util` containing a
+	 * `struct logical_conflict_info`. Note, though, that for each path,
+	 * it only stores the logical conflicts for which that path is the
+	 * primary path; the path might be part of additional conflicts.
 	 */
-	struct strmap output;
+	struct strmap conflicts;
 
 	/*
 	 * renames: various data relating to rename detection
@@ -567,20 +569,20 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
 		struct strmap_entry *e;
 
 		/* Release and free each strbuf found in output */
-		strmap_for_each_entry(&opti->output, &iter, e) {
-			struct strbuf *sb = e->value;
-			strbuf_release(sb);
+		strmap_for_each_entry(&opti->conflicts, &iter, e) {
+			struct string_list *list = e->value;
 			/*
-			 * While strictly speaking we don't need to free(sb)
-			 * here because we could pass free_values=1 when
-			 * calling strmap_clear() on opti->output, that would
-			 * require strmap_clear to do another
-			 * strmap_for_each_entry() loop, so we just free it
-			 * while we're iterating anyway.
+			 * While strictly speaking we don't need to
+			 * free(conflicts) here because we could pass
+			 * free_values=1 when calling strmap_clear() on
+			 * opti->conflicts, that would require strmap_clear
+			 * to do another strmap_for_each_entry() loop, so we
+			 * just free it while we're iterating anyway.
 			 */
-			free(sb);
+			string_list_clear(list, 1);
+			free(list);
 		}
-		strmap_clear(&opti->output, 0);
+		strmap_clear(&opti->conflicts, 0);
 	}
 
 	mem_pool_discard(&opti->pool, 0);
@@ -634,7 +636,9 @@ static void path_msg(struct merge_options *opt,
 		     const char *fmt, ...)
 {
 	va_list ap;
-	struct strbuf *sb, *dest;
+	struct string_list *path_conflicts;
+	struct strbuf buf = STRBUF_INIT;
+	struct strbuf *dest;
 	struct strbuf tmp = STRBUF_INIT;
 
 	if (opt->record_conflict_msgs_as_headers && omittable_hint)
@@ -642,14 +646,16 @@ static void path_msg(struct merge_options *opt,
 	if (opt->priv->call_depth && opt->verbosity < 5)
 		return; /* Ignore messages from inner merges */
 
-	sb = strmap_get(&opt->priv->output, path);
-	if (!sb) {
-		sb = xmalloc(sizeof(*sb));
-		strbuf_init(sb, 0);
-		strmap_put(&opt->priv->output, path, sb);
+	/* Ensure path_conflicts (ptr to array of logical_conflict) allocated */
+	path_conflicts = strmap_get(&opt->priv->conflicts, path);
+	if (!path_conflicts) {
+		path_conflicts = xmalloc(sizeof(*path_conflicts));
+		string_list_init_dup(path_conflicts);
+		strmap_put(&opt->priv->conflicts, path, path_conflicts);
 	}
 
-	dest = (opt->record_conflict_msgs_as_headers ? &tmp : sb);
+	/* Handle message and its format, in normal case */
+	dest = (opt->record_conflict_msgs_as_headers ? &tmp : &buf);
 
 	va_start(ap, fmt);
 	if (opt->priv->call_depth) {
@@ -660,32 +666,31 @@ static void path_msg(struct merge_options *opt,
 	strbuf_vaddf(dest, fmt, ap);
 	va_end(ap);
 
+	/* Handle specialized formatting of message under --remerge-diff */
 	if (opt->record_conflict_msgs_as_headers) {
 		int i_sb = 0, i_tmp = 0;
 
 		/* Start with the specified prefix */
 		if (opt->msg_header_prefix)
-			strbuf_addf(sb, "%s ", opt->msg_header_prefix);
+			strbuf_addf(&buf, "%s ", opt->msg_header_prefix);
 
 		/* Copy tmp to sb, adding spaces after newlines */
-		strbuf_grow(sb, sb->len + 2*tmp.len); /* more than sufficient */
+		strbuf_grow(&buf, buf.len + 2*tmp.len); /* more than sufficient */
 		for (; i_tmp < tmp.len; i_tmp++, i_sb++) {
 			/* Copy next character from tmp to sb */
-			sb->buf[sb->len + i_sb] = tmp.buf[i_tmp];
+			buf.buf[buf.len + i_sb] = tmp.buf[i_tmp];
 
 			/* If we copied a newline, add a space */
 			if (tmp.buf[i_tmp] == '\n')
-				sb->buf[++i_sb] = ' ';
+				buf.buf[++i_sb] = ' ';
 		}
 		/* Update length and ensure it's NUL-terminated */
-		sb->len += i_sb;
-		sb->buf[sb->len] = '\0';
+		buf.len += i_sb;
+		buf.buf[buf.len] = '\0';
 
 		strbuf_release(&tmp);
 	}
-
-	/* Add final newline character to sb */
-	strbuf_addch(sb, '\n');
+	string_list_append_nodup(path_conflicts, strbuf_detach(&buf, NULL));
 }
 
 static struct diff_filespec *pool_alloc_filespec(struct mem_pool *pool,
@@ -4256,7 +4261,6 @@ void merge_display_update_messages(struct merge_options *opt,
 	struct hashmap_iter iter;
 	struct strmap_entry *e;
 	struct string_list olist = STRING_LIST_INIT_NODUP;
-	int i;
 
 	if (opt->record_conflict_msgs_as_headers)
 		BUG("Either display conflict messages or record them as headers, not both");
@@ -4264,20 +4268,20 @@ void merge_display_update_messages(struct merge_options *opt,
 	trace2_region_enter("merge", "display messages", opt->repo);
 
 	/* Hack to pre-allocate olist to the desired size */
-	ALLOC_GROW(olist.items, strmap_get_size(&opti->output),
+	ALLOC_GROW(olist.items, strmap_get_size(&opti->conflicts),
 		   olist.alloc);
 
 	/* Put every entry from output into olist, then sort */
-	strmap_for_each_entry(&opti->output, &iter, e) {
+	strmap_for_each_entry(&opti->conflicts, &iter, e) {
 		string_list_append(&olist, e->key)->util = e->value;
 	}
 	string_list_sort(&olist);
 
 	/* Iterate over the items, printing them */
-	for (i = 0; i < olist.nr; ++i) {
-		struct strbuf *sb = olist.items[i].util;
-
-		printf("%s", sb->buf);
+	for (int path_nr = 0; path_nr < olist.nr; ++path_nr) {
+		struct string_list *conflicts = olist.items[path_nr].util;
+		for (int i = 0; i < conflicts->nr; i++)
+			puts(conflicts->items[i].string);
 	}
 	string_list_clear(&olist, 0);
 
@@ -4366,6 +4370,8 @@ void merge_finalize(struct merge_options *opt,
 		    struct merge_result *result)
 {
 	struct merge_options_internal *opti = result->priv;
+	struct hashmap_iter iter;
+	struct strmap_entry *e;
 
 	if (opt->renormalize)
 		git_attr_set_direction(GIT_ATTR_CHECKIN);
@@ -4373,6 +4379,15 @@ void merge_finalize(struct merge_options *opt,
 
 	clear_or_reinit_internal_opts(opti, 0);
 	FREE_AND_NULL(opti);
+
+	/* Release and free each strbuf found in path_messages */
+	strmap_for_each_entry(result->path_messages, &iter, e) {
+		struct strbuf *buf = e->value;
+
+		strbuf_release(buf);
+	}
+	strmap_clear(result->path_messages, 1);
+	FREE_AND_NULL(result->path_messages);
 }
 
 /*** Function Grouping: helper functions for merge_incore_*() ***/
@@ -4531,11 +4546,11 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
 	strmap_init_with_options(&opt->priv->conflicted, pool, 0);
 
 	/*
-	 * keys & strbufs in output will sometimes need to outlive "paths",
-	 * so it will have a copy of relevant keys.  It's probably a small
-	 * subset of the overall paths that have special output.
+	 * keys & string_lists in conflicts will sometimes need to outlive
+	 * "paths", so it will have a copy of relevant keys.  It's probably
+	 * a small subset of the overall paths that have special output.
 	 */
-	strmap_init(&opt->priv->output);
+	strmap_init(&opt->priv->conflicts);
 
 	trace2_region_leave("merge", "allocate/init", opt->repo);
 }
@@ -4596,6 +4611,8 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
 					    struct merge_result *result)
 {
 	struct object_id working_tree_oid;
+	struct hashmap_iter iter;
+	struct strmap_entry *e;
 
 	if (opt->subtree_shift) {
 		side2 = shift_tree_object(opt->repo, side1, side2,
@@ -4636,7 +4653,27 @@ redo:
 	trace2_region_leave("merge", "process_entries", opt->repo);
 
 	/* Set return values */
-	result->path_messages = &opt->priv->output;
+	result->path_messages = xcalloc(1, sizeof(*result->path_messages));
+	strmap_init_with_options(result->path_messages, NULL, 0);
+	strmap_for_each_entry(&opt->priv->conflicts, &iter, e) {
+		const char *path = e->key;
+		struct strbuf *buf = strmap_get(result->path_messages, path);
+		struct string_list *conflicts = e->value;
+
+		if (!buf) {
+			buf = xcalloc(1, sizeof(*buf));
+			strbuf_init(buf, 0);
+			strmap_put(result->path_messages, path, buf);
+		}
+
+		for (int i = 0; i < conflicts->nr; i++) {
+			if (buf->len)
+				strbuf_addch(buf, '\n');
+			strbuf_addstr(buf, conflicts->items[i].string);
+			strbuf_trim_trailing_newline(buf);
+		}
+	}
+
 	result->tree = parse_tree_indirect(&working_tree_oid);
 	/* existence of conflicted entries implies unclean */
 	result->clean &= strmap_empty(&opt->priv->conflicted);
diff --git a/merge-ort.h b/merge-ort.h
index ddcc39d7270..f9c536ed8c4 100644
--- a/merge-ort.h
+++ b/merge-ort.h
@@ -28,7 +28,7 @@ struct merge_result {
 	/*
 	 * Special messages and conflict notices for various paths
 	 *
-	 * This is a map of pathnames to strbufs.  It contains various
+	 * This is a map of pathnames to strbufs. It contains various
 	 * warning/conflict/notice messages (possibly multiple per path)
 	 * that callers may want to use.
 	 */
-- 
gitgitgadget