[RFC PATCH 0/2] rebase: support --trailer and add --reviewby

[RFC PATCH 0/2] rebase: support --trailer and add --reviewby

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

This patch series enhances `git rebase` and `git am` by introducing three features:

 1. `--trailer <line>` support on rebase's merge backend
    Allows arbitrary trailer lines (e.g. multiple “Reviewed-by: …”) to be appended
    to each rebased commit.  The apply backend (`git am`) is explicitly
    rejected when used with `--trailer`, since it has no message‑filter/trailer support.

    This is especially useful when reviewee add reviewer's Reviewed-by, e.g.,
    git rebase \
    --trailer "Reviewed-by: Bob <bob@example.com>" \
    --trailer "Tested-by: Dana <dana@example.com>" \
    base~1


 2. `--reviewby` shortcut flag for rebase
    A convenience alias for adding a single “Reviewed‑by:” trailer using your configured
    committer identity (user.name/user.email), analogous to --signoff. It works on both
    rebase backends (merge and apply) and automatically disables fast‑forwarding to
    rewrite commits.

    This is especially useful when reviewer adds his/her Reviewed-by for a given patchset
    which is already applied, e.g., git rebase --reviewby base~1

3. BTW, `--reviewby` shortcut flag is also added for `am` cmd, because it's needed
   by rebase's apply backend.

I’ve run make test locally without failures. There are Github CI errors around leak checks that I’m still tracking down (see https://github.com/FirstLoveLife/git/actions/runs/14859027869).

Looking forward to your feedback!

Li Chen (2):
  rebase, am: add --reviewby option
  rebase: support --trailer

 Documentation/git-am.adoc  |   6 +-
 builtin/am.c               |  31 +++++++++
 builtin/rebase.c           |  84 ++++++++++++++++++++++
 builtin/revert.c           |   4 +-
 sequencer.c                | 100 ++++++++++++++++++++++++++-
 sequencer.h                |  12 ++++
 t/meson.build              |   2 +
 t/t3439-rebase-reviewby.sh | 138 +++++++++++++++++++++++++++++++++++++
 t/t3440-rebase-trailer.sh  | 108 +++++++++++++++++++++++++++++
 t/t4150-am.sh              |  75 ++++++++++++++++++++
 10 files changed, 556 insertions(+), 4 deletions(-)
 create mode 100755 t/t3439-rebase-reviewby.sh
 create mode 100755 t/t3440-rebase-trailer.sh

--
2.49.0

[RFC PATCH 1/2] rebase, am: add --reviewby option

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

Introduce a new `--reviewby` flag to `git rebase` and `git am` that appends a
`Reviewed-by: name <email>` trailer to each rebased or applied commit, analogous
to the existing `--signoff` behavior.

especially useful for backporting patches and reviewing new patches.

Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
Reviewed-by: Li Chen <chenl311@chinatelecom.cn>
---
 Documentation/git-am.adoc     |   6 +-
 Documentation/git-rebase.adoc |   4 +
 builtin/am.c                  |  31 ++++++++
 builtin/rebase.c              |  14 ++++
 builtin/revert.c              |   4 +-
 sequencer.c                   |  87 ++++++++++++++++++++-
 sequencer.h                   |  10 +++
 t/meson.build                 |   1 +
 t/t3439-rebase-reviewby.sh    | 138 ++++++++++++++++++++++++++++++++++
 t/t4150-am.sh                 |  75 ++++++++++++++++++
 10 files changed, 366 insertions(+), 4 deletions(-)
 create mode 100755 t/t3439-rebase-reviewby.sh

diff --git a/Documentation/git-am.adoc b/Documentation/git-am.adoc
index 221070de48..1d408f2bab 100644
--- a/Documentation/git-am.adoc
+++ b/Documentation/git-am.adoc
@@ -9,7 +9,7 @@ git-am - Apply a series of patches from a mailbox
 SYNOPSIS
 --------
 [verse]
-'git am' [--signoff] [--keep] [--[no-]keep-cr] [--[no-]utf8] [--no-verify]
+'git am' [--signoff] [--reviewby] [--keep] [--[no-]keep-cr] [--[no-]utf8] [--no-verify]
 	 [--[no-]3way] [--interactive] [--committer-date-is-author-date]
 	 [--ignore-date] [--ignore-space-change | --ignore-whitespace]
 	 [--whitespace=<action>] [-C<n>] [-p<n>] [--directory=<dir>]
@@ -40,7 +40,9 @@ OPTIONS
 	Add a `Signed-off-by` trailer to the commit message, using
 	the committer identity of yourself.
 	See the signoff option in linkgit:git-commit[1] for more information.
-
+--reviewby::
+	Add a `Reviewed-by` trailer to the commit message, using
+	the committer identity of yourself.
 -k::
 --keep::
 	Pass `-k` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]).
diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc
index 956d3048f5..1f3d152035 100644
--- a/Documentation/git-rebase.adoc
+++ b/Documentation/git-rebase.adoc
@@ -521,6 +521,10 @@ See also INCOMPATIBLE OPTIONS below.
 	that if `--interactive` is given then only commits marked to be
 	picked, edited or reworded will have the trailer added.
 +
