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

From: Elijah Newren <newren@gmail.com>
Date: Aug 21, 2021, 23:48 UTC
Message-ID: <CABPp-BGkqfx8kHA9o0sYhBQxcN0c_KSrvWczY+KNExhXD3ZeUQ@mail.gmail.com>
In-Reply-To: <xmqqwnofsa24.fsf@gitster.g>

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

Show 17 quoted lines

>
> Elijah Newren <newren@gmail.com> writes:
>
> >> 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.
>
> But merge_recursive_generic() eventually calls into merge_recursive();
> as long as you hook into the latter, you're covered, no?

Okay, you caught me. merge_ort_recursive() has one API difference from merge_recursive(), namely, it does not allow opt->ancestor to be anything other than NULL, whereas merge_recursive() does. The only caller where that distinction matters is merge_recursive_generic()...but that does prevent merge_recursive_generic() from just calling merge_ort_recursive().

The original patches I sent in for merge_incore_recursive() would have provided for this same ability (and made merge_ort_recursive() actually be a drop in replacement), but you rightfully pointed out that the relevant opt->ancestor portion of the patches made no sense. At the time, I responded (at https://lore.kernel.org/git/CABPp-BFr=1iVY739cfh-1Hp82x-Mny-k4y0f3zZ_YuP3PxiGfQ@mail.gmail.com/):

""" Yeah, you're making me lean towards thinking that merge_recursive_generic() is just a broken API that I shouldn't port over (even as a wrapper), and that I further shouldn't support using the merge_ort_recursive() API in a fashion wanted by that function. """

The problem with opt->ancestor in merge_recursive_generic() is as follows:

  * When there is only one merge base (and opt->ancestor is not set),
merge_ort_recursive() and merge_recursive() will both set
opt->ancestor to the hash of the merge base commit.

  * The hash of the merge base is great when the merge base is a real
commit, less so when fake commits are generated (as
merge_recursive_generic() does) to pass along.

  * Because of the above, merge_recursive_generic() overrides
opt->ancestor with the value "constructed merge base" when there is
exactly one merge base tree.  That was done with git-am in mind, and
makes sense for am because it does create a fake or constructed merge
base.  It does not make sense to me for stash, which has a real
commit.  It also seems suboptimal or wrong for
builtin/merge-recursive.c as well -- it's hard to be sure since
builtin/merge-recursive simply takes an OID rather than actual branch
names and thus those OIDs could correspond to fake or constructed
commits, but builtin/merge-recursive does have the
better_branch_name() function it uses for o.branch1 and o.branch2 and
it seems like it should do the same on the merge base when it's
unique.

Rather than porting this bug (these bugs?) over from merge-recursive, I'll just convert the merge_recursive_generic() callers over to the new API.