Re: [PATCH v5 01/29] trailer: append trailers in-process and drop the fork to `interpret-trailers`

Hi Phillip,

 ---- On Thu, 23 Oct 2025 21:21:28 +0800  Phillip Wood [off-list ref] wrote --- 
 > Hi Li
 > 
 > On 22/10/2025 06:39, Li Chen wrote:
 > > From: Li Chen [off-list ref]
 > > 
 > > 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 [off-list ref]
 > 
 > When I review v3 of this series I said
 > 
 > >> As I said above reusing the existing code as you have done here is a
 > >> much better approach. However it would be much easier to review if
 > >> the code movement was separated from the refactoring. I'm also
 > >> struggling to see the benefit of a lot of the refactoring - I was
 > >> expecting the conversion to use an strubf would essentially look like
 > >> fwrite() being replaced with strbuf_add() and fprintf() being
 > >> replaced with strbuf_addf() etc. rather than reworking the logic.
 > 
 > Unfortunately this version has the same code changes as v3 that make it
 > are virtually impossible to verify if the behavior is changed or not.
 > The diff below shows what I was hoping to see as the first step. It
 > refactors interpret_trailers() in place to factor out the code that
 > processes the trailers into a separate function. That makes it easy to
 > see that fwrite() is replaced with strbuf_add() etc. and so verify that
 > the behavior is unchanged. If you view the diff with "--color-moved"
 > you'll see that virtually all of the code in the new
 > interpret_trailers() function is moved from the old one which in turn
 > makes it easy to verify that there is no change in behavior. I would
 > expect the next patch in the series to move the new function for
 > processing trailers into trailer.c and the patch after that to refactor
 > amend_file_with_trailers() to add and use amend_strbuf_with_trailers()
 > and stop forking "git interpret-trailers". Then builtin/rebase.c can be
 > modified to add support for trailers using amend_strbuf_with_trailers().
 > 
 > Thanks
 > 
 > Phillip
 > 
 > ---- 8< ----
 > diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
 > index 41b0750e5af..4c90580ffff 100644
 > --- a/builtin/interpret-trailers.c
 > +++ b/builtin/interpret-trailers.c
 > @@ -136,32 +136,21 @@ static void read_input_file(struct strbuf *sb, const char *file)
 >       strbuf_complete_line(sb);
 >   }
 >   
 > -static void interpret_trailers(const struct process_trailer_options *opts,
 > -                   struct list_head *new_trailer_head,
 > -                   const char *file)
 > +static void process_trailers(const struct process_trailer_options *opts,
 > +                 struct list_head *new_trailer_head,
 > +                 struct strbuf *sb, struct strbuf *out)
 >   {
 >       LIST_HEAD(head);
 > -    struct strbuf sb = STRBUF_INIT;
 > -    struct strbuf trailer_block_sb = STRBUF_INIT;
 >       struct trailer_block *trailer_block;
 > -    FILE *outfile = stdout;
 > -
 > -    trailer_config_init();
 > -
 > -    read_input_file(&sb, file);
 > -
 > -    if (opts->in_place)
 > -        outfile = create_in_place_tempfile(file);
 > -
 > -    trailer_block = parse_trailers(opts, sb.buf, &head);
 > +
 > +    trailer_block = parse_trailers(opts, sb->buf, &head);
 >   
 >       /* Print the lines before the trailer block */
 >       if (!opts->only_trailers)
 > -        fwrite(sb.buf, 1, trailer_block_start(trailer_block), outfile);
 > +        strbuf_add(out, sb->buf, trailer_block_start(trailer_block));
 >   
 >       if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
 > -        fprintf(outfile, "\n");
 > -
 > +        strbuf_addch(out, '\n');
 >   
 >       if (!opts->only_input) {
 >           LIST_HEAD(config_head);
 > @@ -173,22 +162,40 @@ static void interpret_trailers(const struct process_trailer_options *opts,
 >       }
 >   
 >       /* Print trailer block. */
 > -    format_trailers(opts, &head, &trailer_block_sb);
 > +    format_trailers(opts, &head, out);
 >       free_trailers(&head);
 > -    fwrite(trailer_block_sb.buf, 1, trailer_block_sb.len, outfile);
 > -    strbuf_release(&trailer_block_sb);
 >   
 >       /* Print the lines after the trailer block as is. */
 >       if (!opts->only_trailers)
 > -        fwrite(sb.buf + trailer_block_end(trailer_block), 1,
 > -               sb.len - trailer_block_end(trailer_block), outfile);
 > +        strbuf_add(out, sb->buf + trailer_block_end(trailer_block),
 > +               sb->len - trailer_block_end(trailer_block));
 >       trailer_block_release(trailer_block);
 > -
 > +}
 > +
 > +static void interpret_trailers(const struct process_trailer_options *opts,
 > +                   struct list_head *new_trailer_head,
 > +                   const char *file)
 > +{
 > +    struct strbuf sb = STRBUF_INIT;
 > +    struct strbuf out = STRBUF_INIT;
 > +    FILE *outfile = stdout;
 > +
 > +    trailer_config_init();
 > +
 > +    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);
 >   
 >       strbuf_release(&sb);
 > +    strbuf_release(&out);
 >   }
 >   
 >   int cmd_interpret_trailers(int argc,
 > 

Thank you for your suggestion and kindness. I will re-implement the first patch based on your change and
avoid the die in interpret_trailers (as suggested in v3) and replacing the fwrite with strbuf_write (also suggested in v3).

And then add patches as you said above.

Regards,

Li​