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

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

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

This series routes all trailer insertion through an in-process path, removing
the fork/exec to builtin/interpret-trailers and tempfile juggling. The first
three commits centralize logic to reduce overhead and simplify error handling.
The final commit adds git rebase --trailer, currently supported with the merge
backend only (rejecting apply-only scenarios and validating input early).

all t/*.sh testcases have run successfully.

v6: squash all fix commits and split refactor step from the original patch based on Phillip's suggestion and codes [4].
v5: fix all Kristoffer's review comments form v4[3] in place and without new patches.
v4: fix all reviewer comments in v3. [2], and add patch 1~8 & 10~29 to fix review comments.
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 very very welcome!

[1]: https://lore.kernel.org/git/xmqq8qlzkukw.fsf@gitster.g/
[2]: https://lore.kernel.org/git/20250803150059.402017-1-me@linux.beauty/
[3]: https://lore.kernel.org/git/20251014122452.1851103-1-me@linux.beauty/
[4]: https://lore.kernel.org/git/7d12b046-365f-441c-af8e-8a39d61efbbd@gmail.com/

Li Chen (4):
  interpret-trailers: factor out buffer-based processing to
    process_trailers()
  trailer: move process_trailers to trailer.h
  trailer: append trailers in-process and drop the fork to
    `interpret-trailers`
  rebase: support --trailer

 Documentation/git-rebase.adoc |   9 ++-
 builtin/commit.c              |   2 +-
 builtin/interpret-trailers.c  |  81 ++------------------
 builtin/rebase.c              |  50 +++++++++++++
 builtin/tag.c                 |   3 +-
 sequencer.c                   |  34 +++++++++
 sequencer.h                   |   4 +-
 t/meson.build                 |   1 +
 t/t3440-rebase-trailer.sh     | 134 ++++++++++++++++++++++++++++++++++
 trailer.c                     | 129 +++++++++++++++++++++++++++++---
 trailer.h                     |  13 +++-
 wrapper.c                     |  16 ++++
 wrapper.h                     |   6 ++
 13 files changed, 392 insertions(+), 90 deletions(-)
 create mode 100755 t/t3440-rebase-trailer.sh

-- 
2.51.0

[PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers()

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

Extracted trailer processing into a helper that accumulates output in
a strbuf before writing.

Updated interpret_trailers() to reuse the helper, buffer output, and
clean up both input and output buffers after writing.

Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
---
 builtin/interpret-trailers.c | 51 ++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 22 deletions(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 41b0750e5a..4c90580fff 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -136,32 +136,21 @@ 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)
+static void process_trailers(const struct process_trailer_options *opts,
+			     struct list_head *new_trailer_head,
+			     struct strbuf *sb, struct strbuf *out)
 {
 	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);
+	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);
+		strbuf_add(out, sb->buf, trailer_block_start(trailer_block));
 
 	if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
-		fprintf(outfile, "\n");
-
+		strbuf_addch(out, '\n');
 
 	if (!opts->only_input) {
 		LIST_HEAD(config_head);
@@ -173,22 +162,40 @@ static void interpret_trailers(const struct process_trailer_options *opts,
 	}
 
 	/* Print trailer block. */
-	format_trailers(opts, &head, &trailer_block_sb);
+	format_trailers(opts, &head, out);
 	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);
+		strbuf_add(out, sb->buf + trailer_block_end(trailer_block),
+			   sb->len - trailer_block_end(trailer_block));
 	trailer_block_release(trailer_block);
+}
+
+static void interpret_trailers(const struct process_trailer_options *opts,
+			       struct list_head *new_trailer_head,
+			       const char *file)
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct strbuf out = STRBUF_INIT;
+	FILE *outfile = stdout;
+
+	trailer_config_init();
+
+	read_input_file(&sb, file);
+
+	if (opts->in_place)
+		outfile = create_in_place_tempfile(file);
+
+	process_trailers(opts, new_trailer_head, &sb, &out);
 
+	fwrite(out.buf, out.len, 1, outfile);
 	if (opts->in_place)
 		if (rename_tempfile(&trailers_tempfile, file))
 			die_errno(_("could not rename temporary file to %s"), file);
 
 	strbuf_release(&sb);
+	strbuf_release(&out);
 }
 
 int cmd_interpret_trailers(int argc,
-- 
2.51.0

[PATCH v6 2/4] trailer: move process_trailers to trailer.h

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

This function would be used by trailer_process
in following commits.

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

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 4c90580fff..bce2e791d6 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -136,42 +136,6 @@ static void read_input_file(struct strbuf *sb, const char *file)
 	strbuf_complete_line(sb);
 }
 
-static void process_trailers(const struct process_trailer_options *opts,
-			     struct list_head *new_trailer_head,
-			     struct strbuf *sb, struct strbuf *out)
-{
-	LIST_HEAD(head);
-	struct trailer_block *trailer_block;
-
-	trailer_block = parse_trailers(opts, sb->buf, &head);
-
-	/* Print the lines before the trailer block */
-	if (!opts->only_trailers)
-		strbuf_add(out, sb->buf, trailer_block_start(trailer_block));
-
-	if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
-		strbuf_addch(out, '\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, out);
-	free_trailers(&head);
-
-	/* Print the lines after the trailer block as is. */
-	if (!opts->only_trailers)
-		strbuf_add(out, sb->buf + trailer_block_end(trailer_block),
-			   sb->len - trailer_block_end(trailer_block));
-	trailer_block_release(trailer_block);
-}
-
 static void interpret_trailers(const struct process_trailer_options *opts,
 			       struct list_head *new_trailer_head,
 			       const char *file)
diff --git a/trailer.c b/trailer.c
index 911a81ed99..b735ec8a53 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1235,3 +1235,39 @@ int amend_file_with_trailers(const char *path, const struct strvec *trailer_args
 	strvec_pushv(&run_trailer.args, trailer_args->v);
 	return run_command(&run_trailer);
 }
+
+void process_trailers(const struct process_trailer_options *opts,
+		      struct list_head *new_trailer_head,
+		      struct strbuf *sb, struct strbuf *out)
+{
+	LIST_HEAD(head);
+	struct trailer_block *trailer_block;
+
+	trailer_block = parse_trailers(opts, sb->buf, &head);
+
+	/* Print the lines before the trailer block */
+	if (!opts->only_trailers)
+		strbuf_add(out, sb->buf, trailer_block_start(trailer_block));
+
+	if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
+		strbuf_addch(out, '\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, out);
+	free_trailers(&head);
+
+	/* Print the lines after the trailer block as is. */
+	if (!opts->only_trailers)
+		strbuf_add(out, sb->buf + trailer_block_end(trailer_block),
+			   sb->len - trailer_block_end(trailer_block));
+	trailer_block_release(trailer_block);
+}
diff --git a/trailer.h b/trailer.h
index 4740549586..44d406b763 100644
--- a/trailer.h
+++ b/trailer.h
@@ -202,4 +202,7 @@ void trailer_iterator_release(struct trailer_iterator *iter);
  */
 int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);
 
+void process_trailers(const struct process_trailer_options *opts,
+		      struct list_head *new_trailer_head,
+		      struct strbuf *sb, struct strbuf *out);
 #endif /* TRAILER_H */
-- 
2.51.0

[PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

Route all trailer insertion through trailer_process() and make
builtin/interpret-trailers just do file I/O before calling into it.
amend_file_with_trailers() now shares the same code path.

This removes the fork/exec and tempfile juggling, cutting overhead and
simplifying error handling. No functional change. It also
centralizes logic to prepare for follow-up rebase --trailer patch.

Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
---
 builtin/commit.c             |  2 +-
 builtin/interpret-trailers.c | 46 +++---------------------
 builtin/tag.c                |  3 +-
 trailer.c                    | 68 +++++++++++++++++++++++++++++++-----
 trailer.h                    |  5 ++-
 wrapper.c                    | 16 +++++++++
 wrapper.h                    |  6 ++++
 7 files changed, 90 insertions(+), 56 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 0243f17d53..67070d6a54 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1719,7 +1719,7 @@ int cmd_commit(int argc,
 		OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword specified commit")),
 		OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
 		OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
-		OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG),
+		OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, parse_opt_strvec),
 		OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),
 		OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
 		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index bce2e791d6..268a43372b 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -10,7 +10,6 @@
 #include "gettext.h"
 #include "parse-options.h"
 #include "string-list.h"
-#include "tempfile.h"
 #include "trailer.h"
 #include "config.h"
 
@@ -93,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) {
@@ -142,21 +110,15 @@ static void interpret_trailers(const struct process_trailer_options *opts,
 {
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf out = STRBUF_INIT;
-	FILE *outfile = stdout;
-
-	trailer_config_init();
 
 	read_input_file(&sb, file);
 
-	if (opts->in_place)
-		outfile = create_in_place_tempfile(file);
-
 	process_trailers(opts, new_trailer_head, &sb, &out);
 
-	fwrite(out.buf, out.len, 1, outfile);
 	if (opts->in_place)
-		if (rename_tempfile(&trailers_tempfile, file))
-			die_errno(_("could not rename temporary file to %s"), file);
+		write_file_buf(file, out.buf, out.len);
+	else
+		strbuf_write(&out, stdout);
 
 	strbuf_release(&sb);
 	strbuf_release(&out);
@@ -203,6 +165,8 @@ int cmd_interpret_trailers(int argc,
 			git_interpret_trailers_usage,
 			options);
 
+	trailer_config_init();
+
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)
diff --git a/builtin/tag.c b/builtin/tag.c
index f0665af3ac..65c4a0b36b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -499,8 +499,7 @@ int cmd_tag(int argc,
 		OPT_CALLBACK_F('m', "message", &msg, N_("message"),
 			       N_("tag message"), PARSE_OPT_NONEG, parse_msg_arg),
 		OPT_FILENAME('F', "file", &msgfile, N_("read message from file")),
-		OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"),
-				  N_("add custom trailer(s)"), PARSE_OPT_NONEG),
+		OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, parse_opt_strvec),
 		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of tag message")),
 		OPT_BOOL('s', "sign", &opt.sign, N_("annotated and GPG-signed tag")),
 		OPT_CLEANUP(&cleanup_arg),
diff --git a/trailer.c b/trailer.c
index b735ec8a53..f5838f5699 100644
--- a/trailer.c
+++ b/trailer.c
@@ -9,6 +9,8 @@
 #include "commit.h"
 #include "trailer.h"
 #include "list.h"
+#include "wrapper.h"
+
 /*
  * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
  */
@@ -1224,18 +1226,66 @@ 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 *text = trailer_args->v[i];
+		struct new_trailer_item *item;
+
+		if (!*text)
+			continue;
+		item = xcalloc(1, sizeof(*item));
+		INIT_LIST_HEAD(&item->list);
+		item->text = text;
+		list_add_tail(&item->list, &new_trailer_head);
+	}
+
+	process_trailers(&opts, &new_trailer_head, buf, &out);
+
+	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 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)) {
+		strbuf_release(&buf);
+		return error("failed to append trailers");
+	}
+
+	if (write_file_buf_gently(path, buf.buf, buf.len)) {
+		strbuf_release(&buf);
+		return -1;
+	}
+
+	strbuf_release(&buf);
+	return 0;
+ }
+
 void process_trailers(const struct process_trailer_options *opts,
 		      struct list_head *new_trailer_head,
 		      struct strbuf *sb, struct strbuf *out)
diff --git a/trailer.h b/trailer.h
index 44d406b763..daea46ca5d 100644
--- a/trailer.h
+++ b/trailer.h
@@ -196,9 +196,8 @@ 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);
 
