Re: [PATCH v7 5/5] rebase: support --trailer
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Mar 3, 2026, 15:05 UTC
- Message-ID
- <824809c3-72ac-43fb-8a93-4f48e0727e6a@gmail.com>
- In-Reply-To
- <20260224070552.148591-6-me@linux.beauty>
Hi Li
On 24/02/2026 07:05, Li Chen wrote:
Show 13 quoted lines
> > 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`
Show 11 quoted lines
> 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.
Show 5 quoted lines
> @@ -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
Show 215 quoted lines
> + 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