{"thread":{"id":"44397","subject":"[PATCH 0/4] Make other git commands use trailer layout","startedAt":"2016-10-29T00:05:21Z","lastAt":"2016-11-02T17:29:57Z","messageCount":30,"participants":["Jonathan Tan","Junio C Hamano","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"305142","messageId":"cover.1477698917.git.jonathantanmy@google.com","threadId":"44397","inReplyTo":null,"subject":"[PATCH 0/4] Make other git commands use trailer layout","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-29T00:05:07Z","receivedAt":"2016-10-29T00:05:21Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is built off jt/trailer-with-cruft (commit 60ef86a).\n\nThis patch set makes \"commit -s\", \"cherry-pick -x\", and\n\"format-patch --signoff\" use the new trailer definition implemented in\njt/trailer-with-cruft, with some refactoring along the way. With this\npatch set, the aforementioned commands would now handle trailers like\nthose described in [1].\n\n[1] <84f28caa-2e4b-1231-1a76-3b7e765c0b61@google.com>\n\nJonathan Tan (4):\n commit: make ignore_non_trailer take buf/len\n trailer: avoid unnecessary splitting on lines\n trailer: have function to describe trailer layout\n sequencer: use trailer's trailer layout\n\n builtin/commit.c | 2 +-\n commit.c | 22 ++--\n commit.h | 2 +-\n sequencer.c | 75 +++---------\n t/t3511-cherry-pick-x.sh | 16 ++-\n t/t4014-format-patch.sh | 40 +++++--\n t/t7501-commit.sh | 36 ++++++\n trailer.c | 295 ++++++++++++++++++++++++++++-------------------\n trailer.h | 25 ++++\n 9 files changed, 313 insertions(+), 200 deletions(-)\n\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305143","messageId":"38ab31ef362d58e600860aa9c6111fd7625b46f1.1477698917.git.jonathantanmy@google.com","threadId":"44397","inReplyTo":"cover.1477698917.git.jonathantanmy@google.com","subject":"[PATCH 1/4] commit: make ignore_non_trailer take buf/len","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-29T00:05:08Z","receivedAt":"2016-10-29T00:05:27Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Make ignore_non_trailer take a buf/len pair instead of struct strbuf.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/commit.c | 2 +-\n commit.c | 22 +++++++++++-----------\n commit.h | 2 +-\n trailer.c | 2 +-\n 4 files changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 1cba3b7..c26b939 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -790,7 +790,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tstrbuf_stripspace(&sb, 0);\n \n \tif (signoff)\n-\t\tappend_signoff(&sb, ignore_non_trailer(&sb), 0);\n+\t\tappend_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0);\n \n \tif (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)\n \t\tdie_errno(_(\"could not write commit template\"));\ndiff --git a/commit.c b/commit.c\nindex 856fd4a..f70835a 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1649,7 +1649,7 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n }\n \n /*\n- * Inspect sb and determine the true \"end\" of the log message, in\n+ * Inspect the given string and determine the true \"end\" of the log message, in\n * order to find where to put a new Signed-off-by: line. Ignored are\n * trailing comment lines and blank lines, and also the traditional\n * \"Conflicts:\" block that is not commented out, so that we can use\n@@ -1659,37 +1659,37 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n * Returns the number of bytes from the tail to ignore, to be fed as\n * the second parameter to append_signoff().\n */\n-int ignore_non_trailer(struct strbuf *sb)\n+int ignore_non_trailer(const char *buf, size_t len)\n {\n \tint boc = 0;\n \tint bol = 0;\n \tint in_old_conflicts_block = 0;\n \n-\twhile (bol < sb->len) {\n-\t\tchar *next_line;\n+\twhile (bol < len) {\n+\t\tconst char *next_line;\n \n-\t\tif (!(next_line = memchr(sb->buf + bol, '\\n', sb->len - bol)))\n-\t\t\tnext_line = sb->buf + sb->len;\n+\t\tif (!(next_line = memchr(buf + bol, '\\n', len - bol)))\n+\t\t\tnext_line = buf + len;\n \t\telse\n \t\t\tnext_line++;\n \n-\t\tif (sb->buf[bol] == comment_line_char || sb->buf[bol] == '\\n') {\n+\t\tif (buf[bol] == comment_line_char || buf[bol] == '\\n') {\n \t\t\t/* is this the first of the run of comments? */\n \t\t\tif (!boc)\n \t\t\t\tboc = bol;\n \t\t\t/* otherwise, it is just continuing */\n-\t\t} else if (starts_with(sb->buf + bol, \"Conflicts:\\n\")) {\n+\t\t} else if (starts_with(buf + bol, \"Conflicts:\\n\")) {\n \t\t\tin_old_conflicts_block = 1;\n \t\t\tif (!boc)\n \t\t\t\tboc = bol;\n-\t\t} else if (in_old_conflicts_block && sb->buf[bol] == '\\t') {\n+\t\t} else if (in_old_conflicts_block && buf[bol] == '\\t') {\n \t\t\t; /* a pathname in the conflicts block */\n \t\t} else if (boc) {\n \t\t\t/* the previous was not trailing comment */\n \t\t\tboc = 0;\n \t\t\tin_old_conflicts_block = 0;\n \t\t}\n-\t\tbol = next_line - sb->buf;\n+\t\tbol = next_line - buf;\n \t}\n-\treturn boc ? sb->len - boc : 0;\n+\treturn boc ? len - boc : 0;\n }\ndiff --git a/commit.h b/commit.h\nindex afd14f3..9c12abb 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -355,7 +355,7 @@ extern const char *find_commit_header(const char *msg, const char *key,\n \t\t\t\t size_t *out_len);\n \n /* Find the end of the log message, the right place for a new trailer. */\n-extern int ignore_non_trailer(struct strbuf *sb);\n+extern int ignore_non_trailer(const char *buf, size_t len);\n \n typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,\n \t\t\t\t void *cb_data);\ndiff --git a/trailer.c b/trailer.c\nindex d19a92c..6d8375b 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -828,7 +828,7 @@ static int find_trailer_end(struct strbuf **lines, int patch_start)\n \n \tfor (i = 0; i < patch_start; i++)\n \t\tstrbuf_addbuf(&sb, lines[i]);\n-\tignore_bytes = ignore_non_trailer(&sb);\n+\tignore_bytes = ignore_non_trailer(sb.buf, sb.len);\n \tstrbuf_release(&sb);\n \tfor (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)\n \t\tignore_bytes -= lines[i]->len;\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305144","messageId":"db609e13740415ac4e5e357493661347b0f681f7.1477698917.git.jonathantanmy@google.com","threadId":"44397","inReplyTo":"cover.1477698917.git.jonathantanmy@google.com","subject":"[PATCH 2/4] trailer: avoid unnecessary splitting on lines","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-29T00:05:09Z","receivedAt":"2016-10-29T00:05:30Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"trailer.c currently splits lines while processing a buffer (and also\nrejoins lines when needing to invoke ignore_non_trailer).\n\nAvoid such line splitting, except when generating the strings\ncorresponding to trailers (for ease of use by clients - a subsequent\npatch will allow other components to obtain the layout of a trailer\nblock in a buffer, including the trailers themselves). The main purpose\nof this is to make it easy to return pointers into the original buffer\n(for a subsequent patch), but this also significantly reduces the number\nof memory allocations required.\n\nSigned-off-by: Jonathan Tan \n---\n trailer.c | 215 +++++++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 116 insertions(+), 99 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 6d8375b..d4d9e10 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -102,12 +102,12 @@ static int same_trailer(struct trailer_item *a, struct arg_item *b)\n \treturn same_token(a, b) && same_value(a, b);\n }\n \n-static inline int contains_only_spaces(const char *str)\n+static inline int is_blank_line(const char *str)\n {\n \tconst char *s = str;\n-\twhile (*s && isspace(*s))\n+\twhile (*s && *s != '\\n' && isspace(*s))\n \t\ts++;\n-\treturn !*s;\n+\treturn !*s || *s == '\\n';\n }\n \n static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *b)\n@@ -566,13 +566,18 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le\n }\n \n /*\n- * Return the location of the first separator in line, or -1 if there is no\n- * separator.\n+ * Return the location of the first separator in the given line, or -1 if there\n+ * is no separator.\n+ *\n+ * The interests parameter must contain the acceptable separators and may\n+ * contain '\\n'. If interests contains '\\n', the input line is considered to\n+ * end at the first newline encountered. Otherwise, the input line is\n+ * considered to end at its null terminator.\n */\n-static int find_separator(const char *line, const char *separators)\n+static int find_separator(const char *line, const char *interests)\n {\n-\tint loc = strcspn(line, separators);\n-\tif (!line[loc])\n+\tint loc = strcspn(line, interests);\n+\tif (!line[loc] || line[loc] == '\\n')\n \t\treturn -1;\n \treturn loc;\n }\n@@ -688,51 +693,71 @@ static void process_command_line_args(struct list_head *arg_head,\n \tfree(cl_separators);\n }\n \n-static struct strbuf **read_input_file(const char *file)\n+static void read_input_file(struct strbuf *sb, const char *file)\n {\n-\tstruct strbuf **lines;\n-\tstruct strbuf sb = STRBUF_INIT;\n-\n \tif (file) {\n-\t\tif (strbuf_read_file(&sb, file, 0) < 0)\n+\t\tif (strbuf_read_file(sb, file, 0) < 0)\n \t\t\tdie_errno(_(\"could not read input file '%s'\"), file);\n \t} else {\n-\t\tif (strbuf_read(&sb, fileno(stdin), 0) < 0)\n+\t\tif (strbuf_read(sb, fileno(stdin), 0) < 0)\n \t\t\tdie_errno(_(\"could not read from stdin\"));\n \t}\n+}\n \n-\tlines = strbuf_split(&sb, '\\n');\n+static const char *next_line(const char *str)\n+{\n+\tconst char *nl = strchrnul(str, '\\n');\n+\treturn nl + !!*nl;\n+}\n \n-\tstrbuf_release(&sb);\n+/*\n+ * Return the position of the start of the last line. If len is 0, return -1.\n+ */\n+static int last_line(const char *buf, size_t len)\n+{\n+\tint i;\n+\tif (len == 0)\n+\t\treturn -1;\n+\tif (len == 1)\n+\t\treturn 0;\n+\t/*\n+\t * Skip the last character (in addition to the null terminator),\n+\t * because if the last character is a newline, it is considered as part\n+\t * of the last line anyway.\n+\t */\n+\ti = len - 2;\n \n-\treturn lines;\n+\tfor (; i >= 0; i--) {\n+\t\tif (buf[i] == '\\n')\n+\t\t\treturn i + 1;\n+\t}\n+\treturn 0;\n }\n \n /*\n- * Return the (0 based) index of the start of the patch or the line\n- * count if there is no patch in the message.\n+ * Return the position of the start of the patch or the length of str if there\n+ * is no patch in the message.\n */\n-static int find_patch_start(struct strbuf **lines, int count)\n+static int find_patch_start(const char *str)\n {\n-\tint i;\n+\tconst char *s;\n \n-\t/* Get the start of the patch part if any */\n-\tfor (i = 0; i < count; i++) {\n-\t\tif (starts_with(lines[i]->buf, \"---\"))\n-\t\t\treturn i;\n+\tfor (s = str; *s; s = next_line(s)) {\n+\t\tif (starts_with(s, \"---\"))\n+\t\t\treturn s - str;\n \t}\n \n-\treturn count;\n+\treturn s - str;\n }\n \n /*\n- * Return the (0 based) index of the first trailer line or count if\n- * there are no trailers. Trailers are searched only in the lines from\n- * index (count - 1) down to index 0.\n+ * Return the position of the first trailer line or len if there are no\n+ * trailers.\n */\n-static int find_trailer_start(struct strbuf **lines, int count)\n+static int find_trailer_start(const char *buf, size_t len)\n {\n-\tint start, end_of_title, only_spaces = 1;\n+\tconst char *s;\n+\tint title_end, l, 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@@ -742,15 +767,18 @@ static int find_trailer_start(struct strbuf **lines, int count)\n \t * are to be considered non-trailers).\n \t */\n \tint possible_continuation_lines = 0;\n+\tint ret;\n+\n+\tchar *sep_nl = xstrfmt(\"%s\\n\", separators);\n \n \t/* The first paragraph is the title and cannot be trailers */\n-\tfor (start = 0; start < count; start++) {\n-\t\tif (lines[start]->buf[0] == comment_line_char)\n+\tfor (s = buf; s < buf + len; s = next_line(s)) {\n+\t\tif (s[0] == comment_line_char)\n \t\t\tcontinue;\n-\t\tif (contains_only_spaces(lines[start]->buf))\n+\t\tif (is_blank_line(s))\n \t\t\tbreak;\n \t}\n-\tend_of_title = start;\n+\ttitle_end = s - buf;\n \n \t/*\n \t * Get the start of the trailers by looking starting from the end for a\n@@ -758,30 +786,33 @@ static int find_trailer_start(struct strbuf **lines, int count)\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+\tfor (l = last_line(buf, len); l >= title_end; l = last_line(buf, l)) {\n+\t\tconst char *bol = buf + l;\n \t\tconst char **p;\n \t\tint separator_pos;\n \n-\t\tif (lines[start]->buf[0] == comment_line_char) {\n+\t\tif (bol[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\tif (is_blank_line(bol)) {\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-\t\t\tif (trailer_lines && !non_trailer_lines)\n-\t\t\t\treturn start + 1;\n-\t\t\treturn count;\n+\t\t\t\tret = next_line(bol) - buf;\n+\t\t\telse if (trailer_lines && !non_trailer_lines)\n+\t\t\t\tret = next_line(bol) - buf;\n+\t\t\telse\n+\t\t\t\tret = len;\n+\t\t\tgoto cleanup;\n \t\t}\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\tif (starts_with(bol, *p)) {\n \t\t\t\ttrailer_lines++;\n \t\t\t\tpossible_continuation_lines = 0;\n \t\t\t\trecognized_prefix = 1;\n@@ -789,8 +820,8 @@ static int find_trailer_start(struct strbuf **lines, int count)\n \t\t\t}\n \t\t}\n \n-\t\tseparator_pos = find_separator(lines[start]->buf, separators);\n-\t\tif (separator_pos >= 1 && !isspace(lines[start]->buf[0])) {\n+\t\tseparator_pos = find_separator(bol, sep_nl);\n+\t\tif (separator_pos >= 1 && !isspace(bol[0])) {\n \t\t\tstruct list_head *pos;\n \n \t\t\ttrailer_lines++;\n@@ -800,13 +831,13 @@ static int find_trailer_start(struct strbuf **lines, int count)\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\tif (token_matches_item(bol, 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 if (isspace(lines[start]->buf[0]))\n+\t\t} else if (isspace(bol[0]))\n \t\t\tpossible_continuation_lines++;\n \t\telse {\n \t\t\tnon_trailer_lines++;\n@@ -817,88 +848,73 @@ static int find_trailer_start(struct strbuf **lines, int count)\n \t\t;\n \t}\n \n-\treturn count;\n+\tret = len;\n+cleanup:\n+\tfree(sep_nl);\n+\treturn ret;\n }\n \n-/* Get the index of the end of the trailers */\n-static int find_trailer_end(struct strbuf **lines, int patch_start)\n+/* Return the position of the end of the trailers. */\n+static int find_trailer_end(const char *buf, size_t len)\n {\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tint i, ignore_bytes;\n-\n-\tfor (i = 0; i < patch_start; i++)\n-\t\tstrbuf_addbuf(&sb, lines[i]);\n-\tignore_bytes = ignore_non_trailer(sb.buf, sb.len);\n-\tstrbuf_release(&sb);\n-\tfor (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)\n-\t\tignore_bytes -= lines[i]->len;\n-\n-\treturn i + 1;\n+\treturn len - ignore_non_trailer(buf, len);\n }\n \n-static int has_blank_line_before(struct strbuf **lines, int start)\n+static int ends_with_blank_line(const char *buf, size_t len)\n {\n-\tfor (;start >= 0; start--) {\n-\t\tif (lines[start]->buf[0] == comment_line_char)\n-\t\t\tcontinue;\n-\t\treturn contains_only_spaces(lines[start]->buf);\n-\t}\n-\treturn 0;\n-}\n-\n-static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end)\n-{\n-\tint i;\n-\tfor (i = start; lines[i] && i < end; i++)\n-\t\tfprintf(outfile, \"%s\", lines[i]->buf);\n+\tint ll = last_line(buf, len);\n+\tif (ll < 0)\n+\t\treturn 0;\n+\treturn is_blank_line(buf + ll);\n }\n \n static int process_input_file(FILE *outfile,\n-\t\t\t struct strbuf **lines,\n+\t\t\t const char *str,\n \t\t\t struct list_head *head)\n {\n-\tint count = 0;\n-\tint patch_start, trailer_start, trailer_end, i;\n+\tint patch_start, trailer_start, trailer_end;\n \tstruct strbuf tok = STRBUF_INIT;\n \tstruct strbuf val = STRBUF_INIT;\n \tstruct trailer_item *last = NULL;\n+\tstruct strbuf *trailer, **trailer_lines, **ptr;\n \n-\t/* Get the line count */\n-\twhile (lines[count])\n-\t\tcount++;\n-\n-\tpatch_start = find_patch_start(lines, count);\n-\ttrailer_end = find_trailer_end(lines, patch_start);\n-\ttrailer_start = find_trailer_start(lines, trailer_end);\n+\tpatch_start = find_patch_start(str);\n+\ttrailer_end = find_trailer_end(str, patch_start);\n+\ttrailer_start = find_trailer_start(str, trailer_end);\n \n \t/* Print lines before the trailers as is */\n-\tprint_lines(outfile, lines, 0, trailer_start);\n+\tfwrite(str, 1, trailer_start, outfile);\n \n-\tif (!has_blank_line_before(lines, trailer_start - 1))\n+\tif (!ends_with_blank_line(str, trailer_start))\n \t\tfprintf(outfile, \"\\n\");\n \n \t/* Parse trailer lines */\n-\tfor (i = trailer_start; i < trailer_end; i++) {\n+\ttrailer_lines = strbuf_split_buf(str + trailer_start, \n+\t\t\t\t\t trailer_end - trailer_start,\n+\t\t\t\t\t '\\n',\n+\t\t\t\t\t 0);\n+\tfor (ptr = trailer_lines; *ptr; ptr++) {\n \t\tint separator_pos;\n-\t\tif (lines[i]->buf[0] == comment_line_char)\n+\t\ttrailer = *ptr;\n+\t\tif (trailer->buf[0] == comment_line_char)\n \t\t\tcontinue;\n-\t\tif (last && isspace(lines[i]->buf[0])) {\n+\t\tif (last && isspace(trailer->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_addf(&sb, \"%s\\n%s\", last->value, trailer->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, separators);\n+\t\tseparator_pos = find_separator(trailer->buf, separators);\n \t\tif (separator_pos >= 1) {\n-\t\t\tparse_trailer(&tok, &val, NULL, lines[i]->buf,\n+\t\t\tparse_trailer(&tok, &val, NULL, trailer->buf,\n \t\t\t\t separator_pos);\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_addbuf(&val, trailer);\n \t\t\tstrbuf_strip_suffix(&val, \"\\n\");\n \t\t\tadd_trailer_item(head,\n \t\t\t\t\t NULL,\n@@ -906,6 +922,7 @@ static int process_input_file(FILE *outfile,\n \t\t\tlast = NULL;\n \t\t}\n \t}\n+\tstrbuf_list_free(trailer_lines);\n \n \treturn trailer_end;\n }\n@@ -954,7 +971,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n {\n \tLIST_HEAD(head);\n \tLIST_HEAD(arg_head);\n-\tstruct strbuf **lines;\n+\tstruct strbuf sb = STRBUF_INIT;\n \tint trailer_end;\n \tFILE *outfile = stdout;\n \n@@ -962,13 +979,13 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n \tgit_config(git_trailer_default_config, NULL);\n \tgit_config(git_trailer_config, NULL);\n \n-\tlines = read_input_file(file);\n+\tread_input_file(&sb, file);\n \n \tif (in_place)\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, &head);\n+\ttrailer_end = process_input_file(outfile, sb.buf, &head);\n \n \tprocess_command_line_args(&arg_head, trailers);\n \n@@ -979,11 +996,11 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\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+\tfwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);\n \n \tif (in_place)\n \t\tif (rename_tempfile(&trailers_tempfile, file))\n \t\t\tdie_errno(_(\"could not rename temporary file to %s\"), file);\n \n-\tstrbuf_list_free(lines);\n+\tstrbuf_release(&sb);\n }\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305145","messageId":"d55ff281b88624cdbf5c21a56d817d8d17eff5c0.1477698917.git.jonathantanmy@google.com","threadId":"44397","inReplyTo":"cover.1477698917.git.jonathantanmy@google.com","subject":"[PATCH 3/4] trailer: have function to describe trailer layout","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-29T00:05:10Z","receivedAt":"2016-10-29T00:05:33Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Create a function that, taking a string, describes the position of its\ntrailer block (if available) and the contents thereof, and make trailer\nuse it. This makes it easier for other Git components, in the future, to\ninterpret trailer blocks in the same way as trailer.\n\nIn a subsequent patch, another component will be made to use this.\n\nSigned-off-by: Jonathan Tan \n---\n trailer.c | 120 +++++++++++++++++++++++++++++++++++++++++++-------------------\n trailer.h | 25 +++++++++++++\n 2 files changed, 108 insertions(+), 37 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex d4d9e10..2f5c815 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -46,6 +46,8 @@ static LIST_HEAD(conf_head);\n \n static char *separators = \":\";\n \n+static int configured = 0;\n+\n #define TRAILER_ARG_STRING \"$ARG\"\n \n static const char *git_generated_prefixes[] = {\n@@ -549,6 +551,17 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)\n \treturn 0;\n }\n \n+static void ensure_configured(void)\n+{\n+\tif (configured)\n+\t\treturn;\n+\n+\t/* Default config must be setup first */\n+\tgit_config(git_trailer_default_config, NULL);\n+\tgit_config(git_trailer_config, NULL);\n+\tconfigured = 1;\n+}\n+\n static const char *token_from_item(struct arg_item *item, char *tok)\n {\n \tif (item->conf.key)\n@@ -872,59 +885,43 @@ static int process_input_file(FILE *outfile,\n \t\t\t const char *str,\n \t\t\t struct list_head *head)\n {\n-\tint patch_start, trailer_start, trailer_end;\n+\tstruct trailer_info info;\n \tstruct strbuf tok = STRBUF_INIT;\n \tstruct strbuf val = STRBUF_INIT;\n-\tstruct trailer_item *last = NULL;\n-\tstruct strbuf *trailer, **trailer_lines, **ptr;\n+\tint i;\n \n-\tpatch_start = find_patch_start(str);\n-\ttrailer_end = find_trailer_end(str, patch_start);\n-\ttrailer_start = find_trailer_start(str, trailer_end);\n+\ttrailer_info_get(&info, str);\n \n \t/* Print lines before the trailers as is */\n-\tfwrite(str, 1, trailer_start, outfile);\n+\tfwrite(str, 1, info.trailer_start - str, outfile);\n \n-\tif (!ends_with_blank_line(str, trailer_start))\n+\tif (!info.blank_line_before_trailer)\n \t\tfprintf(outfile, \"\\n\");\n \n-\t/* Parse trailer lines */\n-\ttrailer_lines = strbuf_split_buf(str + trailer_start, \n-\t\t\t\t\t trailer_end - trailer_start,\n-\t\t\t\t\t '\\n',\n-\t\t\t\t\t 0);\n-\tfor (ptr = trailer_lines; *ptr; ptr++) {\n+\tfor (i = 0; i < info.trailer_nr; i++) {\n \t\tint separator_pos;\n-\t\ttrailer = *ptr;\n-\t\tif (trailer->buf[0] == comment_line_char)\n+\t\tchar *trailer = info.trailers[i];\n+\t\tif (trailer[0] == comment_line_char)\n \t\t\tcontinue;\n-\t\tif (last && isspace(trailer->buf[0])) {\n-\t\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\t\tstrbuf_addf(&sb, \"%s\\n%s\", last->value, trailer->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(trailer->buf, separators);\n+\t\tseparator_pos = find_separator(trailer, separators);\n \t\tif (separator_pos >= 1) {\n-\t\t\tparse_trailer(&tok, &val, NULL, trailer->buf,\n-\t\t\t\t separator_pos);\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\tparse_trailer(&tok, &val, NULL, trailer,\n+\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} else {\n-\t\t\tstrbuf_addbuf(&val, trailer);\n+\t\t\tstrbuf_addstr(&val, trailer);\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-\tstrbuf_list_free(trailer_lines);\n \n-\treturn trailer_end;\n+\ttrailer_info_release(&info);\n+\n+\treturn info.trailer_end - str;\n }\n \n static void free_all(struct list_head *head)\n@@ -975,9 +972,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n \tint trailer_end;\n \tFILE *outfile = stdout;\n \n-\t/* Default config must be setup first */\n-\tgit_config(git_trailer_default_config, NULL);\n-\tgit_config(git_trailer_config, NULL);\n+\tensure_configured();\n \n \tread_input_file(&sb, file);\n \n@@ -1004,3 +999,54 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n \n \tstrbuf_release(&sb);\n }\n+\n+void trailer_info_get(struct trailer_info *info, const char *str)\n+{\n+\tint patch_start, trailer_end, trailer_start;\n+\tstruct strbuf **trailer_lines, **ptr;\n+\tchar **trailer_strings = NULL;\n+\tsize_t nr = 0, alloc = 0;\n+\tchar **last = NULL;\n+\n+\tensure_configured();\n+\n+\tpatch_start = find_patch_start(str);\n+\ttrailer_end = find_trailer_end(str, patch_start);\n+\ttrailer_start = find_trailer_start(str, trailer_end);\n+\n+\ttrailer_lines = strbuf_split_buf(str + trailer_start,\n+\t\t\t\t\t trailer_end - trailer_start,\n+\t\t\t\t\t '\\n',\n+\t\t\t\t\t 0);\n+\tfor (ptr = trailer_lines; *ptr; ptr++) {\n+\t\tif (last && isspace((*ptr)->buf[0])) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_attach(&sb, *last, strlen(*last), strlen(*last));\n+\t\t\tstrbuf_addbuf(&sb, *ptr);\n+\t\t\t*last = strbuf_detach(&sb, NULL);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tALLOC_GROW(trailer_strings, nr + 1, alloc);\n+\t\ttrailer_strings[nr] = strbuf_detach(*ptr, NULL);\n+\t\tlast = find_separator(trailer_strings[nr], separators) >= 1\n+\t\t\t? &trailer_strings[nr]\n+\t\t\t: NULL;\n+\t\tnr++;\n+\t}\n+\tstrbuf_list_free(trailer_lines);\n+\n+\tinfo->blank_line_before_trailer = ends_with_blank_line(str,\n+\t\t\t\t\t\t\t trailer_start);\n+\tinfo->trailer_start = str + trailer_start;\n+\tinfo->trailer_end = str + trailer_end;\n+\tinfo->trailers = trailer_strings;\n+\tinfo->trailer_nr = nr;\n+}\n+\n+void trailer_info_release(struct trailer_info *info)\n+{\n+\tint i;\n+\tfor (i = 0; i < info->trailer_nr; i++)\n+\t\tfree(info->trailers[i]);\n+\tfree(info->trailers);\n+}\ndiff --git a/trailer.h b/trailer.h\nindex 36b40b8..65cc5d7 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -1,7 +1,32 @@\n #ifndef TRAILER_H\n #define TRAILER_H\n \n+struct trailer_info {\n+\t/*\n+\t * True if there is a blank line before the location pointed to by\n+\t * trailer_start.\n+\t */\n+\tint blank_line_before_trailer;\n+\n+\t/*\n+\t * Pointers to the start and end of the trailer block found. If there\n+\t * is no trailer block found, these 2 pointers point to the end of the\n+\t * input string.\n+\t */\n+\tconst char *trailer_start, *trailer_end;\n+\n+\t/*\n+\t * Array of trailers found.\n+\t */\n+\tchar **trailers;\n+\tsize_t trailer_nr;\n+};\n+\n void process_trailers(const char *file, int in_place, int trim_empty,\n \t\t struct string_list *trailers);\n \n+void trailer_info_get(struct trailer_info *info, const char *str);\n+\n+void trailer_info_release(struct trailer_info *info);\n+\n #endif /* TRAILER_H */\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305146","messageId":"602ae84920300cdbb439eca8098c5e092ca322f7.1477698917.git.jonathantanmy@google.com","threadId":"44397","inReplyTo":"cover.1477698917.git.jonathantanmy@google.com","subject":"[PATCH 4/4] sequencer: use trailer's trailer layout","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-29T00:05:11Z","receivedAt":"2016-10-29T00:05:37Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Make sequencer use trailer.c's trailer layout definition, as opposed to\nparsing the footer by itself. This makes \"commit -s\", \"cherry-pick -x\",\nand \"format-patch --signoff\" consistent with trailer, allowing\nnon-trailer lines and multiple-line trailers in trailer blocks under\ncertain conditions, and therefore suppressing the extra newline in those\ncases.\n\nConsistency with trailer extends to respecting trailer configs. Tests\nhave been included to show that.\n\nSigned-off-by: Jonathan Tan \n---\n sequencer.c | 75 +++++++++---------------------------------------\n t/t3511-cherry-pick-x.sh | 16 +++++++++--\n t/t4014-format-patch.sh | 40 +++++++++++++++++++++-----\n t/t7501-commit.sh | 36 +++++++++++++++++++++++\n 4 files changed, 96 insertions(+), 71 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex eec8a60..ec0157d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -15,6 +15,7 @@\n #include \"merge-recursive.h\"\n #include \"refs.h\"\n #include \"argv-array.h\"\n+#include \"trailer.h\"\n \n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n \n@@ -26,30 +27,6 @@ static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE)\n static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR)\n static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE)\n \n-static int is_rfc2822_line(const char *buf, int len)\n-{\n-\tint i;\n-\n-\tfor (i = 0; i < len; i++) {\n-\t\tint ch = buf[i];\n-\t\tif (ch == ':')\n-\t\t\treturn 1;\n-\t\tif (!isalnum(ch) && ch != '-')\n-\t\t\tbreak;\n-\t}\n-\n-\treturn 0;\n-}\n-\n-static int is_cherry_picked_from_line(const char *buf, int len)\n-{\n-\t/*\n-\t * We only care that it looks roughly like (cherry picked from ...)\n-\t */\n-\treturn len > strlen(cherry_picked_prefix) + 1 &&\n-\t\tstarts_with(buf, cherry_picked_prefix) && buf[len - 1] == ')';\n-}\n-\n /*\n * Returns 0 for non-conforming footer\n * Returns 1 for conforming footer\n@@ -59,49 +36,25 @@ static int is_cherry_picked_from_line(const char *buf, int len)\n static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \tint ignore_footer)\n {\n-\tchar prev;\n-\tint i, k;\n-\tint len = sb->len - ignore_footer;\n-\tconst char *buf = sb->buf;\n-\tint found_sob = 0;\n-\n-\t/* footer must end with newline */\n-\tif (!len || buf[len - 1] != '\\n')\n-\t\treturn 0;\n+\tstruct trailer_info info;\n+\tint i;\n+\tint found_sob = 0, found_sob_last = 0;\n \n-\tprev = '\\0';\n-\tfor (i = len - 1; i > 0; i--) {\n-\t\tchar ch = buf[i];\n-\t\tif (prev == '\\n' && ch == '\\n') /* paragraph break */\n-\t\t\tbreak;\n-\t\tprev = ch;\n-\t}\n+\ttrailer_info_get(&info, sb->buf);\n \n-\t/* require at least one blank line */\n-\tif (prev != '\\n' || buf[i] != '\\n')\n+\tif (info.trailer_start == info.trailer_end)\n \t\treturn 0;\n \n-\t/* advance to start of last paragraph */\n-\twhile (i < len - 1 && buf[i] == '\\n')\n-\t\ti++;\n-\n-\tfor (; i < len; i = k) {\n-\t\tint found_rfc2822;\n-\n-\t\tfor (k = i; k < len && buf[k] != '\\n'; k++)\n-\t\t\t; /* do nothing */\n-\t\tk++;\n+\tfor (i = 0; i < info.trailer_nr; i++)\n+\t\tif (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {\n+\t\t\tfound_sob = 1;\n+\t\t\tif (i == info.trailer_nr - 1)\n+\t\t\t\tfound_sob_last = 1;\n+\t\t}\n \n-\t\tfound_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);\n-\t\tif (found_rfc2822 && sob &&\n-\t\t !strncmp(buf + i, sob->buf, sob->len))\n-\t\t\tfound_sob = k;\n+\ttrailer_info_release(&info);\n \n-\t\tif (!(found_rfc2822 ||\n-\t\t is_cherry_picked_from_line(buf + i, k - i - 1)))\n-\t\t\treturn 0;\n-\t}\n-\tif (found_sob == i)\n+\tif (found_sob_last)\n \t\treturn 3;\n \tif (found_sob)\n \t\treturn 2;\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex 9cce5ae..bf0a5c9 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -25,9 +25,8 @@ Signed-off-by: B.U. Thor \"\n \n mesg_broken_footer=\"$mesg_no_footer\n \n-The signed-off-by string should begin with the words Signed-off-by followed\n-by a colon and space, and then the signers name and email address. e.g.\n-Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n+This is not recognized as a footer because Myfooter is not a recognized token.\n+Myfooter: A.U. Thor \"\n \n mesg_with_footer_sob=\"$mesg_with_footer\n Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n@@ -112,6 +111,17 @@ test_expect_success 'cherry-pick -s inserts blank line after non-conforming foot\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick -s recognizes trailer config' '\n+\tpristine_detach initial &&\n+\tgit -c \"trailer.Myfooter.ifexists=add\" cherry-pick -s mesg-broken-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_broken_footer\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cherry-pick -x inserts blank line when conforming footer not found' '\n \tpristine_detach initial &&\n \tsha1=$(git rev-parse mesg-no-footer^0) &&\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex ba4902d..635b394 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1277,8 +1277,7 @@ EOF\n 4:Subject: [PATCH] subject\n 8:\n 9:I want to mention about Signed-off-by: here.\n-10:\n-11:Signed-off-by: C O Mitter \n+10:Signed-off-by: C O Mitter \n EOF\n \ttest_cmp expected actual\n '\n@@ -1294,8 +1293,7 @@ EOF\n 4:Subject: [PATCH] subject\n 8:\n 10:Signed-off-by: example happens to be wrapped here.\n-11:\n-12:Signed-off-by: C O Mitter \n+11:Signed-off-by: C O Mitter \n EOF\n \ttest_cmp expected actual\n '\n@@ -1368,7 +1366,7 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'signoff: detect garbage in non-conforming footer' '\n+test_expect_success 'signoff: tolerate garbage in conforming footer' '\n \tappend_signoff <<\\EOF >actual &&\n subject\n \n@@ -1383,8 +1381,36 @@ EOF\n 8:\n 10:\n 13:Signed-off-by: C O Mitter \n-14:\n-15:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: respect trailer config' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+Myfooter: x\n+Some Trash\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+11:\n+12:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual &&\n+\n+\ttest_config trailer.Myfooter.ifexists add &&\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+Myfooter: x\n+Some Trash\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+11:Signed-off-by: C O Mitter \n EOF\n \ttest_cmp expected actual\n '\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex d84897a..4003a27 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -460,6 +460,42 @@ $alt\" &&\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'signoff respects trailer config' '\n+\n+\techo 5 >positive &&\n+\tgit add positive &&\n+\tgit commit -s -m \"subject\n+\n+non-trailer line\n+Myfooter: x\" &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" > actual &&\n+\t(\n+\t\techo subject\n+\t\techo\n+\t\techo non-trailer line\n+\t\techo Myfooter: x\n+\t\techo\n+\t\techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n+\t) >expected &&\n+\ttest_cmp expected actual &&\n+\n+\techo 6 >positive &&\n+\tgit add positive &&\n+\tgit -c \"trailer.Myfooter.ifexists=add\" commit -s -m \"subject\n+\n+non-trailer line\n+Myfooter: x\" &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" > actual &&\n+\t(\n+\t\techo subject\n+\t\techo\n+\t\techo non-trailer line\n+\t\techo Myfooter: x\n+\t\techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n+\t) >expected &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'multiple -m' '\n \n \t>negative &&\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305148","messageId":"xmqqzilovtye.fsf@gitster.mtv.corp.google.com","threadId":"44397","inReplyTo":"cover.1477698917.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/4] Make other git commands use trailer layout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-29T01:12:57Z","receivedAt":"2016-10-29T01:13:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> This is built off jt/trailer-with-cruft (commit 60ef86a).\n>\n> This patch set makes \"commit -s\", \"cherry-pick -x\", and\n> \"format-patch --signoff\" use the new trailer definition implemented in\n> jt/trailer-with-cruft, with some refactoring along the way. With this\n> patch set, the aforementioned commands would now handle trailers like\n> those described in [1].\n>\n> [1] <84f28caa-2e4b-1231-1a76-3b7e765c0b61@google.com>\n\nOoooh. Looks delicious ;-)\n"},{"id":"305162","messageId":"CAP8UFD3o89EzQLLsMtLWxBhKCG4RmbG6u+UrLwzyAAEyezv1cA@mail.gmail.com","threadId":"44397","inReplyTo":"db609e13740415ac4e5e357493661347b0f681f7.1477698917.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/4] trailer: avoid unnecessary splitting on lines","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-29T12:25:19Z","receivedAt":"2016-10-29T12:25:26Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Oct 29, 2016 at 2:05 AM, Jonathan Tan wrote:\n> trailer.c currently splits lines while processing a buffer (and also\n> rejoins lines when needing to invoke ignore_non_trailer).\n>\n> Avoid such line splitting, except when generating the strings\n> corresponding to trailers (for ease of use by clients - a subsequent\n> patch will allow other components to obtain the layout of a trailer\n> block in a buffer, including the trailers themselves). The main purpose\n> of this is to make it easy to return pointers into the original buffer\n> (for a subsequent patch), but this also significantly reduces the number\n> of memory allocations required.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> trailer.c | 215 +++++++++++++++++++++++++++++++++-----------------------------\n> 1 file changed, 116 insertions(+), 99 deletions(-)\n\nIMHO it is telling that this needs 17 more lines.\n\n> @@ -954,7 +971,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n> {\n> LIST_HEAD(head);\n> LIST_HEAD(arg_head);\n> - struct strbuf **lines;\n> + struct strbuf sb = STRBUF_INIT;\n\nWe often use \"sb\" as the name of strbuf variables, but I think at\nleast here (and maybe in other places above) we could use something a\nbit more telling, like \"input_buf\" perhaps.\n\n> int trailer_end;\n> FILE *outfile = stdout;\n>\n"},{"id":"305163","messageId":"CAP8UFD2j9zcZXG0ffuoiyGDHzOjySmtxLpZNzkoqcA0keq0V_A@mail.gmail.com","threadId":"44397","inReplyTo":"xmqqzilovtye.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/4] Make other git commands use trailer layout","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-29T12:37:48Z","receivedAt":"2016-10-29T12:39:01Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Oct 29, 2016 at 3:12 AM, Junio C Hamano wrote:\n> Jonathan Tan writes:\n>\n>> This is built off jt/trailer-with-cruft (commit 60ef86a).\n>>\n>> This patch set makes \"commit -s\", \"cherry-pick -x\", and\n>> \"format-patch --signoff\" use the new trailer definition implemented in\n>> jt/trailer-with-cruft, with some refactoring along the way. With this\n>> patch set, the aforementioned commands would now handle trailers like\n>> those described in [1].\n>>\n>> [1] <84f28caa-2e4b-1231-1a76-3b7e765c0b61@google.com>\n>\n> Ooooh. Looks delicious ;-)\n\nYeah, I am very happy with this goal being reached :-)\n\nThanks,\nChristian.\n"},{"id":"305205","messageId":"xmqqoa20rzrm.fsf@gitster.mtv.corp.google.com","threadId":"44397","inReplyTo":"db609e13740415ac4e5e357493661347b0f681f7.1477698917.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/4] trailer: avoid unnecessary splitting on lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-31T21:10:05Z","receivedAt":"2016-10-31T21:10:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> diff --git a/trailer.c b/trailer.c\n> index 6d8375b..d4d9e10 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -102,12 +102,12 @@ static int same_trailer(struct trailer_item *a, struct arg_item *b)\n> \treturn same_token(a, b) && same_value(a, b);\n> }\n> \n> -static inline int contains_only_spaces(const char *str)\n> +static inline int is_blank_line(const char *str)\n> {\n> \tconst char *s = str;\n> -\twhile (*s && isspace(*s))\n> +\twhile (*s && *s != '\\n' && isspace(*s))\n> \t\ts++;\n> -\treturn !*s;\n> +\treturn !*s || *s == '\\n';\n> }\n\nThis used to be fed a single line and did not have to worry about\nstopping at the end of a line, but now it does have to, and does so\ncorrectly. OK.\n\nAnd the updated function name certainly reflects the fact that this\nis (now) about a line-oriented processing better, and this renaming\nmakes sense.\n\n> @@ -566,13 +566,18 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le\n> }\n> \n> /*\n> - * Return the location of the first separator in line, or -1 if there is no\n> - * separator.\n> + * Return the location of the first separator in the given line, or -1 if there\n> + * is no separator.\n> + *\n> + * The interests parameter must contain the acceptable separators and may\n> + * contain '\\n'. If interests contains '\\n', the input line is considered to\n> + * end at the first newline encountered. Otherwise, the input line is\n> + * considered to end at its null terminator.\n> */\n\nThe name of that terminating byte is NUL, not NULL. \n\n> -static int find_separator(const char *line, const char *separators)\n> +static int find_separator(const char *line, const char *interests)\n> {\n> -\tint loc = strcspn(line, separators);\n> -\tif (!line[loc])\n> +\tint loc = strcspn(line, interests);\n> +\tif (!line[loc] || line[loc] == '\\n')\n> \t\treturn -1;\n> \treturn loc;\n> }\n\nI am not sure interests is a better name than separators. If the\noriginal used \"interests\", I do not think it is worth renaming it to\n\"separators\", and I doubt that the renaming of the parameter in this\npatch is an improvement.\n\n\n> @@ -688,51 +693,71 @@ static void process_command_line_args(struct list_head *arg_head,\n> -static struct strbuf **read_input_file(const char *file)\n> +static void read_input_file(struct strbuf *sb, const char *file)\n> {\n> -\tstruct strbuf **lines;\n> -\tstruct strbuf sb = STRBUF_INIT;\n> -\n> \tif (file) {\n> -\t\tif (strbuf_read_file(&sb, file, 0) < 0)\n> +\t\tif (strbuf_read_file(sb, file, 0) < 0)\n> \t\t\tdie_errno(_(\"could not read input file '%s'\"), file);\n> \t} else {\n> -\t\tif (strbuf_read(&sb, fileno(stdin), 0) < 0)\n> +\t\tif (strbuf_read(sb, fileno(stdin), 0) < 0)\n> \t\t\tdie_errno(_(\"could not read from stdin\"));\n> \t}\n> +}\n\nThe original used to return an array of strbufs but we now just read\ninto a single strbuf. The caller may need to do more, or perhaps\nthe early split into array of strbufs was mostly wasteful use of\nmore flexible data structure. We'll find out when we read what\nhappens to the data read here.\n\n> +static const char *next_line(const char *str)\n> +{\n> +\tconst char *nl = strchrnul(str, '\\n');\n> +\treturn nl + !!*nl;\n> +}\n\n\"next_line()\" gives a pointer to either the beginning of the next\nline, or points at the NUL that terminates the whole buffer. OK.\n\n> +/*\n> + * Return the position of the start of the last line. If len is 0, return -1.\n> + */\n> +static int last_line(const char *buf, size_t len)\n> +{\n> +\tint i;\n> +\tif (len == 0)\n> +\t\treturn -1;\n> +\tif (len == 1)\n> +\t\treturn 0;\n> +\t/*\n> +\t * Skip the last character (in addition to the null terminator),\n> +\t * because if the last character is a newline, it is considered as part\n> +\t * of the last line anyway.\n> +\t */\n> +\ti = len - 2;\n\n... and if the last character is not a newline, the line is an\nincomplete last line. OK.\n\n> -\treturn lines;\n> +\tfor (; i >= 0; i--) {\n> +\t\tif (buf[i] == '\\n')\n> +\t\t\treturn i + 1;\n> +\t}\n> +\treturn 0;\n> }\n\nOK.\n\n> /*\n> - * Return the (0 based) index of the start of the patch or the line\n> - * count if there is no patch in the message.\n> + * Return the position of the start of the patch or the length of str if there\n> + * is no patch in the message.\n> */\n> -static int find_patch_start(struct strbuf **lines, int count)\n> +static int find_patch_start(const char *str)\n> {\n> -\tint i;\n> +\tconst char *s;\n> \n> -\t/* Get the start of the patch part if any */\n> -\tfor (i = 0; i < count; i++) {\n> -\t\tif (starts_with(lines[i]->buf, \"---\"))\n> -\t\t\treturn i;\n> +\tfor (s = str; *s; s = next_line(s)) {\n> +\t\tif (starts_with(s, \"---\"))\n> +\t\t\treturn s - str;\n> \t}\n> \n> -\treturn count;\n> +\treturn s - str;\n> }\n\nThe original assumed that the input was first split into an array of\nstrbufs and found the index that begins with a line \"---\". We have\na flat buffer of input, and find a line that begins with \"---\". The\nreturned value is different (i.e. used to be line-number, now it is\nthe byte offset). OK.\n\n> /*\n> - * Return the (0 based) index of the first trailer line or count if\n> - * there are no trailers. Trailers are searched only in the lines from\n> - * index (count - 1) down to index 0.\n> + * Return the position of the first trailer line or len if there are no\n> + * trailers.\n> */\n\nOK, the idea is the same as the above; it used to be line-based, but\nnow it is counted in byte-offset.\n\n> -static int find_trailer_start(struct strbuf **lines, int count)\n> +static int find_trailer_start(const char *buf, size_t len)\n> {\n> -\tint start, end_of_title, only_spaces = 1;\n> +\tconst char *s;\n> +\tint title_end, l, only_spaces = 1;\n\nbetween end-of-title and title-end, I have slight preference for the\nformer over the latter, but just like separators vs interests, this\nrename is probably more distracting than is worth.\n\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> @@ -742,15 +767,18 @@ static int find_trailer_start(struct strbuf **lines, int count)\n> \t * are to be considered non-trailers).\n> \t */\n> \tint possible_continuation_lines = 0;\n> +\tint ret;\n> +\n> +\tchar *sep_nl = xstrfmt(\"%s\\n\", separators);\n> \n> \t/* The first paragraph is the title and cannot be trailers */\n> -\tfor (start = 0; start < count; start++) {\n> -\t\tif (lines[start]->buf[0] == comment_line_char)\n> +\tfor (s = buf; s < buf + len; s = next_line(s)) {\n> +\t\tif (s[0] == comment_line_char)\n> \t\t\tcontinue;\n> -\t\tif (contains_only_spaces(lines[start]->buf))\n> +\t\tif (is_blank_line(s))\n> \t\t\tbreak;\n> \t}\n> -\tend_of_title = start;\n> +\ttitle_end = s - buf;\n\nOK, we skipped to find the first blank line. The original had an\narray of strbufs to iterate over and didn't have to look for LF but\nnow we do, but it is just the matter of calling next_line() that is\nsimple enough.\n\n> @@ -758,30 +786,33 @@ static int find_trailer_start(struct strbuf **lines, int count)\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> +\tfor (l = last_line(buf, len); l >= title_end; l = last_line(buf, l)) {\n\nThe iteration is conceptually the same. We used to iterate from the\nlast line in the input down to the first blank line that ends the\ntitle paragraph. We do the same but now we are byte-offset based,\nso we start from last_line() of the entire buffer, and go back\none-line-at-a-time by calling last_line(buf, l). OK.\n\n> +\t\tconst char *bol = buf + l;\n> \t\tconst char **p;\n> \t\tint separator_pos;\n> \n> -\t\tif (lines[start]->buf[0] == comment_line_char) {\n> +\t\tif (bol[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\tif (is_blank_line(bol)) {\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> -\t\t\tif (trailer_lines && !non_trailer_lines)\n> -\t\t\t\treturn start + 1;\n> -\t\t\treturn count;\n> +\t\t\t\tret = next_line(bol) - buf;\n\nThe original called the line we are looking at as \"start\"; the\ncorresponding byte-offset is \"l\", and \"bol\" is the pointer to the\nbeginning of the current line. Returning next_line(bol)-buf is\nequivalent to returning \"start + 1\" in the original. OK.\n\n> +\t\t\telse if (trailer_lines && !non_trailer_lines)\n> +\t\t\t\tret = next_line(bol) - buf;\n\nDitto.\n\n> +\t\t\telse\n> +\t\t\t\tret = len;\n\nDitto.\n\n> +\t\t\tgoto cleanup;\n\nWe are no longer \"return\"ing directly from this part of the code,\nand instead saving the return value in \"ret\" and jumping there where\nthe real \"return\" is. We need cleanup because we have an extra\nallocation of separator + LF thing up above.\n\n> ...\n\nThe changes to the remainder of this function shows that the\noriginal lines[] were accessed read-only and it only used\nlines[]->buf without lines[]->len, which is a strong indication that\nan array of strbuf was an overkill. \n\nThe rewrite looks to be a logical equivalent, which is good.\n\n> @@ -817,88 +848,73 @@ static int find_trailer_start(struct strbuf **lines, int count)\n> ...\n> -/* Get the index of the end of the trailers */\n> -static int find_trailer_end(struct strbuf **lines, int patch_start)\n> +/* Return the position of the end of the trailers. */\n> +static int find_trailer_end(const char *buf, size_t len)\n> {\n> -\tstruct strbuf sb = STRBUF_INIT;\n> -\tint i, ignore_bytes;\n> -\n> -\tfor (i = 0; i < patch_start; i++)\n> -\t\tstrbuf_addbuf(&sb, lines[i]);\n> -\tignore_bytes = ignore_non_trailer(sb.buf, sb.len);\n> -\tstrbuf_release(&sb);\n> -\tfor (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)\n> -\t\tignore_bytes -= lines[i]->len;\n> -\n> -\treturn i + 1;\n\nThat indeed was grossly wasteful. Things split into lines[] are\nconcatenated back only to call a single helper function that reports\na byte offset, and then lines[]->len is counted separately to find\nthe line that the byte offset falls in. \n\n> +\treturn len - ignore_non_trailer(buf, len);\n\nUsing the byte-offset based data structure throughout the code makes\nit a lot simpler. Good.\n\n> }\n> \n> -static int has_blank_line_before(struct strbuf **lines, int start)\n> +static int ends_with_blank_line(const char *buf, size_t len)\n> {\n> -\tfor (;start >= 0; start--) {\n> -\t\tif (lines[start]->buf[0] == comment_line_char)\n> -\t\t\tcontinue;\n> -\t\treturn contains_only_spaces(lines[start]->buf);\n> -\t}\n> -\treturn 0;\n> -}\n> -\n> -static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end)\n> -{\n> -\tint i;\n> -\tfor (i = start; lines[i] && i < end; i++)\n> -\t\tfprintf(outfile, \"%s\", lines[i]->buf);\n> +\tint ll = last_line(buf, len);\n> +\tif (ll < 0)\n> +\t\treturn 0;\n> +\treturn is_blank_line(buf + ll);\n> }\n\nTwo helpers \"has-blank-line-before\" and \"print-lines\" are gone.\nLet's see how updated code does things without them.\n\n> static int process_input_file(FILE *outfile,\n> -\t\t\t struct strbuf **lines,\n> +\t\t\t const char *str,\n> \t\t\t struct list_head *head)\n> {\n> -\tint count = 0;\n> -\tint patch_start, trailer_start, trailer_end, i;\n> +\tint patch_start, trailer_start, trailer_end;\n> \tstruct strbuf tok = STRBUF_INIT;\n> \tstruct strbuf val = STRBUF_INIT;\n> \tstruct trailer_item *last = NULL;\n> +\tstruct strbuf *trailer, **trailer_lines, **ptr;\n> \n> -\t/* Get the line count */\n> -\twhile (lines[count])\n> -\t\tcount++;\n> -\n> -\tpatch_start = find_patch_start(lines, count);\n> -\ttrailer_end = find_trailer_end(lines, patch_start);\n> -\ttrailer_start = find_trailer_start(lines, trailer_end);\n> +\tpatch_start = find_patch_start(str);\n> +\ttrailer_end = find_trailer_end(str, patch_start);\n> +\ttrailer_start = find_trailer_start(str, trailer_end);\n> \n> \t/* Print lines before the trailers as is */\n> -\tprint_lines(outfile, lines, 0, trailer_start);\n> +\tfwrite(str, 1, trailer_start, outfile);\n\nAh, of course, if you don't split into an array of strbuf then you\ndo not need a helper that iterates over the array and prints.\n\n> -\tif (!has_blank_line_before(lines, trailer_start - 1))\n> +\tif (!ends_with_blank_line(str, trailer_start))\n> \t\tfprintf(outfile, \"\\n\");\n\nThen if the part before the trailer doesn't end with a blank, we\nforce a blank. \n\nThis is not a new issue, but this makes me wonder what happens when\nthere is no trailer to emit after this. Do we end up with an extra\nblank line?\n\n> \t/* Parse trailer lines */\n> -\tfor (i = trailer_start; i < trailer_end; i++) {\n> +\ttrailer_lines = strbuf_split_buf(str + trailer_start, \n> +\t\t\t\t\t trailer_end - trailer_start,\n> +\t\t\t\t\t '\\n',\n> +\t\t\t\t\t 0);\n\nWe want each line NUL terminated while going over the trailer part\nline-by-line so that it is easy to do _addf(&buf, \"%s\"). Use of\nstrbuf_split_buf() to split them into an array of strbufs is close\nto the way the original did it, so that is why it is done here.\n\nOK. If we had a helper function that would split into an array of\n\"const char *\"s, I suspect it would have worked equally well. In\nthe loop, we only look at ->buf of the strbuf instances and do not\ntake advantage of the fact that the length of each line is known.\nBut that is better left as a further clean-up to the future.\n\nThanks. This step looks like a regression-free rewrite.\n\n\n"},{"id":"305207","messageId":"xmqqd1igrzhj.fsf@gitster.mtv.corp.google.com","threadId":"44397","inReplyTo":"CAP8UFD3o89EzQLLsMtLWxBhKCG4RmbG6u+UrLwzyAAEyezv1cA@mail.gmail.com","subject":"Re: [PATCH 2/4] trailer: avoid unnecessary splitting on lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-31T21:16:08Z","receivedAt":"2016-10-31T21:16:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Sat, Oct 29, 2016 at 2:05 AM, Jonathan Tan wrote:\n>> trailer.c currently splits lines while processing a buffer (and also\n>> rejoins lines when needing to invoke ignore_non_trailer).\n>>\n>> Avoid such line splitting, except when generating the strings\n>> corresponding to trailers (for ease of use by clients - a subsequent\n>> patch will allow other components to obtain the layout of a trailer\n>> block in a buffer, including the trailers themselves). The main purpose\n>> of this is to make it easy to return pointers into the original buffer\n>> (for a subsequent patch), but this also significantly reduces the number\n>> of memory allocations required.\n>>\n>> Signed-off-by: Jonathan Tan \n>> ---\n>> trailer.c | 215 +++++++++++++++++++++++++++++++++-----------------------------\n>> 1 file changed, 116 insertions(+), 99 deletions(-)\n>\n> IMHO it is telling that this needs 17 more lines.\n\nGiven that among which 13 are additional comments to explain what is\ngoing on, I think this is a surprisingly good outcome, relative to\nwhat the patch achieves (reducing a lot of split/rejoin operations\nand need for allocation associated with them that are all made\nunnecessary).\n\nI agree with you that it is telling to see that this needs only 17\nmore lines (or 4, if you only count the true code additions) and\nclearly shows that strbuf_split() helper function was a not good\nmatch for the codepath touched by this patch. The helper function\nis very tempting in that with a simple single call it will give us\ntons of strbufs in an array, each of which are _MUCH_ more flexible\nthan simple strings if we ever need to manipulate them by trimming,\nsplicing and inserting etc. This patch shows us that if we do not\nneed the flexibility, doing strbuf_split() as the first thing on the\ninput and having to deal with an array of strbuf is an overall loss,\nI would think.\n\nAlthough I admit that I carefully read only up to [2/4] I am fairly\nhappy with this series. It finally fixes the second of the two\nissues I pointed out in the review of the original series (the other\none was fixed in the series that this topic depends on), paying down\ntechnical debt.\n\n>> @@ -954,7 +971,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n>> {\n>> LIST_HEAD(head);\n>> LIST_HEAD(arg_head);\n>> - struct strbuf **lines;\n>> + struct strbuf sb = STRBUF_INIT;\n>\n> We often use \"sb\" as the name of strbuf variables, but I think at\n> least here (and maybe in other places above) we could use something a\n> bit more telling, like \"input_buf\" perhaps.\n\nThat sounds sensible.\n\n>\n>> int trailer_end;\n>> FILE *outfile = stdout;\n>>\n"},{"id":"305217","messageId":"xmqqr36wqgep.fsf@gitster.mtv.corp.google.com","threadId":"44397","inReplyTo":"d55ff281b88624cdbf5c21a56d817d8d17eff5c0.1477698917.git.jonathantanmy@google.com","subject":"Re: [PATCH 3/4] trailer: have function to describe trailer layout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-31T22:53:34Z","receivedAt":"2016-10-31T22:53:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> static char *separators = \":\";\n> \n> +static int configured = 0;\n\nAvoid initializing a static to 0 or NULL; instead let .bss take care\nof it.\n\n> static const char *token_from_item(struct arg_item *item, char *tok)\n> {\n> \tif (item->conf.key)\n> @@ -872,59 +885,43 @@ static int process_input_file(FILE *outfile,\n> \t\t\t const char *str,\n> \t\t\t struct list_head *head)\n> {\n> -\tint patch_start, trailer_start, trailer_end;\n> +\tstruct trailer_info info;\n> \tstruct strbuf tok = STRBUF_INIT;\n> \tstruct strbuf val = STRBUF_INIT;\n> -\tstruct trailer_item *last = NULL;\n> -\tstruct strbuf *trailer, **trailer_lines, **ptr;\n> +\tint i;\n> \n> -\tpatch_start = find_patch_start(str);\n> -\ttrailer_end = find_trailer_end(str, patch_start);\n> -\ttrailer_start = find_trailer_start(str, trailer_end);\n> +\ttrailer_info_get(&info, str);\n\nOK, it needs a reading of trailer_info_get() first to understand the\nremainder of this function. The function would grab these fields\ninto info, among doing other things.\n\n> \t/* Print lines before the trailers as is */\n> -\tfwrite(str, 1, trailer_start, outfile);\n> +\tfwrite(str, 1, info.trailer_start - str, outfile);\n> \n> -\tif (!ends_with_blank_line(str, trailer_start))\n> +\tif (!info.blank_line_before_trailer)\n> \t\tfprintf(outfile, \"\\n\");\n\n... and one of the \"other things\" include setting the\n->blank_line_before_trailer field. \n\n> -\t/* Parse trailer lines */\n> -\ttrailer_lines = strbuf_split_buf(str + trailer_start, \n> -\t\t\t\t\t trailer_end - trailer_start,\n> -\t\t\t\t\t '\\n',\n> -\t\t\t\t\t 0);\n> -\tfor (ptr = trailer_lines; *ptr; ptr++) {\n> +\tfor (i = 0; i < info.trailer_nr; i++) {\n> \t\tint separator_pos;\n> -\t\ttrailer = *ptr;\n> -\t\tif (trailer->buf[0] == comment_line_char)\n> +\t\tchar *trailer = info.trailers[i];\n> +\t\tif (trailer[0] == comment_line_char)\n> \t\t\tcontinue;\n\nAnd info.trailers[] is no longer an array of strbuf; it is an array\nof \"char *\", which I alluded to during my review of [2/4]. Looking\ngood.\n\n> -\t\tif (last && isspace(trailer->buf[0])) {\n> -\t\t\tstruct strbuf sb = STRBUF_INIT;\n> -\t\t\tstrbuf_addf(&sb, \"%s\\n%s\", last->value, trailer->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(trailer->buf, separators);\n> +\t\tseparator_pos = find_separator(trailer, separators);\n\n... presumably, the line-folding is already handled in\ntrailer_info_get(), so we do not need the \"last\" handling.\n\n> \t\tif (separator_pos >= 1) {\n\n... and it it is a \"mail-header: looking\" one, then add it one way.\n\n> -\t\t\tparse_trailer(&tok, &val, NULL, trailer->buf,\n> -\t\t\t\t separator_pos);\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\tparse_trailer(&tok, &val, NULL, trailer,\n> +\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} else {\n> -\t\t\tstrbuf_addbuf(&val, trailer);\n> +\t\t\tstrbuf_addstr(&val, trailer);\n\n... otherwise add it another way.\n\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> -\tstrbuf_list_free(trailer_lines);\n> \n> -\treturn trailer_end;\n> +\ttrailer_info_release(&info);\n> +\n> +\treturn info.trailer_end - str;\n> }\n\nNicely done.\n\n> @@ -1004,3 +999,54 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n> \n> \tstrbuf_release(&sb);\n> }\n> +\n> +void trailer_info_get(struct trailer_info *info, const char *str)\n> +{\n> +\tint patch_start, trailer_end, trailer_start;\n> +\tstruct strbuf **trailer_lines, **ptr;\n> +\tchar **trailer_strings = NULL;\n> +\tsize_t nr = 0, alloc = 0;\n> +\tchar **last = NULL;\n> +\n> +\tensure_configured();\n> +\n> +\tpatch_start = find_patch_start(str);\n> +\ttrailer_end = find_trailer_end(str, patch_start);\n> +\ttrailer_start = find_trailer_start(str, trailer_end);\n> +\n> +\ttrailer_lines = strbuf_split_buf(str + trailer_start,\n> +\t\t\t\t\t trailer_end - trailer_start,\n> +\t\t\t\t\t '\\n',\n> +\t\t\t\t\t 0);\n> +\tfor (ptr = trailer_lines; *ptr; ptr++) {\n> +\t\tif (last && isspace((*ptr)->buf[0])) {\n> +\t\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\t\tstrbuf_attach(&sb, *last, strlen(*last), strlen(*last));\n> +\t\t\tstrbuf_addbuf(&sb, *ptr);\n> +\t\t\t*last = strbuf_detach(&sb, NULL);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tALLOC_GROW(trailer_strings, nr + 1, alloc);\n> +\t\ttrailer_strings[nr] = strbuf_detach(*ptr, NULL);\n> +\t\tlast = find_separator(trailer_strings[nr], separators) >= 1\n> +\t\t\t? &trailer_strings[nr]\n> +\t\t\t: NULL;\n> +\t\tnr++;\n> +\t}\n> +\tstrbuf_list_free(trailer_lines);\n> +\n> +\tinfo->blank_line_before_trailer = ends_with_blank_line(str,\n> +\t\t\t\t\t\t\t trailer_start);\n> +\tinfo->trailer_start = str + trailer_start;\n> +\tinfo->trailer_end = str + trailer_end;\n> +\tinfo->trailers = trailer_strings;\n> +\tinfo->trailer_nr = nr;\n> +}\n\nOK, looking good.\n\n"},{"id":"305220","messageId":"xmqqeg2wqa1e.fsf@gitster.mtv.corp.google.com","threadId":"44397","inReplyTo":"602ae84920300cdbb439eca8098c5e092ca322f7.1477698917.git.jonathantanmy@google.com","subject":"Re: [PATCH 4/4] sequencer: use trailer's trailer layout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-01T01:11:09Z","receivedAt":"2016-11-01T01:11:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> @@ -26,30 +27,6 @@ static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE)\n> static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR)\n> static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE)\n> \n> -static int is_rfc2822_line(const char *buf, int len)\n> -{\n> -\tint i;\n> -\n> -\tfor (i = 0; i < len; i++) {\n> -\t\tint ch = buf[i];\n> -\t\tif (ch == ':')\n> -\t\t\treturn 1;\n> -\t\tif (!isalnum(ch) && ch != '-')\n> -\t\t\tbreak;\n> -\t}\n> -\n> -\treturn 0;\n> -}\n> -\n> -static int is_cherry_picked_from_line(const char *buf, int len)\n> -{\n> -\t/*\n> -\t * We only care that it looks roughly like (cherry picked from ...)\n> -\t */\n> -\treturn len > strlen(cherry_picked_prefix) + 1 &&\n> -\t\tstarts_with(buf, cherry_picked_prefix) && buf[len - 1] == ')';\n> -}\n\nWe lost two helper functions here, one to detect \"Mail-header: like\"\nline, the other to detect \"(cherry picked from\") line. Let's see\nhow the updated caller can do without these. We know that both of\nthese can be caught if we grabbed the block of trailer block.\n\n> @@ -59,49 +36,25 @@ static int is_cherry_picked_from_line(const char *buf, int len)\n> static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n> \tint ignore_footer)\n> {\n> -\tchar prev;\n> -\tint i, k;\n> -\tint len = sb->len - ignore_footer;\n> -\tconst char *buf = sb->buf;\n> -\tint found_sob = 0;\n> -\n> -\t/* footer must end with newline */\n> -\tif (!len || buf[len - 1] != '\\n')\n> -\t\treturn 0;\n> +\tstruct trailer_info info;\n> +\tint i;\n> +\tint found_sob = 0, found_sob_last = 0;\n> \n> -\tprev = '\\0';\n> -\tfor (i = len - 1; i > 0; i--) {\n> -\t\tchar ch = buf[i];\n> -\t\tif (prev == '\\n' && ch == '\\n') /* paragraph break */\n> -\t\t\tbreak;\n> -\t\tprev = ch;\n> -\t}\n> +\ttrailer_info_get(&info, sb->buf);\n> \n> -\t/* require at least one blank line */\n> -\tif (prev != '\\n' || buf[i] != '\\n')\n> +\tif (info.trailer_start == info.trailer_end)\n> \t\treturn 0;\n\nSo we feed the thing to trailer_info_get() which will find the\ntrailer block. If there is no trailer block, start and end will\npoint at the same place, which is trivial.\n\n> \n> -\t/* advance to start of last paragraph */\n> -\twhile (i < len - 1 && buf[i] == '\\n')\n> -\t\ti++;\n> -\n> -\tfor (; i < len; i = k) {\n> -\t\tint found_rfc2822;\n> -\n> -\t\tfor (k = i; k < len && buf[k] != '\\n'; k++)\n> -\t\t\t; /* do nothing */\n> -\t\tk++;\n> +\tfor (i = 0; i < info.trailer_nr; i++)\n> +\t\tif (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {\n> +\t\t\tfound_sob = 1;\n> +\t\t\tif (i == info.trailer_nr - 1)\n> +\t\t\t\tfound_sob_last = 1;\n> +\t\t}\n> \n> -\t\tfound_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);\n> -\t\tif (found_rfc2822 && sob &&\n> -\t\t !strncmp(buf + i, sob->buf, sob->len))\n> -\t\t\tfound_sob = k;\n\nThen we scan the trailer block and see if we are looking at the same\ns-o-b line as we are asked to look for, and if it is at the last\nlogical line in the trailer block.\n\n> +\ttrailer_info_release(&info);\n> \n> -\t\tif (!(found_rfc2822 ||\n> -\t\t is_cherry_picked_from_line(buf + i, k - i - 1)))\n> -\t\t\treturn 0;\n\nWe used to reject a \"last paragraph\" that has \"cruft\" other than\n\"Mail-header: like\" line or \"(cherry-picked from\" line. By reusing\nthe trailer code, we are getting consistently looser with its logic.\n\n> -\t}\n> -\tif (found_sob == i)\n> +\tif (found_sob_last)\n> \t\treturn 3;\n> \tif (found_sob)\n> \t\treturn 2;\n\nI found it surprising that you said \"commit -s\", \"cherry-pick -x\"\nand \"format-patch -s\" are covered by this patch and saw a change\nonly to sequencer.c.\n\nIt turns out append_signoff() is the central function that is shared\namong builtin/commit.c::prepare_to_commit() that does \"commit -s\",\nlog-tree.c::show_log() that is called by \"format-patch -s\", and\nsequencer.c::do_recursive_merge() that do_pick_commit() hence \"git\ncherry-pick -s\" uses. And that function decides where to put a new\nS-o-b by calling this has_conforming_footer() function.\n\nIn addition, do_pick_commit() also calls has_conforming_footer() to\ndecide where to add \"(cherry picked from\".\n\nWhoever did the sequencer.c should be proud to structure the code to\nmake this change so easy to make (I know it is not me, and you\nJonathan know it is not you). \n\nNicely done by both of you.\n\n> diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\n> index 9cce5ae..bf0a5c9 100755\n> --- a/t/t3511-cherry-pick-x.sh\n> +++ b/t/t3511-cherry-pick-x.sh\n> @@ -25,9 +25,8 @@ Signed-off-by: B.U. Thor \"\n> \n> mesg_broken_footer=\"$mesg_no_footer\n> \n> -The signed-off-by string should begin with the words Signed-off-by followed\n> -by a colon and space, and then the signers name and email address. e.g.\n> -Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n> +This is not recognized as a footer because Myfooter is not a recognized token.\n> +Myfooter: A.U. Thor \"\n> \n> mesg_with_footer_sob=\"$mesg_with_footer\n> Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n> @@ -112,6 +111,17 @@ test_expect_success 'cherry-pick -s inserts blank line after non-conforming foot\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'cherry-pick -s recognizes trailer config' '\n> +\tpristine_detach initial &&\n> +\tgit -c \"trailer.Myfooter.ifexists=add\" cherry-pick -s mesg-broken-footer &&\n> +\tcat <<-EOF >expect &&\n> +\t\t$mesg_broken_footer\n> +\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n> +\tEOF\n> +\tgit log -1 --pretty=format:%B >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n\nThat \"myfooter\" one is normally not recognized as a valid trailer,\nso the test before this one would add a blank line before adding a\nnew S-o-b; this one adds \"myfooter\" thing to the vocabulary and no\nlonger gets the blank before. Good.\n\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index ba4902d..635b394 100755\n> --- a/t/t4014-format-patch.sh\n> +++ b/t/t4014-format-patch.sh\n> @@ -1277,8 +1277,7 @@ EOF\n> 4:Subject: [PATCH] subject\n> 8:\n> 9:I want to mention about Signed-off-by: here.\n> -10:\n> -11:Signed-off-by: C O Mitter \n> +10:Signed-off-by: C O Mitter \n> EOF\n> \ttest_cmp expected actual\n> '\n\nThe original log message is a single-liner subject line, blank, \"I\nwant to mention...\" and when asked to append S-o-b:, we would want\nto see a blank before the added S-o-b, no?\n\nThis seems a bit weird.\n\n> @@ -1294,8 +1293,7 @@ EOF\n> 4:Subject: [PATCH] subject\n> 8:\n> 10:Signed-off-by: example happens to be wrapped here.\n> -11:\n> -12:Signed-off-by: C O Mitter \n> +11:Signed-off-by: C O Mitter \n> EOF\n> \ttest_cmp expected actual\n> '\n\nThis one's original is a single-liner \"subject\", blank, \"My\nunfortunate\", immediately followed by \"S-o-b:\". The trailer's\nloosened rule (mis)takes the two line body as a trailer block\nbecause one of them is recognised as S-o-b, so it is understandable\nthat we do not see a blank before the new S-o-b.\n\nThis particular example may not look ideal, but it is exactly what\nwe wanted to happen while we were loosening the rule in the previous\nseries, so the outcome is understandable.\n\n> @@ -1368,7 +1366,7 @@ EOF\n> \ttest_cmp expected actual\n> '\n> \n> -test_expect_success 'signoff: detect garbage in non-conforming footer' '\n> +test_expect_success 'signoff: tolerate garbage in conforming footer' '\n> \tappend_signoff <<\\EOF >actual &&\n> subject\n> \n> @@ -1383,8 +1381,36 @@ EOF\n> 8:\n> 10:\n> 13:Signed-off-by: C O Mitter \n> -14:\n> -15:Signed-off-by: C O Mitter \n> +EOF\n> +\ttest_cmp expected actual\n> +'\n\nThis is understandable and desirable. The input has \"Tested-by: \",\na cruft that says \"Some Trash\", and S-o-b and we used to reject that\nthree line as not-a-trailer. We now allow that.\n\n> +test_expect_success 'signoff: respect trailer config' '\n> +\tappend_signoff <<\\EOF >actual &&\n> +subject\n> +\n> +Myfooter: x\n> +Some Trash\n> +EOF\n> +\tcat >expected <<\\EOF &&\n> +4:Subject: [PATCH] subject\n> +8:\n> +11:\n> +12:Signed-off-by: C O Mitter \n> +EOF\n> +\ttest_cmp expected actual &&\n> +\n> +\ttest_config trailer.Myfooter.ifexists add &&\n> +\tappend_signoff <<\\EOF >actual &&\n> +subject\n> +\n> +Myfooter: x\n> +Some Trash\n> +EOF\n> +\tcat >expected <<\\EOF &&\n> +4:Subject: [PATCH] subject\n> +8:\n> +11:Signed-off-by: C O Mitter \n\nOK. It is not easy to see in the test output, but the shift of the\nlocation from 12 to 11 where a new S-o-b appears is because the last\ntwo lines in the original is taken as a trailer block due to \"Myfooter:\"\nbeing configured as a valid trailer element.\n\n> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\n> index d84897a..4003a27 100755\n> --- a/t/t7501-commit.sh\n> +++ b/t/t7501-commit.sh\n> @@ -460,6 +460,42 @@ $alt\" &&\n> \ttest_cmp expected actual\n> '\n> \n> +test_expect_success 'signoff respects trailer config' '\n> +\n> +\techo 5 >positive &&\n> +\tgit add positive &&\n> +\tgit commit -s -m \"subject\n> +\n> +non-trailer line\n> +Myfooter: x\" &&\n> +\tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" > actual &&\n> +\t(\n> +\t\techo subject\n> +\t\techo\n> +\t\techo non-trailer line\n> +\t\techo Myfooter: x\n> +\t\techo\n> +\t\techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n\nAny reason why this does not use \"cat <<-EOF\"? Ahh, this mimicks\nthe style of existing ones. OK.\n\n> +\t) >expected &&\n> +\ttest_cmp expected actual &&\n> +\n> +\techo 6 >positive &&\n> +\tgit add positive &&\n> +\tgit -c \"trailer.Myfooter.ifexists=add\" commit -s -m \"subject\n> +\n> +non-trailer line\n> +Myfooter: x\" &&\n> +\tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" > actual &&\n> +\t(\n> +\t\techo subject\n> +\t\techo\n> +\t\techo non-trailer line\n> +\t\techo Myfooter: x\n> +\t\techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n> +\t) >expected &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> test_expect_success 'multiple -m' '\n> \n> \t>negative &&\n\nLooks good.\n"},{"id":"305236","messageId":"a416ab9b-ff1f-9a71-3e58-60fd4f8a6b8e@google.com","threadId":"44397","inReplyTo":"xmqqeg2wqa1e.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/4] sequencer: use trailer's trailer layout","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-11-01T17:38:43Z","receivedAt":"2016-11-01T17:38:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 10/31/2016 06:11 PM, Junio C Hamano wrote:\n> Jonathan Tan writes:\n>> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n>> index ba4902d..635b394 100755\n>> --- a/t/t4014-format-patch.sh\n>> +++ b/t/t4014-format-patch.sh\n>> @@ -1277,8 +1277,7 @@ EOF\n>> 4:Subject: [PATCH] subject\n>> 8:\n>> 9:I want to mention about Signed-off-by: here.\n>> -10:\n>> -11:Signed-off-by: C O Mitter \n>> +10:Signed-off-by: C O Mitter \n>> EOF\n>> \ttest_cmp expected actual\n>> '\n>\n> The original log message is a single-liner subject line, blank, \"I\n> want to mention...\" and when asked to append S-o-b:, we would want\n> to see a blank before the added S-o-b, no?\n>\n> This seems a bit weird.\n\nThis is because the \"I want to mention\" block has 100% trailer lines \n(since its only line contains a colon). We could forbid spaces in \ntrailer field names, but as you said [1], it might be better to allow \nthem since users might include them.\n\nThe original sequencer.c interpreted this block as not a trailer block, \nbecause it only accepted alphanumeric characters or '-' before the colon \n(and no spaces) - hence the difference in behavior.\n\n[1] \n"},{"id":"305240","messageId":"xmqqtwbroykt.fsf@gitster.mtv.corp.google.com","threadId":"44397","inReplyTo":"a416ab9b-ff1f-9a71-3e58-60fd4f8a6b8e@google.com","subject":"Re: [PATCH 4/4] sequencer: use trailer's trailer layout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-01T18:16:18Z","receivedAt":"2016-11-01T18:16:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>>> 9:I want to mention about Signed-off-by: here.\n>> ...\n>> This seems a bit weird.\n>\n> This is because the \"I want to mention\" block has 100% trailer lines\n> (since its only line contains a colon). We could forbid spaces in\n> trailer field names, but as you said [1], it might be better to allow\n> them since users might include them.\n\nThat merely means that the implementation of the wish expressed in\n[1] was overly loose and needs a bit of tightening, isn't it?\n\n> The original sequencer.c interpreted this block as not a trailer\n> block, because it only accepted alphanumeric characters or '-' before\n> the colon (and no spaces) - hence the difference in behavior.\n\nThat sounds more sensible to me. Would there be an easy way to\nstill allow misspelled \"Thanks to:\" but not be fooled by an obvious\nnonsense like this example, without going deep into natural language\nprocessing? If not, we may want to tighten it back.\n\n>\n> [1] \n"},{"id":"305248","messageId":"cover.1478028700.git.jonathantanmy@google.com","threadId":"44397","inReplyTo":"cover.1477698917.git.jonathantanmy@google.com","subject":"[PATCH v2 0/5] Make other git commands use trailer layout","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-11-01T20:08:20Z","receivedAt":"2016-11-01T20:08:36Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks for all your comments.\n\nThis patch set is now built off master (since jt/trailer-with-cruft is\nmerged).\n\nI couldn't think of an easy way to clearly decide if a token with spaces\nshould be considered a token, so I've tightened the restrictions. One\nbenefit is that we no longer need to create temporary strings that\ninclude '\\n' to be passed into the find_separator method.\n\nIn 2/4 (now 3/5), I've also changed some variable names as requested\n(e.g. sb -> input, and un-did some others).\n\nJonathan Tan (5):\n trailer: be stricter in parsing separators\n commit: make ignore_non_trailer take buf/len\n trailer: avoid unnecessary splitting on lines\n trailer: have function to describe trailer layout\n sequencer: use trailer's trailer layout\n\n builtin/commit.c | 2 +-\n commit.c | 22 ++--\n commit.h | 2 +-\n sequencer.c | 75 +++---------\n t/t3511-cherry-pick-x.sh | 16 ++-\n t/t4014-format-patch.sh | 37 +++++-\n t/t7501-commit.sh | 36 ++++++\n trailer.c | 296 ++++++++++++++++++++++++++++-------------------\n trailer.h | 25 ++++\n 9 files changed, 313 insertions(+), 198 deletions(-)\n\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305249","messageId":"c7db0aafb543845382e1835e3704273d3596e6bb.1478028700.git.jonathantanmy@google.com","threadId":"44397","inReplyTo":"cover.1478028700.git.jonathantanmy@google.com","subject":"[PATCH v2 1/5] trailer: be stricter in parsing separators","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-11-01T20:08:21Z","receivedAt":"2016-11-01T20:08:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, a line is interpreted to be a trailer line if it contains a\nseparator. Make parsing stricter by requiring the text on the left of\nthe separator, if not the empty string, to be of the \"\" form.\n\n(The find_separator function distinguishes the no-separator case from\nthe separator-starts-line case because some callers of this function\nneed such a distinction.)\n\nSigned-off-by: Jonathan Tan \n---\n trailer.c | 23 +++++++++++++++++------\n 1 file changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex f0ecde2..0ee634f 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -563,15 +563,26 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le\n }\n \n /*\n- * Return the location of the first separator in line, or -1 if there is no\n- * separator.\n+ * If the given line is of the form \n+ * \"...\" or \"...\", return the\n+ * location of the separator. Otherwise, return -1.\n */\n static int find_separator(const char *line, const char *separators)\n {\n-\tint loc = strcspn(line, separators);\n-\tif (!line[loc])\n-\t\treturn -1;\n-\treturn loc;\n+\tint whitespace_found = 0;\n+\tconst char *c;\n+\tfor (c = line; *c; c++) {\n+\t\tif (strchr(separators, *c))\n+\t\t\treturn c - line;\n+\t\tif (!whitespace_found && (isalnum(*c) || *c == '-'))\n+\t\t\tcontinue;\n+\t\tif (c != line && (*c == ' ' || *c == '\\t')) {\n+\t\t\twhitespace_found = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tbreak;\n+\t}\n+\treturn -1;\n }\n \n /*\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305250","messageId":"6f7d67f97881272bdcd126b8158ce90e3eae537d.1478028700.git.jonathantanmy@google.com","threadId":"44397","inReplyTo":"cover.1478028700.git.jonathantanmy@google.com","subject":"[PATCH v2 2/5] commit: make ignore_non_trailer take buf/len","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-11-01T20:08:22Z","receivedAt":"2016-11-01T20:08:44Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Make ignore_non_trailer take a buf/len pair instead of struct strbuf.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/commit.c | 2 +-\n commit.c | 22 +++++++++++-----------\n commit.h | 2 +-\n trailer.c | 2 +-\n 4 files changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 8976c3d..887ccc7 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -790,7 +790,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tstrbuf_stripspace(&sb, 0);\n \n \tif (signoff)\n-\t\tappend_signoff(&sb, ignore_non_trailer(&sb), 0);\n+\t\tappend_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0);\n \n \tif (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)\n \t\tdie_errno(_(\"could not write commit template\"));\ndiff --git a/commit.c b/commit.c\nindex 856fd4a..2cf8515 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1649,7 +1649,7 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n }\n \n /*\n- * Inspect sb and determine the true \"end\" of the log message, in\n+ * Inspect the given string and determine the true \"end\" of the log message, in\n * order to find where to put a new Signed-off-by: line. Ignored are\n * trailing comment lines and blank lines, and also the traditional\n * \"Conflicts:\" block that is not commented out, so that we can use\n@@ -1659,37 +1659,37 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n * Returns the number of bytes from the tail to ignore, to be fed as\n * the second parameter to append_signoff().\n */\n-int ignore_non_trailer(struct strbuf *sb)\n+int ignore_non_trailer(const char *buf, size_t len)\n {\n \tint boc = 0;\n \tint bol = 0;\n \tint in_old_conflicts_block = 0;\n \n-\twhile (bol < sb->len) {\n-\t\tchar *next_line;\n+\twhile (bol < len) {\n+\t\tconst char *next_line = memchr(buf + bol, '\\n', len - bol);\n \n-\t\tif (!(next_line = memchr(sb->buf + bol, '\\n', sb->len - bol)))\n-\t\t\tnext_line = sb->buf + sb->len;\n+\t\tif (!next_line)\n+\t\t\tnext_line = buf + len;\n \t\telse\n \t\t\tnext_line++;\n \n-\t\tif (sb->buf[bol] == comment_line_char || sb->buf[bol] == '\\n') {\n+\t\tif (buf[bol] == comment_line_char || buf[bol] == '\\n') {\n \t\t\t/* is this the first of the run of comments? */\n \t\t\tif (!boc)\n \t\t\t\tboc = bol;\n \t\t\t/* otherwise, it is just continuing */\n-\t\t} else if (starts_with(sb->buf + bol, \"Conflicts:\\n\")) {\n+\t\t} else if (starts_with(buf + bol, \"Conflicts:\\n\")) {\n \t\t\tin_old_conflicts_block = 1;\n \t\t\tif (!boc)\n \t\t\t\tboc = bol;\n-\t\t} else if (in_old_conflicts_block && sb->buf[bol] == '\\t') {\n+\t\t} else if (in_old_conflicts_block && buf[bol] == '\\t') {\n \t\t\t; /* a pathname in the conflicts block */\n \t\t} else if (boc) {\n \t\t\t/* the previous was not trailing comment */\n \t\t\tboc = 0;\n \t\t\tin_old_conflicts_block = 0;\n \t\t}\n-\t\tbol = next_line - sb->buf;\n+\t\tbol = next_line - buf;\n \t}\n-\treturn boc ? sb->len - boc : 0;\n+\treturn boc ? len - boc : 0;\n }\ndiff --git a/commit.h b/commit.h\nindex afd14f3..9c12abb 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -355,7 +355,7 @@ extern const char *find_commit_header(const char *msg, const char *key,\n \t\t\t\t size_t *out_len);\n \n /* Find the end of the log message, the right place for a new trailer. */\n-extern int ignore_non_trailer(struct strbuf *sb);\n+extern int ignore_non_trailer(const char *buf, size_t len);\n \n typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,\n \t\t\t\t void *cb_data);\ndiff --git a/trailer.c b/trailer.c\nindex 0ee634f..04edab2 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -836,7 +836,7 @@ static int find_trailer_end(struct strbuf **lines, int patch_start)\n \n \tfor (i = 0; i < patch_start; i++)\n \t\tstrbuf_addbuf(&sb, lines[i]);\n-\tignore_bytes = ignore_non_trailer(&sb);\n+\tignore_bytes = ignore_non_trailer(sb.buf, sb.len);\n \tstrbuf_release(&sb);\n \tfor (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)\n \t\tignore_bytes -= lines[i]->len;\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305251","messageId":"31f32c6118bad2ba3604177ab448e97d4db0282f.1478028700.git.jonathantanmy@google.com","threadId":"44397","inReplyTo":"cover.1478028700.git.jonathantanmy@google.com","subject":"[PATCH v2 3/5] trailer: avoid unnecessary splitting on lines","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-11-01T20:08:23Z","receivedAt":"2016-11-01T20:08:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"trailer.c currently splits lines while processing a buffer (and also\nrejoins lines when needing to invoke ignore_non_trailer).\n\nAvoid such line splitting, except when generating the strings\ncorresponding to trailers (for ease of use by clients - a subsequent\npatch will allow other components to obtain the layout of a trailer\nblock in a buffer, including the trailers themselves). The main purpose\nof this is to make it easy to return pointers into the original buffer\n(for a subsequent patch), but this also significantly reduces the number\nof memory allocations required.\n\nSigned-off-by: Jonathan Tan \n---\n trailer.c | 195 ++++++++++++++++++++++++++++++++------------------------------\n 1 file changed, 101 insertions(+), 94 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 04edab2..f5427ec 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -102,12 +102,12 @@ static int same_trailer(struct trailer_item *a, struct arg_item *b)\n \treturn same_token(a, b) && same_value(a, b);\n }\n \n-static inline int contains_only_spaces(const char *str)\n+static inline int is_blank_line(const char *str)\n {\n \tconst char *s = str;\n-\twhile (*s && isspace(*s))\n+\twhile (*s && *s != '\\n' && isspace(*s))\n \t\ts++;\n-\treturn !*s;\n+\treturn !*s || *s == '\\n';\n }\n \n static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *b)\n@@ -696,51 +696,71 @@ static void process_command_line_args(struct list_head *arg_head,\n \tfree(cl_separators);\n }\n \n-static struct strbuf **read_input_file(const char *file)\n+static void read_input_file(struct strbuf *sb, const char *file)\n {\n-\tstruct strbuf **lines;\n-\tstruct strbuf sb = STRBUF_INIT;\n-\n \tif (file) {\n-\t\tif (strbuf_read_file(&sb, file, 0) < 0)\n+\t\tif (strbuf_read_file(sb, file, 0) < 0)\n \t\t\tdie_errno(_(\"could not read input file '%s'\"), file);\n \t} else {\n-\t\tif (strbuf_read(&sb, fileno(stdin), 0) < 0)\n+\t\tif (strbuf_read(sb, fileno(stdin), 0) < 0)\n \t\t\tdie_errno(_(\"could not read from stdin\"));\n \t}\n+}\n \n-\tlines = strbuf_split(&sb, '\\n');\n+static const char *next_line(const char *str)\n+{\n+\tconst char *nl = strchrnul(str, '\\n');\n+\treturn nl + !!*nl;\n+}\n \n-\tstrbuf_release(&sb);\n+/*\n+ * Return the position of the start of the last line. If len is 0, return -1.\n+ */\n+static int last_line(const char *buf, size_t len)\n+{\n+\tint i;\n+\tif (len == 0)\n+\t\treturn -1;\n+\tif (len == 1)\n+\t\treturn 0;\n+\t/*\n+\t * Skip the last character (in addition to the null terminator),\n+\t * because if the last character is a newline, it is considered as part\n+\t * of the last line anyway.\n+\t */\n+\ti = len - 2;\n \n-\treturn lines;\n+\tfor (; i >= 0; i--) {\n+\t\tif (buf[i] == '\\n')\n+\t\t\treturn i + 1;\n+\t}\n+\treturn 0;\n }\n \n /*\n- * Return the (0 based) index of the start of the patch or the line\n- * count if there is no patch in the message.\n+ * Return the position of the start of the patch or the length of str if there\n+ * is no patch in the message.\n */\n-static int find_patch_start(struct strbuf **lines, int count)\n+static int find_patch_start(const char *str)\n {\n-\tint i;\n+\tconst char *s;\n \n-\t/* Get the start of the patch part if any */\n-\tfor (i = 0; i < count; i++) {\n-\t\tif (starts_with(lines[i]->buf, \"---\"))\n-\t\t\treturn i;\n+\tfor (s = str; *s; s = next_line(s)) {\n+\t\tif (starts_with(s, \"---\"))\n+\t\t\treturn s - str;\n \t}\n \n-\treturn count;\n+\treturn s - str;\n }\n \n /*\n- * Return the (0 based) index of the first trailer line or count if\n- * there are no trailers. Trailers are searched only in the lines from\n- * index (count - 1) down to index 0.\n+ * Return the position of the first trailer line or len if there are no\n+ * trailers.\n */\n-static int find_trailer_start(struct strbuf **lines, int count)\n+static int find_trailer_start(const char *buf, size_t len)\n {\n-\tint start, end_of_title, only_spaces = 1;\n+\tconst char *s;\n+\tint end_of_title, l, 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@@ -750,15 +770,16 @@ static int find_trailer_start(struct strbuf **lines, int count)\n \t * are to be considered non-trailers).\n \t */\n \tint possible_continuation_lines = 0;\n+\tint ret;\n \n \t/* The first paragraph is the title and cannot be trailers */\n-\tfor (start = 0; start < count; start++) {\n-\t\tif (lines[start]->buf[0] == comment_line_char)\n+\tfor (s = buf; s < buf + len; s = next_line(s)) {\n+\t\tif (s[0] == comment_line_char)\n \t\t\tcontinue;\n-\t\tif (contains_only_spaces(lines[start]->buf))\n+\t\tif (is_blank_line(s))\n \t\t\tbreak;\n \t}\n-\tend_of_title = start;\n+\tend_of_title = s - buf;\n \n \t/*\n \t * Get the start of the trailers by looking starting from the end for a\n@@ -766,30 +787,33 @@ static int find_trailer_start(struct strbuf **lines, int count)\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+\tfor (l = last_line(buf, len);\n+\t l >= end_of_title;\n+\t l = last_line(buf, l)) {\n+\t\tconst char *bol = buf + l;\n \t\tconst char **p;\n \t\tint separator_pos;\n \n-\t\tif (lines[start]->buf[0] == comment_line_char) {\n+\t\tif (bol[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\tif (is_blank_line(bol)) {\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-\t\t\tif (trailer_lines && !non_trailer_lines)\n-\t\t\t\treturn start + 1;\n-\t\t\treturn count;\n+\t\t\t\treturn next_line(bol) - buf;\n+\t\t\telse if (trailer_lines && !non_trailer_lines)\n+\t\t\t\treturn next_line(bol) - buf;\n+\t\t\treturn len;\n \t\t}\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\tif (starts_with(bol, *p)) {\n \t\t\t\ttrailer_lines++;\n \t\t\t\tpossible_continuation_lines = 0;\n \t\t\t\trecognized_prefix = 1;\n@@ -797,8 +821,8 @@ static int find_trailer_start(struct strbuf **lines, int count)\n \t\t\t}\n \t\t}\n \n-\t\tseparator_pos = find_separator(lines[start]->buf, separators);\n-\t\tif (separator_pos >= 1 && !isspace(lines[start]->buf[0])) {\n+\t\tseparator_pos = find_separator(bol, separators);\n+\t\tif (separator_pos >= 1 && !isspace(bol[0])) {\n \t\t\tstruct list_head *pos;\n \n \t\t\ttrailer_lines++;\n@@ -808,13 +832,13 @@ static int find_trailer_start(struct strbuf **lines, int count)\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\tif (token_matches_item(bol, 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 if (isspace(lines[start]->buf[0]))\n+\t\t} else if (isspace(bol[0]))\n \t\t\tpossible_continuation_lines++;\n \t\telse {\n \t\t\tnon_trailer_lines++;\n@@ -825,88 +849,70 @@ static int find_trailer_start(struct strbuf **lines, int count)\n \t\t;\n \t}\n \n-\treturn count;\n-}\n-\n-/* Get the index of the end of the trailers */\n-static int find_trailer_end(struct strbuf **lines, int patch_start)\n-{\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tint i, ignore_bytes;\n-\n-\tfor (i = 0; i < patch_start; i++)\n-\t\tstrbuf_addbuf(&sb, lines[i]);\n-\tignore_bytes = ignore_non_trailer(sb.buf, sb.len);\n-\tstrbuf_release(&sb);\n-\tfor (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)\n-\t\tignore_bytes -= lines[i]->len;\n-\n-\treturn i + 1;\n+\treturn len;\n }\n \n-static int has_blank_line_before(struct strbuf **lines, int start)\n+/* Return the position of the end of the trailers. */\n+static int find_trailer_end(const char *buf, size_t len)\n {\n-\tfor (;start >= 0; start--) {\n-\t\tif (lines[start]->buf[0] == comment_line_char)\n-\t\t\tcontinue;\n-\t\treturn contains_only_spaces(lines[start]->buf);\n-\t}\n-\treturn 0;\n+\treturn len - ignore_non_trailer(buf, len);\n }\n \n-static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end)\n+static int ends_with_blank_line(const char *buf, size_t len)\n {\n-\tint i;\n-\tfor (i = start; lines[i] && i < end; i++)\n-\t\tfprintf(outfile, \"%s\", lines[i]->buf);\n+\tint ll = last_line(buf, len);\n+\tif (ll < 0)\n+\t\treturn 0;\n+\treturn is_blank_line(buf + ll);\n }\n \n static int process_input_file(FILE *outfile,\n-\t\t\t struct strbuf **lines,\n+\t\t\t const char *str,\n \t\t\t struct list_head *head)\n {\n-\tint count = 0;\n-\tint patch_start, trailer_start, trailer_end, i;\n+\tint patch_start, trailer_start, trailer_end;\n \tstruct strbuf tok = STRBUF_INIT;\n \tstruct strbuf val = STRBUF_INIT;\n \tstruct trailer_item *last = NULL;\n+\tstruct strbuf *trailer, **trailer_lines, **ptr;\n \n-\t/* Get the line count */\n-\twhile (lines[count])\n-\t\tcount++;\n-\n-\tpatch_start = find_patch_start(lines, count);\n-\ttrailer_end = find_trailer_end(lines, patch_start);\n-\ttrailer_start = find_trailer_start(lines, trailer_end);\n+\tpatch_start = find_patch_start(str);\n+\ttrailer_end = find_trailer_end(str, patch_start);\n+\ttrailer_start = find_trailer_start(str, trailer_end);\n \n \t/* Print lines before the trailers as is */\n-\tprint_lines(outfile, lines, 0, trailer_start);\n+\tfwrite(str, 1, trailer_start, outfile);\n \n-\tif (!has_blank_line_before(lines, trailer_start - 1))\n+\tif (!ends_with_blank_line(str, trailer_start))\n \t\tfprintf(outfile, \"\\n\");\n \n \t/* Parse trailer lines */\n-\tfor (i = trailer_start; i < trailer_end; i++) {\n+\ttrailer_lines = strbuf_split_buf(str + trailer_start,\n+\t\t\t\t\t trailer_end - trailer_start,\n+\t\t\t\t\t '\\n',\n+\t\t\t\t\t 0);\n+\tfor (ptr = trailer_lines; *ptr; ptr++) {\n \t\tint separator_pos;\n-\t\tif (lines[i]->buf[0] == comment_line_char)\n+\t\ttrailer = *ptr;\n+\t\tif (trailer->buf[0] == comment_line_char)\n \t\t\tcontinue;\n-\t\tif (last && isspace(lines[i]->buf[0])) {\n+\t\tif (last && isspace(trailer->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_addf(&sb, \"%s\\n%s\", last->value, trailer->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, separators);\n+\t\tseparator_pos = find_separator(trailer->buf, separators);\n \t\tif (separator_pos >= 1) {\n-\t\t\tparse_trailer(&tok, &val, NULL, lines[i]->buf,\n+\t\t\tparse_trailer(&tok, &val, NULL, trailer->buf,\n \t\t\t\t separator_pos);\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_addbuf(&val, trailer);\n \t\t\tstrbuf_strip_suffix(&val, \"\\n\");\n \t\t\tadd_trailer_item(head,\n \t\t\t\t\t NULL,\n@@ -914,6 +920,7 @@ static int process_input_file(FILE *outfile,\n \t\t\tlast = NULL;\n \t\t}\n \t}\n+\tstrbuf_list_free(trailer_lines);\n \n \treturn trailer_end;\n }\n@@ -962,7 +969,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n {\n \tLIST_HEAD(head);\n \tLIST_HEAD(arg_head);\n-\tstruct strbuf **lines;\n+\tstruct strbuf sb = STRBUF_INIT;\n \tint trailer_end;\n \tFILE *outfile = stdout;\n \n@@ -970,13 +977,13 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n \tgit_config(git_trailer_default_config, NULL);\n \tgit_config(git_trailer_config, NULL);\n \n-\tlines = read_input_file(file);\n+\tread_input_file(&sb, file);\n \n \tif (in_place)\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, &head);\n+\ttrailer_end = process_input_file(outfile, sb.buf, &head);\n \n \tprocess_command_line_args(&arg_head, trailers);\n \n@@ -987,11 +994,11 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\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+\tfwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);\n \n \tif (in_place)\n \t\tif (rename_tempfile(&trailers_tempfile, file))\n \t\t\tdie_errno(_(\"could not rename temporary file to %s\"), file);\n \n-\tstrbuf_list_free(lines);\n+\tstrbuf_release(&sb);\n }\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305252","messageId":"6da41901c6cfc4578f93a1baf36cb2b612853b49.1478028700.git.jonathantanmy@google.com","threadId":"44397","inReplyTo":"cover.1478028700.git.jonathantanmy@google.com","subject":"[PATCH v2 5/5] sequencer: use trailer's trailer layout","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-11-01T20:08:25Z","receivedAt":"2016-11-01T20:08:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Make sequencer use trailer.c's trailer layout definition, as opposed to\nparsing the footer by itself. This makes \"commit -s\", \"cherry-pick -x\",\nand \"format-patch --signoff\" consistent with trailer, allowing\nnon-trailer lines and multiple-line trailers in trailer blocks under\ncertain conditions, and therefore suppressing the extra newline in those\ncases.\n\nConsistency with trailer extends to respecting trailer configs. Tests\nhave been included to show that.\n\nSigned-off-by: Jonathan Tan \n---\n sequencer.c | 75 +++++++++---------------------------------------\n t/t3511-cherry-pick-x.sh | 16 +++++++++--\n t/t4014-format-patch.sh | 37 ++++++++++++++++++++----\n t/t7501-commit.sh | 36 +++++++++++++++++++++++\n 4 files changed, 95 insertions(+), 69 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 5fd75f3..d64c973 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -16,6 +16,7 @@\n #include \"refs.h\"\n #include \"argv-array.h\"\n #include \"quote.h\"\n+#include \"trailer.h\"\n \n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n \n@@ -56,30 +57,6 @@ static const char *get_todo_path(const struct replay_opts *opts)\n \treturn git_path_todo_file();\n }\n \n-static int is_rfc2822_line(const char *buf, int len)\n-{\n-\tint i;\n-\n-\tfor (i = 0; i < len; i++) {\n-\t\tint ch = buf[i];\n-\t\tif (ch == ':')\n-\t\t\treturn 1;\n-\t\tif (!isalnum(ch) && ch != '-')\n-\t\t\tbreak;\n-\t}\n-\n-\treturn 0;\n-}\n-\n-static int is_cherry_picked_from_line(const char *buf, int len)\n-{\n-\t/*\n-\t * We only care that it looks roughly like (cherry picked from ...)\n-\t */\n-\treturn len > strlen(cherry_picked_prefix) + 1 &&\n-\t\tstarts_with(buf, cherry_picked_prefix) && buf[len - 1] == ')';\n-}\n-\n /*\n * Returns 0 for non-conforming footer\n * Returns 1 for conforming footer\n@@ -89,49 +66,25 @@ static int is_cherry_picked_from_line(const char *buf, int len)\n static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \tint ignore_footer)\n {\n-\tchar prev;\n-\tint i, k;\n-\tint len = sb->len - ignore_footer;\n-\tconst char *buf = sb->buf;\n-\tint found_sob = 0;\n-\n-\t/* footer must end with newline */\n-\tif (!len || buf[len - 1] != '\\n')\n-\t\treturn 0;\n+\tstruct trailer_info info;\n+\tint i;\n+\tint found_sob = 0, found_sob_last = 0;\n \n-\tprev = '\\0';\n-\tfor (i = len - 1; i > 0; i--) {\n-\t\tchar ch = buf[i];\n-\t\tif (prev == '\\n' && ch == '\\n') /* paragraph break */\n-\t\t\tbreak;\n-\t\tprev = ch;\n-\t}\n+\ttrailer_info_get(&info, sb->buf);\n \n-\t/* require at least one blank line */\n-\tif (prev != '\\n' || buf[i] != '\\n')\n+\tif (info.trailer_start == info.trailer_end)\n \t\treturn 0;\n \n-\t/* advance to start of last paragraph */\n-\twhile (i < len - 1 && buf[i] == '\\n')\n-\t\ti++;\n-\n-\tfor (; i < len; i = k) {\n-\t\tint found_rfc2822;\n-\n-\t\tfor (k = i; k < len && buf[k] != '\\n'; k++)\n-\t\t\t; /* do nothing */\n-\t\tk++;\n+\tfor (i = 0; i < info.trailer_nr; i++)\n+\t\tif (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {\n+\t\t\tfound_sob = 1;\n+\t\t\tif (i == info.trailer_nr - 1)\n+\t\t\t\tfound_sob_last = 1;\n+\t\t}\n \n-\t\tfound_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);\n-\t\tif (found_rfc2822 && sob &&\n-\t\t !strncmp(buf + i, sob->buf, sob->len))\n-\t\t\tfound_sob = k;\n+\ttrailer_info_release(&info);\n \n-\t\tif (!(found_rfc2822 ||\n-\t\t is_cherry_picked_from_line(buf + i, k - i - 1)))\n-\t\t\treturn 0;\n-\t}\n-\tif (found_sob == i)\n+\tif (found_sob_last)\n \t\treturn 3;\n \tif (found_sob)\n \t\treturn 2;\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex 9cce5ae..bf0a5c9 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -25,9 +25,8 @@ Signed-off-by: B.U. Thor \"\n \n mesg_broken_footer=\"$mesg_no_footer\n \n-The signed-off-by string should begin with the words Signed-off-by followed\n-by a colon and space, and then the signers name and email address. e.g.\n-Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n+This is not recognized as a footer because Myfooter is not a recognized token.\n+Myfooter: A.U. Thor \"\n \n mesg_with_footer_sob=\"$mesg_with_footer\n Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n@@ -112,6 +111,17 @@ test_expect_success 'cherry-pick -s inserts blank line after non-conforming foot\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick -s recognizes trailer config' '\n+\tpristine_detach initial &&\n+\tgit -c \"trailer.Myfooter.ifexists=add\" cherry-pick -s mesg-broken-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_broken_footer\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cherry-pick -x inserts blank line when conforming footer not found' '\n \tpristine_detach initial &&\n \tsha1=$(git rev-parse mesg-no-footer^0) &&\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex ba4902d..482112c 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1294,8 +1294,7 @@ EOF\n 4:Subject: [PATCH] subject\n 8:\n 10:Signed-off-by: example happens to be wrapped here.\n-11:\n-12:Signed-off-by: C O Mitter \n+11:Signed-off-by: C O Mitter \n EOF\n \ttest_cmp expected actual\n '\n@@ -1368,7 +1367,7 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'signoff: detect garbage in non-conforming footer' '\n+test_expect_success 'signoff: tolerate garbage in conforming footer' '\n \tappend_signoff <<\\EOF >actual &&\n subject\n \n@@ -1383,8 +1382,36 @@ EOF\n 8:\n 10:\n 13:Signed-off-by: C O Mitter \n-14:\n-15:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: respect trailer config' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+Myfooter: x\n+Some Trash\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+11:\n+12:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual &&\n+\n+\ttest_config trailer.Myfooter.ifexists add &&\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+Myfooter: x\n+Some Trash\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+11:Signed-off-by: C O Mitter \n EOF\n \ttest_cmp expected actual\n '\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex d84897a..4003a27 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -460,6 +460,42 @@ $alt\" &&\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'signoff respects trailer config' '\n+\n+\techo 5 >positive &&\n+\tgit add positive &&\n+\tgit commit -s -m \"subject\n+\n+non-trailer line\n+Myfooter: x\" &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" > actual &&\n+\t(\n+\t\techo subject\n+\t\techo\n+\t\techo non-trailer line\n+\t\techo Myfooter: x\n+\t\techo\n+\t\techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n+\t) >expected &&\n+\ttest_cmp expected actual &&\n+\n+\techo 6 >positive &&\n+\tgit add positive &&\n+\tgit -c \"trailer.Myfooter.ifexists=add\" commit -s -m \"subject\n+\n+non-trailer line\n+Myfooter: x\" &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" > actual &&\n+\t(\n+\t\techo subject\n+\t\techo\n+\t\techo non-trailer line\n+\t\techo Myfooter: x\n+\t\techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n+\t) >expected &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'multiple -m' '\n \n \t>negative &&\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305253","messageId":"7104681617f0214af77a218120658e51144aaac2.1478028700.git.jonathantanmy@google.com","threadId":"44397","inReplyTo":"cover.1478028700.git.jonathantanmy@google.com","subject":"[PATCH v2 4/5] trailer: have function to describe trailer layout","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-11-01T20:08:24Z","receivedAt":"2016-11-01T20:08:52Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Create a function that, taking a string, describes the position of its\ntrailer block (if available) and the contents thereof, and make trailer\nuse it. This makes it easier for other Git components, in the future, to\ninterpret trailer blocks in the same way as trailer.\n\nIn a subsequent patch, another component will be made to use this.\n\nSigned-off-by: Jonathan Tan \n---\n trailer.c | 118 +++++++++++++++++++++++++++++++++++++++++++-------------------\n trailer.h | 25 +++++++++++++\n 2 files changed, 107 insertions(+), 36 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex f5427ec..7265a50 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -46,6 +46,8 @@ static LIST_HEAD(conf_head);\n \n static char *separators = \":\";\n \n+static int configured;\n+\n #define TRAILER_ARG_STRING \"$ARG\"\n \n static const char *git_generated_prefixes[] = {\n@@ -546,6 +548,17 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)\n \treturn 0;\n }\n \n+static void ensure_configured(void)\n+{\n+\tif (configured)\n+\t\treturn;\n+\n+\t/* Default config must be setup first */\n+\tgit_config(git_trailer_default_config, NULL);\n+\tgit_config(git_trailer_config, NULL);\n+\tconfigured = 1;\n+}\n+\n static const char *token_from_item(struct arg_item *item, char *tok)\n {\n \tif (item->conf.key)\n@@ -870,59 +883,43 @@ static int process_input_file(FILE *outfile,\n \t\t\t const char *str,\n \t\t\t struct list_head *head)\n {\n-\tint patch_start, trailer_start, trailer_end;\n+\tstruct trailer_info info;\n \tstruct strbuf tok = STRBUF_INIT;\n \tstruct strbuf val = STRBUF_INIT;\n-\tstruct trailer_item *last = NULL;\n-\tstruct strbuf *trailer, **trailer_lines, **ptr;\n+\tint i;\n \n-\tpatch_start = find_patch_start(str);\n-\ttrailer_end = find_trailer_end(str, patch_start);\n-\ttrailer_start = find_trailer_start(str, trailer_end);\n+\ttrailer_info_get(&info, str);\n \n \t/* Print lines before the trailers as is */\n-\tfwrite(str, 1, trailer_start, outfile);\n+\tfwrite(str, 1, info.trailer_start - str, outfile);\n \n-\tif (!ends_with_blank_line(str, trailer_start))\n+\tif (!info.blank_line_before_trailer)\n \t\tfprintf(outfile, \"\\n\");\n \n-\t/* Parse trailer lines */\n-\ttrailer_lines = strbuf_split_buf(str + trailer_start,\n-\t\t\t\t\t trailer_end - trailer_start,\n-\t\t\t\t\t '\\n',\n-\t\t\t\t\t 0);\n-\tfor (ptr = trailer_lines; *ptr; ptr++) {\n+\tfor (i = 0; i < info.trailer_nr; i++) {\n \t\tint separator_pos;\n-\t\ttrailer = *ptr;\n-\t\tif (trailer->buf[0] == comment_line_char)\n-\t\t\tcontinue;\n-\t\tif (last && isspace(trailer->buf[0])) {\n-\t\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\t\tstrbuf_addf(&sb, \"%s\\n%s\", last->value, trailer->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\tchar *trailer = info.trailers[i];\n+\t\tif (trailer[0] == comment_line_char)\n \t\t\tcontinue;\n-\t\t}\n-\t\tseparator_pos = find_separator(trailer->buf, separators);\n+\t\tseparator_pos = find_separator(trailer, separators);\n \t\tif (separator_pos >= 1) {\n-\t\t\tparse_trailer(&tok, &val, NULL, trailer->buf,\n+\t\t\tparse_trailer(&tok, &val, NULL, trailer,\n \t\t\t\t separator_pos);\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\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, trailer);\n+\t\t\tstrbuf_addstr(&val, trailer);\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-\tstrbuf_list_free(trailer_lines);\n \n-\treturn trailer_end;\n+\ttrailer_info_release(&info);\n+\n+\treturn info.trailer_end - str;\n }\n \n static void free_all(struct list_head *head)\n@@ -973,9 +970,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n \tint trailer_end;\n \tFILE *outfile = stdout;\n \n-\t/* Default config must be setup first */\n-\tgit_config(git_trailer_default_config, NULL);\n-\tgit_config(git_trailer_config, NULL);\n+\tensure_configured();\n \n \tread_input_file(&sb, file);\n \n@@ -1002,3 +997,54 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n \n \tstrbuf_release(&sb);\n }\n+\n+void trailer_info_get(struct trailer_info *info, const char *str)\n+{\n+\tint patch_start, trailer_end, trailer_start;\n+\tstruct strbuf **trailer_lines, **ptr;\n+\tchar **trailer_strings = NULL;\n+\tsize_t nr = 0, alloc = 0;\n+\tchar **last = NULL;\n+\n+\tensure_configured();\n+\n+\tpatch_start = find_patch_start(str);\n+\ttrailer_end = find_trailer_end(str, patch_start);\n+\ttrailer_start = find_trailer_start(str, trailer_end);\n+\n+\ttrailer_lines = strbuf_split_buf(str + trailer_start,\n+\t\t\t\t\t trailer_end - trailer_start,\n+\t\t\t\t\t '\\n',\n+\t\t\t\t\t 0);\n+\tfor (ptr = trailer_lines; *ptr; ptr++) {\n+\t\tif (last && isspace((*ptr)->buf[0])) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_attach(&sb, *last, strlen(*last), strlen(*last));\n+\t\t\tstrbuf_addbuf(&sb, *ptr);\n+\t\t\t*last = strbuf_detach(&sb, NULL);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tALLOC_GROW(trailer_strings, nr + 1, alloc);\n+\t\ttrailer_strings[nr] = strbuf_detach(*ptr, NULL);\n+\t\tlast = find_separator(trailer_strings[nr], separators) >= 1\n+\t\t\t? &trailer_strings[nr]\n+\t\t\t: NULL;\n+\t\tnr++;\n+\t}\n+\tstrbuf_list_free(trailer_lines);\n+\n+\tinfo->blank_line_before_trailer = ends_with_blank_line(str,\n+\t\t\t\t\t\t\t trailer_start);\n+\tinfo->trailer_start = str + trailer_start;\n+\tinfo->trailer_end = str + trailer_end;\n+\tinfo->trailers = trailer_strings;\n+\tinfo->trailer_nr = nr;\n+}\n+\n+void trailer_info_release(struct trailer_info *info)\n+{\n+\tint i;\n+\tfor (i = 0; i < info->trailer_nr; i++)\n+\t\tfree(info->trailers[i]);\n+\tfree(info->trailers);\n+}\ndiff --git a/trailer.h b/trailer.h\nindex 36b40b8..65cc5d7 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -1,7 +1,32 @@\n #ifndef TRAILER_H\n #define TRAILER_H\n \n+struct trailer_info {\n+\t/*\n+\t * True if there is a blank line before the location pointed to by\n+\t * trailer_start.\n+\t */\n+\tint blank_line_before_trailer;\n+\n+\t/*\n+\t * Pointers to the start and end of the trailer block found. If there\n+\t * is no trailer block found, these 2 pointers point to the end of the\n+\t * input string.\n+\t */\n+\tconst char *trailer_start, *trailer_end;\n+\n+\t/*\n+\t * Array of trailers found.\n+\t */\n+\tchar **trailers;\n+\tsize_t trailer_nr;\n+};\n+\n void process_trailers(const char *file, int in_place, int trim_empty,\n \t\t struct string_list *trailers);\n \n+void trailer_info_get(struct trailer_info *info, const char *str);\n+\n+void trailer_info_release(struct trailer_info *info);\n+\n #endif /* TRAILER_H */\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305255","messageId":"xmqqvaw7ndow.fsf@gitster.mtv.corp.google.com","threadId":"44397","inReplyTo":"c7db0aafb543845382e1835e3704273d3596e6bb.1478028700.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 1/5] trailer: be stricter in parsing separators","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-01T20:32:47Z","receivedAt":"2016-11-01T20:32:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Currently, a line is interpreted to be a trailer line if it contains a\n> separator. Make parsing stricter by requiring the text on the left of\n> the separator, if not the empty string, to be of the \" whitespace>\" form.\n\nHmph. The optional whitespace is to allow for what kind of line?\n\nIt is not for \"Signed off by:\" that is a misspelt \"Signed-off-by:\";\nit may not hurt but I do not think of a case that would be useful\noffhand.\n\n\n\n\n> (The find_separator function distinguishes the no-separator case from\n> the separator-starts-line case because some callers of this function\n> need such a distinction.)\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> trailer.c | 23 +++++++++++++++++------\n> 1 file changed, 17 insertions(+), 6 deletions(-)\n>\n> diff --git a/trailer.c b/trailer.c\n> index f0ecde2..0ee634f 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -563,15 +563,26 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le\n> }\n> \n> /*\n> - * Return the location of the first separator in line, or -1 if there is no\n> - * separator.\n> + * If the given line is of the form \n> + * \"...\" or \"...\", return the\n> + * location of the separator. Otherwise, return -1.\n> */\n> static int find_separator(const char *line, const char *separators)\n> {\n> -\tint loc = strcspn(line, separators);\n> -\tif (!line[loc])\n> -\t\treturn -1;\n> -\treturn loc;\n> +\tint whitespace_found = 0;\n> +\tconst char *c;\n> +\tfor (c = line; *c; c++) {\n> +\t\tif (strchr(separators, *c))\n> +\t\t\treturn c - line;\n> +\t\tif (!whitespace_found && (isalnum(*c) || *c == '-'))\n> +\t\t\tcontinue;\n> +\t\tif (c != line && (*c == ' ' || *c == '\\t')) {\n> +\t\t\twhitespace_found = 1;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tbreak;\n> +\t}\n> +\treturn -1;\n> }\n> \n> /*\n"},{"id":"305257","messageId":"xmqqr36vndgn.fsf@gitster.mtv.corp.google.com","threadId":"44397","inReplyTo":"xmqqvaw7ndow.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/5] trailer: be stricter in parsing separators","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-01T20:37:44Z","receivedAt":"2016-11-01T20:37:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jonathan Tan writes:\n>\n>> Currently, a line is interpreted to be a trailer line if it contains a\n>> separator. Make parsing stricter by requiring the text on the left of\n>> the separator, if not the empty string, to be of the \"> whitespace>\" form.\n>\n> Hmph. The optional whitespace is to allow for what kind of line?\n>\n> It is not for \"Signed off by:\" that is a misspelt \"Signed-off-by:\";\n> it may not hurt but I do not think of a case that would be useful\n> offhand.\n\nOther than this \"Hmph\" (which is not an objection---just something\nthat the reviewer did not understand), the rest looked good to me.\n\nWill re-queue.\n\nThanks.\n"},{"id":"305260","messageId":"0b3e70fe-5e1f-7aaa-4c3d-88f30cb15008@google.com","threadId":"44397","inReplyTo":"xmqqr36vndgn.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/5] trailer: be stricter in parsing separators","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-11-01T20:53:47Z","receivedAt":"2016-11-01T20:53:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 11/01/2016 01:37 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Jonathan Tan writes:\n>>\n>>> Currently, a line is interpreted to be a trailer line if it contains a\n>>> separator. Make parsing stricter by requiring the text on the left of\n>>> the separator, if not the empty string, to be of the \">> whitespace>\" form.\n>>\n>> Hmph. The optional whitespace is to allow for what kind of line?\n>>\n>> It is not for \"Signed off by:\" that is a misspelt \"Signed-off-by:\";\n>> it may not hurt but I do not think of a case that would be useful\n>> offhand.\n\nThis is to allow trailers of the form \"Fix #42\" (mentioned in the \ngit-interpret-trailers documentation and also in the test).\n\n> Other than this \"Hmph\" (which is not an objection---just something\n> that the reviewer did not understand), the rest looked good to me.\n>\n> Will re-queue.\n>\n> Thanks.\n\nThanks!\n"},{"id":"305268","messageId":"xmqq4m3qopsh.fsf@gitster.mtv.corp.google.com","threadId":"44397","inReplyTo":"0b3e70fe-5e1f-7aaa-4c3d-88f30cb15008@google.com","subject":"Re: [PATCH v2 1/5] trailer: be stricter in parsing separators","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-01T21:26:06Z","receivedAt":"2016-11-01T21:26:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>>> Hmph. The optional whitespace is to allow for what kind of line?\n>>>\n>>> It is not for \"Signed off by:\" that is a misspelt \"Signed-off-by:\";\n>>> it may not hurt but I do not think of a case that would be useful\n>>> offhand.\n>\n> This is to allow trailers of the form \"Fix #42\" (mentioned in the\n> git-interpret-trailers documentation and also in the test).\n\nThat piece of information belongs to the in-code comment for the\nfunction, I would think. The comment that describes what it does\n(i.e. \"token with optional spaces and separator\") can be read from\nthe code easily, but the reason why the code implements that\nbehaviour cannot.\n\nThanks.\n"},{"id":"305298","messageId":"cover.1478107666.git.jonathantanmy@google.com","threadId":"44397","inReplyTo":"cover.1477698917.git.jonathantanmy@google.com","subject":"[PATCH v3 0/5] Make other git commands use trailer layout","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-11-02T17:29:15Z","receivedAt":"2016-11-02T17:29:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is the same as v2 except that in 1/5, the comment about\nfind_separators has been moved from the commit message to the code\nitself. Also, a trailing whitespace and unused variable fix.\n\nJonathan Tan (5):\n trailer: be stricter in parsing separators\n commit: make ignore_non_trailer take buf/len\n trailer: avoid unnecessary splitting on lines\n trailer: have function to describe trailer layout\n sequencer: use trailer's trailer layout\n\n builtin/commit.c | 2 +-\n commit.c | 22 ++--\n commit.h | 2 +-\n sequencer.c | 75 +++---------\n t/t3511-cherry-pick-x.sh | 16 ++-\n t/t4014-format-patch.sh | 37 +++++-\n t/t7501-commit.sh | 36 ++++++\n trailer.c | 299 +++++++++++++++++++++++++++++------------------\n trailer.h | 25 ++++\n 9 files changed, 316 insertions(+), 198 deletions(-)\n\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305299","messageId":"4f147d776f906da4a8ba7e256e65c048e9bd8d1a.1478107666.git.jonathantanmy@google.com","threadId":"44397","inReplyTo":"cover.1478107666.git.jonathantanmy@google.com","subject":"[PATCH v3 1/5] trailer: be stricter in parsing separators","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-11-02T17:29:16Z","receivedAt":"2016-11-02T17:29:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, a line is interpreted to be a trailer line if it contains a\nseparator. Make parsing stricter by requiring the text on the left of\nthe separator, if not the empty string, to be of the \"\" form.\n\nSigned-off-by: Jonathan Tan \n---\n trailer.c | 27 +++++++++++++++++++++------\n 1 file changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex f0ecde2..dc525e3 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -563,15 +563,30 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le\n }\n \n /*\n- * Return the location of the first separator in line, or -1 if there is no\n- * separator.\n+ * If the given line is of the form\n+ * \"...\" or \"...\", return the\n+ * location of the separator. Otherwise, return -1.\n+ *\n+ * The separator-starts-line case (in which this function returns 0) is\n+ * distinguished from the non-well-formed-line case (in which this function\n+ * returns -1) because some callers of this function need such a distinction.\n */\n static int find_separator(const char *line, const char *separators)\n {\n-\tint loc = strcspn(line, separators);\n-\tif (!line[loc])\n-\t\treturn -1;\n-\treturn loc;\n+\tint whitespace_found = 0;\n+\tconst char *c;\n+\tfor (c = line; *c; c++) {\n+\t\tif (strchr(separators, *c))\n+\t\t\treturn c - line;\n+\t\tif (!whitespace_found && (isalnum(*c) || *c == '-'))\n+\t\t\tcontinue;\n+\t\tif (c != line && (*c == ' ' || *c == '\\t')) {\n+\t\t\twhitespace_found = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tbreak;\n+\t}\n+\treturn -1;\n }\n \n /*\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305300","messageId":"351c897b0f27c6fc72598b872aafd26515e57e16.1478107666.git.jonathantanmy@google.com","threadId":"44397","inReplyTo":"cover.1478107666.git.jonathantanmy@google.com","subject":"[PATCH v3 2/5] commit: make ignore_non_trailer take buf/len","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-11-02T17:29:17Z","receivedAt":"2016-11-02T17:29:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Make ignore_non_trailer take a buf/len pair instead of struct strbuf.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/commit.c | 2 +-\n commit.c | 22 +++++++++++-----------\n commit.h | 2 +-\n trailer.c | 2 +-\n 4 files changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 8976c3d..887ccc7 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -790,7 +790,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tstrbuf_stripspace(&sb, 0);\n \n \tif (signoff)\n-\t\tappend_signoff(&sb, ignore_non_trailer(&sb), 0);\n+\t\tappend_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0);\n \n \tif (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)\n \t\tdie_errno(_(\"could not write commit template\"));\ndiff --git a/commit.c b/commit.c\nindex 856fd4a..2cf8515 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1649,7 +1649,7 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n }\n \n /*\n- * Inspect sb and determine the true \"end\" of the log message, in\n+ * Inspect the given string and determine the true \"end\" of the log message, in\n * order to find where to put a new Signed-off-by: line. Ignored are\n * trailing comment lines and blank lines, and also the traditional\n * \"Conflicts:\" block that is not commented out, so that we can use\n@@ -1659,37 +1659,37 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n * Returns the number of bytes from the tail to ignore, to be fed as\n * the second parameter to append_signoff().\n */\n-int ignore_non_trailer(struct strbuf *sb)\n+int ignore_non_trailer(const char *buf, size_t len)\n {\n \tint boc = 0;\n \tint bol = 0;\n \tint in_old_conflicts_block = 0;\n \n-\twhile (bol < sb->len) {\n-\t\tchar *next_line;\n+\twhile (bol < len) {\n+\t\tconst char *next_line = memchr(buf + bol, '\\n', len - bol);\n \n-\t\tif (!(next_line = memchr(sb->buf + bol, '\\n', sb->len - bol)))\n-\t\t\tnext_line = sb->buf + sb->len;\n+\t\tif (!next_line)\n+\t\t\tnext_line = buf + len;\n \t\telse\n \t\t\tnext_line++;\n \n-\t\tif (sb->buf[bol] == comment_line_char || sb->buf[bol] == '\\n') {\n+\t\tif (buf[bol] == comment_line_char || buf[bol] == '\\n') {\n \t\t\t/* is this the first of the run of comments? */\n \t\t\tif (!boc)\n \t\t\t\tboc = bol;\n \t\t\t/* otherwise, it is just continuing */\n-\t\t} else if (starts_with(sb->buf + bol, \"Conflicts:\\n\")) {\n+\t\t} else if (starts_with(buf + bol, \"Conflicts:\\n\")) {\n \t\t\tin_old_conflicts_block = 1;\n \t\t\tif (!boc)\n \t\t\t\tboc = bol;\n-\t\t} else if (in_old_conflicts_block && sb->buf[bol] == '\\t') {\n+\t\t} else if (in_old_conflicts_block && buf[bol] == '\\t') {\n \t\t\t; /* a pathname in the conflicts block */\n \t\t} else if (boc) {\n \t\t\t/* the previous was not trailing comment */\n \t\t\tboc = 0;\n \t\t\tin_old_conflicts_block = 0;\n \t\t}\n-\t\tbol = next_line - sb->buf;\n+\t\tbol = next_line - buf;\n \t}\n-\treturn boc ? sb->len - boc : 0;\n+\treturn boc ? len - boc : 0;\n }\ndiff --git a/commit.h b/commit.h\nindex afd14f3..9c12abb 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -355,7 +355,7 @@ extern const char *find_commit_header(const char *msg, const char *key,\n \t\t\t\t size_t *out_len);\n \n /* Find the end of the log message, the right place for a new trailer. */\n-extern int ignore_non_trailer(struct strbuf *sb);\n+extern int ignore_non_trailer(const char *buf, size_t len);\n \n typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,\n \t\t\t\t void *cb_data);\ndiff --git a/trailer.c b/trailer.c\nindex dc525e3..9d7765e 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -840,7 +840,7 @@ static int find_trailer_end(struct strbuf **lines, int patch_start)\n \n \tfor (i = 0; i < patch_start; i++)\n \t\tstrbuf_addbuf(&sb, lines[i]);\n-\tignore_bytes = ignore_non_trailer(&sb);\n+\tignore_bytes = ignore_non_trailer(sb.buf, sb.len);\n \tstrbuf_release(&sb);\n \tfor (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)\n \t\tignore_bytes -= lines[i]->len;\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305301","messageId":"a88d93dadf0bd7b9eb185900d6b6df495c4e5acb.1478107666.git.jonathantanmy@google.com","threadId":"44397","inReplyTo":"cover.1478107666.git.jonathantanmy@google.com","subject":"[PATCH v3 3/5] trailer: avoid unnecessary splitting on lines","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-11-02T17:29:18Z","receivedAt":"2016-11-02T17:29:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"trailer.c currently splits lines while processing a buffer (and also\nrejoins lines when needing to invoke ignore_non_trailer).\n\nAvoid such line splitting, except when generating the strings\ncorresponding to trailers (for ease of use by clients - a subsequent\npatch will allow other components to obtain the layout of a trailer\nblock in a buffer, including the trailers themselves). The main purpose\nof this is to make it easy to return pointers into the original buffer\n(for a subsequent patch), but this also significantly reduces the number\nof memory allocations required.\n\nSigned-off-by: Jonathan Tan \n---\n trailer.c | 194 ++++++++++++++++++++++++++++++++------------------------------\n 1 file changed, 100 insertions(+), 94 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 9d7765e..afbff4b 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -102,12 +102,12 @@ static int same_trailer(struct trailer_item *a, struct arg_item *b)\n \treturn same_token(a, b) && same_value(a, b);\n }\n \n-static inline int contains_only_spaces(const char *str)\n+static inline int is_blank_line(const char *str)\n {\n \tconst char *s = str;\n-\twhile (*s && isspace(*s))\n+\twhile (*s && *s != '\\n' && isspace(*s))\n \t\ts++;\n-\treturn !*s;\n+\treturn !*s || *s == '\\n';\n }\n \n static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *b)\n@@ -700,51 +700,71 @@ static void process_command_line_args(struct list_head *arg_head,\n \tfree(cl_separators);\n }\n \n-static struct strbuf **read_input_file(const char *file)\n+static void read_input_file(struct strbuf *sb, const char *file)\n {\n-\tstruct strbuf **lines;\n-\tstruct strbuf sb = STRBUF_INIT;\n-\n \tif (file) {\n-\t\tif (strbuf_read_file(&sb, file, 0) < 0)\n+\t\tif (strbuf_read_file(sb, file, 0) < 0)\n \t\t\tdie_errno(_(\"could not read input file '%s'\"), file);\n \t} else {\n-\t\tif (strbuf_read(&sb, fileno(stdin), 0) < 0)\n+\t\tif (strbuf_read(sb, fileno(stdin), 0) < 0)\n \t\t\tdie_errno(_(\"could not read from stdin\"));\n \t}\n+}\n \n-\tlines = strbuf_split(&sb, '\\n');\n+static const char *next_line(const char *str)\n+{\n+\tconst char *nl = strchrnul(str, '\\n');\n+\treturn nl + !!*nl;\n+}\n \n-\tstrbuf_release(&sb);\n+/*\n+ * Return the position of the start of the last line. If len is 0, return -1.\n+ */\n+static int last_line(const char *buf, size_t len)\n+{\n+\tint i;\n+\tif (len == 0)\n+\t\treturn -1;\n+\tif (len == 1)\n+\t\treturn 0;\n+\t/*\n+\t * Skip the last character (in addition to the null terminator),\n+\t * because if the last character is a newline, it is considered as part\n+\t * of the last line anyway.\n+\t */\n+\ti = len - 2;\n \n-\treturn lines;\n+\tfor (; i >= 0; i--) {\n+\t\tif (buf[i] == '\\n')\n+\t\t\treturn i + 1;\n+\t}\n+\treturn 0;\n }\n \n /*\n- * Return the (0 based) index of the start of the patch or the line\n- * count if there is no patch in the message.\n+ * Return the position of the start of the patch or the length of str if there\n+ * is no patch in the message.\n */\n-static int find_patch_start(struct strbuf **lines, int count)\n+static int find_patch_start(const char *str)\n {\n-\tint i;\n+\tconst char *s;\n \n-\t/* Get the start of the patch part if any */\n-\tfor (i = 0; i < count; i++) {\n-\t\tif (starts_with(lines[i]->buf, \"---\"))\n-\t\t\treturn i;\n+\tfor (s = str; *s; s = next_line(s)) {\n+\t\tif (starts_with(s, \"---\"))\n+\t\t\treturn s - str;\n \t}\n \n-\treturn count;\n+\treturn s - str;\n }\n \n /*\n- * Return the (0 based) index of the first trailer line or count if\n- * there are no trailers. Trailers are searched only in the lines from\n- * index (count - 1) down to index 0.\n+ * Return the position of the first trailer line or len if there are no\n+ * trailers.\n */\n-static int find_trailer_start(struct strbuf **lines, int count)\n+static int find_trailer_start(const char *buf, size_t len)\n {\n-\tint start, end_of_title, only_spaces = 1;\n+\tconst char *s;\n+\tint end_of_title, l, 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@@ -756,13 +776,13 @@ static int find_trailer_start(struct strbuf **lines, int count)\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-\t\tif (lines[start]->buf[0] == comment_line_char)\n+\tfor (s = buf; s < buf + len; s = next_line(s)) {\n+\t\tif (s[0] == comment_line_char)\n \t\t\tcontinue;\n-\t\tif (contains_only_spaces(lines[start]->buf))\n+\t\tif (is_blank_line(s))\n \t\t\tbreak;\n \t}\n-\tend_of_title = start;\n+\tend_of_title = s - buf;\n \n \t/*\n \t * Get the start of the trailers by looking starting from the end for a\n@@ -770,30 +790,33 @@ static int find_trailer_start(struct strbuf **lines, int count)\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+\tfor (l = last_line(buf, len);\n+\t l >= end_of_title;\n+\t l = last_line(buf, l)) {\n+\t\tconst char *bol = buf + l;\n \t\tconst char **p;\n \t\tint separator_pos;\n \n-\t\tif (lines[start]->buf[0] == comment_line_char) {\n+\t\tif (bol[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\tif (is_blank_line(bol)) {\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-\t\t\tif (trailer_lines && !non_trailer_lines)\n-\t\t\t\treturn start + 1;\n-\t\t\treturn count;\n+\t\t\t\treturn next_line(bol) - buf;\n+\t\t\telse if (trailer_lines && !non_trailer_lines)\n+\t\t\t\treturn next_line(bol) - buf;\n+\t\t\treturn len;\n \t\t}\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\tif (starts_with(bol, *p)) {\n \t\t\t\ttrailer_lines++;\n \t\t\t\tpossible_continuation_lines = 0;\n \t\t\t\trecognized_prefix = 1;\n@@ -801,8 +824,8 @@ static int find_trailer_start(struct strbuf **lines, int count)\n \t\t\t}\n \t\t}\n \n-\t\tseparator_pos = find_separator(lines[start]->buf, separators);\n-\t\tif (separator_pos >= 1 && !isspace(lines[start]->buf[0])) {\n+\t\tseparator_pos = find_separator(bol, separators);\n+\t\tif (separator_pos >= 1 && !isspace(bol[0])) {\n \t\t\tstruct list_head *pos;\n \n \t\t\ttrailer_lines++;\n@@ -812,13 +835,13 @@ static int find_trailer_start(struct strbuf **lines, int count)\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\tif (token_matches_item(bol, 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 if (isspace(lines[start]->buf[0]))\n+\t\t} else if (isspace(bol[0]))\n \t\t\tpossible_continuation_lines++;\n \t\telse {\n \t\t\tnon_trailer_lines++;\n@@ -829,88 +852,70 @@ static int find_trailer_start(struct strbuf **lines, int count)\n \t\t;\n \t}\n \n-\treturn count;\n-}\n-\n-/* Get the index of the end of the trailers */\n-static int find_trailer_end(struct strbuf **lines, int patch_start)\n-{\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tint i, ignore_bytes;\n-\n-\tfor (i = 0; i < patch_start; i++)\n-\t\tstrbuf_addbuf(&sb, lines[i]);\n-\tignore_bytes = ignore_non_trailer(sb.buf, sb.len);\n-\tstrbuf_release(&sb);\n-\tfor (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)\n-\t\tignore_bytes -= lines[i]->len;\n-\n-\treturn i + 1;\n+\treturn len;\n }\n \n-static int has_blank_line_before(struct strbuf **lines, int start)\n+/* Return the position of the end of the trailers. */\n+static int find_trailer_end(const char *buf, size_t len)\n {\n-\tfor (;start >= 0; start--) {\n-\t\tif (lines[start]->buf[0] == comment_line_char)\n-\t\t\tcontinue;\n-\t\treturn contains_only_spaces(lines[start]->buf);\n-\t}\n-\treturn 0;\n+\treturn len - ignore_non_trailer(buf, len);\n }\n \n-static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end)\n+static int ends_with_blank_line(const char *buf, size_t len)\n {\n-\tint i;\n-\tfor (i = start; lines[i] && i < end; i++)\n-\t\tfprintf(outfile, \"%s\", lines[i]->buf);\n+\tint ll = last_line(buf, len);\n+\tif (ll < 0)\n+\t\treturn 0;\n+\treturn is_blank_line(buf + ll);\n }\n \n static int process_input_file(FILE *outfile,\n-\t\t\t struct strbuf **lines,\n+\t\t\t const char *str,\n \t\t\t struct list_head *head)\n {\n-\tint count = 0;\n-\tint patch_start, trailer_start, trailer_end, i;\n+\tint patch_start, trailer_start, trailer_end;\n \tstruct strbuf tok = STRBUF_INIT;\n \tstruct strbuf val = STRBUF_INIT;\n \tstruct trailer_item *last = NULL;\n+\tstruct strbuf *trailer, **trailer_lines, **ptr;\n \n-\t/* Get the line count */\n-\twhile (lines[count])\n-\t\tcount++;\n-\n-\tpatch_start = find_patch_start(lines, count);\n-\ttrailer_end = find_trailer_end(lines, patch_start);\n-\ttrailer_start = find_trailer_start(lines, trailer_end);\n+\tpatch_start = find_patch_start(str);\n+\ttrailer_end = find_trailer_end(str, patch_start);\n+\ttrailer_start = find_trailer_start(str, trailer_end);\n \n \t/* Print lines before the trailers as is */\n-\tprint_lines(outfile, lines, 0, trailer_start);\n+\tfwrite(str, 1, trailer_start, outfile);\n \n-\tif (!has_blank_line_before(lines, trailer_start - 1))\n+\tif (!ends_with_blank_line(str, trailer_start))\n \t\tfprintf(outfile, \"\\n\");\n \n \t/* Parse trailer lines */\n-\tfor (i = trailer_start; i < trailer_end; i++) {\n+\ttrailer_lines = strbuf_split_buf(str + trailer_start,\n+\t\t\t\t\t trailer_end - trailer_start,\n+\t\t\t\t\t '\\n',\n+\t\t\t\t\t 0);\n+\tfor (ptr = trailer_lines; *ptr; ptr++) {\n \t\tint separator_pos;\n-\t\tif (lines[i]->buf[0] == comment_line_char)\n+\t\ttrailer = *ptr;\n+\t\tif (trailer->buf[0] == comment_line_char)\n \t\t\tcontinue;\n-\t\tif (last && isspace(lines[i]->buf[0])) {\n+\t\tif (last && isspace(trailer->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_addf(&sb, \"%s\\n%s\", last->value, trailer->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, separators);\n+\t\tseparator_pos = find_separator(trailer->buf, separators);\n \t\tif (separator_pos >= 1) {\n-\t\t\tparse_trailer(&tok, &val, NULL, lines[i]->buf,\n+\t\t\tparse_trailer(&tok, &val, NULL, trailer->buf,\n \t\t\t\t separator_pos);\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_addbuf(&val, trailer);\n \t\t\tstrbuf_strip_suffix(&val, \"\\n\");\n \t\t\tadd_trailer_item(head,\n \t\t\t\t\t NULL,\n@@ -918,6 +923,7 @@ static int process_input_file(FILE *outfile,\n \t\t\tlast = NULL;\n \t\t}\n \t}\n+\tstrbuf_list_free(trailer_lines);\n \n \treturn trailer_end;\n }\n@@ -966,7 +972,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n {\n \tLIST_HEAD(head);\n \tLIST_HEAD(arg_head);\n-\tstruct strbuf **lines;\n+\tstruct strbuf sb = STRBUF_INIT;\n \tint trailer_end;\n \tFILE *outfile = stdout;\n \n@@ -974,13 +980,13 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n \tgit_config(git_trailer_default_config, NULL);\n \tgit_config(git_trailer_config, NULL);\n \n-\tlines = read_input_file(file);\n+\tread_input_file(&sb, file);\n \n \tif (in_place)\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, &head);\n+\ttrailer_end = process_input_file(outfile, sb.buf, &head);\n \n \tprocess_command_line_args(&arg_head, trailers);\n \n@@ -991,11 +997,11 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\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+\tfwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);\n \n \tif (in_place)\n \t\tif (rename_tempfile(&trailers_tempfile, file))\n \t\t\tdie_errno(_(\"could not rename temporary file to %s\"), file);\n \n-\tstrbuf_list_free(lines);\n+\tstrbuf_release(&sb);\n }\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305302","messageId":"092b31de2aef3f57aa36010d0d016b2d0937e243.1478107666.git.jonathantanmy@google.com","threadId":"44397","inReplyTo":"cover.1478107666.git.jonathantanmy@google.com","subject":"[PATCH v3 5/5] sequencer: use trailer's trailer layout","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-11-02T17:29:20Z","receivedAt":"2016-11-02T17:29:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Make sequencer use trailer.c's trailer layout definition, as opposed to\nparsing the footer by itself. This makes \"commit -s\", \"cherry-pick -x\",\nand \"format-patch --signoff\" consistent with trailer, allowing\nnon-trailer lines and multiple-line trailers in trailer blocks under\ncertain conditions, and therefore suppressing the extra newline in those\ncases.\n\nConsistency with trailer extends to respecting trailer configs. Tests\nhave been included to show that.\n\nSigned-off-by: Jonathan Tan \n---\n sequencer.c | 75 +++++++++---------------------------------------\n t/t3511-cherry-pick-x.sh | 16 +++++++++--\n t/t4014-format-patch.sh | 37 ++++++++++++++++++++----\n t/t7501-commit.sh | 36 +++++++++++++++++++++++\n 4 files changed, 95 insertions(+), 69 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 5fd75f3..d64c973 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -16,6 +16,7 @@\n #include \"refs.h\"\n #include \"argv-array.h\"\n #include \"quote.h\"\n+#include \"trailer.h\"\n \n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n \n@@ -56,30 +57,6 @@ static const char *get_todo_path(const struct replay_opts *opts)\n \treturn git_path_todo_file();\n }\n \n-static int is_rfc2822_line(const char *buf, int len)\n-{\n-\tint i;\n-\n-\tfor (i = 0; i < len; i++) {\n-\t\tint ch = buf[i];\n-\t\tif (ch == ':')\n-\t\t\treturn 1;\n-\t\tif (!isalnum(ch) && ch != '-')\n-\t\t\tbreak;\n-\t}\n-\n-\treturn 0;\n-}\n-\n-static int is_cherry_picked_from_line(const char *buf, int len)\n-{\n-\t/*\n-\t * We only care that it looks roughly like (cherry picked from ...)\n-\t */\n-\treturn len > strlen(cherry_picked_prefix) + 1 &&\n-\t\tstarts_with(buf, cherry_picked_prefix) && buf[len - 1] == ')';\n-}\n-\n /*\n * Returns 0 for non-conforming footer\n * Returns 1 for conforming footer\n@@ -89,49 +66,25 @@ static int is_cherry_picked_from_line(const char *buf, int len)\n static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \tint ignore_footer)\n {\n-\tchar prev;\n-\tint i, k;\n-\tint len = sb->len - ignore_footer;\n-\tconst char *buf = sb->buf;\n-\tint found_sob = 0;\n-\n-\t/* footer must end with newline */\n-\tif (!len || buf[len - 1] != '\\n')\n-\t\treturn 0;\n+\tstruct trailer_info info;\n+\tint i;\n+\tint found_sob = 0, found_sob_last = 0;\n \n-\tprev = '\\0';\n-\tfor (i = len - 1; i > 0; i--) {\n-\t\tchar ch = buf[i];\n-\t\tif (prev == '\\n' && ch == '\\n') /* paragraph break */\n-\t\t\tbreak;\n-\t\tprev = ch;\n-\t}\n+\ttrailer_info_get(&info, sb->buf);\n \n-\t/* require at least one blank line */\n-\tif (prev != '\\n' || buf[i] != '\\n')\n+\tif (info.trailer_start == info.trailer_end)\n \t\treturn 0;\n \n-\t/* advance to start of last paragraph */\n-\twhile (i < len - 1 && buf[i] == '\\n')\n-\t\ti++;\n-\n-\tfor (; i < len; i = k) {\n-\t\tint found_rfc2822;\n-\n-\t\tfor (k = i; k < len && buf[k] != '\\n'; k++)\n-\t\t\t; /* do nothing */\n-\t\tk++;\n+\tfor (i = 0; i < info.trailer_nr; i++)\n+\t\tif (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {\n+\t\t\tfound_sob = 1;\n+\t\t\tif (i == info.trailer_nr - 1)\n+\t\t\t\tfound_sob_last = 1;\n+\t\t}\n \n-\t\tfound_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);\n-\t\tif (found_rfc2822 && sob &&\n-\t\t !strncmp(buf + i, sob->buf, sob->len))\n-\t\t\tfound_sob = k;\n+\ttrailer_info_release(&info);\n \n-\t\tif (!(found_rfc2822 ||\n-\t\t is_cherry_picked_from_line(buf + i, k - i - 1)))\n-\t\t\treturn 0;\n-\t}\n-\tif (found_sob == i)\n+\tif (found_sob_last)\n \t\treturn 3;\n \tif (found_sob)\n \t\treturn 2;\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex 9cce5ae..bf0a5c9 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -25,9 +25,8 @@ Signed-off-by: B.U. Thor \"\n \n mesg_broken_footer=\"$mesg_no_footer\n \n-The signed-off-by string should begin with the words Signed-off-by followed\n-by a colon and space, and then the signers name and email address. e.g.\n-Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n+This is not recognized as a footer because Myfooter is not a recognized token.\n+Myfooter: A.U. Thor \"\n \n mesg_with_footer_sob=\"$mesg_with_footer\n Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n@@ -112,6 +111,17 @@ test_expect_success 'cherry-pick -s inserts blank line after non-conforming foot\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick -s recognizes trailer config' '\n+\tpristine_detach initial &&\n+\tgit -c \"trailer.Myfooter.ifexists=add\" cherry-pick -s mesg-broken-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_broken_footer\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cherry-pick -x inserts blank line when conforming footer not found' '\n \tpristine_detach initial &&\n \tsha1=$(git rev-parse mesg-no-footer^0) &&\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex ba4902d..482112c 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1294,8 +1294,7 @@ EOF\n 4:Subject: [PATCH] subject\n 8:\n 10:Signed-off-by: example happens to be wrapped here.\n-11:\n-12:Signed-off-by: C O Mitter \n+11:Signed-off-by: C O Mitter \n EOF\n \ttest_cmp expected actual\n '\n@@ -1368,7 +1367,7 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'signoff: detect garbage in non-conforming footer' '\n+test_expect_success 'signoff: tolerate garbage in conforming footer' '\n \tappend_signoff <<\\EOF >actual &&\n subject\n \n@@ -1383,8 +1382,36 @@ EOF\n 8:\n 10:\n 13:Signed-off-by: C O Mitter \n-14:\n-15:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: respect trailer config' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+Myfooter: x\n+Some Trash\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+11:\n+12:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual &&\n+\n+\ttest_config trailer.Myfooter.ifexists add &&\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+Myfooter: x\n+Some Trash\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+11:Signed-off-by: C O Mitter \n EOF\n \ttest_cmp expected actual\n '\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex d84897a..4003a27 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -460,6 +460,42 @@ $alt\" &&\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'signoff respects trailer config' '\n+\n+\techo 5 >positive &&\n+\tgit add positive &&\n+\tgit commit -s -m \"subject\n+\n+non-trailer line\n+Myfooter: x\" &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" > actual &&\n+\t(\n+\t\techo subject\n+\t\techo\n+\t\techo non-trailer line\n+\t\techo Myfooter: x\n+\t\techo\n+\t\techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n+\t) >expected &&\n+\ttest_cmp expected actual &&\n+\n+\techo 6 >positive &&\n+\tgit add positive &&\n+\tgit -c \"trailer.Myfooter.ifexists=add\" commit -s -m \"subject\n+\n+non-trailer line\n+Myfooter: x\" &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" > actual &&\n+\t(\n+\t\techo subject\n+\t\techo\n+\t\techo non-trailer line\n+\t\techo Myfooter: x\n+\t\techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n+\t) >expected &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'multiple -m' '\n \n \t>negative &&\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"305303","messageId":"c64edee1f44562f38e4e92f1efb0c2267ef3222a.1478107666.git.jonathantanmy@google.com","threadId":"44397","inReplyTo":"cover.1478107666.git.jonathantanmy@google.com","subject":"[PATCH v3 4/5] trailer: have function to describe trailer layout","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-11-02T17:29:19Z","receivedAt":"2016-11-02T17:29:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Create a function that, taking a string, describes the position of its\ntrailer block (if available) and the contents thereof, and make trailer\nuse it. This makes it easier for other Git components, in the future, to\ninterpret trailer blocks in the same way as trailer.\n\nIn a subsequent patch, another component will be made to use this.\n\nSigned-off-by: Jonathan Tan \n---\n trailer.c | 118 +++++++++++++++++++++++++++++++++++++++++++-------------------\n trailer.h | 25 +++++++++++++\n 2 files changed, 107 insertions(+), 36 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex afbff4b..bc6893b 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -46,6 +46,8 @@ static LIST_HEAD(conf_head);\n \n static char *separators = \":\";\n \n+static int configured;\n+\n #define TRAILER_ARG_STRING \"$ARG\"\n \n static const char *git_generated_prefixes[] = {\n@@ -546,6 +548,17 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)\n \treturn 0;\n }\n \n+static void ensure_configured(void)\n+{\n+\tif (configured)\n+\t\treturn;\n+\n+\t/* Default config must be setup first */\n+\tgit_config(git_trailer_default_config, NULL);\n+\tgit_config(git_trailer_config, NULL);\n+\tconfigured = 1;\n+}\n+\n static const char *token_from_item(struct arg_item *item, char *tok)\n {\n \tif (item->conf.key)\n@@ -873,59 +886,43 @@ static int process_input_file(FILE *outfile,\n \t\t\t const char *str,\n \t\t\t struct list_head *head)\n {\n-\tint patch_start, trailer_start, trailer_end;\n+\tstruct trailer_info info;\n \tstruct strbuf tok = STRBUF_INIT;\n \tstruct strbuf val = STRBUF_INIT;\n-\tstruct trailer_item *last = NULL;\n-\tstruct strbuf *trailer, **trailer_lines, **ptr;\n+\tint i;\n \n-\tpatch_start = find_patch_start(str);\n-\ttrailer_end = find_trailer_end(str, patch_start);\n-\ttrailer_start = find_trailer_start(str, trailer_end);\n+\ttrailer_info_get(&info, str);\n \n \t/* Print lines before the trailers as is */\n-\tfwrite(str, 1, trailer_start, outfile);\n+\tfwrite(str, 1, info.trailer_start - str, outfile);\n \n-\tif (!ends_with_blank_line(str, trailer_start))\n+\tif (!info.blank_line_before_trailer)\n \t\tfprintf(outfile, \"\\n\");\n \n-\t/* Parse trailer lines */\n-\ttrailer_lines = strbuf_split_buf(str + trailer_start,\n-\t\t\t\t\t trailer_end - trailer_start,\n-\t\t\t\t\t '\\n',\n-\t\t\t\t\t 0);\n-\tfor (ptr = trailer_lines; *ptr; ptr++) {\n+\tfor (i = 0; i < info.trailer_nr; i++) {\n \t\tint separator_pos;\n-\t\ttrailer = *ptr;\n-\t\tif (trailer->buf[0] == comment_line_char)\n-\t\t\tcontinue;\n-\t\tif (last && isspace(trailer->buf[0])) {\n-\t\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\t\tstrbuf_addf(&sb, \"%s\\n%s\", last->value, trailer->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\tchar *trailer = info.trailers[i];\n+\t\tif (trailer[0] == comment_line_char)\n \t\t\tcontinue;\n-\t\t}\n-\t\tseparator_pos = find_separator(trailer->buf, separators);\n+\t\tseparator_pos = find_separator(trailer, separators);\n \t\tif (separator_pos >= 1) {\n-\t\t\tparse_trailer(&tok, &val, NULL, trailer->buf,\n+\t\t\tparse_trailer(&tok, &val, NULL, trailer,\n \t\t\t\t separator_pos);\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\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, trailer);\n+\t\t\tstrbuf_addstr(&val, trailer);\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-\tstrbuf_list_free(trailer_lines);\n \n-\treturn trailer_end;\n+\ttrailer_info_release(&info);\n+\n+\treturn info.trailer_end - str;\n }\n \n static void free_all(struct list_head *head)\n@@ -976,9 +973,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n \tint trailer_end;\n \tFILE *outfile = stdout;\n \n-\t/* Default config must be setup first */\n-\tgit_config(git_trailer_default_config, NULL);\n-\tgit_config(git_trailer_config, NULL);\n+\tensure_configured();\n \n \tread_input_file(&sb, file);\n \n@@ -1005,3 +1000,54 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n \n \tstrbuf_release(&sb);\n }\n+\n+void trailer_info_get(struct trailer_info *info, const char *str)\n+{\n+\tint patch_start, trailer_end, trailer_start;\n+\tstruct strbuf **trailer_lines, **ptr;\n+\tchar **trailer_strings = NULL;\n+\tsize_t nr = 0, alloc = 0;\n+\tchar **last = NULL;\n+\n+\tensure_configured();\n+\n+\tpatch_start = find_patch_start(str);\n+\ttrailer_end = find_trailer_end(str, patch_start);\n+\ttrailer_start = find_trailer_start(str, trailer_end);\n+\n+\ttrailer_lines = strbuf_split_buf(str + trailer_start,\n+\t\t\t\t\t trailer_end - trailer_start,\n+\t\t\t\t\t '\\n',\n+\t\t\t\t\t 0);\n+\tfor (ptr = trailer_lines; *ptr; ptr++) {\n+\t\tif (last && isspace((*ptr)->buf[0])) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_attach(&sb, *last, strlen(*last), strlen(*last));\n+\t\t\tstrbuf_addbuf(&sb, *ptr);\n+\t\t\t*last = strbuf_detach(&sb, NULL);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tALLOC_GROW(trailer_strings, nr + 1, alloc);\n+\t\ttrailer_strings[nr] = strbuf_detach(*ptr, NULL);\n+\t\tlast = find_separator(trailer_strings[nr], separators) >= 1\n+\t\t\t? &trailer_strings[nr]\n+\t\t\t: NULL;\n+\t\tnr++;\n+\t}\n+\tstrbuf_list_free(trailer_lines);\n+\n+\tinfo->blank_line_before_trailer = ends_with_blank_line(str,\n+\t\t\t\t\t\t\t trailer_start);\n+\tinfo->trailer_start = str + trailer_start;\n+\tinfo->trailer_end = str + trailer_end;\n+\tinfo->trailers = trailer_strings;\n+\tinfo->trailer_nr = nr;\n+}\n+\n+void trailer_info_release(struct trailer_info *info)\n+{\n+\tint i;\n+\tfor (i = 0; i < info->trailer_nr; i++)\n+\t\tfree(info->trailers[i]);\n+\tfree(info->trailers);\n+}\ndiff --git a/trailer.h b/trailer.h\nindex 36b40b8..65cc5d7 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -1,7 +1,32 @@\n #ifndef TRAILER_H\n #define TRAILER_H\n \n+struct trailer_info {\n+\t/*\n+\t * True if there is a blank line before the location pointed to by\n+\t * trailer_start.\n+\t */\n+\tint blank_line_before_trailer;\n+\n+\t/*\n+\t * Pointers to the start and end of the trailer block found. If there\n+\t * is no trailer block found, these 2 pointers point to the end of the\n+\t * input string.\n+\t */\n+\tconst char *trailer_start, *trailer_end;\n+\n+\t/*\n+\t * Array of trailers found.\n+\t */\n+\tchar **trailers;\n+\tsize_t trailer_nr;\n+};\n+\n void process_trailers(const char *file, int in_place, int trim_empty,\n \t\t struct string_list *trailers);\n \n+void trailer_info_get(struct trailer_info *info, const char *str);\n+\n+void trailer_info_release(struct trailer_info *info);\n+\n #endif /* TRAILER_H */\n-- \n2.8.0.rc3.226.g39d4020\n\n"}]}