Re: [PATCH v3 2/2] rebase: support --trailer

 ---- On Wed, 06 Aug 2025 21:19:57 +0800  Phillip Wood <phillip.wood123@gmail.com> wrote --- 
 > Hi Li
 > 
 > I had a couple more thoughts about the tests ...
 > 
 > On 06/08/2025 11:28, Phillip Wood wrote:
 > > On 03/08/2025 16:00, Li Chen wrote:
 > >> +create_expect() {
 > >> +    cat >"$1" <<-EOF
 > >> +        $2
 > >> +
 > >> +        Reviewed-by: Dev <dev@example.com>
 > >> +    EOF
 > >> +}
 > >> +
 > >> +test_expect_success 'setup repo with a small history' '
 > >> [...]
 >  >> +    create_expect third-signed    "third"                 &&>> +    
 > create_expect conflict-signed "conflict"
 > > 
 > > Normally we create the "expect" file in the test where it is used.
 > 
 > Thinking about this some more, if we want to use test_commit_message 
 > then I think we can change create_expect to write to stdout and do
 > 
 >     test_commit_message HEAD <<-EOF
 >     $(create_expect first)
 >     EOF
 > 
 > rather than having to create a file.
 > 
 > >> +
 > >> +test_expect_success 'reject empty --trailer argument' '
 > >> [...]
 > >> +test_expect_success 'reject trailer with missing key before separator' '
 > 
 > Should we also test for a missing value or are trailers without a value 
 > allowed?
 > 
 > >> +        git rebase -m \
 > >> +            --trailer "Signed-off-by: Dev A <a@ex.com>" \
 > >> +            --trailer "Signed-off-by: Dev B <b@ex.com>" HEAD~1 &&
 > 
 > Lets use example.com here rather than some random domain that might 
 > actually exist.
 > 
 > >> +test_expect_success 'rebase -m --trailer adds trailer after conflicts' '
 > >> +    git reset --hard third &&
 > >> +    test_must_fail git rebase -m \
 > >> +        --trailer "Reviewed-by: Dev <dev@example.com>" \
 > >> +        second third &&
 > >> +    git checkout --theirs file &&
 > >> +    git add file &&
 > >> +    git rebase --continue &&
 > 
 > This checks that the commit with conflicts has a trailer added but it 
 > does not check that the commits picked by "git rebase --continue" do. To 
 > check that we actually save the trailers and use them when continuing we 
 > need to add a fourth commit on top of third and check that has a trailer 
 > add here as well.
 > 
 > A couple more thoughts:
 > 
 >   - We should check that
 >       git -c trailer.review.key=Reviewed-by rebase \
 >           --trailer=review="Dev <dev@example.com>"
 >     adds a "Reviewed-by:" trailer. We can do that by changing one of the
 >     tests in this patch rather than adding a new one. This checks that we
 >     accept '=' as a separator as well a respecting the config.
 > 
 >   - We should check that the todo list
 >       pick first
 >       fixup second
 >     adds the trailer as expected and that
 >       pick first
 >       fixup -C second
 >     also works. To do that we will need to source lib-rebase.sh at the
 >     start of the test file and add a test that uses set_replace_editor()
 >     which should be called in a subshell.
 > 
 > 
 > Do please ask if you have any questions about these suggestions
 > 
 > Thanks
 > 
 > Phillip
 > 
 > 
Regards,

Li​