| Message ID | b45af37e3c0a22cc9e0514eb681300be0b968e02.1712676444.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b4454d5a7bbe8499a56bd428c1e7a671f744c533 |
| Headers | show |
| Series | Cleanup rebase signoff tests | expand |
"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 > '
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 >> '
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.
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. >
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>
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
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
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 '