+--review-by::
+	Append a `Reviewed-by:` trailer whose value is taken from the
+	current committer identity, exactly like `--signoff` appends a
+	Signed-off-by:` trailer.
 See also INCOMPATIBLE OPTIONS below.
 
 -i::
diff --git a/builtin/am.c b/builtin/am.c
index 4afb519830..f8fe95e15d 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -89,6 +89,12 @@ enum signoff_type {
 	SIGNOFF_EXPLICIT /* --signoff was set on the command-line */
 };
 
+enum reviewby_type {
+	REVIEWBY_FALSE = 0,
+	REVIEWBY_TRUE = 1,
+	REVIEWBY_EXPLICIT /* --reviewby was set on the command-line */
+};
+
 enum resume_type {
 	RESUME_FALSE = 0,
 	RESUME_APPLY,
@@ -134,6 +140,7 @@ struct am_state {
 	int threeway;
 	int quiet;
 	int signoff; /* enum signoff_type */
+	int reviewby; /* enum reviewby_type */
 	int utf8;
 	int keep; /* enum keep_type */
 	int message_id;
@@ -422,6 +429,9 @@ static void am_load(struct am_state *state)
 	read_state_file(&sb, state, "sign", 1);
 	state->signoff = !strcmp(sb.buf, "t");
 
+	read_state_file(&sb, state, "review", 1);
+	state->reviewby = !strcmp(sb.buf, "t");
+
 	read_state_file(&sb, state, "utf8", 1);
 	state->utf8 = !strcmp(sb.buf, "t");
 
@@ -1017,6 +1027,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 	write_state_bool(state, "threeway", state->threeway);
 	write_state_bool(state, "quiet", state->quiet);
 	write_state_bool(state, "sign", state->signoff);
+	write_state_bool(state, "review", state->reviewby);
 	write_state_bool(state, "utf8", state->utf8);
 
 	if (state->allow_rerere_autoupdate)
@@ -1191,6 +1202,18 @@ static void am_append_signoff(struct am_state *state)
 	state->msg = strbuf_detach(&sb, &state->msg_len);
 }
 
+/**
+ * Appends reviewby to the "msg" field of the am_state.
+ */
+static void am_append_reviewby(struct am_state *state)
+{
+	struct strbuf sb = STRBUF_INIT;
+
+	strbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);
+	append_reviewby(&sb, 0, 0);
+	state->msg = strbuf_detach(&sb, &state->msg_len);
+}
+
 /**
  * Parses `mail` using git-mailinfo, extracting its patch and authorship info.
  * state->msg will be set to the patch message. state->author_name,
@@ -1849,6 +1872,9 @@ static void am_run(struct am_state *state, int resume)
 			if (state->signoff)
 				am_append_signoff(state);
 
+			if (state->reviewby)
+				am_append_reviewby(state);
+
 			write_author_script(state);
 			write_commit_msg(state);
 		}
@@ -2337,6 +2363,9 @@ int cmd_am(int argc,
 		OPT_SET_INT('s', "signoff", &state.signoff,
 			N_("add a Signed-off-by trailer to the commit message"),
 			SIGNOFF_EXPLICIT),
+		OPT_SET_INT(0, "reviewby", &state.reviewby,
+			N_("add a Reviewed-by trailer to the commit message"),
+			REVIEWBY_EXPLICIT),
 		OPT_BOOL('u', "utf8", &state.utf8,
 			N_("recode into utf8 (default)")),
 		OPT_SET_INT('k', "keep", &state.keep,
@@ -2483,6 +2512,8 @@ int cmd_am(int argc,
 
 		if (state.signoff == SIGNOFF_EXPLICIT)
 			am_append_signoff(&state);
+		if (state.reviewby == REVIEWBY_EXPLICIT)
+			am_append_reviewby(&state);
 	} else {
 		struct strvec paths = STRVEC_INIT;
 		int i;
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 2e8c4ee678..b288aedfb1 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -113,6 +113,7 @@ struct rebase_options {
 	enum action action;
 	char *reflog_action;
 	int signoff;
+	int reviewby;
 	int allow_rerere_autoupdate;
 	int keep_empty;
 	int autosquash;
@@ -177,6 +178,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 	sequencer_init_config(&replay);
 
 	replay.signoff = opts->signoff;
+	replay.reviewby = opts->reviewby;
 	replay.allow_ff = !(opts->flags & REBASE_FORCE);
 	if (opts->allow_rerere_autoupdate)
 		replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
@@ -477,6 +479,10 @@ static int read_basic_state(struct rebase_options *opts)
 		opts->signoff = 1;
 		opts->flags |= REBASE_FORCE;
 	}
+	if (file_exists(state_dir_path("reviewby", opts))) {
+		opts->reviewby = 1;
+		opts->flags |= REBASE_FORCE;
+	}
 
 	if (file_exists(state_dir_path("allow_rerere_autoupdate", opts))) {
 		strbuf_reset(&buf);
@@ -528,6 +534,8 @@ static int rebase_write_basic_state(struct rebase_options *opts)
 			   opts->gpg_sign_opt);
 	if (opts->signoff)
 		write_file(state_dir_path("signoff", opts), "--signoff");
+	if (opts->reviewby)
+		write_file(state_dir_path("reviewby", opts), "--reviewby");
 
 	return 0;
 }
@@ -1134,6 +1142,8 @@ int cmd_rebase(int argc,
 		},
 		OPT_BOOL(0, "signoff", &options.signoff,
 			 N_("add a Signed-off-by trailer to each commit")),
+		OPT_BOOL(0, "reviewby", &options.reviewby,
+			 N_("add a Reviewed-by trailer to each commit")),
 		OPT_BOOL(0, "committer-date-is-author-date",
 			 &options.committer_date_is_author_date,
 			 N_("make committer date match author date")),
@@ -1618,6 +1628,10 @@ int cmd_rebase(int argc,
 		strvec_push(&options.git_am_opts, "--signoff");
 		options.flags |= REBASE_FORCE;
 	}
+	if (options.reviewby) {
+		strvec_push(&options.git_am_opts, "--reviewby");
+		options.flags |= REBASE_FORCE;
+	}
 
 	if (!options.root) {
 		if (argc < 1) {
diff --git a/builtin/revert.c b/builtin/revert.c
index e07c2217fe..c73a773c9d 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -124,7 +124,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
 		OPT_CLEANUP(&cleanup_arg),
 		OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
 		OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
-		OPT_NOOP_NOARG('r', NULL),
+		OPT_BOOL('r', "reviewby", &opts->reviewby, N_("add a Reviewed-by trailer")),
 		OPT_BOOL('s', "signoff", &opts->signoff, N_("add a Signed-off-by trailer")),
 		OPT_CALLBACK('m', "mainline", opts, N_("parent-number"),
 			     N_("select mainline parent"), option_parse_m),
@@ -206,6 +206,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
 		verify_opt_compatible(me, this_operation,
 				"--no-commit", opts->no_commit,
 				"--signoff", opts->signoff,
+				"--reviewby", opts->reviewby,
 				"--mainline", opts->mainline,
 				"--strategy", opts->strategy ? 1 : 0,
 				"--strategy-option", opts->xopts.nr ? 1 : 0,
@@ -226,6 +227,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
 	if (opts->allow_ff)
 		verify_opt_compatible(me, "--ff",
 				"--signoff", opts->signoff,
+				"--reviewby", opts->reviewby,
 				"--no-commit", opts->no_commit,
 				"-x", opts->record_origin,
 				"--edit", opts->edit > 0,
diff --git a/sequencer.c b/sequencer.c
index b5c4043757..01933882ed 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -61,6 +61,7 @@
 #define GIT_MAX_LABEL_LENGTH ((NAME_MAX) - (LOCK_SUFFIX_LEN) - 16)
 
 static const char sign_off_header[] = "Signed-off-by: ";
+static const char review_by_header[] = "Reviewed-by: ";
 static const char cherry_picked_prefix[] = "(cherry picked from commit ";
 
 GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
@@ -199,6 +200,7 @@ static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
 static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
 static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
 static GIT_PATH_FUNC(rebase_path_signoff, "rebase-merge/signoff")
+static GIT_PATH_FUNC(rebase_path_reviewby, "rebase-merge/reviewby")
 static GIT_PATH_FUNC(rebase_path_head_name, "rebase-merge/head-name")
 static GIT_PATH_FUNC(rebase_path_onto, "rebase-merge/onto")
 static GIT_PATH_FUNC(rebase_path_autostash, "rebase-merge/autostash")
@@ -1179,6 +1181,10 @@ static int run_git_commit(const char *defmsg,
 		 !opts->signoff && !opts->record_origin &&
 		 !opts->explicit_cleanup)
 		strvec_push(&cmd.args, "--cleanup=verbatim");
+	else if (!(flags & CLEANUP_MSG) &&
+		 !opts->reviewby && !opts->record_origin &&
+		 !opts->explicit_cleanup)
+		strvec_push(&cmd.args, "--cleanup=verbatim");
 
 	if ((flags & ALLOW_EMPTY))
 		strvec_push(&cmd.args, "--allow-empty");
@@ -1628,6 +1634,9 @@ static int try_to_commit(struct repository *r,
 	else if ((opts->signoff || opts->record_origin) &&
 		 !opts->explicit_cleanup)
 		cleanup = COMMIT_MSG_CLEANUP_SPACE;
+	else if ((opts->reviewby || opts->record_origin) &&
+		 !opts->explicit_cleanup)
+		cleanup = COMMIT_MSG_CLEANUP_SPACE;
 	else
 		cleanup = opts->default_msg_cleanup;
 
@@ -2039,6 +2048,8 @@ static int append_squash_message(struct strbuf *buf, const char *body,
 		 */
 		if (opts->signoff)
 			append_signoff(buf, 0, 0);
+		if (opts->reviewby)
+			append_reviewby(buf, 0, 0);
 
 		if ((command == TODO_FIXUP) &&
 		    (flag & TODO_REPLACE_FIXUP_MSG) &&
@@ -2418,6 +2429,9 @@ static int do_pick_commit(struct repository *r,
 	if (opts->signoff && !is_fixup(command))
 		append_signoff(&ctx->message, 0, 0);
 
+	if (opts->reviewby && !is_fixup(command))
+		append_reviewby(&ctx->message, 0, 0);
+
 	if (is_rebase_i(opts) && write_author_script(msg.message) < 0)
 		res = -1;
 	else if (!opts->strategy ||
@@ -3079,6 +3093,8 @@ static int populate_opts_cb(const char *key, const char *value,
 			git_config_bool_or_int(key, value, ctx->kvi, &error_flag);
 	else if (!strcmp(key, "options.signoff"))
 		opts->signoff = git_config_bool_or_int(key, value, ctx->kvi, &error_flag);
+	else if (!strcmp(key, "options.reviewby"))
+		opts->reviewby = git_config_bool_or_int(key, value, ctx->kvi, &error_flag);
 	else if (!strcmp(key, "options.record-origin"))
 		opts->record_origin = git_config_bool_or_int(key, value, ctx->kvi, &error_flag);
 	else if (!strcmp(key, "options.allow-ff"))
@@ -3180,6 +3196,10 @@ static int read_populate_opts(struct replay_opts *opts)
 			opts->allow_ff = 0;
 			opts->signoff = 1;
 		}
+		if (file_exists(rebase_path_reviewby())) {
+			opts->allow_ff = 0;
+			opts->reviewby = 1;
+		}
 
 		if (file_exists(rebase_path_cdate_is_adate())) {
 			opts->allow_ff = 0;
@@ -3286,6 +3306,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
 		write_file(rebase_path_gpg_sign_opt(), "-S%s\n", opts->gpg_sign);
 	if (opts->signoff)
 		write_file(rebase_path_signoff(), "--signoff\n");
+	if (opts->reviewby)
+		write_file(rebase_path_reviewby(), "--reviewby\n");
 	if (opts->drop_redundant_commits)
 		write_file(rebase_path_drop_redundant_commits(), "%s", "");
 	if (opts->keep_redundant_commits)
@@ -3629,6 +3651,9 @@ static int save_opts(struct replay_opts *opts)
 	if (opts->signoff)
 		res |= git_config_set_in_file_gently(opts_file,
 					"options.signoff", NULL, "true");
+	if (opts->reviewby)
+		res |= git_config_set_in_file_gently(opts_file,
+					"options.reviewby", NULL, "true");
 	if (opts->record_origin)
 		res |= git_config_set_in_file_gently(opts_file,
 					"options.record-origin", NULL, "true");
@@ -4949,7 +4974,7 @@ static int pick_commits(struct repository *r,
 
 	ctx->reflog_message = sequencer_reflog_action(opts);
 	if (opts->allow_ff)
-		ASSERT(!(opts->signoff || opts->no_commit ||
+		ASSERT(!(opts->signoff || opts->reviewby || opts->no_commit ||
 			 opts->record_origin || should_edit(opts) ||
 			 opts->committer_date_is_author_date ||
 			 opts->ignore_date));
@@ -5581,6 +5606,66 @@ int sequencer_pick_revisions(struct repository *r,
 	return res;
 }
 
+void append_reviewby(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)
+{
+	unsigned no_dup_rb = flag & APPEND_REVIEWBY_DEDUP;
+	struct strbuf rb = STRBUF_INIT;
+	int has_footer;
+
+	strbuf_addstr(&rb, review_by_header);
+	strbuf_addstr(&rb, fmt_name(WANT_COMMITTER_IDENT));
+	strbuf_addch(&rb, '\n');
+
+	if (!ignore_footer)
+		strbuf_complete_line(msgbuf);
+
+	/*
+	 * If the whole message buffer is equal to the rb, pretend that we
+	 * found a conforming footer with a matching rb
+	 */
+	if (msgbuf->len - ignore_footer == rb.len &&
+	    !strncmp(msgbuf->buf, rb.buf, rb.len))
+		has_footer = 3;
+	else
+		has_footer = has_conforming_footer(msgbuf, &rb, ignore_footer);
+
+	if (!has_footer) {
+		const char *append_newlines = NULL;
+		size_t len = msgbuf->len - ignore_footer;
+
+		if (!len) {
+			/*
+			 * The buffer is completely empty.  Leave foom for
+			 * the title and body to be filled in by the user.
+			 */
+			append_newlines = "\n\n";
+		} else if (len == 1) {
+			/*
+			 * Buffer contains a single newline.  Add another
+			 * so that we leave room for the title and body.
+			 */
+			append_newlines = "\n";
+		} else if (msgbuf->buf[len - 2] != '\n') {
+			/*
+			 * Buffer ends with a single newline.  Add another
+			 * so that there is an empty line between the message
+			 * body and the rb.
+			 */
+			append_newlines = "\n";
+		} /* else, the buffer already ends with two newlines. */
+
+		if (append_newlines)
+			strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
+				append_newlines, strlen(append_newlines));
+	}
+
+	if (has_footer != 3 && (!no_dup_rb || has_footer != 2))
+		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
+				rb.buf, rb.len);
+
+	strbuf_release(&rb);
+}
+
 void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)
 {
 	unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
diff --git a/sequencer.h b/sequencer.h
index 304ba4b4d3..82b79fd1e8 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -17,6 +17,7 @@ const char *rebase_path_dropped(void);
 extern const char *rebase_resolvemsg;
 
 #define APPEND_SIGNOFF_DEDUP (1u << 0)
+#define APPEND_REVIEWBY_DEDUP (1u << 1)
 
 enum replay_action {
 	REPLAY_REVERT,
@@ -44,6 +45,7 @@ struct replay_opts {
 	int record_origin;
 	int no_commit;
 	int signoff;
+	int reviewby;
 	int allow_ff;
 	int allow_rerere_auto;
 	int allow_empty;
@@ -205,6 +207,14 @@ int todo_list_rearrange_squash(struct todo_list *todo_list);
  */
 void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag);
 
+/*
+ * Append a reviewby to the commit message in "msgbuf". The ignore_footer
+ * parameter specifies the number of bytes at the end of msgbuf that should
+ * not be considered at all. I.e., they are not checked for existing trailers,
+ * and the new reviewby will be spliced into the buffer before those bytes.
+ */
+void append_reviewby(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag);
+
 void append_conflicts_hint(struct index_state *istate,
 		struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode);
 enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
diff --git a/t/meson.build b/t/meson.build
index bfb744e886..327fa461fd 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -392,6 +392,7 @@ integration_tests = [
   't3436-rebase-more-options.sh',
   't3437-rebase-fixup-options.sh',
   't3438-rebase-broken-files.sh',
+  't3439-rebase-reviewby.sh',
   't3500-cherry.sh',
   't3501-revert-cherry-pick.sh',
   't3502-cherry-pick-merge.sh',
diff --git a/t/t3439-rebase-reviewby.sh b/t/t3439-rebase-reviewby.sh
new file mode 100755
index 0000000000..ca7299fbdb
--- /dev/null
+++ b/t/t3439-rebase-reviewby.sh
@@ -0,0 +1,138 @@
+#!/bin/sh
+
+test_description='git rebase --reviewby
+
+This test runs git rebase --reviewby and make sure that it works.
+'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+test_expect_success 'setup' '
+	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>" &&
+
+	# Expected commit message for initial commit after rebase --reviewby
+	cat >expected-initial-reviewed <<-EOF &&
+	Initial empty commit
+
+	Reviewed-by: $ident
+	EOF
+
+	# Expected commit message after rebase --reviewby
+	cat >expected-reviewed <<-EOF &&
+	first
+
+	Reviewed-by: $ident
+	EOF
+
+	# Expected commit message after conflict resolution for rebase --reviewby
+	cat >expected-reviewed-conflict <<-EOF &&
+	third
+
+	Reviewed-by: $ident
+
+	conflict
+
+	Reviewed-by: $ident
+
+	file-2
+
+	Reviewed-by: $ident
+
+	EOF
+
+	# Expected commit message after rebase without --reviewby (or with --no-reviewby)
+	cat >expected-unreviewed <<-EOF &&
+	first
+	EOF
+
+	git config alias.rbr "rebase --reviewby"
+'
+
+test_expect_success 'rebase --apply --reviewby adds a Reviewed-by line' '
+	test_must_fail git rbr --apply second third &&
+	git checkout --theirs file &&
+	git add file &&
+	git rebase --continue &&
+	git log --format=%B -n3 >actual &&
+	test_cmp expected-reviewed-conflict actual
+'
+
+test_expect_success 'rebase --no-reviewby does not add a Reviewed-by line' '
+	git commit --amend -m "first" &&
+	git rbr --no-reviewby HEAD^ &&
+	test_commit_message HEAD expected-unreviewed
+'
+
+test_expect_success 'rebase --exec --reviewby adds a Reviewed-by line' '
+	test_when_finished "rm exec" &&
+	git rebase --exec "touch exec" --reviewby first^ first &&
+	test_path_is_file exec &&
+	test_commit_message HEAD expected-reviewed
+'
+
+test_expect_success 'rebase --root --reviewby adds a Reviewed-by line' '
+	git checkout first &&
+	git rebase --root --keep-empty --reviewby &&
+	test_commit_message HEAD^ expected-initial-reviewed &&
+	test_commit_message HEAD expected-reviewed
+'
+
+test_expect_success 'rebase -m --reviewby adds a Reviewed-by line' '
+	test_must_fail git rebase -m --reviewby second third &&
+	git checkout --theirs file &&
+	git add file &&
+	GIT_EDITOR="sed -n /Conflicts:/,/^\\\$/p >actual" \
+		git rebase --continue &&
+	cat >expect <<-\EOF &&
+	# Conflicts:
+	#	file
+
+	EOF
+	test_cmp expect actual &&
+	git log --format=%B -n3 >actual &&
+	test_cmp expected-reviewed-conflict actual
+'
+
+test_expect_success 'rebase -i --reviewby adds a Reviewed-by line when editing commit' '
+	(
+		set_fake_editor &&
+		FAKE_LINES="edit 1 edit 3 edit 2" \
+			git rebase -i --reviewby first third
+	) &&
+	echo a >a &&
+	git add a &&
+	test_must_fail git rebase --continue &&
+	git checkout --ours file &&
+	echo b >a &&
+	git add a file &&
+	git rebase --continue &&
+	echo c >a &&
+	git add a &&
+	git log --format=%B -n3 >actual &&
+	cat >expect <<-EOF &&
+	conflict
+
+	Reviewed-by: $ident
+
+	third
+
+	Reviewed-by: $ident
+
+	file-2
+
+	Reviewed-by: $ident
+
+	EOF
+	test_cmp expect actual
+'
+
+test_done
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 2ae93d3c96..40d6a2f43a 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -491,6 +491,7 @@ test_expect_success 'am changes committer and keeps author' '
 	     "$(git log -1 --pretty=format:"%cn <%ce>" HEAD)"
 '
 
+########################## signoff begin ##########################
 test_expect_success 'am --signoff adds Signed-off-by: line' '
 	rm -fr .git/rebase-apply &&
 	git reset --hard &&
@@ -572,6 +573,80 @@ test_expect_success 'am without --keep removes Re: and [PATCH] stuff' '
 	git rev-parse topic_2 >actual &&
 	test_cmp expected actual
 '
+########################## signoff end ##########################
+
+########################## review begin ##########################
+test_expect_success 'am --review adds Reviewed-by: line' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
+	git checkout -b topic_3 first &&
+	git am --reviewby <patch2 &&
+	{
+		printf "third\n\nReviewed-by: %s <%s>\n\n" \
+			"$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" &&
+		cat msg &&
+		printf "Reviewed-by: %s <%s>\n\n" \
+			"$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL"
+	} >expected-log &&
+	git log --pretty=%B -2 HEAD >actual &&
+	test_cmp expected-log actual
+'
+test_expect_success 'am stays in branch' '
+	echo refs/heads/topic_3 >expected &&
+	git symbolic-ref HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'am --reviewby does not add Reviewed-by: line if already there' '
+	git format-patch --stdout first >patch3 &&
+	git reset --hard first &&
+	git am --reviewby <patch3 &&
+	git log --pretty=%B -2 HEAD >actual &&
+	test_cmp expected-log actual
+'
+
+test_expect_success 'am --reviewby adds Reviewed-by: if another author is preset' '
+	NAME="A N Other" &&
+	EMAIL="a.n.other@example.com" &&
+	{
+		printf "third\n\nReviewed-by: %s <%s>\nReviewed-by: %s <%s>\n\n" \
+			"$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" \
+			"$NAME" "$EMAIL" &&
+		cat msg &&
+		printf "Reviewed-by: %s <%s>\nReviewed-by: %s <%s>\n\n" \
+			"$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" \
+			"$NAME" "$EMAIL"
+	} >expected-log &&
+	git reset --hard first &&
+	GIT_COMMITTER_NAME="$NAME" GIT_COMMITTER_EMAIL="$EMAIL" \
+		git am --reviewby <patch3 &&
+	git log --pretty=%B -2 HEAD >actual &&
+	test_cmp expected-log actual
+'
+
+test_expect_success 'am --reviewby duplicates Reviewed-by: if it is not the last one' '
+	NAME="A N Other" &&
+	EMAIL="a.n.other@example.com" &&
+	{
+		printf "third\n\nReviewed-by: %s <%s>\n\
+Reviewed-by: %s <%s>\nReviewed-by: %s <%s>\n\n" \
+			"$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" \
+			"$NAME" "$EMAIL" \
+			"$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" &&
+		cat msg &&
+		printf "Reviewed-by: %s <%s>\nReviewed-by: %s <%s>\n\
+Reviewed-by: %s <%s>\n\n" \
+			"$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" \
+			"$NAME" "$EMAIL" \
+			"$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL"
+	} >expected-log &&
+	git format-patch --stdout first >patch3 &&
+	git reset --hard first &&
+	git am --reviewby <patch3 &&
+	git log --pretty=%B -2 HEAD >actual &&
+	test_cmp expected-log actual
+'
+########################## review end ##########################
 
 test_expect_success 'am --keep really keeps the subject' '
 	rm -fr .git/rebase-apply &&
-- 
2.49.0

[RFC PATCH 2/2] rebase: support --trailer

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

Implement a new `--trailer <text>` option for `git rebase`
(support merge backend only now), which appends arbitrary
trailer lines to each rebased commit message. Reject early
if used with the apply backend (git am) since it lacks
message‑filter/trailer hook. Automatically set REBASE_FORCE when
any trailer is supplied.

Reviewed-by: Li Chen <chenl311@chinatelecom.cn>
---
 Documentation/git-rebase.adoc |   8 +++
 builtin/rebase.c              |  70 ++++++++++++++++++++++
 sequencer.c                   |  13 ++++
 sequencer.h                   |   2 +
 t/meson.build                 |   1 +
 t/t3440-rebase-trailer.sh     | 108 ++++++++++++++++++++++++++++++++++
 6 files changed, 202 insertions(+)
 create mode 100755 t/t3440-rebase-trailer.sh

diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc
index 1f3d152035..757016e529 100644
--- a/Documentation/git-rebase.adoc
+++ b/Documentation/git-rebase.adoc
@@ -525,6 +525,14 @@ See also INCOMPATIBLE OPTIONS below.
 	Append a `Reviewed-by:` trailer whose value is taken from the
 	current committer identity, exactly like `--signoff` appends a
 	Signed-off-by:` trailer.
