{"thread":{"id":"44343","subject":"[PATCH v4 1/8] trailer: improve const correctness","startedAt":"2016-10-20T21:40:12Z","lastAt":"2016-10-22T16:19:41Z","messageCount":19,"participants":["Jonathan Tan","Stefan Beller","Junio C Hamano","Christian Couder"],"isPatch":true,"patchVersion":4,"patchTotal":8},"messages":[{"id":"304522","messageId":"701ba7164b587a97ef380bc981c95c5d51db6781.1476998988.git.jonathantanmy@google.com","threadId":"44343","inReplyTo":"cover.1476998988.git.jonathantanmy@google.com","subject":"[PATCH v4 1/8] trailer: improve const correctness","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-20T21:39:46Z","receivedAt":"2016-10-20T21:40:12Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Change \"const char *\" to \"char *\" in struct trailer_item and in the\nreturn value of apply_command (since those strings are owned strings).\n\nChange \"struct conf_info *\" to \"const struct conf_info *\" (since that\nstruct is not modified).\n\nSigned-off-by: Jonathan Tan \n---\n trailer.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex c6ea9ac..1f191b2 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -27,8 +27,8 @@ static struct conf_info default_conf_info;\n struct trailer_item {\n \tstruct trailer_item *previous;\n \tstruct trailer_item *next;\n-\tconst char *token;\n-\tconst char *value;\n+\tchar *token;\n+\tchar *value;\n \tstruct conf_info conf;\n };\n \n@@ -95,8 +95,8 @@ static void free_trailer_item(struct trailer_item *item)\n \tfree(item->conf.name);\n \tfree(item->conf.key);\n \tfree(item->conf.command);\n-\tfree((char *)item->token);\n-\tfree((char *)item->value);\n+\tfree(item->token);\n+\tfree(item->value);\n \tfree(item);\n }\n \n@@ -215,13 +215,13 @@ static struct trailer_item *remove_first(struct trailer_item **first)\n \treturn item;\n }\n \n-static const char *apply_command(const char *command, const char *arg)\n+static char *apply_command(const char *command, const char *arg)\n {\n \tstruct strbuf cmd = STRBUF_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tconst char *argv[] = {NULL, NULL};\n-\tconst char *result;\n+\tchar *result;\n \n \tstrbuf_addstr(&cmd, command);\n \tif (arg)\n@@ -425,7 +425,7 @@ static int set_if_missing(struct conf_info *item, const char *value)\n \treturn 0;\n }\n \n-static void duplicate_conf(struct conf_info *dst, struct conf_info *src)\n+static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)\n {\n \t*dst = *src;\n \tif (src->name)\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"304523","messageId":"cover.1476998988.git.jonathantanmy@google.com","threadId":"44343","inReplyTo":"cover.1476232683.git.jonathantanmy@google.com","subject":"[PATCH v4 0/8] allow non-trailers and multiple-line trailers","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-20T21:39:45Z","receivedAt":"2016-10-20T21:40:17Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Main changes are:\n - implemented the previously discussed trailer block recognizing rule\n (recognized trailer + 25% trailers or 100% trailers)\n - forbidding leading whitespace in trailers to avoid false positives\n\nOnce the recognized trailer + 25% trailers rule is implemented,\nimplementing the 100% trailer rule gives us backwards compatibility and\nis only a few lines of code, so I included it.\n\nSummary of changes from v3:\n 2/6->2/8:\n - squashed Ramsay Jones's \"static\" patch\n new->5/8:\n - new patch\n 5/6->6/8:\n - new trailer block recognizing rule\n - reverted to the existing behavior of ignoring comments, since the\n number of trailers and non-trailers in the trailer block now\n matters more\n new->7/8:\n - new patch\n 6/6->8/8:\n - updated trailer block recognizing code, since the continuation\n lines must not be counted if they follow a trailer line\n\nJonathan Tan (8):\n trailer: improve const correctness\n trailer: use list.h for doubly-linked list\n trailer: streamline trailer item create and add\n trailer: make args have their own struct\n trailer: clarify failure modes in parse_trailer\n trailer: allow non-trailers in trailer block\n trailer: forbid leading whitespace in trailers\n trailer: support values folded to multiple lines\n\n Documentation/git-interpret-trailers.txt | 14 +-\n t/t7513-interpret-trailers.sh | 299 +++++++++++++++\n trailer.c | 619 +++++++++++++++++--------------\n 3 files changed, 651 insertions(+), 281 deletions(-)\n\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"304524","messageId":"f0acfc7d23ce76a2562b9125af6f8b270baf7b8c.1476998988.git.jonathantanmy@google.com","threadId":"44343","inReplyTo":"cover.1476998988.git.jonathantanmy@google.com","subject":"[PATCH v4 2/8] trailer: use list.h for doubly-linked list","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-20T21:39:47Z","receivedAt":"2016-10-20T21:40:23Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Replace the existing handwritten implementation of a doubly-linked list\nin trailer.c with the functions and macros from list.h. This\nsignificantly simplifies the code.\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Ramsay Jones \n---\n trailer.c | 258 ++++++++++++++++++++++----------------------------------------\n 1 file changed, 91 insertions(+), 167 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 1f191b2..4e85aae 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -4,6 +4,7 @@\n #include \"commit.h\"\n #include \"tempfile.h\"\n #include \"trailer.h\"\n+#include \"list.h\"\n /*\n * Copyright (c) 2013, 2014 Christian Couder \n */\n@@ -25,19 +26,24 @@ struct conf_info {\n static struct conf_info default_conf_info;\n \n struct trailer_item {\n-\tstruct trailer_item *previous;\n-\tstruct trailer_item *next;\n+\tstruct list_head list;\n \tchar *token;\n \tchar *value;\n \tstruct conf_info conf;\n };\n \n-static struct trailer_item *first_conf_item;\n+static LIST_HEAD(conf_head);\n \n static char *separators = \":\";\n \n #define TRAILER_ARG_STRING \"$ARG\"\n \n+/* Iterate over the elements of the list. */\n+#define list_for_each_dir(pos, head, is_reverse) \\\n+\tfor (pos = is_reverse ? (head)->prev : (head)->next; \\\n+\t\tpos != (head); \\\n+\t\tpos = is_reverse ? pos->prev : pos->next)\n+\n static int after_or_end(enum action_where where)\n {\n \treturn (where == WHERE_AFTER) || (where == WHERE_END);\n@@ -120,101 +126,49 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)\n \t\tfprintf(outfile, \"%s%c %s\\n\", tok, separators[0], val);\n }\n \n-static void print_all(FILE *outfile, struct trailer_item *first, int trim_empty)\n+static void print_all(FILE *outfile, struct list_head *head, int trim_empty)\n {\n+\tstruct list_head *pos;\n \tstruct trailer_item *item;\n-\tfor (item = first; item; item = item->next) {\n+\tlist_for_each(pos, head) {\n+\t\titem = list_entry(pos, struct trailer_item, list);\n \t\tif (!trim_empty || strlen(item->value) > 0)\n \t\t\tprint_tok_val(outfile, item->token, item->value);\n \t}\n }\n \n-static void update_last(struct trailer_item **last)\n-{\n-\tif (*last)\n-\t\twhile ((*last)->next != NULL)\n-\t\t\t*last = (*last)->next;\n-}\n-\n-static void update_first(struct trailer_item **first)\n-{\n-\tif (*first)\n-\t\twhile ((*first)->previous != NULL)\n-\t\t\t*first = (*first)->previous;\n-}\n-\n static void add_arg_to_input_list(struct trailer_item *on_tok,\n-\t\t\t\t struct trailer_item *arg_tok,\n-\t\t\t\t struct trailer_item **first,\n-\t\t\t\t struct trailer_item **last)\n-{\n-\tif (after_or_end(arg_tok->conf.where)) {\n-\t\targ_tok->next = on_tok->next;\n-\t\ton_tok->next = arg_tok;\n-\t\targ_tok->previous = on_tok;\n-\t\tif (arg_tok->next)\n-\t\t\targ_tok->next->previous = arg_tok;\n-\t\tupdate_last(last);\n-\t} else {\n-\t\targ_tok->previous = on_tok->previous;\n-\t\ton_tok->previous = arg_tok;\n-\t\targ_tok->next = on_tok;\n-\t\tif (arg_tok->previous)\n-\t\t\targ_tok->previous->next = arg_tok;\n-\t\tupdate_first(first);\n-\t}\n+\t\t\t\t struct trailer_item *arg_tok)\n+{\n+\tif (after_or_end(arg_tok->conf.where))\n+\t\tlist_add(&arg_tok->list, &on_tok->list);\n+\telse\n+\t\tlist_add_tail(&arg_tok->list, &on_tok->list);\n }\n \n static int check_if_different(struct trailer_item *in_tok,\n \t\t\t struct trailer_item *arg_tok,\n-\t\t\t int check_all)\n+\t\t\t int check_all,\n+\t\t\t struct list_head *head)\n {\n \tenum action_where where = arg_tok->conf.where;\n+\tstruct list_head *next_head;\n \tdo {\n-\t\tif (!in_tok)\n-\t\t\treturn 1;\n \t\tif (same_trailer(in_tok, arg_tok))\n \t\t\treturn 0;\n \t\t/*\n \t\t * if we want to add a trailer after another one,\n \t\t * we have to check those before this one\n \t\t */\n-\t\tin_tok = after_or_end(where) ? in_tok->previous : in_tok->next;\n+\t\tnext_head = after_or_end(where) ? in_tok->list.prev\n+\t\t\t\t\t\t: in_tok->list.next;\n+\t\tif (next_head == head)\n+\t\t\tbreak;\n+\t\tin_tok = list_entry(next_head, struct trailer_item, list);\n \t} while (check_all);\n \treturn 1;\n }\n \n-static void remove_from_list(struct trailer_item *item,\n-\t\t\t struct trailer_item **first,\n-\t\t\t struct trailer_item **last)\n-{\n-\tstruct trailer_item *next = item->next;\n-\tstruct trailer_item *previous = item->previous;\n-\n-\tif (next) {\n-\t\titem->next->previous = previous;\n-\t\titem->next = NULL;\n-\t} else if (last)\n-\t\t*last = previous;\n-\n-\tif (previous) {\n-\t\titem->previous->next = next;\n-\t\titem->previous = NULL;\n-\t} else if (first)\n-\t\t*first = next;\n-}\n-\n-static struct trailer_item *remove_first(struct trailer_item **first)\n-{\n-\tstruct trailer_item *item = *first;\n-\t*first = item->next;\n-\tif (item->next) {\n-\t\titem->next->previous = NULL;\n-\t\titem->next = NULL;\n-\t}\n-\treturn item;\n-}\n-\n static char *apply_command(const char *command, const char *arg)\n {\n \tstruct strbuf cmd = STRBUF_INIT;\n@@ -266,8 +220,7 @@ static void apply_item_command(struct trailer_item *in_tok, struct trailer_item\n static void apply_arg_if_exists(struct trailer_item *in_tok,\n \t\t\t\tstruct trailer_item *arg_tok,\n \t\t\t\tstruct trailer_item *on_tok,\n-\t\t\t\tstruct trailer_item **in_tok_first,\n-\t\t\t\tstruct trailer_item **in_tok_last)\n+\t\t\t\tstruct list_head *head)\n {\n \tswitch (arg_tok->conf.if_exists) {\n \tcase EXISTS_DO_NOTHING:\n@@ -275,40 +228,34 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,\n \t\tbreak;\n \tcase EXISTS_REPLACE:\n \t\tapply_item_command(in_tok, arg_tok);\n-\t\tadd_arg_to_input_list(on_tok, arg_tok,\n-\t\t\t\t in_tok_first, in_tok_last);\n-\t\tremove_from_list(in_tok, in_tok_first, in_tok_last);\n+\t\tadd_arg_to_input_list(on_tok, arg_tok);\n+\t\tlist_del(&in_tok->list);\n \t\tfree_trailer_item(in_tok);\n \t\tbreak;\n \tcase EXISTS_ADD:\n \t\tapply_item_command(in_tok, arg_tok);\n-\t\tadd_arg_to_input_list(on_tok, arg_tok,\n-\t\t\t\t in_tok_first, in_tok_last);\n+\t\tadd_arg_to_input_list(on_tok, arg_tok);\n \t\tbreak;\n \tcase EXISTS_ADD_IF_DIFFERENT:\n \t\tapply_item_command(in_tok, arg_tok);\n-\t\tif (check_if_different(in_tok, arg_tok, 1))\n-\t\t\tadd_arg_to_input_list(on_tok, arg_tok,\n-\t\t\t\t\t in_tok_first, in_tok_last);\n+\t\tif (check_if_different(in_tok, arg_tok, 1, head))\n+\t\t\tadd_arg_to_input_list(on_tok, arg_tok);\n \t\telse\n \t\t\tfree_trailer_item(arg_tok);\n \t\tbreak;\n \tcase EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:\n \t\tapply_item_command(in_tok, arg_tok);\n-\t\tif (check_if_different(on_tok, arg_tok, 0))\n-\t\t\tadd_arg_to_input_list(on_tok, arg_tok,\n-\t\t\t\t\t in_tok_first, in_tok_last);\n+\t\tif (check_if_different(on_tok, arg_tok, 0, head))\n+\t\t\tadd_arg_to_input_list(on_tok, arg_tok);\n \t\telse\n \t\t\tfree_trailer_item(arg_tok);\n \t\tbreak;\n \t}\n }\n \n-static void apply_arg_if_missing(struct trailer_item **in_tok_first,\n-\t\t\t\t struct trailer_item **in_tok_last,\n+static void apply_arg_if_missing(struct list_head *head,\n \t\t\t\t struct trailer_item *arg_tok)\n {\n-\tstruct trailer_item **in_tok;\n \tenum action_where where;\n \n \tswitch (arg_tok->conf.if_missing) {\n@@ -317,68 +264,60 @@ static void apply_arg_if_missing(struct trailer_item **in_tok_first,\n \t\tbreak;\n \tcase MISSING_ADD:\n \t\twhere = arg_tok->conf.where;\n-\t\tin_tok = after_or_end(where) ? in_tok_last : in_tok_first;\n \t\tapply_item_command(NULL, arg_tok);\n-\t\tif (*in_tok) {\n-\t\t\tadd_arg_to_input_list(*in_tok, arg_tok,\n-\t\t\t\t\t in_tok_first, in_tok_last);\n-\t\t} else {\n-\t\t\t*in_tok_first = arg_tok;\n-\t\t\t*in_tok_last = arg_tok;\n-\t\t}\n-\t\tbreak;\n+\t\tif (after_or_end(where))\n+\t\t\tlist_add_tail(&arg_tok->list, head);\n+\t\telse\n+\t\t\tlist_add(&arg_tok->list, head);\n \t}\n }\n \n-static int find_same_and_apply_arg(struct trailer_item **in_tok_first,\n-\t\t\t\t struct trailer_item **in_tok_last,\n+static int find_same_and_apply_arg(struct list_head *head,\n \t\t\t\t struct trailer_item *arg_tok)\n {\n+\tstruct list_head *pos;\n \tstruct trailer_item *in_tok;\n \tstruct trailer_item *on_tok;\n-\tstruct trailer_item *following_tok;\n \n \tenum action_where where = arg_tok->conf.where;\n \tint middle = (where == WHERE_AFTER) || (where == WHERE_BEFORE);\n \tint backwards = after_or_end(where);\n-\tstruct trailer_item *start_tok = backwards ? *in_tok_last : *in_tok_first;\n+\tstruct trailer_item *start_tok;\n \n-\tfor (in_tok = start_tok; in_tok; in_tok = following_tok) {\n-\t\tfollowing_tok = backwards ? in_tok->previous : in_tok->next;\n+\tif (list_empty(head))\n+\t\treturn 0;\n+\n+\tstart_tok = list_entry(backwards ? head->prev : head->next,\n+\t\t\t struct trailer_item,\n+\t\t\t list);\n+\n+\tlist_for_each_dir(pos, head, backwards) {\n+\t\tin_tok = list_entry(pos, struct trailer_item, list);\n \t\tif (!same_token(in_tok, arg_tok))\n \t\t\tcontinue;\n \t\ton_tok = middle ? in_tok : start_tok;\n-\t\tapply_arg_if_exists(in_tok, arg_tok, on_tok,\n-\t\t\t\t in_tok_first, in_tok_last);\n+\t\tapply_arg_if_exists(in_tok, arg_tok, on_tok, head);\n \t\treturn 1;\n \t}\n \treturn 0;\n }\n \n-static void process_trailers_lists(struct trailer_item **in_tok_first,\n-\t\t\t\t struct trailer_item **in_tok_last,\n-\t\t\t\t struct trailer_item **arg_tok_first)\n+static void process_trailers_lists(struct list_head *head,\n+\t\t\t\t struct list_head *arg_head)\n {\n+\tstruct list_head *pos, *p;\n \tstruct trailer_item *arg_tok;\n-\tstruct trailer_item *next_arg;\n-\n-\tif (!*arg_tok_first)\n-\t\treturn;\n \n-\tfor (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {\n+\tlist_for_each_safe(pos, p, arg_head) {\n \t\tint applied = 0;\n+\t\targ_tok = list_entry(pos, struct trailer_item, list);\n \n-\t\tnext_arg = arg_tok->next;\n-\t\tremove_from_list(arg_tok, arg_tok_first, NULL);\n+\t\tlist_del(pos);\n \n-\t\tapplied = find_same_and_apply_arg(in_tok_first,\n-\t\t\t\t\t\t in_tok_last,\n-\t\t\t\t\t\t arg_tok);\n+\t\tapplied = find_same_and_apply_arg(head, arg_tok);\n \n \t\tif (!applied)\n-\t\t\tapply_arg_if_missing(in_tok_first,\n-\t\t\t\t\t in_tok_last,\n-\t\t\t\t\t arg_tok);\n+\t\t\tapply_arg_if_missing(head, arg_tok);\n \t}\n }\n \n@@ -438,13 +377,12 @@ static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)\n \n static struct trailer_item *get_conf_item(const char *name)\n {\n+\tstruct list_head *pos;\n \tstruct trailer_item *item;\n-\tstruct trailer_item *previous;\n \n \t/* Look up item with same name */\n-\tfor (previous = NULL, item = first_conf_item;\n-\t item;\n-\t previous = item, item = item->next) {\n+\tlist_for_each(pos, &conf_head) {\n+\t\titem = list_entry(pos, struct trailer_item, list);\n \t\tif (!strcasecmp(item->conf.name, name))\n \t\t\treturn item;\n \t}\n@@ -454,12 +392,7 @@ static struct trailer_item *get_conf_item(const char *name)\n \tduplicate_conf(&item->conf, &default_conf_info);\n \titem->conf.name = xstrdup(name);\n \n-\tif (!previous)\n-\t\tfirst_conf_item = item;\n-\telse {\n-\t\tprevious->next = item;\n-\t\titem->previous = previous;\n-\t}\n+\tlist_add_tail(&item->list, &conf_head);\n \n \treturn item;\n }\n@@ -633,6 +566,7 @@ static struct trailer_item *create_trailer_item(const char *string)\n \tstruct strbuf val = STRBUF_INIT;\n \tstruct trailer_item *item;\n \tint tok_len;\n+\tstruct list_head *pos;\n \n \tif (parse_trailer(&tok, &val, string))\n \t\treturn NULL;\n@@ -640,7 +574,8 @@ static struct trailer_item *create_trailer_item(const char *string)\n \ttok_len = token_len_without_separator(tok.buf, tok.len);\n \n \t/* Lookup if the token matches something in the config */\n-\tfor (item = first_conf_item; item; item = item->next) {\n+\tlist_for_each(pos, &conf_head) {\n+\t\titem = list_entry(pos, struct trailer_item, list);\n \t\tif (token_matches_item(tok.buf, item, tok_len))\n \t\t\treturn new_trailer_item(item,\n \t\t\t\t\t\tstrbuf_detach(&tok, NULL),\n@@ -652,44 +587,34 @@ static struct trailer_item *create_trailer_item(const char *string)\n \t\t\t\tstrbuf_detach(&val, NULL));\n }\n \n-static void add_trailer_item(struct trailer_item **first,\n-\t\t\t struct trailer_item **last,\n-\t\t\t struct trailer_item *new)\n+static void add_trailer_item(struct list_head *head, struct trailer_item *new)\n {\n \tif (!new)\n \t\treturn;\n-\tif (!*last) {\n-\t\t*first = new;\n-\t\t*last = new;\n-\t} else {\n-\t\t(*last)->next = new;\n-\t\tnew->previous = *last;\n-\t\t*last = new;\n-\t}\n+\tlist_add_tail(&new->list, head);\n }\n \n-static struct trailer_item *process_command_line_args(struct string_list *trailers)\n+static void process_command_line_args(struct list_head *arg_head, \n+\t\t\t\t struct string_list *trailers)\n {\n-\tstruct trailer_item *arg_tok_first = NULL;\n-\tstruct trailer_item *arg_tok_last = NULL;\n \tstruct string_list_item *tr;\n \tstruct trailer_item *item;\n+\tstruct list_head *pos;\n \n \t/* Add a trailer item for each configured trailer with a command */\n-\tfor (item = first_conf_item; item; item = item->next) {\n+\tlist_for_each(pos, &conf_head) {\n+\t\titem = list_entry(pos, struct trailer_item, list);\n \t\tif (item->conf.command) {\n \t\t\tstruct trailer_item *new = new_trailer_item(item, NULL, NULL);\n-\t\t\tadd_trailer_item(&arg_tok_first, &arg_tok_last, new);\n+\t\t\tadd_trailer_item(arg_head, new);\n \t\t}\n \t}\n \n \t/* Add a trailer item for each trailer on the command line */\n \tfor_each_string_list_item(tr, trailers) {\n \t\tstruct trailer_item *new = create_trailer_item(tr->string);\n-\t\tadd_trailer_item(&arg_tok_first, &arg_tok_last, new);\n+\t\tadd_trailer_item(arg_head, new);\n \t}\n-\n-\treturn arg_tok_first;\n }\n \n static struct strbuf **read_input_file(const char *file)\n@@ -805,8 +730,7 @@ static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end\n \n static int process_input_file(FILE *outfile,\n \t\t\t struct strbuf **lines,\n-\t\t\t struct trailer_item **in_tok_first,\n-\t\t\t struct trailer_item **in_tok_last)\n+\t\t\t struct list_head *head)\n {\n \tint count = 0;\n \tint patch_start, trailer_start, trailer_end, i;\n@@ -829,18 +753,19 @@ static int process_input_file(FILE *outfile,\n \tfor (i = trailer_start; i < trailer_end; i++) {\n \t\tif (lines[i]->buf[0] != comment_line_char) {\n \t\t\tstruct trailer_item *new = create_trailer_item(lines[i]->buf);\n-\t\t\tadd_trailer_item(in_tok_first, in_tok_last, new);\n+\t\t\tadd_trailer_item(head, new);\n \t\t}\n \t}\n \n \treturn trailer_end;\n }\n \n-static void free_all(struct trailer_item **first)\n+static void free_all(struct list_head *head)\n {\n-\twhile (*first) {\n-\t\tstruct trailer_item *item = remove_first(first);\n-\t\tfree_trailer_item(item);\n+\tstruct list_head *pos, *p;\n+\tlist_for_each_safe(pos, p, head) {\n+\t\tlist_del(pos);\n+\t\tfree_trailer_item(list_entry(pos, struct trailer_item, list));\n \t}\n }\n \n@@ -877,9 +802,8 @@ static FILE *create_in_place_tempfile(const char *file)\n \n void process_trailers(const char *file, int in_place, int trim_empty, struct string_list *trailers)\n {\n-\tstruct trailer_item *in_tok_first = NULL;\n-\tstruct trailer_item *in_tok_last = NULL;\n-\tstruct trailer_item *arg_tok_first;\n+\tLIST_HEAD(head);\n+\tLIST_HEAD(arg_head);\n \tstruct strbuf **lines;\n \tint trailer_end;\n \tFILE *outfile = stdout;\n@@ -894,15 +818,15 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n \t\toutfile = create_in_place_tempfile(file);\n \n \t/* Print the lines before the trailers */\n-\ttrailer_end = process_input_file(outfile, lines, &in_tok_first, &in_tok_last);\n+\ttrailer_end = process_input_file(outfile, lines, &head);\n \n-\targ_tok_first = process_command_line_args(trailers);\n+\tprocess_command_line_args(&arg_head, trailers);\n \n-\tprocess_trailers_lists(&in_tok_first, &in_tok_last, &arg_tok_first);\n+\tprocess_trailers_lists(&head, &arg_head);\n \n-\tprint_all(outfile, in_tok_first, trim_empty);\n+\tprint_all(outfile, &head, trim_empty);\n \n-\tfree_all(&in_tok_first);\n+\tfree_all(&head);\n \n \t/* Print the lines after the trailers as is */\n \tprint_lines(outfile, lines, trailer_end, INT_MAX);\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"304525","messageId":"c4d2db03105c8872014ab2514379bb3c05bf3a0e.1476998988.git.jonathantanmy@google.com","threadId":"44343","inReplyTo":"cover.1476998988.git.jonathantanmy@google.com","subject":"[PATCH v4 6/8] trailer: allow non-trailers in trailer block","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-20T21:39:51Z","receivedAt":"2016-10-20T21:40:36Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, interpret-trailers requires all lines of a trailer block to\nbe trailers (or comments) - if not it would not identify that block as a\ntrailer block, and thus create its own trailer block, inserting a blank\nline. For example:\n\n echo -e \"\\nSigned-off-by: x\\nnot trailer\" |\n git interpret-trailers --trailer \"c: d\"\n\nwould result in:\n\n Signed-off-by: x\n not trailer\n\n c: d\n\nRelax the definition of a trailer block to require that the trailers (i)\nare all trailers, or (ii) contain at least one Git-generated trailer and\nconsists of at least 25% trailers.\n\n Signed-off-by: x\n not trailer\n c: d\n\n(i) is the existing functionality. (ii) allows arbitrary lines to be\nincluded in trailer blocks, like those in [1], and still allow\ninterpret-trailers to be used.\n\n[1]\nhttps://kernel.googlesource.com/pub/scm/linux/kernel/git/stable/linux-stable/+/e7d316a02f683864a12389f8808570e37fb90aa3\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/git-interpret-trailers.txt | 5 +-\n t/t7513-interpret-trailers.sh | 115 +++++++++++++++++++++++++++++++\n trailer.c | 89 ++++++++++++++++++++----\n 3 files changed, 194 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt\nindex 93d1db6..cf4c5ea 100644\n--- a/Documentation/git-interpret-trailers.txt\n+++ b/Documentation/git-interpret-trailers.txt\n@@ -48,8 +48,9 @@ with only spaces at the end of the commit message part, one blank line\n will be added before the new trailer.\n \n Existing trailers are extracted from the input message by looking for\n-a group of one or more lines that contain a colon (by default), where\n-the group is preceded by one or more empty (or whitespace-only) lines.\n+a group of one or more lines that (i) are all trailers, or (ii) contains at\n+least one Git-generated trailer and consists of at least 25% trailers.\n+The group must be preceded by one or more empty (or whitespace-only) lines.\n The group must either be at the end of the message or be the last\n non-whitespace lines before a line that starts with '---'. Such three\n minus signs start the patch part of the message.\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex aee785c..003e90f 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -126,6 +126,121 @@ test_expect_success 'with multiline title in the message' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'with non-trailer lines mixed with Signed-off-by' '\n+\tcat >patch <<-\\EOF &&\n+\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\t\tSigned-off-by: a \n+\t\tthis is not a trailer\n+\tEOF\n+\tcat >expected <<-\\EOF &&\n+\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\t\tSigned-off-by: a \n+\t\tthis is not a trailer\n+\t\ttoken: value\n+\tEOF\n+\tgit interpret-trailers --trailer \"token: value\" patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'with non-trailer lines mixed with cherry picked from' '\n+\tcat >patch <<-\\EOF &&\n+\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\t\t(cherry picked from commit x)\n+\t\tthis is not a trailer\n+\tEOF\n+\tcat >expected <<-\\EOF &&\n+\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\t\t(cherry picked from commit x)\n+\t\tthis is not a trailer\n+\t\ttoken: value\n+\tEOF\n+\tgit interpret-trailers --trailer \"token: value\" patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'with non-trailer lines mixed with a configured trailer' '\n+\tcat >patch <<-\\EOF &&\n+\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\t\tMy-trailer: x\n+\t\tthis is not a trailer\n+\tEOF\n+\tcat >expected <<-\\EOF &&\n+\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\t\tMy-trailer: x\n+\t\tthis is not a trailer\n+\t\ttoken: value\n+\tEOF\n+\ttest_config trailer.my.key \"My-trailer: \" &&\n+\tgit interpret-trailers --trailer \"token: value\" patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'with non-trailer lines mixed with a non-configured trailer' '\n+\tcat >patch <<-\\EOF &&\n+\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\t\tI-am-not-configured: x\n+\t\tthis is not a trailer\n+\tEOF\n+\tcat >expected <<-\\EOF &&\n+\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\t\tI-am-not-configured: x\n+\t\tthis is not a trailer\n+\n+\t\ttoken: value\n+\tEOF\n+\ttest_config trailer.my.key \"My-trailer: \" &&\n+\tgit interpret-trailers --trailer \"token: value\" patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'with all non-configured trailers' '\n+\tcat >patch <<-\\EOF &&\n+\n+\t\tI-am-not-configured: x\n+\t\tI-am-also-not-configured: x\n+\tEOF\n+\tcat >expected <<-\\EOF &&\n+\n+\t\tI-am-not-configured: x\n+\t\tI-am-also-not-configured: x\n+\t\ttoken: value\n+\tEOF\n+\ttest_config trailer.my.key \"My-trailer: \" &&\n+\tgit interpret-trailers --trailer \"token: value\" patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'with non-trailer lines only' '\n+\tcat >patch <<-\\EOF &&\n+\n+\t\tthis is not a trailer\n+\tEOF\n+\tcat >expected <<-\\EOF &&\n+\n+\t\tthis is not a trailer\n+\n+\t\ttoken: value\n+\tEOF\n+\tgit interpret-trailers --trailer \"token: value\" patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'with config setup' '\n \tgit config trailer.ack.key \"Acked-by: \" &&\n \tcat >expected <<-\\EOF &&\ndiff --git a/trailer.c b/trailer.c\nindex 137a3fb..da15b79 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -27,6 +27,10 @@ static struct conf_info default_conf_info;\n \n struct trailer_item {\n \tstruct list_head list;\n+\t/*\n+\t * If this is not a trailer line, the line is stored in value\n+\t * (excluding the terminating newline) and token is NULL.\n+\t */\n \tchar *token;\n \tchar *value;\n };\n@@ -44,6 +48,12 @@ static char *separators = \":\";\n \n #define TRAILER_ARG_STRING \"$ARG\"\n \n+static const char *git_generated_prefixes[] = {\n+\t\"Signed-off-by: \",\n+\t\"(cherry picked from commit \",\n+\tNULL\n+};\n+\n /* Iterate over the elements of the list. */\n #define list_for_each_dir(pos, head, is_reverse) \\\n \tfor (pos = is_reverse ? (head)->prev : (head)->next; \\\n@@ -70,9 +80,14 @@ static size_t token_len_without_separator(const char *token, size_t len)\n \n static int same_token(struct trailer_item *a, struct arg_item *b)\n {\n-\tsize_t a_len = token_len_without_separator(a->token, strlen(a->token));\n-\tsize_t b_len = token_len_without_separator(b->token, strlen(b->token));\n-\tsize_t min_len = (a_len > b_len) ? b_len : a_len;\n+\tsize_t a_len, b_len, min_len;\n+\n+\tif (!a->token)\n+\t\treturn 0;\n+\n+\ta_len = token_len_without_separator(a->token, strlen(a->token));\n+\tb_len = token_len_without_separator(b->token, strlen(b->token));\n+\tmin_len = (a_len > b_len) ? b_len : a_len;\n \n \treturn !strncasecmp(a->token, b->token, min_len);\n }\n@@ -130,7 +145,14 @@ static char last_non_space_char(const char *s)\n \n static void print_tok_val(FILE *outfile, const char *tok, const char *val)\n {\n-\tchar c = last_non_space_char(tok);\n+\tchar c;\n+\n+\tif (!tok) {\n+\t\tfprintf(outfile, \"%s\\n\", val);\n+\t\treturn;\n+\t}\n+\n+\tc = last_non_space_char(tok);\n \tif (!c)\n \t\treturn;\n \tif (strchr(separators, c))\n@@ -704,6 +726,7 @@ static int find_patch_start(struct strbuf **lines, int count)\n static int find_trailer_start(struct strbuf **lines, int count)\n {\n \tint start, end_of_title, only_spaces = 1;\n+\tint recognized_prefix = 0, trailer_lines = 0, non_trailer_lines = 0;\n \n \t/* The first paragraph is the title and cannot be trailers */\n \tfor (start = 0; start < count; start++) {\n@@ -715,26 +738,60 @@ static int find_trailer_start(struct strbuf **lines, int count)\n \tend_of_title = start;\n \n \t/*\n-\t * Get the start of the trailers by looking starting from the end\n-\t * for a line with only spaces before lines with one separator.\n+\t * Get the start of the trailers by looking starting from the end for a\n+\t * blank line before a set of non-blank lines that (i) are all\n+\t * trailers, or (ii) contains at least one Git-generated trailer and\n+\t * consists of at least 25% trailers.\n \t */\n \tfor (start = count - 1; start >= end_of_title; start--) {\n+\t\tconst char **p;\n+\t\tint separator_pos;\n+\n \t\tif (lines[start]->buf[0] == comment_line_char)\n \t\t\tcontinue;\n \t\tif (contains_only_spaces(lines[start]->buf)) {\n \t\t\tif (only_spaces)\n \t\t\t\tcontinue;\n-\t\t\treturn start + 1;\n+\t\t\tif (recognized_prefix &&\n+\t\t\t trailer_lines * 3 >= non_trailer_lines)\n+\t\t\t\treturn start + 1;\n+\t\t\tif (trailer_lines && !non_trailer_lines)\n+\t\t\t\treturn start + 1;\n+\t\t\treturn count;\n \t\t}\n-\t\tif (strcspn(lines[start]->buf, separators) < lines[start]->len) {\n-\t\t\tif (only_spaces)\n-\t\t\t\tonly_spaces = 0;\n-\t\t\tcontinue;\n+\t\tonly_spaces = 0;\n+\n+\t\tfor (p = git_generated_prefixes; *p; p++) {\n+\t\t\tif (starts_with(lines[start]->buf, *p)) {\n+\t\t\t\ttrailer_lines++;\n+\t\t\t\trecognized_prefix = 1;\n+\t\t\t\tgoto continue_outer_loop;\n+\t\t\t}\n \t\t}\n-\t\treturn count;\n+\n+\t\tseparator_pos = find_separator(lines[start]->buf);\n+\t\tif (separator_pos >= 1) {\n+\t\t\tstruct list_head *pos;\n+\n+\t\t\ttrailer_lines++;\n+\t\t\tif (recognized_prefix)\n+\t\t\t\tcontinue;\n+\t\t\tlist_for_each(pos, &conf_head) {\n+\t\t\t\tstruct arg_item *item;\n+\t\t\t\titem = list_entry(pos, struct arg_item, list);\n+\t\t\t\tif (token_matches_item(lines[start]->buf, item,\n+\t\t\t\t\t\t separator_pos)) {\n+\t\t\t\t\trecognized_prefix = 1;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t} else\n+\t\t\tnon_trailer_lines++;\n+continue_outer_loop:\n+\t\t;\n \t}\n \n-\treturn only_spaces ? count : 0;\n+\treturn count;\n }\n \n /* Get the index of the end of the trailers */\n@@ -805,6 +862,12 @@ static int process_input_file(FILE *outfile,\n \t\t\tadd_trailer_item(head,\n \t\t\t\t\t strbuf_detach(&tok, NULL),\n \t\t\t\t\t strbuf_detach(&val, NULL));\n+\t\t} else {\n+\t\t\tstrbuf_addbuf(&val, lines[i]);\n+\t\t\tstrbuf_strip_suffix(&val, \"\\n\");\n+\t\t\tadd_trailer_item(head,\n+\t\t\t\t\t NULL,\n+\t\t\t\t\t strbuf_detach(&val, NULL));\n \t\t}\n \t}\n \n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"304526","messageId":"8aa456a551bf5cf44cea5ea7e33f32e190c45658.1476998988.git.jonathantanmy@google.com","threadId":"44343","inReplyTo":"cover.1476998988.git.jonathantanmy@google.com","subject":"[PATCH v4 5/8] trailer: clarify failure modes in parse_trailer","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-20T21:39:50Z","receivedAt":"2016-10-20T21:40:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The parse_trailer function has a few modes of operation, all depending\non whether the separator is present in its input, and if yes, the\nseparator's position. Some of these modes are failure modes, and these\nfailure modes are handled differently depending on whether the trailer\nline was sourced from a file or from a command-line argument.\n\nExtract a function to find the separator, allowing the invokers of\nparse_trailer to determine how to handle the failure modes instead of\nmaking parse_trailer do it.\n\nSigned-off-by: Jonathan Tan \n---\n trailer.c | 70 +++++++++++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 48 insertions(+), 22 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 99018f8..137a3fb 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -543,29 +543,40 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le\n \treturn item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;\n }\n \n-static int parse_trailer(struct strbuf *tok, struct strbuf *val,\n-\t\t\t const struct conf_info **conf, const char *trailer)\n+/*\n+ * Return the location of the first separator or '=' in line, or -1 if either a\n+ * newline or the null terminator is reached first.\n+ */\n+static int find_separator(const char *line)\n+{\n+\tconst char *c;\n+\tfor (c = line; ; c++) {\n+\t\tif (!*c || *c == '\\n')\n+\t\t\treturn -1;\n+\t\tif (*c == '=' || strchr(separators, *c))\n+\t\t\treturn c - line;\n+\t}\n+}\n+\n+/*\n+ * Obtain the token, value, and conf from the given trailer.\n+ *\n+ * separator_pos must not be 0, since the token cannot be an empty string.\n+ *\n+ * If separator_pos is -1, interpret the whole trailer as a token.\n+ */\n+static void parse_trailer(struct strbuf *tok, struct strbuf *val,\n+\t\t\t const struct conf_info **conf, const char *trailer,\n+\t\t\t int separator_pos)\n {\n-\tsize_t len;\n-\tstruct strbuf seps = STRBUF_INIT;\n \tstruct arg_item *item;\n \tint tok_len;\n \tstruct list_head *pos;\n \n-\tstrbuf_addstr(&seps, separators);\n-\tstrbuf_addch(&seps, '=');\n-\tlen = strcspn(trailer, seps.buf);\n-\tstrbuf_release(&seps);\n-\tif (len == 0) {\n-\t\tint l = strlen(trailer);\n-\t\twhile (l > 0 && isspace(trailer[l - 1]))\n-\t\t\tl--;\n-\t\treturn error(_(\"empty trailer token in trailer '%.*s'\"), l, trailer);\n-\t}\n-\tif (len < strlen(trailer)) {\n-\t\tstrbuf_add(tok, trailer, len);\n+\tif (separator_pos != -1) {\n+\t\tstrbuf_add(tok, trailer, separator_pos);\n \t\tstrbuf_trim(tok);\n-\t\tstrbuf_addstr(val, trailer + len + 1);\n+\t\tstrbuf_addstr(val, trailer + separator_pos + 1);\n \t\tstrbuf_trim(val);\n \t} else {\n \t\tstrbuf_addstr(tok, trailer);\n@@ -587,8 +598,6 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,\n \t\t\tbreak;\n \t\t}\n \t}\n-\n-\treturn 0;\n }\n \n static void add_trailer_item(struct list_head *head, char *tok, char *val)\n@@ -631,11 +640,22 @@ static void process_command_line_args(struct list_head *arg_head,\n \n \t/* Add an arg item for each trailer on the command line */\n \tfor_each_string_list_item(tr, trailers) {\n-\t\tif (!parse_trailer(&tok, &val, &conf, tr->string))\n+\t\tint separator_pos = find_separator(tr->string);\n+\t\tif (separator_pos == 0) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_addstr(&sb, tr->string);\n+\t\t\tstrbuf_trim(&sb);\n+\t\t\terror(_(\"empty trailer token in trailer '%.*s'\"),\n+\t\t\t (int) sb.len, sb.buf);\n+\t\t\tstrbuf_release(&sb);\n+\t\t} else {\n+\t\t\tparse_trailer(&tok, &val, &conf, tr->string,\n+\t\t\t\t separator_pos);\n \t\t\tadd_arg_item(arg_head,\n \t\t\t\t strbuf_detach(&tok, NULL),\n \t\t\t\t strbuf_detach(&val, NULL),\n \t\t\t\t conf);\n+\t\t}\n \t}\n }\n \n@@ -775,11 +795,17 @@ static int process_input_file(FILE *outfile,\n \n \t/* Parse trailer lines */\n \tfor (i = trailer_start; i < trailer_end; i++) {\n-\t\tif (lines[i]->buf[0] != comment_line_char &&\n-\t\t !parse_trailer(&tok, &val, NULL, lines[i]->buf))\n+\t\tint separator_pos;\n+\t\tif (lines[i]->buf[0] == comment_line_char)\n+\t\t\tcontinue;\n+\t\tseparator_pos = find_separator(lines[i]->buf);\n+\t\tif (separator_pos >= 1) {\n+\t\t\tparse_trailer(&tok, &val, NULL, lines[i]->buf,\n+\t\t\t\t separator_pos);\n \t\t\tadd_trailer_item(head,\n \t\t\t\t\t strbuf_detach(&tok, NULL),\n \t\t\t\t\t strbuf_detach(&val, NULL));\n+\t\t}\n \t}\n \n \treturn trailer_end;\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"304527","messageId":"e6d7454d3cc040810a3eaefd61a7e071e58dce6d.1476998988.git.jonathantanmy@google.com","threadId":"44343","inReplyTo":"cover.1476998988.git.jonathantanmy@google.com","subject":"[PATCH v4 8/8] trailer: support values folded to multiple lines","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-20T21:39:53Z","receivedAt":"2016-10-20T21:40:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, interpret-trailers requires that a trailer be only on 1 line.\nFor example:\n\na: first line\n second line\n\nwould be interpreted as one trailer line followed by one non-trailer line.\n\nMake interpret-trailers support RFC 822-style folding, treating those\nlines as one logical trailer.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/git-interpret-trailers.txt | 7 +-\n t/t7513-interpret-trailers.sh | 169 +++++++++++++++++++++++++++++++\n trailer.c | 43 ++++++--\n 3 files changed, 210 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt\nindex 4966b5b..e99bda6 100644\n--- a/Documentation/git-interpret-trailers.txt\n+++ b/Documentation/git-interpret-trailers.txt\n@@ -57,11 +57,12 @@ minus signs start the patch part of the message.\n \n When reading trailers, there can be whitespaces after the\n token, the separator and the value. There can also be whitespaces\n-inside the token and the value.\n+inside the token and the value. The value may be split over multiple lines with\n+each subsequent line starting with whitespace, like the \"folding\" in RFC 822.\n \n Note that 'trailers' do not follow and are not intended to follow many\n-rules for RFC 822 headers. For example they do not follow the line\n-folding rules, the encoding rules and probably many other rules.\n+rules for RFC 822 headers. For example they do not follow\n+the encoding rules and probably many other rules.\n \n OPTIONS\n -------\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 3d94b3a..4dd1d7c 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -256,6 +256,175 @@ test_expect_success 'line with leading whitespace is not trailer' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'multiline field treated as one trailer for 25% check' '\n+\tq_to_tab >patch <<-\\EOF &&\n+\n+\t\tSigned-off-by: a \n+\t\tname: value on\n+\t\tQmultiple lines\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\tEOF\n+\tq_to_tab >expected <<-\\EOF &&\n+\n+\t\tSigned-off-by: a \n+\t\tname: value on\n+\t\tQmultiple lines\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\t\tthis is not a trailer\n+\t\tname: value\n+\tEOF\n+\tgit interpret-trailers --trailer \"name: value\" patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'multiline field treated as atomic for placement' '\n+\tq_to_tab >patch <<-\\EOF &&\n+\n+\t\tanother: trailer\n+\t\tname: value on\n+\t\tQmultiple lines\n+\t\tanother: trailer\n+\tEOF\n+\tq_to_tab >expected <<-\\EOF &&\n+\n+\t\tanother: trailer\n+\t\tname: value on\n+\t\tQmultiple lines\n+\t\tname: value\n+\t\tanother: trailer\n+\tEOF\n+\ttest_config trailer.name.where after &&\n+\tgit interpret-trailers --trailer \"name: value\" patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'multiline field treated as atomic for replacement' '\n+\tq_to_tab >patch <<-\\EOF &&\n+\n+\t\tanother: trailer\n+\t\tname: value on\n+\t\tQmultiple lines\n+\t\tanother: trailer\n+\tEOF\n+\tq_to_tab >expected <<-\\EOF &&\n+\n+\t\tanother: trailer\n+\t\tanother: trailer\n+\t\tname: value\n+\tEOF\n+\ttest_config trailer.name.ifexists replace &&\n+\tgit interpret-trailers --trailer \"name: value\" patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'multiline field treated as atomic for difference check' '\n+\tq_to_tab >patch <<-\\EOF &&\n+\n+\t\tanother: trailer\n+\t\tname: first line\n+\t\tQsecond line\n+\t\tanother: trailer\n+\tEOF\n+\ttest_config trailer.name.ifexists addIfDifferent &&\n+\n+\tq_to_tab >trailer <<-\\EOF &&\n+\t\tname: first line\n+\t\tQsecond line\n+\tEOF\n+\tq_to_tab >expected <<-\\EOF &&\n+\n+\t\tanother: trailer\n+\t\tname: first line\n+\t\tQsecond line\n+\t\tanother: trailer\n+\tEOF\n+\tgit interpret-trailers --trailer \"$(cat trailer)\" patch >actual &&\n+\ttest_cmp expected actual &&\n+\n+\tq_to_tab >trailer <<-\\EOF &&\n+\t\tname: first line\n+\t\tQQQQQsecond line\n+\tEOF\n+\tq_to_tab >expected <<-\\EOF &&\n+\n+\t\tanother: trailer\n+\t\tname: first line\n+\t\tQsecond line\n+\t\tanother: trailer\n+\t\tname: first line\n+\t\tQQQQQsecond line\n+\tEOF\n+\tgit interpret-trailers --trailer \"$(cat trailer)\" patch >actual &&\n+\ttest_cmp expected actual &&\n+\n+\tq_to_tab >trailer <<-\\EOF &&\n+\t\tname: first line *DIFFERENT*\n+\t\tQsecond line\n+\tEOF\n+\tq_to_tab >expected <<-\\EOF &&\n+\n+\t\tanother: trailer\n+\t\tname: first line\n+\t\tQsecond line\n+\t\tanother: trailer\n+\t\tname: first line *DIFFERENT*\n+\t\tQsecond line\n+\tEOF\n+\tgit interpret-trailers --trailer \"$(cat trailer)\" patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'multiline field treated as atomic for neighbor check' '\n+\tq_to_tab >patch <<-\\EOF &&\n+\n+\t\tanother: trailer\n+\t\tname: first line\n+\t\tQsecond line\n+\t\tanother: trailer\n+\tEOF\n+\ttest_config trailer.name.where after &&\n+\ttest_config trailer.name.ifexists addIfDifferentNeighbor &&\n+\n+\tq_to_tab >trailer <<-\\EOF &&\n+\t\tname: first line\n+\t\tQsecond line\n+\tEOF\n+\tq_to_tab >expected <<-\\EOF &&\n+\n+\t\tanother: trailer\n+\t\tname: first line\n+\t\tQsecond line\n+\t\tanother: trailer\n+\tEOF\n+\tgit interpret-trailers --trailer \"$(cat trailer)\" patch >actual &&\n+\ttest_cmp expected actual &&\n+\n+\tq_to_tab >trailer <<-\\EOF &&\n+\t\tname: first line\n+\t\tQQQQQsecond line\n+\tEOF\n+\tq_to_tab >expected <<-\\EOF &&\n+\n+\t\tanother: trailer\n+\t\tname: first line\n+\t\tQsecond line\n+\t\tname: first line\n+\t\tQQQQQsecond line\n+\t\tanother: trailer\n+\tEOF\n+\tgit interpret-trailers --trailer \"$(cat trailer)\" patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'with config setup' '\n \tgit config trailer.ack.key \"Acked-by: \" &&\n \tcat >expected <<-\\EOF &&\ndiff --git a/trailer.c b/trailer.c\nindex 3ef5576..306c387 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -622,12 +622,14 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val,\n \t}\n }\n \n-static void add_trailer_item(struct list_head *head, char *tok, char *val)\n+static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,\n+\t\t\t\t\t char *val)\n {\n \tstruct trailer_item *new = xcalloc(sizeof(*new), 1);\n \tnew->token = tok;\n \tnew->value = val;\n \tlist_add_tail(&new->list, head);\n+\treturn new;\n }\n \n static void add_arg_item(struct list_head *arg_head, char *tok, char *val,\n@@ -727,6 +729,14 @@ static int find_trailer_start(struct strbuf **lines, int count)\n {\n \tint start, end_of_title, only_spaces = 1;\n \tint recognized_prefix = 0, trailer_lines = 0, non_trailer_lines = 0;\n+\t/*\n+\t * Number of possible continuation lines encountered. This will be\n+\t * reset to 0 if we encounter a trailer (since those lines are to be\n+\t * considered continuations of that trailer), and added to\n+\t * non_trailer_lines if we encounter a non-trailer (since those lines\n+\t * are to be considered non-trailers).\n+\t */\n+\tint possible_continuation_lines = 0;\n \n \t/* The first paragraph is the title and cannot be trailers */\n \tfor (start = 0; start < count; start++) {\n@@ -747,11 +757,15 @@ static int find_trailer_start(struct strbuf **lines, int count)\n \t\tconst char **p;\n \t\tint separator_pos;\n \n-\t\tif (lines[start]->buf[0] == comment_line_char)\n+\t\tif (lines[start]->buf[0] == comment_line_char) {\n+\t\t\tnon_trailer_lines += possible_continuation_lines;\n+\t\t\tpossible_continuation_lines = 0;\n \t\t\tcontinue;\n+\t\t}\n \t\tif (contains_only_spaces(lines[start]->buf)) {\n \t\t\tif (only_spaces)\n \t\t\t\tcontinue;\n+\t\t\tnon_trailer_lines += possible_continuation_lines;\n \t\t\tif (recognized_prefix &&\n \t\t\t trailer_lines * 3 >= non_trailer_lines)\n \t\t\t\treturn start + 1;\n@@ -764,6 +778,7 @@ static int find_trailer_start(struct strbuf **lines, int count)\n \t\tfor (p = git_generated_prefixes; *p; p++) {\n \t\t\tif (starts_with(lines[start]->buf, *p)) {\n \t\t\t\ttrailer_lines++;\n+\t\t\t\tpossible_continuation_lines = 0;\n \t\t\t\trecognized_prefix = 1;\n \t\t\t\tgoto continue_outer_loop;\n \t\t\t}\n@@ -774,6 +789,7 @@ static int find_trailer_start(struct strbuf **lines, int count)\n \t\t\tstruct list_head *pos;\n \n \t\t\ttrailer_lines++;\n+\t\t\tpossible_continuation_lines = 0;\n \t\t\tif (recognized_prefix)\n \t\t\t\tcontinue;\n \t\t\tlist_for_each(pos, &conf_head) {\n@@ -785,8 +801,13 @@ static int find_trailer_start(struct strbuf **lines, int count)\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t}\n-\t\t} else\n+\t\t} else if (isspace(lines[start]->buf[0]))\n+\t\t\tpossible_continuation_lines++;\n+\t\telse {\n \t\t\tnon_trailer_lines++;\n+\t\t\tnon_trailer_lines += possible_continuation_lines;\n+\t\t\tpossible_continuation_lines = 0;\n+\t\t}\n continue_outer_loop:\n \t\t;\n \t}\n@@ -835,6 +856,7 @@ static int process_input_file(FILE *outfile,\n \tint patch_start, trailer_start, trailer_end, i;\n \tstruct strbuf tok = STRBUF_INIT;\n \tstruct strbuf val = STRBUF_INIT;\n+\tstruct trailer_item *last = NULL;\n \n \t/* Get the line count */\n \twhile (lines[count])\n@@ -855,19 +877,28 @@ static int process_input_file(FILE *outfile,\n \t\tint separator_pos;\n \t\tif (lines[i]->buf[0] == comment_line_char)\n \t\t\tcontinue;\n+\t\tif (last && isspace(lines[i]->buf[0])) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_addf(&sb, \"%s\\n%s\", last->value, lines[i]->buf);\n+\t\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n+\t\t\tfree(last->value);\n+\t\t\tlast->value = strbuf_detach(&sb, NULL);\n+\t\t\tcontinue;\n+\t\t}\n \t\tseparator_pos = find_separator(lines[i]->buf);\n \t\tif (separator_pos >= 1) {\n \t\t\tparse_trailer(&tok, &val, NULL, lines[i]->buf,\n \t\t\t\t separator_pos);\n-\t\t\tadd_trailer_item(head,\n-\t\t\t\t\t strbuf_detach(&tok, NULL),\n-\t\t\t\t\t strbuf_detach(&val, NULL));\n+\t\t\tlast = add_trailer_item(head,\n+\t\t\t\t\t\tstrbuf_detach(&tok, NULL),\n+\t\t\t\t\t\tstrbuf_detach(&val, NULL));\n \t\t} else {\n \t\t\tstrbuf_addbuf(&val, lines[i]);\n \t\t\tstrbuf_strip_suffix(&val, \"\\n\");\n \t\t\tadd_trailer_item(head,\n \t\t\t\t\t NULL,\n \t\t\t\t\t strbuf_detach(&val, NULL));\n+\t\t\tlast = NULL;\n \t\t}\n \t}\n \n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"304528","messageId":"6d5608bb3dadc9b767a0c48216418931f4602f72.1476998988.git.jonathantanmy@google.com","threadId":"44343","inReplyTo":"cover.1476998988.git.jonathantanmy@google.com","subject":"[PATCH v4 4/8] trailer: make args have their own struct","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-20T21:39:49Z","receivedAt":"2016-10-20T21:40:42Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Improve type safety by making arguments (whether from configuration or\nfrom the command line) have their own \"struct arg_item\" type, separate\nfrom the \"struct trailer_item\" type used to represent the trailers in\nthe buffer being manipulated.\n\nThis change also prepares \"struct trailer_item\" to be further\ndifferentiated from \"struct arg_item\" in future patches.\n\nSigned-off-by: Jonathan Tan \n---\n trailer.c | 135 +++++++++++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 85 insertions(+), 50 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex ae3972a..99018f8 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -29,6 +29,12 @@ struct trailer_item {\n \tstruct list_head list;\n \tchar *token;\n \tchar *value;\n+};\n+\n+struct arg_item {\n+\tstruct list_head list;\n+\tchar *token;\n+\tchar *value;\n \tstruct conf_info conf;\n };\n \n@@ -62,7 +68,7 @@ static size_t token_len_without_separator(const char *token, size_t len)\n \treturn len;\n }\n \n-static int same_token(struct trailer_item *a, struct trailer_item *b)\n+static int same_token(struct trailer_item *a, struct arg_item *b)\n {\n \tsize_t a_len = token_len_without_separator(a->token, strlen(a->token));\n \tsize_t b_len = token_len_without_separator(b->token, strlen(b->token));\n@@ -71,12 +77,12 @@ static int same_token(struct trailer_item *a, struct trailer_item *b)\n \treturn !strncasecmp(a->token, b->token, min_len);\n }\n \n-static int same_value(struct trailer_item *a, struct trailer_item *b)\n+static int same_value(struct trailer_item *a, struct arg_item *b)\n {\n \treturn !strcasecmp(a->value, b->value);\n }\n \n-static int same_trailer(struct trailer_item *a, struct trailer_item *b)\n+static int same_trailer(struct trailer_item *a, struct arg_item *b)\n {\n \treturn same_token(a, b) && same_value(a, b);\n }\n@@ -98,6 +104,13 @@ static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *\n \n static void free_trailer_item(struct trailer_item *item)\n {\n+\tfree(item->token);\n+\tfree(item->value);\n+\tfree(item);\n+}\n+\n+static void free_arg_item(struct arg_item *item)\n+{\n \tfree(item->conf.name);\n \tfree(item->conf.key);\n \tfree(item->conf.command);\n@@ -137,17 +150,29 @@ static void print_all(FILE *outfile, struct list_head *head, int trim_empty)\n \t}\n }\n \n+static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)\n+{\n+\tstruct trailer_item *new = xcalloc(sizeof(*new), 1);\n+\tnew->token = arg_tok->token;\n+\tnew->value = arg_tok->value;\n+\targ_tok->token = arg_tok->value = NULL;\n+\tfree_arg_item(arg_tok);\n+\treturn new;\n+}\n+\n static void add_arg_to_input_list(struct trailer_item *on_tok,\n-\t\t\t\t struct trailer_item *arg_tok)\n+\t\t\t\t struct arg_item *arg_tok)\n {\n-\tif (after_or_end(arg_tok->conf.where))\n-\t\tlist_add(&arg_tok->list, &on_tok->list);\n+\tint aoe = after_or_end(arg_tok->conf.where);\n+\tstruct trailer_item *to_add = trailer_from_arg(arg_tok);\n+\tif (aoe)\n+\t\tlist_add(&to_add->list, &on_tok->list);\n \telse\n-\t\tlist_add_tail(&arg_tok->list, &on_tok->list);\n+\t\tlist_add_tail(&to_add->list, &on_tok->list);\n }\n \n static int check_if_different(struct trailer_item *in_tok,\n-\t\t\t struct trailer_item *arg_tok,\n+\t\t\t struct arg_item *arg_tok,\n \t\t\t int check_all,\n \t\t\t struct list_head *head)\n {\n@@ -200,7 +225,7 @@ static char *apply_command(const char *command, const char *arg)\n \treturn result;\n }\n \n-static void apply_item_command(struct trailer_item *in_tok, struct trailer_item *arg_tok)\n+static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok)\n {\n \tif (arg_tok->conf.command) {\n \t\tconst char *arg;\n@@ -218,13 +243,13 @@ static void apply_item_command(struct trailer_item *in_tok, struct trailer_item\n }\n \n static void apply_arg_if_exists(struct trailer_item *in_tok,\n-\t\t\t\tstruct trailer_item *arg_tok,\n+\t\t\t\tstruct arg_item *arg_tok,\n \t\t\t\tstruct trailer_item *on_tok,\n \t\t\t\tstruct list_head *head)\n {\n \tswitch (arg_tok->conf.if_exists) {\n \tcase EXISTS_DO_NOTHING:\n-\t\tfree_trailer_item(arg_tok);\n+\t\tfree_arg_item(arg_tok);\n \t\tbreak;\n \tcase EXISTS_REPLACE:\n \t\tapply_item_command(in_tok, arg_tok);\n@@ -241,39 +266,41 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,\n \t\tif (check_if_different(in_tok, arg_tok, 1, head))\n \t\t\tadd_arg_to_input_list(on_tok, arg_tok);\n \t\telse\n-\t\t\tfree_trailer_item(arg_tok);\n+\t\t\tfree_arg_item(arg_tok);\n \t\tbreak;\n \tcase EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:\n \t\tapply_item_command(in_tok, arg_tok);\n \t\tif (check_if_different(on_tok, arg_tok, 0, head))\n \t\t\tadd_arg_to_input_list(on_tok, arg_tok);\n \t\telse\n-\t\t\tfree_trailer_item(arg_tok);\n+\t\t\tfree_arg_item(arg_tok);\n \t\tbreak;\n \t}\n }\n \n static void apply_arg_if_missing(struct list_head *head,\n-\t\t\t\t struct trailer_item *arg_tok)\n+\t\t\t\t struct arg_item *arg_tok)\n {\n \tenum action_where where;\n+\tstruct trailer_item *to_add;\n \n \tswitch (arg_tok->conf.if_missing) {\n \tcase MISSING_DO_NOTHING:\n-\t\tfree_trailer_item(arg_tok);\n+\t\tfree_arg_item(arg_tok);\n \t\tbreak;\n \tcase MISSING_ADD:\n \t\twhere = arg_tok->conf.where;\n \t\tapply_item_command(NULL, arg_tok);\n+\t\tto_add = trailer_from_arg(arg_tok);\n \t\tif (after_or_end(where))\n-\t\t\tlist_add_tail(&arg_tok->list, head);\n+\t\t\tlist_add_tail(&to_add->list, head);\n \t\telse\n-\t\t\tlist_add(&arg_tok->list, head);\n+\t\t\tlist_add(&to_add->list, head);\n \t}\n }\n \n static int find_same_and_apply_arg(struct list_head *head,\n-\t\t\t\t struct trailer_item *arg_tok)\n+\t\t\t\t struct arg_item *arg_tok)\n {\n \tstruct list_head *pos;\n \tstruct trailer_item *in_tok;\n@@ -306,11 +333,11 @@ static void process_trailers_lists(struct list_head *head,\n \t\t\t\t struct list_head *arg_head)\n {\n \tstruct list_head *pos, *p;\n-\tstruct trailer_item *arg_tok;\n+\tstruct arg_item *arg_tok;\n \n \tlist_for_each_safe(pos, p, arg_head) {\n \t\tint applied = 0;\n-\t\targ_tok = list_entry(pos, struct trailer_item, list);\n+\t\targ_tok = list_entry(pos, struct arg_item, list);\n \n \t\tlist_del(pos);\n \n@@ -375,20 +402,20 @@ static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)\n \t\tdst->command = xstrdup(src->command);\n }\n \n-static struct trailer_item *get_conf_item(const char *name)\n+static struct arg_item *get_conf_item(const char *name)\n {\n \tstruct list_head *pos;\n-\tstruct trailer_item *item;\n+\tstruct arg_item *item;\n \n \t/* Look up item with same name */\n \tlist_for_each(pos, &conf_head) {\n-\t\titem = list_entry(pos, struct trailer_item, list);\n+\t\titem = list_entry(pos, struct arg_item, list);\n \t\tif (!strcasecmp(item->conf.name, name))\n \t\t\treturn item;\n \t}\n \n \t/* Item does not already exists, create it */\n-\titem = xcalloc(sizeof(struct trailer_item), 1);\n+\titem = xcalloc(sizeof(*item), 1);\n \tduplicate_conf(&item->conf, &default_conf_info);\n \titem->conf.name = xstrdup(name);\n \n@@ -442,7 +469,7 @@ static int git_trailer_default_config(const char *conf_key, const char *value, v\n static int git_trailer_config(const char *conf_key, const char *value, void *cb)\n {\n \tconst char *trailer_item, *variable_name;\n-\tstruct trailer_item *item;\n+\tstruct arg_item *item;\n \tstruct conf_info *conf;\n \tchar *name = NULL;\n \tenum trailer_info_type type;\n@@ -500,7 +527,7 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)\n \treturn 0;\n }\n \n-static const char *token_from_item(struct trailer_item *item, char *tok)\n+static const char *token_from_item(struct arg_item *item, char *tok)\n {\n \tif (item->conf.key)\n \t\treturn item->conf.key;\n@@ -509,7 +536,7 @@ static const char *token_from_item(struct trailer_item *item, char *tok)\n \treturn item->conf.name;\n }\n \n-static int token_matches_item(const char *tok, struct trailer_item *item, int tok_len)\n+static int token_matches_item(const char *tok, struct arg_item *item, int tok_len)\n {\n \tif (!strncasecmp(tok, item->conf.name, tok_len))\n \t\treturn 1;\n@@ -521,7 +548,7 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,\n {\n \tsize_t len;\n \tstruct strbuf seps = STRBUF_INIT;\n-\tstruct trailer_item *item;\n+\tstruct arg_item *item;\n \tint tok_len;\n \tstruct list_head *pos;\n \n@@ -547,12 +574,14 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,\n \n \t/* Lookup if the token matches something in the config */\n \ttok_len = token_len_without_separator(tok->buf, tok->len);\n-\t*conf = &default_conf_info;\n+\tif (conf)\n+\t\t*conf = &default_conf_info;\n \tlist_for_each(pos, &conf_head) {\n-\t\titem = list_entry(pos, struct trailer_item, list);\n+\t\titem = list_entry(pos, struct arg_item, list);\n \t\tif (token_matches_item(tok->buf, item, tok_len)) {\n \t\t\tchar *tok_buf = strbuf_detach(tok, NULL);\n-\t\t\t*conf = &item->conf;\n+\t\t\tif (conf)\n+\t\t\t\t*conf = &item->conf;\n \t\t\tstrbuf_addstr(tok, token_from_item(item, tok_buf));\n \t\t\tfree(tok_buf);\n \t\t\tbreak;\n@@ -562,43 +591,51 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,\n \treturn 0;\n }\n \n-static void add_trailer_item(struct list_head *head, char *tok, char *val,\n-\t\t\t const struct conf_info *conf)\n+static void add_trailer_item(struct list_head *head, char *tok, char *val)\n {\n \tstruct trailer_item *new = xcalloc(sizeof(*new), 1);\n \tnew->token = tok;\n \tnew->value = val;\n-\tduplicate_conf(&new->conf, conf);\n \tlist_add_tail(&new->list, head);\n }\n \n+static void add_arg_item(struct list_head *arg_head, char *tok, char *val,\n+\t\t\t const struct conf_info *conf)\n+{\n+\tstruct arg_item *new = xcalloc(sizeof(*new), 1);\n+\tnew->token = tok;\n+\tnew->value = val;\n+\tduplicate_conf(&new->conf, conf);\n+\tlist_add_tail(&new->list, arg_head);\n+}\n+\n static void process_command_line_args(struct list_head *arg_head, \n \t\t\t\t struct string_list *trailers)\n {\n \tstruct string_list_item *tr;\n-\tstruct trailer_item *item;\n+\tstruct arg_item *item;\n \tstruct strbuf tok = STRBUF_INIT;\n \tstruct strbuf val = STRBUF_INIT;\n \tconst struct conf_info *conf;\n \tstruct list_head *pos;\n \n-\t/* Add a trailer item for each configured trailer with a command */\n+\t/* Add an arg item for each configured trailer with a command */\n \tlist_for_each(pos, &conf_head) {\n-\t\titem = list_entry(pos, struct trailer_item, list);\n+\t\titem = list_entry(pos, struct arg_item, list);\n \t\tif (item->conf.command)\n-\t\t\tadd_trailer_item(arg_head,\n-\t\t\t\t\t xstrdup(token_from_item(item, NULL)),\n-\t\t\t\t\t xstrdup(\"\"),\n-\t\t\t\t\t &item->conf);\n+\t\t\tadd_arg_item(arg_head,\n+\t\t\t\t xstrdup(token_from_item(item, NULL)),\n+\t\t\t\t xstrdup(\"\"),\n+\t\t\t\t &item->conf);\n \t}\n \n-\t/* Add a trailer item for each trailer on the command line */\n+\t/* Add an arg item for each trailer on the command line */\n \tfor_each_string_list_item(tr, trailers) {\n \t\tif (!parse_trailer(&tok, &val, &conf, tr->string))\n-\t\t\tadd_trailer_item(arg_head,\n-\t\t\t\t\t strbuf_detach(&tok, NULL),\n-\t\t\t\t\t strbuf_detach(&val, NULL),\n-\t\t\t\t\t conf);\n+\t\t\tadd_arg_item(arg_head,\n+\t\t\t\t strbuf_detach(&tok, NULL),\n+\t\t\t\t strbuf_detach(&val, NULL),\n+\t\t\t\t conf);\n \t}\n }\n \n@@ -721,7 +758,6 @@ static int process_input_file(FILE *outfile,\n \tint patch_start, trailer_start, trailer_end, i;\n \tstruct strbuf tok = STRBUF_INIT;\n \tstruct strbuf val = STRBUF_INIT;\n-\tconst struct conf_info *conf;\n \n \t/* Get the line count */\n \twhile (lines[count])\n@@ -740,11 +776,10 @@ static int process_input_file(FILE *outfile,\n \t/* Parse trailer lines */\n \tfor (i = trailer_start; i < trailer_end; i++) {\n \t\tif (lines[i]->buf[0] != comment_line_char &&\n-\t\t !parse_trailer(&tok, &val, &conf, lines[i]->buf))\n+\t\t !parse_trailer(&tok, &val, NULL, lines[i]->buf))\n \t\t\tadd_trailer_item(head,\n \t\t\t\t\t strbuf_detach(&tok, NULL),\n-\t\t\t\t\t strbuf_detach(&val, NULL),\n-\t\t\t\t\t conf);\n+\t\t\t\t\t strbuf_detach(&val, NULL));\n \t}\n \n \treturn trailer_end;\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"304529","messageId":"37072b5f568747cdde8a54ce4a2f7d8759359c55.1476998988.git.jonathantanmy@google.com","threadId":"44343","inReplyTo":"cover.1476998988.git.jonathantanmy@google.com","subject":"[PATCH v4 7/8] trailer: forbid leading whitespace in trailers","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-20T21:39:52Z","receivedAt":"2016-10-20T21:40:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, interpret-trailers allows leading whitespace in trailer\nlines. This leads to false positives, especially for quoted lines or\nbullet lists.\n\nForbid leading whitespace in trailers.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/git-interpret-trailers.txt | 2 +-\n t/t7513-interpret-trailers.sh | 15 +++++++++++++++\n trailer.c | 2 +-\n 3 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt\nindex cf4c5ea..4966b5b 100644\n--- a/Documentation/git-interpret-trailers.txt\n+++ b/Documentation/git-interpret-trailers.txt\n@@ -55,7 +55,7 @@ The group must either be at the end of the message or be the last\n non-whitespace lines before a line that starts with '---'. Such three\n minus signs start the patch part of the message.\n \n-When reading trailers, there can be whitespaces before and after the\n+When reading trailers, there can be whitespaces after the\n token, the separator and the value. There can also be whitespaces\n inside the token and the value.\n \ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 003e90f..3d94b3a 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -241,6 +241,21 @@ test_expect_success 'with non-trailer lines only' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'line with leading whitespace is not trailer' '\n+\tq_to_tab >patch <<-\\EOF &&\n+\n+\t\tQtoken: value\n+\tEOF\n+\tq_to_tab >expected <<-\\EOF &&\n+\n+\t\tQtoken: value\n+\n+\t\ttoken: value\n+\tEOF\n+\tgit interpret-trailers --trailer \"token: value\" patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'with config setup' '\n \tgit config trailer.ack.key \"Acked-by: \" &&\n \tcat >expected <<-\\EOF &&\ndiff --git a/trailer.c b/trailer.c\nindex da15b79..3ef5576 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -770,7 +770,7 @@ static int find_trailer_start(struct strbuf **lines, int count)\n \t\t}\n \n \t\tseparator_pos = find_separator(lines[start]->buf);\n-\t\tif (separator_pos >= 1) {\n+\t\tif (separator_pos >= 1 && !isspace(lines[start]->buf[0])) {\n \t\t\tstruct list_head *pos;\n \n \t\t\ttrailer_lines++;\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"304530","messageId":"c821f75f4000fc3e224b23633cce983c33b6d4d6.1476998988.git.jonathantanmy@google.com","threadId":"44343","inReplyTo":"cover.1476998988.git.jonathantanmy@google.com","subject":"[PATCH v4 3/8] trailer: streamline trailer item create and add","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-20T21:39:48Z","receivedAt":"2016-10-20T21:40:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, creation and addition (to a list) of trailer items are spread\nacross multiple functions. Streamline this by only having 2 functions:\none to parse the user-supplied string, and one to add the parsed\ninformation to a list.\n\nSigned-off-by: Jonathan Tan \n---\n trailer.c | 130 +++++++++++++++++++++++++++++---------------------------------\n 1 file changed, 60 insertions(+), 70 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 4e85aae..ae3972a 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -500,10 +500,31 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)\n \treturn 0;\n }\n \n-static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char *trailer)\n+static const char *token_from_item(struct trailer_item *item, char *tok)\n+{\n+\tif (item->conf.key)\n+\t\treturn item->conf.key;\n+\tif (tok)\n+\t\treturn tok;\n+\treturn item->conf.name;\n+}\n+\n+static int token_matches_item(const char *tok, struct trailer_item *item, int tok_len)\n+{\n+\tif (!strncasecmp(tok, item->conf.name, tok_len))\n+\t\treturn 1;\n+\treturn item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;\n+}\n+\n+static int parse_trailer(struct strbuf *tok, struct strbuf *val,\n+\t\t\t const struct conf_info **conf, const char *trailer)\n {\n \tsize_t len;\n \tstruct strbuf seps = STRBUF_INIT;\n+\tstruct trailer_item *item;\n+\tint tok_len;\n+\tstruct list_head *pos;\n+\n \tstrbuf_addstr(&seps, separators);\n \tstrbuf_addch(&seps, '=');\n \tlen = strcspn(trailer, seps.buf);\n@@ -523,74 +544,31 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char *tra\n \t\tstrbuf_addstr(tok, trailer);\n \t\tstrbuf_trim(tok);\n \t}\n-\treturn 0;\n-}\n-\n-static const char *token_from_item(struct trailer_item *item, char *tok)\n-{\n-\tif (item->conf.key)\n-\t\treturn item->conf.key;\n-\tif (tok)\n-\t\treturn tok;\n-\treturn item->conf.name;\n-}\n-\n-static struct trailer_item *new_trailer_item(struct trailer_item *conf_item,\n-\t\t\t\t\t char *tok, char *val)\n-{\n-\tstruct trailer_item *new = xcalloc(sizeof(*new), 1);\n-\tnew->value = val ? val : xstrdup(\"\");\n-\n-\tif (conf_item) {\n-\t\tduplicate_conf(&new->conf, &conf_item->conf);\n-\t\tnew->token = xstrdup(token_from_item(conf_item, tok));\n-\t\tfree(tok);\n-\t} else {\n-\t\tduplicate_conf(&new->conf, &default_conf_info);\n-\t\tnew->token = tok;\n-\t}\n-\n-\treturn new;\n-}\n-\n-static int token_matches_item(const char *tok, struct trailer_item *item, int tok_len)\n-{\n-\tif (!strncasecmp(tok, item->conf.name, tok_len))\n-\t\treturn 1;\n-\treturn item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;\n-}\n-\n-static struct trailer_item *create_trailer_item(const char *string)\n-{\n-\tstruct strbuf tok = STRBUF_INIT;\n-\tstruct strbuf val = STRBUF_INIT;\n-\tstruct trailer_item *item;\n-\tint tok_len;\n-\tstruct list_head *pos;\n-\n-\tif (parse_trailer(&tok, &val, string))\n-\t\treturn NULL;\n-\n-\ttok_len = token_len_without_separator(tok.buf, tok.len);\n \n \t/* Lookup if the token matches something in the config */\n+\ttok_len = token_len_without_separator(tok->buf, tok->len);\n+\t*conf = &default_conf_info;\n \tlist_for_each(pos, &conf_head) {\n \t\titem = list_entry(pos, struct trailer_item, list);\n-\t\tif (token_matches_item(tok.buf, item, tok_len))\n-\t\t\treturn new_trailer_item(item,\n-\t\t\t\t\t\tstrbuf_detach(&tok, NULL),\n-\t\t\t\t\t\tstrbuf_detach(&val, NULL));\n+\t\tif (token_matches_item(tok->buf, item, tok_len)) {\n+\t\t\tchar *tok_buf = strbuf_detach(tok, NULL);\n+\t\t\t*conf = &item->conf;\n+\t\t\tstrbuf_addstr(tok, token_from_item(item, tok_buf));\n+\t\t\tfree(tok_buf);\n+\t\t\tbreak;\n+\t\t}\n \t}\n \n-\treturn new_trailer_item(NULL,\n-\t\t\t\tstrbuf_detach(&tok, NULL),\n-\t\t\t\tstrbuf_detach(&val, NULL));\n+\treturn 0;\n }\n \n-static void add_trailer_item(struct list_head *head, struct trailer_item *new)\n+static void add_trailer_item(struct list_head *head, char *tok, char *val,\n+\t\t\t const struct conf_info *conf)\n {\n-\tif (!new)\n-\t\treturn;\n+\tstruct trailer_item *new = xcalloc(sizeof(*new), 1);\n+\tnew->token = tok;\n+\tnew->value = val;\n+\tduplicate_conf(&new->conf, conf);\n \tlist_add_tail(&new->list, head);\n }\n \n@@ -599,21 +577,28 @@ static void process_command_line_args(struct list_head *arg_head,\n {\n \tstruct string_list_item *tr;\n \tstruct trailer_item *item;\n+\tstruct strbuf tok = STRBUF_INIT;\n+\tstruct strbuf val = STRBUF_INIT;\n+\tconst struct conf_info *conf;\n \tstruct list_head *pos;\n \n \t/* Add a trailer item for each configured trailer with a command */\n \tlist_for_each(pos, &conf_head) {\n \t\titem = list_entry(pos, struct trailer_item, list);\n-\t\tif (item->conf.command) {\n-\t\t\tstruct trailer_item *new = new_trailer_item(item, NULL, NULL);\n-\t\t\tadd_trailer_item(arg_head, new);\n-\t\t}\n+\t\tif (item->conf.command)\n+\t\t\tadd_trailer_item(arg_head,\n+\t\t\t\t\t xstrdup(token_from_item(item, NULL)),\n+\t\t\t\t\t xstrdup(\"\"),\n+\t\t\t\t\t &item->conf);\n \t}\n \n \t/* Add a trailer item for each trailer on the command line */\n \tfor_each_string_list_item(tr, trailers) {\n-\t\tstruct trailer_item *new = create_trailer_item(tr->string);\n-\t\tadd_trailer_item(arg_head, new);\n+\t\tif (!parse_trailer(&tok, &val, &conf, tr->string))\n+\t\t\tadd_trailer_item(arg_head,\n+\t\t\t\t\t strbuf_detach(&tok, NULL),\n+\t\t\t\t\t strbuf_detach(&val, NULL),\n+\t\t\t\t\t conf);\n \t}\n }\n \n@@ -734,6 +719,9 @@ static int process_input_file(FILE *outfile,\n {\n \tint count = 0;\n \tint patch_start, trailer_start, trailer_end, i;\n+\tstruct strbuf tok = STRBUF_INIT;\n+\tstruct strbuf val = STRBUF_INIT;\n+\tconst struct conf_info *conf;\n \n \t/* Get the line count */\n \twhile (lines[count])\n@@ -751,10 +739,12 @@ static int process_input_file(FILE *outfile,\n \n \t/* Parse trailer lines */\n \tfor (i = trailer_start; i < trailer_end; i++) {\n-\t\tif (lines[i]->buf[0] != comment_line_char) {\n-\t\t\tstruct trailer_item *new = create_trailer_item(lines[i]->buf);\n-\t\t\tadd_trailer_item(head, new);\n-\t\t}\n+\t\tif (lines[i]->buf[0] != comment_line_char &&\n+\t\t !parse_trailer(&tok, &val, &conf, lines[i]->buf))\n+\t\t\tadd_trailer_item(head,\n+\t\t\t\t\t strbuf_detach(&tok, NULL),\n+\t\t\t\t\t strbuf_detach(&val, NULL),\n+\t\t\t\t\t conf);\n \t}\n \n \treturn trailer_end;\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"304536","messageId":"CAGZ79kYi=+JUqmFyR+Xapwy2bLHtmCX7ZFxd9bGkNHS5tjEdOw@mail.gmail.com","threadId":"44343","inReplyTo":"8aa456a551bf5cf44cea5ea7e33f32e190c45658.1476998988.git.jonathantanmy@google.com","subject":"Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-20T22:07:02Z","receivedAt":"2016-10-20T22:07:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 20, 2016 at 2:39 PM, Jonathan Tan wrote:\n> The parse_trailer function has a few modes of operation, all depending\n> on whether the separator is present in its input, and if yes, the\n> separator's position. Some of these modes are failure modes, and these\n> failure modes are handled differently depending on whether the trailer\n> line was sourced from a file or from a command-line argument.\n>\n> Extract a function to find the separator, allowing the invokers of\n> parse_trailer to determine how to handle the failure modes instead of\n> making parse_trailer do it.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> trailer.c | 70 +++++++++++++++++++++++++++++++++++++++++++--------------------\n> 1 file changed, 48 insertions(+), 22 deletions(-)\n>\n> diff --git a/trailer.c b/trailer.c\n> index 99018f8..137a3fb 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -543,29 +543,40 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le\n> return item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;\n> }\n>\n> -static int parse_trailer(struct strbuf *tok, struct strbuf *val,\n> - const struct conf_info **conf, const char *trailer)\n> +/*\n> + * Return the location of the first separator or '=' in line, or -1 if either a\n> + * newline or the null terminator is reached first.\n> + */\n> +static int find_separator(const char *line)\n> +{\n> + const char *c;\n> + for (c = line; ; c++) {\n> + if (!*c || *c == '\\n')\n> + return -1;\n> + if (*c == '=' || strchr(separators, *c))\n> + return c - line;\n> + }\n\nI was about to suggest this function can be simplified and maybe\neven inlined by the use of strspn or strcspn, but I think manual\nprocessing of the string is fine, too, as it would not really be shorter.\n"},{"id":"304538","messageId":"xmqqvawmekhk.fsf@gitster.mtv.corp.google.com","threadId":"44343","inReplyTo":"CAGZ79kYi=+JUqmFyR+Xapwy2bLHtmCX7ZFxd9bGkNHS5tjEdOw@mail.gmail.com","subject":"Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-20T22:14:31Z","receivedAt":"2016-10-20T22:14:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> +static int find_separator(const char *line)\n>> +{\n>> + const char *c;\n>> + for (c = line; ; c++) {\n>> + if (!*c || *c == '\\n')\n>> + return -1;\n>> + if (*c == '=' || strchr(separators, *c))\n>> + return c - line;\n>> + }\n>\n> I was about to suggest this function can be simplified and maybe\n> even inlined by the use of strspn or strcspn, but I think manual\n> processing of the string is fine, too, as it would not really be shorter.\n\nHmm, I fear that iterating over a line one-byte-at-a-time and\nrunning strchr(separators, *c) on it for each byte has a performance\nimplication over running a single call to strcspn(line, separators).\n"},{"id":"304539","messageId":"22e4b88b-5273-99c9-bcb3-0748c17a7229@google.com","threadId":"44343","inReplyTo":"xmqqvawmekhk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-20T22:40:09Z","receivedAt":"2016-10-20T22:40:18Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 10/20/2016 03:14 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>>> +static int find_separator(const char *line)\n>>> +{\n>>> + const char *c;\n>>> + for (c = line; ; c++) {\n>>> + if (!*c || *c == '\\n')\n>>> + return -1;\n>>> + if (*c == '=' || strchr(separators, *c))\n>>> + return c - line;\n>>> + }\n>>\n>> I was about to suggest this function can be simplified and maybe\n>> even inlined by the use of strspn or strcspn, but I think manual\n>> processing of the string is fine, too, as it would not really be shorter.\n>\n> Hmm, I fear that iterating over a line one-byte-at-a-time and\n> running strchr(separators, *c) on it for each byte has a performance\n> implication over running a single call to strcspn(line, separators).\n\nIf we do that, there is also the necessity of creating a string that \ncombines the separators and '=' (I guess '\\n' is not necessary now, \nsince all the lines are null terminated). I'm OK either way.\n\n(We could cache that string, although I would think that if we did that, \nwe might as well write the loop manually, like in this patch.)\n"},{"id":"304540","messageId":"xmqqr37aej26.fsf@gitster.mtv.corp.google.com","threadId":"44343","inReplyTo":"22e4b88b-5273-99c9-bcb3-0748c17a7229@google.com","subject":"Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-20T22:45:21Z","receivedAt":"2016-10-20T22:45:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> If we do that, there is also the necessity of creating a string that\n> combines the separators and '=' (I guess '\\n' is not necessary now,\n> since all the lines are null terminated). I'm OK either way.\n>\n> (We could cache that string, although I would think that if we did\n> that, we might as well write the loop manually, like in this patch.)\n\nI wonder if there is a legit reason to look for '=' in the first\nplace. \"Signed-off-by= Jonathan Tan \" does not look\nlike a valid trailer line to me.\n\nIsn't that a remnant of lazy coding in the original that tried to\nshare a single parser for contents and command line options or\nsomething?\n"},{"id":"304541","messageId":"d7d2c001-a411-424c-7da0-4ef615da9a54@google.com","threadId":"44343","inReplyTo":"22e4b88b-5273-99c9-bcb3-0748c17a7229@google.com","subject":"Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-20T22:45:34Z","receivedAt":"2016-10-20T22:45:42Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 10/20/2016 03:40 PM, Jonathan Tan wrote:\n> On 10/20/2016 03:14 PM, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>\n>>>> +static int find_separator(const char *line)\n>>>> +{\n>>>> + const char *c;\n>>>> + for (c = line; ; c++) {\n>>>> + if (!*c || *c == '\\n')\n>>>> + return -1;\n>>>> + if (*c == '=' || strchr(separators, *c))\n>>>> + return c - line;\n>>>> + }\n>>>\n>>> I was about to suggest this function can be simplified and maybe\n>>> even inlined by the use of strspn or strcspn, but I think manual\n>>> processing of the string is fine, too, as it would not really be\n>>> shorter.\n>>\n>> Hmm, I fear that iterating over a line one-byte-at-a-time and\n>> running strchr(separators, *c) on it for each byte has a performance\n>> implication over running a single call to strcspn(line, separators).\n>\n> If we do that, there is also the necessity of creating a string that\n> combines the separators and '=' (I guess '\\n' is not necessary now,\n> since all the lines are null terminated). I'm OK either way.\n>\n> (We could cache that string, although I would think that if we did that,\n> we might as well write the loop manually, like in this patch.)\n\nActually I guess we could generate the separators_and_equal string \nwhenever we obtain new separators from the config. I'll do this in the \nnext reroll.\n"},{"id":"304542","messageId":"5656444f-14ba-b58c-4ad6-e72cc15a13cb@google.com","threadId":"44343","inReplyTo":"xmqqr37aej26.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-20T22:49:33Z","receivedAt":"2016-10-20T22:49:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 10/20/2016 03:45 PM, Junio C Hamano wrote:\n> Jonathan Tan writes:\n>\n>> If we do that, there is also the necessity of creating a string that\n>> combines the separators and '=' (I guess '\\n' is not necessary now,\n>> since all the lines are null terminated). I'm OK either way.\n>>\n>> (We could cache that string, although I would think that if we did\n>> that, we might as well write the loop manually, like in this patch.)\n>\n> I wonder if there is a legit reason to look for '=' in the first\n> place. \"Signed-off-by= Jonathan Tan \" does not look\n> like a valid trailer line to me.\n>\n> Isn't that a remnant of lazy coding in the original that tried to\n> share a single parser for contents and command line options or\n> something?\n\nThat is true - I think we can take the allowed separators as an argument \n(meaning that we can have different behavior for file parsing and \ncommand line parsing), and since we already have that string, we can use \nstrcspn. I'll try this out in the next reroll.\n"},{"id":"304549","messageId":"xmqqeg3aeeqe.fsf@gitster.mtv.corp.google.com","threadId":"44343","inReplyTo":"5656444f-14ba-b58c-4ad6-e72cc15a13cb@google.com","subject":"Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-21T00:18:49Z","receivedAt":"2016-10-21T00:19:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> That is true - I think we can take the allowed separators as an\n> argument (meaning that we can have different behavior for file parsing\n> and command line parsing), and since we already have that string, we\n> can use strcspn. I'll try this out in the next reroll.\n\nSounds good. Thanks.\n\n\nThe following is a tangent that I think this topic should ignore,\nbut we may want to revisit it sometime later.\n\nI think the design of the \"separator\" mechanism is one of the things\nwe botched in the current system. If I recall correctly, this was\nintroduced to allow people write \"Bug# 538\" in the trailer section\nand get it recognised as a valid trailer.\n\nWhen I say that this was a botched design, I do not mean to say that\nwe should have instead forced projects to adopt \"Bug: 538\" format.\nThe design is botched because the users' wish to allow \"Bug# 538\" or\n\"Bug #538\" by setting separators to \":#\" from the built-in \":\" does\nnot mean that they would want \"Signed-off-by# me \" to\nbe accepted.\n\nIf I were guiding a topic that introduce this feature from scratch\ntoday, I would probably suggest a pattern based approach, e.g. a\nbuilt-in \"[-A-Za-z0-9]+:\" [*1*] may be the default prefix that is\nused to recognize the beginning of a trailer, and a user or a\nproject that wants \"Bug #538\" would be allowed to add an additional\npattern, e.g. \"Bug *#\", that recognises a custom trailer line that\nis used by the project.\n\n\n[Footnote]\n\n*1* Or more lenient \"[A-Za-z0-9][- A-Za-z0-9]*:\".\n"},{"id":"304676","messageId":"CAP8UFD1eVgWvMsYvb-aBaM9Soox3PsuzC7_-fS3K4+mbNe6=vA@mail.gmail.com","threadId":"44343","inReplyTo":"xmqqr37aej26.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-22T09:29:10Z","receivedAt":"2016-10-22T09:29:19Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Oct 21, 2016 at 12:45 AM, Junio C Hamano wrote:\n> Jonathan Tan writes:\n>\n>> If we do that, there is also the necessity of creating a string that\n>> combines the separators and '=' (I guess '\\n' is not necessary now,\n>> since all the lines are null terminated). I'm OK either way.\n>>\n>> (We could cache that string, although I would think that if we did\n>> that, we might as well write the loop manually, like in this patch.)\n>\n> I wonder if there is a legit reason to look for '=' in the first\n> place. \"Signed-off-by= Jonathan Tan \" does not look\n> like a valid trailer line to me.\n>\n> Isn't that a remnant of lazy coding in the original that tried to\n> share a single parser for contents and command line options or\n> something?\n\nI think the relevant discussion was this one:\n\nhttps://public-inbox.org/git/20140915.080429.1739849931027469667.chriscool@tuxfamily.org/\n"},{"id":"304677","messageId":"CAP8UFD0XvNx6Brio9muDhOj5+dr=HRbhK_Lmr+k4LUPKrAVC9Q@mail.gmail.com","threadId":"44343","inReplyTo":"xmqqeg3aeeqe.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-22T13:07:25Z","receivedAt":"2016-10-22T13:07:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Oct 21, 2016 at 2:18 AM, Junio C Hamano wrote:\n> Jonathan Tan writes:\n>\n>> That is true - I think we can take the allowed separators as an\n>> argument (meaning that we can have different behavior for file parsing\n>> and command line parsing), and since we already have that string, we\n>> can use strcspn. I'll try this out in the next reroll.\n>\n> Sounds good. Thanks.\n>\n>\n> The following is a tangent that I think this topic should ignore,\n> but we may want to revisit it sometime later.\n>\n> I think the design of the \"separator\" mechanism is one of the things\n> we botched in the current system. If I recall correctly, this was\n> introduced to allow people write \"Bug# 538\" in the trailer section\n> and get it recognised as a valid trailer.\n>\n> When I say that this was a botched design, I do not mean to say that\n> we should have instead forced projects to adopt \"Bug: 538\" format.\n> The design is botched because the users' wish to allow \"Bug# 538\" or\n> \"Bug #538\" by setting separators to \":#\" from the built-in \":\" does\n> not mean that they would want \"Signed-off-by# me \" to\n> be accepted.\n>\n> If I were guiding a topic that introduce this feature from scratch\n> today, I would probably suggest a pattern based approach, e.g. a\n> built-in \"[-A-Za-z0-9]+:\" [*1*] may be the default prefix that is\n> used to recognize the beginning of a trailer, and a user or a\n> project that wants \"Bug #538\" would be allowed to add an additional\n> pattern, e.g. \"Bug *#\", that recognises a custom trailer line that\n> is used by the project.\n\nWhen we designed the separator mechanism, we had the following discussions:\n\nhttps://public-inbox.org/git/xmqqa9a1d6xn.fsf@gitster.dls.corp.google.com/\nhttps://public-inbox.org/git/xmqqmwcuzyqx.fsf@gitster.dls.corp.google.com/\n\nThey made me think that you were against too much flexibility, so I\nremoved functionality that allowed to put separators into the \".key\"\nconfig options, and now you are saying that we botched the thing and\nthat you would like more flexibility of this kind back.\n\nAnyway I think it is still possible to add back such kind of\nfunctionality in a backward compatible way for example by adding\n\".extendedKey\" config options.\n"},{"id":"304678","messageId":"xmqqd1is9x0s.fsf@gitster.mtv.corp.google.com","threadId":"44343","inReplyTo":"CAP8UFD0XvNx6Brio9muDhOj5+dr=HRbhK_Lmr+k4LUPKrAVC9Q@mail.gmail.com","subject":"Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-22T16:19:31Z","receivedAt":"2016-10-22T16:19:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Fri, Oct 21, 2016 at 2:18 AM, Junio C Hamano wrote:\n>>\n>> If I were guiding a topic that introduce this feature from scratch\n>> today, I would probably suggest a pattern based approach, e.g. a\n>> built-in \"[-A-Za-z0-9]+:\" [*1*] may be the default prefix that is\n>> used to recognize the beginning of a trailer, and a user or a\n>> project that wants \"Bug #538\" would be allowed to add an additional\n>> pattern, e.g. \"Bug *#\", that recognises a custom trailer line that\n>> is used by the project.\n>\n> When we designed the separator mechanism, we had the following discussions:\n>\n> https://public-inbox.org/git/xmqqa9a1d6xn.fsf@gitster.dls.corp.google.com/\n> https://public-inbox.org/git/xmqqmwcuzyqx.fsf@gitster.dls.corp.google.com/\n>\n> They made me think that you were against too much flexibility, so I\n> removed functionality that allowed to put separators into the \".key\"\n> config options, and now you are saying that we botched the thing and\n> that you would like more flexibility of this kind back.\n\nCorrect. Pay attention to the fact that I said _we_ botched.\n\nIf an initial design made by a topic author is crappy, that may be\nauthor's botch. Once a topic goes through a review cycle by getting\nreviewed, rerolled, re-reviewed, ... to the point that those\ninvolved accept the result, and we later realize that it was not\ngood, the botch no longer is author's alone. If it is shipped as\npart of a release, then it is not just the authors and the reviewers\nbut everybody. We collectively stopped at a place that was not\nideal and share the blame ;-).\n\n> Anyway I think it is still possible to add back such kind of\n> functionality in a backward compatible way for example by adding\n> \".extendedKey\" config options.\n\nYup, or with trailer.keyPattern that is multi-values, or with any\nnumber of alternatives.\n"}]}