Re: [PATCH 2/3] rebase --apply: restore some tests

From: Elijah Newren <newren@gmail.com>
To: Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Victor Gambier <vgambier@excilys.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH 2/3] rebase --apply: restore some tests
Date: Tue, 10 Aug 2021 09:58:09 -0700	[thread overview]
Message-ID: <CABPp-BFJD77wkroL5NFsH++a-yn0h6s6KM=Xfr2Ad_PfPQrUuw@mail.gmail.com> (raw)
In-Reply-To: <f7722dab21dafb73e64754b1286913eaa826f865.1628587917.git.gitgitgadget@gmail.com>

On Tue, Aug 10, 2021 at 2:32 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 980b482d28 ("rebase tests: mark tests specific to the am-backend with
> --am", 2020-02-15) sought to prepare tests testing the "apply" backend
> in preparation for 2ac0d6273f ("rebase: change the default backend
> from "am" to "merge"", 2020-02-15). However some tests seem to have
> been missed leading to us testing the "merge" backend twice. This
> patch fixes some cases that I noticed while adding tests to these
> files, I have not audited all the other rebase test files.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  t/t3403-rebase-skip.sh     | 2 +-
>  t/t3418-rebase-continue.sh | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
> index 6365c5af2f7..a44e68d0ffb 100755
> --- a/t/t3403-rebase-skip.sh
> +++ b/t/t3403-rebase-skip.sh
> @@ -52,7 +52,7 @@ test_expect_success setup '
>         '
>
>  test_expect_success 'rebase with git am -3 (default)' '
> -       test_must_fail git rebase main
> +       test_must_fail git rebase --apply main

Looks good.

>  '
>
>  test_expect_success 'rebase --skip can not be used with other options' '
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index f4c2ee02bc9..e4cb8be0418 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -36,7 +36,7 @@ test_expect_success 'non-interactive rebase --continue works with touched file'
>         git reset --hard &&
>         git checkout main &&
>
> -       test_must_fail git rebase --onto main main topic &&
> +       test_must_fail git rebase --apply --onto main main topic &&

I think the point here is that you're noticing that "git rebase -i"
and "git rebase" are both built on the same rebase backend (the merge
one) and thus that testing interactive vs. non-interactive isn't much
of a test.  The real test we are interested in is merge-backend vs.
apply-backend.  Your code change here is the necessary one to do that,
but it least the test descriptions still talking about interactive vs.
non-interactive even though that's not what we're concentrating on
anymore.

I'd say we'd at least want to change the description for this test,
"non-interactive rebase ---continue works with..." => "rebase
--continue with the apply backend works with...", but that we'd
probably want to change the description of the test before it and
maybe even just use rebase --merge rather than rebase -i.

>         echo "Resolved" >F2 &&
>         git add F2 &&
>         test-tool chmtime =-60 F1 &&
> @@ -254,7 +254,7 @@ test_rerere_autoupdate () {
>         '
>  }
>
> -test_rerere_autoupdate
> +test_rerere_autoupdate --apply

Looks good.

>  test_rerere_autoupdate -m
>  GIT_SEQUENCE_EDITOR=: && export GIT_SEQUENCE_EDITOR
>  test_rerere_autoupdate -i
> --
> gitgitgadget