Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 5, 2025, 17:56 UTC
- Message-ID
- <xmqqqzucl5xr.fsf@gitster.g>
- In-Reply-To
- <20251105142944.73061-4-me@linux.beauty>
Li Chen <me@linux.beauty> writes:
Show 31 quoted lines
> From: Li Chen <chenl311@chinatelecom.cn>
>
> Route all trailer insertion through trailer_process() and make
> builtin/interpret-trailers just do file I/O before calling into it.
> amend_file_with_trailers() now shares the same code path.
>
> This removes the fork/exec and tempfile juggling, cutting overhead and
> simplifying error handling. No functional change. It also
> centralizes logic to prepare for follow-up rebase --trailer patch.
>
> Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
> ---
> builtin/commit.c | 2 +-
> builtin/interpret-trailers.c | 46 +++---------------------
> builtin/tag.c | 3 +-
> trailer.c | 68 +++++++++++++++++++++++++++++++-----
> trailer.h | 5 ++-
> wrapper.c | 16 +++++++++
> wrapper.h | 6 ++++
> 7 files changed, 90 insertions(+), 56 deletions(-)
>
> 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),
> + OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, parse_opt_strvec),What is this change for?
As the external interface of the amend_file_with_trailers() helper did not change in this patch, this cannot be a change that is required to "remove fork/exec and tempfile juggling".
Or did amend_file_with_trailers() changed behaviour without changing its function signature? If so, this patch does too many things in a single step, I am afraid.
Perhaps split this step further into multiple patches.
- update the internal implementation of amend_file_with_trailers() to avoid having to fork/exec an external process, but *without* changing its external interface at all. This step should not have to touch builtin/commit.c and builtin/tag.c at all.
- if the strvec styled after passthru-argv is cumbersome to handle, perform the interface change, such as change from passthru-argv to bare strvec, as a separate step.
There might need another preparatory step to clean up the interpret-trailers.c itself before the above two (or there may not be---I haven't thought it through).
Show 7 quoted lines
> 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) > ... > +int write_file_buf_gently(const char *path, const char *buf, size_t len)
I do not think this new helper is warranted. You only call it from one place anyway.