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

Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Nov 10, 2025, 16:38 UTC
Message-ID
<ef12ada7-13ae-4df0-a823-6f428c797223@gmail.com>
In-Reply-To
<20251105142944.73061-4-me@linux.beauty>
Hi Li
On 05/11/2025 14:29, Li Chen wrote:
Show 11 quoted lines
> From: Li Chen <chenl311@chinatelecom.cn>
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 0243f17d53..67070d6a54 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1719,7 +1719,7 @@ int cmd_commit(int argc,
>   		OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword specified commit")),
>   		OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
>   		OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
> -		OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG),

We have OPT_STRVEC to handle this. The commit message should explain why we're doing this (because we only want to pass the value to amend_file_with_trailers()). Alternatively we could use skip_prefix() in amend_file_with_trailers() to skip the "--trailer=" prefix in this patch and then clean it in a separate patch.

Show 16 quoted lines
> +		OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, parse_opt_strvec),
>   		OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),
>   		OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
>   		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index bce2e791d6..268a43372b 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> 
> @@ -142,21 +110,15 @@ static void interpret_trailers(const struct process_trailer_options *opts,
>   {
>   	struct strbuf sb = STRBUF_INIT;
>   	struct strbuf out = STRBUF_INIT;
> -	FILE *outfile = stdout;
> -
> -	trailer_config_init();
Why is this being moved?
Show 12 quoted lines
>   	read_input_file(&sb, file);
>   
> -	if (opts->in_place)
> -		outfile = create_in_place_tempfile(file);
> -
>   	process_trailers(opts, new_trailer_head, &sb, &out);
>   
> -	fwrite(out.buf, out.len, 1, outfile);
>   	if (opts->in_place)
> -		if (rename_tempfile(&trailers_tempfile, file))
> -			die_errno(_("could not rename temporary file to %s"), file);
> +		write_file_buf(file, out.buf, out.len);

This truncates the existing file which means that if there is a error while writing the new version the user is now left with garbage rather than the original file which does not seem like a good idea.

 > diff --git a/trailer.c b/trailer.c> index b735ec8a53..f5838f5699 100644
Show 32 quoted lines
> --- a/trailer.c
> +++ b/trailer.c
> 
> @@ -1224,18 +1226,66 @@ 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 *text = trailer_args->v[i];
> +		struct new_trailer_item *item;
> +
> +		if (!*text)
> +			continue;
Isn't it an error to pass an empty argument to "--trailer"?
> +		item = xcalloc(1, sizeof(*item));
> +		INIT_LIST_HEAD(&item->list);

I don't think we need this as "item->prev" and "item->next" are set by list_add_tail() below.

We initialize "where", "if_exists" and "if_missing" to zero which matches what builtin/interpret-trailers.c does if the user does not specify any of those options - good.

Show 13 quoted lines
> +		item->text = text;
> +		list_add_tail(&item->list, &new_trailer_head);
> +	}
> +
> +	process_trailers(&opts, &new_trailer_head, buf, &out);
> +
> +	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);
We have free_trailers() to do this for us.
Show 11 quoted lines
> +	}
> +	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;
Isn't it a bug to pass a NULL trailer_args?
Show 16 quoted lines
> +	if (strbuf_read_file(&buf, path, 0) < 0)
> +		return error_errno("could not read '%s'", path);
> +
> +	if (amend_strbuf_with_trailers(&buf, trailer_args)) {
> +		strbuf_release(&buf);
> +		return error("failed to append trailers");
> +	}
> +
> +	if (write_file_buf_gently(path, buf.buf, buf.len)) {
> +		strbuf_release(&buf);
> +		return -1;
> +	}
> +
> +	strbuf_release(&buf);
> +	return 0;
> + }

This looks like a faithful conversion of the original with the caveat that it expects to be passed an array of trailer arguments without the "--trailer=" prefix. Good

I'll take a look at patch 4 tomorrow but so far these version is looking much nicer than the last round.

Thanks
Phillip
Show 63 quoted lines
>   void process_trailers(const struct process_trailer_options *opts,
>   		      struct list_head *new_trailer_head,
>   		      struct strbuf *sb, struct strbuf *out)
> diff --git a/trailer.h b/trailer.h
> index 44d406b763..daea46ca5d 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -196,9 +196,8 @@ 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);
>   
> diff --git a/wrapper.c b/wrapper.c
> index 3d507d4204..1f12dbb2fa 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -688,6 +688,22 @@ void write_file_buf(const char *path, const char *buf, size_t len)
>   		die_errno(_("could not close '%s'"), path);
>   }
>   
> +int write_file_buf_gently(const char *path, const char *buf, size_t len)
> +{
> +	int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
> +
> +	if (fd < 0)
> +		return error_errno(_("could not open '%s'"), path);
> +	if (write_in_full(fd, buf, len) < 0) {
> +		int ret = error_errno(_("could not write to '%s'"), path);
> +		close(fd);
> +		return ret;
> +	}
> +	if (close(fd))
> +		return error_errno(_("could not close '%s'"), path);
> +	return 0;
> +}
> +
>   void write_file(const char *path, const char *fmt, ...)
>   {
>   	va_list params;
> diff --git a/wrapper.h b/wrapper.h
> index 44a8597ac3..e5f867b200 100644
> --- a/wrapper.h
> +++ b/wrapper.h
> @@ -56,6 +56,12 @@ static inline ssize_t write_str_in_full(int fd, const char *str)
>    */
>   void write_file_buf(const char *path, const char *buf, size_t len);
>   
> +/**
> + * Like write_file_buf(), but report errors instead of exiting. Returns 0 on
> + * success or a negative value on error after emitting a message.
> + */
> +int write_file_buf_gently(const char *path, const char *buf, size_t len);
> +
>   /**
>    * Like write_file_buf(), but format the contents into a buffer first.
>    * Additionally, write_file() will append a newline if one is not already
Previous: Li ChenNext: Li Chen
Message 13 of 24 in “rebase: support --trailer”
  1. 0/4 rebase: support --trailerLi Chen, Nov 5, 2025
  2. 1/4 interpret-trailers: factor out buffer-based processing to process_trailers()Li Chen, Nov 5, 2025
  3. Junio C HamanoNov 5, 2025
  4. Phillip WoodNov 10, 2025
  5. Li ChenNov 10, 2025
  6. Junio C HamanoNov 10, 2025
  7. Li ChenNov 10, 2025
  8. 2/4 trailer: move process_trailers to trailer.hLi Chen, Nov 5, 2025
  9. Junio C HamanoNov 5, 2025
  10. 3/4 trailer: append trailers in-process and drop the fork to `interpret-trailers`Li Chen, Nov 5, 2025
  11. Junio C HamanoNov 5, 2025
  12. Li ChenNov 10, 2025
  13. Phillip WoodNov 10, 2025
  14. Li ChenNov 10, 2025
  15. Li ChenFeb 24, 2026
  16. Phillip WoodNov 11, 2025
  17. 4/4 rebase: support --trailerLi Chen, Nov 5, 2025
  18. Phillip WoodNov 12, 2025
  19. Kristoffer HaugsbakkNov 24, 2025
  20. Junio C HamanoJan 20, 2026
  21. Junio C HamanoNov 5, 2025
  22. Li ChenNov 10, 2025
  23. Phillip WoodNov 12, 2025
  24. Li ChenNov 17, 2025

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.