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

Li Chen
2025-11-04T11:53:55Z

Hi Phillip,

---- On Thu, 23 Oct 2025 21:21:28 +0800 Phillip Wood <phillip.wood123@gmail.com> wrote ---
> Hi Li
>
> On 22/10/2025 06:39, Li Chen wrote:
> > 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>
>
> 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​