From: Derrick Stolee Date: Fri, 28 Jun 2024 02:09:42 GMT Subject: Re: [PATCH v2 2/7] merge-ort: maintain expected invariant for priv member Message-ID: <0df65763-9419-4ed4-ba28-a82ea1ac6dcb@gmail.com> In-Reply-To: On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote: > From: Elijah Newren > > 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