Re: [PATCH v7 5/5] rebase: support --trailer

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

Hi Li

On 24/02/2026 07:05, Li Chen wrote:
> 
> diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc
> index e177808004..908717991a 100644
> --- a/Documentation/git-rebase.adoc
> +++ b/Documentation/git-rebase.adoc
> @@ -497,6 +497,13 @@ See also INCOMPATIBLE OPTIONS below.
>   +
>   See also INCOMPATIBLE OPTIONS below.
>   
> +--trailer=<trailer>::
> +	Append the given trailer to every rebased commit message, processed
> +	via linkgit:git-interpret-trailers[1]. This option implies
> +	`--force-rebase` so that fast-forwarded commits are also rewritten.

If the commits are rewritten then they're not fast-forwarded, I just say 
"This option implies `--force-rebase`." as all the other options that 
imply `--force-rebase` do.

> +See also INCOMPATIBLE OPTIONS below.

That section needs updating to include `--trailer`

> diff --git a/sequencer.c b/sequencer.c
> index a3eb39bb25..a60c2a0cde 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> [...]
> @@ -2025,6 +2027,9 @@ static int append_squash_message(struct strbuf *buf, const char *body,
>   		if (opts->signoff)
>   			append_signoff(buf, 0, 0);
>   
> +		if (opts->trailer_args.nr)
> +			amend_strbuf_with_trailers(buf, &opts->trailer_args);

I wonder if it would be better to add the trailers before the signoff so 
that "git rebase --signoff --trailer='Reviewed-by: ...'" adds the 
"Reviewed-by:" trailer before the "Signed-off-by:" trailer.

> @@ -3234,6 +3242,18 @@ static int read_populate_opts(struct replay_opts *opts)
>   
>   		read_strategy_opts(opts, &buf);
>   		strbuf_reset(&buf);
> +		if (strbuf_read_file(&buf, rebase_path_trailer(), 0) >= 0) {

Most of the other options are read with read_oneliner() which warns if 
there is a read error - we should do the same here. We should also warn 
if this file is empty as we only write it when trailer_args.nr > 0.

This is all looking pretty good

Thanks

Phillip

> +			char *p = buf.buf, *nl;
> +
> +			while ((nl = strchr(p, '\n'))) {
> +				*nl = '\0';
> +				if (!*p)
> +					BUG("rebase-merge/trailer has an empty line");
> +				strvec_push(&opts->trailer_args, p);
> +				p = nl + 1;
> +			}
> +			strbuf_reset(&buf);
> +		}
>   
>   		if (read_oneliner(&ctx->current_fixups,
>   				  rebase_path_current_fixups(),
> @@ -3328,6 +3348,14 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
>   		write_file(rebase_path_reschedule_failed_exec(), "%s", "");
>   	else
>   		write_file(rebase_path_no_reschedule_failed_exec(), "%s", "");
> +	if (opts->trailer_args.nr) {
> +		struct strbuf buf = STRBUF_INIT;
> +
> +		for (size_t i = 0; i < opts->trailer_args.nr; i++)
> +			strbuf_addf(&buf, "%s\n", opts->trailer_args.v[i]);
> +		write_file(rebase_path_trailer(), "%s", buf.buf);
> +		strbuf_release(&buf);
> +	}
>   
>   	return 0;
>   }
> diff --git a/sequencer.h b/sequencer.h
> index 719684c8a9..bea20da085 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -57,6 +57,8 @@ struct replay_opts {
>   	int ignore_date;
>   	int commit_use_reference;
>   
> +	struct strvec trailer_args;
> +
>   	int mainline;
>   
>   	char *gpg_sign;
> @@ -84,6 +86,7 @@ struct replay_opts {
>   #define REPLAY_OPTS_INIT {			\
>   	.edit = -1,				\
>   	.action = -1,				\
> +	.trailer_args = STRVEC_INIT,		\
>   	.xopts = STRVEC_INIT,			\
>   	.ctx = replay_ctx_new(),		\
>   }
> diff --git a/t/meson.build b/t/meson.build
> index f80e366cff..1f6f8ac20b 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -388,6 +388,7 @@ integration_tests = [
>     't3436-rebase-more-options.sh',
>     't3437-rebase-fixup-options.sh',
>     't3438-rebase-broken-files.sh',
> +  't3440-rebase-trailer.sh',
>     't3450-history.sh',
>     't3451-history-reword.sh',
>     't3500-cherry.sh',
> diff --git a/t/t3440-rebase-trailer.sh b/t/t3440-rebase-trailer.sh
> new file mode 100755
> index 0000000000..8b47579566
> --- /dev/null
> +++ b/t/t3440-rebase-trailer.sh
> @@ -0,0 +1,147 @@
> +#!/bin/sh
> +#
> +
> +test_description='git rebase --trailer integration tests
> +We verify that --trailer works with the merge backend,
> +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
> +
> +REVIEWED_BY_TRAILER="Reviewed-by: Dev <dev@example.com>"
> +SP=" "
> +
> +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 &&
> +	git checkout main
> +'
> +
> +test_expect_success 'apply backend is rejected with --trailer' '
> +	git checkout -B apply-backend third &&
> +	test_expect_code 128 \
> +		git rebase --apply --trailer "$REVIEWED_BY_TRAILER" HEAD^ 2>err &&
> +	test_grep "fatal: --trailer requires the merge backend" err
> +'
> +
> +test_expect_success 'reject empty --trailer argument' '
> +	git checkout -B empty-trailer third &&
> +	test_expect_code 128 git rebase --trailer "" HEAD^ 2>err &&
> +	test_grep "empty --trailer" err
> +'
> +
> +test_expect_success 'reject trailer with missing key before separator' '
> +	git checkout -B missing-key third &&
> +	test_expect_code 128 git rebase --trailer ": no-key" HEAD^ 2>err &&
> +	test_grep "missing key before separator" err
> +'
> +
> +test_expect_success 'allow trailer with missing value after separator' '
> +	git checkout -B missing-value third &&
> +	git rebase --trailer "Acked-by:" HEAD^ &&
> +	test_commit_message HEAD <<-EOF
> +	third
> +
> +	Acked-by:${SP}
> +	EOF
> +'
> +
> +test_expect_success 'CLI trailer duplicates allowed; replace policy keeps last' '
> +	git checkout -B replace-policy third &&
> +	git -c trailer.Bug.ifexists=replace -c trailer.Bug.ifmissing=add \
> +		rebase --trailer "Bug: 123" --trailer "Bug: 456" HEAD^ &&
> +	test_commit_message HEAD <<-EOF
> +	third
> +
> +	Bug: 456
> +	EOF
> +'
> +
> +test_expect_success 'multiple Signed-off-by trailers all preserved' '
> +	git checkout -B multiple-signoff third &&
> +	git rebase --trailer "Signed-off-by: Dev A <a@example.com>" \
> +		--trailer "Signed-off-by: Dev B <b@example.com>" HEAD^ &&
> +	test_commit_message HEAD <<-EOF
> +	third
> +
> +	Signed-off-by: Dev A <a@example.com>
> +	Signed-off-by: Dev B <b@example.com>
> +	EOF
> +'
> +
> +test_expect_success 'rebase --trailer adds trailer after conflicts' '
> +	git checkout -B trailer-conflict third &&
> +	test_commit fourth file &&
> +	test_must_fail git rebase --trailer "$REVIEWED_BY_TRAILER" second &&
> +	git checkout --theirs file &&
> +	git add file &&
> +	git rebase --continue &&
> +	test_commit_message HEAD <<-EOF &&
> +	fourth
> +
> +	$REVIEWED_BY_TRAILER
> +	EOF
> +	test_commit_message HEAD^ <<-EOF
> +	third
> +
> +	$REVIEWED_BY_TRAILER
> +	EOF
> +'
> +
> +test_expect_success '--trailer handles fixup commands in todo list' '
> +	git checkout -B fixup-trailer third &&
> +	test_commit fixup-base base &&
> +	test_commit fixup-second second &&
> +	cat >todo <<-\EOF &&
> +	pick fixup-base fixup-base
> +	fixup fixup-second fixup-second
> +	EOF
> +	(
> +		set_replace_editor todo &&
> +		git rebase -i --trailer "$REVIEWED_BY_TRAILER" HEAD~2
> +	) &&
> +	test_commit_message HEAD <<-EOF &&
> +	fixup-base
> +
> +	$REVIEWED_BY_TRAILER
> +	EOF
> +	git reset --hard fixup-second &&
> +	cat >todo <<-\EOF &&
> +	pick fixup-base fixup-base
> +	fixup -C fixup-second fixup-second
> +	EOF
> +	(
> +		set_replace_editor todo &&
> +		git rebase -i --trailer "$REVIEWED_BY_TRAILER" HEAD~2
> +	) &&
> +	test_commit_message HEAD <<-EOF
> +	fixup-second
> +
> +	$REVIEWED_BY_TRAILER
> +	EOF
> +'
> +
> +test_expect_success 'rebase --root honors trailer.<name>.key' '
> +	git checkout -B root-trailer first &&
> +	git -c trailer.review.key=Reviewed-by rebase --root \
> +		--trailer=review="Dev <dev@example.com>" &&
> +	test_commit_message HEAD <<-EOF &&
> +	first
> +
> +	Reviewed-by: Dev <dev@example.com>
> +	EOF
> +	test_commit_message HEAD^ <<-EOF
> +	Initial empty commit
> +
> +	Reviewed-by: Dev <dev@example.com>
> +	EOF
> +'
> +test_done