Re: [PATCH v2 13/13] Add testing with merge-ort merge strategy

From: Elijah Newren <newren@gmail.com>
Date: Mar 19, 2021, 15:21 UTC
Message-ID: <CABPp-BFiTCVZhyS551vUit=7bw4cnbU2zdBH0DZ7UKAoieDFFA@mail.gmail.com>
In-Reply-To: <6b619bd0-c924-274e-d8bb-3bb9c67a2625@gmail.com>

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

Show 9 quoted lines

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

Show 11 quoted lines

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

Show 7 quoted lines

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