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

[PATCH v2 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/15558597606/job/43805117563
(except for debian built test because of network issue)

RFC link: https://lore.kernel.org/git/196a5ac1393.f5b4db7d187309.2451613571977217927@linux.beauty/

The review comments from Phillip in the RFC series have been fixed. 
Thanks a lot for Phillip's great review!

Comments welcome!

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

 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 ++++++++++++++++++++++
 trailer.c                     | 147 +++++++++++++++++++++++++++++++---
 trailer.h                     |  17 ++++
 8 files changed, 372 insertions(+), 9 deletions(-)
 create mode 100755 t/t3440-rebase-trailer.sh

-- 
2.49.0

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

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

The built-in commands that support `--trailer` (tag, commit --amend,
rebase --trailer, etc.) have always appended trailers by spawning an
external `git interpret-trailers --in-place` process.  That extra fork
is negligible for a single commit, but it scales poorly when hundreds or
thousands of commits are rewritten (e.g. an interactive rebase or a
history-wide `filter-repo`).  This patch moves the heavy lifting fully
in-process:

* `amend_strbuf_with_trailers()`
  – parses the existing message,
  – merges trailers from the command line and config,
  – formats the final trailer block, and
  – rewrites the supplied `struct strbuf`
  without ever leaving the current process.

* `amend_file_with_trailers()` becomes a thin wrapper that reads a file
  into a `strbuf`, calls the new helper, and writes the result back.

* `builtin/rebase.c` now calls `amend_file_with_trailers()` instead of
  executing `interpret-trailers`.

Edge-cases that used to be handled implicitly by the external helper are
now replicated:

  * `trailer.ifexists=replace` and other per-key policies are honoured by
    running the existing `parse_trailers_from_command_line_args()` logic
    on the raw CLI list, so the same *replace / add / ignore* semantics
    are preserved.

  * A message that contains an RFC-2822 style `---` separator but no
    prior trailer block gets its trailers appended **after** the
    separator, matching the external command’s output.

  * When `git commit --verbose` leaves a diff after the message (comment
    lines beginning with `#`), trailers are inserted before that comment
    section so they survive the subsequent strip-space cleanup.

  * Ordering remains identical to the old path: CLI trailers first,
    followed by trailers introduced via `trailer.<key>.*` config.

Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
---
 trailer.c | 147 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 trailer.h |  17 +++++++
 2 files changed, 155 insertions(+), 9 deletions(-)

diff --git a/trailer.c b/trailer.c
index 310cf582dc..70b04736a8 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1224,14 +1224,143 @@ 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 size_t first_comment_pos(const struct strbuf *buf)
 {
-	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);
+	const char *p = buf->buf;
+	const char *end = buf->buf + buf->len;
+
+	while (p < end) {
+		const char *line = p;
+		const char *nl = memchr(p, '\n', end - p);
+		size_t len = nl ? (size_t)(nl - p) : (size_t)(end - p);
+
+		/* skip leading whitespace */
+		size_t i = 0;
+		while (i < len && isspace((unsigned char)line[i]))
+			i++;
+
+		if (i < len && line[i] == '#')
+			return (size_t)(line - buf->buf); /* comment starts here */
+
+		if (!nl)              /* last line without newline */
+			break;
+		p = nl + 1;
+	}
+	return buf->len;          /* no comment line found */
+}
+
+int amend_strbuf_with_trailers(struct strbuf *buf,
+							   const struct strvec *trailer_args)
+{
+	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+	struct strbuf trailers_sb = STRBUF_INIT;
+	struct strbuf out         = STRBUF_INIT;
+	size_t i;
+	/* 1. parse message ------------------------------------------------- */
+	LIST_HEAD(orig_head);   /* existing trailers, if any          */
+	LIST_HEAD(cfg_head);    /* config trailers                    */
+	LIST_HEAD(cli_raw);     /* new_trailer_item from CLI          */
+	LIST_HEAD(cli_head);    /* arg_item after helper              */
+	LIST_HEAD(all_new);     /* merged list                        */
+	struct trailer_block *blk = parse_trailers(&opts, buf->buf, &orig_head);
+	bool had_trailer_before = !list_empty(&orig_head);
+
+	/* 2. CLI trailers -------------------------------------------------- */
+	if (trailer_args && trailer_args->nr) {
+		for (i = 0; i < trailer_args->nr; i++) {
+				const char *arg  = trailer_args->v[i];
+				const char *text;
+				struct new_trailer_item *ni;
+
+				if (!skip_prefix(arg, "--trailer=", &text))
+						text = arg;
+
+				if (!*text)
+						continue;
+
+				ni = xcalloc(1, sizeof(*ni));
+				INIT_LIST_HEAD(&ni->list);
+				ni->text = (char *)text;
+				list_add_tail(&ni->list, &cli_raw);
+		}
+		parse_trailers_from_command_line_args(&cli_head, &cli_raw);
+
+		while (!list_empty(&cli_raw)) {
+				struct new_trailer_item *ni =
+						list_first_entry(&cli_raw, struct new_trailer_item, list);
+				list_del(&ni->list);
+				free(ni);
+		}
+	}
+
+	/* 3. config trailers ---------------------------------------------- */
+	parse_trailers_from_config(&cfg_head);
+
+	/* 4. merge lists --------------------------------------------------- */
+	list_splice(&cli_head, &all_new);
+	list_splice(&cfg_head, &all_new);
+
+	/* 5. apply --------------------------------------------------------- */
+	process_trailers_lists(&orig_head, &all_new);
+
+	/* 6. format updated trailer block --------------------------------- */
+	format_trailers(&opts, &orig_head, &trailers_sb);
+
+	/* 7. decide insertion point --------------------------------------- */
+	if (had_trailer_before) {
+		/* insert at existing trailer block */
+		strbuf_add(&out, buf->buf, trailer_block_start(blk));
+		if (!blank_line_before_trailer_block(blk))
+			strbuf_addch(&out, '\n');
+		strbuf_addbuf(&out, &trailers_sb);
+		strbuf_add(&out,
+				   buf->buf + trailer_block_end(blk),
+				   buf->len - trailer_block_end(blk));
+	} else {
+		/* insert before first comment (git --verbose) if any */
+		size_t cpos = first_comment_pos(buf);
+
+		/* copy body up to comment (or whole buf if none) */
+		strbuf_add(&out, buf->buf, cpos);
+
+		/* ensure single blank line separating body & trailers */
+		if (!out.len || out.buf[out.len - 1] != '\n')
+			strbuf_addch(&out, '\n');
+		if (out.len >= 2 && out.buf[out.len - 2] != '\n')
+			strbuf_addch(&out, '\n');
+
+		strbuf_addbuf(&out, &trailers_sb);
+
+		/* copy remaining comment lines (if any) */
+		strbuf_add(&out, buf->buf + cpos, buf->len - cpos);
+	}
+
+	strbuf_swap(buf, &out);
+
+	/* 8. cleanup ------------------------------------------------------- */
+	strbuf_release(&out);
+	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..17986d5dd0 100644
--- a/trailer.h
+++ b/trailer.h
@@ -202,4 +202,21 @@ void trailer_iterator_release(struct trailer_iterator *iter);
  */
 int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);
 
