From: Li Chen Date: Mon, 10 Nov 2025 19:27:40 GMT Subject: Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers` Message-ID: <19a6f3cffe1.7bb06d38542332.5515171012908783042@linux.beauty> In-Reply-To: Hi Junio, ---- On Thu, 06 Nov 2025 01:56:00 +0800 Junio C Hamano wrote --- > Li Chen writes: > > > From: Li Chen > > > > 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 > > --- > > 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. it allows remove the use of skip_prefix in amend_file_with_trailers, and I would add seperate patches to make this clearer. > 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). Thanks, I would split into multiple patches in next version. > > > 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. ok, I would remove write_file_buf_gently and do it in-place. Regards, Li​