Re: [PATCH v2 1/3] pretty.c: refactor trailer logic to `format_set_trailers_options()`
From: Junio C Hamano <hidden>
Date: 2021-01-29 23:51:08
"Hariom Verma via GitGitGadget" [off-list ref] writes:
quoted hunk
From: Hariom Verma <redacted> 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 [off-list ref] Mentored-by: Heba Waly [off-list ref] Signed-off-by: Hariom Verma <redacted> --- 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?
quoted hunk
+ 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.