diff --git a/wrapper.c b/wrapper.c
index 3d507d4204..1f12dbb2fa 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -688,6 +688,22 @@ void write_file_buf(const char *path, const char *buf, size_t len)
 		die_errno(_("could not close '%s'"), path);
 }
 
+int write_file_buf_gently(const char *path, const char *buf, size_t len)
+{
+	int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
+
+	if (fd < 0)
+		return error_errno(_("could not open '%s'"), path);
+	if (write_in_full(fd, buf, len) < 0) {
+		int ret = error_errno(_("could not write to '%s'"), path);
+		close(fd);
+		return ret;
+	}
+	if (close(fd))
+		return error_errno(_("could not close '%s'"), path);
+	return 0;
+}
+
 void write_file(const char *path, const char *fmt, ...)
 {
 	va_list params;
diff --git a/wrapper.h b/wrapper.h
index 44a8597ac3..e5f867b200 100644
--- a/wrapper.h
+++ b/wrapper.h
@@ -56,6 +56,12 @@ static inline ssize_t write_str_in_full(int fd, const char *str)
  */
 void write_file_buf(const char *path, const char *buf, size_t len);
 
+/**
+ * Like write_file_buf(), but report errors instead of exiting. Returns 0 on
+ * success or a negative value on error after emitting a message.
+ */
+int write_file_buf_gently(const char *path, const char *buf, size_t len);
+
 /**
  * Like write_file_buf(), but format the contents into a buffer first.
  * Additionally, write_file() will append a newline if one is not already
-- 
2.51.0

[PATCH v6 4/4] 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 it if the user passes an option that requires the
apply backend (git am) since it lacks message‑filter/trailer
hook. otherwise we can just use the merge backend.

Automatically set REBASE_FORCE when any trailer is supplied.

And reject invalid input before user edits the interactive file.

Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
---
 Documentation/git-rebase.adoc |   9 ++-
 builtin/rebase.c              |  50 +++++++++++++
 sequencer.c                   |  34 +++++++++
 sequencer.h                   |   4 +-
 t/meson.build                 |   1 +
 t/t3440-rebase-trailer.sh     | 134 ++++++++++++++++++++++++++++++++++
 trailer.c                     |  29 +++++++-
 trailer.h                     |   5 ++
 8 files changed, 262 insertions(+), 4 deletions(-)
 create mode 100755 t/t3440-rebase-trailer.sh

diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc
index 005caf6164..4d2fe4be6e 100644
--- a/Documentation/git-rebase.adoc
+++ b/Documentation/git-rebase.adoc
@@ -487,9 +487,16 @@ See also INCOMPATIBLE OPTIONS below.
 	Add a `Signed-off-by` trailer to all the rebased commits. Note
 	that if `--interactive` is given then only commits marked to be
 	picked, edited or reworded will have the trailer added.
-+
+
 See also INCOMPATIBLE OPTIONS below.
 
+--trailer=<trailer>::
+       Append the given trailer line(s) to every rebased commit
+       message, processed via linkgit:git-interpret-trailers[1].
+       When this option is present *rebase automatically implies*
+       `--force-rebase` so that fast‑forwarded commits are also
+       rewritten.
+
 -i::
 --interactive::
 	Make a list of the commits which are about to be rebased.  Let the
diff --git a/builtin/rebase.c b/builtin/rebase.c
index c468828189..a88abe08b4 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -36,6 +36,7 @@
 #include "reset.h"
 #include "trace2.h"
 #include "hook.h"
+#include "trailer.h"
 
 static char const * const builtin_rebase_usage[] = {
 	N_("git rebase [-i] [options] [--exec <cmd>] "
@@ -113,6 +114,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 +145,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 +169,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 +181,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;
@@ -500,6 +508,23 @@ static int read_basic_state(struct rebase_options *opts)
 		opts->gpg_sign_opt = xstrdup(buf.buf);
 	}
 
+	strbuf_reset(&buf);
+
+	if (strbuf_read_file(&buf, state_dir_path("trailer", opts), 0) >= 0) {
+		const char *p = buf.buf, *end = buf.buf + buf.len;
+
+		while (p < end) {
+			char *nl = memchr(p, '\n', end - p);
+			if (!nl)
+				die("nl shouldn't be NULL");
+			*nl = '\0';
+
+			if (*p)
+				strvec_push(&opts->trailer_args, p);
+
+			p = nl + 1;
+		}
+	}
 	strbuf_release(&buf);
 
 	return 0;
@@ -528,6 +553,21 @@ static int rebase_write_basic_state(struct rebase_options *opts)
 	if (opts->signoff)
 		write_file(state_dir_path("signoff", opts), "--signoff");
 
+	/*
+	 * save opts->trailer_args into state_dir/trailer
+	 */
+	if (opts->trailer_args.nr) {
+		struct strbuf buf = STRBUF_INIT;
+
+		for (size_t i = 0; i < opts->trailer_args.nr; i++) {
+				strbuf_addstr(&buf, opts->trailer_args.v[i]);
+				strbuf_addch(&buf, '\n');
+		}
+		write_file(state_dir_path("trailer", opts),
+				   "%s", buf.buf);
+		strbuf_release(&buf);
+	}
+
 	return 0;
 }
 
@@ -1132,6 +1172,8 @@ int cmd_rebase(int argc,
 			.flags = PARSE_OPT_NOARG,
 			.defval = REBASE_DIFFSTAT,
 		},
