Re: [PATCH 1/9] t/: new helper for tests that pass with ort but fail with recursive

[Elijah Newren <newren@gmail.com>]

On Sun, Oct 25, 2020 at 6:49 AM Đoàn Trần Công Danh
<congdanhqx@gmail.com> wrote:
>
> On 2020-10-24 09:53:18-0700, Elijah Newren <newren@gmail.com> wrote:
> > On Sat, Oct 24, 2020 at 3:49 AM Đoàn Trần Công Danh
> > <congdanhqx@gmail.com> wrote:
> > >
> > > On 2020-10-23 16:01:16+0000, Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> wrote:
> > > > +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
>
> > 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.
>
> > 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.

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