From: Phillip Wood Date: Mon, 02 Mar 2026 14:56:27 GMT Subject: Re: [PATCH v7 3/5] trailer: append trailers without fork/exec Message-ID: In-Reply-To: <20260224070552.148591-4-me@linux.beauty> Hi Li On 24/02/2026 07:05, Li Chen wrote: > Introduce amend_strbuf_with_trailers() to apply trailer additions to a > message buffer via process_trailers(), avoiding the need to run git > interpret-trailers as a child process. > > Update amend_file_with_trailers() to use the in-process helper and > rewrite the target file via tempfile+rename, preserving the previous > in-place semantics. > > Keep existing callers unchanged by continuing to accept argv-style > --trailer= entries and stripping the prefix before feeding the > in-process implementation. This looks pretty good, there are a few rough edges in the tempfile handling and a couple of unrelated changes but the basics are sound. > Signed-off-by: Li Chen > --- > v7: > Drop wrapper.c/h and validate trailer args via validate_trailer_args(). > Rewrite the target file via tempfile+rename to preserve --in-place semantics. > > builtin/interpret-trailers.c | 4 +- > trailer.c | 162 +++++++++++++++++++++++++++++++++-- > trailer.h | 27 +++++- > 3 files changed, 179 insertions(+), 14 deletions(-) > > diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c > index 1354109e0f..d4aff68746 100644 > --- a/builtin/interpret-trailers.c > +++ b/builtin/interpret-trailers.c > @@ -144,8 +144,6 @@ static void interpret_trailers(const struct process_trailer_options *opts, > struct strbuf out = STRBUF_INIT; > FILE *outfile = stdout; > > - trailer_config_init(); > - > read_input_file(&input, file); > > if (opts->in_place) > @@ -203,6 +201,8 @@ int cmd_interpret_trailers(int argc, > git_interpret_trailers_usage, > options); > > + trailer_config_init(); > + The changes above are unrelated to everything else in this patch. > if (argc) { > int i; > for (i = 0; i < argc; i++) > diff --git a/trailer.c b/trailer.c > index 0c9200506d..8e87d185d9 100644 > --- a/trailer.c > +++ b/trailer.c > @@ -772,6 +775,30 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head, > free(cl_separators); > } > > +void validate_trailer_args(const struct strvec *cli_args) > +{ > + char *cl_separators; > + > + trailer_config_init(); > + > + cl_separators = xstrfmt("=%s", separators); > + > + for (size_t i = 0; i < cli_args->nr; i++) { > + const char *txt = cli_args->v[i]; > + ssize_t separator_pos; > + > + if (!*txt) > + die(_("empty --trailer argument")); > + > + separator_pos = find_separator(txt, cl_separators); > + if (separator_pos == 0) > + die(_("invalid trailer '%s': missing key before separator"), > + txt); > + } > + > + free(cl_separators); > +} This function is never called, is it left over by a mistake, or should it be called by amend_file_with_trailers()? > static const char *next_line(const char *str) > { > const char *nl = strchrnul(str, '\n'); > @@ -1224,16 +1251,133 @@ 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 void new_trailer_items_clear(struct list_head *items) > +{ > + while (!list_empty(items)) { > + struct new_trailer_item *item = > + list_first_entry(items, struct new_trailer_item, list); > + list_del(&item->list); > + free(item); > + } > +} Isn't this just doing what free_trailers() does c.f. https://lore.kernel.org/git/ef12ada7-13ae-4df0-a823-6f428c797223@gmail.com/ > +void amend_strbuf_with_trailers(struct strbuf *buf, > + const struct strvec *trailer_args) > +{ > + 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 *text = trailer_args->v[i]; > + struct new_trailer_item *item; > + > + if (!*text) > + die(_("empty --trailer argument")); The existing callers of amend_file_with_trailers() are not expecting it to die so we should return an error here so they cleanup before exiting. > + item = xcalloc(1, sizeof(*item)); > + item->text = text; > + list_add_tail(&item->list, &new_trailer_head); > + } > + > + trailer_config_init(); > + process_trailers(&opts, &new_trailer_head, buf, &out); > + > + strbuf_swap(buf, &out); > + strbuf_release(&out); > + > + new_trailer_items_clear(&new_trailer_head); > +} > + > +static int write_file_in_place(const char *path, const struct strbuf *buf) > +{ > + struct stat st; > + struct strbuf filename_template = STRBUF_INIT; > + const char *tail; > + struct tempfile *tempfile; > + FILE *outfile; > + > + if (stat(path, &st)) > + return error_errno(_("could not stat %s"), path); > + if (!S_ISREG(st.st_mode)) > + return error(_("file %s is not a regular file"), path); > + if (!(st.st_mode & S_IWUSR)) > + return error(_("file %s is not writable by user"), path); This is copied from create_in_place_tempfile() replacing die() with error() - ok > + /* Create temporary file in the same directory as the original */ > + tail = strrchr(path, '/'); This is also copied from create_in_place_tempfile() but it would be better to use find_last_dir_sep() > + if (tail) > + strbuf_add(&filename_template, path, tail - path + 1); > + strbuf_addstr(&filename_template, "git-interpret-trailers-XXXXXX"); > + > + tempfile = mks_tempfile_sm(filename_template.buf, 0, st.st_mode); This could be mks_tempfile_m() like create_in_place_tempfile() as we don't have a suffix to add. > + strbuf_release(&filename_template); > + if (!tempfile) > + return error_errno(_("could not create temporary file")); > + > + outfile = fdopen_tempfile(tempfile, "w"); > + if (!outfile) { > + int saved_errno = errno; > + delete_tempfile(&tempfile); > + errno = saved_errno; > + return error_errno(_("could not open temporary file")); > + } > + > + if (buf->len && fwrite(buf->buf, 1, buf->len, outfile) < buf->len) { > + int saved_errno = errno; > + delete_tempfile(&tempfile); > + errno = saved_errno; > + return error_errno(_("could not write to temporary file")); > + } As all we're doing is writing the contents of buf we can use write_in_full() here and avoid the call to fdopen_tempfile() above. Also note that the documentation in tempfile.h says * If the program exits before `rename_tempfile()` or * `delete_tempfile()` is called, an `atexit(3)` handler will close * and remove the temporary file. so we don't need to call delete_tempfile() if the write fails. We could probably share this code with builtin/interpret-trailers.c (it no-longer needs to call fdopen_tempfile() either) which would make it more obvious that we're basically copying it here. > + if (rename_tempfile(&tempfile, path)) > + return error_errno(_("could not rename temporary file to %s"), path); > + > + return 0; > +} > + > +int amend_file_with_trailers(const char *path, > + 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 strbuf buf = STRBUF_INIT; > + struct strvec stripped_trailer_args = STRVEC_INIT; > + int ret = 0; > + size_t i; > + > + if (!trailer_args) > + BUG("amend_file_with_trailers called with NULL trailer_args"); > + if (!trailer_args->nr) > + return 0; > + > + for (i = 0; i < trailer_args->nr; i++) { > + const char *txt = trailer_args->v[i]; > + > + /* > + * Historically amend_file_with_trailers() passed its arguments > + * to "git interpret-trailers", which expected argv entries in > + * "--trailer=" form. Continue to accept those for > + * existing callers, but pass only the value portion to the > + * in-process implementation. > + */ > + skip_prefix(txt, "--trailer=", &txt); > + if (!*txt) > + die(_("empty --trailer argument")); It seems a shame to die() here when the rest of the function uses error() especially as the existing callers of amend_file_with_trailers() are not expecting it to die() and may want to perform some cleanup before exiting. > + strvec_push(&stripped_trailer_args, txt); > + } > + > + if (strbuf_read_file(&buf, path, 0) < 0) > + ret = error_errno(_("could not read '%s'"), path); > + else > + amend_strbuf_with_trailers(&buf, &stripped_trailer_args); > + > + if (!ret) > + ret = write_file_in_place(path, &buf); > + > + strvec_clear(&stripped_trailer_args); > + strbuf_release(&buf); > + return ret; > } > [...] > +/* > + * Rewrite the contents of input by processing its trailer block according to > + * opts and (optionally) appending trailers from new_trailer_head. > + * > + * The rewritten message is appended to out (callers should strbuf_reset() > + * first if needed). > + */ This comment should be part of the previous commit that adds process_trailers(). > void process_trailers(const struct process_trailer_options *opts, > struct list_head *new_trailer_head, > struct strbuf *input, struct strbuf *out); Thanks Phillip