+/*
+ * In‑memory variant of amend_file_with_trailers(): instead of rewriting a file
+ * on disk (and therefore spawning a separate `git interpret‑trailers` process)
+ * we operate directly on a struct strbuf that already contains the commit
+ * message.  The rules for positioning, deduplication, and formatting are the
+ * same as those implemented by the builtin `interpret‑trailers` command.
+ *
+ *  - @buf          : the message to be amended.  On success, its contents are
+ *                    replaced with the new message that has the trailers
+ *                    inserted.
+ *  - @trailer_args : the list of trailer strings exactly as would be passed on
+ *                    the command‑line via repeated `--trailer` options.
+ *
+ * Returns 0 on success, <0 on error.
+ */
+int amend_strbuf_with_trailers(struct strbuf *buf,
+                               const struct strvec *trailer_args);
 #endif /* TRAILER_H */
-- 
2.49.0

[PATCH v2 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,
as suggested by Phillip Wood.

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 2e8c4ee678..1e21caace7 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,
@@ -1132,6 +1210,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",
@@ -1282,6 +1361,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"
@@ -1539,6 +1629,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 1ee0abbd45..504642e4c7 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)
@@ -2527,6 +2528,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 d052fc3e23..14ba1d8d75 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',
+  '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.49.0

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

[Junio C Hamano]

Li Chen <me@linux.beauty> writes:

> * `amend_strbuf_with_trailers()`
>   – parses the existing message,
>   – merges trailers from the command line and config,
>   – formats the final trailer block, and
>   – rewrites the supplied `struct strbuf`
>   without ever leaving the current process.

If such a helper function exists, shouldn't "interpret-trailers" be
able to lose quite a lot of lines, at least nearly as many as the
new lines introduced to this new function, by making it call this
function as well?  And that would ensure that the internal call can
safely replace the external call and produce exactly the same
output?  If so, that can be a pure refactoring patch that can become
a commit on its own, I presume?

> * `amend_file_with_trailers()` becomes a thin wrapper that reads a file
>   into a `strbuf`, calls the new helper, and writes the result back.
> * `builtin/rebase.c` now calls `amend_file_with_trailers()` instead of
>   executing `interpret-trailers`.

And then these two changes can become a separate patch on top?

> -int amend_file_with_trailers(const char *path, const struct strvec *trailer_args)

This lone "removed line" can be avoided if the patch did not touch
this line ...

