Re: [PATCH v6 1/4] interpret-trailers: factor out buffer-based processing to process_trailers()

[Junio C Hamano <gitster@pobox.com>]

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.

>  {
>  	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,