From: Phillip Wood <phillip.wood123@gmail•com>
To: Li Chen <me@linux•beauty>,
phillipwood <phillip.wood@dunelm•org.uk>,
git <git@vger•kernel.org>, Junio C Hamano <gitster@pobox•com>,
Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail•com>
Subject: Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`
Date: Mon, 10 Nov 2025 16:38:55 +0000 [thread overview]
Message-ID: <ef12ada7-13ae-4df0-a823-6f428c797223@gmail.com> (raw)
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/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.
> + 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?
> 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
> --- 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.
> + 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.
> + }
> + 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?
> + 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
> 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
next prev parent reply other threads:[~2025-11-10 16:38 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-05 14:29 [PATCH v6 0/4] rebase: support --trailer Li Chen
2025-11-05 14:29 ` [PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers() Li Chen
2025-11-05 16:57 ` Junio C Hamano
2025-11-10 16:27 ` Phillip Wood
2025-11-10 19:29 ` Li Chen
2025-11-10 22:08 ` Junio C Hamano
2025-11-10 19:22 ` Li Chen
2025-11-05 14:29 ` [PATCH v6 2/4] trailer: move process_trailers to trailer.h Li Chen
2025-11-05 17:38 ` Junio C Hamano
2025-11-05 14:29 ` [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers` Li Chen
2025-11-05 17:56 ` Junio C Hamano
2025-11-10 19:27 ` Li Chen
2025-11-10 16:38 ` Phillip Wood [this message]
2025-11-10 19:14 ` Li Chen
2026-02-24 6:36 ` Li Chen
2025-11-11 16:55 ` Phillip Wood
2025-11-05 14:29 ` [PATCH v6 4/4] rebase: support --trailer Li Chen
2025-11-12 14:48 ` Phillip Wood
2025-11-24 15:45 ` Kristoffer Haugsbakk
2026-01-20 20:49 ` Junio C Hamano
2025-11-05 16:30 ` [PATCH v6 0/4] " Junio C Hamano
2025-11-10 19:17 ` Li Chen
2025-11-12 14:50 ` Phillip Wood
2025-11-17 3:38 ` Li Chen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ef12ada7-13ae-4df0-a823-6f428c797223@gmail.com \
--to=phillip.wood123@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=kristofferhaugsbakk@fastmail$(echo .)com \
--cc=me@linux$(echo .)beauty \
--cc=phillip.wood@dunelm$(echo .)org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox