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

From: Elijah Newren <newren@gmail.com>
Date: Mar 19, 2021, 23:21 UTC
Message-ID: <CABPp-BGm1R0OVywK50eMf9OWKv95nA+V2bzX6WLm2iFvfN_i9Q@mail.gmail.com>
In-Reply-To: <80cccc70-296a-05d2-94a8-0e005e4abe1d@gmail.com>

On Fri, Mar 19, 2021 at 6:09 AM Derrick Stolee <stolee@gmail.com> wrote:

Show 26 quoted lines

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