Re: [PATCH v2 1/3] pretty.c: refactor trailer logic to `format_set_trailers_options()`
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 29, 2021, 23:49 UTC
- Message-ID
- <xmqqwnvvqp5o.fsf@gitster.c.googlers.com>
- In-Reply-To
- <fc5fd5217dfc105f3e03a9800a35209a985775a4.1611954543.git.gitgitgadget@gmail.com>
"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 37 quoted lines
> 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?
Show 19 quoted lines
> + 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.
Show 8 quoted lines
> - } 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.