+
+--trailer <trailer>::
+       Append the given trailer line(s) to every rebased commit
+       message, processed via linkgit:git-interpret-trailers[1].
+       When this option is present *rebase automatically enables*
+       `--force-rebase` so that fast‑forwarded commits are also
+       rewritten.
+
 See also INCOMPATIBLE OPTIONS below.
 
 -i::
diff --git a/builtin/rebase.c b/builtin/rebase.c
index b288aedfb1..df65a1e040 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -36,6 +36,9 @@
 #include "reset.h"
 #include "trace2.h"
 #include "hook.h"
+#include "trailer.h"
+
+static const char trailer_state_name[] = "trailer";
 
 static char const * const builtin_rebase_usage[] = {
 	N_("git rebase [-i] [options] [--exec <cmd>] "
@@ -46,6 +49,8 @@ static char const * const builtin_rebase_usage[] = {
 	NULL
 };
 
+static struct strvec trailer_args = STRVEC_INIT;
+
 static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto")
 static GIT_PATH_FUNC(path_interactive, "rebase-merge/interactive")
 static GIT_PATH_FUNC(apply_dir, "rebase-apply")
@@ -114,6 +119,7 @@ struct rebase_options {
 	char *reflog_action;
 	int signoff;
 	int reviewby;
+	struct strvec trailer_args;
 	int allow_rerere_autoupdate;
 	int keep_empty;
 	int autosquash;
@@ -144,6 +150,7 @@ struct rebase_options {
 		.flags = REBASE_NO_QUIET, 		\
 		.git_am_opts = STRVEC_INIT,		\
 		.exec = STRING_LIST_INIT_NODUP,		\
+		.trailer_args = STRVEC_INIT,  \
 		.git_format_patch_opt = STRBUF_INIT,	\
 		.fork_point = -1,			\
 		.reapply_cherry_picks = -1,             \
@@ -167,6 +174,7 @@ static void rebase_options_release(struct rebase_options *opts)
 	free(opts->strategy);
 	string_list_clear(&opts->strategy_opts, 0);
 	strbuf_release(&opts->git_format_patch_opt);
+	strvec_clear(&opts->trailer_args);
 }
 
 static struct replay_opts get_replay_opts(const struct rebase_options *opts)
@@ -179,6 +187,10 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 
 	replay.signoff = opts->signoff;
 	replay.reviewby = opts->reviewby;
+
+	for (size_t i = 0; i < opts->trailer_args.nr; i++)
+        strvec_push(&replay.trailer_args, opts->trailer_args.v[i]);
+
 	replay.allow_ff = !(opts->flags & REBASE_FORCE);
 	if (opts->allow_rerere_autoupdate)
 		replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
@@ -437,6 +449,8 @@ static int read_basic_state(struct rebase_options *opts)
 	struct strbuf head_name = STRBUF_INIT;
 	struct strbuf buf = STRBUF_INIT;
 	struct object_id oid;
+	struct strbuf t = STRBUF_INIT, one = STRBUF_INIT;
+	const char *path = state_dir_path(trailer_state_name, opts);
 
 	if (!read_oneliner(&head_name, state_dir_path("head-name", opts),
 			   READ_ONELINER_WARN_MISSING) ||
@@ -509,6 +523,22 @@ static int read_basic_state(struct rebase_options *opts)
 
 	strbuf_release(&buf);
 
+	if (strbuf_read_file(&t, path, 0) >= 0) {
+		const char *p = t.buf, *end = t.buf + t.len;
+
+		while (p < end) {
+			const char *nl = memchr(p, '\n', end - p);
+			strbuf_reset(&one);
+			strbuf_add(&one, p, nl ? nl - p : end - p);
+			if (one.len) /* skip empty line */
+				strvec_push(&opts->trailer_args,
+					    strbuf_detach(&one, NULL));
+			p = nl ? nl + 1 : end;
+		}
+		strbuf_release(&one);
+	}
+	strbuf_release(&t);
+
 	return 0;
 }
 
@@ -537,6 +567,28 @@ static int rebase_write_basic_state(struct rebase_options *opts)
 	if (opts->reviewby)
 		write_file(state_dir_path("reviewby", opts), "--reviewby");
 
+    /*
+     * save opts->trailer_args into state_dir/trailer
+     */
+    if (opts->trailer_args.nr) {
+            struct strbuf buf = STRBUF_INIT;
+            size_t i;
+
+            for (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_state_name, opts),
+                       "%s", buf.buf);
+            strbuf_release(&buf);
+    } else {
+            /*
+             * but if rebase doesn't pass any --trailer，
+             * and state dir still have residual files，let's delete it。
+             */
+            unlink_or_warn(state_dir_path(trailer_state_name, opts));
+    }
+
 	return 0;
 }
 
@@ -1140,6 +1192,7 @@ int cmd_rebase(int argc,
 			.flags = PARSE_OPT_NOARG,
 			.defval = REBASE_DIFFSTAT,
 		},
+		OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG),
 		OPT_BOOL(0, "signoff", &options.signoff,
 			 N_("add a Signed-off-by trailer to each commit")),
 		OPT_BOOL(0, "reviewby", &options.reviewby,
@@ -1292,6 +1345,13 @@ int cmd_rebase(int argc,
 			     builtin_rebase_options,
 			     builtin_rebase_usage, 0);
 
+    for (i = 0; i < trailer_args.nr; i++)
+ 	   strvec_push(&options.trailer_args, trailer_args.v[i]);
+
+    /* if add --trailer，force rebase */
+    if (options.trailer_args.nr)
+		   options.flags |= REBASE_FORCE;
+
 	if (preserve_merges_selected)
 		die(_("--preserve-merges was replaced by --rebase-merges\n"
 			"Note: Your `pull.rebase` configuration may also be set to 'preserve',\n"
@@ -1549,6 +1609,16 @@ int cmd_rebase(int argc,
 	if (options.root && !options.onto_name)
 		imply_merge(&options, "--root without --onto");
 
+	/*
+	 * The apply‑based backend (git am) cannot append trailers because
+	 * it lacks a message‑filter facility.  Reject early, before any
+	 * state (index, HEAD, etc.) is modified.
+	 */
+	if (options.type == REBASE_APPLY && options.trailer_args.nr)
+		die(_("the --apply backend (git am) cannot currently handle "
+		      "--trailer; please omit --apply or use "
+		      "the merge/interactive backend"));
+
 	if (isatty(2) && options.flags & REBASE_NO_QUIET)
 		strbuf_addstr(&options.git_format_patch_opt, " --progress");
 
diff --git a/sequencer.c b/sequencer.c
index 01933882ed..b61c668c39 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -429,6 +429,7 @@ void replay_opts_release(struct replay_opts *opts)
 	free(opts->revs);
 	replay_ctx_release(ctx);
 	free(opts->ctx);
+	strvec_clear(&opts->trailer_args);
 }
 
 int sequencer_remove_state(struct replay_opts *opts)
@@ -2506,6 +2507,18 @@ 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 */
+
+    if (!res && opts->trailer_args.nr && !drop_commit) {
+            const char *trailer_file =
+                    msg_file ? msg_file : git_path_merge_msg(r);
+
+            if (amend_file_with_trailers(trailer_file,
+                                         &opts->trailer_args)) {
+                    res = error(_("unable to add trailers to commit message"));
+                    goto leave;
+            }
+    }
+
 	if (!opts->no_commit && !drop_commit) {
 		if (author || command == TODO_REVERT || (flags & AMEND_MSG))
 			res = do_commit(r, msg_file, author, opts, flags,
diff --git a/sequencer.h b/sequencer.h
index 82b79fd1e8..4f5ea2d818 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -46,6 +46,7 @@ struct replay_opts {
 	int no_commit;
 	int signoff;
 	int reviewby;
+	struct strvec trailer_args;
 	int allow_ff;
 	int allow_rerere_auto;
 	int allow_empty;
@@ -88,6 +89,7 @@ struct replay_opts {
 	.action = -1,				\
 	.xopts = STRVEC_INIT,			\
 	.ctx = replay_ctx_new(),		\
+	.trailer_args = STRVEC_INIT, \
 }
 
 /*
diff --git a/t/meson.build b/t/meson.build
index 327fa461fd..fc4b64fca1 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -393,6 +393,7 @@ integration_tests = [
   't3437-rebase-fixup-options.sh',
   't3438-rebase-broken-files.sh',
   't3439-rebase-reviewby.sh',
+  't3440-rebase-trailer.sh',
   '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..6dc08e9633
--- /dev/null
+++ b/t/t3440-rebase-trailer.sh
@@ -0,0 +1,108 @@
+#!/bin/sh
+#
+
+test_description='git rebase --trailer integration tests
+We verify that --trailer on the merge/interactive/exec/root backends,
+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
+
+############################################################################
+# 1.  repository setup
+############################################################################
+
+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 () {
+	cat >"$1" <<-EOF
+	$2
+
+	Reviewed-by: Dev <dev@example.com>
+	EOF
+}
+# golden files:
+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"
+
+############################################################################
+# 2.  explicitly reject --apply + --trailer
+############################################################################
+
+test_expect_success 'apply backend is rejected when --trailer is given' '
+	git reset --hard third &&
+	git tag before-apply &&
+	test_expect_code 128 \
+		git rebase --apply --trailer "Reviewed-by: Dev <dev@example.com>" \
+			HEAD^ &&
+	git diff --quiet before-apply..HEAD      # prove nothing moved
+'
+
+############################################################################
+# 3.  --no‑op: same range, no changes
+############################################################################
+
+test_expect_success '--trailer without range change is a no‑op' '
+	git checkout main &&
+	git tag before-noop &&
+	git rebase --trailer "Reviewed-by: Dev <dev@example.com>" HEAD &&
+	git diff --quiet before-noop..HEAD
+'
+
+############################################################################
+# 4.  merge backend (-m), conflict resolution path
+############################################################################
+
+test_expect_success 'rebase -m --trailer adds trailer after conflicts' '
+	git reset --hard third &&
+	test_must_fail git rebase -m \
+		--trailer "Reviewed-by: Dev <dev@example.com>" \
+		second third &&
+	git checkout --theirs file &&
+	git add file &&
+    GIT_EDITOR=: git rebase --continue &&
+	git show --no-patch --pretty=format:%B HEAD~2 >actual &&
+	test_cmp file2-signed actual
+'
+
+############################################################################
+# 5.  --exec path
+############################################################################
+
+test_expect_success 'rebase --exec --trailer adds trailer' '
+	test_when_finished "rm -f touched" &&
+	git rebase --exec "touch touched" \
+		--trailer "Reviewed-by: Dev <dev@example.com>" \
+		first^ first &&
+	test_path_is_file touched &&
+	test_commit_message HEAD first-signed
+'
+
+############################################################################
+# 6.  --root path
+############################################################################
+
+test_expect_success 'rebase --root --trailer updates every commit' '
+	git checkout first &&
+	git rebase --root --keep-empty \
+		--trailer "Reviewed-by: Dev <dev@example.com>" &&
+	test_commit_message HEAD   first-signed &&
+	test_commit_message HEAD^  initial-signed
+'
+test_done
-- 
2.49.0

Re: [RFC PATCH 1/2] rebase, am: add --reviewby option

[Junio C Hamano]

Li Chen <me@linux.beauty> writes:

> From: Li Chen <chenl311@chinatelecom.cn>
>
> Introduce a new `--reviewby` flag to `git rebase` and `git am` that appends a

Shouldn't this (and possibly the other one, I didn't look at the
patch text) be using the internal machinery used by interpret-trailers
so that we can do the usual "do not duplicated existing ones",
"append only at the last one is different" etc.?

I also do not think we want to see one option (like the above) for
each trailer elements (like "Reviewed-by") that is commonly used,
which would lead us to adding "--helped-by", "--acked-by", etc. to
complement this one.

Re: [RFC PATCH 1/2] rebase, am: add --reviewby option

[Li Chen]

Hi Junio,

Thank you for the feedback. My current understanding is as below, please correct me if I am mistaken

---- On Wed, 07 May 2025 02:07:46 +0800  Junio C Hamano <gitster@pobox.com> wrote --- 
 > Li Chen <me@linux.beauty> writes:
 > 
 > > From: Li Chen <chenl311@chinatelecom.cn>
 > >
 > > Introduce a new `--reviewby` flag to `git rebase` and `git am` that appends a
 > 
 > Shouldn't this (and possibly the other one, I didn't look at the
 > patch text) be using the internal machinery used by interpret-trailers
 > so that we can do the usual "do not duplicated existing ones",
 > "append only at the last one is different" etc.?
 
At the moment, git-interpret‑trailers only works on a file or on
stdin. During an interactive/merge rebase we change it on the fly
without format-patch && am, which  is very convenient.

The new --trailer option already re‑uses amend_file_with_trailers()
from trailer.c, so there is very little trailer‑specific code of my own.

Right now --reviewby mirrors the existing --signoff implementation,
and append_signoff() itself does not use the trailer library. If you are
open to keeping a dedicated --reviewby, I can send  a follow‑up
(or roll it into this series) that moves both sign‑off and review‑by to
the common trailer helpers.

 > I also do not think we want to see one option (like the above) for
 > each trailer elements (like "Reviewed-by") that is commonly used,
 > which would lead us to adding "--helped-by", "--acked-by", etc. to
 > complement this one.
 > 

Some projects require every commit to carry a Reviewed-by: line
for accountability, much like the kernel requires Signed-off-by:.
A first‑class option keeps that workflow “out of the box”; otherwise
people need to define an alias such as

[alias]
    rbr = rebase --trailer "Reviewed-by: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>"

which is functional but less convenient.

I would appreciate your further thoughts on whether a dedicated
flag(--reviewby) is acceptable, or whether we should drop it and rely solely on
the generic --trailer interface.

Thanks again for the review.

Regards,
Li

Re: [RFC PATCH 1/2] rebase, am: add --reviewby option

[Phillip Wood]

Hi Li

On 07/05/2025 07:46, Li Chen wrote:
> 
> Some projects require every commit to carry a Reviewed-by: line
> for accountability, much like the kernel requires Signed-off-by:.
> A first‑class option keeps that workflow “out of the box”; otherwise
> people need to define an alias such as
> 
> [alias]
>      rbr = rebase --trailer "Reviewed-by: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>"
> 
> which is functional but less convenient.
> 
> I would appreciate your further thoughts on whether a dedicated
> flag(--reviewby) is acceptable, or whether we should drop it and rely solely on
> the generic --trailer interface.

I think adding support for --trailer is a good idea and if we do that we 
don't need --reviewby. The existence and implementation of --signoff is 
largely a historical artifact - I'm not sure we'd make the same choices 
if we were thinking about adding it today. Different projects have 
different requirements and I don't think it is sensible to add a new 
option catering to the different demands of each project.

I'll take a proper look at the second patch tomorrow.

Best Wishes

Phillip

> Thanks again for the review.
> 
> Regards,
> Li
> 

Re: [RFC PATCH 1/2] rebase, am: add --reviewby option

[Phillip Wood]

On 07/05/2025 11:17, Phillip Wood wrote:
> Hi Li
> 
> On 07/05/2025 07:46, Li Chen wrote:
>>
>> Some projects require every commit to carry a Reviewed-by: line
>> for accountability, much like the kernel requires Signed-off-by:.
>> A first‑class option keeps that workflow “out of the box”; otherwise
>> people need to define an alias such as
>>
>> [alias]
>>      rbr = rebase --trailer "Reviewed-by: $GIT_AUTHOR_NAME 
>> <$GIT_AUTHOR_EMAIL>"
>>
>> which is functional but less convenient.
>>
>> I would appreciate your further thoughts on whether a dedicated
>> flag(--reviewby) is acceptable, or whether we should drop it and rely 
>> solely on
>> the generic --trailer interface.
> 
> I think adding support for --trailer is a good idea and if we do that we 
> don't need --reviewby. The existence and implementation of --signoff is 
> largely a historical artifact - I'm not sure we'd make the same choices 
> if we were thinking about adding it today. Different projects have 
> different requirements and I don't think it is sensible to add a new 
> option catering to the different demands of each project.

It might be worth thinking about how we could extend the trailer option 
so that it uses the committer identity if there is no value specified 
which would reduce the pain of adding things like Reviewed-by:

Best Wishes

Phillip

> I'll take a proper look at the second patch tomorrow.
> 
> Best Wishes
> 
> Phillip
> 
>> Thanks again for the review.
>>
>> Regards,
>> Li
>>
> 

Re: [RFC PATCH 1/2] rebase, am: add --reviewby option

[Kristoffer Haugsbakk]

On Wed, May 7, 2025, at 12:26, Phillip Wood wrote:
> On 07/05/2025 11:17, Phillip Wood wrote:
>> Hi Li
>>
>> On 07/05/2025 07:46, Li Chen wrote:
>>>
>>> Some projects require every commit to carry a Reviewed-by: line
>>> for accountability, much like the kernel requires Signed-off-by:.
>>> A first‑class option keeps that workflow “out of the box”; otherwise
>>> people need to define an alias such as
>>>
>>> [alias]
>>>      rbr = rebase --trailer "Reviewed-by: $GIT_AUTHOR_NAME
>>> <$GIT_AUTHOR_EMAIL>"
>>>
>>> which is functional but less convenient.
>>>
>>> I would appreciate your further thoughts on whether a dedicated
>>> flag(--reviewby) is acceptable, or whether we should drop it and rely
>>> solely on
>>> the generic --trailer interface.
>>
>> I think adding support for --trailer is a good idea and if we do that we
>> don't need --reviewby. The existence and implementation of --signoff is
>> largely a historical artifact - I'm not sure we'd make the same choices
>> if we were thinking about adding it today. Different projects have
>> different requirements and I don't think it is sensible to add a new
>> option catering to the different demands of each project.
>
> It might be worth thinking about how we could extend the trailer option
> so that it uses the committer identity if there is no value specified
> which would reduce the pain of adding things like Reviewed-by:

That could be confusing for people who use trailers for
non-ident metadata.

I was wondering if `git var GIT_COMMITTER_IDENT` could be used.  But
that prints a Unix timestamp with timezone as well. (I don’t really
understand why after reading that part of the manual)

-- 
Kristoffer Haugsbakk

Re: [RFC PATCH 1/2] rebase, am: add --reviewby option

[Phillip Wood]

On 07/05/2025 11:39, Kristoffer Haugsbakk wrote:
> On Wed, May 7, 2025, at 12:26, Phillip Wood wrote:
> 
>> It might be worth thinking about how we could extend the trailer option
>> so that it uses the committer identity if there is no value specified
>> which would reduce the pain of adding things like Reviewed-by:
> 
> That could be confusing for people who use trailers for
> non-ident metadata.

Yes we need to think about how to do it in a way that is helpful for 
things like Reviewed-by: or Tested-by: but does not break other 
workflows. One possibility would be to have a 
trailer.<name>.defaultValue config key but I'd be interested in other 
people's ideas.

One other idea is that "git commit --author Kristoffer" will search 
recent commits for matching author to obtain your full name and email 
address. Something like that could be useful for expanding values 
--tailer=Co-Authored-by" or perhaps we could teach the completion script 
to suggest suitable completions based on some config describing the 
values we expect for certain keys.

> I was wondering if `git var GIT_COMMITTER_IDENT` could be used.  But
> that prints a Unix timestamp with timezone as well. (I don’t really
> understand why after reading that part of the manual)
> 
I think it is because with define GIT_*_IDENT to include the timestamp 
to match the ident lines in commit and tag objects. You can always trim 
the timestamp appending something like "| sed 's/[^>]*$//'".

Best Wishes

Phillip

Re: [RFC PATCH 1/2] rebase, am: add --reviewby option

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> .... The existence and implementation of
> --signoff is largely a historical artifact - I'm not sure we'd make
> the same choices if we were thinking about adding it today.

Very well said.

> I'll take a proper look at the second patch tomorrow.

Thanks.

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

[Phillip Wood]

Hi Li

On 06/05/2025 13:58, Li Chen wrote:
> From: Li Chen <chenl311@chinatelecom.cn>
> 
> Implement a new `--trailer <text>` option for `git rebase`
> (support merge backend only now), which appends arbitrary
> trailer lines to each rebased commit message. Reject early
> if used with the apply backend (git am) since it lacks
> message‑filter/trailer hook. Automatically set REBASE_FORCE when
> any trailer is supplied.

I think this is a reasonable idea but unfortunately I think the trailer 
API needs improving so that the implementation

(a) Checks the trailers given on the command-line before the user edits 
the todo list. That way we reject invalid trailers and exit before the 
user has spent any effort editing the todo list.

(b) Does not fork another process to add the trailers. Without this the 
performance is going to suffer. Hopefully it wont be too difficult to 
modify the existing code to take a struct strbuf and a list of trailers 
to append to it.

(c) Only adds the trailers on the commandline. I'm a bit confused by the 
various trailer config options - the man page reads to me like "git 
interpret-trailers" can add missing trailers that are configured but not 
passed on the commandline.

The changes to the trailer api should be made in one or more preparatory 
commits before adding support for --trailer to "git rebase"

I've left some comments on the changes to builtin/rebase.c and the 
tests, I've skipped the changes to sequencer.c for now as they'll have 
to be updated to avoid forking "git interpret-trailers"

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index b288aedfb1..df65a1e040 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -36,6 +36,9 @@
>   #include "reset.h"
>   #include "trace2.h"
>   #include "hook.h"
> +#include "trailer.h"
> +
> +static const char trailer_state_name[] = "trailer";
>   
>   static char const * const builtin_rebase_usage[] = {
>   	N_("git rebase [-i] [options] [--exec <cmd>] "
> @@ -46,6 +49,8 @@ static char const * const builtin_rebase_usage[] = {
>   	NULL
>   };
>   
> +static struct strvec trailer_args = STRVEC_INIT;

We should not need to add any file scope variables

>   static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto")
>   static GIT_PATH_FUNC(path_interactive, "rebase-merge/interactive")
>   static GIT_PATH_FUNC(apply_dir, "rebase-apply")
> @@ -114,6 +119,7 @@ struct rebase_options {
>   	char *reflog_action;
>   	int signoff;
>   	int reviewby;
> +	struct strvec trailer_args;

This is good and means we don't need the file-scope declaration above.

>   	int allow_rerere_autoupdate;
>   	int keep_empty;
>   	int autosquash;
> @@ -144,6 +150,7 @@ struct rebase_options {
>   		.flags = REBASE_NO_QUIET, 		\
>   		.git_am_opts = STRVEC_INIT,		\
>   		.exec = STRING_LIST_INIT_NODUP,		\
> +		.trailer_args = STRVEC_INIT,  \
>   		.git_format_patch_opt = STRBUF_INIT,	\
>   		.fork_point = -1,			\
>   		.reapply_cherry_picks = -1,             \
> @@ -167,6 +174,7 @@ static void rebase_options_release(struct rebase_options *opts)
>   	free(opts->strategy);
>   	string_list_clear(&opts->strategy_opts, 0);
>   	strbuf_release(&opts->git_format_patch_opt);
> +	strvec_clear(&opts->trailer_args);
>   }
>   
>   static struct replay_opts get_replay_opts(const struct rebase_options *opts)
> @@ -179,6 +187,10 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>   
>   	replay.signoff = opts->signoff;
>   	replay.reviewby = opts->reviewby;
> +
> +	for (size_t i = 0; i < opts->trailer_args.nr; i++)
> +        strvec_push(&replay.trailer_args, opts->trailer_args.v[i]);

The indentation is off here

> +
>   	replay.allow_ff = !(opts->flags & REBASE_FORCE);
>   	if (opts->allow_rerere_autoupdate)
>   		replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
> @@ -437,6 +449,8 @@ static int read_basic_state(struct rebase_options *opts)
>   	struct strbuf head_name = STRBUF_INIT;
>   	struct strbuf buf = STRBUF_INIT;
>   	struct object_id oid;
> +	struct strbuf t = STRBUF_INIT, one = STRBUF_INIT;

You can just use the existing "buf" instead of adding "t" and "one" is 
unneeded (see below)

> +	const char *path = state_dir_path(trailer_state_name, opts);
>   
>   	if (!read_oneliner(&head_name, state_dir_path("head-name", opts),
>   			   READ_ONELINER_WARN_MISSING) ||
> @@ -509,6 +523,22 @@ static int read_basic_state(struct rebase_options *opts)
>   
>   	strbuf_release(&buf);
>   
> +	if (strbuf_read_file(&t, path, 0) >= 0) {
> +		const char *p = t.buf, *end = t.buf + t.len;
> +
> +		while (p < end) {
> +			const char *nl = memchr(p, '\n', end - p);

We expect each line to be terminated with '\n' so we should die if nl is 
NULL
> +			strbuf_reset(&one);
> +			strbuf_add(&one, p, nl ? nl - p : end - p);

Instead of copying the string into an strbuf we can just do "*nl == '\0';"

> +			if (one.len) /* skip empty line */

There should not be any empty lines so we should die here if it is empty

> +				strvec_push(&opts->trailer_args,
> +					    strbuf_detach(&one, NULL));
> +			p = nl ? nl + 1 : end;
> +		}
> +		strbuf_release(&one);
> +	}
> +	strbuf_release(&t);
> +
>   	return 0;
>   }
>   
> @@ -537,6 +567,28 @@ static int rebase_write_basic_state(struct rebase_options *opts)
>   	if (opts->reviewby)
>   		write_file(state_dir_path("reviewby", opts), "--reviewby");
>   
> +    /*
> +     * save opts->trailer_args into state_dir/trailer
> +     */
> +    if (opts->trailer_args.nr) {
> +            struct strbuf buf = STRBUF_INIT;
> +            size_t i;
> +
> +            for (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_state_name, opts),
> +                       "%s", buf.buf);
> +            strbuf_release(&buf);

This looks good

> +    } else {
> +            /*
> +             * but if rebase doesn't pass any --trailer，
> +             * and state dir still have residual files，let's delete it。
> +             */

This should never happen should it?

> +            unlink_or_warn(state_dir_path(trailer_state_name, opts));
> +    }
> +
>   	return 0;
>   }
>   
> @@ -1140,6 +1192,7 @@ int cmd_rebase(int argc,
>   			.flags = PARSE_OPT_NOARG,
>   			.defval = REBASE_DIFFSTAT,
>   		},
> +		OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG),

