Re: [PATCH v2 6/8] merge-ort: format messages slightly different for use in headers

From: Johannes Altmanninger <aclopte@gmail.com>
Date: Dec 28, 2021, 10:56 UTC
Message-ID: <20211228105642.ajmr2kqjm5yikneu@gmail.com>
In-Reply-To: <887e46435c0561e86f1858682fe53e9de926b69c.1640419160.git.gitgitgadget@gmail.com>

On Sat, Dec 25, 2021 at 07:59:17AM +0000, Elijah Newren via GitGitGadget wrote:

Show 12 quoted lines

> From: Elijah Newren <newren@gmail.com>
> 
> When users run
>     git show --remerge-diff $MERGE_COMMIT
> or
>     git log -p --remerge-diff ...
> stdout is not an appropriate location to dump conflict messages, but we
> do want to provide them to users.  We will include them in the diff
> headers instead...but for that to work, we need for any multiline
> messages to replace newlines with both a newline and a space.  Add a new
> flag to signal when we want these messages modified in such a fashion,
> and use it in path_msg() to modify these messages this way.

makes sense (same for patches 4 & 5)

Show 50 quoted lines

> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-ort.c       | 36 ++++++++++++++++++++++++++++++++++--
>  merge-recursive.c |  3 +++
>  merge-recursive.h |  1 +
>  3 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/merge-ort.c b/merge-ort.c
> index 998e92ec593..9142d56e0ad 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -634,17 +634,46 @@ static void path_msg(struct merge_options *opt,
>  		     const char *fmt, ...)
>  {
>  	va_list ap;
> -	struct strbuf *sb = strmap_get(&opt->priv->output, path);
> +	struct strbuf *sb, *dest;
> +	struct strbuf tmp = STRBUF_INIT;
> +
> +	if (opt->record_conflict_msgs_as_headers && omittable_hint)
> +		return; /* Do not record mere hints in tree */
> +	sb = strmap_get(&opt->priv->output, path);
>  	if (!sb) {
>  		sb = xmalloc(sizeof(*sb));
>  		strbuf_init(sb, 0);
>  		strmap_put(&opt->priv->output, path, sb);
>  	}
>  
> +	dest = (opt->record_conflict_msgs_as_headers ? &tmp : sb);
> +
>  	va_start(ap, fmt);
> -	strbuf_vaddf(sb, fmt, ap);
> +	strbuf_vaddf(dest, fmt, ap);
>  	va_end(ap);
>  
> +	if (opt->record_conflict_msgs_as_headers) {
> +		int i_sb = 0, i_tmp = 0;
> +
> +		/* Copy tmp to sb, adding spaces after newlines */
> +		strbuf_grow(sb, 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];
> +
> +			/* If we copied a newline, add a space */
> +			if (tmp.buf[i_tmp] == '\n')
> +				sb->buf[++i_sb] = ' ';
> +		}
> +		/* Update length and ensure it's NUL-terminated */

I think this and the two comments inside the loop are mostly redundant. I'd drop them (except maybe this one because it's a common oversight I guess).

> +		sb->len += i_sb;
> +		sb->buf[sb->len] = '\0';
> +
> +		/* Clean up tmp */

Also this one I guess

Show 46 quoted lines

> +		strbuf_release(&tmp);
> +	}
> +
> +	/* Add final newline character to sb */
>  	strbuf_addch(sb, '\n');
>  }
>  
> @@ -4246,6 +4275,9 @@ void merge_switch_to_result(struct merge_options *opt,
>  		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");
> +
>  		trace2_region_enter("merge", "display messages", opt->repo);
>  
>  		/* Hack to pre-allocate olist to the desired size */
> diff --git a/merge-recursive.c b/merge-recursive.c
> index bc73c52dd84..c9ba7e904a6 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -3714,6 +3714,9 @@ static int merge_start(struct merge_options *opt, struct tree *head)
>  
>  	assert(opt->priv == NULL);
>  
> +	/* Not supported; option specific to merge-ort */
> +	assert(!opt->record_conflict_msgs_as_headers);
> +
>  	/* Sanity check on repo state; index must match head */
>  	if (repo_index_has_changes(opt->repo, head, &sb)) {
>  		err(opt, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
> diff --git a/merge-recursive.h b/merge-recursive.h
> index 0795a1d3ec1..ebfdb7f994e 100644
> --- a/merge-recursive.h
> +++ b/merge-recursive.h
> @@ -46,6 +46,7 @@ struct merge_options {
>  	/* miscellaneous control options */
>  	const char *subtree_shift;
>  	unsigned renormalize : 1;
> +	unsigned record_conflict_msgs_as_headers : 1;
>  
>  	/* internal fields used by the implementation */
>  	struct merge_options_internal *priv;
> -- 
> gitgitgadget
>