From: Li Chen Date: Mon, 10 Nov 2025 19:29:19 GMT Subject: Re: [PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers() Message-ID: <19a6f3e8332.46772ad5543363.4456434926857828677@linux.beauty> In-Reply-To: Hi Phillip, ---- On Tue, 11 Nov 2025 00:27:38 +0800 Phillip Wood wrote --- > On 05/11/2025 16:57, 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. > > This patch is based on my suggestion[1]. I had intended to rename "sb" > to "in" but forgot to do so before posting that diff. Here's my signoff > which Li should add before their own > > Signed-off-by: Phillip Wood I'm sorry that your signoff is missing; I will add it in the next version. Regards, Li​