Re: [PATCH 1/6] t1092: use ORT merge strategy

From: Elijah Newren <newren@gmail.com>
Date: Aug 21, 2021, 00:20 UTC
Message-ID: <CABPp-BHLw-h0oQLNwT+DAXeHZZfJRaiyoM+kDuqx_aT80-zM3w@mail.gmail.com>
In-Reply-To: <xmqq7dgfu1zb.fsf@gitster.g>

On Fri, Aug 20, 2021 at 4:32 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 26 quoted lines

>
> Elijah Newren <newren@gmail.com> writes:
>
> > On Wed, Aug 18, 2021 at 11:42 AM Derrick Stolee <stolee@gmail.com> wrote:
> >>
> >> >     It seems to me that it would let us live in the future in a more
> >> >     comprehensive way if we tweaked merge_recursive() and/or
> >> >     merge_recursive_generic() so that all internal callers, not just
> >> >     builtin/merge.c, would redirect to the ort machinery when say
> >> >     GIT_TEST_REPLACE_RECURSIVE_WITH_ORT environment exists, and
> >> >     doing it that way we do not need to sprinkle "-srecursive" and
> >> >     "-sort" everywhere in our tests at randomly chosen places to
> >> >     give test coverage to both strategies.
> >
> > GIT_TEST_MERGE_ALGORITHM already does this; the testsuite already had
> > `-s recursive` sprinkled everywhere (due to contrast with `-s
> > resolve`), but since I wanted to use all existing recursive tests as
> > ort tests, then rather than tweaking all the test files and copying
> > tests and whatnot, we decided to just have GIT_TEST_MERGE_ALGORITHM
> > reinterpret "recursive" to whatever GIT_TEST_MERGE_ALGORITHM says.
>
> I somehow thought that direct calls to merge_recursive() and
> merge_recursive_generic() are not affected with that environment
> variable, so you cannot influence what happens during "git am -3"
> and "git stash apply" with that, but perhaps I was not reading the
> code correctly.

Sorry for being unclear. I was responding to the "sprinkling" portion of the quote; GIT_TEST_MERGE_ALGORITHM allows us to avoid sprinkling -srecursive and -sort in various places.

You are correct that merge_recursive() and merge_recursive_generic() are unaffected by the environment variable; the environment variable operates at a higher level in the code to choose whether to call e.g. merge_recursive() vs. merge_incore_recursive().

> It seems that merge_recursive() and merge_ort_recursive() are
> interface compatible and the latter can serve as a drop-in
> replacement for the former?

Yes, merge_ort_recursive() and merge_ort_nonrecursive() were meant as interface compatible drop-in replacements for merge_recursive() and merge_trees(), to make it easy to switch callers over.

There is no such replacement for merge_recursive_generic(), though, and builtin/{am, merge-recursive, stash}.c will all need to be modified to work with merge-ort. IIRC, when last we discussed that interface, we realized that the three were using it a bit differently and it had some hardcoded am-specific assumptions that were not appropriate for the other two, so it's not clear to me we should port that interface.