| Message ID | pull.905.v2.git.1616016485.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Declare merge-ort ready for general usage | expand |
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? 2. Did you want to go farther? Mostly: how much do we want to prepare for ORT as the default strategy, at the expense of reducing testing of the recursive strategy? Thanks, -Stolee
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.)
One more thing... 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: ^^^ Note: 18 hours, 42 minutes difference. > > 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. Delay?!? You only took a day and a half to respond! That's a fast turnaround, not a delay. Don't apologize for that, or you'll set unreasonably high expectations that'll scare the rest of us away. :-) Thanks for all your review efforts; it's very much appreciated!
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. [1] https://lore.kernel.org/git/pull.853.git.1615674128.gitgitgadget@gmail.com/ [2] https://lore.kernel.org/git/pull.973.v2.git.git.1615271086.gitgitgadget@gmail.com/ [3] See commit 14c4586c2d ("merge,rebase,revert: select ort or recursive by config or environment", 2020-11-02) Elijah Newren (13): merge-ort: use STABLE_QSORT instead of QSORT where required merge-ort: add a special minimal index just for renormalization merge-ort: have ll_merge() use a special attr_index for renormalization merge-ort: let renormalization change modify/delete into clean delete merge-ort: support subtree shifting t6428: new test for SKIP_WORKTREE handling and conflicts merge-ort: implement CE_SKIP_WORKTREE handling with conflicted entries t: mark several submodule merging tests as fixed under merge-ort merge-ort: write $GIT_DIR/AUTO_MERGE whenever we hit a conflict merge-recursive: add a bunch of FIXME comments documenting known bugs Revert "merge-ort: ignore the directory rename split conflict for now" t6423: mark remaining expected failure under merge-ort as such Add testing with merge-ort merge strategy .github/workflows/main.yml | 1 + branch.c | 1 + builtin/rebase.c | 1 + ci/lib.sh | 6 + merge-ort.c | 242 ++++++++++++++++-- merge-recursive.c | 37 +++ path.c | 1 + path.h | 2 + sequencer.c | 5 + t/t3512-cherry-pick-submodule.sh | 7 +- t/t3513-revert-submodule.sh | 5 +- t/t5572-pull-submodule.sh | 7 +- t/t6423-merge-rename-directories.sh | 2 +- t/t6428-merge-conflicts-sparse.sh | 158 ++++++++++++ t/t6437-submodule-merge.sh | 5 +- t/t6438-submodule-directory-file-conflicts.sh | 7 +- 16 files changed, 449 insertions(+), 38 deletions(-) create mode 100755 t/t6428-merge-conflicts-sparse.sh base-commit: ac0ba91ce275227f5df8f16fb986308ff88b198b Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-905%2Fgitgitgadget%2Fort-readiness-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-905/gitgitgadget/ort-readiness-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/905 Range-diff vs v1: -: ------------ > 1: e223f842748c merge-ort: use STABLE_QSORT instead of QSORT where required -: ------------ > 2: 6d34cc466bd5 merge-ort: add a special minimal index just for renormalization -: ------------ > 3: 4ff23d2f52a0 merge-ort: have ll_merge() use a special attr_index for renormalization -: ------------ > 4: c1c9605c1932 merge-ort: let renormalization change modify/delete into clean delete -: ------------ > 5: 41fffcdd3b78 merge-ort: support subtree shifting -: ------------ > 6: 6aec1f499b80 t6428: new test for SKIP_WORKTREE handling and conflicts -: ------------ > 7: fe3baf696785 merge-ort: implement CE_SKIP_WORKTREE handling with conflicted entries -: ------------ > 8: f9325647a9fc t: mark several submodule merging tests as fixed under merge-ort -: ------------ > 9: 4a79e6134691 merge-ort: write $GIT_DIR/AUTO_MERGE whenever we hit a conflict -: ------------ > 10: a37979454069 merge-recursive: add a bunch of FIXME comments documenting known bugs 1: 7a8e26638a16 = 11: 6bda855f2980 Revert "merge-ort: ignore the directory rename split conflict for now" 2: 0d41038fad91 = 12: 1c6361c9b88a t6423: mark remaining expected failure under merge-ort as such -: ------------ > 13: d8536f56ab29 Add testing with merge-ort merge strategy