[PATCH v3 0/2] rebase: support --trailer

[PATCH v3 0/2] rebase: support --trailer

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

This two-patch series teaches git rebase a new
--trailer <text> option and, as a prerequisite, moves all trailer
handling out of the external interpret-trailers helper and into the
builtin code path, as suggested by Phillip Wood.

Patch 1 switches trailer.c to an in-memory implementation
(amend_strbuf_with_trailers()). It removes every fork/exec.

Patch 2 builds on that helper to implement
git rebase --trailer. When the option is given we:
force the merge backend (apply/am backend lacks a message filter),
automatically enable --force-rebase so that fast-forwarded
commits are rewritten, and append the requested trailer(s) to every
rewritten commit.
State is stored in $state_dir/trailer so an interrupted rebase can
resume safely. A dedicated test-suite (t3440) exercises plain,
conflict, --root, invalid-input scenarios and etc.

All t/*.sh testcases have run successfully.
Github CI tests have all been past: https://github.com/FirstLoveLife/git/actions/runs/16704004515

v3: merges the remaining trailer paths into one in-process helper, dropping the
    duplicate code, as pointed by Junio and Phillip [1]
v2: fix issues pointed by Phillip 
RFC link: https://lore.kernel.org/git/196a5ac1393.f5b4db7d187309.2451613571977217927@linux.beauty/

Comments welcome!

[1]: https://lore.kernel.org/git/xmqq8qlzkukw.fsf@gitster.g/

Li Chen (2):
  trailer: append trailers in-process and drop the fork to
    `interpret-trailers`
  rebase: support --trailer

 Documentation/git-rebase.adoc |   7 ++
 builtin/interpret-trailers.c  | 117 ++++++++-----------------------
 builtin/rebase.c              |  98 ++++++++++++++++++++++++++
 sequencer.c                   |  13 ++++
 sequencer.h                   |   3 +
 t/meson.build                 |   1 +
 t/t3440-rebase-trailer.sh     |  95 ++++++++++++++++++++++++++
 trailer.c                     | 125 +++++++++++++++++++++++++++++++---
 trailer.h                     |  18 ++++-
 9 files changed, 375 insertions(+), 102 deletions(-)
 create mode 100755 t/t3440-rebase-trailer.sh

-- 
2.50.0

[PATCH v3 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers`

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

All trailer insertion now funnels through trailer_process():

* builtin/interpret-trailers.c is reduced to file I/O + a single call.
* amend_file_with_trailers() shares the same path; the old
  amend_strbuf_with_trailers() helper is dropped.
* New helpers parse_trailer_args()/free_new_trailer_list() convert
  --trailer=... strings to new_trailer_item lists.

Behaviour is unchanged; the full test-suite still passes, and the
fork/exec is gone.

Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
---
 builtin/interpret-trailers.c | 117 ++++++++------------------------
 trailer.c                    | 125 ++++++++++++++++++++++++++++++++---
 trailer.h                    |  18 ++++-
 3 files changed, 158 insertions(+), 102 deletions(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 44d8ccddc9..3a49d3d7b0 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -9,7 +9,6 @@
 #include "gettext.h"
 #include "parse-options.h"
 #include "string-list.h"
-#include "tempfile.h"
 #include "trailer.h"
 #include "config.h"
 
@@ -84,6 +83,7 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
 			   int unset)
 {
 	struct process_trailer_options *v = opt->value;
+
 	v->only_trailers = 1;
 	v->only_input = 1;
 	v->unfold = 1;
@@ -92,37 +92,6 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
 	return 0;
 }
 
-static struct tempfile *trailers_tempfile;
-
-static FILE *create_in_place_tempfile(const char *file)
-{
-	struct stat st;
-	struct strbuf filename_template = STRBUF_INIT;
-	const char *tail;
-	FILE *outfile;
-
-	if (stat(file, &st))
-		die_errno(_("could not stat %s"), file);
-	if (!S_ISREG(st.st_mode))
-		die(_("file %s is not a regular file"), file);
-	if (!(st.st_mode & S_IWUSR))
-		die(_("file %s is not writable by user"), file);
-
-	/* Create temporary file in the same directory as the original */
-	tail = strrchr(file, '/');
-	if (tail)
-		strbuf_add(&filename_template, file, tail - file + 1);
-	strbuf_addstr(&filename_template, "git-interpret-trailers-XXXXXX");
-
-	trailers_tempfile = xmks_tempfile_m(filename_template.buf, st.st_mode);
-	strbuf_release(&filename_template);
-	outfile = fdopen_tempfile(trailers_tempfile, "w");
-	if (!outfile)
-		die_errno(_("could not open temporary file"));
-
-	return outfile;
-}
-
 static void read_input_file(struct strbuf *sb, const char *file)
 {
 	if (file) {
@@ -135,61 +104,6 @@ static void read_input_file(struct strbuf *sb, const char *file)
 	strbuf_complete_line(sb);
 }
 
-static void interpret_trailers(const struct process_trailer_options *opts,
-			       struct list_head *new_trailer_head,
-			       const char *file)
-{
-	LIST_HEAD(head);
-	struct strbuf sb = STRBUF_INIT;
-	struct strbuf trailer_block_sb = STRBUF_INIT;
-	struct trailer_block *trailer_block;
-	FILE *outfile = stdout;
-
-	trailer_config_init();
-
-	read_input_file(&sb, file);
-
-	if (opts->in_place)
-		outfile = create_in_place_tempfile(file);
-
-	trailer_block = parse_trailers(opts, sb.buf, &head);
-
-	/* Print the lines before the trailer block */
-	if (!opts->only_trailers)
-		fwrite(sb.buf, 1, trailer_block_start(trailer_block), outfile);
-
-	if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
-		fprintf(outfile, "\n");
-
-
-	if (!opts->only_input) {
-		LIST_HEAD(config_head);
-		LIST_HEAD(arg_head);
-		parse_trailers_from_config(&config_head);
-		parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
-		list_splice(&config_head, &arg_head);
-		process_trailers_lists(&head, &arg_head);
-	}
-
-	/* Print trailer block. */
-	format_trailers(opts, &head, &trailer_block_sb);
-	free_trailers(&head);
-	fwrite(trailer_block_sb.buf, 1, trailer_block_sb.len, outfile);
-	strbuf_release(&trailer_block_sb);
-
-	/* Print the lines after the trailer block as is. */
-	if (!opts->only_trailers)
-		fwrite(sb.buf + trailer_block_end(trailer_block), 1,
-		       sb.len - trailer_block_end(trailer_block), outfile);
-	trailer_block_release(trailer_block);
-
-	if (opts->in_place)
-		if (rename_tempfile(&trailers_tempfile, file))
-			die_errno(_("could not rename temporary file to %s"), file);
-
-	strbuf_release(&sb);
-}
-
 int cmd_interpret_trailers(int argc,
 			   const char **argv,
 			   const char *prefix,
@@ -231,14 +145,37 @@ int cmd_interpret_trailers(int argc,
 			git_interpret_trailers_usage,
 			options);
 
+	trailer_config_init();
+
 	if (argc) {
 		int i;
-		for (i = 0; i < argc; i++)
-			interpret_trailers(&opts, &trailers, argv[i]);
+		for (i = 0; i < argc; i++) {
+			struct strbuf in_buf = STRBUF_INIT;
+			struct strbuf out_buf = STRBUF_INIT;
+
+			read_input_file(&in_buf, argv[i]);
+			if (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)
+				die(_("failed to process trailers for %s"), argv[i]);
+			if (opts.in_place)
+				write_file_buf(argv[i], out_buf.buf, out_buf.len);
+			else
+				fwrite(out_buf.buf, 1, out_buf.len, stdout);
+			strbuf_release(&in_buf);
+			strbuf_release(&out_buf);
+		}
 	} else {
+		struct strbuf in_buf = STRBUF_INIT;
+		struct strbuf out_buf = STRBUF_INIT;
+
 		if (opts.in_place)
 			die(_("no input file given for in-place editing"));
-		interpret_trailers(&opts, &trailers, NULL);
+
+		read_input_file(&in_buf, NULL);
+		if (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)
+			die(_("failed to process trailers"));
+		fwrite(out_buf.buf, 1, out_buf.len, stdout);
+		strbuf_release(&in_buf);
+		strbuf_release(&out_buf);
 	}
 
 	new_trailers_clear(&trailers);
diff --git a/trailer.c b/trailer.c
index 310cf582dc..03814443c3 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1224,14 +1224,121 @@ void trailer_iterator_release(struct trailer_iterator *iter)
 	strbuf_release(&iter->key);
 }
 
-int amend_file_with_trailers(const char *path, const struct strvec *trailer_args)
+static int amend_strbuf_with_trailers(struct strbuf *buf,
+				   const struct strvec *trailer_args)
 {
-	struct child_process run_trailer = CHILD_PROCESS_INIT;
-
-	run_trailer.git_cmd = 1;
-	strvec_pushl(&run_trailer.args, "interpret-trailers",
-		     "--in-place", "--no-divider",
-		     path, NULL);
-	strvec_pushv(&run_trailer.args, trailer_args->v);
-	return run_command(&run_trailer);
+	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+	LIST_HEAD(new_trailer_head);
+	struct strbuf out = STRBUF_INIT;
+	size_t i;
+
+	opts.no_divider = 1;
+
+	for (i = 0; i < trailer_args->nr; i++) {
+		const char *arg = trailer_args->v[i];
+		const char *text;
+		struct new_trailer_item *item;
+		if (!skip_prefix(arg, "--trailer=", &text))
+			text = arg;
+		if (!*text)
+			continue;
+		item = xcalloc(1, sizeof(*item));
+		INIT_LIST_HEAD(&item->list);
+		item->text = text;
+		list_add_tail(&item->list, &new_trailer_head);
+	}
+	if (trailer_process(&opts, buf->buf, &new_trailer_head, &out) < 0)
+		die("failed to process trailers");
+	strbuf_swap(buf, &out);
+	strbuf_release(&out);
+	while (!list_empty(&new_trailer_head)) {
+		struct new_trailer_item *item =
+			list_first_entry(&new_trailer_head, struct new_trailer_item, list);
+		list_del(&item->list);
+		free(item);
+	}
+	return 0;
 }
+
+int trailer_process(const struct process_trailer_options *opts,
+				   const char *msg,
+				   struct list_head *new_trailer_head,
+				   struct strbuf *out)
+{
+		struct trailer_block *blk;
+		LIST_HEAD(orig_head);
+		LIST_HEAD(config_head);
+		LIST_HEAD(arg_head);
+		struct strbuf trailers_sb = STRBUF_INIT;
+		int had_trailer_before;
+
+		blk = parse_trailers(opts, msg, &orig_head);
+		had_trailer_before = !list_empty(&orig_head);
+		if (!opts->only_input) {
+			parse_trailers_from_config(&config_head);
+			parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
+			list_splice(&config_head, &arg_head);
+			process_trailers_lists(&orig_head, &arg_head);
+		}
+		format_trailers(opts, &orig_head, &trailers_sb);
+		if (!opts->only_trailers && !opts->only_input && !opts->unfold &&
+			!opts->trim_empty && list_empty(&orig_head) &&
+			(list_empty(new_trailer_head) || opts->only_input)) {
+			size_t split = trailer_block_start(blk); /* end-of-log-msg */
+			if (!blank_line_before_trailer_block(blk)) {
+				strbuf_add(out, msg, split);
+				strbuf_addch(out, '\n');
+				strbuf_addstr(out, msg + split);
+			} else
+				strbuf_addstr(out, msg);
+
+			strbuf_release(&trailers_sb);
+			trailer_block_release(blk);
+			return 0;
+		}
+		if (opts->only_trailers) {
+			strbuf_addbuf(out, &trailers_sb);
+		} else if (had_trailer_before) {
+			strbuf_add(out, msg, trailer_block_start(blk));
+			if (!blank_line_before_trailer_block(blk))
+				strbuf_addch(out, '\n');
+			strbuf_addbuf(out, &trailers_sb);
+			strbuf_add(out, msg + trailer_block_end(blk),
+						strlen(msg) - trailer_block_end(blk));
+		}
+		else {
+			size_t cpos = trailer_block_start(blk);
+			strbuf_add(out, msg, cpos);
+			if (cpos == 0)                     /* empty body → just one \n */
+				strbuf_addch(out, '\n');
+			else if (!blank_line_before_trailer_block(blk))
+				strbuf_addch(out, '\n');   /* body without trailing blank */
+
+			strbuf_addbuf(out, &trailers_sb);
+			strbuf_add(out, msg + cpos, strlen(msg) - cpos);
+	   }
+		strbuf_release(&trailers_sb);
+		free_trailers(&orig_head);
+		trailer_block_release(blk);
+		return 0;
+}
+
+int amend_file_with_trailers(const char *path,
+							 const struct strvec *trailer_args)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	if (!trailer_args || !trailer_args->nr)
+		return 0;
+
+	if (strbuf_read_file(&buf, path, 0) < 0)
+		return error_errno("could not read '%s'", path);
+
+	if (amend_strbuf_with_trailers(&buf, trailer_args))
+		die("failed to append trailers");
+
+	/* `write_file_buf()` aborts on error internally */
+	write_file_buf(path, buf.buf, buf.len);
+	strbuf_release(&buf);
+	return 0;
+ }
diff --git a/trailer.h b/trailer.h
index 4740549586..01f711fb13 100644
--- a/trailer.h
+++ b/trailer.h
@@ -196,10 +196,22 @@ int trailer_iterator_advance(struct trailer_iterator *iter);
 void trailer_iterator_release(struct trailer_iterator *iter);
 
 /*
- * Augment a file to add trailers to it by running git-interpret-trailers.
- * This calls run_command() and its return value is the same (i.e. 0 for
- * success, various non-zero for other errors). See run-command.h.
+ * 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.
  */
 int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);
 
