From: Phillip Wood <phillip.wood123@gmail•com>
To: Junio C Hamano <gitster@pobox•com>, Li Chen <me@linux•beauty>
Cc: phillipwood <phillip.wood@dunelm•org.uk>,
git <git@vger•kernel.org>,
Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail•com>
Subject: Re: [PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers()
Date: Mon, 10 Nov 2025 16:27:38 +0000 [thread overview]
Message-ID: <f5152523-f7ff-4dee-a685-fb0b74cd6a56@gmail.com> (raw)
In-Reply-To: <xmqq1pmcmn7s.fsf@gitster.g>
On 05/11/2025 16:57, Junio C Hamano wrote:
> Li Chen <me@linux•beauty> writes:
>
>> 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?
>
>>
>> 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.
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 <phillip.wood@dunelm•org.uk>
Thanks
Phillip
[1]
https://lore.kernel.org/git/7d12b046-365f-441c-af8e-8a39d61efbbd@gmail.com
>> {
>> 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?)
>
>> - 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,
>
next prev parent reply other threads:[~2025-11-10 16:27 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-05 14:29 [PATCH v6 0/4] rebase: support --trailer Li Chen
2025-11-05 14:29 ` [PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers() Li Chen
2025-11-05 16:57 ` Junio C Hamano
2025-11-10 16:27 ` Phillip Wood [this message]
2025-11-10 19:29 ` Li Chen
2025-11-10 22:08 ` Junio C Hamano
2025-11-10 19:22 ` Li Chen
2025-11-05 14:29 ` [PATCH v6 2/4] trailer: move process_trailers to trailer.h Li Chen
2025-11-05 17:38 ` Junio C Hamano
2025-11-05 14:29 ` [PATCH v6 3/4] trailer: append trailers in-process and drop the fork to `interpret-trailers` Li Chen
2025-11-05 17:56 ` Junio C Hamano
2025-11-10 19:27 ` Li Chen
2025-11-10 16:38 ` Phillip Wood
2025-11-10 19:14 ` Li Chen
2026-02-24 6:36 ` Li Chen
2025-11-11 16:55 ` Phillip Wood
2025-11-05 14:29 ` [PATCH v6 4/4] rebase: support --trailer Li Chen
2025-11-12 14:48 ` Phillip Wood
2025-11-24 15:45 ` Kristoffer Haugsbakk
2026-01-20 20:49 ` Junio C Hamano
2025-11-05 16:30 ` [PATCH v6 0/4] " Junio C Hamano
2025-11-10 19:17 ` Li Chen
2025-11-12 14:50 ` Phillip Wood
2025-11-17 3:38 ` Li Chen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f5152523-f7ff-4dee-a685-fb0b74cd6a56@gmail.com \
--to=phillip.wood123@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=kristofferhaugsbakk@fastmail$(echo .)com \
--cc=me@linux$(echo .)beauty \
--cc=phillip.wood@dunelm$(echo .)org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox