From: Li Chen Date: Tue, 24 Feb 2026 06:36:13 GMT Subject: Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers` Message-ID: <19c8e5cd208.389d71793180723.4083726630202768168@linux.beauty> In-Reply-To: Hi Phillip, ---- On Tue, 11 Nov 2025 00:38:55 +0800 Phillip Wood wrote --- > Hi Li > > On 05/11/2025 14:29, Li Chen wrote: > > From: Li Chen > > > > 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? In v7 I'll initialize trailer config once in cmd_interpret_trailers() (after option parsing), and keep interpret_trailers() focused on read input / call helper / emit output. > > 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. Great catch! v7 will keep --in-place writing via tempfile+rename (no truncate+write), matching the previous behavior. > ... Regards, Li​