{"thread":{"id":"38519","subject":"[PATCH 1/3] trailer: add a trailer.trimEmpty config option","startedAt":"2015-02-07T13:11:09Z","lastAt":"2015-02-10T22:06:43Z","messageCount":6,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"255692","messageId":"20150207131112.394.30858.chriscool@tuxfamily.org","threadId":"38519","inReplyTo":null,"subject":"[PATCH 1/3] trailer: add a trailer.trimEmpty config option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2015-02-07T13:11:09Z","receivedAt":"2015-02-07T13:11:09Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This way people who always want trimed trailers\ndon't need to specify it on the command line.\n\nSigned-off-by: Christian Couder \n---\n builtin/interpret-trailers.c | 2 +-\n trailer.c | 13 ++++++++++---\n trailer.h | 2 +-\n 3 files changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex 46838d2..1adf814 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -18,7 +18,7 @@ static const char * const git_interpret_trailers_usage[] = {\n \n int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n {\n-\tint trim_empty = 0;\n+\tint trim_empty = -1;\n \tstruct string_list trailers = STRING_LIST_INIT_DUP;\n \n \tstruct option options[] = {\ndiff --git a/trailer.c b/trailer.c\nindex 623adeb..7614182 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -36,6 +36,8 @@ static struct trailer_item *first_conf_item;\n \n static char *separators = \":\";\n \n+static int trim_empty;\n+\n #define TRAILER_ARG_STRING \"$ARG\"\n \n static int after_or_end(enum action_where where)\n@@ -120,7 +122,7 @@ static void print_tok_val(const char *tok, const char *val)\n \t\tprintf(\"%s%c %s\\n\", tok, separators[0], val);\n }\n \n-static void print_all(struct trailer_item *first, int trim_empty)\n+static void print_all(struct trailer_item *first)\n {\n \tstruct trailer_item *item;\n \tfor (item = first; item; item = item->next) {\n@@ -509,6 +511,8 @@ static int git_trailer_default_config(const char *conf_key, const char *value, v\n \t\t\t\t\tvalue, conf_key);\n \t\t} else if (!strcmp(trailer_item, \"separators\")) {\n \t\t\tseparators = xstrdup(value);\n+\t\t} else if (!strcmp(trailer_item, \"trimempty\")) {\n+\t\t\ttrim_empty = git_config_bool(conf_key, value);\n \t\t}\n \t}\n \treturn 0;\n@@ -842,7 +846,7 @@ static void free_all(struct trailer_item **first)\n \t}\n }\n \n-void process_trailers(const char *file, int trim_empty, struct string_list *trailers)\n+void process_trailers(const char *file, int trim, struct string_list *trailers)\n {\n \tstruct trailer_item *in_tok_first = NULL;\n \tstruct trailer_item *in_tok_last = NULL;\n@@ -854,6 +858,9 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai\n \tgit_config(git_trailer_default_config, NULL);\n \tgit_config(git_trailer_config, NULL);\n \n+\tif (trim > -1)\n+\t\ttrim_empty = trim;\n+\n \tlines = read_input_file(file);\n \n \t/* Print the lines before the trailers */\n@@ -863,7 +870,7 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai\n \n \tprocess_trailers_lists(&in_tok_first, &in_tok_last, &arg_tok_first);\n \n-\tprint_all(in_tok_first, trim_empty);\n+\tprint_all(in_tok_first);\n \n \tfree_all(&in_tok_first);\n \ndiff --git a/trailer.h b/trailer.h\nindex 8eb25d5..4f481d0 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -1,6 +1,6 @@\n #ifndef TRAILER_H\n #define TRAILER_H\n \n-void process_trailers(const char *file, int trim_empty, struct string_list *trailers);\n+void process_trailers(const char *file, int trim, struct string_list *trailers);\n \n #endif /* TRAILER_H */\n-- \n2.2.1.313.gcc831f2\n"},{"id":"255707","messageId":"xmqq386hcw33.fsf@gitster.dls.corp.google.com","threadId":"38519","inReplyTo":"20150207131112.394.30858.chriscool@tuxfamily.org","subject":"Re: [PATCH 1/3] trailer: add a trailer.trimEmpty config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-07T20:20:32Z","receivedAt":"2015-02-07T20:20:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> This way people who always want trimed trailers\n> don't need to specify it on the command line.\n\nI sense that print_all() that cares about trimming illustrate a\ndesign mistake in the original code structure. Why isn't the entire\nprogram structured like this instead?\n\n - read input and split that into three parts:\n\n\tstruct {\n\t\tstruct strbuf messsage_proper;\n\n\t\tstruct trailers {\n\t\t\tint nr, alloc;\n\t struct strbuf *array_of_trailers[];\n\t\t} trailers;\n\n\t\tstruct strbuf lines_after_trailers;\n\t};\n\n - do \"trailer stuff\" by calling a central helper that does\n \"trailer stuff\" a pointer to the middle, trailers, struct.\n\n - when the trailer becomes a reusable subsystem, this central\n helper will become the primary entry point to the API.\n\n - \"trailer stuff\" will include adding new ones, removing\n existing ones, and rewriting lines. What you do in the\n current process_command_line_args() and\n process_trailers_lists() [*1*] would come here.\n\n - write out the result, given the outermost struct. This will\n become another entry point in the API.\n\nStructured that way, callers will supply that outermost structure,\nand can trust that the trailers subsystem will not molest\nmessage_proper or lines_after_trailers part. They can even process\nthe parts that the trailer subsystem would not touch, e.g. running\nstripspace to the text in message_proper.\n\nViewed that way, it would be clear that \"strip empty ones\" should be\na new feature in the \"trailer stuff\", not just \"filter out only\nduring the output phase\". Having it in the output phase does not\nfeel that the labor/responsibility is split in the right way.\n\nAnother problem I have with \"filter out during the output phase\"\ncomes from the semantics/correctness in the resulting code, and I\nsuspect that it would need to be done a lot earlier, before you\nallow the logic such as \"if I have X, do this, but if there is no X,\ndo this other thing\". If you have an X that is empty in the input,\ntrimming that before such logic kicks in and trimming that in the\nfinal output phase would give different results, and I think the\nlatter would give a less intuitive result.\n\nAs this is the second time I have to point out that the data\nstructure used by the current code to hold the trailers and other\nstuff to work on smells fishy, I would seriously consider cleaning\nup the existing code to make it easier to allow us to later create\na reusable API and \"trailer subsystem\" out of it, before piling new\nfeatures on top of it, if I were you.\n\n> Signed-off-by: Christian Couder \n> ---\n> builtin/interpret-trailers.c | 2 +-\n> trailer.c | 13 ++++++++++---\n> trailer.h | 2 +-\n> 3 files changed, 12 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n> index 46838d2..1adf814 100644\n> --- a/builtin/interpret-trailers.c\n> +++ b/builtin/interpret-trailers.c\n> @@ -18,7 +18,7 @@ static const char * const git_interpret_trailers_usage[] = {\n> \n> int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n> {\n> -\tint trim_empty = 0;\n> +\tint trim_empty = -1;\n> \tstruct string_list trailers = STRING_LIST_INIT_DUP;\n> \n> \tstruct option options[] = {\n> diff --git a/trailer.c b/trailer.c\n> index 623adeb..7614182 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -36,6 +36,8 @@ static struct trailer_item *first_conf_item;\n> \n> static char *separators = \":\";\n> \n> +static int trim_empty;\n> +\n> #define TRAILER_ARG_STRING \"$ARG\"\n> \n> static int after_or_end(enum action_where where)\n> @@ -120,7 +122,7 @@ static void print_tok_val(const char *tok, const char *val)\n> \t\tprintf(\"%s%c %s\\n\", tok, separators[0], val);\n> }\n> \n> -static void print_all(struct trailer_item *first, int trim_empty)\n> +static void print_all(struct trailer_item *first)\n> {\n> \tstruct trailer_item *item;\n> \tfor (item = first; item; item = item->next) {\n> @@ -509,6 +511,8 @@ static int git_trailer_default_config(const char *conf_key, const char *value, v\n> \t\t\t\t\tvalue, conf_key);\n> \t\t} else if (!strcmp(trailer_item, \"separators\")) {\n> \t\t\tseparators = xstrdup(value);\n> +\t\t} else if (!strcmp(trailer_item, \"trimempty\")) {\n> +\t\t\ttrim_empty = git_config_bool(conf_key, value);\n> \t\t}\n> \t}\n> \treturn 0;\n> @@ -842,7 +846,7 @@ static void free_all(struct trailer_item **first)\n> \t}\n> }\n> \n> -void process_trailers(const char *file, int trim_empty, struct string_list *trailers)\n> +void process_trailers(const char *file, int trim, struct string_list *trailers)\n> {\n> \tstruct trailer_item *in_tok_first = NULL;\n> \tstruct trailer_item *in_tok_last = NULL;\n> @@ -854,6 +858,9 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai\n> \tgit_config(git_trailer_default_config, NULL);\n> \tgit_config(git_trailer_config, NULL);\n> \n> +\tif (trim > -1)\n> +\t\ttrim_empty = trim;\n> +\n> \tlines = read_input_file(file);\n> \n> \t/* Print the lines before the trailers */\n> @@ -863,7 +870,7 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai\n> \n> \tprocess_trailers_lists(&in_tok_first, &in_tok_last, &arg_tok_first);\n> \n> -\tprint_all(in_tok_first, trim_empty);\n> +\tprint_all(in_tok_first);\n> \n> \tfree_all(&in_tok_first);\n> \n> diff --git a/trailer.h b/trailer.h\n> index 8eb25d5..4f481d0 100644\n> --- a/trailer.h\n> +++ b/trailer.h\n> @@ -1,6 +1,6 @@\n> #ifndef TRAILER_H\n> #define TRAILER_H\n> \n> -void process_trailers(const char *file, int trim_empty, struct string_list *trailers);\n> +void process_trailers(const char *file, int trim, struct string_list *trailers);\n> \n> #endif /* TRAILER_H */\n"},{"id":"255711","messageId":"CAP8UFD1pWH5yaJaJ_gV1t5q5qfgs58AtcSr+ZqDTnWzfHK1uBw@mail.gmail.com","threadId":"38519","inReplyTo":"xmqq386hcw33.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/3] trailer: add a trailer.trimEmpty config option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-02-07T22:19:31Z","receivedAt":"2015-02-07T22:19:31Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Feb 7, 2015 at 9:20 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> This way people who always want trimed trailers\n>> don't need to specify it on the command line.\n>\n> I sense that print_all() that cares about trimming illustrate a\n> design mistake in the original code structure. Why isn't the entire\n> program structured like this instead?\n>\n> - read input and split that into three parts:\n>\n> struct {\n> struct strbuf messsage_proper;\n>\n> struct trailers {\n> int nr, alloc;\n> struct strbuf *array_of_trailers[];\n> } trailers;\n>\n> struct strbuf lines_after_trailers;\n> };\n\nIt is not designed like this because you only asked me to design it\nlike this after the facts, when there was another email thread about\nconflicts blocks and one function you created could be used by the\ntrailer code too.\n\nIf you had asked this from the beginning I would certainly have done\nit more like this (though I think the \"struct trailers\" part is too\nsimplistic). Rearchitecturing the code now would bring only small\nperformance improvements and a lot of code churn. And the performance\nis not needed anyway if the code is not used in the first place, so\nI'd rather first make sure that the code can be used.\n\nI think that very few new features are now needed to make it possible\nto use the code in other commands like commit, format-patch, am, etc,\nbut this patch implements one of the needed features.\n\n> - do \"trailer stuff\" by calling a central helper that does\n> \"trailer stuff\" a pointer to the middle, trailers, struct.\n>\n> - when the trailer becomes a reusable subsystem, this central\n> helper will become the primary entry point to the API.\n>\n> - \"trailer stuff\" will include adding new ones, removing\n> existing ones, and rewriting lines. What you do in the\n> current process_command_line_args() and\n> process_trailers_lists() [*1*] would come here.\n>\n> - write out the result, given the outermost struct. This will\n> become another entry point in the API.\n>\n> Structured that way, callers will supply that outermost structure,\n> and can trust that the trailers subsystem will not molest\n> message_proper or lines_after_trailers part.\n\nI don't think it is a big improvement because it is easy to see that\nthe current code doesn't molest the part before and after the\ntrailers.\n\n> They can even process\n> the parts that the trailer subsystem would not touch, e.g. running\n> stripspace to the text in message_proper.\n\nThat could be worth the rearchitecturing if people really wanted that,\nbut I think for now more people have been interested in having ways to\nchange the trailer part, so I prefer to work on this first.\n\n> Viewed that way, it would be clear that \"strip empty ones\" should be\n> a new feature in the \"trailer stuff\", not just \"filter out only\n> during the output phase\". Having it in the output phase does not\n> feel that the labor/responsibility is split in the right way.\n\nMy opinion is that having it in the output phase is best.\n\n> Another problem I have with \"filter out during the output phase\"\n> comes from the semantics/correctness in the resulting code, and I\n> suspect that it would need to be done a lot earlier, before you\n> allow the logic such as \"if I have X, do this, but if there is no X,\n> do this other thing\". If you have an X that is empty in the input,\n> trimming that before such logic kicks in and trimming that in the\n> final output phase would give different results, and I think the\n> latter would give a less intuitive result.\n\nI think it is much better in the output phase because it is very\nnatural for some projects to have a template message with empty\ntrailers like this:\n\nSigned-off-by:\nReviewed-by:\n\nSuch a template message can for example remind contributors to the\nproject that they need to sign off their work and that they need to\nhave it reviewed by at least one person, and that to make it simpler\nfor everyone to review patches, the \"Signed-off-by\" trailers should\ncome before the \"Reviewed-by\" trailers in the commit message.\n\nIn this case, if you trim before you process command line trailers,\nthen, when you process some command line trailers that add sign offs,\nthe meaning of \"where=after\" cannot be based any more on the existing\nempty \"Signed-off-by:\" in the template message.\n\n> As this is the second time I have to point out that the data\n> structure used by the current code to hold the trailers and other\n> stuff to work on smells fishy, I would seriously consider cleaning\n> up the existing code to make it easier to allow us to later create\n> a reusable API and \"trailer subsystem\" out of it, before piling new\n> features on top of it, if I were you.\n\nI am not so sure that it would make it easier to allow us to later\ncreate a reusable API and \"trailer subsystem\" out of it.\nIt is very difficult to predict what will be really needed in the\nfuture and it is not clear at all to me that the current API is not\nreusable.\n\nIf it was so clear that an API similar as what you describe was so\nimportant, why didn't you point out that at the very beginning when I\nstarted working on git interpret-trailers? And it's not like you\ndidn't have time to think about what I was working on as we discussed\nmany other aspects of the design and the developement happened over\none year.\n"},{"id":"255825","messageId":"xmqqy4o7aqqi.fsf@gitster.dls.corp.google.com","threadId":"38519","inReplyTo":"CAP8UFD1pWH5yaJaJ_gV1t5q5qfgs58AtcSr+ZqDTnWzfHK1uBw@mail.gmail.com","subject":"Re: [PATCH 1/3] trailer: add a trailer.trimEmpty config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-09T18:23:33Z","receivedAt":"2015-02-09T18:23:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> It is not designed like this because you only asked me to design it\n> like this after the facts, when there was another email thread about\n> conflicts blocks and one function you created could be used by the\n> trailer code too.\n>\n> If you had asked this from the beginning I would certainly have done...\n\nBecause the process here is not somebody outlines the design and\ngives it to laborer to implement, \"after the fact\" and \"from the\nbeginning\" are complaining to and asking for impossible. You design\nand implement and then it gets reviewed, not the other way around.\n"},{"id":"255901","messageId":"xmqqsiedv37o.fsf@gitster.dls.corp.google.com","threadId":"38519","inReplyTo":"CAP8UFD1pWH5yaJaJ_gV1t5q5qfgs58AtcSr+ZqDTnWzfHK1uBw@mail.gmail.com","subject":"Re: [PATCH 1/3] trailer: add a trailer.trimEmpty config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-10T21:58:19Z","receivedAt":"2015-02-10T21:58:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> I think that very few new features are now needed to make it possible\n> to use the code in other commands like commit, format-patch, am, etc,\n> but this patch implements one of the needed features.\n>\n>> - do \"trailer stuff\" by calling a central helper that does\n>> \"trailer stuff\" a pointer to the middle, trailers, struct.\n>>\n>> - when the trailer becomes a reusable subsystem, this central\n>> helper will become the primary entry point to the API.\n>>\n>> - \"trailer stuff\" will include adding new ones, removing\n>> existing ones, and rewriting lines. What you do in the\n>> current process_command_line_args() and\n>> process_trailers_lists() [*1*] would come here.\n>>\n>> - write out the result, given the outermost struct. This will\n>> become another entry point in the API.\n>>\n>> Structured that way, callers will supply that outermost structure,\n>> and can trust that the trailers subsystem will not molest\n>> message_proper or lines_after_trailers part.\n>\n> I don't think it is a big improvement because it is easy to see that\n> the current code doesn't molest the part before and after the\n> trailers.\n\nYou force the callers that want only the \"trailer\" thing to happen\nto:\n\n - pass first and last around.\n\n - keep each line of the message body in separate strbuf and have\n it in the same array as the trailers\n\nNeither of which is necessary. I recall that during the review of\nthe previous rounds your own code had to work this around by first\nconcatenating lines (each of which are unnecessarily in separate\n\"struct strbuf\"s) into a single strbuf, only to call a helper that\ntakes a single strbuf to count what to ignore in it, and then\niterate over the array of strbuf to add up the lengths of them,\nwhich would have been unnecessary if the underlying data structure\nwere saner.\n"},{"id":"255902","messageId":"xmqqoap1v2to.fsf@gitster.dls.corp.google.com","threadId":"38519","inReplyTo":"CAP8UFD1pWH5yaJaJ_gV1t5q5qfgs58AtcSr+ZqDTnWzfHK1uBw@mail.gmail.com","subject":"Re: [PATCH 1/3] trailer: add a trailer.trimEmpty config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-10T22:06:43Z","receivedAt":"2015-02-10T22:06:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Sat, Feb 7, 2015 at 9:20 PM, Junio C Hamano wrote:\n>\n>> Another problem I have with \"filter out during the output phase\"\n>> comes from the semantics/correctness in the resulting code, and I\n>> suspect that it would need to be done a lot earlier, before you\n>> allow the logic such as \"if I have X, do this, but if there is no X,\n>> do this other thing\". If you have an X that is empty in the input,\n>> trimming that before such logic kicks in and trimming that in the\n>> final output phase would give different results, and I think the\n>> latter would give a less intuitive result.\n>\n> I think it is much better in the output phase because it is very\n> natural for some projects to have a template message with empty\n> trailers like this:\n\nThat is exactly my point. With empty trailers in the input, \"Add\nthis iff there is no existing one\" will be made useless.\n\nI am *not* saying that we must not have a filter at the output\nphrase. If anything, it would allow people to be more sloppy and\nhide the problem under the rug. Your code may have a bug or design\nproblem that adds an empty one somewhere even when the user told you\nthat she does not want an empty one in the result. The user may be\nsloppy and say \"Add this key-value\" unconditionally, instead of\nhaving to do \"What is the value I want to add? Oh, it is not empty,\nso I'll ask the trailer machiner to add this key-value there\".\n\nI am saying that not filtering the input and whatever intermediate\nresult you produce [*1*] will make the end result much less useful.\n\n[Footnote]\n\n*1* Filtering intermediate result of course can be done by making\nsure you do not add an empty one in the first place.\n"}]}