From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 11 Nov 2025 16:55:38 GMT
Subject: Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`
Message-ID: <e13be93f-9d10-4baf-b333-d293c5f46fb5@gmail.com>
In-Reply-To: <20251105142944.73061-4-me@linux.beauty>

Hi Li

On 05/11/2025 14:29, Li Chen wrote:
> From: Li Chen <chenl311@chinatelecom.cn>
> 
> diff --git a/trailer.c b/trailer.c
> index b735ec8a53..f5838f5699 100644
> --- 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)

While reviewing patch 4 I've just realized that this function can never 
fail so should return "void" rather than "int". I've not quite finished 
with patch 4 yet, hopefully I'll post a review tomorrow.

Thanks

Phillip

>   {
> -	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;
> +		item = xcalloc(1, sizeof(*item));
> +		INIT_LIST_HEAD(&item->list);
> +		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);
> +	}
> +	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)) {
> +		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;
> + }
> +
>   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