From: Phillip Wood Date: Wed, 06 Aug 2025 10:28:58 GMT Subject: Re: [PATCH v3 2/2] rebase: support --trailer Message-ID: In-Reply-To: <20250803150059.402017-3-me@linux.beauty> Hi Li On 03/08/2025 16:00, Li Chen wrote: > From: Li Chen Picking up from where I left off yesterday ... > diff --git a/t/meson.build b/t/meson.build > index 09f3068f98..3c58f562da 100644 > --- a/t/meson.build > +++ b/t/meson.build > @@ -373,6 +373,7 @@ integration_tests = [ > 't3436-rebase-more-options.sh', > 't3437-rebase-fixup-options.sh', > 't3438-rebase-broken-files.sh', > + 't3440-rebase-trailer.sh', The alignment looks off here > 't3500-cherry.sh', > 't3501-revert-cherry-pick.sh', > 't3502-cherry-pick-merge.sh', > diff --git a/t/t3440-rebase-trailer.sh b/t/t3440-rebase-trailer.sh > new file mode 100755 > index 0000000000..a580449628 > --- /dev/null > +++ b/t/t3440-rebase-trailer.sh > @@ -0,0 +1,95 @@ > +#!/bin/sh > +# > + > +test_description='git rebase --trailer integration tests > +We verify that --trailer on the merge/interactive/exec/root backends, There are only two backends "apply" and "merge", the other things you have listed are just command line options. We don't actually test --exec or --interactive with --trailer in this file so we should reword that comment. There is no need to add tests for those. > +and that it is rejected early when the apply backend is requested.' > + > +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main > +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > + > +. ./test-lib.sh > +. "$TEST_DIRECTORY"/lib-rebase.sh # test_commit_message, helpers > + > +create_expect() { > + cat >"$1" <<-EOF > + $2 > + > + Reviewed-by: Dev > + EOF > +} > + > +test_expect_success 'setup repo with a small history' ' > + git commit --allow-empty -m "Initial empty commit" && > + test_commit first file a && > + test_commit second file && > + git checkout -b conflict-branch first && > + test_commit file-2 file-2 && > + test_commit conflict file && > + test_commit third file && > + ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" && > + create_expect initial-signed "Initial empty commit" && > + create_expect first-signed "first" && > + create_expect second-signed "second" && > + create_expect file2-signed "file-2" && > + create_expect third-signed "third" && > + create_expect conflict-signed "conflict" Normally we create the "expect" file in the test where it is used. > +' > + > +test_expect_success 'apply backend is rejected with --trailer' ' We know HEAD is at third so we don't need this > + git reset --hard third && > + head_before=$(git rev-parse HEAD) && > + test_expect_code 128 \ The indentation is off here > + git rebase --apply --trailer "Reviewed-by: Dev " \ > + HEAD^ 2>err && > + test_grep "requires the merge backend" err && I think it is worth checking that the error message includes --trailer as well to make sure that is the option that is triggering the error. > + test_cmp_rev HEAD $head_before > +' > + > +test_expect_success 'reject empty --trailer argument' ' > + git reset --hard third && The exact commit is not important here > + test_expect_code 128 git rebase -m --trailer "" HEAD^ 2>err && > + test_grep "empty --trailer" err > +' > + > +test_expect_success 'reject trailer with missing key before separator' ' > + git reset --hard third && Same here - we're only checking the error message so the exact value of HEAD is not important. > + test_expect_code 128 git rebase -m --trailer ": no-key" HEAD^ 2>err && > + test_grep "missing key before separator" err > +' > + > +test_expect_success 'CLI trailer duplicates allowed; replace policy keeps last' ' > + git reset --hard third && > + git -c trailer.Bug.ifexists=replace -c trailer.Bug.ifmissing=add rebase -m --trailer "Bug: 123" --trailer "Bug: 456" HEAD~1 && > + git cat-file commit HEAD | grep "^Bug: 456" && > + git cat-file commit HEAD | grep -v "^Bug: 123" Piping git into another command is discouraged as git can potentially fail without us noticing. Here I think it would be better to use test_commit_message() to check the whole message. > +' > + > +test_expect_success 'multiple Signed-off-by trailers all preserved' ' > + git reset --hard third && We can avoid this by passing the commit we want to checkout to rebase as you do in the test below. > + git rebase -m \ > + --trailer "Signed-off-by: Dev A " \ > + --trailer "Signed-off-by: Dev B " HEAD~1 && > + git cat-file commit HEAD | grep -c "^Signed-off-by:" >count && > + test "$(cat count)" = 2 # two new commits Let check the actual message here as well so if it fails we can see what the message is. > +' > + > +test_expect_success 'rebase -m --trailer adds trailer after conflicts' ' > + git reset --hard third && We don't need this as the rebase command checks out third for us, saving a process which is always nice. > + test_must_fail git rebase -m \ > + --trailer "Reviewed-by: Dev " \ > + second third && > + git checkout --theirs file && > + git add file && > + git rebase --continue && The indentation is off here > + test_commit_message HEAD~2 file2-signed It's good to see this uses test_commit_message but why are we checking HEAD~2 rather than HEAD?> +' > + > +test_expect_success 'rebase --root --trailer updates every commit' ' > + git checkout first && > + git rebase --root --keep-empty \ --keep-empty is the default these days so I think we can drop that. > + --trailer "Reviewed-by: Dev " && > + test_commit_message HEAD first-signed && > + test_commit_message HEAD^ initial-signed Looks good. While there are some small issues to fix the tests look sensible and I think you have good coverage of the new option. Thanks Phillip