Re: [PATCH 03/10] trailer: unify trailer formatting machinery

From: Junio C Hamano <gitster@pobox.com>
To: "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,  Emily Shaffer <nasamuffin@google.com>,
	 Christian Couder <chriscool@tuxfamily.org>,
	 Linus Arver <linusa@google.com>
Subject: Re: [PATCH 03/10] trailer: unify trailer formatting machinery
Date: Thu, 18 Jan 2024 14:56:16 -0800	[thread overview]
Message-ID: <xmqqr0ientdb.fsf@gitster.g> (raw)
In-Reply-To: <d3326021fb6a63707e4ce56f166447143f4e55a2.1704869487.git.gitgitgadget@gmail.com> (Linus Arver via GitGitGadget's message of "Wed, 10 Jan 2024 06:51:19 +0000")

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Linus Arver <linusa@google.com>
>
> Currently have two functions for formatting trailers exposed in
> trailer.h:
>
>     void format_trailers(FILE *outfile, struct list_head *head,
>                         const struct process_trailer_options *opts);
>
>     void format_trailers_from_commit(struct strbuf *out, const char *msg,
>                                     const struct process_trailer_options *opts);
>
> and previously these functions, although similar enough (even taking the
> same process_trailer_options struct pointer), did not build on each
> other.
>
> Make format_trailers_from_commit() rely on format_trailers(). Teach
> format_trailers() to process trailers with the additional
> process_trailer_options fields like opts->key_only which is only used by
> format_trailers_from_commit() and not builtin/interpret-trailers.c.

Yay.  It feels a bit disappointing to see the diffstat and learn
that we are not deleting substantial number of lines.

> ---
>  builtin/interpret-trailers.c |   5 +-
>  pretty.c                     |   2 +-
>  ref-filter.c                 |   2 +-
>  trailer.c                    | 105 +++++++++++++++++++++++++++++------
>  trailer.h                    |  21 +++----
>  5 files changed, 102 insertions(+), 33 deletions(-)

> diff --git a/pretty.c b/pretty.c
> index cf964b060cd..f0721a5214f 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1759,7 +1759,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  				goto trailer_out;
>  		}
>  		if (*arg == ')') {
> -			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
> +			format_trailers_from_commit(msg + c->subject_off, &opts, sb);

I am curious (read: no objection---merely wondering if there is a
guiding principle behind the choice of the new order) why this new
parameter ordering.  I suspect it was originally written with a
strbuf-centric worldview and having sb at the beginning may have
made sense, but if we are moving them around, wouldn't it make more
sense to have &opts as the first parameter, as this is primarily a
"trailers" function?  Unsure until I read through to the end, but
that is my gut reaction.

>  static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
>  {
>  	struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));
> @@ -984,6 +971,78 @@ static void unfold_value(struct strbuf *val)
>  	strbuf_release(&out);
>  }
>  
> +void format_trailers(struct list_head *head,
> +		     const struct process_trailer_options *opts,
> +		     struct strbuf *out)
> +{
> +	struct list_head *pos;
> +	struct trailer_item *item;
> +	int need_separator = 0;
> +
> +	list_for_each(pos, head) {
> +		item = list_entry(pos, struct trailer_item, list);
> +		if (item->token) {
> +			char c;
> + ...
> +			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
> +				if (opts->unfold)
> +					unfold_value(&val);
> +
> +				if (opts->separator && need_separator)
> +					strbuf_addbuf(out, opts->separator);
> +				if (!opts->value_only)
> +					strbuf_addbuf(out, &tok);
> +				if (!opts->key_only && !opts->value_only) {
> +					if (opts->key_value_separator)
> +						strbuf_addbuf(out, opts->key_value_separator);
> +					else {
> +						c = last_non_space_char(tok.buf);
> +						if (c) {
> +							if (!strchr(separators, c))
> +								strbuf_addf(out, "%c ", separators[0]);
> +						}
> +					}

That's an overly deep nesting.  I wonder if a small file-scope
helper function is in order?

	static add_separator(struct process_trailer_options *opts,
        		     const char *token
			     struct strbuf *out)
	{
		if (opts->key_value_separator)
			strbuf_addbuf(out, opts->key_value_separator);
		else
			strbuf_addstr(out, ": ");
	}

Or perhaps inside the context of the loop to go over the list of
trailer items, one iteration of the list_for_each() loop can become
one call to a small helper function format_one_trailer() and that
may make the result easier to follow?

In any case, I didn't see anything glaringly wrong so far in this
series.  Let me keep reading.

Thanks.