Re: [PATCH] merge-ort: fix bug with renormalization and rename/delete conflicts

[Derrick Stolee <stolee@gmail.com>]

On 12/27/2021 7:20 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> Ever since commit a492d5331c ("merge-ort: ensure we consult df_conflict
> and path_conflicts", 2021-06-30), when renormalization is active AND a
> file is involved in a rename/delete conflict BUT the file is unmodified
> (either before or after renormalization), merge-ort was running into an
> assertion failure. 

This "the file is unmodified" is critical, as when I looked at the test,
it seemed too simple. I asked myself, "Why does renormalization matter
here?" Turns out it is just an artifact of the carefully organized cases.

>  		if (opt->renormalize &&
>  		    blob_unchanged(opt, &ci->stages[0], &ci->stages[side],
>  				   path)) {
> -			ci->merged.is_null = 1;
> -			ci->merged.clean = 1;
> -			assert(!ci->df_conflict && !ci->path_conflict);
> +			if (!ci->path_conflict) {
> +				/*
> +				 * Blob unchanged after renormalization, so
> +				 * there's no modify/delete conflict after all;
> +				 * we can just remove the file.
> +				 */
> +				ci->merged.is_null = 1;
> +				ci->merged.clean = 1;
> +				 /*
> +				  * file goes away => even if there was a
> +				  * directory/file conflict there isn't one now.
> +				  */
> +				ci->df_conflict = 0;
> +			} else {
> +				/* rename/delete, so conflict remains */
> +			}

This breakdown of the cases is informative, and I like how self-contained
the change is.

> +test_expect_success 'rename/delete vs. renormalization' '
> +	git init subrepo &&
> +	(
> +		cd subrepo &&
> +		echo foo >oldfile &&
> +		git add oldfile &&
> +		git commit -m original &&
> +
> +		git branch rename &&
> +		git branch nuke &&
> +
> +		git checkout rename &&
> +		git mv oldfile newfile &&
> +		git commit -m renamed &&
> +
> +		git checkout nuke &&
> +		git rm oldfile &&
> +		git commit -m deleted &&
> +
> +		git checkout rename^0 &&
> +		test_must_fail git -c merge.renormalize=true merge nuke >out &&
> +
> +		grep "rename/delete" out
> +	)
> +'
> +
>  test_done

I tested this on the latest 'master' and saw the following:

  git: merge-ort.c:3846: process_entry: Assertion `!ci->df_conflict && !ci->path_conflict' failed

so it indeed hits this case.

This patch looks good to me. Thanks!

Reviewed-by: Derrick Stolee <dstolee@microsoft.com>