Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Nov 11, 2025, 16:55 UTC
- 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:
Show 14 quoted lines
> 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
Show 129 quoted lines
> {
> - 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