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

On Fri, Mar 19, 2021 at 6:09 AM Derrick Stolee [off-list ref] wrote:

On 3/17/2021 5:27 PM, Elijah Newren via GitGitGadget wrote:

quoted

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