Re: [PATCH v2 2/7] merge-ort: maintain expected invariant for priv member

[Derrick Stolee <stolee@gmail.com>]

On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> The calling convention for the merge machinery is
>     One call to          init_merge_options()
>     One or more calls to merge_incore_[non]recursive()
>     One call to          merge_finalize()
>        (possibly indirectly via merge_switch_to_result())
> Both merge_switch_to_result() and merge_finalize() expect
>     opt->priv == NULL && result->priv != NULL
> which is supposed to be set up by our move_opt_priv_to_result_priv()
> function.  However, two codepaths dealing with error cases did not
> execute this necessary logic, which could result in assertion failures
> (or, if assertions were compiled out, could result in segfaults).  Fix
> the oversight and add a test that would have caught one of these
> problems.

"one of these problems" is key here.

> @@ -5050,6 +5050,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
>   		    oid_to_hex(&side1->object.oid),
>   		    oid_to_hex(&side2->object.oid));
>   		result->clean = -1;
> +		move_opt_priv_to_result_priv(opt, result);
>   		return;
>   	} >   	trace2_region_leave("merge", "collect_merge_info", opt->repo);

Removing this line does not cause your test script to fail. This is
understandable as this case only happens during a parse failure, so
it would be unreasonable to generate a test case that only fails
for such a reason.

> @@ -5080,7 +5081,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
>   		/* existence of conflicted entries implies unclean */
>   		result->clean &= strmap_empty(&opt->priv->conflicted);
>   	}
> -	if (!opt->priv->call_depth)
> +	if (!opt->priv->call_depth || result->clean < 0)
>   		move_opt_priv_to_result_priv(opt, result);
>   }

Removing this change does get caught by the new test.

Thanks,
-Stolee