From: Li Chen Date: Tue, 04 Nov 2025 11:53:42 GMT Subject: Re: [PATCH v5 01/29] trailer: append trailers in-process and drop the fork to `interpret-trailers` Message-ID: <19a4eb739bb.71f7f93b817785.4090964595844940221@linux.beauty> In-Reply-To: <7d12b046-365f-441c-af8e-8a39d61efbbd@gmail.com> Hi Phillip, ---- On Thu, 23 Oct 2025 21:21:28 +0800 Phillip Wood wrote --- > Hi Li > > On 22/10/2025 06:39, Li Chen wrote: > > From: Li Chen > > > > 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 > > 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​