git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v7 3/5] trailer: append trailers without fork/exec

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Mar 2, 2026, 14:56 UTC
Message-ID
<e555e29f-57e3-4192-98f3-380181c81fd2@gmail.com>
In-Reply-To
<20260224070552.148591-4-me@linux.beauty>
Hi Li
On 24/02/2026 07:05, Li Chen wrote:
Show 11 quoted lines
> 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=<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.

Show 30 quoted lines
> Signed-off-by: Li Chen <me@linux.beauty>
> ---
> 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.
Show 34 quoted lines
>   	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()?

Show 17 quoted lines
>   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/

Show 16 quoted lines
> +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.

Show 28 quoted lines
> +		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()

Show 5 quoted lines
> +	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.

Show 18 quoted lines
> +	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.

Show 40 quoted lines
> +	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=<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.

Show 15 quoted lines
> +		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;
>   }
Show 8 quoted lines
> [...]   
> +/*
> + * 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
Previous: Li ChenNext: Li Chen
Message 8 of 32 in “rebase: support --trailer”
  1. 0/5 rebase: support --trailerLi Chen, Feb 24, 2026
  2. 1/5 interpret-trailers: factor trailer rewritingLi Chen, Feb 24, 2026
  3. Phillip WoodMar 2, 2026
  4. Li ChenMar 2, 2026
  5. 2/5 trailer: move process_trailers to trailer.hLi Chen, Feb 24, 2026
  6. phillip.wood123@gmail.comMar 2, 2026
  7. 3/5 trailer: append trailers without fork/execLi Chen, Feb 24, 2026
  8. Phillip WoodMar 2, 2026
  9. 4/5 commit, tag: parse --trailer with OPT_STRVECLi Chen, Feb 24, 2026
  10. Phillip WoodMar 2, 2026
  11. 5/5 rebase: support --trailerLi Chen, Feb 24, 2026
  12. Phillip WoodMar 3, 2026
  13. Kristoffer HaugsbakkMar 3, 2026
  14. Junio C HamanoMar 3, 2026
  15. Phillip WoodMar 4, 2026
  16. Junio C HamanoMar 4, 2026
  17. Junio C HamanoFeb 26, 2026
  18. Phillip WoodFeb 26, 2026
  19. Kristoffer HaugsbakkFeb 26, 2026
  20. Phillip WoodMar 4, 2026
  21. Li ChenMar 5, 2026
  22. Phillip WoodMar 6, 2026
  23. 0/6 rebase: support --trailerPhillip Wood, Mar 6, 2026
  24. 1/6 interpret-trailers: factor trailer rewritingPhillip Wood, Mar 6, 2026
  25. Junio C HamanoMar 6, 2026
  26. Phillip WoodMar 9, 2026
  27. 2/6 interpret-trailers: refactor create_in_place_tempfile()Phillip Wood, Mar 6, 2026
  28. Junio C HamanoMar 6, 2026
  29. 3/6 trailer: libify a couple of functionsPhillip Wood, Mar 6, 2026
  30. 4/6 trailer: append trailers without fork/execPhillip Wood, Mar 6, 2026
  31. 5/6 commit, tag: parse --trailer with OPT_STRVECPhillip Wood, Mar 6, 2026
  32. 6/6 rebase: support --trailerPhillip Wood, Mar 6, 2026

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.