[3/5] trailer: split process_command_line_args into separate functions

Commit Message

Linus Arver Aug. 5, 2023, 5:04 a.m. UTC

From: Linus Arver <[e-mail]>

Previously, process_command_line_args did two things:

    (1) parse trailers from the configuration, and
    (2) parse trailers defined on the command line.

Separate these concerns into parse_trailers_from_config and
parse_trailers_from_command_line_args, respectively. Remove (now
redundant) process_command_line_args.

Signed-off-by: Linus Arver <[e-mail]>
---
 trailer.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

Comments

Glen Choo Aug. 7, 2023, 10:51 p.m. UTC | #1

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

> Previously, process_command_line_args did two things:
>
>     (1) parse trailers from the configuration, and
>     (2) parse trailers defined on the command line.

It parses trailers from two places, but it still only does "one thing",
in that it only parses trailers.

> @@ -711,30 +711,35 @@ static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
>  	list_add_tail(&new_item->list, arg_head);
>  }
>  
> -static void process_command_line_args(struct list_head *arg_head,
> -				      struct list_head *new_trailer_head)
> +static void parse_trailers_from_config(struct list_head *config_head)
>  {
>  	struct arg_item *item;
> -	struct strbuf tok = STRBUF_INIT;
> -	struct strbuf val = STRBUF_INIT;
> -	const struct conf_info *conf;
>  	struct list_head *pos;
>  
> -	/*
> -	 * In command-line arguments, '=' is accepted (in addition to the
> -	 * separators that are defined).
> -	 */
> -	char *cl_separators = xstrfmt("=%s", separators);
> -
>  	/* Add an arg item for each configured trailer with a command */
>  	list_for_each(pos, &conf_head) {
>  		item = list_entry(pos, struct arg_item, list);
>  		if (item->conf.command)
> -			add_arg_item(arg_head,
> +			add_arg_item(config_head,
>  				     xstrdup(token_from_item(item, NULL)),
>  				     xstrdup(""),
>  				     &item->conf, NULL);
>  	}
> +}
> +
> +static void parse_trailers_from_command_line_args(struct list_head *arg_head,
> +						  struct list_head *new_trailer_head)
> +{
> +	struct strbuf tok = STRBUF_INIT;
> +	struct strbuf val = STRBUF_INIT;
> +	const struct conf_info *conf;
> +	struct list_head *pos;
> +
> +	/*
> +	 * In command-line arguments, '=' is accepted (in addition to the
> +	 * separators that are defined).
> +	 */
> +	char *cl_separators = xstrfmt("=%s", separators);
>  
>  	/* Add an arg item for each trailer on the command line */
>  	list_for_each(pos, new_trailer_head) {

I find this equally readable as the preimage, which IMO is adequately
scoped and commented.

> @@ -1070,8 +1075,11 @@ void process_trailers(const char *file,
>  
>  
>  	if (!opts->only_input) {
> +		LIST_HEAD(config_head);
>  		LIST_HEAD(arg_head);
> -		process_command_line_args(&arg_head, new_trailer_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);
>  	}

But now, we have to remember to call two functions instead of just one.
This, and the slight additional churn makes me lean negative on this
change. I would be really happy if we had a use case where we only
wanted to call one function but not the other, but it seems like this
isn't the case.

Linus Arver Aug. 11, 2023, 12:59 a.m. UTC | #2

Glen Choo <[e-mail]> writes:

> "Linus Arver via GitGitGadget" <[e-mail]> writes:
>
>> Previously, process_command_line_args did two things:
>>
>>     (1) parse trailers from the configuration, and
>>     (2) parse trailers defined on the command line.
>
> It parses trailers from two places, but it still only does "one thing",
> in that it only parses trailers.

More precisely, it parses trailers from the command line by first
parsing trailers from the configuration. In other words, parsing
trailers from the configuration (independent of the input string!) is a
required dependency for parsing trailers coming from the command line.

If we take a step back, we need to do 3 things:

   (1) parse trailers from the configuration
   (2) parse trailers from the command line
   (3) parse trailers from the input

I think these three should be separated into separate functions. I think
no one wants to combine all three into one function. And I can't think
of a good enough reason to combine (1) and (2) together either. Hence
this patch.

> I find this equally readable as the preimage, which IMO is adequately
> scoped and commented.

Aside: is "preimage" the status quo before applying the patch?

>> @@ -1070,8 +1075,11 @@ void process_trailers(const char *file,
>>  
>>  
>>  	if (!opts->only_input) {
>> +		LIST_HEAD(config_head);
>>  		LIST_HEAD(arg_head);
>> -		process_command_line_args(&arg_head, new_trailer_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);
>>  	}
>
> But now, we have to remember to call two functions instead of just one.

But only inside interpret-trailers.c, right?

Linus Arver Aug. 11, 2023, 1:02 a.m. UTC | #3

Linus Arver <[e-mail]> writes:
>>
>> But now, we have to remember to call two functions instead of just one.
>
> But only inside interpret-trailers.c, right?

Oops, I meant trailer.c.

In the future I expect to move this to interpret-trailers.c.

Glen Choo Aug. 11, 2023, 9:11 p.m. UTC | #4

Linus Arver <[e-mail]> writes:

>> I find this equally readable as the preimage, which IMO is adequately
>> scoped and commented.
>
> Aside: is "preimage" the status quo before applying the patch?

Yup.

Patch

diff --git a/trailer.c b/trailer.c
index 16fbba03d07..89246a0d395 100644
--- a/trailer.c
+++ b/trailer.c
@@ -711,30 +711,35 @@  static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
 	list_add_tail(&new_item->list, arg_head);
 }
 
-static void process_command_line_args(struct list_head *arg_head,
-				      struct list_head *new_trailer_head)
+static void parse_trailers_from_config(struct list_head *config_head)
 {
 	struct arg_item *item;
-	struct strbuf tok = STRBUF_INIT;
-	struct strbuf val = STRBUF_INIT;
-	const struct conf_info *conf;
 	struct list_head *pos;
 
-	/*
-	 * In command-line arguments, '=' is accepted (in addition to the
-	 * separators that are defined).
-	 */
-	char *cl_separators = xstrfmt("=%s", separators);
-
 	/* Add an arg item for each configured trailer with a command */
 	list_for_each(pos, &conf_head) {
 		item = list_entry(pos, struct arg_item, list);
 		if (item->conf.command)
-			add_arg_item(arg_head,
+			add_arg_item(config_head,
 				     xstrdup(token_from_item(item, NULL)),
 				     xstrdup(""),
 				     &item->conf, NULL);
 	}
+}
+
+static void parse_trailers_from_command_line_args(struct list_head *arg_head,
+						  struct list_head *new_trailer_head)
+{
+	struct strbuf tok = STRBUF_INIT;
+	struct strbuf val = STRBUF_INIT;
+	const struct conf_info *conf;
+	struct list_head *pos;
+
+	/*
+	 * In command-line arguments, '=' is accepted (in addition to the
+	 * separators that are defined).
+	 */
+	char *cl_separators = xstrfmt("=%s", separators);
 
 	/* Add an arg item for each trailer on the command line */
 	list_for_each(pos, new_trailer_head) {
@@ -1070,8 +1075,11 @@  void process_trailers(const char *file,
 
 
 	if (!opts->only_input) {
+		LIST_HEAD(config_head);
 		LIST_HEAD(arg_head);
-		process_command_line_args(&arg_head, new_trailer_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);
 	}