{"thread":{"id":"63405","subject":"[RFC PATCH 0/2] rebase: support --trailer and add --reviewby","startedAt":"2025-05-06T12:57:24Z","lastAt":"2025-05-16T10:04:40Z","messageCount":16,"participants":["Li Chen","Junio C Hamano","Phillip Wood","Kristoffer Haugsbakk","phillip.wood123@gmail.com"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"517334","messageId":"196a5ac1393.f5b4db7d187309.2451613571977217927@linux.beauty","threadId":"63405","inReplyTo":null,"subject":"[RFC PATCH 0/2] rebase: support --trailer and add --reviewby","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-05-06T12:57:20Z","receivedAt":"2025-05-06T12:57:24Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"From: Li Chen \n\nThis patch series enhances `git rebase` and `git am` by introducing three features:\n\n 1. `--trailer ` support on rebase's merge backend\n Allows arbitrary trailer lines (e.g. multiple “Reviewed-by: …”) to be appended\n to each rebased commit. The apply backend (`git am`) is explicitly\n rejected when used with `--trailer`, since it has no message‑filter/trailer support.\n\n This is especially useful when reviewee add reviewer's Reviewed-by, e.g.,\n git rebase \\\n --trailer \"Reviewed-by: Bob \" \\\n --trailer \"Tested-by: Dana \" \\\n base~1\n\n\n 2. `--reviewby` shortcut flag for rebase\n A convenience alias for adding a single “Reviewed‑by:” trailer using your configured\n committer identity (user.name/user.email), analogous to --signoff. It works on both\n rebase backends (merge and apply) and automatically disables fast‑forwarding to\n rewrite commits.\n\n This is especially useful when reviewer adds his/her Reviewed-by for a given patchset\n which is already applied, e.g., git rebase --reviewby base~1\n\n3. BTW, `--reviewby` shortcut flag is also added for `am` cmd, because it's needed\n by rebase's apply backend.\n\nI’ve run make test locally without failures. There are Github CI errors around leak checks that I’m still tracking down (see https://github.com/FirstLoveLife/git/actions/runs/14859027869).\n\nLooking forward to your feedback!\n\nLi Chen (2):\n rebase, am: add --reviewby option\n rebase: support --trailer\n\n Documentation/git-am.adoc | 6 +-\n builtin/am.c | 31 +++++++++\n builtin/rebase.c | 84 ++++++++++++++++++++++\n builtin/revert.c | 4 +-\n sequencer.c | 100 ++++++++++++++++++++++++++-\n sequencer.h | 12 ++++\n t/meson.build | 2 +\n t/t3439-rebase-reviewby.sh | 138 +++++++++++++++++++++++++++++++++++++\n t/t3440-rebase-trailer.sh | 108 +++++++++++++++++++++++++++++\n t/t4150-am.sh | 75 ++++++++++++++++++++\n 10 files changed, 556 insertions(+), 4 deletions(-)\n create mode 100755 t/t3439-rebase-reviewby.sh\n create mode 100755 t/t3440-rebase-trailer.sh\n\n--\n2.49.0\n\n"},{"id":"517335","messageId":"196a5aceb00.fdc2d9ff187843.3547183335386278718@linux.beauty","threadId":"63405","inReplyTo":"196a5ac1393.f5b4db7d187309.2451613571977217927@linux.beauty","subject":"[RFC PATCH 1/2] rebase, am: add --reviewby option","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-05-06T12:58:15Z","receivedAt":"2025-05-06T12:58:19Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"From: Li Chen \n\nIntroduce a new `--reviewby` flag to `git rebase` and `git am` that appends a\n`Reviewed-by: name ` trailer to each rebased or applied commit, analogous\nto the existing `--signoff` behavior.\n\nespecially useful for backporting patches and reviewing new patches.\n\nSigned-off-by: Li Chen \nReviewed-by: Li Chen \n---\n Documentation/git-am.adoc | 6 +-\n Documentation/git-rebase.adoc | 4 +\n builtin/am.c | 31 ++++++++\n builtin/rebase.c | 14 ++++\n builtin/revert.c | 4 +-\n sequencer.c | 87 ++++++++++++++++++++-\n sequencer.h | 10 +++\n t/meson.build | 1 +\n t/t3439-rebase-reviewby.sh | 138 ++++++++++++++++++++++++++++++++++\n t/t4150-am.sh | 75 ++++++++++++++++++\n 10 files changed, 366 insertions(+), 4 deletions(-)\n create mode 100755 t/t3439-rebase-reviewby.sh\n\ndiff --git a/Documentation/git-am.adoc b/Documentation/git-am.adoc\nindex 221070de48..1d408f2bab 100644\n--- a/Documentation/git-am.adoc\n+++ b/Documentation/git-am.adoc\n@@ -9,7 +9,7 @@ git-am - Apply a series of patches from a mailbox\n SYNOPSIS\n --------\n [verse]\n-'git am' [--signoff] [--keep] [--[no-]keep-cr] [--[no-]utf8] [--no-verify]\n+'git am' [--signoff] [--reviewby] [--keep] [--[no-]keep-cr] [--[no-]utf8] [--no-verify]\n \t [--[no-]3way] [--interactive] [--committer-date-is-author-date]\n \t [--ignore-date] [--ignore-space-change | --ignore-whitespace]\n \t [--whitespace=] [-C] [-p] [--directory=]\n@@ -40,7 +40,9 @@ OPTIONS\n \tAdd a `Signed-off-by` trailer to the commit message, using\n \tthe committer identity of yourself.\n \tSee the signoff option in linkgit:git-commit[1] for more information.\n-\n+--reviewby::\n+\tAdd a `Reviewed-by` trailer to the commit message, using\n+\tthe committer identity of yourself.\n -k::\n --keep::\n \tPass `-k` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]).\ndiff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc\nindex 956d3048f5..1f3d152035 100644\n--- a/Documentation/git-rebase.adoc\n+++ b/Documentation/git-rebase.adoc\n@@ -521,6 +521,10 @@ 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+--review-by::\n+\tAppend a `Reviewed-by:` trailer whose value is taken from the\n+\tcurrent committer identity, exactly like `--signoff` appends a\n+\tSigned-off-by:` trailer.\n See also INCOMPATIBLE OPTIONS below.\n \n -i::\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 4afb519830..f8fe95e15d 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -89,6 +89,12 @@ enum signoff_type {\n \tSIGNOFF_EXPLICIT /* --signoff was set on the command-line */\n };\n \n+enum reviewby_type {\n+\tREVIEWBY_FALSE = 0,\n+\tREVIEWBY_TRUE = 1,\n+\tREVIEWBY_EXPLICIT /* --reviewby was set on the command-line */\n+};\n+\n enum resume_type {\n \tRESUME_FALSE = 0,\n \tRESUME_APPLY,\n@@ -134,6 +140,7 @@ struct am_state {\n \tint threeway;\n \tint quiet;\n \tint signoff; /* enum signoff_type */\n+\tint reviewby; /* enum reviewby_type */\n \tint utf8;\n \tint keep; /* enum keep_type */\n \tint message_id;\n@@ -422,6 +429,9 @@ static void am_load(struct am_state *state)\n \tread_state_file(&sb, state, \"sign\", 1);\n \tstate->signoff = !strcmp(sb.buf, \"t\");\n \n+\tread_state_file(&sb, state, \"review\", 1);\n+\tstate->reviewby = !strcmp(sb.buf, \"t\");\n+\n \tread_state_file(&sb, state, \"utf8\", 1);\n \tstate->utf8 = !strcmp(sb.buf, \"t\");\n \n@@ -1017,6 +1027,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \twrite_state_bool(state, \"threeway\", state->threeway);\n \twrite_state_bool(state, \"quiet\", state->quiet);\n \twrite_state_bool(state, \"sign\", state->signoff);\n+\twrite_state_bool(state, \"review\", state->reviewby);\n \twrite_state_bool(state, \"utf8\", state->utf8);\n \n \tif (state->allow_rerere_autoupdate)\n@@ -1191,6 +1202,18 @@ static void am_append_signoff(struct am_state *state)\n \tstate->msg = strbuf_detach(&sb, &state->msg_len);\n }\n \n+/**\n+ * Appends reviewby to the \"msg\" field of the am_state.\n+ */\n+static void am_append_reviewby(struct am_state *state)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tstrbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);\n+\tappend_reviewby(&sb, 0, 0);\n+\tstate->msg = strbuf_detach(&sb, &state->msg_len);\n+}\n+\n /**\n * Parses `mail` using git-mailinfo, extracting its patch and authorship info.\n * state->msg will be set to the patch message. state->author_name,\n@@ -1849,6 +1872,9 @@ static void am_run(struct am_state *state, int resume)\n \t\t\tif (state->signoff)\n \t\t\t\tam_append_signoff(state);\n \n+\t\t\tif (state->reviewby)\n+\t\t\t\tam_append_reviewby(state);\n+\n \t\t\twrite_author_script(state);\n \t\t\twrite_commit_msg(state);\n \t\t}\n@@ -2337,6 +2363,9 @@ int cmd_am(int argc,\n \t\tOPT_SET_INT('s', \"signoff\", &state.signoff,\n \t\t\tN_(\"add a Signed-off-by trailer to the commit message\"),\n \t\t\tSIGNOFF_EXPLICIT),\n+\t\tOPT_SET_INT(0, \"reviewby\", &state.reviewby,\n+\t\t\tN_(\"add a Reviewed-by trailer to the commit message\"),\n+\t\t\tREVIEWBY_EXPLICIT),\n \t\tOPT_BOOL('u', \"utf8\", &state.utf8,\n \t\t\tN_(\"recode into utf8 (default)\")),\n \t\tOPT_SET_INT('k', \"keep\", &state.keep,\n@@ -2483,6 +2512,8 @@ int cmd_am(int argc,\n \n \t\tif (state.signoff == SIGNOFF_EXPLICIT)\n \t\t\tam_append_signoff(&state);\n+\t\tif (state.reviewby == REVIEWBY_EXPLICIT)\n+\t\t\tam_append_reviewby(&state);\n \t} else {\n \t\tstruct strvec paths = STRVEC_INIT;\n \t\tint i;\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 2e8c4ee678..b288aedfb1 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -113,6 +113,7 @@ struct rebase_options {\n \tenum action action;\n \tchar *reflog_action;\n \tint signoff;\n+\tint reviewby;\n \tint allow_rerere_autoupdate;\n \tint keep_empty;\n \tint autosquash;\n@@ -177,6 +178,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \tsequencer_init_config(&replay);\n \n \treplay.signoff = opts->signoff;\n+\treplay.reviewby = opts->reviewby;\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@@ -477,6 +479,10 @@ static int read_basic_state(struct rebase_options *opts)\n \t\topts->signoff = 1;\n \t\topts->flags |= REBASE_FORCE;\n \t}\n+\tif (file_exists(state_dir_path(\"reviewby\", opts))) {\n+\t\topts->reviewby = 1;\n+\t\topts->flags |= REBASE_FORCE;\n+\t}\n \n \tif (file_exists(state_dir_path(\"allow_rerere_autoupdate\", opts))) {\n \t\tstrbuf_reset(&buf);\n@@ -528,6 +534,8 @@ static int rebase_write_basic_state(struct rebase_options *opts)\n \t\t\t opts->gpg_sign_opt);\n \tif (opts->signoff)\n \t\twrite_file(state_dir_path(\"signoff\", opts), \"--signoff\");\n+\tif (opts->reviewby)\n+\t\twrite_file(state_dir_path(\"reviewby\", opts), \"--reviewby\");\n \n \treturn 0;\n }\n@@ -1134,6 +1142,8 @@ int cmd_rebase(int argc,\n \t\t},\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, \"reviewby\", &options.reviewby,\n+\t\t\t N_(\"add a Reviewed-by trailer to each commit\")),\n \t\tOPT_BOOL(0, \"committer-date-is-author-date\",\n \t\t\t &options.committer_date_is_author_date,\n \t\t\t N_(\"make committer date match author date\")),\n@@ -1618,6 +1628,10 @@ int cmd_rebase(int argc,\n \t\tstrvec_push(&options.git_am_opts, \"--signoff\");\n \t\toptions.flags |= REBASE_FORCE;\n \t}\n+\tif (options.reviewby) {\n+\t\tstrvec_push(&options.git_am_opts, \"--reviewby\");\n+\t\toptions.flags |= REBASE_FORCE;\n+\t}\n \n \tif (!options.root) {\n \t\tif (argc < 1) {\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex e07c2217fe..c73a773c9d 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -124,7 +124,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t\tOPT_CLEANUP(&cleanup_arg),\n \t\tOPT_BOOL('n', \"no-commit\", &opts->no_commit, N_(\"don't automatically commit\")),\n \t\tOPT_BOOL('e', \"edit\", &opts->edit, N_(\"edit the commit message\")),\n-\t\tOPT_NOOP_NOARG('r', NULL),\n+\t\tOPT_BOOL('r', \"reviewby\", &opts->reviewby, N_(\"add a Reviewed-by trailer\")),\n \t\tOPT_BOOL('s', \"signoff\", &opts->signoff, N_(\"add a Signed-off-by trailer\")),\n \t\tOPT_CALLBACK('m', \"mainline\", opts, N_(\"parent-number\"),\n \t\t\t N_(\"select mainline parent\"), option_parse_m),\n@@ -206,6 +206,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t\tverify_opt_compatible(me, this_operation,\n \t\t\t\t\"--no-commit\", opts->no_commit,\n \t\t\t\t\"--signoff\", opts->signoff,\n+\t\t\t\t\"--reviewby\", opts->reviewby,\n \t\t\t\t\"--mainline\", opts->mainline,\n \t\t\t\t\"--strategy\", opts->strategy ? 1 : 0,\n \t\t\t\t\"--strategy-option\", opts->xopts.nr ? 1 : 0,\n@@ -226,6 +227,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \tif (opts->allow_ff)\n \t\tverify_opt_compatible(me, \"--ff\",\n \t\t\t\t\"--signoff\", opts->signoff,\n+\t\t\t\t\"--reviewby\", opts->reviewby,\n \t\t\t\t\"--no-commit\", opts->no_commit,\n \t\t\t\t\"-x\", opts->record_origin,\n \t\t\t\t\"--edit\", opts->edit > 0,\ndiff --git a/sequencer.c b/sequencer.c\nindex b5c4043757..01933882ed 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -61,6 +61,7 @@\n #define GIT_MAX_LABEL_LENGTH ((NAME_MAX) - (LOCK_SUFFIX_LEN) - 16)\n \n static const char sign_off_header[] = \"Signed-off-by: \";\n+static const char review_by_header[] = \"Reviewed-by: \";\n static const char cherry_picked_prefix[] = \"(cherry picked from commit \";\n \n GIT_PATH_FUNC(git_path_commit_editmsg, \"COMMIT_EDITMSG\")\n@@ -199,6 +200,7 @@ static GIT_PATH_FUNC(rebase_path_orig_head, \"rebase-merge/orig-head\")\n static GIT_PATH_FUNC(rebase_path_verbose, \"rebase-merge/verbose\")\n static GIT_PATH_FUNC(rebase_path_quiet, \"rebase-merge/quiet\")\n static GIT_PATH_FUNC(rebase_path_signoff, \"rebase-merge/signoff\")\n+static GIT_PATH_FUNC(rebase_path_reviewby, \"rebase-merge/reviewby\")\n static GIT_PATH_FUNC(rebase_path_head_name, \"rebase-merge/head-name\")\n static GIT_PATH_FUNC(rebase_path_onto, \"rebase-merge/onto\")\n static GIT_PATH_FUNC(rebase_path_autostash, \"rebase-merge/autostash\")\n@@ -1179,6 +1181,10 @@ static int run_git_commit(const char *defmsg,\n \t\t !opts->signoff && !opts->record_origin &&\n \t\t !opts->explicit_cleanup)\n \t\tstrvec_push(&cmd.args, \"--cleanup=verbatim\");\n+\telse if (!(flags & CLEANUP_MSG) &&\n+\t\t !opts->reviewby && !opts->record_origin &&\n+\t\t !opts->explicit_cleanup)\n+\t\tstrvec_push(&cmd.args, \"--cleanup=verbatim\");\n \n \tif ((flags & ALLOW_EMPTY))\n \t\tstrvec_push(&cmd.args, \"--allow-empty\");\n@@ -1628,6 +1634,9 @@ static int try_to_commit(struct repository *r,\n \telse if ((opts->signoff || opts->record_origin) &&\n \t\t !opts->explicit_cleanup)\n \t\tcleanup = COMMIT_MSG_CLEANUP_SPACE;\n+\telse if ((opts->reviewby || opts->record_origin) &&\n+\t\t !opts->explicit_cleanup)\n+\t\tcleanup = COMMIT_MSG_CLEANUP_SPACE;\n \telse\n \t\tcleanup = opts->default_msg_cleanup;\n \n@@ -2039,6 +2048,8 @@ static int append_squash_message(struct strbuf *buf, const char *body,\n \t\t */\n \t\tif (opts->signoff)\n \t\t\tappend_signoff(buf, 0, 0);\n+\t\tif (opts->reviewby)\n+\t\t\tappend_reviewby(buf, 0, 0);\n \n \t\tif ((command == TODO_FIXUP) &&\n \t\t (flag & TODO_REPLACE_FIXUP_MSG) &&\n@@ -2418,6 +2429,9 @@ static int do_pick_commit(struct repository *r,\n \tif (opts->signoff && !is_fixup(command))\n \t\tappend_signoff(&ctx->message, 0, 0);\n \n+\tif (opts->reviewby && !is_fixup(command))\n+\t\tappend_reviewby(&ctx->message, 0, 0);\n+\n \tif (is_rebase_i(opts) && write_author_script(msg.message) < 0)\n \t\tres = -1;\n \telse if (!opts->strategy ||\n@@ -3079,6 +3093,8 @@ static int populate_opts_cb(const char *key, const char *value,\n \t\t\tgit_config_bool_or_int(key, value, ctx->kvi, &error_flag);\n \telse if (!strcmp(key, \"options.signoff\"))\n \t\topts->signoff = git_config_bool_or_int(key, value, ctx->kvi, &error_flag);\n+\telse if (!strcmp(key, \"options.reviewby\"))\n+\t\topts->reviewby = git_config_bool_or_int(key, value, ctx->kvi, &error_flag);\n \telse if (!strcmp(key, \"options.record-origin\"))\n \t\topts->record_origin = git_config_bool_or_int(key, value, ctx->kvi, &error_flag);\n \telse if (!strcmp(key, \"options.allow-ff\"))\n@@ -3180,6 +3196,10 @@ static int read_populate_opts(struct replay_opts *opts)\n \t\t\topts->allow_ff = 0;\n \t\t\topts->signoff = 1;\n \t\t}\n+\t\tif (file_exists(rebase_path_reviewby())) {\n+\t\t\topts->allow_ff = 0;\n+\t\t\topts->reviewby = 1;\n+\t\t}\n \n \t\tif (file_exists(rebase_path_cdate_is_adate())) {\n \t\t\topts->allow_ff = 0;\n@@ -3286,6 +3306,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\twrite_file(rebase_path_gpg_sign_opt(), \"-S%s\\n\", opts->gpg_sign);\n \tif (opts->signoff)\n \t\twrite_file(rebase_path_signoff(), \"--signoff\\n\");\n+\tif (opts->reviewby)\n+\t\twrite_file(rebase_path_reviewby(), \"--reviewby\\n\");\n \tif (opts->drop_redundant_commits)\n \t\twrite_file(rebase_path_drop_redundant_commits(), \"%s\", \"\");\n \tif (opts->keep_redundant_commits)\n@@ -3629,6 +3651,9 @@ static int save_opts(struct replay_opts *opts)\n \tif (opts->signoff)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\t\"options.signoff\", NULL, \"true\");\n+\tif (opts->reviewby)\n+\t\tres |= git_config_set_in_file_gently(opts_file,\n+\t\t\t\t\t\"options.reviewby\", NULL, \"true\");\n \tif (opts->record_origin)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\t\"options.record-origin\", NULL, \"true\");\n@@ -4949,7 +4974,7 @@ static int pick_commits(struct repository *r,\n \n \tctx->reflog_message = sequencer_reflog_action(opts);\n \tif (opts->allow_ff)\n-\t\tASSERT(!(opts->signoff || opts->no_commit ||\n+\t\tASSERT(!(opts->signoff || opts->reviewby || opts->no_commit ||\n \t\t\t opts->record_origin || should_edit(opts) ||\n \t\t\t opts->committer_date_is_author_date ||\n \t\t\t opts->ignore_date));\n@@ -5581,6 +5606,66 @@ int sequencer_pick_revisions(struct repository *r,\n \treturn res;\n }\n \n+void append_reviewby(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)\n+{\n+\tunsigned no_dup_rb = flag & APPEND_REVIEWBY_DEDUP;\n+\tstruct strbuf rb = STRBUF_INIT;\n+\tint has_footer;\n+\n+\tstrbuf_addstr(&rb, review_by_header);\n+\tstrbuf_addstr(&rb, fmt_name(WANT_COMMITTER_IDENT));\n+\tstrbuf_addch(&rb, '\\n');\n+\n+\tif (!ignore_footer)\n+\t\tstrbuf_complete_line(msgbuf);\n+\n+\t/*\n+\t * If the whole message buffer is equal to the rb, pretend that we\n+\t * found a conforming footer with a matching rb\n+\t */\n+\tif (msgbuf->len - ignore_footer == rb.len &&\n+\t !strncmp(msgbuf->buf, rb.buf, rb.len))\n+\t\thas_footer = 3;\n+\telse\n+\t\thas_footer = has_conforming_footer(msgbuf, &rb, ignore_footer);\n+\n+\tif (!has_footer) {\n+\t\tconst char *append_newlines = NULL;\n+\t\tsize_t len = msgbuf->len - ignore_footer;\n+\n+\t\tif (!len) {\n+\t\t\t/*\n+\t\t\t * The buffer is completely empty. Leave foom for\n+\t\t\t * the title and body to be filled in by the user.\n+\t\t\t */\n+\t\t\tappend_newlines = \"\\n\\n\";\n+\t\t} else if (len == 1) {\n+\t\t\t/*\n+\t\t\t * Buffer contains a single newline. Add another\n+\t\t\t * so that we leave room for the title and body.\n+\t\t\t */\n+\t\t\tappend_newlines = \"\\n\";\n+\t\t} else if (msgbuf->buf[len - 2] != '\\n') {\n+\t\t\t/*\n+\t\t\t * Buffer ends with a single newline. Add another\n+\t\t\t * so that there is an empty line between the message\n+\t\t\t * body and the rb.\n+\t\t\t */\n+\t\t\tappend_newlines = \"\\n\";\n+\t\t} /* else, the buffer already ends with two newlines. */\n+\n+\t\tif (append_newlines)\n+\t\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\n+\t\t\t\tappend_newlines, strlen(append_newlines));\n+\t}\n+\n+\tif (has_footer != 3 && (!no_dup_rb || has_footer != 2))\n+\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\n+\t\t\t\trb.buf, rb.len);\n+\n+\tstrbuf_release(&rb);\n+}\n+\n void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)\n {\n \tunsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;\ndiff --git a/sequencer.h b/sequencer.h\nindex 304ba4b4d3..82b79fd1e8 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -17,6 +17,7 @@ const char *rebase_path_dropped(void);\n extern const char *rebase_resolvemsg;\n \n #define APPEND_SIGNOFF_DEDUP (1u << 0)\n+#define APPEND_REVIEWBY_DEDUP (1u << 1)\n \n enum replay_action {\n \tREPLAY_REVERT,\n@@ -44,6 +45,7 @@ struct replay_opts {\n \tint record_origin;\n \tint no_commit;\n \tint signoff;\n+\tint reviewby;\n \tint allow_ff;\n \tint allow_rerere_auto;\n \tint allow_empty;\n@@ -205,6 +207,14 @@ int todo_list_rearrange_squash(struct todo_list *todo_list);\n */\n void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag);\n \n+/*\n+ * Append a reviewby to the commit message in \"msgbuf\". The ignore_footer\n+ * parameter specifies the number of bytes at the end of msgbuf that should\n+ * not be considered at all. I.e., they are not checked for existing trailers,\n+ * and the new reviewby will be spliced into the buffer before those bytes.\n+ */\n+void append_reviewby(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag);\n+\n void append_conflicts_hint(struct index_state *istate,\n \t\tstruct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode);\n enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,\ndiff --git a/t/meson.build b/t/meson.build\nindex bfb744e886..327fa461fd 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -392,6 +392,7 @@ integration_tests = [\n 't3436-rebase-more-options.sh',\n 't3437-rebase-fixup-options.sh',\n 't3438-rebase-broken-files.sh',\n+ 't3439-rebase-reviewby.sh',\n 't3500-cherry.sh',\n 't3501-revert-cherry-pick.sh',\n 't3502-cherry-pick-merge.sh',\ndiff --git a/t/t3439-rebase-reviewby.sh b/t/t3439-rebase-reviewby.sh\nnew file mode 100755\nindex 0000000000..ca7299fbdb\n--- /dev/null\n+++ b/t/t3439-rebase-reviewby.sh\n@@ -0,0 +1,138 @@\n+#!/bin/sh\n+\n+test_description='git rebase --reviewby\n+\n+This test runs git rebase --reviewby and make sure that it works.\n+'\n+\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-rebase.sh\n+\n+test_expect_success 'setup' '\n+\tgit commit --allow-empty -m \"Initial empty commit\" &&\n+\ttest_commit first file a &&\n+\ttest_commit second file &&\n+\tgit checkout -b conflict-branch first &&\n+\ttest_commit file-2 file-2 &&\n+\ttest_commit conflict file &&\n+\ttest_commit third file &&\n+\n+\tident=\"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" &&\n+\n+\t# Expected commit message for initial commit after rebase --reviewby\n+\tcat >expected-initial-reviewed <<-EOF &&\n+\tInitial empty commit\n+\n+\tReviewed-by: $ident\n+\tEOF\n+\n+\t# Expected commit message after rebase --reviewby\n+\tcat >expected-reviewed <<-EOF &&\n+\tfirst\n+\n+\tReviewed-by: $ident\n+\tEOF\n+\n+\t# Expected commit message after conflict resolution for rebase --reviewby\n+\tcat >expected-reviewed-conflict <<-EOF &&\n+\tthird\n+\n+\tReviewed-by: $ident\n+\n+\tconflict\n+\n+\tReviewed-by: $ident\n+\n+\tfile-2\n+\n+\tReviewed-by: $ident\n+\n+\tEOF\n+\n+\t# Expected commit message after rebase without --reviewby (or with --no-reviewby)\n+\tcat >expected-unreviewed <<-EOF &&\n+\tfirst\n+\tEOF\n+\n+\tgit config alias.rbr \"rebase --reviewby\"\n+'\n+\n+test_expect_success 'rebase --apply --reviewby adds a Reviewed-by line' '\n+\ttest_must_fail git rbr --apply second third &&\n+\tgit checkout --theirs file &&\n+\tgit add file &&\n+\tgit rebase --continue &&\n+\tgit log --format=%B -n3 >actual &&\n+\ttest_cmp expected-reviewed-conflict actual\n+'\n+\n+test_expect_success 'rebase --no-reviewby does not add a Reviewed-by line' '\n+\tgit commit --amend -m \"first\" &&\n+\tgit rbr --no-reviewby HEAD^ &&\n+\ttest_commit_message HEAD expected-unreviewed\n+'\n+\n+test_expect_success 'rebase --exec --reviewby adds a Reviewed-by line' '\n+\ttest_when_finished \"rm exec\" &&\n+\tgit rebase --exec \"touch exec\" --reviewby first^ first &&\n+\ttest_path_is_file exec &&\n+\ttest_commit_message HEAD expected-reviewed\n+'\n+\n+test_expect_success 'rebase --root --reviewby adds a Reviewed-by line' '\n+\tgit checkout first &&\n+\tgit rebase --root --keep-empty --reviewby &&\n+\ttest_commit_message HEAD^ expected-initial-reviewed &&\n+\ttest_commit_message HEAD expected-reviewed\n+'\n+\n+test_expect_success 'rebase -m --reviewby adds a Reviewed-by line' '\n+\ttest_must_fail git rebase -m --reviewby second third &&\n+\tgit checkout --theirs file &&\n+\tgit add file &&\n+\tGIT_EDITOR=\"sed -n /Conflicts:/,/^\\\\\\$/p >actual\" \\\n+\t\tgit rebase --continue &&\n+\tcat >expect <<-\\EOF &&\n+\t# Conflicts:\n+\t#\tfile\n+\n+\tEOF\n+\ttest_cmp expect actual &&\n+\tgit log --format=%B -n3 >actual &&\n+\ttest_cmp expected-reviewed-conflict actual\n+'\n+\n+test_expect_success 'rebase -i --reviewby adds a Reviewed-by line when editing commit' '\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_LINES=\"edit 1 edit 3 edit 2\" \\\n+\t\t\tgit rebase -i --reviewby first third\n+\t) &&\n+\techo a >a &&\n+\tgit add a &&\n+\ttest_must_fail git rebase --continue &&\n+\tgit checkout --ours file &&\n+\techo b >a &&\n+\tgit add a file &&\n+\tgit rebase --continue &&\n+\techo c >a &&\n+\tgit add a &&\n+\tgit log --format=%B -n3 >actual &&\n+\tcat >expect <<-EOF &&\n+\tconflict\n+\n+\tReviewed-by: $ident\n+\n+\tthird\n+\n+\tReviewed-by: $ident\n+\n+\tfile-2\n+\n+\tReviewed-by: $ident\n+\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_done\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex 2ae93d3c96..40d6a2f43a 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -491,6 +491,7 @@ test_expect_success 'am changes committer and keeps author' '\n \t \"$(git log -1 --pretty=format:\"%cn <%ce>\" HEAD)\"\n '\n \n+########################## signoff begin ##########################\n test_expect_success 'am --signoff adds Signed-off-by: line' '\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n@@ -572,6 +573,80 @@ test_expect_success 'am without --keep removes Re: and [PATCH] stuff' '\n \tgit rev-parse topic_2 >actual &&\n \ttest_cmp expected actual\n '\n+########################## signoff end ##########################\n+\n+########################## review begin ##########################\n+test_expect_success 'am --review adds Reviewed-by: line' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout -b topic_3 first &&\n+\tgit am --reviewby \\n\\n\" \\\n+\t\t\t\"$GIT_COMMITTER_NAME\" \"$GIT_COMMITTER_EMAIL\" &&\n+\t\tcat msg &&\n+\t\tprintf \"Reviewed-by: %s <%s>\\n\\n\" \\\n+\t\t\t\"$GIT_COMMITTER_NAME\" \"$GIT_COMMITTER_EMAIL\"\n+\t} >expected-log &&\n+\tgit log --pretty=%B -2 HEAD >actual &&\n+\ttest_cmp expected-log actual\n+'\n+test_expect_success 'am stays in branch' '\n+\techo refs/heads/topic_3 >expected &&\n+\tgit symbolic-ref HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'am --reviewby does not add Reviewed-by: line if already there' '\n+\tgit format-patch --stdout first >patch3 &&\n+\tgit reset --hard first &&\n+\tgit am --reviewby actual &&\n+\ttest_cmp expected-log actual\n+'\n+\n+test_expect_success 'am --reviewby adds Reviewed-by: if another author is preset' '\n+\tNAME=\"A N Other\" &&\n+\tEMAIL=\"a.n.other@example.com\" &&\n+\t{\n+\t\tprintf \"third\\n\\nReviewed-by: %s <%s>\\nReviewed-by: %s <%s>\\n\\n\" \\\n+\t\t\t\"$GIT_COMMITTER_NAME\" \"$GIT_COMMITTER_EMAIL\" \\\n+\t\t\t\"$NAME\" \"$EMAIL\" &&\n+\t\tcat msg &&\n+\t\tprintf \"Reviewed-by: %s <%s>\\nReviewed-by: %s <%s>\\n\\n\" \\\n+\t\t\t\"$GIT_COMMITTER_NAME\" \"$GIT_COMMITTER_EMAIL\" \\\n+\t\t\t\"$NAME\" \"$EMAIL\"\n+\t} >expected-log &&\n+\tgit reset --hard first &&\n+\tGIT_COMMITTER_NAME=\"$NAME\" GIT_COMMITTER_EMAIL=\"$EMAIL\" \\\n+\t\tgit am --reviewby actual &&\n+\ttest_cmp expected-log actual\n+'\n+\n+test_expect_success 'am --reviewby duplicates Reviewed-by: if it is not the last one' '\n+\tNAME=\"A N Other\" &&\n+\tEMAIL=\"a.n.other@example.com\" &&\n+\t{\n+\t\tprintf \"third\\n\\nReviewed-by: %s <%s>\\n\\\n+Reviewed-by: %s <%s>\\nReviewed-by: %s <%s>\\n\\n\" \\\n+\t\t\t\"$GIT_COMMITTER_NAME\" \"$GIT_COMMITTER_EMAIL\" \\\n+\t\t\t\"$NAME\" \"$EMAIL\" \\\n+\t\t\t\"$GIT_COMMITTER_NAME\" \"$GIT_COMMITTER_EMAIL\" &&\n+\t\tcat msg &&\n+\t\tprintf \"Reviewed-by: %s <%s>\\nReviewed-by: %s <%s>\\n\\\n+Reviewed-by: %s <%s>\\n\\n\" \\\n+\t\t\t\"$GIT_COMMITTER_NAME\" \"$GIT_COMMITTER_EMAIL\" \\\n+\t\t\t\"$NAME\" \"$EMAIL\" \\\n+\t\t\t\"$GIT_COMMITTER_NAME\" \"$GIT_COMMITTER_EMAIL\"\n+\t} >expected-log &&\n+\tgit format-patch --stdout first >patch3 &&\n+\tgit reset --hard first &&\n+\tgit am --reviewby actual &&\n+\ttest_cmp expected-log actual\n+'\n+########################## review end ##########################\n \n test_expect_success 'am --keep really keeps the subject' '\n \trm -fr .git/rebase-apply &&\n-- \n2.49.0\n\n\n"},{"id":"517336","messageId":"196a5ad7fff.c587c495188189.5980390754046003962@linux.beauty","threadId":"63405","inReplyTo":"196a5ac1393.f5b4db7d187309.2451613571977217927@linux.beauty","subject":"[RFC PATCH 2/2] rebase: support --trailer","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-05-06T12:58:53Z","receivedAt":"2025-05-06T12:58:59Z","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\nReviewed-by: Li Chen \n---\n Documentation/git-rebase.adoc | 8 +++\n builtin/rebase.c | 70 ++++++++++++++++++++++\n sequencer.c | 13 ++++\n sequencer.h | 2 +\n t/meson.build | 1 +\n t/t3440-rebase-trailer.sh | 108 ++++++++++++++++++++++++++++++++++\n 6 files changed, 202 insertions(+)\n create mode 100755 t/t3440-rebase-trailer.sh\n\ndiff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc\nindex 1f3d152035..757016e529 100644\n--- a/Documentation/git-rebase.adoc\n+++ b/Documentation/git-rebase.adoc\n@@ -525,6 +525,14 @@ See also INCOMPATIBLE OPTIONS below.\n \tAppend a `Reviewed-by:` trailer whose value is taken from the\n \tcurrent committer identity, exactly like `--signoff` appends a\n \tSigned-off-by:` trailer.\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 b288aedfb1..df65a1e040 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -36,6 +36,9 @@\n #include \"reset.h\"\n #include \"trace2.h\"\n #include \"hook.h\"\n+#include \"trailer.h\"\n+\n+static const char trailer_state_name[] = \"trailer\";\n \n static char const * const builtin_rebase_usage[] = {\n \tN_(\"git rebase [-i] [options] [--exec ] \"\n@@ -46,6 +49,8 @@ static char const * const builtin_rebase_usage[] = {\n \tNULL\n };\n \n+static struct strvec trailer_args = STRVEC_INIT;\n+\n static GIT_PATH_FUNC(path_squash_onto, \"rebase-merge/squash-onto\")\n static GIT_PATH_FUNC(path_interactive, \"rebase-merge/interactive\")\n static GIT_PATH_FUNC(apply_dir, \"rebase-apply\")\n@@ -114,6 +119,7 @@ struct rebase_options {\n \tchar *reflog_action;\n \tint signoff;\n \tint reviewby;\n+\tstruct strvec trailer_args;\n \tint allow_rerere_autoupdate;\n \tint keep_empty;\n \tint autosquash;\n@@ -144,6 +150,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@@ -167,6 +174,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@@ -179,6 +187,10 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \n \treplay.signoff = opts->signoff;\n \treplay.reviewby = opts->reviewby;\n+\n+\tfor (size_t i = 0; i < opts->trailer_args.nr; i++)\n+ strvec_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@@ -437,6 +449,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+\tstruct strbuf t = STRBUF_INIT, one = STRBUF_INIT;\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@@ -509,6 +523,22 @@ static int read_basic_state(struct rebase_options *opts)\n \n \tstrbuf_release(&buf);\n \n+\tif (strbuf_read_file(&t, path, 0) >= 0) {\n+\t\tconst char *p = t.buf, *end = t.buf + t.len;\n+\n+\t\twhile (p < end) {\n+\t\t\tconst char *nl = memchr(p, '\\n', end - p);\n+\t\t\tstrbuf_reset(&one);\n+\t\t\tstrbuf_add(&one, p, nl ? nl - p : end - p);\n+\t\t\tif (one.len) /* skip empty line */\n+\t\t\t\tstrvec_push(&opts->trailer_args,\n+\t\t\t\t\t strbuf_detach(&one, NULL));\n+\t\t\tp = nl ? nl + 1 : end;\n+\t\t}\n+\t\tstrbuf_release(&one);\n+\t}\n+\tstrbuf_release(&t);\n+\n \treturn 0;\n }\n \n@@ -537,6 +567,28 @@ static int rebase_write_basic_state(struct rebase_options *opts)\n \tif (opts->reviewby)\n \t\twrite_file(state_dir_path(\"reviewby\", opts), \"--reviewby\");\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+ } else {\n+ /*\n+ * but if rebase doesn't pass any --trailer，\n+ * and state dir still have residual files，let's delete it。\n+ */\n+ unlink_or_warn(state_dir_path(trailer_state_name, opts));\n+ }\n+\n \treturn 0;\n }\n \n@@ -1140,6 +1192,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_PASSTHRU_ARGV(0, \"trailer\", &trailer_args, N_(\"trailer\"), N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG),\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, \"reviewby\", &options.reviewby,\n@@ -1292,6 +1345,13 @@ int cmd_rebase(int argc,\n \t\t\t builtin_rebase_options,\n \t\t\t builtin_rebase_usage, 0);\n \n+ for (i = 0; i < trailer_args.nr; i++)\n+ \t strvec_push(&options.trailer_args, trailer_args.v[i]);\n+\n+ /* if add --trailer，force rebase */\n+ if (options.trailer_args.nr)\n+\t\t options.flags |= REBASE_FORCE;\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@@ -1549,6 +1609,16 @@ 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.type == REBASE_APPLY && options.trailer_args.nr)\n+\t\tdie(_(\"the --apply backend (git am) cannot currently handle \"\n+\t\t \"--trailer; please omit --apply or use \"\n+\t\t \"the merge/interactive backend\"));\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 01933882ed..b61c668c39 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -429,6 +429,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@@ -2506,6 +2507,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, opts, flags,\ndiff --git a/sequencer.h b/sequencer.h\nindex 82b79fd1e8..4f5ea2d818 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -46,6 +46,7 @@ struct replay_opts {\n \tint no_commit;\n \tint signoff;\n \tint reviewby;\n+\tstruct strvec trailer_args;\n \tint allow_ff;\n \tint allow_rerere_auto;\n \tint allow_empty;\n@@ -88,6 +89,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 327fa461fd..fc4b64fca1 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -393,6 +393,7 @@ integration_tests = [\n 't3437-rebase-fixup-options.sh',\n 't3438-rebase-broken-files.sh',\n 't3439-rebase-reviewby.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..6dc08e9633\n--- /dev/null\n+++ b/t/t3440-rebase-trailer.sh\n@@ -0,0 +1,108 @@\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+############################################################################\n+# 1. repository setup\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+'\n+\n+create_expect () {\n+\tcat >\"$1\" <<-EOF\n+\t$2\n+\n+\tReviewed-by: Dev \n+\tEOF\n+}\n+# golden files:\n+create_expect initial-signed \"Initial empty commit\"\n+create_expect first-signed \"first\"\n+create_expect second-signed \"second\"\n+create_expect file2-signed \"file-2\"\n+create_expect third-signed \"third\"\n+create_expect conflict-signed \"conflict\"\n+\n+############################################################################\n+# 2. explicitly reject --apply + --trailer\n+############################################################################\n+\n+test_expect_success 'apply backend is rejected when --trailer is given' '\n+\tgit reset --hard third &&\n+\tgit tag before-apply &&\n+\ttest_expect_code 128 \\\n+\t\tgit rebase --apply --trailer \"Reviewed-by: Dev \" \\\n+\t\t\tHEAD^ &&\n+\tgit diff --quiet before-apply..HEAD # prove nothing moved\n+'\n+\n+############################################################################\n+# 3. --no‑op: same range, no changes\n+############################################################################\n+\n+test_expect_success '--trailer without range change is a no‑op' '\n+\tgit checkout main &&\n+\tgit tag before-noop &&\n+\tgit rebase --trailer \"Reviewed-by: Dev \" HEAD &&\n+\tgit diff --quiet before-noop..HEAD\n+'\n+\n+############################################################################\n+# 4. merge backend (-m), conflict resolution path\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_EDITOR=: git rebase --continue &&\n+\tgit show --no-patch --pretty=format:%B HEAD~2 >actual &&\n+\ttest_cmp file2-signed actual\n+'\n+\n+############################################################################\n+# 5. --exec path\n+############################################################################\n+\n+test_expect_success 'rebase --exec --trailer adds trailer' '\n+\ttest_when_finished \"rm -f touched\" &&\n+\tgit rebase --exec \"touch touched\" \\\n+\t\t--trailer \"Reviewed-by: Dev \" \\\n+\t\tfirst^ first &&\n+\ttest_path_is_file touched &&\n+\ttest_commit_message HEAD first-signed\n+'\n+\n+############################################################################\n+# 6. --root path\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.49.0\n\n\n"},{"id":"517375","messageId":"xmqqv7qdk3yl.fsf@gitster.g","threadId":"63405","inReplyTo":"196a5aceb00.fdc2d9ff187843.3547183335386278718@linux.beauty","subject":"Re: [RFC PATCH 1/2] rebase, am: add --reviewby option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-06T18:07:46Z","receivedAt":"2025-05-06T18:07:50Z","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> Introduce a new `--reviewby` flag to `git rebase` and `git am` that appends a\n\nShouldn't this (and possibly the other one, I didn't look at the\npatch text) be using the internal machinery used by interpret-trailers\nso that we can do the usual \"do not duplicated existing ones\",\n\"append only at the last one is different\" etc.?\n\nI also do not think we want to see one option (like the above) for\neach trailer elements (like \"Reviewed-by\") that is commonly used,\nwhich would lead us to adding \"--helped-by\", \"--acked-by\", etc. to\ncomplement this one.\n"},{"id":"517444","messageId":"196a97f45e6.ee3375ac536926.7531113088063277926@linux.beauty","threadId":"63405","inReplyTo":"xmqqv7qdk3yl.fsf@gitster.g","subject":"Re: [RFC PATCH 1/2] rebase, am: add --reviewby option","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-05-07T06:46:52Z","receivedAt":"2025-05-07T06:46:59Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"Hi Junio,\n\nThank you for the feedback. My current understanding is as below, please correct me if I am mistaken\n\n---- On Wed, 07 May 2025 02:07:46 +0800 Junio C Hamano wrote --- \n > Li Chen writes:\n > \n > > From: Li Chen \n > >\n > > Introduce a new `--reviewby` flag to `git rebase` and `git am` that appends a\n > \n > Shouldn't this (and possibly the other one, I didn't look at the\n > patch text) be using the internal machinery used by interpret-trailers\n > so that we can do the usual \"do not duplicated existing ones\",\n > \"append only at the last one is different\" etc.?\n \nAt the moment, git-interpret‑trailers only works on a file or on\nstdin. During an interactive/merge rebase we change it on the fly\nwithout format-patch && am, which is very convenient.\n\nThe new --trailer option already re‑uses amend_file_with_trailers()\nfrom trailer.c, so there is very little trailer‑specific code of my own.\n\nRight now --reviewby mirrors the existing --signoff implementation,\nand append_signoff() itself does not use the trailer library. If you are\nopen to keeping a dedicated --reviewby, I can send a follow‑up\n(or roll it into this series) that moves both sign‑off and review‑by to\nthe common trailer helpers.\n\n > I also do not think we want to see one option (like the above) for\n > each trailer elements (like \"Reviewed-by\") that is commonly used,\n > which would lead us to adding \"--helped-by\", \"--acked-by\", etc. to\n > complement this one.\n > \n\nSome projects require every commit to carry a Reviewed-by: line\nfor accountability, much like the kernel requires Signed-off-by:.\nA first‑class option keeps that workflow “out of the box”; otherwise\npeople need to define an alias such as\n\n[alias]\n rbr = rebase --trailer \"Reviewed-by: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>\"\n\nwhich is functional but less convenient.\n\nI would appreciate your further thoughts on whether a dedicated\nflag(--reviewby) is acceptable, or whether we should drop it and rely solely on\nthe generic --trailer interface.\n\nThanks again for the review.\n\nRegards,\nLi\n"},{"id":"517471","messageId":"48b86c85-bc39-40ba-a2b3-67de707dd798@crinan.ddns.net","threadId":"63405","inReplyTo":"196a97f45e6.ee3375ac536926.7531113088063277926@linux.beauty","subject":"Re: [RFC PATCH 1/2] rebase, am: add --reviewby option","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-07T10:17:33Z","receivedAt":"2025-05-07T10:17:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Li\n\nOn 07/05/2025 07:46, Li Chen wrote:\n> \n> Some projects require every commit to carry a Reviewed-by: line\n> for accountability, much like the kernel requires Signed-off-by:.\n> A first‑class option keeps that workflow “out of the box”; otherwise\n> people need to define an alias such as\n> \n> [alias]\n> rbr = rebase --trailer \"Reviewed-by: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>\"\n> \n> which is functional but less convenient.\n> \n> I would appreciate your further thoughts on whether a dedicated\n> flag(--reviewby) is acceptable, or whether we should drop it and rely solely on\n> the generic --trailer interface.\n\nI think adding support for --trailer is a good idea and if we do that we \ndon't need --reviewby. The existence and implementation of --signoff is \nlargely a historical artifact - I'm not sure we'd make the same choices \nif we were thinking about adding it today. Different projects have \ndifferent requirements and I don't think it is sensible to add a new \noption catering to the different demands of each project.\n\nI'll take a proper look at the second patch tomorrow.\n\nBest Wishes\n\nPhillip\n\n> Thanks again for the review.\n> \n> Regards,\n> Li\n> \n\n"},{"id":"517472","messageId":"0c4721c6-9ca9-41b4-a2aa-39f521d66426@gmail.com","threadId":"63405","inReplyTo":"48b86c85-bc39-40ba-a2b3-67de707dd798@crinan.ddns.net","subject":"Re: [RFC PATCH 1/2] rebase, am: add --reviewby option","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-07T10:26:07Z","receivedAt":"2025-05-07T10:26:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 07/05/2025 11:17, Phillip Wood wrote:\n> Hi Li\n> \n> On 07/05/2025 07:46, Li Chen wrote:\n>>\n>> Some projects require every commit to carry a Reviewed-by: line\n>> for accountability, much like the kernel requires Signed-off-by:.\n>> A first‑class option keeps that workflow “out of the box”; otherwise\n>> people need to define an alias such as\n>>\n>> [alias]\n>>      rbr = rebase --trailer \"Reviewed-by: $GIT_AUTHOR_NAME \n>> <$GIT_AUTHOR_EMAIL>\"\n>>\n>> which is functional but less convenient.\n>>\n>> I would appreciate your further thoughts on whether a dedicated\n>> flag(--reviewby) is acceptable, or whether we should drop it and rely \n>> solely on\n>> the generic --trailer interface.\n> \n> I think adding support for --trailer is a good idea and if we do that we \n> don't need --reviewby. The existence and implementation of --signoff is \n> largely a historical artifact - I'm not sure we'd make the same choices \n> if we were thinking about adding it today. Different projects have \n> different requirements and I don't think it is sensible to add a new \n> option catering to the different demands of each project.\n\nIt might be worth thinking about how we could extend the trailer option \nso that it uses the committer identity if there is no value specified \nwhich would reduce the pain of adding things like Reviewed-by:\n\nBest Wishes\n\nPhillip\n\n> I'll take a proper look at the second patch tomorrow.\n> \n> Best Wishes\n> \n> Phillip\n> \n>> Thanks again for the review.\n>>\n>> Regards,\n>> Li\n>>\n> \n\n"},{"id":"517473","messageId":"fdf7a827-d8ca-4edc-8427-a2c6eb470e1a@app.fastmail.com","threadId":"63405","inReplyTo":"0c4721c6-9ca9-41b4-a2aa-39f521d66426@gmail.com","subject":"Re: [RFC PATCH 1/2] rebase, am: add --reviewby option","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-05-07T10:39:11Z","receivedAt":"2025-05-07T10:39:35Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Wed, May 7, 2025, at 12:26, Phillip Wood wrote:\n> On 07/05/2025 11:17, Phillip Wood wrote:\n>> Hi Li\n>>\n>> On 07/05/2025 07:46, Li Chen wrote:\n>>>\n>>> Some projects require every commit to carry a Reviewed-by: line\n>>> for accountability, much like the kernel requires Signed-off-by:.\n>>> A first‑class option keeps that workflow “out of the box”; otherwise\n>>> people need to define an alias such as\n>>>\n>>> [alias]\n>>>      rbr = rebase --trailer \"Reviewed-by: $GIT_AUTHOR_NAME\n>>> <$GIT_AUTHOR_EMAIL>\"\n>>>\n>>> which is functional but less convenient.\n>>>\n>>> I would appreciate your further thoughts on whether a dedicated\n>>> flag(--reviewby) is acceptable, or whether we should drop it and rely\n>>> solely on\n>>> the generic --trailer interface.\n>>\n>> I think adding support for --trailer is a good idea and if we do that we\n>> don't need --reviewby. The existence and implementation of --signoff is\n>> largely a historical artifact - I'm not sure we'd make the same choices\n>> if we were thinking about adding it today. Different projects have\n>> different requirements and I don't think it is sensible to add a new\n>> option catering to the different demands of each project.\n>\n> It might be worth thinking about how we could extend the trailer option\n> so that it uses the committer identity if there is no value specified\n> which would reduce the pain of adding things like Reviewed-by:\n\nThat could be confusing for people who use trailers for\nnon-ident metadata.\n\nI was wondering if `git var GIT_COMMITTER_IDENT` could be used. But\nthat prints a Unix timestamp with timezone as well. (I don’t really\nunderstand why after reading that part of the manual)\n\n-- \nKristoffer Haugsbakk\n"},{"id":"517491","messageId":"67417874-d22a-40bf-b35e-4aaaefe6d2b5@gmail.com","threadId":"63405","inReplyTo":"fdf7a827-d8ca-4edc-8427-a2c6eb470e1a@app.fastmail.com","subject":"Re: [RFC PATCH 1/2] rebase, am: add --reviewby option","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-07T13:38:47Z","receivedAt":"2025-05-07T13:38:50Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 07/05/2025 11:39, Kristoffer Haugsbakk wrote:\n> On Wed, May 7, 2025, at 12:26, Phillip Wood wrote:\n> \n>> It might be worth thinking about how we could extend the trailer option\n>> so that it uses the committer identity if there is no value specified\n>> which would reduce the pain of adding things like Reviewed-by:\n> \n> That could be confusing for people who use trailers for\n> non-ident metadata.\n\nYes we need to think about how to do it in a way that is helpful for \nthings like Reviewed-by: or Tested-by: but does not break other \nworkflows. One possibility would be to have a \ntrailer..defaultValue config key but I'd be interested in other \npeople's ideas.\n\nOne other idea is that \"git commit --author Kristoffer\" will search \nrecent commits for matching author to obtain your full name and email \naddress. Something like that could be useful for expanding values \n--tailer=Co-Authored-by\" or perhaps we could teach the completion script \nto suggest suitable completions based on some config describing the \nvalues we expect for certain keys.\n\n> I was wondering if `git var GIT_COMMITTER_IDENT` could be used. But\n> that prints a Unix timestamp with timezone as well. (I don’t really\n> understand why after reading that part of the manual)\n> \nI think it is because with define GIT_*_IDENT to include the timestamp \nto match the ident lines in commit and tag objects. You can always trim \nthe timestamp appending something like \"| sed 's/[^>]*$//'\".\n\nBest Wishes\n\nPhillip\n"},{"id":"517508","messageId":"xmqqselgcoc5.fsf@gitster.g","threadId":"63405","inReplyTo":"48b86c85-bc39-40ba-a2b3-67de707dd798@crinan.ddns.net","subject":"Re: [RFC PATCH 1/2] rebase, am: add --reviewby option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-07T17:39:22Z","receivedAt":"2025-05-07T17:39:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> .... The existence and implementation of\n> --signoff is largely a historical artifact - I'm not sure we'd make\n> the same choices if we were thinking about adding it today.\n\nVery well said.\n\n> I'll take a proper look at the second patch tomorrow.\n\nThanks.\n"},{"id":"517554","messageId":"951d3343-fe97-4e7e-bb73-1c569ff10ee6@gmail.com","threadId":"63405","inReplyTo":"196a5ad7fff.c587c495188189.5980390754046003962@linux.beauty","subject":"Re: [RFC PATCH 2/2] rebase: support --trailer","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-08T14:17:17Z","receivedAt":"2025-05-08T14:17:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Li\n\nOn 06/05/2025 13:58, 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. Automatically set REBASE_FORCE when\n> any trailer is supplied.\n\nI think this is a reasonable idea but unfortunately I think the trailer \nAPI needs improving so that the implementation\n\n(a) Checks the trailers given on the command-line before the user edits \nthe todo list. That way we reject invalid trailers and exit before the \nuser has spent any effort editing the todo list.\n\n(b) Does not fork another process to add the trailers. Without this the \nperformance is going to suffer. Hopefully it wont be too difficult to \nmodify the existing code to take a struct strbuf and a list of trailers \nto append to it.\n\n(c) Only adds the trailers on the commandline. I'm a bit confused by the \nvarious trailer config options - the man page reads to me like \"git \ninterpret-trailers\" can add missing trailers that are configured but not \npassed on the commandline.\n\nThe changes to the trailer api should be made in one or more preparatory \ncommits before adding support for --trailer to \"git rebase\"\n\nI've left some comments on the changes to builtin/rebase.c and the \ntests, I've skipped the changes to sequencer.c for now as they'll have \nto be updated to avoid forking \"git interpret-trailers\"\n\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index b288aedfb1..df65a1e040 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -36,6 +36,9 @@\n> #include \"reset.h\"\n> #include \"trace2.h\"\n> #include \"hook.h\"\n> +#include \"trailer.h\"\n> +\n> +static const char trailer_state_name[] = \"trailer\";\n> \n> static char const * const builtin_rebase_usage[] = {\n> \tN_(\"git rebase [-i] [options] [--exec ] \"\n> @@ -46,6 +49,8 @@ static char const * const builtin_rebase_usage[] = {\n> \tNULL\n> };\n> \n> +static struct strvec trailer_args = STRVEC_INIT;\n\nWe should not need to add any file scope variables\n\n> static GIT_PATH_FUNC(path_squash_onto, \"rebase-merge/squash-onto\")\n> static GIT_PATH_FUNC(path_interactive, \"rebase-merge/interactive\")\n> static GIT_PATH_FUNC(apply_dir, \"rebase-apply\")\n> @@ -114,6 +119,7 @@ struct rebase_options {\n> \tchar *reflog_action;\n> \tint signoff;\n> \tint reviewby;\n> +\tstruct strvec trailer_args;\n\nThis is good and means we don't need the file-scope declaration above.\n\n> \tint allow_rerere_autoupdate;\n> \tint keep_empty;\n> \tint autosquash;\n> @@ -144,6 +150,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> @@ -167,6 +174,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> @@ -179,6 +187,10 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n> \n> \treplay.signoff = opts->signoff;\n> \treplay.reviewby = opts->reviewby;\n> +\n> +\tfor (size_t i = 0; i < opts->trailer_args.nr; i++)\n> + strvec_push(&replay.trailer_args, opts->trailer_args.v[i]);\n\nThe indentation is off here\n\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> @@ -437,6 +449,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> +\tstruct strbuf t = STRBUF_INIT, one = STRBUF_INIT;\n\nYou can just use the existing \"buf\" instead of adding \"t\" and \"one\" is \nunneeded (see below)\n\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> @@ -509,6 +523,22 @@ static int read_basic_state(struct rebase_options *opts)\n> \n> \tstrbuf_release(&buf);\n> \n> +\tif (strbuf_read_file(&t, path, 0) >= 0) {\n> +\t\tconst char *p = t.buf, *end = t.buf + t.len;\n> +\n> +\t\twhile (p < end) {\n> +\t\t\tconst char *nl = memchr(p, '\\n', end - p);\n\nWe expect each line to be terminated with '\\n' so we should die if nl is \nNULL\n> +\t\t\tstrbuf_reset(&one);\n> +\t\t\tstrbuf_add(&one, p, nl ? nl - p : end - p);\n\nInstead of copying the string into an strbuf we can just do \"*nl == '\\0';\"\n\n> +\t\t\tif (one.len) /* skip empty line */\n\nThere should not be any empty lines so we should die here if it is empty\n\n> +\t\t\t\tstrvec_push(&opts->trailer_args,\n> +\t\t\t\t\t strbuf_detach(&one, NULL));\n> +\t\t\tp = nl ? nl + 1 : end;\n> +\t\t}\n> +\t\tstrbuf_release(&one);\n> +\t}\n> +\tstrbuf_release(&t);\n> +\n> \treturn 0;\n> }\n> \n> @@ -537,6 +567,28 @@ static int rebase_write_basic_state(struct rebase_options *opts)\n> \tif (opts->reviewby)\n> \t\twrite_file(state_dir_path(\"reviewby\", opts), \"--reviewby\");\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\nThis looks good\n\n> + } else {\n> + /*\n> + * but if rebase doesn't pass any --trailer，\n> + * and state dir still have residual files，let's delete it。\n> + */\n\nThis should never happen should it?\n\n> + unlink_or_warn(state_dir_path(trailer_state_name, opts));\n> + }\n> +\n> \treturn 0;\n> }\n> \n> @@ -1140,6 +1192,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_PASSTHRU_ARGV(0, \"trailer\", &trailer_args, N_(\"trailer\"), N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG),\n\nThis should be OPT_STRVEC() and should populate &options.trailers\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, \"reviewby\", &options.reviewby,\n> @@ -1292,6 +1345,13 @@ int cmd_rebase(int argc,\n> \t\t\t builtin_rebase_options,\n> \t\t\t builtin_rebase_usage, 0);\n> \n> + for (i = 0; i < trailer_args.nr; i++)\n> + \t strvec_push(&options.trailer_args, trailer_args.v[i]);\n> +\n> + /* if add --trailer，force rebase */\n> + if (options.trailer_args.nr)\n> +\t\t options.flags |= REBASE_FORCE;\n\nThis is good\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> @@ -1549,6 +1609,16 @@ 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.type == REBASE_APPLY && options.trailer_args.nr)\n> +\t\tdie(_(\"the --apply backend (git am) cannot currently handle \"\n> +\t\t \"--trailer; please omit --apply or use \"\n> +\t\t \"the merge/interactive backend\"));\n\nIt's good you're checking this but we should call imply_merge() to do \nthat instead.\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> +############################################################################\n> +# 1. repository setup\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\nThe alignment is off here - it would be much simpler just to separate \neach argument with a single space\n\n> +\tident=\"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n> +'\n> +\n> +create_expect () {\n> +\tcat >\"$1\" <<-EOF\n> +\t$2\n> +\n> +\tReviewed-by: Dev \n> +\tEOF\n> +}\n> +# golden files:\n> +create_expect initial-signed \"Initial empty commit\"\n> +create_expect first-signed \"first\"\n> +create_expect second-signed \"second\"\n> +create_expect file2-signed \"file-2\"\n> +create_expect third-signed \"third\"\n> +create_expect conflict-signed \"conflict\"\n\nThanks for adding these tests. This should be inside the setup test - \nnoting should be run outside of test_expect_success()\n\n> +\n> +############################################################################\n> +# 2. explicitly reject --apply + --trailer\n> +############################################################################\n> +\n> +test_expect_success 'apply backend is rejected when --trailer is given' '\n> +\tgit reset --hard third &&\n> +\tgit tag before-apply &&\n> +\ttest_expect_code 128 \\\n> +\t\tgit rebase --apply --trailer \"Reviewed-by: Dev \" \\\n> +\t\t\tHEAD^ &&\n\nWe should check the error message here by redirecting stderr to a file \nand checking that with test_grep or test_cmp so we can be confidant that \nrebase is failing for the reason it should be and not some other bug.\n\n> +\tgit diff --quiet before-apply..HEAD # prove nothing moved\n\nIf you want to check that HEAD is unchanged I'd run \"head=$(git show-ref \n--verify -s HEAD)\" before the rebase and \"test_cmp_rev HEAD $head\" \nafterwards.\n\n> +'\n> +\n> +############################################################################\n> +# 3. --no‑op: same range, no changes\n> +############################################################################\n> +\n> +test_expect_success '--trailer without range change is a no‑op' '\n> +\tgit checkout main &&\n> +\tgit tag before-noop &&\n> +\tgit rebase --trailer \"Reviewed-by: Dev \" HEAD &&\n> +\tgit diff --quiet before-noop..HEAD\n\nDo we really need this - it isn't really checking anything to do with \nthe implementation of --trailer.\n\n> +\n> +############################################################################\n> +# 4. merge backend (-m), conflict resolution path\n> +############################################################################\n\nExcellent\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_EDITOR=: git rebase --continue &&\nGIT_EDITOR=: unless you call test_set_editor() which should always be \ndone in a subshell so this isn't needed.\n\n> +\tgit show --no-patch --pretty=format:%B HEAD~2 >actual &&\n> +\ttest_cmp file2-signed actual\n\nI think test_commit_message would help simplify this\n\n> +'\n> +\n> +############################################################################\n> +# 5. --exec path\n> +############################################################################\n> +\n> +test_expect_success 'rebase --exec --trailer adds trailer' '\n> +\ttest_when_finished \"rm -f touched\" &&\n> +\tgit rebase --exec \"touch touched\" \\\n> +\t\t--trailer \"Reviewed-by: Dev \" \\\n> +\t\tfirst^ first &&\n> +\ttest_path_is_file touched &&\n> +\ttest_commit_message HEAD first-signed\n\nI'm not sure this has anything to do with --trailer\n\n> +'\n> +\n> +############################################################################\n> +# 6. --root path\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\nThis test looks good.\n\nBest Wishes\n\nPhillip\n\n> +'\n> +test_done\nG\n"},{"id":"517568","messageId":"196b09c1324.fba686d21234508.1995945025408421751@linux.beauty","threadId":"63405","inReplyTo":"951d3343-fe97-4e7e-bb73-1c569ff10ee6@gmail.com","subject":"Re: [RFC PATCH 2/2] rebase: support --trailer","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-05-08T15:55:40Z","receivedAt":"2025-05-08T15:55:54Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"Hi Phillip,\n\n ---- On Thu, 08 May 2025 22:17:17 +0800 Phillip Wood wrote --- \n > Hi Li\n > \n > On 06/05/2025 13:58, 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. Automatically set REBASE_FORCE when\n > > any trailer is supplied.\n > \n > I think this is a reasonable idea but unfortunately I think the trailer \n > API needs improving so that the implementation\n > \n > (a) Checks the trailers given on the command-line before the user edits \n > the todo list. That way we reject invalid trailers and exit before the \n > user has spent any effort editing the todo list.\n > \n > (b) Does not fork another process to add the trailers. Without this the \n > performance is going to suffer. Hopefully it wont be too difficult to \n > modify the existing code to take a struct strbuf and a list of trailers \n > to append to it.\n > \n > (c) Only adds the trailers on the commandline. I'm a bit confused by the \n > various trailer config options - the man page reads to me like \"git \n > interpret-trailers\" can add missing trailers that are configured but not \n > passed on the commandline.\n > \n > The changes to the trailer api should be made in one or more preparatory \n > commits before adding support for --trailer to \"git rebase\"\n\nThanks a lot for the detailed feedback and for outlining the gaps in the\ncurrent trailer API – it really helps me see the next steps more\nclearly.\n\nI have one questions:\n\nWho should take the API work?\n\nI’m very happy to roll up my sleeves and prototype the improvements\nyou described (reject invalid trailers up‑front, add an in‑process API\nthat works on a struct strbuf, and ensure only the CLI‑supplied\ntrailers are added). That would give me a chance to contribute a bit\nmore deeply to Git. Of course, if you already have something in mind\nand would rather drive that part yourself, I’m equally happy to wait\nand re‑base my series on top of it. Please let me know which you\nprefer.\n\n > I've left some comments on the changes to builtin/rebase.c and the \n > tests, I've skipped the changes to sequencer.c for now as they'll have \n > to be updated to avoid forking \"git interpret-trailers\"\n\nThanks for all your great reviews!\n\nI'll address all your reviews in next version.\n \n\n"},{"id":"517569","messageId":"1b6545a0-5981-4377-9709-39ff29aeda6a@gmail.com","threadId":"63405","inReplyTo":"196b09c1324.fba686d21234508.1995945025408421751@linux.beauty","subject":"Re: [RFC PATCH 2/2] rebase: support --trailer","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-08T15:58:44Z","receivedAt":"2025-05-08T15:58:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Li\n\nOn 08/05/2025 16:55, Li Chen wrote:\n> ---- On Thu, 08 May 2025 22:17:17 +0800 Phillip Wood wrote ---\n> > On 06/05/2025 13:58, 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. Automatically set REBASE_FORCE when\n> > > any trailer is supplied.\n> >\n> > I think this is a reasonable idea but unfortunately I think the trailer\n> > API needs improving so that the implementation\n> >\n> > (a) Checks the trailers given on the command-line before the user edits\n> > the todo list. That way we reject invalid trailers and exit before the\n> > user has spent any effort editing the todo list.\n> >\n> > (b) Does not fork another process to add the trailers. Without this the\n> > performance is going to suffer. Hopefully it wont be too difficult to\n> > modify the existing code to take a struct strbuf and a list of trailers\n> > to append to it.\n> >\n> > (c) Only adds the trailers on the commandline. I'm a bit confused by the\n> > various trailer config options - the man page reads to me like \"git\n> > interpret-trailers\" can add missing trailers that are configured but not\n> > passed on the commandline.\n> >\n> > The changes to the trailer api should be made in one or more preparatory\n> > commits before adding support for --trailer to \"git rebase\"\n> \n> Thanks a lot for the detailed feedback and for outlining the gaps in the\n> current trailer API – it really helps me see the next steps more\n> clearly.\n> \n> I have one questions:\n> \n> Who should take the API work?\n> \n> I’m very happy to roll up my sleeves and prototype the improvements\n> you described (reject invalid trailers up‑front, add an in‑process API\n> that works on a struct strbuf, and ensure only the CLI‑supplied\n> trailers are added). That would give me a chance to contribute a bit\n> more deeply to Git. Of course, if you already have something in mind\n> and would rather drive that part yourself, I’m equally happy to wait\n> and re‑base my series on top of it. Please let me know which you\n> prefer.\n\nIf you're interested in doing the work on the trailer api that would be \ngreat - I don't have time myself.\n\nThanks\n\nPhillip\n\n> > I've left some comments on the changes to builtin/rebase.c and the\n> > tests, I've skipped the changes to sequencer.c for now as they'll have\n> > to be updated to avoid forking \"git interpret-trailers\"\n> \n> Thanks for all your great reviews!\n> \n> I'll address all your reviews in next version.\n> \n> \n\n"},{"id":"517572","messageId":"98415c84-a5ab-4bf0-b361-dd60ff3e7dd2@gmail.com","threadId":"63405","inReplyTo":"196b09c1324.fba686d21234508.1995945025408421751@linux.beauty","subject":"Re: [RFC PATCH 2/2] rebase: support --trailer","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-08T16:29:01Z","receivedAt":"2025-05-08T16:28:57Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Li\n\nOn 08/05/2025 16:55, Li Chen wrote:\n> ---- On Thu, 08 May 2025 22:17:17 +0800 Phillip Wood wrote ---\n> \n> > I've left some comments on the changes to builtin/rebase.c and the\n> > tests, I've skipped the changes to sequencer.c for now as they'll have\n> > to be updated to avoid forking \"git interpret-trailers\"\n> \n> Thanks for all your great reviews!\n> \n> I'll address all your reviews in next version.\n\nI forgot to say earlier that if you have any questions or queries about \nmy review or the trailer code do feel free to ask.\n\nBest Wishes\n\nPhillip\n\n\n"},{"id":"518213","messageId":"196d79d922d.127d7eb7f870411.3309775784450179401@linux.beauty","threadId":"63405","inReplyTo":"951d3343-fe97-4e7e-bb73-1c569ff10ee6@gmail.com","subject":"Re: [RFC PATCH 2/2] rebase: support --trailer","fromName":"Li Chen","fromEmail":"me@linux.beauty","sentAt":"2025-05-16T05:42:30Z","receivedAt":"2025-05-16T05:42:40Z","isPatch":true,"sender":{"key":"me@linux.beauty","avatar":"https://avatars.githubusercontent.com/u/37442588?v=4"},"body":"Hi Phillip,\n\n ---- On Thu, 08 May 2025 22:17:17 +0800 Phillip Wood wrote --- \n > Hi Li\n > \n > On 06/05/2025 13:58, 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. Automatically set REBASE_FORCE when\n > > any trailer is supplied.\n > \n > I think this is a reasonable idea but unfortunately I think the trailer \n > API needs improving so that the implementation\n > \n > (a) ...\n > \n > (b) ...\n > \n > (c) Only adds the trailers on the commandline. I'm a bit confused by the \n > various trailer config options - the man page reads to me like \"git \n > interpret-trailers\" can add missing trailers that are configured but not \n > passed on the commandline.\n\nAbout part (c), just to be sure I understand correctly:\n\nDo you want the trailer implementation to completely drop any handling of trailer configuration \n(i.e. remove parse_trailers_from_config() and related config-based behavior from the codepath and man page/documents)?\n\nOr would you rather leave the config machinery in place, but have rebase --trailer explicitly\nignore all trailer.* configuration and only append the exact trailers passed on its command line?\n\nPlease let me know which you prefer (or if there’s a third path I’m missing) and I’ll add the patches accordingly.\n\nRegards,\nLi\n"},{"id":"518229","messageId":"8b431310-ff03-424b-b24c-672d9b492057@gmail.com","threadId":"63405","inReplyTo":"196d79d922d.127d7eb7f870411.3309775784450179401@linux.beauty","subject":"Re: [RFC PATCH 2/2] rebase: support --trailer","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-16T10:04:37Z","receivedAt":"2025-05-16T10:04:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Li\n\nOn 16/05/2025 06:42, Li Chen wrote:\n> Hi Phillip,\n> \n> ---- On Thu, 08 May 2025 22:17:17 +0800 Phillip Wood wrote ---\n> >\n> > (c) Only adds the trailers on the commandline. I'm a bit confused by the\n> > various trailer config options - the man page reads to me like \"git\n> > interpret-trailers\" can add missing trailers that are configured but not\n> > passed on the commandline.\n> \n> About part (c), just to be sure I understand correctly:\n> \n> Do you want the trailer implementation to completely drop any handling of trailer configuration\n> (i.e. remove parse_trailers_from_config() and related config-based behavior from the codepath and man page/documents)?\n> \n> Or would you rather leave the config machinery in place, but have rebase --trailer explicitly\n> ignore all trailer.* configuration and only append the exact trailers passed on its command line?\n\nI think I had misunderstood what trailer.ifMissing did. I was concerned \nthat it could add trailers that were not on the command-line but I don't \nthink that's the case. We certainly want to respect the config for \ntrailer..key and trailer..command as they make it possible \nfor the user to set \"trailer.review.key=Reviewed-by\" and \n\"trailer.review.command=git var GIT_COMMITTER_IDENT | sed 's/[^>]*$//' \n#\" and then run \"git rebase --trailer=review\" to add their Reviewed-by: \ntrailer. I think it makes sense to respect the other config as well - \nthat does mean that trailers that the user passes on the command-line \nmay not be added because they already exist or are configured not to be \nadded if they are missing but is consistent with \"git \ninterpret-trailers\". It means that the user can set \n\"trailer.myKey.ifExists=doNothing\" and then run \"git rebase --trailer \nMyKey=value\" to ensure all the commits have a MyKey trailer without \nduplicating it in the commits where it already exists.\n\nThat's a long-winded way of saying that on reflection I think respecting \nthe trailer config setting is the right thing to do after all.\n\nBest Wishes\n\nPhillip\n\n> Please let me know which you prefer (or if there’s a third path I’m missing) and I’ll add the patches accordingly.\n> \n> Regards,\n> Li\n\n"}]}