+/*
+ * Process trailer lines for a commit message in-memory.
+ * @opts: trailer processing options (e.g. from parse-options)
+ * @msg: the input message string
+ * @new_trailer_head: list of new trailers to add (struct new_trailer_item)
+ * @out: strbuf to store the resulting message (must be initialized)
+ *
+ * Returns 0 on success, <0 on error.
+ */
+int trailer_process(const struct process_trailer_options *opts,
+			const char *msg,
+			struct list_head *new_trailer_head,
+			struct strbuf *out);
 #endif /* TRAILER_H */
-- 
2.50.0

[PATCH v3 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.

And reject invalid input before user edit the interactive file.

Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
---
 Documentation/git-rebase.adoc |  7 +++
 builtin/rebase.c              | 98 +++++++++++++++++++++++++++++++++++
 sequencer.c                   | 13 +++++
 sequencer.h                   |  3 ++
 t/meson.build                 |  1 +
 t/t3440-rebase-trailer.sh     | 95 +++++++++++++++++++++++++++++++++
 6 files changed, 217 insertions(+)
 create mode 100755 t/t3440-rebase-trailer.sh

diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc
index 956d3048f5..df8fb97526 100644
--- a/Documentation/git-rebase.adoc
+++ b/Documentation/git-rebase.adoc
@@ -521,6 +521,13 @@ 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.
 +
+--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 e90562a3b8..3b4c45a616 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -36,6 +36,8 @@
 #include "reset.h"
 #include "trace2.h"
 #include "hook.h"
+#include "trailer.h"
+#include "parse-options.h"
 
 static char const * const builtin_rebase_usage[] = {
 	N_("git rebase [-i] [options] [--exec <cmd>] "
@@ -113,6 +115,7 @@ struct rebase_options {
 	enum action action;
 	char *reflog_action;
 	int signoff;
+	struct strvec trailer_args;
 	int allow_rerere_autoupdate;
 	int keep_empty;
 	int autosquash;
@@ -143,6 +146,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,             \
@@ -166,6 +170,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)
@@ -177,6 +182,10 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 	sequencer_init_config(&replay);
 
 	replay.signoff = opts->signoff;
+
+	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;
@@ -435,6 +444,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;
+	const char trailer_state_name[] = "trailer";
+	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) ||
@@ -503,11 +514,31 @@ static int read_basic_state(struct rebase_options *opts)
 
 	strbuf_release(&buf);
 
+	if (strbuf_read_file(&buf, path, 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;
+		}
+		strbuf_release(&buf);
+	}
+	strbuf_release(&buf);
+
 	return 0;
 }
 
 static int rebase_write_basic_state(struct rebase_options *opts)
 {
+	const char trailer_state_name[] = "trailer";
+
 	write_file(state_dir_path("head-name", opts), "%s",
 		   opts->head_name ? opts->head_name : "detached HEAD");
 	write_file(state_dir_path("onto", opts), "%s",
@@ -529,6 +560,22 @@ 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
+     */
+    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);
+    }
+
 	return 0;
 }
 
@@ -1085,6 +1132,37 @@ static int check_exec_cmd(const char *cmd)
 	return 0;
 }
 
+static int validate_trailer_args_after_config(const struct strvec *cli_args,
+				       struct strbuf *err)
+{
+	size_t i;
+
+	for (i = 0; i < cli_args->nr; i++) {
+		const char *raw = cli_args->v[i];
+		const char *txt; // Key[:=]Val
+		const char *sep;
+
+		if (!skip_prefix(raw, "--trailer=", &txt))
+			txt = raw;
+
+		if (!*txt) {
+			strbuf_addstr(err, _("empty --trailer argument"));
+			return -1;
+		}
+
+		sep = strpbrk(txt, ":=");
+
+		/* there must be key bfore seperator */
+		if (sep && sep == txt) {
+			strbuf_addf(err,
+				    _("invalid trailer '%s': missing key before separator"),
+				    txt);
+			return -1;
+		}
+	}
+	return 0;
+}
+
 int cmd_rebase(int argc,
 	       const char **argv,
 	       const char *prefix,
@@ -1133,6 +1211,7 @@ 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)")),
 		OPT_BOOL(0, "signoff", &options.signoff,
 			 N_("add a Signed-off-by trailer to each commit")),
 		OPT_BOOL(0, "committer-date-is-author-date",
@@ -1283,6 +1362,17 @@ int cmd_rebase(int argc,
 			     builtin_rebase_options,
 			     builtin_rebase_usage, 0);
 
+    /* if add --trailer，force rebase */
+	if (options.trailer_args.nr) {
+        struct strbuf err = STRBUF_INIT;
+
+		if (validate_trailer_args_after_config(&options.trailer_args, &err))
+			die("%s", err.buf);
+
+        options.flags |= REBASE_FORCE;
+        strbuf_release(&err);
+	}
+
 	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"
@@ -1540,6 +1630,14 @@ 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.trailer_args.nr)
+		imply_merge(&options, "--trailer");
+
 	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 67e4310edc..58faf6aed5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -422,6 +422,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)
@@ -2529,6 +2530,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, reflog_action,
diff --git a/sequencer.h b/sequencer.h
index 304ba4b4d3..28f2da6375 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -4,6 +4,7 @@
 #include "strbuf.h"
 #include "strvec.h"
 #include "wt-status.h"
+#include <stddef.h>
 
 struct commit;
 struct index_state;
