[PATCH 0/3] Cleanup rebase signoff tests

[PATCH 0/3] Cleanup rebase signoff tests

[Phillip Wood via GitGitGadget]

This series cleans up the tests for "git rebase --signoff" in preparation
for extending them and fixing a couple of bugs in a future series. The
cleanups are:

 * move test setup into "test_expect_success"
 * stop running git upstream of a pipe
 * restore "git rebase --apply --signoff" coverage

Phillip Wood (3):
  t3428: modernize test setup
  t3428: use test_commit_message
  t3428: restore coverage for "apply" backend

 t/t3428-rebase-signoff.sh | 67 ++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 37 deletions(-)


base-commit: 19981daefd7c147444462739375462b49412ce33
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1713%2Fphillipwood%2Fcleanup-rebase-signoff-tests-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1713/phillipwood/cleanup-rebase-signoff-tests-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1713
-- 
gitgitgadget

[PATCH 1/3] t3428: modernize test setup

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Perform the setup in a dedicated test so the later tests can be run
independently. Also avoid running git upstream of a pipe and take
advantage of test_commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3428-rebase-signoff.sh | 42 +++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
index e1b1e947647..975b859ce09 100755
--- a/t/t3428-rebase-signoff.sh
+++ b/t/t3428-rebase-signoff.sh
@@ -8,37 +8,37 @@ This test runs git rebase --signoff and make sure that it works.
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-# A simple file to commit
-cat >file <<EOF
-a
-EOF
+test_expect_success 'setup' '
+	git commit --allow-empty -m "Initial empty commit" &&
+	test_commit first file a &&
+
+	ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" &&
 
-# Expected commit message for initial commit after rebase --signoff
-cat >expected-initial-signed <<EOF
-Initial empty commit
+	# Expected commit message for initial commit after rebase --signoff
+	cat >expected-initial-signed <<-EOF &&
+	Initial empty commit
 
-Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")
-EOF
+	Signed-off-by: $ident
+	EOF
 
-# Expected commit message after rebase --signoff
-cat >expected-signed <<EOF
-first
+	# Expected commit message after rebase --signoff
+	cat >expected-signed <<-EOF &&
+	first
 
-Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")
-EOF
+	Signed-off-by: $ident
+	EOF
 
-# Expected commit message after rebase without --signoff (or with --no-signoff)
-cat >expected-unsigned <<EOF
-first
-EOF
+	# Expected commit message after rebase without --signoff (or with --no-signoff)
+	cat >expected-unsigned <<-EOF &&
+	first
+	EOF
 
+	git config alias.rbs "rebase --signoff"
+'
 
 # We configure an alias to do the rebase --signoff so that
 # on the next subtest we can show that --no-signoff overrides the alias
 test_expect_success 'rebase --signoff adds a sign-off line' '
-	git commit --allow-empty -m "Initial empty commit" &&
-	git add file && git commit -m first &&
-	git config alias.rbs "rebase --signoff" &&
 	git rbs HEAD^ &&
 	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
 	test_cmp expected-signed actual
-- 
gitgitgadget

[PATCH 2/3] t3428: use test_commit_message

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Using a helper function makes the tests shorter and avoids running "git
cat-file" upstream of a pipe.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3428-rebase-signoff.sh | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
index 975b859ce09..133e54114f6 100755
--- a/t/t3428-rebase-signoff.sh
+++ b/t/t3428-rebase-signoff.sh
@@ -40,15 +40,13 @@ test_expect_success 'setup' '
 # on the next subtest we can show that --no-signoff overrides the alias
 test_expect_success 'rebase --signoff adds a sign-off line' '
 	git rbs HEAD^ &&
-	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
-	test_cmp expected-signed actual
+	test_commit_message HEAD expected-signed
 '
 
 test_expect_success 'rebase --no-signoff does not add a sign-off line' '
 	git commit --amend -m "first" &&
 	git rbs --no-signoff HEAD^ &&
-	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
-	test_cmp expected-unsigned actual
+	test_commit_message HEAD expected-unsigned
 '
 
 test_expect_success 'rebase --exec --signoff adds a sign-off line' '
@@ -56,30 +54,25 @@ test_expect_success 'rebase --exec --signoff adds a sign-off line' '
 	git commit --amend -m "first" &&
 	git rebase --exec "touch exec" --signoff HEAD^ &&
 	test_path_is_file exec &&
-	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
-	test_cmp expected-signed actual
+	test_commit_message HEAD expected-signed
 '
 
 test_expect_success 'rebase --root --signoff adds a sign-off line' '
 	git commit --amend -m "first" &&
 	git rebase --root --keep-empty --signoff &&
