From: Li Chen Date: Mon, 10 Nov 2025 19:22:17 GMT Subject: Re: [PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers() Message-ID: <19a6f38130d.beb422c538849.8699301123463603361@linux.beauty> In-Reply-To: Hi Junio, ---- On Thu, 06 Nov 2025 00:57:27 +0800 Junio C Hamano wrote --- > Li Chen writes: > > > From: Li Chen > > > > Extracted trailer processing into a helper that accumulates output in > > a strbuf before writing. > > > > Updated interpret_trailers() to reuse the helper, buffer output, and > > clean up both input and output buffers after writing. > > Imperative? > > > > > Signed-off-by: Li Chen > > --- > > builtin/interpret-trailers.c | 51 ++++++++++++++++++++---------------- > > 1 file changed, 29 insertions(+), 22 deletions(-) > > > > diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c > > index 41b0750e5a..4c90580fff 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) > > So we gained *out strbuf; in the preimage below I see fwrite(), > fprintf(), etc. to outfile that is either stdout or tempfile, but > presumably the output all will be captured in the strbuf instead, > which makes sense. It is a bit curious what the new paramater sb > is, but this is a file-scope static helper, so it does not strictly > require documenting. Having a comment would still be nicer, though, > unlike "struct process_trailer_options" that is very limited > purpose, "strbuf" can be used for any string processing, so a good > variable name like "out" that conveys what it is used for by > implication is good, but "sb", which is obvious abbreviation for > "Str Buf", conveys no useful information. Thanks, I would rename the variable in next version. > > > { > > LIST_HEAD(head); > > - struct strbuf sb = STRBUF_INIT; > > - struct strbuf trailer_block_sb = STRBUF_INIT; > > We no longer need a separate strbuf only for trailer block; we will > see why before we read through this helper function, hopefully. > > > 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); > > OK, so the original code read the input (either "file", or standard > input) into a tempfile and prepared the output file stream. > Presumably it is now the responsibility of the caller of this new > function. Initializing the trailer configuration is also what the > caller of this function is reponsible for, as well. > > So this answers one of the questions I had upon starting to read > this function, i.e. "what is sb?" It holds the input string, which > is what? Something that look like a commit message that has title, > body and then a trailer block? We may want to give the parameter a > better name? I dunno (as this is file-scope static, as long as it > is obvious to the local caller, it may be OK, but on the other hand, > the caller needs to differenciate two strbuf parameters to the > helper function, one used for input and the other output, so if you > are calling the latter "out", perhaps you would want to call it > "in", or "input", perhaps?) Yes, in is a better name. > > > - trailer_block = parse_trailers(opts, sb.buf, &head); > > + trailer_block = parse_trailers(opts, sb->buf, &head); > > So we parse existing trailers from the input strbuf that is supplied > by the caller. The rest of this hunk is rewriting FILE* I/O with > strbuf addition. > > > @@ -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); > > The format_trailers() helper function appends appends to the strbuf > that is given to it, so instead of using an extra strbuf (and then > appending that to the output), we just pass our output strbuf to it, > which is why we no longer need the trailer_block_sb strbuf anymore. > Makes sense. > > > /* 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); > > +} > > And again, FILE* I/O is replaced with appending to the output strbuf > in the rest of this helper function. Good. > > > +static void interpret_trailers(const struct process_trailer_options *opts, > > + struct list_head *new_trailer_head, > > + const char *file) > > So the original caller of interpret_trailers() now call this outer > shell, which has the same name and the same function signature as > the original. Our new process_trailers() helper assumes a handful > of preparatory steps are already done by the caller, so what we are > going read here will be mostly those preparation, a call to our new > helper, and then printing the result to "file" or standard output. > > > +{ > > + 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); > > And these are exactly the lines we lost from the new helper. > Looking good. > > > + process_trailers(opts, new_trailer_head, &sb, &out); > > And our call. "out" should have what we wanted to output to > outfile, so ... > > > + fwrite(out.buf, out.len, 1, outfile); > > ... we write it out. Good. For a single long string that can never > have NUL in it, I'd personally find it more natural to call fputs(), > though. Use of fwrite() makes readers unnecessarily wonder if there > is something unusual (like needing to be able to handle NULs in the > buffer). > > > 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); > > OK. We could release out a bit earlier, immediately after fwrite(). > > Looking mostly good. > > > } > > > > int cmd_interpret_trailers(int argc, > Regards, Li​