Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers`

Hi Junio,

 ---- On Thu, 06 Nov 2025 01:56:00 +0800  Junio C Hamano <gitster@pobox.com> wrote --- 
 > Li Chen <me@linux.beauty> writes:
 > 
 > > 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.

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​