Re: A merge-ort TODO comment, and how to test merge-ort?

[Elijah Newren <newren@gmail.com>]

Hi Ævar,

On Thu, Mar 4, 2021 at 8:28 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> On Fri, Jan 01 2021, Elijah Newren via GitGitGadget wrote:
>
> > +     else {
> > +             /* must be the 100644/100755 case */
> > +             assert(S_ISREG(a->mode));
> > +             result->mode = a->mode;
> > +             clean = (b->mode == o->mode);
> > +             /*
> > +              * FIXME: If opt->priv->call_depth && !clean, then we really
> > +              * should not make result->mode match either a->mode or
> > +              * b->mode; that causes t6036 "check conflicting mode for
> > +              * regular file" to fail.  It would be best to use some other
> > +              * mode, but we'll confuse all kinds of stuff if we use one
> > +              * where S_ISREG(result->mode) isn't true, and if we use
> > +              * something like 0100666, then tree-walk.c's calls to
> > +              * canon_mode() will just normalize that to 100644 for us and
> > +              * thus not solve anything.
> > +              *
> > +              * Figure out if there's some kind of way we can work around
> > +              * this...
> > +              */
>
> So if tree-walk.c didn't call canon_mode() you would do:
>
>     if (opt->priv->call_depth && !clean)
>         result->mode = 0100666;
>     else
>         result->mode = a->mode;
>
> I haven't looked at this bit closer, but that doesn't make the test
> referenced here pass.
>
> I'm refactoring tree-walk.h to do that in a WIP series, and ran into
> this.

Interesting.  Yeah, there might be more steps to make that particular
test work, but I couldn't go any further due to canon_mode().  It's a
testcase that has always failed under merge-recursive, and which I was
resigned to always have fail under merge-ort too; I suspect it's
enough of a corner case that no one but me ever really cared before.
(And I didn't hit it in the wild or know anyone that did, I just
learned of it by trying to clean up merge-recursive.)

> As an aside, how does one run the merge-ort tests in such a way as
> they'll pass on master now? There's now a bunch of failures with
> GIT_TEST_MERGE_ALGORITHM=ort, well, just for t*merge*.sh:
>
>     t6409-merge-subtree.sh                        (Wstat: 256 Tests: 12 Failed: 1)
>       Failed test:  12
>       Non-zero exit status: 1
>     t6418-merge-text-auto.sh                      (Wstat: 256 Tests: 10 Failed: 3)
>       Failed tests:  4-5, 10
>       Non-zero exit status: 1
>     t6437-submodule-merge.sh                      (Wstat: 0 Tests: 18 Failed: 0)
>       TODO passed:   13, 17
>     t6423-merge-rename-directories.sh             (Wstat: 256 Tests: 68 Failed: 4)
>       Failed tests:  7, 53, 55, 59
>       Non-zero exit status: 1

Right, I've been sending merge-ort upstream as fast as possible since
last September or so, but there's only so much reviewer bandwidth so
I've been forced to hold back on dozens of patches.

Currently there are 8 test failures (all shown in your output here --
1 in t6409, 3 in t6418, and 4 in t6423), and 12 TODO passed (only two
of which you show here).  I was forced to switch my ordering of
sending patches upstream late last year due to an intern project that
was planned to do significant work within diffcore-rename; I was
worried about major conflicts, so I needed to get the diffcore-rename
changes upstream earlier.  That's still in-process.

By the way, if you'd like to help accelerate the merge-ort work; it's
almost entirely review bound.
https://lore.kernel.org/git/pull.845.git.1614484707.gitgitgadget@gmail.com/
still has no comments, then I have optimization series 10-14 to send
(viewable up at
https://github.com/gitgitgadget/git/pulls?q=is%3Apr+author%3Anewren+Optimization+batch),
then I have other fixes -- mostly for the testsuite (viewable at
https://github.com/newren/git/tree/ort-remainder), then I need to fix
up the TODO passed submodule tests.  Due to how the submodule testing
framework functions, I can't just make a simple
s/test_expect_failure/test_expect_success/ -- the tests are structured
a bit funny and the tests are themselves buggy in some cases.  I
talked with jrnieder about it a little bit, just need to spend more
time on it.  But it hasn't been critical because the rest of the code
was so far away from finally landing anyway.  Finally, and optionally,
comes the --remerge-diff and --remerge-diff-only options to log/show
(viewable at https://github.com/newren/git/tree/remerge-diff, but
these patches need to both be cleaned up and rebased on
ort-remainder).

> And both test_expect_merge_algorithm and what seems to be a common
> pattern of e.g.:
>
>     t6400-merge-df.sh:      if test "$GIT_TEST_MERGE_ALGORITHM" = ort
>     t6400-merge-df.sh-      then
>     t6400-merge-df.sh-              test 0 -eq $(git ls-files -o | wc -l)
>     t6400-merge-df.sh-      else
>     t6400-merge-df.sh-              test 1 -eq $(git ls-files -o | wc -l)
>     t6400-merge-df.sh-      fi &&
>
> Will not run tests on both backends, I was expecting to find something
> so we can the test N times for the backends, and declared if things were
> to be skipped on ort or whatever.

Yeah, multiple ways of testing were discussed mid last year.  There
were lots of tradeoffs.  I think the thing that pushed in this
direction is that we're not just aiming to add another optional merge
backend, we're aiming to replace merge-recursive entirely.  Since
merge tests appear all throughout the code base, many as rebase or
cherry-pick or revert or stash tests...or just as simple setup tests,
we want all of those tested with the new backend.  Trying to duplicate
all those tests in any way other than just re-running the testsuite
with a different knob would require huge changes to hundreds
(thousands?) of testfiles and conflict with nearly every other topic.
So I made an environment variable that would choose which backend to
use, but with the downside of having to re-run the testsuite again.

> I understand that this is still WIP code, but it would be nice to have
> it in a state where one can confidently touch merge-ort.c when changing
> some API or whatever and have it be tested by default.

Thanks for proactively checking.  To make it easier for you, I'll see
if I can submit a series later today that mostly completes the
merge-ort implementation; the t6423 testcases won't be fixed until
"Optimization batch 12" lands, and I might not be able to fix the
"TODO passed" submodule tests in this series, but the rest of the
stuff can be fixed with about 10-12 patches.  I had been worried about
overloading the list with too many patches at once, but since it
sounds like you're willing to review these particular patches...  :-)

> Or maybe that's the case, and I've missed how it's happening...