This should be OPT_STRVEC() and should populate &options.trailers

>   		OPT_BOOL(0, "signoff", &options.signoff,
>   			 N_("add a Signed-off-by trailer to each commit")),
>   		OPT_BOOL(0, "reviewby", &options.reviewby,
> @@ -1292,6 +1345,13 @@ int cmd_rebase(int argc,
>   			     builtin_rebase_options,
>   			     builtin_rebase_usage, 0);
>   
> +    for (i = 0; i < trailer_args.nr; i++)
> + 	   strvec_push(&options.trailer_args, trailer_args.v[i]);
> +
> +    /* if add --trailer，force rebase */
> +    if (options.trailer_args.nr)
> +		   options.flags |= REBASE_FORCE;

This is good

>   	if (preserve_merges_selected)
>   		die(_("--preserve-merges was replaced by --rebase-merges\n"
>   			"Note: Your `pull.rebase` configuration may also be set to 'preserve',\n"
> @@ -1549,6 +1609,16 @@ int cmd_rebase(int argc,
>   	if (options.root && !options.onto_name)
>   		imply_merge(&options, "--root without --onto");
>   
> +	/*
> +	 * The apply‑based backend (git am) cannot append trailers because
> +	 * it lacks a message‑filter facility.  Reject early, before any
> +	 * state (index, HEAD, etc.) is modified.
> +	 */
> +	if (options.type == REBASE_APPLY && options.trailer_args.nr)
> +		die(_("the --apply backend (git am) cannot currently handle "
> +		      "--trailer; please omit --apply or use "
> +		      "the merge/interactive backend"));

It's good you're checking this but we should call imply_merge() to do 
that instead.

> +test_description='git rebase --trailer integration tests
> +We verify that --trailer on the merge/interactive/exec/root backends,
> +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
> +
> +############################################################################
> +# 1.  repository setup
> +############################################################################
> +
> +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 &&

The alignment is off here - it would be much simpler just to separate 
each argument with a single space

> +	ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
> +'
> +
> +create_expect () {
> +	cat >"$1" <<-EOF
> +	$2
> +
> +	Reviewed-by: Dev <dev@example.com>
> +	EOF
> +}
> +# golden files:
> +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"

Thanks for adding these tests. This should be inside the setup test - 
noting should be run outside of test_expect_success()

> +
> +############################################################################
> +# 2.  explicitly reject --apply + --trailer
> +############################################################################
> +
> +test_expect_success 'apply backend is rejected when --trailer is given' '
> +	git reset --hard third &&
> +	git tag before-apply &&
> +	test_expect_code 128 \
> +		git rebase --apply --trailer "Reviewed-by: Dev <dev@example.com>" \
> +			HEAD^ &&

We should check the error message here by redirecting stderr to a file 
and checking that with test_grep or test_cmp so we can be confidant that 
rebase is failing for the reason it should be and not some other bug.

> +	git diff --quiet before-apply..HEAD      # prove nothing moved

If you want to check that HEAD is unchanged I'd run "head=$(git show-ref 
--verify -s HEAD)" before the rebase and "test_cmp_rev HEAD $head" 
afterwards.

> +'
> +
> +############################################################################
> +# 3.  --no‑op: same range, no changes
> +############################################################################
> +
> +test_expect_success '--trailer without range change is a no‑op' '
> +	git checkout main &&
> +	git tag before-noop &&
> +	git rebase --trailer "Reviewed-by: Dev <dev@example.com>" HEAD &&
> +	git diff --quiet before-noop..HEAD

Do we really need this - it isn't really checking anything to do with 
the implementation of --trailer.

> +
> +############################################################################
> +# 4.  merge backend (-m), conflict resolution path
> +############################################################################

Excellent

> +
> +test_expect_success 'rebase -m --trailer adds trailer after conflicts' '
> +	git reset --hard third &&
> +	test_must_fail git rebase -m \
> +		--trailer "Reviewed-by: Dev <dev@example.com>" \
> +		second third &&
> +	git checkout --theirs file &&
> +	git add file &&
> +    GIT_EDITOR=: git rebase --continue &&
GIT_EDITOR=: unless you call test_set_editor() which should always be 
done in a subshell so this isn't needed.

> +	git show --no-patch --pretty=format:%B HEAD~2 >actual &&
> +	test_cmp file2-signed actual

I think test_commit_message would help simplify this

> +'
> +
> +############################################################################
> +# 5.  --exec path
> +############################################################################
> +
> +test_expect_success 'rebase --exec --trailer adds trailer' '
> +	test_when_finished "rm -f touched" &&
> +	git rebase --exec "touch touched" \
> +		--trailer "Reviewed-by: Dev <dev@example.com>" \
> +		first^ first &&
> +	test_path_is_file touched &&
> +	test_commit_message HEAD first-signed

I'm not sure this has anything to do with --trailer

> +'
> +
> +############################################################################
> +# 6.  --root path
> +############################################################################
> +
> +test_expect_success 'rebase --root --trailer updates every commit' '
> +	git checkout first &&
> +	git rebase --root --keep-empty \
> +		--trailer "Reviewed-by: Dev <dev@example.com>" &&
> +	test_commit_message HEAD   first-signed &&
> +	test_commit_message HEAD^  initial-signed

This test looks good.

Best Wishes

Phillip

> +'
> +test_done
G

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

[Li Chen]

Hi Phillip,

 ---- On Thu, 08 May 2025 22:17:17 +0800  Phillip Wood <phillip.wood123@gmail.com> wrote --- 
 > Hi Li
 > 
 > On 06/05/2025 13:58, Li Chen wrote:
 > > From: Li Chen <chenl311@chinatelecom.cn>
 > > 
 > > Implement a new `--trailer <text>` option for `git rebase`
 > > (support merge backend only now), which appends arbitrary
 > > trailer lines to each rebased commit message. Reject early
 > > if used with the apply backend (git am) since it lacks
 > > message‑filter/trailer hook. Automatically set REBASE_FORCE when
 > > any trailer is supplied.
 > 
 > I think this is a reasonable idea but unfortunately I think the trailer 
 > API needs improving so that the implementation
 > 
 > (a) Checks the trailers given on the command-line before the user edits 
 > the todo list. That way we reject invalid trailers and exit before the 
 > user has spent any effort editing the todo list.
 > 
 > (b) Does not fork another process to add the trailers. Without this the 
 > performance is going to suffer. Hopefully it wont be too difficult to 
 > modify the existing code to take a struct strbuf and a list of trailers 
 > to append to it.
 > 
 > (c) Only adds the trailers on the commandline. I'm a bit confused by the 
 > various trailer config options - the man page reads to me like "git 
 > interpret-trailers" can add missing trailers that are configured but not 
 > passed on the commandline.
 > 
 > The changes to the trailer api should be made in one or more preparatory 
 > commits before adding support for --trailer to "git rebase"

Thanks a lot for the detailed feedback and for outlining the gaps in the
current trailer API – it really helps me see the next steps more
clearly.

I have one questions:

Who should take the API work?

I’m very happy to roll up my sleeves and prototype the improvements
you described (reject invalid trailers up‑front, add an in‑process API
that works on a struct strbuf, and ensure only the CLI‑supplied
trailers are added). That would give me a chance to contribute a bit
more deeply to Git. Of course, if you already have something in mind
and would rather drive that part yourself, I’m equally happy to wait
and re‑base my series on top of it. Please let me know which you
prefer.

 > I've left some comments on the changes to builtin/rebase.c and the 
 > tests, I've skipped the changes to sequencer.c for now as they'll have 
 > to be updated to avoid forking "git interpret-trailers"

Thanks for all your great reviews!

I'll address all your reviews in next version.
 

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

[phillip.wood123]

Hi Li

On 08/05/2025 16:55, Li Chen wrote:
>   ---- On Thu, 08 May 2025 22:17:17 +0800  Phillip Wood <phillip.wood123@gmail.com> wrote ---
>   > On 06/05/2025 13:58, Li Chen wrote:
>   > > From: Li Chen <chenl311@chinatelecom.cn>
>   > >
>   > > Implement a new `--trailer <text>` option for `git rebase`
>   > > (support merge backend only now), which appends arbitrary
>   > > trailer lines to each rebased commit message. Reject early
>   > > if used with the apply backend (git am) since it lacks
>   > > message‑filter/trailer hook. Automatically set REBASE_FORCE when
>   > > any trailer is supplied.
>   >
>   > I think this is a reasonable idea but unfortunately I think the trailer
>   > API needs improving so that the implementation
>   >
>   > (a) Checks the trailers given on the command-line before the user edits
>   > the todo list. That way we reject invalid trailers and exit before the
>   > user has spent any effort editing the todo list.
>   >
>   > (b) Does not fork another process to add the trailers. Without this the
>   > performance is going to suffer. Hopefully it wont be too difficult to
>   > modify the existing code to take a struct strbuf and a list of trailers
>   > to append to it.
>   >
>   > (c) Only adds the trailers on the commandline. I'm a bit confused by the
>   > various trailer config options - the man page reads to me like "git
>   > interpret-trailers" can add missing trailers that are configured but not
>   > passed on the commandline.
>   >
>   > The changes to the trailer api should be made in one or more preparatory
>   > commits before adding support for --trailer to "git rebase"
> 
> Thanks a lot for the detailed feedback and for outlining the gaps in the
> current trailer API – it really helps me see the next steps more
> clearly.
> 
> I have one questions:
> 
> Who should take the API work?
> 
> I’m very happy to roll up my sleeves and prototype the improvements
> you described (reject invalid trailers up‑front, add an in‑process API
> that works on a struct strbuf, and ensure only the CLI‑supplied
> trailers are added). That would give me a chance to contribute a bit
> more deeply to Git. Of course, if you already have something in mind
> and would rather drive that part yourself, I’m equally happy to wait
> and re‑base my series on top of it. Please let me know which you
> prefer.

If you're interested in doing the work on the trailer api that would be 
great - I don't have time myself.

Thanks

Phillip

>   > I've left some comments on the changes to builtin/rebase.c and the
>   > tests, I've skipped the changes to sequencer.c for now as they'll have
>   > to be updated to avoid forking "git interpret-trailers"
> 
> Thanks for all your great reviews!
> 
> I'll address all your reviews in next version.
>   
> 

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

[Phillip Wood]

Hi Li

On 08/05/2025 16:55, Li Chen wrote:
>   ---- On Thu, 08 May 2025 22:17:17 +0800  Phillip Wood <phillip.wood123@gmail.com> wrote ---
> 
>   > I've left some comments on the changes to builtin/rebase.c and the
>   > tests, I've skipped the changes to sequencer.c for now as they'll have
>   > to be updated to avoid forking "git interpret-trailers"
> 
> Thanks for all your great reviews!
> 
> I'll address all your reviews in next version.

I forgot to say earlier that if you have any questions or queries about 
my review or the trailer code do feel free to ask.

Best Wishes

Phillip

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

[Li Chen]

Hi Phillip,

 ---- On Thu, 08 May 2025 22:17:17 +0800  Phillip Wood <phillip.wood123@gmail.com> wrote --- 
 > Hi Li
 > 
 > On 06/05/2025 13:58, Li Chen wrote:
 > > From: Li Chen <chenl311@chinatelecom.cn>
 > > 
 > > Implement a new `--trailer <text>` option for `git rebase`
 > > (support merge backend only now), which appends arbitrary
 > > trailer lines to each rebased commit message. Reject early
 > > if used with the apply backend (git am) since it lacks
 > > message‑filter/trailer hook. Automatically set REBASE_FORCE when
 > > any trailer is supplied.
 > 
 > I think this is a reasonable idea but unfortunately I think the trailer 
 > API needs improving so that the implementation
 > 
 > (a) ...
 > 
 > (b) ...
 > 
 > (c) Only adds the trailers on the commandline. I'm a bit confused by the 
 > various trailer config options - the man page reads to me like "git 
 > interpret-trailers" can add missing trailers that are configured but not 
 > passed on the commandline.

About part (c), just to be sure I understand correctly:

Do you want the trailer implementation to completely drop any handling of trailer configuration 
(i.e. remove parse_trailers_from_config() and related config-based behavior from the codepath and man page/documents)?

Or would you rather leave the config machinery in place, but have rebase --trailer explicitly
ignore all trailer.* configuration and only append the exact trailers passed on its command line?

Please let me know which you prefer (or if there’s a third path I’m missing) and I’ll add the patches accordingly.

Regards,
Li

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

[Phillip Wood]

Hi Li

On 16/05/2025 06:42, Li Chen wrote:
> Hi Phillip,
> 
>   ---- On Thu, 08 May 2025 22:17:17 +0800  Phillip Wood <phillip.wood123@gmail.com> wrote ---
>   >
>   > (c) Only adds the trailers on the commandline. I'm a bit confused by the
>   > various trailer config options - the man page reads to me like "git
>   > interpret-trailers" can add missing trailers that are configured but not
>   > passed on the commandline.
> 
> About part (c), just to be sure I understand correctly:
> 
> Do you want the trailer implementation to completely drop any handling of trailer configuration
> (i.e. remove parse_trailers_from_config() and related config-based behavior from the codepath and man page/documents)?
> 
> Or would you rather leave the config machinery in place, but have rebase --trailer explicitly
> ignore all trailer.* configuration and only append the exact trailers passed on its command line?

I think I had misunderstood what trailer.ifMissing did. I was concerned 
that it could add trailers that were not on the command-line but I don't 
think that's the case. We certainly want to respect the config for 
trailer.<alias>.key and trailer.<alias>.command as they make it possible 
for the user to set "trailer.review.key=Reviewed-by" and 
"trailer.review.command=git var GIT_COMMITTER_IDENT | sed 's/[^>]*$//' 
#" and then run "git rebase --trailer=review" to add their Reviewed-by: 
trailer. I think it makes sense to respect the other config as well - 
that does mean that trailers that the user passes on the command-line 
may not be added because they already exist or are configured not to be 
added if they are missing but is consistent with "git 
interpret-trailers". It means that the user can set 
"trailer.myKey.ifExists=doNothing" and then run "git rebase --trailer 
MyKey=value" to ensure all the commits have a MyKey trailer without 
duplicating it in the commits where it already exists.

That's a long-winded way of saying that on reflection I think respecting 
the trailer config setting is the right thing to do after all.

Best Wishes

Phillip

> Please let me know which you prefer (or if there’s a third path I’m missing) and I’ll add the patches accordingly.
> 
> Regards,
> Li