> +static size_t first_comment_pos(const struct strbuf *buf)
>  {
> -	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);
> +	const char *p = buf->buf;
> +	const char *end = buf->buf + buf->len;
> +
> +	while (p < end) {
> +		const char *line = p;
> +		const char *nl = memchr(p, '\n', end - p);
> +		size_t len = nl ? (size_t)(nl - p) : (size_t)(end - p);
> +
> +		/* skip leading whitespace */
> +		size_t i = 0;
> +		while (i < len && isspace((unsigned char)line[i]))
> +			i++;
> +
> +		if (i < len && line[i] == '#')
> +			return (size_t)(line - buf->buf); /* comment starts here */
> +
> +		if (!nl)              /* last line without newline */
> +			break;
> +		p = nl + 1;
> +	}
> +	return buf->len;          /* no comment line found */
> +}
> +
> +int amend_strbuf_with_trailers(struct strbuf *buf,
> +							   const struct strvec *trailer_args)

... like this?

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

[phillip.wood123]

On 11/06/2025 01:09, Junio C Hamano wrote:
> Li Chen <me@linux.beauty> writes:
> 
>> * `amend_strbuf_with_trailers()`
>>    – parses the existing message,
>>    – merges trailers from the command line and config,
>>    – formats the final trailer block, and
>>    – rewrites the supplied `struct strbuf`
>>    without ever leaving the current process.
> 
> If such a helper function exists, shouldn't "interpret-trailers" be
> able to lose quite a lot of lines, at least nearly as many as the
> new lines introduced to this new function, by making it call this
> function as well?  And that would ensure that the internal call can
> safely replace the external call and produce exactly the same
> output?  If so, that can be a pure refactoring patch that can become
> a commit on its own, I presume?

Exactly - I was expecting to see a refactoring of interpret_trailers() 
in builtin/interpret-trailers.c that moved most of the function body 
into a new function in trailer.c that added the trailers to an strbuf. 
This seems to be a parallel implementation which doesn't sound like the 
best plan.

I'm going to be off the list for a couple of weeks, I'll take a more 
detailed look at this series when I'm back

Best Wishes

Phillip

> 
>> * `amend_file_with_trailers()` becomes a thin wrapper that reads a file
>>    into a `strbuf`, calls the new helper, and writes the result back.
>> * `builtin/rebase.c` now calls `amend_file_with_trailers()` instead of
>>    executing `interpret-trailers`.
> 
> And then these two changes can become a separate patch on top?
> 
>> -int amend_file_with_trailers(const char *path, const struct strvec *trailer_args)
> 
> This lone "removed line" can be avoided if the patch did not touch
> this line ...
> 
>> +static size_t first_comment_pos(const struct strbuf *buf)
>>   {
>> -	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);
>> +	const char *p = buf->buf;
>> +	const char *end = buf->buf + buf->len;
>> +
>> +	while (p < end) {
>> +		const char *line = p;
>> +		const char *nl = memchr(p, '\n', end - p);
>> +		size_t len = nl ? (size_t)(nl - p) : (size_t)(end - p);
>> +
>> +		/* skip leading whitespace */
>> +		size_t i = 0;
>> +		while (i < len && isspace((unsigned char)line[i]))
>> +			i++;
>> +
>> +		if (i < len && line[i] == '#')
>> +			return (size_t)(line - buf->buf); /* comment starts here */
>> +
>> +		if (!nl)              /* last line without newline */
>> +			break;
>> +		p = nl + 1;
>> +	}
>> +	return buf->len;          /* no comment line found */
>> +}
>> +
>> +int amend_strbuf_with_trailers(struct strbuf *buf,
>> +							   const struct strvec *trailer_args)
> 
> ... like this?

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

[Junio C Hamano]

phillip.wood123@gmail.com writes:

> Exactly - I was expecting to see a refactoring of interpret_trailers()
> in builtin/interpret-trailers.c that moved most of the function body
> into a new function in trailer.c that added the trailers to an
> strbuf. This seems to be a parallel implementation which doesn't sound
> like the best plan.
>
> I'm going to be off the list for a couple of weeks, I'll take a more
> detailed look at this series when I'm back

Thanks.

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

[Li Chen]

Hi Junio & Phillip,


 ---- On Wed, 11 Jun 2025 23:34:07 +0800  Junio C Hamano <gitster@pobox.com> wrote --- 
 > phillip.wood123@gmail.com writes:
 > 
 > > Exactly - I was expecting to see a refactoring of interpret_trailers()
 > > in builtin/interpret-trailers.c that moved most of the function body
 > > into a new function in trailer.c that added the trailers to an
 > > strbuf. This seems to be a parallel implementation which doesn't sound
 > > like the best plan.
 > >
 > > I'm going to be off the list for a couple of weeks, I'll take a more
 > > detailed look at this series when I'm back

Thanks for your suggestion; I will refactor it in the next version.

Regards,
Li