git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v6 4/4] rebase: support --trailer

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Nov 12, 2025, 14:48 UTC
Message-ID
<0413bf2e-52c4-4944-a349-2a922dd463a2@gmail.com>
In-Reply-To
<20251105142944.73061-5-me@linux.beauty>
Hi Li
On 05/11/2025 14:29, Li Chen wrote:
Show 16 quoted lines
> From: Li Chen <chenl311@chinatelecom.cn>
> 
> diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc
> index 005caf6164..4d2fe4be6e 100644
> --- a/Documentation/git-rebase.adoc
> +++ b/Documentation/git-rebase.adoc
> @@ -487,9 +487,16 @@ See also INCOMPATIBLE OPTIONS below.
>   	Add a `Signed-off-by` trailer to all the rebased commits. Note
>   	that if `--interactive` is given then only commits marked to be
>   	picked, edited or reworded will have the trailer added.
> -+
> +
>   See also INCOMPATIBLE OPTIONS below.
>   
> +--trailer=<trailer>::
> +       Append the given trailer line(s) to every rebased commit

I'm not sure we need to say "line(s)" here. "Append the given trailer to every ..." would be fine I think.

> +       message, processed via linkgit:git-interpret-trailers[1].
> +       When this option is present *rebase automatically implies*
> +       `--force-rebase` so that fast‑forwarded commits are also
> +       rewritten.

Normally in cases like this we just say "This option implies `--force-rebase`".

Show 25 quoted lines
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index c468828189..a88abe08b4 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -500,6 +508,23 @@ static int read_basic_state(struct rebase_options *opts)
>   		opts->gpg_sign_opt = xstrdup(buf.buf);
>   	}
>   
> +	strbuf_reset(&buf);
> +
> +	if (strbuf_read_file(&buf, state_dir_path("trailer", opts), 0) >= 0) {
> +		const char *p = buf.buf, *end = buf.buf + buf.len;
> +
> +		while (p < end) {
> +			char *nl = memchr(p, '\n', end - p);
> +			if (!nl)
> +				die("nl shouldn't be NULL");
> +			*nl = '\0';
> +
> +			if (*p)
> +				strvec_push(&opts->trailer_args, p);
> +
> +			p = nl + 1;
> +		}
> +	}

As "--trailer" is only supported by the "merge" backend we only need to read this file in sequencer.c:read_populate_opts(), there is no point in reading it here.

Show 10 quoted lines
>   	strbuf_release(&buf);
>   
>   	return 0;
> @@ -528,6 +553,21 @@ static int rebase_write_basic_state(struct rebase_options *opts)
>   	if (opts->signoff)
>   		write_file(state_dir_path("signoff", opts), "--signoff");
>   
> +	/*
> +	 * save opts->trailer_args into state_dir/trailer
> +	 */

As "--trailer" is not supported by the "apply" backend we can just rely on this being written by sequener.c:write_basic_state(), we don't need to do it here

Show 21 quoted lines
> +	if (opts->trailer_args.nr) {
> +		struct strbuf buf = STRBUF_INIT;
> +
> +		for (size_t i = 0; i < opts->trailer_args.nr; i++) {
> +				strbuf_addstr(&buf, opts->trailer_args.v[i]);
> +				strbuf_addch(&buf, '\n');
> +		}
> +		write_file(state_dir_path("trailer", opts),
> +				   "%s", buf.buf);
> +		strbuf_release(&buf);
> +	}
> +
>   	return 0;
>   }
>   
> @@ -1132,6 +1172,8 @@ int cmd_rebase(int argc,
>   			.flags = PARSE_OPT_NOARG,
>   			.defval = REBASE_DIFFSTAT,
>   		},
> +		OPT_STRVEC(0, "trailer", &options.trailer_args, N_("trailer"),
> +				N_("add custom trailer(s)")),

This line should be indented so that the "N_" aligns with "0" above like all the other options here. Putting this next to "--signoff" is a good choice.

Show 11 quoted lines
> diff --git a/sequencer.c b/sequencer.c
> index 5476d39ba9..fbf35cb474 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2025,6 +2027,10 @@ 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))
> +			return error(_("unable to add trailers to commit message"));
amend_strbuf_with_trailers() cannot really fail so this should be
		if (opts->trailer_args.nr)
			amend_strbuf_with_trailers(buf, &opts->trailer_args));
