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

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

[Christian Couder]

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

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 */
-- 
2.2.1.313.gcc831f2

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

[Junio C Hamano]

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 */

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

[Christian Couder]

On Sat, Feb 7, 2015 at 9:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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;
>         };

It is not designed like this because you only asked me to design it
like this after the facts, when there was another email thread about
conflicts blocks and one function you created could be used by the
trailer code too.

If you had asked this from the beginning I would certainly have done
it more like this (though I think the "struct trailers" part is too
simplistic). Rearchitecturing the code now would bring only small
performance improvements and a lot of code churn. And the performance
is not needed anyway if the code is not used in the first place, so
I'd rather first make sure that the code can be used.

I think that very few new features are now needed to make it possible
to use the code in other commands like commit, format-patch, am, etc,
but this patch implements one of the needed features.

>     - 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.

I don't think it is a big improvement because it is easy to see that
the current code doesn't molest the part before and after the
trailers.

> They can even process
> the parts that the trailer subsystem would not touch, e.g. running
> stripspace to the text in message_proper.

That could be worth the rearchitecturing if people really wanted that,
but I think for now more people have been interested in having ways to
change the trailer part, so I prefer to work on this first.

> 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.

My opinion is that having it in the output phase is best.

> 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.

I think it is much better in the output phase because it is very
natural for some projects to have a template message with empty
trailers like this:

Signed-off-by:
Reviewed-by:

Such a template message can for example remind contributors to the
project that they need to sign off their work and that they need to
have it reviewed by at least one person, and that to make it simpler
for everyone to review patches, the "Signed-off-by" trailers should
come before the "Reviewed-by" trailers in the commit message.

In this case, if you trim before you process command line trailers,
then, when you process some command line trailers that add sign offs,
the meaning of "where=after" cannot be based any more on the existing
empty "Signed-off-by:" in the template message.

> 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.

I am not so sure that it would make it easier to allow us to later
create a reusable API and "trailer subsystem" out of it.
It is very difficult to predict what will be really needed in the
future and it is not clear at all to me that the current API is not
reusable.

If it was so clear that an API similar as what you describe was so
important, why didn't you point out that at the very beginning when I
started working on git interpret-trailers? And it's not like you
didn't have time to think about what I was working on as we discussed
many other aspects of the design and the developement happened over
one year.

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

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> It is not designed like this because you only asked me to design it
> like this after the facts, when there was another email thread about
> conflicts blocks and one function you created could be used by the
> trailer code too.
>
> If you had asked this from the beginning I would certainly have done...

Because the process here is not somebody outlines the design and
gives it to laborer to implement, "after the fact" and "from the
beginning" are complaining to and asking for impossible.  You design
and implement and then it gets reviewed, not the other way around.

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

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> I think that very few new features are now needed to make it possible
> to use the code in other commands like commit, format-patch, am, etc,
> but this patch implements one of the needed features.
>
>>     - 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.
>
> I don't think it is a big improvement because it is easy to see that
> the current code doesn't molest the part before and after the
> trailers.

You force the callers that want only the "trailer" thing to happen
to:

 - pass first and last around.

 - keep each line of the message body in separate strbuf and have
   it in the same array as the trailers

Neither of which is necessary.  I recall that during the review of
the previous rounds your own code had to work this around by first
concatenating lines (each of which are unnecessarily in separate
"struct strbuf"s) into a single strbuf, only to call a helper that
takes a single strbuf to count what to ignore in it, and then
iterate over the array of strbuf to add up the lengths of them,
which would have been unnecessary if the underlying data structure
were saner.

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

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Sat, Feb 7, 2015 at 9:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> 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.
>
> I think it is much better in the output phase because it is very
> natural for some projects to have a template message with empty
> trailers like this:

That is exactly my point.  With empty trailers in the input, "Add
this iff there is no existing one" will be made useless.

I am *not* saying that we must not have a filter at the output
phrase.  If anything, it would allow people to be more sloppy and
hide the problem under the rug.  Your code may have a bug or design
problem that adds an empty one somewhere even when the user told you
that she does not want an empty one in the result.  The user may be
sloppy and say "Add this key-value" unconditionally, instead of
having to do "What is the value I want to add?  Oh, it is not empty,
so I'll ask the trailer machiner to add this key-value there".

I am saying that not filtering the input and whatever intermediate
result you produce [*1*] will make the end result much less useful.

[Footnote]

*1* Filtering intermediate result of course can be done by making
sure you do not add an empty one in the first place.