Re: [PATCH 03/10] trailer: unify trailer formatting machinery
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 18, 2024, 22:56 UTC
- Message-ID
- <xmqqr0ientdb.fsf@gitster.g>
- In-Reply-To
- <d3326021fb6a63707e4ce56f166447143f4e55a2.1704869487.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 19 quoted lines
> 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.
Show 7 quoted 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(-)
Show 10 quoted lines
> 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.
Show 38 quoted lines
> 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.