Re: [PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 5, 2025, 16:57 UTC
- Message-ID
- <xmqq1pmcmn7s.fsf@gitster.g>
- In-Reply-To
- <20251105142944.73061-2-me@linux.beauty>
Li Chen <me@linux.beauty> writes:
Show 7 quoted lines
> From: Li Chen <chenl311@chinatelecom.cn> > > 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?
Show 20 quoted lines
> > Signed-off-by: Li Chen <chenl311@chinatelecom.cn> > --- > 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.
> {
> 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.
Show 9 quoted lines
> 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?)
> - 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.
Show 9 quoted lines
> @@ -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.
Show 8 quoted lines
> /* 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.
Show 10 quoted lines
> +{
> + 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).
Show 6 quoted lines
> 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,