+		OPT_STRVEC(0, "trailer", &options.trailer_args, N_("trailer"),
+				N_("add custom trailer(s)")),
 		OPT_BOOL(0, "signoff", &options.signoff,
 			 N_("add a Signed-off-by trailer to each commit")),
 		OPT_BOOL(0, "committer-date-is-author-date",
@@ -1285,6 +1327,11 @@ int cmd_rebase(int argc,
 			     builtin_rebase_options,
 			     builtin_rebase_usage, 0);
 
+	if (options.trailer_args.nr) {
+		validate_trailer_args_after_config(&options.trailer_args);
+		options.flags |= REBASE_FORCE;
+	}
+
 	if (preserve_merges_selected)
 		die(_("--preserve-merges was replaced by --rebase-merges\n"
 			"Note: Your `pull.rebase` configuration may also be set to 'preserve',\n"
@@ -1542,6 +1589,9 @@ int cmd_rebase(int argc,
 	if (options.root && !options.onto_name)
 		imply_merge(&options, "--root without --onto");
 
+	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 5476d39ba9..fbf35cb474 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -209,6 +209,7 @@ static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedul
 static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, "rebase-merge/no-reschedule-failed-exec")
 static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, "rebase-merge/drop_redundant_commits")
 static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redundant_commits")
+static GIT_PATH_FUNC(rebase_path_trailer, "rebase-merge/trailer")
 
 /*
  * A 'struct replay_ctx' represents the private state of the sequencer.
@@ -420,6 +421,7 @@ void replay_opts_release(struct replay_opts *opts)
 	if (opts->revs)
 		release_revisions(opts->revs);
 	free(opts->revs);
+	strvec_clear(&opts->trailer_args);
 	replay_ctx_release(ctx);
 	free(opts->ctx);
 }
@@ -2025,6 +2027,10 @@ static int append_squash_message(struct strbuf *buf, const char *body,
 		if (opts->signoff)
 			append_signoff(buf, 0, 0);
 
+		if (opts->trailer_args.nr &&
+			amend_strbuf_with_trailers(buf, &opts->trailer_args))
+			return error(_("unable to add trailers to commit message"));
+
 		if ((command == TODO_FIXUP) &&
 		    (flag & TODO_REPLACE_FIXUP_MSG) &&
 		    (file_exists(rebase_path_fixup_msg()) ||
@@ -2443,6 +2449,14 @@ static int do_pick_commit(struct repository *r,
 	if (opts->signoff && !is_fixup(command))
 		append_signoff(&ctx->message, 0, 0);
 
+	if (opts->trailer_args.nr && !is_fixup(command)) {
+		if (amend_strbuf_with_trailers(&ctx->message,
+					       &opts->trailer_args)) {
+			res = error(_("unable to add trailers to commit message"));
+			goto leave;
+		}
+	}
+
 	if (is_rebase_i(opts) && write_author_script(msg.message) < 0)
 		res = -1;
 	else if (!opts->strategy ||
@@ -2517,6 +2531,7 @@ static int do_pick_commit(struct repository *r,
 			_("dropping %s %s -- patch contents already upstream\n"),
 			oid_to_hex(&commit->object.oid), msg.subject);
 	} /* else allow == 0 and there's nothing special to do */
+
 	if (!opts->no_commit && !drop_commit) {
 		if (author || command == TODO_REVERT || (flags & AMEND_MSG))
 			res = do_commit(r, msg_file, author, reflog_action,
@@ -3234,6 +3249,17 @@ static int read_populate_opts(struct replay_opts *opts)
 
 		read_strategy_opts(opts, &buf);
 		strbuf_reset(&buf);
+		if (strbuf_read_file(&buf, rebase_path_trailer(), 0) >= 0) {
+			char *p = buf.buf, *nl;
+
+			while ((nl = strchr(p, '\n'))) {
+				*nl = '\0';
+				if (*p)
+					strvec_push(&opts->trailer_args, p);
+				p = nl + 1;
+			}
+			strbuf_reset(&buf);
+		}
 
 		if (read_oneliner(&ctx->current_fixups,
 				  rebase_path_current_fixups(),
@@ -3328,6 +3354,14 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
 		write_file(rebase_path_reschedule_failed_exec(), "%s", "");
 	else
 		write_file(rebase_path_no_reschedule_failed_exec(), "%s", "");
+	if (opts->trailer_args.nr) {
+		struct strbuf buf = STRBUF_INIT;
+
+		for (size_t i = 0; i < opts->trailer_args.nr; i++)
+			strbuf_addf(&buf, "%s\n", opts->trailer_args.v[i]);
+		write_file(rebase_path_trailer(), "%s", buf.buf);
+		strbuf_release(&buf);
+	}
 
 	return 0;
 }
diff --git a/sequencer.h b/sequencer.h
index 719684c8a9..e21835c5a0 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -44,6 +44,7 @@ struct replay_opts {
 	int record_origin;
 	int no_commit;
 	int signoff;
+	struct strvec trailer_args;
 	int allow_ff;
 	int allow_rerere_auto;
 	int allow_empty;
@@ -82,8 +83,9 @@ struct replay_opts {
 	struct replay_ctx *ctx;
 };
 #define REPLAY_OPTS_INIT {			\
-	.edit = -1,				\
 	.action = -1,				\
+	.edit = -1,				\
+	.trailer_args = STRVEC_INIT, \
 	.xopts = STRVEC_INIT,			\
 	.ctx = replay_ctx_new(),		\
 }
diff --git a/t/meson.build b/t/meson.build
index c9ddd89889..6ebb08feca 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -384,6 +384,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..d0e0434664
--- /dev/null
+++ b/t/t3440-rebase-trailer.sh
@@ -0,0 +1,134 @@
+#!/bin/sh
+#
+
+test_description='git rebase --trailer integration tests
+We verify that --trailer works with the merge backend,
+and that it is rejected early when the apply backend is requested.'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh # test_commit_message, helpers
+
+REVIEWED_BY_TRAILER="Reviewed-by: Dev <dev@example.com>"
+
+expect_trailer_msg() {
+	test_commit_message "$1" <<-EOF
+	$2
+
+	${3:-$REVIEWED_BY_TRAILER}
+	EOF
+}
+
+test_expect_success 'setup repo with a small history' '
+	git commit --allow-empty -m "Initial empty commit" &&
+	test_commit first file a &&
+	test_commit second file &&
+	git checkout -b conflict-branch first &&
+	test_commit file-2 file-2 &&
+	test_commit conflict file &&
+	test_commit third file
+'
+
+test_expect_success 'apply backend is rejected with --trailer' '
+	head_before=$(git rev-parse HEAD) &&
+	test_expect_code 128 \
+	git rebase --apply --trailer "$REVIEWED_BY_TRAILER" \
+				HEAD^ 2>err &&
+	test_grep "fatal: --trailer requires the merge backend" err &&
+	test_cmp_rev HEAD $head_before
+'
+
+test_expect_success 'reject empty --trailer argument' '
+	test_expect_code 128 git rebase -m --trailer "" HEAD^ 2>err &&
+	test_grep "empty --trailer" err
+'
+
+test_expect_success 'reject trailer with missing key before separator' '
+	test_expect_code 128 git rebase -m --trailer ": no-key" HEAD^ 2>err &&
+	test_grep "missing key before separator" err
+'
+
+test_expect_success 'allow trailer with missing value after separator' '
+	git rebase -m --trailer "Acked-by:" HEAD~1 third &&
+	sed -e "s/_/ /g" <<-\EOF >expect &&
+	third
+
+	Acked-by:_
+	EOF
+	test_commit_message HEAD expect
+'
+
+test_expect_success 'CLI trailer duplicates allowed; replace policy keeps last' '
+	git -c trailer.Bug.ifexists=replace -c trailer.Bug.ifmissing=add \
+		rebase -m --trailer "Bug: 123" --trailer "Bug: 456" HEAD~1 third &&
+	cat >expect <<-\EOF &&
+	third
+
+	Bug: 456
+	EOF
+	test_commit_message HEAD expect
+'
+
+test_expect_success 'multiple Signed-off-by trailers all preserved' '
+	git rebase -m \
+			--trailer "Signed-off-by: Dev A <a@example.com>" \
+			--trailer "Signed-off-by: Dev B <b@example.com>" HEAD~1 third &&
+	cat >expect <<-\EOF &&
+	third
+
+	Signed-off-by: Dev A <a@example.com>
+	Signed-off-by: Dev B <b@example.com>
+	EOF
+	test_commit_message HEAD expect
+'
+
+test_expect_success 'rebase -m --trailer adds trailer after conflicts' '
+	git checkout -B conflict-branch third &&
+	test_commit fourth file &&
+	test_must_fail git rebase -m \
+			--trailer "$REVIEWED_BY_TRAILER" \
+			second &&
+	git checkout --theirs file &&
+	git add file &&
+	git rebase --continue &&
+	expect_trailer_msg HEAD "fourth" &&
+	expect_trailer_msg HEAD^ "third"
+'
+
+test_expect_success '--trailer handles fixup commands in todo list' '
+	git checkout -B fixup-trailer HEAD &&
+	test_commit fixup-base base &&
+	test_commit fixup-second second &&
+	first_short=$(git rev-parse --short fixup-base) &&
+	second_short=$(git rev-parse --short fixup-second) &&
+	cat >todo <<EOF &&
+pick $first_short fixup-base
+fixup $second_short fixup-second
+EOF
+	(
+		set_replace_editor todo &&
+		git rebase -i --trailer "$REVIEWED_BY_TRAILER" HEAD~2
+	) &&
+	expect_trailer_msg HEAD "fixup-base" &&
+	git reset --hard fixup-second &&
+	cat >todo <<EOF &&
+pick $first_short fixup-base
+fixup -C $second_short fixup-second
+EOF
+	(
+		set_replace_editor todo &&
+		git rebase -i --trailer "$REVIEWED_BY_TRAILER" HEAD~2
+	) &&
+	expect_trailer_msg HEAD "fixup-second"
+'
+
+test_expect_success 'rebase --root --trailer updates every commit' '
+	git checkout first &&
+	git -c trailer.review.key=Reviewed-by rebase --root \
+		--trailer=review="Dev <dev@example.com>" &&
+	expect_trailer_msg HEAD  "first" &&
+	expect_trailer_msg HEAD^ "Initial empty commit"
+'
+test_done
diff --git a/trailer.c b/trailer.c
index f5838f5699..f6ff2f01ee 100644
--- a/trailer.c
+++ b/trailer.c
@@ -7,6 +7,7 @@
 #include "string-list.h"
 #include "run-command.h"
 #include "commit.h"
+#include "strvec.h"
 #include "trailer.h"
 #include "list.h"
 #include "wrapper.h"
@@ -774,6 +775,30 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
 	free(cl_separators);
 }
 
+void validate_trailer_args_after_config(const struct strvec *cli_args)
+{
+	char *cl_separators;
+
+	trailer_config_init();
+
+	cl_separators = xstrfmt("=%s", separators);
+
+	for (size_t i = 0; i < cli_args->nr; i++) {
+		const char *txt = cli_args->v[i];
+		ssize_t separator_pos;
+
+		if (!*txt)
+			die(_("empty --trailer argument"));
+
+		separator_pos = find_separator(txt, cl_separators);
+		if (separator_pos == 0)
+			die(_("invalid trailer '%s': missing key before separator"),
+		    txt);
+	}
+
+	free(cl_separators);
+}
+
 static const char *next_line(const char *str)
 {
 	const char *nl = strchrnul(str, '\n');
@@ -1226,8 +1251,8 @@ void trailer_iterator_release(struct trailer_iterator *iter)
 	strbuf_release(&iter->key);
 }
 
-static int amend_strbuf_with_trailers(struct strbuf *buf,
-				      const struct strvec *trailer_args)
+int amend_strbuf_with_trailers(struct strbuf *buf,
+			       const struct strvec *trailer_args)
 {
 	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
 	LIST_HEAD(new_trailer_head);
diff --git a/trailer.h b/trailer.h
index daea46ca5d..541657a11f 100644
--- a/trailer.h
+++ b/trailer.h
@@ -68,6 +68,8 @@ void parse_trailers_from_config(struct list_head *config_head);
 void parse_trailers_from_command_line_args(struct list_head *arg_head,
 					   struct list_head *new_trailer_head);
 
+void validate_trailer_args_after_config(const struct strvec *cli_args);
+
 void process_trailers_lists(struct list_head *head,
 			    struct list_head *arg_head);
 
@@ -195,6 +197,9 @@ int trailer_iterator_advance(struct trailer_iterator *iter);
  */
 void trailer_iterator_release(struct trailer_iterator *iter);
 
+int amend_strbuf_with_trailers(struct strbuf *buf,
+			       const struct strvec *trailer_args);
+
 /*
  * Augment a file to add trailers to it (similar to 'git interpret-trailers').
  * Returns 0 on success or a non-zero error code on failure.
-- 
2.51.0

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

[Junio C Hamano]

Li Chen <me@linux.beauty> writes:

> From: Li Chen <chenl311@chinatelecom.cn>
>
> This series routes all trailer insertion through an in-process path, removing
> the fork/exec to builtin/interpret-trailers and tempfile juggling. 

This description makes it sound as if the code before this patch
series drove "interpret-trailers" via fork/exec and tempfile
juggling.  And that contradicts the title of the topic, "rebase:
support --trailer", which implies that the topic is about the "git
rebase" command, and that "git rebase" before this patch series did
not support trailers, not even with fork/exec and tempfile juggling.

Which is it?

I see trailer.c:amend_file_with_trailers() does fork out to the
"git interpret-trailers" command and is called by "git commit" and
"git tag".  Perhaps you are updating the amend_file_with_trailers()
helper function to do the in-process thing, so that "git commit" and
"git tag" no longer needs fork/exec and tempfile juggling?  

That would be great, regardless of "rebase", and if you used that
updated helper function to teach "rebase" to deal with trailers
in-process, that would be wonderful.

If the main part of the series (i.e. [1/4]-[4/4]) needs rerolling,
could you be a bit more careful when writing the cover letter to
make it easier for even those who are seeing this series for the
first time to understand what is going on?

> The first
> three commits centralize logic to reduce overhead and simplify error handling.

... in what code paths?  "In command X and Y where they do Z", "All
the call flows that lead to helper function F by eliminating the
need to do G that is costly and replacing it with H", etc., is what
I would expect to see in such a description.

> The final commit adds git rebase --trailer, currently supported
> with the merge backend only (rejecting apply-only scenarios and
> validating input early).

Sounds sensible.
Li Chen <me@linux.beauty> writes:

Re: [PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers()

[Junio C Hamano]

Li Chen <me@linux.beauty> writes:

> From: Li Chen <chenl311@chinatelecom.cn>
>
> Extracted trailer processing into a helper that accumulates output in
> a strbuf before writing.
>
> Updated interpret_trailers() to reuse the helper, buffer output, and
> clean up both input and output buffers after writing.

Imperative?

>
> Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
> ---
>  builtin/interpret-trailers.c | 51 ++++++++++++++++++++----------------
>  1 file changed, 29 insertions(+), 22 deletions(-)
>
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index 41b0750e5a..4c90580fff 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -136,32 +136,21 @@ 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)
> +static void process_trailers(const struct process_trailer_options *opts,
> +			     struct list_head *new_trailer_head,
> +			     struct strbuf *sb, struct strbuf *out)

So we gained *out strbuf; in the preimage below I see fwrite(),
fprintf(), etc. to outfile that is either stdout or tempfile, but
presumably the output all will be captured in the strbuf instead,
which makes sense.  It is a bit curious what the new paramater sb
is, but this is a file-scope static helper, so it does not strictly
require documenting.  Having a comment would still be nicer, though,
unlike "struct process_trailer_options" that is very limited
purpose, "strbuf" can be used for any string processing, so a good
variable name like "out" that conveys what it is used for by
implication is good, but "sb", which is obvious abbreviation for
"Str Buf", conveys no useful information.

>  {
>  	LIST_HEAD(head);
> -	struct strbuf sb = STRBUF_INIT;
> -	struct strbuf trailer_block_sb = STRBUF_INIT;

We no longer need a separate strbuf only for trailer block; we will
see why before we read through this helper function, hopefully.

>  	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);

OK, so the original code read the input (either "file", or standard
input) into a tempfile and prepared the output file stream.
Presumably it is now the responsibility of the caller of this new
function.  Initializing the trailer configuration is also what the
caller of this function is reponsible for, as well.

So this answers one of the questions I had upon starting to read
this function, i.e. "what is sb?"  It holds the input string, which
is what?  Something that look like a commit message that has title,
body and then a trailer block?  We may want to give the parameter a
better name?  I dunno (as this is file-scope static, as long as it
is obvious to the local caller, it may be OK, but on the other hand,
the caller needs to differenciate two strbuf parameters to the
helper function, one used for input and the other output, so if you
are calling the latter "out", perhaps you would want to call it
"in", or "input", perhaps?)

> -	trailer_block = parse_trailers(opts, sb.buf, &head);
> +	trailer_block = parse_trailers(opts, sb->buf, &head);

So we parse existing trailers from the input strbuf that is supplied
by the caller.  The rest of this hunk is rewriting FILE* I/O with
strbuf addition.

> @@ -173,22 +162,40 @@ static void interpret_trailers(const struct process_trailer_options *opts,
>  	}
>  
>  	/* Print trailer block. */
> -	format_trailers(opts, &head, &trailer_block_sb);
> +	format_trailers(opts, &head, out);
>  	free_trailers(&head);
> -	fwrite(trailer_block_sb.buf, 1, trailer_block_sb.len, outfile);
> -	strbuf_release(&trailer_block_sb);

The format_trailers() helper function appends appends to the strbuf
that is given to it, so instead of using an extra strbuf (and then
appending that to the output), we just pass our output strbuf to it,
which is why we no longer need the trailer_block_sb strbuf anymore.
Makes sense.

>  	/* 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);
> +		strbuf_add(out, sb->buf + trailer_block_end(trailer_block),
> +			   sb->len - trailer_block_end(trailer_block));
>  	trailer_block_release(trailer_block);
> +}

And again, FILE* I/O is replaced with appending to the output strbuf
in the rest of this helper function.  Good.

> +static void interpret_trailers(const struct process_trailer_options *opts,
> +			       struct list_head *new_trailer_head,
> +			       const char *file)

So the original caller of interpret_trailers() now call this outer
shell, which has the same name and the same function signature as
the original.  Our new process_trailers() helper assumes a handful
of preparatory steps are already done by the caller, so what we are
going read here will be mostly those preparation, a call to our new
helper, and then printing the result to "file" or standard output.

> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	struct strbuf out = STRBUF_INIT;
> +	FILE *outfile = stdout;
> +
> +	trailer_config_init();
> +
> +	read_input_file(&sb, file);
> +	if (opts->in_place)
> +		outfile = create_in_place_tempfile(file);

And these are exactly the lines we lost from the new helper.
Looking good.

> +	process_trailers(opts, new_trailer_head, &sb, &out);

And our call.  "out" should have what we wanted to output to
outfile, so ...

> +	fwrite(out.buf, out.len, 1, outfile);

... we write it out.  Good.  For a single long string that can never
have NUL in it, I'd personally find it more natural to call fputs(),
though.  Use of fwrite() makes readers unnecessarily wonder if there
is something unusual (like needing to be able to handle NULs in the
buffer).

>  	if (opts->in_place)
>  		if (rename_tempfile(&trailers_tempfile, file))
>  			die_errno(_("could not rename temporary file to %s"), file);
>
>  	strbuf_release(&sb);
> +	strbuf_release(&out);

OK.  We could release out a bit earlier, immediately after fwrite().

Looking mostly good.

>  }
>  
>  int cmd_interpret_trailers(int argc,

Re: [PATCH v6 2/4] trailer: move process_trailers to trailer.h

[Junio C Hamano]

Li Chen <me@linux.beauty> writes:

> From: Li Chen <chenl311@chinatelecom.cn>
>
> This function would be used by trailer_process
> in following commits.

Please make sure that the body is understandable without the title
of the commit.  Are you going to use process_trailers() from
trailer_process()?  Can the pair be named less confusingly?

> Subject: Re: [PATCH v6 2/4] trailer: move process_trailers to trailer.h

Declaring a helper that used to be a file-scope static to a public
header file is better described as "make process_trailers() public".

> diff --git a/trailer.h b/trailer.h
> index 4740549586..44d406b763 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -202,4 +202,7 @@ void trailer_iterator_release(struct trailer_iterator *iter);
>   */
>  int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);
>  

Befero the function, instruct potential future callers what this
function is about, what parameters it expects, and what side effect
it makes.  As pointed out in the previous step, "sb" definitely has
to be renamed if this becomes public.

> +void process_trailers(const struct process_trailer_options *opts,
> +		      struct list_head *new_trailer_head,
> +		      struct strbuf *sb, struct strbuf *out);
>  #endif /* TRAILER_H */

Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`

[Junio C Hamano]

Li Chen <me@linux.beauty> writes:

> From: Li Chen <chenl311@chinatelecom.cn>
>
> Route all trailer insertion through trailer_process() and make
> builtin/interpret-trailers just do file I/O before calling into it.
> amend_file_with_trailers() now shares the same code path.
>
> This removes the fork/exec and tempfile juggling, cutting overhead and
> simplifying error handling. No functional change. It also
> centralizes logic to prepare for follow-up rebase --trailer patch.
>
> Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
> ---
>  builtin/commit.c             |  2 +-
>  builtin/interpret-trailers.c | 46 +++---------------------
>  builtin/tag.c                |  3 +-
>  trailer.c                    | 68 +++++++++++++++++++++++++++++++-----
>  trailer.h                    |  5 ++-
>  wrapper.c                    | 16 +++++++++
>  wrapper.h                    |  6 ++++
>  7 files changed, 90 insertions(+), 56 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 0243f17d53..67070d6a54 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1719,7 +1719,7 @@ int cmd_commit(int argc,
>  		OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword specified commit")),
>  		OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
>  		OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
> -		OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG),
> +		OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, parse_opt_strvec),

What is this change for?

As the external interface of the amend_file_with_trailers() helper
did not change in this patch, this cannot be a change that is
required to "remove fork/exec and tempfile juggling".  

Or did amend_file_with_trailers() changed behaviour without changing
its function signature?  If so, this patch does too many things in a
single step, I am afraid.

Perhaps split this step further into multiple patches.

 - update the internal implementation of amend_file_with_trailers()
   to avoid having to fork/exec an external process, but *without*
   changing its external interface at all.  This step should not have
   to touch builtin/commit.c and builtin/tag.c at all.

 - if the strvec styled after passthru-argv is cumbersome to handle,
   perform the interface change, such as change from passthru-argv
   to bare strvec, as a separate step.

There might need another preparatory step to clean up the
interpret-trailers.c itself before the above two (or there may not
be---I haven't thought it through).

> diff --git a/wrapper.c b/wrapper.c
> index 3d507d4204..1f12dbb2fa 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -688,6 +688,22 @@ void write_file_buf(const char *path, const char *buf, size_t len)
> ...
> +int write_file_buf_gently(const char *path, const char *buf, size_t len)

I do not think this new helper is warranted.  You only call it from
one place anyway.

Re: [PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers()

[Phillip Wood]

On 05/11/2025 16:57, Junio C Hamano wrote:
> Li Chen <me@linux.beauty> writes:
> 
>> From: Li Chen <chenl311@chinatelecom.cn>
>>
>> Extracted trailer processing into a helper that accumulates output in
>> a strbuf before writing.
>>
>> Updated interpret_trailers() to reuse the helper, buffer output, and
>> clean up both input and output buffers after writing.
> 
> Imperative?
> 
>>
>> Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
>> ---
>>   builtin/interpret-trailers.c | 51 ++++++++++++++++++++----------------
>>   1 file changed, 29 insertions(+), 22 deletions(-)
>>
>> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
>> index 41b0750e5a..4c90580fff 100644
>> --- a/builtin/interpret-trailers.c
>> +++ b/builtin/interpret-trailers.c
>> @@ -136,32 +136,21 @@ 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)
>> +static void process_trailers(const struct process_trailer_options *opts,
>> +			     struct list_head *new_trailer_head,
>> +			     struct strbuf *sb, struct strbuf *out)
> 
> So we gained *out strbuf; in the preimage below I see fwrite(),
> fprintf(), etc. to outfile that is either stdout or tempfile, but
> presumably the output all will be captured in the strbuf instead,
> which makes sense.  It is a bit curious what the new paramater sb
> is, but this is a file-scope static helper, so it does not strictly
> require documenting.  Having a comment would still be nicer, though,
> unlike "struct process_trailer_options" that is very limited
> purpose, "strbuf" can be used for any string processing, so a good
> variable name like "out" that conveys what it is used for by
> implication is good, but "sb", which is obvious abbreviation for
> "Str Buf", conveys no useful information.

This patch is based on my suggestion[1]. I had intended to rename "sb" 
to "in" but forgot to do so before posting that diff. Here's my signoff 
which Li should add before their own

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>

Thanks

Phillip

[1] 
https://lore.kernel.org/git/7d12b046-365f-441c-af8e-8a39d61efbbd@gmail.com
>>   {
>>   	LIST_HEAD(head);
>> -	struct strbuf sb = STRBUF_INIT;
>> -	struct strbuf trailer_block_sb = STRBUF_INIT;
> 
> We no longer need a separate strbuf only for trailer block; we will
> see why before we read through this helper function, hopefully.
> 
>>   	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);
> 
> OK, so the original code read the input (either "file", or standard
> input) into a tempfile and prepared the output file stream.
> Presumably it is now the responsibility of the caller of this new
> function.  Initializing the trailer configuration is also what the
> caller of this function is reponsible for, as well.
> 
> So this answers one of the questions I had upon starting to read
> this function, i.e. "what is sb?"  It holds the input string, which
> is what?  Something that look like a commit message that has title,
> body and then a trailer block?  We may want to give the parameter a
> better name?  I dunno (as this is file-scope static, as long as it
> is obvious to the local caller, it may be OK, but on the other hand,
> the caller needs to differenciate two strbuf parameters to the
> helper function, one used for input and the other output, so if you
> are calling the latter "out", perhaps you would want to call it
> "in", or "input", perhaps?)
> 
>> -	trailer_block = parse_trailers(opts, sb.buf, &head);
>> +	trailer_block = parse_trailers(opts, sb->buf, &head);
> 
> So we parse existing trailers from the input strbuf that is supplied
> by the caller.  The rest of this hunk is rewriting FILE* I/O with
> strbuf addition.
> 
>> @@ -173,22 +162,40 @@ static void interpret_trailers(const struct process_trailer_options *opts,
>>   	}
>>   
>>   	/* Print trailer block. */
>> -	format_trailers(opts, &head, &trailer_block_sb);
>> +	format_trailers(opts, &head, out);
>>   	free_trailers(&head);
>> -	fwrite(trailer_block_sb.buf, 1, trailer_block_sb.len, outfile);
>> -	strbuf_release(&trailer_block_sb);
> 
> The format_trailers() helper function appends appends to the strbuf
> that is given to it, so instead of using an extra strbuf (and then
> appending that to the output), we just pass our output strbuf to it,
> which is why we no longer need the trailer_block_sb strbuf anymore.
> Makes sense.
> 
>>   	/* 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);
>> +		strbuf_add(out, sb->buf + trailer_block_end(trailer_block),
>> +			   sb->len - trailer_block_end(trailer_block));
>>   	trailer_block_release(trailer_block);
>> +}
> 
> And again, FILE* I/O is replaced with appending to the output strbuf
> in the rest of this helper function.  Good.
> 
>> +static void interpret_trailers(const struct process_trailer_options *opts,
>> +			       struct list_head *new_trailer_head,
>> +			       const char *file)
> 
> So the original caller of interpret_trailers() now call this outer
> shell, which has the same name and the same function signature as
> the original.  Our new process_trailers() helper assumes a handful
> of preparatory steps are already done by the caller, so what we are
> going read here will be mostly those preparation, a call to our new
> helper, and then printing the result to "file" or standard output.
> 
>> +{
>> +	struct strbuf sb = STRBUF_INIT;
>> +	struct strbuf out = STRBUF_INIT;
>> +	FILE *outfile = stdout;
>> +
>> +	trailer_config_init();
>> +
>> +	read_input_file(&sb, file);
>> +	if (opts->in_place)
>> +		outfile = create_in_place_tempfile(file);
> 
> And these are exactly the lines we lost from the new helper.
> Looking good.
> 
>> +	process_trailers(opts, new_trailer_head, &sb, &out);
> 
> And our call.  "out" should have what we wanted to output to
> outfile, so ...
> 
>> +	fwrite(out.buf, out.len, 1, outfile);
> 
> ... we write it out.  Good.  For a single long string that can never
> have NUL in it, I'd personally find it more natural to call fputs(),
> though.  Use of fwrite() makes readers unnecessarily wonder if there
> is something unusual (like needing to be able to handle NULs in the
> buffer).
> 
>>   	if (opts->in_place)
>>   		if (rename_tempfile(&trailers_tempfile, file))
>>   			die_errno(_("could not rename temporary file to %s"), file);
>>
>>   	strbuf_release(&sb);
>> +	strbuf_release(&out);
> 
> OK.  We could release out a bit earlier, immediately after fwrite().
> 
> Looking mostly good.
> 
>>   }
>>   
>>   int cmd_interpret_trailers(int argc,
> 

Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`

[Phillip Wood]

Hi Li

On 05/11/2025 14:29, Li Chen wrote:
> From: Li Chen <chenl311@chinatelecom.cn>
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 0243f17d53..67070d6a54 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1719,7 +1719,7 @@ int cmd_commit(int argc,
>   		OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword specified commit")),
>   		OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
>   		OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
> -		OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG),

We have OPT_STRVEC to handle this. The commit message should explain why 
we're doing this (because we only want to pass the value to 
amend_file_with_trailers()). Alternatively we could use skip_prefix() in 
amend_file_with_trailers() to skip the "--trailer=" prefix in this patch 
and then clean it in a separate patch.

> +		OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, parse_opt_strvec),
>   		OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),
>   		OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
>   		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index bce2e791d6..268a43372b 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> 
> @@ -142,21 +110,15 @@ static void interpret_trailers(const struct process_trailer_options *opts,
>   {
>   	struct strbuf sb = STRBUF_INIT;
>   	struct strbuf out = STRBUF_INIT;
> -	FILE *outfile = stdout;
> -
> -	trailer_config_init();

Why is this being moved?
>   	read_input_file(&sb, file);
>   
> -	if (opts->in_place)
> -		outfile = create_in_place_tempfile(file);
> -
>   	process_trailers(opts, new_trailer_head, &sb, &out);
>   
> -	fwrite(out.buf, out.len, 1, outfile);
>   	if (opts->in_place)
> -		if (rename_tempfile(&trailers_tempfile, file))
> -			die_errno(_("could not rename temporary file to %s"), file);
> +		write_file_buf(file, out.buf, out.len);

This truncates the existing file which means that if there is a error 
while writing the new version the user is now left with garbage rather 
than the original file which does not seem like a good idea.

 > diff --git a/trailer.c b/trailer.c> index b735ec8a53..f5838f5699 100644
> --- a/trailer.c
> +++ b/trailer.c
> 
> @@ -1224,18 +1226,66 @@ 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 *text = trailer_args->v[i];
> +		struct new_trailer_item *item;
> +
> +		if (!*text)
> +			continue;

Isn't it an error to pass an empty argument to "--trailer"?

> +		item = xcalloc(1, sizeof(*item));
> +		INIT_LIST_HEAD(&item->list);

I don't think we need this as "item->prev" and "item->next" are set by 
list_add_tail() below.

We initialize "where", "if_exists" and "if_missing" to zero which 
matches what builtin/interpret-trailers.c does if the user does not 
specify any of those options - good.

> +		item->text = text;
> +		list_add_tail(&item->list, &new_trailer_head);
> +	}
> +
> +	process_trailers(&opts, &new_trailer_head, buf, &out);
> +
> +	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);

We have free_trailers() to do this for us.

> +	}
> +	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;