-	git cat-file commit HEAD^ | sed -e "1,/^\$/d" >actual &&
-	test_cmp expected-initial-signed actual &&
-	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
-	test_cmp expected-signed actual
+	test_commit_message HEAD^ expected-initial-signed &&
+	test_commit_message HEAD expected-signed
 '
 
 test_expect_success 'rebase -i --signoff fails' '
 	git commit --amend -m "first" &&
 	git rebase -i --signoff HEAD^ &&
-	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
-	test_cmp expected-signed actual
+	test_commit_message HEAD expected-signed
 '
 
 test_expect_success 'rebase -m --signoff fails' '
 	git commit --amend -m "first" &&
 	git rebase -m --signoff HEAD^ &&
-	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
-	test_cmp expected-signed actual
+	test_commit_message HEAD expected-signed
 '
 test_done
-- 
gitgitgadget

Re: [PATCH 2/3] t3428: use test_commit_message

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Using a helper function makes the tests shorter and avoids running "git
> cat-file" upstream of a pipe.

Nice.

[PATCH 3/3] t3428: restore coverage for "apply" backend

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

This test file assumes the "apply" backend is the default which is not
the case since 2ac0d6273f (rebase: change the default backend from "am"
to "merge", 2020-02-15). Make sure the "apply" backend is tested by
specifying it explicitly.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3428-rebase-signoff.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
index 133e54114f6..1bebd1ce74a 100755
--- a/t/t3428-rebase-signoff.sh
+++ b/t/t3428-rebase-signoff.sh
@@ -38,8 +38,8 @@ test_expect_success 'setup' '
 
 # We configure an alias to do the rebase --signoff so that
 # on the next subtest we can show that --no-signoff overrides the alias
-test_expect_success 'rebase --signoff adds a sign-off line' '
-	git rbs HEAD^ &&
+test_expect_success 'rebase --apply --signoff adds a sign-off line' '
+	git rbs --apply HEAD^ &&
 	test_commit_message HEAD expected-signed
 '
 
-- 
gitgitgadget

Re: [PATCH 3/3] t3428: restore coverage for "apply" backend

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> This test file assumes the "apply" backend is the default which is not
> the case since 2ac0d6273f (rebase: change the default backend from "am"
> to "merge", 2020-02-15). Make sure the "apply" backend is tested by
> specifying it explicitly.

Hmph, doesn't this lose coverage for the merge backend, though?

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  t/t3428-rebase-signoff.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
> index 133e54114f6..1bebd1ce74a 100755
> --- a/t/t3428-rebase-signoff.sh
> +++ b/t/t3428-rebase-signoff.sh
> @@ -38,8 +38,8 @@ test_expect_success 'setup' '
>  
>  # We configure an alias to do the rebase --signoff so that
>  # on the next subtest we can show that --no-signoff overrides the alias
> -test_expect_success 'rebase --signoff adds a sign-off line' '
> -	git rbs HEAD^ &&
> +test_expect_success 'rebase --apply --signoff adds a sign-off line' '
> +	git rbs --apply HEAD^ &&
>  	test_commit_message HEAD expected-signed
>  '

Re: [PATCH 3/3] t3428: restore coverage for "apply" backend

[phillip.wood123]

Hi Junio

On 10/04/2024 00:08, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> This test file assumes the "apply" backend is the default which is not
>> the case since 2ac0d6273f (rebase: change the default backend from "am"
>> to "merge", 2020-02-15). Make sure the "apply" backend is tested by
>> specifying it explicitly.
> 
> Hmph, doesn't this lose coverage for the merge backend, though?

I don't think so, we had coverage for the merge backend from the other 
tests before 2ac0d6273f as all of the other tests in this file use the 
merge backend. We're no longer testing "--signoff" without specifying 
some other option that selects a backend but it seems unlikely that we 
could break that without breaking one of the other tests.

Best Wishes

Phillip

> 
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   t/t3428-rebase-signoff.sh | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
>> index 133e54114f6..1bebd1ce74a 100755
>> --- a/t/t3428-rebase-signoff.sh
>> +++ b/t/t3428-rebase-signoff.sh
>> @@ -38,8 +38,8 @@ test_expect_success 'setup' '
>>   
>>   # We configure an alias to do the rebase --signoff so that
>>   # on the next subtest we can show that --no-signoff overrides the alias
>> -test_expect_success 'rebase --signoff adds a sign-off line' '
>> -	git rbs HEAD^ &&
>> +test_expect_success 'rebase --apply --signoff adds a sign-off line' '
>> +	git rbs --apply HEAD^ &&
>>   	test_commit_message HEAD expected-signed
>>   '

Re: [PATCH 3/3] t3428: restore coverage for "apply" backend

[Junio C Hamano]

phillip.wood123@gmail.com writes:

>> Hmph, doesn't this lose coverage for the merge backend, though?
>
> I don't think so, we had coverage for the merge backend from the other
> tests before 2ac0d6273f as all of the other tests in this file use the
> merge backend. We're no longer testing "--signoff" without specifying
> some other option that selects a backend but it seems unlikely that we
> could break that without breaking one of the other tests.

