diff mbox series

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

Message ID fc5fd5217dfc105f3e03a9800a35209a985775a4.1611954543.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Unify trailers formatting logic for pretty.c and ref-filter.c | expand

Commit Message

Hariom verma Jan. 29, 2021, 9:09 p.m. UTC 
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(-)

Comments

Junio C Hamano Jan. 29, 2021, 11:49 p.m. UTC | #1 
"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.
diff mbox series

Patch

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;
+
+			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;
+		} else if (!match_placeholder_bool_arg(*arg, "only", arg, &opts->only_trailers) &&
+			   !match_placeholder_bool_arg(*arg, "unfold", arg, &opts->unfold) &&
+			   !match_placeholder_bool_arg(*arg, "keyonly", arg, &opts->key_only) &&
+			   !match_placeholder_bool_arg(*arg, "valueonly", arg, &opts->value_only))
+			break;
+	}
+	return 0;
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1425,45 +1469,8 @@  static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 
 		if (*arg == ':') {
 			arg++;
-			for (;;) {
-				const char *argval;
-				size_t arglen;
-
-				if (match_placeholder_arg_value(arg, "key", &arg, &argval, &arglen)) {
-					uintptr_t len = arglen;
-
-					if (!argval)
-						goto trailer_out;
-
-					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;
-
-					strbuf_reset(&sepbuf);
-					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;
-
-					strbuf_reset(&kvsepbuf);
-					fmt = xstrndup(argval, arglen);
-					strbuf_expand(&kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);
-					free(fmt);
-					opts.key_value_separator = &kvsepbuf;
-				} else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
-					   !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) &&
-					   !match_placeholder_bool_arg(arg, "keyonly", &arg, &opts.key_only) &&
-					   !match_placeholder_bool_arg(arg, "valueonly", &arg, &opts.value_only))
-					break;
-			}
+			if (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg))
+				goto trailer_out;
 		}
 		if (*arg == ')') {
 			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
diff --git a/pretty.h b/pretty.h
index 7ce6c0b437b..7369cf7e148 100644
--- a/pretty.h
+++ b/pretty.h
@@ -6,6 +6,7 @@ 
 
 struct commit;
 struct strbuf;
+struct process_trailer_options;
 
 /* Commit formats */
 enum cmit_fmt {
@@ -142,4 +143,14 @@  int commit_format_is_empty(enum cmit_fmt);
 /* Make subject of commit message suitable for filename */
 void format_sanitized_subject(struct strbuf *sb, const char *msg, size_t len);
 
+/*
+ * Set values of fields in "struct process_trailer_options"
+ * according to trailers arguments.
+ */
+int format_set_trailers_options(struct process_trailer_options *opts,
+			struct string_list *filter_list,
+			struct strbuf *sepbuf,
+			struct strbuf *kvsepbuf,
+			const char **arg);
+
 #endif /* PRETTY_H */