diff mbox series

[4/4] rebase: remove a couple of redundant strategy tests

Message ID 3e02eeff78b23711187de47a1a820f9bde683200.1678893298.git.phillip.wood@dunelm.org.uk (mailing list archive)
State Superseded
Headers show
Series rebase: cleanup merge strategy option handling | expand

Commit Message

Phillip Wood March 15, 2023, 3:14 p.m. UTC 
From: Phillip Wood <phillip.wood@dunelm.org.uk>

The test removed in t3402 has been redundant ever since 80ff47957b
(rebase: remember strategy and strategy options, 2011-02-06) which added
a new test the first part of which (as noted in the commit message)
duplicated the existing test. The test removed in t3418 has been
redundant since the merge backend was removed in 68aa495b59 (rebase:
implement --merge via the interactive machinery, 2018-12-11) as it now
tests the same code paths as the preceding test.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3402-rebase-merge.sh    | 21 ---------------------
 t/t3418-rebase-continue.sh | 32 --------------------------------
 2 files changed, 53 deletions(-)

Comments

Elijah Newren April 1, 2023, 7:31 p.m. UTC | #1 
On Wed, Mar 15, 2023 at 8:38 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The test removed in t3402 has been redundant ever since 80ff47957b
> (rebase: remember strategy and strategy options, 2011-02-06) which added
> a new test the first part of which (as noted in the commit message)
> duplicated the existing test. The test removed in t3418 has been
> redundant since the merge backend was removed in 68aa495b59 (rebase:
> implement --merge via the interactive machinery, 2018-12-11) as it now
> tests the same code paths as the preceding test.

I was really confused by this commit message at first.  Eventually, I
figured out you were talking about the changes in this commit, just in
past tense.  Could we change it to imperative tense?  E.g.

Remove a test in t3402 that has been redundant ever since 80ff47957b
(rebase: remember strategy and strategy options, 2011-02-06).  That commit added
a new test, the first part of which (as noted in the old commit message)
duplicated an existing test.

Also remove a test t3418 that has been redundant since the merge
backend was removed in 68aa495b59 (rebase: implement --merge
via the interactive machinery, 2018-12-11), since it now tests the
same code paths as the preceding test.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  t/t3402-rebase-merge.sh    | 21 ---------------------
>  t/t3418-rebase-continue.sh | 32 --------------------------------
>  2 files changed, 53 deletions(-)
>
> diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh
> index 7e46f4ca85..79b0640c00 100755
> --- a/t/t3402-rebase-merge.sh
> +++ b/t/t3402-rebase-merge.sh
> @@ -131,27 +131,6 @@ test_expect_success 'picking rebase' '
>         esac
>  '
>
> -test_expect_success 'rebase -s funny -Xopt' '
> -       test_when_finished "rm -fr test-bin funny.was.run" &&
> -       mkdir test-bin &&
> -       cat >test-bin/git-merge-funny <<-EOF &&
> -       #!$SHELL_PATH
> -       case "\$1" in --opt) ;; *) exit 2 ;; esac
> -       shift &&
> -       >funny.was.run &&
> -       exec git merge-recursive "\$@"
> -       EOF
> -       chmod +x test-bin/git-merge-funny &&
> -       git reset --hard &&
> -       git checkout -b test-funny main^ &&
> -       test_commit funny &&
> -       (
> -               PATH=./test-bin:$PATH &&
> -               git rebase -s funny -Xopt main
> -       ) &&
> -       test -f funny.was.run
> -'
> -
>  test_expect_success 'rebase --skip works with two conflicts in a row' '
>         git checkout second-side  &&
>         tr "[A-Z]" "[a-z]" <newfile >tmp &&
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 42c3954125..2d0789e554 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
>         test_cmp expect actual
>  '
>
> -test_expect_success 'rebase -i --continue handles merge strategy and options' '
> -       rm -fr .git/rebase-* &&
> -       git reset --hard commit-new-file-F2-on-topic-branch &&
> -       test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 &&
> -       test_when_finished "rm -fr test-bin funny.was.run funny.args" &&
> -       mkdir test-bin &&
> -       cat >test-bin/git-merge-funny <<-EOF &&
> -       #!$SHELL_PATH
> -       echo "\$@" >>funny.args
> -       case "\$1" in --opt) ;; *) exit 2 ;; esac
> -       case "\$2" in --foo) ;; *) exit 2 ;; esac
> -       case "\$4" in --) ;; *) exit 2 ;; esac
> -       shift 2 &&
> -       >funny.was.run &&
> -       exec git merge-recursive "\$@"
> -       EOF
> -       chmod +x test-bin/git-merge-funny &&
> -       (
> -               PATH=./test-bin:$PATH &&
> -               test_must_fail git rebase -i -s funny -Xopt -Xfoo main topic
> -       ) &&
> -       test -f funny.was.run &&
> -       rm funny.was.run &&
> -       echo "Resolved" >F2 &&
> -       git add F2 &&
> -       (
> -               PATH=./test-bin:$PATH &&
> -               git rebase --continue
> -       ) &&
> -       test -f funny.was.run
> -'
> -
>  test_expect_success 'rebase -r passes merge strategy options correctly' '
>         rm -fr .git/rebase-* &&
>         git reset --hard commit-new-file-F3-on-topic-branch &&
> --
> 2.39.2

