Re: [PATCH 1/3] trailer: add a trailer.trimEmpty config option

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

Christian Couder <chriscool@tuxfamily.org> writes:

> This way people who always want trimed trailers
> don't need to specify it on the command line.

I sense that print_all() that cares about trimming illustrate a
design mistake in the original code structure.  Why isn't the entire
program structured like this instead?

    - read input and split that into three parts:

	struct {
		struct strbuf messsage_proper;

		struct trailers {
			int nr, alloc;
	                struct strbuf *array_of_trailers[];
		} trailers;

		struct strbuf lines_after_trailers;
	};

    - do "trailer stuff" by calling a central helper that does
      "trailer stuff" a pointer to the middle, trailers, struct.

      - when the trailer becomes a reusable subsystem, this central
        helper will become the primary entry point to the API.

      - "trailer stuff" will include adding new ones, removing
        existing ones, and rewriting lines.  What you do in the
        current process_command_line_args() and
        process_trailers_lists() [*1*] would come here.

    - write out the result, given the outermost struct.  This will
      become another entry point in the API.

Structured that way, callers will supply that outermost structure,
and can trust that the trailers subsystem will not molest
message_proper or lines_after_trailers part.  They can even process
the parts that the trailer subsystem would not touch, e.g. running
stripspace to the text in message_proper.

Viewed that way, it would be clear that "strip empty ones" should be
a new feature in the "trailer stuff", not just "filter out only
during the output phase".  Having it in the output phase does not
feel that the labor/responsibility is split in the right way.

Another problem I have with "filter out during the output phase"
comes from the semantics/correctness in the resulting code, and I
suspect that it would need to be done a lot earlier, before you
allow the logic such as "if I have X, do this, but if there is no X,
do this other thing".  If you have an X that is empty in the input,
trimming that before such logic kicks in and trimming that in the
final output phase would give different results, and I think the
latter would give a less intuitive result.

As this is the second time I have to point out that the data
structure used by the current code to hold the trailers and other
stuff to work on smells fishy, I would seriously consider cleaning
up the existing code to make it easier to allow us to later create
a reusable API and "trailer subsystem" out of it, before piling new
features on top of it, if I were you.

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  builtin/interpret-trailers.c |  2 +-
>  trailer.c                    | 13 ++++++++++---
>  trailer.h                    |  2 +-
>  3 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index 46838d2..1adf814 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -18,7 +18,7 @@ static const char * const git_interpret_trailers_usage[] = {
>  
>  int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
>  {
> -	int trim_empty = 0;
> +	int trim_empty = -1;
>  	struct string_list trailers = STRING_LIST_INIT_DUP;
>  
>  	struct option options[] = {
> diff --git a/trailer.c b/trailer.c
> index 623adeb..7614182 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -36,6 +36,8 @@ static struct trailer_item *first_conf_item;
>  
>  static char *separators = ":";
>  
> +static int trim_empty;
> +
>  #define TRAILER_ARG_STRING "$ARG"
>  
>  static int after_or_end(enum action_where where)
> @@ -120,7 +122,7 @@ static void print_tok_val(const char *tok, const char *val)
>  		printf("%s%c %s\n", tok, separators[0], val);
>  }
>  
> -static void print_all(struct trailer_item *first, int trim_empty)
> +static void print_all(struct trailer_item *first)
>  {
>  	struct trailer_item *item;
>  	for (item = first; item; item = item->next) {
> @@ -509,6 +511,8 @@ static int git_trailer_default_config(const char *conf_key, const char *value, v
>  					value, conf_key);
>  		} else if (!strcmp(trailer_item, "separators")) {
>  			separators = xstrdup(value);
> +		} else if (!strcmp(trailer_item, "trimempty")) {
> +			trim_empty = git_config_bool(conf_key, value);
>  		}
>  	}
>  	return 0;
> @@ -842,7 +846,7 @@ static void free_all(struct trailer_item **first)
>  	}
>  }
>  
> -void process_trailers(const char *file, int trim_empty, struct string_list *trailers)
> +void process_trailers(const char *file, int trim, struct string_list *trailers)
>  {
>  	struct trailer_item *in_tok_first = NULL;
>  	struct trailer_item *in_tok_last = NULL;
> @@ -854,6 +858,9 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai
>  	git_config(git_trailer_default_config, NULL);
>  	git_config(git_trailer_config, NULL);
>  
> +	if (trim > -1)
> +		trim_empty = trim;
> +
>  	lines = read_input_file(file);
>  
>  	/* Print the lines before the trailers */
> @@ -863,7 +870,7 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai
>  
>  	process_trailers_lists(&in_tok_first, &in_tok_last, &arg_tok_first);
>  
> -	print_all(in_tok_first, trim_empty);
> +	print_all(in_tok_first);
>  
>  	free_all(&in_tok_first);
>  
> diff --git a/trailer.h b/trailer.h
> index 8eb25d5..4f481d0 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -1,6 +1,6 @@
>  #ifndef TRAILER_H
>  #define TRAILER_H
>  
> -void process_trailers(const char *file, int trim_empty, struct string_list *trailers);
> +void process_trailers(const char *file, int trim, struct string_list *trailers);
>  
>  #endif /* TRAILER_H */