Isn't it a bug to pass a NULL trailer_args?

> +	if (strbuf_read_file(&buf, path, 0) < 0)
> +		return error_errno("could not read '%s'", path);
> +
> +	if (amend_strbuf_with_trailers(&buf, trailer_args)) {
> +		strbuf_release(&buf);
> +		return error("failed to append trailers");
> +	}
> +
> +	if (write_file_buf_gently(path, buf.buf, buf.len)) {
> +		strbuf_release(&buf);
> +		return -1;
> +	}
> +
> +	strbuf_release(&buf);
> +	return 0;
> + }

This looks like a faithful conversion of the original with the caveat 
that it expects to be passed an array of trailer arguments without the 
"--trailer=" prefix. Good

I'll take a look at patch 4 tomorrow but so far these version is looking 
much nicer than the last round.

Thanks

Phillip

>   void process_trailers(const struct process_trailer_options *opts,
>   		      struct list_head *new_trailer_head,
>   		      struct strbuf *sb, struct strbuf *out)
> diff --git a/trailer.h b/trailer.h
> index 44d406b763..daea46ca5d 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -196,9 +196,8 @@ 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);
>   
> diff --git a/wrapper.c b/wrapper.c
> index 3d507d4204..1f12dbb2fa 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -688,6 +688,22 @@ void write_file_buf(const char *path, const char *buf, size_t len)
>   		die_errno(_("could not close '%s'"), path);
>   }
>   
> +int write_file_buf_gently(const char *path, const char *buf, size_t len)
> +{
> +	int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
> +
> +	if (fd < 0)
> +		return error_errno(_("could not open '%s'"), path);
> +	if (write_in_full(fd, buf, len) < 0) {
> +		int ret = error_errno(_("could not write to '%s'"), path);
> +		close(fd);
> +		return ret;
> +	}
> +	if (close(fd))
> +		return error_errno(_("could not close '%s'"), path);
> +	return 0;
> +}
> +
>   void write_file(const char *path, const char *fmt, ...)
>   {
>   	va_list params;
> diff --git a/wrapper.h b/wrapper.h
> index 44a8597ac3..e5f867b200 100644
> --- a/wrapper.h
> +++ b/wrapper.h
> @@ -56,6 +56,12 @@ static inline ssize_t write_str_in_full(int fd, const char *str)
>    */
>   void write_file_buf(const char *path, const char *buf, size_t len);
>   
> +/**
> + * Like write_file_buf(), but report errors instead of exiting. Returns 0 on
> + * success or a negative value on error after emitting a message.
> + */
> +int write_file_buf_gently(const char *path, const char *buf, size_t len);
> +
>   /**
>    * Like write_file_buf(), but format the contents into a buffer first.
>    * Additionally, write_file() will append a newline if one is not already

Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`

[Li Chen]

Hi Phillip,

 ---- On Tue, 11 Nov 2025 00:38:55 +0800  Phillip Wood <phillip.wood123@gmail.com> wrote --- 
 > Hi Li
 > 
 > On 05/11/2025 14:29, Li Chen wrote:
 > > From: Li Chen <chenl311@chinatelecom.cn>
 > > 
 > > diff --git a/builtin/commit.c b/builtin/commit.c
 > > index 0243f17d53..67070d6a54 100644
 > > --- a/builtin/commit.c
 > > +++ b/builtin/commit.c
 > > @@ -1719,7 +1719,7 @@ int cmd_commit(int argc,
 > >           OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword specified commit")),
 > >           OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
 > >           OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
 > > -        OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG),
 > 
 > We have OPT_STRVEC to handle this. The commit message should explain why 
 > we're doing this (because we only want to pass the value to 
 > amend_file_with_trailers()). Alternatively we could use skip_prefix() in 
 > amend_file_with_trailers() to skip the "--trailer=" prefix in this patch 
 > and then clean it in a separate patch.

Thanks for the reminder, I will try to split into two patches in the next reversion. The first one
use skip_prefix() in amend_file_with_trailers(), and the second one switch to amend_file_with_trailers().

 > 
 > > +        OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, parse_opt_strvec),
 > >           OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),
 > >           OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
 > >           OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
 > > diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
 > > index bce2e791d6..268a43372b 100644
 > > --- a/builtin/interpret-trailers.c
 > > +++ b/builtin/interpret-trailers.c
 > > 
 > > @@ -142,21 +110,15 @@ static void interpret_trailers(const struct process_trailer_options *opts,
 > >   {
 > >       struct strbuf sb = STRBUF_INIT;
 > >       struct strbuf out = STRBUF_INIT;
 > > -    FILE *outfile = stdout;
 > > -
 > > -    trailer_config_init();
 > 
 > Why is this being moved?

Since trailer_config_init only needs to run once, it's better to move it outside the cmd_interpret_trailers loop,
even though it already uses a configured global variable.

 > >       read_input_file(&sb, file);
 > >   
 > > -    if (opts->in_place)
 > > -        outfile = create_in_place_tempfile(file);
 > > -
 > >       process_trailers(opts, new_trailer_head, &sb, &out);
 > >   
 > > -    fwrite(out.buf, out.len, 1, outfile);
 > >       if (opts->in_place)
 > > -        if (rename_tempfile(&trailers_tempfile, file))
 > > -            die_errno(_("could not rename temporary file to %s"), file);
 > > +        write_file_buf(file, out.buf, out.len);
 > 
 > This truncates the existing file which means that if there is a error 
 > while writing the new version the user is now left with garbage rather 
 > than the original file which does not seem like a good idea.

Thanks for catching this. I'll switch back to using a temp file for atomic.

 > 
 >  > diff --git a/trailer.c b/trailer.c> index b735ec8a53..f5838f5699 100644
 > > --- a/trailer.c
 > > +++ b/trailer.c
 > > 
 > > @@ -1224,18 +1226,66 @@ 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 *text = trailer_args->v[i];
 > > +        struct new_trailer_item *item;
 > > +
 > > +        if (!*text)
 > > +            continue;
 > 
 > Isn't it an error to pass an empty argument to "--trailer"?
 
Nice catch, I would refactor amend_strbuf_with_trailers to return error(_("empty --trailer argument"));
here and handle resource cleanup then make amend_file_with_trailers return this error.

 > > +        item = xcalloc(1, sizeof(*item));
 > > +        INIT_LIST_HEAD(&item->list);
 > 
 > I don't think we need this as "item->prev" and "item->next" are set by 
 > list_add_tail() below.
 
ok, I would remove this.

 > We initialize "where", "if_exists" and "if_missing" to zero which 
 > matches what builtin/interpret-trailers.c does if the user does not 
 > specify any of those options - good.
 > 
 > > +        item->text = text;
 > > +        list_add_tail(&item->list, &new_trailer_head);
 > > +    }
 > > +
 > > +    process_trailers(&opts, &new_trailer_head, buf, &out);
 > > +
 > > +    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);
 > 
 > We have free_trailers() to do this for us.
 
I would replace them with free_trailers.

 > > +    }
 > > +    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;
 > 
 > Isn't it a bug to pass a NULL trailer_args?
 
Sounds right, I would let it return an error msg.

 > > +    if (strbuf_read_file(&buf, path, 0) < 0)
 > > +        return error_errno("could not read '%s'", path);
 > > +
 > > +    if (amend_strbuf_with_trailers(&buf, trailer_args)) {
 > > +        strbuf_release(&buf);
 > > +        return error("failed to append trailers");
 > > +    }
 > > +
 > > +    if (write_file_buf_gently(path, buf.buf, buf.len)) {
 > > +        strbuf_release(&buf);
 > > +        return -1;
 > > +    }
 > > +
 > > +    strbuf_release(&buf);
 > > +    return 0;
 > > + }
 > 
 > This looks like a faithful conversion of the original with the caveat 
 > that it expects to be passed an array of trailer arguments without the 
 > "--trailer=" prefix. Good

Okay, thanks. Junio notes that write_file_buf_gently is only used in this context and
 doesn't need wrok as a helper function. I'll replace it with an in-place operation.

 > I'll take a look at patch 4 tomorrow but so far these version is looking 
 > much nicer than the last round.

Thanks a lot.

Regards,

Li​

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

[Li Chen]

Hi Junio,

 ---- On Thu, 06 Nov 2025 00:30:04 +0800  Junio C Hamano <gitster@pobox.com> wrote --- 
 > Li Chen <me@linux.beauty> writes:
 > 
 > > From: Li Chen <chenl311@chinatelecom.cn>
 > >
 > > This series routes all trailer insertion through an in-process path, removing
 > > the fork/exec to builtin/interpret-trailers and tempfile juggling. 
 > 
 > This description makes it sound as if the code before this patch
 > series drove "interpret-trailers" via fork/exec and tempfile
 > juggling.  And that contradicts the title of the topic, "rebase:
 > support --trailer", which implies that the topic is about the "git
 > rebase" command, and that "git rebase" before this patch series did
 > not support trailers, not even with fork/exec and tempfile juggling.
 > 
 > Which is it?
 > 
 > I see trailer.c:amend_file_with_trailers() does fork out to the
 > "git interpret-trailers" command and is called by "git commit" and
 > "git tag".  Perhaps you are updating the amend_file_with_trailers()
 > helper function to do the in-process thing, so that "git commit" and
 > "git tag" no longer needs fork/exec and tempfile juggling?  
 > 
 > That would be great, regardless of "rebase", and if you used that
 > updated helper function to teach "rebase" to deal with trailers
 > in-process, that would be wonderful.
 > 
 > If the main part of the series (i.e. [1/4]-[4/4]) needs rerolling,
 > could you be a bit more careful when writing the cover letter to
 > make it easier for even those who are seeing this series for the
 > first time to understand what is going on?
 > 
 > > The first
 > > three commits centralize logic to reduce overhead and simplify error handling.
 > 
 > ... in what code paths?  "In command X and Y where they do Z", "All
 > the call flows that lead to helper function F by eliminating the
 > need to do G that is costly and replacing it with H", etc., is what
 > I would expect to see in such a description.
 > 
 > > The final commit adds git rebase --trailer, currently supported
 > > with the merge backend only (rejecting apply-only scenarios and
 > > validating input early).
 > 
 > Sounds sensible.
 > Li Chen <me@linux.beauty> writes:
 > 
 > 

Thanks for reviewing the cover letter. I will incorporate your suggestions to improve 
its clarity and effectiveness in the next version.

Regards,

Li​

Re: [PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers()

[Li Chen]

Hi Junio,


 ---- On Thu, 06 Nov 2025 00:57:27 +0800  Junio C Hamano <gitster@pobox.com> wrote --- 
 > Li Chen <me@linux.beauty> writes:
 > 
 > > From: Li Chen <chenl311@chinatelecom.cn>
 > >
 > > Extracted trailer processing into a helper that accumulates output in
 > > a strbuf before writing.
 > >
 > > Updated interpret_trailers() to reuse the helper, buffer output, and
 > > clean up both input and output buffers after writing.
 > 
 > Imperative?
 > 
 > >
 > > Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
 > > ---
 > >  builtin/interpret-trailers.c | 51 ++++++++++++++++++++----------------
 > >  1 file changed, 29 insertions(+), 22 deletions(-)
 > >
 > > diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
 > > index 41b0750e5a..4c90580fff 100644
 > > --- a/builtin/interpret-trailers.c
 > > +++ b/builtin/interpret-trailers.c
 > > @@ -136,32 +136,21 @@ 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)
 > > +static void process_trailers(const struct process_trailer_options *opts,
 > > +                 struct list_head *new_trailer_head,
 > > +                 struct strbuf *sb, struct strbuf *out)
 > 
 > So we gained *out strbuf; in the preimage below I see fwrite(),
 > fprintf(), etc. to outfile that is either stdout or tempfile, but
 > presumably the output all will be captured in the strbuf instead,
 > which makes sense.  It is a bit curious what the new paramater sb
 > is, but this is a file-scope static helper, so it does not strictly
 > require documenting.  Having a comment would still be nicer, though,
 > unlike "struct process_trailer_options" that is very limited
 > purpose, "strbuf" can be used for any string processing, so a good
 > variable name like "out" that conveys what it is used for by
 > implication is good, but "sb", which is obvious abbreviation for
 > "Str Buf", conveys no useful information.

Thanks, I would rename the variable in next version.

 > 
 > >  {
 > >      LIST_HEAD(head);
 > > -    struct strbuf sb = STRBUF_INIT;
 > > -    struct strbuf trailer_block_sb = STRBUF_INIT;
 > 
 > We no longer need a separate strbuf only for trailer block; we will
 > see why before we read through this helper function, hopefully.
 > 
 > >      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);
 > 
 > OK, so the original code read the input (either "file", or standard
 > input) into a tempfile and prepared the output file stream.
 > Presumably it is now the responsibility of the caller of this new
 > function.  Initializing the trailer configuration is also what the
 > caller of this function is reponsible for, as well.
 > 
 > So this answers one of the questions I had upon starting to read
 > this function, i.e. "what is sb?"  It holds the input string, which
 > is what?  Something that look like a commit message that has title,
 > body and then a trailer block?  We may want to give the parameter a
 > better name?  I dunno (as this is file-scope static, as long as it
 > is obvious to the local caller, it may be OK, but on the other hand,
 > the caller needs to differenciate two strbuf parameters to the
 > helper function, one used for input and the other output, so if you
 > are calling the latter "out", perhaps you would want to call it
 > "in", or "input", perhaps?)

Yes, in is a better name.

 > 
 > > -    trailer_block = parse_trailers(opts, sb.buf, &head);
 > > +    trailer_block = parse_trailers(opts, sb->buf, &head);
 > 
 > So we parse existing trailers from the input strbuf that is supplied
 > by the caller.  The rest of this hunk is rewriting FILE* I/O with
 > strbuf addition.
 > 
 > > @@ -173,22 +162,40 @@ static void interpret_trailers(const struct process_trailer_options *opts,
 > >      }
 > >  
 > >      /* Print trailer block. */
 > > -    format_trailers(opts, &head, &trailer_block_sb);
 > > +    format_trailers(opts, &head, out);
 > >      free_trailers(&head);
 > > -    fwrite(trailer_block_sb.buf, 1, trailer_block_sb.len, outfile);
 > > -    strbuf_release(&trailer_block_sb);
 > 
 > The format_trailers() helper function appends appends to the strbuf
 > that is given to it, so instead of using an extra strbuf (and then
 > appending that to the output), we just pass our output strbuf to it,
 > which is why we no longer need the trailer_block_sb strbuf anymore.
 > Makes sense.
 > 
 > >      /* 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);
 > > +        strbuf_add(out, sb->buf + trailer_block_end(trailer_block),
 > > +               sb->len - trailer_block_end(trailer_block));
 > >      trailer_block_release(trailer_block);
 > > +}
 > 
 > And again, FILE* I/O is replaced with appending to the output strbuf
 > in the rest of this helper function.  Good.
 > 
 > > +static void interpret_trailers(const struct process_trailer_options *opts,
 > > +                   struct list_head *new_trailer_head,
 > > +                   const char *file)
 > 
 > So the original caller of interpret_trailers() now call this outer
 > shell, which has the same name and the same function signature as
 > the original.  Our new process_trailers() helper assumes a handful
 > of preparatory steps are already done by the caller, so what we are
 > going read here will be mostly those preparation, a call to our new
 > helper, and then printing the result to "file" or standard output.
 > 
 > > +{
 > > +    struct strbuf sb = STRBUF_INIT;
 > > +    struct strbuf out = STRBUF_INIT;
 > > +    FILE *outfile = stdout;
 > > +
 > > +    trailer_config_init();
 > > +
 > > +    read_input_file(&sb, file);
 > > +    if (opts->in_place)
 > > +        outfile = create_in_place_tempfile(file);
 > 
 > And these are exactly the lines we lost from the new helper.
 > Looking good.
 > 
 > > +    process_trailers(opts, new_trailer_head, &sb, &out);
 > 
 > And our call.  "out" should have what we wanted to output to
 > outfile, so ...
 > 
 > > +    fwrite(out.buf, out.len, 1, outfile);
 > 
 > ... we write it out.  Good.  For a single long string that can never
 > have NUL in it, I'd personally find it more natural to call fputs(),
 > though.  Use of fwrite() makes readers unnecessarily wonder if there
 > is something unusual (like needing to be able to handle NULs in the
 > buffer).
 > 
 > >      if (opts->in_place)
 > >          if (rename_tempfile(&trailers_tempfile, file))
 > >              die_errno(_("could not rename temporary file to %s"), file);
 > >
 > >      strbuf_release(&sb);
 > > +    strbuf_release(&out);
 > 
 > OK.  We could release out a bit earlier, immediately after fwrite().
 > 
 > Looking mostly good.
 > 
 > >  }
 > >  
 > >  int cmd_interpret_trailers(int argc,
 > 

Regards,

Li​

Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`

[Li Chen]

Hi Junio,

 ---- On Thu, 06 Nov 2025 01:56:00 +0800  Junio C Hamano <gitster@pobox.com> wrote --- 
 > Li Chen <me@linux.beauty> writes:
 > 
 > > From: Li Chen <chenl311@chinatelecom.cn>
 > >
 > > Route all trailer insertion through trailer_process() and make
 > > builtin/interpret-trailers just do file I/O before calling into it.
 > > amend_file_with_trailers() now shares the same code path.
 > >
 > > This removes the fork/exec and tempfile juggling, cutting overhead and
 > > simplifying error handling. No functional change. It also
 > > centralizes logic to prepare for follow-up rebase --trailer patch.
 > >
 > > Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
 > > ---
 > >  builtin/commit.c             |  2 +-
 > >  builtin/interpret-trailers.c | 46 +++---------------------
 > >  builtin/tag.c                |  3 +-
 > >  trailer.c                    | 68 +++++++++++++++++++++++++++++++-----
 > >  trailer.h                    |  5 ++-
 > >  wrapper.c                    | 16 +++++++++
 > >  wrapper.h                    |  6 ++++
 > >  7 files changed, 90 insertions(+), 56 deletions(-)
 > >
 > > diff --git a/builtin/commit.c b/builtin/commit.c
 > > index 0243f17d53..67070d6a54 100644
 > > --- a/builtin/commit.c
 > > +++ b/builtin/commit.c
 > > @@ -1719,7 +1719,7 @@ int cmd_commit(int argc,
 > >          OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword specified commit")),
 > >          OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
 > >          OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
 > > -        OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG),
 > > +        OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, parse_opt_strvec),
 > 
 > What is this change for?
 > As the external interface of the amend_file_with_trailers() helper
 > did not change in this patch, this cannot be a change that is
 > required to "remove fork/exec and tempfile juggling".  
 > 
 > Or did amend_file_with_trailers() changed behaviour without changing
 > its function signature?  If so, this patch does too many things in a
 > single step, I am afraid.

it allows remove the use of skip_prefix in amend_file_with_trailers, and I would add seperate
patches to make this clearer.

 > Perhaps split this step further into multiple patches.
 > 
 >  - update the internal implementation of amend_file_with_trailers()
 >    to avoid having to fork/exec an external process, but *without*
 >    changing its external interface at all.  This step should not have
 >    to touch builtin/commit.c and builtin/tag.c at all.
 > 
 >  - if the strvec styled after passthru-argv is cumbersome to handle,
 >    perform the interface change, such as change from passthru-argv
 >    to bare strvec, as a separate step.
 > 
 > There might need another preparatory step to clean up the
 > interpret-trailers.c itself before the above two (or there may not
 > be---I haven't thought it through).

Thanks, I would split into multiple patches in next version.

 > 
 > > diff --git a/wrapper.c b/wrapper.c
 > > index 3d507d4204..1f12dbb2fa 100644
 > > --- a/wrapper.c
 > > +++ b/wrapper.c
 > > @@ -688,6 +688,22 @@ void write_file_buf(const char *path, const char *buf, size_t len)
 > > ...
 > > +int write_file_buf_gently(const char *path, const char *buf, size_t len)
 > 
 > I do not think this new helper is warranted.  You only call it from
 > one place anyway.

ok, I would remove write_file_buf_gently and do it in-place.

Regards,

Li​

Re: [PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers()

[Li Chen]

Hi Phillip,


 ---- On Tue, 11 Nov 2025 00:27:38 +0800  Phillip Wood <phillip.wood123@gmail.com> wrote --- 
 > On 05/11/2025 16:57, Junio C Hamano wrote:
 > > Li Chen <me@linux.beauty> writes:
 > > 
 > >> From: Li Chen <chenl311@chinatelecom.cn>
 > >>
 > >> Extracted trailer processing into a helper that accumulates output in
 > >> a strbuf before writing.
 > >>
 > >> Updated interpret_trailers() to reuse the helper, buffer output, and
 > >> clean up both input and output buffers after writing.
 > > 
 > > Imperative?
 > > 
 > >>
 > >> Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
 > >> ---
 > >>   builtin/interpret-trailers.c | 51 ++++++++++++++++++++----------------
 > >>   1 file changed, 29 insertions(+), 22 deletions(-)
 > >>
 > >> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
 > >> index 41b0750e5a..4c90580fff 100644
 > >> --- a/builtin/interpret-trailers.c
 > >> +++ b/builtin/interpret-trailers.c
 > >> @@ -136,32 +136,21 @@ 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)
 > >> +static void process_trailers(const struct process_trailer_options *opts,
 > >> +                 struct list_head *new_trailer_head,
 > >> +                 struct strbuf *sb, struct strbuf *out)
 > > 
 > > So we gained *out strbuf; in the preimage below I see fwrite(),
 > > fprintf(), etc. to outfile that is either stdout or tempfile, but
 > > presumably the output all will be captured in the strbuf instead,
 > > which makes sense.  It is a bit curious what the new paramater sb
 > > is, but this is a file-scope static helper, so it does not strictly
 > > require documenting.  Having a comment would still be nicer, though,
 > > unlike "struct process_trailer_options" that is very limited
 > > purpose, "strbuf" can be used for any string processing, so a good
 > > variable name like "out" that conveys what it is used for by
 > > implication is good, but "sb", which is obvious abbreviation for
 > > "Str Buf", conveys no useful information.
 > 
 > This patch is based on my suggestion[1]. I had intended to rename "sb" 
 > to "in" but forgot to do so before posting that diff. Here's my signoff 
 > which Li should add before their own
 > 
 > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>

I'm sorry that your signoff is missing; I will add it in the next version.

Regards,

Li​

Re: [PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers()

[Junio C Hamano]

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

> This patch is based on my suggestion[1]. I had intended to rename "sb" 
> to "in" but forgot to do so before posting that diff. Here's my signoff 
> which Li should add before their own
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Thanks
>
> Phillip

Ah, thanks for clarifying the origin of this patch.

Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`

[Phillip Wood]

Hi Li

On 05/11/2025 14:29, Li Chen wrote:
> From: Li Chen <chenl311@chinatelecom.cn>
> 
> diff --git a/trailer.c b/trailer.c
> index b735ec8a53..f5838f5699 100644
> --- a/trailer.c
> +++ b/trailer.c
> 
> @@ -1224,18 +1226,66 @@ 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)

While reviewing patch 4 I've just realized that this function can never 
fail so should return "void" rather than "int". I've not quite finished 
with patch 4 yet, hopefully I'll post a review tomorrow.

Thanks

Phillip

>   {
> -	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 *text = trailer_args->v[i];
> +		struct new_trailer_item *item;
> +
> +		if (!*text)
> +			continue;
> +		item = xcalloc(1, sizeof(*item));
> +		INIT_LIST_HEAD(&item->list);
> +		item->text = text;
> +		list_add_tail(&item->list, &new_trailer_head);
> +	}
> +
> +	process_trailers(&opts, &new_trailer_head, buf, &out);
> +
> +	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 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)) {
> +		strbuf_release(&buf);
> +		return error("failed to append trailers");
> +	}
> +
> +	if (write_file_buf_gently(path, buf.buf, buf.len)) {
> +		strbuf_release(&buf);
> +		return -1;
> +	}
> +
> +	strbuf_release(&buf);
> +	return 0;
> + }
> +
>   void process_trailers(const struct process_trailer_options *opts,
>   		      struct list_head *new_trailer_head,
>   		      struct strbuf *sb, struct strbuf *out)
> diff --git a/trailer.h b/trailer.h
> index 44d406b763..daea46ca5d 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -196,9 +196,8 @@ 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);
>   
> diff --git a/wrapper.c b/wrapper.c
> index 3d507d4204..1f12dbb2fa 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -688,6 +688,22 @@ void write_file_buf(const char *path, const char *buf, size_t len)
>   		die_errno(_("could not close '%s'"), path);
>   }
>   
> +int write_file_buf_gently(const char *path, const char *buf, size_t len)
> +{
> +	int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
> +
> +	if (fd < 0)
> +		return error_errno(_("could not open '%s'"), path);
> +	if (write_in_full(fd, buf, len) < 0) {
> +		int ret = error_errno(_("could not write to '%s'"), path);
> +		close(fd);
> +		return ret;
> +	}
> +	if (close(fd))
> +		return error_errno(_("could not close '%s'"), path);
> +	return 0;
> +}
> +
>   void write_file(const char *path, const char *fmt, ...)
>   {
>   	va_list params;
> diff --git a/wrapper.h b/wrapper.h
> index 44a8597ac3..e5f867b200 100644
> --- a/wrapper.h
> +++ b/wrapper.h
> @@ -56,6 +56,12 @@ static inline ssize_t write_str_in_full(int fd, const char *str)
>    */
>   void write_file_buf(const char *path, const char *buf, size_t len);
>   
> +/**
> + * Like write_file_buf(), but report errors instead of exiting. Returns 0 on
> + * success or a negative value on error after emitting a message.
> + */
> +int write_file_buf_gently(const char *path, const char *buf, size_t len);
> +
>   /**
>    * Like write_file_buf(), but format the contents into a buffer first.
>    * Additionally, write_file() will append a newline if one is not already

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

[Phillip Wood]

Hi Li

On 05/11/2025 14:29, Li Chen wrote:
> From: Li Chen <chenl311@chinatelecom.cn>
> 
> diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc
> index 005caf6164..4d2fe4be6e 100644
> --- a/Documentation/git-rebase.adoc
> +++ b/Documentation/git-rebase.adoc
> @@ -487,9 +487,16 @@ See also INCOMPATIBLE OPTIONS below.
>   	Add a `Signed-off-by` trailer to all the rebased commits. Note
>   	that if `--interactive` is given then only commits marked to be
>   	picked, edited or reworded will have the trailer added.
> -+
> +
>   See also INCOMPATIBLE OPTIONS below.
>   
> +--trailer=<trailer>::
> +       Append the given trailer line(s) to every rebased commit

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

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

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

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

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

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

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

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

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

> diff --git a/sequencer.c b/sequencer.c
> index 5476d39ba9..fbf35cb474 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2025,6 +2027,10 @@ static int append_squash_message(struct strbuf *buf, const char *body,
>   		if (opts->signoff)
>   			append_signoff(buf, 0, 0);
>   
> +		if (opts->trailer_args.nr &&
> +			amend_strbuf_with_trailers(buf, &opts->trailer_args))
> +			return error(_("unable to add trailers to commit message"));

amend_strbuf_with_trailers() cannot really fail so this should be

		if (opts->trailer_args.nr)
			amend_strbuf_with_trailers(buf, &opts->trailer_args));

> @@ -2443,6 +2449,14 @@ static int do_pick_commit(struct repository *r,
>   	if (opts->signoff && !is_fixup(command))
>   		append_signoff(&ctx->message, 0, 0);
>   
> +	if (opts->trailer_args.nr && !is_fixup(command)) {
> +		if (amend_strbuf_with_trailers(&ctx->message,
> +					       &opts->trailer_args)) {
> +			res = error(_("unable to add trailers to commit message"));
> +			goto leave;
> +		}

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

> +	}
> +
>   	if (is_rebase_i(opts) && write_author_script(msg.message) < 0)
>   		res = -1;
>   	else if (!opts->strategy ||
> @@ -2517,6 +2531,7 @@ static int do_pick_commit(struct repository *r,
>   			_("dropping %s %s -- patch contents already upstream\n"),
>   			oid_to_hex(&commit->object.oid), msg.subject);
>   	} /* else allow == 0 and there's nothing special to do */
> +

We try to avoid introducing unrelated white space changes

> @@ -3234,6 +3249,17 @@ static int read_populate_opts(struct replay_opts *opts)
>   
>   		read_strategy_opts(opts, &buf);
>   		strbuf_reset(&buf);
> +		if (strbuf_read_file(&buf, rebase_path_trailer(), 0) >= 0) {
> +			char *p = buf.buf, *nl;
> +
> +			while ((nl = strchr(p, '\n'))) {
> +				*nl = '\0';
> +				if (*p)

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

> diff --git a/sequencer.h b/sequencer.h
> index 719684c8a9..e21835c5a0 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -44,6 +44,7 @@ struct replay_opts {
>   	int record_origin;
>   	int no_commit;
>   	int signoff;
> +	struct strvec trailer_args;

I think it would be better to add this after all the flag options

>   	int allow_ff;
>   	int allow_rerere_auto;
>   	int allow_empty;
> @@ -82,8 +83,9 @@ struct replay_opts {
>   	struct replay_ctx *ctx;
>   };
>   #define REPLAY_OPTS_INIT {			\
> -	.edit = -1,				\

".edit" is the first member so it makes sense to leave this at the start.

>   	.action = -1,				\
> +	.edit = -1,				\
> +	.trailer_args = STRVEC_INIT, \
>   	.xopts = STRVEC_INIT,			\
>   	.ctx = replay_ctx_new(),		\
>   }

> diff --git a/t/t3440-rebase-trailer.sh b/t/t3440-rebase-trailer.sh
> new file mode 100755
> index 0000000000..d0e0434664
> --- /dev/null
> +++ b/t/t3440-rebase-trailer.sh
> @@ -0,0 +1,134 @@
> +#!/bin/sh
> +#
> +
> +test_description='git rebase --trailer integration tests
> +We verify that --trailer works with the merge backend,
> +and that it is rejected early when the apply backend is requested.'
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-rebase.sh # test_commit_message, helpers
> +
> +REVIEWED_BY_TRAILER="Reviewed-by: Dev <dev@example.com>"
> +
> +expect_trailer_msg() {
> +	test_commit_message "$1" <<-EOF
> +	$2
> +
> +	${3:-$REVIEWED_BY_TRAILER}
> +	EOF
> +}
> +
> +test_expect_success 'setup repo with a small history' '
> +	git commit --allow-empty -m "Initial empty commit" &&
> +	test_commit first file a &&
> +	test_commit second file &&
> +	git checkout -b conflict-branch first &&
> +	test_commit file-2 file-2 &&
> +	test_commit conflict file &&
> +	test_commit third file

This leaves us with conflict-branch checked out, is that intentional?

> +'
> +
> +test_expect_success 'apply backend is rejected with --trailer' '
> +	head_before=$(git rev-parse HEAD) &&

I'm not sure we really need to check that HEAD is unchanged here

> +	test_expect_code 128 \
> +	git rebase --apply --trailer "$REVIEWED_BY_TRAILER" \
> +				HEAD^ 2>err &&
> +	test_grep "fatal: --trailer requires the merge backend" err &&
> +	test_cmp_rev HEAD $head_before
> +'
> +
> +test_expect_success 'reject empty --trailer argument' '
> +	test_expect_code 128 git rebase -m --trailer "" HEAD^ 2>err &&

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

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

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

	test_commit_message HEAD <<-EOF
	third
	
	Acked-by:${SP}
	EOF

> +'
> +
> +test_expect_success 'CLI trailer duplicates allowed; replace policy keeps last' '
> +	git -c trailer.Bug.ifexists=replace -c trailer.Bug.ifmissing=add \
> +		rebase -m --trailer "Bug: 123" --trailer "Bug: 456" HEAD~1 third &&
> +	cat >expect <<-\EOF &&
> +	third
> +
> +	Bug: 456
> +	EOF
> +	test_commit_message HEAD expect
> +'
> +
> +test_expect_success 'multiple Signed-off-by trailers all preserved' '
> +	git rebase -m \
> +			--trailer "Signed-off-by: Dev A <a@example.com>" \
> +			--trailer "Signed-off-by: Dev B <b@example.com>" HEAD~1 third &&

The massive indentation here leads to overly long lines.

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

fits our 80 column limit

> +	cat >expect <<-\EOF &&
> +	third
> +
> +	Signed-off-by: Dev A <a@example.com>
> +	Signed-off-by: Dev B <b@example.com>
> +	EOF
> +	test_commit_message HEAD expect
> +'
> +
> +test_expect_success 'rebase -m --trailer adds trailer after conflicts' '
> +	git checkout -B conflict-branch third &&

I'm not sure why we're recreating conflict-branch here, you can just run 
"git rebase [options] second third"
  > +	test_commit fourth file &&
> +	test_must_fail git rebase -m \
> +			--trailer "$REVIEWED_BY_TRAILER" \
> +			second &&

If you unfold this line it is 79 characters long which is perfctly fine.

> +	git checkout --theirs file &&
> +	git add file &&
> +	git rebase --continue &&
> +	expect_trailer_msg HEAD "fourth" &&

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

> +	expect_trailer_msg HEAD^ "third"

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

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

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

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

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

> +	cat >todo <<EOF &&
> +pick $first_short fixup-base
> +fixup $second_short fixup-second
> +EOF

becomes
	cat >todo <<-\EOF &&
	pick fixup-base first-base
	fixup fixup-second fixup-second
	EOF
> +	(
> +		set_replace_editor todo &&
> +		git rebase -i --trailer "$REVIEWED_BY_TRAILER" HEAD~2
> +	) &&
> +	expect_trailer_msg HEAD "fixup-base" &&

We check there is only one trailer after the fixup - good

> +	git reset --hard fixup-second &&
> +	cat >todo <<EOF &&
> +pick $first_short fixup-base
> +fixup -C $second_short fixup-second
> +EOF
> +	(
> +		set_replace_editor todo &&
> +		git rebase -i --trailer "$REVIEWED_BY_TRAILER" HEAD~2
> +	) &&
> +	expect_trailer_msg HEAD "fixup-second"

We check that we add a trailer with "fixup -C" - good> +'
> +
> +test_expect_success 'rebase --root --trailer updates every commit' '
> +	git checkout first &&
> +	git -c trailer.review.key=Reviewed-by rebase --root \
> +		--trailer=review="Dev <dev@example.com>" &&

It would be good if the test title mentioned that we're also checking 
'trailer.<name>.key' here as well which is more interesting than "--root"
  > +	expect_trailer_msg HEAD  "first" &&
> +	expect_trailer_msg HEAD^ "Initial empty commit"

The test coverage looks good

> +'
> +test_done
> diff --git a/trailer.c b/trailer.c
> index f5838f5699..f6ff2f01ee 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -774,6 +775,30 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
>   	free(cl_separators);
>   }
>   
> +void validate_trailer_args_after_config(const struct strvec *cli_args)

I think "validate_trailer_args" would be a sufficient name
> +{
> +	char *cl_separators;
> +
> +	trailer_config_init();
> +
> +	cl_separators = xstrfmt("=%s", separators);
> +
> +	for (size_t i = 0; i < cli_args->nr; i++) {
> +		const char *txt = cli_args->v[i];
> +		ssize_t separator_pos;
> +
> +		if (!*txt)
> +			die(_("empty --trailer argument"));
> +
> +		separator_pos = find_separator(txt, cl_separators);
> +		if (separator_pos == 0)
> +			die(_("invalid trailer '%s': missing key before separator"),
> +		    txt);

Strange indentation here, but the implementation looks sensible.

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

Thanks

Phillip

> +	}
> +
> +	free(cl_separators);
> +}
> +
>   static const char *next_line(const char *str)
>   {
>   	const char *nl = strchrnul(str, '\n');
> @@ -1226,8 +1251,8 @@ void trailer_iterator_release(struct trailer_iterator *iter)
>   	strbuf_release(&iter->key);
>   }
>   
> -static int amend_strbuf_with_trailers(struct strbuf *buf,
> -				      const struct strvec *trailer_args)
> +int amend_strbuf_with_trailers(struct strbuf *buf,
> +			       const struct strvec *trailer_args)
>   {
>   	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
>   	LIST_HEAD(new_trailer_head);
> diff --git a/trailer.h b/trailer.h
> index daea46ca5d..541657a11f 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -68,6 +68,8 @@ void parse_trailers_from_config(struct list_head *config_head);
>   void parse_trailers_from_command_line_args(struct list_head *arg_head,
>   					   struct list_head *new_trailer_head);
>   
> +void validate_trailer_args_after_config(const struct strvec *cli_args);
> +
>   void process_trailers_lists(struct list_head *head,
>   			    struct list_head *arg_head);
>   
> @@ -195,6 +197,9 @@ int trailer_iterator_advance(struct trailer_iterator *iter);
>    */
>   void trailer_iterator_release(struct trailer_iterator *iter);
>   
> +int amend_strbuf_with_trailers(struct strbuf *buf,
> +			       const struct strvec *trailer_args);
> +
>   /*
>    * Augment a file to add trailers to it (similar to 'git interpret-trailers').
>    * Returns 0 on success or a non-zero error code on failure.

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

[Phillip Wood]

Hi Li

On 05/11/2025 14:29, Li Chen wrote:
> From: Li Chen <chenl311@chinatelecom.cn>
> 
> This series routes all trailer insertion through an in-process path, removing
> the fork/exec to builtin/interpret-trailers and tempfile juggling. The first
> three commits centralize logic to reduce overhead and simplify error handling.
> The final commit adds git rebase --trailer, currently supported with the merge
> backend only (rejecting apply-only scenarios and validating input early).

I've left quite a few comments but overall this is looking much better 
now, it needs a bit of cleaning up but I didn't spot any major issues.

Thanks for working on it

Phillip

> all t/*.sh testcases have run successfully.
> 
> v6: squash all fix commits and split refactor step from the original patch based on Phillip's suggestion and codes [4].
> v5: fix all Kristoffer's review comments form v4[3] in place and without new patches.
> v4: fix all reviewer comments in v3. [2], and add patch 1~8 & 10~29 to fix review comments.
> 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 very very welcome!
> 
> [1]: https://lore.kernel.org/git/xmqq8qlzkukw.fsf@gitster.g/
> [2]: https://lore.kernel.org/git/20250803150059.402017-1-me@linux.beauty/
> [3]: https://lore.kernel.org/git/20251014122452.1851103-1-me@linux.beauty/
> [4]: https://lore.kernel.org/git/7d12b046-365f-441c-af8e-8a39d61efbbd@gmail.com/
> 
> Li Chen (4):
>    interpret-trailers: factor out buffer-based processing to
>      process_trailers()
>    trailer: move process_trailers to trailer.h
>    trailer: append trailers in-process and drop the fork to
>      `interpret-trailers`
>    rebase: support --trailer
> 
>   Documentation/git-rebase.adoc |   9 ++-
>   builtin/commit.c              |   2 +-
>   builtin/interpret-trailers.c  |  81 ++------------------
>   builtin/rebase.c              |  50 +++++++++++++
>   builtin/tag.c                 |   3 +-
>   sequencer.c                   |  34 +++++++++
>   sequencer.h                   |   4 +-
>   t/meson.build                 |   1 +
>   t/t3440-rebase-trailer.sh     | 134 ++++++++++++++++++++++++++++++++++
>   trailer.c                     | 129 +++++++++++++++++++++++++++++---
>   trailer.h                     |  13 +++-
>   wrapper.c                     |  16 ++++
>   wrapper.h                     |   6 ++
>   13 files changed, 392 insertions(+), 90 deletions(-)
>   create mode 100755 t/t3440-rebase-trailer.sh
> 

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

[Li Chen]

Hi Phillip,

Sorry for my late reply.

 ---- On Wed, 12 Nov 2025 22:50:27 +0800  Phillip Wood <phillip.wood123@gmail.com> wrote --- 
 > Hi Li
 > 
 > On 05/11/2025 14:29, Li Chen wrote:
 > > From: Li Chen <chenl311@chinatelecom.cn>
 > > 
 > > This series routes all trailer insertion through an in-process path, removing
 > > the fork/exec to builtin/interpret-trailers and tempfile juggling. The first
 > > three commits centralize logic to reduce overhead and simplify error handling.
 > > The final commit adds git rebase --trailer, currently supported with the merge
 > > backend only (rejecting apply-only scenarios and validating input early).
 > 
 > I've left quite a few comments but overall this is looking much better 
 > now, it needs a bit of cleaning up but I didn't spot any major issues.
 > 
 > Thanks for working on it
 
Thanks for your kind reviews! I will address them in the next version.

Regards,

Li​

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

[Kristoffer Haugsbakk]

On Wed, Nov 5, 2025, at 15:29, 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 it if the user passes an option that requires the
> apply backend (git am) since it lacks message‑filter/trailer
> hook. otherwise we can just use the merge backend.
>
> Automatically set REBASE_FORCE when any trailer is supplied.
>
> And reject invalid input before user edits the interactive file.
>
> Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
>[snip]
> diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc
> index 005caf6164..4d2fe4be6e 100644
> --- a/Documentation/git-rebase.adoc
> +++ b/Documentation/git-rebase.adoc
> @@ -487,9 +487,16 @@ See also INCOMPATIBLE OPTIONS below.
>  	Add a `Signed-off-by` trailer to all the rebased commits. Note
>  	that if `--interactive` is given then only commits marked to be
>  	picked, edited or reworded will have the trailer added.
> -+
> +
>  See also INCOMPATIBLE OPTIONS below.
>

Same problem as I commented on in https://lore.kernel.org/git/cbe93380-e145-4ebd-a213-928b8c3ba085@app.fastmail.com/

The `See also INCOMPATIBLE OPTIONS below.` is not indented to the same
level as `--signoff`, where it belongs.

> +--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 implies*
> +       `--force-rebase` so that fast‑forwarded commits are also
> +       rewritten.
> +
>[snip]