Looks good otherwise.
Phillip Wood April 4, 2023, 1:04 p.m. UTC | #2 
On 01/04/2023 20:31, Elijah Newren wrote:
> On Wed, Mar 15, 2023 at 8:38 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> The test removed in t3402 has been redundant ever since 80ff47957b
>> (rebase: remember strategy and strategy options, 2011-02-06) which added
>> a new test the first part of which (as noted in the commit message)
>> duplicated the existing test. The test removed in t3418 has been
>> redundant since the merge backend was removed in 68aa495b59 (rebase:
>> implement --merge via the interactive machinery, 2018-12-11) as it now
>> tests the same code paths as the preceding test.
> 
> I was really confused by this commit message at first.  Eventually, I
> figured out you were talking about the changes in this commit, just in
> past tense.  Could we change it to imperative tense?  E.g.
> 
> Remove a test in t3402 that has been redundant ever since 80ff47957b
> (rebase: remember strategy and strategy options, 2011-02-06).  That commit added
> a new test, the first part of which (as noted in the old commit message)
> duplicated an existing test.
> 
> Also remove a test t3418 that has been redundant since the merge
> backend was removed in 68aa495b59 (rebase: implement --merge
> via the interactive machinery, 2018-12-11), since it now tests the
> same code paths as the preceding test.

Thanks, that is clearer, I'll use your suggested wording when I re-roll

Best Wishes

Phillip

>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   t/t3402-rebase-merge.sh    | 21 ---------------------
>>   t/t3418-rebase-continue.sh | 32 --------------------------------
>>   2 files changed, 53 deletions(-)
>>
>> diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh
>> index 7e46f4ca85..79b0640c00 100755
>> --- a/t/t3402-rebase-merge.sh
>> +++ b/t/t3402-rebase-merge.sh
>> @@ -131,27 +131,6 @@ test_expect_success 'picking rebase' '
>>          esac
>>   '
>>
>> -test_expect_success 'rebase -s funny -Xopt' '
>> -       test_when_finished "rm -fr test-bin funny.was.run" &&
>> -       mkdir test-bin &&
>> -       cat >test-bin/git-merge-funny <<-EOF &&
>> -       #!$SHELL_PATH
>> -       case "\$1" in --opt) ;; *) exit 2 ;; esac
>> -       shift &&
>> -       >funny.was.run &&
>> -       exec git merge-recursive "\$@"
>> -       EOF
>> -       chmod +x test-bin/git-merge-funny &&
>> -       git reset --hard &&
>> -       git checkout -b test-funny main^ &&
>> -       test_commit funny &&
>> -       (
>> -               PATH=./test-bin:$PATH &&
>> -               git rebase -s funny -Xopt main
>> -       ) &&
>> -       test -f funny.was.run
>> -'
>> -
>>   test_expect_success 'rebase --skip works with two conflicts in a row' '
>>          git checkout second-side  &&
>>          tr "[A-Z]" "[a-z]" <newfile >tmp &&
>> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
>> index 42c3954125..2d0789e554 100755
>> --- a/t/t3418-rebase-continue.sh
>> +++ b/t/t3418-rebase-continue.sh
>> @@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
>>          test_cmp expect actual
>>   '
>>
>> -test_expect_success 'rebase -i --continue handles merge strategy and options' '
>> -       rm -fr .git/rebase-* &&
>> -       git reset --hard commit-new-file-F2-on-topic-branch &&
>> -       test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 &&
>> -       test_when_finished "rm -fr test-bin funny.was.run funny.args" &&
>> -       mkdir test-bin &&
>> -       cat >test-bin/git-merge-funny <<-EOF &&
>> -       #!$SHELL_PATH
>> -       echo "\$@" >>funny.args
>> -       case "\$1" in --opt) ;; *) exit 2 ;; esac
>> -       case "\$2" in --foo) ;; *) exit 2 ;; esac
>> -       case "\$4" in --) ;; *) exit 2 ;; esac
>> -       shift 2 &&
>> -       >funny.was.run &&
>> -       exec git merge-recursive "\$@"
>> -       EOF
>> -       chmod +x test-bin/git-merge-funny &&
>> -       (
>> -               PATH=./test-bin:$PATH &&
>> -               test_must_fail git rebase -i -s funny -Xopt -Xfoo main topic
>> -       ) &&
>> -       test -f funny.was.run &&
>> -       rm funny.was.run &&
>> -       echo "Resolved" >F2 &&
>> -       git add F2 &&
>> -       (
>> -               PATH=./test-bin:$PATH &&
>> -               git rebase --continue
>> -       ) &&
>> -       test -f funny.was.run
>> -'
>> -
>>   test_expect_success 'rebase -r passes merge strategy options correctly' '
>>          rm -fr .git/rebase-* &&
>>          git reset --hard commit-new-file-F3-on-topic-branch &&
>> --
>> 2.39.2
> 
> Looks good otherwise.
diff mbox series

