{"thread":{"id":"64444","subject":"[PATCH v6 0/4] rebase: support --trailer","startedAt":"2025-11-05T14:30:09Z","lastAt":"2026-02-24T06:36:27Z","messageCount":24,"participants":["Li Chen","Junio C Hamano","Phillip Wood","Kristoffer Haugsbakk"],"isPatch":true,"patchVersion":6,"patchTotal":4},"messages":[{"id":"530243","messageId":"20251105142944.73061-1-me@linux.beauty","threadId":"64444","inReplyTo":null,"subject":"[PATCH v6 0/4] rebase: support --trailer","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-11-05T14:29:40Z","receivedAt":"2025-11-05T14:30:09Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"From: Li Chen \n\nThis series routes all trailer insertion through an in-process path, removing\nthe fork/exec to builtin/interpret-trailers and tempfile juggling. The first\nthree commits centralize logic to reduce overhead and simplify error handling.\nThe final commit adds git rebase --trailer, currently supported with the merge\nbackend only (rejecting apply-only scenarios and validating input early).\n\nall t/*.sh testcases have run successfully.\n\nv6: squash all fix commits and split refactor step from the original patch based on Phillip's suggestion and codes [4].\nv5: fix all Kristoffer's review comments form v4[3] in place and without new patches.\nv4: fix all reviewer comments in v3. [2], and add patch 1~8 & 10~29 to fix review comments.\nv3: merges the remaining trailer paths into one in-process helper, dropping the\nduplicate code, as pointed by Junio and Phillip [1]\nv2: fix issues pointed by Phillip\nRFC link: https://lore.kernel.org/git/196a5ac1393.f5b4db7d187309.2451613571977217927@linux.beauty/\n\nComments very very welcome!\n\n[1]: https://lore.kernel.org/git/xmqq8qlzkukw.fsf@gitster.g/\n[2]: https://lore.kernel.org/git/20250803150059.402017-1-me@linux.beauty/\n[3]: https://lore.kernel.org/git/20251014122452.1851103-1-me@linux.beauty/\n[4]: https://lore.kernel.org/git/7d12b046-365f-441c-af8e-8a39d61efbbd@gmail.com/\n\nLi Chen (4):\n interpret-trailers: factor out buffer-based processing to\n process_trailers()\n trailer: move process_trailers to trailer.h\n trailer: append trailers in-process and drop the fork to\n `interpret-trailers`\n rebase: support --trailer\n\n Documentation/git-rebase.adoc | 9 ++-\n builtin/commit.c | 2 +-\n builtin/interpret-trailers.c | 81 ++------------------\n builtin/rebase.c | 50 +++++++++++++\n builtin/tag.c | 3 +-\n sequencer.c | 34 +++++++++\n sequencer.h | 4 +-\n t/meson.build | 1 +\n t/t3440-rebase-trailer.sh | 134 ++++++++++++++++++++++++++++++++++\n trailer.c | 129 +++++++++++++++++++++++++++++---\n trailer.h | 13 +++-\n wrapper.c | 16 ++++\n wrapper.h | 6 ++\n 13 files changed, 392 insertions(+), 90 deletions(-)\n create mode 100755 t/t3440-rebase-trailer.sh\n\n-- \n2.51.0\n\n"},{"id":"530244","messageId":"20251105142944.73061-2-me@linux.beauty","threadId":"64444","inReplyTo":"20251105142944.73061-1-me@linux.beauty","subject":"[PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers()","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-11-05T14:29:41Z","receivedAt":"2025-11-05T14:30:21Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"From: Li Chen \n\nExtracted trailer processing into a helper that accumulates output in\na strbuf before writing.\n\nUpdated interpret_trailers() to reuse the helper, buffer output, and\nclean up both input and output buffers after writing.\n\nSigned-off-by: Li Chen \n---\n builtin/interpret-trailers.c | 51 ++++++++++++++++++++----------------\n 1 file changed, 29 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex 41b0750e5a..4c90580fff 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -136,32 +136,21 @@ static void read_input_file(struct strbuf *sb, const char *file)\n \tstrbuf_complete_line(sb);\n }\n \n-static void interpret_trailers(const struct process_trailer_options *opts,\n-\t\t\t struct list_head *new_trailer_head,\n-\t\t\t const char *file)\n+static void process_trailers(const struct process_trailer_options *opts,\n+\t\t\t struct list_head *new_trailer_head,\n+\t\t\t struct strbuf *sb, struct strbuf *out)\n {\n \tLIST_HEAD(head);\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tstruct strbuf trailer_block_sb = STRBUF_INIT;\n \tstruct trailer_block *trailer_block;\n-\tFILE *outfile = stdout;\n-\n-\ttrailer_config_init();\n \n-\tread_input_file(&sb, file);\n-\n-\tif (opts->in_place)\n-\t\toutfile = create_in_place_tempfile(file);\n-\n-\ttrailer_block = parse_trailers(opts, sb.buf, &head);\n+\ttrailer_block = parse_trailers(opts, sb->buf, &head);\n \n \t/* Print the lines before the trailer block */\n \tif (!opts->only_trailers)\n-\t\tfwrite(sb.buf, 1, trailer_block_start(trailer_block), outfile);\n+\t\tstrbuf_add(out, sb->buf, trailer_block_start(trailer_block));\n \n \tif (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))\n-\t\tfprintf(outfile, \"\\n\");\n-\n+\t\tstrbuf_addch(out, '\\n');\n \n \tif (!opts->only_input) {\n \t\tLIST_HEAD(config_head);\n@@ -173,22 +162,40 @@ static void interpret_trailers(const struct process_trailer_options *opts,\n \t}\n \n \t/* Print trailer block. */\n-\tformat_trailers(opts, &head, &trailer_block_sb);\n+\tformat_trailers(opts, &head, out);\n \tfree_trailers(&head);\n-\tfwrite(trailer_block_sb.buf, 1, trailer_block_sb.len, outfile);\n-\tstrbuf_release(&trailer_block_sb);\n \n \t/* Print the lines after the trailer block as is. */\n \tif (!opts->only_trailers)\n-\t\tfwrite(sb.buf + trailer_block_end(trailer_block), 1,\n-\t\t sb.len - trailer_block_end(trailer_block), outfile);\n+\t\tstrbuf_add(out, sb->buf + trailer_block_end(trailer_block),\n+\t\t\t sb->len - trailer_block_end(trailer_block));\n \ttrailer_block_release(trailer_block);\n+}\n+\n+static void interpret_trailers(const struct process_trailer_options *opts,\n+\t\t\t struct list_head *new_trailer_head,\n+\t\t\t const char *file)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf out = STRBUF_INIT;\n+\tFILE *outfile = stdout;\n+\n+\ttrailer_config_init();\n+\n+\tread_input_file(&sb, file);\n+\n+\tif (opts->in_place)\n+\t\toutfile = create_in_place_tempfile(file);\n+\n+\tprocess_trailers(opts, new_trailer_head, &sb, &out);\n \n+\tfwrite(out.buf, out.len, 1, outfile);\n \tif (opts->in_place)\n \t\tif (rename_tempfile(&trailers_tempfile, file))\n \t\t\tdie_errno(_(\"could not rename temporary file to %s\"), file);\n \n \tstrbuf_release(&sb);\n+\tstrbuf_release(&out);\n }\n \n int cmd_interpret_trailers(int argc,\n-- \n2.51.0\n\n"},{"id":"530245","messageId":"20251105142944.73061-3-me@linux.beauty","threadId":"64444","inReplyTo":"20251105142944.73061-1-me@linux.beauty","subject":"[PATCH v6 2/4] trailer: move process_trailers to trailer.h","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-11-05T14:29:42Z","receivedAt":"2025-11-05T14:30:32Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"From: Li Chen \n\nThis function would be used by trailer_process\nin following commits.\n\nSigned-off-by: Li Chen \n---\n builtin/interpret-trailers.c | 36 ------------------------------------\n trailer.c | 36 ++++++++++++++++++++++++++++++++++++\n trailer.h | 3 +++\n 3 files changed, 39 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex 4c90580fff..bce2e791d6 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -136,42 +136,6 @@ static void read_input_file(struct strbuf *sb, const char *file)\n \tstrbuf_complete_line(sb);\n }\n \n-static void process_trailers(const struct process_trailer_options *opts,\n-\t\t\t struct list_head *new_trailer_head,\n-\t\t\t struct strbuf *sb, struct strbuf *out)\n-{\n-\tLIST_HEAD(head);\n-\tstruct trailer_block *trailer_block;\n-\n-\ttrailer_block = parse_trailers(opts, sb->buf, &head);\n-\n-\t/* Print the lines before the trailer block */\n-\tif (!opts->only_trailers)\n-\t\tstrbuf_add(out, sb->buf, trailer_block_start(trailer_block));\n-\n-\tif (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))\n-\t\tstrbuf_addch(out, '\\n');\n-\n-\tif (!opts->only_input) {\n-\t\tLIST_HEAD(config_head);\n-\t\tLIST_HEAD(arg_head);\n-\t\tparse_trailers_from_config(&config_head);\n-\t\tparse_trailers_from_command_line_args(&arg_head, new_trailer_head);\n-\t\tlist_splice(&config_head, &arg_head);\n-\t\tprocess_trailers_lists(&head, &arg_head);\n-\t}\n-\n-\t/* Print trailer block. */\n-\tformat_trailers(opts, &head, out);\n-\tfree_trailers(&head);\n-\n-\t/* Print the lines after the trailer block as is. */\n-\tif (!opts->only_trailers)\n-\t\tstrbuf_add(out, sb->buf + trailer_block_end(trailer_block),\n-\t\t\t sb->len - trailer_block_end(trailer_block));\n-\ttrailer_block_release(trailer_block);\n-}\n-\n static void interpret_trailers(const struct process_trailer_options *opts,\n \t\t\t struct list_head *new_trailer_head,\n \t\t\t const char *file)\ndiff --git a/trailer.c b/trailer.c\nindex 911a81ed99..b735ec8a53 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1235,3 +1235,39 @@ int amend_file_with_trailers(const char *path, const struct strvec *trailer_args\n \tstrvec_pushv(&run_trailer.args, trailer_args->v);\n \treturn run_command(&run_trailer);\n }\n+\n+void process_trailers(const struct process_trailer_options *opts,\n+\t\t struct list_head *new_trailer_head,\n+\t\t struct strbuf *sb, struct strbuf *out)\n+{\n+\tLIST_HEAD(head);\n+\tstruct trailer_block *trailer_block;\n+\n+\ttrailer_block = parse_trailers(opts, sb->buf, &head);\n+\n+\t/* Print the lines before the trailer block */\n+\tif (!opts->only_trailers)\n+\t\tstrbuf_add(out, sb->buf, trailer_block_start(trailer_block));\n+\n+\tif (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))\n+\t\tstrbuf_addch(out, '\\n');\n+\n+\tif (!opts->only_input) {\n+\t\tLIST_HEAD(config_head);\n+\t\tLIST_HEAD(arg_head);\n+\t\tparse_trailers_from_config(&config_head);\n+\t\tparse_trailers_from_command_line_args(&arg_head, new_trailer_head);\n+\t\tlist_splice(&config_head, &arg_head);\n+\t\tprocess_trailers_lists(&head, &arg_head);\n+\t}\n+\n+\t/* Print trailer block. */\n+\tformat_trailers(opts, &head, out);\n+\tfree_trailers(&head);\n+\n+\t/* Print the lines after the trailer block as is. */\n+\tif (!opts->only_trailers)\n+\t\tstrbuf_add(out, sb->buf + trailer_block_end(trailer_block),\n+\t\t\t sb->len - trailer_block_end(trailer_block));\n+\ttrailer_block_release(trailer_block);\n+}\ndiff --git a/trailer.h b/trailer.h\nindex 4740549586..44d406b763 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -202,4 +202,7 @@ void trailer_iterator_release(struct trailer_iterator *iter);\n */\n int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);\n \n+void process_trailers(const struct process_trailer_options *opts,\n+\t\t struct list_head *new_trailer_head,\n+\t\t struct strbuf *sb, struct strbuf *out);\n #endif /* TRAILER_H */\n-- \n2.51.0\n\n"},{"id":"530246","messageId":"20251105142944.73061-4-me@linux.beauty","threadId":"64444","inReplyTo":"20251105142944.73061-1-me@linux.beauty","subject":"[PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-11-05T14:29:43Z","receivedAt":"2025-11-05T14:30:44Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"From: Li Chen \n\nRoute all trailer insertion through trailer_process() and make\nbuiltin/interpret-trailers just do file I/O before calling into it.\namend_file_with_trailers() now shares the same code path.\n\nThis removes the fork/exec and tempfile juggling, cutting overhead and\nsimplifying error handling. No functional change. It also\ncentralizes logic to prepare for follow-up rebase --trailer patch.\n\nSigned-off-by: Li Chen \n---\n builtin/commit.c | 2 +-\n builtin/interpret-trailers.c | 46 +++---------------------\n builtin/tag.c | 3 +-\n trailer.c | 68 +++++++++++++++++++++++++++++++-----\n trailer.h | 5 ++-\n wrapper.c | 16 +++++++++\n wrapper.h | 6 ++++\n 7 files changed, 90 insertions(+), 56 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 0243f17d53..67070d6a54 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1719,7 +1719,7 @@ int cmd_commit(int argc,\n \t\tOPT_STRING(0, \"fixup\", &fixup_message, N_(\"[(amend|reword):]commit\"), N_(\"use autosquash formatted message to fixup or amend/reword specified commit\")),\n \t\tOPT_STRING(0, \"squash\", &squash_message, N_(\"commit\"), N_(\"use autosquash formatted message to squash specified commit\")),\n \t\tOPT_BOOL(0, \"reset-author\", &renew_authorship, N_(\"the commit is authored by me now (used with -C/-c/--amend)\")),\n-\t\tOPT_PASSTHRU_ARGV(0, \"trailer\", &trailer_args, N_(\"trailer\"), N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG),\n+\t\tOPT_CALLBACK_F(0, \"trailer\", &trailer_args, N_(\"trailer\"), N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG, parse_opt_strvec),\n \t\tOPT_BOOL('s', \"signoff\", &signoff, N_(\"add a Signed-off-by trailer\")),\n \t\tOPT_FILENAME('t', \"template\", &template_file, N_(\"use specified template file\")),\n \t\tOPT_BOOL('e', \"edit\", &edit_flag, N_(\"force edit of commit\")),\ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex bce2e791d6..268a43372b 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -10,7 +10,6 @@\n #include \"gettext.h\"\n #include \"parse-options.h\"\n #include \"string-list.h\"\n-#include \"tempfile.h\"\n #include \"trailer.h\"\n #include \"config.h\"\n \n@@ -93,37 +92,6 @@ static int parse_opt_parse(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n-static struct tempfile *trailers_tempfile;\n-\n-static FILE *create_in_place_tempfile(const char *file)\n-{\n-\tstruct stat st;\n-\tstruct strbuf filename_template = STRBUF_INIT;\n-\tconst char *tail;\n-\tFILE *outfile;\n-\n-\tif (stat(file, &st))\n-\t\tdie_errno(_(\"could not stat %s\"), file);\n-\tif (!S_ISREG(st.st_mode))\n-\t\tdie(_(\"file %s is not a regular file\"), file);\n-\tif (!(st.st_mode & S_IWUSR))\n-\t\tdie(_(\"file %s is not writable by user\"), file);\n-\n-\t/* Create temporary file in the same directory as the original */\n-\ttail = strrchr(file, '/');\n-\tif (tail)\n-\t\tstrbuf_add(&filename_template, file, tail - file + 1);\n-\tstrbuf_addstr(&filename_template, \"git-interpret-trailers-XXXXXX\");\n-\n-\ttrailers_tempfile = xmks_tempfile_m(filename_template.buf, st.st_mode);\n-\tstrbuf_release(&filename_template);\n-\toutfile = fdopen_tempfile(trailers_tempfile, \"w\");\n-\tif (!outfile)\n-\t\tdie_errno(_(\"could not open temporary file\"));\n-\n-\treturn outfile;\n-}\n-\n static void read_input_file(struct strbuf *sb, const char *file)\n {\n \tif (file) {\n@@ -142,21 +110,15 @@ static void interpret_trailers(const struct process_trailer_options *opts,\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct strbuf out = STRBUF_INIT;\n-\tFILE *outfile = stdout;\n-\n-\ttrailer_config_init();\n \n \tread_input_file(&sb, file);\n \n-\tif (opts->in_place)\n-\t\toutfile = create_in_place_tempfile(file);\n-\n \tprocess_trailers(opts, new_trailer_head, &sb, &out);\n \n-\tfwrite(out.buf, out.len, 1, outfile);\n \tif (opts->in_place)\n-\t\tif (rename_tempfile(&trailers_tempfile, file))\n-\t\t\tdie_errno(_(\"could not rename temporary file to %s\"), file);\n+\t\twrite_file_buf(file, out.buf, out.len);\n+\telse\n+\t\tstrbuf_write(&out, stdout);\n \n \tstrbuf_release(&sb);\n \tstrbuf_release(&out);\n@@ -203,6 +165,8 @@ int cmd_interpret_trailers(int argc,\n \t\t\tgit_interpret_trailers_usage,\n \t\t\toptions);\n \n+\ttrailer_config_init();\n+\n \tif (argc) {\n \t\tint i;\n \t\tfor (i = 0; i < argc; i++)\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex f0665af3ac..65c4a0b36b 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -499,8 +499,7 @@ int cmd_tag(int argc,\n \t\tOPT_CALLBACK_F('m', \"message\", &msg, N_(\"message\"),\n \t\t\t N_(\"tag message\"), PARSE_OPT_NONEG, parse_msg_arg),\n \t\tOPT_FILENAME('F', \"file\", &msgfile, N_(\"read message from file\")),\n-\t\tOPT_PASSTHRU_ARGV(0, \"trailer\", &trailer_args, N_(\"trailer\"),\n-\t\t\t\t N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG),\n+\t\tOPT_CALLBACK_F(0, \"trailer\", &trailer_args, N_(\"trailer\"), N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG, parse_opt_strvec),\n \t\tOPT_BOOL('e', \"edit\", &edit_flag, N_(\"force edit of tag message\")),\n \t\tOPT_BOOL('s', \"sign\", &opt.sign, N_(\"annotated and GPG-signed tag\")),\n \t\tOPT_CLEANUP(&cleanup_arg),\ndiff --git a/trailer.c b/trailer.c\nindex b735ec8a53..f5838f5699 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -9,6 +9,8 @@\n #include \"commit.h\"\n #include \"trailer.h\"\n #include \"list.h\"\n+#include \"wrapper.h\"\n+\n /*\n * Copyright (c) 2013, 2014 Christian Couder \n */\n@@ -1224,18 +1226,66 @@ void trailer_iterator_release(struct trailer_iterator *iter)\n \tstrbuf_release(&iter->key);\n }\n \n-int amend_file_with_trailers(const char *path, const struct strvec *trailer_args)\n+static int amend_strbuf_with_trailers(struct strbuf *buf,\n+\t\t\t\t const struct strvec *trailer_args)\n {\n-\tstruct child_process run_trailer = CHILD_PROCESS_INIT;\n-\n-\trun_trailer.git_cmd = 1;\n-\tstrvec_pushl(&run_trailer.args, \"interpret-trailers\",\n-\t\t \"--in-place\", \"--no-divider\",\n-\t\t path, NULL);\n-\tstrvec_pushv(&run_trailer.args, trailer_args->v);\n-\treturn run_command(&run_trailer);\n+\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n+\tLIST_HEAD(new_trailer_head);\n+\tstruct strbuf out = STRBUF_INIT;\n+\tsize_t i;\n+\n+\topts.no_divider = 1;\n+\n+\tfor (i = 0; i < trailer_args->nr; i++) {\n+\t\tconst char *text = trailer_args->v[i];\n+\t\tstruct new_trailer_item *item;\n+\n+\t\tif (!*text)\n+\t\t\tcontinue;\n+\t\titem = xcalloc(1, sizeof(*item));\n+\t\tINIT_LIST_HEAD(&item->list);\n+\t\titem->text = text;\n+\t\tlist_add_tail(&item->list, &new_trailer_head);\n+\t}\n+\n+\tprocess_trailers(&opts, &new_trailer_head, buf, &out);\n+\n+\tstrbuf_swap(buf, &out);\n+\tstrbuf_release(&out);\n+\twhile (!list_empty(&new_trailer_head)) {\n+\t\tstruct new_trailer_item *item =\n+\t\t\tlist_first_entry(&new_trailer_head, struct new_trailer_item, list);\n+\t\tlist_del(&item->list);\n+\t\tfree(item);\n+\t}\n+\treturn 0;\n }\n \n+int amend_file_with_trailers(const char *path,\n+\t\t\t const struct strvec *trailer_args)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tif (!trailer_args || !trailer_args->nr)\n+\t\treturn 0;\n+\n+\tif (strbuf_read_file(&buf, path, 0) < 0)\n+\t\treturn error_errno(\"could not read '%s'\", path);\n+\n+\tif (amend_strbuf_with_trailers(&buf, trailer_args)) {\n+\t\tstrbuf_release(&buf);\n+\t\treturn error(\"failed to append trailers\");\n+\t}\n+\n+\tif (write_file_buf_gently(path, buf.buf, buf.len)) {\n+\t\tstrbuf_release(&buf);\n+\t\treturn -1;\n+\t}\n+\n+\tstrbuf_release(&buf);\n+\treturn 0;\n+ }\n+\n void process_trailers(const struct process_trailer_options *opts,\n \t\t struct list_head *new_trailer_head,\n \t\t struct strbuf *sb, struct strbuf *out)\ndiff --git a/trailer.h b/trailer.h\nindex 44d406b763..daea46ca5d 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -196,9 +196,8 @@ int trailer_iterator_advance(struct trailer_iterator *iter);\n void trailer_iterator_release(struct trailer_iterator *iter);\n \n /*\n- * Augment a file to add trailers to it by running git-interpret-trailers.\n- * This calls run_command() and its return value is the same (i.e. 0 for\n- * success, various non-zero for other errors). See run-command.h.\n+ * Augment a file to add trailers to it (similar to 'git interpret-trailers').\n+ * Returns 0 on success or a non-zero error code on failure.\n */\n int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);\n \ndiff --git a/wrapper.c b/wrapper.c\nindex 3d507d4204..1f12dbb2fa 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -688,6 +688,22 @@ void write_file_buf(const char *path, const char *buf, size_t len)\n \t\tdie_errno(_(\"could not close '%s'\"), path);\n }\n \n+int write_file_buf_gently(const char *path, const char *buf, size_t len)\n+{\n+\tint fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n+\n+\tif (fd < 0)\n+\t\treturn error_errno(_(\"could not open '%s'\"), path);\n+\tif (write_in_full(fd, buf, len) < 0) {\n+\t\tint ret = error_errno(_(\"could not write to '%s'\"), path);\n+\t\tclose(fd);\n+\t\treturn ret;\n+\t}\n+\tif (close(fd))\n+\t\treturn error_errno(_(\"could not close '%s'\"), path);\n+\treturn 0;\n+}\n+\n void write_file(const char *path, const char *fmt, ...)\n {\n \tva_list params;\ndiff --git a/wrapper.h b/wrapper.h\nindex 44a8597ac3..e5f867b200 100644\n--- a/wrapper.h\n+++ b/wrapper.h\n@@ -56,6 +56,12 @@ static inline ssize_t write_str_in_full(int fd, const char *str)\n */\n void write_file_buf(const char *path, const char *buf, size_t len);\n \n+/**\n+ * Like write_file_buf(), but report errors instead of exiting. Returns 0 on\n+ * success or a negative value on error after emitting a message.\n+ */\n+int write_file_buf_gently(const char *path, const char *buf, size_t len);\n+\n /**\n * Like write_file_buf(), but format the contents into a buffer first.\n * Additionally, write_file() will append a newline if one is not already\n-- \n2.51.0\n\n"},{"id":"530247","messageId":"20251105142944.73061-5-me@linux.beauty","threadId":"64444","inReplyTo":"20251105142944.73061-1-me@linux.beauty","subject":"[PATCH v6 4/4] rebase: support --trailer","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-11-05T14:29:44Z","receivedAt":"2025-11-05T14:30:55Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"From: Li Chen \n\nImplement a new `--trailer ` option for `git rebase`\n(support merge backend only now), which appends arbitrary\ntrailer lines to each rebased commit message.\n\nReject it if the user passes an option that requires the\napply backend (git am) since it lacks message‑filter/trailer\nhook. otherwise we can just use the merge backend.\n\nAutomatically set REBASE_FORCE when any trailer is supplied.\n\nAnd reject invalid input before user edits the interactive file.\n\nSigned-off-by: Li Chen \n---\n Documentation/git-rebase.adoc | 9 ++-\n builtin/rebase.c | 50 +++++++++++++\n sequencer.c | 34 +++++++++\n sequencer.h | 4 +-\n t/meson.build | 1 +\n t/t3440-rebase-trailer.sh | 134 ++++++++++++++++++++++++++++++++++\n trailer.c | 29 +++++++-\n trailer.h | 5 ++\n 8 files changed, 262 insertions(+), 4 deletions(-)\n create mode 100755 t/t3440-rebase-trailer.sh\n\ndiff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc\nindex 005caf6164..4d2fe4be6e 100644\n--- a/Documentation/git-rebase.adoc\n+++ b/Documentation/git-rebase.adoc\n@@ -487,9 +487,16 @@ See also INCOMPATIBLE OPTIONS below.\n \tAdd a `Signed-off-by` trailer to all the rebased commits. Note\n \tthat if `--interactive` is given then only commits marked to be\n \tpicked, edited or reworded will have the trailer added.\n-+\n+\n See also INCOMPATIBLE OPTIONS below.\n \n+--trailer=::\n+ Append the given trailer line(s) to every rebased commit\n+ message, processed via linkgit:git-interpret-trailers[1].\n+ When this option is present *rebase automatically implies*\n+ `--force-rebase` so that fast‑forwarded commits are also\n+ rewritten.\n+\n -i::\n --interactive::\n \tMake a list of the commits which are about to be rebased. Let the\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex c468828189..a88abe08b4 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -36,6 +36,7 @@\n #include \"reset.h\"\n #include \"trace2.h\"\n #include \"hook.h\"\n+#include \"trailer.h\"\n \n static char const * const builtin_rebase_usage[] = {\n \tN_(\"git rebase [-i] [options] [--exec ] \"\n@@ -113,6 +114,7 @@ struct rebase_options {\n \tenum action action;\n \tchar *reflog_action;\n \tint signoff;\n+\tstruct strvec trailer_args;\n \tint allow_rerere_autoupdate;\n \tint keep_empty;\n \tint autosquash;\n@@ -143,6 +145,7 @@ struct rebase_options {\n \t\t.flags = REBASE_NO_QUIET, \t\t\\\n \t\t.git_am_opts = STRVEC_INIT,\t\t\\\n \t\t.exec = STRING_LIST_INIT_NODUP,\t\t\\\n+\t\t.trailer_args = STRVEC_INIT, \\\n \t\t.git_format_patch_opt = STRBUF_INIT,\t\\\n \t\t.fork_point = -1,\t\t\t\\\n \t\t.reapply_cherry_picks = -1, \\\n@@ -166,6 +169,7 @@ static void rebase_options_release(struct rebase_options *opts)\n \tfree(opts->strategy);\n \tstring_list_clear(&opts->strategy_opts, 0);\n \tstrbuf_release(&opts->git_format_patch_opt);\n+\tstrvec_clear(&opts->trailer_args);\n }\n \n static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n@@ -177,6 +181,10 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \tsequencer_init_config(&replay);\n \n \treplay.signoff = opts->signoff;\n+\n+\tfor (size_t i = 0; i < opts->trailer_args.nr; i++)\n+\t\tstrvec_push(&replay.trailer_args, opts->trailer_args.v[i]);\n+\n \treplay.allow_ff = !(opts->flags & REBASE_FORCE);\n \tif (opts->allow_rerere_autoupdate)\n \t\treplay.allow_rerere_auto = opts->allow_rerere_autoupdate;\n@@ -500,6 +508,23 @@ static int read_basic_state(struct rebase_options *opts)\n \t\topts->gpg_sign_opt = xstrdup(buf.buf);\n \t}\n \n+\tstrbuf_reset(&buf);\n+\n+\tif (strbuf_read_file(&buf, state_dir_path(\"trailer\", opts), 0) >= 0) {\n+\t\tconst char *p = buf.buf, *end = buf.buf + buf.len;\n+\n+\t\twhile (p < end) {\n+\t\t\tchar *nl = memchr(p, '\\n', end - p);\n+\t\t\tif (!nl)\n+\t\t\t\tdie(\"nl shouldn't be NULL\");\n+\t\t\t*nl = '\\0';\n+\n+\t\t\tif (*p)\n+\t\t\t\tstrvec_push(&opts->trailer_args, p);\n+\n+\t\t\tp = nl + 1;\n+\t\t}\n+\t}\n \tstrbuf_release(&buf);\n \n \treturn 0;\n@@ -528,6 +553,21 @@ static int rebase_write_basic_state(struct rebase_options *opts)\n \tif (opts->signoff)\n \t\twrite_file(state_dir_path(\"signoff\", opts), \"--signoff\");\n \n+\t/*\n+\t * save opts->trailer_args into state_dir/trailer\n+\t */\n+\tif (opts->trailer_args.nr) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\tfor (size_t i = 0; i < opts->trailer_args.nr; i++) {\n+\t\t\t\tstrbuf_addstr(&buf, opts->trailer_args.v[i]);\n+\t\t\t\tstrbuf_addch(&buf, '\\n');\n+\t\t}\n+\t\twrite_file(state_dir_path(\"trailer\", opts),\n+\t\t\t\t \"%s\", buf.buf);\n+\t\tstrbuf_release(&buf);\n+\t}\n+\n \treturn 0;\n }\n \n@@ -1132,6 +1172,8 @@ int cmd_rebase(int argc,\n \t\t\t.flags = PARSE_OPT_NOARG,\n \t\t\t.defval = REBASE_DIFFSTAT,\n \t\t},\n+\t\tOPT_STRVEC(0, \"trailer\", &options.trailer_args, N_(\"trailer\"),\n+\t\t\t\tN_(\"add custom trailer(s)\")),\n \t\tOPT_BOOL(0, \"signoff\", &options.signoff,\n \t\t\t N_(\"add a Signed-off-by trailer to each commit\")),\n \t\tOPT_BOOL(0, \"committer-date-is-author-date\",\n@@ -1285,6 +1327,11 @@ int cmd_rebase(int argc,\n \t\t\t builtin_rebase_options,\n \t\t\t builtin_rebase_usage, 0);\n \n+\tif (options.trailer_args.nr) {\n+\t\tvalidate_trailer_args_after_config(&options.trailer_args);\n+\t\toptions.flags |= REBASE_FORCE;\n+\t}\n+\n \tif (preserve_merges_selected)\n \t\tdie(_(\"--preserve-merges was replaced by --rebase-merges\\n\"\n \t\t\t\"Note: Your `pull.rebase` configuration may also be set to 'preserve',\\n\"\n@@ -1542,6 +1589,9 @@ int cmd_rebase(int argc,\n \tif (options.root && !options.onto_name)\n \t\timply_merge(&options, \"--root without --onto\");\n \n+\tif (options.trailer_args.nr)\n+\t\timply_merge(&options, \"--trailer\");\n+\n \tif (isatty(2) && options.flags & REBASE_NO_QUIET)\n \t\tstrbuf_addstr(&options.git_format_patch_opt, \" --progress\");\n \ndiff --git a/sequencer.c b/sequencer.c\nindex 5476d39ba9..fbf35cb474 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -209,6 +209,7 @@ static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, \"rebase-merge/reschedul\n static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, \"rebase-merge/no-reschedule-failed-exec\")\n static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, \"rebase-merge/drop_redundant_commits\")\n static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, \"rebase-merge/keep_redundant_commits\")\n+static GIT_PATH_FUNC(rebase_path_trailer, \"rebase-merge/trailer\")\n \n /*\n * A 'struct replay_ctx' represents the private state of the sequencer.\n@@ -420,6 +421,7 @@ void replay_opts_release(struct replay_opts *opts)\n \tif (opts->revs)\n \t\trelease_revisions(opts->revs);\n \tfree(opts->revs);\n+\tstrvec_clear(&opts->trailer_args);\n \treplay_ctx_release(ctx);\n \tfree(opts->ctx);\n }\n@@ -2025,6 +2027,10 @@ static int append_squash_message(struct strbuf *buf, const char *body,\n \t\tif (opts->signoff)\n \t\t\tappend_signoff(buf, 0, 0);\n \n+\t\tif (opts->trailer_args.nr &&\n+\t\t\tamend_strbuf_with_trailers(buf, &opts->trailer_args))\n+\t\t\treturn error(_(\"unable to add trailers to commit message\"));\n+\n \t\tif ((command == TODO_FIXUP) &&\n \t\t (flag & TODO_REPLACE_FIXUP_MSG) &&\n \t\t (file_exists(rebase_path_fixup_msg()) ||\n@@ -2443,6 +2449,14 @@ static int do_pick_commit(struct repository *r,\n \tif (opts->signoff && !is_fixup(command))\n \t\tappend_signoff(&ctx->message, 0, 0);\n \n+\tif (opts->trailer_args.nr && !is_fixup(command)) {\n+\t\tif (amend_strbuf_with_trailers(&ctx->message,\n+\t\t\t\t\t &opts->trailer_args)) {\n+\t\t\tres = error(_(\"unable to add trailers to commit message\"));\n+\t\t\tgoto leave;\n+\t\t}\n+\t}\n+\n \tif (is_rebase_i(opts) && write_author_script(msg.message) < 0)\n \t\tres = -1;\n \telse if (!opts->strategy ||\n@@ -2517,6 +2531,7 @@ static int do_pick_commit(struct repository *r,\n \t\t\t_(\"dropping %s %s -- patch contents already upstream\\n\"),\n \t\t\toid_to_hex(&commit->object.oid), msg.subject);\n \t} /* else allow == 0 and there's nothing special to do */\n+\n \tif (!opts->no_commit && !drop_commit) {\n \t\tif (author || command == TODO_REVERT || (flags & AMEND_MSG))\n \t\t\tres = do_commit(r, msg_file, author, reflog_action,\n@@ -3234,6 +3249,17 @@ static int read_populate_opts(struct replay_opts *opts)\n \n \t\tread_strategy_opts(opts, &buf);\n \t\tstrbuf_reset(&buf);\n+\t\tif (strbuf_read_file(&buf, rebase_path_trailer(), 0) >= 0) {\n+\t\t\tchar *p = buf.buf, *nl;\n+\n+\t\t\twhile ((nl = strchr(p, '\\n'))) {\n+\t\t\t\t*nl = '\\0';\n+\t\t\t\tif (*p)\n+\t\t\t\t\tstrvec_push(&opts->trailer_args, p);\n+\t\t\t\tp = nl + 1;\n+\t\t\t}\n+\t\t\tstrbuf_reset(&buf);\n+\t\t}\n \n \t\tif (read_oneliner(&ctx->current_fixups,\n \t\t\t\t rebase_path_current_fixups(),\n@@ -3328,6 +3354,14 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\twrite_file(rebase_path_reschedule_failed_exec(), \"%s\", \"\");\n \telse\n \t\twrite_file(rebase_path_no_reschedule_failed_exec(), \"%s\", \"\");\n+\tif (opts->trailer_args.nr) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\tfor (size_t i = 0; i < opts->trailer_args.nr; i++)\n+\t\t\tstrbuf_addf(&buf, \"%s\\n\", opts->trailer_args.v[i]);\n+\t\twrite_file(rebase_path_trailer(), \"%s\", buf.buf);\n+\t\tstrbuf_release(&buf);\n+\t}\n \n \treturn 0;\n }\ndiff --git a/sequencer.h b/sequencer.h\nindex 719684c8a9..e21835c5a0 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -44,6 +44,7 @@ struct replay_opts {\n \tint record_origin;\n \tint no_commit;\n \tint signoff;\n+\tstruct strvec trailer_args;\n \tint allow_ff;\n \tint allow_rerere_auto;\n \tint allow_empty;\n@@ -82,8 +83,9 @@ struct replay_opts {\n \tstruct replay_ctx *ctx;\n };\n #define REPLAY_OPTS_INIT {\t\t\t\\\n-\t.edit = -1,\t\t\t\t\\\n \t.action = -1,\t\t\t\t\\\n+\t.edit = -1,\t\t\t\t\\\n+\t.trailer_args = STRVEC_INIT, \\\n \t.xopts = STRVEC_INIT,\t\t\t\\\n \t.ctx = replay_ctx_new(),\t\t\\\n }\ndiff --git a/t/meson.build b/t/meson.build\nindex c9ddd89889..6ebb08feca 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -384,6 +384,7 @@ integration_tests = [\n 't3436-rebase-more-options.sh',\n 't3437-rebase-fixup-options.sh',\n 't3438-rebase-broken-files.sh',\n+ 't3440-rebase-trailer.sh',\n 't3500-cherry.sh',\n 't3501-revert-cherry-pick.sh',\n 't3502-cherry-pick-merge.sh',\ndiff --git a/t/t3440-rebase-trailer.sh b/t/t3440-rebase-trailer.sh\nnew file mode 100755\nindex 0000000000..d0e0434664\n--- /dev/null\n+++ b/t/t3440-rebase-trailer.sh\n@@ -0,0 +1,134 @@\n+#!/bin/sh\n+#\n+\n+test_description='git rebase --trailer integration tests\n+We verify that --trailer works with the merge backend,\n+and that it is rejected early when the apply backend is requested.'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-rebase.sh # test_commit_message, helpers\n+\n+REVIEWED_BY_TRAILER=\"Reviewed-by: Dev \"\n+\n+expect_trailer_msg() {\n+\ttest_commit_message \"$1\" <<-EOF\n+\t$2\n+\n+\t${3:-$REVIEWED_BY_TRAILER}\n+\tEOF\n+}\n+\n+test_expect_success 'setup repo with a small history' '\n+\tgit commit --allow-empty -m \"Initial empty commit\" &&\n+\ttest_commit first file a &&\n+\ttest_commit second file &&\n+\tgit checkout -b conflict-branch first &&\n+\ttest_commit file-2 file-2 &&\n+\ttest_commit conflict file &&\n+\ttest_commit third file\n+'\n+\n+test_expect_success 'apply backend is rejected with --trailer' '\n+\thead_before=$(git rev-parse HEAD) &&\n+\ttest_expect_code 128 \\\n+\tgit rebase --apply --trailer \"$REVIEWED_BY_TRAILER\" \\\n+\t\t\t\tHEAD^ 2>err &&\n+\ttest_grep \"fatal: --trailer requires the merge backend\" err &&\n+\ttest_cmp_rev HEAD $head_before\n+'\n+\n+test_expect_success 'reject empty --trailer argument' '\n+\ttest_expect_code 128 git rebase -m --trailer \"\" HEAD^ 2>err &&\n+\ttest_grep \"empty --trailer\" err\n+'\n+\n+test_expect_success 'reject trailer with missing key before separator' '\n+\ttest_expect_code 128 git rebase -m --trailer \": no-key\" HEAD^ 2>err &&\n+\ttest_grep \"missing key before separator\" err\n+'\n+\n+test_expect_success 'allow trailer with missing value after separator' '\n+\tgit rebase -m --trailer \"Acked-by:\" HEAD~1 third &&\n+\tsed -e \"s/_/ /g\" <<-\\EOF >expect &&\n+\tthird\n+\n+\tAcked-by:_\n+\tEOF\n+\ttest_commit_message HEAD expect\n+'\n+\n+test_expect_success 'CLI trailer duplicates allowed; replace policy keeps last' '\n+\tgit -c trailer.Bug.ifexists=replace -c trailer.Bug.ifmissing=add \\\n+\t\trebase -m --trailer \"Bug: 123\" --trailer \"Bug: 456\" HEAD~1 third &&\n+\tcat >expect <<-\\EOF &&\n+\tthird\n+\n+\tBug: 456\n+\tEOF\n+\ttest_commit_message HEAD expect\n+'\n+\n+test_expect_success 'multiple Signed-off-by trailers all preserved' '\n+\tgit rebase -m \\\n+\t\t\t--trailer \"Signed-off-by: Dev A \" \\\n+\t\t\t--trailer \"Signed-off-by: Dev B \" HEAD~1 third &&\n+\tcat >expect <<-\\EOF &&\n+\tthird\n+\n+\tSigned-off-by: Dev A \n+\tSigned-off-by: Dev B \n+\tEOF\n+\ttest_commit_message HEAD expect\n+'\n+\n+test_expect_success 'rebase -m --trailer adds trailer after conflicts' '\n+\tgit checkout -B conflict-branch third &&\n+\ttest_commit fourth file &&\n+\ttest_must_fail git rebase -m \\\n+\t\t\t--trailer \"$REVIEWED_BY_TRAILER\" \\\n+\t\t\tsecond &&\n+\tgit checkout --theirs file &&\n+\tgit add file &&\n+\tgit rebase --continue &&\n+\texpect_trailer_msg HEAD \"fourth\" &&\n+\texpect_trailer_msg HEAD^ \"third\"\n+'\n+\n+test_expect_success '--trailer handles fixup commands in todo list' '\n+\tgit checkout -B fixup-trailer HEAD &&\n+\ttest_commit fixup-base base &&\n+\ttest_commit fixup-second second &&\n+\tfirst_short=$(git rev-parse --short fixup-base) &&\n+\tsecond_short=$(git rev-parse --short fixup-second) &&\n+\tcat >todo <todo <\" &&\n+\texpect_trailer_msg HEAD \"first\" &&\n+\texpect_trailer_msg HEAD^ \"Initial empty commit\"\n+'\n+test_done\ndiff --git a/trailer.c b/trailer.c\nindex f5838f5699..f6ff2f01ee 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -7,6 +7,7 @@\n #include \"string-list.h\"\n #include \"run-command.h\"\n #include \"commit.h\"\n+#include \"strvec.h\"\n #include \"trailer.h\"\n #include \"list.h\"\n #include \"wrapper.h\"\n@@ -774,6 +775,30 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,\n \tfree(cl_separators);\n }\n \n+void validate_trailer_args_after_config(const struct strvec *cli_args)\n+{\n+\tchar *cl_separators;\n+\n+\ttrailer_config_init();\n+\n+\tcl_separators = xstrfmt(\"=%s\", separators);\n+\n+\tfor (size_t i = 0; i < cli_args->nr; i++) {\n+\t\tconst char *txt = cli_args->v[i];\n+\t\tssize_t separator_pos;\n+\n+\t\tif (!*txt)\n+\t\t\tdie(_(\"empty --trailer argument\"));\n+\n+\t\tseparator_pos = find_separator(txt, cl_separators);\n+\t\tif (separator_pos == 0)\n+\t\t\tdie(_(\"invalid trailer '%s': missing key before separator\"),\n+\t\t txt);\n+\t}\n+\n+\tfree(cl_separators);\n+}\n+\n static const char *next_line(const char *str)\n {\n \tconst char *nl = strchrnul(str, '\\n');\n@@ -1226,8 +1251,8 @@ void trailer_iterator_release(struct trailer_iterator *iter)\n \tstrbuf_release(&iter->key);\n }\n \n-static int amend_strbuf_with_trailers(struct strbuf *buf,\n-\t\t\t\t const struct strvec *trailer_args)\n+int amend_strbuf_with_trailers(struct strbuf *buf,\n+\t\t\t const struct strvec *trailer_args)\n {\n \tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n \tLIST_HEAD(new_trailer_head);\ndiff --git a/trailer.h b/trailer.h\nindex daea46ca5d..541657a11f 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -68,6 +68,8 @@ void parse_trailers_from_config(struct list_head *config_head);\n void parse_trailers_from_command_line_args(struct list_head *arg_head,\n \t\t\t\t\t struct list_head *new_trailer_head);\n \n+void validate_trailer_args_after_config(const struct strvec *cli_args);\n+\n void process_trailers_lists(struct list_head *head,\n \t\t\t struct list_head *arg_head);\n \n@@ -195,6 +197,9 @@ int trailer_iterator_advance(struct trailer_iterator *iter);\n */\n void trailer_iterator_release(struct trailer_iterator *iter);\n \n+int amend_strbuf_with_trailers(struct strbuf *buf,\n+\t\t\t const struct strvec *trailer_args);\n+\n /*\n * Augment a file to add trailers to it (similar to 'git interpret-trailers').\n * Returns 0 on success or a non-zero error code on failure.\n-- \n2.51.0\n\n"},{"id":"530254","messageId":"xmqqecqcmohf.fsf@gitster.g","threadId":"64444","inReplyTo":"20251105142944.73061-1-me@linux.beauty","subject":"Re: [PATCH v6 0/4] rebase: support --trailer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-05T16:30:04Z","receivedAt":"2025-11-05T16:30:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Li Chen writes:\n\n> From: Li Chen \n>\n> This series routes all trailer insertion through an in-process path, removing\n> the fork/exec to builtin/interpret-trailers and tempfile juggling. \n\nThis description makes it sound as if the code before this patch\nseries drove \"interpret-trailers\" via fork/exec and tempfile\njuggling. And that contradicts the title of the topic, \"rebase:\nsupport --trailer\", which implies that the topic is about the \"git\nrebase\" command, and that \"git rebase\" before this patch series did\nnot support trailers, not even with fork/exec and tempfile juggling.\n\nWhich is it?\n\nI see trailer.c:amend_file_with_trailers() does fork out to the\n\"git interpret-trailers\" command and is called by \"git commit\" and\n\"git tag\". Perhaps you are updating the amend_file_with_trailers()\nhelper function to do the in-process thing, so that \"git commit\" and\n\"git tag\" no longer needs fork/exec and tempfile juggling? \n\nThat would be great, regardless of \"rebase\", and if you used that\nupdated helper function to teach \"rebase\" to deal with trailers\nin-process, that would be wonderful.\n\nIf the main part of the series (i.e. [1/4]-[4/4]) needs rerolling,\ncould you be a bit more careful when writing the cover letter to\nmake it easier for even those who are seeing this series for the\nfirst time to understand what is going on?\n\n> The first\n> three commits centralize logic to reduce overhead and simplify error handling.\n\n... in what code paths? \"In command X and Y where they do Z\", \"All\nthe call flows that lead to helper function F by eliminating the\nneed to do G that is costly and replacing it with H\", etc., is what\nI would expect to see in such a description.\n\n> The final commit adds git rebase --trailer, currently supported\n> with the merge backend only (rejecting apply-only scenarios and\n> validating input early).\n\nSounds sensible.\nLi Chen writes:\n\n"},{"id":"530256","messageId":"xmqq1pmcmn7s.fsf@gitster.g","threadId":"64444","inReplyTo":"20251105142944.73061-2-me@linux.beauty","subject":"Re: [PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-05T16:57:27Z","receivedAt":"2025-11-05T16:57:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Li Chen writes:\n\n> From: Li Chen \n>\n> Extracted trailer processing into a helper that accumulates output in\n> a strbuf before writing.\n>\n> Updated interpret_trailers() to reuse the helper, buffer output, and\n> clean up both input and output buffers after writing.\n\nImperative?\n\n>\n> Signed-off-by: Li Chen \n> ---\n> builtin/interpret-trailers.c | 51 ++++++++++++++++++++----------------\n> 1 file changed, 29 insertions(+), 22 deletions(-)\n>\n> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n> index 41b0750e5a..4c90580fff 100644\n> --- a/builtin/interpret-trailers.c\n> +++ b/builtin/interpret-trailers.c\n> @@ -136,32 +136,21 @@ static void read_input_file(struct strbuf *sb, const char *file)\n> \tstrbuf_complete_line(sb);\n> }\n> \n> -static void interpret_trailers(const struct process_trailer_options *opts,\n> -\t\t\t struct list_head *new_trailer_head,\n> -\t\t\t const char *file)\n> +static void process_trailers(const struct process_trailer_options *opts,\n> +\t\t\t struct list_head *new_trailer_head,\n> +\t\t\t struct strbuf *sb, struct strbuf *out)\n\nSo we gained *out strbuf; in the preimage below I see fwrite(),\nfprintf(), etc. to outfile that is either stdout or tempfile, but\npresumably the output all will be captured in the strbuf instead,\nwhich makes sense. It is a bit curious what the new paramater sb\nis, but this is a file-scope static helper, so it does not strictly\nrequire documenting. Having a comment would still be nicer, though,\nunlike \"struct process_trailer_options\" that is very limited\npurpose, \"strbuf\" can be used for any string processing, so a good\nvariable name like \"out\" that conveys what it is used for by\nimplication is good, but \"sb\", which is obvious abbreviation for\n\"Str Buf\", conveys no useful information.\n\n> {\n> \tLIST_HEAD(head);\n> -\tstruct strbuf sb = STRBUF_INIT;\n> -\tstruct strbuf trailer_block_sb = STRBUF_INIT;\n\nWe no longer need a separate strbuf only for trailer block; we will\nsee why before we read through this helper function, hopefully.\n\n> \tstruct trailer_block *trailer_block;\n> -\tFILE *outfile = stdout;\n> -\n> -\ttrailer_config_init();\n> \n> -\tread_input_file(&sb, file);\n> -\n> -\tif (opts->in_place)\n> -\t\toutfile = create_in_place_tempfile(file);\n\nOK, so the original code read the input (either \"file\", or standard\ninput) into a tempfile and prepared the output file stream.\nPresumably it is now the responsibility of the caller of this new\nfunction. Initializing the trailer configuration is also what the\ncaller of this function is reponsible for, as well.\n\nSo this answers one of the questions I had upon starting to read\nthis function, i.e. \"what is sb?\" It holds the input string, which\nis what? Something that look like a commit message that has title,\nbody and then a trailer block? We may want to give the parameter a\nbetter name? I dunno (as this is file-scope static, as long as it\nis obvious to the local caller, it may be OK, but on the other hand,\nthe caller needs to differenciate two strbuf parameters to the\nhelper function, one used for input and the other output, so if you\nare calling the latter \"out\", perhaps you would want to call it\n\"in\", or \"input\", perhaps?)\n\n> -\ttrailer_block = parse_trailers(opts, sb.buf, &head);\n> +\ttrailer_block = parse_trailers(opts, sb->buf, &head);\n\nSo we parse existing trailers from the input strbuf that is supplied\nby the caller. The rest of this hunk is rewriting FILE* I/O with\nstrbuf addition.\n\n> @@ -173,22 +162,40 @@ static void interpret_trailers(const struct process_trailer_options *opts,\n> \t}\n> \n> \t/* Print trailer block. */\n> -\tformat_trailers(opts, &head, &trailer_block_sb);\n> +\tformat_trailers(opts, &head, out);\n> \tfree_trailers(&head);\n> -\tfwrite(trailer_block_sb.buf, 1, trailer_block_sb.len, outfile);\n> -\tstrbuf_release(&trailer_block_sb);\n\nThe format_trailers() helper function appends appends to the strbuf\nthat is given to it, so instead of using an extra strbuf (and then\nappending that to the output), we just pass our output strbuf to it,\nwhich is why we no longer need the trailer_block_sb strbuf anymore.\nMakes sense.\n\n> \t/* Print the lines after the trailer block as is. */\n> \tif (!opts->only_trailers)\n> -\t\tfwrite(sb.buf + trailer_block_end(trailer_block), 1,\n> -\t\t sb.len - trailer_block_end(trailer_block), outfile);\n> +\t\tstrbuf_add(out, sb->buf + trailer_block_end(trailer_block),\n> +\t\t\t sb->len - trailer_block_end(trailer_block));\n> \ttrailer_block_release(trailer_block);\n> +}\n\nAnd again, FILE* I/O is replaced with appending to the output strbuf\nin the rest of this helper function. Good.\n\n> +static void interpret_trailers(const struct process_trailer_options *opts,\n> +\t\t\t struct list_head *new_trailer_head,\n> +\t\t\t const char *file)\n\nSo the original caller of interpret_trailers() now call this outer\nshell, which has the same name and the same function signature as\nthe original. Our new process_trailers() helper assumes a handful\nof preparatory steps are already done by the caller, so what we are\ngoing read here will be mostly those preparation, a call to our new\nhelper, and then printing the result to \"file\" or standard output.\n\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tstruct strbuf out = STRBUF_INIT;\n> +\tFILE *outfile = stdout;\n> +\n> +\ttrailer_config_init();\n> +\n> +\tread_input_file(&sb, file);\n> +\tif (opts->in_place)\n> +\t\toutfile = create_in_place_tempfile(file);\n\nAnd these are exactly the lines we lost from the new helper.\nLooking good.\n\n> +\tprocess_trailers(opts, new_trailer_head, &sb, &out);\n\nAnd our call. \"out\" should have what we wanted to output to\noutfile, so ...\n\n> +\tfwrite(out.buf, out.len, 1, outfile);\n\n... we write it out. Good. For a single long string that can never\nhave NUL in it, I'd personally find it more natural to call fputs(),\nthough. Use of fwrite() makes readers unnecessarily wonder if there\nis something unusual (like needing to be able to handle NULs in the\nbuffer).\n\n> \tif (opts->in_place)\n> \t\tif (rename_tempfile(&trailers_tempfile, file))\n> \t\t\tdie_errno(_(\"could not rename temporary file to %s\"), file);\n>\n> \tstrbuf_release(&sb);\n> +\tstrbuf_release(&out);\n\nOK. We could release out a bit earlier, immediately after fwrite().\n\nLooking mostly good.\n\n> }\n> \n> int cmd_interpret_trailers(int argc,\n"},{"id":"530257","messageId":"xmqqv7jol6qb.fsf@gitster.g","threadId":"64444","inReplyTo":"20251105142944.73061-3-me@linux.beauty","subject":"Re: [PATCH v6 2/4] trailer: move process_trailers to trailer.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-05T17:38:52Z","receivedAt":"2025-11-05T17:38:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Li Chen writes:\n\n> From: Li Chen \n>\n> This function would be used by trailer_process\n> in following commits.\n\nPlease make sure that the body is understandable without the title\nof the commit. Are you going to use process_trailers() from\ntrailer_process()? Can the pair be named less confusingly?\n\n> Subject: Re: [PATCH v6 2/4] trailer: move process_trailers to trailer.h\n\nDeclaring a helper that used to be a file-scope static to a public\nheader file is better described as \"make process_trailers() public\".\n\n> diff --git a/trailer.h b/trailer.h\n> index 4740549586..44d406b763 100644\n> --- a/trailer.h\n> +++ b/trailer.h\n> @@ -202,4 +202,7 @@ void trailer_iterator_release(struct trailer_iterator *iter);\n> */\n> int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);\n> \n\nBefero the function, instruct potential future callers what this\nfunction is about, what parameters it expects, and what side effect\nit makes. As pointed out in the previous step, \"sb\" definitely has\nto be renamed if this becomes public.\n\n> +void process_trailers(const struct process_trailer_options *opts,\n> +\t\t struct list_head *new_trailer_head,\n> +\t\t struct strbuf *sb, struct strbuf *out);\n> #endif /* TRAILER_H */\n"},{"id":"530258","messageId":"xmqqqzucl5xr.fsf@gitster.g","threadId":"64444","inReplyTo":"20251105142944.73061-4-me@linux.beauty","subject":"Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-05T17:56:00Z","receivedAt":"2025-11-05T17:56:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Li Chen writes:\n\n> From: Li Chen \n>\n> Route all trailer insertion through trailer_process() and make\n> builtin/interpret-trailers just do file I/O before calling into it.\n> amend_file_with_trailers() now shares the same code path.\n>\n> This removes the fork/exec and tempfile juggling, cutting overhead and\n> simplifying error handling. No functional change. It also\n> centralizes logic to prepare for follow-up rebase --trailer patch.\n>\n> Signed-off-by: Li Chen \n> ---\n> builtin/commit.c | 2 +-\n> builtin/interpret-trailers.c | 46 +++---------------------\n> builtin/tag.c | 3 +-\n> trailer.c | 68 +++++++++++++++++++++++++++++++-----\n> trailer.h | 5 ++-\n> wrapper.c | 16 +++++++++\n> wrapper.h | 6 ++++\n> 7 files changed, 90 insertions(+), 56 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 0243f17d53..67070d6a54 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1719,7 +1719,7 @@ int cmd_commit(int argc,\n> \t\tOPT_STRING(0, \"fixup\", &fixup_message, N_(\"[(amend|reword):]commit\"), N_(\"use autosquash formatted message to fixup or amend/reword specified commit\")),\n> \t\tOPT_STRING(0, \"squash\", &squash_message, N_(\"commit\"), N_(\"use autosquash formatted message to squash specified commit\")),\n> \t\tOPT_BOOL(0, \"reset-author\", &renew_authorship, N_(\"the commit is authored by me now (used with -C/-c/--amend)\")),\n> -\t\tOPT_PASSTHRU_ARGV(0, \"trailer\", &trailer_args, N_(\"trailer\"), N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG),\n> +\t\tOPT_CALLBACK_F(0, \"trailer\", &trailer_args, N_(\"trailer\"), N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG, parse_opt_strvec),\n\nWhat is this change for?\n\nAs the external interface of the amend_file_with_trailers() helper\ndid not change in this patch, this cannot be a change that is\nrequired to \"remove fork/exec and tempfile juggling\". \n\nOr did amend_file_with_trailers() changed behaviour without changing\nits function signature? If so, this patch does too many things in a\nsingle step, I am afraid.\n\nPerhaps split this step further into multiple patches.\n\n - update the internal implementation of amend_file_with_trailers()\n to avoid having to fork/exec an external process, but *without*\n changing its external interface at all. This step should not have\n to touch builtin/commit.c and builtin/tag.c at all.\n\n - if the strvec styled after passthru-argv is cumbersome to handle,\n perform the interface change, such as change from passthru-argv\n to bare strvec, as a separate step.\n\nThere might need another preparatory step to clean up the\ninterpret-trailers.c itself before the above two (or there may not\nbe---I haven't thought it through).\n\n> diff --git a/wrapper.c b/wrapper.c\n> index 3d507d4204..1f12dbb2fa 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -688,6 +688,22 @@ void write_file_buf(const char *path, const char *buf, size_t len)\n> ...\n> +int write_file_buf_gently(const char *path, const char *buf, size_t len)\n\nI do not think this new helper is warranted. You only call it from\none place anyway.\n"},{"id":"530449","messageId":"f5152523-f7ff-4dee-a685-fb0b74cd6a56@gmail.com","threadId":"64444","inReplyTo":"xmqq1pmcmn7s.fsf@gitster.g","subject":"Re: [PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-11-10T16:27:38Z","receivedAt":"2025-11-10T16:27:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 05/11/2025 16:57, Junio C Hamano wrote:\n> Li Chen writes:\n> \n>> From: Li Chen \n>>\n>> Extracted trailer processing into a helper that accumulates output in\n>> a strbuf before writing.\n>>\n>> Updated interpret_trailers() to reuse the helper, buffer output, and\n>> clean up both input and output buffers after writing.\n> \n> Imperative?\n> \n>>\n>> Signed-off-by: Li Chen \n>> ---\n>> builtin/interpret-trailers.c | 51 ++++++++++++++++++++----------------\n>> 1 file changed, 29 insertions(+), 22 deletions(-)\n>>\n>> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n>> index 41b0750e5a..4c90580fff 100644\n>> --- a/builtin/interpret-trailers.c\n>> +++ b/builtin/interpret-trailers.c\n>> @@ -136,32 +136,21 @@ static void read_input_file(struct strbuf *sb, const char *file)\n>> \tstrbuf_complete_line(sb);\n>> }\n>> \n>> -static void interpret_trailers(const struct process_trailer_options *opts,\n>> -\t\t\t struct list_head *new_trailer_head,\n>> -\t\t\t const char *file)\n>> +static void process_trailers(const struct process_trailer_options *opts,\n>> +\t\t\t struct list_head *new_trailer_head,\n>> +\t\t\t struct strbuf *sb, struct strbuf *out)\n> \n> So we gained *out strbuf; in the preimage below I see fwrite(),\n> fprintf(), etc. to outfile that is either stdout or tempfile, but\n> presumably the output all will be captured in the strbuf instead,\n> which makes sense. It is a bit curious what the new paramater sb\n> is, but this is a file-scope static helper, so it does not strictly\n> require documenting. Having a comment would still be nicer, though,\n> unlike \"struct process_trailer_options\" that is very limited\n> purpose, \"strbuf\" can be used for any string processing, so a good\n> variable name like \"out\" that conveys what it is used for by\n> implication is good, but \"sb\", which is obvious abbreviation for\n> \"Str Buf\", conveys no useful information.\n\nThis patch is based on my suggestion[1]. I had intended to rename \"sb\" \nto \"in\" but forgot to do so before posting that diff. Here's my signoff \nwhich Li should add before their own\n\nSigned-off-by: Phillip Wood \n\nThanks\n\nPhillip\n\n[1] \nhttps://lore.kernel.org/git/7d12b046-365f-441c-af8e-8a39d61efbbd@gmail.com\n>> {\n>> \tLIST_HEAD(head);\n>> -\tstruct strbuf sb = STRBUF_INIT;\n>> -\tstruct strbuf trailer_block_sb = STRBUF_INIT;\n> \n> We no longer need a separate strbuf only for trailer block; we will\n> see why before we read through this helper function, hopefully.\n> \n>> \tstruct trailer_block *trailer_block;\n>> -\tFILE *outfile = stdout;\n>> -\n>> -\ttrailer_config_init();\n>> \n>> -\tread_input_file(&sb, file);\n>> -\n>> -\tif (opts->in_place)\n>> -\t\toutfile = create_in_place_tempfile(file);\n> \n> OK, so the original code read the input (either \"file\", or standard\n> input) into a tempfile and prepared the output file stream.\n> Presumably it is now the responsibility of the caller of this new\n> function. Initializing the trailer configuration is also what the\n> caller of this function is reponsible for, as well.\n> \n> So this answers one of the questions I had upon starting to read\n> this function, i.e. \"what is sb?\" It holds the input string, which\n> is what? Something that look like a commit message that has title,\n> body and then a trailer block? We may want to give the parameter a\n> better name? I dunno (as this is file-scope static, as long as it\n> is obvious to the local caller, it may be OK, but on the other hand,\n> the caller needs to differenciate two strbuf parameters to the\n> helper function, one used for input and the other output, so if you\n> are calling the latter \"out\", perhaps you would want to call it\n> \"in\", or \"input\", perhaps?)\n> \n>> -\ttrailer_block = parse_trailers(opts, sb.buf, &head);\n>> +\ttrailer_block = parse_trailers(opts, sb->buf, &head);\n> \n> So we parse existing trailers from the input strbuf that is supplied\n> by the caller. The rest of this hunk is rewriting FILE* I/O with\n> strbuf addition.\n> \n>> @@ -173,22 +162,40 @@ static void interpret_trailers(const struct process_trailer_options *opts,\n>> \t}\n>> \n>> \t/* Print trailer block. */\n>> -\tformat_trailers(opts, &head, &trailer_block_sb);\n>> +\tformat_trailers(opts, &head, out);\n>> \tfree_trailers(&head);\n>> -\tfwrite(trailer_block_sb.buf, 1, trailer_block_sb.len, outfile);\n>> -\tstrbuf_release(&trailer_block_sb);\n> \n> The format_trailers() helper function appends appends to the strbuf\n> that is given to it, so instead of using an extra strbuf (and then\n> appending that to the output), we just pass our output strbuf to it,\n> which is why we no longer need the trailer_block_sb strbuf anymore.\n> Makes sense.\n> \n>> \t/* Print the lines after the trailer block as is. */\n>> \tif (!opts->only_trailers)\n>> -\t\tfwrite(sb.buf + trailer_block_end(trailer_block), 1,\n>> -\t\t sb.len - trailer_block_end(trailer_block), outfile);\n>> +\t\tstrbuf_add(out, sb->buf + trailer_block_end(trailer_block),\n>> +\t\t\t sb->len - trailer_block_end(trailer_block));\n>> \ttrailer_block_release(trailer_block);\n>> +}\n> \n> And again, FILE* I/O is replaced with appending to the output strbuf\n> in the rest of this helper function. Good.\n> \n>> +static void interpret_trailers(const struct process_trailer_options *opts,\n>> +\t\t\t struct list_head *new_trailer_head,\n>> +\t\t\t const char *file)\n> \n> So the original caller of interpret_trailers() now call this outer\n> shell, which has the same name and the same function signature as\n> the original. Our new process_trailers() helper assumes a handful\n> of preparatory steps are already done by the caller, so what we are\n> going read here will be mostly those preparation, a call to our new\n> helper, and then printing the result to \"file\" or standard output.\n> \n>> +{\n>> +\tstruct strbuf sb = STRBUF_INIT;\n>> +\tstruct strbuf out = STRBUF_INIT;\n>> +\tFILE *outfile = stdout;\n>> +\n>> +\ttrailer_config_init();\n>> +\n>> +\tread_input_file(&sb, file);\n>> +\tif (opts->in_place)\n>> +\t\toutfile = create_in_place_tempfile(file);\n> \n> And these are exactly the lines we lost from the new helper.\n> Looking good.\n> \n>> +\tprocess_trailers(opts, new_trailer_head, &sb, &out);\n> \n> And our call. \"out\" should have what we wanted to output to\n> outfile, so ...\n> \n>> +\tfwrite(out.buf, out.len, 1, outfile);\n> \n> ... we write it out. Good. For a single long string that can never\n> have NUL in it, I'd personally find it more natural to call fputs(),\n> though. Use of fwrite() makes readers unnecessarily wonder if there\n> is something unusual (like needing to be able to handle NULs in the\n> buffer).\n> \n>> \tif (opts->in_place)\n>> \t\tif (rename_tempfile(&trailers_tempfile, file))\n>> \t\t\tdie_errno(_(\"could not rename temporary file to %s\"), file);\n>>\n>> \tstrbuf_release(&sb);\n>> +\tstrbuf_release(&out);\n> \n> OK. We could release out a bit earlier, immediately after fwrite().\n> \n> Looking mostly good.\n> \n>> }\n>> \n>> int cmd_interpret_trailers(int argc,\n> \n\n"},{"id":"530450","messageId":"ef12ada7-13ae-4df0-a823-6f428c797223@gmail.com","threadId":"64444","inReplyTo":"20251105142944.73061-4-me@linux.beauty","subject":"Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-11-10T16:38:55Z","receivedAt":"2025-11-10T16:38:59Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Li\n\nOn 05/11/2025 14:29, Li Chen wrote:\n> From: Li Chen \n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 0243f17d53..67070d6a54 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1719,7 +1719,7 @@ int cmd_commit(int argc,\n> \t\tOPT_STRING(0, \"fixup\", &fixup_message, N_(\"[(amend|reword):]commit\"), N_(\"use autosquash formatted message to fixup or amend/reword specified commit\")),\n> \t\tOPT_STRING(0, \"squash\", &squash_message, N_(\"commit\"), N_(\"use autosquash formatted message to squash specified commit\")),\n> \t\tOPT_BOOL(0, \"reset-author\", &renew_authorship, N_(\"the commit is authored by me now (used with -C/-c/--amend)\")),\n> -\t\tOPT_PASSTHRU_ARGV(0, \"trailer\", &trailer_args, N_(\"trailer\"), N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG),\n\nWe have OPT_STRVEC to handle this. The commit message should explain why \nwe're doing this (because we only want to pass the value to \namend_file_with_trailers()). Alternatively we could use skip_prefix() in \namend_file_with_trailers() to skip the \"--trailer=\" prefix in this patch \nand then clean it in a separate patch.\n\n> +\t\tOPT_CALLBACK_F(0, \"trailer\", &trailer_args, N_(\"trailer\"), N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG, parse_opt_strvec),\n> \t\tOPT_BOOL('s', \"signoff\", &signoff, N_(\"add a Signed-off-by trailer\")),\n> \t\tOPT_FILENAME('t', \"template\", &template_file, N_(\"use specified template file\")),\n> \t\tOPT_BOOL('e', \"edit\", &edit_flag, N_(\"force edit of commit\")),\n> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n> index bce2e791d6..268a43372b 100644\n> --- a/builtin/interpret-trailers.c\n> +++ b/builtin/interpret-trailers.c\n> \n> @@ -142,21 +110,15 @@ static void interpret_trailers(const struct process_trailer_options *opts,\n> {\n> \tstruct strbuf sb = STRBUF_INIT;\n> \tstruct strbuf out = STRBUF_INIT;\n> -\tFILE *outfile = stdout;\n> -\n> -\ttrailer_config_init();\n\nWhy is this being moved?\n> \tread_input_file(&sb, file);\n> \n> -\tif (opts->in_place)\n> -\t\toutfile = create_in_place_tempfile(file);\n> -\n> \tprocess_trailers(opts, new_trailer_head, &sb, &out);\n> \n> -\tfwrite(out.buf, out.len, 1, outfile);\n> \tif (opts->in_place)\n> -\t\tif (rename_tempfile(&trailers_tempfile, file))\n> -\t\t\tdie_errno(_(\"could not rename temporary file to %s\"), file);\n> +\t\twrite_file_buf(file, out.buf, out.len);\n\nThis truncates the existing file which means that if there is a error \nwhile writing the new version the user is now left with garbage rather \nthan the original file which does not seem like a good idea.\n\n > diff --git a/trailer.c b/trailer.c> index b735ec8a53..f5838f5699 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> \n> @@ -1224,18 +1226,66 @@ void trailer_iterator_release(struct trailer_iterator *iter)\n> \tstrbuf_release(&iter->key);\n> }\n> \n> -int amend_file_with_trailers(const char *path, const struct strvec *trailer_args)\n> +static int amend_strbuf_with_trailers(struct strbuf *buf,\n> +\t\t\t\t const struct strvec *trailer_args)\n> {\n> -\tstruct child_process run_trailer = CHILD_PROCESS_INIT;\n> -\n> -\trun_trailer.git_cmd = 1;\n> -\tstrvec_pushl(&run_trailer.args, \"interpret-trailers\",\n> -\t\t \"--in-place\", \"--no-divider\",\n> -\t\t path, NULL);\n> -\tstrvec_pushv(&run_trailer.args, trailer_args->v);\n> -\treturn run_command(&run_trailer);\n> +\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n> +\tLIST_HEAD(new_trailer_head);\n> +\tstruct strbuf out = STRBUF_INIT;\n> +\tsize_t i;\n> +\n> +\topts.no_divider = 1;\n> +\n> +\tfor (i = 0; i < trailer_args->nr; i++) {\n> +\t\tconst char *text = trailer_args->v[i];\n> +\t\tstruct new_trailer_item *item;\n> +\n> +\t\tif (!*text)\n> +\t\t\tcontinue;\n\nIsn't it an error to pass an empty argument to \"--trailer\"?\n\n> +\t\titem = xcalloc(1, sizeof(*item));\n> +\t\tINIT_LIST_HEAD(&item->list);\n\nI don't think we need this as \"item->prev\" and \"item->next\" are set by \nlist_add_tail() below.\n\nWe initialize \"where\", \"if_exists\" and \"if_missing\" to zero which \nmatches what builtin/interpret-trailers.c does if the user does not \nspecify any of those options - good.\n\n> +\t\titem->text = text;\n> +\t\tlist_add_tail(&item->list, &new_trailer_head);\n> +\t}\n> +\n> +\tprocess_trailers(&opts, &new_trailer_head, buf, &out);\n> +\n> +\tstrbuf_swap(buf, &out);\n> +\tstrbuf_release(&out);\n> +\twhile (!list_empty(&new_trailer_head)) {\n> +\t\tstruct new_trailer_item *item =\n> +\t\t\tlist_first_entry(&new_trailer_head, struct new_trailer_item, list);\n> +\t\tlist_del(&item->list);\n> +\t\tfree(item);\n\nWe have free_trailers() to do this for us.\n\n> +\t}\n> +\treturn 0;\n> }\n> \n> +int amend_file_with_trailers(const char *path,\n> +\t\t\t const struct strvec *trailer_args)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\tif (!trailer_args || !trailer_args->nr)\n> +\t\treturn 0;\n\nIsn't it a bug to pass a NULL trailer_args?\n\n> +\tif (strbuf_read_file(&buf, path, 0) < 0)\n> +\t\treturn error_errno(\"could not read '%s'\", path);\n> +\n> +\tif (amend_strbuf_with_trailers(&buf, trailer_args)) {\n> +\t\tstrbuf_release(&buf);\n> +\t\treturn error(\"failed to append trailers\");\n> +\t}\n> +\n> +\tif (write_file_buf_gently(path, buf.buf, buf.len)) {\n> +\t\tstrbuf_release(&buf);\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tstrbuf_release(&buf);\n> +\treturn 0;\n> + }\n\nThis looks like a faithful conversion of the original with the caveat \nthat it expects to be passed an array of trailer arguments without the \n\"--trailer=\" prefix. Good\n\nI'll take a look at patch 4 tomorrow but so far these version is looking \nmuch nicer than the last round.\n\nThanks\n\nPhillip\n\n> void process_trailers(const struct process_trailer_options *opts,\n> \t\t struct list_head *new_trailer_head,\n> \t\t struct strbuf *sb, struct strbuf *out)\n> diff --git a/trailer.h b/trailer.h\n> index 44d406b763..daea46ca5d 100644\n> --- a/trailer.h\n> +++ b/trailer.h\n> @@ -196,9 +196,8 @@ int trailer_iterator_advance(struct trailer_iterator *iter);\n> void trailer_iterator_release(struct trailer_iterator *iter);\n> \n> /*\n> - * Augment a file to add trailers to it by running git-interpret-trailers.\n> - * This calls run_command() and its return value is the same (i.e. 0 for\n> - * success, various non-zero for other errors). See run-command.h.\n> + * Augment a file to add trailers to it (similar to 'git interpret-trailers').\n> + * Returns 0 on success or a non-zero error code on failure.\n> */\n> int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);\n> \n> diff --git a/wrapper.c b/wrapper.c\n> index 3d507d4204..1f12dbb2fa 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -688,6 +688,22 @@ void write_file_buf(const char *path, const char *buf, size_t len)\n> \t\tdie_errno(_(\"could not close '%s'\"), path);\n> }\n> \n> +int write_file_buf_gently(const char *path, const char *buf, size_t len)\n> +{\n> +\tint fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n> +\n> +\tif (fd < 0)\n> +\t\treturn error_errno(_(\"could not open '%s'\"), path);\n> +\tif (write_in_full(fd, buf, len) < 0) {\n> +\t\tint ret = error_errno(_(\"could not write to '%s'\"), path);\n> +\t\tclose(fd);\n> +\t\treturn ret;\n> +\t}\n> +\tif (close(fd))\n> +\t\treturn error_errno(_(\"could not close '%s'\"), path);\n> +\treturn 0;\n> +}\n> +\n> void write_file(const char *path, const char *fmt, ...)\n> {\n> \tva_list params;\n> diff --git a/wrapper.h b/wrapper.h\n> index 44a8597ac3..e5f867b200 100644\n> --- a/wrapper.h\n> +++ b/wrapper.h\n> @@ -56,6 +56,12 @@ static inline ssize_t write_str_in_full(int fd, const char *str)\n> */\n> void write_file_buf(const char *path, const char *buf, size_t len);\n> \n> +/**\n> + * Like write_file_buf(), but report errors instead of exiting. Returns 0 on\n> + * success or a negative value on error after emitting a message.\n> + */\n> +int write_file_buf_gently(const char *path, const char *buf, size_t len);\n> +\n> /**\n> * Like write_file_buf(), but format the contents into a buffer first.\n> * Additionally, write_file() will append a newline if one is not already\n\n"},{"id":"530467","messageId":"19a6f310cc5.17364397534057.8623048406766685580@linux.beauty","threadId":"64444","inReplyTo":"ef12ada7-13ae-4df0-a823-6f428c797223@gmail.com","subject":"Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-11-10T19:14:36Z","receivedAt":"2025-11-10T19:14:50Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"Hi Phillip,\n\n ---- On Tue, 11 Nov 2025 00:38:55 +0800 Phillip Wood wrote --- \n > Hi Li\n > \n > On 05/11/2025 14:29, Li Chen wrote:\n > > From: Li Chen \n > > \n > > diff --git a/builtin/commit.c b/builtin/commit.c\n > > index 0243f17d53..67070d6a54 100644\n > > --- a/builtin/commit.c\n > > +++ b/builtin/commit.c\n > > @@ -1719,7 +1719,7 @@ int cmd_commit(int argc,\n > > OPT_STRING(0, \"fixup\", &fixup_message, N_(\"[(amend|reword):]commit\"), N_(\"use autosquash formatted message to fixup or amend/reword specified commit\")),\n > > OPT_STRING(0, \"squash\", &squash_message, N_(\"commit\"), N_(\"use autosquash formatted message to squash specified commit\")),\n > > OPT_BOOL(0, \"reset-author\", &renew_authorship, N_(\"the commit is authored by me now (used with -C/-c/--amend)\")),\n > > - OPT_PASSTHRU_ARGV(0, \"trailer\", &trailer_args, N_(\"trailer\"), N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG),\n > \n > We have OPT_STRVEC to handle this. The commit message should explain why \n > we're doing this (because we only want to pass the value to \n > amend_file_with_trailers()). Alternatively we could use skip_prefix() in \n > amend_file_with_trailers() to skip the \"--trailer=\" prefix in this patch \n > and then clean it in a separate patch.\n\nThanks for the reminder, I will try to split into two patches in the next reversion. The first one\nuse skip_prefix() in amend_file_with_trailers(), and the second one switch to amend_file_with_trailers().\n\n > \n > > + OPT_CALLBACK_F(0, \"trailer\", &trailer_args, N_(\"trailer\"), N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG, parse_opt_strvec),\n > > OPT_BOOL('s', \"signoff\", &signoff, N_(\"add a Signed-off-by trailer\")),\n > > OPT_FILENAME('t', \"template\", &template_file, N_(\"use specified template file\")),\n > > OPT_BOOL('e', \"edit\", &edit_flag, N_(\"force edit of commit\")),\n > > diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n > > index bce2e791d6..268a43372b 100644\n > > --- a/builtin/interpret-trailers.c\n > > +++ b/builtin/interpret-trailers.c\n > > \n > > @@ -142,21 +110,15 @@ static void interpret_trailers(const struct process_trailer_options *opts,\n > > {\n > > struct strbuf sb = STRBUF_INIT;\n > > struct strbuf out = STRBUF_INIT;\n > > - FILE *outfile = stdout;\n > > -\n > > - trailer_config_init();\n > \n > Why is this being moved?\n\nSince trailer_config_init only needs to run once, it's better to move it outside the cmd_interpret_trailers loop,\neven though it already uses a configured global variable.\n\n > > read_input_file(&sb, file);\n > > \n > > - if (opts->in_place)\n > > - outfile = create_in_place_tempfile(file);\n > > -\n > > process_trailers(opts, new_trailer_head, &sb, &out);\n > > \n > > - fwrite(out.buf, out.len, 1, outfile);\n > > if (opts->in_place)\n > > - if (rename_tempfile(&trailers_tempfile, file))\n > > - die_errno(_(\"could not rename temporary file to %s\"), file);\n > > + write_file_buf(file, out.buf, out.len);\n > \n > This truncates the existing file which means that if there is a error \n > while writing the new version the user is now left with garbage rather \n > than the original file which does not seem like a good idea.\n\nThanks for catching this. I'll switch back to using a temp file for atomic.\n\n > \n > > diff --git a/trailer.c b/trailer.c> index b735ec8a53..f5838f5699 100644\n > > --- a/trailer.c\n > > +++ b/trailer.c\n > > \n > > @@ -1224,18 +1226,66 @@ void trailer_iterator_release(struct trailer_iterator *iter)\n > > strbuf_release(&iter->key);\n > > }\n > > \n > > -int amend_file_with_trailers(const char *path, const struct strvec *trailer_args)\n > > +static int amend_strbuf_with_trailers(struct strbuf *buf,\n > > + const struct strvec *trailer_args)\n > > {\n > > - struct child_process run_trailer = CHILD_PROCESS_INIT;\n > > -\n > > - run_trailer.git_cmd = 1;\n > > - strvec_pushl(&run_trailer.args, \"interpret-trailers\",\n > > - \"--in-place\", \"--no-divider\",\n > > - path, NULL);\n > > - strvec_pushv(&run_trailer.args, trailer_args->v);\n > > - return run_command(&run_trailer);\n > > + struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n > > + LIST_HEAD(new_trailer_head);\n > > + struct strbuf out = STRBUF_INIT;\n > > + size_t i;\n > > +\n > > + opts.no_divider = 1;\n > > +\n > > + for (i = 0; i < trailer_args->nr; i++) {\n > > + const char *text = trailer_args->v[i];\n > > + struct new_trailer_item *item;\n > > +\n > > + if (!*text)\n > > + continue;\n > \n > Isn't it an error to pass an empty argument to \"--trailer\"?\n \nNice catch, I would refactor amend_strbuf_with_trailers to return error(_(\"empty --trailer argument\"));\nhere and handle resource cleanup then make amend_file_with_trailers return this error.\n\n > > + item = xcalloc(1, sizeof(*item));\n > > + INIT_LIST_HEAD(&item->list);\n > \n > I don't think we need this as \"item->prev\" and \"item->next\" are set by \n > list_add_tail() below.\n \nok, I would remove this.\n\n > We initialize \"where\", \"if_exists\" and \"if_missing\" to zero which \n > matches what builtin/interpret-trailers.c does if the user does not \n > specify any of those options - good.\n > \n > > + item->text = text;\n > > + list_add_tail(&item->list, &new_trailer_head);\n > > + }\n > > +\n > > + process_trailers(&opts, &new_trailer_head, buf, &out);\n > > +\n > > + strbuf_swap(buf, &out);\n > > + strbuf_release(&out);\n > > + while (!list_empty(&new_trailer_head)) {\n > > + struct new_trailer_item *item =\n > > + list_first_entry(&new_trailer_head, struct new_trailer_item, list);\n > > + list_del(&item->list);\n > > + free(item);\n > \n > We have free_trailers() to do this for us.\n \nI would replace them with free_trailers.\n\n > > + }\n > > + return 0;\n > > }\n > > \n > > +int amend_file_with_trailers(const char *path,\n > > + const struct strvec *trailer_args)\n > > +{\n > > + struct strbuf buf = STRBUF_INIT;\n > > +\n > > + if (!trailer_args || !trailer_args->nr)\n > > + return 0;\n > \n > Isn't it a bug to pass a NULL trailer_args?\n \nSounds right, I would let it return an error msg.\n\n > > + if (strbuf_read_file(&buf, path, 0) < 0)\n > > + return error_errno(\"could not read '%s'\", path);\n > > +\n > > + if (amend_strbuf_with_trailers(&buf, trailer_args)) {\n > > + strbuf_release(&buf);\n > > + return error(\"failed to append trailers\");\n > > + }\n > > +\n > > + if (write_file_buf_gently(path, buf.buf, buf.len)) {\n > > + strbuf_release(&buf);\n > > + return -1;\n > > + }\n > > +\n > > + strbuf_release(&buf);\n > > + return 0;\n > > + }\n > \n > This looks like a faithful conversion of the original with the caveat \n > that it expects to be passed an array of trailer arguments without the \n > \"--trailer=\" prefix. Good\n\nOkay, thanks. Junio notes that write_file_buf_gently is only used in this context and\n doesn't need wrok as a helper function. I'll replace it with an in-place operation.\n\n > I'll take a look at patch 4 tomorrow but so far these version is looking \n > much nicer than the last round.\n\nThanks a lot.\n\nRegards,\n\nLi​\n\n"},{"id":"530468","messageId":"19a6f33e7b7.34ee7a88535992.903448798239861574@linux.beauty","threadId":"64444","inReplyTo":"xmqqecqcmohf.fsf@gitster.g","subject":"Re: [PATCH v6 0/4] rebase: support --trailer","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-11-10T19:17:43Z","receivedAt":"2025-11-10T19:17:57Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"Hi Junio,\n\n ---- On Thu, 06 Nov 2025 00:30:04 +0800 Junio C Hamano wrote --- \n > Li Chen writes:\n > \n > > From: Li Chen \n > >\n > > This series routes all trailer insertion through an in-process path, removing\n > > the fork/exec to builtin/interpret-trailers and tempfile juggling. \n > \n > This description makes it sound as if the code before this patch\n > series drove \"interpret-trailers\" via fork/exec and tempfile\n > juggling. And that contradicts the title of the topic, \"rebase:\n > support --trailer\", which implies that the topic is about the \"git\n > rebase\" command, and that \"git rebase\" before this patch series did\n > not support trailers, not even with fork/exec and tempfile juggling.\n > \n > Which is it?\n > \n > I see trailer.c:amend_file_with_trailers() does fork out to the\n > \"git interpret-trailers\" command and is called by \"git commit\" and\n > \"git tag\". Perhaps you are updating the amend_file_with_trailers()\n > helper function to do the in-process thing, so that \"git commit\" and\n > \"git tag\" no longer needs fork/exec and tempfile juggling? \n > \n > That would be great, regardless of \"rebase\", and if you used that\n > updated helper function to teach \"rebase\" to deal with trailers\n > in-process, that would be wonderful.\n > \n > If the main part of the series (i.e. [1/4]-[4/4]) needs rerolling,\n > could you be a bit more careful when writing the cover letter to\n > make it easier for even those who are seeing this series for the\n > first time to understand what is going on?\n > \n > > The first\n > > three commits centralize logic to reduce overhead and simplify error handling.\n > \n > ... in what code paths? \"In command X and Y where they do Z\", \"All\n > the call flows that lead to helper function F by eliminating the\n > need to do G that is costly and replacing it with H\", etc., is what\n > I would expect to see in such a description.\n > \n > > The final commit adds git rebase --trailer, currently supported\n > > with the merge backend only (rejecting apply-only scenarios and\n > > validating input early).\n > \n > Sounds sensible.\n > Li Chen writes:\n > \n > \n\nThanks for reviewing the cover letter. I will incorporate your suggestions to improve \nits clarity and effectiveness in the next version.\n\nRegards,\n\nLi​\n\n"},{"id":"530469","messageId":"19a6f38130d.beb422c538849.8699301123463603361@linux.beauty","threadId":"64444","inReplyTo":"xmqq1pmcmn7s.fsf@gitster.g","subject":"Re: [PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers()","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-11-10T19:22:17Z","receivedAt":"2025-11-10T19:22:28Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"Hi Junio,\n\n\n ---- On Thu, 06 Nov 2025 00:57:27 +0800 Junio C Hamano wrote --- \n > Li Chen writes:\n > \n > > From: Li Chen \n > >\n > > Extracted trailer processing into a helper that accumulates output in\n > > a strbuf before writing.\n > >\n > > Updated interpret_trailers() to reuse the helper, buffer output, and\n > > clean up both input and output buffers after writing.\n > \n > Imperative?\n > \n > >\n > > Signed-off-by: Li Chen \n > > ---\n > > builtin/interpret-trailers.c | 51 ++++++++++++++++++++----------------\n > > 1 file changed, 29 insertions(+), 22 deletions(-)\n > >\n > > diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n > > index 41b0750e5a..4c90580fff 100644\n > > --- a/builtin/interpret-trailers.c\n > > +++ b/builtin/interpret-trailers.c\n > > @@ -136,32 +136,21 @@ static void read_input_file(struct strbuf *sb, const char *file)\n > > strbuf_complete_line(sb);\n > > }\n > > \n > > -static void interpret_trailers(const struct process_trailer_options *opts,\n > > - struct list_head *new_trailer_head,\n > > - const char *file)\n > > +static void process_trailers(const struct process_trailer_options *opts,\n > > + struct list_head *new_trailer_head,\n > > + struct strbuf *sb, struct strbuf *out)\n > \n > So we gained *out strbuf; in the preimage below I see fwrite(),\n > fprintf(), etc. to outfile that is either stdout or tempfile, but\n > presumably the output all will be captured in the strbuf instead,\n > which makes sense. It is a bit curious what the new paramater sb\n > is, but this is a file-scope static helper, so it does not strictly\n > require documenting. Having a comment would still be nicer, though,\n > unlike \"struct process_trailer_options\" that is very limited\n > purpose, \"strbuf\" can be used for any string processing, so a good\n > variable name like \"out\" that conveys what it is used for by\n > implication is good, but \"sb\", which is obvious abbreviation for\n > \"Str Buf\", conveys no useful information.\n\nThanks, I would rename the variable in next version.\n\n > \n > > {\n > > LIST_HEAD(head);\n > > - struct strbuf sb = STRBUF_INIT;\n > > - struct strbuf trailer_block_sb = STRBUF_INIT;\n > \n > We no longer need a separate strbuf only for trailer block; we will\n > see why before we read through this helper function, hopefully.\n > \n > > struct trailer_block *trailer_block;\n > > - FILE *outfile = stdout;\n > > -\n > > - trailer_config_init();\n > > \n > > - read_input_file(&sb, file);\n > > -\n > > - if (opts->in_place)\n > > - outfile = create_in_place_tempfile(file);\n > \n > OK, so the original code read the input (either \"file\", or standard\n > input) into a tempfile and prepared the output file stream.\n > Presumably it is now the responsibility of the caller of this new\n > function. Initializing the trailer configuration is also what the\n > caller of this function is reponsible for, as well.\n > \n > So this answers one of the questions I had upon starting to read\n > this function, i.e. \"what is sb?\" It holds the input string, which\n > is what? Something that look like a commit message that has title,\n > body and then a trailer block? We may want to give the parameter a\n > better name? I dunno (as this is file-scope static, as long as it\n > is obvious to the local caller, it may be OK, but on the other hand,\n > the caller needs to differenciate two strbuf parameters to the\n > helper function, one used for input and the other output, so if you\n > are calling the latter \"out\", perhaps you would want to call it\n > \"in\", or \"input\", perhaps?)\n\nYes, in is a better name.\n\n > \n > > - trailer_block = parse_trailers(opts, sb.buf, &head);\n > > + trailer_block = parse_trailers(opts, sb->buf, &head);\n > \n > So we parse existing trailers from the input strbuf that is supplied\n > by the caller. The rest of this hunk is rewriting FILE* I/O with\n > strbuf addition.\n > \n > > @@ -173,22 +162,40 @@ static void interpret_trailers(const struct process_trailer_options *opts,\n > > }\n > > \n > > /* Print trailer block. */\n > > - format_trailers(opts, &head, &trailer_block_sb);\n > > + format_trailers(opts, &head, out);\n > > free_trailers(&head);\n > > - fwrite(trailer_block_sb.buf, 1, trailer_block_sb.len, outfile);\n > > - strbuf_release(&trailer_block_sb);\n > \n > The format_trailers() helper function appends appends to the strbuf\n > that is given to it, so instead of using an extra strbuf (and then\n > appending that to the output), we just pass our output strbuf to it,\n > which is why we no longer need the trailer_block_sb strbuf anymore.\n > Makes sense.\n > \n > > /* Print the lines after the trailer block as is. */\n > > if (!opts->only_trailers)\n > > - fwrite(sb.buf + trailer_block_end(trailer_block), 1,\n > > - sb.len - trailer_block_end(trailer_block), outfile);\n > > + strbuf_add(out, sb->buf + trailer_block_end(trailer_block),\n > > + sb->len - trailer_block_end(trailer_block));\n > > trailer_block_release(trailer_block);\n > > +}\n > \n > And again, FILE* I/O is replaced with appending to the output strbuf\n > in the rest of this helper function. Good.\n > \n > > +static void interpret_trailers(const struct process_trailer_options *opts,\n > > + struct list_head *new_trailer_head,\n > > + const char *file)\n > \n > So the original caller of interpret_trailers() now call this outer\n > shell, which has the same name and the same function signature as\n > the original. Our new process_trailers() helper assumes a handful\n > of preparatory steps are already done by the caller, so what we are\n > going read here will be mostly those preparation, a call to our new\n > helper, and then printing the result to \"file\" or standard output.\n > \n > > +{\n > > + struct strbuf sb = STRBUF_INIT;\n > > + struct strbuf out = STRBUF_INIT;\n > > + FILE *outfile = stdout;\n > > +\n > > + trailer_config_init();\n > > +\n > > + read_input_file(&sb, file);\n > > + if (opts->in_place)\n > > + outfile = create_in_place_tempfile(file);\n > \n > And these are exactly the lines we lost from the new helper.\n > Looking good.\n > \n > > + process_trailers(opts, new_trailer_head, &sb, &out);\n > \n > And our call. \"out\" should have what we wanted to output to\n > outfile, so ...\n > \n > > + fwrite(out.buf, out.len, 1, outfile);\n > \n > ... we write it out. Good. For a single long string that can never\n > have NUL in it, I'd personally find it more natural to call fputs(),\n > though. Use of fwrite() makes readers unnecessarily wonder if there\n > is something unusual (like needing to be able to handle NULs in the\n > buffer).\n > \n > > if (opts->in_place)\n > > if (rename_tempfile(&trailers_tempfile, file))\n > > die_errno(_(\"could not rename temporary file to %s\"), file);\n > >\n > > strbuf_release(&sb);\n > > + strbuf_release(&out);\n > \n > OK. We could release out a bit earlier, immediately after fwrite().\n > \n > Looking mostly good.\n > \n > > }\n > > \n > > int cmd_interpret_trailers(int argc,\n > \n\nRegards,\n\nLi​\n\n"},{"id":"530471","messageId":"19a6f3cffe1.7bb06d38542332.5515171012908783042@linux.beauty","threadId":"64444","inReplyTo":"xmqqqzucl5xr.fsf@gitster.g","subject":"Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-11-10T19:27:40Z","receivedAt":"2025-11-10T19:27:50Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"Hi Junio,\n\n ---- On Thu, 06 Nov 2025 01:56:00 +0800 Junio C Hamano wrote --- \n > Li Chen writes:\n > \n > > From: Li Chen \n > >\n > > Route all trailer insertion through trailer_process() and make\n > > builtin/interpret-trailers just do file I/O before calling into it.\n > > amend_file_with_trailers() now shares the same code path.\n > >\n > > This removes the fork/exec and tempfile juggling, cutting overhead and\n > > simplifying error handling. No functional change. It also\n > > centralizes logic to prepare for follow-up rebase --trailer patch.\n > >\n > > Signed-off-by: Li Chen \n > > ---\n > > builtin/commit.c | 2 +-\n > > builtin/interpret-trailers.c | 46 +++---------------------\n > > builtin/tag.c | 3 +-\n > > trailer.c | 68 +++++++++++++++++++++++++++++++-----\n > > trailer.h | 5 ++-\n > > wrapper.c | 16 +++++++++\n > > wrapper.h | 6 ++++\n > > 7 files changed, 90 insertions(+), 56 deletions(-)\n > >\n > > diff --git a/builtin/commit.c b/builtin/commit.c\n > > index 0243f17d53..67070d6a54 100644\n > > --- a/builtin/commit.c\n > > +++ b/builtin/commit.c\n > > @@ -1719,7 +1719,7 @@ int cmd_commit(int argc,\n > > OPT_STRING(0, \"fixup\", &fixup_message, N_(\"[(amend|reword):]commit\"), N_(\"use autosquash formatted message to fixup or amend/reword specified commit\")),\n > > OPT_STRING(0, \"squash\", &squash_message, N_(\"commit\"), N_(\"use autosquash formatted message to squash specified commit\")),\n > > OPT_BOOL(0, \"reset-author\", &renew_authorship, N_(\"the commit is authored by me now (used with -C/-c/--amend)\")),\n > > - OPT_PASSTHRU_ARGV(0, \"trailer\", &trailer_args, N_(\"trailer\"), N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG),\n > > + OPT_CALLBACK_F(0, \"trailer\", &trailer_args, N_(\"trailer\"), N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG, parse_opt_strvec),\n > \n > What is this change for?\n > As the external interface of the amend_file_with_trailers() helper\n > did not change in this patch, this cannot be a change that is\n > required to \"remove fork/exec and tempfile juggling\". \n > \n > Or did amend_file_with_trailers() changed behaviour without changing\n > its function signature? If so, this patch does too many things in a\n > single step, I am afraid.\n\nit allows remove the use of skip_prefix in amend_file_with_trailers, and I would add seperate\npatches to make this clearer.\n\n > Perhaps split this step further into multiple patches.\n > \n > - update the internal implementation of amend_file_with_trailers()\n > to avoid having to fork/exec an external process, but *without*\n > changing its external interface at all. This step should not have\n > to touch builtin/commit.c and builtin/tag.c at all.\n > \n > - if the strvec styled after passthru-argv is cumbersome to handle,\n > perform the interface change, such as change from passthru-argv\n > to bare strvec, as a separate step.\n > \n > There might need another preparatory step to clean up the\n > interpret-trailers.c itself before the above two (or there may not\n > be---I haven't thought it through).\n\nThanks, I would split into multiple patches in next version.\n\n > \n > > diff --git a/wrapper.c b/wrapper.c\n > > index 3d507d4204..1f12dbb2fa 100644\n > > --- a/wrapper.c\n > > +++ b/wrapper.c\n > > @@ -688,6 +688,22 @@ void write_file_buf(const char *path, const char *buf, size_t len)\n > > ...\n > > +int write_file_buf_gently(const char *path, const char *buf, size_t len)\n > \n > I do not think this new helper is warranted. You only call it from\n > one place anyway.\n\nok, I would remove write_file_buf_gently and do it in-place.\n\nRegards,\n\nLi​\n\n"},{"id":"530473","messageId":"19a6f3e8332.46772ad5543363.4456434926857828677@linux.beauty","threadId":"64444","inReplyTo":"f5152523-f7ff-4dee-a685-fb0b74cd6a56@gmail.com","subject":"Re: [PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers()","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-11-10T19:29:19Z","receivedAt":"2025-11-10T19:29:31Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"Hi Phillip,\n\n\n ---- On Tue, 11 Nov 2025 00:27:38 +0800 Phillip Wood wrote --- \n > On 05/11/2025 16:57, Junio C Hamano wrote:\n > > Li Chen writes:\n > > \n > >> From: Li Chen \n > >>\n > >> Extracted trailer processing into a helper that accumulates output in\n > >> a strbuf before writing.\n > >>\n > >> Updated interpret_trailers() to reuse the helper, buffer output, and\n > >> clean up both input and output buffers after writing.\n > > \n > > Imperative?\n > > \n > >>\n > >> Signed-off-by: Li Chen \n > >> ---\n > >> builtin/interpret-trailers.c | 51 ++++++++++++++++++++----------------\n > >> 1 file changed, 29 insertions(+), 22 deletions(-)\n > >>\n > >> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n > >> index 41b0750e5a..4c90580fff 100644\n > >> --- a/builtin/interpret-trailers.c\n > >> +++ b/builtin/interpret-trailers.c\n > >> @@ -136,32 +136,21 @@ static void read_input_file(struct strbuf *sb, const char *file)\n > >> strbuf_complete_line(sb);\n > >> }\n > >> \n > >> -static void interpret_trailers(const struct process_trailer_options *opts,\n > >> - struct list_head *new_trailer_head,\n > >> - const char *file)\n > >> +static void process_trailers(const struct process_trailer_options *opts,\n > >> + struct list_head *new_trailer_head,\n > >> + struct strbuf *sb, struct strbuf *out)\n > > \n > > So we gained *out strbuf; in the preimage below I see fwrite(),\n > > fprintf(), etc. to outfile that is either stdout or tempfile, but\n > > presumably the output all will be captured in the strbuf instead,\n > > which makes sense. It is a bit curious what the new paramater sb\n > > is, but this is a file-scope static helper, so it does not strictly\n > > require documenting. Having a comment would still be nicer, though,\n > > unlike \"struct process_trailer_options\" that is very limited\n > > purpose, \"strbuf\" can be used for any string processing, so a good\n > > variable name like \"out\" that conveys what it is used for by\n > > implication is good, but \"sb\", which is obvious abbreviation for\n > > \"Str Buf\", conveys no useful information.\n > \n > This patch is based on my suggestion[1]. I had intended to rename \"sb\" \n > to \"in\" but forgot to do so before posting that diff. Here's my signoff \n > which Li should add before their own\n > \n > Signed-off-by: Phillip Wood \n\nI'm sorry that your signoff is missing; I will add it in the next version.\n\nRegards,\n\nLi​\n\n"},{"id":"530476","messageId":"xmqq4ir1zgl8.fsf@gitster.g","threadId":"64444","inReplyTo":"f5152523-f7ff-4dee-a685-fb0b74cd6a56@gmail.com","subject":"Re: [PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-10T22:08:03Z","receivedAt":"2025-11-10T22:08:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> This patch is based on my suggestion[1]. I had intended to rename \"sb\" \n> to \"in\" but forgot to do so before posting that diff. Here's my signoff \n> which Li should add before their own\n>\n> Signed-off-by: Phillip Wood \n>\n> Thanks\n>\n> Phillip\n\nAh, thanks for clarifying the origin of this patch.\n"},{"id":"530520","messageId":"e13be93f-9d10-4baf-b333-d293c5f46fb5@gmail.com","threadId":"64444","inReplyTo":"20251105142944.73061-4-me@linux.beauty","subject":"Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-11-11T16:55:38Z","receivedAt":"2025-11-11T16:55:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Li\n\nOn 05/11/2025 14:29, Li Chen wrote:\n> From: Li Chen \n> \n> diff --git a/trailer.c b/trailer.c\n> index b735ec8a53..f5838f5699 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> \n> @@ -1224,18 +1226,66 @@ void trailer_iterator_release(struct trailer_iterator *iter)\n> \tstrbuf_release(&iter->key);\n> }\n> \n> -int amend_file_with_trailers(const char *path, const struct strvec *trailer_args)\n> +static int amend_strbuf_with_trailers(struct strbuf *buf,\n> +\t\t\t\t const struct strvec *trailer_args)\n\nWhile reviewing patch 4 I've just realized that this function can never \nfail so should return \"void\" rather than \"int\". I've not quite finished \nwith patch 4 yet, hopefully I'll post a review tomorrow.\n\nThanks\n\nPhillip\n\n> {\n> -\tstruct child_process run_trailer = CHILD_PROCESS_INIT;\n> -\n> -\trun_trailer.git_cmd = 1;\n> -\tstrvec_pushl(&run_trailer.args, \"interpret-trailers\",\n> -\t\t \"--in-place\", \"--no-divider\",\n> -\t\t path, NULL);\n> -\tstrvec_pushv(&run_trailer.args, trailer_args->v);\n> -\treturn run_command(&run_trailer);\n> +\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n> +\tLIST_HEAD(new_trailer_head);\n> +\tstruct strbuf out = STRBUF_INIT;\n> +\tsize_t i;\n> +\n> +\topts.no_divider = 1;\n> +\n> +\tfor (i = 0; i < trailer_args->nr; i++) {\n> +\t\tconst char *text = trailer_args->v[i];\n> +\t\tstruct new_trailer_item *item;\n> +\n> +\t\tif (!*text)\n> +\t\t\tcontinue;\n> +\t\titem = xcalloc(1, sizeof(*item));\n> +\t\tINIT_LIST_HEAD(&item->list);\n> +\t\titem->text = text;\n> +\t\tlist_add_tail(&item->list, &new_trailer_head);\n> +\t}\n> +\n> +\tprocess_trailers(&opts, &new_trailer_head, buf, &out);\n> +\n> +\tstrbuf_swap(buf, &out);\n> +\tstrbuf_release(&out);\n> +\twhile (!list_empty(&new_trailer_head)) {\n> +\t\tstruct new_trailer_item *item =\n> +\t\t\tlist_first_entry(&new_trailer_head, struct new_trailer_item, list);\n> +\t\tlist_del(&item->list);\n> +\t\tfree(item);\n> +\t}\n> +\treturn 0;\n> }\n> \n> +int amend_file_with_trailers(const char *path,\n> +\t\t\t const struct strvec *trailer_args)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\tif (!trailer_args || !trailer_args->nr)\n> +\t\treturn 0;\n> +\n> +\tif (strbuf_read_file(&buf, path, 0) < 0)\n> +\t\treturn error_errno(\"could not read '%s'\", path);\n> +\n> +\tif (amend_strbuf_with_trailers(&buf, trailer_args)) {\n> +\t\tstrbuf_release(&buf);\n> +\t\treturn error(\"failed to append trailers\");\n> +\t}\n> +\n> +\tif (write_file_buf_gently(path, buf.buf, buf.len)) {\n> +\t\tstrbuf_release(&buf);\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tstrbuf_release(&buf);\n> +\treturn 0;\n> + }\n> +\n> void process_trailers(const struct process_trailer_options *opts,\n> \t\t struct list_head *new_trailer_head,\n> \t\t struct strbuf *sb, struct strbuf *out)\n> diff --git a/trailer.h b/trailer.h\n> index 44d406b763..daea46ca5d 100644\n> --- a/trailer.h\n> +++ b/trailer.h\n> @@ -196,9 +196,8 @@ int trailer_iterator_advance(struct trailer_iterator *iter);\n> void trailer_iterator_release(struct trailer_iterator *iter);\n> \n> /*\n> - * Augment a file to add trailers to it by running git-interpret-trailers.\n> - * This calls run_command() and its return value is the same (i.e. 0 for\n> - * success, various non-zero for other errors). See run-command.h.\n> + * Augment a file to add trailers to it (similar to 'git interpret-trailers').\n> + * Returns 0 on success or a non-zero error code on failure.\n> */\n> int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);\n> \n> diff --git a/wrapper.c b/wrapper.c\n> index 3d507d4204..1f12dbb2fa 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -688,6 +688,22 @@ void write_file_buf(const char *path, const char *buf, size_t len)\n> \t\tdie_errno(_(\"could not close '%s'\"), path);\n> }\n> \n> +int write_file_buf_gently(const char *path, const char *buf, size_t len)\n> +{\n> +\tint fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n> +\n> +\tif (fd < 0)\n> +\t\treturn error_errno(_(\"could not open '%s'\"), path);\n> +\tif (write_in_full(fd, buf, len) < 0) {\n> +\t\tint ret = error_errno(_(\"could not write to '%s'\"), path);\n> +\t\tclose(fd);\n> +\t\treturn ret;\n> +\t}\n> +\tif (close(fd))\n> +\t\treturn error_errno(_(\"could not close '%s'\"), path);\n> +\treturn 0;\n> +}\n> +\n> void write_file(const char *path, const char *fmt, ...)\n> {\n> \tva_list params;\n> diff --git a/wrapper.h b/wrapper.h\n> index 44a8597ac3..e5f867b200 100644\n> --- a/wrapper.h\n> +++ b/wrapper.h\n> @@ -56,6 +56,12 @@ static inline ssize_t write_str_in_full(int fd, const char *str)\n> */\n> void write_file_buf(const char *path, const char *buf, size_t len);\n> \n> +/**\n> + * Like write_file_buf(), but report errors instead of exiting. Returns 0 on\n> + * success or a negative value on error after emitting a message.\n> + */\n> +int write_file_buf_gently(const char *path, const char *buf, size_t len);\n> +\n> /**\n> * Like write_file_buf(), but format the contents into a buffer first.\n> * Additionally, write_file() will append a newline if one is not already\n\n"},{"id":"530587","messageId":"0413bf2e-52c4-4944-a349-2a922dd463a2@gmail.com","threadId":"64444","inReplyTo":"20251105142944.73061-5-me@linux.beauty","subject":"Re: [PATCH v6 4/4] rebase: support --trailer","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-11-12T14:48:21Z","receivedAt":"2025-11-12T14:48:24Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Li\n\nOn 05/11/2025 14:29, Li Chen wrote:\n> From: Li Chen \n> \n> diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc\n> index 005caf6164..4d2fe4be6e 100644\n> --- a/Documentation/git-rebase.adoc\n> +++ b/Documentation/git-rebase.adoc\n> @@ -487,9 +487,16 @@ See also INCOMPATIBLE OPTIONS below.\n> \tAdd a `Signed-off-by` trailer to all the rebased commits. Note\n> \tthat if `--interactive` is given then only commits marked to be\n> \tpicked, edited or reworded will have the trailer added.\n> -+\n> +\n> See also INCOMPATIBLE OPTIONS below.\n> \n> +--trailer=::\n> + Append the given trailer line(s) to every rebased commit\n\nI'm not sure we need to say \"line(s)\" here. \"Append the given trailer to \nevery ...\" would be fine I think.\n\n> + message, processed via linkgit:git-interpret-trailers[1].\n> + When this option is present *rebase automatically implies*\n> + `--force-rebase` so that fast‑forwarded commits are also\n> + rewritten.\n\nNormally in cases like this we just say \"This option implies \n`--force-rebase`\".\n\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index c468828189..a88abe08b4 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -500,6 +508,23 @@ static int read_basic_state(struct rebase_options *opts)\n> \t\topts->gpg_sign_opt = xstrdup(buf.buf);\n> \t}\n> \n> +\tstrbuf_reset(&buf);\n> +\n> +\tif (strbuf_read_file(&buf, state_dir_path(\"trailer\", opts), 0) >= 0) {\n> +\t\tconst char *p = buf.buf, *end = buf.buf + buf.len;\n> +\n> +\t\twhile (p < end) {\n> +\t\t\tchar *nl = memchr(p, '\\n', end - p);\n> +\t\t\tif (!nl)\n> +\t\t\t\tdie(\"nl shouldn't be NULL\");\n> +\t\t\t*nl = '\\0';\n> +\n> +\t\t\tif (*p)\n> +\t\t\t\tstrvec_push(&opts->trailer_args, p);\n> +\n> +\t\t\tp = nl + 1;\n> +\t\t}\n> +\t}\n\nAs \"--trailer\" is only supported by the \"merge\" backend we only need to \nread this file in sequencer.c:read_populate_opts(), there is no point in \nreading it here.\n\n> \tstrbuf_release(&buf);\n> \n> \treturn 0;\n> @@ -528,6 +553,21 @@ static int rebase_write_basic_state(struct rebase_options *opts)\n> \tif (opts->signoff)\n> \t\twrite_file(state_dir_path(\"signoff\", opts), \"--signoff\");\n> \n> +\t/*\n> +\t * save opts->trailer_args into state_dir/trailer\n> +\t */\n\nAs \"--trailer\" is not supported by the \"apply\" backend we can just rely \non this being written by sequener.c:write_basic_state(), we don't need \nto do it here\n\n> +\tif (opts->trailer_args.nr) {\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\t\tfor (size_t i = 0; i < opts->trailer_args.nr; i++) {\n> +\t\t\t\tstrbuf_addstr(&buf, opts->trailer_args.v[i]);\n> +\t\t\t\tstrbuf_addch(&buf, '\\n');\n> +\t\t}\n> +\t\twrite_file(state_dir_path(\"trailer\", opts),\n> +\t\t\t\t \"%s\", buf.buf);\n> +\t\tstrbuf_release(&buf);\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> @@ -1132,6 +1172,8 @@ int cmd_rebase(int argc,\n> \t\t\t.flags = PARSE_OPT_NOARG,\n> \t\t\t.defval = REBASE_DIFFSTAT,\n> \t\t},\n> +\t\tOPT_STRVEC(0, \"trailer\", &options.trailer_args, N_(\"trailer\"),\n> +\t\t\t\tN_(\"add custom trailer(s)\")),\n\nThis line should be indented so that the \"N_\" aligns with \"0\" above like \nall the other options here. Putting this next to \"--signoff\" is a good \nchoice.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 5476d39ba9..fbf35cb474 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2025,6 +2027,10 @@ static int append_squash_message(struct strbuf *buf, const char *body,\n> \t\tif (opts->signoff)\n> \t\t\tappend_signoff(buf, 0, 0);\n> \n> +\t\tif (opts->trailer_args.nr &&\n> +\t\t\tamend_strbuf_with_trailers(buf, &opts->trailer_args))\n> +\t\t\treturn error(_(\"unable to add trailers to commit message\"));\n\namend_strbuf_with_trailers() cannot really fail so this should be\n\n\t\tif (opts->trailer_args.nr)\n\t\t\tamend_strbuf_with_trailers(buf, &opts->trailer_args));\n\n> @@ -2443,6 +2449,14 @@ static int do_pick_commit(struct repository *r,\n> \tif (opts->signoff && !is_fixup(command))\n> \t\tappend_signoff(&ctx->message, 0, 0);\n> \n> +\tif (opts->trailer_args.nr && !is_fixup(command)) {\n> +\t\tif (amend_strbuf_with_trailers(&ctx->message,\n> +\t\t\t\t\t &opts->trailer_args)) {\n> +\t\t\tres = error(_(\"unable to add trailers to commit message\"));\n> +\t\t\tgoto leave;\n> +\t\t}\n\nAs above amend_strbuf_with_trailers() cannot fail so we don't need this \nerror handling\n\n> +\t}\n> +\n> \tif (is_rebase_i(opts) && write_author_script(msg.message) < 0)\n> \t\tres = -1;\n> \telse if (!opts->strategy ||\n> @@ -2517,6 +2531,7 @@ static int do_pick_commit(struct repository *r,\n> \t\t\t_(\"dropping %s %s -- patch contents already upstream\\n\"),\n> \t\t\toid_to_hex(&commit->object.oid), msg.subject);\n> \t} /* else allow == 0 and there's nothing special to do */\n> +\n\nWe try to avoid introducing unrelated white space changes\n\n> @@ -3234,6 +3249,17 @@ static int read_populate_opts(struct replay_opts *opts)\n> \n> \t\tread_strategy_opts(opts, &buf);\n> \t\tstrbuf_reset(&buf);\n> +\t\tif (strbuf_read_file(&buf, rebase_path_trailer(), 0) >= 0) {\n> +\t\t\tchar *p = buf.buf, *nl;\n> +\n> +\t\t\twhile ((nl = strchr(p, '\\n'))) {\n> +\t\t\t\t*nl = '\\0';\n> +\t\t\t\tif (*p)\n\nAs we're in control of what's written to the file it is a BUG() if to \ncontains any empty line.\n\n> diff --git a/sequencer.h b/sequencer.h\n> index 719684c8a9..e21835c5a0 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -44,6 +44,7 @@ struct replay_opts {\n> \tint record_origin;\n> \tint no_commit;\n> \tint signoff;\n> +\tstruct strvec trailer_args;\n\nI think it would be better to add this after all the flag options\n\n> \tint allow_ff;\n> \tint allow_rerere_auto;\n> \tint allow_empty;\n> @@ -82,8 +83,9 @@ struct replay_opts {\n> \tstruct replay_ctx *ctx;\n> };\n> #define REPLAY_OPTS_INIT {\t\t\t\\\n> -\t.edit = -1,\t\t\t\t\\\n\n\".edit\" is the first member so it makes sense to leave this at the start.\n\n> \t.action = -1,\t\t\t\t\\\n> +\t.edit = -1,\t\t\t\t\\\n> +\t.trailer_args = STRVEC_INIT, \\\n> \t.xopts = STRVEC_INIT,\t\t\t\\\n> \t.ctx = replay_ctx_new(),\t\t\\\n> }\n\n> diff --git a/t/t3440-rebase-trailer.sh b/t/t3440-rebase-trailer.sh\n> new file mode 100755\n> index 0000000000..d0e0434664\n> --- /dev/null\n> +++ b/t/t3440-rebase-trailer.sh\n> @@ -0,0 +1,134 @@\n> +#!/bin/sh\n> +#\n> +\n> +test_description='git rebase --trailer integration tests\n> +We verify that --trailer works with the merge backend,\n> +and that it is rejected early when the apply backend is requested.'\n> +\n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> +\n> +. ./test-lib.sh\n> +. \"$TEST_DIRECTORY\"/lib-rebase.sh # test_commit_message, helpers\n> +\n> +REVIEWED_BY_TRAILER=\"Reviewed-by: Dev \"\n> +\n> +expect_trailer_msg() {\n> +\ttest_commit_message \"$1\" <<-EOF\n> +\t$2\n> +\n> +\t${3:-$REVIEWED_BY_TRAILER}\n> +\tEOF\n> +}\n> +\n> +test_expect_success 'setup repo with a small history' '\n> +\tgit commit --allow-empty -m \"Initial empty commit\" &&\n> +\ttest_commit first file a &&\n> +\ttest_commit second file &&\n> +\tgit checkout -b conflict-branch first &&\n> +\ttest_commit file-2 file-2 &&\n> +\ttest_commit conflict file &&\n> +\ttest_commit third file\n\nThis leaves us with conflict-branch checked out, is that intentional?\n\n> +'\n> +\n> +test_expect_success 'apply backend is rejected with --trailer' '\n> +\thead_before=$(git rev-parse HEAD) &&\n\nI'm not sure we really need to check that HEAD is unchanged here\n\n> +\ttest_expect_code 128 \\\n> +\tgit rebase --apply --trailer \"$REVIEWED_BY_TRAILER\" \\\n> +\t\t\t\tHEAD^ 2>err &&\n> +\ttest_grep \"fatal: --trailer requires the merge backend\" err &&\n> +\ttest_cmp_rev HEAD $head_before\n> +'\n> +\n> +test_expect_success 'reject empty --trailer argument' '\n> +\ttest_expect_code 128 git rebase -m --trailer \"\" HEAD^ 2>err &&\n\nThere is no need to pass \"-m\" in any of these tests as it is the default \nand if the default backend changes in the future we will want \n\"--trailer\" keeps working with the new default.\n\n> +\ttest_grep \"empty --trailer\" err\n> +'\n> +\n> +test_expect_success 'reject trailer with missing key before separator' '\n> +\ttest_expect_code 128 git rebase -m --trailer \": no-key\" HEAD^ 2>err &&\n> +\ttest_grep \"missing key before separator\" err\n> +'\n> +\n> +test_expect_success 'allow trailer with missing value after separator' '\n> +\tgit rebase -m --trailer \"Acked-by:\" HEAD~1 third &&\n> +\tsed -e \"s/_/ /g\" <<-\\EOF >expect &&\n> +\tthird\n> +\n> +\tAcked-by:_\n> +\tEOF\n> +\ttest_commit_message HEAD expect\n\ntest_commit_message accepts a message on stdin so we don't need to \ncreate expect in all these tests. It is curious that we add a trailing \nspace to the trailer, it would be nice if we could clean that up. \nFailing that other tests use SP=\" \" and then use ${SP} in the here \ndocument like\n\n\ttest_commit_message HEAD <<-EOF\n\tthird\n\t\n\tAcked-by:${SP}\n\tEOF\n\n> +'\n> +\n> +test_expect_success 'CLI trailer duplicates allowed; replace policy keeps last' '\n> +\tgit -c trailer.Bug.ifexists=replace -c trailer.Bug.ifmissing=add \\\n> +\t\trebase -m --trailer \"Bug: 123\" --trailer \"Bug: 456\" HEAD~1 third &&\n> +\tcat >expect <<-\\EOF &&\n> +\tthird\n> +\n> +\tBug: 456\n> +\tEOF\n> +\ttest_commit_message HEAD expect\n> +'\n> +\n> +test_expect_success 'multiple Signed-off-by trailers all preserved' '\n> +\tgit rebase -m \\\n> +\t\t\t--trailer \"Signed-off-by: Dev A \" \\\n> +\t\t\t--trailer \"Signed-off-by: Dev B \" HEAD~1 third &&\n\nThe massive indentation here leads to overly long lines.\n\n\tgit rebase --trailer \"Signed-off-by: Dev A \" \\\n\t\t--trailer \"Signed-off-by: Dev B \" HEAD~1 third &&\n\nfits our 80 column limit\n\n> +\tcat >expect <<-\\EOF &&\n> +\tthird\n> +\n> +\tSigned-off-by: Dev A \n> +\tSigned-off-by: Dev B \n> +\tEOF\n> +\ttest_commit_message HEAD expect\n> +'\n> +\n> +test_expect_success 'rebase -m --trailer adds trailer after conflicts' '\n> +\tgit checkout -B conflict-branch third &&\n\nI'm not sure why we're recreating conflict-branch here, you can just run \n\"git rebase [options] second third\"\n > +\ttest_commit fourth file &&\n> +\ttest_must_fail git rebase -m \\\n> +\t\t\t--trailer \"$REVIEWED_BY_TRAILER\" \\\n> +\t\t\tsecond &&\n\nIf you unfold this line it is 79 characters long which is perfctly fine.\n\n> +\tgit checkout --theirs file &&\n> +\tgit add file &&\n> +\tgit rebase --continue &&\n> +\texpect_trailer_msg HEAD \"fourth\" &&\n\nI think it would be easier to see what's being checked if we just used \ntest_commit_message as we have done up to here and got rid of the \ntest_trailer_msg.\n\n> +\texpect_trailer_msg HEAD^ \"third\"\n\nThis is good because we check that the conflicting commit gets a trailer \nand the commit picked by \"git rebase --continue\" does too.\n\n> +'\n> +\n> +test_expect_success '--trailer handles fixup commands in todo list' '\n> +\tgit checkout -B fixup-trailer HEAD &&\n\nIf you're going to create a new branch you should do it from a tag so it \nhas a known starting point that is not dependent on the previous tests.\n\n> +\ttest_commit fixup-base base &&\n> +\ttest_commit fixup-second second &&\n> +\tfirst_short=$(git rev-parse --short fixup-base) &&\n> +\tsecond_short=$(git rev-parse --short fixup-second) &&\n\nThese two lines are unecessary as test_commit() creates a tag which we \ncan use in the todo list. The here document should also be indented. So\n\n> +\tcat >todo < +pick $first_short fixup-base\n> +fixup $second_short fixup-second\n> +EOF\n\nbecomes\n\tcat >todo <<-\\EOF &&\n\tpick fixup-base first-base\n\tfixup fixup-second fixup-second\n\tEOF\n> +\t(\n> +\t\tset_replace_editor todo &&\n> +\t\tgit rebase -i --trailer \"$REVIEWED_BY_TRAILER\" HEAD~2\n> +\t) &&\n> +\texpect_trailer_msg HEAD \"fixup-base\" &&\n\nWe check there is only one trailer after the fixup - good\n\n> +\tgit reset --hard fixup-second &&\n> +\tcat >todo < +pick $first_short fixup-base\n> +fixup -C $second_short fixup-second\n> +EOF\n> +\t(\n> +\t\tset_replace_editor todo &&\n> +\t\tgit rebase -i --trailer \"$REVIEWED_BY_TRAILER\" HEAD~2\n> +\t) &&\n> +\texpect_trailer_msg HEAD \"fixup-second\"\n\nWe check that we add a trailer with \"fixup -C\" - good> +'\n> +\n> +test_expect_success 'rebase --root --trailer updates every commit' '\n> +\tgit checkout first &&\n> +\tgit -c trailer.review.key=Reviewed-by rebase --root \\\n> +\t\t--trailer=review=\"Dev \" &&\n\nIt would be good if the test title mentioned that we're also checking \n'trailer..key' here as well which is more interesting than \"--root\"\n > +\texpect_trailer_msg HEAD \"first\" &&\n> +\texpect_trailer_msg HEAD^ \"Initial empty commit\"\n\nThe test coverage looks good\n\n> +'\n> +test_done\n> diff --git a/trailer.c b/trailer.c\n> index f5838f5699..f6ff2f01ee 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -774,6 +775,30 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,\n> \tfree(cl_separators);\n> }\n> \n> +void validate_trailer_args_after_config(const struct strvec *cli_args)\n\nI think \"validate_trailer_args\" would be a sufficient name\n> +{\n> +\tchar *cl_separators;\n> +\n> +\ttrailer_config_init();\n> +\n> +\tcl_separators = xstrfmt(\"=%s\", separators);\n> +\n> +\tfor (size_t i = 0; i < cli_args->nr; i++) {\n> +\t\tconst char *txt = cli_args->v[i];\n> +\t\tssize_t separator_pos;\n> +\n> +\t\tif (!*txt)\n> +\t\t\tdie(_(\"empty --trailer argument\"));\n> +\n> +\t\tseparator_pos = find_separator(txt, cl_separators);\n> +\t\tif (separator_pos == 0)\n> +\t\t\tdie(_(\"invalid trailer '%s': missing key before separator\"),\n> +\t\t txt);\n\nStrange indentation here, but the implementation looks sensible.\n\nOverall this is looking good. I've left lots of comments but they're all \nsmall issues\n\nThanks\n\nPhillip\n\n> +\t}\n> +\n> +\tfree(cl_separators);\n> +}\n> +\n> static const char *next_line(const char *str)\n> {\n> \tconst char *nl = strchrnul(str, '\\n');\n> @@ -1226,8 +1251,8 @@ void trailer_iterator_release(struct trailer_iterator *iter)\n> \tstrbuf_release(&iter->key);\n> }\n> \n> -static int amend_strbuf_with_trailers(struct strbuf *buf,\n> -\t\t\t\t const struct strvec *trailer_args)\n> +int amend_strbuf_with_trailers(struct strbuf *buf,\n> +\t\t\t const struct strvec *trailer_args)\n> {\n> \tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n> \tLIST_HEAD(new_trailer_head);\n> diff --git a/trailer.h b/trailer.h\n> index daea46ca5d..541657a11f 100644\n> --- a/trailer.h\n> +++ b/trailer.h\n> @@ -68,6 +68,8 @@ void parse_trailers_from_config(struct list_head *config_head);\n> void parse_trailers_from_command_line_args(struct list_head *arg_head,\n> \t\t\t\t\t struct list_head *new_trailer_head);\n> \n> +void validate_trailer_args_after_config(const struct strvec *cli_args);\n> +\n> void process_trailers_lists(struct list_head *head,\n> \t\t\t struct list_head *arg_head);\n> \n> @@ -195,6 +197,9 @@ int trailer_iterator_advance(struct trailer_iterator *iter);\n> */\n> void trailer_iterator_release(struct trailer_iterator *iter);\n> \n> +int amend_strbuf_with_trailers(struct strbuf *buf,\n> +\t\t\t const struct strvec *trailer_args);\n> +\n> /*\n> * Augment a file to add trailers to it (similar to 'git interpret-trailers').\n> * Returns 0 on success or a non-zero error code on failure.\n\n"},{"id":"530588","messageId":"bfa6c82d-f0b9-4248-88be-8a95bc22ebc1@gmail.com","threadId":"64444","inReplyTo":"20251105142944.73061-1-me@linux.beauty","subject":"Re: [PATCH v6 0/4] rebase: support --trailer","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-11-12T14:50:27Z","receivedAt":"2025-11-12T14:50:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Li\n\nOn 05/11/2025 14:29, Li Chen wrote:\n> From: Li Chen \n> \n> This series routes all trailer insertion through an in-process path, removing\n> the fork/exec to builtin/interpret-trailers and tempfile juggling. The first\n> three commits centralize logic to reduce overhead and simplify error handling.\n> The final commit adds git rebase --trailer, currently supported with the merge\n> backend only (rejecting apply-only scenarios and validating input early).\n\nI've left quite a few comments but overall this is looking much better \nnow, it needs a bit of cleaning up but I didn't spot any major issues.\n\nThanks for working on it\n\nPhillip\n\n> all t/*.sh testcases have run successfully.\n> \n> v6: squash all fix commits and split refactor step from the original patch based on Phillip's suggestion and codes [4].\n> v5: fix all Kristoffer's review comments form v4[3] in place and without new patches.\n> v4: fix all reviewer comments in v3. [2], and add patch 1~8 & 10~29 to fix review comments.\n> v3: merges the remaining trailer paths into one in-process helper, dropping the\n> duplicate code, as pointed by Junio and Phillip [1]\n> v2: fix issues pointed by Phillip\n> RFC link: https://lore.kernel.org/git/196a5ac1393.f5b4db7d187309.2451613571977217927@linux.beauty/\n> \n> Comments very very welcome!\n> \n> [1]: https://lore.kernel.org/git/xmqq8qlzkukw.fsf@gitster.g/\n> [2]: https://lore.kernel.org/git/20250803150059.402017-1-me@linux.beauty/\n> [3]: https://lore.kernel.org/git/20251014122452.1851103-1-me@linux.beauty/\n> [4]: https://lore.kernel.org/git/7d12b046-365f-441c-af8e-8a39d61efbbd@gmail.com/\n> \n> Li Chen (4):\n> interpret-trailers: factor out buffer-based processing to\n> process_trailers()\n> trailer: move process_trailers to trailer.h\n> trailer: append trailers in-process and drop the fork to\n> `interpret-trailers`\n> rebase: support --trailer\n> \n> Documentation/git-rebase.adoc | 9 ++-\n> builtin/commit.c | 2 +-\n> builtin/interpret-trailers.c | 81 ++------------------\n> builtin/rebase.c | 50 +++++++++++++\n> builtin/tag.c | 3 +-\n> sequencer.c | 34 +++++++++\n> sequencer.h | 4 +-\n> t/meson.build | 1 +\n> t/t3440-rebase-trailer.sh | 134 ++++++++++++++++++++++++++++++++++\n> trailer.c | 129 +++++++++++++++++++++++++++++---\n> trailer.h | 13 +++-\n> wrapper.c | 16 ++++\n> wrapper.h | 6 ++\n> 13 files changed, 392 insertions(+), 90 deletions(-)\n> create mode 100755 t/t3440-rebase-trailer.sh\n> \n\n"},{"id":"530787","messageId":"19a8fe42354.3909481a3912041.7970296104893780556@linux.beauty","threadId":"64444","inReplyTo":"bfa6c82d-f0b9-4248-88be-8a95bc22ebc1@gmail.com","subject":"Re: [PATCH v6 0/4] rebase: support --trailer","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-11-17T03:38:04Z","receivedAt":"2025-11-17T03:38:19Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"Hi Phillip,\n\nSorry for my late reply.\n\n ---- On Wed, 12 Nov 2025 22:50:27 +0800 Phillip Wood wrote --- \n > Hi Li\n > \n > On 05/11/2025 14:29, Li Chen wrote:\n > > From: Li Chen \n > > \n > > This series routes all trailer insertion through an in-process path, removing\n > > the fork/exec to builtin/interpret-trailers and tempfile juggling. The first\n > > three commits centralize logic to reduce overhead and simplify error handling.\n > > The final commit adds git rebase --trailer, currently supported with the merge\n > > backend only (rejecting apply-only scenarios and validating input early).\n > \n > I've left quite a few comments but overall this is looking much better \n > now, it needs a bit of cleaning up but I didn't spot any major issues.\n > \n > Thanks for working on it\n \nThanks for your kind reviews! I will address them in the next version.\n\nRegards,\n\nLi​\n\n"},{"id":"531217","messageId":"cb5a792f-c763-4fbf-bfcc-52f66c895c9e@app.fastmail.com","threadId":"64444","inReplyTo":"20251105142944.73061-5-me@linux.beauty","subject":"Re: [PATCH v6 4/4] rebase: support --trailer","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-11-24T15:45:49Z","receivedAt":"2025-11-24T15:46:11Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Wed, Nov 5, 2025, at 15:29, Li Chen wrote:\n> From: Li Chen \n>\n> Implement a new `--trailer ` option for `git rebase`\n> (support merge backend only now), which appends arbitrary\n> trailer lines to each rebased commit message.\n>\n> Reject it if the user passes an option that requires the\n> apply backend (git am) since it lacks message‑filter/trailer\n> hook. otherwise we can just use the merge backend.\n>\n> Automatically set REBASE_FORCE when any trailer is supplied.\n>\n> And reject invalid input before user edits the interactive file.\n>\n> Signed-off-by: Li Chen \n>[snip]\n> diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc\n> index 005caf6164..4d2fe4be6e 100644\n> --- a/Documentation/git-rebase.adoc\n> +++ b/Documentation/git-rebase.adoc\n> @@ -487,9 +487,16 @@ See also INCOMPATIBLE OPTIONS below.\n> \tAdd a `Signed-off-by` trailer to all the rebased commits. Note\n> \tthat if `--interactive` is given then only commits marked to be\n> \tpicked, edited or reworded will have the trailer added.\n> -+\n> +\n> See also INCOMPATIBLE OPTIONS below.\n>\n\nSame problem as I commented on in https://lore.kernel.org/git/cbe93380-e145-4ebd-a213-928b8c3ba085@app.fastmail.com/\n\nThe `See also INCOMPATIBLE OPTIONS below.` is not indented to the same\nlevel as `--signoff`, where it belongs.\n\n> +--trailer=::\n> + Append the given trailer line(s) to every rebased commit\n> + message, processed via linkgit:git-interpret-trailers[1].\n> + When this option is present *rebase automatically implies*\n> + `--force-rebase` so that fast‑forwarded commits are also\n> + rewritten.\n> +\n>[snip]\n"},{"id":"534291","messageId":"xmqqsec0xb50.fsf@gitster.g","threadId":"64444","inReplyTo":"cb5a792f-c763-4fbf-bfcc-52f66c895c9e@app.fastmail.com","subject":"Re: [PATCH v6 4/4] rebase: support --trailer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-20T20:49:31Z","receivedAt":"2026-01-20T20:49:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> On Wed, Nov 5, 2025, at 15:29, Li Chen wrote:\n>> From: Li Chen \n> ...\n> Same problem as I commented on in https://lore.kernel.org/git/cbe93380-e145-4ebd-a213-928b8c3ba085@app.fastmail.com/\n>\n> The `See also INCOMPATIBLE OPTIONS below.` is not indented to the same\n> level as `--signoff`, where it belongs.\n>\n>> +--trailer=::\n>> + Append the given trailer line(s) to every rebased commit\n>> + message, processed via linkgit:git-interpret-trailers[1].\n>> + When this option is present *rebase automatically implies*\n>> + `--force-rebase` so that fast‑forwarded commits are also\n>> + rewritten.\n>> +\n>>[snip]\n\n\nAfter this and [*] the discussion stopped and the topic has been\ndormant since then for full two months. I'd drop the topic from\n'seen' soonish but that does not mean an improved version of this\npatch is unwelcome.\n\nThanks.\n\n[Reference]\n * https://lore.kernel.org/git/19a8fe42354.3909481a3912041.7970296104893780556@linux.beauty/\n"},{"id":"536921","messageId":"19c8e5cd208.389d71793180723.4083726630202768168@linux.beauty","threadId":"64444","inReplyTo":"ef12ada7-13ae-4df0-a823-6f428c797223@gmail.com","subject":"Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2026-02-24T06:36:13Z","receivedAt":"2026-02-24T06:36:27Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"Hi Phillip,\n\n ---- On Tue, 11 Nov 2025 00:38:55 +0800 Phillip Wood wrote --- \n > Hi Li\n > \n > On 05/11/2025 14:29, Li Chen wrote:\n > > From: Li Chen \n > > \n > > diff --git a/builtin/commit.c b/builtin/commit.c\n > > index 0243f17d53..67070d6a54 100644\n > > --- a/builtin/commit.c\n > > +++ b/builtin/commit.c\n > > @@ -1719,7 +1719,7 @@ int cmd_commit(int argc,\n > > OPT_STRING(0, \"fixup\", &fixup_message, N_(\"[(amend|reword):]commit\"), N_(\"use autosquash formatted message to fixup or amend/reword specified commit\")),\n > > OPT_STRING(0, \"squash\", &squash_message, N_(\"commit\"), N_(\"use autosquash formatted message to squash specified commit\")),\n > > OPT_BOOL(0, \"reset-author\", &renew_authorship, N_(\"the commit is authored by me now (used with -C/-c/--amend)\")),\n > > - OPT_PASSTHRU_ARGV(0, \"trailer\", &trailer_args, N_(\"trailer\"), N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG),\n > \n > We have OPT_STRVEC to handle this. The commit message should explain why \n > we're doing this (because we only want to pass the value to \n > amend_file_with_trailers()). Alternatively we could use skip_prefix() in \n > amend_file_with_trailers() to skip the \"--trailer=\" prefix in this patch \n > and then clean it in a separate patch.\n > \n > > + OPT_CALLBACK_F(0, \"trailer\", &trailer_args, N_(\"trailer\"), N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG, parse_opt_strvec),\n > > OPT_BOOL('s', \"signoff\", &signoff, N_(\"add a Signed-off-by trailer\")),\n > > OPT_FILENAME('t', \"template\", &template_file, N_(\"use specified template file\")),\n > > OPT_BOOL('e', \"edit\", &edit_flag, N_(\"force edit of commit\")),\n > > diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n > > index bce2e791d6..268a43372b 100644\n > > --- a/builtin/interpret-trailers.c\n > > +++ b/builtin/interpret-trailers.c\n > > \n > > @@ -142,21 +110,15 @@ static void interpret_trailers(const struct process_trailer_options *opts,\n > > {\n > > struct strbuf sb = STRBUF_INIT;\n > > struct strbuf out = STRBUF_INIT;\n > > - FILE *outfile = stdout;\n > > -\n > > - trailer_config_init();\n > \n > Why is this being moved?\n\nIn v7 I'll initialize trailer config once in\ncmd_interpret_trailers() (after option parsing), and keep\ninterpret_trailers() focused on read input / call helper / emit output.\n\n > > read_input_file(&sb, file);\n > > \n > > - if (opts->in_place)\n > > - outfile = create_in_place_tempfile(file);\n > > -\n > > process_trailers(opts, new_trailer_head, &sb, &out);\n > > \n > > - fwrite(out.buf, out.len, 1, outfile);\n > > if (opts->in_place)\n > > - if (rename_tempfile(&trailers_tempfile, file))\n > > - die_errno(_(\"could not rename temporary file to %s\"), file);\n > > + write_file_buf(file, out.buf, out.len);\n > \n > This truncates the existing file which means that if there is a error \n > while writing the new version the user is now left with garbage rather \n > than the original file which does not seem like a good idea.\n\nGreat catch! v7 will keep --in-place writing via tempfile+rename (no\ntruncate+write), matching the previous behavior.\n\n> ...\n\nRegards,\nLi​\n\n"}]}