threads / patch / 61300

patch, 3 partsCleanup rebase signoff tests

Subject: [PATCH 0/3] Cleanup rebase signoff tests

## tl;dr

12 messages between Apr 9, 2024 and Apr 12, 2024. Diffs are folded; open one to read it.

replies: 11people: 2as markdown or json

Phillip Wood via GitGitGadget· Apr 9, 2024, 15:27 UTC · lore

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
Phillip Wood via GitGitGadget· Apr 9, 2024, 15:27 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH 1/3] t3428: modernize test setup

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(-)
Show changes to t/t3428-rebase-signoff.sh +21 −21
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
Phillip Wood via GitGitGadget· Apr 9, 2024, 15:27 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH 2/3] t3428: use test_commit_message

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(-)
Show changes to t/t3428-rebase-signoff.sh +7 −14
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
Junio C Hamano· Apr 9, 2024, 23:07 UTC · re: Phillip Wood via GitGitGadget · lore

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

"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.
Phillip Wood via GitGitGadget· Apr 9, 2024, 15:27 UTC · re: Phillip Wood via GitGitGadget · lore

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

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(-)
Show changes to t/t3428-rebase-signoff.sh +2 −2
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
Junio C Hamano· Apr 9, 2024, 23:08 UTC · re: Phillip Wood via GitGitGadget · lore

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

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 6 quoted lines
> 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?
Show 19 quoted lines
> 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
>  '
phillip.wood123@gmail.com· Apr 10, 2024, 09:42 UTC · re: Junio C Hamano · lore

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

Hi Junio
On 10/04/2024 00:08, Junio C Hamano wrote:
Show 10 quoted lines
> "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
Show 20 quoted lines
> 
>> 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
>>   '
Junio C Hamano· Apr 10, 2024, 14:22 UTC · re: phillip.wood123@gmail.com · lore

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

phillip.wood123@gmail.com writes:
Show 7 quoted lines
>> 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.
phillip.wood123@gmail.com· Apr 10, 2024, 15:23 UTC · re: Junio C Hamano · lore

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

On 10/04/2024 15:22, Junio C Hamano wrote:
Show 12 quoted lines
> 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.
>
Junio C Hamano· Apr 10, 2024, 16:45 UTC · re: phillip.wood123@gmail.com · lore

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

phillip.wood123@gmail.com writes:
Show 14 quoted lines
> 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>
phillip.wood123@gmail.com· Apr 12, 2024, 09:33 UTC · re: Junio C Hamano · lore

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

On 10/04/2024 17:45, Junio C Hamano wrote:
Show 30 quoted lines
> 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
Phillip Wood· Apr 12, 2024, 13:53 UTC · re: phillip.wood123@gmail.com · lore

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

On 12/04/2024 10:33, phillip.wood123@gmail.com wrote:
Show 15 quoted lines
> 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

← back to recent threads