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

From: Elijah Newren <newren@gmail.com>
Date: Jun 19, 2024, 02:58 UTC
Message-ID: <CABPp-BGhWkjdd+9oa_RuUA07exVRtMHWqg8Ri_Sg1twUBkdxWA@mail.gmail.com>
In-Reply-To: <Zmt5tiuGIpTHLHRC@nand.local>

On Thu, Jun 13, 2024 at 10:59 PM Taylor Blau <me@ttaylorr.com> wrote:

Show 15 quoted lines

>
> On Thu, Jun 13, 2024 at 08:25:02PM +0000, 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 codepath dealing with error cases did not
>
> s/codepath/&s/ ?

Indeed.

Show 8 quoted lines

> > 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.

Show 5 quoted lines

> 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.