Re: [PATCH 05/11] merge-ort: let renormalization change modify/delete into clean delete

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> When we have a modify/delete conflict, but the only change to the
> modification is e.g. change of line endings, then if renormalization is
> requested then we should be able to recognize such a case as a
> not-modified/delete and resolve the conflict automatically.
>
> This fixes t6418.10 under GIT_TEST_MERGE_ALGORITHM=ort.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-ort.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 87c553c0882c..c4bd88b9d3db 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -2416,6 +2416,60 @@ static int string_list_df_name_compare(const char *one, const char *two)
>  	return onelen - twolen;
>  }
>  
> +static int read_oid_strbuf(struct merge_options *opt,
> +			   const struct object_id *oid,
> +			   struct strbuf *dst)
> +{
> +	void *buf;
> +	enum object_type type;
> +	unsigned long size;
> +	buf = read_object_file(oid, &type, &size);
> +	if (!buf)
> +		return err(opt, _("cannot read object %s"), oid_to_hex(oid));
> +	if (type != OBJ_BLOB) {
> +		free(buf);
> +		return err(opt, _("object %s is not a blob"), oid_to_hex(oid));

As an aside I've got another series I'll submit soon which refactors all
these "object is not xyz" calls to a utility function, so in this case
we'd also say what it was other than a blob.

Fine to keep this here, just a #leftoverbits note to myself to
eventually migrate this.

> +	}
> +	strbuf_attach(dst, buf, size, size + 1);
> +	return 0;
> +}
> +
> +static int blob_unchanged(struct merge_options *opt,
> +			  const struct version_info *base,
> +			  const struct version_info *side,
> +			  const char *path)
> +{
> +	struct strbuf basebuf = STRBUF_INIT;
> +	struct strbuf sidebuf = STRBUF_INIT;
> +	int ret = 0; /* assume changed for safety */
> +	const struct index_state *idx = &opt->priv->attr_index;
> +
> +	initialize_attr_index(opt);
> +
> +	if (base->mode != side->mode)
> +		return 0;
> +	if (oideq(&base->oid, &side->oid))
> +		return 1;
> +
> +	if (read_oid_strbuf(opt, &base->oid, &basebuf) ||
> +	    read_oid_strbuf(opt, &side->oid, &sidebuf))
> +		goto error_return;
> +	/*
> +	 * Note: binary | is used so that both renormalizations are
> +	 * performed.  Comparison can be skipped if both files are
> +	 * unchanged since their sha1s have already been compared.
> +	 */
> +	if (renormalize_buffer(idx, path, basebuf.buf, basebuf.len, &basebuf) |
> +	    renormalize_buffer(idx, path, sidebuf.buf, sidebuf.len, &sidebuf))
> +		ret = (basebuf.len == sidebuf.len &&
> +		       !memcmp(basebuf.buf, sidebuf.buf, basebuf.len));
> +
> +error_return:
> +	strbuf_release(&basebuf);
> +	strbuf_release(&sidebuf);
> +	return ret;
> +}
> +
>
>  struct directory_versions {
>  	/*
>  	 * versions: list of (basename -> version_info)
> @@ -3003,8 +3057,13 @@ static void process_entry(struct merge_options *opt,
>  		modify_branch = (side == 1) ? opt->branch1 : opt->branch2;
>  		delete_branch = (side == 1) ? opt->branch2 : opt->branch1;
>  
> -		if (ci->path_conflict &&
> -		    oideq(&ci->stages[0].oid, &ci->stages[side].oid)) {
> +		if (opt->renormalize &&
> +		    blob_unchanged(opt, &ci->stages[0], &ci->stages[side],
> +				   path)) {
> +			ci->merged.is_null = 1;
> +			ci->merged.clean = 1;
> +		} else if (ci->path_conflict &&
> +			   oideq(&ci->stages[0].oid, &ci->stages[side].oid)) {

Small note (no need for re-roll or whatever) on having read a bit of
merge-ort.c code recently: I'd find this thing a bit easier on the eyes
if ci->stages[0] and ci->stages[side] were split into a variable before
the if/else, i.e. used as "side_0.oid and side_n.oid" and "side_0 and
side_n" in this case..

That would also avoid the wrapping of at least one argument list here.