Re: [PATCH v2 1/3] pretty.c: refactor trailer logic to `format_set_trailers_options()`

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

"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Hariom Verma <hariom18599@gmail.com>
>
> Refactored trailers formatting logic inside pretty.c to a new function
> `format_set_trailers_options()`. This change will allow us to reuse
> the same logic in other places.
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Heba Waly <heba.waly@gmail.com>
> Signed-off-by: Hariom Verma <hariom18599@gmail.com>
> ---
>  pretty.c | 85 ++++++++++++++++++++++++++++++--------------------------
>  pretty.h | 11 ++++++++
>  2 files changed, 57 insertions(+), 39 deletions(-)
>
> diff --git a/pretty.c b/pretty.c
> index 3922f6f9f24..bb6a3c634ac 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1148,6 +1148,50 @@ static int format_trailer_match_cb(const struct strbuf *key, void *ud)
>  	return 0;
>  }
>  
> +int format_set_trailers_options(struct process_trailer_options *opts,
> +				struct string_list *filter_list,
> +				struct strbuf *sepbuf,
> +				struct strbuf *kvsepbuf,
> +				const char **arg)
> +{
> +	for (;;) {
> +		const char *argval;
> +		size_t arglen;
> +
> +		if (match_placeholder_arg_value(*arg, "key", arg, &argval, &arglen)) {
> +			uintptr_t len = arglen;
> +
> +			if (!argval)
> +				return 1;

The convention in this codebase is to signal unusual/error return
with a negative value, especially when a successful exit is signaled
by returning 0.  Perhaps return -1 from here instead?

> +			if (len && argval[len - 1] == ':')
> +				len--;
> +			string_list_append(filter_list, argval)->util = (char *)len;
> +
> +			opts->filter = format_trailer_match_cb;
> +			opts->filter_data = filter_list;
> +			opts->only_trailers = 1;
> +		} else if (match_placeholder_arg_value(*arg, "separator", arg, &argval, &arglen)) {
> +			char *fmt;
> +			fmt = xstrndup(argval, arglen);
> +			strbuf_expand(sepbuf, fmt, strbuf_expand_literal_cb, NULL);
> +			free(fmt);
> +			opts->separator = sepbuf;
> +		} else if (match_placeholder_arg_value(*arg, "key_value_separator", arg, &argval, &arglen)) {
> +			char *fmt;
> +			fmt = xstrndup(argval, arglen);
> +			strbuf_expand(kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);
> +			free(fmt);
> +			opts->key_value_separator = kvsepbuf;

In these two else-if clauses, the original code clears sepbuf and
kvsepbuf before calling strbuf_expand(), but this one does not.

As strbuf_expand() is an appending function, this distinction would
matter if the for(;;) loop causes these two else-if clauses to be
entered twice.  The original code will implement the "last one wins"
semantics, and this new one acumulates what it sees.

Intended?  If so, the reason why we want the accumulating semantics,
instead of the last-one-wins we've been using, needs to be explained
in the log message.

> -				} else if (match_placeholder_arg_value(arg, "separator", &arg, &argval, &arglen)) {
> -					char *fmt;
> -
> -					strbuf_reset(&sepbuf);
> -					fmt = xstrndup(argval, arglen);
> -					strbuf_expand(&sepbuf, fmt, strbuf_expand_literal_cb, NULL);
> -					free(fmt);
> -					opts.separator = &sepbuf;

Here you can see that the original clears what was in sepbuf before
reading the separator anew.

Thanks.