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

[Junio C Hamano <gitster@pobox.com>]

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.

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).

> 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.