| Message ID | d8536f56ab296171c09e667e5c9299e95078388e.1616016485.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Declare merge-ort ready for general usage | expand |
On 3/17/2021 5:28 PM, Elijah Newren via GitGitGadget wrote: > From: Elijah Newren <newren@gmail.com> > > In preparation for switching from merge-recursive to merge-ort as the > default strategy, ensure that we are testing it. OK, so here we are testing it by default in CI, not just in that second round of optional environment variables. If that is what you intended, then this is the diff I was expecting: diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh index a66b5e8c75a..c013e9e646a 100755 --- a/ci/run-build-and-tests.sh +++ b/ci/run-build-and-tests.sh @@ -25,6 +25,7 @@ linux-gcc) export GIT_TEST_ADD_I_USE_BUILTIN=1 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master export GIT_TEST_WRITE_REV_INDEX=1 + export GIT_TEST_MERGE_STRATEGY=org make test ;; linux-clang) However, if we want to be aggressive in adopting the ort strategy very soon, then your approach of testing more frequently is valuable. Would it be worth setting GIT_TEST_MERGE_ALGORITHM=ort by default in test-lib.sh, too? That could help developers working on top of your topic avoid creating test failures that only show up in CI. Thanks, -Stolee
On Fri, Mar 19, 2021 at 6:05 AM Derrick Stolee <stolee@gmail.com> wrote: > > On 3/17/2021 5:28 PM, Elijah Newren via GitGitGadget wrote: > > From: Elijah Newren <newren@gmail.com> > > > > In preparation for switching from merge-recursive to merge-ort as the > > default strategy, ensure that we are testing it. > > OK, so here we are testing it by default in CI, not just in that > second round of optional environment variables. If that is what Note quite; my patch adds testing of merge-ort with *-gcc variants on each of the major platforms, but still includes merge-recursive testing in the *-clang variants. > you intended, then this is the diff I was expecting: > > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh > index a66b5e8c75a..c013e9e646a 100755 > --- a/ci/run-build-and-tests.sh > +++ b/ci/run-build-and-tests.sh > @@ -25,6 +25,7 @@ linux-gcc) > export GIT_TEST_ADD_I_USE_BUILTIN=1 > export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master > export GIT_TEST_WRITE_REV_INDEX=1 > + export GIT_TEST_MERGE_STRATEGY=org "ort", not "org". :-) > make test > ;; > linux-clang) > > However, if we want to be aggressive in adopting the ort strategy > very soon, then your approach of testing more frequently is > valuable. I don't think testing limited to linux-gcc is very valuable: * I've got opt-in volunteers (~60 of them) using it for the last half year for their daily work (on Mac & Linux) * For these volunteers, `git log -p` _also_ remerges every 2-parent merge automatically (there's a --remerge-diff capability) * We also had a bug where an internal script ('sparsify') looked for files changed in "local-only" commits via `git log --name-only HEAD --not --remotes=origin`. * At least one person renamed their default remote to something other than "origin" The upshot is we not only have many testers using it for daily work, we had at least one guinea pig redoing every merge in his repository...and found a platform specific bug in some commit from years ago because of it. In the last half year, I've only had three bug reports, none reported by more than one person: * Platform specific merge issue triggered only in a commit in a certain repository from years ago (fixed by first patch of this series, the STABLE_QSORT() one) * Mis-handling of present-despite-SKIP_WORKTREE file that *also* was involved in a merge conflict (see t6428 addition in this series; note that merge-recursive still fails it) * A bug in the --remerge-diff results So, at this point, beyond avoiding regressions the primary value I find in testing is in expanding it. windows is my big hole so more than anything I want ort testing there. > Would it be worth setting GIT_TEST_MERGE_ALGORITHM=ort by default > in test-lib.sh, too? That could help developers working on top of > your topic avoid creating test failures that only show up in CI. Actually, that sounds like a good idea, and we could just pick one or two special CI jobs to test with GIT_TEST_MERGE_STRATEGY=recursive.
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 5f2f884b92f6..e1f59861a2ca 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -252,6 +252,7 @@ jobs: MSYSTEM: MINGW64 NO_SVN_TESTS: 1 GIT_TEST_SKIP_REBASE_P: 1 + GIT_TEST_MERGE_ALGORITHM: ort run: | & .\git-sdk-64-minimal\usr\bin\bash.exe -lc @" # Let Git ignore the SDK and the test-cache diff --git a/ci/lib.sh b/ci/lib.sh index d848c036c50f..2a869b598c7f 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -182,6 +182,12 @@ export DEFAULT_TEST_TARGET=prove export GIT_TEST_CLONE_2GB=true export SKIP_DASHED_BUILT_INS=YesPlease +case "$jobname" in +*-gcc) + export GIT_TEST_MERGE_ALGORITHM=ort + ;; +esac + case "$jobname" in linux-clang|linux-gcc) if [ "$jobname" = linux-gcc ]