[09/10] trailer: move arg handling to interpret-trailers.c

Commit Message

Linus Arver Jan. 10, 2024, 6:51 a.m. UTC

From: Linus Arver <[e-mail]>

We don't move the "arg_item" struct to interpret-trailers.c, because it
is now a struct that contains information about trailers that should be
injected into the input text's own trailers. We will rename this
language as such in a follow-up patch to keep the diff here small.

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

Comments

Junio C Hamano Jan. 19, 2024, 1:14 a.m. UTC | #1

"Linus Arver via GitGitGadget" <[e-mail]> writes:

> diff --git a/trailer.c b/trailer.c
> index e2d541372a3..0a86e0d5afa 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -66,6 +66,11 @@ static LIST_HEAD(conf_head);
>  
>  static char *separators = ":";
>  
> +const char *default_separators(void)
> +{
> +	return separators;
> +}

This allows API users to peek into the current set of separator
bytes (either the default ":" or specified by the configuration
varaible "trailer.separators"), which is an improvement over
directly exposing the "separators" variable, but in a longer term,
do we need to have some "trailer context" object that holds this
and possibly other global variables like this?

I do not demand such further abstraction in this series, but I'd
prefer to see if we all have shared vision into the future.

Thanks.

Linus Arver Jan. 20, 2024, 8:14 p.m. UTC | #2

Junio C Hamano <[e-mail]> writes:

> "Linus Arver via GitGitGadget" <[e-mail]> writes:
>
>> diff --git a/trailer.c b/trailer.c
>> index e2d541372a3..0a86e0d5afa 100644
>> --- a/trailer.c
>> +++ b/trailer.c
>> @@ -66,6 +66,11 @@ static LIST_HEAD(conf_head);
>>  
>>  static char *separators = ":";
>>  
>> +const char *default_separators(void)
>> +{
>> +	return separators;
>> +}
>
> This allows API users to peek into the current set of separator
> bytes (either the default ":" or specified by the configuration
> varaible "trailer.separators"), which is an improvement over
> directly exposing the "separators" variable, but in a longer term,
> do we need to have some "trailer context" object that holds this
> and possibly other global variables like this?

Yes, and I've implemented a "trailer_subsystem_conf" struct to hold all
configuration-related bits (and hardcoded defaults) relating to trailers
in one place, in the larger series.

> I do not demand such further abstraction in this series, but I'd
> prefer to see if we all have shared vision into the future.

Makes sense. Thanks.

Patch

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 42d9ca07a56..4da4eac3b46 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -45,23 +45,17 @@  static int option_parse_if_missing(const struct option *opt,
 	return trailer_set_if_missing(opt->value, arg);
 }
 
