[v4,26/28] trailer: unify "--trailer ..." arg handling

Commit Message

Linus Arver Feb. 6, 2024, 5:12 a.m. UTC

From: Linus Arver <[e-mail]>

Move the logic of parse_trailer_from_command_line_arg() into
option_parse_trailer(), because that is the only caller and there's no
benefit in keeping these two separate.

Signed-off-by: Linus Arver <[e-mail]>
---
 builtin/interpret-trailers.c | 42 +++++++++++++++++++++++++++++-
 trailer.c                    | 50 ------------------------------------
 trailer.h                    |  6 -----
 3 files changed, 41 insertions(+), 57 deletions(-)

Comments

Christian Couder Feb. 12, 2024, 11:39 p.m. UTC | #1

On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
<[e-mail]> wrote:
>
> From: Linus Arver <[e-mail]>
>
> Move the logic of parse_trailer_from_command_line_arg() into
> option_parse_trailer(), because that is the only caller and there's no
> benefit in keeping these two separate.

Well one benefit could be that 2 small functions might be easier to
understand than a big one. So perhaps
parse_trailer_from_command_line_arg() could just have been made
static?

Linus Arver Feb. 13, 2024, 6:12 p.m. UTC | #2

Christian Couder <[e-mail]> writes:

> On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
> <[e-mail]> wrote:
>>
>> From: Linus Arver <[e-mail]>
>>
>> Move the logic of parse_trailer_from_command_line_arg() into
>> option_parse_trailer(), because that is the only caller and there's no
>> benefit in keeping these two separate.
>
> Well one benefit could be that 2 small functions might be easier to
> understand than a big one.

True.

> So perhaps
> parse_trailer_from_command_line_arg() could just have been made
> static?

In this case I don't think keeping these two functions separate would
make sense because parse_trailer_from_command_line_arg() is much more
heavyweight than option_parse_trailer() (one is just a thin wrapper
around the other). And I didn't like the thought of having 2 function
names that look very different:

    parse_trailer_from_command_line_arg()
    option_parse_trailer()

be so closely related in behavior.

And I already have some more patches (not in this series) that refactors
this area a bit also, so I wanted to wait until the dust settled down a
bit before deciding (esp. when unit tests are added) whether keep this
function separate. It's not clear to me yet whether we do want to add
unit tests for parse_trailer_from_command_line_arg() (if we do end up
"resurrecting it" in this patch or later), so IDK. The main reason is
because I think the first set of unit tests should be for the exposed
functions in <trailer.h>, not so much the helper functions that only the
builtin uses.

Patch

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 943be5b360e..9657b0d067c 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -49,6 +49,11 @@  static int option_parse_trailer(const struct option *opt,
 				   const char *arg, int unset)
 {
 	struct list_head *trailers = opt->value;
+	struct strbuf tok = STRBUF_INIT;
+	struct strbuf val = STRBUF_INIT;
+	const struct trailer_conf *conf;
+	ssize_t separator_pos;
+	static char *cl_separators;
 
 	if (unset) {
 		free_new_trailers(trailers);
@@ -58,7 +63,42 @@  static int option_parse_trailer(const struct option *opt,
 	if (!arg)
 		return -1;
 
-	parse_trailer_from_command_line_arg(arg, where, if_exists, if_missing, trailers);
+	/*
+	 * In command-line arguments, '=' is accepted (in addition to the
+	 * separators that are defined).
+	 */
+	cl_separators = xstrfmt("=%s", trailer_default_separators());
+	separator_pos = find_separator(arg, cl_separators);
+	free(cl_separators);
+
+	if (separator_pos == 0) {
+		struct strbuf sb = STRBUF_INIT;
+		strbuf_addstr(&sb, arg);
+		strbuf_trim(&sb);
+		error(_("empty trailer token in trailer '%.*s'"),
+			(int) sb.len, sb.buf);
+		strbuf_release(&sb);
+	} else {
+		struct trailer_conf *conf_current = new_trailer_conf();
+		parse_trailer(arg, separator_pos, &tok, &val, &conf);
+		duplicate_trailer_conf(conf_current, conf);
+
+		/*
+		 * Override conf_current with settings specified via CLI flags.
+		 */
+		if (where != WHERE_DEFAULT)
+			trailer_set_conf_where(where, conf_current);
+		if (if_exists != EXISTS_DEFAULT)
+			trailer_set_conf_if_exists(if_exists, conf_current);
+		if (if_missing != MISSING_DEFAULT)
+			trailer_set_conf_if_missing(if_missing, conf_current);
+
+		trailer_add_arg_item(trailers,
+				     strbuf_detach(&tok, NULL),
+				     strbuf_detach(&val, NULL),
+				     conf_current);
+		free_trailer_conf(conf_current);
+	}
 
 	return 0;
 }
diff --git a/trailer.c b/trailer.c
index 0893175553a..b0b067ab12c 100644
--- a/trailer.c
+++ b/trailer.c
@@ -754,56 +754,6 @@  void parse_trailers_from_config(struct list_head *config_head)
 	}
 }
 
-void parse_trailer_from_command_line_arg(const char *line,
-					 enum trailer_where where,
-					 enum trailer_if_exists if_exists,
-					 enum trailer_if_missing if_missing,
-					 struct list_head *arg_head)
-{
-	struct strbuf tok = STRBUF_INIT;
-	struct strbuf val = STRBUF_INIT;
-	const struct trailer_conf *conf;
-
-	/*
-	 * In command-line arguments, '=' is accepted (in addition to the
-	 * separators that are defined).
-	 */
-	char *cl_separators = xstrfmt("=%s", trailer_default_separators());
-
-	/* Add an arg item for a trailer from the command line */
-	ssize_t separator_pos = find_separator(line, cl_separators);
-	free(cl_separators);
-
-	if (separator_pos == 0) {
-		struct strbuf sb = STRBUF_INIT;
-		strbuf_addstr(&sb, line);
-		strbuf_trim(&sb);
-		error(_("empty trailer token in trailer '%.*s'"),
-		      (int) sb.len, sb.buf);
-		strbuf_release(&sb);
-	} else {
-		struct trailer_conf *conf_current = new_trailer_conf();
-		parse_trailer(line, separator_pos, &tok, &val, &conf);
-		duplicate_trailer_conf(conf_current, conf);
-
-		/*
-		 * Override conf_current with settings specified via CLI flags.
-		 */
-		if (where != WHERE_DEFAULT)
-			trailer_set_conf_where(where, conf_current);
-		if (if_exists != EXISTS_DEFAULT)
-			trailer_set_conf_if_exists(if_exists, conf_current);
-		if (if_missing != MISSING_DEFAULT)
-			trailer_set_conf_if_missing(if_missing, conf_current);
-
-		trailer_add_arg_item(arg_head,
-				     strbuf_detach(&tok, NULL),
-				     strbuf_detach(&val, NULL),
-				     conf_current);
-		free_trailer_conf(conf_current);
-	}
-}
-
 static const char *next_line(const char *str)
 {
 	const char *nl = strchrnul(str, '\n');
diff --git a/trailer.h b/trailer.h
index 2848a0d086c..af55032625d 100644
--- a/trailer.h
+++ b/trailer.h
@@ -62,12 +62,6 @@  struct process_trailer_options {
 
 void parse_trailers_from_config(struct list_head *config_head);
 
-void parse_trailer_from_command_line_arg(const char *line,
-					 enum trailer_where where,
-					 enum trailer_if_exists if_exists,
-					 enum trailer_if_missing if_missing,
-					 struct list_head *arg_head);
-
 void process_trailers_lists(struct list_head *head,
 			    struct list_head *arg_head);