@@ -44,6 +45,7 @@ struct replay_opts {
 	int record_origin;
 	int no_commit;
 	int signoff;
+	struct strvec trailer_args;
 	int allow_ff;
 	int allow_rerere_auto;
 	int allow_empty;
@@ -86,6 +88,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 09f3068f98..3c58f562da 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -373,6 +373,7 @@ integration_tests = [
   't3436-rebase-more-options.sh',
   't3437-rebase-fixup-options.sh',
   't3438-rebase-broken-files.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..a580449628
--- /dev/null
+++ b/t/t3440-rebase-trailer.sh
@@ -0,0 +1,95 @@
+#!/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
+
+create_expect() {
+	cat >"$1" <<-EOF
+		$2
+
+		Reviewed-by: Dev <dev@example.com>
+	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 &&
+	ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" &&
+	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"
+'
+
+test_expect_success 'apply backend is rejected with --trailer' '
+	git reset --hard third &&
+	head_before=$(git rev-parse HEAD) &&
+    test_expect_code 128 \
+    		git rebase --apply --trailer "Reviewed-by: Dev <dev@example.com>" \
+    			HEAD^ 2>err &&
+	test_grep "requires the merge backend" err &&
+	test_cmp_rev HEAD $head_before
+'
+
+test_expect_success 'reject empty --trailer argument' '
+        git reset --hard third &&
+        test_expect_code 128 git rebase -m --trailer "" HEAD^ 2>err &&
+        test_grep "empty --trailer" err
+'
+
+test_expect_success 'reject trailer with missing key before separator' '
+        git reset --hard third &&
+        test_expect_code 128 git rebase -m --trailer ": no-key" HEAD^ 2>err &&
+        test_grep "missing key before separator" err
+'
+
+test_expect_success 'CLI trailer duplicates allowed; replace policy keeps last' '
+        git reset --hard third &&
+        git -c trailer.Bug.ifexists=replace -c trailer.Bug.ifmissing=add rebase -m --trailer "Bug: 123" --trailer "Bug: 456" HEAD~1 &&
+        git cat-file commit HEAD | grep "^Bug: 456" &&
+        git cat-file commit HEAD | grep -v "^Bug: 123"
+'
+
+test_expect_success 'multiple Signed-off-by trailers all preserved' '
+        git reset --hard third &&
+        git rebase -m \
+            --trailer "Signed-off-by: Dev A <a@ex.com>" \
+            --trailer "Signed-off-by: Dev B <b@ex.com>" HEAD~1 &&
+        git cat-file commit HEAD | grep -c "^Signed-off-by:" >count &&
+        test "$(cat count)" = 2   # two new commits
+'
+
+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 rebase --continue &&
+	test_commit_message HEAD~2 file2-signed
+'
+
+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.50.0

Re: [PATCH v3 0/2] rebase: support --trailer

[Junio C Hamano]

Li Chen <me@linux.beauty> writes:

> From: Li Chen <chenl311@chinatelecom.cn>
>
> This two-patch series teaches git rebase a new
> --trailer <text> option and, as a prerequisite, moves all trailer
> handling out of the external interpret-trailers helper and into the
> builtin code path, as suggested by Phillip Wood.
>
> Patch 1 switches trailer.c to an in-memory implementation
> (amend_strbuf_with_trailers()). It removes every fork/exec.
>
> Patch 2 builds on that helper to implement
> git rebase --trailer.

Try running "git show --check" on this commit.  My attempt found a
handful of whitespace breakages ("indent with spaces.").

Thanks.

Re: [PATCH v3 0/2] rebase: support --trailer

[Li Chen]

Hi Junio,

 ---- On Mon, 04 Aug 2025 00:35:57 +0800  Junio C Hamano <gitster@pobox.com> wrote --- 
 > Li Chen <me@linux.beauty> writes:
 > 
 > > From: Li Chen <chenl311@chinatelecom.cn>
 > >
 > > This two-patch series teaches git rebase a new
 > > --trailer <text> option and, as a prerequisite, moves all trailer
 > > handling out of the external interpret-trailers helper and into the
 > > builtin code path, as suggested by Phillip Wood.
 > >
 > > Patch 1 switches trailer.c to an in-memory implementation
 > > (amend_strbuf_with_trailers()). It removes every fork/exec.
 > >
 > > Patch 2 builds on that helper to implement
 > > git rebase --trailer.
 > 
 > Try running "git show --check" on this commit.  My attempt found a
 > handful of whitespace breakages ("indent with spaces.").
 > 
 > Thanks.
 > 
 > 

That's a great tool, I wasn't aware of that command, and I will fix them in
Next version. Thanks, Junio.

Regards,
Li

Re: [PATCH v3 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers`

[Phillip Wood]

Hi Li

On 03/08/2025 16:00, Li Chen wrote:
> From: Li Chen <chenl311@chinatelecom.cn>
> 
> All trailer insertion now funnels through trailer_process():
> 
> * builtin/interpret-trailers.c is reduced to file I/O + a single call.
> * amend_file_with_trailers() shares the same path; the old
>    amend_strbuf_with_trailers() helper is dropped.
> * New helpers parse_trailer_args()/free_new_trailer_list() convert
>    --trailer=... strings to new_trailer_item lists.
> 
> Behaviour is unchanged; the full test-suite still passes, and the
> fork/exec is gone.

Normally commit messages should be written in prose rather than a bullet 
list and the message should explain the reason for the change.

This patch has much less code duplication than the last iteration which 
is most welcome. Whenever you are moving and refactoring code you should 
split the move into its own commit followed by the refactoring. That 
makes it much easier to review as the reviewer can clearly see the 
refactoring rather than having to manually compare the added code in one 
file to the deleted code in another.

> @@ -84,6 +83,7 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
>   			   int unset)
>   {
>   	struct process_trailer_options *v = opt->value;
> +

Let's not clutter this patch with unrelated changes.

>   	v->only_trailers = 1;
>   	v->only_input = 1;
>   	v->unfold = 1;
> @@ -92,37 +92,6 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
>   	return 0;
>   }
>   
> -static FILE *create_in_place_tempfile(const char *file)
> -{
> [...]
> -}

We don't need to create a temporary file anymore so this can be deleted 
- good.

> -static void interpret_trailers(const struct process_trailer_options *opts,
> -			       struct list_head *new_trailer_head,
> -			       const char *file)
> -{
> -	LIST_HEAD(head);
> -	struct strbuf sb = STRBUF_INIT;
> -	struct strbuf trailer_block_sb = STRBUF_INIT;
> -	struct trailer_block *trailer_block;
> -	FILE *outfile = stdout;
> -
> -	trailer_config_init();
> -
> -	read_input_file(&sb, file);
> -
> -	if (opts->in_place)
> -		outfile = create_in_place_tempfile(file);
> -
> -	trailer_block = parse_trailers(opts, sb.buf, &head);
> -
> -	/* Print the lines before the trailer block */
> -	if (!opts->only_trailers)
> -		fwrite(sb.buf, 1, trailer_block_start(trailer_block), outfile);
> -
> -	if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
> -		fprintf(outfile, "\n");
> -
> -
> -	if (!opts->only_input) {
> -		LIST_HEAD(config_head);
> -		LIST_HEAD(arg_head);
> -		parse_trailers_from_config(&config_head);
> -		parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
> -		list_splice(&config_head, &arg_head);
> -		process_trailers_lists(&head, &arg_head);
> -	}
> -
> -	/* Print trailer block. */
> -	format_trailers(opts, &head, &trailer_block_sb);
> -	free_trailers(&head);
> -	fwrite(trailer_block_sb.buf, 1, trailer_block_sb.len, outfile);
> -	strbuf_release(&trailer_block_sb);
> -
> -	/* Print the lines after the trailer block as is. */
> -	if (!opts->only_trailers)
> -		fwrite(sb.buf + trailer_block_end(trailer_block), 1,
> -		       sb.len - trailer_block_end(trailer_block), outfile);
> -	trailer_block_release(trailer_block);
> -
> -	if (opts->in_place)
> -		if (rename_tempfile(&trailers_tempfile, file))
> -			die_errno(_("could not rename temporary file to %s"), file);
> -
> -	strbuf_release(&sb);
> -}

This code is moved to trailer.c which is good but it is heavily 
refactored at the same time which makes it hard to review. Completely 
removing this function leads to some duplication in 
cmd_interpret_trailers() which could be avoided by making 
interpret_trailers() a wrapper around process_trailers()

>   int cmd_interpret_trailers(int argc,
>   			   const char **argv,
>   			   const char *prefix,
> @@ -231,14 +145,37 @@ int cmd_interpret_trailers(int argc,
>   			git_interpret_trailers_usage,
>   			options);
>   
> +	trailer_config_init();
> +
>   	if (argc) {
>   		int i;
> -		for (i = 0; i < argc; i++)
> -			interpret_trailers(&opts, &trailers, argv[i]);
> +		for (i = 0; i < argc; i++) {
> +			struct strbuf in_buf = STRBUF_INIT;
> +			struct strbuf out_buf = STRBUF_INIT;
> +
> +			read_input_file(&in_buf, argv[i]);
> +			if (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)
> +				die(_("failed to process trailers for %s"), argv[i]);
> +			if (opts.in_place)
> +				write_file_buf(argv[i], out_buf.buf, out_buf.len);
> +			else
> +				fwrite(out_buf.buf, 1, out_buf.len, stdout);
> +			strbuf_release(&in_buf);
> +			strbuf_release(&out_buf);
> +		}
>   	} else {
> +		struct strbuf in_buf = STRBUF_INIT;
> +		struct strbuf out_buf = STRBUF_INIT;
> +
>   		if (opts.in_place)
>   			die(_("no input file given for in-place editing"));
> -		interpret_trailers(&opts, &trailers, NULL);
> +
> +		read_input_file(&in_buf, NULL);
> +		if (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)
> +			die(_("failed to process trailers"));
> +		fwrite(out_buf.buf, 1, out_buf.len, stdout);
> +		strbuf_release(&in_buf);
> +		strbuf_release(&out_buf);
>   	}

There is quite a bit of duplication here that could be avoided if you 
modified interpret_trailers() to call trailer_process() rather than 
deleting it entirely.

>   	new_trailers_clear(&trailers);
> diff --git a/trailer.c b/trailer.c
> index 310cf582dc..03814443c3 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1224,14 +1224,121 @@ void trailer_iterator_release(struct trailer_iterator *iter)
>   	strbuf_release(&iter->key);
>   }
>   
> -int amend_file_with_trailers(const char *path, const struct strvec *trailer_args)
> +static int amend_strbuf_with_trailers(struct strbuf *buf,
> +				   const struct strvec *trailer_args)

Function argument declarations should be aligned

>   {
> -	struct child_process run_trailer = CHILD_PROCESS_INIT;
> -
> -	run_trailer.git_cmd = 1;
> -	strvec_pushl(&run_trailer.args, "interpret-trailers",
> -		     "--in-place", "--no-divider",
> -		     path, NULL);
> -	strvec_pushv(&run_trailer.args, trailer_args->v);
> -	return run_command(&run_trailer);
> +	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
> +	LIST_HEAD(new_trailer_head);
> +	struct strbuf out = STRBUF_INIT;
> +	size_t i;
> +
> +	opts.no_divider = 1;
> +
> +	for (i = 0; i < trailer_args->nr; i++) {
> +		const char *arg = trailer_args->v[i];
> +		const char *text;
> +		struct new_trailer_item *item;

There should be a blank line after the variable declarations at the 
start of each block of code.

> +		if (!skip_prefix(arg, "--trailer=", &text))

Why do we need this? It would be much cleaner if we required the caller 
to pass a list of trailers without any optional prefix.

> +			text = arg;
> +		if (!*text)
> +			continue;
> +		item = xcalloc(1, sizeof(*item));
> +		INIT_LIST_HEAD(&item->list);
> +		item->text = text;
> +		list_add_tail(&item->list, &new_trailer_head);
> +	}
> +	if (trailer_process(&opts, buf->buf, &new_trailer_head, &out) < 0)
> +		die("failed to process trailers");

As this is library code lets return an error here rather than dying.

> +	strbuf_swap(buf, &out);
> +	strbuf_release(&out);
> +	while (!list_empty(&new_trailer_head)) {
> +		struct new_trailer_item *item =
> +			list_first_entry(&new_trailer_head, struct new_trailer_item, list);
> +		list_del(&item->list);
> +		free(item);
> +	}
> +	return 0;
>   }
> +
> +int trailer_process(const struct process_trailer_options *opts,
> +				   const char *msg,
> +				   struct list_head *new_trailer_head,
> +				   struct strbuf *out)

Argument alignment again

> +{
> +		struct trailer_block *blk;

This is trailer_block in the original but has been re-ordered with 
respect to the other variable declarations making the patch harder to 
review.

> +		LIST_HEAD(orig_head);

This is head in the original but moved relative to the other variable 
declarations

> +		LIST_HEAD(config_head);
> +		LIST_HEAD(ar1g_head);

These two have been moved from inside the if (!opts->only_input) below. 
They are only referenced there so do not need to be declared here. 
Moving them makes this patch harder to review.

> +		struct strbuf trailers_sb = STRBUF_INIT;

This is from the original but moved relative to the other variable 
declarations.

> +		int had_trailer_before;

This is new - lets see how it is used. We've just started using bool for 
boolean variables in the last few weeks so this could be a bool now.

 From here to

> +		blk = parse_trailers(opts, msg, &orig_head);
> +		had_trailer_before = !list_empty(&orig_head);
> +		if (!opts->only_input) {
> +			parse_trailers_from_config(&config_head);
> +			parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
> +			list_splice(&config_head, &arg_head);
> +			process_trailers_lists(&orig_head, &arg_head);
> +		}
> +		format_trailers(opts, &orig_head, &trailers_sb);

here is copied from the original minus the code that copied the commit 
message to the output file. Rather than deleting the code that copied 
the commit message we could have replaced the calls to fwrite() and 
fprintf() with strbuf_add() and strbuf_addf() which would make it 
obvious that the behavior is not changed. The original then frees 
orig_head but that is done later here.

> +		if (!opts->only_trailers && !opts->only_input && !opts->unfold &&
> +			!opts->trim_empty && list_empty(&orig_head) &&
> +			(list_empty(new_trailer_head) || opts->only_input)) {

I'm not sure what is happening here. By this point the original has 
copied the original commit message and is ready to append the new 
trailers. Instead the new version seems to have completely refactored 
the logic for adding the new trailers making it harder to see if the 
behavior has changed.

> +			size_t split = trailer_block_start(blk); /* end-of-log-msg */
> +			if (!blank_line_before_trailer_block(blk)) {
> +				strbuf_add(out, msg, split);
> +				strbuf_addch(out, '\n');
> +				strbuf_addstr(out, msg + split);

This copies the original message but adds a newline before the trailer 
block if it is missing.

> +			} else
> +				strbuf_addstr(out, msg);

This just copies the whole message.

> +			strbuf_rel2ease(&trailers_sb);
> +			trailer_block_release(blk);


> +			return 0;

We return a copy of the original message with no new trailers added. We 
do not free orig_head, arg_head or config_head. I'm still confused why 
we need to special case this.


> +		}
> +		if (opts->only_trailers) {
> +			strbuf_addbuf(out, &trailers_sb);

This flips the logic in the original to handle opts->only_trailers 
separately making it harder to review.

> +		} else if (had_trailer_before) {
> +			strbuf_add(out, msg, trailer_block_start(blk));
> +			if (!blank_line_before_trailer_block(blk))
> +				strbuf_addch(out, '\n');
> +			strbuf_addbuf(out, &trailers_sb);
> +			strbuf_add(out, msg + trailer_block_end(blk),
> +						strlen(msg) - trailer_block_end(blk));

This handles the case where we're replacing the headers in the original 
message

> +		}
> +		else {

Style - this should be "} else {"

> +			size_t cpos = trailer_block_start(blk);
> +			strbuf_add(out, msg, cpos);
> +			if (cpos == 0)                     /* empty body → just one \n */
> +				strbuf_addch(out, '\n');
> +			else if (!blank_line_before_trailer_block(blk))
> +				strbuf_addch(out, '\n');   /* body without trailing blank */
> +
> +			strbuf_addbuf(out, &trailers_sb);
> +			strbuf_add(out, msg + cpos, strlen(msg) - cpos);
> +	   }

I'm confused why we need a separate case for when the original did not 
have any trailers - was the original code broken? If it was we should 
separate out the bug fix from the refactoring. If not what's the point 
of this change?

> +		strbuf_release(&trailers_sb);
> +		free_trailers(&orig_head);
> +		trailer_block_release(blk);
> +		return 0;
> +}
> +
> +int amend_file_with_trailers(const char *path,
> +							 const struct strvec *trailer_args)

Alignment again

> +{
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	if (!trailer_args || !trailer_args->nr)
> +		return 0;
> +
> +	if (strbuf_read_file(&buf, path, 0) < 0)
> +		return error_errno("could not read '%s'", path);
> +
> +	if (amend_strbuf_with_trailers(&buf, trailer_args))
> +		die("failed to append trailers");

Why return an error() above but die() here? This is library code so lets 
return an error.

> +
> +	/* `write_file_buf()` aborts on error internally */
> +	write_file_buf(path, buf.buf, buf.len);

Dying here is a change in behavior which callers might not be expecting. 
The original code always returned a error because it forked a 
sub-process to do the trailer processing. Ideally, in a separate commit, 
we'd update any existing callers that have the message in an strbuf so 
they don't have to write it to a file just to add some trailers to it.

> +	strbuf_release(&buf);
> +	return 0;
> + }

As I said above reusing the existing code as you have done here is a 
much better approach. However it would be much easier to review if the 
code movement was separated from the refactoring. I'm also struggling to 
see the benefit of a lot of the refactoring - I was expecting the 
conversion to use an strubf would essentially look like fwrite() being 
replaced with strbuf_add() and fprintf() being replaced with 
strbuf_addf() etc. rather than reworking the logic.

Thanks


Phillip

> diff --git a/trailer.h b/trailer.h
> index 4740549586..01f711fb13 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -196,10 +196,22 @@ int trailer_iterator_advance(struct trailer_iterator *iter);
>   void trailer_iterator_release(struct trailer_iterator *iter);
>   
>   /*
> - * Augment a file to add trailers to it by running git-interpret-trailers.
> - * This calls run_command() and its return value is the same (i.e. 0 for
> - * success, various non-zero for other errors). See run-command.h.
> + * 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.
>    */
>   int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);
>   
> +/*
> + * Process trailer lines for a commit message in-memory.
> + * @opts: trailer processing options (e.g. from parse-options)
> + * @msg: the input message string
> + * @new_trailer_head: list of new trailers to add (struct new_trailer_item)
> + * @out: strbuf to store the resulting message (must be initialized)
> + *
> + * Returns 0 on success, <0 on error.
> + */
> +int trailer_process(const struct process_trailer_options *opts,
> +			const char *msg,
> +			struct list_head *new_trailer_head,
> +			struct strbuf *out);
>   #endif /* TRAILER_H */

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

[Phillip Wood]

On 03/08/2025 16:00, 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.

We only want to reject it if the user passes an option that requires the 
apply backend, otherwise we can just use the merge backend.

> Automatically set REBASE_FORCE when
> any trailer is supplied.

Makes sense
> And reject invalid input before user edit the interactive file.

> +--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.

The options like --reset-author-date just say "implies `--force-rebase`"

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index e90562a3b8..3b4c45a616 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -36,6 +36,8 @@
>   #include "reset.h"
>   #include "trace2.h"
>   #include "hook.h"
> +#include "trailer.h"
> +#include "parse-options.h"

"parse-options.h" is already included.
> @@ -435,6 +444,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;
> +	const char trailer_state_name[] = "trailer";
> +	const char *path = state_dir_path(trailer_state_name, opts);

We only use this once, so lets pass state_dir_path("trailer", opts) 
directly to strbuf_read_file() like we do when reading all the other 
state files in this function.

>   	if (!read_oneliner(&head_name, state_dir_path("head-name", opts),
>   			   READ_ONELINER_WARN_MISSING) ||
> @@ -503,11 +514,31 @@ static int read_basic_state(struct rebase_options *opts)
>   
>   	strbuf_release(&buf);

This should become strbuf_reset(&buf) so that we can reuse the allocation.

> +	if (strbuf_read_file(&buf, path, 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)

This is needed because we allow an empty file but reading an empty file 
is a bug because we only write this file if the user gave --trailers on 
the command line and we should reject empty trailers before getting here.

> +				strvec_push(&opts->trailer_args, p);
> +
> +			p = nl + 1;
> +		}
> +		strbuf_release(&buf);

This is unnecessary, it is released below which is a better place as it 
means the buffer is freed even if strbuf_read_file() fails.

> +	}
> +	strbuf_release(&buf);
> +
>   	return 0;
>   }
>   
>   static int rebase_write_basic_state(struct rebase_options *opts)
>   {
> +	const char trailer_state_name[] = "trailer";

This is unnecessary, lets follow the existing style of just using the 
filename directly in the one place it is needed.

> +    /*
> +     * 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++) {

We can use "for (size_t i = 0; i < ...) {" here

> +                    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);
> +    }
> +
>   	return 0;
>   }

This looks good
> +static int validate_trailer_args_after_config(const struct strvec *cli_args,
> +				       struct strbuf *err)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < cli_args->nr; i++) {

As i is only used in the loop we can say "for (size_t i = 0; ...) {" and 
drop the declaration above.

> +		const char *raw = cli_args->v[i];
> +		const char *txt; // Key[:=]Val
> +		const char *sep;
> +
> +		if (!skip_prefix(raw, "--trailer=", &txt))
> +			txt = raw;

OPT_STRVEC() only stores the argument, not the option name so we don't 
need this

> +
> +		if (!*txt) {
> +			strbuf_addstr(err, _("empty --trailer argument"));
> +			return -1;

I'm not sure we need the err buf here - we can just

	return error(_("empty --trailer argument"));

> +		}
> +
> +		sep = strpbrk(txt, ":=");

The list of separators is configurable - if we move this function into 
trailer.c then we can use find_separator().

> +
> +		/* there must be key bfore seperator */
> +		if (sep && sep == txt) {
> +			strbuf_addf(err,
> +				    _("invalid trailer '%s': missing key before separator"),
> +				    txt);

We can use return error(...) here as well.

> @@ -1133,6 +1211,7 @@ 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 is rather long, can we fold it like the one below please.

>   		OPT_BOOL(0, "signoff", &options.signoff,
>   			 N_("add a Signed-off-by trailer to each commit")),
>   		OPT_BOOL(0, "committer-date-is-author-date",
> @@ -1283,6 +1362,17 @@ int cmd_rebase(int argc,
>   			     builtin_rebase_options,
>   			     builtin_rebase_usage, 0);
>   
> +    /* if add --trailer，force rebase */

I'm not sure what "add --trailer" means. Forcing the rebase when 
--trailer is given is a good idea.

> +	if (options.trailer_args.nr) {
> +        struct strbuf err = STRBUF_INIT;

The indentation in this block is off here and below

> +		if (validate_trailer_args_after_config(&options.trailer_args, &err))
> +			die("%s", err.buf);
> +
> +        options.flags |= REBASE_FORCE;
> +        strbuf_release(&err);
> +	}

> +	/*
> +	 * 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.
> +	 */

This comment is a bit confusing - what are we rejecting here? The call 
to imply_merge() is correct.

> +	if (options.trailer_args.nr)
> +		imply_merge(&options, "--trailer");
> +
>   	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 67e4310edc..58faf6aed5 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -422,6 +422,7 @@ void replay_opts_release(struct replay_opts *opts)
>   	free(opts->revs);
>   	replay_ctx_release(ctx);
>   	free(opts->ctx);
> +	strvec_clear(&opts->trailer_args);

Lets move this up a bit before the ctx stuff as that should the last 
member of the struct.

> @@ -2529,6 +2530,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 */

This feels a bit late in the function to be doing this. Can be add the 
trailers just after we add the sign off around line 2457. That way we 
can add the trailers to ctx->message before writing the commit message 
file. Like --signoff we also need to make sure we don't add trailers 
when we're processing a fixup or squash command.

> +    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, reflog_action,
> diff --git a/sequencer.h b/sequencer.h
> index 304ba4b4d3..28f2da6375 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -4,6 +4,7 @@
>   #include "strbuf.h"
>   #include "strvec.h"
>   #include "wt-status.h"
> +#include <stddef.h>

We never add system headers like this. git-compat-util.h takes care of 
that in a portable manner.

>   struct commit;
>   struct index_state;
> @@ -44,6 +45,7 @@ struct replay_opts {
>   	int record_origin;
>   	int no_commit;
>   	int signoff;
> +	struct strvec trailer_args;
>   	int allow_ff;
>   	int allow_rerere_auto;
>   	int allow_empty;
> @@ -86,6 +88,7 @@ struct replay_opts {
>   	.action = -1,				\
>   	.xopts = STRVEC_INIT,			\
>   	.ctx = replay_ctx_new(),		\
> +	.trailer_args = STRVEC_INIT, \
>   }

Lets keep the order of members in the initializer the same as in the 
struct declaration.

I've run out of time for today, I'll take a look at the tests later in 
the week. Although I've left quite a few comments the code in this patch 
looks basically sound.

Thanks

Phillip
>   /*
> diff --git a/t/meson.build b/t/meson.build
> index 09f3068f98..3c58f562da 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -373,6 +373,7 @@ integration_tests = [
>     't3436-rebase-more-options.sh',
>     't3437-rebase-fixup-options.sh',
>     't3438-rebase-broken-files.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..a580449628
> --- /dev/null
> +++ b/t/t3440-rebase-trailer.sh
> @@ -0,0 +1,95 @@
> +#!/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
> +
> +create_expect() {
> +	cat >"$1" <<-EOF
> +		$2
> +
> +		Reviewed-by: Dev <dev@example.com>
> +	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 &&
> +	ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" &&
> +	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"
> +'
> +
> +test_expect_success 'apply backend is rejected with --trailer' '
> +	git reset --hard third &&
> +	head_before=$(git rev-parse HEAD) &&
> +    test_expect_code 128 \
> +    		git rebase --apply --trailer "Reviewed-by: Dev <dev@example.com>" \
> +    			HEAD^ 2>err &&
> +	test_grep "requires the merge backend" err &&
> +	test_cmp_rev HEAD $head_before
> +'
> +
> +test_expect_success 'reject empty --trailer argument' '
> +        git reset --hard third &&
> +        test_expect_code 128 git rebase -m --trailer "" HEAD^ 2>err &&
> +        test_grep "empty --trailer" err
> +'
> +
> +test_expect_success 'reject trailer with missing key before separator' '
> +        git reset --hard third &&
> +        test_expect_code 128 git rebase -m --trailer ": no-key" HEAD^ 2>err &&
> +        test_grep "missing key before separator" err
> +'
> +
> +test_expect_success 'CLI trailer duplicates allowed; replace policy keeps last' '
> +        git reset --hard third &&
> +        git -c trailer.Bug.ifexists=replace -c trailer.Bug.ifmissing=add rebase -m --trailer "Bug: 123" --trailer "Bug: 456" HEAD~1 &&
> +        git cat-file commit HEAD | grep "^Bug: 456" &&
> +        git cat-file commit HEAD | grep -v "^Bug: 123"
> +'
> +
> +test_expect_success 'multiple Signed-off-by trailers all preserved' '
> +        git reset --hard third &&
> +        git rebase -m \
> +            --trailer "Signed-off-by: Dev A <a@ex.com>" \
> +            --trailer "Signed-off-by: Dev B <b@ex.com>" HEAD~1 &&
> +        git cat-file commit HEAD | grep -c "^Signed-off-by:" >count &&
> +        test "$(cat count)" = 2   # two new commits
> +'
> +
> +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 rebase --continue &&
> +	test_commit_message HEAD~2 file2-signed
> +'
> +
> +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

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

[Phillip Wood]

Hi Li

On 03/08/2025 16:00, Li Chen wrote:
> From: Li Chen <chenl311@chinatelecom.cn>
Picking up from where I left off yesterday ...

> diff --git a/t/meson.build b/t/meson.build
> index 09f3068f98..3c58f562da 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -373,6 +373,7 @@ integration_tests = [
>     't3436-rebase-more-options.sh',
>     't3437-rebase-fixup-options.sh',
>     't3438-rebase-broken-files.sh',
> +  't3440-rebase-trailer.sh',

The alignment looks off here

>     '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..a580449628
> --- /dev/null
> +++ b/t/t3440-rebase-trailer.sh
> @@ -0,0 +1,95 @@
> +#!/bin/sh
> +#
> +
> +test_description='git rebase --trailer integration tests
> +We verify that --trailer on the merge/interactive/exec/root backends,

There are only two backends "apply" and "merge", the other things you 
have listed are just command line options. We don't actually test --exec 
or --interactive with --trailer in this file so we should reword that 
comment. There is no need to add tests for those.

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

Normally we create the "expect" file in the test where it is used.

> +'
> +
> +test_expect_success 'apply backend is rejected with --trailer' '

We know HEAD is at third so we don't need this

> +	git reset --hard third &&
> +	head_before=$(git rev-parse HEAD) &&
> +    test_expect_code 128 \

The indentation is off here

> +    		git rebase --apply --trailer "Reviewed-by: Dev <dev@example.com>" \
> +    			HEAD^ 2>err &&
> +	test_grep "requires the merge backend" err &&

I think it is worth checking that the error message includes --trailer 
as well to make sure that is the option that is triggering the error.

> +	test_cmp_rev HEAD $head_before
> +'
> +
> +test_expect_success 'reject empty --trailer argument' '
> +        git reset --hard third &&

The exact commit is not important here

> +        test_expect_code 128 git rebase -m --trailer "" HEAD^ 2>err &&
> +        test_grep "empty --trailer" err
> +'
> +
> +test_expect_success 'reject trailer with missing key before separator' '
> +        git reset --hard third &&

Same here - we're only checking the error message so the exact value of 
HEAD is not important.

> +        test_expect_code 128 git rebase -m --trailer ": no-key" HEAD^ 2>err &&
> +        test_grep "missing key before separator" err
> +'
> +
> +test_expect_success 'CLI trailer duplicates allowed; replace policy keeps last' '
> +        git reset --hard third &&
> +        git -c trailer.Bug.ifexists=replace -c trailer.Bug.ifmissing=add rebase -m --trailer "Bug: 123" --trailer "Bug: 456" HEAD~1 &&
> +        git cat-file commit HEAD | grep "^Bug: 456" &&
> +        git cat-file commit HEAD | grep -v "^Bug: 123"

Piping git into another command is discouraged as git can potentially 
fail without us noticing. Here I think it would be better to use 
test_commit_message() to check the whole message.
> +'
> +
> +test_expect_success 'multiple Signed-off-by trailers all preserved' '
> +        git reset --hard third &&

We can avoid this by passing the commit we want to checkout to rebase as 
you do in the test below.

> +        git rebase -m \
> +            --trailer "Signed-off-by: Dev A <a@ex.com>" \
> +            --trailer "Signed-off-by: Dev B <b@ex.com>" HEAD~1 &&
> +        git cat-file commit HEAD | grep -c "^Signed-off-by:" >count &&
> +        test "$(cat count)" = 2   # two new commits

Let check the actual message here as well so if it fails we can see what 
the message is.

> +'
> +
> +test_expect_success 'rebase -m --trailer adds trailer after conflicts' '
> +	git reset --hard third &&

We don't need this as the rebase command checks out third for us, saving 
a process which is always nice.

> +	test_must_fail git rebase -m \
> +		--trailer "Reviewed-by: Dev <dev@example.com>" \
> +		second third &&
> +	git checkout --theirs file &&
> +	git add file &&
> +    git rebase --continue &&

The indentation is off here

> +	test_commit_message HEAD~2 file2-signed

It's good to see this uses test_commit_message but why are we checking 
HEAD~2 rather than HEAD?> +'
> +
> +test_expect_success 'rebase --root --trailer updates every commit' '
> +	git checkout first &&
> +	git rebase --root --keep-empty \

--keep-empty is the default these days so I think we can drop that.

> +		--trailer "Reviewed-by: Dev <dev@example.com>" &&
> +	test_commit_message HEAD   first-signed &&
> +	test_commit_message HEAD^  initial-signed

Looks good.

While there are some small issues to fix the tests look sensible and I 
think you have good coverage of the new option.

Thanks

Phillip

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

[Phillip Wood]

Hi Li

I had a couple more thoughts about the tests ...

On 06/08/2025 11:28, Phillip Wood wrote:
> On 03/08/2025 16:00, Li Chen wrote:
>> +create_expect() {
>> +    cat >"$1" <<-EOF
>> +        $2
>> +
>> +        Reviewed-by: Dev <dev@example.com>
>> +    EOF
>> +}
>> +
>> +test_expect_success 'setup repo with a small history' '
>> [...]
 >> +    create_expect third-signed    "third"                 &&>> +    
create_expect conflict-signed "conflict"
> 
> Normally we create the "expect" file in the test where it is used.

Thinking about this some more, if we want to use test_commit_message 
then I think we can change create_expect to write to stdout and do

	test_commit_message HEAD <<-EOF
	$(create_expect first)
	EOF

rather than having to create a file.

>> +
>> +test_expect_success 'reject empty --trailer argument' '
>> [...]
>> +test_expect_success 'reject trailer with missing key before separator' '

Should we also test for a missing value or are trailers without a value 
allowed?

>> +        git rebase -m \
>> +            --trailer "Signed-off-by: Dev A <a@ex.com>" \
>> +            --trailer "Signed-off-by: Dev B <b@ex.com>" HEAD~1 &&

Lets use example.com here rather than some random domain that might 
actually exist.

>> +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 rebase --continue &&

This checks that the commit with conflicts has a trailer added but it 
does not check that the commits picked by "git rebase --continue" do. To 
check that we actually save the trailers and use them when continuing we 
need to add a fourth commit on top of third and check that has a trailer 
add here as well.

A couple more thoughts:

  - We should check that
      git -c trailer.review.key=Reviewed-by rebase \
          --trailer=review="Dev <dev@example.com>"
    adds a "Reviewed-by:" trailer. We can do that by changing one of the
    tests in this patch rather than adding a new one. This checks that we
    accept '=' as a separator as well a respecting the config.

  - We should check that the todo list
      pick first
      fixup second
    adds the trailer as expected and that
      pick first
      fixup -C second
    also works. To do that we will need to source lib-rebase.sh at the
    start of the test file and add a test that uses set_replace_editor()
    which should be called in a subshell.


Do please ask if you have any questions about these suggestions

Thanks

Phillip

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

[Li Chen]

Hi Phillip, 

Thanks for your thorough review; I will address them in the next version.

 ---- On Wed, 06 Aug 2025 21:19:57 +0800  Phillip Wood <phillip.wood123@gmail.com> wrote --- 
 > Hi Li
 > 
 > I had a couple more thoughts about the tests ...
 > 
 > On 06/08/2025 11:28, Phillip Wood wrote:
 > > On 03/08/2025 16:00, Li Chen wrote:
 > >> +create_expect() {
 > >> +    cat >"$1" <<-EOF
 > >> +        $2
 > >> +
 > >> +        Reviewed-by: Dev <dev@example.com>
 > >> +    EOF
 > >> +}
 > >> +
 > >> +test_expect_success 'setup repo with a small history' '
 > >> [...]
 >  >> +    create_expect third-signed    "third"                 &&>> +    
 > create_expect conflict-signed "conflict"
 > > 
 > > Normally we create the "expect" file in the test where it is used.
 > 
 > Thinking about this some more, if we want to use test_commit_message 
 > then I think we can change create_expect to write to stdout and do
 > 
 >     test_commit_message HEAD <<-EOF
 >     $(create_expect first)
 >     EOF
 > 
 > rather than having to create a file.
 > 
 > >> +
 > >> +test_expect_success 'reject empty --trailer argument' '
 > >> [...]
 > >> +test_expect_success 'reject trailer with missing key before separator' '
 > 
 > Should we also test for a missing value or are trailers without a value 
 > allowed?
 > 
 > >> +        git rebase -m \
 > >> +            --trailer "Signed-off-by: Dev A <a@ex.com>" \
 > >> +            --trailer "Signed-off-by: Dev B <b@ex.com>" HEAD~1 &&
 > 
 > Lets use example.com here rather than some random domain that might 
 > actually exist.
 > 
 > >> +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 rebase --continue &&
 > 
 > This checks that the commit with conflicts has a trailer added but it 
 > does not check that the commits picked by "git rebase --continue" do. To 
 > check that we actually save the trailers and use them when continuing we 
 > need to add a fourth commit on top of third and check that has a trailer 
 > add here as well.
 > 
 > A couple more thoughts:
 > 
 >   - We should check that
 >       git -c trailer.review.key=Reviewed-by rebase \
 >           --trailer=review="Dev <dev@example.com>"
 >     adds a "Reviewed-by:" trailer. We can do that by changing one of the
 >     tests in this patch rather than adding a new one. This checks that we
 >     accept '=' as a separator as well a respecting the config.
 > 
 >   - We should check that the todo list
 >       pick first
 >       fixup second
 >     adds the trailer as expected and that
 >       pick first
 >       fixup -C second
 >     also works. To do that we will need to source lib-rebase.sh at the
 >     start of the test file and add a test that uses set_replace_editor()
 >     which should be called in a subshell.
 > 
 > 
 > Do please ask if you have any questions about these suggestions

Regards,

Li​

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

[Li Chen]

 ---- On Wed, 06 Aug 2025 21:19:57 +0800  Phillip Wood <phillip.wood123@gmail.com> wrote --- 
 > Hi Li
 > 
 > I had a couple more thoughts about the tests ...
 > 
 > On 06/08/2025 11:28, Phillip Wood wrote:
 > > On 03/08/2025 16:00, Li Chen wrote:
 > >> +create_expect() {
 > >> +    cat >"$1" <<-EOF
 > >> +        $2
 > >> +
 > >> +        Reviewed-by: Dev <dev@example.com>
 > >> +    EOF
 > >> +}
 > >> +
 > >> +test_expect_success 'setup repo with a small history' '
 > >> [...]
 >  >> +    create_expect third-signed    "third"                 &&>> +    
 > create_expect conflict-signed "conflict"
 > > 
 > > Normally we create the "expect" file in the test where it is used.
 > 
 > Thinking about this some more, if we want to use test_commit_message 
 > then I think we can change create_expect to write to stdout and do
 > 
 >     test_commit_message HEAD <<-EOF
 >     $(create_expect first)
 >     EOF
 > 
 > rather than having to create a file.
 > 
 > >> +
 > >> +test_expect_success 'reject empty --trailer argument' '
 > >> [...]
 > >> +test_expect_success 'reject trailer with missing key before separator' '
 > 
 > Should we also test for a missing value or are trailers without a value 
 > allowed?
 > 
 > >> +        git rebase -m \
 > >> +            --trailer "Signed-off-by: Dev A <a@ex.com>" \
 > >> +            --trailer "Signed-off-by: Dev B <b@ex.com>" HEAD~1 &&
 > 
 > Lets use example.com here rather than some random domain that might 
 > actually exist.
 > 
 > >> +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 rebase --continue &&
 > 
 > This checks that the commit with conflicts has a trailer added but it 
 > does not check that the commits picked by "git rebase --continue" do. To 
 > check that we actually save the trailers and use them when continuing we 
 > need to add a fourth commit on top of third and check that has a trailer 
 > add here as well.
 > 
 > A couple more thoughts:
 > 
 >   - We should check that
 >       git -c trailer.review.key=Reviewed-by rebase \
 >           --trailer=review="Dev <dev@example.com>"
 >     adds a "Reviewed-by:" trailer. We can do that by changing one of the
 >     tests in this patch rather than adding a new one. This checks that we
 >     accept '=' as a separator as well a respecting the config.
 > 
 >   - We should check that the todo list
 >       pick first
 >       fixup second
 >     adds the trailer as expected and that
 >       pick first
 >       fixup -C second
 >     also works. To do that we will need to source lib-rebase.sh at the
 >     start of the test file and add a test that uses set_replace_editor()
 >     which should be called in a subshell.
 > 
 > 
 > Do please ask if you have any questions about these suggestions
 > 
 > Thanks
 > 
 > Phillip
 > 
 > 
Regards,

Li​

Re: [PATCH v3 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers`

[Li Chen]

Hi Phillip,

 ---- On Tue, 05 Aug 2025 21:17:01 +0800  Phillip Wood <phillip.wood123@gmail.com> wrote --- 
 > Hi Li
 > 
 > On 03/08/2025 16:00, Li Chen wrote:
 > > From: Li Chen <chenl311@chinatelecom.cn>
 > > 
 > > All trailer insertion now funnels through trailer_process():
 > > 
 > > * builtin/interpret-trailers.c is reduced to file I/O + a single call.
 > > * amend_file_with_trailers() shares the same path; the old
 > >    amend_strbuf_with_trailers() helper is dropped.
 > > * New helpers parse_trailer_args()/free_new_trailer_list() convert
 > >    --trailer=... strings to new_trailer_item lists.
 > > 
 > > Behaviour is unchanged; the full test-suite still passes, and the
 > > fork/exec is gone.
 > 
 > Normally commit messages should be written in prose rather than a bullet 
 > list and the message should explain the reason for the change.
 > 
 > This patch has much less code duplication than the last iteration which 
 > is most welcome. Whenever you are moving and refactoring code you should 
 > split the move into its own commit followed by the refactoring. That 
 > makes it much easier to review as the reviewer can clearly see the 
 > refactoring rather than having to manually compare the added code in one 
 > file to the deleted code in another.
 
I apologize for this, and I will add new commits to resolve all issues in the next versions.
 
 > > @@ -84,6 +83,7 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
 > >                  int unset)
 > >   {
 > >       struct process_trailer_options *v = opt->value;
 > > +
 > 
 > Let's not clutter this patch with unrelated changes.
 > 
 > >       v->only_trailers = 1;
 > >       v->only_input = 1;
 > >       v->unfold = 1;
 > > @@ -92,37 +92,6 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
 > >       return 0;
 > >   }
 > >   
 > > -static FILE *create_in_place_tempfile(const char *file)
 > > -{
 > > [...]
 > > -}
 > 
 > We don't need to create a temporary file anymore so this can be deleted 
 > - good.
 > 
 > > -static void interpret_trailers(const struct process_trailer_options *opts,
 > > -                   struct list_head *new_trailer_head,
 > > -                   const char *file)
 > > -{
 > > -    LIST_HEAD(head);
 > > -    struct strbuf sb = STRBUF_INIT;
 > > -    struct strbuf trailer_block_sb = STRBUF_INIT;
 > > -    struct trailer_block *trailer_block;
 > > -    FILE *outfile = stdout;
 > > -
 > > -    trailer_config_init();
 > > -
 > > -    read_input_file(&sb, file);
 > > -
 > > -    if (opts->in_place)
 > > -        outfile = create_in_place_tempfile(file);
 > > -
 > > -    trailer_block = parse_trailers(opts, sb.buf, &head);
 > > -
 > > -    /* Print the lines before the trailer block */
 > > -    if (!opts->only_trailers)
 > > -        fwrite(sb.buf, 1, trailer_block_start(trailer_block), outfile);
 > > -
 > > -    if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
 > > -        fprintf(outfile, "\n");
 > > -
 > > -
 > > -    if (!opts->only_input) {
 > > -        LIST_HEAD(config_head);
 > > -        LIST_HEAD(arg_head);
 > > -        parse_trailers_from_config(&config_head);
 > > -        parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
 > > -        list_splice(&config_head, &arg_head);
 > > -        process_trailers_lists(&head, &arg_head);
 > > -    }
 > > -
 > > -    /* Print trailer block. */
 > > -    format_trailers(opts, &head, &trailer_block_sb);
 > > -    free_trailers(&head);
 > > -    fwrite(trailer_block_sb.buf, 1, trailer_block_sb.len, outfile);
 > > -    strbuf_release(&trailer_block_sb);
 > > -
 > > -    /* Print the lines after the trailer block as is. */
 > > -    if (!opts->only_trailers)
 > > -        fwrite(sb.buf + trailer_block_end(trailer_block), 1,
 > > -               sb.len - trailer_block_end(trailer_block), outfile);
 > > -    trailer_block_release(trailer_block);
 > > -
 > > -    if (opts->in_place)
 > > -        if (rename_tempfile(&trailers_tempfile, file))
 > > -            die_errno(_("could not rename temporary file to %s"), file);
 > > -
 > > -    strbuf_release(&sb);
 > > -}
 > 
 > This code is moved to trailer.c which is good but it is heavily 
 > refactored at the same time which makes it hard to review. Completely 
 > removing this function leads to some duplication in 
 > cmd_interpret_trailers() which could be avoided by making 
 > interpret_trailers() a wrapper around process_trailers()
 > 
 > >   int cmd_interpret_trailers(int argc,
 > >                  const char **argv,
 > >                  const char *prefix,
 > > @@ -231,14 +145,37 @@ int cmd_interpret_trailers(int argc,
 > >               git_interpret_trailers_usage,
 > >               options);
 > >   
 > > +    trailer_config_init();
 > > +
 > >       if (argc) {
 > >           int i;
 > > -        for (i = 0; i < argc; i++)
 > > -            interpret_trailers(&opts, &trailers, argv[i]);
 > > +        for (i = 0; i < argc; i++) {
 > > +            struct strbuf in_buf = STRBUF_INIT;
 > > +            struct strbuf out_buf = STRBUF_INIT;
 > > +
 > > +            read_input_file(&in_buf, argv[i]);
 > > +            if (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)
 > > +                die(_("failed to process trailers for %s"), argv[i]);
 > > +            if (opts.in_place)
 > > +                write_file_buf(argv[i], out_buf.buf, out_buf.len);
 > > +            else
 > > +                fwrite(out_buf.buf, 1, out_buf.len, stdout);
 > > +            strbuf_release(&in_buf);
 > > +            strbuf_release(&out_buf);
 > > +        }
 > >       } else {
 > > +        struct strbuf in_buf = STRBUF_INIT;
 > > +        struct strbuf out_buf = STRBUF_INIT;
 > > +
 > >           if (opts.in_place)
 > >               die(_("no input file given for in-place editing"));
 > > -        interpret_trailers(&opts, &trailers, NULL);
 > > +
 > > +        read_input_file(&in_buf, NULL);
 > > +        if (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)
 > > +            die(_("failed to process trailers"));
 > > +        fwrite(out_buf.buf, 1, out_buf.len, stdout);
 > > +        strbuf_release(&in_buf);
 > > +        strbuf_release(&out_buf);
 > >       }
 > 
 > There is quite a bit of duplication here that could be avoided if you 
 > modified interpret_trailers() to call trailer_process() rather than 
 > deleting it entirely.
 > 
 > >       new_trailers_clear(&trailers);
 > > diff --git a/trailer.c b/trailer.c
 > > index 310cf582dc..03814443c3 100644
 > > --- a/trailer.c
 > > +++ b/trailer.c
 > > @@ -1224,14 +1224,121 @@ void trailer_iterator_release(struct trailer_iterator *iter)
 > >       strbuf_release(&iter->key);
 > >   }
 > >   
 > > -int amend_file_with_trailers(const char *path, const struct strvec *trailer_args)
 > > +static int amend_strbuf_with_trailers(struct strbuf *buf,
 > > +                   const struct strvec *trailer_args)
 > 
 > Function argument declarations should be aligned
 > 
 > >   {
 > > -    struct child_process run_trailer = CHILD_PROCESS_INIT;
 > > -
 > > -    run_trailer.git_cmd = 1;
 > > -    strvec_pushl(&run_trailer.args, "interpret-trailers",
 > > -             "--in-place", "--no-divider",
 > > -             path, NULL);
 > > -    strvec_pushv(&run_trailer.args, trailer_args->v);
 > > -    return run_command(&run_trailer);
 > > +    struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
 > > +    LIST_HEAD(new_trailer_head);
 > > +    struct strbuf out = STRBUF_INIT;
 > > +    size_t i;
 > > +
 > > +    opts.no_divider = 1;
 > > +
 > > +    for (i = 0; i < trailer_args->nr; i++) {
 > > +        const char *arg = trailer_args->v[i];
 > > +        const char *text;
 > > +        struct new_trailer_item *item;
 > 
 > There should be a blank line after the variable declarations at the 
 > start of each block of code.
 > 
 > > +        if (!skip_prefix(arg, "--trailer=", &text))
 > 
 > Why do we need this? It would be much cleaner if we required the caller 
 > to pass a list of trailers without any optional prefix.
 > 
 > > +            text = arg;
 > > +        if (!*text)
 > > +            continue;
 > > +        item = xcalloc(1, sizeof(*item));
 > > +        INIT_LIST_HEAD(&item->list);
 > > +        item->text = text;
 > > +        list_add_tail(&item->list, &new_trailer_head);
 > > +    }
 > > +    if (trailer_process(&opts, buf->buf, &new_trailer_head, &out) < 0)
 > > +        die("failed to process trailers");
 > 
 > As this is library code lets return an error here rather than dying.
 > 
 > > +    strbuf_swap(buf, &out);
 > > +    strbuf_release(&out);
 > > +    while (!list_empty(&new_trailer_head)) {
 > > +        struct new_trailer_item *item =
 > > +            list_first_entry(&new_trailer_head, struct new_trailer_item, list);
 > > +        list_del(&item->list);
 > > +        free(item);
 > > +    }
 > > +    return 0;
 > >   }
 > > +
 > > +int trailer_process(const struct process_trailer_options *opts,
 > > +                   const char *msg,
 > > +                   struct list_head *new_trailer_head,
 > > +                   struct strbuf *out)
 > 
 > Argument alignment again
 > 
 > > +{
 > > +        struct trailer_block *blk;
 > 
 > This is trailer_block in the original but has been re-ordered with 
 > respect to the other variable declarations making the patch harder to 
 > review.
 > 
 > > +        LIST_HEAD(orig_head);
 > 
 > This is head in the original but moved relative to the other variable 
 > declarations
 > 
 > > +        LIST_HEAD(config_head);
 > > +        LIST_HEAD(ar1g_head);
 > 
 > These two have been moved from inside the if (!opts->only_input) below. 
 > They are only referenced there so do not need to be declared here. 
 > Moving them makes this patch harder to review.
 > 
 > > +        struct strbuf trailers_sb = STRBUF_INIT;
 > 
 > This is from the original but moved relative to the other variable 
 > declarations.
 > 
 > > +        int had_trailer_before;
 > 
 > This is new - lets see how it is used. We've just started using bool for 
 > boolean variables in the last few weeks so this could be a bool now.
 > 
 >  From here to
 > 
 > > +        blk = parse_trailers(opts, msg, &orig_head);
 > > +        had_trailer_before = !list_empty(&orig_head);
 > > +        if (!opts->only_input) {
 > > +            parse_trailers_from_config(&config_head);
 > > +            parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
 > > +            list_splice(&config_head, &arg_head);
 > > +            process_trailers_lists(&orig_head, &arg_head);
 > > +        }
 > > +        format_trailers(opts, &orig_head, &trailers_sb);
 > 
 > here is copied from the original minus the code that copied the commit 
 > message to the output file. Rather than deleting the code that copied 
 > the commit message we could have replaced the calls to fwrite() and 
 > fprintf() with strbuf_add() and strbuf_addf() which would make it 
 > obvious that the behavior is not changed. The original then frees 
 > orig_head but that is done later here.
 > 
 > > +        if (!opts->only_trailers && !opts->only_input && !opts->unfold &&
 > > +            !opts->trim_empty && list_empty(&orig_head) &&
 > > +            (list_empty(new_trailer_head) || opts->only_input)) {
 > 
 > I'm not sure what is happening here. By this point the original has 
 > copied the original commit message and is ready to append the new 
 > trailers. Instead the new version seems to have completely refactored 
 > the logic for adding the new trailers making it harder to see if the 
 > behavior has changed.
 > 
 > > +            size_t split = trailer_block_start(blk); /* end-of-log-msg */
 > > +            if (!blank_line_before_trailer_block(blk)) {
 > > +                strbuf_add(out, msg, split);
 > > +                strbuf_addch(out, '\n');
 > > +                strbuf_addstr(out, msg + split);
 > 
 > This copies the original message but adds a newline before the trailer 
 > block if it is missing.
 > 
 > > +            } else
 > > +                strbuf_addstr(out, msg);
 > 
 > This just copies the whole message.
 > 
 > > +            strbuf_rel2ease(&trailers_sb);
 > > +            trailer_block_release(blk);
 > 
 > 
 > > +            return 0;
 > 
 > We return a copy of the original message with no new trailers added. We 
 > do not free orig_head, arg_head or config_head. I'm still confused why 
 > we need to special case this.
 > 
 > 
 > > +        }
 > > +        if (opts->only_trailers) {
 > > +            strbuf_addbuf(out, &trailers_sb);
 > 
 > This flips the logic in the original to handle opts->only_trailers 
 > separately making it harder to review.
 > 
 > > +        } else if (had_trailer_before) {
 > > +            strbuf_add(out, msg, trailer_block_start(blk));
 > > +            if (!blank_line_before_trailer_block(blk))
 > > +                strbuf_addch(out, '\n');
 > > +            strbuf_addbuf(out, &trailers_sb);
 > > +            strbuf_add(out, msg + trailer_block_end(blk),
 > > +                        strlen(msg) - trailer_block_end(blk));
 > 
 > This handles the case where we're replacing the headers in the original 
 > message
 > 
 > > +        }
 > > +        else {
 > 
 > Style - this should be "} else {"
 > 
 > > +            size_t cpos = trailer_block_start(blk);
 > > +            strbuf_add(out, msg, cpos);
 > > +            if (cpos == 0)                     /* empty body → just one \n */
 > > +                strbuf_addch(out, '\n');
 > > +            else if (!blank_line_before_trailer_block(blk))
 > > +                strbuf_addch(out, '\n');   /* body without trailing blank */
 > > +
 > > +            strbuf_addbuf(out, &trailers_sb);
 > > +            strbuf_add(out, msg + cpos, strlen(msg) - cpos);
 > > +       }
 > 
 > I'm confused why we need a separate case for when the original did not 
 > have any trailers - was the original code broken? If it was we should 
 > separate out the bug fix from the refactoring. If not what's the point 
 > of this change?
 > 
 > > +        strbuf_release(&trailers_sb);
 > > +        free_trailers(&orig_head);
 > > +        trailer_block_release(blk);
 > > +        return 0;
 > > +}
 > > +
 > > +int amend_file_with_trailers(const char *path,
 > > +                             const struct strvec *trailer_args)
 > 
 > Alignment again
 > 
 > > +{
 > > +    struct strbuf buf = STRBUF_INIT;
 > > +
 > > +    if (!trailer_args || !trailer_args->nr)
 > > +        return 0;
 > > +
 > > +    if (strbuf_read_file(&buf, path, 0) < 0)
 > > +        return error_errno("could not read '%s'", path);
 > > +
 > > +    if (amend_strbuf_with_trailers(&buf, trailer_args))
 > > +        die("failed to append trailers");
 > 
 > Why return an error() above but die() here? This is library code so lets 
 > return an error.
 > 
 > > +
 > > +    /* `write_file_buf()` aborts on error internally */
 > > +    write_file_buf(path, buf.buf, buf.len);
 > 
 > Dying here is a change in behavior which callers might not be expecting. 
 > The original code always returned a error because it forked a 
 > sub-process to do the trailer processing. Ideally, in a separate commit, 
 > we'd update any existing callers that have the message in an strbuf so 
 > they don't have to write it to a file just to add some trailers to it.
 > 
 > > +    strbuf_release(&buf);
 > > +    return 0;
 > > + }
 > 
 > As I said above reusing the existing code as you have done here is a 
 > much better approach. However it would be much easier to review if the 
 > code movement was separated from the refactoring. I'm also struggling to 
 > see the benefit of a lot of the refactoring - I was expecting the 
 > conversion to use an strubf would essentially look like fwrite() being 
 > replaced with strbuf_add() and fprintf() being replaced with 
 > strbuf_addf() etc. rather than reworking the logic.
 > 
 > Thanks
 > 
 > 
 > Phillip
 > 
 > > diff --git a/trailer.h b/trailer.h
 > > index 4740549586..01f711fb13 100644
 > > --- a/trailer.h
 > > +++ b/trailer.h
 > > @@ -196,10 +196,22 @@ int trailer_iterator_advance(struct trailer_iterator *iter);
 > >   void trailer_iterator_release(struct trailer_iterator *iter);
 > >   
 > >   /*
 > > - * Augment a file to add trailers to it by running git-interpret-trailers.
 > > - * This calls run_command() and its return value is the same (i.e. 0 for
 > > - * success, various non-zero for other errors). See run-command.h.
 > > + * 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.
 > >    */
 > >   int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);
 > >   
 > > +/*
 > > + * Process trailer lines for a commit message in-memory.
 > > + * @opts: trailer processing options (e.g. from parse-options)
 > > + * @msg: the input message string
 > > + * @new_trailer_head: list of new trailers to add (struct new_trailer_item)
 > > + * @out: strbuf to store the resulting message (must be initialized)
 > > + *
 > > + * Returns 0 on success, <0 on error.
 > > + */
 > > +int trailer_process(const struct process_trailer_options *opts,
 > > +            const char *msg,
 > > +            struct list_head *new_trailer_head,
 > > +            struct strbuf *out);
 > >   #endif /* TRAILER_H */
 > 
 > 
Regards,

Li​

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

[Junio C Hamano]

Li Chen <me@linux.beauty> writes:

> Hi Phillip, 
>
> Thanks for your thorough review; I will address them in the next version.

As I do not want to keep an inactive topic in 'seen' for more than a
month, I was doing my usual "sweep" of the topics, and found this
exchange.  

Is this still being worked on?  No rush, but just checking to see
what the status is.

Since the summer is a slow season, I do not mind keeping the topic
for a few more weeks in 'seen', but I can simply discard the one I
have, and requeue a new version in 'seen' when it materializes.

Thanks.

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

[Li Chen]

Hi Junio,

I apologize for the delayed response.

 ---- On Fri, 29 Aug 2025 07:35:45 +0800  Junio C Hamano <gitster@pobox.com> wrote --- 
 > Li Chen <me@linux.beauty> writes:
 > 
 > > Hi Phillip, 
 > >
 > > Thanks for your thorough review; I will address them in the next version.
 > 
 > As I do not want to keep an inactive topic in 'seen' for more than a
 > month, I was doing my usual "sweep" of the topics, and found this
 > exchange.  
 > 
 > Is this still being worked on?  No rush, but just checking to see
 > what the status is.
 > 
 > Since the summer is a slow season, I do not mind keeping the topic
 > for a few more weeks in 'seen', but I can simply discard the one I
 > have, and requeue a new version in 'seen' when it materializes.

Yes, it's still in progress, though I've had limited time recently. I aim to finish the next
version before October 8th, taking advantage of the 7-day Chinese National Day holiday
to work on it.

Regards,
Li

Re: [PATCH v3 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers`

[Li Chen]

Hi Phillip,

 > >       new_trailers_clear(&trailers);
 > > diff --git a/trailer.c b/trailer.c
 > > index 310cf582dc..03814443c3 100644
 > > --- a/trailer.c
 > > +++ b/trailer.c
 > > @@ -1224,14 +1224,121 @@ void trailer_iterator_release(struct trailer_iterator *iter)
 > >       strbuf_release(&iter->key);
 > >   }
 > >   
 > > -int amend_file_with_trailers(const char *path, const struct strvec *trailer_args)
 > > +static int amend_strbuf_with_trailers(struct strbuf *buf,
 > > +                   const struct strvec *trailer_args)
 > 
 > Function argument declarations should be aligned
 
My editor uses a 4-space tab width, but the project view displays tabs as 8 spaces wide.
This discrepancy caused the same errors in v4, which I plan to correct with clang-format in v5.

Regards,

Li​