From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 06 Aug 2025 13:19:57 GMT
Subject: Re: [PATCH v3 2/2] rebase: support --trailer
Message-ID: <499da566-66a8-4c38-a2b3-13c06092568f@gmail.com>
In-Reply-To: <e911d897-8664-40a7-b7a9-8eb9f71a8735@gmail.com>

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