Re: [PATCH v3 2/2] rebase: support --trailer
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Aug 6, 2025, 10:28 UTC
- Message-ID
- <e911d897-8664-40a7-b7a9-8eb9f71a8735@gmail.com>
- In-Reply-To
- <20250803150059.402017-3-me@linux.beauty>
Hi Li
On 03/08/2025 16:00, Li Chen wrote:
> From: Li Chen <chenl311@chinatelecom.cn>
Picking up from where I left off yesterday ...
Show 9 quoted lines
> 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
Show 14 quoted lines
> '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.
Show 31 quoted lines
> +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 <dev@example.com>
> + 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 <dev@example.com>" \ > + 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.
Show 5 quoted lines
> + test_cmp_rev HEAD $head_before > +' > + > +test_expect_success 'reject empty --trailer argument' ' > + git reset --hard third &&
The exact commit is not important here
Show 6 quoted lines
> + 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.
Show 9 quoted lines
> + 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.
Show 5 quoted lines
> + git rebase -m \ > + --trailer "Signed-off-by: Dev A <a@ex.com>" \ > + --trailer "Signed-off-by: Dev B <b@ex.com>" 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.
Show 6 quoted lines
> + 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 &&
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 <dev@example.com>" && > + 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