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

Hi Phillip,

 ---- On Tue, 11 Nov 2025 00:38:55 +0800  Phillip Wood <phillip.wood123@gmail.com> wrote --- 
 > Hi Li
 > 
 > On 05/11/2025 14:29, Li Chen wrote:
 > > From: Li Chen <chenl311@chinatelecom.cn>
 > > 
 > > 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​