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

Derrick Stolee
2024-06-28T02:09:44Z

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