git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Elijah Newren <newren@gmail.com>
To: "Đoàn Trần Công Danh" <congdanhqx@gmail.com>
Cc: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH 1/9] t/: new helper for tests that pass with ort but fail with recursive
Date: Mon, 26 Oct 2020 07:56:59 -0700	[thread overview]
Message-ID: <CABPp-BEmJA92tDymypgJqGmi=eMsvU+eP=KeTKvKcJEvFVztEA@mail.gmail.com> (raw)
In-Reply-To: <20201025134905.GB15823@danh.dev>

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.

  reply	other threads:[~2020-10-26 14:57 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-23 16:01 [PATCH 0/9] Support both merge backends in the testsuite, via environment variable Elijah Newren via GitGitGadget
2020-10-23 16:01 ` [PATCH 1/9] t/: new helper for tests that pass with ort but fail with recursive Elijah Newren via GitGitGadget
2020-10-23 16:48   ` Junio C Hamano
2020-10-23 17:25     ` Elijah Newren
2020-10-23 18:27       ` Elijah Newren
2020-10-24 10:49   ` Đoàn Trần Công Danh
2020-10-24 16:53     ` Elijah Newren
2020-10-25 13:49       ` Đoàn Trần Công Danh
2020-10-26 14:56         ` Elijah Newren [this message]
2020-10-26 17:43           ` Junio C Hamano
2020-10-23 16:01 ` [PATCH 2/9] merge tests: expect improved directory/file conflict handling in ort Elijah Newren via GitGitGadget
2020-10-23 17:40   ` Elijah Newren
2020-10-23 16:01 ` [PATCH 3/9] t6416: correct expectation for rename/rename(1to2) + directory/file Elijah Newren via GitGitGadget
2020-10-23 16:01 ` [PATCH 4/9] t6404, t6423: expect improved rename/delete handling in ort backend Elijah Newren via GitGitGadget
2020-10-23 16:01 ` [PATCH 5/9] t6423: expect improved conflict markers labels in the " Elijah Newren via GitGitGadget
2020-10-23 16:01 ` [PATCH 6/9] merge tests: expect slight differences in output for recursive vs. ort Elijah Newren via GitGitGadget
2020-10-24 16:06   ` Elijah Newren
2020-10-23 16:01 ` [PATCH 7/9] t6423, t6436: note improved ort handling with dirty files Elijah Newren via GitGitGadget
2020-10-23 16:01 ` [PATCH 8/9] t6423: note improved ort handling with untracked files Elijah Newren via GitGitGadget
2020-10-23 16:01 ` [PATCH 9/9] t6423: add more details about direct resolution of directories Elijah Newren via GitGitGadget
2020-10-23 20:12   ` Elijah Newren
2020-10-26 17:01 ` [PATCH v2 0/9] Support both merge backends in the testsuite, via environment variable Elijah Newren via GitGitGadget
2020-10-26 17:01   ` [PATCH v2 1/9] t/: new helper for tests that pass with ort but fail with recursive Elijah Newren via GitGitGadget
2020-10-26 17:01   ` [PATCH v2 2/9] merge tests: expect improved directory/file conflict handling in ort Elijah Newren via GitGitGadget
2020-10-26 17:01   ` [PATCH v2 3/9] t6416: correct expectation for rename/rename(1to2) + directory/file Elijah Newren via GitGitGadget
2020-10-26 17:01   ` [PATCH v2 4/9] t6404, t6423: expect improved rename/delete handling in ort backend Elijah Newren via GitGitGadget
2020-10-26 17:01   ` [PATCH v2 5/9] t6423: expect improved conflict markers labels in the " Elijah Newren via GitGitGadget
2020-10-26 17:01   ` [PATCH v2 6/9] merge tests: expect slight differences in output for recursive vs. ort Elijah Newren via GitGitGadget
2020-10-26 17:01   ` [PATCH v2 7/9] t6423, t6436: note improved ort handling with dirty files Elijah Newren via GitGitGadget
2020-10-26 17:01   ` [PATCH v2 8/9] t6423: note improved ort handling with untracked files Elijah Newren via GitGitGadget
2020-10-26 17:01   ` [PATCH v2 9/9] t6423: add more details about direct resolution of directories Elijah Newren via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABPp-BEmJA92tDymypgJqGmi=eMsvU+eP=KeTKvKcJEvFVztEA@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=congdanhqx@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --subject='Re: [PATCH 1/9] t/: new helper for tests that pass with ort but fail with recursive' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).