-static void new_trailers_clear(struct list_head *trailers)
-{
-	struct list_head *pos, *tmp;
-	struct new_trailer_item *item;
-
-	list_for_each_safe(pos, tmp, trailers) {
-		item = list_entry(pos, struct new_trailer_item, list);
-		list_del(pos);
-		free(item);
-	}
-}
+static char *cl_separators;
 
 static int option_parse_trailer(const struct option *opt,
 				   const char *arg, int unset)
 {
 	struct list_head *trailers = opt->value;
-	struct new_trailer_item *item;
+	struct strbuf tok = STRBUF_INIT;
+	struct strbuf val = STRBUF_INIT;
+	const struct trailer_conf *conf;
+	struct trailer_conf *conf_current = new_trailer_conf();
+	ssize_t separator_pos;
 
 	if (unset) {
 		new_trailers_clear(trailers);
@@ -71,12 +65,31 @@  static int option_parse_trailer(const struct option *opt,
 	if (!arg)
 		return -1;
 
-	item = xmalloc(sizeof(*item));
-	item->text = arg;
-	item->where = where;
-	item->if_exists = if_exists;
-	item->if_missing = if_missing;
-	list_add_tail(&item->list, trailers);
+	separator_pos = find_separator(arg, cl_separators);
+	if (separator_pos) {
+		parse_trailer(arg, separator_pos, &tok, &val, &conf);
+		duplicate_trailer_conf(conf_current, conf);
+
+		/*
+		 * Override conf_current with settings specified via CLI flags.
+		 */
+		trailer_conf_set(where, if_exists, if_missing, conf_current);
+
+		add_arg_item(strbuf_detach(&tok, NULL),
+			     strbuf_detach(&val, NULL),
+			     conf_current,
+			     trailers);
+	} else {
+		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);
+	}
+
+	free(conf_current);
+
 	return 0;
 }
 
@@ -136,7 +149,7 @@  static void read_input_file(struct strbuf *sb, const char *file)
 
 static void interpret_trailers(const char *file,
 			       const struct process_trailer_options *opts,
-			       struct list_head *new_trailer_head)
+			       struct list_head *arg_trailers)
 {
 	LIST_HEAD(head);
 	struct strbuf sb = STRBUF_INIT;
@@ -144,8 +157,6 @@  static void interpret_trailers(const char *file,
 	struct trailer_block *trailer_block;
 	FILE *outfile = stdout;
 
-	trailer_config_init();
-
 	read_input_file(&sb, file);
 
 	if (opts->in_place)
@@ -162,12 +173,7 @@  static void interpret_trailers(const char *file,
 
 
 	if (!opts->only_input) {
-		LIST_HEAD(config_head);
-		LIST_HEAD(arg_head);
-		parse_trailers_from_config(&config_head);
-		parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
-		list_splice(&config_head, &arg_head);
-		process_trailers_lists(&head, &arg_head);
+		process_trailers_lists(&head, arg_trailers);
 	}
 
 	/* Print trailer block. */
@@ -193,7 +199,8 @@  static void interpret_trailers(const char *file,
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
 	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
-	LIST_HEAD(trailers);
+	LIST_HEAD(configured_trailers);
+	LIST_HEAD(arg_trailers);
 
 	struct option options[] = {
 		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
@@ -212,33 +219,48 @@  int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F(0, "parse", &opts, NULL, N_("alias for --only-trailers --only-input --unfold"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse),
 		OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat \"---\" as the end of input")),
-		OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),
+		OPT_CALLBACK(0, "trailer", &arg_trailers, N_("trailer"),
 				N_("trailer(s) to add"), option_parse_trailer),
 		OPT_END()
 	};
 
 	git_config(git_default_config, NULL);
+	trailer_config_init();
+
+	if (!opts.only_input) {
+		parse_trailers_from_config(&configured_trailers);
+	}
+
+	/*
+	* In command-line arguments, '=' is accepted (in addition to the
+	* separators that are defined).
+	*/
+	cl_separators = xstrfmt("=%s", default_separators());
 
 	argc = parse_options(argc, argv, prefix, options,
 			     git_interpret_trailers_usage, 0);
 
-	if (opts.only_input && !list_empty(&trailers))
+	free(cl_separators);
+
+	if (opts.only_input && !list_empty(&arg_trailers))
 		usage_msg_opt(
 			_("--trailer with --only-input does not make sense"),
 			git_interpret_trailers_usage,
 			options);
 
+	list_splice(&configured_trailers, &arg_trailers);
+
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)
-			interpret_trailers(argv[i], &opts, &trailers);
+			interpret_trailers(argv[i], &opts, &arg_trailers);
 	} else {
 		if (opts.in_place)
 			die(_("no input file given for in-place editing"));
-		interpret_trailers(NULL, &opts, &trailers);
+		interpret_trailers(NULL, &opts, &arg_trailers);
 	}
 
-	new_trailers_clear(&trailers);
+	new_trailers_clear(&arg_trailers);
 
 	return 0;
 }
diff --git a/trailer.c b/trailer.c
index e2d541372a3..0a86e0d5afa 100644
--- a/trailer.c
+++ b/trailer.c
@@ -66,6 +66,11 @@  static LIST_HEAD(conf_head);
 
 static char *separators = ":";
 
+const char *default_separators(void)
+{
+	return separators;
+}
+
 static int configured;
 
 #define TRAILER_ARG_STRING "$ARG"
@@ -424,6 +429,25 @@  int trailer_set_if_missing(enum trailer_if_missing *item, const char *value)
 	return 0;
 }
 
