Re: [PATCH v2 00/13] Declare merge-ort ready for general usage

[Elijah Newren <newren@gmail.com>]

On Fri, Mar 19, 2021 at 6:09 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 3/17/2021 5:27 PM, Elijah Newren via GitGitGadget wrote:
> > This series depends on ort-perf-batch-10[1], and obsoletes the ort-remainder
> > topic[2] (that hadn't been picked up yet, so hopefully this doesn't cause
> > any confusion)
> >
> > With this series, merge-ort is ready for general usage -- it passes all
> > tests, passes dozens of tests that don't under merge-recursive, and
> > merge-ort is is already significantly faster than merge-recursive when
> > rename detection is involved. Users can select merge-ort by (a) passing
> > -sort to either git merge or git rebase, or (b) by setting pull.twohead=ort
> > [3], or (c) by setting GIT_TEST_MERGE_ALGORITHM=ort.
> >
> > Changes since v1:
> >
> >  * subsumed the ort-remainder topic (the first 10 patches), which has
> >    already been reviewed by both Ævar and Stolee[2].
> >  * the next two patches were the original v1, reviewed by Stolee
> >  * the final patch is new and adds testing.
>
> Sorry for the delay in looking at this. I read the two series before
> this, and found this to be a good union of them.
>
> My only question on the final patch is a two parter:
>
> 1. Did you mean to go this far?

At least, yes.

> 2. Did you want to go farther?

I like your suggestion in the other email; I'll resubmit to take
advantage of it.  :-)

> Mostly: how much do we want to prepare for ORT as the default
> strategy, at the expense of reducing testing of the recursive
> strategy?

We definitely should prepare for merge-ort as the default.  There's a
question of how soon the switch should be, but no question in my mind
that we should move towards it.

What do others think is needed before we switch the default?
Personally, I think there are three things:

1) merge-ort must handle the same cases that merge-recursive does
2) merge-ort must provide some benefit over merge-recursive
3) folks on the mailing list need to be comfortable with the default switch.

What would others add?

The first 2 conditions are already met, in spades.  For all the code
that calls merge-ort, merge-ort handles all the same cases, is more
correct, more performant, and more featureful than merge-recursive.  I
was surprised by how smooth the roll-out was and has continued to be
for internal users at $DAYJOB.

The only question is item #3.  If it weren't for that, I'd say we
should switch the default now, because AFAICT delaying the default
switch will just delay when expanded testing occurs, and I have run
out of other ways to expand testing.  But I realize I'm the only one
who knows that and is comfortable with that.  So I'm not proposing a
default switch yet; I want to hear feedback on what others want to see
done before we switch.  (At some point in the future, say another year
or two, I'll ask what needs to be done before we *delete*
merge-recursive.[ch].  But that's still off in the distant future.)