Re: [PATCH v3 08/10] trailer: move arg handling to interpret-trailers.c

[Junio C Hamano <gitster@pobox.com>]

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Linus Arver <linusa@google.com>
>
> We don't move the "arg_item" struct to interpret-trailers.c, because it
> is now a struct that contains information about trailers that could be
> added into the input text's own trailers. This is a generic concept that
> extends beyond trailers defined as CLI arguments (it applies to trailers
> defined in configuration as well). We will rename "arg_item" to
> "trailer_template" in a follow-up patch to keep the diff here small.
>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
>  builtin/interpret-trailers.c | 88 ++++++++++++++++++++++--------------
>  trailer.c                    | 62 ++++++++++++++++++-------
>  trailer.h                    | 12 +++++
>  3 files changed, 112 insertions(+), 50 deletions(-)
>
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index 9f0ba39b317..9a902012912 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 free_new_trailers(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;

It somehow feels ugly and goes backward to depend on a new global,
especially when you are aiming to libify more things.  If we are
doing something like ...

>  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) {
>  		free_new_trailers(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.
> +		 */

... this, which apparently needs to know that we are dealing with
input from CLI, shouldn't we be able to do the "CLI allows '=' as
well as whatever is the default" here, instead of mucking with the
global variable in the caller?

Even if you plan to later make this function callable from outside
the context of parse_options() callback, and if you do not want "CLI
allows '=' as well" for such new callers, we should be able to have
a shared helper function that is the bulk of this function but takes
additional parameter to tweak its behaviour slightly depending on
the needs of the callers?

> +		trailer_conf_set(where, if_exists, if_missing, conf_current);

I am getting an impression that the use of and the introduction of
the new helper, mixed with movement of the responsibility, makes
reviewing the change unnecessarily more cumbersome.  An equivalent
series with a few more steps, each of them focusing on a small
change (like, "updating the conf_current members is done with
assignments that appear as a pattern very often---introduce a helper
to reduce boilerplate") would have been more enticing to reviewers.

> +		trailer_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;
>  }
>  
> @@ -135,7 +148,7 @@ static void read_input_file(struct strbuf *sb, const char *file)
>  }
>  
>  static void interpret_trailers(const struct process_trailer_options *opts,
> -			       struct list_head *new_trailer_head,
> +			       struct list_head *arg_trailers,
>  			       const char *file)
>  {
>  	LIST_HEAD(head);
> @@ -144,8 +157,6 @@ static void interpret_trailers(const struct process_trailer_options *opts,
>  	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 struct process_trailer_options *opts,
>  
>  
>  	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);
>  	}

So, the bulk of the parsing is no longer responsibility of this
function.  Instead, the caller (e.g., cmd_interpret_trailers()) is
expected to do that before they call us.

> ...
>  	git_config(git_default_config, NULL);
> +	trailer_config_init();
> +
> +	if (!opts.only_input) {
> +		parse_trailers_from_config(&configured_trailers);
> +	}

By the way, until we add more statements in this block, lose the
unnecessary {} around it.

> +	/*
> +	* In command-line arguments, '=' is accepted (in addition to the
> +	* separators that are defined).
> +	*/
> +	cl_separators = xstrfmt("=%s", trailer_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);

This corresponds to what the old code did in interpret_trailers(),
OK.

The move of the responsibility may make sense.

>  	if (argc) {
>  		int i;
>  		for (i = 0; i < argc; i++)
> -			interpret_trailers(&opts, &trailers, argv[i]);
> +			interpret_trailers(&opts, &arg_trailers, argv[i]);
>  	} else {
>  		if (opts.in_place)
>  			die(_("no input file given for in-place editing"));
> -		interpret_trailers(&opts, &trailers, NULL);
> +		interpret_trailers(&opts, &arg_trailers, NULL);
>  	}
>  
> -	free_new_trailers(&trailers);
> +	free_new_trailers(&arg_trailers);
>  
>  	return 0;
>  }
> diff --git a/trailer.c b/trailer.c
> index c16f552b463..19637ff295d 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -66,6 +66,11 @@ static LIST_HEAD(conf_head);
>  
>  static char *separators = ":";
>  
> +const char *trailer_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;
> +}

So, this is such a helper function. It is curious that it
deliberately lacks the ability to reset what has already been
configured back to the default.

For example, we earlier saw this original code ...

> -	item = xmalloc(sizeof(*item));
> -	item->text = arg;
> -	item->where = where;
> -	item->if_exists = if_exists;
> -	item->if_missing = if_missing;

... gets replaced with this call.

> +	struct trailer_conf *conf_current = new_trailer_conf();
> ...
> +		trailer_conf_set(where, if_exists, if_missing, conf_current);

For this conversion to be correct, we require that the value of the
*_DEFAULT macro to be 0 and/or these three values getting assigned
are not *_DEFAULT values.  Maybe that may happen to be the case in
today's code, but such an unwritten assumption makes me feel nervous.

I am not sure if it is worth the confusion factor to make a function
whose name is $anything_set() to be making a conditional assignment.
If such a conditional assignment *also* happens often and deserves
to have its own helper, it is fine to have such a helper for
conditional assignment, but calling it $anything_set() is probably a
mistake.

Other than that, the main thrust of this step seems sensible.

Thanks.