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