* [PATCH v3 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers`
2025-08-03 15:00 [PATCH v3 0/2] rebase: support --trailer Li Chen
@ 2025-08-03 15:00 ` Li Chen
2025-08-05 13:17 ` Phillip Wood
2025-08-03 15:00 ` [PATCH v3 2/2] rebase: support --trailer Li Chen
2025-08-03 16:35 ` [PATCH v3 0/2] " Junio C Hamano
2 siblings, 1 reply; 15+ messages in thread
From: Li Chen @ 2025-08-03 15:00 UTC (permalink / raw)
To: phillipwood, git, Junio C Hamano
From: Li Chen <chenl311@chinatelecom.cn>
All trailer insertion now funnels through trailer_process():
* builtin/interpret-trailers.c is reduced to file I/O + a single call.
* amend_file_with_trailers() shares the same path; the old
amend_strbuf_with_trailers() helper is dropped.
* New helpers parse_trailer_args()/free_new_trailer_list() convert
--trailer=... strings to new_trailer_item lists.
Behaviour is unchanged; the full test-suite still passes, and the
fork/exec is gone.
Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
---
builtin/interpret-trailers.c | 117 ++++++++------------------------
trailer.c | 125 ++++++++++++++++++++++++++++++++---
trailer.h | 18 ++++-
3 files changed, 158 insertions(+), 102 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 44d8ccddc9..3a49d3d7b0 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -9,7 +9,6 @@
#include "gettext.h"
#include "parse-options.h"
#include "string-list.h"
-#include "tempfile.h"
#include "trailer.h"
#include "config.h"
@@ -84,6 +83,7 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
int unset)
{
struct process_trailer_options *v = opt->value;
+
v->only_trailers = 1;
v->only_input = 1;
v->unfold = 1;
@@ -92,37 +92,6 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
return 0;
}
-static struct tempfile *trailers_tempfile;
-
-static FILE *create_in_place_tempfile(const char *file)
-{
- struct stat st;
- struct strbuf filename_template = STRBUF_INIT;
- const char *tail;
- FILE *outfile;
-
- if (stat(file, &st))
- die_errno(_("could not stat %s"), file);
- if (!S_ISREG(st.st_mode))
- die(_("file %s is not a regular file"), file);
- if (!(st.st_mode & S_IWUSR))
- die(_("file %s is not writable by user"), file);
-
- /* Create temporary file in the same directory as the original */
- tail = strrchr(file, '/');
- if (tail)
- strbuf_add(&filename_template, file, tail - file + 1);
- strbuf_addstr(&filename_template, "git-interpret-trailers-XXXXXX");
-
- trailers_tempfile = xmks_tempfile_m(filename_template.buf, st.st_mode);
- strbuf_release(&filename_template);
- outfile = fdopen_tempfile(trailers_tempfile, "w");
- if (!outfile)
- die_errno(_("could not open temporary file"));
-
- return outfile;
-}
-
static void read_input_file(struct strbuf *sb, const char *file)
{
if (file) {
@@ -135,61 +104,6 @@ static void read_input_file(struct strbuf *sb, const char *file)
strbuf_complete_line(sb);
}
-static void interpret_trailers(const struct process_trailer_options *opts,
- struct list_head *new_trailer_head,
- const char *file)
-{
- LIST_HEAD(head);
- struct strbuf sb = STRBUF_INIT;
- struct strbuf trailer_block_sb = STRBUF_INIT;
- struct trailer_block *trailer_block;
- FILE *outfile = stdout;
-
- trailer_config_init();
-
- read_input_file(&sb, file);
-
- if (opts->in_place)
- outfile = create_in_place_tempfile(file);
-
- trailer_block = parse_trailers(opts, sb.buf, &head);
-
- /* Print the lines before the trailer block */
- if (!opts->only_trailers)
- fwrite(sb.buf, 1, trailer_block_start(trailer_block), outfile);
-
- if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
- fprintf(outfile, "\n");
-
-
- if (!opts->only_input) {
- LIST_HEAD(config_head);
- LIST_HEAD(arg_head);
- parse_trailers_from_config(&config_head);
- parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
- list_splice(&config_head, &arg_head);
- process_trailers_lists(&head, &arg_head);
- }
-
- /* Print trailer block. */
- format_trailers(opts, &head, &trailer_block_sb);
- free_trailers(&head);
- fwrite(trailer_block_sb.buf, 1, trailer_block_sb.len, outfile);
- strbuf_release(&trailer_block_sb);
-
- /* Print the lines after the trailer block as is. */
- if (!opts->only_trailers)
- fwrite(sb.buf + trailer_block_end(trailer_block), 1,
- sb.len - trailer_block_end(trailer_block), outfile);
- trailer_block_release(trailer_block);
-
- if (opts->in_place)
- if (rename_tempfile(&trailers_tempfile, file))
- die_errno(_("could not rename temporary file to %s"), file);
-
- strbuf_release(&sb);
-}
-
int cmd_interpret_trailers(int argc,
const char **argv,
const char *prefix,
@@ -231,14 +145,37 @@ int cmd_interpret_trailers(int argc,
git_interpret_trailers_usage,
options);
+ trailer_config_init();
+
if (argc) {
int i;
- for (i = 0; i < argc; i++)
- interpret_trailers(&opts, &trailers, argv[i]);
+ for (i = 0; i < argc; i++) {
+ struct strbuf in_buf = STRBUF_INIT;
+ struct strbuf out_buf = STRBUF_INIT;
+
+ read_input_file(&in_buf, argv[i]);
+ if (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)
+ die(_("failed to process trailers for %s"), argv[i]);
+ if (opts.in_place)
+ write_file_buf(argv[i], out_buf.buf, out_buf.len);
+ else
+ fwrite(out_buf.buf, 1, out_buf.len, stdout);
+ strbuf_release(&in_buf);
+ strbuf_release(&out_buf);
+ }
} else {
+ struct strbuf in_buf = STRBUF_INIT;
+ struct strbuf out_buf = STRBUF_INIT;
+
if (opts.in_place)
die(_("no input file given for in-place editing"));
- interpret_trailers(&opts, &trailers, NULL);
+
+ read_input_file(&in_buf, NULL);
+ if (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)
+ die(_("failed to process trailers"));
+ fwrite(out_buf.buf, 1, out_buf.len, stdout);
+ strbuf_release(&in_buf);
+ strbuf_release(&out_buf);
}
new_trailers_clear(&trailers);
diff --git a/trailer.c b/trailer.c
index 310cf582dc..03814443c3 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1224,14 +1224,121 @@ void trailer_iterator_release(struct trailer_iterator *iter)
strbuf_release(&iter->key);
}
-int amend_file_with_trailers(const char *path, const struct strvec *trailer_args)
+static int amend_strbuf_with_trailers(struct strbuf *buf,
+ const struct strvec *trailer_args)
{
- struct child_process run_trailer = CHILD_PROCESS_INIT;
-
- run_trailer.git_cmd = 1;
- strvec_pushl(&run_trailer.args, "interpret-trailers",
- "--in-place", "--no-divider",
- path, NULL);
- strvec_pushv(&run_trailer.args, trailer_args->v);
- return run_command(&run_trailer);
+ struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+ LIST_HEAD(new_trailer_head);
+ struct strbuf out = STRBUF_INIT;
+ size_t i;
+
+ opts.no_divider = 1;
+
+ for (i = 0; i < trailer_args->nr; i++) {
+ const char *arg = trailer_args->v[i];
+ const char *text;
+ struct new_trailer_item *item;
+ if (!skip_prefix(arg, "--trailer=", &text))
+ text = arg;
+ if (!*text)
+ continue;
+ item = xcalloc(1, sizeof(*item));
+ INIT_LIST_HEAD(&item->list);
+ item->text = text;
+ list_add_tail(&item->list, &new_trailer_head);
+ }
+ if (trailer_process(&opts, buf->buf, &new_trailer_head, &out) < 0)
+ die("failed to process trailers");
+ strbuf_swap(buf, &out);
+ strbuf_release(&out);
+ while (!list_empty(&new_trailer_head)) {
+ struct new_trailer_item *item =
+ list_first_entry(&new_trailer_head, struct new_trailer_item, list);
+ list_del(&item->list);
+ free(item);
+ }
+ return 0;
}
+
+int trailer_process(const struct process_trailer_options *opts,
+ const char *msg,
+ struct list_head *new_trailer_head,
+ struct strbuf *out)
+{
+ struct trailer_block *blk;
+ LIST_HEAD(orig_head);
+ LIST_HEAD(config_head);
+ LIST_HEAD(arg_head);
+ struct strbuf trailers_sb = STRBUF_INIT;
+ int had_trailer_before;
+
+ blk = parse_trailers(opts, msg, &orig_head);
+ had_trailer_before = !list_empty(&orig_head);
+ if (!opts->only_input) {
+ parse_trailers_from_config(&config_head);
+ parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
+ list_splice(&config_head, &arg_head);
+ process_trailers_lists(&orig_head, &arg_head);
+ }
+ format_trailers(opts, &orig_head, &trailers_sb);
+ if (!opts->only_trailers && !opts->only_input && !opts->unfold &&
+ !opts->trim_empty && list_empty(&orig_head) &&
+ (list_empty(new_trailer_head) || opts->only_input)) {
+ size_t split = trailer_block_start(blk); /* end-of-log-msg */
+ if (!blank_line_before_trailer_block(blk)) {
+ strbuf_add(out, msg, split);
+ strbuf_addch(out, '\n');
+ strbuf_addstr(out, msg + split);
+ } else
+ strbuf_addstr(out, msg);
+
+ strbuf_release(&trailers_sb);
+ trailer_block_release(blk);
+ return 0;
+ }
+ if (opts->only_trailers) {
+ strbuf_addbuf(out, &trailers_sb);
+ } else if (had_trailer_before) {
+ strbuf_add(out, msg, trailer_block_start(blk));
+ if (!blank_line_before_trailer_block(blk))
+ strbuf_addch(out, '\n');
+ strbuf_addbuf(out, &trailers_sb);
+ strbuf_add(out, msg + trailer_block_end(blk),
+ strlen(msg) - trailer_block_end(blk));
+ }
+ else {
+ size_t cpos = trailer_block_start(blk);
+ strbuf_add(out, msg, cpos);
+ if (cpos == 0) /* empty body → just one \n */
+ strbuf_addch(out, '\n');
+ else if (!blank_line_before_trailer_block(blk))
+ strbuf_addch(out, '\n'); /* body without trailing blank */
+
+ strbuf_addbuf(out, &trailers_sb);
+ strbuf_add(out, msg + cpos, strlen(msg) - cpos);
+ }
+ strbuf_release(&trailers_sb);
+ free_trailers(&orig_head);
+ trailer_block_release(blk);
+ return 0;
+}
+
+int amend_file_with_trailers(const char *path,
+ const struct strvec *trailer_args)
+{
+ struct strbuf buf = STRBUF_INIT;
+
+ if (!trailer_args || !trailer_args->nr)
+ return 0;
+
+ if (strbuf_read_file(&buf, path, 0) < 0)
+ return error_errno("could not read '%s'", path);
+
+ if (amend_strbuf_with_trailers(&buf, trailer_args))
+ die("failed to append trailers");
+
+ /* `write_file_buf()` aborts on error internally */
+ write_file_buf(path, buf.buf, buf.len);
+ strbuf_release(&buf);
+ return 0;
+ }
diff --git a/trailer.h b/trailer.h
index 4740549586..01f711fb13 100644
--- a/trailer.h
+++ b/trailer.h
@@ -196,10 +196,22 @@ int trailer_iterator_advance(struct trailer_iterator *iter);
void trailer_iterator_release(struct trailer_iterator *iter);
/*
- * Augment a file to add trailers to it by running git-interpret-trailers.
- * This calls run_command() and its return value is the same (i.e. 0 for
- * success, various non-zero for other errors). See run-command.h.
+ * Augment a file to add trailers to it (similar to 'git interpret-trailers').
+ * Returns 0 on success or a non-zero error code on failure.
*/
int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);
+/*
+ * Process trailer lines for a commit message in-memory.
+ * @opts: trailer processing options (e.g. from parse-options)
+ * @msg: the input message string
+ * @new_trailer_head: list of new trailers to add (struct new_trailer_item)
+ * @out: strbuf to store the resulting message (must be initialized)
+ *
+ * Returns 0 on success, <0 on error.
+ */
+int trailer_process(const struct process_trailer_options *opts,
+ const char *msg,
+ struct list_head *new_trailer_head,
+ struct strbuf *out);
#endif /* TRAILER_H */
--
2.50.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers`
2025-08-03 15:00 ` [PATCH v3 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers` Li Chen
@ 2025-08-05 13:17 ` Phillip Wood
2025-08-07 2:45 ` Li Chen
2025-10-21 10:01 ` Li Chen
0 siblings, 2 replies; 15+ messages in thread
From: Phillip Wood @ 2025-08-05 13:17 UTC (permalink / raw)
To: Li Chen, phillipwood, git, Junio C Hamano
Hi Li
On 03/08/2025 16:00, Li Chen wrote:
> From: Li Chen <chenl311@chinatelecom.cn>
>
> All trailer insertion now funnels through trailer_process():
>
> * builtin/interpret-trailers.c is reduced to file I/O + a single call.
> * amend_file_with_trailers() shares the same path; the old
> amend_strbuf_with_trailers() helper is dropped.
> * New helpers parse_trailer_args()/free_new_trailer_list() convert
> --trailer=... strings to new_trailer_item lists.
>
> Behaviour is unchanged; the full test-suite still passes, and the
> fork/exec is gone.
Normally commit messages should be written in prose rather than a bullet
list and the message should explain the reason for the change.
This patch has much less code duplication than the last iteration which
is most welcome. Whenever you are moving and refactoring code you should
split the move into its own commit followed by the refactoring. That
makes it much easier to review as the reviewer can clearly see the
refactoring rather than having to manually compare the added code in one
file to the deleted code in another.
> @@ -84,6 +83,7 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
> int unset)
> {
> struct process_trailer_options *v = opt->value;
> +
Let's not clutter this patch with unrelated changes.
> v->only_trailers = 1;
> v->only_input = 1;
> v->unfold = 1;
> @@ -92,37 +92,6 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
> return 0;
> }
>
> -static FILE *create_in_place_tempfile(const char *file)
> -{
> [...]
> -}
We don't need to create a temporary file anymore so this can be deleted
- good.
> -static void interpret_trailers(const struct process_trailer_options *opts,
> - struct list_head *new_trailer_head,
> - const char *file)
> -{
> - LIST_HEAD(head);
> - struct strbuf sb = STRBUF_INIT;
> - struct strbuf trailer_block_sb = STRBUF_INIT;
> - struct trailer_block *trailer_block;
> - FILE *outfile = stdout;
> -
> - trailer_config_init();
> -
> - read_input_file(&sb, file);
> -
> - if (opts->in_place)
> - outfile = create_in_place_tempfile(file);
> -
> - trailer_block = parse_trailers(opts, sb.buf, &head);
> -
> - /* Print the lines before the trailer block */
> - if (!opts->only_trailers)
> - fwrite(sb.buf, 1, trailer_block_start(trailer_block), outfile);
> -
> - if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
> - fprintf(outfile, "\n");
> -
> -
> - if (!opts->only_input) {
> - LIST_HEAD(config_head);
> - LIST_HEAD(arg_head);
> - parse_trailers_from_config(&config_head);
> - parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
> - list_splice(&config_head, &arg_head);
> - process_trailers_lists(&head, &arg_head);
> - }
> -
> - /* Print trailer block. */
> - format_trailers(opts, &head, &trailer_block_sb);
> - free_trailers(&head);
> - fwrite(trailer_block_sb.buf, 1, trailer_block_sb.len, outfile);
> - strbuf_release(&trailer_block_sb);
> -
> - /* Print the lines after the trailer block as is. */
> - if (!opts->only_trailers)
> - fwrite(sb.buf + trailer_block_end(trailer_block), 1,
> - sb.len - trailer_block_end(trailer_block), outfile);
> - trailer_block_release(trailer_block);
> -
> - if (opts->in_place)
> - if (rename_tempfile(&trailers_tempfile, file))
> - die_errno(_("could not rename temporary file to %s"), file);
> -
> - strbuf_release(&sb);
> -}
This code is moved to trailer.c which is good but it is heavily
refactored at the same time which makes it hard to review. Completely
removing this function leads to some duplication in
cmd_interpret_trailers() which could be avoided by making
interpret_trailers() a wrapper around process_trailers()
> int cmd_interpret_trailers(int argc,
> const char **argv,
> const char *prefix,
> @@ -231,14 +145,37 @@ int cmd_interpret_trailers(int argc,
> git_interpret_trailers_usage,
> options);
>
> + trailer_config_init();
> +
> if (argc) {
> int i;
> - for (i = 0; i < argc; i++)
> - interpret_trailers(&opts, &trailers, argv[i]);
> + for (i = 0; i < argc; i++) {
> + struct strbuf in_buf = STRBUF_INIT;
> + struct strbuf out_buf = STRBUF_INIT;
> +
> + read_input_file(&in_buf, argv[i]);
> + if (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)
> + die(_("failed to process trailers for %s"), argv[i]);
> + if (opts.in_place)
> + write_file_buf(argv[i], out_buf.buf, out_buf.len);
> + else
> + fwrite(out_buf.buf, 1, out_buf.len, stdout);
> + strbuf_release(&in_buf);
> + strbuf_release(&out_buf);
> + }
> } else {
> + struct strbuf in_buf = STRBUF_INIT;
> + struct strbuf out_buf = STRBUF_INIT;
> +
> if (opts.in_place)
> die(_("no input file given for in-place editing"));
> - interpret_trailers(&opts, &trailers, NULL);
> +
> + read_input_file(&in_buf, NULL);
> + if (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)
> + die(_("failed to process trailers"));
> + fwrite(out_buf.buf, 1, out_buf.len, stdout);
> + strbuf_release(&in_buf);
> + strbuf_release(&out_buf);
> }
There is quite a bit of duplication here that could be avoided if you
modified interpret_trailers() to call trailer_process() rather than
deleting it entirely.
> new_trailers_clear(&trailers);
> diff --git a/trailer.c b/trailer.c
> index 310cf582dc..03814443c3 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1224,14 +1224,121 @@ void trailer_iterator_release(struct trailer_iterator *iter)
> strbuf_release(&iter->key);
> }
>
> -int amend_file_with_trailers(const char *path, const struct strvec *trailer_args)
> +static int amend_strbuf_with_trailers(struct strbuf *buf,
> + const struct strvec *trailer_args)
Function argument declarations should be aligned
> {
> - struct child_process run_trailer = CHILD_PROCESS_INIT;
> -
> - run_trailer.git_cmd = 1;
> - strvec_pushl(&run_trailer.args, "interpret-trailers",
> - "--in-place", "--no-divider",
> - path, NULL);
> - strvec_pushv(&run_trailer.args, trailer_args->v);
> - return run_command(&run_trailer);
> + struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
> + LIST_HEAD(new_trailer_head);
> + struct strbuf out = STRBUF_INIT;
> + size_t i;
> +
> + opts.no_divider = 1;
> +
> + for (i = 0; i < trailer_args->nr; i++) {
> + const char *arg = trailer_args->v[i];
> + const char *text;
> + struct new_trailer_item *item;
There should be a blank line after the variable declarations at the
start of each block of code.
> + if (!skip_prefix(arg, "--trailer=", &text))
Why do we need this? It would be much cleaner if we required the caller
to pass a list of trailers without any optional prefix.
> + text = arg;
> + if (!*text)
> + continue;
> + item = xcalloc(1, sizeof(*item));
> + INIT_LIST_HEAD(&item->list);
> + item->text = text;
> + list_add_tail(&item->list, &new_trailer_head);
> + }
> + if (trailer_process(&opts, buf->buf, &new_trailer_head, &out) < 0)
> + die("failed to process trailers");
As this is library code lets return an error here rather than dying.
> + strbuf_swap(buf, &out);
> + strbuf_release(&out);
> + while (!list_empty(&new_trailer_head)) {
> + struct new_trailer_item *item =
> + list_first_entry(&new_trailer_head, struct new_trailer_item, list);
> + list_del(&item->list);
> + free(item);
> + }
> + return 0;
> }
> +
> +int trailer_process(const struct process_trailer_options *opts,
> + const char *msg,
> + struct list_head *new_trailer_head,
> + struct strbuf *out)
Argument alignment again
> +{
> + struct trailer_block *blk;
This is trailer_block in the original but has been re-ordered with
respect to the other variable declarations making the patch harder to
review.
> + LIST_HEAD(orig_head);
This is head in the original but moved relative to the other variable
declarations
> + LIST_HEAD(config_head);
> + LIST_HEAD(ar1g_head);
These two have been moved from inside the if (!opts->only_input) below.
They are only referenced there so do not need to be declared here.
Moving them makes this patch harder to review.
> + struct strbuf trailers_sb = STRBUF_INIT;
This is from the original but moved relative to the other variable
declarations.
> + int had_trailer_before;
This is new - lets see how it is used. We've just started using bool for
boolean variables in the last few weeks so this could be a bool now.
From here to
> + blk = parse_trailers(opts, msg, &orig_head);
> + had_trailer_before = !list_empty(&orig_head);
> + if (!opts->only_input) {
> + parse_trailers_from_config(&config_head);
> + parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
> + list_splice(&config_head, &arg_head);
> + process_trailers_lists(&orig_head, &arg_head);
> + }
> + format_trailers(opts, &orig_head, &trailers_sb);
here is copied from the original minus the code that copied the commit
message to the output file. Rather than deleting the code that copied
the commit message we could have replaced the calls to fwrite() and
fprintf() with strbuf_add() and strbuf_addf() which would make it
obvious that the behavior is not changed. The original then frees
orig_head but that is done later here.
> + if (!opts->only_trailers && !opts->only_input && !opts->unfold &&
> + !opts->trim_empty && list_empty(&orig_head) &&
> + (list_empty(new_trailer_head) || opts->only_input)) {
I'm not sure what is happening here. By this point the original has
copied the original commit message and is ready to append the new
trailers. Instead the new version seems to have completely refactored
the logic for adding the new trailers making it harder to see if the
behavior has changed.
> + size_t split = trailer_block_start(blk); /* end-of-log-msg */
> + if (!blank_line_before_trailer_block(blk)) {
> + strbuf_add(out, msg, split);
> + strbuf_addch(out, '\n');
> + strbuf_addstr(out, msg + split);
This copies the original message but adds a newline before the trailer
block if it is missing.
> + } else
> + strbuf_addstr(out, msg);
This just copies the whole message.
> + strbuf_rel2ease(&trailers_sb);
> + trailer_block_release(blk);
> + return 0;
We return a copy of the original message with no new trailers added. We
do not free orig_head, arg_head or config_head. I'm still confused why
we need to special case this.
> + }
> + if (opts->only_trailers) {
> + strbuf_addbuf(out, &trailers_sb);
This flips the logic in the original to handle opts->only_trailers
separately making it harder to review.
> + } else if (had_trailer_before) {
> + strbuf_add(out, msg, trailer_block_start(blk));
> + if (!blank_line_before_trailer_block(blk))
> + strbuf_addch(out, '\n');
> + strbuf_addbuf(out, &trailers_sb);
> + strbuf_add(out, msg + trailer_block_end(blk),
> + strlen(msg) - trailer_block_end(blk));
This handles the case where we're replacing the headers in the original
message
> + }
> + else {
Style - this should be "} else {"
> + size_t cpos = trailer_block_start(blk);
> + strbuf_add(out, msg, cpos);
> + if (cpos == 0) /* empty body → just one \n */
> + strbuf_addch(out, '\n');
> + else if (!blank_line_before_trailer_block(blk))
> + strbuf_addch(out, '\n'); /* body without trailing blank */
> +
> + strbuf_addbuf(out, &trailers_sb);
> + strbuf_add(out, msg + cpos, strlen(msg) - cpos);
> + }
I'm confused why we need a separate case for when the original did not
have any trailers - was the original code broken? If it was we should
separate out the bug fix from the refactoring. If not what's the point
of this change?
> + strbuf_release(&trailers_sb);
> + free_trailers(&orig_head);
> + trailer_block_release(blk);
> + return 0;
> +}
> +
> +int amend_file_with_trailers(const char *path,
> + const struct strvec *trailer_args)
Alignment again
> +{
> + struct strbuf buf = STRBUF_INIT;
> +
> + if (!trailer_args || !trailer_args->nr)
> + return 0;
> +
> + if (strbuf_read_file(&buf, path, 0) < 0)
> + return error_errno("could not read '%s'", path);
> +
> + if (amend_strbuf_with_trailers(&buf, trailer_args))
> + die("failed to append trailers");
Why return an error() above but die() here? This is library code so lets
return an error.
> +
> + /* `write_file_buf()` aborts on error internally */
> + write_file_buf(path, buf.buf, buf.len);
Dying here is a change in behavior which callers might not be expecting.
The original code always returned a error because it forked a
sub-process to do the trailer processing. Ideally, in a separate commit,
we'd update any existing callers that have the message in an strbuf so
they don't have to write it to a file just to add some trailers to it.
> + strbuf_release(&buf);
> + return 0;
> + }
As I said above reusing the existing code as you have done here is a
much better approach. However it would be much easier to review if the
code movement was separated from the refactoring. I'm also struggling to
see the benefit of a lot of the refactoring - I was expecting the
conversion to use an strubf would essentially look like fwrite() being
replaced with strbuf_add() and fprintf() being replaced with
strbuf_addf() etc. rather than reworking the logic.
Thanks
Phillip
> diff --git a/trailer.h b/trailer.h
> index 4740549586..01f711fb13 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -196,10 +196,22 @@ int trailer_iterator_advance(struct trailer_iterator *iter);
> void trailer_iterator_release(struct trailer_iterator *iter);
>
> /*
> - * Augment a file to add trailers to it by running git-interpret-trailers.
> - * This calls run_command() and its return value is the same (i.e. 0 for
> - * success, various non-zero for other errors). See run-command.h.
> + * Augment a file to add trailers to it (similar to 'git interpret-trailers').
> + * Returns 0 on success or a non-zero error code on failure.
> */
> int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);
>
> +/*
> + * Process trailer lines for a commit message in-memory.
> + * @opts: trailer processing options (e.g. from parse-options)
> + * @msg: the input message string
> + * @new_trailer_head: list of new trailers to add (struct new_trailer_item)
> + * @out: strbuf to store the resulting message (must be initialized)
> + *
> + * Returns 0 on success, <0 on error.
> + */
> +int trailer_process(const struct process_trailer_options *opts,
> + const char *msg,
> + struct list_head *new_trailer_head,
> + struct strbuf *out);
> #endif /* TRAILER_H */
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers`
2025-08-05 13:17 ` Phillip Wood
@ 2025-08-07 2:45 ` Li Chen
2025-10-21 10:01 ` Li Chen
1 sibling, 0 replies; 15+ messages in thread
From: Li Chen @ 2025-08-07 2:45 UTC (permalink / raw)
To: phillipwood; +Cc: git, Junio C Hamano
Hi Phillip,
---- On Tue, 05 Aug 2025 21:17:01 +0800 Phillip Wood <phillip.wood123@gmail.com> wrote ---
> Hi Li
>
> On 03/08/2025 16:00, Li Chen wrote:
> > From: Li Chen <chenl311@chinatelecom.cn>
> >
> > All trailer insertion now funnels through trailer_process():
> >
> > * builtin/interpret-trailers.c is reduced to file I/O + a single call.
> > * amend_file_with_trailers() shares the same path; the old
> > amend_strbuf_with_trailers() helper is dropped.
> > * New helpers parse_trailer_args()/free_new_trailer_list() convert
> > --trailer=... strings to new_trailer_item lists.
> >
> > Behaviour is unchanged; the full test-suite still passes, and the
> > fork/exec is gone.
>
> Normally commit messages should be written in prose rather than a bullet
> list and the message should explain the reason for the change.
>
> This patch has much less code duplication than the last iteration which
> is most welcome. Whenever you are moving and refactoring code you should
> split the move into its own commit followed by the refactoring. That
> makes it much easier to review as the reviewer can clearly see the
> refactoring rather than having to manually compare the added code in one
> file to the deleted code in another.
I apologize for this, and I will add new commits to resolve all issues in the next versions.
> > @@ -84,6 +83,7 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
> > int unset)
> > {
> > struct process_trailer_options *v = opt->value;
> > +
>
> Let's not clutter this patch with unrelated changes.
>
> > v->only_trailers = 1;
> > v->only_input = 1;
> > v->unfold = 1;
> > @@ -92,37 +92,6 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
> > return 0;
> > }
> >
> > -static FILE *create_in_place_tempfile(const char *file)
> > -{
> > [...]
> > -}
>
> We don't need to create a temporary file anymore so this can be deleted
> - good.
>
> > -static void interpret_trailers(const struct process_trailer_options *opts,
> > - struct list_head *new_trailer_head,
> > - const char *file)
> > -{
> > - LIST_HEAD(head);
> > - struct strbuf sb = STRBUF_INIT;
> > - struct strbuf trailer_block_sb = STRBUF_INIT;
> > - struct trailer_block *trailer_block;
> > - FILE *outfile = stdout;
> > -
> > - trailer_config_init();
> > -
> > - read_input_file(&sb, file);
> > -
> > - if (opts->in_place)
> > - outfile = create_in_place_tempfile(file);
> > -
> > - trailer_block = parse_trailers(opts, sb.buf, &head);
> > -
> > - /* Print the lines before the trailer block */
> > - if (!opts->only_trailers)
> > - fwrite(sb.buf, 1, trailer_block_start(trailer_block), outfile);
> > -
> > - if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
> > - fprintf(outfile, "\n");
> > -
> > -
> > - if (!opts->only_input) {
> > - LIST_HEAD(config_head);
> > - LIST_HEAD(arg_head);
> > - parse_trailers_from_config(&config_head);
> > - parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
> > - list_splice(&config_head, &arg_head);
> > - process_trailers_lists(&head, &arg_head);
> > - }
> > -
> > - /* Print trailer block. */
> > - format_trailers(opts, &head, &trailer_block_sb);
> > - free_trailers(&head);
> > - fwrite(trailer_block_sb.buf, 1, trailer_block_sb.len, outfile);
> > - strbuf_release(&trailer_block_sb);
> > -
> > - /* Print the lines after the trailer block as is. */
> > - if (!opts->only_trailers)
> > - fwrite(sb.buf + trailer_block_end(trailer_block), 1,
> > - sb.len - trailer_block_end(trailer_block), outfile);
> > - trailer_block_release(trailer_block);
> > -
> > - if (opts->in_place)
> > - if (rename_tempfile(&trailers_tempfile, file))
> > - die_errno(_("could not rename temporary file to %s"), file);
> > -
> > - strbuf_release(&sb);
> > -}
>
> This code is moved to trailer.c which is good but it is heavily
> refactored at the same time which makes it hard to review. Completely
> removing this function leads to some duplication in
> cmd_interpret_trailers() which could be avoided by making
> interpret_trailers() a wrapper around process_trailers()
>
> > int cmd_interpret_trailers(int argc,
> > const char **argv,
> > const char *prefix,
> > @@ -231,14 +145,37 @@ int cmd_interpret_trailers(int argc,
> > git_interpret_trailers_usage,
> > options);
> >
> > + trailer_config_init();
> > +
> > if (argc) {
> > int i;
> > - for (i = 0; i < argc; i++)
> > - interpret_trailers(&opts, &trailers, argv[i]);
> > + for (i = 0; i < argc; i++) {
> > + struct strbuf in_buf = STRBUF_INIT;
> > + struct strbuf out_buf = STRBUF_INIT;
> > +
> > + read_input_file(&in_buf, argv[i]);
> > + if (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)
> > + die(_("failed to process trailers for %s"), argv[i]);
> > + if (opts.in_place)
> > + write_file_buf(argv[i], out_buf.buf, out_buf.len);
> > + else
> > + fwrite(out_buf.buf, 1, out_buf.len, stdout);
> > + strbuf_release(&in_buf);
> > + strbuf_release(&out_buf);
> > + }
> > } else {
> > + struct strbuf in_buf = STRBUF_INIT;
> > + struct strbuf out_buf = STRBUF_INIT;
> > +
> > if (opts.in_place)
> > die(_("no input file given for in-place editing"));
> > - interpret_trailers(&opts, &trailers, NULL);
> > +
> > + read_input_file(&in_buf, NULL);
> > + if (trailer_process(&opts, in_buf.buf, &trailers, &out_buf) < 0)
> > + die(_("failed to process trailers"));
> > + fwrite(out_buf.buf, 1, out_buf.len, stdout);
> > + strbuf_release(&in_buf);
> > + strbuf_release(&out_buf);
> > }
>
> There is quite a bit of duplication here that could be avoided if you
> modified interpret_trailers() to call trailer_process() rather than
> deleting it entirely.
>
> > new_trailers_clear(&trailers);
> > diff --git a/trailer.c b/trailer.c
> > index 310cf582dc..03814443c3 100644
> > --- a/trailer.c
> > +++ b/trailer.c
> > @@ -1224,14 +1224,121 @@ void trailer_iterator_release(struct trailer_iterator *iter)
> > strbuf_release(&iter->key);
> > }
> >
> > -int amend_file_with_trailers(const char *path, const struct strvec *trailer_args)
> > +static int amend_strbuf_with_trailers(struct strbuf *buf,
> > + const struct strvec *trailer_args)
>
> Function argument declarations should be aligned
>
> > {
> > - struct child_process run_trailer = CHILD_PROCESS_INIT;
> > -
> > - run_trailer.git_cmd = 1;
> > - strvec_pushl(&run_trailer.args, "interpret-trailers",
> > - "--in-place", "--no-divider",
> > - path, NULL);
> > - strvec_pushv(&run_trailer.args, trailer_args->v);
> > - return run_command(&run_trailer);
> > + struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
> > + LIST_HEAD(new_trailer_head);
> > + struct strbuf out = STRBUF_INIT;
> > + size_t i;
> > +
> > + opts.no_divider = 1;
> > +
> > + for (i = 0; i < trailer_args->nr; i++) {
> > + const char *arg = trailer_args->v[i];
> > + const char *text;
> > + struct new_trailer_item *item;
>
> There should be a blank line after the variable declarations at the
> start of each block of code.
>
> > + if (!skip_prefix(arg, "--trailer=", &text))
>
> Why do we need this? It would be much cleaner if we required the caller
> to pass a list of trailers without any optional prefix.
>
> > + text = arg;
> > + if (!*text)
> > + continue;
> > + item = xcalloc(1, sizeof(*item));
> > + INIT_LIST_HEAD(&item->list);
> > + item->text = text;
> > + list_add_tail(&item->list, &new_trailer_head);
> > + }
> > + if (trailer_process(&opts, buf->buf, &new_trailer_head, &out) < 0)
> > + die("failed to process trailers");
>
> As this is library code lets return an error here rather than dying.
>
> > + strbuf_swap(buf, &out);
> > + strbuf_release(&out);
> > + while (!list_empty(&new_trailer_head)) {
> > + struct new_trailer_item *item =
> > + list_first_entry(&new_trailer_head, struct new_trailer_item, list);
> > + list_del(&item->list);
> > + free(item);
> > + }
> > + return 0;
> > }
> > +
> > +int trailer_process(const struct process_trailer_options *opts,
> > + const char *msg,
> > + struct list_head *new_trailer_head,
> > + struct strbuf *out)
>
> Argument alignment again
>
> > +{
> > + struct trailer_block *blk;
>
> This is trailer_block in the original but has been re-ordered with
> respect to the other variable declarations making the patch harder to
> review.
>
> > + LIST_HEAD(orig_head);
>
> This is head in the original but moved relative to the other variable
> declarations
>
> > + LIST_HEAD(config_head);
> > + LIST_HEAD(ar1g_head);
>
> These two have been moved from inside the if (!opts->only_input) below.
> They are only referenced there so do not need to be declared here.
> Moving them makes this patch harder to review.
>
> > + struct strbuf trailers_sb = STRBUF_INIT;
>
> This is from the original but moved relative to the other variable
> declarations.
>
> > + int had_trailer_before;
>
> This is new - lets see how it is used. We've just started using bool for
> boolean variables in the last few weeks so this could be a bool now.
>
> From here to
>
> > + blk = parse_trailers(opts, msg, &orig_head);
> > + had_trailer_before = !list_empty(&orig_head);
> > + if (!opts->only_input) {
> > + parse_trailers_from_config(&config_head);
> > + parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
> > + list_splice(&config_head, &arg_head);
> > + process_trailers_lists(&orig_head, &arg_head);
> > + }
> > + format_trailers(opts, &orig_head, &trailers_sb);
>
> here is copied from the original minus the code that copied the commit
> message to the output file. Rather than deleting the code that copied
> the commit message we could have replaced the calls to fwrite() and
> fprintf() with strbuf_add() and strbuf_addf() which would make it
> obvious that the behavior is not changed. The original then frees
> orig_head but that is done later here.
>
> > + if (!opts->only_trailers && !opts->only_input && !opts->unfold &&
> > + !opts->trim_empty && list_empty(&orig_head) &&
> > + (list_empty(new_trailer_head) || opts->only_input)) {
>
> I'm not sure what is happening here. By this point the original has
> copied the original commit message and is ready to append the new
> trailers. Instead the new version seems to have completely refactored
> the logic for adding the new trailers making it harder to see if the
> behavior has changed.
>
> > + size_t split = trailer_block_start(blk); /* end-of-log-msg */
> > + if (!blank_line_before_trailer_block(blk)) {
> > + strbuf_add(out, msg, split);
> > + strbuf_addch(out, '\n');
> > + strbuf_addstr(out, msg + split);
>
> This copies the original message but adds a newline before the trailer
> block if it is missing.
>
> > + } else
> > + strbuf_addstr(out, msg);
>
> This just copies the whole message.
>
> > + strbuf_rel2ease(&trailers_sb);
> > + trailer_block_release(blk);
>
>
> > + return 0;
>
> We return a copy of the original message with no new trailers added. We
> do not free orig_head, arg_head or config_head. I'm still confused why
> we need to special case this.
>
>
> > + }
> > + if (opts->only_trailers) {
> > + strbuf_addbuf(out, &trailers_sb);
>
> This flips the logic in the original to handle opts->only_trailers
> separately making it harder to review.
>
> > + } else if (had_trailer_before) {
> > + strbuf_add(out, msg, trailer_block_start(blk));
> > + if (!blank_line_before_trailer_block(blk))
> > + strbuf_addch(out, '\n');
> > + strbuf_addbuf(out, &trailers_sb);
> > + strbuf_add(out, msg + trailer_block_end(blk),
> > + strlen(msg) - trailer_block_end(blk));
>
> This handles the case where we're replacing the headers in the original
> message
>
> > + }
> > + else {
>
> Style - this should be "} else {"
>
> > + size_t cpos = trailer_block_start(blk);
> > + strbuf_add(out, msg, cpos);
> > + if (cpos == 0) /* empty body → just one \n */
> > + strbuf_addch(out, '\n');
> > + else if (!blank_line_before_trailer_block(blk))
> > + strbuf_addch(out, '\n'); /* body without trailing blank */
> > +
> > + strbuf_addbuf(out, &trailers_sb);
> > + strbuf_add(out, msg + cpos, strlen(msg) - cpos);
> > + }
>
> I'm confused why we need a separate case for when the original did not
> have any trailers - was the original code broken? If it was we should
> separate out the bug fix from the refactoring. If not what's the point
> of this change?
>
> > + strbuf_release(&trailers_sb);
> > + free_trailers(&orig_head);
> > + trailer_block_release(blk);
> > + return 0;
> > +}
> > +
> > +int amend_file_with_trailers(const char *path,
> > + const struct strvec *trailer_args)
>
> Alignment again
>
> > +{
> > + struct strbuf buf = STRBUF_INIT;
> > +
> > + if (!trailer_args || !trailer_args->nr)
> > + return 0;
> > +
> > + if (strbuf_read_file(&buf, path, 0) < 0)
> > + return error_errno("could not read '%s'", path);
> > +
> > + if (amend_strbuf_with_trailers(&buf, trailer_args))
> > + die("failed to append trailers");
>
> Why return an error() above but die() here? This is library code so lets
> return an error.
>
> > +
> > + /* `write_file_buf()` aborts on error internally */
> > + write_file_buf(path, buf.buf, buf.len);
>
> Dying here is a change in behavior which callers might not be expecting.
> The original code always returned a error because it forked a
> sub-process to do the trailer processing. Ideally, in a separate commit,
> we'd update any existing callers that have the message in an strbuf so
> they don't have to write it to a file just to add some trailers to it.
>
> > + strbuf_release(&buf);
> > + return 0;
> > + }
>
> As I said above reusing the existing code as you have done here is a
> much better approach. However it would be much easier to review if the
> code movement was separated from the refactoring. I'm also struggling to
> see the benefit of a lot of the refactoring - I was expecting the
> conversion to use an strubf would essentially look like fwrite() being
> replaced with strbuf_add() and fprintf() being replaced with
> strbuf_addf() etc. rather than reworking the logic.
>
> Thanks
>
>
> Phillip
>
> > diff --git a/trailer.h b/trailer.h
> > index 4740549586..01f711fb13 100644
> > --- a/trailer.h
> > +++ b/trailer.h
> > @@ -196,10 +196,22 @@ int trailer_iterator_advance(struct trailer_iterator *iter);
> > void trailer_iterator_release(struct trailer_iterator *iter);
> >
> > /*
> > - * Augment a file to add trailers to it by running git-interpret-trailers.
> > - * This calls run_command() and its return value is the same (i.e. 0 for
> > - * success, various non-zero for other errors). See run-command.h.
> > + * Augment a file to add trailers to it (similar to 'git interpret-trailers').
> > + * Returns 0 on success or a non-zero error code on failure.
> > */
> > int amend_file_with_trailers(const char *path, const struct strvec *trailer_args);
> >
> > +/*
> > + * Process trailer lines for a commit message in-memory.
> > + * @opts: trailer processing options (e.g. from parse-options)
> > + * @msg: the input message string
> > + * @new_trailer_head: list of new trailers to add (struct new_trailer_item)
> > + * @out: strbuf to store the resulting message (must be initialized)
> > + *
> > + * Returns 0 on success, <0 on error.
> > + */
> > +int trailer_process(const struct process_trailer_options *opts,
> > + const char *msg,
> > + struct list_head *new_trailer_head,
> > + struct strbuf *out);
> > #endif /* TRAILER_H */
>
>
Regards,
Li
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers`
2025-08-05 13:17 ` Phillip Wood
2025-08-07 2:45 ` Li Chen
@ 2025-10-21 10:01 ` Li Chen
1 sibling, 0 replies; 15+ messages in thread
From: Li Chen @ 2025-10-21 10:01 UTC (permalink / raw)
To: phillipwood; +Cc: git, Junio C Hamano
Hi Phillip,
> > new_trailers_clear(&trailers);
> > diff --git a/trailer.c b/trailer.c
> > index 310cf582dc..03814443c3 100644
> > --- a/trailer.c
> > +++ b/trailer.c
> > @@ -1224,14 +1224,121 @@ void trailer_iterator_release(struct trailer_iterator *iter)
> > strbuf_release(&iter->key);
> > }
> >
> > -int amend_file_with_trailers(const char *path, const struct strvec *trailer_args)
> > +static int amend_strbuf_with_trailers(struct strbuf *buf,
> > + const struct strvec *trailer_args)
>
> Function argument declarations should be aligned
My editor uses a 4-space tab width, but the project view displays tabs as 8 spaces wide.
This discrepancy caused the same errors in v4, which I plan to correct with clang-format in v5.
Regards,
Li
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 2/2] rebase: support --trailer
2025-08-03 15:00 [PATCH v3 0/2] rebase: support --trailer Li Chen
2025-08-03 15:00 ` [PATCH v3 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers` Li Chen
@ 2025-08-03 15:00 ` Li Chen
2025-08-05 15:38 ` Phillip Wood
2025-08-06 10:28 ` Phillip Wood
2025-08-03 16:35 ` [PATCH v3 0/2] " Junio C Hamano
2 siblings, 2 replies; 15+ messages in thread
From: Li Chen @ 2025-08-03 15:00 UTC (permalink / raw)
To: phillipwood, git, Junio C Hamano
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.
And reject invalid input before user edit the interactive file.
Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
---
Documentation/git-rebase.adoc | 7 +++
builtin/rebase.c | 98 +++++++++++++++++++++++++++++++++++
sequencer.c | 13 +++++
sequencer.h | 3 ++
t/meson.build | 1 +
t/t3440-rebase-trailer.sh | 95 +++++++++++++++++++++++++++++++++
6 files changed, 217 insertions(+)
create mode 100755 t/t3440-rebase-trailer.sh
diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc
index 956d3048f5..df8fb97526 100644
--- a/Documentation/git-rebase.adoc
+++ b/Documentation/git-rebase.adoc
@@ -521,6 +521,13 @@ 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.
+
+--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 e90562a3b8..3b4c45a616 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -36,6 +36,8 @@
#include "reset.h"
#include "trace2.h"
#include "hook.h"
+#include "trailer.h"
+#include "parse-options.h"
static char const * const builtin_rebase_usage[] = {
N_("git rebase [-i] [options] [--exec <cmd>] "
@@ -113,6 +115,7 @@ struct rebase_options {
enum action action;
char *reflog_action;
int signoff;
+ struct strvec trailer_args;
int allow_rerere_autoupdate;
int keep_empty;
int autosquash;
@@ -143,6 +146,7 @@ struct rebase_options {
.flags = REBASE_NO_QUIET, \
.git_am_opts = STRVEC_INIT, \
.exec = STRING_LIST_INIT_NODUP, \
+ .trailer_args = STRVEC_INIT, \
.git_format_patch_opt = STRBUF_INIT, \
.fork_point = -1, \
.reapply_cherry_picks = -1, \
@@ -166,6 +170,7 @@ static void rebase_options_release(struct rebase_options *opts)
free(opts->strategy);
string_list_clear(&opts->strategy_opts, 0);
strbuf_release(&opts->git_format_patch_opt);
+ strvec_clear(&opts->trailer_args);
}
static struct replay_opts get_replay_opts(const struct rebase_options *opts)
@@ -177,6 +182,10 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
sequencer_init_config(&replay);
replay.signoff = opts->signoff;
+
+ for (size_t i = 0; i < opts->trailer_args.nr; i++)
+ strvec_push(&replay.trailer_args, opts->trailer_args.v[i]);
+
replay.allow_ff = !(opts->flags & REBASE_FORCE);
if (opts->allow_rerere_autoupdate)
replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
@@ -435,6 +444,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;
+ const char trailer_state_name[] = "trailer";
+ 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) ||
@@ -503,11 +514,31 @@ static int read_basic_state(struct rebase_options *opts)
strbuf_release(&buf);
+ if (strbuf_read_file(&buf, path, 0) >= 0) {
+ const char *p = buf.buf, *end = buf.buf + buf.len;
+
+ while (p < end) {
+ char *nl = memchr(p, '\n', end - p);
+ if (!nl)
+ die("nl shouldn't be NULL");
+ *nl = '\0';
+
+ if (*p)
+ strvec_push(&opts->trailer_args, p);
+
+ p = nl + 1;
+ }
+ strbuf_release(&buf);
+ }
+ strbuf_release(&buf);
+
return 0;
}
static int rebase_write_basic_state(struct rebase_options *opts)
{
+ const char trailer_state_name[] = "trailer";
+
write_file(state_dir_path("head-name", opts), "%s",
opts->head_name ? opts->head_name : "detached HEAD");
write_file(state_dir_path("onto", opts), "%s",
@@ -529,6 +560,22 @@ static int rebase_write_basic_state(struct rebase_options *opts)
if (opts->signoff)
write_file(state_dir_path("signoff", opts), "--signoff");
+ /*
+ * save opts->trailer_args into state_dir/trailer
+ */
+ if (opts->trailer_args.nr) {
+ struct strbuf buf = STRBUF_INIT;
+ 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);
+ }
+
return 0;
}
@@ -1085,6 +1132,37 @@ static int check_exec_cmd(const char *cmd)
return 0;
}
+static int validate_trailer_args_after_config(const struct strvec *cli_args,
+ struct strbuf *err)
+{
+ size_t i;
+
+ for (i = 0; i < cli_args->nr; i++) {
+ const char *raw = cli_args->v[i];
+ const char *txt; // Key[:=]Val
+ const char *sep;
+
+ if (!skip_prefix(raw, "--trailer=", &txt))
+ txt = raw;
+
+ if (!*txt) {
+ strbuf_addstr(err, _("empty --trailer argument"));
+ return -1;
+ }
+
+ sep = strpbrk(txt, ":=");
+
+ /* there must be key bfore seperator */
+ if (sep && sep == txt) {
+ strbuf_addf(err,
+ _("invalid trailer '%s': missing key before separator"),
+ txt);
+ return -1;
+ }
+ }
+ return 0;
+}
+
int cmd_rebase(int argc,
const char **argv,
const char *prefix,
@@ -1133,6 +1211,7 @@ int cmd_rebase(int argc,
.flags = PARSE_OPT_NOARG,
.defval = REBASE_DIFFSTAT,
},
+ OPT_STRVEC(0, "trailer", &options.trailer_args, N_("trailer"), N_("add custom trailer(s)")),
OPT_BOOL(0, "signoff", &options.signoff,
N_("add a Signed-off-by trailer to each commit")),
OPT_BOOL(0, "committer-date-is-author-date",
@@ -1283,6 +1362,17 @@ int cmd_rebase(int argc,
builtin_rebase_options,
builtin_rebase_usage, 0);
+ /* if add --trailer，force rebase */
+ if (options.trailer_args.nr) {
+ struct strbuf err = STRBUF_INIT;
+
+ if (validate_trailer_args_after_config(&options.trailer_args, &err))
+ die("%s", err.buf);
+
+ options.flags |= REBASE_FORCE;
+ strbuf_release(&err);
+ }
+
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"
@@ -1540,6 +1630,14 @@ 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.trailer_args.nr)
+ imply_merge(&options, "--trailer");
+
if (isatty(2) && options.flags & REBASE_NO_QUIET)
strbuf_addstr(&options.git_format_patch_opt, " --progress");
diff --git a/sequencer.c b/sequencer.c
index 67e4310edc..58faf6aed5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -422,6 +422,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)
@@ -2529,6 +2530,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, reflog_action,
diff --git a/sequencer.h b/sequencer.h
index 304ba4b4d3..28f2da6375 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -4,6 +4,7 @@
#include "strbuf.h"
#include "strvec.h"
#include "wt-status.h"
+#include <stddef.h>
struct commit;
struct index_state;
@@ -44,6 +45,7 @@ struct replay_opts {
int record_origin;
int no_commit;
int signoff;
+ struct strvec trailer_args;
int allow_ff;
int allow_rerere_auto;
int allow_empty;
@@ -86,6 +88,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 09f3068f98..3c58f562da 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -373,6 +373,7 @@ integration_tests = [
't3436-rebase-more-options.sh',
't3437-rebase-fixup-options.sh',
't3438-rebase-broken-files.sh',
+ 't3440-rebase-trailer.sh',
't3500-cherry.sh',
't3501-revert-cherry-pick.sh',
't3502-cherry-pick-merge.sh',
diff --git a/t/t3440-rebase-trailer.sh b/t/t3440-rebase-trailer.sh
new file mode 100755
index 0000000000..a580449628
--- /dev/null
+++ b/t/t3440-rebase-trailer.sh
@@ -0,0 +1,95 @@
+#!/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
+
+create_expect() {
+ cat >"$1" <<-EOF
+ $2
+
+ Reviewed-by: Dev <dev@example.com>
+ EOF
+}
+
+test_expect_success 'setup repo with a small history' '
+ git commit --allow-empty -m "Initial empty commit" &&
+ test_commit first file a &&
+ test_commit second file &&
+ git checkout -b conflict-branch first &&
+ test_commit file-2 file-2 &&
+ test_commit conflict file &&
+ test_commit third file &&
+ ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" &&
+ 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"
+'
+
+test_expect_success 'apply backend is rejected with --trailer' '
+ git reset --hard third &&
+ head_before=$(git rev-parse HEAD) &&
+ test_expect_code 128 \
+ git rebase --apply --trailer "Reviewed-by: Dev <dev@example.com>" \
+ HEAD^ 2>err &&
+ test_grep "requires the merge backend" err &&
+ test_cmp_rev HEAD $head_before
+'
+
+test_expect_success 'reject empty --trailer argument' '
+ git reset --hard third &&
+ test_expect_code 128 git rebase -m --trailer "" HEAD^ 2>err &&
+ test_grep "empty --trailer" err
+'
+
+test_expect_success 'reject trailer with missing key before separator' '
+ git reset --hard third &&
+ test_expect_code 128 git rebase -m --trailer ": no-key" HEAD^ 2>err &&
+ test_grep "missing key before separator" err
+'
+
+test_expect_success 'CLI trailer duplicates allowed; replace policy keeps last' '
+ git reset --hard third &&
+ git -c trailer.Bug.ifexists=replace -c trailer.Bug.ifmissing=add rebase -m --trailer "Bug: 123" --trailer "Bug: 456" HEAD~1 &&
+ git cat-file commit HEAD | grep "^Bug: 456" &&
+ git cat-file commit HEAD | grep -v "^Bug: 123"
+'
+
+test_expect_success 'multiple Signed-off-by trailers all preserved' '
+ git reset --hard third &&
+ git rebase -m \
+ --trailer "Signed-off-by: Dev A <a@ex.com>" \
+ --trailer "Signed-off-by: Dev B <b@ex.com>" HEAD~1 &&
+ git cat-file commit HEAD | grep -c "^Signed-off-by:" >count &&
+ test "$(cat count)" = 2 # two new commits
+'
+
+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 rebase --continue &&
+ test_commit_message HEAD~2 file2-signed
+'
+
+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.50.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3 2/2] rebase: support --trailer
2025-08-03 15:00 ` [PATCH v3 2/2] rebase: support --trailer Li Chen
@ 2025-08-05 15:38 ` Phillip Wood
2025-08-06 10:28 ` Phillip Wood
1 sibling, 0 replies; 15+ messages in thread
From: Phillip Wood @ 2025-08-05 15:38 UTC (permalink / raw)
To: Li Chen, phillipwood, git, Junio C Hamano
On 03/08/2025 16:00, 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.
We only want to reject it if the user passes an option that requires the
apply backend, otherwise we can just use the merge backend.
> Automatically set REBASE_FORCE when
> any trailer is supplied.
Makes sense
> And reject invalid input before user edit the interactive file.
> +--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.
The options like --reset-author-date just say "implies `--force-rebase`"
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index e90562a3b8..3b4c45a616 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -36,6 +36,8 @@
> #include "reset.h"
> #include "trace2.h"
> #include "hook.h"
> +#include "trailer.h"
> +#include "parse-options.h"
"parse-options.h" is already included.
> @@ -435,6 +444,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;
> + const char trailer_state_name[] = "trailer";
> + const char *path = state_dir_path(trailer_state_name, opts);
We only use this once, so lets pass state_dir_path("trailer", opts)
directly to strbuf_read_file() like we do when reading all the other
state files in this function.
> if (!read_oneliner(&head_name, state_dir_path("head-name", opts),
> READ_ONELINER_WARN_MISSING) ||
> @@ -503,11 +514,31 @@ static int read_basic_state(struct rebase_options *opts)
>
> strbuf_release(&buf);
This should become strbuf_reset(&buf) so that we can reuse the allocation.
> + if (strbuf_read_file(&buf, path, 0) >= 0) {
> + const char *p = buf.buf, *end = buf.buf + buf.len;
> +
> + while (p < end) {
> + char *nl = memchr(p, '\n', end - p);
> + if (!nl)
> + die("nl shouldn't be NULL");
> + *nl = '\0';
> +
> + if (*p)
This is needed because we allow an empty file but reading an empty file
is a bug because we only write this file if the user gave --trailers on
the command line and we should reject empty trailers before getting here.
> + strvec_push(&opts->trailer_args, p);
> +
> + p = nl + 1;
> + }
> + strbuf_release(&buf);
This is unnecessary, it is released below which is a better place as it
means the buffer is freed even if strbuf_read_file() fails.
> + }
> + strbuf_release(&buf);
> +
> return 0;
> }
>
> static int rebase_write_basic_state(struct rebase_options *opts)
> {
> + const char trailer_state_name[] = "trailer";
This is unnecessary, lets follow the existing style of just using the
filename directly in the one place it is needed.
> + /*
> + * 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++) {
We can use "for (size_t i = 0; i < ...) {" here
> + 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);
> + }
> +
> return 0;
> }
This looks good
> +static int validate_trailer_args_after_config(const struct strvec *cli_args,
> + struct strbuf *err)
> +{
> + size_t i;
> +
> + for (i = 0; i < cli_args->nr; i++) {
As i is only used in the loop we can say "for (size_t i = 0; ...) {" and
drop the declaration above.
> + const char *raw = cli_args->v[i];
> + const char *txt; // Key[:=]Val
> + const char *sep;
> +
> + if (!skip_prefix(raw, "--trailer=", &txt))
> + txt = raw;
OPT_STRVEC() only stores the argument, not the option name so we don't
need this
> +
> + if (!*txt) {
> + strbuf_addstr(err, _("empty --trailer argument"));
> + return -1;
I'm not sure we need the err buf here - we can just
return error(_("empty --trailer argument"));
> + }
> +
> + sep = strpbrk(txt, ":=");
The list of separators is configurable - if we move this function into
trailer.c then we can use find_separator().
> +
> + /* there must be key bfore seperator */
> + if (sep && sep == txt) {
> + strbuf_addf(err,
> + _("invalid trailer '%s': missing key before separator"),
> + txt);
We can use return error(...) here as well.
> @@ -1133,6 +1211,7 @@ int cmd_rebase(int argc,
> .flags = PARSE_OPT_NOARG,
> .defval = REBASE_DIFFSTAT,
> },
> + OPT_STRVEC(0, "trailer", &options.trailer_args, N_("trailer"), N_("add custom trailer(s)")),
This line is rather long, can we fold it like the one below please.
> OPT_BOOL(0, "signoff", &options.signoff,
> N_("add a Signed-off-by trailer to each commit")),
> OPT_BOOL(0, "committer-date-is-author-date",
> @@ -1283,6 +1362,17 @@ int cmd_rebase(int argc,
> builtin_rebase_options,
> builtin_rebase_usage, 0);
>
> + /* if add --trailer，force rebase */
I'm not sure what "add --trailer" means. Forcing the rebase when
--trailer is given is a good idea.
> + if (options.trailer_args.nr) {
> + struct strbuf err = STRBUF_INIT;
The indentation in this block is off here and below
> + if (validate_trailer_args_after_config(&options.trailer_args, &err))
> + die("%s", err.buf);
> +
> + options.flags |= REBASE_FORCE;
> + strbuf_release(&err);
> + }
> + /*
> + * 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.
> + */
This comment is a bit confusing - what are we rejecting here? The call
to imply_merge() is correct.
> + if (options.trailer_args.nr)
> + imply_merge(&options, "--trailer");
> +
> if (isatty(2) && options.flags & REBASE_NO_QUIET)
> strbuf_addstr(&options.git_format_patch_opt, " --progress");
>
> diff --git a/sequencer.c b/sequencer.c
> index 67e4310edc..58faf6aed5 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -422,6 +422,7 @@ void replay_opts_release(struct replay_opts *opts)
> free(opts->revs);
> replay_ctx_release(ctx);
> free(opts->ctx);
> + strvec_clear(&opts->trailer_args);
Lets move this up a bit before the ctx stuff as that should the last
member of the struct.
> @@ -2529,6 +2530,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 */
This feels a bit late in the function to be doing this. Can be add the
trailers just after we add the sign off around line 2457. That way we
can add the trailers to ctx->message before writing the commit message
file. Like --signoff we also need to make sure we don't add trailers
when we're processing a fixup or squash command.
> + 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, reflog_action,
> diff --git a/sequencer.h b/sequencer.h
> index 304ba4b4d3..28f2da6375 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -4,6 +4,7 @@
> #include "strbuf.h"
> #include "strvec.h"
> #include "wt-status.h"
> +#include <stddef.h>
We never add system headers like this. git-compat-util.h takes care of
that in a portable manner.
> struct commit;
> struct index_state;
> @@ -44,6 +45,7 @@ struct replay_opts {
> int record_origin;
> int no_commit;
> int signoff;
> + struct strvec trailer_args;
> int allow_ff;
> int allow_rerere_auto;
> int allow_empty;
> @@ -86,6 +88,7 @@ struct replay_opts {
> .action = -1, \
> .xopts = STRVEC_INIT, \
> .ctx = replay_ctx_new(), \
> + .trailer_args = STRVEC_INIT, \
> }
Lets keep the order of members in the initializer the same as in the
struct declaration.
I've run out of time for today, I'll take a look at the tests later in
the week. Although I've left quite a few comments the code in this patch
looks basically sound.
Thanks
Phillip
> /*
> diff --git a/t/meson.build b/t/meson.build
> index 09f3068f98..3c58f562da 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -373,6 +373,7 @@ integration_tests = [
> 't3436-rebase-more-options.sh',
> 't3437-rebase-fixup-options.sh',
> 't3438-rebase-broken-files.sh',
> + 't3440-rebase-trailer.sh',
> 't3500-cherry.sh',
> 't3501-revert-cherry-pick.sh',
> 't3502-cherry-pick-merge.sh',
> diff --git a/t/t3440-rebase-trailer.sh b/t/t3440-rebase-trailer.sh
> new file mode 100755
> index 0000000000..a580449628
> --- /dev/null
> +++ b/t/t3440-rebase-trailer.sh
> @@ -0,0 +1,95 @@
> +#!/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
> +
> +create_expect() {
> + cat >"$1" <<-EOF
> + $2
> +
> + Reviewed-by: Dev <dev@example.com>
> + EOF
> +}
> +
> +test_expect_success 'setup repo with a small history' '
> + git commit --allow-empty -m "Initial empty commit" &&
> + test_commit first file a &&
> + test_commit second file &&
> + git checkout -b conflict-branch first &&
> + test_commit file-2 file-2 &&
> + test_commit conflict file &&
> + test_commit third file &&
> + ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" &&
> + 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"
> +'
> +
> +test_expect_success 'apply backend is rejected with --trailer' '
> + git reset --hard third &&
> + head_before=$(git rev-parse HEAD) &&
> + test_expect_code 128 \
> + git rebase --apply --trailer "Reviewed-by: Dev <dev@example.com>" \
> + HEAD^ 2>err &&
> + test_grep "requires the merge backend" err &&
> + test_cmp_rev HEAD $head_before
> +'
> +
> +test_expect_success 'reject empty --trailer argument' '
> + git reset --hard third &&
> + test_expect_code 128 git rebase -m --trailer "" HEAD^ 2>err &&
> + test_grep "empty --trailer" err
> +'
> +
> +test_expect_success 'reject trailer with missing key before separator' '
> + git reset --hard third &&
> + test_expect_code 128 git rebase -m --trailer ": no-key" HEAD^ 2>err &&
> + test_grep "missing key before separator" err
> +'
> +
> +test_expect_success 'CLI trailer duplicates allowed; replace policy keeps last' '
> + git reset --hard third &&
> + git -c trailer.Bug.ifexists=replace -c trailer.Bug.ifmissing=add rebase -m --trailer "Bug: 123" --trailer "Bug: 456" HEAD~1 &&
> + git cat-file commit HEAD | grep "^Bug: 456" &&
> + git cat-file commit HEAD | grep -v "^Bug: 123"
> +'
> +
> +test_expect_success 'multiple Signed-off-by trailers all preserved' '
> + git reset --hard third &&
> + git rebase -m \
> + --trailer "Signed-off-by: Dev A <a@ex.com>" \
> + --trailer "Signed-off-by: Dev B <b@ex.com>" HEAD~1 &&
> + git cat-file commit HEAD | grep -c "^Signed-off-by:" >count &&
> + test "$(cat count)" = 2 # two new commits
> +'
> +
> +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 rebase --continue &&
> + test_commit_message HEAD~2 file2-signed
> +'
> +
> +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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 2/2] rebase: support --trailer
2025-08-03 15:00 ` [PATCH v3 2/2] rebase: support --trailer Li Chen
2025-08-05 15:38 ` Phillip Wood
@ 2025-08-06 10:28 ` Phillip Wood
2025-08-06 13:19 ` Phillip Wood
1 sibling, 1 reply; 15+ messages in thread
From: Phillip Wood @ 2025-08-06 10:28 UTC (permalink / raw)
To: Li Chen, phillipwood, git, Junio C Hamano
Hi Li
On 03/08/2025 16:00, Li Chen wrote:
> From: Li Chen <chenl311@chinatelecom.cn>
Picking up from where I left off yesterday ...
> diff --git a/t/meson.build b/t/meson.build
> index 09f3068f98..3c58f562da 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -373,6 +373,7 @@ integration_tests = [
> 't3436-rebase-more-options.sh',
> 't3437-rebase-fixup-options.sh',
> 't3438-rebase-broken-files.sh',
> + 't3440-rebase-trailer.sh',
The alignment looks off here
> '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..a580449628
> --- /dev/null
> +++ b/t/t3440-rebase-trailer.sh
> @@ -0,0 +1,95 @@
> +#!/bin/sh
> +#
> +
> +test_description='git rebase --trailer integration tests
> +We verify that --trailer on the merge/interactive/exec/root backends,
There are only two backends "apply" and "merge", the other things you
have listed are just command line options. We don't actually test --exec
or --interactive with --trailer in this file so we should reword that
comment. There is no need to add tests for those.
> +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
> +
> +create_expect() {
> + cat >"$1" <<-EOF
> + $2
> +
> + Reviewed-by: Dev <dev@example.com>
> + EOF
> +}
> +
> +test_expect_success 'setup repo with a small history' '
> + git commit --allow-empty -m "Initial empty commit" &&
> + test_commit first file a &&
> + test_commit second file &&
> + git checkout -b conflict-branch first &&
> + test_commit file-2 file-2 &&
> + test_commit conflict file &&
> + test_commit third file &&
> + ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" &&
> + 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"
Normally we create the "expect" file in the test where it is used.
> +'
> +
> +test_expect_success 'apply backend is rejected with --trailer' '
We know HEAD is at third so we don't need this
> + git reset --hard third &&
> + head_before=$(git rev-parse HEAD) &&
> + test_expect_code 128 \
The indentation is off here
> + git rebase --apply --trailer "Reviewed-by: Dev <dev@example.com>" \
> + HEAD^ 2>err &&
> + test_grep "requires the merge backend" err &&
I think it is worth checking that the error message includes --trailer
as well to make sure that is the option that is triggering the error.
> + test_cmp_rev HEAD $head_before
> +'
> +
> +test_expect_success 'reject empty --trailer argument' '
> + git reset --hard third &&
The exact commit is not important here
> + test_expect_code 128 git rebase -m --trailer "" HEAD^ 2>err &&
> + test_grep "empty --trailer" err
> +'
> +
> +test_expect_success 'reject trailer with missing key before separator' '
> + git reset --hard third &&
Same here - we're only checking the error message so the exact value of
HEAD is not important.
> + test_expect_code 128 git rebase -m --trailer ": no-key" HEAD^ 2>err &&
> + test_grep "missing key before separator" err
> +'
> +
> +test_expect_success 'CLI trailer duplicates allowed; replace policy keeps last' '
> + git reset --hard third &&
> + git -c trailer.Bug.ifexists=replace -c trailer.Bug.ifmissing=add rebase -m --trailer "Bug: 123" --trailer "Bug: 456" HEAD~1 &&
> + git cat-file commit HEAD | grep "^Bug: 456" &&
> + git cat-file commit HEAD | grep -v "^Bug: 123"
Piping git into another command is discouraged as git can potentially
fail without us noticing. Here I think it would be better to use
test_commit_message() to check the whole message.
> +'
> +
> +test_expect_success 'multiple Signed-off-by trailers all preserved' '
> + git reset --hard third &&
We can avoid this by passing the commit we want to checkout to rebase as
you do in the test below.
> + git rebase -m \
> + --trailer "Signed-off-by: Dev A <a@ex.com>" \
> + --trailer "Signed-off-by: Dev B <b@ex.com>" HEAD~1 &&
> + git cat-file commit HEAD | grep -c "^Signed-off-by:" >count &&
> + test "$(cat count)" = 2 # two new commits
Let check the actual message here as well so if it fails we can see what
the message is.
> +'
> +
> +test_expect_success 'rebase -m --trailer adds trailer after conflicts' '
> + git reset --hard third &&
We don't need this as the rebase command checks out third for us, saving
a process which is always nice.
> + test_must_fail git rebase -m \
> + --trailer "Reviewed-by: Dev <dev@example.com>" \
> + second third &&
> + git checkout --theirs file &&
> + git add file &&
> + git rebase --continue &&
The indentation is off here
> + test_commit_message HEAD~2 file2-signed
It's good to see this uses test_commit_message but why are we checking
HEAD~2 rather than HEAD?> +'
> +
> +test_expect_success 'rebase --root --trailer updates every commit' '
> + git checkout first &&
> + git rebase --root --keep-empty \
--keep-empty is the default these days so I think we can drop that.
> + --trailer "Reviewed-by: Dev <dev@example.com>" &&
> + test_commit_message HEAD first-signed &&
> + test_commit_message HEAD^ initial-signed
Looks good.
While there are some small issues to fix the tests look sensible and I
think you have good coverage of the new option.
Thanks
Phillip
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 2/2] rebase: support --trailer
2025-08-06 10:28 ` Phillip Wood
@ 2025-08-06 13:19 ` Phillip Wood
2025-08-07 2:40 ` Li Chen
2025-08-07 2:40 ` Li Chen
0 siblings, 2 replies; 15+ messages in thread
From: Phillip Wood @ 2025-08-06 13:19 UTC (permalink / raw)
To: Li Chen, phillipwood, git, Junio C Hamano
Hi Li
I had a couple more thoughts about the tests ...
On 06/08/2025 11:28, Phillip Wood wrote:
> On 03/08/2025 16:00, Li Chen wrote:
>> +create_expect() {
>> + cat >"$1" <<-EOF
>> + $2
>> +
>> + Reviewed-by: Dev <dev@example.com>
>> + EOF
>> +}
>> +
>> +test_expect_success 'setup repo with a small history' '
>> [...]
>> + create_expect third-signed "third" &&>> +
create_expect conflict-signed "conflict"
>
> Normally we create the "expect" file in the test where it is used.
Thinking about this some more, if we want to use test_commit_message
then I think we can change create_expect to write to stdout and do
test_commit_message HEAD <<-EOF
$(create_expect first)
EOF
rather than having to create a file.
>> +
>> +test_expect_success 'reject empty --trailer argument' '
>> [...]
>> +test_expect_success 'reject trailer with missing key before separator' '
Should we also test for a missing value or are trailers without a value
allowed?
>> + git rebase -m \
>> + --trailer "Signed-off-by: Dev A <a@ex.com>" \
>> + --trailer "Signed-off-by: Dev B <b@ex.com>" HEAD~1 &&
Lets use example.com here rather than some random domain that might
actually exist.
>> +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 rebase --continue &&
This checks that the commit with conflicts has a trailer added but it
does not check that the commits picked by "git rebase --continue" do. To
check that we actually save the trailers and use them when continuing we
need to add a fourth commit on top of third and check that has a trailer
add here as well.
A couple more thoughts:
- We should check that
git -c trailer.review.key=Reviewed-by rebase \
--trailer=review="Dev <dev@example.com>"
adds a "Reviewed-by:" trailer. We can do that by changing one of the
tests in this patch rather than adding a new one. This checks that we
accept '=' as a separator as well a respecting the config.
- We should check that the todo list
pick first
fixup second
adds the trailer as expected and that
pick first
fixup -C second
also works. To do that we will need to source lib-rebase.sh at the
start of the test file and add a test that uses set_replace_editor()
which should be called in a subshell.
Do please ask if you have any questions about these suggestions
Thanks
Phillip
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 2/2] rebase: support --trailer
2025-08-06 13:19 ` Phillip Wood
@ 2025-08-07 2:40 ` Li Chen
2025-08-28 23:35 ` Junio C Hamano
2025-08-07 2:40 ` Li Chen
1 sibling, 1 reply; 15+ messages in thread
From: Li Chen @ 2025-08-07 2:40 UTC (permalink / raw)
To: phillipwood; +Cc: git, Junio C Hamano
Hi Phillip,
Thanks for your thorough review; I will address them in the next version.
---- On Wed, 06 Aug 2025 21:19:57 +0800 Phillip Wood <phillip.wood123@gmail.com> wrote ---
> Hi Li
>
> I had a couple more thoughts about the tests ...
>
> On 06/08/2025 11:28, Phillip Wood wrote:
> > On 03/08/2025 16:00, Li Chen wrote:
> >> +create_expect() {
> >> + cat >"$1" <<-EOF
> >> + $2
> >> +
> >> + Reviewed-by: Dev <dev@example.com>
> >> + EOF
> >> +}
> >> +
> >> +test_expect_success 'setup repo with a small history' '
> >> [...]
> >> + create_expect third-signed "third" &&>> +
> create_expect conflict-signed "conflict"
> >
> > Normally we create the "expect" file in the test where it is used.
>
> Thinking about this some more, if we want to use test_commit_message
> then I think we can change create_expect to write to stdout and do
>
> test_commit_message HEAD <<-EOF
> $(create_expect first)
> EOF
>
> rather than having to create a file.
>
> >> +
> >> +test_expect_success 'reject empty --trailer argument' '
> >> [...]
> >> +test_expect_success 'reject trailer with missing key before separator' '
>
> Should we also test for a missing value or are trailers without a value
> allowed?
>
> >> + git rebase -m \
> >> + --trailer "Signed-off-by: Dev A <a@ex.com>" \
> >> + --trailer "Signed-off-by: Dev B <b@ex.com>" HEAD~1 &&
>
> Lets use example.com here rather than some random domain that might
> actually exist.
>
> >> +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 rebase --continue &&
>
> This checks that the commit with conflicts has a trailer added but it
> does not check that the commits picked by "git rebase --continue" do. To
> check that we actually save the trailers and use them when continuing we
> need to add a fourth commit on top of third and check that has a trailer
> add here as well.
>
> A couple more thoughts:
>
> - We should check that
> git -c trailer.review.key=Reviewed-by rebase \
> --trailer=review="Dev <dev@example.com>"
> adds a "Reviewed-by:" trailer. We can do that by changing one of the
> tests in this patch rather than adding a new one. This checks that we
> accept '=' as a separator as well a respecting the config.
>
> - We should check that the todo list
> pick first
> fixup second
> adds the trailer as expected and that
> pick first
> fixup -C second
> also works. To do that we will need to source lib-rebase.sh at the
> start of the test file and add a test that uses set_replace_editor()
> which should be called in a subshell.
>
>
> Do please ask if you have any questions about these suggestions
Regards,
Li
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 2/2] rebase: support --trailer
2025-08-07 2:40 ` Li Chen
@ 2025-08-28 23:35 ` Junio C Hamano
2025-09-18 8:36 ` Li Chen
0 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2025-08-28 23:35 UTC (permalink / raw)
To: Li Chen; +Cc: phillipwood, git
Li Chen <me@linux.beauty> writes:
> Hi Phillip,
>
> Thanks for your thorough review; I will address them in the next version.
As I do not want to keep an inactive topic in 'seen' for more than a
month, I was doing my usual "sweep" of the topics, and found this
exchange.
Is this still being worked on? No rush, but just checking to see
what the status is.
Since the summer is a slow season, I do not mind keeping the topic
for a few more weeks in 'seen', but I can simply discard the one I
have, and requeue a new version in 'seen' when it materializes.
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/2] rebase: support --trailer
2025-08-28 23:35 ` Junio C Hamano
@ 2025-09-18 8:36 ` Li Chen
0 siblings, 0 replies; 15+ messages in thread
From: Li Chen @ 2025-09-18 8:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: phillipwood, git
Hi Junio,
I apologize for the delayed response.
---- On Fri, 29 Aug 2025 07:35:45 +0800 Junio C Hamano <gitster@pobox.com> wrote ---
> Li Chen <me@linux.beauty> writes:
>
> > Hi Phillip,
> >
> > Thanks for your thorough review; I will address them in the next version.
>
> As I do not want to keep an inactive topic in 'seen' for more than a
> month, I was doing my usual "sweep" of the topics, and found this
> exchange.
>
> Is this still being worked on? No rush, but just checking to see
> what the status is.
>
> Since the summer is a slow season, I do not mind keeping the topic
> for a few more weeks in 'seen', but I can simply discard the one I
> have, and requeue a new version in 'seen' when it materializes.
Yes, it's still in progress, though I've had limited time recently. I aim to finish the next
version before October 8th, taking advantage of the 7-day Chinese National Day holiday
to work on it.
Regards,
Li
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/2] rebase: support --trailer
2025-08-06 13:19 ` Phillip Wood
2025-08-07 2:40 ` Li Chen
@ 2025-08-07 2:40 ` Li Chen
1 sibling, 0 replies; 15+ messages in thread
From: Li Chen @ 2025-08-07 2:40 UTC (permalink / raw)
To: phillipwood; +Cc: git, Junio C Hamano
---- On Wed, 06 Aug 2025 21:19:57 +0800 Phillip Wood <phillip.wood123@gmail.com> wrote ---
> Hi Li
>
> I had a couple more thoughts about the tests ...
>
> On 06/08/2025 11:28, Phillip Wood wrote:
> > On 03/08/2025 16:00, Li Chen wrote:
> >> +create_expect() {
> >> + cat >"$1" <<-EOF
> >> + $2
> >> +
> >> + Reviewed-by: Dev <dev@example.com>
> >> + EOF
> >> +}
> >> +
> >> +test_expect_success 'setup repo with a small history' '
> >> [...]
> >> + create_expect third-signed "third" &&>> +
> create_expect conflict-signed "conflict"
> >
> > Normally we create the "expect" file in the test where it is used.
>
> Thinking about this some more, if we want to use test_commit_message
> then I think we can change create_expect to write to stdout and do
>
> test_commit_message HEAD <<-EOF
> $(create_expect first)
> EOF
>
> rather than having to create a file.
>
> >> +
> >> +test_expect_success 'reject empty --trailer argument' '
> >> [...]
> >> +test_expect_success 'reject trailer with missing key before separator' '
>
> Should we also test for a missing value or are trailers without a value
> allowed?
>
> >> + git rebase -m \
> >> + --trailer "Signed-off-by: Dev A <a@ex.com>" \
> >> + --trailer "Signed-off-by: Dev B <b@ex.com>" HEAD~1 &&
>
> Lets use example.com here rather than some random domain that might
> actually exist.
>
> >> +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 rebase --continue &&
>
> This checks that the commit with conflicts has a trailer added but it
> does not check that the commits picked by "git rebase --continue" do. To
> check that we actually save the trailers and use them when continuing we
> need to add a fourth commit on top of third and check that has a trailer
> add here as well.
>
> A couple more thoughts:
>
> - We should check that
> git -c trailer.review.key=Reviewed-by rebase \
> --trailer=review="Dev <dev@example.com>"
> adds a "Reviewed-by:" trailer. We can do that by changing one of the
> tests in this patch rather than adding a new one. This checks that we
> accept '=' as a separator as well a respecting the config.
>
> - We should check that the todo list
> pick first
> fixup second
> adds the trailer as expected and that
> pick first
> fixup -C second
> also works. To do that we will need to source lib-rebase.sh at the
> start of the test file and add a test that uses set_replace_editor()
> which should be called in a subshell.
>
>
> Do please ask if you have any questions about these suggestions
>
> Thanks
>
> Phillip
>
>
Regards,
Li
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 0/2] rebase: support --trailer
2025-08-03 15:00 [PATCH v3 0/2] rebase: support --trailer Li Chen
2025-08-03 15:00 ` [PATCH v3 1/2] trailer: append trailers in-process and drop the fork to `interpret-trailers` Li Chen
2025-08-03 15:00 ` [PATCH v3 2/2] rebase: support --trailer Li Chen
@ 2025-08-03 16:35 ` Junio C Hamano
2025-08-04 1:44 ` Li Chen
2 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2025-08-03 16:35 UTC (permalink / raw)
To: Li Chen; +Cc: phillipwood, git
Li Chen <me@linux.beauty> writes:
> From: Li Chen <chenl311@chinatelecom.cn>
>
> This two-patch series teaches git rebase a new
> --trailer <text> option and, as a prerequisite, moves all trailer
> handling out of the external interpret-trailers helper and into the
> builtin code path, as suggested by Phillip Wood.
>
> Patch 1 switches trailer.c to an in-memory implementation
> (amend_strbuf_with_trailers()). It removes every fork/exec.
>
> Patch 2 builds on that helper to implement
> git rebase --trailer.
Try running "git show --check" on this commit. My attempt found a
handful of whitespace breakages ("indent with spaces.").
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 0/2] rebase: support --trailer
2025-08-03 16:35 ` [PATCH v3 0/2] " Junio C Hamano
@ 2025-08-04 1:44 ` Li Chen
0 siblings, 0 replies; 15+ messages in thread
From: Li Chen @ 2025-08-04 1:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: phillipwood, git
Hi Junio,
---- On Mon, 04 Aug 2025 00:35:57 +0800 Junio C Hamano <gitster@pobox.com> wrote ---
> Li Chen <me@linux.beauty> writes:
>
> > From: Li Chen <chenl311@chinatelecom.cn>
> >
> > This two-patch series teaches git rebase a new
> > --trailer <text> option and, as a prerequisite, moves all trailer
> > handling out of the external interpret-trailers helper and into the
> > builtin code path, as suggested by Phillip Wood.
> >
> > Patch 1 switches trailer.c to an in-memory implementation
> > (amend_strbuf_with_trailers()). It removes every fork/exec.
> >
> > Patch 2 builds on that helper to implement
> > git rebase --trailer.
>
> Try running "git show --check" on this commit. My attempt found a
> handful of whitespace breakages ("indent with spaces.").
>
> Thanks.
>
>
That's a great tool, I wasn't aware of that command, and I will fix them in
Next version. Thanks, Junio.
Regards,
Li
^ permalink raw reply [flat|nested] 15+ messages in thread