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

[Phillip Wood <phillip.wood123@gmail.com>]

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 ...

> 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 <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.

> +	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 <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.

> +	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