From: Phillip Wood Date: Tue, 03 Mar 2026 15:05:10 GMT Subject: Re: [PATCH v7 5/5] rebase: support --trailer 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: > > 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=:: > + 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 " > +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 " \ > + --trailer "Signed-off-by: Dev B " HEAD^ && > + test_commit_message HEAD <<-EOF > + third > + > + Signed-off-by: Dev A > + Signed-off-by: Dev B > + 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..key' ' > + git checkout -B root-trailer first && > + git -c trailer.review.key=Reviewed-by rebase --root \ > + --trailer=review="Dev " && > + test_commit_message HEAD <<-EOF && > + first > + > + Reviewed-by: Dev > + EOF > + test_commit_message HEAD^ <<-EOF > + Initial empty commit > + > + Reviewed-by: Dev > + EOF > +' > +test_done