Show 10 quoted lines
> @@ -2443,6 +2449,14 @@ static int do_pick_commit(struct repository *r,
>   	if (opts->signoff && !is_fixup(command))
>   		append_signoff(&ctx->message, 0, 0);
>   
> +	if (opts->trailer_args.nr && !is_fixup(command)) {
> +		if (amend_strbuf_with_trailers(&ctx->message,
> +					       &opts->trailer_args)) {
> +			res = error(_("unable to add trailers to commit message"));
> +			goto leave;
> +		}

As above amend_strbuf_with_trailers() cannot fail so we don't need this error handling

Show 10 quoted lines
> +	}
> +
>   	if (is_rebase_i(opts) && write_author_script(msg.message) < 0)
>   		res = -1;
>   	else if (!opts->strategy ||
> @@ -2517,6 +2531,7 @@ static int do_pick_commit(struct repository *r,
>   			_("dropping %s %s -- patch contents already upstream\n"),
>   			oid_to_hex(&commit->object.oid), msg.subject);
>   	} /* else allow == 0 and there's nothing special to do */
> +
We try to avoid introducing unrelated white space changes
Show 10 quoted lines
> @@ -3234,6 +3249,17 @@ 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) {
> +			char *p = buf.buf, *nl;
> +
> +			while ((nl = strchr(p, '\n'))) {
> +				*nl = '\0';
> +				if (*p)

As we're in control of what's written to the file it is a BUG() if to contains any empty line.

Show 9 quoted lines
> diff --git a/sequencer.h b/sequencer.h
> index 719684c8a9..e21835c5a0 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -44,6 +44,7 @@ struct replay_opts {
>   	int record_origin;
>   	int no_commit;
>   	int signoff;
> +	struct strvec trailer_args;
I think it would be better to add this after all the flag options
Show 8 quoted lines
>   	int allow_ff;
>   	int allow_rerere_auto;
>   	int allow_empty;
> @@ -82,8 +83,9 @@ struct replay_opts {
>   	struct replay_ctx *ctx;
>   };
>   #define REPLAY_OPTS_INIT {			\
> -	.edit = -1,				\
".edit" is the first member so it makes sense to leave this at the start.
Show 6 quoted lines
>   	.action = -1,				\
> +	.edit = -1,				\
> +	.trailer_args = STRVEC_INIT, \
>   	.xopts = STRVEC_INIT,			\
>   	.ctx = replay_ctx_new(),		\
>   }
Show 37 quoted lines
> diff --git a/t/t3440-rebase-trailer.sh b/t/t3440-rebase-trailer.sh
> new file mode 100755
> index 0000000000..d0e0434664
> --- /dev/null
> +++ b/t/t3440-rebase-trailer.sh
> @@ -0,0 +1,134 @@
> +#!/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>"
> +
> +expect_trailer_msg() {
> +	test_commit_message "$1" <<-EOF
> +	$2
> +
> +	${3:-$REVIEWED_BY_TRAILER}
> +	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
This leaves us with conflict-branch checked out, is that intentional?
> +'
> +
> +test_expect_success 'apply backend is rejected with --trailer' '
> +	head_before=$(git rev-parse HEAD) &&
I'm not sure we really need to check that HEAD is unchanged here
Show 9 quoted lines
> +	test_expect_code 128 \
> +	git rebase --apply --trailer "$REVIEWED_BY_TRAILER" \
> +				HEAD^ 2>err &&
> +	test_grep "fatal: --trailer requires the merge backend" err &&
> +	test_cmp_rev HEAD $head_before
> +'
> +
> +test_expect_success 'reject empty --trailer argument' '
> +	test_expect_code 128 git rebase -m --trailer "" HEAD^ 2>err &&

There is no need to pass "-m" in any of these tests as it is the default and if the default backend changes in the future we will want "--trailer" keeps working with the new default.

Show 16 quoted lines
> +	test_grep "empty --trailer" err
> +'
> +
> +test_expect_success 'reject trailer with missing key before separator' '
> +	test_expect_code 128 git rebase -m --trailer ": no-key" HEAD^ 2>err &&
> +	test_grep "missing key before separator" err
> +'
> +
> +test_expect_success 'allow trailer with missing value after separator' '
> +	git rebase -m --trailer "Acked-by:" HEAD~1 third &&
> +	sed -e "s/_/ /g" <<-\EOF >expect &&
> +	third
> +
> +	Acked-by:_
> +	EOF
> +	test_commit_message HEAD expect

test_commit_message accepts a message on stdin so we don't need to create expect in all these tests. It is curious that we add a trailing space to the trailer, it would be nice if we could clean that up. Failing that other tests use SP=" " and then use ${SP} in the here document like

	test_commit_message HEAD <<-EOF
	third
	
	Acked-by:${SP}
	EOF
Show 17 quoted lines
> +'
> +
> +test_expect_success 'CLI trailer duplicates allowed; replace policy keeps last' '
> +	git -c trailer.Bug.ifexists=replace -c trailer.Bug.ifmissing=add \
> +		rebase -m --trailer "Bug: 123" --trailer "Bug: 456" HEAD~1 third &&
> +	cat >expect <<-\EOF &&
> +	third
> +
> +	Bug: 456
> +	EOF
> +	test_commit_message HEAD expect
> +'
> +
> +test_expect_success 'multiple Signed-off-by trailers all preserved' '
> +	git rebase -m \
> +			--trailer "Signed-off-by: Dev A <a@example.com>" \
> +			--trailer "Signed-off-by: Dev B <b@example.com>" HEAD~1 third &&
The massive indentation here leads to overly long lines.
	git rebase --trailer "Signed-off-by: Dev A <a@example.com>" \
		--trailer "Signed-off-by: Dev B <b@example.com>" HEAD~1 third &&
fits our 80 column limit
Show 11 quoted lines
> +	cat >expect <<-\EOF &&
> +	third
> +
> +	Signed-off-by: Dev A <a@example.com>
> +	Signed-off-by: Dev B <b@example.com>
> +	EOF
> +	test_commit_message HEAD expect
> +'
> +
> +test_expect_success 'rebase -m --trailer adds trailer after conflicts' '
> +	git checkout -B conflict-branch third &&
I'm not sure why we're recreating conflict-branch here, you can just run 
"git rebase [options] second third"
  > +	test_commit fourth file &&
> +	test_must_fail git rebase -m \
> +			--trailer "$REVIEWED_BY_TRAILER" \
> +			second &&
If you unfold this line it is 79 characters long which is perfctly fine.
> +	git checkout --theirs file &&
> +	git add file &&
> +	git rebase --continue &&
> +	expect_trailer_msg HEAD "fourth" &&

I think it would be easier to see what's being checked if we just used test_commit_message as we have done up to here and got rid of the test_trailer_msg.

> +	expect_trailer_msg HEAD^ "third"

This is good because we check that the conflicting commit gets a trailer and the commit picked by "git rebase --continue" does too.

> +'
> +
> +test_expect_success '--trailer handles fixup commands in todo list' '
> +	git checkout -B fixup-trailer HEAD &&

If you're going to create a new branch you should do it from a tag so it has a known starting point that is not dependent on the previous tests.

> +	test_commit fixup-base base &&
> +	test_commit fixup-second second &&
> +	first_short=$(git rev-parse --short fixup-base) &&
> +	second_short=$(git rev-parse --short fixup-second) &&

These two lines are unecessary as test_commit() creates a tag which we can use in the todo list. The here document should also be indented. So

> +	cat >todo <<EOF &&
> +pick $first_short fixup-base
> +fixup $second_short fixup-second
> +EOF
becomes
	cat >todo <<-\EOF &&
	pick fixup-base first-base
	fixup fixup-second fixup-second
	EOF
Show 5 quoted lines
> +	(
> +		set_replace_editor todo &&
> +		git rebase -i --trailer "$REVIEWED_BY_TRAILER" HEAD~2
> +	) &&
> +	expect_trailer_msg HEAD "fixup-base" &&
We check there is only one trailer after the fixup - good
Show 10 quoted lines
> +	git reset --hard fixup-second &&
> +	cat >todo <<EOF &&
> +pick $first_short fixup-base
> +fixup -C $second_short fixup-second
> +EOF
> +	(
> +		set_replace_editor todo &&
> +		git rebase -i --trailer "$REVIEWED_BY_TRAILER" HEAD~2
> +	) &&
> +	expect_trailer_msg HEAD "fixup-second"
We check that we add a trailer with "fixup -C" - good> +'
Show 5 quoted lines
> +
> +test_expect_success 'rebase --root --trailer updates every commit' '
> +	git checkout first &&
> +	git -c trailer.review.key=Reviewed-by rebase --root \
> +		--trailer=review="Dev <dev@example.com>" &&
It would be good if the test title mentioned that we're also checking 
'trailer.<name>.key' here as well which is more interesting than "--root"
  > +	expect_trailer_msg HEAD  "first" &&
> +	expect_trailer_msg HEAD^ "Initial empty commit"
The test coverage looks good
Show 11 quoted lines
> +'
> +test_done
> diff --git a/trailer.c b/trailer.c
> index f5838f5699..f6ff2f01ee 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -774,6 +775,30 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
>   	free(cl_separators);
>   }
>   
> +void validate_trailer_args_after_config(const struct strvec *cli_args)
I think "validate_trailer_args" would be a sufficient name
Show 18 quoted lines
> +{
> +	char *cl_separators;
> +
> +	trailer_config_init();
> +
> +	cl_separators = xstrfmt("=%s", separators);
> +
> +	for (size_t i = 0; i < cli_args->nr; i++) {
> +		const char *txt = cli_args->v[i];
> +		ssize_t separator_pos;
> +
> +		if (!*txt)
> +			die(_("empty --trailer argument"));
> +
> +		separator_pos = find_separator(txt, cl_separators);
> +		if (separator_pos == 0)
> +			die(_("invalid trailer '%s': missing key before separator"),
> +		    txt);
Strange indentation here, but the implementation looks sensible.

Overall this is looking good. I've left lots of comments but they're all small issues

Thanks
Phillip
Show 42 quoted lines
> +	}
> +
> +	free(cl_separators);
> +}
> +
>   static const char *next_line(const char *str)
>   {
>   	const char *nl = strchrnul(str, '\n');
> @@ -1226,8 +1251,8 @@ void trailer_iterator_release(struct trailer_iterator *iter)
>   	strbuf_release(&iter->key);
>   }
>   
> -static int amend_strbuf_with_trailers(struct strbuf *buf,
> -				      const struct strvec *trailer_args)
> +int amend_strbuf_with_trailers(struct strbuf *buf,
> +			       const struct strvec *trailer_args)
>   {
>   	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
>   	LIST_HEAD(new_trailer_head);
> diff --git a/trailer.h b/trailer.h
> index daea46ca5d..541657a11f 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -68,6 +68,8 @@ void parse_trailers_from_config(struct list_head *config_head);
>   void parse_trailers_from_command_line_args(struct list_head *arg_head,
>   					   struct list_head *new_trailer_head);
>   
> +void validate_trailer_args_after_config(const struct strvec *cli_args);
> +
>   void process_trailers_lists(struct list_head *head,
>   			    struct list_head *arg_head);
>   
> @@ -195,6 +197,9 @@ int trailer_iterator_advance(struct trailer_iterator *iter);
>    */
>   void trailer_iterator_release(struct trailer_iterator *iter);
>   
> +int amend_strbuf_with_trailers(struct strbuf *buf,
> +			       const struct strvec *trailer_args);
> +
>   /*
>    * Augment a file to add trailers to it (similar to 'git interpret-trailers').
>    * Returns 0 on success or a non-zero error code on failure.
Previous: Li ChenNext: Kristoffer Haugsbakk
Message 18 of 24 in “rebase: support --trailer”
  1. 0/4 rebase: support --trailerLi Chen, Nov 5, 2025
  2. 1/4 interpret-trailers: factor out buffer-based processing to process_trailers()Li Chen, Nov 5, 2025
  3. Junio C HamanoNov 5, 2025
  4. Phillip WoodNov 10, 2025
  5. Li ChenNov 10, 2025
  6. Junio C HamanoNov 10, 2025
  7. Li ChenNov 10, 2025
  8. 2/4 trailer: move process_trailers to trailer.hLi Chen, Nov 5, 2025
  9. Junio C HamanoNov 5, 2025
  10. 3/4 trailer: append trailers in-process and drop the fork to `interpret-trailers`Li Chen, Nov 5, 2025
  11. Junio C HamanoNov 5, 2025
  12. Li ChenNov 10, 2025
  13. Phillip WoodNov 10, 2025
  14. Li ChenNov 10, 2025
  15. Li ChenFeb 24, 2026
  16. Phillip WoodNov 11, 2025
  17. 4/4 rebase: support --trailerLi Chen, Nov 5, 2025
  18. Phillip WoodNov 12, 2025
  19. Kristoffer HaugsbakkNov 24, 2025
  20. Junio C HamanoJan 20, 2026
  21. Junio C HamanoNov 5, 2025
  22. Li ChenNov 10, 2025
  23. Phillip WoodNov 12, 2025
  24. Li ChenNov 17, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.