+void trailer_conf_set(enum trailer_where where,
+		      enum trailer_if_exists if_exists,
+		      enum trailer_if_missing if_missing,
+		      struct trailer_conf *conf)
+{
+	if (where != WHERE_DEFAULT)
+		conf->where = where;
+	if (if_exists != EXISTS_DEFAULT)
+		conf->if_exists = if_exists;
+	if (if_missing != MISSING_DEFAULT)
+		conf->if_missing = if_missing;
+}
+
+struct trailer_conf *new_trailer_conf(void)
+{
+	struct trailer_conf *new = xcalloc(1, sizeof(*new));
+	return new;
+}
+
 void duplicate_trailer_conf(struct trailer_conf *dst,
 			    const struct trailer_conf *src)
 {
@@ -642,6 +666,9 @@  ssize_t find_separator(const char *line, const char *separators)
 /*
  * Obtain the token, value, and conf from the given trailer.
  *
+ * The conf needs special handling. We first read hardcoded defaults, and
+ * override them if we find a matching trailer configuration in the config.
+ *
  * separator_pos must not be 0, since the token cannot be an empty string.
  *
  * If separator_pos is -1, interpret the whole trailer as a token.
@@ -691,22 +718,14 @@  static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
 	return new_item;
 }
 
-static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
-			 const struct trailer_conf *conf,
-			 const struct new_trailer_item *new_trailer_item)
+void add_arg_item(char *tok, char *val, const struct trailer_conf *conf,
+		  struct list_head *arg_head)
+
 {
 	struct arg_item *new_item = xcalloc(1, sizeof(*new_item));
 	new_item->token = tok;
 	new_item->value = val;
 	duplicate_trailer_conf(&new_item->conf, conf);
-	if (new_trailer_item) {
-		if (new_trailer_item->where != WHERE_DEFAULT)
-			new_item->conf.where = new_trailer_item->where;
-		if (new_trailer_item->if_exists != EXISTS_DEFAULT)
-			new_item->conf.if_exists = new_trailer_item->if_exists;
-		if (new_trailer_item->if_missing != MISSING_DEFAULT)
-			new_item->conf.if_missing = new_trailer_item->if_missing;
-	}
 	list_add_tail(&new_item->list, arg_head);
 }
 
@@ -719,10 +738,10 @@  void parse_trailers_from_config(struct list_head *config_head)
 	list_for_each(pos, &conf_head) {
 		item = list_entry(pos, struct arg_item, list);
 		if (item->conf.command)
-			add_arg_item(config_head,
-				     xstrdup(token_from_item(item, NULL)),
+			add_arg_item(xstrdup(token_from_item(item, NULL)),
 				     xstrdup(""),
-				     &item->conf, NULL);
+				     &item->conf,
+				     config_head);
 	}
 }
 
@@ -755,10 +774,10 @@  void parse_trailers_from_command_line_args(struct list_head *arg_head,
 			strbuf_release(&sb);
 		} else {
 			parse_trailer(tr->text, separator_pos, &tok, &val, &conf);
-			add_arg_item(arg_head,
-				     strbuf_detach(&tok, NULL),
+			add_arg_item(strbuf_detach(&tok, NULL),
 				     strbuf_detach(&val, NULL),
-				     conf, tr);
+				     conf,
+				     arg_head);
 		}
 	}
 
@@ -1148,6 +1167,16 @@  void free_trailers(struct list_head *head)
 	}
 }
 
+void new_trailers_clear(struct list_head *trailers)
+{
+	struct list_head *pos, *p;
+
+	list_for_each_safe(pos, p, trailers) {
+		list_del(pos);
+		free_arg_item(list_entry(pos, struct arg_item, list));
+	}
+}
+
 size_t trailer_block_start(struct trailer_block *trailer_block)
 {
 	return trailer_block->start;
diff --git a/trailer.h b/trailer.h
index fe49a9bad52..9b86acfe2d4 100644
--- a/trailer.h
+++ b/trailer.h
@@ -46,9 +46,20 @@  struct new_trailer_item {
 	enum trailer_if_missing if_missing;
 };
 
+void trailer_conf_set(enum trailer_where where,
+		      enum trailer_if_exists if_exists,
+		      enum trailer_if_missing if_missing,
+		      struct trailer_conf *conf);
+
+struct trailer_conf *new_trailer_conf(void);
 void duplicate_trailer_conf(struct trailer_conf *dst,
 			    const struct trailer_conf *src);
 
+const char *default_separators(void);
+
+void add_arg_item(char *tok, char *val, const struct trailer_conf *conf,
+		  struct list_head *arg_head);
+
 struct process_trailer_options {
 	int in_place;
 	int trim_empty;
@@ -92,6 +103,7 @@  void trailer_block_release(struct trailer_block *trailer_block);
 
 void trailer_config_init(void);
 void free_trailers(struct list_head *trailers);
+void new_trailers_clear(struct list_head *trailers);
 
 void format_trailers(struct list_head *head,
 		     const struct process_trailer_options *opts,