{"thread":{"id":"63897","subject":"[PATCH v3 0/2] rebase: support --trailer","startedAt":"2025-08-03T15:01:29Z","lastAt":"2025-10-21T10:02:04Z","messageCount":15,"participants":["Li Chen","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"523340","messageId":"20250803150059.402017-1-me@linux.beauty","threadId":"63897","inReplyTo":null,"subject":"[PATCH v3 0/2] rebase: support --trailer","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-08-03T15:00:55Z","receivedAt":"2025-08-03T15:01:29Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"From: Li Chen \n\nThis two-patch series teaches git rebase a new\n--trailer option and, as a prerequisite, moves all trailer\nhandling out of the external interpret-trailers helper and into the\nbuiltin code path, as suggested by Phillip Wood.\n\nPatch 1 switches trailer.c to an in-memory implementation\n(amend_strbuf_with_trailers()). It removes every fork/exec.\n\nPatch 2 builds on that helper to implement\ngit rebase --trailer. When the option is given we:\nforce the merge backend (apply/am backend lacks a message filter),\nautomatically enable --force-rebase so that fast-forwarded\ncommits are rewritten, and append the requested trailer(s) to every\nrewritten commit.\nState is stored in $state_dir/trailer so an interrupted rebase can\nresume safely. A dedicated test-suite (t3440) exercises plain,\nconflict, --root, invalid-input scenarios and etc.\n\nAll t/*.sh testcases have run successfully.\nGithub CI tests have all been past: https://github.com/FirstLoveLife/git/actions/runs/16704004515\n\nv3: merges the remaining trailer paths into one in-process helper, dropping the\n duplicate 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 welcome!\n\n[1]: https://lore.kernel.org/git/xmqq8qlzkukw.fsf@gitster.g/\n\nLi Chen (2):\n trailer: append trailers in-process and drop the fork to\n `interpret-trailers`\n rebase: support --trailer\n\n Documentation/git-rebase.adoc | 7 ++\n builtin/interpret-trailers.c | 117 ++++++++-----------------------\n builtin/rebase.c | 98 ++++++++++++++++++++++++++\n sequencer.c | 13 ++++\n sequencer.h | 3 +\n t/meson.build | 1 +\n t/t3440-rebase-trailer.sh | 95 ++++++++++++++++++++++++++\n trailer.c | 125 +++++++++++++++++++++++++++++++---\n trailer.h | 18 ++++-\n 9 files changed, 375 insertions(+), 102 deletions(-)\n create mode 100755 t/t3440-rebase-trailer.sh\n\n-- \n2.50.0\n\n"},{"id":"523341","messageId":"20250803150059.402017-2-me@linux.beauty","threadId":"63897","inReplyTo":"20250803150059.402017-1-me@linux.beauty","subject":"[PATCH v3 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers`","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-08-03T15:00:56Z","receivedAt":"2025-08-03T15:01:39Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"From: Li Chen \n\nAll trailer insertion now funnels through trailer_process():\n\n* builtin/interpret-trailers.c is reduced to file I/O + a single call.\n* amend_file_with_trailers() shares the same path; the old\n amend_strbuf_with_trailers() helper is dropped.\n* New helpers parse_trailer_args()/free_new_trailer_list() convert\n --trailer=... strings to new_trailer_item lists.\n\nBehaviour is unchanged; the full test-suite still passes, and the\nfork/exec is gone.\n\nSigned-off-by: Li Chen \n---\n builtin/interpret-trailers.c | 117 ++++++++------------------------\n trailer.c | 125 ++++++++++++++++++++++++++++++++---\n trailer.h | 18 ++++-\n 3 files changed, 158 insertions(+), 102 deletions(-)\n\ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex 44d8ccddc9..3a49d3d7b0 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -9,7 +9,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@@ -84,6 +83,7 @@ static int parse_opt_parse(const struct option *opt, const char *arg,\n \t\t\t int unset)\n {\n \tstruct process_trailer_options *v = opt->value;\n+\n \tv->only_trailers = 1;\n \tv->only_input = 1;\n \tv->unfold = 1;\n@@ -92,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@@ -135,61 +104,6 @@ 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-{\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-\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-\n-\tif (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))\n-\t\tfprintf(outfile, \"\\n\");\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, &trailer_block_sb);\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-\ttrailer_block_release(trailer_block);\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-}\n-\n int cmd_interpret_trailers(int argc,\n \t\t\t const char **argv,\n \t\t\t const char *prefix,\n@@ -231,14 +145,37 @@ 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++)\n-\t\t\tinterpret_trailers(&opts, &trailers, argv[i]);\n+\t\tfor (i = 0; i < argc; i++) {\n+\t\t\tstruct strbuf in_buf = STRBUF_INIT;\n+\t\t\tstruct strbuf out_buf = STRBUF_INIT;\n+\n+\t\t\tread_input_file(&in_buf, argv[i]);\n+\t\t\tif (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)\n+\t\t\t\tdie(_(\"failed to process trailers for %s\"), argv[i]);\n+\t\t\tif (opts.in_place)\n+\t\t\t\twrite_file_buf(argv[i], out_buf.buf, out_buf.len);\n+\t\t\telse\n+\t\t\t\tfwrite(out_buf.buf, 1, out_buf.len, stdout);\n+\t\t\tstrbuf_release(&in_buf);\n+\t\t\tstrbuf_release(&out_buf);\n+\t\t}\n \t} else {\n+\t\tstruct strbuf in_buf = STRBUF_INIT;\n+\t\tstruct strbuf out_buf = STRBUF_INIT;\n+\n \t\tif (opts.in_place)\n \t\t\tdie(_(\"no input file given for in-place editing\"));\n-\t\tinterpret_trailers(&opts, &trailers, NULL);\n+\n+\t\tread_input_file(&in_buf, NULL);\n+\t\tif (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)\n+\t\t\tdie(_(\"failed to process trailers\"));\n+\t\tfwrite(out_buf.buf, 1, out_buf.len, stdout);\n+\t\tstrbuf_release(&in_buf);\n+\t\tstrbuf_release(&out_buf);\n \t}\n \n \tnew_trailers_clear(&trailers);\ndiff --git a/trailer.c b/trailer.c\nindex 310cf582dc..03814443c3 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1224,14 +1224,121 @@ 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 *arg = trailer_args->v[i];\n+\t\tconst char *text;\n+\t\tstruct new_trailer_item *item;\n+\t\tif (!skip_prefix(arg, \"--trailer=\", &text))\n+\t\t\ttext = arg;\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+\tif (trailer_process(&opts, buf->buf, &new_trailer_head, &out) < 0)\n+\t\tdie(\"failed to process trailers\");\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 trailer_process(const struct process_trailer_options *opts,\n+\t\t\t\t const char *msg,\n+\t\t\t\t struct list_head *new_trailer_head,\n+\t\t\t\t struct strbuf *out)\n+{\n+\t\tstruct trailer_block *blk;\n+\t\tLIST_HEAD(orig_head);\n+\t\tLIST_HEAD(config_head);\n+\t\tLIST_HEAD(arg_head);\n+\t\tstruct strbuf trailers_sb = STRBUF_INIT;\n+\t\tint had_trailer_before;\n+\n+\t\tblk = parse_trailers(opts, msg, &orig_head);\n+\t\thad_trailer_before = !list_empty(&orig_head);\n+\t\tif (!opts->only_input) {\n+\t\t\tparse_trailers_from_config(&config_head);\n+\t\t\tparse_trailers_from_command_line_args(&arg_head, new_trailer_head);\n+\t\t\tlist_splice(&config_head, &arg_head);\n+\t\t\tprocess_trailers_lists(&orig_head, &arg_head);\n+\t\t}\n+\t\tformat_trailers(opts, &orig_head, &trailers_sb);\n+\t\tif (!opts->only_trailers && !opts->only_input && !opts->unfold &&\n+\t\t\t!opts->trim_empty && list_empty(&orig_head) &&\n+\t\t\t(list_empty(new_trailer_head) || opts->only_input)) {\n+\t\t\tsize_t split = trailer_block_start(blk); /* end-of-log-msg */\n+\t\t\tif (!blank_line_before_trailer_block(blk)) {\n+\t\t\t\tstrbuf_add(out, msg, split);\n+\t\t\t\tstrbuf_addch(out, '\\n');\n+\t\t\t\tstrbuf_addstr(out, msg + split);\n+\t\t\t} else\n+\t\t\t\tstrbuf_addstr(out, msg);\n+\n+\t\t\tstrbuf_release(&trailers_sb);\n+\t\t\ttrailer_block_release(blk);\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (opts->only_trailers) {\n+\t\t\tstrbuf_addbuf(out, &trailers_sb);\n+\t\t} else if (had_trailer_before) {\n+\t\t\tstrbuf_add(out, msg, trailer_block_start(blk));\n+\t\t\tif (!blank_line_before_trailer_block(blk))\n+\t\t\t\tstrbuf_addch(out, '\\n');\n+\t\t\tstrbuf_addbuf(out, &trailers_sb);\n+\t\t\tstrbuf_add(out, msg + trailer_block_end(blk),\n+\t\t\t\t\t\tstrlen(msg) - trailer_block_end(blk));\n+\t\t}\n+\t\telse {\n+\t\t\tsize_t cpos = trailer_block_start(blk);\n+\t\t\tstrbuf_add(out, msg, cpos);\n+\t\t\tif (cpos == 0) /* empty body → just one \\n */\n+\t\t\t\tstrbuf_addch(out, '\\n');\n+\t\t\telse if (!blank_line_before_trailer_block(blk))\n+\t\t\t\tstrbuf_addch(out, '\\n'); /* body without trailing blank */\n+\n+\t\t\tstrbuf_addbuf(out, &trailers_sb);\n+\t\t\tstrbuf_add(out, msg + cpos, strlen(msg) - cpos);\n+\t }\n+\t\tstrbuf_release(&trailers_sb);\n+\t\tfree_trailers(&orig_head);\n+\t\ttrailer_block_release(blk);\n+\t\treturn 0;\n+}\n+\n+int amend_file_with_trailers(const char *path,\n+\t\t\t\t\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\tdie(\"failed to append trailers\");\n+\n+\t/* `write_file_buf()` aborts on error internally */\n+\twrite_file_buf(path, buf.buf, buf.len);\n+\tstrbuf_release(&buf);\n+\treturn 0;\n+ }\ndiff --git a/trailer.h b/trailer.h\nindex 4740549586..01f711fb13 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -196,10 +196,22 @@ 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+/*\n+ * Process trailer lines for a commit message in-memory.\n+ * @opts: trailer processing options (e.g. from parse-options)\n+ * @msg: the input message string\n+ * @new_trailer_head: list of new trailers to add (struct new_trailer_item)\n+ * @out: strbuf to store the resulting message (must be initialized)\n+ *\n+ * Returns 0 on success, <0 on error.\n+ */\n+int trailer_process(const struct process_trailer_options *opts,\n+\t\t\tconst char *msg,\n+\t\t\tstruct list_head *new_trailer_head,\n+\t\t\tstruct strbuf *out);\n #endif /* TRAILER_H */\n-- \n2.50.0\n\n"},{"id":"523342","messageId":"20250803150059.402017-3-me@linux.beauty","threadId":"63897","inReplyTo":"20250803150059.402017-1-me@linux.beauty","subject":"[PATCH v3 2/2] rebase: support --trailer","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-08-03T15:00:57Z","receivedAt":"2025-08-03T15:01:49Z","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. Reject early\nif used with the apply backend (git am) since it lacks\nmessage‑filter/trailer hook. Automatically set REBASE_FORCE when\nany trailer is supplied.\n\nAnd reject invalid input before user edit the interactive file.\n\nSigned-off-by: Li Chen \n---\n Documentation/git-rebase.adoc | 7 +++\n builtin/rebase.c | 98 +++++++++++++++++++++++++++++++++++\n sequencer.c | 13 +++++\n sequencer.h | 3 ++\n t/meson.build | 1 +\n t/t3440-rebase-trailer.sh | 95 +++++++++++++++++++++++++++++++++\n 6 files changed, 217 insertions(+)\n create mode 100755 t/t3440-rebase-trailer.sh\n\ndiff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc\nindex 956d3048f5..df8fb97526 100644\n--- a/Documentation/git-rebase.adoc\n+++ b/Documentation/git-rebase.adoc\n@@ -521,6 +521,13 @@ See also INCOMPATIBLE OPTIONS below.\n \tthat if `--interactive` is given then only commits marked to be\n \tpicked, edited or reworded will have the trailer added.\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 enables*\n+ `--force-rebase` so that fast‑forwarded commits are also\n+ rewritten.\n+\n See also INCOMPATIBLE OPTIONS below.\n \n -i::\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex e90562a3b8..3b4c45a616 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -36,6 +36,8 @@\n #include \"reset.h\"\n #include \"trace2.h\"\n #include \"hook.h\"\n+#include \"trailer.h\"\n+#include \"parse-options.h\"\n \n static char const * const builtin_rebase_usage[] = {\n \tN_(\"git rebase [-i] [options] [--exec ] \"\n@@ -113,6 +115,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 +146,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 +170,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 +182,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@@ -435,6 +444,8 @@ static int read_basic_state(struct rebase_options *opts)\n \tstruct strbuf head_name = STRBUF_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct object_id oid;\n+\tconst char trailer_state_name[] = \"trailer\";\n+\tconst char *path = state_dir_path(trailer_state_name, opts);\n \n \tif (!read_oneliner(&head_name, state_dir_path(\"head-name\", opts),\n \t\t\t READ_ONELINER_WARN_MISSING) ||\n@@ -503,11 +514,31 @@ static int read_basic_state(struct rebase_options *opts)\n \n \tstrbuf_release(&buf);\n \n+\tif (strbuf_read_file(&buf, path, 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\tstrbuf_release(&buf);\n+\t}\n+\tstrbuf_release(&buf);\n+\n \treturn 0;\n }\n \n static int rebase_write_basic_state(struct rebase_options *opts)\n {\n+\tconst char trailer_state_name[] = \"trailer\";\n+\n \twrite_file(state_dir_path(\"head-name\", opts), \"%s\",\n \t\t opts->head_name ? opts->head_name : \"detached HEAD\");\n \twrite_file(state_dir_path(\"onto\", opts), \"%s\",\n@@ -529,6 +560,22 @@ 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+ /*\n+ * save opts->trailer_args into state_dir/trailer\n+ */\n+ if (opts->trailer_args.nr) {\n+ struct strbuf buf = STRBUF_INIT;\n+ size_t i;\n+\n+ for (i = 0; i < opts->trailer_args.nr; i++) {\n+ strbuf_addstr(&buf, opts->trailer_args.v[i]);\n+ strbuf_addch(&buf, '\\n');\n+ }\n+ write_file(state_dir_path(trailer_state_name, opts),\n+ \"%s\", buf.buf);\n+ strbuf_release(&buf);\n+ }\n+\n \treturn 0;\n }\n \n@@ -1085,6 +1132,37 @@ static int check_exec_cmd(const char *cmd)\n \treturn 0;\n }\n \n+static int validate_trailer_args_after_config(const struct strvec *cli_args,\n+\t\t\t\t struct strbuf *err)\n+{\n+\tsize_t i;\n+\n+\tfor (i = 0; i < cli_args->nr; i++) {\n+\t\tconst char *raw = cli_args->v[i];\n+\t\tconst char *txt; // Key[:=]Val\n+\t\tconst char *sep;\n+\n+\t\tif (!skip_prefix(raw, \"--trailer=\", &txt))\n+\t\t\ttxt = raw;\n+\n+\t\tif (!*txt) {\n+\t\t\tstrbuf_addstr(err, _(\"empty --trailer argument\"));\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\tsep = strpbrk(txt, \":=\");\n+\n+\t\t/* there must be key bfore seperator */\n+\t\tif (sep && sep == txt) {\n+\t\t\tstrbuf_addf(err,\n+\t\t\t\t _(\"invalid trailer '%s': missing key before separator\"),\n+\t\t\t\t txt);\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n int cmd_rebase(int argc,\n \t const char **argv,\n \t const char *prefix,\n@@ -1133,6 +1211,7 @@ 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_(\"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@@ -1283,6 +1362,17 @@ int cmd_rebase(int argc,\n \t\t\t builtin_rebase_options,\n \t\t\t builtin_rebase_usage, 0);\n \n+ /* if add --trailer，force rebase */\n+\tif (options.trailer_args.nr) {\n+ struct strbuf err = STRBUF_INIT;\n+\n+\t\tif (validate_trailer_args_after_config(&options.trailer_args, &err))\n+\t\t\tdie(\"%s\", err.buf);\n+\n+ options.flags |= REBASE_FORCE;\n+ strbuf_release(&err);\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@@ -1540,6 +1630,14 @@ int cmd_rebase(int argc,\n \tif (options.root && !options.onto_name)\n \t\timply_merge(&options, \"--root without --onto\");\n \n+\t/*\n+\t * The apply‑based backend (git am) cannot append trailers because\n+\t * it lacks a message‑filter facility. Reject early, before any\n+\t * state (index, HEAD, etc.) is modified.\n+\t */\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 67e4310edc..58faf6aed5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -422,6 +422,7 @@ void replay_opts_release(struct replay_opts *opts)\n \tfree(opts->revs);\n \treplay_ctx_release(ctx);\n \tfree(opts->ctx);\n+\tstrvec_clear(&opts->trailer_args);\n }\n \n int sequencer_remove_state(struct replay_opts *opts)\n@@ -2529,6 +2530,18 @@ 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+ if (!res && opts->trailer_args.nr && !drop_commit) {\n+ const char *trailer_file =\n+ msg_file ? msg_file : git_path_merge_msg(r);\n+\n+ if (amend_file_with_trailers(trailer_file,\n+ &opts->trailer_args)) {\n+ res = error(_(\"unable to add trailers to commit message\"));\n+ goto leave;\n+ }\n+ }\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,\ndiff --git a/sequencer.h b/sequencer.h\nindex 304ba4b4d3..28f2da6375 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -4,6 +4,7 @@\n #include \"strbuf.h\"\n #include \"strvec.h\"\n #include \"wt-status.h\"\n+#include \n \n struct commit;\n struct index_state;\n@@ -44,6 +45,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@@ -86,6 +88,7 @@ struct replay_opts {\n \t.action = -1,\t\t\t\t\\\n \t.xopts = STRVEC_INIT,\t\t\t\\\n \t.ctx = replay_ctx_new(),\t\t\\\n+\t.trailer_args = STRVEC_INIT, \\\n }\n \n /*\ndiff --git a/t/meson.build b/t/meson.build\nindex 09f3068f98..3c58f562da 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -373,6 +373,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..a580449628\n--- /dev/null\n+++ b/t/t3440-rebase-trailer.sh\n@@ -0,0 +1,95 @@\n+#!/bin/sh\n+#\n+\n+test_description='git rebase --trailer integration tests\n+We verify that --trailer on the merge/interactive/exec/root backends,\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+create_expect() {\n+\tcat >\"$1\" <<-EOF\n+\t\t$2\n+\n+\t\tReviewed-by: Dev \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+\tident=\"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" &&\n+\tcreate_expect initial-signed \"Initial empty commit\" &&\n+\tcreate_expect first-signed \"first\" &&\n+\tcreate_expect second-signed \"second\" &&\n+\tcreate_expect file2-signed \"file-2\" &&\n+\tcreate_expect third-signed \"third\" &&\n+\tcreate_expect conflict-signed \"conflict\"\n+'\n+\n+test_expect_success 'apply backend is rejected with --trailer' '\n+\tgit reset --hard third &&\n+\thead_before=$(git rev-parse HEAD) &&\n+ test_expect_code 128 \\\n+ \t\tgit rebase --apply --trailer \"Reviewed-by: Dev \" \\\n+ \t\t\tHEAD^ 2>err &&\n+\ttest_grep \"requires the merge backend\" err &&\n+\ttest_cmp_rev HEAD $head_before\n+'\n+\n+test_expect_success 'reject empty --trailer argument' '\n+ git reset --hard third &&\n+ test_expect_code 128 git rebase -m --trailer \"\" HEAD^ 2>err &&\n+ test_grep \"empty --trailer\" err\n+'\n+\n+test_expect_success 'reject trailer with missing key before separator' '\n+ git reset --hard third &&\n+ test_expect_code 128 git rebase -m --trailer \": no-key\" HEAD^ 2>err &&\n+ test_grep \"missing key before separator\" err\n+'\n+\n+test_expect_success 'CLI trailer duplicates allowed; replace policy keeps last' '\n+ git reset --hard third &&\n+ git -c trailer.Bug.ifexists=replace -c trailer.Bug.ifmissing=add rebase -m --trailer \"Bug: 123\" --trailer \"Bug: 456\" HEAD~1 &&\n+ git cat-file commit HEAD | grep \"^Bug: 456\" &&\n+ git cat-file commit HEAD | grep -v \"^Bug: 123\"\n+'\n+\n+test_expect_success 'multiple Signed-off-by trailers all preserved' '\n+ git reset --hard third &&\n+ git rebase -m \\\n+ --trailer \"Signed-off-by: Dev A \" \\\n+ --trailer \"Signed-off-by: Dev B \" HEAD~1 &&\n+ git cat-file commit HEAD | grep -c \"^Signed-off-by:\" >count &&\n+ test \"$(cat count)\" = 2 # two new commits\n+'\n+\n+test_expect_success 'rebase -m --trailer adds trailer after conflicts' '\n+\tgit reset --hard third &&\n+\ttest_must_fail git rebase -m \\\n+\t\t--trailer \"Reviewed-by: Dev \" \\\n+\t\tsecond third &&\n+\tgit checkout --theirs file &&\n+\tgit add file &&\n+ git rebase --continue &&\n+\ttest_commit_message HEAD~2 file2-signed\n+'\n+\n+test_expect_success 'rebase --root --trailer updates every commit' '\n+\tgit checkout first &&\n+\tgit rebase --root --keep-empty \\\n+\t\t--trailer \"Reviewed-by: Dev \" &&\n+\ttest_commit_message HEAD first-signed &&\n+\ttest_commit_message HEAD^ initial-signed\n+'\n+test_done\n-- \n2.50.0\n\n"},{"id":"523349","messageId":"xmqq8qk0fjma.fsf@gitster.g","threadId":"63897","inReplyTo":"20250803150059.402017-1-me@linux.beauty","subject":"Re: [PATCH v3 0/2] rebase: support --trailer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-03T16:35:57Z","receivedAt":"2025-08-03T16:36:00Z","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 two-patch series teaches git rebase a new\n> --trailer option and, as a prerequisite, moves all trailer\n> handling out of the external interpret-trailers helper and into the\n> builtin code path, as suggested by Phillip Wood.\n>\n> Patch 1 switches trailer.c to an in-memory implementation\n> (amend_strbuf_with_trailers()). It removes every fork/exec.\n>\n> Patch 2 builds on that helper to implement\n> git rebase --trailer.\n\nTry running \"git show --check\" on this commit. My attempt found a\nhandful of whitespace breakages (\"indent with spaces.\").\n\nThanks.\n\n"},{"id":"523367","messageId":"19872c0a7f9.4f0ce123219344.1150677041521426116@linux.beauty","threadId":"63897","inReplyTo":"xmqq8qk0fjma.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] rebase: support --trailer","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-08-04T01:44:45Z","receivedAt":"2025-08-04T01:44:57Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"Hi Junio,\n\n ---- On Mon, 04 Aug 2025 00:35:57 +0800 Junio C Hamano wrote --- \n > Li Chen writes:\n > \n > > From: Li Chen \n > >\n > > This two-patch series teaches git rebase a new\n > > --trailer option and, as a prerequisite, moves all trailer\n > > handling out of the external interpret-trailers helper and into the\n > > builtin code path, as suggested by Phillip Wood.\n > >\n > > Patch 1 switches trailer.c to an in-memory implementation\n > > (amend_strbuf_with_trailers()). It removes every fork/exec.\n > >\n > > Patch 2 builds on that helper to implement\n > > git rebase --trailer.\n > \n > Try running \"git show --check\" on this commit. My attempt found a\n > handful of whitespace breakages (\"indent with spaces.\").\n > \n > Thanks.\n > \n > \n\nThat's a great tool, I wasn't aware of that command, and I will fix them in\nNext version. Thanks, Junio.\n\nRegards,\nLi\n"},{"id":"523546","messageId":"d4c9f082-52be-48d9-b817-fcb8a72e1bd7@gmail.com","threadId":"63897","inReplyTo":"20250803150059.402017-2-me@linux.beauty","subject":"Re: [PATCH v3 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers`","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-08-05T13:17:01Z","receivedAt":"2025-08-05T13:17:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Li\n\nOn 03/08/2025 16:00, Li Chen wrote:\n> From: Li Chen \n> \n> All trailer insertion now funnels through trailer_process():\n> \n> * builtin/interpret-trailers.c is reduced to file I/O + a single call.\n> * amend_file_with_trailers() shares the same path; the old\n> amend_strbuf_with_trailers() helper is dropped.\n> * New helpers parse_trailer_args()/free_new_trailer_list() convert\n> --trailer=... strings to new_trailer_item lists.\n> \n> Behaviour is unchanged; the full test-suite still passes, and the\n> fork/exec is gone.\n\nNormally commit messages should be written in prose rather than a bullet \nlist and the message should explain the reason for the change.\n\nThis patch has much less code duplication than the last iteration which \nis most welcome. Whenever you are moving and refactoring code you should \nsplit the move into its own commit followed by the refactoring. That \nmakes it much easier to review as the reviewer can clearly see the \nrefactoring rather than having to manually compare the added code in one \nfile to the deleted code in another.\n\n> @@ -84,6 +83,7 @@ static int parse_opt_parse(const struct option *opt, const char *arg,\n> \t\t\t int unset)\n> {\n> \tstruct process_trailer_options *v = opt->value;\n> +\n\nLet's not clutter this patch with unrelated changes.\n\n> \tv->only_trailers = 1;\n> \tv->only_input = 1;\n> \tv->unfold = 1;\n> @@ -92,37 +92,6 @@ static int parse_opt_parse(const struct option *opt, const char *arg,\n> \treturn 0;\n> }\n> \n> -static FILE *create_in_place_tempfile(const char *file)\n> -{\n> [...]\n> -}\n\nWe don't need to create a temporary file anymore so this can be deleted \n- 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> -\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> -\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> -\n> -\tif (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))\n> -\t\tfprintf(outfile, \"\\n\");\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, &trailer_block_sb);\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> -\ttrailer_block_release(trailer_block);\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> -}\n\nThis code is moved to trailer.c which is good but it is heavily \nrefactored at the same time which makes it hard to review. Completely \nremoving this function leads to some duplication in \ncmd_interpret_trailers() which could be avoided by making \ninterpret_trailers() a wrapper around process_trailers()\n\n> int cmd_interpret_trailers(int argc,\n> \t\t\t const char **argv,\n> \t\t\t const char *prefix,\n> @@ -231,14 +145,37 @@ 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++)\n> -\t\t\tinterpret_trailers(&opts, &trailers, argv[i]);\n> +\t\tfor (i = 0; i < argc; i++) {\n> +\t\t\tstruct strbuf in_buf = STRBUF_INIT;\n> +\t\t\tstruct strbuf out_buf = STRBUF_INIT;\n> +\n> +\t\t\tread_input_file(&in_buf, argv[i]);\n> +\t\t\tif (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)\n> +\t\t\t\tdie(_(\"failed to process trailers for %s\"), argv[i]);\n> +\t\t\tif (opts.in_place)\n> +\t\t\t\twrite_file_buf(argv[i], out_buf.buf, out_buf.len);\n> +\t\t\telse\n> +\t\t\t\tfwrite(out_buf.buf, 1, out_buf.len, stdout);\n> +\t\t\tstrbuf_release(&in_buf);\n> +\t\t\tstrbuf_release(&out_buf);\n> +\t\t}\n> \t} else {\n> +\t\tstruct strbuf in_buf = STRBUF_INIT;\n> +\t\tstruct strbuf out_buf = STRBUF_INIT;\n> +\n> \t\tif (opts.in_place)\n> \t\t\tdie(_(\"no input file given for in-place editing\"));\n> -\t\tinterpret_trailers(&opts, &trailers, NULL);\n> +\n> +\t\tread_input_file(&in_buf, NULL);\n> +\t\tif (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)\n> +\t\t\tdie(_(\"failed to process trailers\"));\n> +\t\tfwrite(out_buf.buf, 1, out_buf.len, stdout);\n> +\t\tstrbuf_release(&in_buf);\n> +\t\tstrbuf_release(&out_buf);\n> \t}\n\nThere is quite a bit of duplication here that could be avoided if you \nmodified interpret_trailers() to call trailer_process() rather than \ndeleting it entirely.\n\n> \tnew_trailers_clear(&trailers);\n> diff --git a/trailer.c b/trailer.c\n> index 310cf582dc..03814443c3 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -1224,14 +1224,121 @@ 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\nFunction argument declarations should be aligned\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 *arg = trailer_args->v[i];\n> +\t\tconst char *text;\n> +\t\tstruct new_trailer_item *item;\n\nThere should be a blank line after the variable declarations at the \nstart of each block of code.\n\n> +\t\tif (!skip_prefix(arg, \"--trailer=\", &text))\n\nWhy do we need this? It would be much cleaner if we required the caller \nto pass a list of trailers without any optional prefix.\n\n> +\t\t\ttext = arg;\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> +\tif (trailer_process(&opts, buf->buf, &new_trailer_head, &out) < 0)\n> +\t\tdie(\"failed to process trailers\");\n\nAs this is library code lets return an error here rather than dying.\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 trailer_process(const struct process_trailer_options *opts,\n> +\t\t\t\t const char *msg,\n> +\t\t\t\t struct list_head *new_trailer_head,\n> +\t\t\t\t struct strbuf *out)\n\nArgument alignment again\n\n> +{\n> +\t\tstruct trailer_block *blk;\n\nThis is trailer_block in the original but has been re-ordered with \nrespect to the other variable declarations making the patch harder to \nreview.\n\n> +\t\tLIST_HEAD(orig_head);\n\nThis is head in the original but moved relative to the other variable \ndeclarations\n\n> +\t\tLIST_HEAD(config_head);\n> +\t\tLIST_HEAD(ar1g_head);\n\nThese two have been moved from inside the if (!opts->only_input) below. \nThey are only referenced there so do not need to be declared here. \nMoving them makes this patch harder to review.\n\n> +\t\tstruct strbuf trailers_sb = STRBUF_INIT;\n\nThis is from the original but moved relative to the other variable \ndeclarations.\n\n> +\t\tint had_trailer_before;\n\nThis is new - lets see how it is used. We've just started using bool for \nboolean variables in the last few weeks so this could be a bool now.\n\n From here to\n\n> +\t\tblk = parse_trailers(opts, msg, &orig_head);\n> +\t\thad_trailer_before = !list_empty(&orig_head);\n> +\t\tif (!opts->only_input) {\n> +\t\t\tparse_trailers_from_config(&config_head);\n> +\t\t\tparse_trailers_from_command_line_args(&arg_head, new_trailer_head);\n> +\t\t\tlist_splice(&config_head, &arg_head);\n> +\t\t\tprocess_trailers_lists(&orig_head, &arg_head);\n> +\t\t}\n> +\t\tformat_trailers(opts, &orig_head, &trailers_sb);\n\nhere is copied from the original minus the code that copied the commit \nmessage to the output file. Rather than deleting the code that copied \nthe commit message we could have replaced the calls to fwrite() and \nfprintf() with strbuf_add() and strbuf_addf() which would make it \nobvious that the behavior is not changed. The original then frees \norig_head but that is done later here.\n\n> +\t\tif (!opts->only_trailers && !opts->only_input && !opts->unfold &&\n> +\t\t\t!opts->trim_empty && list_empty(&orig_head) &&\n> +\t\t\t(list_empty(new_trailer_head) || opts->only_input)) {\n\nI'm not sure what is happening here. By this point the original has \ncopied the original commit message and is ready to append the new \ntrailers. Instead the new version seems to have completely refactored \nthe logic for adding the new trailers making it harder to see if the \nbehavior has changed.\n\n> +\t\t\tsize_t split = trailer_block_start(blk); /* end-of-log-msg */\n> +\t\t\tif (!blank_line_before_trailer_block(blk)) {\n> +\t\t\t\tstrbuf_add(out, msg, split);\n> +\t\t\t\tstrbuf_addch(out, '\\n');\n> +\t\t\t\tstrbuf_addstr(out, msg + split);\n\nThis copies the original message but adds a newline before the trailer \nblock if it is missing.\n\n> +\t\t\t} else\n> +\t\t\t\tstrbuf_addstr(out, msg);\n\nThis just copies the whole message.\n\n> +\t\t\tstrbuf_rel2ease(&trailers_sb);\n> +\t\t\ttrailer_block_release(blk);\n\n\n> +\t\t\treturn 0;\n\nWe return a copy of the original message with no new trailers added. We \ndo not free orig_head, arg_head or config_head. I'm still confused why \nwe need to special case this.\n\n\n> +\t\t}\n> +\t\tif (opts->only_trailers) {\n> +\t\t\tstrbuf_addbuf(out, &trailers_sb);\n\nThis flips the logic in the original to handle opts->only_trailers \nseparately making it harder to review.\n\n> +\t\t} else if (had_trailer_before) {\n> +\t\t\tstrbuf_add(out, msg, trailer_block_start(blk));\n> +\t\t\tif (!blank_line_before_trailer_block(blk))\n> +\t\t\t\tstrbuf_addch(out, '\\n');\n> +\t\t\tstrbuf_addbuf(out, &trailers_sb);\n> +\t\t\tstrbuf_add(out, msg + trailer_block_end(blk),\n> +\t\t\t\t\t\tstrlen(msg) - trailer_block_end(blk));\n\nThis handles the case where we're replacing the headers in the original \nmessage\n\n> +\t\t}\n> +\t\telse {\n\nStyle - this should be \"} else {\"\n\n> +\t\t\tsize_t cpos = trailer_block_start(blk);\n> +\t\t\tstrbuf_add(out, msg, cpos);\n> +\t\t\tif (cpos == 0) /* empty body → just one \\n */\n> +\t\t\t\tstrbuf_addch(out, '\\n');\n> +\t\t\telse if (!blank_line_before_trailer_block(blk))\n> +\t\t\t\tstrbuf_addch(out, '\\n'); /* body without trailing blank */\n> +\n> +\t\t\tstrbuf_addbuf(out, &trailers_sb);\n> +\t\t\tstrbuf_add(out, msg + cpos, strlen(msg) - cpos);\n> +\t }\n\nI'm confused why we need a separate case for when the original did not \nhave any trailers - was the original code broken? If it was we should \nseparate out the bug fix from the refactoring. If not what's the point \nof this change?\n\n> +\t\tstrbuf_release(&trailers_sb);\n> +\t\tfree_trailers(&orig_head);\n> +\t\ttrailer_block_release(blk);\n> +\t\treturn 0;\n> +}\n> +\n> +int amend_file_with_trailers(const char *path,\n> +\t\t\t\t\t\t\t const struct strvec *trailer_args)\n\nAlignment again\n\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\tdie(\"failed to append trailers\");\n\nWhy return an error() above but die() here? This is library code so lets \nreturn an error.\n\n> +\n> +\t/* `write_file_buf()` aborts on error internally */\n> +\twrite_file_buf(path, buf.buf, buf.len);\n\nDying here is a change in behavior which callers might not be expecting. \nThe original code always returned a error because it forked a \nsub-process to do the trailer processing. Ideally, in a separate commit, \nwe'd update any existing callers that have the message in an strbuf so \nthey don't have to write it to a file just to add some trailers to it.\n\n> +\tstrbuf_release(&buf);\n> +\treturn 0;\n> + }\n\nAs I said above reusing the existing code as you have done here is a \nmuch better approach. However it would be much easier to review if the \ncode movement was separated from the refactoring. I'm also struggling to \nsee the benefit of a lot of the refactoring - I was expecting the \nconversion to use an strubf would essentially look like fwrite() being \nreplaced with strbuf_add() and fprintf() being replaced with \nstrbuf_addf() etc. rather than reworking the logic.\n\nThanks\n\n\nPhillip\n\n> diff --git a/trailer.h b/trailer.h\n> index 4740549586..01f711fb13 100644\n> --- a/trailer.h\n> +++ b/trailer.h\n> @@ -196,10 +196,22 @@ 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> +/*\n> + * Process trailer lines for a commit message in-memory.\n> + * @opts: trailer processing options (e.g. from parse-options)\n> + * @msg: the input message string\n> + * @new_trailer_head: list of new trailers to add (struct new_trailer_item)\n> + * @out: strbuf to store the resulting message (must be initialized)\n> + *\n> + * Returns 0 on success, <0 on error.\n> + */\n> +int trailer_process(const struct process_trailer_options *opts,\n> +\t\t\tconst char *msg,\n> +\t\t\tstruct list_head *new_trailer_head,\n> +\t\t\tstruct strbuf *out);\n> #endif /* TRAILER_H */\n\n"},{"id":"523563","messageId":"539639d8-8751-453a-b445-9651842bca1e@gmail.com","threadId":"63897","inReplyTo":"20250803150059.402017-3-me@linux.beauty","subject":"Re: [PATCH v3 2/2] rebase: support --trailer","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-08-05T15:38:19Z","receivedAt":"2025-08-05T15:38:28Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 03/08/2025 16:00, 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. Reject early\n> if used with the apply backend (git am) since it lacks\n> message‑filter/trailer hook.\n\nWe only want to reject it if the user passes an option that requires the \napply backend, otherwise we can just use the merge backend.\n\n> Automatically set REBASE_FORCE when\n> any trailer is supplied.\n\nMakes sense\n> And reject invalid input before user edit the interactive file.\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 enables*\n> + `--force-rebase` so that fast‑forwarded commits are also\n> + rewritten.\n\nThe options like --reset-author-date just say \"implies `--force-rebase`\"\n\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index e90562a3b8..3b4c45a616 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -36,6 +36,8 @@\n> #include \"reset.h\"\n> #include \"trace2.h\"\n> #include \"hook.h\"\n> +#include \"trailer.h\"\n> +#include \"parse-options.h\"\n\n\"parse-options.h\" is already included.\n> @@ -435,6 +444,8 @@ static int read_basic_state(struct rebase_options *opts)\n> \tstruct strbuf head_name = STRBUF_INIT;\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tstruct object_id oid;\n> +\tconst char trailer_state_name[] = \"trailer\";\n> +\tconst char *path = state_dir_path(trailer_state_name, opts);\n\nWe only use this once, so lets pass state_dir_path(\"trailer\", opts) \ndirectly to strbuf_read_file() like we do when reading all the other \nstate files in this function.\n\n> \tif (!read_oneliner(&head_name, state_dir_path(\"head-name\", opts),\n> \t\t\t READ_ONELINER_WARN_MISSING) ||\n> @@ -503,11 +514,31 @@ static int read_basic_state(struct rebase_options *opts)\n> \n> \tstrbuf_release(&buf);\n\nThis should become strbuf_reset(&buf) so that we can reuse the allocation.\n\n> +\tif (strbuf_read_file(&buf, path, 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\nThis is needed because we allow an empty file but reading an empty file \nis a bug because we only write this file if the user gave --trailers on \nthe command line and we should reject empty trailers before getting here.\n\n> +\t\t\t\tstrvec_push(&opts->trailer_args, p);\n> +\n> +\t\t\tp = nl + 1;\n> +\t\t}\n> +\t\tstrbuf_release(&buf);\n\nThis is unnecessary, it is released below which is a better place as it \nmeans the buffer is freed even if strbuf_read_file() fails.\n\n> +\t}\n> +\tstrbuf_release(&buf);\n> +\n> \treturn 0;\n> }\n> \n> static int rebase_write_basic_state(struct rebase_options *opts)\n> {\n> +\tconst char trailer_state_name[] = \"trailer\";\n\nThis is unnecessary, lets follow the existing style of just using the \nfilename directly in the one place it is needed.\n\n> + /*\n> + * save opts->trailer_args into state_dir/trailer\n> + */\n> + if (opts->trailer_args.nr) {\n> + struct strbuf buf = STRBUF_INIT;\n> + size_t i;\n> +\n> + for (i = 0; i < opts->trailer_args.nr; i++) {\n\nWe can use \"for (size_t i = 0; i < ...) {\" here\n\n> + strbuf_addstr(&buf, opts->trailer_args.v[i]);\n> + strbuf_addch(&buf, '\\n');\n> + }\n> + write_file(state_dir_path(trailer_state_name, opts),\n> + \"%s\", buf.buf);\n> + strbuf_release(&buf);\n> + }\n> +\n> \treturn 0;\n> }\n\nThis looks good\n> +static int validate_trailer_args_after_config(const struct strvec *cli_args,\n> +\t\t\t\t struct strbuf *err)\n> +{\n> +\tsize_t i;\n> +\n> +\tfor (i = 0; i < cli_args->nr; i++) {\n\nAs i is only used in the loop we can say \"for (size_t i = 0; ...) {\" and \ndrop the declaration above.\n\n> +\t\tconst char *raw = cli_args->v[i];\n> +\t\tconst char *txt; // Key[:=]Val\n> +\t\tconst char *sep;\n> +\n> +\t\tif (!skip_prefix(raw, \"--trailer=\", &txt))\n> +\t\t\ttxt = raw;\n\nOPT_STRVEC() only stores the argument, not the option name so we don't \nneed this\n\n> +\n> +\t\tif (!*txt) {\n> +\t\t\tstrbuf_addstr(err, _(\"empty --trailer argument\"));\n> +\t\t\treturn -1;\n\nI'm not sure we need the err buf here - we can just\n\n\treturn error(_(\"empty --trailer argument\"));\n\n> +\t\t}\n> +\n> +\t\tsep = strpbrk(txt, \":=\");\n\nThe list of separators is configurable - if we move this function into \ntrailer.c then we can use find_separator().\n\n> +\n> +\t\t/* there must be key bfore seperator */\n> +\t\tif (sep && sep == txt) {\n> +\t\t\tstrbuf_addf(err,\n> +\t\t\t\t _(\"invalid trailer '%s': missing key before separator\"),\n> +\t\t\t\t txt);\n\nWe can use return error(...) here as well.\n\n> @@ -1133,6 +1211,7 @@ 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_(\"add custom trailer(s)\")),\n\nThis line is rather long, can we fold it like the one below please.\n\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> @@ -1283,6 +1362,17 @@ int cmd_rebase(int argc,\n> \t\t\t builtin_rebase_options,\n> \t\t\t builtin_rebase_usage, 0);\n> \n> + /* if add --trailer，force rebase */\n\nI'm not sure what \"add --trailer\" means. Forcing the rebase when \n--trailer is given is a good idea.\n\n> +\tif (options.trailer_args.nr) {\n> + struct strbuf err = STRBUF_INIT;\n\nThe indentation in this block is off here and below\n\n> +\t\tif (validate_trailer_args_after_config(&options.trailer_args, &err))\n> +\t\t\tdie(\"%s\", err.buf);\n> +\n> + options.flags |= REBASE_FORCE;\n> + strbuf_release(&err);\n> +\t}\n\n> +\t/*\n> +\t * The apply‑based backend (git am) cannot append trailers because\n> +\t * it lacks a message‑filter facility. Reject early, before any\n> +\t * state (index, HEAD, etc.) is modified.\n> +\t */\n\nThis comment is a bit confusing - what are we rejecting here? The call \nto imply_merge() is correct.\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> \n> diff --git a/sequencer.c b/sequencer.c\n> index 67e4310edc..58faf6aed5 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -422,6 +422,7 @@ void replay_opts_release(struct replay_opts *opts)\n> \tfree(opts->revs);\n> \treplay_ctx_release(ctx);\n> \tfree(opts->ctx);\n> +\tstrvec_clear(&opts->trailer_args);\n\nLets move this up a bit before the ctx stuff as that should the last \nmember of the struct.\n\n> @@ -2529,6 +2530,18 @@ 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\nThis feels a bit late in the function to be doing this. Can be add the \ntrailers just after we add the sign off around line 2457. That way we \ncan add the trailers to ctx->message before writing the commit message \nfile. Like --signoff we also need to make sure we don't add trailers \nwhen we're processing a fixup or squash command.\n\n> + if (!res && opts->trailer_args.nr && !drop_commit) {\n> + const char *trailer_file =\n> + msg_file ? msg_file : git_path_merge_msg(r);\n> +\n> + if (amend_file_with_trailers(trailer_file,\n> + &opts->trailer_args)) {\n> + res = error(_(\"unable to add trailers to commit message\"));\n> + goto leave;\n> + }\n> + }\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> diff --git a/sequencer.h b/sequencer.h\n> index 304ba4b4d3..28f2da6375 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -4,6 +4,7 @@\n> #include \"strbuf.h\"\n> #include \"strvec.h\"\n> #include \"wt-status.h\"\n> +#include \n\nWe never add system headers like this. git-compat-util.h takes care of \nthat in a portable manner.\n\n> struct commit;\n> struct index_state;\n> @@ -44,6 +45,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> @@ -86,6 +88,7 @@ struct replay_opts {\n> \t.action = -1,\t\t\t\t\\\n> \t.xopts = STRVEC_INIT,\t\t\t\\\n> \t.ctx = replay_ctx_new(),\t\t\\\n> +\t.trailer_args = STRVEC_INIT, \\\n> }\n\nLets keep the order of members in the initializer the same as in the \nstruct declaration.\n\nI've run out of time for today, I'll take a look at the tests later in \nthe week. Although I've left quite a few comments the code in this patch \nlooks basically sound.\n\nThanks\n\nPhillip\n> /*\n> diff --git a/t/meson.build b/t/meson.build\n> index 09f3068f98..3c58f562da 100644\n> --- a/t/meson.build\n> +++ b/t/meson.build\n> @@ -373,6 +373,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',\n> diff --git a/t/t3440-rebase-trailer.sh b/t/t3440-rebase-trailer.sh\n> new file mode 100755\n> index 0000000000..a580449628\n> --- /dev/null\n> +++ b/t/t3440-rebase-trailer.sh\n> @@ -0,0 +1,95 @@\n> +#!/bin/sh\n> +#\n> +\n> +test_description='git rebase --trailer integration tests\n> +We verify that --trailer on the merge/interactive/exec/root backends,\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> +create_expect() {\n> +\tcat >\"$1\" <<-EOF\n> +\t\t$2\n> +\n> +\t\tReviewed-by: Dev \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> +\tident=\"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" &&\n> +\tcreate_expect initial-signed \"Initial empty commit\" &&\n> +\tcreate_expect first-signed \"first\" &&\n> +\tcreate_expect second-signed \"second\" &&\n> +\tcreate_expect file2-signed \"file-2\" &&\n> +\tcreate_expect third-signed \"third\" &&\n> +\tcreate_expect conflict-signed \"conflict\"\n> +'\n> +\n> +test_expect_success 'apply backend is rejected with --trailer' '\n> +\tgit reset --hard third &&\n> +\thead_before=$(git rev-parse HEAD) &&\n> + test_expect_code 128 \\\n> + \t\tgit rebase --apply --trailer \"Reviewed-by: Dev \" \\\n> + \t\t\tHEAD^ 2>err &&\n> +\ttest_grep \"requires the merge backend\" err &&\n> +\ttest_cmp_rev HEAD $head_before\n> +'\n> +\n> +test_expect_success 'reject empty --trailer argument' '\n> + git reset --hard third &&\n> + test_expect_code 128 git rebase -m --trailer \"\" HEAD^ 2>err &&\n> + test_grep \"empty --trailer\" err\n> +'\n> +\n> +test_expect_success 'reject trailer with missing key before separator' '\n> + git reset --hard third &&\n> + test_expect_code 128 git rebase -m --trailer \": no-key\" HEAD^ 2>err &&\n> + test_grep \"missing key before separator\" err\n> +'\n> +\n> +test_expect_success 'CLI trailer duplicates allowed; replace policy keeps last' '\n> + git reset --hard third &&\n> + git -c trailer.Bug.ifexists=replace -c trailer.Bug.ifmissing=add rebase -m --trailer \"Bug: 123\" --trailer \"Bug: 456\" HEAD~1 &&\n> + git cat-file commit HEAD | grep \"^Bug: 456\" &&\n> + git cat-file commit HEAD | grep -v \"^Bug: 123\"\n> +'\n> +\n> +test_expect_success 'multiple Signed-off-by trailers all preserved' '\n> + git reset --hard third &&\n> + git rebase -m \\\n> + --trailer \"Signed-off-by: Dev A \" \\\n> + --trailer \"Signed-off-by: Dev B \" HEAD~1 &&\n> + git cat-file commit HEAD | grep -c \"^Signed-off-by:\" >count &&\n> + test \"$(cat count)\" = 2 # two new commits\n> +'\n> +\n> +test_expect_success 'rebase -m --trailer adds trailer after conflicts' '\n> +\tgit reset --hard third &&\n> +\ttest_must_fail git rebase -m \\\n> +\t\t--trailer \"Reviewed-by: Dev \" \\\n> +\t\tsecond third &&\n> +\tgit checkout --theirs file &&\n> +\tgit add file &&\n> + git rebase --continue &&\n> +\ttest_commit_message HEAD~2 file2-signed\n> +'\n> +\n> +test_expect_success 'rebase --root --trailer updates every commit' '\n> +\tgit checkout first &&\n> +\tgit rebase --root --keep-empty \\\n> +\t\t--trailer \"Reviewed-by: Dev \" &&\n> +\ttest_commit_message HEAD first-signed &&\n> +\ttest_commit_message HEAD^ initial-signed\n> +'\n> +test_done\n"},{"id":"523641","messageId":"e911d897-8664-40a7-b7a9-8eb9f71a8735@gmail.com","threadId":"63897","inReplyTo":"20250803150059.402017-3-me@linux.beauty","subject":"Re: [PATCH v3 2/2] rebase: support --trailer","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-08-06T10:28:58Z","receivedAt":"2025-08-06T10:29:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Li\n\nOn 03/08/2025 16:00, Li Chen wrote:\n> From: Li Chen \nPicking up from where I left off yesterday ...\n\n> diff --git a/t/meson.build b/t/meson.build\n> index 09f3068f98..3c58f562da 100644\n> --- a/t/meson.build\n> +++ b/t/meson.build\n> @@ -373,6 +373,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\nThe alignment looks off here\n\n> 't3500-cherry.sh',\n> 't3501-revert-cherry-pick.sh',\n> 't3502-cherry-pick-merge.sh',\n> diff --git a/t/t3440-rebase-trailer.sh b/t/t3440-rebase-trailer.sh\n> new file mode 100755\n> index 0000000000..a580449628\n> --- /dev/null\n> +++ b/t/t3440-rebase-trailer.sh\n> @@ -0,0 +1,95 @@\n> +#!/bin/sh\n> +#\n> +\n> +test_description='git rebase --trailer integration tests\n> +We verify that --trailer on the merge/interactive/exec/root backends,\n\nThere are only two backends \"apply\" and \"merge\", the other things you \nhave listed are just command line options. We don't actually test --exec \nor --interactive with --trailer in this file so we should reword that \ncomment. There is no need to add tests for those.\n\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> +create_expect() {\n> +\tcat >\"$1\" <<-EOF\n> +\t\t$2\n> +\n> +\t\tReviewed-by: Dev \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> +\tident=\"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" &&\n> +\tcreate_expect initial-signed \"Initial empty commit\" &&\n> +\tcreate_expect first-signed \"first\" &&\n> +\tcreate_expect second-signed \"second\" &&\n> +\tcreate_expect file2-signed \"file-2\" &&\n> +\tcreate_expect third-signed \"third\" &&\n> +\tcreate_expect conflict-signed \"conflict\"\n\nNormally we create the \"expect\" file in the test where it is used.\n\n> +'\n> +\n> +test_expect_success 'apply backend is rejected with --trailer' '\n\nWe know HEAD is at third so we don't need this\n\n> +\tgit reset --hard third &&\n> +\thead_before=$(git rev-parse HEAD) &&\n> + test_expect_code 128 \\\n\nThe indentation is off here\n\n> + \t\tgit rebase --apply --trailer \"Reviewed-by: Dev \" \\\n> + \t\t\tHEAD^ 2>err &&\n> +\ttest_grep \"requires the merge backend\" err &&\n\nI think it is worth checking that the error message includes --trailer \nas well to make sure that is the option that is triggering the error.\n\n> +\ttest_cmp_rev HEAD $head_before\n> +'\n> +\n> +test_expect_success 'reject empty --trailer argument' '\n> + git reset --hard third &&\n\nThe exact commit is not important here\n\n> + test_expect_code 128 git rebase -m --trailer \"\" HEAD^ 2>err &&\n> + test_grep \"empty --trailer\" err\n> +'\n> +\n> +test_expect_success 'reject trailer with missing key before separator' '\n> + git reset --hard third &&\n\nSame here - we're only checking the error message so the exact value of \nHEAD is not important.\n\n> + test_expect_code 128 git rebase -m --trailer \": no-key\" HEAD^ 2>err &&\n> + test_grep \"missing key before separator\" err\n> +'\n> +\n> +test_expect_success 'CLI trailer duplicates allowed; replace policy keeps last' '\n> + git reset --hard third &&\n> + git -c trailer.Bug.ifexists=replace -c trailer.Bug.ifmissing=add rebase -m --trailer \"Bug: 123\" --trailer \"Bug: 456\" HEAD~1 &&\n> + git cat-file commit HEAD | grep \"^Bug: 456\" &&\n> + git cat-file commit HEAD | grep -v \"^Bug: 123\"\n\nPiping git into another command is discouraged as git can potentially \nfail without us noticing. Here I think it would be better to use \ntest_commit_message() to check the whole message.\n> +'\n> +\n> +test_expect_success 'multiple Signed-off-by trailers all preserved' '\n> + git reset --hard third &&\n\nWe can avoid this by passing the commit we want to checkout to rebase as \nyou do in the test below.\n\n> + git rebase -m \\\n> + --trailer \"Signed-off-by: Dev A \" \\\n> + --trailer \"Signed-off-by: Dev B \" HEAD~1 &&\n> + git cat-file commit HEAD | grep -c \"^Signed-off-by:\" >count &&\n> + test \"$(cat count)\" = 2 # two new commits\n\nLet check the actual message here as well so if it fails we can see what \nthe message is.\n\n> +'\n> +\n> +test_expect_success 'rebase -m --trailer adds trailer after conflicts' '\n> +\tgit reset --hard third &&\n\nWe don't need this as the rebase command checks out third for us, saving \na process which is always nice.\n\n> +\ttest_must_fail git rebase -m \\\n> +\t\t--trailer \"Reviewed-by: Dev \" \\\n> +\t\tsecond third &&\n> +\tgit checkout --theirs file &&\n> +\tgit add file &&\n> + git rebase --continue &&\n\nThe indentation is off here\n\n> +\ttest_commit_message HEAD~2 file2-signed\n\nIt's good to see this uses test_commit_message but why are we checking \nHEAD~2 rather than HEAD?> +'\n> +\n> +test_expect_success 'rebase --root --trailer updates every commit' '\n> +\tgit checkout first &&\n> +\tgit rebase --root --keep-empty \\\n\n--keep-empty is the default these days so I think we can drop that.\n\n> +\t\t--trailer \"Reviewed-by: Dev \" &&\n> +\ttest_commit_message HEAD first-signed &&\n> +\ttest_commit_message HEAD^ initial-signed\n\nLooks good.\n\nWhile there are some small issues to fix the tests look sensible and I \nthink you have good coverage of the new option.\n\nThanks\n\nPhillip\n"},{"id":"523657","messageId":"499da566-66a8-4c38-a2b3-13c06092568f@gmail.com","threadId":"63897","inReplyTo":"e911d897-8664-40a7-b7a9-8eb9f71a8735@gmail.com","subject":"Re: [PATCH v3 2/2] rebase: support --trailer","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-08-06T13:19:57Z","receivedAt":"2025-08-06T13:20:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Li\n\nI had a couple more thoughts about the tests ...\n\nOn 06/08/2025 11:28, Phillip Wood wrote:\n> On 03/08/2025 16:00, Li Chen wrote:\n>> +create_expect() {\n>> +    cat >\"$1\" <<-EOF\n>> +        $2\n>> +\n>> +        Reviewed-by: Dev \n>> +    EOF\n>> +}\n>> +\n>> +test_expect_success 'setup repo with a small history' '\n>> [...]\n >> +    create_expect third-signed    \"third\"                 &&>> +    \ncreate_expect conflict-signed \"conflict\"\n> \n> Normally we create the \"expect\" file in the test where it is used.\n\nThinking about this some more, if we want to use test_commit_message \nthen I think we can change create_expect to write to stdout and do\n\n\ttest_commit_message HEAD <<-EOF\n\t$(create_expect first)\n\tEOF\n\nrather than having to create a file.\n\n>> +\n>> +test_expect_success 'reject empty --trailer argument' '\n>> [...]\n>> +test_expect_success 'reject trailer with missing key before separator' '\n\nShould we also test for a missing value or are trailers without a value \nallowed?\n\n>> +        git rebase -m \\\n>> +            --trailer \"Signed-off-by: Dev A \" \\\n>> +            --trailer \"Signed-off-by: Dev B \" HEAD~1 &&\n\nLets use example.com here rather than some random domain that might \nactually exist.\n\n>> +test_expect_success 'rebase -m --trailer adds trailer after conflicts' '\n>> +    git reset --hard third &&\n>> +    test_must_fail git rebase -m \\\n>> +        --trailer \"Reviewed-by: Dev \" \\\n>> +        second third &&\n>> +    git checkout --theirs file &&\n>> +    git add file &&\n>> +    git rebase --continue &&\n\nThis checks that the commit with conflicts has a trailer added but it \ndoes not check that the commits picked by \"git rebase --continue\" do. To \ncheck that we actually save the trailers and use them when continuing we \nneed to add a fourth commit on top of third and check that has a trailer \nadd here as well.\n\nA couple more thoughts:\n\n - We should check that\n git -c trailer.review.key=Reviewed-by rebase \\\n --trailer=review=\"Dev \"\n adds a \"Reviewed-by:\" trailer. We can do that by changing one of the\n tests in this patch rather than adding a new one. This checks that we\n accept '=' as a separator as well a respecting the config.\n\n - We should check that the todo list\n pick first\n fixup second\n adds the trailer as expected and that\n pick first\n fixup -C second\n also works. To do that we will need to source lib-rebase.sh at the\n start of the test file and add a test that uses set_replace_editor()\n which should be called in a subshell.\n\n\nDo please ask if you have any questions about these suggestions\n\nThanks\n\nPhillip\n\n"},{"id":"523695","messageId":"198826665b6.317113211709957.1514728503207030488@linux.beauty","threadId":"63897","inReplyTo":"499da566-66a8-4c38-a2b3-13c06092568f@gmail.com","subject":"Re: [PATCH v3 2/2] rebase: support --trailer","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-08-07T02:40:05Z","receivedAt":"2025-08-07T02:40:18Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"Hi Phillip, \n\nThanks for your thorough review; I will address them in the next version.\n\n ---- On Wed, 06 Aug 2025 21:19:57 +0800 Phillip Wood wrote --- \n > Hi Li\n > \n > I had a couple more thoughts about the tests ...\n > \n > On 06/08/2025 11:28, Phillip Wood wrote:\n > > On 03/08/2025 16:00, Li Chen wrote:\n > >> +create_expect() {\n > >> + cat >\"$1\" <<-EOF\n > >> + $2\n > >> +\n > >> + Reviewed-by: Dev \n > >> + EOF\n > >> +}\n > >> +\n > >> +test_expect_success 'setup repo with a small history' '\n > >> [...]\n > >> + create_expect third-signed \"third\" &&>> + \n > create_expect conflict-signed \"conflict\"\n > > \n > > Normally we create the \"expect\" file in the test where it is used.\n > \n > Thinking about this some more, if we want to use test_commit_message \n > then I think we can change create_expect to write to stdout and do\n > \n > test_commit_message HEAD <<-EOF\n > $(create_expect first)\n > EOF\n > \n > rather than having to create a file.\n > \n > >> +\n > >> +test_expect_success 'reject empty --trailer argument' '\n > >> [...]\n > >> +test_expect_success 'reject trailer with missing key before separator' '\n > \n > Should we also test for a missing value or are trailers without a value \n > allowed?\n > \n > >> + git rebase -m \\\n > >> + --trailer \"Signed-off-by: Dev A \" \\\n > >> + --trailer \"Signed-off-by: Dev B \" HEAD~1 &&\n > \n > Lets use example.com here rather than some random domain that might \n > actually exist.\n > \n > >> +test_expect_success 'rebase -m --trailer adds trailer after conflicts' '\n > >> + git reset --hard third &&\n > >> + test_must_fail git rebase -m \\\n > >> + --trailer \"Reviewed-by: Dev \" \\\n > >> + second third &&\n > >> + git checkout --theirs file &&\n > >> + git add file &&\n > >> + git rebase --continue &&\n > \n > This checks that the commit with conflicts has a trailer added but it \n > does not check that the commits picked by \"git rebase --continue\" do. To \n > check that we actually save the trailers and use them when continuing we \n > need to add a fourth commit on top of third and check that has a trailer \n > add here as well.\n > \n > A couple more thoughts:\n > \n > - We should check that\n > git -c trailer.review.key=Reviewed-by rebase \\\n > --trailer=review=\"Dev \"\n > adds a \"Reviewed-by:\" trailer. We can do that by changing one of the\n > tests in this patch rather than adding a new one. This checks that we\n > accept '=' as a separator as well a respecting the config.\n > \n > - We should check that the todo list\n > pick first\n > fixup second\n > adds the trailer as expected and that\n > pick first\n > fixup -C second\n > also works. To do that we will need to source lib-rebase.sh at the\n > start of the test file and add a test that uses set_replace_editor()\n > which should be called in a subshell.\n > \n > \n > Do please ask if you have any questions about these suggestions\n\nRegards,\n\nLi​\n\n"},{"id":"523696","messageId":"1988266b4aa.56246ae51710041.3057149707985856726@linux.beauty","threadId":"63897","inReplyTo":"499da566-66a8-4c38-a2b3-13c06092568f@gmail.com","subject":"Re: [PATCH v3 2/2] rebase: support --trailer","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-08-07T02:40:25Z","receivedAt":"2025-08-07T02:40:36Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"\n\n\n ---- On Wed, 06 Aug 2025 21:19:57 +0800 Phillip Wood wrote --- \n > Hi Li\n > \n > I had a couple more thoughts about the tests ...\n > \n > On 06/08/2025 11:28, Phillip Wood wrote:\n > > On 03/08/2025 16:00, Li Chen wrote:\n > >> +create_expect() {\n > >> + cat >\"$1\" <<-EOF\n > >> + $2\n > >> +\n > >> + Reviewed-by: Dev \n > >> + EOF\n > >> +}\n > >> +\n > >> +test_expect_success 'setup repo with a small history' '\n > >> [...]\n > >> + create_expect third-signed \"third\" &&>> + \n > create_expect conflict-signed \"conflict\"\n > > \n > > Normally we create the \"expect\" file in the test where it is used.\n > \n > Thinking about this some more, if we want to use test_commit_message \n > then I think we can change create_expect to write to stdout and do\n > \n > test_commit_message HEAD <<-EOF\n > $(create_expect first)\n > EOF\n > \n > rather than having to create a file.\n > \n > >> +\n > >> +test_expect_success 'reject empty --trailer argument' '\n > >> [...]\n > >> +test_expect_success 'reject trailer with missing key before separator' '\n > \n > Should we also test for a missing value or are trailers without a value \n > allowed?\n > \n > >> + git rebase -m \\\n > >> + --trailer \"Signed-off-by: Dev A \" \\\n > >> + --trailer \"Signed-off-by: Dev B \" HEAD~1 &&\n > \n > Lets use example.com here rather than some random domain that might \n > actually exist.\n > \n > >> +test_expect_success 'rebase -m --trailer adds trailer after conflicts' '\n > >> + git reset --hard third &&\n > >> + test_must_fail git rebase -m \\\n > >> + --trailer \"Reviewed-by: Dev \" \\\n > >> + second third &&\n > >> + git checkout --theirs file &&\n > >> + git add file &&\n > >> + git rebase --continue &&\n > \n > This checks that the commit with conflicts has a trailer added but it \n > does not check that the commits picked by \"git rebase --continue\" do. To \n > check that we actually save the trailers and use them when continuing we \n > need to add a fourth commit on top of third and check that has a trailer \n > add here as well.\n > \n > A couple more thoughts:\n > \n > - We should check that\n > git -c trailer.review.key=Reviewed-by rebase \\\n > --trailer=review=\"Dev \"\n > adds a \"Reviewed-by:\" trailer. We can do that by changing one of the\n > tests in this patch rather than adding a new one. This checks that we\n > accept '=' as a separator as well a respecting the config.\n > \n > - We should check that the todo list\n > pick first\n > fixup second\n > adds the trailer as expected and that\n > pick first\n > fixup -C second\n > also works. To do that we will need to source lib-rebase.sh at the\n > start of the test file and add a test that uses set_replace_editor()\n > which should be called in a subshell.\n > \n > \n > Do please ask if you have any questions about these suggestions\n > \n > Thanks\n > \n > Phillip\n > \n > \nRegards,\n\nLi​\n\n"},{"id":"523697","messageId":"198826af571.62b85cb31711042.2415806544948206668@linux.beauty","threadId":"63897","inReplyTo":"d4c9f082-52be-48d9-b817-fcb8a72e1bd7@gmail.com","subject":"Re: [PATCH v3 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers`","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-08-07T02:45:04Z","receivedAt":"2025-08-07T02:45:13Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"Hi Phillip,\n\n ---- On Tue, 05 Aug 2025 21:17:01 +0800 Phillip Wood wrote --- \n > Hi Li\n > \n > On 03/08/2025 16:00, Li Chen wrote:\n > > From: Li Chen \n > > \n > > All trailer insertion now funnels through trailer_process():\n > > \n > > * builtin/interpret-trailers.c is reduced to file I/O + a single call.\n > > * amend_file_with_trailers() shares the same path; the old\n > > amend_strbuf_with_trailers() helper is dropped.\n > > * New helpers parse_trailer_args()/free_new_trailer_list() convert\n > > --trailer=... strings to new_trailer_item lists.\n > > \n > > Behaviour is unchanged; the full test-suite still passes, and the\n > > fork/exec is gone.\n > \n > Normally commit messages should be written in prose rather than a bullet \n > list and the message should explain the reason for the change.\n > \n > This patch has much less code duplication than the last iteration which \n > is most welcome. Whenever you are moving and refactoring code you should \n > split the move into its own commit followed by the refactoring. That \n > makes it much easier to review as the reviewer can clearly see the \n > refactoring rather than having to manually compare the added code in one \n > file to the deleted code in another.\n \nI apologize for this, and I will add new commits to resolve all issues in the next versions.\n \n > > @@ -84,6 +83,7 @@ static int parse_opt_parse(const struct option *opt, const char *arg,\n > > int unset)\n > > {\n > > struct process_trailer_options *v = opt->value;\n > > +\n > \n > Let's not clutter this patch with unrelated changes.\n > \n > > v->only_trailers = 1;\n > > v->only_input = 1;\n > > v->unfold = 1;\n > > @@ -92,37 +92,6 @@ static int parse_opt_parse(const struct option *opt, const char *arg,\n > > return 0;\n > > }\n > > \n > > -static FILE *create_in_place_tempfile(const char *file)\n > > -{\n > > [...]\n > > -}\n > \n > We don't need to create a temporary file anymore so this can be deleted \n > - 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 > > - LIST_HEAD(head);\n > > - struct strbuf sb = STRBUF_INIT;\n > > - struct strbuf trailer_block_sb = STRBUF_INIT;\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 > > - trailer_block = parse_trailers(opts, sb.buf, &head);\n > > -\n > > - /* Print the lines before the trailer block */\n > > - if (!opts->only_trailers)\n > > - fwrite(sb.buf, 1, trailer_block_start(trailer_block), outfile);\n > > -\n > > - if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))\n > > - fprintf(outfile, \"\\n\");\n > > -\n > > -\n > > - if (!opts->only_input) {\n > > - LIST_HEAD(config_head);\n > > - LIST_HEAD(arg_head);\n > > - parse_trailers_from_config(&config_head);\n > > - parse_trailers_from_command_line_args(&arg_head, new_trailer_head);\n > > - list_splice(&config_head, &arg_head);\n > > - process_trailers_lists(&head, &arg_head);\n > > - }\n > > -\n > > - /* Print trailer block. */\n > > - format_trailers(opts, &head, &trailer_block_sb);\n > > - free_trailers(&head);\n > > - fwrite(trailer_block_sb.buf, 1, trailer_block_sb.len, outfile);\n > > - strbuf_release(&trailer_block_sb);\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 > > - trailer_block_release(trailer_block);\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 > > -}\n > \n > This code is moved to trailer.c which is good but it is heavily \n > refactored at the same time which makes it hard to review. Completely \n > removing this function leads to some duplication in \n > cmd_interpret_trailers() which could be avoided by making \n > interpret_trailers() a wrapper around process_trailers()\n > \n > > int cmd_interpret_trailers(int argc,\n > > const char **argv,\n > > const char *prefix,\n > > @@ -231,14 +145,37 @@ int cmd_interpret_trailers(int argc,\n > > git_interpret_trailers_usage,\n > > options);\n > > \n > > + trailer_config_init();\n > > +\n > > if (argc) {\n > > int i;\n > > - for (i = 0; i < argc; i++)\n > > - interpret_trailers(&opts, &trailers, argv[i]);\n > > + for (i = 0; i < argc; i++) {\n > > + struct strbuf in_buf = STRBUF_INIT;\n > > + struct strbuf out_buf = STRBUF_INIT;\n > > +\n > > + read_input_file(&in_buf, argv[i]);\n > > + if (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)\n > > + die(_(\"failed to process trailers for %s\"), argv[i]);\n > > + if (opts.in_place)\n > > + write_file_buf(argv[i], out_buf.buf, out_buf.len);\n > > + else\n > > + fwrite(out_buf.buf, 1, out_buf.len, stdout);\n > > + strbuf_release(&in_buf);\n > > + strbuf_release(&out_buf);\n > > + }\n > > } else {\n > > + struct strbuf in_buf = STRBUF_INIT;\n > > + struct strbuf out_buf = STRBUF_INIT;\n > > +\n > > if (opts.in_place)\n > > die(_(\"no input file given for in-place editing\"));\n > > - interpret_trailers(&opts, &trailers, NULL);\n > > +\n > > + read_input_file(&in_buf, NULL);\n > > + if (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)\n > > + die(_(\"failed to process trailers\"));\n > > + fwrite(out_buf.buf, 1, out_buf.len, stdout);\n > > + strbuf_release(&in_buf);\n > > + strbuf_release(&out_buf);\n > > }\n > \n > There is quite a bit of duplication here that could be avoided if you \n > modified interpret_trailers() to call trailer_process() rather than \n > deleting it entirely.\n > \n > > new_trailers_clear(&trailers);\n > > diff --git a/trailer.c b/trailer.c\n > > index 310cf582dc..03814443c3 100644\n > > --- a/trailer.c\n > > +++ b/trailer.c\n > > @@ -1224,14 +1224,121 @@ 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 > Function argument declarations should be aligned\n > \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 *arg = trailer_args->v[i];\n > > + const char *text;\n > > + struct new_trailer_item *item;\n > \n > There should be a blank line after the variable declarations at the \n > start of each block of code.\n > \n > > + if (!skip_prefix(arg, \"--trailer=\", &text))\n > \n > Why do we need this? It would be much cleaner if we required the caller \n > to pass a list of trailers without any optional prefix.\n > \n > > + text = arg;\n > > + if (!*text)\n > > + continue;\n > > + item = xcalloc(1, sizeof(*item));\n > > + INIT_LIST_HEAD(&item->list);\n > > + item->text = text;\n > > + list_add_tail(&item->list, &new_trailer_head);\n > > + }\n > > + if (trailer_process(&opts, buf->buf, &new_trailer_head, &out) < 0)\n > > + die(\"failed to process trailers\");\n > \n > As this is library code lets return an error here rather than dying.\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 > > + return 0;\n > > }\n > > +\n > > +int trailer_process(const struct process_trailer_options *opts,\n > > + const char *msg,\n > > + struct list_head *new_trailer_head,\n > > + struct strbuf *out)\n > \n > Argument alignment again\n > \n > > +{\n > > + struct trailer_block *blk;\n > \n > This is trailer_block in the original but has been re-ordered with \n > respect to the other variable declarations making the patch harder to \n > review.\n > \n > > + LIST_HEAD(orig_head);\n > \n > This is head in the original but moved relative to the other variable \n > declarations\n > \n > > + LIST_HEAD(config_head);\n > > + LIST_HEAD(ar1g_head);\n > \n > These two have been moved from inside the if (!opts->only_input) below. \n > They are only referenced there so do not need to be declared here. \n > Moving them makes this patch harder to review.\n > \n > > + struct strbuf trailers_sb = STRBUF_INIT;\n > \n > This is from the original but moved relative to the other variable \n > declarations.\n > \n > > + int had_trailer_before;\n > \n > This is new - lets see how it is used. We've just started using bool for \n > boolean variables in the last few weeks so this could be a bool now.\n > \n > From here to\n > \n > > + blk = parse_trailers(opts, msg, &orig_head);\n > > + had_trailer_before = !list_empty(&orig_head);\n > > + if (!opts->only_input) {\n > > + parse_trailers_from_config(&config_head);\n > > + parse_trailers_from_command_line_args(&arg_head, new_trailer_head);\n > > + list_splice(&config_head, &arg_head);\n > > + process_trailers_lists(&orig_head, &arg_head);\n > > + }\n > > + format_trailers(opts, &orig_head, &trailers_sb);\n > \n > here is copied from the original minus the code that copied the commit \n > message to the output file. Rather than deleting the code that copied \n > the commit message we could have replaced the calls to fwrite() and \n > fprintf() with strbuf_add() and strbuf_addf() which would make it \n > obvious that the behavior is not changed. The original then frees \n > orig_head but that is done later here.\n > \n > > + if (!opts->only_trailers && !opts->only_input && !opts->unfold &&\n > > + !opts->trim_empty && list_empty(&orig_head) &&\n > > + (list_empty(new_trailer_head) || opts->only_input)) {\n > \n > I'm not sure what is happening here. By this point the original has \n > copied the original commit message and is ready to append the new \n > trailers. Instead the new version seems to have completely refactored \n > the logic for adding the new trailers making it harder to see if the \n > behavior has changed.\n > \n > > + size_t split = trailer_block_start(blk); /* end-of-log-msg */\n > > + if (!blank_line_before_trailer_block(blk)) {\n > > + strbuf_add(out, msg, split);\n > > + strbuf_addch(out, '\\n');\n > > + strbuf_addstr(out, msg + split);\n > \n > This copies the original message but adds a newline before the trailer \n > block if it is missing.\n > \n > > + } else\n > > + strbuf_addstr(out, msg);\n > \n > This just copies the whole message.\n > \n > > + strbuf_rel2ease(&trailers_sb);\n > > + trailer_block_release(blk);\n > \n > \n > > + return 0;\n > \n > We return a copy of the original message with no new trailers added. We \n > do not free orig_head, arg_head or config_head. I'm still confused why \n > we need to special case this.\n > \n > \n > > + }\n > > + if (opts->only_trailers) {\n > > + strbuf_addbuf(out, &trailers_sb);\n > \n > This flips the logic in the original to handle opts->only_trailers \n > separately making it harder to review.\n > \n > > + } else if (had_trailer_before) {\n > > + strbuf_add(out, msg, trailer_block_start(blk));\n > > + if (!blank_line_before_trailer_block(blk))\n > > + strbuf_addch(out, '\\n');\n > > + strbuf_addbuf(out, &trailers_sb);\n > > + strbuf_add(out, msg + trailer_block_end(blk),\n > > + strlen(msg) - trailer_block_end(blk));\n > \n > This handles the case where we're replacing the headers in the original \n > message\n > \n > > + }\n > > + else {\n > \n > Style - this should be \"} else {\"\n > \n > > + size_t cpos = trailer_block_start(blk);\n > > + strbuf_add(out, msg, cpos);\n > > + if (cpos == 0) /* empty body → just one \\n */\n > > + strbuf_addch(out, '\\n');\n > > + else if (!blank_line_before_trailer_block(blk))\n > > + strbuf_addch(out, '\\n'); /* body without trailing blank */\n > > +\n > > + strbuf_addbuf(out, &trailers_sb);\n > > + strbuf_add(out, msg + cpos, strlen(msg) - cpos);\n > > + }\n > \n > I'm confused why we need a separate case for when the original did not \n > have any trailers - was the original code broken? If it was we should \n > separate out the bug fix from the refactoring. If not what's the point \n > of this change?\n > \n > > + strbuf_release(&trailers_sb);\n > > + free_trailers(&orig_head);\n > > + trailer_block_release(blk);\n > > + return 0;\n > > +}\n > > +\n > > +int amend_file_with_trailers(const char *path,\n > > + const struct strvec *trailer_args)\n > \n > Alignment again\n > \n > > +{\n > > + struct strbuf buf = STRBUF_INIT;\n > > +\n > > + if (!trailer_args || !trailer_args->nr)\n > > + return 0;\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 > > + die(\"failed to append trailers\");\n > \n > Why return an error() above but die() here? This is library code so lets \n > return an error.\n > \n > > +\n > > + /* `write_file_buf()` aborts on error internally */\n > > + write_file_buf(path, buf.buf, buf.len);\n > \n > Dying here is a change in behavior which callers might not be expecting. \n > The original code always returned a error because it forked a \n > sub-process to do the trailer processing. Ideally, in a separate commit, \n > we'd update any existing callers that have the message in an strbuf so \n > they don't have to write it to a file just to add some trailers to it.\n > \n > > + strbuf_release(&buf);\n > > + return 0;\n > > + }\n > \n > As I said above reusing the existing code as you have done here is a \n > much better approach. However it would be much easier to review if the \n > code movement was separated from the refactoring. I'm also struggling to \n > see the benefit of a lot of the refactoring - I was expecting the \n > conversion to use an strubf would essentially look like fwrite() being \n > replaced with strbuf_add() and fprintf() being replaced with \n > strbuf_addf() etc. rather than reworking the logic.\n > \n > Thanks\n > \n > \n > Phillip\n > \n > > diff --git a/trailer.h b/trailer.h\n > > index 4740549586..01f711fb13 100644\n > > --- a/trailer.h\n > > +++ b/trailer.h\n > > @@ -196,10 +196,22 @@ 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 > > +/*\n > > + * Process trailer lines for a commit message in-memory.\n > > + * @opts: trailer processing options (e.g. from parse-options)\n > > + * @msg: the input message string\n > > + * @new_trailer_head: list of new trailers to add (struct new_trailer_item)\n > > + * @out: strbuf to store the resulting message (must be initialized)\n > > + *\n > > + * Returns 0 on success, <0 on error.\n > > + */\n > > +int trailer_process(const struct process_trailer_options *opts,\n > > + const char *msg,\n > > + struct list_head *new_trailer_head,\n > > + struct strbuf *out);\n > > #endif /* TRAILER_H */\n > \n > \nRegards,\n\nLi​\n\n"},{"id":"525170","messageId":"xmqqiki7qasu.fsf@gitster.g","threadId":"63897","inReplyTo":"198826665b6.317113211709957.1514728503207030488@linux.beauty","subject":"Re: [PATCH v3 2/2] rebase: support --trailer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-28T23:35:45Z","receivedAt":"2025-08-28T23:35:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Li Chen writes:\n\n> Hi Phillip, \n>\n> Thanks for your thorough review; I will address them in the next version.\n\nAs I do not want to keep an inactive topic in 'seen' for more than a\nmonth, I was doing my usual \"sweep\" of the topics, and found this\nexchange. \n\nIs this still being worked on? No rush, but just checking to see\nwhat the status is.\n\nSince the summer is a slow season, I do not mind keeping the topic\nfor a few more weeks in 'seen', but I can simply discard the one I\nhave, and requeue a new version in 'seen' when it materializes.\n\nThanks.\n"},{"id":"526693","messageId":"1995bf77c93.3eeb42b4972717.3783775021840050008@linux.beauty","threadId":"63897","inReplyTo":"xmqqiki7qasu.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] rebase: support --trailer","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-09-18T08:36:10Z","receivedAt":"2025-09-18T08:36:22Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"Hi Junio,\n\nI apologize for the delayed response.\n\n ---- On Fri, 29 Aug 2025 07:35:45 +0800 Junio C Hamano wrote --- \n > Li Chen writes:\n > \n > > Hi Phillip, \n > >\n > > Thanks for your thorough review; I will address them in the next version.\n > \n > As I do not want to keep an inactive topic in 'seen' for more than a\n > month, I was doing my usual \"sweep\" of the topics, and found this\n > exchange. \n > \n > Is this still being worked on? No rush, but just checking to see\n > what the status is.\n > \n > Since the summer is a slow season, I do not mind keeping the topic\n > for a few more weeks in 'seen', but I can simply discard the one I\n > have, and requeue a new version in 'seen' when it materializes.\n\nYes, it's still in progress, though I've had limited time recently. I aim to finish the next\nversion before October 8th, taking advantage of the 7-day Chinese National Day holiday\nto work on it.\n\nRegards,\nLi\n"},{"id":"529248","messageId":"19a0637d7c6.56c0ee933118113.6864199134170276242@linux.beauty","threadId":"63897","inReplyTo":"d4c9f082-52be-48d9-b817-fcb8a72e1bd7@gmail.com","subject":"Re: [PATCH v3 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers`","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-10-21T10:01:54Z","receivedAt":"2025-10-21T10:02:04Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"Hi Phillip,\n\n > > new_trailers_clear(&trailers);\n > > diff --git a/trailer.c b/trailer.c\n > > index 310cf582dc..03814443c3 100644\n > > --- a/trailer.c\n > > +++ b/trailer.c\n > > @@ -1224,14 +1224,121 @@ 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 > Function argument declarations should be aligned\n \nMy editor uses a 4-space tab width, but the project view displays tabs as 8 spaces wide.\nThis discrepancy caused the same errors in v4, which I plan to correct with clang-format in v5.\n\nRegards,\n\nLi​\n\n"}]}