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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Mar 04 2021, Elijah Newren wrote:

> On Thu, Mar 4, 2021 at 1:29 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>>
>> On Thu, Mar 04 2021, Elijah Newren wrote:
>>
>> > 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.)
>>
>> I'll send those patches out sooner than later, but as a quick question,
>> for merges / writing new files to the index/trees etc. we basically:
>>
>>  1. sanitize the mode with canon_mode()
>>  2. write it to a new object, either index or TREE object
>>
>> I've been trying to refactor things so those things have canon_mode() as
>> close to the time of writing as possible.
>>
>> Well, mostly to replace the whole S_*(mode) macros all over the place
>> with checks against "enum object_type", which is what most of it wants
>> anyway.
>>
>> Do you think the merge logic generally wants to operate on the "raw"
>> mode bits (including what may not even pass fsck checks), or the
>> sanitized canon_mode()?
>
> This one little special case is the only one when it'd care about the
> raw mode bits.  I'm worried that making the code work on raw mode bits
> wouldn't be trivial.  In general mode differences between different
> sides or the mode base is a conflict as well, so I'd need to add code
> around it's-not-really-a-conflict if canon_mode() on both modes map to
> the same value.

It wasn't trivial, but let's see what you think of the end result, soon
on-list.

>> >> 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).
>>
>> Maybe something like this with a bit of test prereq sprinkle on top? :)
>>
>>     diff --git a/t/t6423-merge-rename-directories-TARGET-ort.sh b/t/t6423-merge-rename-directories-TARGET-ort.sh
>>     new file mode 120000
>>     index 00000000000..6bf750c4036
>>     --- /dev/null
>>     +++ b/t/t6423-merge-rename-directories-TARGET-ort.sh
>>     @@ -0,0 +1 @@
>>     +t6423-merge-rename-directories.sh
>>     \ No newline at end of file
>>     diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
>>     index 5d3b711fe68..1e571384223 100755
>>     --- a/t/t6423-merge-rename-directories.sh
>>     +++ b/t/t6423-merge-rename-directories.sh
>>     @@ -3,2 +3,4 @@
>>      test_description="recursive merge with directory renames"
>>     +# TARGET-ort: GIT_TEST_MERGE_ALGORITHM=ort && export GIT_TEST_MERGE_ALGORITHM=ort
>
> To test my understanding of your proposal, would you need to add this
> line to hundreds of files?
>
>>     +
>>      # includes checking of many corner cases, with a similar methodology to:
>>     diff --git a/t/test-lib.sh b/t/test-lib.sh
>>     index d3f6af6a654..8d5da7e0ba9 100644
>>     --- a/t/test-lib.sh
>>     +++ b/t/test-lib.sh
>>     @@ -247,2 +247,11 @@ TEST_STRESS_JOB_SFX="${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}"
>>      TEST_NAME="$(basename "$0" .sh)"
>>     +if test -L "$0"
>>     +then
>>     +       target=$(echo "$0" | grep -o "TARGET-[^.]*")
>>     +       if test -n "$target"
>>     +       then
>>     +               to_eval=$(grep "^# $target: " "$0" | sed 's/.*://')
>>     +               eval $to_eval
>>     +       fi
>>     +fi
>>      TEST_NUMBER="${TEST_NAME%%-*}"
>>
>> That implementation's a bit of a hack, and requires SYMLINK (could be
>> changed), but now I can:
>>
>>     ./t6423-merge-rename-directories-TARGET-ort.sh
>>     ./t6423-merge-rename-directories.sh
>>
>> And run the whole thing with/without ort in one go.
>>
>> Then with a trivial symlink-in-a-loop $(git grep -l MERGE_ALGORITHM)
>> loop on top I've got ort and non-ort merge tests all in one go on a WIP
>> topic.
>
> Do you need a symlink for each file as well, thus hundreds of symlinks?
>
> Your idea is a quick way to get testing, that's much better than
> duplicating all the files.  I'm still a bit worried that it'd
> encourage people to only add it to the "most important" or "most
> obvious" test files, which goes somewhat counter to testing merge-ort
> as a full replacement of merge-recursive.  While developing merge-ort,
> I'd sometimes see failures outside t6*, even when everything under t6*
> passed.  For example, t3[45]* and t76*.

Yes, this wouldn't make sense for merge-ort then. I was assuming that it
was fairly isolated (at least mostly) to a few test files.

That's mostly me not having read the ort traffic carefully, I'm
embarrased to say that I managed to miss that it was a full "recursive"
replacement, I thought it was (mostly) a new merge driver mode and we'd
keep both.

So nevermind :)

I do think it's interesting to have something like this, but it's way
out of scope for merge-ort work.

E.g. we could start by making sure for all N tests in a file, we can run
run each N times in a loop, i.e. individual --stress tests.

That in itself would be a big undertaking, and would require e.g. having
a "test_expect_success_setup" for tests that do one-off setup, which
we'd skip.

Then we could instrument the git_env_bool("GIT_TEST_*" with some
replacement which logged if we ended up deciding something on whether
that was true/false.

And finally, log that with trace2, then for each test that encountered
differing modes we'd run them for the N modes, or all combinations of
modes (would quickly get expensive for things that touch a lot of
things).

Anyway, just take the above as rambling :)

>> >> 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...  :-)
>>
>> *nod*
>>
>> Also, I'll try to get to reviewing some of it, thanks for all your work.
>>
>> B.t.w., if the original E-Mail sounded like complaining that wasn't the
>> intent. I just thought I'd shoot off a quick message about if I missed
>> something about the in-flight status / testing of the ort work...
>
> Nope, didn't sound like complaining; thanks again for checking.

And thanks a lot for your good work on merge-ort & other things :)