* [RFC PATCH 0/2] rebase: support --trailer and add --reviewby
@ 2025-05-06 12:57 Li Chen
2025-05-06 12:58 ` [RFC PATCH 1/2] rebase, am: add --reviewby option Li Chen
2025-05-06 12:58 ` [RFC PATCH 2/2] rebase: support --trailer Li Chen
0 siblings, 2 replies; 16+ messages in thread
From: Li Chen @ 2025-05-06 12:57 UTC (permalink / raw)
To: git
From: Li Chen <chenl311@chinatelecom.cn>
This patch series enhances `git rebase` and `git am` by introducing three features:
1. `--trailer <line>` support on rebase's merge backend
Allows arbitrary trailer lines (e.g. multiple “Reviewed-by: …”) to be appended
to each rebased commit. The apply backend (`git am`) is explicitly
rejected when used with `--trailer`, since it has no message‑filter/trailer support.
This is especially useful when reviewee add reviewer's Reviewed-by, e.g.,
git rebase \
--trailer "Reviewed-by: Bob <bob@example.com>" \
--trailer "Tested-by: Dana <dana@example.com>" \
base~1
2. `--reviewby` shortcut flag for rebase
A convenience alias for adding a single “Reviewed‑by:” trailer using your configured
committer identity (user.name/user.email), analogous to --signoff. It works on both
rebase backends (merge and apply) and automatically disables fast‑forwarding to
rewrite commits.
This is especially useful when reviewer adds his/her Reviewed-by for a given patchset
which is already applied, e.g., git rebase --reviewby base~1
3. BTW, `--reviewby` shortcut flag is also added for `am` cmd, because it's needed
by rebase's apply backend.
I’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).
Looking forward to your feedback!
Li Chen (2):
rebase, am: add --reviewby option
rebase: support --trailer
Documentation/git-am.adoc | 6 +-
builtin/am.c | 31 +++++++++
builtin/rebase.c | 84 ++++++++++++++++++++++
builtin/revert.c | 4 +-
sequencer.c | 100 ++++++++++++++++++++++++++-
sequencer.h | 12 ++++
t/meson.build | 2 +
t/t3439-rebase-reviewby.sh | 138 +++++++++++++++++++++++++++++++++++++
t/t3440-rebase-trailer.sh | 108 +++++++++++++++++++++++++++++
t/t4150-am.sh | 75 ++++++++++++++++++++
10 files changed, 556 insertions(+), 4 deletions(-)
create mode 100755 t/t3439-rebase-reviewby.sh
create mode 100755 t/t3440-rebase-trailer.sh
--
2.49.0
^ permalink raw reply [flat|nested] 16+ messages in thread* [RFC PATCH 1/2] rebase, am: add --reviewby option 2025-05-06 12:57 [RFC PATCH 0/2] rebase: support --trailer and add --reviewby Li Chen @ 2025-05-06 12:58 ` Li Chen 2025-05-06 18:07 ` Junio C Hamano 2025-05-06 12:58 ` [RFC PATCH 2/2] rebase: support --trailer Li Chen 1 sibling, 1 reply; 16+ messages in thread From: Li Chen @ 2025-05-06 12:58 UTC (permalink / raw) To: git From: Li Chen <chenl311@chinatelecom.cn> Introduce a new `--reviewby` flag to `git rebase` and `git am` that appends a `Reviewed-by: name <email>` trailer to each rebased or applied commit, analogous to the existing `--signoff` behavior. especially useful for backporting patches and reviewing new patches. Signed-off-by: Li Chen <chenl311@chinatelecom.cn> Reviewed-by: Li Chen <chenl311@chinatelecom.cn> --- Documentation/git-am.adoc | 6 +- Documentation/git-rebase.adoc | 4 + builtin/am.c | 31 ++++++++ builtin/rebase.c | 14 ++++ builtin/revert.c | 4 +- sequencer.c | 87 ++++++++++++++++++++- sequencer.h | 10 +++ t/meson.build | 1 + t/t3439-rebase-reviewby.sh | 138 ++++++++++++++++++++++++++++++++++ t/t4150-am.sh | 75 ++++++++++++++++++ 10 files changed, 366 insertions(+), 4 deletions(-) create mode 100755 t/t3439-rebase-reviewby.sh diff --git a/Documentation/git-am.adoc b/Documentation/git-am.adoc index 221070de48..1d408f2bab 100644 --- a/Documentation/git-am.adoc +++ b/Documentation/git-am.adoc @@ -9,7 +9,7 @@ git-am - Apply a series of patches from a mailbox SYNOPSIS -------- [verse] -'git am' [--signoff] [--keep] [--[no-]keep-cr] [--[no-]utf8] [--no-verify] +'git am' [--signoff] [--reviewby] [--keep] [--[no-]keep-cr] [--[no-]utf8] [--no-verify] [--[no-]3way] [--interactive] [--committer-date-is-author-date] [--ignore-date] [--ignore-space-change | --ignore-whitespace] [--whitespace=<action>] [-C<n>] [-p<n>] [--directory=<dir>] @@ -40,7 +40,9 @@ OPTIONS Add a `Signed-off-by` trailer to the commit message, using the committer identity of yourself. See the signoff option in linkgit:git-commit[1] for more information. - +--reviewby:: + Add a `Reviewed-by` trailer to the commit message, using + the committer identity of yourself. -k:: --keep:: Pass `-k` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]). diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc index 956d3048f5..1f3d152035 100644 --- a/Documentation/git-rebase.adoc +++ b/Documentation/git-rebase.adoc @@ -521,6 +521,10 @@ See also INCOMPATIBLE OPTIONS below. that if `--interactive` is given then only commits marked to be picked, edited or reworded will have the trailer added. + +--review-by:: + Append a `Reviewed-by:` trailer whose value is taken from the + current committer identity, exactly like `--signoff` appends a + Signed-off-by:` trailer. See also INCOMPATIBLE OPTIONS below. -i:: diff --git a/builtin/am.c b/builtin/am.c index 4afb519830..f8fe95e15d 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -89,6 +89,12 @@ enum signoff_type { SIGNOFF_EXPLICIT /* --signoff was set on the command-line */ }; +enum reviewby_type { + REVIEWBY_FALSE = 0, + REVIEWBY_TRUE = 1, + REVIEWBY_EXPLICIT /* --reviewby was set on the command-line */ +}; + enum resume_type { RESUME_FALSE = 0, RESUME_APPLY, @@ -134,6 +140,7 @@ struct am_state { int threeway; int quiet; int signoff; /* enum signoff_type */ + int reviewby; /* enum reviewby_type */ int utf8; int keep; /* enum keep_type */ int message_id; @@ -422,6 +429,9 @@ static void am_load(struct am_state *state) read_state_file(&sb, state, "sign", 1); state->signoff = !strcmp(sb.buf, "t"); + read_state_file(&sb, state, "review", 1); + state->reviewby = !strcmp(sb.buf, "t"); + read_state_file(&sb, state, "utf8", 1); state->utf8 = !strcmp(sb.buf, "t"); @@ -1017,6 +1027,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format, write_state_bool(state, "threeway", state->threeway); write_state_bool(state, "quiet", state->quiet); write_state_bool(state, "sign", state->signoff); + write_state_bool(state, "review", state->reviewby); write_state_bool(state, "utf8", state->utf8); if (state->allow_rerere_autoupdate) @@ -1191,6 +1202,18 @@ static void am_append_signoff(struct am_state *state) state->msg = strbuf_detach(&sb, &state->msg_len); } +/** + * Appends reviewby to the "msg" field of the am_state. + */ +static void am_append_reviewby(struct am_state *state) +{ + struct strbuf sb = STRBUF_INIT; + + strbuf_attach(&sb, state->msg, state->msg_len, state->msg_len); + append_reviewby(&sb, 0, 0); + state->msg = strbuf_detach(&sb, &state->msg_len); +} + /** * Parses `mail` using git-mailinfo, extracting its patch and authorship info. * state->msg will be set to the patch message. state->author_name, @@ -1849,6 +1872,9 @@ static void am_run(struct am_state *state, int resume) if (state->signoff) am_append_signoff(state); + if (state->reviewby) + am_append_reviewby(state); + write_author_script(state); write_commit_msg(state); } @@ -2337,6 +2363,9 @@ int cmd_am(int argc, OPT_SET_INT('s', "signoff", &state.signoff, N_("add a Signed-off-by trailer to the commit message"), SIGNOFF_EXPLICIT), + OPT_SET_INT(0, "reviewby", &state.reviewby, + N_("add a Reviewed-by trailer to the commit message"), + REVIEWBY_EXPLICIT), OPT_BOOL('u', "utf8", &state.utf8, N_("recode into utf8 (default)")), OPT_SET_INT('k', "keep", &state.keep, @@ -2483,6 +2512,8 @@ int cmd_am(int argc, if (state.signoff == SIGNOFF_EXPLICIT) am_append_signoff(&state); + if (state.reviewby == REVIEWBY_EXPLICIT) + am_append_reviewby(&state); } else { struct strvec paths = STRVEC_INIT; int i; diff --git a/builtin/rebase.c b/builtin/rebase.c index 2e8c4ee678..b288aedfb1 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -113,6 +113,7 @@ struct rebase_options { enum action action; char *reflog_action; int signoff; + int reviewby; int allow_rerere_autoupdate; int keep_empty; int autosquash; @@ -177,6 +178,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts) sequencer_init_config(&replay); replay.signoff = opts->signoff; + replay.reviewby = opts->reviewby; replay.allow_ff = !(opts->flags & REBASE_FORCE); if (opts->allow_rerere_autoupdate) replay.allow_rerere_auto = opts->allow_rerere_autoupdate; @@ -477,6 +479,10 @@ static int read_basic_state(struct rebase_options *opts) opts->signoff = 1; opts->flags |= REBASE_FORCE; } + if (file_exists(state_dir_path("reviewby", opts))) { + opts->reviewby = 1; + opts->flags |= REBASE_FORCE; + } if (file_exists(state_dir_path("allow_rerere_autoupdate", opts))) { strbuf_reset(&buf); @@ -528,6 +534,8 @@ static int rebase_write_basic_state(struct rebase_options *opts) opts->gpg_sign_opt); if (opts->signoff) write_file(state_dir_path("signoff", opts), "--signoff"); + if (opts->reviewby) + write_file(state_dir_path("reviewby", opts), "--reviewby"); return 0; } @@ -1134,6 +1142,8 @@ int cmd_rebase(int argc, }, OPT_BOOL(0, "signoff", &options.signoff, N_("add a Signed-off-by trailer to each commit")), + OPT_BOOL(0, "reviewby", &options.reviewby, + N_("add a Reviewed-by trailer to each commit")), OPT_BOOL(0, "committer-date-is-author-date", &options.committer_date_is_author_date, N_("make committer date match author date")), @@ -1618,6 +1628,10 @@ int cmd_rebase(int argc, strvec_push(&options.git_am_opts, "--signoff"); options.flags |= REBASE_FORCE; } + if (options.reviewby) { + strvec_push(&options.git_am_opts, "--reviewby"); + options.flags |= REBASE_FORCE; + } if (!options.root) { if (argc < 1) { diff --git a/builtin/revert.c b/builtin/revert.c index e07c2217fe..c73a773c9d 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -124,7 +124,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, OPT_CLEANUP(&cleanup_arg), OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")), OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")), - OPT_NOOP_NOARG('r', NULL), + OPT_BOOL('r', "reviewby", &opts->reviewby, N_("add a Reviewed-by trailer")), OPT_BOOL('s', "signoff", &opts->signoff, N_("add a Signed-off-by trailer")), OPT_CALLBACK('m', "mainline", opts, N_("parent-number"), N_("select mainline parent"), option_parse_m), @@ -206,6 +206,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, verify_opt_compatible(me, this_operation, "--no-commit", opts->no_commit, "--signoff", opts->signoff, + "--reviewby", opts->reviewby, "--mainline", opts->mainline, "--strategy", opts->strategy ? 1 : 0, "--strategy-option", opts->xopts.nr ? 1 : 0, @@ -226,6 +227,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, if (opts->allow_ff) verify_opt_compatible(me, "--ff", "--signoff", opts->signoff, + "--reviewby", opts->reviewby, "--no-commit", opts->no_commit, "-x", opts->record_origin, "--edit", opts->edit > 0, diff --git a/sequencer.c b/sequencer.c index b5c4043757..01933882ed 100644 --- a/sequencer.c +++ b/sequencer.c @@ -61,6 +61,7 @@ #define GIT_MAX_LABEL_LENGTH ((NAME_MAX) - (LOCK_SUFFIX_LEN) - 16) static const char sign_off_header[] = "Signed-off-by: "; +static const char review_by_header[] = "Reviewed-by: "; static const char cherry_picked_prefix[] = "(cherry picked from commit "; GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG") @@ -199,6 +200,7 @@ static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head") static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose") static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet") static GIT_PATH_FUNC(rebase_path_signoff, "rebase-merge/signoff") +static GIT_PATH_FUNC(rebase_path_reviewby, "rebase-merge/reviewby") static GIT_PATH_FUNC(rebase_path_head_name, "rebase-merge/head-name") static GIT_PATH_FUNC(rebase_path_onto, "rebase-merge/onto") static GIT_PATH_FUNC(rebase_path_autostash, "rebase-merge/autostash") @@ -1179,6 +1181,10 @@ static int run_git_commit(const char *defmsg, !opts->signoff && !opts->record_origin && !opts->explicit_cleanup) strvec_push(&cmd.args, "--cleanup=verbatim"); + else if (!(flags & CLEANUP_MSG) && + !opts->reviewby && !opts->record_origin && + !opts->explicit_cleanup) + strvec_push(&cmd.args, "--cleanup=verbatim"); if ((flags & ALLOW_EMPTY)) strvec_push(&cmd.args, "--allow-empty"); @@ -1628,6 +1634,9 @@ static int try_to_commit(struct repository *r, else if ((opts->signoff || opts->record_origin) && !opts->explicit_cleanup) cleanup = COMMIT_MSG_CLEANUP_SPACE; + else if ((opts->reviewby || opts->record_origin) && + !opts->explicit_cleanup) + cleanup = COMMIT_MSG_CLEANUP_SPACE; else cleanup = opts->default_msg_cleanup; @@ -2039,6 +2048,8 @@ static int append_squash_message(struct strbuf *buf, const char *body, */ if (opts->signoff) append_signoff(buf, 0, 0); + if (opts->reviewby) + append_reviewby(buf, 0, 0); if ((command == TODO_FIXUP) && (flag & TODO_REPLACE_FIXUP_MSG) && @@ -2418,6 +2429,9 @@ static int do_pick_commit(struct repository *r, if (opts->signoff && !is_fixup(command)) append_signoff(&ctx->message, 0, 0); + if (opts->reviewby && !is_fixup(command)) + append_reviewby(&ctx->message, 0, 0); + if (is_rebase_i(opts) && write_author_script(msg.message) < 0) res = -1; else if (!opts->strategy || @@ -3079,6 +3093,8 @@ static int populate_opts_cb(const char *key, const char *value, git_config_bool_or_int(key, value, ctx->kvi, &error_flag); else if (!strcmp(key, "options.signoff")) opts->signoff = git_config_bool_or_int(key, value, ctx->kvi, &error_flag); + else if (!strcmp(key, "options.reviewby")) + opts->reviewby = git_config_bool_or_int(key, value, ctx->kvi, &error_flag); else if (!strcmp(key, "options.record-origin")) opts->record_origin = git_config_bool_or_int(key, value, ctx->kvi, &error_flag); else if (!strcmp(key, "options.allow-ff")) @@ -3180,6 +3196,10 @@ static int read_populate_opts(struct replay_opts *opts) opts->allow_ff = 0; opts->signoff = 1; } + if (file_exists(rebase_path_reviewby())) { + opts->allow_ff = 0; + opts->reviewby = 1; + } if (file_exists(rebase_path_cdate_is_adate())) { opts->allow_ff = 0; @@ -3286,6 +3306,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name, write_file(rebase_path_gpg_sign_opt(), "-S%s\n", opts->gpg_sign); if (opts->signoff) write_file(rebase_path_signoff(), "--signoff\n"); + if (opts->reviewby) + write_file(rebase_path_reviewby(), "--reviewby\n"); if (opts->drop_redundant_commits) write_file(rebase_path_drop_redundant_commits(), "%s", ""); if (opts->keep_redundant_commits) @@ -3629,6 +3651,9 @@ static int save_opts(struct replay_opts *opts) if (opts->signoff) res |= git_config_set_in_file_gently(opts_file, "options.signoff", NULL, "true"); + if (opts->reviewby) + res |= git_config_set_in_file_gently(opts_file, + "options.reviewby", NULL, "true"); if (opts->record_origin) res |= git_config_set_in_file_gently(opts_file, "options.record-origin", NULL, "true"); @@ -4949,7 +4974,7 @@ static int pick_commits(struct repository *r, ctx->reflog_message = sequencer_reflog_action(opts); if (opts->allow_ff) - ASSERT(!(opts->signoff || opts->no_commit || + ASSERT(!(opts->signoff || opts->reviewby || opts->no_commit || opts->record_origin || should_edit(opts) || opts->committer_date_is_author_date || opts->ignore_date)); @@ -5581,6 +5606,66 @@ int sequencer_pick_revisions(struct repository *r, return res; } +void append_reviewby(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag) +{ + unsigned no_dup_rb = flag & APPEND_REVIEWBY_DEDUP; + struct strbuf rb = STRBUF_INIT; + int has_footer; + + strbuf_addstr(&rb, review_by_header); + strbuf_addstr(&rb, fmt_name(WANT_COMMITTER_IDENT)); + strbuf_addch(&rb, '\n'); + + if (!ignore_footer) + strbuf_complete_line(msgbuf); + + /* + * If the whole message buffer is equal to the rb, pretend that we + * found a conforming footer with a matching rb + */ + if (msgbuf->len - ignore_footer == rb.len && + !strncmp(msgbuf->buf, rb.buf, rb.len)) + has_footer = 3; + else + has_footer = has_conforming_footer(msgbuf, &rb, ignore_footer); + + if (!has_footer) { + const char *append_newlines = NULL; + size_t len = msgbuf->len - ignore_footer; + + if (!len) { + /* + * The buffer is completely empty. Leave foom for + * the title and body to be filled in by the user. + */ + append_newlines = "\n\n"; + } else if (len == 1) { + /* + * Buffer contains a single newline. Add another + * so that we leave room for the title and body. + */ + append_newlines = "\n"; + } else if (msgbuf->buf[len - 2] != '\n') { + /* + * Buffer ends with a single newline. Add another + * so that there is an empty line between the message + * body and the rb. + */ + append_newlines = "\n"; + } /* else, the buffer already ends with two newlines. */ + + if (append_newlines) + strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, + append_newlines, strlen(append_newlines)); + } + + if (has_footer != 3 && (!no_dup_rb || has_footer != 2)) + strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, + rb.buf, rb.len); + + strbuf_release(&rb); +} + void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag) { unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP; diff --git a/sequencer.h b/sequencer.h index 304ba4b4d3..82b79fd1e8 100644 --- a/sequencer.h +++ b/sequencer.h @@ -17,6 +17,7 @@ const char *rebase_path_dropped(void); extern const char *rebase_resolvemsg; #define APPEND_SIGNOFF_DEDUP (1u << 0) +#define APPEND_REVIEWBY_DEDUP (1u << 1) enum replay_action { REPLAY_REVERT, @@ -44,6 +45,7 @@ struct replay_opts { int record_origin; int no_commit; int signoff; + int reviewby; int allow_ff; int allow_rerere_auto; int allow_empty; @@ -205,6 +207,14 @@ int todo_list_rearrange_squash(struct todo_list *todo_list); */ void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag); +/* + * Append a reviewby to the commit message in "msgbuf". The ignore_footer + * parameter specifies the number of bytes at the end of msgbuf that should + * not be considered at all. I.e., they are not checked for existing trailers, + * and the new reviewby will be spliced into the buffer before those bytes. + */ +void append_reviewby(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag); + void append_conflicts_hint(struct index_state *istate, struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode); enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg, diff --git a/t/meson.build b/t/meson.build index bfb744e886..327fa461fd 100644 --- a/t/meson.build +++ b/t/meson.build @@ -392,6 +392,7 @@ integration_tests = [ 't3436-rebase-more-options.sh', 't3437-rebase-fixup-options.sh', 't3438-rebase-broken-files.sh', + 't3439-rebase-reviewby.sh', 't3500-cherry.sh', 't3501-revert-cherry-pick.sh', 't3502-cherry-pick-merge.sh', diff --git a/t/t3439-rebase-reviewby.sh b/t/t3439-rebase-reviewby.sh new file mode 100755 index 0000000000..ca7299fbdb --- /dev/null +++ b/t/t3439-rebase-reviewby.sh @@ -0,0 +1,138 @@ +#!/bin/sh + +test_description='git rebase --reviewby + +This test runs git rebase --reviewby and make sure that it works. +' + +. ./test-lib.sh +. "$TEST_DIRECTORY"/lib-rebase.sh + +test_expect_success 'setup' ' + git commit --allow-empty -m "Initial empty commit" && + test_commit first file a && + test_commit second file && + git checkout -b conflict-branch first && + test_commit file-2 file-2 && + test_commit conflict file && + test_commit third file && + + ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" && + + # Expected commit message for initial commit after rebase --reviewby + cat >expected-initial-reviewed <<-EOF && + Initial empty commit + + Reviewed-by: $ident + EOF + + # Expected commit message after rebase --reviewby + cat >expected-reviewed <<-EOF && + first + + Reviewed-by: $ident + EOF + + # Expected commit message after conflict resolution for rebase --reviewby + cat >expected-reviewed-conflict <<-EOF && + third + + Reviewed-by: $ident + + conflict + + Reviewed-by: $ident + + file-2 + + Reviewed-by: $ident + + EOF + + # Expected commit message after rebase without --reviewby (or with --no-reviewby) + cat >expected-unreviewed <<-EOF && + first + EOF + + git config alias.rbr "rebase --reviewby" +' + +test_expect_success 'rebase --apply --reviewby adds a Reviewed-by line' ' + test_must_fail git rbr --apply second third && + git checkout --theirs file && + git add file && + git rebase --continue && + git log --format=%B -n3 >actual && + test_cmp expected-reviewed-conflict actual +' + +test_expect_success 'rebase --no-reviewby does not add a Reviewed-by line' ' + git commit --amend -m "first" && + git rbr --no-reviewby HEAD^ && + test_commit_message HEAD expected-unreviewed +' + +test_expect_success 'rebase --exec --reviewby adds a Reviewed-by line' ' + test_when_finished "rm exec" && + git rebase --exec "touch exec" --reviewby first^ first && + test_path_is_file exec && + test_commit_message HEAD expected-reviewed +' + +test_expect_success 'rebase --root --reviewby adds a Reviewed-by line' ' + git checkout first && + git rebase --root --keep-empty --reviewby && + test_commit_message HEAD^ expected-initial-reviewed && + test_commit_message HEAD expected-reviewed +' + +test_expect_success 'rebase -m --reviewby adds a Reviewed-by line' ' + test_must_fail git rebase -m --reviewby second third && + git checkout --theirs file && + git add file && + GIT_EDITOR="sed -n /Conflicts:/,/^\\\$/p >actual" \ + git rebase --continue && + cat >expect <<-\EOF && + # Conflicts: + # file + + EOF + test_cmp expect actual && + git log --format=%B -n3 >actual && + test_cmp expected-reviewed-conflict actual +' + +test_expect_success 'rebase -i --reviewby adds a Reviewed-by line when editing commit' ' + ( + set_fake_editor && + FAKE_LINES="edit 1 edit 3 edit 2" \ + git rebase -i --reviewby first third + ) && + echo a >a && + git add a && + test_must_fail git rebase --continue && + git checkout --ours file && + echo b >a && + git add a file && + git rebase --continue && + echo c >a && + git add a && + git log --format=%B -n3 >actual && + cat >expect <<-EOF && + conflict + + Reviewed-by: $ident + + third + + Reviewed-by: $ident + + file-2 + + Reviewed-by: $ident + + EOF + test_cmp expect actual +' + +test_done diff --git a/t/t4150-am.sh b/t/t4150-am.sh index 2ae93d3c96..40d6a2f43a 100755 --- a/t/t4150-am.sh +++ b/t/t4150-am.sh @@ -491,6 +491,7 @@ test_expect_success 'am changes committer and keeps author' ' "$(git log -1 --pretty=format:"%cn <%ce>" HEAD)" ' +########################## signoff begin ########################## test_expect_success 'am --signoff adds Signed-off-by: line' ' rm -fr .git/rebase-apply && git reset --hard && @@ -572,6 +573,80 @@ test_expect_success 'am without --keep removes Re: and [PATCH] stuff' ' git rev-parse topic_2 >actual && test_cmp expected actual ' +########################## signoff end ########################## + +########################## review begin ########################## +test_expect_success 'am --review adds Reviewed-by: line' ' + rm -fr .git/rebase-apply && + git reset --hard && + git checkout -b topic_3 first && + git am --reviewby <patch2 && + { + printf "third\n\nReviewed-by: %s <%s>\n\n" \ + "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" && + cat msg && + printf "Reviewed-by: %s <%s>\n\n" \ + "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" + } >expected-log && + git log --pretty=%B -2 HEAD >actual && + test_cmp expected-log actual +' +test_expect_success 'am stays in branch' ' + echo refs/heads/topic_3 >expected && + git symbolic-ref HEAD >actual && + test_cmp expected actual +' + +test_expect_success 'am --reviewby does not add Reviewed-by: line if already there' ' + git format-patch --stdout first >patch3 && + git reset --hard first && + git am --reviewby <patch3 && + git log --pretty=%B -2 HEAD >actual && + test_cmp expected-log actual +' + +test_expect_success 'am --reviewby adds Reviewed-by: if another author is preset' ' + NAME="A N Other" && + EMAIL="a.n.other@example.com" && + { + printf "third\n\nReviewed-by: %s <%s>\nReviewed-by: %s <%s>\n\n" \ + "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" \ + "$NAME" "$EMAIL" && + cat msg && + printf "Reviewed-by: %s <%s>\nReviewed-by: %s <%s>\n\n" \ + "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" \ + "$NAME" "$EMAIL" + } >expected-log && + git reset --hard first && + GIT_COMMITTER_NAME="$NAME" GIT_COMMITTER_EMAIL="$EMAIL" \ + git am --reviewby <patch3 && + git log --pretty=%B -2 HEAD >actual && + test_cmp expected-log actual +' + +test_expect_success 'am --reviewby duplicates Reviewed-by: if it is not the last one' ' + NAME="A N Other" && + EMAIL="a.n.other@example.com" && + { + printf "third\n\nReviewed-by: %s <%s>\n\ +Reviewed-by: %s <%s>\nReviewed-by: %s <%s>\n\n" \ + "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" \ + "$NAME" "$EMAIL" \ + "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" && + cat msg && + printf "Reviewed-by: %s <%s>\nReviewed-by: %s <%s>\n\ +Reviewed-by: %s <%s>\n\n" \ + "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" \ + "$NAME" "$EMAIL" \ + "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" + } >expected-log && + git format-patch --stdout first >patch3 && + git reset --hard first && + git am --reviewby <patch3 && + git log --pretty=%B -2 HEAD >actual && + test_cmp expected-log actual +' +########################## review end ########################## test_expect_success 'am --keep really keeps the subject' ' rm -fr .git/rebase-apply && -- 2.49.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/2] rebase, am: add --reviewby option 2025-05-06 12:58 ` [RFC PATCH 1/2] rebase, am: add --reviewby option Li Chen @ 2025-05-06 18:07 ` Junio C Hamano 2025-05-07 6:46 ` Li Chen 0 siblings, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2025-05-06 18:07 UTC (permalink / raw) To: Li Chen; +Cc: git Li Chen <me@linux.beauty> writes: > From: Li Chen <chenl311@chinatelecom.cn> > > Introduce a new `--reviewby` flag to `git rebase` and `git am` that appends a Shouldn't this (and possibly the other one, I didn't look at the patch text) be using the internal machinery used by interpret-trailers so that we can do the usual "do not duplicated existing ones", "append only at the last one is different" etc.? I also do not think we want to see one option (like the above) for each trailer elements (like "Reviewed-by") that is commonly used, which would lead us to adding "--helped-by", "--acked-by", etc. to complement this one. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/2] rebase, am: add --reviewby option 2025-05-06 18:07 ` Junio C Hamano @ 2025-05-07 6:46 ` Li Chen 2025-05-07 10:17 ` Phillip Wood 0 siblings, 1 reply; 16+ messages in thread From: Li Chen @ 2025-05-07 6:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Hi Junio, Thank you for the feedback. My current understanding is as below, please correct me if I am mistaken ---- On Wed, 07 May 2025 02:07:46 +0800 Junio C Hamano <gitster@pobox.com> wrote --- > Li Chen <me@linux.beauty> writes: > > > From: Li Chen <chenl311@chinatelecom.cn> > > > > Introduce a new `--reviewby` flag to `git rebase` and `git am` that appends a > > Shouldn't this (and possibly the other one, I didn't look at the > patch text) be using the internal machinery used by interpret-trailers > so that we can do the usual "do not duplicated existing ones", > "append only at the last one is different" etc.? At the moment, git-interpret‑trailers only works on a file or on stdin. During an interactive/merge rebase we change it on the fly without format-patch && am, which is very convenient. The new --trailer option already re‑uses amend_file_with_trailers() from trailer.c, so there is very little trailer‑specific code of my own. Right now --reviewby mirrors the existing --signoff implementation, and append_signoff() itself does not use the trailer library. If you are open to keeping a dedicated --reviewby, I can send a follow‑up (or roll it into this series) that moves both sign‑off and review‑by to the common trailer helpers. > I also do not think we want to see one option (like the above) for > each trailer elements (like "Reviewed-by") that is commonly used, > which would lead us to adding "--helped-by", "--acked-by", etc. to > complement this one. > Some projects require every commit to carry a Reviewed-by: line for accountability, much like the kernel requires Signed-off-by:. A first‑class option keeps that workflow “out of the box”; otherwise people need to define an alias such as [alias] rbr = rebase --trailer "Reviewed-by: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>" which is functional but less convenient. I would appreciate your further thoughts on whether a dedicated flag(--reviewby) is acceptable, or whether we should drop it and rely solely on the generic --trailer interface. Thanks again for the review. Regards, Li ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/2] rebase, am: add --reviewby option 2025-05-07 6:46 ` Li Chen @ 2025-05-07 10:17 ` Phillip Wood 2025-05-07 10:26 ` Phillip Wood 2025-05-07 17:39 ` Junio C Hamano 0 siblings, 2 replies; 16+ messages in thread From: Phillip Wood @ 2025-05-07 10:17 UTC (permalink / raw) To: Li Chen, Junio C Hamano; +Cc: git Hi Li On 07/05/2025 07:46, Li Chen wrote: > > Some projects require every commit to carry a Reviewed-by: line > for accountability, much like the kernel requires Signed-off-by:. > A first‑class option keeps that workflow “out of the box”; otherwise > people need to define an alias such as > > [alias] > rbr = rebase --trailer "Reviewed-by: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>" > > which is functional but less convenient. > > I would appreciate your further thoughts on whether a dedicated > flag(--reviewby) is acceptable, or whether we should drop it and rely solely on > the generic --trailer interface. I think adding support for --trailer is a good idea and if we do that we don't need --reviewby. The existence and implementation of --signoff is largely a historical artifact - I'm not sure we'd make the same choices if we were thinking about adding it today. Different projects have different requirements and I don't think it is sensible to add a new option catering to the different demands of each project. I'll take a proper look at the second patch tomorrow. Best Wishes Phillip > Thanks again for the review. > > Regards, > Li > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/2] rebase, am: add --reviewby option 2025-05-07 10:17 ` Phillip Wood @ 2025-05-07 10:26 ` Phillip Wood 2025-05-07 10:39 ` Kristoffer Haugsbakk 2025-05-07 17:39 ` Junio C Hamano 1 sibling, 1 reply; 16+ messages in thread From: Phillip Wood @ 2025-05-07 10:26 UTC (permalink / raw) To: Li Chen, Junio C Hamano; +Cc: git On 07/05/2025 11:17, Phillip Wood wrote: > Hi Li > > On 07/05/2025 07:46, Li Chen wrote: >> >> Some projects require every commit to carry a Reviewed-by: line >> for accountability, much like the kernel requires Signed-off-by:. >> A first‑class option keeps that workflow “out of the box”; otherwise >> people need to define an alias such as >> >> [alias] >> rbr = rebase --trailer "Reviewed-by: $GIT_AUTHOR_NAME >> <$GIT_AUTHOR_EMAIL>" >> >> which is functional but less convenient. >> >> I would appreciate your further thoughts on whether a dedicated >> flag(--reviewby) is acceptable, or whether we should drop it and rely >> solely on >> the generic --trailer interface. > > I think adding support for --trailer is a good idea and if we do that we > don't need --reviewby. The existence and implementation of --signoff is > largely a historical artifact - I'm not sure we'd make the same choices > if we were thinking about adding it today. Different projects have > different requirements and I don't think it is sensible to add a new > option catering to the different demands of each project. It might be worth thinking about how we could extend the trailer option so that it uses the committer identity if there is no value specified which would reduce the pain of adding things like Reviewed-by: Best Wishes Phillip > I'll take a proper look at the second patch tomorrow. > > Best Wishes > > Phillip > >> Thanks again for the review. >> >> Regards, >> Li >> > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/2] rebase, am: add --reviewby option 2025-05-07 10:26 ` Phillip Wood @ 2025-05-07 10:39 ` Kristoffer Haugsbakk 2025-05-07 13:38 ` Phillip Wood 0 siblings, 1 reply; 16+ messages in thread From: Kristoffer Haugsbakk @ 2025-05-07 10:39 UTC (permalink / raw) To: Phillip Wood, Li Chen, Junio C Hamano; +Cc: git On Wed, May 7, 2025, at 12:26, Phillip Wood wrote: > On 07/05/2025 11:17, Phillip Wood wrote: >> Hi Li >> >> On 07/05/2025 07:46, Li Chen wrote: >>> >>> Some projects require every commit to carry a Reviewed-by: line >>> for accountability, much like the kernel requires Signed-off-by:. >>> A first‑class option keeps that workflow “out of the box”; otherwise >>> people need to define an alias such as >>> >>> [alias] >>> rbr = rebase --trailer "Reviewed-by: $GIT_AUTHOR_NAME >>> <$GIT_AUTHOR_EMAIL>" >>> >>> which is functional but less convenient. >>> >>> I would appreciate your further thoughts on whether a dedicated >>> flag(--reviewby) is acceptable, or whether we should drop it and rely >>> solely on >>> the generic --trailer interface. >> >> I think adding support for --trailer is a good idea and if we do that we >> don't need --reviewby. The existence and implementation of --signoff is >> largely a historical artifact - I'm not sure we'd make the same choices >> if we were thinking about adding it today. Different projects have >> different requirements and I don't think it is sensible to add a new >> option catering to the different demands of each project. > > It might be worth thinking about how we could extend the trailer option > so that it uses the committer identity if there is no value specified > which would reduce the pain of adding things like Reviewed-by: That could be confusing for people who use trailers for non-ident metadata. I was wondering if `git var GIT_COMMITTER_IDENT` could be used. But that prints a Unix timestamp with timezone as well. (I don’t really understand why after reading that part of the manual) -- Kristoffer Haugsbakk ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/2] rebase, am: add --reviewby option 2025-05-07 10:39 ` Kristoffer Haugsbakk @ 2025-05-07 13:38 ` Phillip Wood 0 siblings, 0 replies; 16+ messages in thread From: Phillip Wood @ 2025-05-07 13:38 UTC (permalink / raw) To: Kristoffer Haugsbakk, Li Chen, Junio C Hamano; +Cc: git On 07/05/2025 11:39, Kristoffer Haugsbakk wrote: > On Wed, May 7, 2025, at 12:26, Phillip Wood wrote: > >> It might be worth thinking about how we could extend the trailer option >> so that it uses the committer identity if there is no value specified >> which would reduce the pain of adding things like Reviewed-by: > > That could be confusing for people who use trailers for > non-ident metadata. Yes we need to think about how to do it in a way that is helpful for things like Reviewed-by: or Tested-by: but does not break other workflows. One possibility would be to have a trailer.<name>.defaultValue config key but I'd be interested in other people's ideas. One other idea is that "git commit --author Kristoffer" will search recent commits for matching author to obtain your full name and email address. Something like that could be useful for expanding values --tailer=Co-Authored-by" or perhaps we could teach the completion script to suggest suitable completions based on some config describing the values we expect for certain keys. > I was wondering if `git var GIT_COMMITTER_IDENT` could be used. But > that prints a Unix timestamp with timezone as well. (I don’t really > understand why after reading that part of the manual) > I think it is because with define GIT_*_IDENT to include the timestamp to match the ident lines in commit and tag objects. You can always trim the timestamp appending something like "| sed 's/[^>]*$//'". Best Wishes Phillip ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/2] rebase, am: add --reviewby option 2025-05-07 10:17 ` Phillip Wood 2025-05-07 10:26 ` Phillip Wood @ 2025-05-07 17:39 ` Junio C Hamano 1 sibling, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2025-05-07 17:39 UTC (permalink / raw) To: Phillip Wood; +Cc: Li Chen, git Phillip Wood <phillip.wood123@gmail.com> writes: > .... The existence and implementation of > --signoff is largely a historical artifact - I'm not sure we'd make > the same choices if we were thinking about adding it today. Very well said. > I'll take a proper look at the second patch tomorrow. Thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 2/2] rebase: support --trailer 2025-05-06 12:57 [RFC PATCH 0/2] rebase: support --trailer and add --reviewby Li Chen 2025-05-06 12:58 ` [RFC PATCH 1/2] rebase, am: add --reviewby option Li Chen @ 2025-05-06 12:58 ` Li Chen 2025-05-08 14:17 ` Phillip Wood 1 sibling, 1 reply; 16+ messages in thread From: Li Chen @ 2025-05-06 12:58 UTC (permalink / raw) To: git From: Li Chen <chenl311@chinatelecom.cn> Implement a new `--trailer <text>` option for `git rebase` (support merge backend only now), which appends arbitrary trailer lines to each rebased commit message. Reject early if used with the apply backend (git am) since it lacks message‑filter/trailer hook. Automatically set REBASE_FORCE when any trailer is supplied. Reviewed-by: Li Chen <chenl311@chinatelecom.cn> --- Documentation/git-rebase.adoc | 8 +++ builtin/rebase.c | 70 ++++++++++++++++++++++ sequencer.c | 13 ++++ sequencer.h | 2 + t/meson.build | 1 + t/t3440-rebase-trailer.sh | 108 ++++++++++++++++++++++++++++++++++ 6 files changed, 202 insertions(+) create mode 100755 t/t3440-rebase-trailer.sh diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc index 1f3d152035..757016e529 100644 --- a/Documentation/git-rebase.adoc +++ b/Documentation/git-rebase.adoc @@ -525,6 +525,14 @@ See also INCOMPATIBLE OPTIONS below. Append a `Reviewed-by:` trailer whose value is taken from the current committer identity, exactly like `--signoff` appends a Signed-off-by:` trailer. + +--trailer <trailer>:: + Append the given trailer line(s) to every rebased commit + message, processed via linkgit:git-interpret-trailers[1]. + When this option is present *rebase automatically enables* + `--force-rebase` so that fast‑forwarded commits are also + rewritten. + See also INCOMPATIBLE OPTIONS below. -i:: diff --git a/builtin/rebase.c b/builtin/rebase.c index b288aedfb1..df65a1e040 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -36,6 +36,9 @@ #include "reset.h" #include "trace2.h" #include "hook.h" +#include "trailer.h" + +static const char trailer_state_name[] = "trailer"; static char const * const builtin_rebase_usage[] = { N_("git rebase [-i] [options] [--exec <cmd>] " @@ -46,6 +49,8 @@ static char const * const builtin_rebase_usage[] = { NULL }; +static struct strvec trailer_args = STRVEC_INIT; + static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto") static GIT_PATH_FUNC(path_interactive, "rebase-merge/interactive") static GIT_PATH_FUNC(apply_dir, "rebase-apply") @@ -114,6 +119,7 @@ struct rebase_options { char *reflog_action; int signoff; int reviewby; + struct strvec trailer_args; int allow_rerere_autoupdate; int keep_empty; int autosquash; @@ -144,6 +150,7 @@ struct rebase_options { .flags = REBASE_NO_QUIET, \ .git_am_opts = STRVEC_INIT, \ .exec = STRING_LIST_INIT_NODUP, \ + .trailer_args = STRVEC_INIT, \ .git_format_patch_opt = STRBUF_INIT, \ .fork_point = -1, \ .reapply_cherry_picks = -1, \ @@ -167,6 +174,7 @@ static void rebase_options_release(struct rebase_options *opts) free(opts->strategy); string_list_clear(&opts->strategy_opts, 0); strbuf_release(&opts->git_format_patch_opt); + strvec_clear(&opts->trailer_args); } static struct replay_opts get_replay_opts(const struct rebase_options *opts) @@ -179,6 +187,10 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts) replay.signoff = opts->signoff; replay.reviewby = opts->reviewby; + + for (size_t i = 0; i < opts->trailer_args.nr; i++) + strvec_push(&replay.trailer_args, opts->trailer_args.v[i]); + replay.allow_ff = !(opts->flags & REBASE_FORCE); if (opts->allow_rerere_autoupdate) replay.allow_rerere_auto = opts->allow_rerere_autoupdate; @@ -437,6 +449,8 @@ static int read_basic_state(struct rebase_options *opts) struct strbuf head_name = STRBUF_INIT; struct strbuf buf = STRBUF_INIT; struct object_id oid; + struct strbuf t = STRBUF_INIT, one = STRBUF_INIT; + const char *path = state_dir_path(trailer_state_name, opts); if (!read_oneliner(&head_name, state_dir_path("head-name", opts), READ_ONELINER_WARN_MISSING) || @@ -509,6 +523,22 @@ static int read_basic_state(struct rebase_options *opts) strbuf_release(&buf); + if (strbuf_read_file(&t, path, 0) >= 0) { + const char *p = t.buf, *end = t.buf + t.len; + + while (p < end) { + const char *nl = memchr(p, '\n', end - p); + strbuf_reset(&one); + strbuf_add(&one, p, nl ? nl - p : end - p); + if (one.len) /* skip empty line */ + strvec_push(&opts->trailer_args, + strbuf_detach(&one, NULL)); + p = nl ? nl + 1 : end; + } + strbuf_release(&one); + } + strbuf_release(&t); + return 0; } @@ -537,6 +567,28 @@ static int rebase_write_basic_state(struct rebase_options *opts) if (opts->reviewby) write_file(state_dir_path("reviewby", opts), "--reviewby"); + /* + * save opts->trailer_args into state_dir/trailer + */ + if (opts->trailer_args.nr) { + struct strbuf buf = STRBUF_INIT; + size_t i; + + for (i = 0; i < opts->trailer_args.nr; i++) { + strbuf_addstr(&buf, opts->trailer_args.v[i]); + strbuf_addch(&buf, '\n'); + } + write_file(state_dir_path(trailer_state_name, opts), + "%s", buf.buf); + strbuf_release(&buf); + } else { + /* + * but if rebase doesn't pass any --trailer， + * and state dir still have residual files，let's delete it。 + */ + unlink_or_warn(state_dir_path(trailer_state_name, opts)); + } + return 0; } @@ -1140,6 +1192,7 @@ int cmd_rebase(int argc, .flags = PARSE_OPT_NOARG, .defval = REBASE_DIFFSTAT, }, + OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG), OPT_BOOL(0, "signoff", &options.signoff, N_("add a Signed-off-by trailer to each commit")), OPT_BOOL(0, "reviewby", &options.reviewby, @@ -1292,6 +1345,13 @@ int cmd_rebase(int argc, builtin_rebase_options, builtin_rebase_usage, 0); + for (i = 0; i < trailer_args.nr; i++) + strvec_push(&options.trailer_args, trailer_args.v[i]); + + /* if add --trailer，force rebase */ + if (options.trailer_args.nr) + options.flags |= REBASE_FORCE; + if (preserve_merges_selected) die(_("--preserve-merges was replaced by --rebase-merges\n" "Note: Your `pull.rebase` configuration may also be set to 'preserve',\n" @@ -1549,6 +1609,16 @@ int cmd_rebase(int argc, if (options.root && !options.onto_name) imply_merge(&options, "--root without --onto"); + /* + * The apply‑based backend (git am) cannot append trailers because + * it lacks a message‑filter facility. Reject early, before any + * state (index, HEAD, etc.) is modified. + */ + if (options.type == REBASE_APPLY && options.trailer_args.nr) + die(_("the --apply backend (git am) cannot currently handle " + "--trailer; please omit --apply or use " + "the merge/interactive backend")); + if (isatty(2) && options.flags & REBASE_NO_QUIET) strbuf_addstr(&options.git_format_patch_opt, " --progress"); diff --git a/sequencer.c b/sequencer.c index 01933882ed..b61c668c39 100644 --- a/sequencer.c +++ b/sequencer.c @@ -429,6 +429,7 @@ void replay_opts_release(struct replay_opts *opts) free(opts->revs); replay_ctx_release(ctx); free(opts->ctx); + strvec_clear(&opts->trailer_args); } int sequencer_remove_state(struct replay_opts *opts) @@ -2506,6 +2507,18 @@ static int do_pick_commit(struct repository *r, _("dropping %s %s -- patch contents already upstream\n"), oid_to_hex(&commit->object.oid), msg.subject); } /* else allow == 0 and there's nothing special to do */ + + if (!res && opts->trailer_args.nr && !drop_commit) { + const char *trailer_file = + msg_file ? msg_file : git_path_merge_msg(r); + + if (amend_file_with_trailers(trailer_file, + &opts->trailer_args)) { + res = error(_("unable to add trailers to commit message")); + goto leave; + } + } + if (!opts->no_commit && !drop_commit) { if (author || command == TODO_REVERT || (flags & AMEND_MSG)) res = do_commit(r, msg_file, author, opts, flags, diff --git a/sequencer.h b/sequencer.h index 82b79fd1e8..4f5ea2d818 100644 --- a/sequencer.h +++ b/sequencer.h @@ -46,6 +46,7 @@ struct replay_opts { int no_commit; int signoff; int reviewby; + struct strvec trailer_args; int allow_ff; int allow_rerere_auto; int allow_empty; @@ -88,6 +89,7 @@ struct replay_opts { .action = -1, \ .xopts = STRVEC_INIT, \ .ctx = replay_ctx_new(), \ + .trailer_args = STRVEC_INIT, \ } /* diff --git a/t/meson.build b/t/meson.build index 327fa461fd..fc4b64fca1 100644 --- a/t/meson.build +++ b/t/meson.build @@ -393,6 +393,7 @@ integration_tests = [ 't3437-rebase-fixup-options.sh', 't3438-rebase-broken-files.sh', 't3439-rebase-reviewby.sh', + 't3440-rebase-trailer.sh', 't3500-cherry.sh', 't3501-revert-cherry-pick.sh', 't3502-cherry-pick-merge.sh', diff --git a/t/t3440-rebase-trailer.sh b/t/t3440-rebase-trailer.sh new file mode 100755 index 0000000000..6dc08e9633 --- /dev/null +++ b/t/t3440-rebase-trailer.sh @@ -0,0 +1,108 @@ +#!/bin/sh +# + +test_description='git rebase --trailer integration tests +We verify that --trailer on the merge/interactive/exec/root backends, +and that it is rejected early when the apply backend is requested.' + +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + +. ./test-lib.sh +. "$TEST_DIRECTORY"/lib-rebase.sh # test_commit_message, helpers + +############################################################################ +# 1. repository setup +############################################################################ + +test_expect_success 'setup repo with a small history' ' + git commit --allow-empty -m "Initial empty commit" && + test_commit first file a && + test_commit second file && + git checkout -b conflict-branch first && + test_commit file-2 file-2 && + test_commit conflict file && + test_commit third file && + ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" +' + +create_expect () { + cat >"$1" <<-EOF + $2 + + Reviewed-by: Dev <dev@example.com> + EOF +} +# golden files: +create_expect initial-signed "Initial empty commit" +create_expect first-signed "first" +create_expect second-signed "second" +create_expect file2-signed "file-2" +create_expect third-signed "third" +create_expect conflict-signed "conflict" + +############################################################################ +# 2. explicitly reject --apply + --trailer +############################################################################ + +test_expect_success 'apply backend is rejected when --trailer is given' ' + git reset --hard third && + git tag before-apply && + test_expect_code 128 \ + git rebase --apply --trailer "Reviewed-by: Dev <dev@example.com>" \ + HEAD^ && + git diff --quiet before-apply..HEAD # prove nothing moved +' + +############################################################################ +# 3. --no‑op: same range, no changes +############################################################################ + +test_expect_success '--trailer without range change is a no‑op' ' + git checkout main && + git tag before-noop && + git rebase --trailer "Reviewed-by: Dev <dev@example.com>" HEAD && + git diff --quiet before-noop..HEAD +' + +############################################################################ +# 4. merge backend (-m), conflict resolution path +############################################################################ + +test_expect_success 'rebase -m --trailer adds trailer after conflicts' ' + git reset --hard third && + test_must_fail git rebase -m \ + --trailer "Reviewed-by: Dev <dev@example.com>" \ + second third && + git checkout --theirs file && + git add file && + GIT_EDITOR=: git rebase --continue && + git show --no-patch --pretty=format:%B HEAD~2 >actual && + test_cmp file2-signed actual +' + +############################################################################ +# 5. --exec path +############################################################################ + +test_expect_success 'rebase --exec --trailer adds trailer' ' + test_when_finished "rm -f touched" && + git rebase --exec "touch touched" \ + --trailer "Reviewed-by: Dev <dev@example.com>" \ + first^ first && + test_path_is_file touched && + test_commit_message HEAD first-signed +' + +############################################################################ +# 6. --root path +############################################################################ + +test_expect_success 'rebase --root --trailer updates every commit' ' + git checkout first && + git rebase --root --keep-empty \ + --trailer "Reviewed-by: Dev <dev@example.com>" && + test_commit_message HEAD first-signed && + test_commit_message HEAD^ initial-signed +' +test_done -- 2.49.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/2] rebase: support --trailer 2025-05-06 12:58 ` [RFC PATCH 2/2] rebase: support --trailer Li Chen @ 2025-05-08 14:17 ` Phillip Wood 2025-05-08 15:55 ` Li Chen 2025-05-16 5:42 ` Li Chen 0 siblings, 2 replies; 16+ messages in thread From: Phillip Wood @ 2025-05-08 14:17 UTC (permalink / raw) To: Li Chen, git; +Cc: Junio C Hamano Hi Li On 06/05/2025 13:58, Li Chen wrote: > From: Li Chen <chenl311@chinatelecom.cn> > > Implement a new `--trailer <text>` option for `git rebase` > (support merge backend only now), which appends arbitrary > trailer lines to each rebased commit message. Reject early > if used with the apply backend (git am) since it lacks > message‑filter/trailer hook. Automatically set REBASE_FORCE when > any trailer is supplied. I think this is a reasonable idea but unfortunately I think the trailer API needs improving so that the implementation (a) Checks the trailers given on the command-line before the user edits the todo list. That way we reject invalid trailers and exit before the user has spent any effort editing the todo list. (b) Does not fork another process to add the trailers. Without this the performance is going to suffer. Hopefully it wont be too difficult to modify the existing code to take a struct strbuf and a list of trailers to append to it. (c) Only adds the trailers on the commandline. I'm a bit confused by the various trailer config options - the man page reads to me like "git interpret-trailers" can add missing trailers that are configured but not passed on the commandline. The changes to the trailer api should be made in one or more preparatory commits before adding support for --trailer to "git rebase" I've left some comments on the changes to builtin/rebase.c and the tests, I've skipped the changes to sequencer.c for now as they'll have to be updated to avoid forking "git interpret-trailers" > diff --git a/builtin/rebase.c b/builtin/rebase.c > index b288aedfb1..df65a1e040 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -36,6 +36,9 @@ > #include "reset.h" > #include "trace2.h" > #include "hook.h" > +#include "trailer.h" > + > +static const char trailer_state_name[] = "trailer"; > > static char const * const builtin_rebase_usage[] = { > N_("git rebase [-i] [options] [--exec <cmd>] " > @@ -46,6 +49,8 @@ static char const * const builtin_rebase_usage[] = { > NULL > }; > > +static struct strvec trailer_args = STRVEC_INIT; We should not need to add any file scope variables > static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto") > static GIT_PATH_FUNC(path_interactive, "rebase-merge/interactive") > static GIT_PATH_FUNC(apply_dir, "rebase-apply") > @@ -114,6 +119,7 @@ struct rebase_options { > char *reflog_action; > int signoff; > int reviewby; > + struct strvec trailer_args; This is good and means we don't need the file-scope declaration above. > int allow_rerere_autoupdate; > int keep_empty; > int autosquash; > @@ -144,6 +150,7 @@ struct rebase_options { > .flags = REBASE_NO_QUIET, \ > .git_am_opts = STRVEC_INIT, \ > .exec = STRING_LIST_INIT_NODUP, \ > + .trailer_args = STRVEC_INIT, \ > .git_format_patch_opt = STRBUF_INIT, \ > .fork_point = -1, \ > .reapply_cherry_picks = -1, \ > @@ -167,6 +174,7 @@ static void rebase_options_release(struct rebase_options *opts) > free(opts->strategy); > string_list_clear(&opts->strategy_opts, 0); > strbuf_release(&opts->git_format_patch_opt); > + strvec_clear(&opts->trailer_args); > } > > static struct replay_opts get_replay_opts(const struct rebase_options *opts) > @@ -179,6 +187,10 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts) > > replay.signoff = opts->signoff; > replay.reviewby = opts->reviewby; > + > + for (size_t i = 0; i < opts->trailer_args.nr; i++) > + strvec_push(&replay.trailer_args, opts->trailer_args.v[i]); The indentation is off here > + > replay.allow_ff = !(opts->flags & REBASE_FORCE); > if (opts->allow_rerere_autoupdate) > replay.allow_rerere_auto = opts->allow_rerere_autoupdate; > @@ -437,6 +449,8 @@ static int read_basic_state(struct rebase_options *opts) > struct strbuf head_name = STRBUF_INIT; > struct strbuf buf = STRBUF_INIT; > struct object_id oid; > + struct strbuf t = STRBUF_INIT, one = STRBUF_INIT; You can just use the existing "buf" instead of adding "t" and "one" is unneeded (see below) > + const char *path = state_dir_path(trailer_state_name, opts); > > if (!read_oneliner(&head_name, state_dir_path("head-name", opts), > READ_ONELINER_WARN_MISSING) || > @@ -509,6 +523,22 @@ static int read_basic_state(struct rebase_options *opts) > > strbuf_release(&buf); > > + if (strbuf_read_file(&t, path, 0) >= 0) { > + const char *p = t.buf, *end = t.buf + t.len; > + > + while (p < end) { > + const char *nl = memchr(p, '\n', end - p); We expect each line to be terminated with '\n' so we should die if nl is NULL > + strbuf_reset(&one); > + strbuf_add(&one, p, nl ? nl - p : end - p); Instead of copying the string into an strbuf we can just do "*nl == '\0';" > + if (one.len) /* skip empty line */ There should not be any empty lines so we should die here if it is empty > + strvec_push(&opts->trailer_args, > + strbuf_detach(&one, NULL)); > + p = nl ? nl + 1 : end; > + } > + strbuf_release(&one); > + } > + strbuf_release(&t); > + > return 0; > } > > @@ -537,6 +567,28 @@ static int rebase_write_basic_state(struct rebase_options *opts) > if (opts->reviewby) > write_file(state_dir_path("reviewby", opts), "--reviewby"); > > + /* > + * save opts->trailer_args into state_dir/trailer > + */ > + if (opts->trailer_args.nr) { > + struct strbuf buf = STRBUF_INIT; > + size_t i; > + > + for (i = 0; i < opts->trailer_args.nr; i++) { > + strbuf_addstr(&buf, opts->trailer_args.v[i]); > + strbuf_addch(&buf, '\n'); > + } > + write_file(state_dir_path(trailer_state_name, opts), > + "%s", buf.buf); > + strbuf_release(&buf); This looks good > + } else { > + /* > + * but if rebase doesn't pass any --trailer， > + * and state dir still have residual files，let's delete it。 > + */ This should never happen should it? > + unlink_or_warn(state_dir_path(trailer_state_name, opts)); > + } > + > return 0; > } > > @@ -1140,6 +1192,7 @@ int cmd_rebase(int argc, > .flags = PARSE_OPT_NOARG, > .defval = REBASE_DIFFSTAT, > }, > + OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG), This should be OPT_STRVEC() and should populate &options.trailers > OPT_BOOL(0, "signoff", &options.signoff, > N_("add a Signed-off-by trailer to each commit")), > OPT_BOOL(0, "reviewby", &options.reviewby, > @@ -1292,6 +1345,13 @@ int cmd_rebase(int argc, > builtin_rebase_options, > builtin_rebase_usage, 0); > > + for (i = 0; i < trailer_args.nr; i++) > + strvec_push(&options.trailer_args, trailer_args.v[i]); > + > + /* if add --trailer，force rebase */ > + if (options.trailer_args.nr) > + options.flags |= REBASE_FORCE; This is good > if (preserve_merges_selected) > die(_("--preserve-merges was replaced by --rebase-merges\n" > "Note: Your `pull.rebase` configuration may also be set to 'preserve',\n" > @@ -1549,6 +1609,16 @@ int cmd_rebase(int argc, > if (options.root && !options.onto_name) > imply_merge(&options, "--root without --onto"); > > + /* > + * The apply‑based backend (git am) cannot append trailers because > + * it lacks a message‑filter facility. Reject early, before any > + * state (index, HEAD, etc.) is modified. > + */ > + if (options.type == REBASE_APPLY && options.trailer_args.nr) > + die(_("the --apply backend (git am) cannot currently handle " > + "--trailer; please omit --apply or use " > + "the merge/interactive backend")); It's good you're checking this but we should call imply_merge() to do that instead. > +test_description='git rebase --trailer integration tests > +We verify that --trailer on the merge/interactive/exec/root backends, > +and that it is rejected early when the apply backend is requested.' > + > +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main > +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > + > +. ./test-lib.sh > +. "$TEST_DIRECTORY"/lib-rebase.sh # test_commit_message, helpers > + > +############################################################################ > +# 1. repository setup > +############################################################################ > + > +test_expect_success 'setup repo with a small history' ' > + git commit --allow-empty -m "Initial empty commit" && > + test_commit first file a && > + test_commit second file && > + git checkout -b conflict-branch first && > + test_commit file-2 file-2 && > + test_commit conflict file && > + test_commit third file && The alignment is off here - it would be much simpler just to separate each argument with a single space > + ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" > +' > + > +create_expect () { > + cat >"$1" <<-EOF > + $2 > + > + Reviewed-by: Dev <dev@example.com> > + EOF > +} > +# golden files: > +create_expect initial-signed "Initial empty commit" > +create_expect first-signed "first" > +create_expect second-signed "second" > +create_expect file2-signed "file-2" > +create_expect third-signed "third" > +create_expect conflict-signed "conflict" Thanks for adding these tests. This should be inside the setup test - noting should be run outside of test_expect_success() > + > +############################################################################ > +# 2. explicitly reject --apply + --trailer > +############################################################################ > + > +test_expect_success 'apply backend is rejected when --trailer is given' ' > + git reset --hard third && > + git tag before-apply && > + test_expect_code 128 \ > + git rebase --apply --trailer "Reviewed-by: Dev <dev@example.com>" \ > + HEAD^ && We should check the error message here by redirecting stderr to a file and checking that with test_grep or test_cmp so we can be confidant that rebase is failing for the reason it should be and not some other bug. > + git diff --quiet before-apply..HEAD # prove nothing moved If you want to check that HEAD is unchanged I'd run "head=$(git show-ref --verify -s HEAD)" before the rebase and "test_cmp_rev HEAD $head" afterwards. > +' > + > +############################################################################ > +# 3. --no‑op: same range, no changes > +############################################################################ > + > +test_expect_success '--trailer without range change is a no‑op' ' > + git checkout main && > + git tag before-noop && > + git rebase --trailer "Reviewed-by: Dev <dev@example.com>" HEAD && > + git diff --quiet before-noop..HEAD Do we really need this - it isn't really checking anything to do with the implementation of --trailer. > + > +############################################################################ > +# 4. merge backend (-m), conflict resolution path > +############################################################################ Excellent > + > +test_expect_success 'rebase -m --trailer adds trailer after conflicts' ' > + git reset --hard third && > + test_must_fail git rebase -m \ > + --trailer "Reviewed-by: Dev <dev@example.com>" \ > + second third && > + git checkout --theirs file && > + git add file && > + GIT_EDITOR=: git rebase --continue && GIT_EDITOR=: unless you call test_set_editor() which should always be done in a subshell so this isn't needed. > + git show --no-patch --pretty=format:%B HEAD~2 >actual && > + test_cmp file2-signed actual I think test_commit_message would help simplify this > +' > + > +############################################################################ > +# 5. --exec path > +############################################################################ > + > +test_expect_success 'rebase --exec --trailer adds trailer' ' > + test_when_finished "rm -f touched" && > + git rebase --exec "touch touched" \ > + --trailer "Reviewed-by: Dev <dev@example.com>" \ > + first^ first && > + test_path_is_file touched && > + test_commit_message HEAD first-signed I'm not sure this has anything to do with --trailer > +' > + > +############################################################################ > +# 6. --root path > +############################################################################ > + > +test_expect_success 'rebase --root --trailer updates every commit' ' > + git checkout first && > + git rebase --root --keep-empty \ > + --trailer "Reviewed-by: Dev <dev@example.com>" && > + test_commit_message HEAD first-signed && > + test_commit_message HEAD^ initial-signed This test looks good. Best Wishes Phillip > +' > +test_done G ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/2] rebase: support --trailer 2025-05-08 14:17 ` Phillip Wood @ 2025-05-08 15:55 ` Li Chen 2025-05-08 15:58 ` phillip.wood123 2025-05-08 16:29 ` Phillip Wood 2025-05-16 5:42 ` Li Chen 1 sibling, 2 replies; 16+ messages in thread From: Li Chen @ 2025-05-08 15:55 UTC (permalink / raw) To: phillipwood; +Cc: git, Junio C Hamano Hi Phillip, ---- On Thu, 08 May 2025 22:17:17 +0800 Phillip Wood <phillip.wood123@gmail.com> wrote --- > Hi Li > > On 06/05/2025 13:58, Li Chen wrote: > > From: Li Chen <chenl311@chinatelecom.cn> > > > > Implement a new `--trailer <text>` option for `git rebase` > > (support merge backend only now), which appends arbitrary > > trailer lines to each rebased commit message. Reject early > > if used with the apply backend (git am) since it lacks > > message‑filter/trailer hook. Automatically set REBASE_FORCE when > > any trailer is supplied. > > I think this is a reasonable idea but unfortunately I think the trailer > API needs improving so that the implementation > > (a) Checks the trailers given on the command-line before the user edits > the todo list. That way we reject invalid trailers and exit before the > user has spent any effort editing the todo list. > > (b) Does not fork another process to add the trailers. Without this the > performance is going to suffer. Hopefully it wont be too difficult to > modify the existing code to take a struct strbuf and a list of trailers > to append to it. > > (c) Only adds the trailers on the commandline. I'm a bit confused by the > various trailer config options - the man page reads to me like "git > interpret-trailers" can add missing trailers that are configured but not > passed on the commandline. > > The changes to the trailer api should be made in one or more preparatory > commits before adding support for --trailer to "git rebase" Thanks a lot for the detailed feedback and for outlining the gaps in the current trailer API – it really helps me see the next steps more clearly. I have one questions: Who should take the API work? I’m very happy to roll up my sleeves and prototype the improvements you described (reject invalid trailers up‑front, add an in‑process API that works on a struct strbuf, and ensure only the CLI‑supplied trailers are added). That would give me a chance to contribute a bit more deeply to Git. Of course, if you already have something in mind and would rather drive that part yourself, I’m equally happy to wait and re‑base my series on top of it. Please let me know which you prefer. > I've left some comments on the changes to builtin/rebase.c and the > tests, I've skipped the changes to sequencer.c for now as they'll have > to be updated to avoid forking "git interpret-trailers" Thanks for all your great reviews! I'll address all your reviews in next version. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/2] rebase: support --trailer 2025-05-08 15:55 ` Li Chen @ 2025-05-08 15:58 ` phillip.wood123 2025-05-08 16:29 ` Phillip Wood 1 sibling, 0 replies; 16+ messages in thread From: phillip.wood123 @ 2025-05-08 15:58 UTC (permalink / raw) To: Li Chen, phillipwood; +Cc: git, Junio C Hamano Hi Li On 08/05/2025 16:55, Li Chen wrote: > ---- On Thu, 08 May 2025 22:17:17 +0800 Phillip Wood <phillip.wood123@gmail.com> wrote --- > > On 06/05/2025 13:58, Li Chen wrote: > > > From: Li Chen <chenl311@chinatelecom.cn> > > > > > > Implement a new `--trailer <text>` option for `git rebase` > > > (support merge backend only now), which appends arbitrary > > > trailer lines to each rebased commit message. Reject early > > > if used with the apply backend (git am) since it lacks > > > message‑filter/trailer hook. Automatically set REBASE_FORCE when > > > any trailer is supplied. > > > > I think this is a reasonable idea but unfortunately I think the trailer > > API needs improving so that the implementation > > > > (a) Checks the trailers given on the command-line before the user edits > > the todo list. That way we reject invalid trailers and exit before the > > user has spent any effort editing the todo list. > > > > (b) Does not fork another process to add the trailers. Without this the > > performance is going to suffer. Hopefully it wont be too difficult to > > modify the existing code to take a struct strbuf and a list of trailers > > to append to it. > > > > (c) Only adds the trailers on the commandline. I'm a bit confused by the > > various trailer config options - the man page reads to me like "git > > interpret-trailers" can add missing trailers that are configured but not > > passed on the commandline. > > > > The changes to the trailer api should be made in one or more preparatory > > commits before adding support for --trailer to "git rebase" > > Thanks a lot for the detailed feedback and for outlining the gaps in the > current trailer API – it really helps me see the next steps more > clearly. > > I have one questions: > > Who should take the API work? > > I’m very happy to roll up my sleeves and prototype the improvements > you described (reject invalid trailers up‑front, add an in‑process API > that works on a struct strbuf, and ensure only the CLI‑supplied > trailers are added). That would give me a chance to contribute a bit > more deeply to Git. Of course, if you already have something in mind > and would rather drive that part yourself, I’m equally happy to wait > and re‑base my series on top of it. Please let me know which you > prefer. If you're interested in doing the work on the trailer api that would be great - I don't have time myself. Thanks Phillip > > I've left some comments on the changes to builtin/rebase.c and the > > tests, I've skipped the changes to sequencer.c for now as they'll have > > to be updated to avoid forking "git interpret-trailers" > > Thanks for all your great reviews! > > I'll address all your reviews in next version. > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/2] rebase: support --trailer 2025-05-08 15:55 ` Li Chen 2025-05-08 15:58 ` phillip.wood123 @ 2025-05-08 16:29 ` Phillip Wood 1 sibling, 0 replies; 16+ messages in thread From: Phillip Wood @ 2025-05-08 16:29 UTC (permalink / raw) To: Li Chen, phillipwood; +Cc: git, Junio C Hamano Hi Li On 08/05/2025 16:55, Li Chen wrote: > ---- On Thu, 08 May 2025 22:17:17 +0800 Phillip Wood <phillip.wood123@gmail.com> wrote --- > > > I've left some comments on the changes to builtin/rebase.c and the > > tests, I've skipped the changes to sequencer.c for now as they'll have > > to be updated to avoid forking "git interpret-trailers" > > Thanks for all your great reviews! > > I'll address all your reviews in next version. I forgot to say earlier that if you have any questions or queries about my review or the trailer code do feel free to ask. Best Wishes Phillip ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/2] rebase: support --trailer 2025-05-08 14:17 ` Phillip Wood 2025-05-08 15:55 ` Li Chen @ 2025-05-16 5:42 ` Li Chen 2025-05-16 10:04 ` Phillip Wood 1 sibling, 1 reply; 16+ messages in thread From: Li Chen @ 2025-05-16 5:42 UTC (permalink / raw) To: phillipwood; +Cc: git, Junio C Hamano Hi Phillip, ---- On Thu, 08 May 2025 22:17:17 +0800 Phillip Wood <phillip.wood123@gmail.com> wrote --- > Hi Li > > On 06/05/2025 13:58, Li Chen wrote: > > From: Li Chen <chenl311@chinatelecom.cn> > > > > Implement a new `--trailer <text>` option for `git rebase` > > (support merge backend only now), which appends arbitrary > > trailer lines to each rebased commit message. Reject early > > if used with the apply backend (git am) since it lacks > > message‑filter/trailer hook. Automatically set REBASE_FORCE when > > any trailer is supplied. > > I think this is a reasonable idea but unfortunately I think the trailer > API needs improving so that the implementation > > (a) ... > > (b) ... > > (c) Only adds the trailers on the commandline. I'm a bit confused by the > various trailer config options - the man page reads to me like "git > interpret-trailers" can add missing trailers that are configured but not > passed on the commandline. About part (c), just to be sure I understand correctly: Do you want the trailer implementation to completely drop any handling of trailer configuration (i.e. remove parse_trailers_from_config() and related config-based behavior from the codepath and man page/documents)? Or would you rather leave the config machinery in place, but have rebase --trailer explicitly ignore all trailer.* configuration and only append the exact trailers passed on its command line? Please let me know which you prefer (or if there’s a third path I’m missing) and I’ll add the patches accordingly. Regards, Li ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/2] rebase: support --trailer 2025-05-16 5:42 ` Li Chen @ 2025-05-16 10:04 ` Phillip Wood 0 siblings, 0 replies; 16+ messages in thread From: Phillip Wood @ 2025-05-16 10:04 UTC (permalink / raw) To: Li Chen, phillipwood; +Cc: git, Junio C Hamano Hi Li On 16/05/2025 06:42, Li Chen wrote: > Hi Phillip, > > ---- On Thu, 08 May 2025 22:17:17 +0800 Phillip Wood <phillip.wood123@gmail.com> wrote --- > > > > (c) Only adds the trailers on the commandline. I'm a bit confused by the > > various trailer config options - the man page reads to me like "git > > interpret-trailers" can add missing trailers that are configured but not > > passed on the commandline. > > About part (c), just to be sure I understand correctly: > > Do you want the trailer implementation to completely drop any handling of trailer configuration > (i.e. remove parse_trailers_from_config() and related config-based behavior from the codepath and man page/documents)? > > Or would you rather leave the config machinery in place, but have rebase --trailer explicitly > ignore all trailer.* configuration and only append the exact trailers passed on its command line? I think I had misunderstood what trailer.ifMissing did. I was concerned that it could add trailers that were not on the command-line but I don't think that's the case. We certainly want to respect the config for trailer.<alias>.key and trailer.<alias>.command as they make it possible for the user to set "trailer.review.key=Reviewed-by" and "trailer.review.command=git var GIT_COMMITTER_IDENT | sed 's/[^>]*$//' #" and then run "git rebase --trailer=review" to add their Reviewed-by: trailer. I think it makes sense to respect the other config as well - that does mean that trailers that the user passes on the command-line may not be added because they already exist or are configured not to be added if they are missing but is consistent with "git interpret-trailers". It means that the user can set "trailer.myKey.ifExists=doNothing" and then run "git rebase --trailer MyKey=value" to ensure all the commits have a MyKey trailer without duplicating it in the commits where it already exists. That's a long-winded way of saying that on reflection I think respecting the trailer config setting is the right thing to do after all. Best Wishes Phillip > Please let me know which you prefer (or if there’s a third path I’m missing) and I’ll add the patches accordingly. > > Regards, > Li ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2025-05-16 10:04 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-05-06 12:57 [RFC PATCH 0/2] rebase: support --trailer and add --reviewby Li Chen 2025-05-06 12:58 ` [RFC PATCH 1/2] rebase, am: add --reviewby option Li Chen 2025-05-06 18:07 ` Junio C Hamano 2025-05-07 6:46 ` Li Chen 2025-05-07 10:17 ` Phillip Wood 2025-05-07 10:26 ` Phillip Wood 2025-05-07 10:39 ` Kristoffer Haugsbakk 2025-05-07 13:38 ` Phillip Wood 2025-05-07 17:39 ` Junio C Hamano 2025-05-06 12:58 ` [RFC PATCH 2/2] rebase: support --trailer Li Chen 2025-05-08 14:17 ` Phillip Wood 2025-05-08 15:55 ` Li Chen 2025-05-08 15:58 ` phillip.wood123 2025-05-08 16:29 ` Phillip Wood 2025-05-16 5:42 ` Li Chen 2025-05-16 10:04 ` Phillip Wood
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).