On Sun, Oct 25, 2020 at 6:49 AM Đoàn Trần Công Danh
[off-list ref] wrote:

On 2020-10-24 09:53:18-0700, Elijah Newren [off-list ref] wrote:

quoted

On Sat, Oct 24, 2020 at 3:49 AM Đoàn Trần Công Danh
[off-list ref] wrote:

quoted

On 2020-10-23 16:01:16+0000, Elijah Newren via GitGitGadget [off-list ref] wrote:

quoted

+test_expect_merge_algorithm () {
+     status_for_recursive=$1
+     shift
+     status_for_ort=$1
+     shift
+
+     if test "$GIT_TEST_MERGE_ALGORITHM" = ort
+     then
+             test_expect_${status_for_ort} "$@"
+     else
+             test_expect_${status_for_recursive} "$@"
-test_expect_failure 'check symlink modify/modify' '
+test_expect_merge_algorithm failure success 'check symlink modify/modify' '

I find this series of "failure success" hard to decode without
understanding what it would be, then I need to keep rememberring which
status is corresponding with with algorithm.

Perhaps this patch is a bit easier to read. This is largely based on
your patch. (I haven't read other patches, yet).

What do you think?

It is easier to read and I think something along these lines would
make a lot of sense if this weren't a transient change (the idea is to
eventually drop the recursive backend in favor of ort, and then these
can all switch to just using test_expect_success).  Maybe it still
makes sense to make further changes here anyway, but if we do go this
route, there are 1-2 things we can/should change:

First, while a lot of my contributions aren't that important, and the

Mine aren't that important, either

quoted

new test_expect_* function certainly falls in that category, one of
the driving goals behind a new merge algorithm was fixing up edge and
corner cases that were just too problematic in the recursive backend.
Thus, the patch where I get to flip the test expectation is one that I
care about more than most out of the (I'm guessing on this number)

Make sense.

quoted

100+ patches that will be part of this new merge algorithm.  Having
you take over ownership of that patch thus isn't right; we should
instead keep my original patch and apply your suggested changes on top
(or have a patch from you introducing a new function first, and then
have a patch from me using it to flip test expectations on top).

You can take back the ownership, the patch was based on yours, anyway.

I wrote like that since I need to rewrite part of the message to match
with my changes ;)

No need to generate extra noise of additional patch.

Just to be clear, others have made suggestions like yours in the past
where they've taken over ownership of a patch in a series and I've
been totally fine with it.  Your suggestion to do the same would have
been fine here, but I'm just kinda attached to being able to flip the
test expectation for these tests; I've been working towards it for a
_long_ time.

I think an extra patch, attributed to you, actually makes the most sense here.

quoted

Second, I think that lines like
    test_expect_merge_success recursive=failure ...
read like a contradiction and are also confusing.  I think it'd be
better if it read something like
    test_expect_merge recursive=failure ort=success ...
or something along those lines.

When I wrote the patch, I was expecting something like

        test_expect_merge_success recursive=failure,other=failure ...

in order to merge all algorithm into single parameters.

How about something like:

        test_expect_merge_success exception=recursive,other ...

Not that we have "other" algorithm to begin with.

Sure, sounds great.  I wouldn't spend any time trying to make it work
with a 3rd backend, though.  The goal is to have two merge backends
only long enough for people to become comfortable with the new backend
and discover any unknown issues with it that we can fix, then we'll
rip it out the old "recursive" backend and we'll translate any
requests for "recursive" to mean "ort".  We'll also rip the
test_expect_merge_success() function out since it'll be unneeded (so
efforts towards future proofing of that function will be wasted).
Then years will go by before another merge backend comes along, if one
ever does.