On Thu, Jun 13, 2024 at 10:59 PM Taylor Blau <me@xxxxxxxxxxxx> wrote:
>
> On Thu, Jun 13, 2024 at 08:25:02PM +0000, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@xxxxxxxxx>
> >
> > 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 codepath dealing with error cases did not
>
> s/codepath/&s/ ?
Indeed.
> > 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.
> >
> > While at it, also tighten an existing test for a non-recursive merge
> > to verify that it fails correctly, i.e. with the expected exit code
> > rather than with an assertion failure.
It turns out my logic here was faulty; if a
test_must_fail git ARGS...
command is run and git hits an assertion failure, the test_must_fail
will fail with
test_must_fail: died by signal 6: git ARGS...
similar to how it would fail if git were to segfault.
However, I still like the idea of testing the exit status here because
of this comment in builtin/merge.c:
/*
* The backend exits with 1 when conflicts are
* left to be resolved, with 2 when it does not
* handle the given merge at all.
*/
and this is one of the few tests in the testsuite where we are
explicitly testing a case where the merge is neither success nor
conflicts, but failed-to-handle.
> I suspect that this test was flaky, too, since if the assertion errors
> were compiled out and it died via a segfault, the test would have failed
> outright as test_must_fail does not allow for segfaults typically.
>
> So I'm glad to see us tightening up that area of the test suite.
[...]
> > + test_must_fail git merge base-b &&
>
> Here and below, do you care about the particular exit code of merging as
> above? IOW, should these be `test_expect_code`'s as well?
Both of these comments led me on the path to check something out and
discover my error above, which will lead to slightly different changes
to the patch.
