From: Li Chen Date: Mon, 10 Nov 2025 19:14:36 GMT Subject: Re: [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers` Message-ID: <19a6f310cc5.17364397534057.8623048406766685580@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. Thanks for the reminder, I will try to split into two patches in the next reversion. The first one use skip_prefix() in amend_file_with_trailers(), and the second one switch to amend_file_with_trailers(). > > > + 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? Since trailer_config_init only needs to run once, it's better to move it outside the cmd_interpret_trailers loop, even though it already uses a configured global variable. > > 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. Thanks for catching this. I'll switch back to using a temp file for atomic. > > > diff --git a/trailer.c b/trailer.c> index b735ec8a53..f5838f5699 100644 > > --- a/trailer.c > > +++ b/trailer.c > > > > @@ -1224,18 +1226,66 @@ void trailer_iterator_release(struct trailer_iterator *iter) > > strbuf_release(&iter->key); > > } > > > > -int amend_file_with_trailers(const char *path, const struct strvec *trailer_args) > > +static int amend_strbuf_with_trailers(struct strbuf *buf, > > + const struct strvec *trailer_args) > > { > > - struct child_process run_trailer = CHILD_PROCESS_INIT; > > - > > - run_trailer.git_cmd = 1; > > - strvec_pushl(&run_trailer.args, "interpret-trailers", > > - "--in-place", "--no-divider", > > - path, NULL); > > - strvec_pushv(&run_trailer.args, trailer_args->v); > > - return run_command(&run_trailer); > > + struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; > > + LIST_HEAD(new_trailer_head); > > + struct strbuf out = STRBUF_INIT; > > + size_t i; > > + > > + opts.no_divider = 1; > > + > > + for (i = 0; i < trailer_args->nr; i++) { > > + const char *text = trailer_args->v[i]; > > + struct new_trailer_item *item; > > + > > + if (!*text) > > + continue; > > Isn't it an error to pass an empty argument to "--trailer"? Nice catch, I would refactor amend_strbuf_with_trailers to return error(_("empty --trailer argument")); here and handle resource cleanup then make amend_file_with_trailers return this error. > > + item = xcalloc(1, sizeof(*item)); > > + INIT_LIST_HEAD(&item->list); > > I don't think we need this as "item->prev" and "item->next" are set by > list_add_tail() below. ok, I would remove this. > We initialize "where", "if_exists" and "if_missing" to zero which > matches what builtin/interpret-trailers.c does if the user does not > specify any of those options - good. > > > + item->text = text; > > + list_add_tail(&item->list, &new_trailer_head); > > + } > > + > > + process_trailers(&opts, &new_trailer_head, buf, &out); > > + > > + strbuf_swap(buf, &out); > > + strbuf_release(&out); > > + while (!list_empty(&new_trailer_head)) { > > + struct new_trailer_item *item = > > + list_first_entry(&new_trailer_head, struct new_trailer_item, list); > > + list_del(&item->list); > > + free(item); > > We have free_trailers() to do this for us. I would replace them with free_trailers. > > + } > > + return 0; > > } > > > > +int amend_file_with_trailers(const char *path, > > + const struct strvec *trailer_args) > > +{ > > + struct strbuf buf = STRBUF_INIT; > > + > > + if (!trailer_args || !trailer_args->nr) > > + return 0; > > Isn't it a bug to pass a NULL trailer_args? Sounds right, I would let it return an error msg. > > + if (strbuf_read_file(&buf, path, 0) < 0) > > + return error_errno("could not read '%s'", path); > > + > > + if (amend_strbuf_with_trailers(&buf, trailer_args)) { > > + strbuf_release(&buf); > > + return error("failed to append trailers"); > > + } > > + > > + if (write_file_buf_gently(path, buf.buf, buf.len)) { > > + strbuf_release(&buf); > > + return -1; > > + } > > + > > + strbuf_release(&buf); > > + return 0; > > + } > > This looks like a faithful conversion of the original with the caveat > that it expects to be passed an array of trailer arguments without the > "--trailer=" prefix. Good Okay, thanks. Junio notes that write_file_buf_gently is only used in this context and doesn't need wrok as a helper function. I'll replace it with an in-place operation. > I'll take a look at patch 4 tomorrow but so far these version is looking > much nicer than the last round. Thanks a lot. Regards, Li​