Patch

diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh
index 7e46f4ca85..79b0640c00 100755
--- a/t/t3402-rebase-merge.sh
+++ b/t/t3402-rebase-merge.sh
@@ -131,27 +131,6 @@  test_expect_success 'picking rebase' '
 	esac
 '
 
-test_expect_success 'rebase -s funny -Xopt' '
-	test_when_finished "rm -fr test-bin funny.was.run" &&
-	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF &&
-	#!$SHELL_PATH
-	case "\$1" in --opt) ;; *) exit 2 ;; esac
-	shift &&
-	>funny.was.run &&
-	exec git merge-recursive "\$@"
-	EOF
-	chmod +x test-bin/git-merge-funny &&
-	git reset --hard &&
-	git checkout -b test-funny main^ &&
-	test_commit funny &&
-	(
-		PATH=./test-bin:$PATH &&
-		git rebase -s funny -Xopt main
-	) &&
-	test -f funny.was.run
-'
-
 test_expect_success 'rebase --skip works with two conflicts in a row' '
 	git checkout second-side  &&
 	tr "[A-Z]" "[a-z]" <newfile >tmp &&
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 42c3954125..2d0789e554 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -97,38 +97,6 @@  test_expect_success 'rebase --continue remembers merge strategy and options' '
 	test_cmp expect actual
 '
 
-test_expect_success 'rebase -i --continue handles merge strategy and options' '
-	rm -fr .git/rebase-* &&
-	git reset --hard commit-new-file-F2-on-topic-branch &&
-	test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 &&
-	test_when_finished "rm -fr test-bin funny.was.run funny.args" &&
-	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF &&
-	#!$SHELL_PATH
-	echo "\$@" >>funny.args
-	case "\$1" in --opt) ;; *) exit 2 ;; esac
-	case "\$2" in --foo) ;; *) exit 2 ;; esac
-	case "\$4" in --) ;; *) exit 2 ;; esac
-	shift 2 &&
-	>funny.was.run &&
-	exec git merge-recursive "\$@"
-	EOF
-	chmod +x test-bin/git-merge-funny &&
-	(
-		PATH=./test-bin:$PATH &&
-		test_must_fail git rebase -i -s funny -Xopt -Xfoo main topic
-	) &&
-	test -f funny.was.run &&
-	rm funny.was.run &&
-	echo "Resolved" >F2 &&
-	git add F2 &&
-	(
-		PATH=./test-bin:$PATH &&
-		git rebase --continue
-	) &&
-	test -f funny.was.run
-'
-
 test_expect_success 'rebase -r passes merge strategy options correctly' '
 	rm -fr .git/rebase-* &&
 	git reset --hard commit-new-file-F3-on-topic-branch &&