OK, so we have "rebase --merge --signoff" tested elsewhere and we
are replacing "rebase --signoff" with "rebase --apply --signoff"?

Thanks.

Re: [PATCH 3/3] t3428: restore coverage for "apply" backend

[phillip.wood123]

On 10/04/2024 15:22, Junio C Hamano wrote:
> phillip.wood123@gmail.com writes:
> 
>>> Hmph, doesn't this lose coverage for the merge backend, though?
>>
>> I don't think so, we had coverage for the merge backend from the other
>> tests before 2ac0d6273f as all of the other tests in this file use the
>> merge backend. We're no longer testing "--signoff" without specifying
>> some other option that selects a backend but it seems unlikely that we
>> could break that without breaking one of the other tests.
> 
> OK, so we have "rebase --merge --signoff" tested elsewhere and we
> are replacing "rebase --signoff" with "rebase --apply --signoff"?

Exactly

> Thanks.
> 

Re: [PATCH 3/3] t3428: restore coverage for "apply" backend

[Junio C Hamano]

phillip.wood123@gmail.com writes:

> On 10/04/2024 15:22, Junio C Hamano wrote:
>> phillip.wood123@gmail.com writes:
>> 
>>>> Hmph, doesn't this lose coverage for the merge backend, though?
>>>
>>> I don't think so, we had coverage for the merge backend from the other
>>> tests before 2ac0d6273f as all of the other tests in this file use the
>>> merge backend. We're no longer testing "--signoff" without specifying
>>> some other option that selects a backend but it seems unlikely that we
>>> could break that without breaking one of the other tests.
>> OK, so we have "rebase --merge --signoff" tested elsewhere and we
>> are replacing "rebase --signoff" with "rebase --apply --signoff"?
>
> Exactly

Perhaps we can write that in the log message to help the next person
who reads the patch?  Something like...

    t3428: restore coverage for "apply" backend
    
    This test file assumes the "apply" backend is the default which is
    not the case since 2ac0d6273f (rebase: change the default backend
    from "am" to "merge", 2020-02-15).  The way "merge" backend honors
    "--signoff" is already tested elsewhere, so make sure the "apply"
    backend is tested by specifying it explicitly.
    
    Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: [PATCH 3/3] t3428: restore coverage for "apply" backend

[phillip.wood123]

On 10/04/2024 17:45, Junio C Hamano wrote:
> phillip.wood123@gmail.com writes:
> 
>> On 10/04/2024 15:22, Junio C Hamano wrote:
>>> phillip.wood123@gmail.com writes:
>>>
>>>>> Hmph, doesn't this lose coverage for the merge backend, though?
>>>>
>>>> I don't think so, we had coverage for the merge backend from the other
>>>> tests before 2ac0d6273f as all of the other tests in this file use the
>>>> merge backend. We're no longer testing "--signoff" without specifying
>>>> some other option that selects a backend but it seems unlikely that we
>>>> could break that without breaking one of the other tests.
>>> OK, so we have "rebase --merge --signoff" tested elsewhere and we
>>> are replacing "rebase --signoff" with "rebase --apply --signoff"?
>>
>> Exactly
> 
> Perhaps we can write that in the log message to help the next person
> who reads the patch?  Something like...
> 
>      t3428: restore coverage for "apply" backend
>      
>      This test file assumes the "apply" backend is the default which is
>      not the case since 2ac0d6273f (rebase: change the default backend
>      from "am" to "merge", 2020-02-15).  The way "merge" backend honors
>      "--signoff" is already tested elsewhere, so make sure the "apply"
>      backend is tested by specifying it explicitly.
>      
>      Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>      Signed-off-by: Junio C Hamano <gitster@pobox.com>

Sounds good, I'll send a re-roll

Thanks

Phillip

Re: [PATCH 3/3] t3428: restore coverage for "apply" backend

[Phillip Wood]

On 12/04/2024 10:33, phillip.wood123@gmail.com wrote:
> On 10/04/2024 17:45, Junio C Hamano wrote:
>> phillip.wood123@gmail.com writes:
>> Perhaps we can write that in the log message to help the next person
>> who reads the patch?  Something like...
>>
>>      t3428: restore coverage for "apply" backend
>>      This test file assumes the "apply" backend is the default which is
>>      not the case since 2ac0d6273f (rebase: change the default backend
>>      from "am" to "merge", 2020-02-15).  The way "merge" backend honors
>>      "--signoff" is already tested elsewhere, so make sure the "apply"
>>      backend is tested by specifying it explicitly.
>>      Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>>      Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
> Sounds good, I'll send a re-roll

Oh, it looks like this hit next yesterday

Phillip