"Linus Arver via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Linus Arver <linusa@xxxxxxxxxx>
>
> 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.
